Fix fallout of #2671 and complete the feature with optional interrupt_pipe_no_events_tolerance setting

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

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.

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,13 @@ 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;
+			else
+				upsdebugx(1, "Got unhandled result from HIDGetEvents(): %i\n"
+					"Please report it to NUT developers, with an 'upsc' output for your device,\n"
+					"versions of NUT and libusb used, and verbose driver debug log if possible.",
+					evtCount);
 			break;
 		}
 	} else {
@@ -1273,6 +1303,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 */

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