-
Notifications
You must be signed in to change notification settings - Fork 441
Properly determine libusb read size for large reports (Fixes #274) #728
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this, I'll perform a thorrow review a bit later.
@Be-ing can you confirm it fixes the issue with long reports too?
libusb/hid.c
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
pp_data->caps_info[0]->ReportByteLength = 16 |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
int HID_API_EXPORT_CALL hid_winapi_descriptor_reconstruct_pp_data(void *hidp_preparsed_data, unsigned char *buf, size_t buf_size); |
Although, now that I am looking, it is not marked in the C file:
int hid_winapi_descriptor_reconstruct_pp_data(void *preparsed_data, unsigned char *buf, size_t buf_size) |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
pp_data->caps_info[0]->FirstCap = 0 | |
pp_data->caps_info[0]->LastCap = 2 | |
pp_data->caps_info[0]->NumberOfCaps = 2 | |
pp_data->caps_info[0]->ReportByteLength = 9 |
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
pp_data->cap[0]->BitSize = 1 | |
pp_data->cap[0]->ReportCount = 8 |
hidapi/windows/test/data/046A_0011_0006_0001.pp_data
Lines 83 to 84 in 0ab6c14
pp_data->cap[1]->BitSize = 8 | |
pp_data->cap[1]->ReportCount = 6 |
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. |
|
52c6cd9
to
2708264
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can put them into https://eleccelerator.com/usbdescreqparser/ to unify the format. That makes them also human readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense!
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 |
@@ -70,8 +70,8 @@ if(HIDAPI_ENABLE_ASAN) | |||
endif() | |||
endif() | |||
|
|||
if(WIN32) | |||
# so far only Windows has tests | |||
if(WIN32 OR HIDAPI_WITH_LIBUSB) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also need to update builds.yml
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe move the test data to <root>/test_data
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
Would you please run your test device against this PR for hidspi libusb backend under your FreeBSD (as well as other OS you have access to)? Thanks. |
@mcuee builds okay, could you please provide test steps, its a long thread :-P |
Great. Test steps: using the normal libusb backend of HIDAPI and this PR.
I have mentioned my test device in the comments above (EZ-USB FX2LP) but you can use other test device if you can. |
Thanks. I finally got my EZ-USB FX2LP dev board from ebay. I've never worked with these before. How do I flash the hex file onto them? This is as far as I got:
No errors, but nothing seems to happen. No hid device shows up & there is no USB re-enumeration. (Fedora 42, Linux kernel 6.14.11) |
I also ordered EZ-USB FX2LP will arrive next week will report back test results :-) |
You are almost there. Just need to add FX --> EZ-USB FX
|
Fedora 42's fxload (kind of original version of fxload) is a bit different and it does not support FX3. But it does support FX2LP.
|
Interestingly Arch Linux AUR fxload is actually packing libusb project's fxload example. https://aur.archlinux.org/packages/fxload |
Interestingly there is a Fedora request to package libusb's fxload example (probably as libusb-fxload) but it is not implemented in the end. Take note libusb's fxload does not support EEPROM. So the suggeston from @jwrdegoede is good in the above discussion. libusb's fxload example cannot replace the original fxload, it is better to have both packages in Fedora. Initial commit in libusb project FX3 support: |
@mcuee Thanks, I have it running now. I am experiencing the same things you are. I added a quick
So, this PR is computing what I would have expected for the max input report size from the device descriptor. But I notice with hidraw, it is only reading 128 bytes (not 129). So as a quick kludge test, I removed the
It looks like the input report doesn't use a report number, so there are only 128 bytes sent. Right now, I'm guessing that the issue is something to do with the fact that 128 is an even multiple of the packet size, so USB isn't sending any short packet. I've got a cold right now, so my head has been a bit fuzzy. I will have to revisit this at a later time with a clearer head. tl;dr It looks like the code in this PR to parse the report descriptor is working, but libusb isn't reading a 128 byte message when it expected a 129-byte one. Anybody have more insight into this? Cheers! 🍻 |
For interupt transfers, which are send without requests from the device to the computer, the ReportId byte ist only prepend to the report data, if the device uses ReportIDs. |
I think you are close to find the root cause of the issue. https://github.com/sudobash1/hidapi/blob/libusb_input_size/libusb/hid.c
It is kind of a complicated topic. Please refer to the following discussion. Good comments in that issue by @JoergAtGithub
You can also refer to my comment.
|
f56c2d7
to
40132e9
Compare
Thank you @JoergAtGithub and @mcuee. That is clearly the issue. I reviewed the HID spec again and indeed it says:
The Windows behavior when there is no ReportID was tripping me up. I've pushed a simple commit to address this, and the FX2LP 128-byte report test works fine for me now. The tests also needed to be fixed for the cases when there is no ReportID ( Am I forgetting anything, or does that finish everything outstanding on this PR (provided CI completes and @mcuee's & @cederom's tests pass)? |
@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.
|
c06470e
to
44328f8
Compare
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). |
Co-authored-by: Ihor Dutchak <[email protected]>
Currently the libusb version of hidapi simply reads up to
wMaxPacketSize
bytes 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_transfer
buffer. (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_size
method. 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