[Nut-upsdev] Driver for Powercom BNT/KIN/IMP/IMD series

[Michael Tokarev]

Charles Lepple wrote:
> On Jan 9, 2008 5:08 AM, Alexey Sidorov <alex at reutman.ru> wrote:
>> Alexey Sidorov пишет:
>>> renew http://www.reutman.ru/files/powercom.tar.bz2
>>> - fix detection of KIN-2200AP (was can detected as KIN-2500AP)
>>> - add patch for drivers/Makefile.* ( add -lm)
>>> - add patch for data/driver.list
>>> - add patch for man/powercom.8
>>>
>> attached powercom.tar.bz2
> 
> Thanks, committed to the SVN trunk as r1236. Sorry for the delay.

Ughrm... That driver is very poorly written and needs quite
some work before it will be acceptable... ;)  I already sent
some notes to Alexey, but he didn't reply.  Here are some
notes:

o code like this is everywhere:

        if ( types[type].name=="BNT") ...

 it works only with recent GCC which places identical string
 literals into the same place in memory.  There's no word in
 C standards that this construct will work or not - the behavior
 is undefined.

o variables defined in powercom.h file.  (By the way, why .h
  files are used here in the first place?  E.g. powercom.h is
  only included by powercom.c - ditto for many other pairs).

o obvious error:

        if ( types[type].name=="BNT" || types[type].name=="KIN"){
                statINV=raw_data[STATUS_A] & ONLINE;
                statAVR=raw_data[STATUS_A] & AVR_ON;
                statAVRMode=raw_data[STATUS_A] & AVR_MODE;  <====
        }
        if ( types[type].name=="BNT"){
                if (statINV==0) {
                        if (statAVR==0){
                                tmp=2.2*raw_data[OUTPUT_VOLTAGE]-24;
                        } else {
                                if (statAVRMode==1)
                                    ^^^^^^^^^^^^^^
                                        tmp=(2.2*raw_data[OUTPUT_VOLTAGE]-24)*31/27;
                                else
                                        tmp=(2.22*raw_data[OUTPUT_VOLTAGE]-24)*27/31;
                        }

  AVR_MODE is a constant, =8, so statAVRMode will either be 0 or 8,
  but not 1.

o the whole logic - calculating voltages etc - seems to be quite
  suspicious, -- needs some explanations, probably excerpts from
  the original docs.

There were several more points, but I don't remember which ones
offhand...

/mjt