Opened 3 years ago

Closed 3 years ago

#16647 closed bug (fixed)

Problem with HIDReportItem and padding

Reported by: lt_henry Owned by: nobody
Priority: normal Milestone: R1/beta4
Component: Drivers/Input/HID/USB Version: R1/beta2
Keywords: Cc: mmlr
Blocked By: Blocking: #17189
Platform: All

Description (last modified by pulkomandy)

So it seems that some usb keyboards are not working on Haiku because they send keystroke reports using a bitmap instead of the more common way of using arrays. Perhaps, I should have opened a bug ticket with this... Instead, I have ended trying to fix myself, and I almost done, but I am blocked by a bug.

The report descriptor, from what I understand, defines a 8 bit constant padding and a 112 bit with current keystroke status. Whenever I press key A, with usage 0x04, I get an I, with usage 0x0C, and so on with B,C,D,... So it seems that padding is not being honored.

disclaimer: I am not a usb hid guru and I may have done some wrong assumptions

Correct explanation

The report defines two successive usage pages (E0 to E7 and then 00 to 67). The HID spec says these should each consume the corresponding number of bits from the report, but our parser instead erases the values from the first usage page with the values from the second one.

This results in all key sending incorrect key codes. The key that should be E0 is now 00, E1 is 01, ... E7 is 07, and then 00 becomes 08.

Our HID parser must be adjusted to handle multiple usage ranges and correctly assign the keycodes.

Attachments (1)

report-5.txt (3.2 KB ) - added by lt_henry 3 years ago.
Report descriptor with human friendly comments

Download all attachments as: .zip

Change History (14)

by lt_henry, 3 years ago

Attachment: report-5.txt added

Report descriptor with human friendly comments

comment:1 by waddlesplash, 3 years ago

Cc: mmlr added
Component: Drivers/USBDrivers/Input/USB-HID
Keywords: usb_hid removed
Owner: changed from mmlr to nobody

comment:2 by pulkomandy, 3 years ago

Your report *starts* with 112 bits (14 bytes) reporting the keystrokes, and then 8 bits of padding.

So I don't see how this could be a problem with the padding, which comes after the data?

comment:3 by lt_henry, 3 years ago

Your report *starts* with 112 bits (14 bytes) reporting the keystrokes

Your are right, I understood the other way.

So I don't see how this could be a problem with the padding, which comes after the data?

What I can say for sure is that Items detected by keystrokes are +8 from the expected Usage ID, when I thought padding was first, that made sense.

So, could be a device sending bad formatted reports?

comment:4 by pulkomandy, 3 years ago

Maybe, but if it works in Linux, we must be doing something wrong, or they have "quirks" we can copy from in their USB driver. It would be surprising for a keyboard to be this obviously wrong however, so I assume we are doing something wrong.

I see this in the report;

10	0x19, 0xE0,        //   Usage Minimum (0xE0)
11	0x29, 0xE7,        //   Usage Maximum (0xE7)
12	0x19, 0x00,        //   Usage Minimum (0x00)
13	0x29, 0x67,        //   Usage Maximum (0x67)

That could mean the first 8 keys are E0-E7, and then there are the keys from 0x00 to 0x67. If we ignore the first minimum/maximum pair, that could also explain the offset of 8 keys?

I didn't know you could have multiple ranges like this, and did not check the USB HID spec to see if that's indeed allowed. But it would explain the offset, at least.

comment:5 by lt_henry, 3 years ago

Maybe, but if it works in Linux, we must be doing something wrong, or they have "quirks" we can copy

For my keyboard model there is only a HID_QUIRK_NO_INIT_REPORTS flag, I've followed down into the rabbit hole until this:

https://elixir.bootlin.com/linux/v4.1/source/drivers/hid/usbhid/hid-core.c#L749

This flag avoids initializing reports, but I am not sure what it really does. They set this flag for a lot of devices... Doesn't Haiku need this?

Anyway, that's another story, thing is, there is no custom cooked hid report for this device in Linux.

I see this in the report

From my poor hid understanding, It look like a bad descriptor (but not broken). So following a bit your theory this is what I've founded:

E0-E7 usages map to modifier keys (ctrl,alt,...), so It seems your theory is correct, because pressing this keys generates 0x00-0x08 usages on KeyboardProtocolHanlder.

Right Control key (0xE4) strokes generates A (0x04) events right now.

However, I am not qualified enough to say if this is a bug in Haiku, or a weird standard hid interpretation that Linux knows how to workaround.

comment:6 by pulkomandy, 3 years ago

So I looked into the HID spec (https://usb.org/sites/default/files/hid1_11.pdf):

Because button bitmaps and arrays can represent multiple buttons or switches with a single item, it may be useful to assign multiple usages to a Main item. Usage Minimum specifies the usage to be associated with the first unassociated control in the array or bitmap. Usage Maximum specifies the end of the range of usage values to be associated with item elements. The following example illustrates how this could be used for a 105-key keyboard.

So, yes, it seems multiple minimum/maximum ranges can be specified and they will "consume" the corresponding number of buttons.

But our code does not seem to do this here: https://git.haiku-os.org/haiku/tree/src/add-ons/kernel/drivers/input/usb_hid/HIDParser.cpp#n313

It seems we need to store more than just a single minimum and maximum in the "local state", so that each range defined by a minimum/maximum adds itself to the state, instead of replacing the previous defined ranges. And then adjust HIDCollection to handle such states properly.

comment:7 by lt_henry, 3 years ago

So, perhaps we can close this ticket as it is no longer valid and a new one should be created with that specific bug/missing feature.

comment:8 by pulkomandy, 3 years ago

I can edit the ticket description, that's simpler

comment:9 by pulkomandy, 3 years ago

Description: modified (diff)

comment:10 by lt_henry, 3 years ago

Blocking: 17189 added

comment:11 by pulkomandy, 3 years ago

Is it fixed with hrev55413, or are there furhter steps needed?

comment:12 by lt_henry, 3 years ago

No, in theory this bug is already solved and no more steps are needed.

comment:13 by pulkomandy, 3 years ago

Milestone: UnscheduledR1/beta4
Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.