From a56281bf2691f5b3f940ce7294c1db03aa9c3436 Mon Sep 17 00:00:00 2001 From: Nicola Fontana Date: Thu, 16 May 2024 15:41:01 +0200 Subject: [PATCH 01/23] bicker_ser: Bicker UPSes based on PSZ-1063 Add a new driver for Bicker DC UPS systems based on the PSZ-1063 extension module. This includes UPSIC-1205, UPSIC-2403 and DC2412-UPS(LD) models. Signed-off-by: Nicola Fontana --- drivers/Makefile.am | 4 +- drivers/bicker_ser.c | 306 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 309 insertions(+), 1 deletion(-) create mode 100644 drivers/bicker_ser.c diff --git a/drivers/Makefile.am b/drivers/Makefile.am index 670e6116ed..d8db1d3661 100644 --- a/drivers/Makefile.am +++ b/drivers/Makefile.am @@ -57,7 +57,7 @@ SERIAL_DRIVERLIST = al175 bcmxcp belkin belkinunv bestfcom \ gamatronic genericups isbmex liebert liebert-esp2 masterguard metasys \ mge-utalk microdowell microsol-apc mge-shut oneac optiups powercom rhino \ safenet nutdrv_siemens-sitop solis tripplite tripplitesu upscode2 victronups powerpanel \ - blazer_ser ivtscd apcsmart apcsmart-old riello_ser sms_ser + blazer_ser ivtscd apcsmart apcsmart-old riello_ser sms_ser bicker_ser SNMP_DRIVERLIST = snmp-ups USB_LIBUSB_DRIVERLIST = usbhid-ups bcmxcp_usb tripplite_usb \ blazer_usb richcomm_usb riello_usb \ @@ -186,6 +186,8 @@ riello_ser_SOURCES = riello.c riello_ser.c riello_ser_LDADD = $(LDADD) -lm sms_ser_SOURCES = sms_ser.c sms_ser_LDADD = $(LDADD) -lm +bicker_ser_SOURCES = bicker_ser.c +bicker_ser_LDADD = $(LDADD) -lm # non-serial drivers: these use custom LDADD and/or CFLAGS diff --git a/drivers/bicker_ser.c b/drivers/bicker_ser.c new file mode 100644 index 0000000000..5be95bdc79 --- /dev/null +++ b/drivers/bicker_ser.c @@ -0,0 +1,306 @@ +/* + * bicker_ser.c: support for Bicker SuperCapacitors DC UPSes + * + * Copyright (C) 2024 - Nicola Fontana + * + * 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 + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + */ + +/* The protocol is detailed in section 14 of the DC/UPSIC user's manual, + * downloadable somewhere from the Bicker website. + * + * Basically, this is a binary protocol without checksums: + * + * 1 byte 1 byte 1 byte 1 byte 0..252 bytes 1 byte + * +-------+-------+-------+-------+--- - - - ---+-------+ + * | SOH | Size | Index | CMD | Data | EOT | + * +-------+-------+-------+-------+--- - - - ---+-------+ + * + * where: + * - `SOH` is the start signal ('\x01') + * - `Size` is the length of the `Data` field (in bytes) plus 3 + * - `Index` is the command index (always '\x03') + * - `CMD` is the command code to execute + * - `Data` is the (optional) argument of the command + * - `EOT` is the end signal ('\x04') + * + * The same format is used for incoming and outcoming packets. Although + * not explicitly documented, the data returned by the `Data` field + * seems to be in little-endian order. + */ + +#include "config.h" +#include "main.h" +#include "attribute.h" + +#include "serial.h" + +#define DRIVER_NAME "Bicker serial protocol" +#define DRIVER_VERSION "0.01" + +#define BICKER_PACKET (1+1+1+1+252+1) +#define BICKER_SOH '\x01' +#define BICKER_EOT '\x04' + +upsdrv_info_t upsdrv_info = { + DRIVER_NAME, + DRIVER_VERSION, + "Nicola Fontana ", + DRV_EXPERIMENTAL, + { NULL } +}; + +static ssize_t bicker_send(char cmd, const char *data) +{ + char buf[BICKER_PACKET]; + size_t datalen, buflen; + ssize_t ret; + + ser_flush_io(upsfd); + + if (data != NULL) { + datalen = strlen(data); + if (datalen > 252) { + upslogx(LOG_ERR, "Data size exceeded: %d > 252", + (int)datalen); + return 0; + } + memcpy(buf + 4, data, datalen); + } else { + datalen = 0; + } + + buf[0] = BICKER_SOH; + buf[1] = datalen + 3; /* Packet size must include the header */ + buf[2] = '\x03'; /* Command index is always 3 */ + buf[3] = cmd; + buf[4 + datalen] = BICKER_EOT; + + /* The full packet includes SOH and EOT bytes too */ + buflen = datalen + 3 + 2; + + ret = ser_send_buf(upsfd, buf, buflen); + if (ret < 0) { + upslog_with_errno(LOG_WARNING, "ser_send_buf failed"); + return ret; + } else if ((size_t) ret != buflen) { + upslogx(LOG_WARNING, "ser_send_buf returned %d instead of %d", + (int)ret, (int)buflen); + return 0; + } + + upsdebug_hex(3, "bicker_send", buf, buflen); + return ret; +} + +static ssize_t bicker_receive_buf(void *dst, size_t dstlen) +{ + ssize_t ret; + + ret = ser_get_buf(upsfd, dst, dstlen, 1, 1); + if (ret < 0) { + upslog_with_errno(LOG_WARNING, "ser_get_buf failed"); + return ret; + } else if ((size_t) ret != dstlen) { + upslogx(LOG_WARNING, "ser_get_buf returned %d instead of %d", + (int)ret, (int)dstlen); + return 0; + } + + upsdebug_hex(3, "bicker_receive_buf", dst, dstlen); + return ret; +} + +static ssize_t bicker_receive(char cmd, void *dst, size_t dstlen) +{ + ssize_t ret; + size_t datalen; + char buf[BICKER_PACKET]; + + ret = bicker_receive_buf(buf, 1); + if (ret <= 0) { + return ret; + } else if (buf[0] != BICKER_SOH) { + upslogx(LOG_WARNING, "Received 0x%02X instead of SOH (0x%02X)", + (unsigned)buf[0], (unsigned)BICKER_SOH); + return 0; + } + + ret = bicker_receive_buf(buf + 1, 1); + if (ret <= 0) { + return ret; + } + + datalen = buf[1] - 3; + + ret = bicker_receive_buf(buf + 2, datalen + 3); + if (ret <= 0) { + return ret; + } else if (buf[datalen + 4] != BICKER_EOT) { + upslogx(LOG_WARNING, "Received 0x%02X instead of EOT (0x%02X)", + (unsigned)buf[datalen + 4], (unsigned)BICKER_EOT); + return 0; + } else if (buf[3] != cmd) { + upslogx(LOG_WARNING, "Commands do not match: sent 0x%02X, received 0x%02X", + (unsigned)cmd, (unsigned)buf[1]); + return 0; + } else if (dstlen < datalen) { + upslogx(LOG_ERR, "Not enough space for the payload: %d < %d", + (int)dstlen, (int)datalen); + return 0; + } + + memcpy(dst, buf + 4, datalen); + + upsdebug_hex(3, "bicker_receive", buf, datalen + 5); + return datalen; +} + +static ssize_t bicker_read_int16(char cmd, int16_t *dst) +{ + ssize_t ret; + + ret = bicker_send(cmd, NULL); + if (ret <= 0) { + return ret; + } + + /* XXX: by default, data is stored in little-endian so, + * on big-endian platforms, a byte swap should be needed */ + return bicker_receive(cmd, dst, 2); +} + +void upsdrv_initinfo(void) +{ + dstate_setinfo("device.type", "ups"); + dstate_setinfo("device.mfr", "Bicker Elektronik GmbH"); + /* No way to detect the UPS model within the protocol but the + * serial communication is provided by the PSZ-1063 extension + * module, so it seems correct to put that into the model */ + dstate_setinfo("device.model", "PSZ-1063"); +} + +void upsdrv_updateinfo(void) +{ + int16_t data, charge_current; + ssize_t ret; + + ret = bicker_read_int16('\x25', &data); + if (ret <= 0) { + dstate_datastale(); + return; + } + dstate_setinfo("input.voltage", "%.1f", (double) data / 1000); + + ret = bicker_read_int16('\x28', &data); + if (ret <= 0) { + dstate_datastale(); + return; + } + dstate_setinfo("input.current", "%.3f", (double) data / 1000); + + ret = bicker_read_int16('\x27', &data); + if (ret <= 0) { + dstate_datastale(); + return; + } + dstate_setinfo("output.voltage", "%.3f", (double) data / 1000); + + /* This is a supercap UPS so, in this context, + * the "battery" is the supercap stack */ + ret = bicker_read_int16('\x26', &data); + if (ret <= 0) { + dstate_datastale(); + return; + } + dstate_setinfo("battery.voltage", "%.3f", (double) data / 1000); + + ret = bicker_read_int16('\x29', &charge_current); + if (ret <= 0) { + dstate_datastale(); + return; + } + dstate_setinfo("battery.current", "%.3f", (double) charge_current / 1000); + + /* GetChargeStaturRegister returns a 16 bit register: + * + * 0. SD Shows that the device is in step-down (charging) mode. + * 1. SU Shows that the device is in step-up (backup) mode. + * 2. CV Shows that the charger is in constant voltage mode. + * 3. UV Shows that the charger is in undervoltage lockout. + * 4. CL Shows that the device is in input current limit. + * 5. CG Shows that the capacitor voltage is above power good threshold. + * 6. CS Shows that the capacitor manager is shunting. + * 7. CB Shows that the capacitor manager is balancing. + * 8. CD Shows that the charger is temporarily disabled for capacitance measurement. + * 9. CC Shows that the charger is in constant current mode. + * 10. RV Reserved Bit + * 11. PF Shows that the input voltage is below the Power Fail Input (PFI) threshold. + * 12. RV Reserved Bit + * 13. RV Reserved Bit + * 14. RV Reserved Bit + * 15. RV Reserved Bit + */ + ret = bicker_read_int16('\x1B', &data); + if (ret <= 0) { + dstate_datastale(); + return; + } + + status_init(); + + /* Check PF (bit 11) to know if the UPS is on line/battery */ + status_set((data & 0x0800) > 0 ? "OB" : "OL"); + + /* Check CG (bit 5) to know if the battery is low */ + if ((data & 0x0020) == 0) { + status_set("LB"); + } + + /* If there is a current of more than 1 A flowing towards + * the supercaps, consider the battery in charging mode */ + if (charge_current > 1000) { + status_set("CHRG"); + } + + status_commit(); + + dstate_dataok(); +} + +void upsdrv_shutdown(void) +{ + upslogx(LOG_ERR, "shutdown not supported"); + set_exit_flag(-1); +} + +void upsdrv_help(void) +{ +} + +void upsdrv_makevartable(void) +{ +} + +void upsdrv_initups(void) +{ + upsfd = ser_open(device_path); + ser_set_speed(upsfd, device_path, B38400); +} + +void upsdrv_cleanup(void) +{ + ser_close(upsfd, device_path); +} From 021472d53055a3e0ebd35c470a84f1c7d2b06548 Mon Sep 17 00:00:00 2001 From: Nicola Fontana Date: Fri, 17 May 2024 13:26:25 +0200 Subject: [PATCH 02/23] bicker_ser: add shutdown support The protocol supports shutdown after a custom delay settable between 0 and 255 seconds. Unfortunately the real device (UPSIC-2403D) seems to always enforce a 2 seconds delay. Signed-off-by: Nicola Fontana --- drivers/bicker_ser.c | 85 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 79 insertions(+), 6 deletions(-) diff --git a/drivers/bicker_ser.c b/drivers/bicker_ser.c index 5be95bdc79..1d46ca6cb8 100644 --- a/drivers/bicker_ser.c +++ b/drivers/bicker_ser.c @@ -53,6 +53,8 @@ #define BICKER_PACKET (1+1+1+1+252+1) #define BICKER_SOH '\x01' #define BICKER_EOT '\x04' +#define BICKER_DELAY 20 +#define BICKER_RETRIES 3 upsdrv_info_t upsdrv_info = { DRIVER_NAME, @@ -62,16 +64,15 @@ upsdrv_info_t upsdrv_info = { { NULL } }; -static ssize_t bicker_send(char cmd, const char *data) +static ssize_t bicker_send(char cmd, const void *data, size_t datalen) { char buf[BICKER_PACKET]; - size_t datalen, buflen; + size_t buflen; ssize_t ret; ser_flush_io(upsfd); if (data != NULL) { - datalen = strlen(data); if (datalen > 252) { upslogx(LOG_ERR, "Data size exceeded: %d > 252", (int)datalen); @@ -162,7 +163,9 @@ static ssize_t bicker_receive(char cmd, void *dst, size_t dstlen) return 0; } - memcpy(dst, buf + 4, datalen); + if (dst != NULL) { + memcpy(dst, buf + 4, datalen); + } upsdebug_hex(3, "bicker_receive", buf, datalen + 5); return datalen; @@ -172,7 +175,7 @@ static ssize_t bicker_read_int16(char cmd, int16_t *dst) { ssize_t ret; - ret = bicker_send(cmd, NULL); + ret = bicker_send(cmd, NULL, 0); if (ret <= 0) { return ret; } @@ -182,6 +185,60 @@ static ssize_t bicker_read_int16(char cmd, int16_t *dst) return bicker_receive(cmd, dst, 2); } +/* For some reason the `seconds` delay (at least on my UPSIC-2403D) + * is not honored: the shutdown is always delayed by 2 seconds. This + * fixed delay seems to be independent from the state of the UPS (on + * line or on battery) and from the dip switches setting. + * + * As response I get the same command with `0xE0` in the data field. + */ +static ssize_t bicker_delayed_shutdown(uint8_t seconds) +{ + ssize_t ret; + uint8_t response; + + ret = bicker_send('\x32', &seconds, 1); + if (ret <= 0) { + return ret; + } + + ret = bicker_receive('\x32', &response, 1); + if (ret > 0) { + upslogx(LOG_INFO, "Shutting down in %d seconds: response = 0x%02X", + (int)seconds, (unsigned)response); + } + + return ret; +} + +static ssize_t bicker_shutdown(void) +{ + const char *str; + int delay; + + str = dstate_getinfo("ups.delay.shutdown"); + delay = atoi(str); + if (delay > 255) { + upslogx(LOG_WARNING, "Shutdown delay too big: %d > 255", + delay); + delay = 255; + } + + return bicker_delayed_shutdown(delay); +} + +static int bicker_instcmd(const char *cmdname, const char *extra) +{ + NUT_UNUSED_VARIABLE(extra); + + if (!strcasecmp(cmdname, "shutdown.return")) { + bicker_shutdown(); + } + + upslogx(LOG_NOTICE, "instcmd: unknown command [%s]", cmdname); + return STAT_INSTCMD_UNKNOWN; +} + void upsdrv_initinfo(void) { dstate_setinfo("device.type", "ups"); @@ -190,6 +247,8 @@ void upsdrv_initinfo(void) * serial communication is provided by the PSZ-1063 extension * module, so it seems correct to put that into the model */ dstate_setinfo("device.model", "PSZ-1063"); + + upsh.instcmd = bicker_instcmd; } void upsdrv_updateinfo(void) @@ -282,7 +341,16 @@ void upsdrv_updateinfo(void) void upsdrv_shutdown(void) { - upslogx(LOG_ERR, "shutdown not supported"); + int retry; + + for (retry = 1; retry <= BICKER_RETRIES; retry++) { + if (bicker_shutdown() > 0) { + set_exit_flag(-2); /* EXIT_SUCCESS */ + return; + } + } + + upslogx(LOG_ERR, "Shutdown failed!"); set_exit_flag(-1); } @@ -298,6 +366,11 @@ void upsdrv_initups(void) { upsfd = ser_open(device_path); ser_set_speed(upsfd, device_path, B38400); + + /* Adding this variable here because upsdrv_initinfo() is not + * called when triggering a forced shutdown and + * "ups.delay.shutdown" is needed right there */ + dstate_setinfo("ups.delay.shutdown", "%u", BICKER_DELAY); } void upsdrv_cleanup(void) From 3ae6b50f74495087a31e346beffc81aedfc32c53 Mon Sep 17 00:00:00 2001 From: Nicola Fontana Date: Fri, 17 May 2024 14:05:43 +0200 Subject: [PATCH 03/23] data/driver.list.in: add bicker_ser based UPSes Signed-off-by: Nicola Fontana --- data/driver.list.in | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/data/driver.list.in b/data/driver.list.in index 7543a1fc02..2480f23247 100644 --- a/data/driver.list.in +++ b/data/driver.list.in @@ -196,6 +196,11 @@ "Best Power" "ups" "1" "Micro-Ferrups" "" "bestuferrups" "Best Power" "ups" "1" "Fortress/Ferrups" "f-command support" "bestfcom" +"Bicker" "ups" "3" "UPSIC-1205" "" "bicker_ser" +"Bicker" "ups" "3" "UPSIC-2403" "" "bicker_ser" +"Bicker" "ups" "3" "DC2412-UPS" "" "bicker_ser" +"Bicker" "ups" "3" "DC2412-UPS-LD" "" "bicker_ser" + "Borri" "ups" "2" "B400-010-B/B400-020-B/B400-030-B/B400-010-C/B400-020-C/B400-030-C" "USB" "blazer_usb" "Borri" "ups" "2" "B400-R010-B/B400-R020-B/B400-R030-B/B400-R010-C/B400-R020-C/B400-R030-C" "USB" "blazer_usb" "Borri" "ups" "2" "B500-060-B/B500-100-B/B500-060-C/B500-100-C" "USB" "blazer_usb" From 0b53475254fb216d1643de0d7e1c5a571b018132 Mon Sep 17 00:00:00 2001 From: Nicola Fontana Date: Fri, 17 May 2024 20:26:17 +0200 Subject: [PATCH 04/23] bicker_ser: add big-endian platform support Signed-off-by: Nicola Fontana --- drivers/bicker_ser.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/drivers/bicker_ser.c b/drivers/bicker_ser.c index 1d46ca6cb8..c6828c27c8 100644 --- a/drivers/bicker_ser.c +++ b/drivers/bicker_ser.c @@ -55,6 +55,7 @@ #define BICKER_EOT '\x04' #define BICKER_DELAY 20 #define BICKER_RETRIES 3 +#define BYTESWAP(in) ((((uint16_t)(in) & 0x00FF) << 8) + (((uint16_t)(in) & 0xFF00) >> 8)) upsdrv_info_t upsdrv_info = { DRIVER_NAME, @@ -180,9 +181,18 @@ static ssize_t bicker_read_int16(char cmd, int16_t *dst) return ret; } - /* XXX: by default, data is stored in little-endian so, - * on big-endian platforms, a byte swap should be needed */ - return bicker_receive(cmd, dst, 2); + ret = bicker_receive(cmd, dst, 2); + if (ret <= 0) { + return ret; + } + +#ifdef WORDS_BIGENDIAN + /* By default data is stored in little-endian so, on big-endian + * platforms, a byte swap must be performed */ + *dst = BYTESWAP(*dst); +#endif + + return ret; } /* For some reason the `seconds` delay (at least on my UPSIC-2403D) From 6538f2f4ce89654dfb78d5fdb2a73d50728adb44 Mon Sep 17 00:00:00 2001 From: Nicola Fontana Date: Fri, 17 May 2024 20:46:01 +0200 Subject: [PATCH 05/23] bicker_ser: use length macros to increase expressiveness Signed-off-by: Nicola Fontana --- drivers/bicker_ser.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/drivers/bicker_ser.c b/drivers/bicker_ser.c index c6828c27c8..dadd9f1d27 100644 --- a/drivers/bicker_ser.c +++ b/drivers/bicker_ser.c @@ -27,10 +27,11 @@ * +-------+-------+-------+-------+--- - - - ---+-------+ * | SOH | Size | Index | CMD | Data | EOT | * +-------+-------+-------+-------+--- - - - ---+-------+ + * | HEADER | * * where: * - `SOH` is the start signal ('\x01') - * - `Size` is the length of the `Data` field (in bytes) plus 3 + * - `Size` is the length (in bytes) of the header and the data field * - `Index` is the command index (always '\x03') * - `CMD` is the command code to execute * - `Data` is the (optional) argument of the command @@ -50,13 +51,17 @@ #define DRIVER_NAME "Bicker serial protocol" #define DRIVER_VERSION "0.01" -#define BICKER_PACKET (1+1+1+1+252+1) #define BICKER_SOH '\x01' #define BICKER_EOT '\x04' #define BICKER_DELAY 20 #define BICKER_RETRIES 3 #define BYTESWAP(in) ((((uint16_t)(in) & 0x00FF) << 8) + (((uint16_t)(in) & 0xFF00) >> 8)) +/* Protocol fixed lengths */ +#define BICKER_HEADER 3 +#define BICKER_MAXDATA (255 - BICKER_HEADER) +#define BICKER_PACKET (1 + BICKER_HEADER + BICKER_MAXDATA + 1) + upsdrv_info_t upsdrv_info = { DRIVER_NAME, DRIVER_VERSION, @@ -74,9 +79,9 @@ static ssize_t bicker_send(char cmd, const void *data, size_t datalen) ser_flush_io(upsfd); if (data != NULL) { - if (datalen > 252) { - upslogx(LOG_ERR, "Data size exceeded: %d > 252", - (int)datalen); + if (datalen > BICKER_MAXDATA) { + upslogx(LOG_ERR, "Data size exceeded: %d > %d", + (int)datalen, BICKER_MAXDATA); return 0; } memcpy(buf + 4, data, datalen); @@ -85,13 +90,11 @@ static ssize_t bicker_send(char cmd, const void *data, size_t datalen) } buf[0] = BICKER_SOH; - buf[1] = datalen + 3; /* Packet size must include the header */ + buf[1] = datalen + BICKER_HEADER; buf[2] = '\x03'; /* Command index is always 3 */ buf[3] = cmd; - buf[4 + datalen] = BICKER_EOT; - - /* The full packet includes SOH and EOT bytes too */ - buflen = datalen + 3 + 2; + buf[1 + BICKER_HEADER + datalen] = BICKER_EOT; + buflen = 1 + BICKER_HEADER + datalen + 1; ret = ser_send_buf(upsfd, buf, buflen); if (ret < 0) { @@ -145,8 +148,10 @@ static ssize_t bicker_receive(char cmd, void *dst, size_t dstlen) return ret; } - datalen = buf[1] - 3; + datalen = buf[1] - BICKER_HEADER; + /* Still to be read: command index (1 byte), command (1 byte), data + * (datalen bytes) and EOT (1 byte), i.e. `datalen + 3` bytes */ ret = bicker_receive_buf(buf + 2, datalen + 3); if (ret <= 0) { return ret; From 67ffe25b363d651c4b41b311a0767e987a8207b1 Mon Sep 17 00:00:00 2001 From: Nicola Fontana Date: Fri, 17 May 2024 21:47:43 +0200 Subject: [PATCH 06/23] bicker_ser: add manpage Signed-off-by: Nicola Fontana --- docs/man/Makefile.am | 3 ++ docs/man/bicker_ser.txt | 66 +++++++++++++++++++++++++++++++++++++++++ docs/nut.dict | 5 +++- 3 files changed, 73 insertions(+), 1 deletion(-) create mode 100644 docs/man/bicker_ser.txt diff --git a/docs/man/Makefile.am b/docs/man/Makefile.am index 921cc1f9d3..52669c1cd4 100644 --- a/docs/man/Makefile.am +++ b/docs/man/Makefile.am @@ -430,6 +430,7 @@ SRC_SERIAL_PAGES = \ bestuferrups.txt \ bestups.txt \ bestfcom.txt \ + bicker_ser.txt \ blazer-common.txt \ blazer_ser.txt \ clone.txt \ @@ -477,6 +478,7 @@ MAN_SERIAL_PAGES = \ bestuferrups.8 \ bestups.8 \ bestfcom.8 \ + bicker_ser.8 \ blazer_ser.8 \ clone.8 \ dummy-ups.8 \ @@ -526,6 +528,7 @@ HTML_SERIAL_MANS = \ bestuferrups.html \ bestups.html \ bestfcom.html \ + bicker_ser.html \ blazer_ser.html \ clone.html \ dummy-ups.html \ diff --git a/docs/man/bicker_ser.txt b/docs/man/bicker_ser.txt new file mode 100644 index 0000000000..f5122cf819 --- /dev/null +++ b/docs/man/bicker_ser.txt @@ -0,0 +1,66 @@ +BICKER_SER(8) +============= + +NAME +---- + +bicker_ser - Driver for Bicker DC UPS via serial port connections + +SYNOPSIS +-------- + +*bicker_ser* -h + +*bicker_ser* -a 'UPS_NAME' ['OPTIONS'] + +NOTE: This man page only documents the hardware-specific features of the +*bicker_ser* driver. For information about the core driver, see +linkman:nutupsdrv[8]. + +SUPPORTED HARDWARE +------------------ + +*bicker_ser* supports all Bicker UPSes shipped with the PSZ-1053 extension +module such as UPSIC-1205, UPSIC-2403, DC2412-UPS and DC2412-UPS-LD. + +CABLING +------- + +The needed cable is a standard pin-to-pin serial cable with pins 2, 3 and 5 +(on DB9 connector) connected. + +EXTRA ARGUMENTS +--------------- + +This driver supports no extra arguments from linkman:ups.conf[5]. + +INSTANT COMMANDS +---------------- + +*shutdown.return*:: +Turn off the load and return when power is back. + +KNOWN ISSUES AND BUGS +--------------------- + +*ups.delay.shutdown is not honored*:: +Although that delay is properly set when sending the shutdown command, it seems +some UPS ignore it and use a fixed 2 seconds delay instead. + +AUTHOR +------ + +Nicola Fontana + +SEE ALSO +-------- + +The core driver: +~~~~~~~~~~~~~~~~ + +linkman:nutupsdrv[8] + +Internet resources: +~~~~~~~~~~~~~~~~~~~ + +The NUT (Network UPS Tools) home page: https://www.networkupstools.org/ diff --git a/docs/nut.dict b/docs/nut.dict index 7f99a82718..b0ad18af66 100644 --- a/docs/nut.dict +++ b/docs/nut.dict @@ -1,4 +1,4 @@ -personal_ws-1.1 en 3149 utf-8 +personal_ws-1.1 en 3151 utf-8 AAC AAS ABI @@ -373,6 +373,7 @@ Fideltronik Filipozzi Fiskars FlossMetrics +Fontana Forza Fosshost Frama @@ -858,6 +859,7 @@ PSSENTR PSUs PSW PSX +PSZ PThreads PULS PV @@ -1245,6 +1247,7 @@ UPS's UPSCONN UPSDESC UPSHOST +UPSIC UPSIMAGEPATH UPSLC UPSNOTIFY From ffd42d16024f230aa1e5b7891c9884762c842f84 Mon Sep 17 00:00:00 2001 From: Nicola Fontana Date: Sat, 18 May 2024 09:07:38 +0200 Subject: [PATCH 07/23] NEWS.adoc: announce bicker_ser driver [#2318] Signed-off-by: Nicola Fontana --- NEWS.adoc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/NEWS.adoc b/NEWS.adoc index 0313d740df..ee34f0eb3e 100644 --- a/NEWS.adoc +++ b/NEWS.adoc @@ -146,6 +146,10 @@ https://github.com/networkupstools/nut/milestone/11 respective warnings issued by the new generations of analysis tools. [#823, #2437, link:https://github.com/networkupstools/nut-website/issues/52[nut-website issue #52]] + - 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 + extension module. [PR #2448] + Release notes for NUT 2.8.2 - what's new since 2.8.1 ---------------------------------------------------- From d5e7ce5c5198a7e64fd808cde63df30c4c187716 Mon Sep 17 00:00:00 2001 From: Nicola Fontana Date: Tue, 21 May 2024 11:13:40 +0200 Subject: [PATCH 08/23] bicker_ser: update protocol documentation Bicker told me to refer to the UPS Gen software's user manual for more details on their protocol, and in fact that manual contains much more info. Added a summary of available commands to the comments, as a quick reference for feature implementation. Signed-off-by: Nicola Fontana --- drivers/bicker_ser.c | 72 +++++++++++++++++++++++++++++++++++++++----- 1 file changed, 65 insertions(+), 7 deletions(-) diff --git a/drivers/bicker_ser.c b/drivers/bicker_ser.c index dadd9f1d27..00d49163ba 100644 --- a/drivers/bicker_ser.c +++ b/drivers/bicker_ser.c @@ -18,8 +18,10 @@ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */ -/* The protocol is detailed in section 14 of the DC/UPSIC user's manual, - * downloadable somewhere from the Bicker website. +/* The protocol is reported in many Bicker's manuals but (according to + * Bicker itself) the best source is the UPS Gen Software's user manual: + * + * https://www.bicker.de/media/pdf/ff/cc/fe/en_user_manual_ups-gen2-configuration-software.pdf * * Basically, this is a binary protocol without checksums: * @@ -32,14 +34,70 @@ * where: * - `SOH` is the start signal ('\x01') * - `Size` is the length (in bytes) of the header and the data field - * - `Index` is the command index (always '\x03') - * - `CMD` is the command code to execute + * - `Index` is the command index: see AVAILABLE COMMANDS + * - `CMD` is the command code to execute: see AVAILABLE COMMANDS * - `Data` is the (optional) argument of the command * - `EOT` is the end signal ('\x04') * - * The same format is used for incoming and outcoming packets. Although - * not explicitly documented, the data returned by the `Data` field - * seems to be in little-endian order. + * The same format is used for incoming and outcoming packets. The data + * returned in the `Data` field is always in little-endian order. + * + * AVAILABLE COMMANDS + * ------------------ + * + * - Index = 0x01 (GENERIC) + * - CMD = 0x40 (status flags) + * - CMD = 0x41 (input voltage) + * - CMD = 0x42 (input current) + * - CMD = 0x43 (output voltage) + * - CMD = 0x44 (output current) + * - CMD = 0x45 (battery voltage) + * - CMD = 0x46 (battery current) + * - CMD = 0x47 (battery state of charge) + * - CMD = 0x48 (battery state of health) + * - CMD = 0x49 (battery cycles) + * - CMD = 0x4A (battery temperature) + * - CMD = 0x60 (manufacturer) + * - CMD = 0x61 (serial number) + * - CMD = 0x62 (device name) + * - CMD = 0x63 (firmware version) + * - CMD = 0x64 (battery pack) + * - CMD = 0x65 (firmware core version) + * - CMD = 0x66 (CPU temperature) + * - CMD = 0x67 (hardware revision) + * - CMD = 0x21 (UPS output) + * - CMD = 0x2F (shutdown flag) + * - CMD = 0x7A (reset parameter settings) + * + * - Index = 0x07 (PARAMETER) + * - CMD = 0x00 (get/set dummy entry: do not use!) + * - CMD = 0x01 (get/set load sensor) + * - CMD = 0x02 (get/set maximum backup time) + * - CMD = 0x03 (get/set os shutdown by timer) + * - CMD = 0x04 (get/set restart delay timer) + * - CMD = 0x05 (get/set minimum capacity to start) + * - CMD = 0x06 (get/set maximum backup time by in-1) + * - CMD = 0x07 (get/set os shutdown by soc) + * - CMD = 0x08 (get/set battery soc low threshold) + * - CMD = 0x09 (get/set relay event configuration) + * - CMD = 0x0A (get/set RS232 port configuration: place holder!) + * + * - Index = 0x03 (COMMANDS GOT FROM UPSIC MANUAL) + * - CMD = 0x1B (GetChargeStatusRegister) + * - CMD = 0x1C (GetMonitorStatusRegister) + * - CMD = 0x1E (GetCapacity) + * - CMD = 0x1F (GetEsr) + * - CMD = 0x20 (GetVCap1Voltage) + * - CMD = 0x21 (GetVCap2Voltage) + * - CMD = 0x22 (GetVCap3Voltage) + * - CMD = 0x23 (GetVCap4Voltage) + * - CMD = 0x25 (GetInputVoltage) + * - CMD = 0x26 (GetCapStackVoltage) + * - CMD = 0x27 (GetOutputVoltage) + * - CMD = 0x28 (GetInputCurrent) + * - CMD = 0x29 (GetChargeCurrent) + * - CMD = 0x31 (StartCapEsrMeasurement) + * - CMD = 0x32 (SetTimeToShutdown) */ #include "config.h" From 162e39ee316018f3b495e476ca2bbdc863a035b8 Mon Sep 17 00:00:00 2001 From: Nicola Fontana Date: Tue, 21 May 2024 14:18:24 +0200 Subject: [PATCH 09/23] bicker_ser: explicitly pass the command index In the previous implementation the command index was always '\x03', as stated by the UPSIC manual. After new evidence has been found (UPS-Gen2 software user manual), it came out there are much more commands grouped in different indexes. Signed-off-by: Nicola Fontana --- drivers/bicker_ser.c | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/drivers/bicker_ser.c b/drivers/bicker_ser.c index 00d49163ba..c8b09bf18d 100644 --- a/drivers/bicker_ser.c +++ b/drivers/bicker_ser.c @@ -128,7 +128,7 @@ upsdrv_info_t upsdrv_info = { { NULL } }; -static ssize_t bicker_send(char cmd, const void *data, size_t datalen) +static ssize_t bicker_send(char idx, char cmd, const void *data, size_t datalen) { char buf[BICKER_PACKET]; size_t buflen; @@ -149,7 +149,7 @@ static ssize_t bicker_send(char cmd, const void *data, size_t datalen) buf[0] = BICKER_SOH; buf[1] = datalen + BICKER_HEADER; - buf[2] = '\x03'; /* Command index is always 3 */ + buf[2] = idx; buf[3] = cmd; buf[1 + BICKER_HEADER + datalen] = BICKER_EOT; buflen = 1 + BICKER_HEADER + datalen + 1; @@ -186,7 +186,7 @@ static ssize_t bicker_receive_buf(void *dst, size_t dstlen) return ret; } -static ssize_t bicker_receive(char cmd, void *dst, size_t dstlen) +static ssize_t bicker_receive(char idx, char cmd, void *dst, size_t dstlen) { ssize_t ret; size_t datalen; @@ -217,9 +217,13 @@ static ssize_t bicker_receive(char cmd, void *dst, size_t dstlen) upslogx(LOG_WARNING, "Received 0x%02X instead of EOT (0x%02X)", (unsigned)buf[datalen + 4], (unsigned)BICKER_EOT); return 0; + } else if (buf[2] != idx) { + upslogx(LOG_WARNING, "Command indexes do not match: sent 0x%02X, received 0x%02X", + (unsigned)idx, (unsigned)buf[2]); + return 0; } else if (buf[3] != cmd) { upslogx(LOG_WARNING, "Commands do not match: sent 0x%02X, received 0x%02X", - (unsigned)cmd, (unsigned)buf[1]); + (unsigned)cmd, (unsigned)buf[3]); return 0; } else if (dstlen < datalen) { upslogx(LOG_ERR, "Not enough space for the payload: %d < %d", @@ -235,16 +239,16 @@ static ssize_t bicker_receive(char cmd, void *dst, size_t dstlen) return datalen; } -static ssize_t bicker_read_int16(char cmd, int16_t *dst) +static ssize_t bicker_read_int16(char idx, char cmd, int16_t *dst) { ssize_t ret; - ret = bicker_send(cmd, NULL, 0); + ret = bicker_send(idx, cmd, NULL, 0); if (ret <= 0) { return ret; } - ret = bicker_receive(cmd, dst, 2); + ret = bicker_receive(idx, cmd, dst, 2); if (ret <= 0) { return ret; } @@ -270,12 +274,12 @@ static ssize_t bicker_delayed_shutdown(uint8_t seconds) ssize_t ret; uint8_t response; - ret = bicker_send('\x32', &seconds, 1); + ret = bicker_send('\x03', '\x32', &seconds, 1); if (ret <= 0) { return ret; } - ret = bicker_receive('\x32', &response, 1); + ret = bicker_receive('\x03', '\x32', &response, 1); if (ret > 0) { upslogx(LOG_INFO, "Shutting down in %d seconds: response = 0x%02X", (int)seconds, (unsigned)response); @@ -329,21 +333,21 @@ void upsdrv_updateinfo(void) int16_t data, charge_current; ssize_t ret; - ret = bicker_read_int16('\x25', &data); + ret = bicker_read_int16('\x03', '\x25', &data); if (ret <= 0) { dstate_datastale(); return; } dstate_setinfo("input.voltage", "%.1f", (double) data / 1000); - ret = bicker_read_int16('\x28', &data); + ret = bicker_read_int16('\x03', '\x28', &data); if (ret <= 0) { dstate_datastale(); return; } dstate_setinfo("input.current", "%.3f", (double) data / 1000); - ret = bicker_read_int16('\x27', &data); + ret = bicker_read_int16('\x03', '\x27', &data); if (ret <= 0) { dstate_datastale(); return; @@ -352,14 +356,14 @@ void upsdrv_updateinfo(void) /* This is a supercap UPS so, in this context, * the "battery" is the supercap stack */ - ret = bicker_read_int16('\x26', &data); + ret = bicker_read_int16('\x03', '\x26', &data); if (ret <= 0) { dstate_datastale(); return; } dstate_setinfo("battery.voltage", "%.3f", (double) data / 1000); - ret = bicker_read_int16('\x29', &charge_current); + ret = bicker_read_int16('\x03', '\x29', &charge_current); if (ret <= 0) { dstate_datastale(); return; @@ -385,7 +389,7 @@ void upsdrv_updateinfo(void) * 14. RV Reserved Bit * 15. RV Reserved Bit */ - ret = bicker_read_int16('\x1B', &data); + ret = bicker_read_int16('\x03', '\x1B', &data); if (ret <= 0) { dstate_datastale(); return; From 8abde1a7cbe93c32ef2713c70fba6af6d67993d2 Mon Sep 17 00:00:00 2001 From: Nicola Fontana Date: Tue, 21 May 2024 15:54:24 +0200 Subject: [PATCH 10/23] bicker_ser: refactor to allow more complex packet handling I'm planning to add functions for reading strings, i.e. without knowing beforehand the size of the response packets. This required some refactoring of the receiving functions. Also, while at it, added docblocks to many Bicker functions, hopefully easing the maintenance in the future. Signed-off-by: Nicola Fontana --- drivers/bicker_ser.c | 160 +++++++++++++++++++++++++++---------------- 1 file changed, 102 insertions(+), 58 deletions(-) diff --git a/drivers/bicker_ser.c b/drivers/bicker_ser.c index c8b09bf18d..2ca288a23e 100644 --- a/drivers/bicker_ser.c +++ b/drivers/bicker_ser.c @@ -111,6 +111,7 @@ #define BICKER_SOH '\x01' #define BICKER_EOT '\x04' +#define BICKER_TIMEOUT 1 #define BICKER_DELAY 20 #define BICKER_RETRIES 3 #define BYTESWAP(in) ((((uint16_t)(in) & 0x00FF) << 8) + (((uint16_t)(in) & 0xFF00) >> 8)) @@ -128,6 +129,14 @@ upsdrv_info_t upsdrv_info = { { NULL } }; +/** + * Send a packet. + * @param idx Command index + * @param cmd Command + * @param data Source data or NULL for no data field + * @param datalen Size of the source data field or 0 + * @return `datalen` on success or -1 on errors. + */ static ssize_t bicker_send(char idx, char cmd, const void *data, size_t datalen) { char buf[BICKER_PACKET]; @@ -140,7 +149,7 @@ static ssize_t bicker_send(char idx, char cmd, const void *data, size_t datalen) if (datalen > BICKER_MAXDATA) { upslogx(LOG_ERR, "Data size exceeded: %d > %d", (int)datalen, BICKER_MAXDATA); - return 0; + return -1; } memcpy(buf + 4, data, datalen); } else { @@ -157,99 +166,134 @@ static ssize_t bicker_send(char idx, char cmd, const void *data, size_t datalen) ret = ser_send_buf(upsfd, buf, buflen); if (ret < 0) { upslog_with_errno(LOG_WARNING, "ser_send_buf failed"); - return ret; + return -1; } else if ((size_t) ret != buflen) { upslogx(LOG_WARNING, "ser_send_buf returned %d instead of %d", (int)ret, (int)buflen); - return 0; + return -1; } upsdebug_hex(3, "bicker_send", buf, buflen); - return ret; -} - -static ssize_t bicker_receive_buf(void *dst, size_t dstlen) -{ - ssize_t ret; - - ret = ser_get_buf(upsfd, dst, dstlen, 1, 1); - if (ret < 0) { - upslog_with_errno(LOG_WARNING, "ser_get_buf failed"); - return ret; - } else if ((size_t) ret != dstlen) { - upslogx(LOG_WARNING, "ser_get_buf returned %d instead of %d", - (int)ret, (int)dstlen); - return 0; - } - - upsdebug_hex(3, "bicker_receive_buf", dst, dstlen); - return ret; + return datalen; } -static ssize_t bicker_receive(char idx, char cmd, void *dst, size_t dstlen) +/** + * Receive a packet with a data field of unknown size. + * @param idx Command index + * @param cmd Command + * @param data Destination buffer or NULL to discard the data field + * @return The size of the data field on success or -1 on errors. + * + * The data field is stored directly in the destination buffer. `data`, + * if not NULL, must have at least BICKER_MAXDATA bytes. + */ +static ssize_t bicker_receive(char idx, char cmd, void *data) { ssize_t ret; size_t datalen; char buf[BICKER_PACKET]; - ret = bicker_receive_buf(buf, 1); - if (ret <= 0) { - return ret; + /* Read first two bytes (SOH + size) */ + ret = ser_get_buf(upsfd, buf, 2, BICKER_TIMEOUT, 0); + if (ret < 0) { + upslog_with_errno(LOG_WARNING, "Initial ser_get_buf failed"); + return -1; + } else if (ret < 2) { + upslogx(LOG_WARNING, "Timeout waiting for response packet"); + return -1; } else if (buf[0] != BICKER_SOH) { upslogx(LOG_WARNING, "Received 0x%02X instead of SOH (0x%02X)", (unsigned)buf[0], (unsigned)BICKER_SOH); - return 0; - } - - ret = bicker_receive_buf(buf + 1, 1); - if (ret <= 0) { - return ret; + return -1; } datalen = buf[1] - BICKER_HEADER; - /* Still to be read: command index (1 byte), command (1 byte), data - * (datalen bytes) and EOT (1 byte), i.e. `datalen + 3` bytes */ - ret = bicker_receive_buf(buf + 2, datalen + 3); - if (ret <= 0) { - return ret; + /* Read the rest: command index (1 byte), command (1 byte), data + * (datalen bytes) and EOT (1 byte), i.e. `datalen + 3` bytes */ + ret = ser_get_buf(upsfd, buf + 2, datalen + 3, BICKER_TIMEOUT, 0); + if (ret < 0) { + upslog_with_errno(LOG_WARNING, "ser_get_buf failed"); + return -1; + } else if ((size_t)ret < datalen + 3) { + upslogx(LOG_WARNING, "Timeout waiting for the end of the packet"); + return -1; } else if (buf[datalen + 4] != BICKER_EOT) { upslogx(LOG_WARNING, "Received 0x%02X instead of EOT (0x%02X)", (unsigned)buf[datalen + 4], (unsigned)BICKER_EOT); - return 0; + return -1; } else if (buf[2] != idx) { upslogx(LOG_WARNING, "Command indexes do not match: sent 0x%02X, received 0x%02X", (unsigned)idx, (unsigned)buf[2]); - return 0; + return -1; } else if (buf[3] != cmd) { upslogx(LOG_WARNING, "Commands do not match: sent 0x%02X, received 0x%02X", (unsigned)cmd, (unsigned)buf[3]); - return 0; - } else if (dstlen < datalen) { - upslogx(LOG_ERR, "Not enough space for the payload: %d < %d", - (int)dstlen, (int)datalen); - return 0; + return -1; } - if (dst != NULL) { - memcpy(dst, buf + 4, datalen); + if (data != NULL) { + memcpy(data, buf + 4, datalen); } upsdebug_hex(3, "bicker_receive", buf, datalen + 5); return datalen; } +/** + * Receive a packet with a data field of known size. + * @param idx Command index + * @param cmd Command + * @param data Destination buffer or NULL to discard the data field + * @param datalen The expected size of the data field + * @return The size of the data field on success or -1 on errors. + * + * `data`, if specified, must have at least `datalen` bytes. If + * `datalen` is less than the received data size, an error is thrown. + */ +static ssize_t bicker_receive_known(char idx, char cmd, void *data, size_t datalen) +{ + ssize_t ret; + size_t real_datalen; + char real_data[BICKER_MAXDATA]; + + ret = bicker_receive(idx, cmd, real_data); + if (ret < 0) { + return ret; + } + + real_datalen = (size_t)ret; + + if (datalen < real_datalen) { + upslogx(LOG_ERR, "Not enough space for the payload: %d < %d", + (int)datalen, (int)real_datalen); + return -1; + } + + if (data != NULL) { + memcpy(data, real_data, real_datalen); + } + + return real_datalen; +} + +/** + * Execute a command that returns an int16_t value. + * @param idx Command index + * @param cmd Command + * @param dst Destination for the value + */ static ssize_t bicker_read_int16(char idx, char cmd, int16_t *dst) { ssize_t ret; ret = bicker_send(idx, cmd, NULL, 0); - if (ret <= 0) { + if (ret < 0) { return ret; } - ret = bicker_receive(idx, cmd, dst, 2); - if (ret <= 0) { + ret = bicker_receive_known(idx, cmd, dst, 2); + if (ret < 0) { return ret; } @@ -275,12 +319,12 @@ static ssize_t bicker_delayed_shutdown(uint8_t seconds) uint8_t response; ret = bicker_send('\x03', '\x32', &seconds, 1); - if (ret <= 0) { + if (ret < 0) { return ret; } - ret = bicker_receive('\x03', '\x32', &response, 1); - if (ret > 0) { + ret = bicker_receive_known('\x03', '\x32', &response, 1); + if (ret >= 0) { upslogx(LOG_INFO, "Shutting down in %d seconds: response = 0x%02X", (int)seconds, (unsigned)response); } @@ -334,21 +378,21 @@ void upsdrv_updateinfo(void) ssize_t ret; ret = bicker_read_int16('\x03', '\x25', &data); - if (ret <= 0) { + if (ret < 0) { dstate_datastale(); return; } dstate_setinfo("input.voltage", "%.1f", (double) data / 1000); ret = bicker_read_int16('\x03', '\x28', &data); - if (ret <= 0) { + if (ret < 0) { dstate_datastale(); return; } dstate_setinfo("input.current", "%.3f", (double) data / 1000); ret = bicker_read_int16('\x03', '\x27', &data); - if (ret <= 0) { + if (ret < 0) { dstate_datastale(); return; } @@ -357,14 +401,14 @@ void upsdrv_updateinfo(void) /* This is a supercap UPS so, in this context, * the "battery" is the supercap stack */ ret = bicker_read_int16('\x03', '\x26', &data); - if (ret <= 0) { + if (ret < 0) { dstate_datastale(); return; } dstate_setinfo("battery.voltage", "%.3f", (double) data / 1000); ret = bicker_read_int16('\x03', '\x29', &charge_current); - if (ret <= 0) { + if (ret < 0) { dstate_datastale(); return; } @@ -390,7 +434,7 @@ void upsdrv_updateinfo(void) * 15. RV Reserved Bit */ ret = bicker_read_int16('\x03', '\x1B', &data); - if (ret <= 0) { + if (ret < 0) { dstate_datastale(); return; } From eaddb347bf3da3525ba0110d69508687106c88f9 Mon Sep 17 00:00:00 2001 From: Nicola Fontana Date: Tue, 21 May 2024 19:09:58 +0200 Subject: [PATCH 11/23] bicker_ser: add more state variables Identification fields are now queried from the device. Also "battery.charge" is now set properly. Signed-off-by: Nicola Fontana --- drivers/bicker_ser.c | 223 ++++++++++++++++++++++++++++++++----------- 1 file changed, 166 insertions(+), 57 deletions(-) diff --git a/drivers/bicker_ser.c b/drivers/bicker_ser.c index 2ca288a23e..8306102bcd 100644 --- a/drivers/bicker_ser.c +++ b/drivers/bicker_ser.c @@ -114,13 +114,15 @@ #define BICKER_TIMEOUT 1 #define BICKER_DELAY 20 #define BICKER_RETRIES 3 -#define BYTESWAP(in) ((((uint16_t)(in) & 0x00FF) << 8) + (((uint16_t)(in) & 0xFF00) >> 8)) /* Protocol fixed lengths */ #define BICKER_HEADER 3 #define BICKER_MAXDATA (255 - BICKER_HEADER) #define BICKER_PACKET (1 + BICKER_HEADER + BICKER_MAXDATA + 1) +#define TOUINT(ch) ((unsigned)(uint8_t)(ch)) +#define BYTESWAP(in) ((((uint16_t)(in) & 0x00FF) << 8) + (((uint16_t)(in) & 0xFF00) >> 8)) + upsdrv_info_t upsdrv_info = { DRIVER_NAME, DRIVER_VERSION, @@ -194,16 +196,16 @@ static ssize_t bicker_receive(char idx, char cmd, void *data) char buf[BICKER_PACKET]; /* Read first two bytes (SOH + size) */ - ret = ser_get_buf(upsfd, buf, 2, BICKER_TIMEOUT, 0); + ret = ser_get_buf_len(upsfd, buf, 2, BICKER_TIMEOUT, 0); if (ret < 0) { - upslog_with_errno(LOG_WARNING, "Initial ser_get_buf failed"); + upslog_with_errno(LOG_WARNING, "Initial ser_get_buf_len failed"); return -1; } else if (ret < 2) { upslogx(LOG_WARNING, "Timeout waiting for response packet"); return -1; } else if (buf[0] != BICKER_SOH) { upslogx(LOG_WARNING, "Received 0x%02X instead of SOH (0x%02X)", - (unsigned)buf[0], (unsigned)BICKER_SOH); + TOUINT(buf[0]), TOUINT(BICKER_SOH)); return -1; } @@ -211,24 +213,31 @@ static ssize_t bicker_receive(char idx, char cmd, void *data) /* Read the rest: command index (1 byte), command (1 byte), data * (datalen bytes) and EOT (1 byte), i.e. `datalen + 3` bytes */ - ret = ser_get_buf(upsfd, buf + 2, datalen + 3, BICKER_TIMEOUT, 0); + ret = ser_get_buf_len(upsfd, buf + 2, datalen + 3, BICKER_TIMEOUT, 0); if (ret < 0) { - upslog_with_errno(LOG_WARNING, "ser_get_buf failed"); + upslog_with_errno(LOG_WARNING, "ser_get_buf_len failed"); return -1; - } else if ((size_t)ret < datalen + 3) { + } + + upsdebug_hex(3, "bicker_receive", buf, ret + 2); + + if ((size_t)ret < datalen + 3) { upslogx(LOG_WARNING, "Timeout waiting for the end of the packet"); return -1; } else if (buf[datalen + 4] != BICKER_EOT) { upslogx(LOG_WARNING, "Received 0x%02X instead of EOT (0x%02X)", - (unsigned)buf[datalen + 4], (unsigned)BICKER_EOT); + TOUINT(buf[datalen + 4]), TOUINT(BICKER_EOT)); return -1; } else if (buf[2] != idx) { - upslogx(LOG_WARNING, "Command indexes do not match: sent 0x%02X, received 0x%02X", - (unsigned)idx, (unsigned)buf[2]); + /* This probably suggests the command is correct but not + * supported, so no logging performed here */ + upsdebugx(2, "Indexes do not match, maybe the command is not supported?" + " Sent 0x%02X, received 0x%02X", + TOUINT(idx), TOUINT(buf[2])); return -1; } else if (buf[3] != cmd) { upslogx(LOG_WARNING, "Commands do not match: sent 0x%02X, received 0x%02X", - (unsigned)cmd, (unsigned)buf[3]); + TOUINT(cmd), TOUINT(buf[3])); return -1; } @@ -236,7 +245,6 @@ static ssize_t bicker_receive(char idx, char cmd, void *data) memcpy(data, buf + 4, datalen); } - upsdebug_hex(3, "bicker_receive", buf, datalen + 5); return datalen; } @@ -277,11 +285,31 @@ static ssize_t bicker_receive_known(char idx, char cmd, void *data, size_t datal return real_datalen; } +/** + * Execute a command that returns an uint8_t value. + * @param idx Command index + * @param cmd Command + * @param dst Destination for the value + * @return The size of the data field on success or -1 on errors. + */ +static ssize_t bicker_read_uint8(char idx, char cmd, uint8_t *dst) +{ + ssize_t ret; + + ret = bicker_send(idx, cmd, NULL, 0); + if (ret < 0) { + return ret; + } + + return bicker_receive_known(idx, cmd, dst, 1); +} + /** * Execute a command that returns an int16_t value. * @param idx Command index * @param cmd Command * @param dst Destination for the value + * @return The size of the data field on success or -1 on errors. */ static ssize_t bicker_read_int16(char idx, char cmd, int16_t *dst) { @@ -306,6 +334,38 @@ static ssize_t bicker_read_int16(char idx, char cmd, int16_t *dst) return ret; } +static ssize_t bicker_read_uint16(char idx, char cmd, uint16_t *dst) +{ + return bicker_read_int16(idx, cmd, (int16_t *) dst); +} + +/** + * Execute a command that returns a string. + * @param idx Command index + * @param cmd Command + * @param dst Destination for the string + * + * `dst` must have at least BICKER_MAXDATA+1 bytes, the additional byte + * needed to accomodate the ending '\0'. + */ +static ssize_t bicker_read_string(char idx, char cmd, char *dst) +{ + ssize_t ret; + + ret = bicker_send(idx, cmd, NULL, 0); + if (ret < 0) { + return ret; + } + + ret = bicker_receive(idx, cmd, dst); + if (ret < 0) { + return ret; + } + + dst[ret] = '\0'; + return ret; +} + /* For some reason the `seconds` delay (at least on my UPSIC-2403D) * is not honored: the shutdown is always delayed by 2 seconds. This * fixed delay seems to be independent from the state of the UPS (on @@ -362,98 +422,131 @@ static int bicker_instcmd(const char *cmdname, const char *extra) void upsdrv_initinfo(void) { + char string[BICKER_MAXDATA + 1]; + dstate_setinfo("device.type", "ups"); - dstate_setinfo("device.mfr", "Bicker Elektronik GmbH"); - /* No way to detect the UPS model within the protocol but the - * serial communication is provided by the PSZ-1063 extension - * module, so it seems correct to put that into the model */ - dstate_setinfo("device.model", "PSZ-1063"); + + if (bicker_read_string('\x01', '\x60', string) >= 0) { + dstate_setinfo("device.mfr", string); + } + + if (bicker_read_string('\x01', '\x61', string) >= 0) { + dstate_setinfo("device.serial", string); + } + + if (bicker_read_string('\x01', '\x62', string) >= 0) { + dstate_setinfo("device.model", string); + } upsh.instcmd = bicker_instcmd; } void upsdrv_updateinfo(void) { - int16_t data, charge_current; + uint8_t u8; + uint16_t u16; + int16_t i16; ssize_t ret; - ret = bicker_read_int16('\x03', '\x25', &data); + ret = bicker_read_uint16('\x01', '\x41', &u16); + if (ret < 0) { + dstate_datastale(); + return; + } + dstate_setinfo("input.voltage", "%.1f", (double) u16 / 1000); + + ret = bicker_read_uint16('\x01', '\x42', &u16); if (ret < 0) { dstate_datastale(); return; } - dstate_setinfo("input.voltage", "%.1f", (double) data / 1000); + dstate_setinfo("input.current", "%.3f", (double) u16 / 1000); - ret = bicker_read_int16('\x03', '\x28', &data); + ret = bicker_read_uint16('\x01', '\x43', &u16); if (ret < 0) { dstate_datastale(); return; } - dstate_setinfo("input.current", "%.3f", (double) data / 1000); + dstate_setinfo("output.voltage", "%.3f", (double) u16 / 1000); - ret = bicker_read_int16('\x03', '\x27', &data); + ret = bicker_read_uint16('\x01', '\x44', &u16); if (ret < 0) { dstate_datastale(); return; } - dstate_setinfo("output.voltage", "%.3f", (double) data / 1000); + dstate_setinfo("output.current", "%.3f", (double) u16 / 1000); /* This is a supercap UPS so, in this context, * the "battery" is the supercap stack */ - ret = bicker_read_int16('\x03', '\x26', &data); + ret = bicker_read_uint16('\x01', '\x45', &u16); if (ret < 0) { dstate_datastale(); return; } - dstate_setinfo("battery.voltage", "%.3f", (double) data / 1000); + dstate_setinfo("battery.voltage", "%.3f", (double) u16 / 1000); - ret = bicker_read_int16('\x03', '\x29', &charge_current); + ret = bicker_read_int16('\x01', '\x46', &i16); if (ret < 0) { dstate_datastale(); return; } - dstate_setinfo("battery.current", "%.3f", (double) charge_current / 1000); - - /* GetChargeStaturRegister returns a 16 bit register: - * - * 0. SD Shows that the device is in step-down (charging) mode. - * 1. SU Shows that the device is in step-up (backup) mode. - * 2. CV Shows that the charger is in constant voltage mode. - * 3. UV Shows that the charger is in undervoltage lockout. - * 4. CL Shows that the device is in input current limit. - * 5. CG Shows that the capacitor voltage is above power good threshold. - * 6. CS Shows that the capacitor manager is shunting. - * 7. CB Shows that the capacitor manager is balancing. - * 8. CD Shows that the charger is temporarily disabled for capacitance measurement. - * 9. CC Shows that the charger is in constant current mode. - * 10. RV Reserved Bit - * 11. PF Shows that the input voltage is below the Power Fail Input (PFI) threshold. - * 12. RV Reserved Bit - * 13. RV Reserved Bit - * 14. RV Reserved Bit - * 15. RV Reserved Bit - */ - ret = bicker_read_int16('\x03', '\x1B', &data); + dstate_setinfo("battery.current", "%.3f", (double) i16 / 1000); + + /* Not implemented for all energy packs: failure acceptable */ + if (bicker_read_uint16('\x01', '\x4A', &u16) >= 0) { + dstate_setinfo("battery.temperature", "%.1f", (double) u16 - 273.16); + } + + /* Not implemented for all energy packs: failure acceptable */ + if (bicker_read_uint8('\x01', '\x48', &u8) >= 0) { + dstate_setinfo("battery.status", "%d%%", u8); + } + + ret = bicker_read_uint8('\x01', '\x47', &u8); if (ret < 0) { dstate_datastale(); return; } + dstate_setinfo("battery.charge", "%d", u8); status_init(); - /* Check PF (bit 11) to know if the UPS is on line/battery */ - status_set((data & 0x0800) > 0 ? "OB" : "OL"); - - /* Check CG (bit 5) to know if the battery is low */ - if ((data & 0x0020) == 0) { + /* Consider the battery low when its charge is < 30% */ + if (u8 < 30) { status_set("LB"); } - /* If there is a current of more than 1 A flowing towards - * the supercaps, consider the battery in charging mode */ - if (charge_current > 1000) { + /* StatusFlags() returns an 8 bit register: + * 0. Charging + * 1. Discharging + * 2. Power present + * 3. Battery present + * 4. Shutdown received + * 5. Overcurrent + * 6. --- + * 7. --- + */ + ret = bicker_read_uint8('\x01', '\x40', &u8); + if (ret < 0) { + dstate_datastale(); + return; + } + + if ((u8 & 0x01) > 0) { status_set("CHRG"); } + if ((u8 & 0x02) > 0) { + status_set("DISCHRG"); + } + dstate_setinfo("battery.charger.status", + (u8 & 0x01) > 0 ? "charging" : + (u8 & 0x02) > 0 ? "discharging" : + "resting"); + + status_set((u8 & 0x04) > 0 ? "OL" : "OB"); + if ((u8 & 0x20) > 0) { + status_set("OVER"); + } status_commit(); @@ -485,13 +578,29 @@ void upsdrv_makevartable(void) void upsdrv_initups(void) { + char string[BICKER_MAXDATA + 1]; + upsfd = ser_open(device_path); ser_set_speed(upsfd, device_path, B38400); + ser_set_dtr(upsfd, 1); /* Adding this variable here because upsdrv_initinfo() is not * called when triggering a forced shutdown and * "ups.delay.shutdown" is needed right there */ dstate_setinfo("ups.delay.shutdown", "%u", BICKER_DELAY); + + if (bicker_read_string('\x01', '\x63', string) >= 0) { + dstate_setinfo("ups.firmware", string); + } + + if (bicker_read_string('\x01', '\x64', string) >= 0) { + dstate_setinfo("battery.type", string); + } + + /* Not implemented on all UPSes */ + if (bicker_read_string('\x01', '\x65', string) >= 0) { + dstate_setinfo("ups.firmware.aux", string); + } } void upsdrv_cleanup(void) From c659e314c00910d7bc9a69ab05b486650ec0b305 Mon Sep 17 00:00:00 2001 From: Nicola Fontana Date: Tue, 21 May 2024 20:39:29 +0200 Subject: [PATCH 12/23] bicker_ser: use variable for low battery limit Signed-off-by: Nicola Fontana --- drivers/bicker_ser.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/bicker_ser.c b/drivers/bicker_ser.c index 8306102bcd..0fe07fb9d9 100644 --- a/drivers/bicker_ser.c +++ b/drivers/bicker_ser.c @@ -443,6 +443,7 @@ void upsdrv_initinfo(void) void upsdrv_updateinfo(void) { + const char *str; uint8_t u8; uint16_t u16; int16_t i16; @@ -512,7 +513,8 @@ void upsdrv_updateinfo(void) status_init(); /* Consider the battery low when its charge is < 30% */ - if (u8 < 30) { + str = dstate_getinfo("battery.charge.low"); + if (u8 < atoi(str)) { status_set("LB"); } @@ -597,6 +599,8 @@ void upsdrv_initups(void) dstate_setinfo("battery.type", string); } + dstate_setinfo("battery.charge.low", "%d", 30); + /* Not implemented on all UPSes */ if (bicker_read_string('\x01', '\x65', string) >= 0) { dstate_setinfo("ups.firmware.aux", string); From a4c3767202516bd2f5ec2a36a357df86479bf14e Mon Sep 17 00:00:00 2001 From: Nicola Fontana Date: Tue, 21 May 2024 22:38:28 +0200 Subject: [PATCH 13/23] bicker_ser: add parameter support Added the needed infrastructure for getting and setting parameters. Actually the only parameter that is surely working is "ups.delay.start". Signed-off-by: Nicola Fontana --- drivers/bicker_ser.c | 135 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 135 insertions(+) diff --git a/drivers/bicker_ser.c b/drivers/bicker_ser.c index 0fe07fb9d9..2b31b11302 100644 --- a/drivers/bicker_ser.c +++ b/drivers/bicker_ser.c @@ -123,6 +123,12 @@ #define TOUINT(ch) ((unsigned)(uint8_t)(ch)) #define BYTESWAP(in) ((((uint16_t)(in) & 0x00FF) << 8) + (((uint16_t)(in) & 0xFF00) >> 8)) +#ifdef WORDS_BIGENDIAN +#define SWAPONBE(in) BYTESWAP(in) +#else +#define SWAPONBE(in) (in) +#endif + upsdrv_info_t upsdrv_info = { DRIVER_NAME, DRIVER_VERSION, @@ -131,6 +137,15 @@ upsdrv_info_t upsdrv_info = { { NULL } }; +typedef struct { + uint8_t id; + uint16_t min; + uint16_t max; + uint16_t std; + uint8_t enabled; + uint16_t val; +} BickerParameter; + /** * Send a packet. * @param idx Command index @@ -366,6 +381,88 @@ static ssize_t bicker_read_string(char idx, char cmd, char *dst) return ret; } +static ssize_t bicker_receive_parameter(BickerParameter *parameter) +{ + ssize_t ret; + uint8_t data[10]; + + ret = bicker_receive_known(0x07, parameter->id, data, 10); + if (ret < 0) { + return ret; + } + + /* The returned `data` is in the format: + * [AA] [bbBB] [ccCC] [ddDD] [EE] [ffFF] + * where: + * [AA] = parameter ID (Byte) + * [BBbb] = minimum value (UInt16) + * [CCcc] = maximum value (UInt16) + * [DDdd] = standard value (UInt16) + * [EE] = enabled (Bool) + * [FFff] = value (UInt16) + */ + parameter->id = data[0]; + parameter->min = SWAPONBE(* (uint16_t *) &data[1]); + parameter->max = SWAPONBE(* (uint16_t *) &data[3]); + parameter->std = SWAPONBE(* (uint16_t *) &data[5]); + parameter->enabled = data[7]; + parameter->val = SWAPONBE(* (uint16_t *) &data[8]); + + upsdebugx(3, "Parameter %d = %d (%s, min = %d, max = %d, std = %d)", + parameter->id, parameter->val, + parameter->enabled ? "enabled" : "disabled", + parameter->min, parameter->max, parameter->std); + + return ret; +} + +/** + * Get a Bicker parameter. + * @param parameter In/out parameter struct + * @return The size of the data field on success or -1 on errors. + * + * You must fill at least the `parameter->id` field. + */ +static ssize_t bicker_get(BickerParameter *parameter) +{ + ssize_t ret; + + ret = bicker_send(0x07, parameter->id, NULL, 0); + if (ret < 0) { + return ret; + } + + return bicker_receive_parameter(parameter); +} + +/** + * Set a Bicker parameter. + * @param parameter In parameter struct + * @return The size of the data field on success or -1 on errors. + * + * You must fill at least the `parameter->id`, `parameter->enabled` and + * `parameter->val` fields. + */ +static ssize_t bicker_set(BickerParameter *parameter) +{ + ssize_t ret; + uint8_t data[3]; + + /* Format of `data` is "[EE] [ffFF]" + * where: + * [EE] = enabled (Bool) + * [FFff] = value (UInt16) + */ + data[0] = parameter->enabled; + * (uint16_t *) &data[1] = SWAPONBE(parameter->val); + ret = bicker_send(0x07, parameter->id, data, 3); + if (ret < 0) { + return ret; + } + + return bicker_receive_parameter(parameter); +} + /* For some reason the `seconds` delay (at least on my UPSIC-2403D) * is not honored: the shutdown is always delayed by 2 seconds. This * fixed delay seems to be independent from the state of the UPS (on @@ -420,6 +517,29 @@ static int bicker_instcmd(const char *cmdname, const char *extra) return STAT_INSTCMD_UNKNOWN; } +static int bicker_setvar(const char *varname, const char *val) +{ + if (!strcasecmp(varname, "battery.charge.restart")) { + BickerParameter parameter; + parameter.id = 0x05; + parameter.enabled = 1; + parameter.val = atoi(val); + dstate_setinfo("battery.charge.restart", "%u", parameter.val); + } else if (!strcasecmp(varname, "ups.delay.start")) { + BickerParameter parameter; + parameter.id = 0x04; + parameter.enabled = 1; + parameter.val = atoi(val); + if (bicker_set(¶meter) >= 0) { + dstate_setinfo("ups.delay.start", "%u", parameter.val); + } + return STAT_SET_HANDLED; + } + + upslogx(LOG_NOTICE, "setvar: unknown variable [%s]", varname); + return STAT_SET_UNKNOWN; +} + void upsdrv_initinfo(void) { char string[BICKER_MAXDATA + 1]; @@ -439,6 +559,7 @@ void upsdrv_initinfo(void) } upsh.instcmd = bicker_instcmd; + upsh.setvar = bicker_setvar; } void upsdrv_updateinfo(void) @@ -581,6 +702,7 @@ void upsdrv_makevartable(void) void upsdrv_initups(void) { char string[BICKER_MAXDATA + 1]; + BickerParameter parameter; upsfd = ser_open(device_path); ser_set_speed(upsfd, device_path, B38400); @@ -605,6 +727,19 @@ void upsdrv_initups(void) if (bicker_read_string('\x01', '\x65', string) >= 0) { dstate_setinfo("ups.firmware.aux", string); } + + parameter.id = 0x05; + if (bicker_get(¶meter) >= 0) { + /* XXX: it seems to not work */ + dstate_setinfo("battery.charge.restart", "%u", + parameter.enabled ? parameter.val : 0); + } + + parameter.id = 0x04; + if (bicker_get(¶meter) >= 0) { + dstate_setinfo("ups.delay.start", "%u", + parameter.enabled ? parameter.val : 0); + } } void upsdrv_cleanup(void) From ee71353209123a046f77809b3255a473b50e780a Mon Sep 17 00:00:00 2001 From: Nicola Fontana Date: Wed, 22 May 2024 10:12:37 +0200 Subject: [PATCH 14/23] bicker_ser: differentiate between error and unsupported When a command is unsupported, the "\x01\x03\xEE\x07\x04" response packet is returned. This is different from a response packet with a wrong command index. WARNING: the "unsupported" packet has been found sperimentally and it is an undocumented feature. Signed-off-by: Nicola Fontana --- drivers/bicker_ser.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/drivers/bicker_ser.c b/drivers/bicker_ser.c index 2b31b11302..141e5dc075 100644 --- a/drivers/bicker_ser.c +++ b/drivers/bicker_ser.c @@ -243,12 +243,15 @@ static ssize_t bicker_receive(char idx, char cmd, void *data) upslogx(LOG_WARNING, "Received 0x%02X instead of EOT (0x%02X)", TOUINT(buf[datalen + 4]), TOUINT(BICKER_EOT)); return -1; + } else if (idx != '\xEE' && buf[2] == '\xEE') { + /* I found sperimentally that, when the syntax is + * formally correct but a feature is not supported, + * the device returns "\x01\x03\xEE\x07\x04". */ + upsdebugx(2, "Command is not supported"); + return -1; } else if (buf[2] != idx) { - /* This probably suggests the command is correct but not - * supported, so no logging performed here */ - upsdebugx(2, "Indexes do not match, maybe the command is not supported?" - " Sent 0x%02X, received 0x%02X", - TOUINT(idx), TOUINT(buf[2])); + upslogx(LOG_WARNING, "Indexes do not match: sent 0x%02X, received 0x%02X", + TOUINT(idx), TOUINT(buf[2])); return -1; } else if (buf[3] != cmd) { upslogx(LOG_WARNING, "Commands do not match: sent 0x%02X, received 0x%02X", From c9fed627197ab43f62da675d26ae3c75b8d63f77 Mon Sep 17 00:00:00 2001 From: Nicola Fontana Date: Wed, 22 May 2024 10:21:42 +0200 Subject: [PATCH 15/23] bicker_ser: fix string dstate setting User provided strings could contain printf special sequences and as such they should never be used in the format argument of dstate_setinfo(). Signed-off-by: Nicola Fontana --- drivers/bicker_ser.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/bicker_ser.c b/drivers/bicker_ser.c index 141e5dc075..758c133d41 100644 --- a/drivers/bicker_ser.c +++ b/drivers/bicker_ser.c @@ -550,15 +550,15 @@ void upsdrv_initinfo(void) dstate_setinfo("device.type", "ups"); if (bicker_read_string('\x01', '\x60', string) >= 0) { - dstate_setinfo("device.mfr", string); + dstate_setinfo("device.mfr", "%s", string); } if (bicker_read_string('\x01', '\x61', string) >= 0) { - dstate_setinfo("device.serial", string); + dstate_setinfo("device.serial", "%s", string); } if (bicker_read_string('\x01', '\x62', string) >= 0) { - dstate_setinfo("device.model", string); + dstate_setinfo("device.model", "%s", string); } upsh.instcmd = bicker_instcmd; @@ -717,18 +717,18 @@ void upsdrv_initups(void) dstate_setinfo("ups.delay.shutdown", "%u", BICKER_DELAY); if (bicker_read_string('\x01', '\x63', string) >= 0) { - dstate_setinfo("ups.firmware", string); + dstate_setinfo("ups.firmware", "%s", string); } if (bicker_read_string('\x01', '\x64', string) >= 0) { - dstate_setinfo("battery.type", string); + dstate_setinfo("battery.type", "%s", string); } dstate_setinfo("battery.charge.low", "%d", 30); /* Not implemented on all UPSes */ if (bicker_read_string('\x01', '\x65', string) >= 0) { - dstate_setinfo("ups.firmware.aux", string); + dstate_setinfo("ups.firmware.aux", "%s", string); } parameter.id = 0x05; From c9ba796d336b9af44bb2c79665283328a6997bae Mon Sep 17 00:00:00 2001 From: Nicola Fontana Date: Wed, 22 May 2024 11:34:46 +0200 Subject: [PATCH 16/23] bicker_ser: fix alignment and endianness problems Accessing an uint16_t on odd addresses is not portable. Avoid having to deal with that issue by always constructing words mathematically from single bytes. This also solves any endianness difference. Signed-off-by: Nicola Fontana --- drivers/bicker_ser.c | 49 +++++++++++++++++++++++--------------------- 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/drivers/bicker_ser.c b/drivers/bicker_ser.c index 758c133d41..2c534dbc42 100644 --- a/drivers/bicker_ser.c +++ b/drivers/bicker_ser.c @@ -121,13 +121,9 @@ #define BICKER_PACKET (1 + BICKER_HEADER + BICKER_MAXDATA + 1) #define TOUINT(ch) ((unsigned)(uint8_t)(ch)) -#define BYTESWAP(in) ((((uint16_t)(in) & 0x00FF) << 8) + (((uint16_t)(in) & 0xFF00) >> 8)) - -#ifdef WORDS_BIGENDIAN -#define SWAPONBE(in) BYTESWAP(in) -#else -#define SWAPONBE(in) (in) -#endif +#define LOWBYTE(w) ((uint8_t)((uint16_t)(w) & 0x00FF)) +#define HIGHBYTE(w) ((uint8_t)(((uint16_t)(w) & 0xFF00) >> 8)) +#define WORDLH(l,h) ((uint16_t)((l) + ((h) << 8))) upsdrv_info_t upsdrv_info = { DRIVER_NAME, @@ -323,38 +319,44 @@ static ssize_t bicker_read_uint8(char idx, char cmd, uint8_t *dst) } /** - * Execute a command that returns an int16_t value. + * Execute a command that returns an uint16_t value. * @param idx Command index * @param cmd Command - * @param dst Destination for the value + * @param dst Destination for the value of NULL to discard * @return The size of the data field on success or -1 on errors. */ -static ssize_t bicker_read_int16(char idx, char cmd, int16_t *dst) +static ssize_t bicker_read_uint16(char idx, char cmd, uint16_t *dst) { ssize_t ret; + uint8_t data[2]; ret = bicker_send(idx, cmd, NULL, 0); if (ret < 0) { return ret; } - ret = bicker_receive_known(idx, cmd, dst, 2); + ret = bicker_receive_known(idx, cmd, data, 2); if (ret < 0) { return ret; } -#ifdef WORDS_BIGENDIAN - /* By default data is stored in little-endian so, on big-endian - * platforms, a byte swap must be performed */ - *dst = BYTESWAP(*dst); -#endif + if (dst != NULL) { + *dst = WORDLH(data[0], data[1]); + } return ret; } -static ssize_t bicker_read_uint16(char idx, char cmd, uint16_t *dst) +/** + * Execute a command that returns an int16_t value. + * @param idx Command index + * @param cmd Command + * @param dst Destination for the value or NULL to discard + * @return The size of the data field on success or -1 on errors. + */ +static ssize_t bicker_read_int16(char idx, char cmd, int16_t *dst) { - return bicker_read_int16(idx, cmd, (int16_t *) dst); + return bicker_read_uint16(idx, cmd, (uint16_t *) dst); } /** @@ -405,11 +407,11 @@ static ssize_t bicker_receive_parameter(BickerParameter *parameter) * [FFff] = value (UInt16) */ parameter->id = data[0]; - parameter->min = SWAPONBE(* (uint16_t *) &data[1]); - parameter->max = SWAPONBE(* (uint16_t *) &data[3]); - parameter->std = SWAPONBE(* (uint16_t *) &data[5]); + parameter->min = WORDLH(data[1], data[2]); + parameter->max = WORDLH(data[3], data[4]); + parameter->std = WORDLH(data[5], data[6]); parameter->enabled = data[7]; - parameter->val = SWAPONBE(* (uint16_t *) &data[8]); + parameter->val = WORDLH(data[8], data[9]); upsdebugx(3, "Parameter %d = %d (%s, min = %d, max = %d, std = %d)", parameter->id, parameter->val, @@ -457,7 +459,8 @@ static ssize_t bicker_set(BickerParameter *parameter) * [FFff] = value (UInt16) */ data[0] = parameter->enabled; - * (uint16_t *) &data[1] = SWAPONBE(parameter->val); + data[1] = LOWBYTE(parameter->val); + data[2] = HIGHBYTE(parameter->val); ret = bicker_send(0x07, parameter->id, data, 3); if (ret < 0) { return ret; From beb26a68472e98992dec22db26044fbd747311d7 Mon Sep 17 00:00:00 2001 From: Nicola Fontana Date: Wed, 22 May 2024 11:57:44 +0200 Subject: [PATCH 17/23] bicker_ser: cosmetic fixes Signed-off-by: Nicola Fontana --- drivers/bicker_ser.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/drivers/bicker_ser.c b/drivers/bicker_ser.c index 2c534dbc42..1c59135ada 100644 --- a/drivers/bicker_ser.c +++ b/drivers/bicker_ser.c @@ -240,7 +240,7 @@ static ssize_t bicker_receive(char idx, char cmd, void *data) TOUINT(buf[datalen + 4]), TOUINT(BICKER_EOT)); return -1; } else if (idx != '\xEE' && buf[2] == '\xEE') { - /* I found sperimentally that, when the syntax is + /* I found experimentally that, when the syntax is * formally correct but a feature is not supported, * the device returns "\x01\x03\xEE\x07\x04". */ upsdebugx(2, "Command is not supported"); @@ -267,10 +267,10 @@ static ssize_t bicker_receive(char idx, char cmd, void *data) * @param idx Command index * @param cmd Command * @param data Destination buffer or NULL to discard the data field - * @param datalen The expected size of the data field + * @param datalen The expected size of the data field * @return The size of the data field on success or -1 on errors. * - * `data`, if specified, must have at least `datalen` bytes. If + * `data`, if not NULL, must have at least `datalen` bytes. If * `datalen` is less than the received data size, an error is thrown. */ static ssize_t bicker_receive_known(char idx, char cmd, void *data, size_t datalen) @@ -363,10 +363,11 @@ static ssize_t bicker_read_int16(char idx, char cmd, int16_t *dst) * Execute a command that returns a string. * @param idx Command index * @param cmd Command - * @param dst Destination for the string + * @param dst Destination for the string or NULL to discard + * @return The size of the data field on success or -1 on errors. * - * `dst` must have at least BICKER_MAXDATA+1 bytes, the additional byte - * needed to accomodate the ending '\0'. + * `dst`, if not NULL, must have at least BICKER_MAXDATA+1 bytes, the + * additional byte needed to accomodate the ending '\0'. */ static ssize_t bicker_read_string(char idx, char cmd, char *dst) { @@ -472,7 +473,7 @@ static ssize_t bicker_set(BickerParameter *parameter) /* For some reason the `seconds` delay (at least on my UPSIC-2403D) * is not honored: the shutdown is always delayed by 2 seconds. This * fixed delay seems to be independent from the state of the UPS (on - * line or on battery) and from the dip switches setting. + * line or on battery) and from the DIP switches setting. * * As response I get the same command with `0xE0` in the data field. */ From 0c80ece5927dcbab5e5609ba34aed38ca6853a1a Mon Sep 17 00:00:00 2001 From: Nicola Fontana Date: Wed, 22 May 2024 14:58:35 +0200 Subject: [PATCH 18/23] bicker_ser: use uint8_t for bytes instead of char `char` is by default signed and (as the name implies) designed to access string characters. For accessing bytes, the `uint8_t` type is more appropriate. Signed-off-by: Nicola Fontana --- drivers/bicker_ser.c | 68 ++++++++++++++++++++++---------------------- 1 file changed, 34 insertions(+), 34 deletions(-) diff --git a/drivers/bicker_ser.c b/drivers/bicker_ser.c index 1c59135ada..6b31c1c590 100644 --- a/drivers/bicker_ser.c +++ b/drivers/bicker_ser.c @@ -32,12 +32,12 @@ * | HEADER | * * where: - * - `SOH` is the start signal ('\x01') + * - `SOH` is the start signal (0x01) * - `Size` is the length (in bytes) of the header and the data field * - `Index` is the command index: see AVAILABLE COMMANDS * - `CMD` is the command code to execute: see AVAILABLE COMMANDS * - `Data` is the (optional) argument of the command - * - `EOT` is the end signal ('\x04') + * - `EOT` is the end signal (0x04) * * The same format is used for incoming and outcoming packets. The data * returned in the `Data` field is always in little-endian order. @@ -109,8 +109,8 @@ #define DRIVER_NAME "Bicker serial protocol" #define DRIVER_VERSION "0.01" -#define BICKER_SOH '\x01' -#define BICKER_EOT '\x04' +#define BICKER_SOH 0x01 +#define BICKER_EOT 0x04 #define BICKER_TIMEOUT 1 #define BICKER_DELAY 20 #define BICKER_RETRIES 3 @@ -150,9 +150,9 @@ typedef struct { * @param datalen Size of the source data field or 0 * @return `datalen` on success or -1 on errors. */ -static ssize_t bicker_send(char idx, char cmd, const void *data, size_t datalen) +static ssize_t bicker_send(uint8_t idx, uint8_t cmd, const void *data, size_t datalen) { - char buf[BICKER_PACKET]; + uint8_t buf[BICKER_PACKET]; size_t buflen; ssize_t ret; @@ -200,11 +200,11 @@ static ssize_t bicker_send(char idx, char cmd, const void *data, size_t datalen) * The data field is stored directly in the destination buffer. `data`, * if not NULL, must have at least BICKER_MAXDATA bytes. */ -static ssize_t bicker_receive(char idx, char cmd, void *data) +static ssize_t bicker_receive(uint8_t idx, uint8_t cmd, void *data) { ssize_t ret; size_t datalen; - char buf[BICKER_PACKET]; + uint8_t buf[BICKER_PACKET]; /* Read first two bytes (SOH + size) */ ret = ser_get_buf_len(upsfd, buf, 2, BICKER_TIMEOUT, 0); @@ -239,10 +239,10 @@ static ssize_t bicker_receive(char idx, char cmd, void *data) upslogx(LOG_WARNING, "Received 0x%02X instead of EOT (0x%02X)", TOUINT(buf[datalen + 4]), TOUINT(BICKER_EOT)); return -1; - } else if (idx != '\xEE' && buf[2] == '\xEE') { + } else if (idx != 0xEE && buf[2] == 0xEE) { /* I found experimentally that, when the syntax is * formally correct but a feature is not supported, - * the device returns "\x01\x03\xEE\x07\x04". */ + * the device returns 0x01 0x03 0xEE 0x07 0x04. */ upsdebugx(2, "Command is not supported"); return -1; } else if (buf[2] != idx) { @@ -273,11 +273,11 @@ static ssize_t bicker_receive(char idx, char cmd, void *data) * `data`, if not NULL, must have at least `datalen` bytes. If * `datalen` is less than the received data size, an error is thrown. */ -static ssize_t bicker_receive_known(char idx, char cmd, void *data, size_t datalen) +static ssize_t bicker_receive_known(uint8_t idx, uint8_t cmd, void *data, size_t datalen) { ssize_t ret; size_t real_datalen; - char real_data[BICKER_MAXDATA]; + uint8_t real_data[BICKER_MAXDATA]; ret = bicker_receive(idx, cmd, real_data); if (ret < 0) { @@ -306,7 +306,7 @@ static ssize_t bicker_receive_known(char idx, char cmd, void *data, size_t datal * @param dst Destination for the value * @return The size of the data field on success or -1 on errors. */ -static ssize_t bicker_read_uint8(char idx, char cmd, uint8_t *dst) +static ssize_t bicker_read_uint8(uint8_t idx, uint8_t cmd, uint8_t *dst) { ssize_t ret; @@ -325,7 +325,7 @@ static ssize_t bicker_read_uint8(char idx, char cmd, uint8_t *dst) * @param dst Destination for the value of NULL to discard * @return The size of the data field on success or -1 on errors. */ -static ssize_t bicker_read_uint16(char idx, char cmd, uint16_t *dst) +static ssize_t bicker_read_uint16(uint8_t idx, uint8_t cmd, uint16_t *dst) { ssize_t ret; uint8_t data[2]; @@ -354,7 +354,7 @@ static ssize_t bicker_read_uint16(char idx, char cmd, uint16_t *dst) * @param dst Destination for the value or NULL to discard * @return The size of the data field on success or -1 on errors. */ -static ssize_t bicker_read_int16(char idx, char cmd, int16_t *dst) +static ssize_t bicker_read_int16(uint8_t idx, uint8_t cmd, int16_t *dst) { return bicker_read_uint16(idx, cmd, (uint16_t *) dst); } @@ -369,7 +369,7 @@ static ssize_t bicker_read_int16(char idx, char cmd, int16_t *dst) * `dst`, if not NULL, must have at least BICKER_MAXDATA+1 bytes, the * additional byte needed to accomodate the ending '\0'. */ -static ssize_t bicker_read_string(char idx, char cmd, char *dst) +static ssize_t bicker_read_string(uint8_t idx, uint8_t cmd, char *dst) { ssize_t ret; @@ -482,12 +482,12 @@ static ssize_t bicker_delayed_shutdown(uint8_t seconds) ssize_t ret; uint8_t response; - ret = bicker_send('\x03', '\x32', &seconds, 1); + ret = bicker_send(0x03, 0x32, &seconds, 1); if (ret < 0) { return ret; } - ret = bicker_receive_known('\x03', '\x32', &response, 1); + ret = bicker_receive_known(0x03, 0x32, &response, 1); if (ret >= 0) { upslogx(LOG_INFO, "Shutting down in %d seconds: response = 0x%02X", (int)seconds, (unsigned)response); @@ -553,15 +553,15 @@ void upsdrv_initinfo(void) dstate_setinfo("device.type", "ups"); - if (bicker_read_string('\x01', '\x60', string) >= 0) { + if (bicker_read_string(0x01, 0x60, string) >= 0) { dstate_setinfo("device.mfr", "%s", string); } - if (bicker_read_string('\x01', '\x61', string) >= 0) { + if (bicker_read_string(0x01, 0x61, string) >= 0) { dstate_setinfo("device.serial", "%s", string); } - if (bicker_read_string('\x01', '\x62', string) >= 0) { + if (bicker_read_string(0x01, 0x62, string) >= 0) { dstate_setinfo("device.model", "%s", string); } @@ -577,28 +577,28 @@ void upsdrv_updateinfo(void) int16_t i16; ssize_t ret; - ret = bicker_read_uint16('\x01', '\x41', &u16); + ret = bicker_read_uint16(0x01, 0x41, &u16); if (ret < 0) { dstate_datastale(); return; } dstate_setinfo("input.voltage", "%.1f", (double) u16 / 1000); - ret = bicker_read_uint16('\x01', '\x42', &u16); + ret = bicker_read_uint16(0x01, 0x42, &u16); if (ret < 0) { dstate_datastale(); return; } dstate_setinfo("input.current", "%.3f", (double) u16 / 1000); - ret = bicker_read_uint16('\x01', '\x43', &u16); + ret = bicker_read_uint16(0x01, 0x43, &u16); if (ret < 0) { dstate_datastale(); return; } dstate_setinfo("output.voltage", "%.3f", (double) u16 / 1000); - ret = bicker_read_uint16('\x01', '\x44', &u16); + ret = bicker_read_uint16(0x01, 0x44, &u16); if (ret < 0) { dstate_datastale(); return; @@ -607,14 +607,14 @@ void upsdrv_updateinfo(void) /* This is a supercap UPS so, in this context, * the "battery" is the supercap stack */ - ret = bicker_read_uint16('\x01', '\x45', &u16); + ret = bicker_read_uint16(0x01, 0x45, &u16); if (ret < 0) { dstate_datastale(); return; } dstate_setinfo("battery.voltage", "%.3f", (double) u16 / 1000); - ret = bicker_read_int16('\x01', '\x46', &i16); + ret = bicker_read_int16(0x01, 0x46, &i16); if (ret < 0) { dstate_datastale(); return; @@ -622,16 +622,16 @@ void upsdrv_updateinfo(void) dstate_setinfo("battery.current", "%.3f", (double) i16 / 1000); /* Not implemented for all energy packs: failure acceptable */ - if (bicker_read_uint16('\x01', '\x4A', &u16) >= 0) { + if (bicker_read_uint16(0x01, 0x4A, &u16) >= 0) { dstate_setinfo("battery.temperature", "%.1f", (double) u16 - 273.16); } /* Not implemented for all energy packs: failure acceptable */ - if (bicker_read_uint8('\x01', '\x48', &u8) >= 0) { + if (bicker_read_uint8(0x01, 0x48, &u8) >= 0) { dstate_setinfo("battery.status", "%d%%", u8); } - ret = bicker_read_uint8('\x01', '\x47', &u8); + ret = bicker_read_uint8(0x01, 0x47, &u8); if (ret < 0) { dstate_datastale(); return; @@ -656,7 +656,7 @@ void upsdrv_updateinfo(void) * 6. --- * 7. --- */ - ret = bicker_read_uint8('\x01', '\x40', &u8); + ret = bicker_read_uint8(0x01, 0x40, &u8); if (ret < 0) { dstate_datastale(); return; @@ -720,18 +720,18 @@ void upsdrv_initups(void) * "ups.delay.shutdown" is needed right there */ dstate_setinfo("ups.delay.shutdown", "%u", BICKER_DELAY); - if (bicker_read_string('\x01', '\x63', string) >= 0) { + if (bicker_read_string(0x01, 0x63, string) >= 0) { dstate_setinfo("ups.firmware", "%s", string); } - if (bicker_read_string('\x01', '\x64', string) >= 0) { + if (bicker_read_string(0x01, 0x64, string) >= 0) { dstate_setinfo("battery.type", "%s", string); } dstate_setinfo("battery.charge.low", "%d", 30); /* Not implemented on all UPSes */ - if (bicker_read_string('\x01', '\x65', string) >= 0) { + if (bicker_read_string(0x01, 0x65, string) >= 0) { dstate_setinfo("ups.firmware.aux", "%s", string); } From 4a33b3a7c0a723868f81f380b869a6f2a7933636 Mon Sep 17 00:00:00 2001 From: Nicola Fontana Date: Wed, 22 May 2024 15:20:07 +0200 Subject: [PATCH 19/23] bicker_ser: use proper format modifiers in logs Ensure the format string in logging messages is the correct one without subcasting. Furthermore, due to the promotion rules of the C language (where any given type, even unsigned, is promoted to int when possible), explicitly cast to unsigned where required. Signed-off-by: Nicola Fontana --- drivers/bicker_ser.c | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/drivers/bicker_ser.c b/drivers/bicker_ser.c index 6b31c1c590..b99fe97b3d 100644 --- a/drivers/bicker_ser.c +++ b/drivers/bicker_ser.c @@ -103,6 +103,7 @@ #include "config.h" #include "main.h" #include "attribute.h" +#include "nut_stdint.h" #include "serial.h" @@ -160,8 +161,9 @@ static ssize_t bicker_send(uint8_t idx, uint8_t cmd, const void *data, size_t da if (data != NULL) { if (datalen > BICKER_MAXDATA) { - upslogx(LOG_ERR, "Data size exceeded: %d > %d", - (int)datalen, BICKER_MAXDATA); + upslogx(LOG_ERR, + "Data size exceeded: %" PRIuSIZE " > %d", + datalen, BICKER_MAXDATA); return -1; } memcpy(buf + 4, data, datalen); @@ -181,8 +183,9 @@ static ssize_t bicker_send(uint8_t idx, uint8_t cmd, const void *data, size_t da upslog_with_errno(LOG_WARNING, "ser_send_buf failed"); return -1; } else if ((size_t) ret != buflen) { - upslogx(LOG_WARNING, "ser_send_buf returned %d instead of %d", - (int)ret, (int)buflen); + upslogx(LOG_WARNING, "ser_send_buf returned %" + PRIiSIZE " instead of %" PRIuSIZE, + ret, buflen); return -1; } @@ -216,7 +219,7 @@ static ssize_t bicker_receive(uint8_t idx, uint8_t cmd, void *data) return -1; } else if (buf[0] != BICKER_SOH) { upslogx(LOG_WARNING, "Received 0x%02X instead of SOH (0x%02X)", - TOUINT(buf[0]), TOUINT(BICKER_SOH)); + (unsigned)buf[0], (unsigned)BICKER_SOH); return -1; } @@ -237,7 +240,7 @@ static ssize_t bicker_receive(uint8_t idx, uint8_t cmd, void *data) return -1; } else if (buf[datalen + 4] != BICKER_EOT) { upslogx(LOG_WARNING, "Received 0x%02X instead of EOT (0x%02X)", - TOUINT(buf[datalen + 4]), TOUINT(BICKER_EOT)); + (unsigned)buf[datalen + 4], (unsigned)BICKER_EOT); return -1; } else if (idx != 0xEE && buf[2] == 0xEE) { /* I found experimentally that, when the syntax is @@ -247,11 +250,11 @@ static ssize_t bicker_receive(uint8_t idx, uint8_t cmd, void *data) return -1; } else if (buf[2] != idx) { upslogx(LOG_WARNING, "Indexes do not match: sent 0x%02X, received 0x%02X", - TOUINT(idx), TOUINT(buf[2])); + (unsigned)idx, (unsigned)buf[2]); return -1; } else if (buf[3] != cmd) { upslogx(LOG_WARNING, "Commands do not match: sent 0x%02X, received 0x%02X", - TOUINT(cmd), TOUINT(buf[3])); + (unsigned)cmd, (unsigned)buf[3]); return -1; } @@ -287,8 +290,9 @@ static ssize_t bicker_receive_known(uint8_t idx, uint8_t cmd, void *data, size_t real_datalen = (size_t)ret; if (datalen < real_datalen) { - upslogx(LOG_ERR, "Not enough space for the payload: %d < %d", - (int)datalen, (int)real_datalen); + upslogx(LOG_ERR, "Not enough space for the payload: %" + PRIuSIZE " < %" PRIuSIZE, + datalen, real_datalen); return -1; } @@ -414,10 +418,11 @@ static ssize_t bicker_receive_parameter(BickerParameter *parameter) parameter->enabled = data[7]; parameter->val = WORDLH(data[8], data[9]); - upsdebugx(3, "Parameter %d = %d (%s, min = %d, max = %d, std = %d)", - parameter->id, parameter->val, + upsdebugx(3, "Parameter %u = %u (%s, min = %u, max = %u, std = %u)", + (unsigned)parameter->id, (unsigned)parameter->val, parameter->enabled ? "enabled" : "disabled", - parameter->min, parameter->max, parameter->std); + (unsigned)parameter->min, (unsigned)parameter->max, + (unsigned)parameter->std); return ret; } @@ -490,7 +495,7 @@ static ssize_t bicker_delayed_shutdown(uint8_t seconds) ret = bicker_receive_known(0x03, 0x32, &response, 1); if (ret >= 0) { upslogx(LOG_INFO, "Shutting down in %d seconds: response = 0x%02X", - (int)seconds, (unsigned)response); + seconds, (unsigned)response); } return ret; From c78524c3f99b50ce3c08eedafb5bb0ac2eb0411e Mon Sep 17 00:00:00 2001 From: Nicola Fontana Date: Wed, 22 May 2024 15:56:12 +0200 Subject: [PATCH 20/23] bicker_ser: refactor to clarify packet manipulation Change BICKER_PACKET from a constant value to a macro that depends on the data length. This should make clear that the packet size depends directly from the data size. Refactored some code here and there trying to minimize the presence of magic numbers and clarify the intentions. Signed-off-by: Nicola Fontana --- drivers/bicker_ser.c | 43 ++++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/drivers/bicker_ser.c b/drivers/bicker_ser.c index b99fe97b3d..85fe8e03e6 100644 --- a/drivers/bicker_ser.c +++ b/drivers/bicker_ser.c @@ -116,10 +116,10 @@ #define BICKER_DELAY 20 #define BICKER_RETRIES 3 -/* Protocol fixed lengths */ -#define BICKER_HEADER 3 -#define BICKER_MAXDATA (255 - BICKER_HEADER) -#define BICKER_PACKET (1 + BICKER_HEADER + BICKER_MAXDATA + 1) +/* Protocol lengths */ +#define BICKER_HEADER 3 +#define BICKER_MAXDATA (255 - BICKER_HEADER) +#define BICKER_PACKET(datalen) (1 + BICKER_HEADER + (datalen) + 1) #define TOUINT(ch) ((unsigned)(uint8_t)(ch)) #define LOWBYTE(w) ((uint8_t)((uint16_t)(w) & 0x00FF)) @@ -153,12 +153,10 @@ typedef struct { */ static ssize_t bicker_send(uint8_t idx, uint8_t cmd, const void *data, size_t datalen) { - uint8_t buf[BICKER_PACKET]; + uint8_t buf[BICKER_PACKET(BICKER_MAXDATA)]; size_t buflen; ssize_t ret; - ser_flush_io(upsfd); - if (data != NULL) { if (datalen > BICKER_MAXDATA) { upslogx(LOG_ERR, @@ -166,17 +164,19 @@ static ssize_t bicker_send(uint8_t idx, uint8_t cmd, const void *data, size_t da datalen, BICKER_MAXDATA); return -1; } - memcpy(buf + 4, data, datalen); + memcpy(&buf[1 + BICKER_HEADER], data, datalen); } else { datalen = 0; } + ser_flush_io(upsfd); + + buflen = BICKER_PACKET(datalen); buf[0] = BICKER_SOH; - buf[1] = datalen + BICKER_HEADER; + buf[1] = BICKER_HEADER + datalen; buf[2] = idx; buf[3] = cmd; - buf[1 + BICKER_HEADER + datalen] = BICKER_EOT; - buflen = 1 + BICKER_HEADER + datalen + 1; + buf[buflen - 1] = BICKER_EOT; ret = ser_send_buf(upsfd, buf, buflen); if (ret < 0) { @@ -206,8 +206,8 @@ static ssize_t bicker_send(uint8_t idx, uint8_t cmd, const void *data, size_t da static ssize_t bicker_receive(uint8_t idx, uint8_t cmd, void *data) { ssize_t ret; - size_t datalen; - uint8_t buf[BICKER_PACKET]; + size_t buflen, datalen; + uint8_t buf[BICKER_PACKET(BICKER_MAXDATA)]; /* Read first two bytes (SOH + size) */ ret = ser_get_buf_len(upsfd, buf, 2, BICKER_TIMEOUT, 0); @@ -223,11 +223,12 @@ static ssize_t bicker_receive(uint8_t idx, uint8_t cmd, void *data) return -1; } + /* buf[1] (the size field) is BICKER_HEADER + data length, so */ datalen = buf[1] - BICKER_HEADER; - /* Read the rest: command index (1 byte), command (1 byte), data - * (datalen bytes) and EOT (1 byte), i.e. `datalen + 3` bytes */ - ret = ser_get_buf_len(upsfd, buf + 2, datalen + 3, BICKER_TIMEOUT, 0); + /* Read the rest of the packet */ + buflen = BICKER_PACKET(datalen); + ret = ser_get_buf_len(upsfd, buf + 2, buflen - 2, BICKER_TIMEOUT, 0); if (ret < 0) { upslog_with_errno(LOG_WARNING, "ser_get_buf_len failed"); return -1; @@ -235,12 +236,12 @@ static ssize_t bicker_receive(uint8_t idx, uint8_t cmd, void *data) upsdebug_hex(3, "bicker_receive", buf, ret + 2); - if ((size_t)ret < datalen + 3) { + if ((size_t)ret < buflen - 2) { upslogx(LOG_WARNING, "Timeout waiting for the end of the packet"); return -1; - } else if (buf[datalen + 4] != BICKER_EOT) { + } else if (buf[buflen - 1] != BICKER_EOT) { upslogx(LOG_WARNING, "Received 0x%02X instead of EOT (0x%02X)", - (unsigned)buf[datalen + 4], (unsigned)BICKER_EOT); + (unsigned)buf[buflen - 1], (unsigned)BICKER_EOT); return -1; } else if (idx != 0xEE && buf[2] == 0xEE) { /* I found experimentally that, when the syntax is @@ -259,7 +260,7 @@ static ssize_t bicker_receive(uint8_t idx, uint8_t cmd, void *data) } if (data != NULL) { - memcpy(data, buf + 4, datalen); + memcpy(data, &buf[1 + BICKER_HEADER], datalen); } return datalen; @@ -326,7 +327,7 @@ static ssize_t bicker_read_uint8(uint8_t idx, uint8_t cmd, uint8_t *dst) * Execute a command that returns an uint16_t value. * @param idx Command index * @param cmd Command - * @param dst Destination for the value of NULL to discard + * @param dst Destination for the value or NULL to discard * @return The size of the data field on success or -1 on errors. */ static ssize_t bicker_read_uint16(uint8_t idx, uint8_t cmd, uint16_t *dst) From 27bdc8751ce567b1ba516344967664cf29f182ed Mon Sep 17 00:00:00 2001 From: Nicola Fontana Date: Thu, 23 May 2024 08:33:49 +0200 Subject: [PATCH 21/23] bicker_ser: make parameter handling less error prone Using an in/out struct and making some specific field mandatory is cumbersome and error prone. Now the data needed for setting or getting parameters is explicitly required by the arguments. Signed-off-by: Nicola Fontana --- drivers/bicker_ser.c | 106 +++++++++++++++++++++---------------------- 1 file changed, 52 insertions(+), 54 deletions(-) diff --git a/drivers/bicker_ser.c b/drivers/bicker_ser.c index 85fe8e03e6..96fc67b688 100644 --- a/drivers/bicker_ser.c +++ b/drivers/bicker_ser.c @@ -140,7 +140,7 @@ typedef struct { uint16_t max; uint16_t std; uint8_t enabled; - uint16_t val; + uint16_t value; } BickerParameter; /** @@ -402,44 +402,45 @@ static ssize_t bicker_receive_parameter(BickerParameter *parameter) return ret; } - /* The returned `data` is in the format: - * [AA] [bbBB] [ccCC] [ddDD] [EE] [ffFF] - * where: - * [AA] = parameter ID (Byte) - * [BBbb] = minimum value (UInt16) - * [CCcc] = maximum value (UInt16) - * [DDdd] = standard value (UInt16) - * [EE] = enabled (Bool) - * [FFff] = value (UInt16) - */ - parameter->id = data[0]; - parameter->min = WORDLH(data[1], data[2]); - parameter->max = WORDLH(data[3], data[4]); - parameter->std = WORDLH(data[5], data[6]); - parameter->enabled = data[7]; - parameter->val = WORDLH(data[8], data[9]); - - upsdebugx(3, "Parameter %u = %u (%s, min = %u, max = %u, std = %u)", - (unsigned)parameter->id, (unsigned)parameter->val, - parameter->enabled ? "enabled" : "disabled", - (unsigned)parameter->min, (unsigned)parameter->max, - (unsigned)parameter->std); + if (parameter != NULL) { + /* The returned `data` is in the format: + * [AA] [bbBB] [ccCC] [ddDD] [EE] [ffFF] + * where: + * [AA] = parameter ID (Byte) + * [BBbb] = minimum value (UInt16) + * [CCcc] = maximum value (UInt16) + * [DDdd] = standard value (UInt16) + * [EE] = enabled (Bool) + * [FFff] = value (UInt16) + */ + parameter->id = data[0]; + parameter->min = WORDLH(data[1], data[2]); + parameter->max = WORDLH(data[3], data[4]); + parameter->std = WORDLH(data[5], data[6]); + parameter->enabled = data[7]; + parameter->value = WORDLH(data[8], data[9]); + + upsdebugx(3, "Parameter %u = %u (%s, min = %u, max = %u, std = %u)", + (unsigned)parameter->id, (unsigned)parameter->value, + parameter->enabled ? "enabled" : "disabled", + (unsigned)parameter->min, (unsigned)parameter->max, + (unsigned)parameter->std); + } return ret; } /** * Get a Bicker parameter. - * @param parameter In/out parameter struct + * @param id ID of the parameter (0x01..0x0A) + * @param parameter Where to store the response or NULL to discard * @return The size of the data field on success or -1 on errors. - * - * You must fill at least the `parameter->id` field. */ -static ssize_t bicker_get(BickerParameter *parameter) +static ssize_t bicker_get(uint8_t id, BickerParameter *parameter) { ssize_t ret; - ret = bicker_send(0x07, parameter->id, NULL, 0); + ret = bicker_send(0x07, id, NULL, 0); if (ret < 0) { return ret; } @@ -449,13 +450,13 @@ static ssize_t bicker_get(BickerParameter *parameter) /** * Set a Bicker parameter. - * @param parameter In parameter struct + * @param id ID of the parameter (0x01..0x0A) + * @param enabled 0 to disable, 1 to enable + * @param value + * @param parameter Where to store the response or NULL to discard * @return The size of the data field on success or -1 on errors. - * - * You must fill at least the `parameter->id`, `parameter->enabled` and - * `parameter->val` fields. */ -static ssize_t bicker_set(BickerParameter *parameter) +static ssize_t bicker_set(uint8_t id, uint8_t enabled, uint16_t value, BickerParameter *parameter) { ssize_t ret; uint8_t data[3]; @@ -465,10 +466,10 @@ static ssize_t bicker_set(BickerParameter *parameter) * [EE] = enabled (Bool) * [FFff] = value (UInt16) */ - data[0] = parameter->enabled; - data[1] = LOWBYTE(parameter->val); - data[2] = HIGHBYTE(parameter->val); - ret = bicker_send(0x07, parameter->id, data, 3); + data[0] = enabled; + data[1] = LOWBYTE(value); + data[2] = HIGHBYTE(value); + ret = bicker_send(0x07, id, data, 3); if (ret < 0) { return ret; } @@ -533,19 +534,18 @@ static int bicker_instcmd(const char *cmdname, const char *extra) static int bicker_setvar(const char *varname, const char *val) { if (!strcasecmp(varname, "battery.charge.restart")) { - BickerParameter parameter; - parameter.id = 0x05; - parameter.enabled = 1; - parameter.val = atoi(val); - dstate_setinfo("battery.charge.restart", "%u", parameter.val); + int value = (uint16_t)atoi(val); + if (bicker_set(0x05, 1, value, NULL) < 0) { + return STAT_SET_FAILED; + } + dstate_setinfo("battery.charge.restart", "%u", value); + return STAT_SET_HANDLED; } else if (!strcasecmp(varname, "ups.delay.start")) { - BickerParameter parameter; - parameter.id = 0x04; - parameter.enabled = 1; - parameter.val = atoi(val); - if (bicker_set(¶meter) >= 0) { - dstate_setinfo("ups.delay.start", "%u", parameter.val); + int value = (uint16_t)atoi(val); + if (bicker_set(0x04, 1, value, NULL) < 0) { + return STAT_SET_FAILED; } + dstate_setinfo("ups.delay.start", "%u", value); return STAT_SET_HANDLED; } @@ -741,17 +741,15 @@ void upsdrv_initups(void) dstate_setinfo("ups.firmware.aux", "%s", string); } - parameter.id = 0x05; - if (bicker_get(¶meter) >= 0) { + if (bicker_get(0x05, ¶meter) >= 0) { /* XXX: it seems to not work */ dstate_setinfo("battery.charge.restart", "%u", - parameter.enabled ? parameter.val : 0); + parameter.enabled ? parameter.value : 0); } - parameter.id = 0x04; - if (bicker_get(¶meter) >= 0) { + if (bicker_get(0x04, ¶meter) >= 0) { dstate_setinfo("ups.delay.start", "%u", - parameter.enabled ? parameter.val : 0); + parameter.enabled ? parameter.value : 0); } } From fca995785b94fe1d87692daef2fe88b160bd26cb Mon Sep 17 00:00:00 2001 From: Nicola Fontana Date: Thu, 23 May 2024 08:52:16 +0200 Subject: [PATCH 22/23] bicker_ser: add sanity checks to bicker_set These devices seem to be picky on what you use to set parameters. Signed-off-by: Nicola Fontana --- drivers/bicker_ser.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/drivers/bicker_ser.c b/drivers/bicker_ser.c index 96fc67b688..37aaede920 100644 --- a/drivers/bicker_ser.c +++ b/drivers/bicker_ser.c @@ -115,6 +115,7 @@ #define BICKER_TIMEOUT 1 #define BICKER_DELAY 20 #define BICKER_RETRIES 3 +#define BICKER_MAXID 0x0A /* Max parameter ID */ /* Protocol lengths */ #define BICKER_HEADER 3 @@ -461,6 +462,17 @@ static ssize_t bicker_set(uint8_t id, uint8_t enabled, uint16_t value, BickerPar ssize_t ret; uint8_t data[3]; + if (id < 1 || id > BICKER_MAXID) { + upslogx(LOG_ERR, "bicker_set(0x%02X, %d, %u): id out of range (0x01..0x%02X)", + (unsigned)id, enabled, (unsigned)value, + (unsigned)BICKER_MAXID); + return -1; + } else if (enabled > 1) { + upslogx(LOG_ERR, "bicker_set(0x%02X, %d, %u): enabled must be 0 or 1", + (unsigned)id, enabled, (unsigned)value); + return -1; + } + /* Format of `data` is "[EE] [ffFF]" * where: * [EE] = enabled (Bool) From 4cbe12dce899aad4290f07afbe04ec31efeae745 Mon Sep 17 00:00:00 2001 From: Nicola Fontana Date: Thu, 23 May 2024 10:29:13 +0200 Subject: [PATCH 23/23] bicker_ser: expose mappable parameters Every Bicker parameter that has a correspondence in nut-names.txt has been mapped. If the parameter is disabled on the device, no NUT variable is created. If the NUT variable is set to an empty string, the parameter is reset and disabled on the device and that NUT variable is removed. Signed-off-by: Nicola Fontana --- drivers/bicker_ser.c | 108 ++++++++++++++++++++++++++++++------------- 1 file changed, 76 insertions(+), 32 deletions(-) diff --git a/drivers/bicker_ser.c b/drivers/bicker_ser.c index 37aaede920..fbbe68c444 100644 --- a/drivers/bicker_ser.c +++ b/drivers/bicker_ser.c @@ -144,6 +144,18 @@ typedef struct { uint16_t value; } BickerParameter; +typedef struct { + uint8_t bicker_id; + const char *nut_name; +} BickerMapping; + +static const BickerMapping bicker_mappings[] = { + { 0x02, "ups.delay.shutdown" }, + { 0x04, "ups.delay.start" }, + { 0x05, "battery.charge.restart" }, + { 0x07, "battery.charge.low" }, +}; + /** * Send a packet. * @param idx Command index @@ -489,6 +501,38 @@ static ssize_t bicker_set(uint8_t id, uint8_t enabled, uint16_t value, BickerPar return bicker_receive_parameter(parameter); } +/** + * Write to a Bicker parameter. + * @param id ID of the parameter (0x01..0x0A) + * @param val A string with the value to write + * @param parameter Where to store the response (required!) + * @return The size of the data field on success or -1 on errors. + * + * This function is similar to bicker_set() but accepts string values. + * If `val` is NULL or empty, the underlying Bicker parameter is + * disabled and reset to its standard value. + */ +static int bicker_write(uint8_t id, const char *val, BickerParameter *parameter) +{ + ssize_t ret; + uint8_t enabled; + uint16_t value; + + if (val == NULL || val[0] == '\0') { + ret = bicker_get(id, parameter); + if (ret < 0) { + return ret; + } + enabled = 0; + value = parameter->std; + } else { + enabled = 1; + value = atoi(val); + } + + return bicker_set(id, enabled, value, parameter); +} + /* For some reason the `seconds` delay (at least on my UPSIC-2403D) * is not honored: the shutdown is always delayed by 2 seconds. This * fixed delay seems to be independent from the state of the UPS (on @@ -521,7 +565,7 @@ static ssize_t bicker_shutdown(void) int delay; str = dstate_getinfo("ups.delay.shutdown"); - delay = atoi(str); + delay = str != NULL ? atoi(str) : 0; if (delay > 255) { upslogx(LOG_WARNING, "Shutdown delay too big: %d > 255", delay); @@ -545,20 +589,26 @@ static int bicker_instcmd(const char *cmdname, const char *extra) static int bicker_setvar(const char *varname, const char *val) { - if (!strcasecmp(varname, "battery.charge.restart")) { - int value = (uint16_t)atoi(val); - if (bicker_set(0x05, 1, value, NULL) < 0) { - return STAT_SET_FAILED; - } - dstate_setinfo("battery.charge.restart", "%u", value); - return STAT_SET_HANDLED; - } else if (!strcasecmp(varname, "ups.delay.start")) { - int value = (uint16_t)atoi(val); - if (bicker_set(0x04, 1, value, NULL) < 0) { - return STAT_SET_FAILED; + const BickerMapping *mapping; + unsigned i; + BickerParameter parameter; + + /* Handle mapped parameters */ + for (i = 0; i < SIZEOF_ARRAY(bicker_mappings); ++i) { + mapping = &bicker_mappings[i]; + if (!strcasecmp(varname, mapping->nut_name)) { + if (bicker_write(mapping->bicker_id, val, ¶meter) < 0) { + return STAT_SET_FAILED; + } + + if (parameter.enabled) { + dstate_setinfo(varname, "%u", parameter.value); + } else { + /* Disabled parameters are removed from NUT */ + dstate_delinfo(varname); + } + return STAT_SET_HANDLED; } - dstate_setinfo("ups.delay.start", "%u", value); - return STAT_SET_HANDLED; } upslogx(LOG_NOTICE, "setvar: unknown variable [%s]", varname); @@ -658,9 +708,9 @@ void upsdrv_updateinfo(void) status_init(); - /* Consider the battery low when its charge is < 30% */ + /* On no "battery.charge.low" variable, use 30% */ str = dstate_getinfo("battery.charge.low"); - if (u8 < atoi(str)) { + if (u8 < (str != NULL ? atoi(str) : 30)) { status_set("LB"); } @@ -728,16 +778,13 @@ void upsdrv_initups(void) { char string[BICKER_MAXDATA + 1]; BickerParameter parameter; + const BickerMapping *mapping; + unsigned i; upsfd = ser_open(device_path); ser_set_speed(upsfd, device_path, B38400); ser_set_dtr(upsfd, 1); - /* Adding this variable here because upsdrv_initinfo() is not - * called when triggering a forced shutdown and - * "ups.delay.shutdown" is needed right there */ - dstate_setinfo("ups.delay.shutdown", "%u", BICKER_DELAY); - if (bicker_read_string(0x01, 0x63, string) >= 0) { dstate_setinfo("ups.firmware", "%s", string); } @@ -746,22 +793,19 @@ void upsdrv_initups(void) dstate_setinfo("battery.type", "%s", string); } - dstate_setinfo("battery.charge.low", "%d", 30); - /* Not implemented on all UPSes */ if (bicker_read_string(0x01, 0x65, string) >= 0) { dstate_setinfo("ups.firmware.aux", "%s", string); } - if (bicker_get(0x05, ¶meter) >= 0) { - /* XXX: it seems to not work */ - dstate_setinfo("battery.charge.restart", "%u", - parameter.enabled ? parameter.value : 0); - } - - if (bicker_get(0x04, ¶meter) >= 0) { - dstate_setinfo("ups.delay.start", "%u", - parameter.enabled ? parameter.value : 0); + /* Initialize mapped parameters */ + for (i = 0; i < SIZEOF_ARRAY(bicker_mappings); ++i) { + mapping = &bicker_mappings[i]; + if (bicker_get(mapping->bicker_id, ¶meter) >= 0 && + parameter.enabled) { + dstate_setinfo(mapping->nut_name, "%u", + (unsigned)parameter.value); + } } }