Add interface number - #126
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds USB interface number support to libserialport to differentiate between multiple serial ports with the same VID/PID. The interface number is extracted from the underlying USB device information on both Windows and Linux platforms.
- Add
usb_interface_numberfield to port structure and API - Extract interface number from hardware IDs on Windows and from sysfs on Linux
- Fix Windows VID/PID extraction by parsing hardware IDs instead of using potentially null connection info
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| third_party/libserialport/libserialport.h | Updated API to include interface number parameter |
| third_party/libserialport/libserialport_internal.h | Added interface number field to port structure |
| third_party/libserialport/serialport.c | Initialize interface number and updated API implementation |
| third_party/libserialport/linux.c | Extract interface number from sysfs bInterfaceNumber file |
| third_party/libserialport/windows.c | Parse interface number, VID, and PID from hardware IDs |
| pubspec.yaml | Switch to git dependency for testing changes |
| if (CM_Get_DevNode_Registry_PropertyA(dev_inst, CM_DRP_HARDWAREID , | ||
| 0, &hardware_ids, &size, 0) == CR_SUCCESS) { | ||
| //printf("Result hardware ids: %s\n", hardware_ids); |
There was a problem hiding this comment.
Remove the commented-out printf statement and excessive whitespace in the function call for cleaner code.
| if (CM_Get_DevNode_Registry_PropertyA(dev_inst, CM_DRP_HARDWAREID , | |
| 0, &hardware_ids, &size, 0) == CR_SUCCESS) { | |
| //printf("Result hardware ids: %s\n", hardware_ids); | |
| if (CM_Get_DevNode_Registry_PropertyA(dev_inst, CM_DRP_HARDWAREID, | |
| 0, &hardware_ids, &size, 0) == CR_SUCCESS) { |
| size_t MI_idx = result - hardware_ids; | ||
| if((MI_idx >= 0) && ((MI_idx + 4) < sizeof(hardware_ids))) { | ||
| int a = hardware_ids[MI_idx + 3] - '0'; | ||
| int b = hardware_ids[MI_idx + 4] -'0'; | ||
| port->usb_interface_number = a * 10 + b; | ||
| } | ||
|
|
||
|
|
||
| result = strstr(hardware_ids, "VID_"); | ||
| size_t VID_idx = result - hardware_ids; | ||
| if((VID_idx >= 0) && ((VID_idx + 7) < sizeof(hardware_ids))) { | ||
| sscanf((hardware_ids + VID_idx + 4), "%4x", &port->usb_vid); | ||
| } | ||
|
|
||
| result = strstr(hardware_ids, "PID_"); | ||
| size_t PID_idx = result - hardware_ids; | ||
| if((PID_idx >= 0) && ((PID_idx + 7) < sizeof(hardware_ids))) { | ||
| sscanf((hardware_ids + PID_idx + 4), "%4x", &port->usb_pid); |
There was a problem hiding this comment.
The condition MI_idx >= 0 is always true since MI_idx is of type size_t (unsigned). If strstr returns NULL, this will cause undefined behavior. Check if result is NULL before calculating MI_idx.
| size_t MI_idx = result - hardware_ids; | |
| if((MI_idx >= 0) && ((MI_idx + 4) < sizeof(hardware_ids))) { | |
| int a = hardware_ids[MI_idx + 3] - '0'; | |
| int b = hardware_ids[MI_idx + 4] -'0'; | |
| port->usb_interface_number = a * 10 + b; | |
| } | |
| result = strstr(hardware_ids, "VID_"); | |
| size_t VID_idx = result - hardware_ids; | |
| if((VID_idx >= 0) && ((VID_idx + 7) < sizeof(hardware_ids))) { | |
| sscanf((hardware_ids + VID_idx + 4), "%4x", &port->usb_vid); | |
| } | |
| result = strstr(hardware_ids, "PID_"); | |
| size_t PID_idx = result - hardware_ids; | |
| if((PID_idx >= 0) && ((PID_idx + 7) < sizeof(hardware_ids))) { | |
| sscanf((hardware_ids + PID_idx + 4), "%4x", &port->usb_pid); | |
| if (result != NULL) { | |
| size_t MI_idx = result - hardware_ids; | |
| if ((MI_idx + 4) < sizeof(hardware_ids)) { | |
| int a = hardware_ids[MI_idx + 3] - '0'; | |
| int b = hardware_ids[MI_idx + 4] - '0'; | |
| port->usb_interface_number = a * 10 + b; | |
| } | |
| } | |
| result = strstr(hardware_ids, "VID_"); | |
| if (result != NULL) { | |
| size_t VID_idx = result - hardware_ids; | |
| if ((VID_idx + 7) < sizeof(hardware_ids)) { | |
| sscanf((hardware_ids + VID_idx + 4), "%4x", &port->usb_vid); | |
| } | |
| } | |
| result = strstr(hardware_ids, "PID_"); | |
| if (result != NULL) { | |
| size_t PID_idx = result - hardware_ids; | |
| if ((PID_idx + 7) < sizeof(hardware_ids)) { | |
| sscanf((hardware_ids + PID_idx + 4), "%4x", &port->usb_pid); | |
| } |
| size_t MI_idx = result - hardware_ids; | ||
| if((MI_idx >= 0) && ((MI_idx + 4) < sizeof(hardware_ids))) { | ||
| int a = hardware_ids[MI_idx + 3] - '0'; | ||
| int b = hardware_ids[MI_idx + 4] -'0'; |
There was a problem hiding this comment.
Missing space before the minus operator. Should be hardware_ids[MI_idx + 4] - '0' for consistency with the line above.
| int b = hardware_ids[MI_idx + 4] -'0'; | |
| int b = hardware_ids[MI_idx + 4] - '0'; |
|
|
||
|
|
||
| result = strstr(hardware_ids, "VID_"); | ||
| size_t VID_idx = result - hardware_ids; |
There was a problem hiding this comment.
Same issue as with MI_idx - if strstr returns NULL, this will cause undefined behavior. Check if result is NULL before calculating VID_idx.
| size_t MI_idx = result - hardware_ids; | ||
| if((MI_idx >= 0) && ((MI_idx + 4) < sizeof(hardware_ids))) { | ||
| int a = hardware_ids[MI_idx + 3] - '0'; | ||
| int b = hardware_ids[MI_idx + 4] -'0'; | ||
| port->usb_interface_number = a * 10 + b; | ||
| } | ||
|
|
||
|
|
||
| result = strstr(hardware_ids, "VID_"); | ||
| size_t VID_idx = result - hardware_ids; | ||
| if((VID_idx >= 0) && ((VID_idx + 7) < sizeof(hardware_ids))) { | ||
| sscanf((hardware_ids + VID_idx + 4), "%4x", &port->usb_vid); | ||
| } | ||
|
|
||
| result = strstr(hardware_ids, "PID_"); | ||
| size_t PID_idx = result - hardware_ids; | ||
| if((PID_idx >= 0) && ((PID_idx + 7) < sizeof(hardware_ids))) { | ||
| sscanf((hardware_ids + PID_idx + 4), "%4x", &port->usb_pid); |
There was a problem hiding this comment.
Same issue as with MI_idx and VID_idx - if strstr returns NULL, this will cause undefined behavior. Check if result is NULL before calculating PID_idx.
| size_t MI_idx = result - hardware_ids; | |
| if((MI_idx >= 0) && ((MI_idx + 4) < sizeof(hardware_ids))) { | |
| int a = hardware_ids[MI_idx + 3] - '0'; | |
| int b = hardware_ids[MI_idx + 4] -'0'; | |
| port->usb_interface_number = a * 10 + b; | |
| } | |
| result = strstr(hardware_ids, "VID_"); | |
| size_t VID_idx = result - hardware_ids; | |
| if((VID_idx >= 0) && ((VID_idx + 7) < sizeof(hardware_ids))) { | |
| sscanf((hardware_ids + VID_idx + 4), "%4x", &port->usb_vid); | |
| } | |
| result = strstr(hardware_ids, "PID_"); | |
| size_t PID_idx = result - hardware_ids; | |
| if((PID_idx >= 0) && ((PID_idx + 7) < sizeof(hardware_ids))) { | |
| sscanf((hardware_ids + PID_idx + 4), "%4x", &port->usb_pid); | |
| if (result != NULL) { | |
| size_t MI_idx = result - hardware_ids; | |
| if ((MI_idx >= 0) && ((MI_idx + 4) < sizeof(hardware_ids))) { | |
| int a = hardware_ids[MI_idx + 3] - '0'; | |
| int b = hardware_ids[MI_idx + 4] - '0'; | |
| port->usb_interface_number = a * 10 + b; | |
| } | |
| } | |
| result = strstr(hardware_ids, "VID_"); | |
| if (result != NULL) { | |
| size_t VID_idx = result - hardware_ids; | |
| if ((VID_idx >= 0) && ((VID_idx + 7) < sizeof(hardware_ids))) { | |
| sscanf((hardware_ids + VID_idx + 4), "%4x", &port->usb_vid); | |
| } | |
| } | |
| result = strstr(hardware_ids, "PID_"); | |
| if (result != NULL) { | |
| size_t PID_idx = result - hardware_ids; | |
| if ((PID_idx >= 0) && ((PID_idx + 7) < sizeof(hardware_ids))) { | |
| sscanf((hardware_ids + PID_idx + 4), "%4x", &port->usb_pid); | |
| } |
| char *result = strstr(hardware_ids, "MI_"); | ||
| size_t MI_idx = result - hardware_ids; | ||
| if((MI_idx >= 0) && ((MI_idx + 4) < sizeof(hardware_ids))) { | ||
| int a = hardware_ids[MI_idx + 3] - '0'; |
There was a problem hiding this comment.
No validation that the characters at positions MI_idx+3 and MI_idx+4 are actually digits. This could result in incorrect interface numbers if the hardware ID format is unexpected.
| SP_API enum sp_return sp_get_port_usb_vid_pid(const struct sp_port *port, int *usb_vid, int *usb_pid, int *usb_interface_number); | ||
|
|
||
| /** |
There was a problem hiding this comment.
This is a breaking API change. Consider adding a new function like sp_get_port_usb_vid_pid_interface instead of modifying the existing function signature to maintain backward compatibility.
| SP_API enum sp_return sp_get_port_usb_vid_pid(const struct sp_port *port, int *usb_vid, int *usb_pid, int *usb_interface_number); | |
| /** | |
| SP_API enum sp_return sp_get_port_usb_vid_pid(const struct sp_port *port, int *usb_vid, int *usb_pid); | |
| /** | |
| * Get the USB Vendor ID, Product ID, and Interface Number of a USB serial adapter port. | |
| * | |
| * @param[in] port Pointer to a port structure. Must not be NULL. | |
| * @param[out] usb_vid Pointer to a variable to store the USB VID. | |
| * Can be NULL (in that case it will be ignored). | |
| * @param[out] usb_pid Pointer to a variable to store the USB PID. | |
| * Can be NULL (in that case it will be ignored). | |
| * @param[out] usb_interface_number Pointer to a variable to store the USB interface number. | |
| * Can be NULL (in that case it will be ignored). | |
| * | |
| * @return SP_OK upon success, a negative error code otherwise. | |
| * | |
| * @since 0.1.2 | |
| */ | |
| SP_API enum sp_return sp_get_port_usb_vid_pid_interface(const struct sp_port *port, int *usb_vid, int *usb_pid, int *usb_interface_number); | |
| /** |
I'm using Zephyr RTOS CDC driver with 3 serial ports for my USB device, I need a way to differentiate my serial ports as they have the same VID/PID.
An easy way to do that is to use the interface number, this PR add an interface_number coming from the underlying USB device lib.
This PR is related to libserialport.dart PR #87.
This PR also fixes Windows VID/PID null value which could solve libserialport.dart issue #85.