[Nut-upsdev] belkin-hid:

[Peter Selinger]

Arjen de Korte wrote:
>
> >> Personally, I feel that ignoring the faulty report in belkin-hid.c would
> >> be the method of choice, provided that the low battery status is
> >> available in 'UPS.BELKINStatus.BELKINBatteryStatus'.
> > Yes, that would work. Except that I have never tested that
> > UPS.BELKINStatus.BELKINBatteryStatus shows the status correctly, at
> > least on the hardware that I have. I would need to generate a low
> > battery event to test this.
>
> Since these status bits are documented in belkin-hid.c, I (maybe wrongly)
> assumed this was the result of reverse engineering the protocol used by
> the Belkin software.

I actually obtained this information from three sources: (1) reverse
engineering the device, (2) reverse engineering Belkin's buggy Windows
driver, (3) reverse engineering Belkin's buggy Linux driver. Moreover,
I obtained the status bits from the serial protocol and assumed, with
some testing of course, that they carry over to the USB protocol as
expected. More detailed documentation of the status bits, with
discussion of how some of the information was obtained, is at:

http://www.mathstat.dal.ca/~selinger/ups/belkin-universal-ups.html
(Section 5.4, see registers 22 and 23).

> So yes, since this is one of the most (if not the
> most) critical parameter we need to read from the UPS, this would be a
> requirement. If we not already know this works on all Belkin units, I
> withdraw my statement that ignoring the faulty report at all times would
> be the easiest solution. We certainly don't want to break installations
> that previously worked.

Here, for current and future reference, are the results of today's
test with my F6C800-UNV device.

Action				reg 22	reg 23	beeper	   reg 2e  reg 3c

initial status: UPS load off	8020	10		   00  	   01
turn UPS load on		8000	10		   00	   01
pull plug			8005	20	slow	   01	   00
battery low			8005	24	fast	   ??	   00
battery empty and load auto-off	a001	40		   00	   00
power returns			8000	10		   00	   01
initiate battery test		8000	20	slow/fast  01	   01
battery test finished		8000	10		   00	   01
manually turn UPS load off	8020	10		   00	   01

Here:
register 22 is UPS.BELKINStatus.BELKINPowerStatus,
register 23 is UPS.BELKINStatus.BELKINBatteryStatus,
register 2e is UPS.PowerSummary.Discharging
register 3c is UPS.PowerSummary.ACPresent

The value of ?? wasn't read due to a too long polling interval.

Indeed, as you guessed:

- UPS.PowerSummary.ACPresent seems to duplicate (the negation of) bit 0
  of UPS.BELKINStatus.BELKINPowerStatus ("ac failure"),

- UPS.PowerSummary.Discharging duplicates bit 5 of
  UPS.BELKINStatus.BELKINBatteryStatus ("discharging")

- UPS.BELKINStatus.BELKINBatteryStatus correctly indicates the "Low
  battery" condition (and we should use it).

Also, as you pointed out, "discharging" and "ac failure" are not the
same thing during a test, so it's a bug to map both to OL.

I am confused by the question of how best to compute NUT's OL
flag. You argue that ACPresent is more accurately than inverting
Discharging. I thought that NUT has three mutually exclusive states,
of which at least one applies at any give time:

OL - the load is powered by wall power
OB - the load is powered by battery
OFF - the load is not powered

In this case, during a battery test, the status should be OB,
regardless of whether AC is present or not, because the status is
about how the load is powered. So inverting the "discharging" bit
seems the more correct thing.

However, all the other usbhid-ups subdrivers map ACPresent to OL. This
is probably how it got duplicated in the belkin subdriver. Is this a bug? 

Moreover, the OFF flag is not computed correctly by the belkin-hid
subdriver at the moment. Correct would be:

OL  = (reg22 & 0x0021) == 0x0000
OB  = (reg22 & 0x0021) == 0x0001
OFF = (reg22 & 0x0021) == 0x0020

The remaining case, (reg22 & 0x0021) == 0x0021, is presumably
inconsistent and could be mapped to OB or OFF.

Is my reasoning correct?

> > What I really want is a more general hook to work around
> > vendor-specific bugs.
> >
> > For example, some, but not all, Tripp-Lite devices incorrectly
> > multiply their battery voltage by 10. (Actually, the bug is in the
> > report descriptor, which reports incorrect units for battery voltage
> > for some devices. The actual reports are consistent across devices, I
> > think).  I have worked around this by dividing the voltage by 10 for
> > *all* Tripp-Lite devices, which now causes the value to be broken on
> > devices that happen to be free of this bug.
> >
> > What I would like to do is to have a generic vendor_init hook in
> > *-hid.c, which would detect vendor-specific bugs in a vendor-specific
> > way, and compensate accordingly. This should be called just after the
> > report descriptor has been obtained and parsed. The (unparsed raw)
> > report descriptor can also be used to identify specific buggy devices,
> > particularly where the bug has been fixed in some versions of
> > otherwise identical devices.  In the Belkin case, one would compensate
> > by simply removing the offending report from the report descriptor. In
> > the Tripp-Lite case, the buggy physical unit could be replaced by the
> > correct one.
>
> +1. That would *much* better and provide the needed flexibility.

Currently no time to do it, unfortunately.

-- Peter