[Nut-upsdev] [nut-commits] svn commit r1765 - in trunk: . drivers man

[Arjen de Korte]

Citeren Arnaud Quette <aquette.dev at gmail.com>:

> 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