Stabilize path for libusb devices: #406 from signal11/hidapi

[gavincangan]

MIgrating @derekatkins 's changes from this PR: signal11/hidapi#406

Referenced by this issue: signal11/hidapi#405 in signal11/hidapi

[Youw]

please follow this document code style:
replace all spaces with tabs, and, I think, we good to go

[derekatkins]

HI. What is the status of this PR?
I guess it's not been merged??

[Youw]

soon to be merged
I'll play with it a little bit locally

[bearsh]

is there a good reason to pack two levels into a 16bit hex number instead of separating each level with a colon?

[derekatkins]

@bearsh IMHO, to make it look more like an IPv6 address? People are already used to seeing quad-hex strings, so it makes sense to keep it that way. It also shortens the string a little.

I suppose you COULD have a byte-order problem, but these strings are not portable across machines so it shouldn't matter. I find it very improbable that you have one machine that changes byte-order.

[bearsh]

@derekatkins valid reason... and in the end I don't really care as long as all levels are in the string

just some thoughts:

• make the first block 8bit hex to emphasize it's the root hub, like the linux kernel does (e.g. /sys/bus/usb/devices/3-1.1.3.4)
• if the levels are separated, it make it very easy for a human to find a device
• use a approach from the kernel but with a fixed length, e.g. 03-01.01.03.04.00.00.00.00:1d numbers a hex

[Youw]

use a approach from the kernel

This sounds like execent reasoning

[bearsh]

@Youw what's your plan on this? do you expect a change in the PR?

[Youw]

The more I think about this, the more I like the idea to have device path look as much similar to what linux kernel have as possible. Even though current PR gives an improvement, I wouldn't want this to be changed more than once.

Does anyone can reference a linux kernel implementation for this part?

[bearsh]

Does anyone can reference a linux kernel implementation for this part?

I've found following sites 'talking' about the usb sysfs interface

• https://www.kernel.org/doc/html/latest/admin-guide/abi-stable.html#symbols-under-sys-bus
• http://www.linux-usb.org/FAQ.html#i6
• https://unix.stackexchange.com/questions/297178/how-usb-bus-number-and-device-number-been-assigned/297515

the most authoritative entry may be the following line from the ABI doc (direct link):

/sys/bus/usb/devices/<busnum>-<port[.port]>…:<config num>-<interface num>/supports_autosuspend

[Youw]

Very good. I'm pretty sure that <busnum>-<port[.port]>… is exactly what we need.

[bearsh]

if it should be like <busnum>-<port[.port]>, what about the interface number? appended with with a : or - as separator?

[Youw]

Right. I answered this (my) morning in a hurry. Didn't pay attention to all the details.

I say we try to do exactly same way as Linux kernel does:
<busnum>-<port[.port]>…:<config num>-<interface num>

So we need to include configuration number/index too.

A also believe, that we shouldn't include all 7 port numbers (as max supported by USB3.0), but only as much as reported by libusb_get_port_numbers in a return value.

[derekatkins]

A also believe, that we shouldn't include all 7 port numbers (as max supported by USB3.0), but only as much as reported by libusb_get_port_numbers in a return value.

I agree with this statement. It should only display the number of ports in use.

[bearsh]

something like this?

diff --git a/hidapi/libusb/hid.c b/hidapi/libusb/hid.c
index 0b8aa1e..3d71767 100644
--- a/hidapi/libusb/hid.c
+++ b/hidapi/libusb/hid.c
@@ -484,15 +484,29 @@ err:
 	return str;
 }
 
-static char *make_path(libusb_device *dev, int interface_number)
+static char *make_path(libusb_device *dev, int interface_number, int config_number)
 {
-	char str[64];
-	snprintf(str, sizeof(str), "%04x:%04x:%02x",
-		libusb_get_bus_number(dev),
-		libusb_get_device_address(dev),
-		interface_number);
-	str[sizeof(str)-1] = '\0';
-
+	char str[64]; // max length "000-000.000.000.000.000.000.000:000.000"
+	// Note that USB3 port count limit is 7; use 8 here for alignment
+	uint8_t port_numbers[8] = {0, 0, 0, 0, 0, 0, 0, 0};
+	int num_ports = libusb_get_port_numbers(dev, port_numbers, 8);
+
+	if (num_ports > 0) {
+		int n = snprintf(str, sizeof("000-000"), "%u-%u", libusb_get_bus_number(dev), port_numbers[0]);
+		for (uint8_t i = 1; i < num_ports; i++) {
+			n += snprintf(&str[n], sizeof(".000"), ".%u", port_numbers[i]);
+		}
+		n += snprintf(&str[n], sizeof(":000.000"), ":%u.%u", (uint8_t)config_number, (uint8_t)interface_number);
+		str[n] = '\0';
+	} else {
+		// USB3.0 specs limit number of ports to 7 and buffer size here is 8
+		if (num_ports == LIBUSB_ERROR_OVERFLOW) {
+			LOG("make_path() failed. buffer overflow error\n");
+		} else {
+			LOG("make_path() failed. unknown error\n");
+		}
+		str[0] = '\0';
+	}
 	return strdup(str);
 }
 
@@ -590,7 +604,7 @@ struct hid_device_info  HID_API_EXPORT *hid_enumerate(unsigned short vendor_id,
 
 							/* Fill out the record */
 							cur_dev->next = NULL;
-							cur_dev->path = make_path(dev, interface_num);
+							cur_dev->path = make_path(dev, interface_num, conf_desc->bConfigurationValue);
 
 							res = libusb_open(dev, &handle);
 
@@ -915,7 +929,7 @@ hid_device * HID_API_EXPORT hid_open_path(const char *path)
 				const struct libusb_interface_descriptor *intf_desc;
 				intf_desc = &intf->altsetting[k];
 				if (intf_desc->bInterfaceClass == LIBUSB_CLASS_HID) {
-					char *dev_path = make_path(usb_dev, intf_desc->bInterfaceNumber);
+					char *dev_path = make_path(usb_dev, intf_desc->bInterfaceNumber, conf_desc->bConfigurationValue);
 					if (!strcmp(dev_path, path)) {
 						/* Matched Paths. Open this device */
 

[bearsh]

by the way, according to my running system it's not
<busnum>-<port[.port]>…:<config num>-<interface num>
but
<busnum>-<port[.port]>…:<config num>.<interface num> (dot instead of a hyphen between config num and interface num)

[bearsh]

I've created a new pull request for this: #291

[Youw]

Locking this thread if favour of #291