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

Query registry for USB serial numbers in win #1483

Merged
merged 1 commit into from
Feb 28, 2018

Conversation

IvanSanchez
Copy link
Contributor

@IvanSanchez IvanSanchez commented Feb 15, 2018

Fixes #1459.

This PR makes the code for win32 fetch the ContainerID of the device node holding the serial port, then make a registry lookup for the composite USB device with a matching ContainerID. The serial number from the device node of the composite device is then returned and used.

The code might not be the cleanest ever, but it's in a working state.

Special kudos to @CoqRogue for all the work put on this.

@@ -9,7 +9,7 @@ class WindowsBinding extends BaseBinding {
return promisify(binding.list)().then(ports => {
// Grab the serial number from the pnp id
ports.forEach(port => {
if (port.pnpId) {
if (port.pnpId && !port.serialNumber) {
Copy link
Member

Choose a reason for hiding this comment

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

👍


// NOTE: Devices might have a containerUuid like {00000000-0000-0000-FFFF-FFFFFFFFFFFF}
// This means they're non-removable, and are not handled (yet).
// Maybe they should inherit the s/n from somewhere else.
Copy link
Member

Choose a reason for hiding this comment

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

What's non-removable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That means that the serial port is not part of what win32 considers a removable device. In other words: the RS-232 connector soldered to your motherboard.

pnpId = strdup(szBuffer);

pnpId = strdup(szBuffer);
Copy link
Member

Choose a reason for hiding this comment

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

nit: whitespace

@reconbot
Copy link
Member

cc @dustmop && @Fumon if you have some time, would you mind reviewing this? I'm not very good at reading c++ for errors. The logic seems ok for what I understand about querying the registry.

@IvanSanchez are there any parts you'd like to highlight for review?

@IvanSanchez
Copy link
Contributor Author

@reconbot Just test that this works for whatever USB serial adapters you have lying around, specially if you have two or more of the same brand/model. This works for all the stuff in my test bench, but I'm not sure if there is any edge case that we overlooked.

Luckily @CoqRogue did an over-the-shoulder review of the c++ code, but we'll appreciate extra eyes looking at it.

@codecov-io
Copy link

codecov-io commented Feb 15, 2018

Codecov Report

Merging #1483 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1483   +/-   ##
=======================================
  Coverage   79.04%   79.04%           
=======================================
  Files          21       21           
  Lines         940      940           
  Branches      170      170           
=======================================
  Hits          743      743           
  Misses        197      197
Impacted Files Coverage Δ
lib/bindings/win32.js 66.66% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 270c2be...04ee499. Read the comment docs.

@reconbot
Copy link
Member

Hey @IvanSanchez I want to land this but I landed your linting PR first and this doesn't pass anymore =x

Mind rebasing?

Thanks!

@IvanSanchez
Copy link
Contributor Author

Oh, the joys of linting. I'll have a look.

@IvanSanchez IvanSanchez force-pushed the win-serial-number branch 2 times, most recently from afac78f to b6e7007 Compare February 28, 2018 09:48
…rialport#1483)

Serial numbers in win32 from USB devices cannot be readily derived from the PnP ID.
This adds some logic to iterate through the win32 registry, as per the research in serialport#1459.
@IvanSanchez
Copy link
Contributor Author

Rebased, linted, and squashed.

@reconbot
Copy link
Member

reconbot commented Feb 28, 2018 via email

@reconbot reconbot merged commit 45b3a2f into serialport:master Feb 28, 2018
@reconbot
Copy link
Member

🎉

@IvanSanchez
Copy link
Contributor Author

🎉

Given the amount of rebasing and linting involved in getting this merged, I'd rather go into my usual "if it works, don't touch it" stance. 😁

@IvanSanchez IvanSanchez deleted the win-serial-number branch February 28, 2018 13:11
@reconbot
Copy link
Member

reconbot commented Feb 28, 2018 via email

oteku pushed a commit to Cutii/node-serialport that referenced this pull request Apr 9, 2018
…rialport#1483)

Serial numbers in win32 from USB devices cannot be readily derived from the PnP ID.
This adds some logic to iterate through the win32 registry, as per the research in serialport#1459.
@lock lock bot locked as resolved and limited conversation to collaborators Aug 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants