Merge pull request networkupstools#2480 from chbgdn/powercom-shutdown…

…-fix

drivers/powercom-hid.c: fix bytes order in shutdown commands

NEWS.adoc

@@ -95,6 +95,10 @@ https://github.com/networkupstools/nut/milestone/11
      series (largely guessing, feedback and PRs for adaptation to actual
      string values reported by devices via USB are welcome), so these devices
      would now report `battery.voltage` and `battery.voltage.nominal`. [#2380]
+   * `powercom-hid` subdriver sent UPS shutdown commands in wrong byte order,
+     at least for devices currently in the field. A toggle was added to set
+     the old behavior (if some devices do need it), while a fix is applied
+     by default: `powercom_sdcmd_byte_order_fallback`. [PR #2480]
 
  - bicker_ser: added new driver for Bicker 12/24Vdc UPS via RS-232 serial
    communication protocol, which supports any UPS shipped with the PSZ-1053

UPGRADING.adoc

@@ -26,6 +26,11 @@ Changes from 2.8.2 to 2.8.3
 
 - PLANNED: Keep track of any further API clean-up?
 
+- `usbhid-ups` subdriver `PowerCOM HID` subdriver sent UPS `shutdown` and
+  `stayoff` commands in wrong byte order, at least for devices currently
+  in the field. Driver now sends the commands in a way that satisfies new
+  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]
 
 - Enabled installation of built PDF and HTML (including man page renditions)
   files under the configured `docdir`. It seems previously they were only

docs/man/usbhid-ups.txt

@@ -174,6 +174,15 @@ It is also always possible that NUT fix-ups cause issues on some devices,
 whether due to NUT bugs or because the vendor protocol implementation is
 broken in more than one place.
 
+*powercom_sdcmd_byte_order_fallback*::
+Original `PowerCOM HID` subdriver code (until version 0.7) sent UPS `shutdown`
+and `stayoff` commands in a wrong byte order, than what is needed by actual
+devices seen in the field in 2024.  The byte order is fixed to satisfy new
+devices by default since version 0.71.  Just in case there are different
+firmwares out there with opposite behaviors, we provide this toggle to use
+old behavior in a particular deployment.  Maybe it was just a bug and nobody
+needs this fall-back...
+
 *explore*::
 With this option, the driver will connect to any device, including
 ones that are not yet supported. This must always be combined with the

docs/nut.dict

@@ -1,4 +1,4 @@
-personal_ws-1.1 en 3188 utf-8
+personal_ws-1.1 en 3189 utf-8
 AAC
 AAS
 ABI
@@ -2700,6 +2700,7 @@ screenshot
 screenshots
 scriptname
 sd
+sdcmd
 sddelay
 sdk
 sdl

drivers/powercom-hid.c

@@ -4,7 +4,7 @@
  *  2003 - 2009	Arnaud Quette <[email protected]>
  *  2005 - 2006	Peter Selinger <[email protected]>
  *  2008 - 2009	Arjen de Korte <[email protected]>
- *  2020 - 2022	Jim Klimov <[email protected]>
+ *  2020 - 2024	Jim Klimov <[email protected]>
  *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -26,7 +26,7 @@
 #include "powercom-hid.h"
 #include "usb-common.h"
 
-#define POWERCOM_HID_VERSION	"PowerCOM HID 0.7"
+#define POWERCOM_HID_VERSION	"PowerCOM HID 0.71"
 /* FIXME: experimental flag to be put in upsdrv_info */
 
 /* PowerCOM */
@@ -54,6 +54,14 @@ static usb_device_id_t powercom_usb_device_table[] = {
 
 static char powercom_scratch_buf[32];
 
+/* Original subdriver code until version 0.7 sent shutdown commands
+ * in a wrong byte order than is needed by devices seen in the field
+ * in 2024. Just in case there are different firmwares, we provide
+ * a toggle to use old behavior. Maybe it was just a bug and nobody
+ * needs this fall-back...
+ */
+static char powercom_sdcmd_byte_order_fallback = 0;
+
 static const char *powercom_startup_fun(double value)
 {
 	uint16_t	i = value;
@@ -93,7 +101,14 @@ static const char *powercom_shutdown_fun(double value)
 {
 	uint16_t	i = value;
 
-	snprintf(powercom_scratch_buf, sizeof(powercom_scratch_buf), "%d", 60 * (i & 0x00FF) + (i >> 8));
+	if (powercom_sdcmd_byte_order_fallback) {
+		/* Legacy behavior */
+		snprintf(powercom_scratch_buf, sizeof(powercom_scratch_buf), "%d", 60 * (i & 0x00FF) + (i >> 8));
+	} else {
+		/* New default */
+		snprintf(powercom_scratch_buf, sizeof(powercom_scratch_buf), "%d", 60 * (i >> 8) + (i & 0x00FF));
+	}
+
 	upsdebugx(3, "%s: value = %.0f, buf = %s", __func__, value, powercom_scratch_buf);
 
 	return powercom_scratch_buf;
@@ -113,8 +128,16 @@ static double powercom_shutdown_nuf(const char *value)
 
 	val = (uint16_t)iv;
 	val = val ? val : 1;    /* 0 sets the maximum delay */
-	command = ((uint16_t)((val % 60) << 8)) + (uint16_t)(val / 60);
-	command |= 0x4000;	/* AC RESTART NORMAL ENABLE */
+	if (powercom_sdcmd_byte_order_fallback) {
+		/* Legacy behavior */
+		command = ((uint16_t)((val % 60) << 8)) + (uint16_t)(val / 60);
+		command |= 0x4000;	/* AC RESTART NORMAL ENABLE */
+	} else {
+		/* New default */
+		command = ((uint16_t)((val / 60) << 8)) + (uint16_t)(val % 60);
+		command |= 0x0040;	/* AC RESTART NORMAL ENABLE */
+	}
+
 	upsdebugx(3, "%s: value = %s, command = %04X", __func__, value, command);
 
 	return command;
@@ -138,8 +161,16 @@ static double powercom_stayoff_nuf(const char *value)
 
 	val = (uint16_t)iv;
 	val = val ? val : 1;    /* 0 sets the maximum delay */
-	command = ((uint16_t)((val % 60) << 8)) + (uint16_t)(val / 60);
-	command |= 0x8000;	/* AC RESTART NORMAL DISABLE */
+	if (powercom_sdcmd_byte_order_fallback) {
+		/* Legacy behavior */
+		command = ((uint16_t)((val % 60) << 8)) + (uint16_t)(val / 60);
+		command |= 0x8000;	/* AC RESTART NORMAL DISABLE */
+	} else {
+		/* New default */
+		command = ((uint16_t)((val / 60) << 8)) + (uint16_t)(val % 60);
+		command |= 0x0080;	/* AC RESTART NORMAL DISABLE */
+	}
+
 	upsdebugx(3, "%s: value = %s, command = %04X", __func__, value, command);
 
 	return command;
@@ -541,8 +572,9 @@ static int powercom_claim(HIDDevice_t *hd)
 		}
 		/* by default, reject, unless the productid option is given */
 		if (getval("productid")) {
-			return 1;
+			goto accept;
 		}
+		/* report and reject */
 		possibly_supported("PowerCOM", hd);
 		return 0;
 
@@ -551,12 +583,17 @@ static int powercom_claim(HIDDevice_t *hd)
 			interrupt_only = 1;
 			interrupt_size = 8;
 		}
-		return 1;
+		goto accept;
 
 	case NOT_SUPPORTED:
 	default:
 		return 0;
 	}
+
+accept:
+	powercom_sdcmd_byte_order_fallback = testvar("powercom_sdcmd_byte_order_fallback");
+
+	return 1;
 }
 
 subdriver_t powercom_subdriver = {

drivers/usbhid-ups.c

@@ -29,7 +29,7 @@
  */
 
 #define DRIVER_NAME	"Generic HID driver"
-#define DRIVER_VERSION	"0.54"
+#define DRIVER_VERSION	"0.55"
 
 #define HU_VAR_WAITBEFORERECONNECT "waitbeforereconnect"
 
@@ -1018,6 +1018,9 @@ void upsdrv_makevartable(void)
 	addvar(VAR_FLAG, "disable_fix_report_desc",
 		"Set to disable fix-ups for broken USB encoding, etc. which we apply by default on certain vendors/products");
 
+	addvar(VAR_FLAG, "powercom_sdcmd_byte_order_fallback",
+		"Set to use legacy byte order for Powercom HID shutdown commands. Either it was wrong forever, or some older devices/firmwares had it the other way around");
+
 #if !((defined SHUT_MODE) && SHUT_MODE)
 	addvar(VAR_VALUE, "subdriver", "Explicit USB HID subdriver selection");