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

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

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

Citeren Elio Corbolante <eliocor op microdowell.com>:
>>>> 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:
For the input power and a double conversion UPS, this might be the  
case, although I doubt that you'll meet the EU harmonic content  
regulations easily with such a poor powerfactor.
> Very few UPS have a cos phi of 1.0.
Very few small (up to a kVa) UPSes have a (near) unity powerfactor.  
Many larger units have however, since for high loads you'll also have  
to pay for reactive power as well, so it pays to keep the powerfactor  
near unity.

I do expect however that this will change in the not so distant  
future, now that it is cheaper to meet harmonic content regulations by  
using active PFC. So I expect older UPS designs gradually to be  
replaced by newer ones, at least for the online models. For offline /  
line interactive devices this probably isn't an issue.
> 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.
Understood. But this only works if the value is measured on the input  
of the UPS. Usually what is expected to be reported here (at least for  
NUT), is output power (VA an W). There can never be a fixed relation  
between those in the general case.
> The output in W is 0.6 * VA
This is getting confusing. Please explain what is reported 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.
>>
>> 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.
In that case, please remove it. This is a value that is set through  
'ups.conf' and which happens to default to 2 seconds as well. There  
can be reasons to deviate from that (usually higher values to prevent  
beating a UPS to death with polls), but in that case it should be  
clearly documented why.
>>> > 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!!!
This is not the concern here. The dstate_setinfo() function reports a  
value to the upsd server. But at the time the upsdrv_shutdown()  
function runs, the server isn't running anymore, so there is no one  
listening to us anymore. Therefor, the dstate_setinfo() function must  
not be used here.
>>> > 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.
These functions where added in the docs/new-drivers.txt document in  
June 2007. The debugging functions have existed for more than five  
years already (see docs/developers.txt). The latter is mandatory  
reading for developers anyway.

Best regards, Arjen
-- 
Please keep list traffic on the list