fix: fix get hid device interface number is always 0 on macos 13.3 #530
No reviewers
Labels
No labels
3rdparty
Android
API
Author does not respond
bsd
bug
build system/CI
ci-virtual-device
CMake
Contributions Welcome
Core
dependencies
device-specific
documentation
don't_merge
duplicate
enhancement
Example
Haiku
hidraw
hotplug
info required
invalid
libusb
macOS
Misc
Not an issue
question
Release
Test
Test GUI
Windows
wontfix
No milestone
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gh-libusb/hidapi!530
Loading…
Reference in a new issue
No description provided.
Delete branch "mac13.3"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
fix get hid device interface number is always 0 on macos 13.3
@ -552,6 +552,36 @@ static struct hid_device_info *create_device_info_with_usage(IOHIDDeviceRef dev,cur_dev->interface_number = -1;}if (cur_dev->interface_number == -1 || cur_dev->interface_number == 0) {cur_dev->interface_number == 0is a valid interface numberit doesn't look right to consider it for a fallback
@ -555,0 +563,4 @@if (res == KERN_SUCCESS)temp_dev_path = strdup(temp_dev_path_str);elsetemp_dev_path = strdup("");temp_dev_pathnever released - that's a resourse leak@ -555,0 +567,4 @@}if (temp_dev_path) {const char* match_prefix_string = "Interface@";no documentation guarantees that the interface number is going to be a part of the Path
even if it is functional - this is a workaround
I'd want to search for a better solution
@ -552,6 +552,36 @@ static struct hid_device_info *create_device_info_with_usage(IOHIDDeviceRef dev,cur_dev->interface_number = -1;}if (cur_dev->interface_number == -1 || cur_dev->interface_number == 0) {I cause this problem: my device interface number is not 0. but the old code return 0 on macos 13.3. on macos13.2,the old code works fine(not 0) . so 0 may not be valid!
@ -552,6 +552,36 @@ static struct hid_device_info *create_device_info_with_usage(IOHIDDeviceRef dev,cur_dev->interface_number = -1;}if (cur_dev->interface_number == -1 || cur_dev->interface_number == 0) {maybe it is not vaid in your case, but generally the
0is a valid interface numbermaybe we need to change the
get_int_property(dev, CFSTR(kUSBInterfaceNumber));part to return-1in case of failure@ -555,0 +563,4 @@if (res == KERN_SUCCESS)temp_dev_path = strdup(temp_dev_path_str);elsetemp_dev_path = strdup("");OK,My mistake
@ -555,0 +567,4 @@}if (temp_dev_path) {const char* match_prefix_string = "Interface@";https://github.com/libusb/hidapi/discussions/527. The author of this discussion is my workmate. Now we have no other good solution to solve this problem except this one.
@ -555,0 +567,4 @@}if (temp_dev_path) {const char* match_prefix_string = "Interface@";let me investigate this
there is always a solution to find a corresponding USB/device interface
having the Location ID - that one is guaranteed to matchI agree with this one.
Left a few comments and some draft code in them.
@ -552,6 +552,36 @@ static struct hid_device_info *create_device_info_with_usage(IOHIDDeviceRef dev,cur_dev->interface_number = -1;}if (cur_dev->interface_number == -1 || cur_dev->interface_number == 0) {The indentation of the entire block is wrong. The commit indents with 4 spaces while the rest of the file indents with tabs.
Everything in the taken branch is actually dependent on
iokit_dev != MACH_PORT_NULLso it should be evaluated in the outermost conditional. Currently ifiokit_devis null the code enters the branch just to look at a char pointer that is guaranteed to beNULLanyway. So why not:@ -555,0 +556,4 @@int old_interface_number = cur_dev->interface_number;//try to Fallback to older interface number find rulesio_string_t temp_dev_path_str;char* temp_dev_path = NULL;The
char *temp_dev_pathvariable is not needed. Allocating a new string withstrdup(temp_dev_path_str)is unnecessary, andstrdup("")doubly so.io_string_t temp_dev_path_stralone is sufficient as it's an alias ofchar[512]and can be passed tostrstr().With this in mind the only conditional you need to evaluate is
res == KERN_SUCCESS, and the program flow remains identical.@ -555,0 +567,4 @@}if (temp_dev_path) {const char* match_prefix_string = "Interface@";A string literal would do just fine here.
@ -555,0 +568,4 @@if (temp_dev_path) {const char* match_prefix_string = "Interface@";char* interface_component = strstr(temp_dev_path, match_prefix_string);@ -555,0 +570,4 @@const char* match_prefix_string = "Interface@";char* interface_component = strstr(temp_dev_path, match_prefix_string);if (interface_component) {char* decimal_str = interface_component + strlen(match_prefix_string);A string literal instead of a
const char *would avoid potentially compiling in a runtime call tostrlen(), assizeof("...") - 1is equal tostrlen("...")but is guaranteed to be evaluated compile-time. Doingstrlen(s)on aconst char *s = "..."might get optimized away with some compiler flags, but it definitely won't always be.Of course a string literal would have to be typed out twice, once for
strstr()and then a second time forsizeof(). If you can stomach it you can define a temporary macro or two.Because at this point I think I've written enough comment spam I'll just post an alternative to the whole code block, with a couple of additional minor tweaks in addition to the ones I've mentioned. Haven't compiled, no guarantees that it actually works. It just might.
Just chiming in here that without a fix like this, this lib is totally broken on macOS 13.3^. The changes in this PR fixed the issue for me, and I suspect many others will have the same experience.
You may want to file a feedback to Apple as well using the link here: https://developer.apple.com/bug-reporting/. The more people send the feedbacks to Apple, it may get their attention faster and they may fix the issue faster.
Reference: https://github.com/libusb/hidapi/issues/531#issuecomment-1510034397
Since nothing has so far happened I forked this PR branch, rebased it onto current master, and added an improvement commit (for the most part the same one I pasted in the discussions here). I don't have access to a mac I could test it on, so if anyone wants to have a go please do:
https://github.com/imaami/hidapi/tree/mac13.3
Please notice this is still fundamentally the same fix, I have done nothing original except clean and simplify the implementation a little.
Thanks @imaami
Anyway, I'm still looking into a better solution.
Got my hands to upgrade my own mac pro to latest macOS (so I could reproduce it at least) and will look at it closer soon.
As for the improvement, we will start with #534
@imaami
Yes your mod is working but as @Youw mentioned that this PR is not the right fix to go with.
Closing in favor of #534.
@ -552,6 +552,36 @@ static struct hid_device_info *create_device_info_with_usage(IOHIDDeviceRef dev,cur_dev->interface_number = -1;}if (cur_dev->interface_number == -1 || cur_dev->interface_number == 0) {Actually, the width of tabs on different system is different , but the width of spaces is the same. so use spaces can croass-platform ,show consistency
@ -555,0 +570,4 @@const char* match_prefix_string = "Interface@";char* interface_component = strstr(temp_dev_path, match_prefix_string);if (interface_component) {char* decimal_str = interface_component + strlen(match_prefix_string);thanks for suggestion
@ -552,6 +552,36 @@ static struct hid_device_info *create_device_info_with_usage(IOHIDDeviceRef dev,cur_dev->interface_number = -1;}if (cur_dev->interface_number == -1 || cur_dev->interface_number == 0) {The point is that the commit is not consistent with its own surroundings. Arguments for or against tab indentation are completely irrelevant in the context of one commit that adds code to an existing file.
Attached screenshot shows what this looks like.
@ -552,6 +552,36 @@ static struct hid_device_info *create_device_info_with_usage(IOHIDDeviceRef dev,cur_dev->interface_number = -1;}if (cur_dev->interface_number == -1 || cur_dev->interface_number == 0) {if you explain like this ,I got it.
Pull request closed