bthidd: Consider usage ranges when dealing with array inputs.

[rakuco]

Summary

So far, we were always using HID_USAGE() to determine the Usage ID of a
certain HID report input item. This does not work as intended if a field
is an array and the allowed usages are specified with a usage range, as
HID_USAGE() will return 0. We need to use the field value as an index in
the usage range list in this case instead.

This makes the volume keys in a Microsoft Bluetooth Mobile Keyboard
5000 be properly recognized. The relevant part of the HID report looks
like this:

0xA1, 0x01,        // Collection (Application)
0x85, 0x07,        //   Report ID (7)
0x05, 0x0C,        //   Usage Page (Consumer)
0x19, 0x00,        //   Usage Minimum (Unassigned)
0x2A, 0xFF, 0x03,  //   Usage Maximum (0x03FF)
0x95, 0x01,        //   Report Count (1)
0x75, 0x10,        //   Report Size (16)
0x15, 0x00,        //   Logical Minimum (0)
0x27, 0xFF, 0x03, 0x00, 0x00,  //   Logical Maximum (1023)
0x81, 0x00,        //   Input (Data,Array,Abs,No Wrap,Linear,Preferred
                   //   State,No Null Position)

When a key such as "volume down" is pressed, the following data is
transferred through Interrupt In:

0x07 0xEA 0x00

[rakuco]

I'd appreciate some feedback here. Should I check for overflows in the code I've added? Does it make sense that calls to hid_get_item() return a single item for an array even if the report count is > 1? The keyboard handling code that manually works around it, but I wonder if this should be done in libusbhid (and in the analogous kernel code) instead.

[emax]

please see minor inline comment. thank you for your work!

usr.sbin/bluetooth/bthidd/hid.c

182 ↗

(On Diff #4680)

can you please switch blocks within if() so it looks like if (( h & HIO_VARIABLE)) { usage = HID_USAGE(..); } else { new code; }. its easier to read this way. also usage_offset can be made local (it does not seems be used anywhere else). otherwise, looks fine.

[rakuco]

Patch updated with the if check inverted. I thought all variables were supposed to be declared at the top, thankfully it doesn't seem to be the case :-)

Is the code enough as-is or should I add some overflow checks?

[emax]

usr.sbin/bluetooth/bthidd/hid.c

176 ↗

(On Diff #4681)

please add extra set of brackets around h & HIO_VARIABLE, i.e. if ((h & HIO_VARIABLE)). this is to avoid possible warning.

can you please explain which overflow are you worrying here about?

[rakuco]

Additional ()'s added to the if check.

[emax]

looks fine to me.

[rakuco]

Weird cases that I have in mind:

• h.logical_minimum being larger than val.
• (h.usage_minimum + usage_offset) > h.usage_maximum.
• An array being defined with a usage ID instead of a usage range (all items would have the same usage, which does not make much sense to me and I haven't seen any examples of this so far).

[emax]

i don't think its worth worrying about. HID_USAGE is basically

#define HID_USAGE(u) ((u) & 0xffff)

so, if HID descriptor and/or value is wrong (i.e. out of range), nothing terrible would happen (imo).

[rakuco]

Closed by commit rS281116 (authored by @rakuco).