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

2019-11-13T12:28:58-06:00

gavincangan commented

2019-11-13 12:28:58 -06:00

(Migrated from github.com)

MIgrating @derekatkins 's changes from this PR: https://github.com/signal11/hidapi/pull/406

Referenced by this issue: https://github.com/signal11/hidapi/issues/405 in signal11/hidapi

MIgrating @derekatkins 's changes from this PR: https://github.com/signal11/hidapi/pull/406 Referenced by this issue: https://github.com/signal11/hidapi/issues/405 in signal11/hidapi

Qbicz (Migrated from github.com) reviewed

2019-11-13 12:28:58 -06:00

Youw (Migrated from github.com) reviewed

2019-11-15 09:49:26 -06:00

libusb/hid.c

					
				@ -481,16 +481,28 @@ err:

				static char *make_path(libusb_device *dev, int interface_number)

what about checking the result and error handling?

Youw (Migrated from github.com) reviewed

2019-11-15 09:51:41 -06:00

libusb/hid.c

					
				@ -481,16 +481,28 @@ err:

				static char *make_path(libusb_device *dev, int interface_number)

also, why not initialize num_ports variable in the same line as it is defined

also, why not initialize `num_ports` variable in the same line as it is defined

Youw (Migrated from github.com) reviewed

2019-11-20 09:34:18 -06:00

Youw (Migrated from github.com) left a comment

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

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

libusb/hid.c

static char *make_path(libusb_device *dev, int interface_number)
{

```suggestion static char *make_path(libusb_device *dev, int interface_number) { ```

derekatkins commented

2020-11-03 06:32:57 -06:00

(Migrated from github.com)

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

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

Youw commented

2020-11-03 07:05:56 -06:00

(Migrated from github.com)

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

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

👍 1

bearsh commented

2020-11-12 06:27:28 -06:00

(Migrated from github.com)

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

derekatkins commented

2020-11-12 06:44:55 -06:00

(Migrated from github.com)

@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 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 commented

2020-11-12 08:34:40 -06:00

(Migrated from github.com)

@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

@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 commented

2020-11-12 09:06:09 -06:00

(Migrated from github.com)

use a approach from the kernel

This sounds like execent reasoning

> use a approach from the kernel This sounds like execent reasoning

bearsh commented

2021-01-25 09:33:27 -06:00

(Migrated from github.com)

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

Youw commented

2021-01-25 10:00:21 -06:00

(Migrated from github.com)

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?

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 commented

2021-02-05 02:05:58 -06:00

(Migrated from github.com)

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

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

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

> 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](https://www.kernel.org/doc/html/latest/admin-guide/abi-stable.html#abi-sys-bus-usb-devices-busnum-port-port-config-num-interface-num-supports-autosuspend)): `/sys/bus/usb/devices/<busnum>-<port[.port]>…:<config num>-<interface num>/supports_autosuspend`

Youw commented

2021-02-05 04:45:55 -06:00

(Migrated from github.com)

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

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

bearsh commented

2021-02-05 06:08:47 -06:00

(Migrated from github.com)

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

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

Youw commented

2021-02-05 06:54:04 -06:00

(Migrated from github.com)

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.

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 commented

2021-02-05 07:01:25 -06:00

(Migrated from github.com)

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.

> 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 commented

2021-02-05 08:22:18 -06:00

(Migrated from github.com)

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 */

something like this? ```diff 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 commented

2021-02-05 08:24:01 -06:00

(Migrated from github.com)

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)

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 commented

2021-06-25 01:45:23 -05:00

(Migrated from github.com)