Skip to content

fix: fix get hid device interface number is always 0 on macos 13.3 #530

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

fix: fix get hid device interface number is always 0 on macos 13.3 #530

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions mac/hid.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cur_dev->interface_number == 0 is a valid interface number
it doesn't look right to consider it for a fallback

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it is not vaid in your case, but generally the 0 is a valid interface number
maybe we need to change the get_int_property(dev, CFSTR(kUSBInterfaceNumber)); part to return -1 in case of failure

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indentation of the entire block is wrong. The commit indents with 4 spaces while the rest of the file indents with tabs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Screenshot_20230525-074350~2

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you explain like this ,I got it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything in the taken branch is actually dependent on iokit_dev != MACH_PORT_NULL so it should be evaluated in the outermost conditional. Currently if iokit_dev is null the code enters the branch just to look at a char pointer that is guaranteed to be NULL anyway. So why not:

	if (iokit_dev != MACH_PORT_NULL &&
	    (cur_dev->interface_number == -1 || cur_dev->interface_number == 0)) {

int old_interface_number = cur_dev->interface_number;
//try to Fallback to older interface number find rules
io_string_t temp_dev_path_str;
char* temp_dev_path = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The char *temp_dev_path variable is not needed. Allocating a new string with strdup(temp_dev_path_str) is unnecessary, and strdup("") doubly so. io_string_t temp_dev_path_str alone is sufficient as it's an alias of char[512] and can be passed to strstr().

With this in mind the only conditional you need to evaluate is res == KERN_SUCCESS, and the program flow remains identical.

	if (iokit_dev != MACH_PORT_NULL &&
	    (cur_dev->interface_number == -1 || cur_dev->interface_number == 0)) {
		io_string_t temp_dev_path_str;
		temp_dev_path_str[0] = '\0';

		/* Fill in the path (IOService plane) */
		res = IORegistryEntryGetPath(iokit_dev, kIOServicePlane, temp_dev_path_str);
		if (res == KERN_SUCCESS) {
			char *interface_component = strstr(temp_dev_path_str,  // ...

/* Fill in the path (IOService plane) */
if (iokit_dev != MACH_PORT_NULL) {
res = IORegistryEntryGetPath(iokit_dev, kIOServicePlane, temp_dev_path_str);
if (res == KERN_SUCCESS)
temp_dev_path = strdup(temp_dev_path_str);
else
temp_dev_path = strdup("");
}

if (temp_dev_path) {
const char* match_prefix_string = "Interface@";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no documentation guarantees that the interface number is going to be a part of the Path

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#527. The author of this discussion is my workmate. Now we have no other good solution to solve this problem except this one.

Copy link
Member

@Youw Youw Apr 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have no other good solution

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 match

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A string literal would do just fine here.

char* interface_component = strstr(temp_dev_path, match_prefix_string);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

			char *interface_component = strstr(temp_dev_path_str, "Interface@");

if (interface_component) {
char* decimal_str = interface_component + strlen(match_prefix_string);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A string literal instead of a const char * would avoid potentially compiling in a runtime call to strlen(), as sizeof("...") - 1 is equal to strlen("...") but is guaranteed to be evaluated compile-time. Doing strlen(s) on a const 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 for sizeof(). 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.

	if (iokit_dev != MACH_PORT_NULL &&
	    (cur_dev->interface_number == -1 || cur_dev->interface_number == 0)) {
		io_string_t temp_dev_path_str;
		temp_dev_path_str[0] = '\0';

		/* Fill in the path (IOService plane) */
		res = IORegistryEntryGetPath(iokit_dev, kIOServicePlane, temp_dev_path_str);
		if (res == KERN_SUCCESS) {
			#define match_prefix_str "Interface@"
			#define match_prefix_len (sizeof(match_prefix_str) - 1)

			char *interface_component = strstr(temp_dev_path_str, match_prefix_str);
			if (interface_component) {
				char *decimal_str = interface_component + match_prefix_len;
				if (decimal_str[0]) {
					char *endptr = decimal_str;
					int interface_number = strtol(decimal_str, &endptr, 10);
					if (endptr != decimal_str) {
						/* Parsing succeeded, update the interface number. */
						cur_dev->interface_number = interface_number;
					}
				}
			}

			#undef match_prefix_len
			#undef match_prefix_str
		}
	}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for suggestion

char* endptr = NULL;
cur_dev->interface_number = strtol(decimal_str, &endptr, 10);
if (endptr == decimal_str) {
/* The parsing failed. Set interface_number to old_interface_number. */
cur_dev->interface_number = old_interface_number;
}
}
free(temp_dev_path);
}
}

/* Bus Type */
transport_prop = IOHIDDeviceGetProperty(dev, CFSTR(kIOHIDTransportKey));

Expand Down