Add and use usb.ids to show more specific names of USB devices #468

paulpv · 2022-10-20T20:36:52Z

THIS IS A CONVERSATION PIECE FOR NOW

This work is incomplete, but I wanted to get the conversation going of if this is worth pursing or not.

This does bloat the app a bit with a <800KB <26,000 line file, and the <100ms runtime hit to parse this file, but it I think the pros outweighs these cons.
There might also be a better way to accomplish what this does.
I am extremely surprised that this type of usb id database isn't already built in to Windows 10's or Windows 11's 32GB+ footprint.

When I run usbipd list I see that most [but very interestingly not all] devices have generic not very useful names:

C:\path>usbipd.exe list
Connected:
BUSID  VID:PID    DEVICE                                                        STATE
1-1    048d:8297  USB Input Device                                              Not shared
3-3    0557:2405  USB Input Device                                              Not shared
6-2    0557:2405  USB Input Device                                              Not shared
7-1    046d:085c  c922 Pro Stream Webcam, C922 Pro Stream Webcam                Not shared
7-2    1b1c:0a14  CORSAIR VOID PRO Wireless Gaming Headset, USB Input Device    Not shared
8-1    1b1c:1b8b  USB Input Device                                              Shared
8-2    1209:2201  USB Serial Device (COM3), USB Input Device                    Shared

I cannot [easily] tell from this list what devices I want to bind to.
Adding and using http://www.linux-usb.org/usb.ids now shows a [I feel] more useful list:

C:\path>usbipd.exe list
Connected:
BUSID  VID:PID    DEVICE                                                        STATE
1-1    048d:8297  Integrated Technology Express, Inc. IT8297 RGB LED Contro...  Not shared
3-3    0557:2405  ATEN International Co., Ltd USB Input Device                  Not shared
6-2    0557:2405  ATEN International Co., Ltd USB Input Device                  Not shared
7-1    046d:085c  Logitech, Inc. C922 Pro Stream Webcam                         Not shared
7-2    1b1c:0a14  Corsair CORSAIR VOID PRO Wireless Gaming Headset, USB Inp...  Not shared
8-1    1b1c:1b8b  Corsair USB Input Device                                      Shared
8-2    1209:2201  Generic Dygma Shortcut Keyboard                               Shared

When I run `usbipd list` I see that most [but very interestingly not all] devices have generic not very useful names: ``` C:\path>usbipd.exe list Connected: BUSID VID:PID DEVICE STATE 1-1 048d:8297 USB Input Device Not shared 3-3 0557:2405 USB Input Device Not shared 6-2 0557:2405 USB Input Device Not shared 7-1 046d:085c c922 Pro Stream Webcam, C922 Pro Stream Webcam Not shared 7-2 1b1c:0a14 CORSAIR VOID PRO Wireless Gaming Headset, USB Input Device Not shared 8-1 1b1c:1b8b USB Input Device Shared 8-2 1209:2201 USB Serial Device (COM3), USB Input Device Shared ``` I cannot [easily] tell from this list what devices I want to bind to. Adding and using http://www.linux-usb.org/usb.ids now shows a [I feel] more useful list: ``` C:\path>usbipd.exe list Connected: BUSID VID:PID DEVICE STATE 1-1 048d:8297 Integrated Technology Express, Inc. IT8297 RGB LED Contro... Not shared 3-3 0557:2405 ATEN International Co., Ltd USB Input Device Not shared 6-2 0557:2405 ATEN International Co., Ltd USB Input Device Not shared 7-1 046d:085c Logitech, Inc. C922 Pro Stream Webcam Not shared 7-2 1b1c:0a14 Corsair CORSAIR VOID PRO Wireless Gaming Headset, USB Inp... Not shared 8-1 1b1c:1b8b Corsair USB Input Device Shared 8-2 1209:2201 Generic Dygma Shortcut Keyboard Shared ```

codecov · 2022-10-21T22:55:34Z

Codecov Report

Base: 41.65% // Head: 39.27% // Decreases project coverage by -2.37% ⚠️

Coverage data is based on head (03250fe) compared to base (694a6c2).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #468      +/-   ##
==========================================
- Coverage   41.65%   39.27%   -2.38%     
==========================================
  Files          28       29       +1     
  Lines        2612     2770     +158     
==========================================
  Hits         1088     1088              
- Misses       1524     1682     +158

Impacted Files	Coverage Δ
Usbipd/CommandHandlers.cs	`0.00% <0.00%> (ø)`
Usbipd/UsbIds.cs	`0.00% <0.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

dorssel · 2022-10-23T14:03:59Z

Hi Paul,

Thank you for contributing to usbipd-win!
I like the idea of providing more useful feedback to the user, helping in better/more easily selecting the device of interest. Please read the full review, as I do think this may lead to an improvement in the end. However, as is, this PR will not be merged.

usbipd-win is a Windows product, and it is intended to be in line with the Windows terminology and user experience.
My main concern with the PR is internationalization. Over 50% of the users use non-English, most of those even with a non-Latin alphabet. See for example #177. Cyrillic is also quite common.
The current descriptions are the Windows device descriptions, as they can be found in the device manager. It actually does a little bit more: it also lists the underlying device descriptions in case of a multi-function device. Even in your example:

USB Serial Device (COM3), USB Input Device

The comma indicates that this device actually has two functions. I do not see this reflected in the usb.ids alternative. In this sense, the current descriptions actually provide more information than usb.ids.
Some generic devices (such as mice, keyboards, etc.) are handled generically by a Microsoft driver. Those often (but not always, they could still be vendor/device specific) will get names like "USB Input Device", which I agree is less descriptive if you have more than one. Devices that require a vendor driver are usually quite descriptive (and localized!). The majority of devices that are used with usbipd-win are of the latter type, although I do recognize that some people use it for one of those generic devices, e.g., to forward a gamepad or so.

Often, finding out which device to share is a one-off. Once you know, you're done. See the suggestions in Feature - attach devices by specifying Vendor:Product pair #336 (which actually made it in) and Feature - attach devices by specifying NAME matching pattern e.g. "*Camera*" #343 (which didn't) for rationales.

Conclusion: I can see the benefits of using usb.ids. However, the downside of losing internationalization and deviating from the descriptions in device manager do not warrant an outright replacement. Maybe the descriptions could be used additionally in a new "--verbose" option. Or maybe they could be used if both of the following are true: the driver vendor is Microsoft and there is no FriendlyName associated with the instance (as a kind of heuristic way of figuring out if the Windows name will be too generic). And even then, maybe the usb.ids description should be added, as the generic description will at least be localized.

Besides this rationale, there are some issues with the code, but those can be easily fixed. Let's first figure out what we want to achieve here. What problem are we trying to solve? What are the drawbacks of the proposed solution? Do the pros outweigh the cons?

PS: This is in no way to discourage you from contributing! In fact, I welcome your input and the fact that your ideas will lead to an improvement of usbipd-win.

paulpv · 2022-10-28T00:10:47Z

@dorssel Thanks for your comments. I will get back to this soon, since it is an important part of a personal project I am working on.

Yes, this PR was just to get the conversation going and at a minimum I will:

fix the code coverage and clang issues
print names for all of the descriptors for multi-function devices, not just the first one
put the names in state and any other appropriate commands

Regarding #1, Internationalization, perhaps http://www.linux-usb.org would be willing to crowd source translating those, but most of those entries are proper names of manufacturers and products and the majority of them would not need to be translated. I am certain there are many entries that would benefit from being properly translated, but even if I could only read Greek, Integrated Technology Express, Inc. IT8297 RGB LED Controller, and perhaps 80-90% of the other names, would still be more helpful for me to figure out which device that is than USB Input Device.

Again, I will update this PR with some improvements, worst case just appending the names if a --verbose type of command (I am thinking something along the lines of --usbids or --names would make a little more sense).

dorssel · 2022-10-28T09:11:45Z

Yes, I think this is very testable.
I don't think that can be done. The additional functions are not VID:PID. They are device class + subclass, something that isn't exposed in usb.ids.
I actually think this should be added to the UsbId automation class. See my inline review that will follow shortly.

Don't worry too much about internationalization. I think we agree that the usb.ids are additional information. If it helps the user: great; if it doesn't, then they will have what they have now: no regression. Localizing usb.ids is a can of worms that I would not open...

I like the --usbids better than --verbose. My suggestion would be exactly that. Keep the default behavior as it is and add a --usbids option to list and wsl list that will use usbids instead. This also solves the problem of column widths. It keeps the list tidy, and the user has the option to select Windows device names (as currently is the case) or Linux-style names (with the new --usbids option) as an alternative.

With this proposal (which is up for discussion, of course) you get exactly what you had in your original screenshots. Except that they are now both available!

dorssel · 2022-10-30T08:30:05Z

Usbipd/CommandHandlers.cs

+            var hardwareId = device.HardwareId;
+            string description = UsbIds.GetProductName(hardwareId.Vid, hardwareId.Pid, device.Description);


This will be even more useful if we move the whole usb.ids to the Automation part.
And then make the vendor and product strings accessible directly from the current VidPid class. This makes it not only available in the list commands, but also from PowerShell and within the state data.

See "lazy initialization" below for performance considerations.

dorssel · 2022-10-30T08:33:14Z

Usbipd/UsbIds.cs

@@ -0,0 +1,340 @@
+//#define USBIDS_DBG_VERBOSE
+#define USBIDS_STOPWATCH


I wouldn't worry too much about timing. With the --usbids option, the user has opted in anyway. We'll make is as efficient as possible; best effort.

In any case, please guard this with at least an #ifdef DEBUG. It should never be enabled for release builds. (or: just take it out).

dorssel · 2022-10-30T08:34:01Z

Usbipd/UsbIds.cs

+using System.IO;
+using System.Text.RegularExpressions;
+
+namespace Usbipd


nitpicking: coding style is namespace Something;. Saves a whole lot of indenting for the rest of the file.

dorssel · 2022-10-30T08:34:19Z

Usbipd/UsbIds.cs

+    /// <summary>
+    /// A C# port of https://github.com/cezanne/usbip-win/blob/master/userspace/lib/names.c (GPLv2+)
+    /// </summary>
+    class UsbIds


Please make all non-inherited classes sealed.

dorssel · 2022-10-30T08:36:49Z

Usbipd/UsbIds.cs

+    /// </summary>
+    class UsbIds
+    {
+        static UsbIds()


I would prefer a "lazy loader".
In fact: how about a static singleton that will trigger the one-off lazy loader?
See RegistryUtils

This allows direct access to a single lookup table, that automatically initializes on first use.

dorssel · 2022-10-30T08:54:40Z

Usbipd/UsbIds.cs

+            public string name { get; private set; }
+        }
+
+        private static Dictionary<uint, UsbVendor> vendors = new Dictionary<uint, UsbVendor>();


nitpicking:
Coding style leaves out default access modifiers. Reason: cleaner code. C# is safe in the sense that it defaults to the most restrictive access. In other words: just leave out private. You already left out private in the private (!) nested classes earlier.

dorssel · 2022-10-30T08:58:48Z

Usbipd/UsbIds.cs

+                    Match match;
+
+                    string? buf;
+                    while ((buf = f.ReadLine()) != null)


I looked at the original C code to understand how this parser came to be...
Considering that the original is about the worst parser I have ever seen, you actually did a pretty good job improving it. However, we need at least a unit test for this. I'll be refactoring this quite a bit afterwards. For now, it's good enough, provided a unit test is added.

dorssel · 2022-10-30T08:59:42Z

Usbipd/CommandHandlers.cs

-            console.WriteTruncated(device.Description, 60, true);
+            console.WriteTruncated(description, 60, true);


As discussed, add --usbids option. This will then output an alternative description.
I would also change the column header when that option is used, perhaps "USBID" instead of "DEVICE"? (above, at line 130)

dorssel · 2022-10-30T09:05:45Z

Usbipd/UsbIds.cs

+
+        class UsbProduct
+        {
+            public uint vendorid { get; private set; }


C# has record for this. (mentioned once, but true for all these nested classes.

dorssel · 2022-10-30T09:10:14Z

Usbipd/Usbipd.csproj

@@ -36,6 +36,9 @@ SPDX-License-Identifier: GPL-2.0-only
  </ItemGroup>

  <ItemGroup>
+    <None Update="usb.ids">
+      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
+    </None>


Not for this PR (i.e., we can do that in a separate PR), but I like the auto-updater in https://github.com/woodruffw/usb-ids.rs/blob/main/.github/workflows/usbids-updater.yml
Of course, they got the licensing all wrong (!), but we are doing the right thing (GPLv2).

dorssel · 2023-02-25T20:53:06Z

I've created #545 that replaces this MR.

paulpv force-pushed the usb_ids_names branch 2 times, most recently from 861bfae to d36b860 Compare October 20, 2022 20:50

paulpv force-pushed the usb_ids_names branch from d36b860 to 03250fe Compare October 20, 2022 22:02

dorssel self-requested a review October 21, 2022 22:50

dorssel reviewed Oct 30, 2022

View reviewed changes

dorssel closed this Feb 26, 2023

dorssel mentioned this pull request May 21, 2024

can you add manually vendorid and productid of unknown devices? #955

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add and use usb.ids to show more specific names of USB devices #468

Add and use usb.ids to show more specific names of USB devices #468

paulpv commented Oct 20, 2022 •

edited

codecov bot commented Oct 21, 2022

dorssel commented Oct 23, 2022

paulpv commented Oct 28, 2022

dorssel commented Oct 28, 2022

dorssel Oct 30, 2022

dorssel Oct 30, 2022

dorssel Oct 30, 2022

dorssel Oct 30, 2022

dorssel Oct 30, 2022

dorssel Oct 30, 2022

dorssel Oct 30, 2022

dorssel Oct 30, 2022

dorssel Oct 30, 2022

dorssel Oct 30, 2022

dorssel commented Feb 25, 2023

		var hardwareId = device.HardwareId;
		string description = UsbIds.GetProductName(hardwareId.Vid, hardwareId.Pid, device.Description);

		@@ -0,0 +1,340 @@
		//#define USBIDS_DBG_VERBOSE
		#define USBIDS_STOPWATCH

		console.WriteTruncated(device.Description, 60, true);
		console.WriteTruncated(description, 60, true);

Add and use usb.ids to show more specific names of USB devices #468

Add and use usb.ids to show more specific names of USB devices #468

Conversation

paulpv commented Oct 20, 2022 • edited

THIS IS A CONVERSATION PIECE FOR NOW

codecov bot commented Oct 21, 2022

Codecov Report

dorssel commented Oct 23, 2022

paulpv commented Oct 28, 2022

dorssel commented Oct 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dorssel commented Feb 25, 2023

paulpv commented Oct 20, 2022 •

edited