huawei-ups2000: fix broken instant command invocations and undefined Modbus behaviors.

NEWS.adoc

@@ -117,6 +117,21 @@ https://github.com/networkupstools/nut/milestone/13
       authentication configuration discovery. It accepts `default`, `none`,
       or a specific authconf file path. [issue #3329]
 
+ - `huawei-ups2000` driver updates:
+    * Fixed a Modbus compatibility problem that made changing any variables
+      or sending any instant commands impossible with some UPS models. This
+      was caused by the use of `modbus_write_registers()` (Modbus command
+      `0x10`), which is not officially supported, making any register write
+      impossible on some models. All uses of `modbus_write_registers()` have
+      been changed to `modbus_write_register()` (Modbus command `0x06`) to
+      avoid this problem. [issue #3593, PR #3604].
+
+    * Fixed a serious problem previously existed in all NUT versions that
+      made many instant commands unusable, including `bypass.start`,
+      `shutdown.return`, `shutdown.reboot`, and `shutdown.reboot.graceful`.
+      Any attempt to use them would fail, with an error logged to `syslog`:
+      `reg1 is negative`. [issue #3603, PR #3604].
+
  - `nhs_ser` driver updates:
     * Modernized serial communication and added validated settings for baud
       rate, data bits, parity, stop bits, flow control, per-byte read timeout,

UPGRADING.adoc

@@ -87,6 +87,16 @@ The new `-A filename` option defaults to trying to use a `nutauth.conf` file
   device, please add `cypress_drain_quirk = 0` in your `ups.conf` and let us
   know. [issue #2534, PR #3563]
 
+- The `huawei-ups2000` driver fixed two serious problems that affected all
+  previous NUT versions. All `huawei-ups2000` users are strongly recommended
+  to upgrade to NUT 2.8.6, all previous versions are considered too unreliable
+  to use. Due to a Modbus compatibility problem, it was not possible to *write
+  to any registers at all* on some UPS models, meaning that changing any
+  *variable* or sending any *instant command* was impossible. Furthermore, due
+  to a broken instant command handler, some instant commands would always fail
+  with a `reg1 is negative` error (even with a compatible UPS), including
+  `bypass.start`, `shutdown.return`, `shutdown.reboot`, and
+  `shutdown.reboot.graceful`. [issue #3593, #3603, PR #3604].
 
 Changes from 2.8.4 to 2.8.5
 ---------------------------

docs/man/huawei-ups2000.txt

@@ -375,6 +375,16 @@ to endless read failures and permanent data stalls.
 Driver v0.12 (NUT v2.8.5) fixed support for Microsoft Windows,
 Windows was previously unsupported due to a file locking bug.
 
+Driver v0.14 (NUT v2.8.6) fixed two serious problems that affected all
+previous NUT versions, upgrading is strongly recommended. Due to a broken
+instant command handler, it was impossible to send some instant commands
+to the UPS, including *bypass.start*, *shutdown.return*, *shutdown.reboot*,
+and *shutdown.reboot.graceful*, any attempt to use them will fail with an
+"reg1 is negative" error in the syslog. Due to a separate Modbus compatibility
+problem, the negative impact is worse on some UPS models: it was not possible
+to *write to any registers at all*, meaning that changing any *variable* or
+sending any *instant command* is impossible.
+
 See *Historical bugs* at the bottom of this manual for links.
 
 Battery status has a non-fatal read failure
@@ -492,6 +502,12 @@ https://github.com/networkupstools/nut/issues/3244
 * Commit: call ser_close() before modbus_connect(), fix #3244:
 https://github.com/networkupstools/nut/commit/db2e148503d654726d5520d68e20e71117335813
 
+* Issue 3593: huawei-ups2000: shutdown commands never power off the UPS on firmware V2R1C1SPC50 - the unit ignores Modbus function 0x10 writes; only 0x06 works
+https://github.com/networkupstools/nut/issues/3593
+
+* Issue 3603: huawei-ups2000: bypass.start, shutdown.return, shutdown.reboot, and shutdown.reboot.graceful are broken in all NUT versions
+https://github.com/networkupstools/nut/issues/3603
+
 Windows Drivers:
 ~~~~~~~~~~~~~~~~
 

drivers/huawei-ups2000.c

@@ -55,7 +55,7 @@
 #endif
 
 #define DRIVER_NAME	"NUT Huawei UPS2000 (1kVA-3kVA) RS-232 Modbus driver (libmodbus link type: " NUT_MODBUS_LINKTYPE_STR ")"
-#define DRIVER_VERSION	"0.13"
+#define DRIVER_VERSION	"0.14"
 
 #define CHECK_BIT(var,pos) ((var) & (1<<(pos)))
 #define MODBUS_SLAVE_ID 1
@@ -162,7 +162,6 @@ static void ups2000_device_identification(void);
 static size_t ups2000_read_serial(uint8_t *buf, size_t buf_len);
 static int ups2000_read_registers(modbus_t *ctx, int addr, int nb, uint16_t *dest);
 static int ups2000_write_register(modbus_t *ctx, int addr, uint16_t val);
-static int ups2000_write_registers(modbus_t *ctx, int addr, int nb, uint16_t *src);
 static uint16_t crc16(uint8_t *buffer, size_t buffer_length);
 static time_t time_seek(time_t t, int seconds);
 
@@ -1446,14 +1445,17 @@ static int ups2000_delay_set(const char *var, const char *string)
  *
  * 3. Calling "*handler_func" and passing "reg1". This is
  * used to handle commands that needs additional processing.
- * If "reg1" is not necessary or unsuitable, "-1" is used.
+ * If "reg1" is not necessary or unsuitable, "0" is used.
  */
-#define REG_NULL  -1, -1
+#define REG_NULL  0, -1
 #define FUNC_NULL NULL
 
 static struct ups2000_cmd_t {
 	const char *cmd;
-	const int16_t reg1, val1, reg2, val2;
+	const uint16_t reg1;
+	const int16_t val1;
+	const uint16_t reg2;
+	const int16_t val2;
 	int (*const handler_func)(const uint16_t);
 } ups2000_cmd[] =
 {
@@ -1471,7 +1473,7 @@ static struct ups2000_cmd_t {
 	{ "shutdown.return",          REG_NULL, REG_NULL, ups2000_instcmd_shutdown_return          },
 	{ "shutdown.reboot",          REG_NULL, REG_NULL, ups2000_instcmd_shutdown_reboot          },
 	{ "shutdown.reboot.graceful", REG_NULL, REG_NULL, ups2000_instcmd_shutdown_reboot_graceful },
-	{ NULL, -1, -1, -1, -1, NULL },
+	{ NULL, 0, -1, 0, -1, NULL },
 };
 
 
@@ -1510,15 +1512,9 @@ static int instcmd(const char *cmd, const char *extra)
 
 	if (cmd_action->handler_func) {
 		/* handled by a function */
-		if (cmd_action->reg1 < 0) {
-			/* FIXME: ...INSTCMD_CONVERSION_FAILED ? */
-			upslogx(LOG_INSTCMD_UNKNOWN, "instcmd: command [%s] reg1 is negative", cmd);
-			return STAT_INSTCMD_UNKNOWN;
-		} else {
-			status = cmd_action->handler_func((uint16_t)cmd_action->reg1);
-		}
+		status = cmd_action->handler_func(cmd_action->reg1);
 	}
-	else if (cmd_action->reg1 >= 0 && cmd_action->val1 >= 0) {
+	else if (cmd_action->reg1 > 0 && cmd_action->val1 >= 0) {
 		/* handled by a register write */
 		int r = ups2000_write_register(modbus_ctx,
 			10000 + cmd_action->reg1,
@@ -1532,7 +1528,7 @@ static int instcmd(const char *cmd, const char *extra)
 		 * if the previous write succeeds and there is an additional
 		 * register to write.
 		 */
-		if (r == 1 && cmd_action->reg2 >= 0 && cmd_action->val2 >= 0) {
+		if (r == 1 && cmd_action->reg2 > 0 && cmd_action->val2 >= 0) {
 			r = ups2000_write_register(modbus_ctx,
 				10000 + cmd_action->reg2,
 				(uint16_t)cmd_action->val2);
@@ -1559,6 +1555,12 @@ static int ups2000_instcmd_load_on(const uint16_t reg)
 	int r;
 	const char *status;
 
+	if (reg == 0) {
+		upslogx(LOG_INSTCMD_FAILED,
+			"invalid register in LUT, please file a bug report!");
+		return STAT_INSTCMD_FAILED;
+	}
+
 	/* force refresh UPS status */
 	status_init();
 	r = ups2000_update_status();
@@ -1650,6 +1652,12 @@ static int ups2000_instcmd_beeper_toggle(const uint16_t reg)
 	int r;
 	const char *string;
 
+	if (reg == 0) {
+		upslogx(LOG_INSTCMD_FAILED,
+			"invalid register in LUT, please file a bug report!");
+		return STAT_INSTCMD_FAILED;
+	}
+
 	r = ups2000_beeper_get(reg);
 	if (r != 0)
 		return STAT_INSTCMD_FAILED;
@@ -1678,6 +1686,12 @@ static int ups2000_instcmd_shutdown_stayoff(const uint16_t reg)
 	uint16_t val;
 	int r;
 
+	if (reg == 0) {
+		upslogx(LOG_INSTCMD_FAILED,
+			"invalid register in LUT, please file a bug report!");
+		return STAT_INSTCMD_FAILED;
+	}
+
 	r = setvar("ups.start.auto", "no");
 	if (r != STAT_SET_HANDLED)
 		return STAT_INSTCMD_FAILED;
@@ -1716,8 +1730,12 @@ static int ups2000_shutdown_guaranteed_return(uint16_t offdelay, uint16_t ondela
 	val[0] = (offdelay * 10) / 60;
 	val[1] = ondelay / 60;
 
-	r = ups2000_write_registers(modbus_ctx, 1047 + 10000, 2, val);
-	if (r != 2)
+	r = ups2000_write_register(modbus_ctx, 1047 + 10000, val[0]);
+	if (r != 1)
+		return STAT_INSTCMD_FAILED;
+
+	r = ups2000_write_register(modbus_ctx, 1048 + 10000, val[1]);
+	if (r != 1)
 		return STAT_INSTCMD_FAILED;
 
 	return STAT_INSTCMD_HANDLED;
@@ -2046,7 +2064,14 @@ static int ups2000_read_registers(modbus_t *ctx, int addr, int nb, uint16_t *des
 }
 
 
-static int ups2000_write_registers(modbus_t *ctx, int addr, int nb, uint16_t *src)
+/*
+ * Huawei UPS2000 doesn't officially support multiple-register writes (Modbus
+ * command 0x10), the official datasheet only supports single-register writes
+ * (Modbus command 0x06). Multiple-register writes worked on some models, but
+ * this turned out to be undefined behavior, it doesn't work with all models.
+ * This is why this function is not symmetric to ups2000_read_registers().
+ */
+static int ups2000_write_register(modbus_t *ctx, int addr, uint16_t val)
 {
 	int i;
 	int r = -1;
@@ -2057,37 +2082,31 @@ static int ups2000_write_registers(modbus_t *ctx, int addr, int nb, uint16_t *sr
 			"Please file a bug report!", addr);
 
 	for (i = 0; i < 3; i++) {
-		r = modbus_write_registers(ctx, addr, nb, src);
+		r = modbus_write_register(ctx, addr, val);
 
 		/* generic retry for modbus write failures. */
-		if (retry_status == RETRY_ENABLE && r != nb) {
-			upslogx(LOG_WARNING, "modbus_write_registers() failed (%d, errno %d): %s",
+		if (retry_status == RETRY_ENABLE && r != 1) {
+			upslogx(LOG_WARNING, "modbus_write_register() failed (%d, errno %d): %s",
 				r, errno, modbus_strerror(errno));
 			upslogx(LOG_WARNING, "Register %04d has a write failure. Retrying...", addr);
 			sleep(1);
 			continue;
 		}
-		else if (r == nb)
+		else if (r == 1)
 			retry_status = RETRY_ENABLE;
 
 		return r;
 	}
 
 	/* Give up */
-	upslogx(LOG_ERR, "modbus_write_registers() failed (%d, errno %d): %s",
+	upslogx(LOG_ERR, "modbus_write_register() failed (%d, errno %d): %s",
 		r, errno, modbus_strerror(errno));
 	upslogx(LOG_ERR, "Register %04d has a fatal write failure.", addr);
 	retry_status = RETRY_DISABLE_TEMPORARY;
 	return r;
 }
 
 
-static int ups2000_write_register(modbus_t *ctx, int addr, uint16_t val)
-{
-	return ups2000_write_registers(ctx, addr, 1, &val);
-}
-
-
 /*
  * The following CRC-16 code was copied from libmodbus.
  *