Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nut-scanner USB: disable some output results #2221

Merged
merged 19 commits into from
Jan 14, 2024

Conversation

aquette
Copy link
Member

@aquette aquette commented Dec 10, 2023

Some additional fields, like 'device' (device number / path), may introduce more issues than solving. Hence disable these

Some additional fields, like 'device' (device number / path), may introduce
more issues than solving. Hence disable these

Signed-off-by: Arnaud Quette <Arnaud.Quette@free.fr>
@aquette aquette added the Home Assistant (HA) Use of NUT with third-party plugin for Home Assistant (HA) label Dec 10, 2023
@AppVeyorBot
Copy link

@jimklimov
Copy link
Member

Cheers, just to be clear: the device_port is deemed problematic, but bus and bus_port are more physically oriented - so are not an issue?

For "bcd" - need to comment away the variable definition too, then (avoid unused variable). Or maybe add an option that the parser may/may-not use comments (or inject them in some other manner - also for alternative maybe suitable drivers, etc.). Or handle the bcd in driver args (even if ignoring in some cases) so it is just not an invalid string anymore.

…currently use [networkupstools#2221]

Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…BCD_DEVICE [networkupstools#2221]

Earlier approaches to only partially comment away code trigger static analysis
warnings here or there, and we do not want to fully drop it here just yet.

Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
@AppVeyorBot
Copy link

@desertwitch
Copy link
Contributor

desertwitch commented Dec 12, 2023

I might be barking up the wrong tree here, but would disabling some of those outputs (particularly physically-oriented ones regarding a device's location on the system such as device, bus or busport) not make it harder for a UPS driver to find that specific UPS device in more tricky environments? I'm talking systems where that particular UPS device is not easily or otherwise found "out of the box" with say driver usbhid-ups and port on auto?

I've had users who had their UPS not detected out of the box and exactly such a scenario was where nut-scanner came to the rescue by pointing the UPS driver exactly to the very location of that particular UPS via those physically-oriented outputs (even knowing where they fall short, such as making the UPS driver "blind" to just that particular device in that particular location)

Perhaps for sake of principle of least surprise an additional toggle could be implemented (something like --omit-physical-options) which would drop the specific physically-oriented outputs so that the configuration can become more flexible when considering reboots, changing device locations/IDs etc..? Entirely removing those outputs seems a bit extreme, although I agree they can be a bit of a mixed bag and do sometimes introduce more problems than they solve.

@jimklimov jimklimov added this to the 2.8.3 milestone Dec 15, 2023
@jimklimov
Copy link
Member

Seems this area needs a bit more of a holistic design than chopping stuff away :) So delaying until a later release (hoping to cut one soon to push existing bug fixes out).

  • figure out passing "commented-away" entries (like bcdDevice) in a format-agnostic manner - this could be used for other entities like the device_port
  • option to show or hide the USB physicalities, maybe with more levels about it (all three or certain ones?)

@aquette
Copy link
Member Author

aquette commented Dec 23, 2023

quite frankly, as long as the device reports its serial, it's good ;)

All the other means are just to be 400% sure we can keep up with cheaper units (reporting mfr 0000 or 0001 or such) in case there are multiple, and with all cases that may happen in the multiverse.
As for bcdDevice, I don't remember having seen this value moving in more than 2 decades...

Still, @desertwitch idea of a filter (more a minimal, with vendorid, productid and serial) is great!

For 2.8.2 Jim please. The current situation would cause regressions!

@jimklimov jimklimov modified the milestones: 2.8.3, 2.8.2 Jan 3, 2024
@jimklimov
Copy link
Member

Ack on 2.8.2 - I hoped to issue one before NY to have a release out which includes some errata fixes over 2.8.1, but that window has sailed so might as well bundle a few more improvements like this one into the 2.8.2 when it appears.

…utput

Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…t_simulation.c [networkupstools#2246]

Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…code [networkupstools#2246]

Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…utput

Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…utput

Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…tion section level of detail [networkupstools#2221]

Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…s#2205]

Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…es instead of hacks in prepared config data [networkupstools#2221]

Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…_commented_option_to_device() for link-specific details if not suggesting to use them directly [networkupstools#2221]

Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
@jimklimov jimklimov force-pushed the nutscanner-trim-output branch from 5fbbbfe to 4243814 Compare January 12, 2024 16:57
@jimklimov
Copy link
Member

Based on preceding discussions, I took a shot at updating this PR to allow enterprising users to still have the bus, busport and device values by passing several -U flags to nut-scanner, but to safely not have them by default.

Thought a bit about defaulting the flags based on amount of discovered devices and whether they have usable serial number values (all different, non-trivial, not repetitive characters like all-spaces or all-zeroes - logic for all that is present in nut-display.c sanity checker), or if all vendor/product IDs are different. E.g. if there are several detected devices and they seem similar, enable bus/busport/device(?) by default; for one device or several unique devices - keep these hidden.

Also extended libnutscan API to explicitly tag "suggested but not active" driver options, so they can be rendered as comments in some formats (ups.conf) and ignored in others (parsable; later maybe json/yaml/whatever).

@aquette : any ideas how to best register one text file source as the man page for several method names? There's gotta be a better way than to ship a renamed copy, right? :)

@aquette, @desertwitch : testing welcome, so far just developed in a VM to pass the build and spell-checker :)

@jimklimov
Copy link
Member

Seems to work.

  • For ups.conf formatted outputs, with comments where due:
root@pve:/usr/src/nut-pve# ./tools/nut-scanner/nut-scanner -U
Scanning USB bus.
[nutdev-usb1]
        driver = "usbhid-ups"
        port = "auto"
        vendorid = "0463"
        productid = "FFFF"
        product = "Ellipse ECO"
        serial = "000000000"
        vendor = "EATON"
        # bus = "003"
        # device = "002"
        # busport = "002"

# WARNING: all-same character "serial" with 9 copies of '0' (0x30) reported in some devices: nutdev-serial1

root@pve:/usr/src/nut-pve# ./tools/nut-scanner/nut-scanner -UU
Scanning USB bus.
[nutdev-usb1]
        driver = "usbhid-ups"
        port = "auto"
        vendorid = "0463"
        productid = "FFFF"
        product = "Ellipse ECO"
        serial = "000000000"
        vendor = "EATON"
        bus = "003"
        # device = "002"
        busport = "002"

# WARNING: all-same character "serial" with 9 copies of '0' (0x30) reported in some devices: nutdev-serial1

root@pve:/usr/src/nut-pve# ./tools/nut-scanner/nut-scanner -UUU
Scanning USB bus.
[nutdev-usb1]
        driver = "usbhid-ups"
        port = "auto"
        vendorid = "0463"
        productid = "FFFF"
        product = "Ellipse ECO"
        serial = "000000000"
        vendor = "EATON"
        bus = "003"
        device = "002"
        busport = "002"

# WARNING: all-same character "serial" with 9 copies of '0' (0x30) reported in some devices: nutdev-serial1

root@pve:/usr/src/nut-pve# ./tools/nut-scanner/nut-scanner -UUUU
Scanning USB bus.
[nutdev-usb1]
        driver = "usbhid-ups"
        port = "auto"
        vendorid = "0463"
        productid = "FFFF"
        product = "Ellipse ECO"
        serial = "000000000"
        vendor = "EATON"
        bus = "003"
        device = "002"
        busport = "002"
        ###NOTMATCHED-YET### bcdDevice = "0100"

# WARNING: all-same character "serial" with 9 copies of '0' (0x30) reported in some devices: nutdev-serial1
  • And for parsable output (skipping comment-tagged entries):
root@pve:/usr/src/nut-pve# ./tools/nut-scanner/nut-scanner -UUUUP
Scanning USB bus.
USB:driver="usbhid-ups",port="auto",vendorid="0463",productid="FFFF",product="Ellipse ECO",serial="000000000",vendor="EATON",bus="003",device="002",busport="002"

root@pve:/usr/src/nut-pve# ./tools/nut-scanner/nut-scanner -UP
Scanning USB bus.
USB:driver="usbhid-ups",port="auto",vendorid="0463",productid="FFFF",product="Ellipse ECO",serial="000000000",vendor="EATON"

root@pve:/usr/src/nut-pve# ./tools/nut-scanner/nut-scanner -UPU
Scanning USB bus.
USB:driver="usbhid-ups",port="auto",vendorid="0463",productid="FFFF",product="Ellipse ECO",serial="000000000",vendor="EATON",bus="003",busport="002"

@desertwitch
Copy link
Contributor

Seems to work in my testing too, also this is a beautiful solution to the problem - thanks to all. 💯

@aquette
Copy link
Member Author

aquette commented Jan 13, 2024

Privet @jimklimov, hi @desertwitch
Happy new year to all!
Thanks for completing the work Jim.

As for manpages aliasing, check malloc/calloc for ex, these use .Nm iirc to provide additional names (aliases).
Not sure for asciidoc then...

…il_level variables [networkupstools#2221]

Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
@jimklimov jimklimov force-pushed the nutscanner-trim-output branch from ed1ddce to 56ce7e9 Compare January 13, 2024 18:16
…markup for a single asciidoc text describing several command names [networkupstools#2221]

https://asciidoc-py.github.io/userguide.html#X1

Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…s#2221]

Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
@jimklimov jimklimov force-pushed the nutscanner-trim-output branch from 56ce7e9 to e289c79 Compare January 13, 2024 20:34
… recipe for BSD make [networkupstools#2221]

Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…_to_device.html recipe for now [networkupstools#2221]

Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
@jimklimov
Copy link
Member

Uff, wrighting the wrongs of makefiles is oft more burdensome than dev'p'ment itself, if I may say so myself!

@jimklimov jimklimov merged commit da4dcb6 into networkupstools:master Jan 14, 2024
12 checks passed
@jimklimov
Copy link
Member

quite frankly, as long as the device reports its serial, it's good ;)

Ironically, Eaton's (Ellipse ECO at least) devices all report 000000000 so their serial value is useless to differentiate two connected UPSes ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Home Assistant (HA) Use of NUT with third-party plugin for Home Assistant (HA) nut-scanner
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants