[Nut-upsdev] Small fixes needed for Riello driver

[Charles Lepple]

On Jul 5, 2013, at 8:45 AM, Elio Parisi wrote:

> But the diff file that I'll now produce is related to the 3857 revision, so is also
> inclusive of previous changes (i.e. BYTE to uint8_t). Is it Ok for you?

I can probably make that work, but in the long term, I would recommend either switching to Git, or if you prefer to stay with SVN, creating a new checkout from the GitHub SVN URL:

   https://github.com/networkupstools/nut/branches/riello-fixes

(You can copy your modified files over into the new SVN checkout.)

While looking at some of the boolean variables, I noticed that there might be some other issues.

1) Semantically, a "true" value in C is non-zero. In reading through the code quickly, I would assume that a variable "xyzOK" means that the value for "xyz" is OK/valid if the variable is non-zero. However, in drivers/riello_ser.c, line 749, several dstate values are set to zero if getnominalOK is true.

I hate to sound picky, but this is open source - if you want anyone to contribute improvements back to the code, they need to be able to quickly understand what is going on.

2) I don't know that it makes sense to set the aforementioned values to zero if they cannot be read. If there is an issue reading them, they should not be set at all (or deleted, if this happens every update cycle). However, do the nominal values need to be read every update cycle? USB should register a disconnect if someone swaps out the UPS, and for serial, the driver should probably be killed and restarted.

3) In riello_usb.c lines 1052-1057, the format string was changed from "%lu" to "%ul". This seems incorrect, as it would print a number followed by "l".

When I asked about whether the driver was tested, I also meant looking at the values to ensure that they make sense. (Changing types is less likely to crash the driver, and more likely to introduce subtle inaccuracies.) In addition, it would also be useful to check what happens if the serial or USB cable is removed. That would exercise a few more code paths.

-- 
Charles Lepple
clepple at gmail