From 8fe9e46147dd7c56c5866fc3f8e5989982a8d234 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sat, 16 Nov 2024 19:01:49 +0100 Subject: [PATCH] drivers/usbhid-ups.c: fix fallout of #2671 and complete the feature with optional `interrupt_pipe_no_events_tolerance` setting [#2681] Signed-off-by: Jim Klimov --- NEWS.adoc | 10 ++++++---- docs/man/usbhid-ups.txt | 9 +++++++++ drivers/usbhid-ups.c | 40 +++++++++++++++++++++++++++++++++++++--- include/nutconf.hpp | 2 ++ 4 files changed, 54 insertions(+), 7 deletions(-) diff --git a/NEWS.adoc b/NEWS.adoc index b20ded77f0..a8e5e12898 100644 --- a/NEWS.adoc +++ b/NEWS.adoc @@ -146,10 +146,12 @@ https://github.com/networkupstools/nut/milestone/11 - 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] - * Prevent UPS Lockup on "0 HID Objects" by reconnecting on stale data. - Note that while some devices just report information upon subsequent - poll, others (APC BXnnnnMI) were seen to lock up until full connection - restart. [#2671] + * Added support for `interrupt_pipe_no_events_tolerance=N` setting to + optionally prevent UPS lockup, indicated by continuous "Got 0 HID Objects" + situation as a clue, by reconnecting on stale data. Note that while some + devices just report information upon subsequent poll and just have nothing + urgent to declare with an USB interrupt, others (e.g. APC BXnnnnMI) were + seen to lock up until a full connection restart. [#2671, #2681] * Added support for `lbrb_log_delay_sec=N` setting to delay propagation of `LB` or `LB+RB` state (buggy with APC BXnnnnMI devices circa 2023-2024). This may work better with flags like `onlinedischarge_calibration` and diff --git a/docs/man/usbhid-ups.txt b/docs/man/usbhid-ups.txt index ea89ffee35..28201add10 100644 --- a/docs/man/usbhid-ups.txt +++ b/docs/man/usbhid-ups.txt @@ -123,6 +123,15 @@ If this flag is set, the driver will not use Interrupt In transfers during the shorter "pollinterval" cycles (not recommended, but needed if these reports are broken on your UPS). +*interrupt_pipe_no_events_tolerance*='num':: +Set the tolerance for how many times in a row could we have "Got 0 HID objects" +when using USB interrupt mode? This may normally be due to a device having +nothing urgent to report, so the default value is `-1` and this situation is +not handled in any way specially. However with some devices this was seen +in conjunction with a frozen controller, where only a driver reconnection +restored the data exchange (e.g. APC BXnnnnMI) -- in such cases you may want +to use a reasonable non-negative value here. + *onlinedischarge_battery*:: If this flag is set, the driver will treat `OL+DISCHRG` status as offline/on-battery. diff --git a/drivers/usbhid-ups.c b/drivers/usbhid-ups.c index 235cb2986a..6096bf54e5 100644 --- a/drivers/usbhid-ups.c +++ b/drivers/usbhid-ups.c @@ -29,7 +29,7 @@ */ #define DRIVER_NAME "Generic HID driver" -#define DRIVER_VERSION "0.57" +#define DRIVER_VERSION "0.58" #define HU_VAR_WAITBEFORERECONNECT "waitbeforereconnect" @@ -139,6 +139,18 @@ bool_t use_interrupt_pipe = TRUE; bool_t use_interrupt_pipe = FALSE; #endif static size_t interrupt_pipe_EIO_count = 0; /* How many times we had I/O errors since last reconnect? */ + +/** + * How many times do we tolerate having "0 HID objects" in a row? + * Default -1 means indefinitely, but when some controllers hang, + * this is a clue that we want to fully restart the connection. + */ +static long interrupt_pipe_no_events_tolerance = -1; +/* How many times did we actually have "Got 0 HID objects" in a row? */ +static long interrupt_pipe_no_events_count = 0; +/* How HIDGetEvents() below reports no events found */ +#define NUT_LIBUSB_CODE_NO_EVENTS 0 + static time_t lastpoll; /* Timestamp the last polling */ hid_dev_handle_t udev = HID_DEV_HANDLE_CLOSED; @@ -1044,6 +1056,8 @@ void upsdrv_makevartable(void) addvar(VAR_FLAG, "pollonly", "Don't use interrupt pipe, only use polling"); + addvar(VAR_VALUE, "interrupt_pipe_no_events_tolerance", "How many times in a row do we tolerate \"Got 0 HID objects\" from USB interrupts?"); + addvar(VAR_FLAG, "onlinedischarge", "Set to treat discharging while online as being offline/on-battery (DEPRECATED, use onlinedischarge_onbattery)"); @@ -1095,7 +1109,6 @@ void upsdrv_makevartable(void) } #define MAX_EVENT_NUM 32 -#define NO_EVENTS 0 void upsdrv_updateinfo(void) { @@ -1131,6 +1144,7 @@ void upsdrv_updateinfo(void) hd = &curDevice; interrupt_pipe_EIO_count = 0; + interrupt_pipe_no_events_count = 0; if (hid_ups_walk(HU_WALKMODE_INIT) == FALSE) { hd = NULL; @@ -1149,10 +1163,19 @@ void upsdrv_updateinfo(void) case LIBUSB_ERROR_BUSY: /* Device or resource busy */ upslog_with_errno(LOG_CRIT, "Got disconnected by another driver"); goto fallthrough_reconnect; + case NUT_LIBUSB_CODE_NO_EVENTS: /* No HID Events */ + interrupt_pipe_no_events_count++; + upsdebugx(1, "Got 0 HID objects (%ld times in a row, tolerance is %ld)...", + interrupt_pipe_no_events_count, interrupt_pipe_no_events_tolerance); + if (interrupt_pipe_no_events_tolerance >= 0 + && interrupt_pipe_no_events_tolerance < interrupt_pipe_no_events_count + ) { + goto fallthrough_reconnect; + } + break; #if WITH_LIBUSB_0_1 /* limit to libusb 0.1 implementation */ case -EPERM: /* Operation not permitted */ #endif - case NO_EVENTS: /* No HID Events */ case LIBUSB_ERROR_NO_DEVICE: /* No such device */ case LIBUSB_ERROR_ACCESS: /* Permission denied */ #if WITH_LIBUSB_0_1 /* limit to libusb 0.1 implementation */ @@ -1173,6 +1196,8 @@ void upsdrv_updateinfo(void) return; default: upsdebugx(1, "Got %i HID objects...", (evtCount >= 0) ? evtCount : 0); + if (evtCount > 0) + interrupt_pipe_no_events_count = 0; break; } } else { @@ -1273,6 +1298,15 @@ void upsdrv_initinfo(void) use_interrupt_pipe = FALSE; } + val = getval("interrupt_pipe_no_events_tolerance"); + if (!val || !str_to_long(val, &interrupt_pipe_no_events_tolerance, 10)) { + interrupt_pipe_no_events_tolerance = -1; + if (val) + upslogx(LOG_WARNING, "Invalid setting for interrupt_pipe_no_events_tolerance: '%s', defaulting to %ld", + val, interrupt_pipe_no_events_tolerance); + } + dstate_setinfo("driver.parameter.interrupt_pipe_no_events_tolerance", "%ld", interrupt_pipe_no_events_tolerance); + time(&lastpoll); /* install handlers */ diff --git a/include/nutconf.hpp b/include/nutconf.hpp index 5d555dbc78..a97298ce82 100644 --- a/include/nutconf.hpp +++ b/include/nutconf.hpp @@ -1861,6 +1861,7 @@ class UpsConfiguration : public GenericConfiguration inline long long int getI2C_address(const std::string & ups) const { return getInt(ups, "i2c_address"); } inline long long int getIdleLoad(const std::string & ups) const { return getInt(ups, "idleload"); } // CHECKME inline long long int getInputTimeout(const std::string & ups) const { return getInt(ups, "input_timeout"); } // CHECKME + inline long long int getInterruptPipeNoEventsTolerance(const std::string & ups) const { return getInt(ups, "interrupt_pipe_no_events_tolerance"); } inline long long int getInterruptSize(const std::string & ups) const { return getInt(ups, "interruptsize"); } inline long long int getLineVoltage(const std::string & ups) const { return getInt(ups, "linevoltage"); } // CHECKME inline long long int getLoadpercentage(const std::string & ups) const { return getInt(ups, "loadPercentage"); } // CHECKME @@ -2075,6 +2076,7 @@ class UpsConfiguration : public GenericConfiguration inline void setI2C_address(const std::string & ups, long long int val) { setInt(ups, "i2c_address", val); } inline void setIdleLoad(const std::string & ups, long long int idleload) { setInt(ups, "idleload", idleload); } // CHECKME inline void setInputTimeout(const std::string & ups, long long int timeout) { setInt(ups, "input_timeout", timeout); } // CHECKME + inline void setInterruptPipeNoEventsTolerance(const std::string & ups, long long int val) { setInt(ups, "interrupt_pipe_no_events_tolerance", val); } inline void setInterruptSize(const std::string & ups, long long int val) { setInt(ups, "interruptsize", val); } inline void setLineVoltage(const std::string & ups, long long int linevoltage) { setInt(ups, "linevoltage", linevoltage); } // CHECKME inline void setLoadpercentage(const std::string & ups, long long int load) { setInt(ups, "loadPercentage", load); } // CHECKME