Properly determine libusb read size for large reports (Fixes #274)#728
Properly determine libusb read size for large reports (Fixes #274)#728sudobash1 wants to merge 9 commits intolibusb:masterfrom
Conversation
068fd62 to
52c6cd9
Compare
|
PR updated to remove different signedness comparison compiler warning when compiling with |
|
Thank you for finally taking care of this longstanding bug. |
| Requires an opened device with *claimed interface*. | ||
|
|
||
| The return value is the size on success and -1 on failure. */ | ||
| static size_t get_max_input_size(libusb_device_handle *handle, int interface_num, uint16_t expected_report_descriptor_size) |
There was a problem hiding this comment.
This function seems to be the functional the same as InputReportByteLength from https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/hidpi/ns-hidpi-_hidp_caps on Windows.
We have a lot of test data for the Windows unit test, which stores the result of this function:
together with the real ReportDescriptor https://github.com/libusb/hidapi/blob/master/windows/test/data/045E_02FF_0005_0001_real.rpt_desc
What do you think about creating a unit test for the new get_max_input_size function using the _real.rpt_desc files as input and compare the result with the value stored in the .pp_data.
There was a problem hiding this comment.
Nice! That does look like a great dataset. I'll see if I can figure a clean way to make a unit test, and see how it fares with that data.
Currently the get_max_input_size is a static function, but I need a way to use it externally in the unit test. It seems that the windows library has some extensions prefixed with winapi. Should I rename the get max function to hid_libusbapi_get_max_input_report_size and mark it HID_API_EXPORT_CALL?
I will also need to change how it gets the report descriptor, but that won't be a problem.
There was a problem hiding this comment.
Making it with HID_API_EXPORT_CALL would make it a public (at least on binary level) function, which is undesirable, if it is required for tests only. Maybe export it only when building unit-tests etc. or try to avoid it by using static linking or smth else (or have it in an internal header file?)
There was a problem hiding this comment.
Have a look at the Windows unit test ( https://github.com/libusb/hidapi/tree/master/windows/test ) for the report descriptor reconstructor. There we had exactly the same challenges, hid_winapi_descriptor_reconstruct_pp_data was the internal function to test there.
There was a problem hiding this comment.
@JoergAtGithub I did actually look at hid_winapi_descriptor_reconstruct_pp_data, and it is marked as HID_API_EXPORT_CALL in the header file:
hidapi/windows/hidapi_winapi.h
Line 68 in 0ab6c14
Although, now that I am looking, it is not marked in the C file:
WRT @Youw's suggestion. I could simplify the function a bit so that it doesn't depend on anything in hid.c and then move it to hid_max_input_report.h. If that is acceptable, it is probably the easiest solution.
There was a problem hiding this comment.
hid_winapi_descriptor_reconstruct_pp_data, and it is marked as HID_API_EXPORT_CALL in the header file
Right - that is a public API function. That is also the reason why there is a winapi prefix in the name.
it is not marked in the C file
Not required, if it is marked in the header.
I could simplify the function a bit so that it doesn't depend on anything in hid.c and then move it to hid_max_input_report.h. If that is acceptable, it is probably the easiest solution.
Yes, sounds like simples solution for now. Go for it.
There was a problem hiding this comment.
I have a test, but it isn't passing. It could be that I am misunderstanding the Windows HID structures. I have only a vague understanding of them.
For example, in the 046A_0011_0006_0001 test data, it gives a max input size of 9 bytes:
hidapi/windows/test/data/046A_0011_0006_0001.pp_data
Lines 16 to 19 in 0ab6c14
But two input caps (cap[0] and cap[1]) are:
hidapi/windows/test/data/046A_0011_0006_0001.pp_data
Lines 37 to 38 in 0ab6c14
hidapi/windows/test/data/046A_0011_0006_0001.pp_data
Lines 83 to 84 in 0ab6c14
According to the documentation I read, the report size is just BitSize * ReportCount (plus a byte for the report number I assume).
This gives report sizes of (1*8)/8 + 1 = 2 and (8*6)/8 + 1 = 7. So I don't know where the ReportByteLength = 9 value is coming from.
Do either of you know what I am missing here?
|
BTW, there is a typo here. |
|
I put The InputReport contains:
|
There is something missing, as this is autogenerated by pp_data_dump, I guess there was something in the manufacturer_string , that pp_data_dump couldn't handle. Could you please open a dedicated issue for this, as this is unrelated to this PR. |
|
2708264 to
3710895
Compare
| if(NOT EXISTS "${TEST_PP_DATA}") | ||
| message(FATAL_ERROR "Missing '${TEST_PP_DATA}' file for '${TEST_CASE}' test case") | ||
| endif() | ||
| set(TEST_EXPECTED_DESCRIPTOR "${CMAKE_CURRENT_LIST_DIR}/../../windows/test/data/${TEST_CASE}_expected.rpt_desc") |
There was a problem hiding this comment.
You need to parse the _real.rpt_desc, not the _expected.rpt_desc. The real is what is dumped from the device, and the expected is what the Windows ReportDescriptor-Reconstructor generates out of the .pp_data.
There was a problem hiding this comment.
I see. I assumed that the real and expected should contain equivalent reports.
The _real.rpt_desc files do not follow a consistent format, so should I parse them manually (should be pretty doable with a bit of regex) into new files and add them to the repo?
There was a problem hiding this comment.
Maybe you can put them into https://eleccelerator.com/usbdescreqparser/ to unify the format. That makes them also human readable.
| if (report_count < 0 || report_size < 0) { | ||
| /* We are missing size or count. That isn't good. */ | ||
| return 0; |
There was a problem hiding this comment.
| if (report_count < 0 || report_size < 0) { | |
| /* We are missing size or count. That isn't good. */ | |
| return 0; | |
| if (report_count < 0 || report_size < 0) { | |
| /* We are missing size or count. That isn't good. */ | |
| return -1; |
This would mean a corrupt ReportDescriptor
There was a problem hiding this comment.
I wasn't consistent with whether I was using size_t or ssize_t. In my last commit, I settled on size_t, but perhaps I should change it back to ssize_t so that we can more clearly differentiate between errors and a zero value (if there are no feature reports, it will return 0, which is not an error).
|
Thanks @JoergAtGithub for walking me through that. I was misreading the descriptor. I have added tests for libusb using the same data as the windows tests. And I extended the functionality of the libusb method to be able to calculate the maximum output and feature report sizes as well. This has no functional use currently, but it lets us run three times as many tests since the pp_data files have all three max sizes available. |
Independent of this PR, I think it would generally make sense to store these 3 values in the device structure. On Windows we would have to use the values |
|
|
||
| if(WIN32) | ||
| # so far only Windows has tests | ||
| if(WIN32 OR HIDAPI_WITH_LIBUSB) |
| set(CMAKE_VERSION_SUPPORTS_ENVIRONMENT_MODIFICATION "3.22") | ||
|
|
||
| foreach(TEST_CASE ${HID_DESCRIPTOR_RECONSTRUCT_TEST_CASES}) | ||
| set(TEST_PP_DATA "${CMAKE_CURRENT_LIST_DIR}/../../windows/test/data/${TEST_CASE}.pp_data") |
There was a problem hiding this comment.
maybe move the test data to <root>/test_data ?
Youw
left a comment
There was a problem hiding this comment.
I don't have a strong opinion on test implementation (I trust @JoergAtGithub on this one).
libusb implementation seem fine.
Lets make sure it runs with CI on Github Actions and we're good to go here.
Lets continue here: #731 |
|
This PR does not seem to work. Test device is the same as the one used in Issue #274. The FW is a mod of Jan Axelson's FX2HID example and codes are included in the following #274 comment. I can reproduce the issue reported in #274 with hidapi git libusb backend, hidraw backend is okay. With this PR, no change in hidraw backend behavior, which is good. But the libusb backend fix is not working. |
|
HID Report Descriptor. hidtest is okay. |
|
If I have time, I'll try to address @JoergAtGithub and @Youw's comments on Monday. @mcuee I tried feeding the report descriptor that you sent into I did notice a typo in your test. When you ran |
|
Let me try again. |
|
Same problem under FreeBSD 14.1 Release. This is with a physical machine, Chuwi mini PC, Intel J4125 CPU, 8GB/256GB configuration. Without this PR, hidapi git has the problem mentioned in #274. But then somehow this PR does not work. |
|
If I use the original fx2hid example (loop back of two bytes output report and input report), it seems to me this PR works fine. But that just means this PR has no regression. |
|
@sudobash1 great news! I will be able to provide soonest feedback around Monday sorry if you are ready before and want to go ahead then go ahead :-) I am waiting for the board and will have to see how to set things up but then I will be able to help you in other cases :-) Is there any sort of bad-usb device out there that could provide us various test cases from hardware point of view (i.e. simulate bad descriptors, strange transfers, etc)? |
|
Other than testing, @Youw will need to review the codes and then approve this PR. But now the first thing is to fix the CI build -- two builds failed because of the same issue. |
44328f8 to
e2391b6
Compare
Oops. I thought I had fixed that. I guess not. I have pushed e2391b6 to address compiler warnings. It compiles cleanly with |
Yes, CI builds are okay now. |
|
Ah, it looks like there a few comments from @Youw that I need to address too. Most significant is probably updating builds.yml (to make sure the tests are enabled). I'll look through this on Friday. |
|
It would be good if it could clarify in the comments what exactly the “report size” returned by get_max_report_size comprises.
In other code of mine, I introduced the unofficial term payload-size for the report size without the ReportID-byte. There I add 1 depending on the context where it is used. There I've also a boolean that indicates, if the device uses ReportIDs. |
|
First test under Windows -- it seems the reported write/read length is not consistent with native API, one byte higher. Native Windows HID backend-- wrote 129 bytes, read 128 bytes But this is getting close. I will test under Linux and FreeBSD later. Dirty fix to be able to build under Windows. hidapitester Makefile hack and binaries (against hidapi git and this PR, Windows native backend and libusb backend). |
This comment was marked as outdated.
This comment was marked as outdated.
|
Sorry for the delay, extremely overloaded, I have the CY7C680 board, working on FreeBSD. Had to power on board with button pressed to be able to flash firmware: Now it shows as: |
|
I realised I forgot to update my previously flawed comment - the issue was obviously on my end. |
Co-authored-by: Ihor Dutchak <ihor.youw@gmail.com>
a4c4da9 to
e19ba07
Compare
|
Just wondering if you have any updates on this one. I think it is pretty close now. |
|
Just update the branch to trigger a new build. It will be good to move this PR forward. May need more testing and reviews. |
|
Just wondering if it is worth asking Github Copilot to review this PR. It seems to be pretty close to be okay. |
There was a problem hiding this comment.
Coming back to this after dropping it for a while — did a spec/kernel cross-check pass and a code re-read. Leaving hardware testing to those with the devices.
Algorithm validation
get_max_report_size matches HID 1.11 (§6.2.2.7, §8 for Report ID semantics) and is structurally identical to Linux's report-descriptor parsing in drivers/hid/hid-core.c:
hid_add_field():report->size += parser->global.report_size * parser->global.report_count;hid_register_report():if (id != 0) report_enum->numbered = 1;- On receive, one byte is consumed as the ID prefix iff
numbered— which is exactly the+ report_id_usedsemantic here.
The FX2 128-byte fix is the subtle one: the hang wasn't the size math, it was the unconditional +1 making the buffer a non-multiple of wMaxPacketSize and suppressing the libusb short-packet terminator. Good catch on making it conditional on the Report ID tag actually being used.
Reusing windows/test/data/*.pp_data for a cross-backend parity test is a nice consistency proof.
Suggestions before merge
- Scope of "Fixes #274" — this resolves the long-report truncation aspect, but #274 also asks for Full-Speed misconfiguration detection / reporting to the app. Consider softening to "Partially fixes" or adding a note.
- Platform length asymmetry that @mcuee observed on Windows (libusb
wrote 130 / read 129vs native HIDwrote 129 / read 128) — worth a sentence in the PR body pointing to #731 so users don't re-file it as a regression. - Inline nits on specific lines below.
Algorithm + spec alignment LGTM on the core change.
Extended version:
Comparison with the spec and other implementations
HID 1.11 spec (§6.2.2.7 & §5.6 / §8): "If a Report ID tag is used anywhere in Report descriptor, all data reports for the device are preceded by a single byte ID field." Report ID = 0 is a placeholder meaning "no report IDs". Reports are sized as Σ(report_count × report_size) bits, rounded up to bytes. The PR's algorithm and the conditional +report_id_used byte match the spec exactly.
Windows (HIDP_CAPS): InputReportByteLength/OutputReportByteLength/FeatureReportByteLength — these always include the Report-ID byte (value 0 when unused). That's the source of the read/write-length platform asymmetry and is exactly what follow-up issue #731 is about. The PR's test case subtracts 1 byte from these values when ReportID == 0x00 to reconcile with the libusb convention (see parse_max_input_report_size in max_input_report_size_test.c).
Linux kernel (drivers/hid/hid-core.c): Uses the same algorithm. hid_add_field() accumulates report->size += parser->global.report_size * parser->global.report_count. In hid_register_report(), if (id != 0) report_enum->numbered = 1;. In hid_report_raw_event, when numbered is true the first data byte is consumed as the ID (cdata++; csize--;). Size is bounded by max_buffer_size - 1 bits when numbered. This is structurally identical to what the PR does — so the PR's approach is directly validated by the reference kernel implementation.
hidapi/linux (hidraw backend): Avoids parsing entirely — delegates to the kernel: read(dev->device_handle, data, length) with user-supplied length, because the kernel has already sized the per-report buffer. That's why hidraw "just works" for the 128-byte FX2 firmware and why the libusb backend is the outlier.
Strengths
Correct algorithm. Matches HID 1.11 and Linux kernel's parsing exactly, including the subtle conditional Report-ID byte.
Good test coverage via reuse of 25 real-world Windows pp_data fixtures — this is a nice cross-backend consistency proof.
Safe fallback to input_ep_max_packet_size when the descriptor is corrupt/unparseable — no regression risk for devices already working.
Fixes the classic "exact multiple of max packet size hangs" libusb gotcha — the conditional +1 was the critical insight; without it devices with payloads that happen to be multiples of 64 would hang forever waiting for a short packet.
Remaining concerns worth flagging on review
Re-read of report descriptor. hidapi_initialize_device now calls hid_get_report_descriptor_libusb during init (new control transfer on every open). This is a second descriptor fetch in addition to any caller-driven hid_get_report_descriptor(). Consider caching.
HID_API_MAX_REPORT_DESCRIPTOR_SIZE buffer on stack (4096 bytes typically). libusb/hid.c:1314 in the diff allocates this on the stack — acceptable, but worth noting for embedded targets.
cur_size reset timing. In the parser, cur_size is reset on each Report ID encounter. For descriptors that interleave INPUT/OUTPUT items between Report ID changes this is correct; just note that the max_size comparison happens only at Report-ID boundaries and at the end — which matches the spec semantics where each Report ID opens a fresh report namespace.
Platform-length asymmetry (mcuee's Windows observation) is intentionally out of scope — tracked in #731. Worth linking from the PR description.
PR description still says "Fixes #274" but #274 has broader asks (detection of USB Full-Speed misconfiguration, speed reporting to app). This PR fixes the "long reports get truncated" aspect; the speed/error-reporting aspect remains open.
Typo in test comment (.dpt_desc → .rpt_desc) in libusb/test/CMakeLists.txt.
Bottom line
The PR is technically sound, aligned with HID 1.11 and the Linux kernel reference, and has been hardware-validated on Linux, FreeBSD and Android. The earlier "doesn't work" reports were a spec-compliance bug in the Report-ID handling that sudobash1 fixed. Primary blockers now are administrative: final Youw approval + merge, plus perhaps addressing the minor concerns above. The broader cross-platform length-consistency question is correctly deferred to #731.
| # <name>_expected.rpt_desc - reconstructed HID Report Descriptor out of .pp_data file; | ||
| # | ||
| # (Non-required by test): | ||
| # <name>_real.dpt_desc - the original report rescriptor used to create a test case; |
There was a problem hiding this comment.
Two typos: dpt_desc → rpt_desc, and rescriptor → descriptor.
|
|
||
| dev->report_descriptor_size = get_report_descriptor_size_from_interface_descriptors(intf_desc); | ||
|
|
||
| unsigned char report_descriptor[HID_API_MAX_REPORT_DESCRIPTOR_SIZE]; |
There was a problem hiding this comment.
Two minor things in this block:
- 4 KiB on the stack (
HID_API_MAX_REPORT_DESCRIPTOR_SIZE) — fine for desktop, may be tight on some embedded targets. Worth heap-allocating to match the rest of the backend. - This is now a second descriptor fetch per
hid_open(one here, and another whenever a caller later invokeshid_get_report_descriptor()). Consider caching the bytes onhid_deviceso both consumers share them.
There was a problem hiding this comment.
1 - totally dismiss, 4KB is fine on any platform where HIDAPI shall be considered to be used
|
This is the result of the review by Claude Opus 4.7 (locally). |
I think this statement is not correct. The USB standard specifies the max read size per speed category. With this PR every read size should work and therefore every USB speed. |
Currently the libusb version of hidapi simply reads up to
wMaxPacketSizebytes as the report. This is problematic when reports are longer thanwMaxPacketSize. The current behavior will split that report up.The proper solution is to review the report descriptor to find the longest input report and use that as the length of the
libusb_fill_interrupt_transferbuffer. (Note: there is no need to manually get multiple USB packets and concatenate them together to fit the report length. USB already handles that for us.)This will still work for HID devices when some input reports are shorter than others. The HID device will just send a short packet terminator and libusb will give us the shorter buffer.
The substance of these changes is in the
get_max_input_sizemethod. It uses the same basic report descriptor parsing asget_usage. I considered changing the code so that there could be shared parsing code, but I decided that was overkill for this.Fixes #274