[Nut-upsdev] belkin-hid:

[Arjen de Korte]

Peter Selinger wrote:

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

That's what I understood from the comments in usbhid-ups.c.

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

Clear. I was just wondering why this was causing any problems. Before
(nut-2.0.4) all was well, but this changed with upgrading (nut-2.0.5). I
couldn't find a reason for that in belkin-hid.c or usbhid-ups.c. Now I
understand, the caching mechanism wasn't available until the latter was
released. Bummer! ;-)

> The real source of Justin's bug is probably that the logic of the
> "special case" treatment in usbhid-ups.c is broken.

Well, it will probably catch the reports received via the interrupt
pipeline, but...

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

...it would fail to prevent them from being cached. So we would probably
need to duplicate this code in libhid.c too to prevent this.

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

I disagree, this would probably need to be done in libhid.c. I don't
like that, since it is putting vendor/device specific code in something
that should be vendor/device independent. Imagine that other vendors
make an equal mess of their protocols... :-(

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

It looks to me that ignoring this report altogether, would have the
least impact on the non-Belkin specific code. So from a coding point of
view, I would prefer using 'UPS.BELKINStatus.BELKINBatteryStatus'
instead and ignore the faulty descriptor in belkin-hid.c. We could also
remove the 'special case' code from usbhid-ups.c in that case.

> None of these solutions are pretty.

Indeed, but some are less ugly than others. :-)

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

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

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

Well, the only reason for me to suggest to change this is that
currently, the value from 'UPS.BELKINStatus.BELKINBatteryStatus' uses
the 'discharging' bit, while we also use the
'UPS.PowerSummary.ACPresent' report. I assume that during battery tests
(while AC is present) the 'discharging' bit will be set. In that case,
the UPS would (erroneously) indicate that the power went out.

There was a discussion about a similar issue a while ago, where I
questioned the need for a CHRG and DISCHRG state. I (erroneously) argued
that you could assume that the power was out when a UPS indicates that
it was discharging and that this duplicated the OB state. How wrong
could I be. Likewise, you can't assume the power is out when it is no
longer charging (the batteries may be full). Therefor, I would prefer to
use bit 0 from 'UPS.BELKINStatus.BELKINPowerStatus' instead of the
inverted bit 5 from 'UPS.BELKINStatus.BELKINBatteryStatus'.

Best regards, Arjen