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: [email protected] 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:[email protected]] 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@gmail
riello.diff.gz
Description: riello.diff.gz
_______________________________________________ Nut-upsdev mailing list [email protected] http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/nut-upsdev
