onsdag 04 februari 2009 23:24:11 skrev Arjen de Korte: Hi Arjen, (Sorry for the bottom quote of original mail)
As I put it in the trunk I feel that I have to fix some of the code. Send a copy of the mail to Elio. I have made some changes to the code, as you suggested. But I leave it up to Elio to check it out. I don't want to make to may changes. But it is in the trunk now. regards /Kjell P.S. Made some comments in the rest of the mail. > Citeren Arnaud Quette <[email protected]>: > > Author: keyson-guest > > Date: Wed Feb 4 15:03:36 2009 > > New Revision: 1765 > > > > Log: > > - Added mocrodowell.c/h into drivers/. add man/microdowell.8. Adjusted > > drivers/Makefile.am and man/makefile.am. Adjusted the code for 2.4.0 in > > the microdowell.c. > > Besides the things already mentioned by others and the obvious > > formatting issue, just a couple of nits to pick: > > 93 static char *ErrMessages[] = { > > [...] > > > 187 } Not done anything about this as.... > > Consider returning the error message through something like > > > 124 case ERR_NO_ERROR: > > 125 return "OK"; > > instead of doing this indirectly through an index in an array of error > strings. This makes sure these don't get out of sync. > ..it depends on how this is handled. Leave this to Elio > > 547 dstate_setinfo("ups.StatusUPS", "%08lX", > > ups.StatusUPS) ; > > 548 dstate_setinfo("ups.ShortStatus", "%04X", > > ups.ShortStatus) ; > > Don't 'invent' new variables without discussing this on the > development mailinglist. Since these are probably useful only for > debugging purposes, they shouldn't be exposed to the server/clients > anyway. Changed this to debug statements. > > > 629 dstate_setinfo("ups.realpower", "%d", > > (int)((float)(p[4]*256 + p[5]) * 0.6)) ; > > This suggests a fixed relationship between apparent and active power. > This might be the case for the nominal values, but this is not true in > the general case. So don't export 'ups.realpower' unless it is truly > measured (which is not the case here). Made a comment that it is calculated. Can this be measured? > > > 645 poll_interval = 2; > > Any particular reason to hardcode this? This might surprise people > that attempt to override this value, so unless there is a real good > reason to do this, it will be confusing. I leave it in. Have you any good reason to have it hardcoded Elio? > > > 899 dstate_setinfo("ups.StatusUPS", "%08lX", > > ups.StatusUPS) ; > > 900 dstate_setinfo("ups.ShortStatus", "%04X", > > ups.ShortStatus) ; > > 905 dstate_setinfo("ups.time", "%02d:%02d:%02d", > > p[6], p[7], p[8]) ; > > The above is a waste of effort. By the time upsdrv_shutdown() runs, > the server won't be listening anymore. > Changed the status to debug messages. And removed the ups.time her, as the daemon would not get it anyway. > > 958 addvar(VAR_VALUE, "ups.delay.shutdown", "Override > > shutdown delay (120s)"); > > 959 addvar(VAR_VALUE, "ups.delay.start", "Override restart > > delay (10s)"); > > This doesn't work without using getval() somewhere in the driver. Fixed this and moved some code. This can now be set from ups.conf. And as before changed during runtime. > > > 971 ioctl(upsfd, TIOCMBIC, &rts_bit); > > 972 ioctl(upsfd, TIOCMBIC, &dtr_bit); > > We have library functions in serial.c to handle this. Use them. Used the library functions. ( Check that I set them right). _______________________________________________ Nut-upsdev mailing list [email protected] http://lists.alioth.debian.org/mailman/listinfo/nut-upsdev
