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

usbhid-ups.c: complete the support of onlinedischarge_log_throttle_hovercharge setting #2428

Merged
merged 3 commits into from
May 13, 2024

Conversation

jimklimov
Copy link
Member

Follows-up from: #2215 / #2216

Closes: #2423

CC @desertwitch : you seem to be great at testing strange corner cases with shutdown criteria and logging settings like this, may I ask for a bit of your time to look at this feature? Seems to have been touted for NUT v2.8.2, but not actually completed by the earlier PR - at least as far as end-user ability to practically configure it is concerned. So probably not well tested either :\

As a reminder, this started for UPSes which normally only charge up to a 100% level and then let their battery seep away until it hits a threshold (90-95%) so it is boosted back to 100% again. With this setting, if in such case the UPS reports OL+DISCHRG state, we can throttle logging of the situation and avoid the needless log spam.

@jimklimov jimklimov added enhancement USB Shutdowns and overrides and battery level triggers Issues and PRs about system shutdown, especially if battery charge/runtime remaining is involved impacts-release-2.8.2 Issues reported against NUT release 2.8.2 (maybe vanilla or with minor packaging tweaks) labels May 2, 2024
@jimklimov jimklimov added this to the 2.8.3 milestone May 2, 2024
@desertwitch
Copy link
Contributor

Sure, I'll test this - sorry didn't see the CC earlier for some reason.

@@ -1348,6 +1349,21 @@ void upsdrv_initups(void)
}
}

val = getval("onlinedischarge_log_throttle_hovercharge");
Copy link
Member Author

@jimklimov jimklimov May 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One point cursorily raised in another discussion (#2430) and posted to mailing list is that maybe this setting name and internal NUT concept could boil down to a (default.|override.)battery.charge.high (not currently defined in docs/nut-names.txt) and/or a HB flag. At least, if we as a community remember and define what such a flag means - some alert about battery overcharge as harmful for its health, or something similar/identical to hovering over a "well-charged" state which is not 100% (max voltage) exactly but e.g. fluctuating between 85-95-100%. The downwards part of this battery state cycle, depleting from ~100% to the "high" watermark, would in fact be "online and discharging".

Possibly the high name should be the watermark charge the battery aims to reach when charging (100% or less), and another name (hover?) would be what it deflates to before beginning to charge again (some 85%-ish?).

Probably not gonna be a focus of this PR, but can well pop up as continuation of the subject. Discussion in ML started.

Copy link
Contributor

@desertwitch desertwitch May 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably hard to set up for the end-user without knowing what voltage a UPS actually considers as 100% and at what percentage (compared to voltage) it is in fact starting to be "harmful" for the battery, especially when we're dealing with percentages rather than voltages. And even if the end-user were alerted of reaching such a threshold "high" percentage - what would be the consequence? We can't stop charging the battery after all (or can we - upscmd?), a shutdown probably also is out of the question, not sure what the benefit could be other than knowing it is what was defined (or rather guessed) as HB. If I understood the idea correctly there... 😎

@desertwitch
Copy link
Contributor

desertwitch commented May 11, 2024

Sorry been a bit busy this week, all seems to work as it should on my test system and looking good code wise too - thanks for the efforts. Just a quick sanity check - why are we putting all the onlinedischarge settings in a specific driver and not upsmon (for all drivers to use)? Unless it is a problem specific to usbhid-ups, wouldn't such settings be better located in upsmon down the road?

... or is it because it is specific to the driver's way of logging/reporting/handling of statuses?

@jimklimov
Copy link
Member Author

jimklimov commented May 12, 2024

Good questions, I've pondered about those too.

Regarding the "location" of this logic - drivers are closer to the hardware and "knowledge" (built-in or tweaked like here) about whatever meaning of seen symptoms. At least, they are better positioned for this than a generic client like upsmon that may not be running in some deployments at all (with other NUT clients in place).

Historically, some USB HID devices were the first (still only?) ones to report both OL and DISCHRG states in more and more practical situations not meaning a catastrophic event, so handling appeared in this driver.

I think it (and runtimecal, and maybe some other tricks) can be generalized into drivers/main.c (part of why I'm thinking about suitable and sustainable names), so that all drivers can share the behavior and config ways, or perhaps common methods to use for that at will. But it probably should be the drivers remaining responsible here.

Another part of "why" here and not upsmon IIRC (gotta check though) is that the driver raises some critical situation flag and causes a shutdown - unless it is taught by such hacks that for the particular device it is okay.

Finally, the driver is apparently too verbose about puzzling situations, so most logged issues/reports were initially about too much noise logged from usbhid-ups program :)

As with other such tunables, it is up to the user to learn the need to use via practice (e.g. too many logs) it and the numbers involved by experiment/observation with their device. Maybe upslog can help though, as nicely presented in #2430.

@desertwitch
Copy link
Contributor

desertwitch commented May 13, 2024

Good questions, I've pondered about those too.

Regarding the "location" of this logic - drivers are closer to the hardware and "knowledge" (built-in or tweaked like here) about whatever meaning of seen symptoms. At least, they are better positioned for this than a generic client like upsmon that may not be running in some deployments at all (with other NUT clients in place).

Historically, some USB HID devices were the first (still only?) ones to report both OL and DISCHRG states in more and more practical situations not meaning a catastrophic event, so handling appeared in this driver.

I think it (and runtimecal, and maybe some other tricks) can be generalized into drivers/main.c (part of why I'm thinking about suitable and sustainable names), so that all drivers can share the behavior and config ways, or perhaps common methods to use for that at will. But it probably should be the drivers remaining responsible here.

Another part of "why" here and not upsmon IIRC (gotta check though) is that the driver raises some critical situation flag and causes a shutdown - unless it is taught by such hacks that for the particular device it is okay.

Finally, the driver is apparently too verbose about puzzling situations, so most logged issues/reports were initially about too much noise logged from usbhid-ups program :)

As with other such tunables, it is up to the user to learn the need to use via practice (e.g. too many logs) it and the numbers involved by experiment/observation with their device. Maybe upslog can help though, as nicely presented in #2430.

Makes sense, probably also good to find some middle ground between creating an infinite amount of toggles for "too verbose" situations (which do have their use for debugging problems imho) and leaving it to end-users to actually filter their syslogs themselves. Particularly since many OSes (especially home-NAS oriented ones such as Unraid) will just log everything, including all debug-level messages, resulting exactly in what many consider as "noisiness". Time and again users approach me with how NUT should show as little as just possible - best nothing, while simultaneously having their syslog daemons push literally any debug-level messages into it... I think that just misses the point entirely. I'm still a follower of the concept that the syslog should first and foremost be helpful and "too verbose" is only when the syslog actually becomes unreadable due to the the many events logged.

@jimklimov jimklimov merged commit bab2163 into networkupstools:master May 13, 2024
13 of 16 checks passed
@jimklimov jimklimov deleted the issue-2423 branch May 13, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement impacts-release-2.8.2 Issues reported against NUT release 2.8.2 (maybe vanilla or with minor packaging tweaks) Shutdowns and overrides and battery level triggers Issues and PRs about system shutdown, especially if battery charge/runtime remaining is involved USB
Projects
None yet
Development

Successfully merging this pull request may close these issues.

usbhid-ups: Complete the onlinedischarge_log_throttle_hovercharge option support
2 participants