gh-libusb/hidapi

Watch

Fork 0

Possible regression in mac get_feature_report #60

New issue

Closed

opened 2019-08-01 10:43:32 -05:00 by DanielVanNoord · 8 comments

DanielVanNoord commented

2019-08-01 10:43:32 -05:00

(Migrated from github.com)

Prior to /pull/3 being merged get_feature_report was working identically for me on all platforms. Now the mac driver is failing as the returned buffer is different from the other platforms. My device does use report ids.
Is it possible that the behavior of OSX was changed from when this change was originally requested back in 2015?
Can anyone reproduce this issue?

Prior to /pull/3 being merged get_feature_report was working identically for me on all platforms. Now the mac driver is failing as the returned buffer is different from the other platforms. My device does use report ids. Is it possible that the behavior of OSX was changed from when this change was originally requested back in 2015? Can anyone reproduce this issue?

Youw commented

2019-08-01 10:51:11 -05:00

(Migrated from github.com)

Can you describe how different is it?
And is it different from previous release shipped with homebrew (0.8.0)?

Can you describe how different is it? And is it different from previous release shipped with homebrew (0.8.0)?

DanielVanNoord commented

2019-08-01 12:15:48 -05:00

(Migrated from github.com)

Other platforms include the rid in the (outbound) report and responded with the expected rid, bytes.

I wasn't using homebrew, I am building this myself. I was using Signal11/HidAPI (where it worked) but am trying to switch to yours as it is maintained. Once I reverted this pull everything worked as it did before.

The message that it is sending on the mac does not appear to include the report ID, so my device reports an error. If I write the rid in both the 1st and 2nd byte it sends the rid as part of the report. However I get a response on the mac of rid, rid, bytes as the mac library is writing the response out starting at the 2nd byte rather then the 1st. Prior to this change it worked correctly. Other platforms include the rid in the (outbound) report and responded with the expected rid, bytes. I wasn't using homebrew, I am building this myself. I was using Signal11/HidAPI (where it worked) but am trying to switch to yours as it is maintained. Once I reverted this pull everything worked as it did before.

Youw commented

2019-08-01 12:31:25 -05:00

(Migrated from github.com)

Documentation of IOHIDDeviceGetReport is very explicit, that report should contain only inbound report data, and that is what #3 is about.

Can you confirm, that rid is other than 0x0 ?

Documentation of [IOHIDDeviceGetReport](https://developer.apple.com/documentation/iokit/1588659-iohiddevicegetreport?language=objc) is very explicit, that `report` should contain only inbound report data, and that is what #3 is about. Can you confirm, that rid is other than 0x0 ?

DanielVanNoord commented

2019-08-01 12:46:36 -05:00

(Migrated from github.com)

I agree that it is very explicit, that's why I asked if anyone else experienced this issue. In this case the report id values that are not working are 0x14, 0x15 and 0x16.

It is possible that there is something out of spec with the device I have been using for testing. However it works just fine on all other platforms and on mac without this change.

I agree that it is very explicit, that's why I asked if anyone else experienced this issue. In this case the report id values that are not working are 0x14, 0x15 and 0x16. It is possible that there is something out of spec with the device I have been using for testing. However it works just fine on all other platforms and on mac without this change.

micolous commented

2019-08-11 03:03:53 -05:00

(Migrated from github.com)

What's happening at a protocol level (see section 8.2 of HID 1.11 spec), is that the Report ID will be removed from a response when a device doesn't use Report IDs.

This leaves some gaps – it's now up to the kernel to figure out if there's a report ID in there or not.

HID has other platform differences:

Linux's HIDIOCGFEATURE ioctl documentation says (emphasis mine):

HIDIOCGFEATURE(len): Get a Feature Report
This ioctl will request a feature report from the device using the control
endpoint. The first byte of the supplied buffer should be set to the report
number of the requested report. For devices which do not use numbered
reports, set the first byte to 0. The report will be returned starting at
the first byte of the buffer (ie: the report number is not returned).

HIDAPI does not modify the return value, and so get_feature_report with hidraw returns just the report, and never the report ID (even if specified).

I've tried to trace this through the kernel, and I think the report ID statement is actually false -- HIDIOCGFEATURE -> hidraw_get_report -> hid_hw_raw_request -> hid_device->ll_driver goes into usb_hid_driver -> usbhid_raw_request -> usbhid_get_raw_report which adds report_id if report_id=0 (otherwise, it presumes it'll get one back from the device, per protocol).
libusb (Linux/BSD) always returns the report_number, adding one in if report_id=0 (otherwise, it presuming it'll get one back from the device)
The Windows API docs say (emphasis mine):

The first byte of the buffer, which the requester uses to input a report ID or zero, is unchanged. The feature report, excluding its report ID, if report IDs are used, is returned at ((PUCHAR)Irp->MdlAddress + 1).

This would suggest Windows will always need an extra byte for the Report ID, even if not used, and this is always returned by HIDAPI.

macOS's documentation suggests that it should never return a report ID, and the patch in #3 would change it to make it like all the others. However, I don't think I have something that uses multiple feature descriptors.

Running captures on XHC20 (USB sniffing) for something with one feature descriptor on 10.14 gives me weirdness for set_feature_report(report_id + 8 bytes): Adding a non-zero report_id (0x13) sends URB_CONTROL with wValue=0x0313; wLength=9 and "completes" with data report_id + 7 bytes (truncated):

But doing a request with report_id=0 does the "complete" step without appending the report_id and truncating:

But, the more I look at this, the more that I see the API is unclear in the first instance. I'm of the opinion that returning a report_id is essentially irrelevant, because it was already passed in the API in the first place.

Chromium's mac implementation of this may be a useful reference

I think the best way to solve this is to explicitly break compatibility with all callers, with a new method signature that makes intent clearer:

int hid_get_feature_report(hid_device *dev, unsigned char report_id, unsigned char* buffer, size_t length);

report_id is the Report ID to get a feature report for. This may be 0.
buf is a buffer that HIDAPI can write to, or read from. The returned report always starts at byte 0 of that buffer.
length is the exact number of bytes to request from the device.

What is needed here is XHC20 (USB) captures from your software and device running with and without PR #3 on macOS. I suspect there is some other weirdness going on here. You'll need to bring up XHC20 with sudo ifconfig XHC20 up before you can capture it with Wireshark on macOS.

Then compare it to other platforms with the same code.

What's happening at a protocol level ([see section 8.2 of HID 1.11 spec](https://www.usb.org/sites/default/files/documents/hid1_11.pdf)), is that the Report ID will be removed from a response when a device doesn't use Report IDs. This leaves some gaps – it's now up to the kernel to figure out if there's a report ID in there or not. HID has other platform differences: * [Linux's `HIDIOCGFEATURE` ioctl documentation says](https://www.kernel.org/doc/Documentation/hid/hidraw.txt) (emphasis mine): > HIDIOCGFEATURE(len): Get a Feature Report > This ioctl will request a feature report from the device using the control > endpoint. The first byte of the supplied buffer should be set to the report > number of the requested report. For devices which do not use numbered > reports, set the first byte to 0. **The report will be returned starting at > the first byte of the buffer (ie: the report number is not returned).** [HIDAPI does not modify the return value](https://github.com/libusb/hidapi/blob/7da5cc91fc0d2dbe4df4f08cd31f6ca1a262418f/linux/hid.c#L755), and so `get_feature_report` with `hidraw` returns just the report, and **never** the report ID (even if specified). I've tried to trace this through the kernel, and I think the report ID statement is actually false -- [`HIDIOCGFEATURE`](https://elixir.bootlin.com/linux/v5.2.8/source/drivers/hid/hidraw.c#L424) -> [`hidraw_get_report`](https://elixir.bootlin.com/linux/v5.2.8/source/drivers/hid/hidraw.c#L228) -> [`hid_hw_raw_request`](https://elixir.bootlin.com/linux/v5.2.8/source/include/linux/hid.h#L1079) -> [hid_device->ll_driver goes into `usb_hid_driver`](https://elixir.bootlin.com/linux/v5.2.8/source/drivers/hid/usbhid/hid-core.c#L1294) -> [`usbhid_raw_request`](https://elixir.bootlin.com/linux/v5.2.8/source/drivers/hid/usbhid/hid-core.c#L1257) -> [`usbhid_get_raw_report`](https://elixir.bootlin.com/linux/v5.2.8/source/drivers/hid/usbhid/hid-core.c#L860) which adds `report_id` if `report_id=0` (otherwise, it presumes it'll get one back from the device, per protocol). * [libusb (Linux/BSD) always returns the report_number](https://github.com/libusb/hidapi/blob/651150d8b5ae78c9e1d5a1a30c486e50e5d53950/libusb/hid.c#L1206-L1227), adding one in if `report_id=0` (otherwise, it presuming it'll get one back from the device) * [The Windows API docs say](https://docs.microsoft.com/en-au/windows-hardware/drivers/ddi/content/hidclass/ni-hidclass-ioctl_hid_get_feature) (emphasis mine): > The first byte of the buffer, which the requester uses to input a report ID or zero, is unchanged. The feature report, **excluding its report ID, if report IDs are used,** is returned at **`((PUCHAR)Irp->MdlAddress + 1)`**. This would suggest Windows will always need an extra byte for the Report ID, even if not used, and [this is always returned by HIDAPI](https://github.com/libusb/hidapi/blob/7da5cc91fc0d2dbe4df4f08cd31f6ca1a262418f/windows/hid.c#L789-L815). macOS's documentation suggests that it should never return a report ID, and the patch in #3 would change it to make it like all the others. However, I don't think I have something that uses multiple feature descriptors. Running captures on `XHC20` (USB sniffing) for something with **one** feature descriptor on 10.14 gives me weirdness for `set_feature_report(report_id + 8 bytes)`: Adding a non-zero `report_id (0x13)` sends `URB_CONTROL` with `wValue=0x0313; wLength=9` and "completes" with data `report_id + 7 bytes (truncated)`: ![Screen Shot 2019-08-11 at 17 23 42](https://user-images.githubusercontent.com/246847/62831069-ce8ef800-bc5c-11e9-962e-476bd3d00dbe.png) ![Screen Shot 2019-08-11 at 17 24 24](https://user-images.githubusercontent.com/246847/62831086-101fa300-bc5d-11e9-9483-c556c68115de.png) But doing a request with `report_id=0` does the "complete" step without appending the `report_id` and truncating: ![Screen Shot 2019-08-11 at 17 27 25](https://user-images.githubusercontent.com/246847/62831114-6db3ef80-bc5d-11e9-801b-294bef219368.png) ![Screen Shot 2019-08-11 at 17 27 42](https://user-images.githubusercontent.com/246847/62831115-6db3ef80-bc5d-11e9-92f6-b581d057586c.png) But, the more I look at this, the more that I see the API is unclear in the first instance. I'm of the opinion that returning a `report_id` is essentially irrelevant, because it was already passed in the API in the first place. [Chromium's mac implementation of this may be a useful reference](https://chromium.googlesource.com/chromium/chromium/+/refs/heads/trunk/device/hid/hid_connection_mac.cc) I think the best way to solve this is to _explicitly break_ compatibility with all callers, with a new method signature that makes intent clearer: ```c int hid_get_feature_report(hid_device *dev, unsigned char report_id, unsigned char* buffer, size_t length); ``` * `report_id` is the Report ID to get a feature report for. This may be `0`. * `buf` is a buffer that HIDAPI can write to, or read from. The returned report _always_ starts at byte 0 of that buffer. * `length` is the exact number of bytes to request from the device. What is needed here is XHC20 (USB) captures from your software and device running with and without PR #3 on macOS. I suspect there is some other weirdness going on here. You'll need to bring up XHC20 with `sudo ifconfig XHC20 up` before you can capture it with Wireshark on macOS. Then compare it to other platforms with the same code.

Youw commented

2019-08-11 14:10:43 -05:00

(Migrated from github.com)

and I think the report ID statement is actually false

yeah, looks like that documentation isn't right (even though it was written by the same person, as hidapi itself)

int hid_get_feature_report(hid_device dev, unsigned char report_id, unsigned char buffer, size_t length);

this is questionable solution:

it will require creating new continues buffer in Windows/linux/libusb implementations, to copy Report ID before the data
it still doesn't not give a complete answer what is the correct macOS implementation - before or after #3 (or should be it same as for hid_send_feature_report)

> and I think the report ID statement is actually false yeah, looks like [that](https://www.kernel.org/doc/Documentation/hid/hidraw.txt) documentation isn't right (even though it was written by the same person, as hidapi itself) > int hid_get_feature_report(hid_device *dev, unsigned char report_id, unsigned char* buffer, size_t length); this is questionable solution: 1) it will require creating new continues buffer in Windows/linux/libusb implementations, to copy Report ID before the data 2) it still doesn't not give a complete answer what is the correct macOS implementation - before or after #3 (or should be it same as for [hid_send_feature_report](https://github.com/libusb/hidapi/blob/651150d8b5ae78c9e1d5a1a30c486e50e5d53950/mac/hid.c#L774))

Youw commented

2019-08-11 17:36:02 -05:00

(Migrated from github.com)

I did my best to understand Chromium's implementation if HID Get/SetFuature report, and so far I come to a conclusion, that IOHIDDeviceGetReport's 4-th parameter report, which is: Pointer to preallocated buffer in which to copy inbound report data. will contain a Report ID as its first byte, but only if device is using numbered reports.

To properly know, whether device is using numbered reports or not, it would require parsing device's descriptor and searching if there defined at least one Report ID. (And that is exactly what Chromium's implementation does (e.g. here and here), and I believe that is the right way to do it).

So far, hidapi makes a simpler assumption, aka: "device uses numbered reports, if Report ID is non-zero", so simplest fix I see here, is to make the same check for get_feature_report as we do for hid_send_feature_report, and adjust IOHIDDeviceGetReport's arguments conditionally.

Even though it looks like a proper fix to me, I don't have a devices to verify, and it probably may break existing application on macOS, that are already using builds before #3 or after (since patch #3 was applied to homebrew's formula for some while).

My personal observation: I have one HID device (and the application), that uses numbered reports, and #3 broke that application on macOS (I'm still using a local copy of hidapi for that one with a few patches on my own for now).
Having conditional argument change will not brake it though.

I did my best to understand [Chromium's implementation](https://chromium.googlesource.com/chromium/src.git/+/35f714b3deb9580047208ba73423d9c20ea8dd4a/services/device/hid/hid_connection_mac.cc) if HID Get/SetFuature report, and so far I come to a conclusion, that `IOHIDDeviceGetReport`'s 4-th parameter `report`, which [is](https://developer.apple.com/documentation/iokit/1588659-iohiddevicegetreport?language=objc): `Pointer to preallocated buffer in which to copy inbound report data.` will contain a Report ID as its first byte, but only if device is using numbered reports. To properly know, whether device is using numbered reports or not, it would require parsing device's descriptor and searching if there defined at least one Report ID. (And that is exactly what Chromium's implementation does (e.g. [here](https://chromium.googlesource.com/chromium/src.git/+/35f714b3deb9580047208ba73423d9c20ea8dd4a/services/device/hid/hid_device_info.cc#52) and [here](https://chromium.googlesource.com/chromium/src.git/+/35f714b3deb9580047208ba73423d9c20ea8dd4a/services/device/hid/hid_connection_mac.cc#111)), and I believe that is the right way to do it). So far, `hidapi` makes a simpler assumption, aka: "device uses numbered reports, if Report ID is non-zero", so simplest fix I see here, is to make the same check for `get_feature_report` as we do for [hid_send_feature_report](https://github.com/libusb/hidapi/blob/651150d8b5ae78c9e1d5a1a30c486e50e5d53950/mac/hid.c#L774), and adjust `IOHIDDeviceGetReport`'s arguments _conditionally_. Even though it looks like a proper fix to me, I don't have a devices to verify, and it probably may break existing application on macOS, that are already using builds before #3 or after (since patch #3 was applied to homebrew's formula for some while). --- My personal observation: I have one HID device (and the application), that uses _numbered_ reports, and #3 broke that application on macOS (I'm still using a local copy of hidapi for that one with a few patches on my own for now). Having conditional argument change will not brake it though.

Youw commented

2019-09-24 05:35:27 -05:00

(Migrated from github.com)