[Nut-upsdev] belkin-hid:

[Peter Selinger]

Arjen de Korte wrote:
> 
> Peter,
> 
> Apparently, some Belkin UPSes still have problems with this. When we
> receive this report through the interrupt pipeline it is ignored (the
> special case in usbhid-ups.c) and even after that it looks like there
> are still some problems with it:
>  http://lists.alioth.debian.org/pipermail/nut-upsuser/2007-July/003014.html

Hi Arjen,

here's the information I have. On my own Belkin (which is a 050D/0980,
F6C800-UNV), I have observed the bug that is documented in the
"special case" in usbhid-ups.c. Namely, report 3d (decimal 61) shows
"low battery" when sent via the interrupt pipeline, but not when
requested as a feature report. The same is true for the log files that
Justin Piszcz sent. Viz:

$ grep 3D nut-2.0.4-4-debug.log
Report : (8 bytes) => 3D 00 00 00 00 00 00 00                         
Report : (8 bytes) => 3D 00 00 00 00 00 00 00                         
Report : (8 bytes) => 3D 00 04 00 00 00 00 00                         
Report : (8 bytes) => 3D 00 04 00 00 00 00 00                         
Notification: (2 bytes) => 3D 01                                           
Notification: (2 bytes) => 3D 01                                           
Report : (8 bytes) => 3D 00 04 00 00 00 00 00                         
Notification: (2 bytes) => 3D 01                                           
Notification: (2 bytes) => 3D 01                                           

$ grep 3d nut-2.0.5-3+b1-debug.log
8d b1 02 85 3c 09 d0 75 08 95 01 15 00 26 ff 00 b1 82 09 d0 81 82 85 3d 09
Report[r]: (2 bytes) => 3d 00
Notification: (2 bytes) => 3d 01
Report[i]: (2 bytes) => 3d 01
Notification: (2 bytes) => 3d 01
Report[i]: (2 bytes) => 3d 01
Report[r]: (2 bytes) => 3d 00
Notification: (2 bytes) => 3d 01
Report[i]: (2 bytes) => 3d 01
Notification: (2 bytes) => 3d 01
Report[i]: (2 bytes) => 3d 01
Report[r]: (2 bytes) => 3d 00

Note that, although the debug format has changed between versions, the
substance remains: on the interrupt pipeline, the device sends "3d 01"
instead of "3d 00", with the latter being the correct value. 

The real source of Justin's bug is probably that the logic of the
"special case" treatment in usbhid-ups.c is broken. When an interrupt
report arrives, it is filed to a report cache (in HIDGetEvents, which
is called at usbhid-ups.c:562), and then afterwards the value is
ignored in usbhid-ups.c:583. The problem might be that the incorrect
value, while not used immediately, is still stored in the cache; so if
a feature request for this value is done before the cache expires, the
incorrect value might still be used. This would happen intermittently.

> Wouldn't it be an option to just to ignore this completely and instead
> use bit 2 from 'UPS.BELKINStatus.BELKINBatteryStatus'? It looks like the
> battery low status is duplicated there.

The simplest solution is to move the "special case" code to another
place, ignoring the faulty report completely. Another possible
solution is to have a separate cache for interrupt transfers than for
regular transfers. Finally, one could ignore the buggy report
altogether as you suggest, and use
UPS.BELKINStatus.BELKINBatteryStatus instead (which doesn't have an
interrupt transfer).

None of these solutions are pretty. Before anyone starts complaining
about how NUT is such a messy piece of software, I should preemptively
emphasize that the real cause for this bug is Belkin's faulty firmware.
We can only work around their bugs as best as we can.

> Another thing that puzzles me, is why we use two ways to set the online
> status of the UPS (through 'UPS.PowerSummary.ACPresent' and bit 5 of
> 'UPS.BELKINStatus.BELKINBatteryStatus'). Is this because some models
> only support the latter and not the first? Could we also use bit 0 from
> 'UPS.BELKINStatus.BELKINPowerStatus' which indicates directly (according
> to the comments) the AC status, instead of inverting the discharging bit
> from 'UPS.BELKINStatus.BELKINBatteryStatus'?

I have no idea why this is duplicated. I figure the reason is indeed
that perhaps some models don't have both. Since this does not seem to
break anything, I would be reluctant to change it without cause. 

-- Peter