Op Fri, 28 Feb 2020 11:14:43 +0100 schreef Martin Pieuchot
<[email protected]>:
On 28/02/20(Fri) 10:02, Boudewijn Dijkstra wrote:
Op Thu, 27 Feb 2020 17:30:34 +0100 schreef Martin Pieuchot
<[email protected]>:
> On 27/02/20(Thu) 16:58, [email protected] wrote:
> > >Synopsis: boolean indicators in sensorsd.conf(5) are too cumbersome
> > >Category: system
> > >Environment:
> > System : OpenBSD 6.6
> > Details : OpenBSD 6.6 (GENERIC.MP) #372: Sat Oct 12 10:56:27
> > MDT 2019
[email protected]:/usr/src/sys/arch/amd64/compile/GENERIC.MP
> >
> > Architecture: OpenBSD.amd64
> > Machine : amd64
> > >Description:
> > Some upd(4) devices use -1 for "On" and some use 1. sysctl(8) and
> > sensorsd(8) hide this detail from the user, which makes it difficult
> > to define low and high values in sensorsd.conf(5).
>
> Which device reports "-1" for which usage? Is this from any
> specification or is it a workaround for your device?
In the misc@ thread I linked it was reported that different devices use
different values. My device happens to report -1 for "On". Given how
sensorsd.conf currently works, it would be most convenient if 0 and 1
were the only possible values.
You're rephrasing your diff in words. My question is: can there be any
drawback to this approach?
They're boolean indicators, I don't think there can be any. sensorsd(8)
is the only program in base that can use upd(4) sensors. sysctl(8)
already treats them as booleans. Obviously some people will have to
change their sensorsd.conf(5) if this goes in.
Did you check the spec?
I checked "Universal Serial Bus Usage Tables for HID Power Devices"
https://www.usb.org/sites/default/files/documents/pdcv10.pdf
For every boolean indicator it specifies two allowed values: 0 and 1.
Why is your UPS returning -1 and not 1 in this case?
No idea. It's working fine. Some further testing revealed that On==-1
(and Off==0) for all the indicators that I can easily toggle (Charging,
Discharging, ACPresent).
Is this the right place to fix the bug?
I think so. It's a violation of HID Power, not generic HID, so it should
be fixed in a place where HID Power data is (first) interpreted.
> Diff looks fine, although we could do simpler, see below.
>
> Index: upd.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/upd.c,v
> retrieving revision 1.26
> diff -u -p -r1.26 upd.c
> --- upd.c 8 Apr 2017 02:57:25 -0000 1.26
> +++ upd.c 27 Feb 2020 16:25:24 -0000
> @@ -425,7 +425,10 @@ upd_sensor_update(struct upd_softc *sc,
> }
> hdata = hid_get_data(buf, len, &sensor->hitem.loc);
> - sensor->ksensor.value = hdata * adjust;
> + if (sensor->ksensor.type == SENSOR_INDICATOR)
> + sensor->ksensor.value = hdata ? 1 : 0;
> + else
> + sensor->ksensor.value = hdata * adjust;
> sensor->ksensor.status = SENSOR_S_OK;
> sensor->ksensor.flags &= ~SENSOR_FINVALID;
Your diff is indeed simpler, but I thought it would be cleaner to not
assign 'adjust' when it's not needed.
That's an improvement indeed, but it isn't related to the bug you're
trying to fix ;)
--
Boudewijn Dijkstra
Indes-IDS B.V.
+31 345 545 535
