Skip to content
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.

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

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 6 additions & 2 deletions Usbipd/CommandHandlers.cs
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,8 @@ Task<ExitCode> ICommandHandlers.List(IConsole console, CancellationToken cancell
foreach (var device in allDevices.Where(d => d.BusId.HasValue).OrderBy(d => d.BusId.GetValueOrDefault()))
{
Debug.Assert(device.BusId.HasValue);
var hardwareId = device.HardwareId;
string description = UsbIds.GetProductName(hardwareId.Vid, hardwareId.Pid, device.Description);
Comment on lines +134 to +135
Copy link
Owner

Choose a reason for hiding this comment

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

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.

string state;
if (device.IPAddress is not null)
{
Expand All @@ -147,7 +149,7 @@ Task<ExitCode> ICommandHandlers.List(IConsole console, CancellationToken cancell
// NOTE: Strictly speaking, both Bus and Port can be > 99. If you have one of those, you win a prize!
console.Write($"{device.BusId.Value,-5} ");
console.Write($"{device.HardwareId,-9} ");
console.WriteTruncated(device.Description, 60, true);
console.WriteTruncated(description, 60, true);
Comment on lines -150 to +152
Copy link
Owner

Choose a reason for hiding this comment

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

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)

console.WriteLine($" {state}");
}
console.WriteLine(string.Empty);
Expand All @@ -157,8 +159,10 @@ Task<ExitCode> ICommandHandlers.List(IConsole console, CancellationToken cancell
foreach (var device in allDevices.Where(d => !d.BusId.HasValue && d.Guid.HasValue).OrderBy(d => d.Guid.GetValueOrDefault()))
{
Debug.Assert(device.Guid.HasValue);
var hardwareId = device.HardwareId;
string description = UsbIds.GetProductName(hardwareId.Vid, hardwareId.Pid, device.Description);
console.Write($"{device.Guid.Value,-36:D} ");
console.WriteTruncated(device.Description, 60, false);
console.WriteTruncated(description, 60, false);
console.WriteLine(string.Empty);
}
console.WriteLine(string.Empty);
Expand Down
340 changes: 340 additions & 0 deletions Usbipd/UsbIds.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,340 @@
//#define USBIDS_DBG_VERBOSE
Copy link
Owner

Choose a reason for hiding this comment

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

Please add your SPDX license information. Don't forget the original authors.

#define USBIDS_STOPWATCH
Copy link
Owner

Choose a reason for hiding this comment

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

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).


using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Text.RegularExpressions;

namespace Usbipd
Copy link
Owner

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Please make all non-inherited classes sealed.

{
static UsbIds()
Copy link
Owner

Choose a reason for hiding this comment

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

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.

{
Load();
}

class UsbVendor
{
public UsbVendor(string name, uint vendorid)
{
this.name = name;
this.vendorid = vendorid;
}

public string name { get; private set; }
public uint vendorid { get; private set; }
public Dictionary<uint, UsbProduct> products = new Dictionary<uint, UsbProduct>();
}

class UsbProduct
{
public uint vendorid { get; private set; }
Copy link
Owner

Choose a reason for hiding this comment

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

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

public uint productid { get; private set; }
public string name { get; private set; }

public UsbProduct(string name, uint vendorid, uint productid)
{
this.name = name;
this.vendorid = vendorid;
this.productid = productid;
}
}

class UsbClass
{
public UsbClass(string name, uint classid)
{
this.name = name;
this.classid = classid;
}

public string name { get; private set; }
public uint classid { get; private set; }
public Dictionary<uint, UsbSubclass> subclasses = new Dictionary<uint, UsbSubclass>();
}

class UsbSubclass
{
public UsbSubclass(string name, uint classid, uint subclassid)
{
this.name = name;
this.classid = classid;
this.subclassid = subclassid;
}

public uint classid { get; private set; }
public uint subclassid { get; private set; }
public string name { get; private set; }
public Dictionary<uint, UsbClassProtocol> protocols = new Dictionary<uint, UsbClassProtocol>();
}

class UsbClassProtocol
{
public UsbClassProtocol(string name, uint classid, uint subclassid, uint protocolid)
{
this.name = name;
this.classid = classid;
this.subclassid = subclassid;
this.protocolid = protocolid;
}

public uint classid { get; private set; }
public uint subclassid { get; private set; }
public uint protocolid { get; private set; }
public string name { get; private set; }
}

private static Dictionary<uint, UsbVendor> vendors = new Dictionary<uint, UsbVendor>();
Copy link
Owner

Choose a reason for hiding this comment

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

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.

private static Dictionary<uint, UsbClass> classes = new Dictionary<uint, UsbClass>();

private static bool isxdigit(char c)
{
return c >= '0' && c <= '9' || ('a' <= c && c <= 'f') || ('A' <= c && c <= 'F');
}

private static bool isspace(char c)
{
return c == ' ';
}
Comment on lines +96 to +104
Copy link
Owner

Choose a reason for hiding this comment

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

This needs a serious refactor. This is not how parsers are written in C#. (I know this was originally C...)


private static bool new_vendor(string name, uint vendorid)
{
return vendors.TryAdd(vendorid, new UsbVendor(name, vendorid));
}

public static string GetVendorName(uint vendorid)
{
vendors.TryGetValue(vendorid, out var usbvendor);
return usbvendor?.name ?? String.Empty;
Copy link
Owner

Choose a reason for hiding this comment

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

I prefer returning null if not found (i.e., make the method return string?). Empty is a "magic value" in this case.

The caller can then (for example) simply use GetVendorName(1234) ?? "<unknown>".

}

private static bool new_product(string name, uint vendorid, uint productid)
{
var usbvendor = vendors[vendorid];
return usbvendor.products.TryAdd(productid, new UsbProduct(name, vendorid, productid));
}

public static string GetProductName(uint vendorid, uint productid, string defaultValue)
{
UsbProduct? usbproduct = null;
if (!vendors.TryGetValue(vendorid, out var usbvendor))
{
return defaultValue;
}
if (!usbvendor.products.TryGetValue(productid, out usbproduct))
{
return String.Format("{0} {1}", usbvendor.name, defaultValue);
}
return String.Format("{0} {1}", usbvendor.name, usbproduct.name);
}

private static bool new_class(string name, uint classid)
{
return classes.TryAdd(classid, new UsbClass(name, classid));
}

private static bool new_subclass(string name, uint classid, uint subclassid)
{
var usbclass = classes[classid];
return usbclass.subclasses.TryAdd(subclassid, new UsbSubclass(name, classid, subclassid));
}

private static bool new_protocol(string name, uint classid, uint subclassid, uint protocolid)
{
var usbclass = classes[classid];
var usbsubclass = usbclass.subclasses[subclassid];
return usbsubclass.protocols.TryAdd(protocolid, new UsbClassProtocol(name, classid, subclassid, protocolid));
}

private static void dbg(string format, params object?[] args)
{
Debug.WriteLine(String.Format(format, args));
}

private static uint Load()
{
dbg("+UsbDevice.Load()");
#if USBIDS_STOPWATCH
var stopwatch = new System.Diagnostics.Stopwatch();
stopwatch.Start();
#endif
uint numlines = 0;
var filename = "usb.ids";
if (File.Exists(filename))
{
using (var f = File.OpenText(filename))
{
int lastvendor = -1;
int lastclass = -1;
int lastsubclass = -1;
int lasthut = -1;
int lastlang = -1;

int numvendors = 0;
int numproducts = 0;
int numclasses = 0;
int numsubclasses = 0;
int numprotocols = 0;

var patternVendor = @"\A(?'id'[0-9a-f]{4}) +(?'name'.*)\Z";
var patternProduct = @"\A\t(?'id'[0-9a-f]{4}) +(?'name'.*)\Z";
var patternClass = @"\AC (?'id'[0-9a-f]{2}) +(?'name'.*)\Z";
var patternSubclass = @"\A\t(?'id'[0-9a-f]{2}) +(?'name'.*)\Z";
var patternProtocol = @"\A\t\t(?'id'[0-9a-f]{2}) +(?'name'.*)\Z";
var patternHidUsage = @"\AHUT (?'id'[0-9a-f]{2}) +(?'name'.*)\Z";
var patternLang = @"\AL (?'id'[0-9a-f]{4}) +(?'name'.*)\Z";
Comment on lines +187 to +191
Copy link
Owner

Choose a reason for hiding this comment

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

I hadn't realized usb.ids contained more than just vendors and products. Maybe there is more to it after all...
Is "lang" actually used anywhere?

Match match;

string? buf;
while ((buf = f.ReadLine()) != null)
Copy link
Owner

Choose a reason for hiding this comment

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

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.

{
numlines++;

if (buf.Length < 1 || buf[0] == '#')
{
lastvendor = lastclass = lastsubclass = lasthut = lastlang = -1;
continue;
}

match = Regex.Match(buf, patternVendor);
if (match.Success)
{
var id = Convert.ToUInt32(match.Groups["id"].Value, 16);
var name = match.Groups["name"].Value;
#if USBIDS_DBG_VERBOSE
dbg("{0:D5}: vendor id={1:X4}, name=\"{2}\"", numlines, id, name);
#endif
if (new_vendor(name, id))
{
++numvendors;
}
else
{
dbg("Duplicate vendor spec at line {0:D5} vendor {1:X4} {2}",
numlines, id, name);
}
lastvendor = (int)id;
lasthut = lastlang = lastclass = lastsubclass = -1;
continue;
}
match = Regex.Match(buf, patternProduct);
if (match.Success && lastvendor != -1)
{
var id = Convert.ToUInt32(match.Groups["id"].Value, 16);
var name = match.Groups["name"].Value;
#if USBIDS_DBG_VERBOSE
dbg("{0:D5}: product id={1:X4}, name=\"{2}\"", numlines, id, name);
#endif
if (new_product(name, (uint)lastvendor, id))
{
++numproducts;
}
else
{
dbg("Duplicate product spec at line {0:D5} product {1:X4}:{2:X4} {3}",
numlines, lastvendor, id, name);
}
continue;
}
match = Regex.Match(buf, patternClass);
if (match.Success)
{
var id = Convert.ToUInt32(match.Groups["id"].Value, 16);
var name = match.Groups["name"].Value;
#if USBIDS_DBG_VERBOSE
dbg("{0:D5}: class id= {1:X2}, name=\"{2}\"", numlines, id, name);
#endif
if (new_class(name, id))
{
++numclasses;
}
else
{
dbg("Duplicate class spec at line {0:D5} class {1:X2} {2}",
numlines, id, name);
}
lastclass = (int)id;
lasthut = lastlang = lastvendor = lastsubclass = -1;
continue;
}
match = Regex.Match(buf, patternSubclass);
if (match.Success && lastclass != -1)
{
var id = Convert.ToUInt32(match.Groups["id"].Value, 16);
var name = match.Groups["name"].Value;
#if USBIDS_DBG_VERBOSE
dbg("{0:D5}: subclass id= {1:X2}, name=\"{2}\"", numlines, id, name);
#endif
if (new_subclass(name, (uint)lastclass, id))
{
++numsubclasses;
}
else
{
dbg("Duplicate subclass spec at line {0:D5} class {1:X2}:{2:X2} {3}",
numlines, lastclass, id, name);
}
lastsubclass = (int)id;
continue;
}
match = Regex.Match(buf, patternProtocol);
if (match.Success && lastclass != -1 && lastsubclass != -1)
{
var id = Convert.ToUInt32(match.Groups["id"].Value, 16);
var name = match.Groups["name"].Value;
#if USBIDS_DBG_VERBOSE
dbg("{0:D5}: protocol id= {1:X2}, name=\"{2}\"", numlines, id, name);
#endif
if (new_protocol(name, (uint)lastclass, (uint)lastsubclass, id))
{
++numprotocols;
}
else
{
dbg("Duplicate protocol spec at line {0:D5} class {1:X2}:{2:X2}:{3:X2} {4}",
numlines, lastclass, lastsubclass, id, name);
}
continue;
}
match = Regex.Match(buf, patternHidUsage);
if (match.Success)
{
/*
* set 1 as pseudo-id to indicate that the parser is
* in a `HUT' section.
*/
lasthut = 1;
lastlang = lastclass = lastvendor = lastsubclass = -1;
continue;
}
match = Regex.Match(buf, patternLang);
if (match.Success)
{
/*
* set 1 as pseudo-id to indicate that the parser is
* in a `L' section.
*/
lastlang = 1;
lasthut = lastclass = lastvendor = lastsubclass = -1;
continue;
}
} // while
dbg($"UsbDevice.Load() DONE: numvendors={numvendors}, numproducts={numproducts}, numclasses={numclasses}, numsubclasses={numsubclasses}, numprotocols={numprotocols}");
} // using
} // exists
#if USBIDS_STOPWATCH
stopwatch.Stop();
dbg($"-UsbDevice.Load(); numlines={numlines}; took {stopwatch.ElapsedMilliseconds} ms");
#else
dbg($"-UsbDevice.Load(); numlines={numlines}");
#endif
return numlines;
}
}
}
5 changes: 4 additions & 1 deletion Usbipd/Usbipd.csproj
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
<?xml version="1.0" encoding="utf-8"?>
Copy link
Owner

Choose a reason for hiding this comment

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

What editor are you using? It is messing up the UTF-8 BOM...

It's partly my fault. I will add "charset = utf-8-bom" to .editorconfig for csproj files...

<!--
SPDX-FileCopyrightText: 2020 Frans van Dorsselaer

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

<ItemGroup>
<None Update="usb.ids">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
Copy link
Owner

Choose a reason for hiding this comment

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

We also need a usb.ids.license file. Not in this project file, but just add it to the repo (for REUSE). See other .license files in the repo.

</None>
Copy link
Owner

Choose a reason for hiding this comment

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

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).

<None Update="wsl-scripts\auto-attach.sh">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</None>
Expand Down