Add Bicker serial driver #2448

ntd · 2024-05-17T12:50:50Z

This driver supports some interesting supercap DC UPSes manufactured by Bicker. I'm planning to use them in the near future on small Linux-based industrial PC (mainly amd64 or arm64).

AFAICS, there are two main issues:

the code does not work on big-endian platforms (see the relevant comment)
the way I'm using ups.delay.shutdown stinks: most drivers defines it in upsdrv_initinfo but then my upsdrv_shutdown function crashes when trying to access it. I followed the usbhid-ups.c example and put it inside upsdrv_initups.

jimklimov

For the first cursory look at this, looks quite solid - thanks! Feel welcome to update NEWS.adoc and perhaps acknowledgements.txt

Also a man page would be needed.

drivers/bicker_ser.c

ntd · 2024-05-18T07:17:05Z

For the first cursory look at this, looks quite solid - thanks! Feel welcome to update NEWS.adoc and perhaps acknowledgements.txt

Also a man page would be needed.

First of all, many thanks for the review.

I just added manpage and updated NEWS.adoc. Not knowing where to put it (this is my first contribution to NUT), I added the announcement at the end of the Release notes for NUT 2.8.3. Feel free to move wherever more appropriate or drop me a line if I must do it.

ntd · 2024-05-18T07:30:03Z

Rebased on c8d7e3c.

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 <[email protected]>

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 <[email protected]>

Signed-off-by: Nicola Fontana <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

Identification fields are now queried from the device. Also "battery.charge" is now set properly. Signed-off-by: Nicola Fontana <[email protected]>

Signed-off-by: Nicola Fontana <[email protected]>

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 <[email protected]>

ntd · 2024-05-21T20:56:42Z

After the manufacturer provided more info, I've been able to add a lot more stuff. Now I'm quite happy with the result.
I'll try to get Bicker involved in some way, at least to get some feedback.

jimklimov · 2024-05-22T08:07:44Z

  CC       bicker_ser.o
�[01m�[K../../drivers/bicker_ser.c:�[m�[K In function '�[01m�[Kupsdrv_initinfo�[m�[K':
�[01m�[K../../drivers/bicker_ser.c:550:46:�[m�[K �[01;31m�[Kerror: �[m�[Kformat not a string literal and no format arguments [�[01;31m�[K-Werror=format-security�[m�[K]
  550 |                 dstate_setinfo("device.mfr", �[01;31m�[Kstring�[m�[K);
      |                                              �[01;31m�[K^~~~~~�[m�[K
�[01m�[K../../drivers/bicker_ser.c:554:49:�[m�[K �[01;31m�[Kerror: �[m�[Kformat not a string literal and no format arguments [�[01;31m�[K-Werror=format-security�[m�[K]
  554 |                 dstate_setinfo("device.serial", �[01;31m�[Kstring�[m�[K);
      |                                                 �[01;31m�[K^~~~~~�[m�[K
�[01m�[K../../drivers/bicker_ser.c:558:48:�[m�[K �[01;31m�[Kerror: �[m�[Kformat not a string literal and no format arguments [�[01;31m�[K-Werror=format-security�[m�[K]
  558 |                 dstate_setinfo("device.model", �[01;31m�[Kstring�[m�[K);
      |                                                �[01;31m�[K^~~~~~�[m�[K
�[01m�[K../../drivers/bicker_ser.c:�[m�[K In function '�[01m�[Kupsdrv_initups�[m�[K':
�[01m�[K../../drivers/bicker_ser.c:717:48:�[m�[K �[01;31m�[Kerror: �[m�[Kformat not a string literal and no format arguments [�[01;31m�[K-Werror=format-security�[m�[K]
  717 |                 dstate_setinfo("ups.firmware", �[01;31m�[Kstring�[m�[K);
      |                                                �[01;31m�[K^~~~~~�[m�[K
�[01m�[K../../drivers/bicker_ser.c:721:48:�[m�[K �[01;31m�[Kerror: �[m�[Kformat not a string literal and no format arguments [�[01;31m�[K-Werror=format-security�[m�[K]
  721 |                 dstate_setinfo("battery.type", �[01;31m�[Kstring�[m�[K);
      |                                                �[01;31m�[K^~~~~~�[m�[K
�[01m�[K../../drivers/bicker_ser.c:728:52:�[m�[K �[01;31m�[Kerror: �[m�[Kformat not a string literal and no format arguments [�[01;31m�[K-Werror=format-security�[m�[K]
  728 |                 dstate_setinfo("ups.firmware.aux", �[01;31m�[Kstring�[m�[K);
      |                                                    �[01;31m�[K^~~~~~�[m�[K
�[01m�[K../../drivers/bicker_ser.c:�[m�[K At top level:

IIRC those should be dstate_setinfo("some.name", "%s", stringvar); wherever you have fixed strings provided by a third party (so they are not interpreted as a formatting pattern inadvertently). We do have a few cases of formatting strings being variables (coming from our tables) with macros hushing this warning, but that is a special case which needs extra care and supervision :)

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 <[email protected]>

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 <[email protected]>

ntd · 2024-05-22T08:26:59Z

IIRC those should be dstate_setinfo("some.name", "%s", stringvar); wherever you have fixed strings provided by a third party (so they are not interpreted as a formatting pattern inadvertently).

Of course you are right, sorry about that. I'm very well aware user provided strings in printf format arguments can lead to nasty bugs. I just fixed that in c9fed62.

jimklimov · 2024-05-22T08:39:57Z

Thanks! And that reminded me to post a TODO #2450 :)

jimklimov · 2024-05-22T08:54:38Z

Ugh, in clang scenarios above another concern has surfaced:

...
bicker_ser.c:129:23: note: expanded from macro 'SWAPONBE'
#define SWAPONBE(in)    (in)
                         ^~
bicker_ser.c:460:4: error: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint16_t *' (aka 'unsigned short *') increases required alignment from 1 to 2 [-Werror,-Wcast-align]
        * (uint16_t *) &data[1] = SWAPONBE(parameter->val);
          ^~~~~~~~~~~~~~~~~~~~~
5 errors generated.

Maybe casting in the macro would suffice? Like

#define SWAPONBE(in)    ((uint16_t)in)

ntd · 2024-05-22T09:03:50Z

Yes, I've seen that. I was fearing that could be an issue: I think the problem is not SWAPONBE itself but the calling code that is trying to access uint16_t on odd addresses. I'll try to come up with a solution. At the very least I can extract the uint16_t using mathematic.

jimklimov

Looks very solid, thank you again. I've got some suggestions for further perfection or double-checking, but I think none of those are critical/blockers.

drivers/bicker_ser.c

jimklimov · 2024-05-22T09:23:03Z

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


For posterity: This mixed casing of Vac scratched my eye a bit - I'm used to seeing acronyms as upper-cased. But lower-case ac/dc seem to be the modern IEEE recommendation, according to internet discussions of many others whose eyes this has also got irritated :)

I copy&pasted the adelsystem_cbi announcement, that seems the closer one to the bicker_ser description.

But now the protocol is based on the UPS-Gen² software user manual so, according to that manual, more UPSes are supported (even USB one):

At the end I think that description is at least incomplete. We'll see if Bicker has anything to say about that.

jimklimov · 2024-05-22T09:33:12Z

Yes, I've seen that. I was fearing that could be an issue: I think the problem is not SWAPONBE itself but the calling code that is trying to access uint16_t on odd addresses. I'll try to come up with a solution. At the very least I can extract the uint16_t using mathematic.

I think you can try compiler hints to align(1) (don't remember the syntax OTOH) so it would not try to optimize memory access to the opaque buffer you are actually dealing with. Although I think it is implicit for a char[] buffer anyway. With a four-byte preamble (from buffer start to data) as per current offset definition, operations with the only two "data" bytes which are the 16-bit word would actually be aligned to whatever 1/2(/4?) byte alignment involved, so little hit performance-wise on platforms where getting to this starting data byte invokes extra CPU cycles. Dunno about 8-byte (64-bit) alignment though, might have some cost there.

By "extracting" you mean constructing a new uint16_t using byte selection and bit-shift - just without swapping them?

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 <[email protected]>

ntd · 2024-05-22T09:43:25Z

By "extracting" you mean constructing a new uint16_t using byte selection and bit-shift - just without swapping them?

Yes, I ended up doing that. The resulting code seems even cleaner and it does not rely on endianness anymore. I will push shortly after solving some of the issue you posted (and some cosmetic issues that are striking my eyes too).

Signed-off-by: Nicola Fontana <[email protected]>

`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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

These devices seem to be picky on what you use to set parameters. Signed-off-by: Nicola Fontana <[email protected]>

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 <[email protected]>

ntd · 2024-05-23T10:09:19Z

Basically the code is now complete.

I mapped the relevant Bicker parameters to nut-names but unfortunately I cannot do more testing because I borked the UPS I was using. I think I corrupted the parameter memory by passing 2 as enabled to bicker_set(), so to minimize this kind of errors I added sanity checks with fca9957.

jimklimov · 2024-05-24T00:38:53Z

My condolences about the device... its sacrifice will be remembered :(
And I hope you find a way to un-brick it :)

jimklimov

Looks great and a lot better than much of the older code, I am out of ideas what else to complain about. Thank you very much for bearing with me and my nits :D

jimklimov · 2024-05-24T00:46:02Z

drivers/bicker_ser.c

- 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);


Just to be sure / for posterity: buflen - 2 here (and a bit below) is the whole packet (including BICKER_EOT final byte) other than the first two bytes we've just read above?

Yes, exactly that in both cases.

jimklimov reviewed May 17, 2024

View reviewed changes

drivers/bicker_ser.c Outdated Show resolved Hide resolved

drivers/bicker_ser.c Show resolved Hide resolved

drivers/bicker_ser.c Outdated Show resolved Hide resolved

ntd force-pushed the bicker branch from c3a380f to d110c2a Compare May 18, 2024 07:27

ntd added 13 commits May 21, 2024 22:44

bicker_ser: Bicker UPSes based on PSZ-1063

a56281b

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 <[email protected]>

bicker_ser: add shutdown support

021472d

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 <[email protected]>

data/driver.list.in: add bicker_ser based UPSes

3ae6b50

Signed-off-by: Nicola Fontana <[email protected]>

bicker_ser: add big-endian platform support

0b53475

Signed-off-by: Nicola Fontana <[email protected]>

bicker_ser: use length macros to increase expressiveness

6538f2f

Signed-off-by: Nicola Fontana <[email protected]>

bicker_ser: add manpage

67ffe25

Signed-off-by: Nicola Fontana <[email protected]>

NEWS.adoc: announce bicker_ser driver [networkupstools#2318]

ffd42d1

Signed-off-by: Nicola Fontana <[email protected]>

bicker_ser: add more state variables

eaddb34

Identification fields are now queried from the device. Also "battery.charge" is now set properly. Signed-off-by: Nicola Fontana <[email protected]>

bicker_ser: use variable for low battery limit

c659e31

Signed-off-by: Nicola Fontana <[email protected]>

bicker_ser: add parameter support

a4c3767

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 <[email protected]>

ntd force-pushed the bicker branch from d110c2a to a4c3767 Compare May 21, 2024 20:44

ntd added 2 commits May 22, 2024 10:12

bicker_ser: fix string dstate setting

c9fed62

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 <[email protected]>

jimklimov requested changes May 22, 2024

View reviewed changes

ntd added 7 commits May 22, 2024 11:57

bicker_ser: cosmetic fixes

beb26a6

Signed-off-by: Nicola Fontana <[email protected]>

bicker_ser: use uint8_t for bytes instead of char

0c80ece

`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 <[email protected]>

bicker_ser: add sanity checks to bicker_set

fca9957

These devices seem to be picky on what you use to set parameters. Signed-off-by: Nicola Fontana <[email protected]>

jimklimov approved these changes May 24, 2024

View reviewed changes

jimklimov merged commit 9fad1df into networkupstools:master May 24, 2024
30 checks passed

jimklimov changed the title ~~WIP: Bicker serial driver~~ Add Bicker serial driver May 24, 2024

jimklimov added the enhancement label May 24, 2024

jimklimov added this to the 2.8.3 milestone May 24, 2024

jimklimov added the feature label May 24, 2024

ntd deleted the bicker branch May 24, 2024 13:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Bicker serial driver #2448

Add Bicker serial driver #2448

ntd commented May 17, 2024

jimklimov left a comment

ntd commented May 18, 2024

ntd commented May 18, 2024

ntd commented May 21, 2024

jimklimov commented May 22, 2024

ntd commented May 22, 2024 •

edited

jimklimov commented May 22, 2024

jimklimov commented May 22, 2024 •

edited

ntd commented May 22, 2024

jimklimov left a comment

jimklimov May 22, 2024 •

edited

ntd May 22, 2024 •

edited

ntd May 22, 2024 •

edited

jimklimov commented May 22, 2024

ntd commented May 22, 2024

ntd commented May 23, 2024

jimklimov commented May 24, 2024

jimklimov left a comment

jimklimov May 24, 2024

ntd May 24, 2024

Add Bicker serial driver #2448

Add Bicker serial driver #2448

Conversation

ntd commented May 17, 2024

jimklimov left a comment

Choose a reason for hiding this comment

ntd commented May 18, 2024

ntd commented May 18, 2024

ntd commented May 21, 2024

jimklimov commented May 22, 2024

ntd commented May 22, 2024 • edited

jimklimov commented May 22, 2024

jimklimov commented May 22, 2024 • edited

ntd commented May 22, 2024

jimklimov left a comment

Choose a reason for hiding this comment

jimklimov May 22, 2024 • edited

Choose a reason for hiding this comment

ntd May 22, 2024 • edited

Choose a reason for hiding this comment

ntd May 22, 2024 • edited

Choose a reason for hiding this comment

jimklimov commented May 22, 2024

ntd commented May 22, 2024

ntd commented May 23, 2024

jimklimov commented May 24, 2024

jimklimov left a comment

Choose a reason for hiding this comment

jimklimov May 24, 2024

Choose a reason for hiding this comment

ntd May 24, 2024

Choose a reason for hiding this comment

ntd commented May 22, 2024 •

edited

jimklimov commented May 22, 2024 •

edited

jimklimov May 22, 2024 •

edited

ntd May 22, 2024 •

edited

ntd May 22, 2024 •

edited