bluekitchen / btstack

Dual-mode Bluetooth stack, with small memory footprint.
http://bluekitchen-gmbh.com
Other
1.74k stars 613 forks source link

HID Parser: fails to parse Tank Mouse HID preport #541

Closed ricardoquesada closed 1 year ago

ricardoquesada commented 1 year ago

Describe the bug

A clear and concise description of what the bug is.

To Reproduce

Attached test in this Pull Request: https://github.com/bluekitchen/btstack/pull/540

Expected behavior Axis X, Axis Y and Scroll Wheel usages should be received.

ricardoquesada commented 1 year ago

Adding more context:

Tank Mouse ( https://tank-mouse.com/ ) is a mouse that works Ok on PC.

When I use it with BTstack, it seems that the BTstack HID parser is not interpreting the Tank Mouse HID Descriptor correctly.

I should receive

But I only receive the buttons. I didn't have the chance to dig further, but it seems that HID parser believes that there are 3 usages, but there should more than that.

The pull request is to quickly test the bug. Not really mean to be merged since it is not complete. But feel free to merge + finish it once the fix is ready.

The HID descriptor and the HID report are included in the pull request

mringwal commented 1 year ago

Thanks for the nice preparation and the unit test. I need to work on other stuff for a while, but we'll look into it.

mringwal commented 1 year ago

Hi Ricardo

Did you look at the HID Descriptor in detail? It has the following buttons

  0x05, 0x09,        //     Usage Page (Button)
    0x19, 0x01,        //     Usage Minimum (0x01)
    0x29, 0x05,        //     Usage Maximum (0x03)
    0x15, 0x00,        //     Logical Minimum (0)
    0x25, 0x01,        //     Logical Maximum (1)
    0x75, 0x01,        //     Report Size (1)
    0x95, 0x05,        //     Report Count (5)
    0x81, 0x02,        //     Input (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position)

So, it wants to have 5 inputs with a single bit, but it also lists Usage Min = 1 and Usage Max = 3. If you update Usage Maximum to 5, the parsing works as expected. While this certainly is something that could/should be fixed in the Tank Mouse, did you find anything in the USB HID Spec about how to deal with this inconsistency? Which information should have more priority: the report count or the usage range?

ricardoquesada commented 1 year ago

TBH, I haven't read the spec... probably I should since I'd like to learn more about HID descriptor internals. But in this case, I'd take the "practical" approach, which is emulate what Linux / BlueZ / etc... are doing.

I've just tested on Linux and works Ok. ...looking at the Linux source code... trying to find the HID parser...

ricardoquesada commented 1 year ago

This is my theory:

    0x85, 0x03,        //   Report ID (3)
   ...
   //  Report Size / Report Count and Input are first declared here
    0x75, 0x01,        //     Report Size (1)
    0x95, 0x05,        //     Report Count (5)
    0x81, 0x02,        //     Input (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position)

   // But immediately after, they are re-declared.
   // Notice that it uses one array of length 3.
    0x75, 0x03,        //     Report Size (3)
    0x95, 0x01,        //     Report Count (1)
    // NOTICE IT IS AN ARRAY
    0x81, 0x01,        //     Input (Const,Array,Abs,No Wrap,Linear,Preferred State,No Null Position)

   // ... here starts the other report Id ...
    0x05, 0x01,        //     Usage Page (Generic Desktop Ctrls)
   ...

So my guess is that BTstack is getting just the first appearance of the "keys", and instead it should get the latest one ?

In any case, I tried commenting out the first appearance of report size / report count / input... and I didn't receive any "buttons". So perhaps "input array" is not supported on btstack ?

mringwal commented 1 year ago

There are 5 usages for the buttons, however with Usage Min = 1 and Max =3 which doesn't make sense - it should be either report size = 5 with usage max =5 OR report size = 3 with usage max = 3 (as any other mouse on the planet). Then there's a 3 bit array of padding (marked as const). With just 3 buttons, that's usually 5 bit to align the next one.

I didn't check the code yet, but I assume that BTstack detects the inconsistency and aborts parsing. Re-read the HID spec a bit and it makes sense to just ignore the 4. and 5. usage in the 5 bit array and just continue. I'll try to add code to handle that. It would be better if Linux/Windows etc. wouldn't accept sloppy descriptors as it would allow for less complex code...

mringwal commented 1 year ago

Fixed on develop

ricardoquesada commented 1 year ago

Thanks for the quick fix!