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 <[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
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: [email protected]
[email protected]
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
_______________________________________________
Nut-upsdev mailing list
[email protected]
http://lists.alioth.debian.org/mailman/listinfo/nut-upsdev
