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

Fri Feb 6 15:03:18 UTC 2009

Returned to office right now... :-(
Now I will take a look to your corrections.

.    best regards
_       Elio Corbolante.

At 23:54 2009/02/05 +0100, Kjell Claesson wrote:
>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

I will check it out

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

OK

> >
> > > 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?

Yes, the relation between the VA and W is fixed as in the most UPS 
sold on the market:
Very few UPS have a cos phi of 1.0.
This particular line of UPSs has a cos phi of 0.6 and the calculation 
of the VA value is made integrating the current measurement (10 
samples for a half sine) so the result is very near to an RMS value.
The output in W is 0.6 * VA

> >
> > > 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?

No, there were no particular reason.
It can be any value but I thought that 2 seconds was reasonable.

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

The UPS timer countinue to work by itself even if the computer is 
turned off: if you need, you can program/enable up to 6 weekly 
schedules in the UPS and it will turn ON/OFF without requiring any 
attached computer to it!!!

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

OK

> > > 971                 ioctl(upsfd, TIOCMBIC, &rts_bit);
> > > 972                 ioctl(upsfd, TIOCMBIC, &dtr_bit);
> >
> > We have library functions in serial.c to handle this. Use them.

Unfortunately (even for the debugging functions) when I wrote the 
driver (if I remember correctly) there were NO official documentation 
on the OFFICIAL functions to be used in the developement of the drivers.

.     best regards
_       Elio Corbolante.

Elio Corbolante
e-mail:         eliocor at microdowell.com
         research at microdowell.com
Microdowell S.p.A.
Via dei Boschi, 2
33040 - Pradamano (UD) - Italia
Tel. +39-0432-671758
Tel. +39-0432-640142 (Assistenza Tecnica)
Fax. +39-0432-671760
<http://www.microdowell.com/>http://www.microdowell.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.alioth.debian.org/pipermail/nut-upsdev/attachments/20090206/dd1ab83b/attachment.htm 