Compatibility for composite USB devices on Windows

[okyeron]

This PR adds a function to walk up the USB device tree on Windows to get the serial number from the parent device.

Background:
A neotrellis grid user IntaUnta posted about having problems with serialosc on Windows and that they’d come up with a solution way down in libmonome’s windows.c source.

The problem is that with a USB Composite Device, Windows appears not to pass the device name/serial/etc along to the actual COM port when it enumerates. Thus the serial number is missing and the device is not seen as a grid.

When building the neotrellis code using Arduino or PlatformIO, there’s not enough control over the USB stack to force it to be a single device, thus it shows up as a composite device on Windows (MacOS and Linux don’t seem to care either way). If using PicoSDK there’s better control of the USB descriptors and it can be worked around.

I tested and built serialosc with this PR on Windows10 and a composite USB device properly gets recognized as a grid.

[artfwo]

Thanks. The fix is correct in spirit, but the implementation looks careless and could be tidied up a bit. Can you collapse the duplicated tree-walk into a loop, i.e.

do {
// ...
} while (CM_Get_Parent(&devinst, devinst, 0) == CR_SUCCESS);

The new function could even be inlined then.

The comment style is inconsistent with the file (actually, none of comments are of any value here so might as well be cleared).

What's the new #include for?

What's the increment di++ for?

[okyeron]

Removed comments, removed spurious include and replaced tree-walk logic with do/while loop. Tested on Windows 10.

d++ is there to go over all COM-port devices until a match is found.

[artfwo]

d++ is there to go over all COM-port devices until a match is found.

I see. This fixes a different problem and a much better fix here would be a for loop over di that would ensure the value is incremented for each continue. It's a somewhat bigger change, but if you really want to fix it, can you either push it here as separate commit (see below why) or raise another PR for it?

Removed comments, removed spurious include and replaced tree-walk logic with do/while loop. Tested on Windows 10.

The entire function m_get_serial_from_devinst could be inlined in monome_platform_get_dev_serial saving a couple of duplicated serial, instance_id initializers.

depth and cr are not necessary here.

The condition if (serial) could be replaced by if (serial = m_get_serial_from_instance_id(instance_id)).

Squash the commits and give a clear message on what you changed. Windows platform code has historically been a pain to maintain in serialosc and digging through its history is easier if commit messages are at least somewhat meaningful.

I see that the forum post you linked above points to practically the same code as the code you've initially proposed. Please give credit to the original author.

[okyeron]

Apologies for a dumb question - I'm unsure how best to give credit to the original author. Should that be here in the PR, added as a comment in the code itself, or in the git commit?

[artfwo]

Apologies for a dumb question - I'm unsure how best to give credit to the original author. Should that be here in the PR, added as a comment in the code itself, or in the git commit?

git commits have Co-authored-by: trailer fields.

By the way, is this PR related to #88 in any way?

As to the actual state of the code, you have a done a couple of changes that weren't requested. I appreciate the initiative, but additions like this make reviewing slower because they expand the scope and introduce new things we haven't discussed yet.

Specifically, the inner for loop is way bigger than it should be, and is better as a do..while loop. There's no need to introduce i or devinst variables (capping the depth isn't necessary, as it can't produce a runaway limit and I'd prefer readability of code over micro-optimization here; devinst variable isn't necessary either, if it's only there to preserve a struct field which doesn't need preserving).

There's no need for double parens around if ((serial = ...)), the convention in the code seems to be if (x = y)

There's a double break in this code, that makes it a bit hard to follow (breaking out of while, then for loop).

for (...) {

	do {
		if (...)
			break;
	} while (...);

	break;
}

I'd suggest adding a label right before SetupDiDestroyDeviceInfoList, e.g. done and calling goto done; when m_get_serial_from_instance_id returns a valid number.

If you're up for amending the commits, consider rewriting the messages that both describe what the diff already shows and omit the one thing the diff can't - why. git show already tells me the code became a for loop and that it walks the device tree. Restating that adds no information (also, the body of the first repeats the subject, the body of the 2nd describes the for conversion but no reason for it).

Something like Fix serial detection on pico/composite USB devices or Fix infinite loop when a COM port has no PortName are explaining what's happening way better and the PR link will be included in the merge commit too.

[okyeron]

By the way, is this PR related to #88 in any way?

I did not see or reference that PR previously but at a glance it does appear to be addressing the same issue - walking up the device tree to get the correct info.

What is the best path forward? Continuing with this PR or revisiting the previous one?

Thanks for the tips regarding Git and commit messages. I'm sadly somewhat bad at Git.

[artfwo]

What is the best path forward? Continuing with this PR or revisiting the previous one?

I'd say proceed with this, as you've got potential infinite loop thing covered, and the device parents loop version looks more robust. I can't test this, having no pico devices, but can test with ftdi or cdc grids, once the code is finalized.

[artfwo]

One thing I'm wondering about looking at #88 proposal is if stepping only one parent up fixes the problem. Is there any difference there between teensy and pico-based clones?

[okyeron]

One thing I'm wondering about looking at #88 proposal is if stepping only one parent up fixes the problem. Is there any difference there between teensy and pico-based clones?

In my testing so far with Pico based devices (that show up as a composite USB device) - I see the device as one level down - a parent device with the serial number and then the COM port instance as a child.

I could test the previous PR as well as this new one against a few different controllers (Pico and Teensy)

[artfwo]

One thing I'm wondering about looking at #88 proposal is if stepping only one parent up fixes the problem. Is there any difference there between teensy and pico-based clones?

In my testing so far with Pico based devices (that show up as a composite USB device) - I see the device as one level down - a parent device with the serial number and then the COM port instance as a child.

I could test the previous PR as well as this new one against a few different controllers (Pico and Teensy)

Yeah, check please. If it's one level up only, we shouldn't need the whole dancing with the loop.

[okyeron]

Ok - I did some testing with PR #88 and found it worked OK with the Teensy 4.0 but did not work consistently with Pico composite devices. (I think windows may be taking a couple retries to enumerate?) My PR had more consistent connections.

Squashed, re-commented and pushed a new commit with the changes suggested above and it appears to be working with the various composite devices I have on hand (Pico, Teensy4) and non-composite (Teensy 3.2)

[artfwo]

What exactly do you mean by inconsistent? Do they register sometimes and then not? This might point at a somewhat different problem I'm afraid. Also check a formatting remark below.

[artfwo]

The indentation here is inconsistent - spaces mixed with tabs. Also labels are not indented in this code, e.g. look here for a formatting example:

libmonome/src/platform/windows.c

Line 73 in 38a7038

err_protocol_new:

[okyeron]

fixed spaces and indent

[okyeron]

What exactly do you mean by inconsistent? Do they register sometimes and then not? This might point at a somewhat different problem I'm afraid.

Apologies, I did a bunch of back and forth testing of a few different controllers and did not take notes. I am not sure how to elaborate. I think with the other PR (88) I had the pico connect once, but then was unable to get it to connect again. Perhaps "consistent" is not the right word to use? The current PR (90) would connect/disconnect properly each time I tried.

[artfwo]

What exactly do you mean by inconsistent? Do they register sometimes and then not? This might point at a somewhat different problem I'm afraid.

Apologies, I did a bunch of back and forth testing of a few different controllers and did not take notes. I am not sure how to elaborate. I think with the other PR (88) I had the pico connect once, but then was unable to get it to connect again. Perhaps "consistent" is not the right word to use? The current PR (90) would connect/disconnect properly each time I tried.

It's suspicious if it connected once and then not. Are you sure you've been running a different build?