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>