search

bluekitchen / btstack

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

hid_subevent_report_get_report is off by 1 byte #602

Closed Slion closed 3 months ago

Slion commented 3 months ago

Using hid_host_demo on Raspberry Pi Pico W with SDK 1.5.1. Connect to a classic Bluetooth device of your choice, preferably one you know the format of the reports. Notice there is one byte missing at the end of all reports coming through hid_host_handle_interrupt_report. You can printf_hexdump(packet, size); from HID_SUBEVENT_REPORT to find your missing byte. The length is ok but the start of the buffer is off by one AFAICT.

To work around it you need to:

// +1 For report offset as otherwise it starts at the report type for whatever reason and then there is a missing byte at the end 
hid_host_handle_interrupt_report(hid_subevent_report_get_report(packet)+1, hid_subevent_report_get_report_len(packet));

Also modify hid_host_handle_interrupt_report implementation for it to expect the report to start with the report ID rather than the 0xA1 report type. It's interesting that this function was implemented to take that bug into account not realizing there is one missing byte.

I reckon it could just be a bug in hid_subevent_report_get_report but someone with more intimate knowledge of HID should really take a look at that issue. I doubt it is Pico W specific.

mringwal commented 3 months ago

Thanks for pointing that out. While you're correct that the report type doesn't need to be part of the returned report, we only corrected the report length to avoid issues for existing code. Report len was corrected in d9d6144

Slion commented 3 months ago

There can't be that many people using this otherwise you would have received bug reports already. People who really make use of it would have had to workaround it one way or another and that means they will need to adjust their code anyway when the fix is released, myself included. So I guess you may as well do what makes more sense from the specifications point of view.

My understanding is that this report type byte is not supposed to be part of the report itself. The first byte of the report is the report id, if any, then comes the report data. But again I'm not exactly an expert 😁

You could also leave that function broken as it is and deprecate it.Then introduce a new fixed function.

Slion commented 3 months ago

I'm not exactly sure what's that A1 byte either cause the report type is suppose to take values ranging from 1 to 3. So it could be report type from bits 0 to 3 and something else from bits 4 to 7. I'm guessing that's part of the Bluetooth HID profile specs rather than the USB HID specs. I should really take a look at those.

mringwal commented 3 months ago

The A1 is the Bluetooth HID Protocol Header. The upper 4 bit are the message type, which is DATA for 0xA and the lower indicate the report type (as you've guessed correctly).

While you're right that this header byte isn't part of the HID spec, it doesn't hurt and we'll leave it there for now.

Most existing HID Host apps most likely deal with HID keyboards where there's an array of keycodes. With the last byte missing, they are only missing e.g. the 6th key that's pressed down in parallel - so even a CTRL-ALT-Delete (if that's the correct one for Windows) or Alt-Command-Escape on mac is working.