-
Notifications
You must be signed in to change notification settings - Fork 114
Conversation
Hi, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! Seems to work well 👍
Mostly small nits and I am curious if @jasondana has any comments
Here is the output from a Windows 10 PC for example,
find 17 [ { locationId: 0,
vendorId: 0,
productId: 0,
deviceName: 'USB Root Hub (USB 3.0)',
manufacturer: '(Standard USB HUBs)',
serialNumber: '',
deviceAddress: 10 },
{ locationId: 0,
vendorId: 0,
productId: 0,
deviceName: 'USB Root Hub (USB 3.0)',
manufacturer: '(Standard USB HUBs)',
serialNumber: '',
deviceAddress: 15 },
{ locationId: 0,
vendorId: 1118,
productId: 746,
deviceName: 'Xbox One Controller',
manufacturer: 'Microsoft',
serialNumber: '3032363030303739393032363336',
deviceAddress: 17 },
{ locationId: 0,
vendorId: 1118,
productId: 767,
deviceName: 'USB Input Device',
manufacturer: '(Standard system devices)',
serialNumber: '00&00&00009D605980ED7E',
deviceAddress: 7 },
{ locationId: 0,
vendorId: 1133,
productId: 49742,
deviceName: 'USB Composite Device',
manufacturer: '(Standard USB Host Controller)',
serialNumber: '9025267AF90009',
deviceAddress: 1 },
{ locationId: 0,
vendorId: 1133,
productId: 49742,
deviceName: 'USB Input Device',
manufacturer: '(Standard system devices)',
serialNumber: '',
deviceAddress: 2 },
{ locationId: 0,
vendorId: 1133,
productId: 49742,
deviceName: 'USB Input Device',
manufacturer: '(Standard system devices)',
serialNumber: '',
deviceAddress: 4 },
{ locationId: 0,
vendorId: 2652,
productId: 8680,
deviceName: 'Broadcom BCM20702 Bluetooth 4.0 USB Device',
manufacturer: 'Broadcom',
serialNumber: '5CF3707AD674',
deviceAddress: 9 },
{ locationId: 0,
vendorId: 6720,
productId: 257,
deviceName: 'Generic USB Hub',
manufacturer: '(Standard USB HUBs)',
serialNumber: '',
deviceAddress: 5 },
{ locationId: 0,
vendorId: 8457,
productId: 10257,
deviceName: 'Generic USB Hub',
manufacturer: '(Standard USB HUBs)',
serialNumber: '',
deviceAddress: 16 },
{ locationId: 0,
vendorId: 8457,
productId: 10257,
deviceName: 'Generic USB Hub',
manufacturer: '(Standard USB HUBs)',
serialNumber: '',
deviceAddress: 6 },
{ locationId: 0,
vendorId: 8457,
productId: 33040,
deviceName: 'Generic SuperSpeed USB Hub',
manufacturer: '(Standard USB HUBs)',
serialNumber: '',
deviceAddress: 3 },
{ locationId: 0,
vendorId: 8457,
productId: 33040,
deviceName: 'Generic SuperSpeed USB Hub',
manufacturer: '(Standard USB HUBs)',
serialNumber: '',
deviceAddress: 14 },
{ locationId: 0,
vendorId: 9494,
productId: 60,
deviceName: 'USB Composite Device',
manufacturer: '(Standard USB Host Controller)',
serialNumber: '',
deviceAddress: 8 },
{ locationId: 0,
vendorId: 9494,
productId: 60,
deviceName: 'USB Input Device',
manufacturer: '(Standard system devices)',
serialNumber: '',
deviceAddress: 12 },
{ locationId: 0,
vendorId: 9494,
productId: 60,
deviceName: 'USB Input Device',
manufacturer: '(Standard system devices)',
serialNumber: '',
deviceAddress: 11 },
{ locationId: 0,
vendorId: 9494,
productId: 60,
deviceName: 'USB Input Device',
manufacturer: '(Standard system devices)',
serialNumber: '',
deviceAddress: 13 } ]
src/detection_win.cpp
Outdated
// If device has a CM_DEVCAP_UNIQUEID capability, then we assume it has a serial number | ||
if ((dwCap & CM_DEVCAP_UNIQUEID) == CM_DEVCAP_UNIQUEID) { | ||
if (DllSetupDiGetDeviceInstanceId(hDevInfo, pspDevInfoData, buf, buffSize, &nSize)) { | ||
// extract Serial Number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a comment about what we are trying to parse
// Format: <device-ID>\<instance-specific-ID>
//
// Ex. `USB\VID_2109&PID_8110\5&376ABA2D&0&21`
// - `<device-ID>`: `USB\VID_2109&PID_8110`
// - `<instance-specific-ID>`: `5&376ABA2D&0&21`
//
// [Device instance IDs](https://docs.microsoft.com/en-us/windows-hardware/drivers/install/device-instance-ids) ->
// - [Device IDs](https://docs.microsoft.com/en-us/windows-hardware/drivers/install/device-ids) -> [Hardware IDs](https://docs.microsoft.com/en-us/windows-hardware/drivers/install/hardware-ids) -> [Device identifier formats](https://docs.microsoft.com/en-us/windows-hardware/drivers/install/device-identifier-formats) -> [Identifiers for USB devices](https://docs.microsoft.com/en-us/windows-hardware/drivers/install/identifiers-for-usb-devices)
// - [Standard USB Identifiers](https://docs.microsoft.com/en-us/windows-hardware/drivers/install/standard-usb-identifiers)
// - [Special USB Identifiers](https://docs.microsoft.com/en-us/windows-hardware/drivers/install/special-usb-identifiers)
// - [Instance specific ID](https://docs.microsoft.com/en-us/windows-hardware/drivers/install/instance-ids)
From #50 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am OK with that
src/detection_win.cpp
Outdated
if (DllSetupDiGetDeviceInstanceId(hDevInfo, pspDevInfoData, buf, buffSize, &nSize)) { | ||
// extract Serial Number | ||
string str = buf; | ||
size_t t = str.find_last_of("\\\\"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are parsing strings like USB\VID_046D&PID_C24E\9025267AF90009
Why are we using a double backslash \\\\
? It seems to work just fine with str.find_last_of("\\");
. According to the docs, we should only need to define the backslash once, http://www.cplusplus.com/reference/string/string/find_last_of/ 🤷♂️
Let's rename this variable to size_t serialNumberIndex = ...
to make this more clear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why double backslash?
I have no idea :) just miswritten.
src/detection_win.cpp
Outdated
if ((dwCap & CM_DEVCAP_UNIQUEID) == CM_DEVCAP_UNIQUEID) { | ||
if (DllSetupDiGetDeviceInstanceId(hDevInfo, pspDevInfoData, buf, buffSize, &nSize)) { | ||
// extract Serial Number | ||
string str = buf; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rename this variable to string deviceInstanceId = buf;
so it better represents what it holds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am OK with that
src/detection_win.cpp
Outdated
@@ -7,6 +7,7 @@ | |||
|
|||
// Include Windows headers | |||
#include <windows.h> | |||
#include <Cfgmgr32.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CM_DEVCAP_UNIQUEID is defined in that header file, so we have to include it.
I added a comment near header file for future reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see it listed here
- https://msdn.microsoft.com/en-us/library/windows/hardware/mt813619%28v=vs.85%29.aspx?f=255&MSPPError=-2147217396
- https://github.com/msysgit/msysgit/blob/e6cc6bf1bba06c7029b2d59aedb0c975a0c4f6fe/mingw/include/ddk/cfgmgr32.h
But the docs seem to indicate it should be 👍
SPDRP_CAPABILITIES
The function retrieves a bitwise OR of the following CM_DEVCAP_Xxx flags in a DWORD. The device capabilities that are represented by these flags correspond to the device capabilities that are represented by the members of the DEVICE_CAPABILITIES structure. The CM_DEVCAP_Xxx constants are defined in Cfgmgr32.h.
https://msdn.microsoft.com/en-us/library/windows/hardware/ff551967(v=vs.85).aspx
src/detection_win.cpp
Outdated
@@ -423,6 +424,21 @@ void ExtractDeviceInfo(HDEVINFO hDevInfo, SP_DEVINFO_DATA* pspDevInfoData, TCHAR | |||
// Use this to extract VID / PID | |||
extractVidPid(buf, resultItem); | |||
} | |||
|
|||
DWORD dwCap = 0x0; | |||
if (DllSetupDiGetDeviceRegistryProperty(hDevInfo, pspDevInfoData, SPDRP_CAPABILITIES, &DataT, (PBYTE)&dwCap, 4, &nSize)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 4
instead of buffSize
like the other instances we call DllSetupDiGetDeviceRegistryProperty
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we are trying to get a DWORD, so it is exactly 4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use sizeOf(DWORD)
or something similar?
Why do we use buffSize
for all other instances? What's different about this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it is defined as;
typedef unsigned long DWORD;
And will not change in the future.
But no problem we can use sizeof(DWORD).
Why do we use buffSize for all other instances? What's different about this one?
In other instances we are reading data to buf variable. In this instance we are reading data to dwCapabilities which is a DWORD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forget to push (no sizeof
in the code atm)?
Perhaps even better sizeof(dwCapabilities)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/detection_win.cpp
Outdated
@@ -423,6 +424,21 @@ void ExtractDeviceInfo(HDEVINFO hDevInfo, SP_DEVINFO_DATA* pspDevInfoData, TCHAR | |||
// Use this to extract VID / PID | |||
extractVidPid(buf, resultItem); | |||
} | |||
|
|||
DWORD dwCap = 0x0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dwCapabilities
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am OK with that
src/detection_win.cpp
Outdated
DWORD dwCap = 0x0; | ||
if (DllSetupDiGetDeviceRegistryProperty(hDevInfo, pspDevInfoData, SPDRP_CAPABILITIES, &DataT, (PBYTE)&dwCap, 4, &nSize)) { | ||
// If device has a CM_DEVCAP_UNIQUEID capability, then we assume it has a serial number | ||
if ((dwCap & CM_DEVCAP_UNIQUEID) == CM_DEVCAP_UNIQUEID) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for reference, I found this other usage,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think our way is better & more clear.
Hi, |
src/detection_win.cpp
Outdated
@@ -7,6 +7,7 @@ | |||
|
|||
// Include Windows headers | |||
#include <windows.h> | |||
#include <Cfgmgr32.h> // CM_DEVCAP_UNIQUEID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's put the comment above
// Include `CM_DEVCAP_UNIQUEID`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
src/detection_win.cpp
Outdated
@@ -7,6 +7,7 @@ | |||
|
|||
// Include Windows headers | |||
#include <windows.h> | |||
#include <Cfgmgr32.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see it listed here
- https://msdn.microsoft.com/en-us/library/windows/hardware/mt813619%28v=vs.85%29.aspx?f=255&MSPPError=-2147217396
- https://github.com/msysgit/msysgit/blob/e6cc6bf1bba06c7029b2d59aedb0c975a0c4f6fe/mingw/include/ddk/cfgmgr32.h
But the docs seem to indicate it should be 👍
SPDRP_CAPABILITIES
The function retrieves a bitwise OR of the following CM_DEVCAP_Xxx flags in a DWORD. The device capabilities that are represented by these flags correspond to the device capabilities that are represented by the members of the DEVICE_CAPABILITIES structure. The CM_DEVCAP_Xxx constants are defined in Cfgmgr32.h.
https://msdn.microsoft.com/en-us/library/windows/hardware/ff551967(v=vs.85).aspx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates!
One nit comment and question here #62 (comment)
@doganmurat Still a question here, #62 (comment) |
Hi @MadLittleMods , |
@doganmurat Same comment, #62 (comment) |
Hi, |
@doganmurat I linked the specific |
@doganmurat Thanks for the contribution 💖 I merged and created v3.1.0, https://github.com/MadLittleMods/node-usb-detection/releases/tag/v3.1.0 I am going to wait for the prebuild binaries to trickle in from Travis and AppVeyor and publish the release to npm in the morning 🚢 |
Hi @MadLittleMods , Good news! ✌️ |
@doganmurat v3.1.0 published to npm 🚀, https://www.npmjs.com/package/usb-detection |
Fix #36
Add Serial support for Windows OS
Alternative to #50