diff --git a/NEWS.adoc b/NEWS.adoc index 8da073ce8e..8ab6ff846d 100644 --- a/NEWS.adoc +++ b/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, diff --git a/UPGRADING.adoc b/UPGRADING.adoc index 9a01aee6cf..0485efadcb 100644 --- a/UPGRADING.adoc +++ b/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 --------------------------- diff --git a/docs/man/huawei-ups2000.txt b/docs/man/huawei-ups2000.txt index 72a6238a26..c294fbdbf1 100644 --- a/docs/man/huawei-ups2000.txt +++ b/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: ~~~~~~~~~~~~~~~~ diff --git a/drivers/huawei-ups2000.c b/drivers/huawei-ups2000.c index b04c873a29..6de21ea9e9 100644 --- a/drivers/huawei-ups2000.c +++ b/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,24 +2082,24 @@ 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; @@ -2082,12 +2107,6 @@ static int ups2000_write_registers(modbus_t *ctx, int addr, int nb, uint16_t *sr } -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. *