Merge pull request #2722 from jimklimov/cps-hid-timings

Fix default CPS HID timings

NEWS.adoc

@@ -203,6 +203,11 @@ https://github.com/networkupstools/nut/milestone/11
      by default: `powercom_sdcmd_byte_order_fallback`. [PR #2480]
    * `cps-hid` subdriver now supports more variables, as available on e.g.
      CP1350EPFCLCD model, including temperature. [PRs #2540, #2711]
+   * loudly suggest to set `pollonly` flag and default a shorter `pollfreq`
+     for CPS devices, to try avoiding device-driven timeouts. [#1689]
+     Also adjust default `offdelay` and `ondelay` to reasonable values,
+     and warn the users with CPS devices if their configured values are
+     not multiples of 60. [#432, #1394]
    * in `cps-hid` subdriver, `cps_fix_report_desc()` method should now handle
      mismatched `LogMax` ranges for input and output voltages, whose USB Report
      Descriptors are wrongly encoded by some firmware versions. [#1512]

UPGRADING.adoc

@@ -73,6 +73,15 @@ Changes from 2.8.2 to 2.8.3
   devices; just in case a flag toggle `powercom_sdcmd_byte_order_fallback`
   was added to set the old behavior (if some devices do need it). [PR #2480]
 
+- `usbhid-ups` subdriver `CyberPower HID` default `pollfreq` sped up to
+  12 seconds (common default is 30 seconds). Feedback is welcome if this
+  improves connection stability or overwhelms the UPS controller instead.
+  [issue #1689, PR #2718]
+
+- `usbhid-ups` subdriver `CyberPower HID` default `offdelay` is set to 60
+  and `ondelay` to 120 seconds, in accordance with man page suggestions;
+  users with custom settings not divisible by 60 will be loudly warned. [#1394]
+
 - Added support for `lbrb_log_delay_sec=N` setting to delay propagation of
   `LB` or `LB+RB` state (buggy with APC BXnnnnMI devices/firmwares issued
   circa 2023-2024 which flood the logs with spurious LOWBATT and REPLACEBATT

docs/man/usbhid-ups.txt

@@ -88,8 +88,9 @@ helpful when the UPS sets this value to a lower percentage than intended.
 Set the timer before the UPS is turned off after the kill power command is
 sent (via the *-k* switch).
 +
-The default value is 20 (in seconds). Usually this *must be lower* than
-'ondelay', but the driver will *not* warn you upon startup if it isn't.
+The default value is 20 (in seconds), or 60 for CPS devices.
+Usually this *must be lower* than 'ondelay', but the driver will *not* warn
+you upon startup if it isn't.
 +
 Note that many Cyber Power Systems (CPS) models tend to divide this delay by
 60 and round down, so the minimum advisable value is 60 to avoid powering off
@@ -102,21 +103,22 @@ kill power command had been sent, but before the actual switch off. This
 ensures the machines connected to the UPS are, in all cases, rebooted after
 a power failure.
 +
-The default value is 30 (in seconds). Usually this *must be greater*
-than offdelay, but the driver will *not* warn you upon startup if it
-isn't. Some UPSes will restart no matter what, even if the power is
-(still) out at the moment this timer elapses. In that case, you could see
-whether setting `ondelay = -1` in *ups.conf* helps.
+The default value is 30 (in seconds), or 120 for CPS devices.
+Usually this *must be greater* than offdelay, but the driver will *not* warn
+you upon startup if it isn't. Some UPSes will restart no matter what, even
+if the power is (still) out at the moment this timer elapses. In that case,
+you could see whether setting `ondelay = -1` in *ups.conf* helps.
 +
 Note that many CPS models tend to divide this delay by 60 and round down, so
 the minimum advisable value is 120 to allow a short delay between when the UPS
-shuts down, and when the power returns.
+shuts down, and when the power returns. According to support statement (for at
+least some CPS models), "our UPS systems are unable to set up power on delay".
 
 *pollfreq*='num'::
 Set polling frequency for full updates, in seconds. Compared to the quick
 updates performed every "pollinterval" (the latter option is described in
 linkman:ups.conf[5]), the "pollfreq" interval is for polling the less-critical
-variables.  The default value is 30 (in seconds).
+variables.  The default value is 30 (in seconds), or 12 sec for CPS devices.
 
 *pollonly*::
 If this flag is set, the driver will not use Interrupt In transfers during the
@@ -342,6 +344,11 @@ be tuned with a kernel boot parameter (via GRUB etc.):
 
         usbhid.quirks=0x0463:0xffff:0x08
 
+Conversely, some hardware controllers may "fall asleep" when not contacted
+for too long; CPS devices are commonly associated with such behaviour.
+In this case, consider enabling `pollonly` flag and/or keeping `pollfreq`
+and/or `pollinterval` small.
+
 Got EPERM: Operation not permitted upon driver startup
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 

drivers/cps-hid.c

@@ -228,8 +228,8 @@ static hid_info_t cps_hid2nut[] = {
   { "ups.power.nominal", 0, 0, "UPS.Output.ConfigApparentPower", NULL, "%.0f", 0, NULL },
   { "ups.realpower", 0, 0, "UPS.Output.ActivePower", NULL, "%.0f", 0, NULL },
   { "ups.realpower.nominal", 0, 0, "UPS.Output.ConfigActivePower", NULL, "%.0f", 0, NULL },
-  { "ups.delay.start", ST_FLAG_RW | ST_FLAG_STRING, 10, "UPS.Output.DelayBeforeStartup", NULL, DEFAULT_ONDELAY, HU_FLAG_ABSENT, NULL},
-  { "ups.delay.shutdown", ST_FLAG_RW | ST_FLAG_STRING, 10, "UPS.Output.DelayBeforeShutdown", NULL, DEFAULT_OFFDELAY, HU_FLAG_ABSENT, NULL},
+  { "ups.delay.start", ST_FLAG_RW | ST_FLAG_STRING, 10, "UPS.Output.DelayBeforeStartup", NULL, DEFAULT_ONDELAY_CPS, HU_FLAG_ABSENT, NULL},
+  { "ups.delay.shutdown", ST_FLAG_RW | ST_FLAG_STRING, 10, "UPS.Output.DelayBeforeShutdown", NULL, DEFAULT_OFFDELAY_CPS, HU_FLAG_ABSENT, NULL},
   { "ups.timer.start", 0, 0, "UPS.Output.DelayBeforeStartup", NULL, "%.0f", HU_FLAG_QUICK_POLL, NULL},
   { "ups.timer.shutdown", 0, 0, "UPS.Output.DelayBeforeShutdown", NULL, "%.0f", HU_FLAG_QUICK_POLL, NULL},
   { "ups.timer.reboot", 0, 0, "UPS.Output.DelayBeforeReboot", NULL, "%.0f", HU_FLAG_QUICK_POLL, NULL},
@@ -266,8 +266,8 @@ static hid_info_t cps_hid2nut[] = {
   { "test.battery.start.quick", 0, 0, "UPS.Output.Test", NULL, "1", HU_TYPE_CMD, NULL },
   { "test.battery.start.deep", 0, 0, "UPS.Output.Test", NULL, "2", HU_TYPE_CMD, NULL },
   { "test.battery.stop", 0, 0, "UPS.Output.Test", NULL, "3", HU_TYPE_CMD, NULL },
-  { "load.off.delay", 0, 0, "UPS.Output.DelayBeforeShutdown", NULL, DEFAULT_OFFDELAY, HU_TYPE_CMD, NULL },
-  { "load.on.delay", 0, 0, "UPS.Output.DelayBeforeStartup", NULL, DEFAULT_ONDELAY, HU_TYPE_CMD, NULL },
+  { "load.off.delay", 0, 0, "UPS.Output.DelayBeforeShutdown", NULL, DEFAULT_OFFDELAY_CPS, HU_TYPE_CMD, NULL },
+  { "load.on.delay", 0, 0, "UPS.Output.DelayBeforeStartup", NULL, DEFAULT_ONDELAY_CPS, HU_TYPE_CMD, NULL },
   { "shutdown.stop", 0, 0, "UPS.Output.DelayBeforeShutdown", NULL, "-1", HU_TYPE_CMD, NULL },
   { "shutdown.reboot", 0, 0, "UPS.Output.DelayBeforeReboot", NULL, "10", HU_TYPE_CMD, NULL },
   { "beeper.on", 0, 0, "UPS.PowerSummary.AudibleAlarmControl", NULL, "2", HU_TYPE_CMD, NULL },

drivers/usbhid-ups.c

@@ -1042,21 +1042,22 @@ void upsdrv_makevartable(void)
 	addvar (VAR_VALUE, HU_VAR_LOWBATT, temp);
 
 	snprintf(temp, sizeof(temp),
-		"Set shutdown delay, in seconds (default=%s)",
-		DEFAULT_OFFDELAY);
+		"Set shutdown delay, in seconds (default=%s, or %s for CPS devices)",
+		DEFAULT_OFFDELAY, DEFAULT_OFFDELAY_CPS);
 	addvar(VAR_VALUE, HU_VAR_OFFDELAY, temp);
 
 	snprintf(temp, sizeof(temp),
-		"Set startup delay, in seconds (default=%s)",
-		DEFAULT_ONDELAY);
+		"Set startup delay, in seconds (default=%s, or %s for CPS devices)",
+		DEFAULT_ONDELAY, DEFAULT_ONDELAY_CPS);
 	addvar(VAR_VALUE, HU_VAR_ONDELAY, temp);
 
 	snprintf(temp, sizeof(temp),
-		"Set polling frequency, in seconds, to reduce data flow (default=%d)",
-		DEFAULT_POLLFREQ);
+		"Set polling frequency, in seconds, to reduce data flow "
+		"(default=%d, or %d for CPS devices)",
+		DEFAULT_POLLFREQ, DEFAULT_POLLFREQ_CPS);
 	addvar(VAR_VALUE, HU_VAR_POLLFREQ, temp);
 
-	addvar(VAR_FLAG, "pollonly", "Don't use interrupt pipe, only use polling");
+	addvar(VAR_FLAG, "pollonly", "Don't use interrupt pipe, only use polling (recommended for CPS devices)");
 
 	addvar(VAR_VALUE, "interrupt_pipe_no_events_tolerance", "How many times in a row do we tolerate \"Got 0 HID objects\" from USB interrupts?");
 
@@ -1292,17 +1293,34 @@ void upsdrv_initinfo(void)
 
 	dstate_setinfo("driver.version.data", "%s", subdriver->name);
 
-	/* init polling frequency */
+	/* init polling frequency for full updates */
 	val = getval(HU_VAR_POLLFREQ);
 	if (val) {
 		pollfreq = atoi(val);
+#if !((defined SHUT_MODE) && SHUT_MODE)
+	} else {
+		/* note, there is also 'pollinterval'/poll_interval (C var)
+		 * common delay between main.c loops */
+		if (subdriver == &cps_subdriver) {
+			upslogx(LOG_INFO, "Defaulting '%s' to %d for CPS devices",
+				HU_VAR_POLLFREQ, DEFAULT_POLLFREQ_CPS);
+			pollfreq = DEFAULT_POLLFREQ_CPS;
+		}
+#endif
 	}
 
 	dstate_setinfo("driver.parameter.pollfreq", "%d", pollfreq);
 
 	/* ignore (broken) interrupt pipe */
 	if (testvar("pollonly")) {
 		use_interrupt_pipe = FALSE;
+#if !((defined SHUT_MODE) && SHUT_MODE)
+	} else {
+		if (subdriver == &cps_subdriver) {
+			upslogx(LOG_WARNING, "You may want to set 'pollonly' "
+				"flag on CPS devices");
+		}
+#endif
 	}
 
 	val = getval("interrupt_pipe_no_events_tolerance");
@@ -1579,15 +1597,33 @@ void upsdrv_initups(void)
 		/* Retrieve user defined delay settings */
 		val = getval(HU_VAR_ONDELAY);
 		if (val) {
-			dstate_setinfo("ups.delay.start", "%ld", strtol(val, NULL, 10));
+			long	l = strtol(val, NULL, 10);
+#if !((defined SHUT_MODE) && SHUT_MODE)
+			if (subdriver == &cps_subdriver
+			 && (l < 60 || l % 60)
+			) {
+				upslogx(LOG_WARNING, "CPS devices tend to round delays by 60 sec down (ondelay=120 is the suggested minimum; see more in the man page)");
+			}
+#endif
+			dstate_setinfo("ups.delay.start", "%ld", l);
 		}
 	}
 
 	if (dstate_getinfo("ups.delay.shutdown")) {
 		/* Retrieve user defined delay settings */
 		val = getval(HU_VAR_OFFDELAY);
 		if (val) {
-			dstate_setinfo("ups.delay.shutdown", "%ld", strtol(val, NULL, 10));
+			long	l = strtol(val, NULL, 10);
+#if !((defined SHUT_MODE) && SHUT_MODE)
+			if (subdriver == &cps_subdriver
+			 && (l > 0 && (l < 60 || l % 60))
+			) {
+				/* Note: zero and negative values may
+				 * have special meanings for the firmware */
+				upslogx(LOG_WARNING, "CPS devices tend to round delays by 60 sec down (offdelay=60 is the suggested minimum; see more in the man page)");
+			}
+#endif
+			dstate_setinfo("ups.delay.shutdown", "%ld", l);
 		}
 	}
 

drivers/usbhid-ups.h

@@ -53,13 +53,18 @@ extern bool_t	 	use_interrupt_pipe;	/* Set to FALSE if interrupt reports should
 /* Parameters default values */
 #define DEFAULT_LOWBATT		"30"	/* percentage of battery charge to consider the UPS in low battery state  */
 #define DEFAULT_ONDELAY		"30"	/* Delay between return of utility power */
-					/* and powering up of load, in seconds */
+					/* and powering up of load, in seconds (as string!) */
 					/* CAUTION: ondelay > offdelay */
-#define DEFAULT_OFFDELAY	"20"	/* Delay before power off, in seconds */
+#define DEFAULT_OFFDELAY	"20"	/* Delay before power off, in seconds (as string!) */
 #define DEFAULT_POLLFREQ	30	/* Polling interval, in seconds */
 					/* The driver will wait for Interrupt */
 					/* and do "light poll" in the meantime */
 
+/* Parameters default values for CyberPower HID */
+#define DEFAULT_POLLFREQ_CPS	12	/* Polling interval, in seconds, default for CPS devices */
+#define DEFAULT_ONDELAY_CPS	"120"	/* Delay between return of utility power, default for CPS devices */
+#define DEFAULT_OFFDELAY_CPS	"60"	/* Delay before power off, default for CPS devices */
+
 #ifndef MAX_STRING_SIZE
 #define MAX_STRING_SIZE	128
 #endif