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

[Kjell Claesson]

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 <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 	}
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).