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
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions NEWS.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,10 @@ https://github.com/networkupstools/nut/milestone/11
as known supported by `nutdrv_qx` (Megatec protocol) since at least
NUT v2.7.4 release [#2395]

- usbhid-ups updates:
* Support of the `onlinedischarge_log_throttle_hovercharge` in the NUT
v2.8.2 release was found to be incomplete. [#2423, follow-up to #2215]

- USB drivers could log `(nut_)libusb_get_string: Success` due to either
reading an empty string or getting a success code `0` from libusb.
This difference should now be better logged, and not into syslog. [#2399]
Expand Down
62 changes: 49 additions & 13 deletions drivers/usbhid-ups.c
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
* 2005-2006 Peter Selinger <[email protected]>
* 2007-2009 Arjen de Korte <[email protected]>
* 2016 Eaton / Arnaud Quette <[email protected]>
* 2020-2024 Jim Klimov <[email protected]>
*
* This program was sponsored by MGE UPS SYSTEMS, and now Eaton
*
Expand All @@ -28,7 +29,7 @@
*/

#define DRIVER_NAME "Generic HID driver"
#define DRIVER_VERSION "0.53"
#define DRIVER_VERSION "0.54"

#define HU_VAR_WAITBEFORERECONNECT "waitbeforereconnect"

Expand Down Expand Up @@ -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... 😎

if (val) {
int ipv = atoi(val);
if (ipv < 1 || ipv > 100) {
onlinedischarge_log_throttle_hovercharge = 100;
upslogx(LOG_WARNING,
"Warning: invalid value for "
"onlinedischarge_log_throttle_hovercharge: %s, "
"defaulting to %d",
val, onlinedischarge_log_throttle_hovercharge);
} else {
onlinedischarge_log_throttle_hovercharge = ipv;
}
}

if (testvar("disable_fix_report_desc")) {
disable_fix_report_desc = 1;
}
Expand Down Expand Up @@ -1970,8 +1986,8 @@ static void ups_status_set(void)
|| onlinedischarge_log_throttle_charge != -1
)) {
upsdebugx(1,
"%s: seems that UPS [%s] was in OL+DISCHRG state, "
"but no longer is now.",
"%s: seems that UPS [%s] was in OL+DISCHRG state "
"previously, but no is longer discharging now.",
__func__, upsname);
onlinedischarge_log_throttle_timestamp = 0;
onlinedischarge_log_throttle_charge = -1;
Expand Down Expand Up @@ -2011,23 +2027,38 @@ static void ups_status_set(void)
/* First disable, then enable if OK for noise*/
do_logmsg = 0;

/* Time or not, did the charge change since last log? */
/* Time or not, did the charge change since last log?
* Reminder: "onlinedischarge_log_throttle_charge" is
* the last-reported charge in OL+DISCHRG situation,
* as the battery remainder trickles down and we only
* report the changes (throttling the message stream).
* The "onlinedischarge_log_throttle_hovercharge" lets
* us ignore sufficiently high battery charges, where
* the user configuration (or defaults at 100%) tell
* us this is just about the battery not accepting the
* external power *all* the time so its charge "hovers"
* (typically between 90%-100%) to benefit the chemical
* process back-end of the battery and its life time.
*/
if ((s = dstate_getinfo("battery.charge"))) {
/* NOTE: "0" may mean a conversion error: */
/* NOTE: exact "0" may mean a conversion error: */
current_charge = atoi(s);
if (current_charge > 0
&& current_charge != onlinedischarge_log_throttle_charge
) {
/* Charge has changed, but is it
* now low enough to worry? */
if (onlinedischarge_log_throttle_hovercharge
< onlinedischarge_log_throttle_charge
if (current_charge
< onlinedischarge_log_throttle_hovercharge
) {
upsdebugx(3, "%s: current "
"battery.charge=%d is under "
"onlinedischarge_log_throttle_charge=%d",
"onlinedischarge_log_throttle_hovercharge=%d "
"(previous onlinedischarge_log_throttle_charge=%d): %s",
__func__, current_charge,
onlinedischarge_log_throttle_charge);
onlinedischarge_log_throttle_hovercharge,
onlinedischarge_log_throttle_charge,
(current_charge > onlinedischarge_log_throttle_charge ? "charging" : "draining"));
do_logmsg = 1;
} else {
/* All seems OK, don't spam log
Expand All @@ -2036,9 +2067,12 @@ static void ups_status_set(void)
upsdebugx(5, "%s: current "
"battery.charge=%d "
"is okay compared to "
"onlinedischarge_log_throttle_charge=%d",
"onlinedischarge_log_throttle_hovercharge=%d "
"(previous onlinedischarge_log_throttle_charge=%d): %s",
__func__, current_charge,
onlinedischarge_log_throttle_charge);
onlinedischarge_log_throttle_hovercharge,
onlinedischarge_log_throttle_charge,
(current_charge > onlinedischarge_log_throttle_charge ? "charging" : "draining"));
}
}
} else {
Expand Down Expand Up @@ -2073,6 +2107,7 @@ static void ups_status_set(void)
}

if (do_logmsg) {
/* If OL+DISCHRG, and not-throttled against log spam */
char msg_charge[LARGEBUF];
msg_charge[0] = '\0';

Expand All @@ -2091,9 +2126,10 @@ static void ups_status_set(void)
} else {
snprintf(msg_charge, sizeof(msg_charge),
"Battery charge changed from %d to %d "
"since last such report. ",
"since last such report (%s). ",
onlinedischarge_log_throttle_charge,
current_charge);
current_charge,
(current_charge > onlinedischarge_log_throttle_charge ? "charging" : "draining"));
}
onlinedischarge_log_throttle_charge = current_charge;
}
Expand Down
Loading