[Nut-upsdev] R: Small fixes needed for Riello driver

[Elio Parisi]

Hi Charles,
I fixed (and tested) the source file according to your tips.
The patch are attached.

Let me know if there still something that do not reflect your guidelines.







Elio Parisi

Centro Ricerche
RPS SpA
Viale Europa, 7
37045 Legnago VR
Tel. +39 0442 635811 Fax. +39 0442 635934
Skype Id:  - Voip:
E-mail: E.Parisi at riello-ups.com
Web: www.riello-ups.com
--------------------------------------------------------------------------
Per favore non stampare questo messaggio se proprio non è
necessario
Please consider the environment before printing this e-mail
--------------------------------------------------------------------------

Chi riceve il presente  messaggio e` tenuto a
verificare se lo stesso non gli sia pervenuto per  errore.  In tal
caso e` pregato di avvisare immediatamente il mittente  e,
tenuto  conto  delle  responsabilita` connesse  all'indebito
utilizzo e/o  divulgazione  del  messaggio  e/o
delle  informazioni  in esso contenute,  voglia
cancellare  l'originale  e distruggere  le varie copie  o
stampe.

The receiver  of this message is required to check
if  he/she has received it erroneously.  If  so,  the
receiver  is  requested to immediately inform the sender and - in
consideration of the responsibilities  arising from undue use and/or
disclosure of the message  and/or  the information contained therein -
destroy the original message and any copy or printout thereof.



-----Messaggio originale-----
Da: Charles Lepple [mailto:clepple at gmail.com]
Inviato: venerdì 5 luglio 2013 17.01
A: Elio Parisi
Cc: nut-upsdev list
Oggetto: Re: [Nut-upsdev] Small fixes needed for Riello driver

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



-------------- next part --------------
A non-text attachment was scrubbed...
Name: riello.diff.gz
Type: application/x-gzip
Size: 2682 bytes
Desc: riello.diff.gz
URL: <http://lists.alioth.debian.org/pipermail/nut-upsdev/attachments/20130709/d85d0920/attachment.bin>