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 }
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.
> 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.
> 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).
> 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.
> 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.
> 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.
> 971 ioctl(upsfd, TIOCMBIC, &rts_bit);
> 972 ioctl(upsfd, TIOCMBIC, &dtr_bit);
We have library functions in serial.c to handle this. Use them.
Best regards, Arjen
--
Please keep list traffic on the list
_______________________________________________
Nut-upsdev mailing list
[email protected]
http://lists.alioth.debian.org/mailman/listinfo/nut-upsdev
