Rewrite based around napi and context aware

[Julusian]

fixes: #102, #119, #57, #116, #42, #53
replaces: #113, #125, #118

I haven't fixed up mac or windows yet, but shall get to that soon.

This has become quite a large rewrite due to how parts were written.
I have tried to split it across commits in different phases to make it easier to understand.

Support for anything below node 10 has been dropped, due to limitations of napi. I dont see this as a problem as everything still officially supported is covered. The benefit to this is that the builds are guaranteed to work in future versions of node without needing to be recompiled. There is also no need to recompile for electron, as it will use the same binaries

All the usages of #include<uv.h> have to be replaced as they are not abi safe. Because of this I have pretty much rewritten the contents of detection_linux.cpp, taking inspiration and reusable snippets from the old file.

To be context aware, nothing can be in the global scope so various bits had to be refactored to remove any use of statics.

For now this change is not trying to optimise for multiple threads, so is initialising a new listener/watcher for each thread. This can be improved on later.

For now this I have avoided any api changes, but it would be nice to make some to better reflect ownership of the listener. One concern I have is that if two libraries uses this and one calls stopMonitoring, that will stop events for the other or the root application. It would be nice to be able to scope listeners instead, by returning something from the start function that will allow for manual stopping and restarting, and will auto-stop when it gets garbage collected.

[MadLittleMods]

Hey @Julusian,

Sorry for the delay. These are huge refactor changes that are daunting for me to pop-in and review. I've been putting it off and it would be nice if someone else can come help review this code as well.

Mind putting some comments in places to explain any bits or why you chose that way, etc?

---

One concern I have is that if two libraries uses this and one calls stopMonitoring, that will stop events for the other or the root application. It would be nice to be able to scope listeners instead, by returning something from the start function that will allow for manual stopping and restarting, and will auto-stop when it gets garbage collected.

Very interesting edge case! I'd be fine with iterating this in another PR and make a major breaking change. Let's wait on that until I can manage this PR though.

[Julusian]

@MadLittleMods no problem, I havent gotten around to doing the work for mac or windows yet, I have gotten distracted on other projects.
I can understand that. I shall add some comments and double check naming and things when I can. I shall let you know once I think it is ready for review

[Julusian]

@MadLittleMods I think this is ready to be looked over. I would suggest not trying to read the diff, but instead reading just the new code, and having the old code to hand as a reference to explain why some things are as they are. A lot of it isnt new, but it has been shuffled around a lot so really confuses the diffs. I havent added much on comments, I shall try and give it another look over, but please do ask questions to help direct what need comments

I have rewritten and tested all 3 platforms, but some additional testing and bugfixing is needed.

I am aware of a segfault on mac that occurs when multiple worker_threads are constanting starting and stopping monitoring.

I think I have checked for memory leaks when starting and stopping monitoring (revealing the mac segfault). My criteria for a leak was no notable memory usage change after at least an hour of starting and stopping monitoring every 100ms.
I still need to rig a stress/leak test with a usb device being added and removed.

I should note, that I have tested the mac version exclusively on M1, as that is what I have easier access to and it should behave the same. As part of this, the mac build is a multiarch binary, which will resolve #141

[Julusian]

@thegecko I did not notice that node-usb supports hotplug events, and through libusb which should make everything simpler and much more maintainable.
Theres more code here than in node-usb, which feels wrong for something with such a limited scope.

Can you think of any reason not to deprecate this library and instead point users to using node-usb?

It looks like this project predates libusb hotplug detection by a few months, and there isnt any other reference to libusb, so I suspect it hasn't been considered moving to libusb before. Lack of official windows support is another issue that you appear to be handling.

[thegecko]

Can you think of any reason not to deprecate this library and instead point users to using node-usb?

Thanks for raising this, this has come to my mind recently, too. There are reasons not to deprecate this library IMO, but it may be better to consider merging them somehow.

Lack of official windows support is another issue that you appear to be handling.

This is our major area of issues now. In my day-job, we develop a product which actually uses both of these libraries due to two issues, both of which are on Windows:

• Libusb doesn't support hotplug events (attach/detach) for Windows (see Windows: libusb Windows hotplug support libusb/libusb#86).
  Support for this was previously hacked into an old fork of libusb and used, but it meant node-usb couldn't take advantage of recent libusb fixes. node-usb recently moved to the upstream libusb implementation, but added the windows hotplug events in JavaScript using polling (see Libusb update to v1.0.23 node-usb/node-usb#453), which is far from ideal.

• On Windows, libusb doesn't always accurately return serial numbers. We have found certain devices with double-byte characters which don't work with just node-usb and we've used an amalgamation of both libraries to resolve this. This issue is a bit more specific and would need a public use case for investigation.

Is there a scenario where we can merge node-usb-detection into node-usb (or at least the windows implementation) to cover the deficiencies in libusb and align efforts? I'd love to see the JavaScript polling removed and native events used instead.

[Julusian]

Is there a scenario where we can merge node-usb-detection into node-usb (or at least the windows implementation) to cover the deficiencies in libusb and align efforts? I'd love to see the JavaScript polling removed and native events used instead.

This is really a disccusion that @MadLittleMods needs to be involved in. I'm a user with no prior contribution history to this project. :)

It occurred to me to check node-usb while I was sitting reviewing my rewritten mac code to find a memory leak, and I was surprised to see that it supported hotplug events. I guess I never thought to check, as why else would this library exist

I can look at writing a PR for node-usb bringing the windows code across, doing that means I can test just the one platform for memory leaks and segfaults.
For a moment I did consider about pulling libusb into here for platforms that support it, but that feels like a lot of effort when node-usb could be used instead

which actually uses both of these libraries due to two issues, both of which are on Windows

To clarify, does your product also run on other oses and they behave fine?

[thegecko]

This is really a disccusion that @MadLittleMods needs to be involved in

Agreed

I can look at writing a PR for node-usb bringing the windows code across, doing that means I can test just the one platform for memory leaks and segfaults.

That would be awesome

To clarify, does your product also run on other oses and they behave fine?

Yes, MacOS and Linux and they both work fine.

[MadLittleMods]

Is there a scenario where we can merge node-usb-detection into node-usb (or at least the windows implementation) to cover the deficiencies in libusb and align efforts? I'd love to see the JavaScript polling removed and native events used instead.

-- @thegecko, #127 (comment)

The reason I started using this project 6 years ago was because it was the only project that actually worked on Windows (and the other OS's). Although not my use case, it also seems like a lot of people use this with Electron so it would be good to have a compatible option there too.

The second criteria is documentation which looks to be available on https://github.com/node-usb/node-usb#usbdetection

A third criteria is it actually being able to be built and installed easily. This means prebuilds or ideally no C++ code necessary if that was even possible.

If those can be addressed(which some already are), then I could see this being deprecated in favor of usb.

[MadLittleMods]

@thegecko (repost that in this thread)

[thegecko]

@MadLittleMods I believe with the v2.x.x release of node-usb, all of your criteria are now met

[MadLittleMods]

@thegecko Tested and seems to work. I think we can move forward with deprecating as I'm running into a wall trying to get prebuilds to work anyway (#165 (comment)).

const { usb, getDeviceList} = require('usb');

const devices = getDeviceList();
console.log('devices', devices);

usb.on('attach', function(device) { console.log('attach', device) });

usb.on('detach', function(device) { console.log('detach', device) });

---

To note some differences and preemptively answer some peoples questions, how do we get deviceName, manufacturer, serialNumber when using the usb package? Maybe I'm just not understanding how to get a USBDevice which seems to have those properties as described in the readme and somehow mixing the legacy API.

usb-detection

{
    locationId: 336592896,
    vendorId: 6353,
    productId: 20193,
    deviceName: 'Pixel 4a',
    manufacturer: 'Google',
    serialNumber: 'xxx',
    deviceAddress: 7
  }

usb

{
  busNumber: 20,
  deviceAddress: 5,
  deviceDescriptor: {
    bLength: 18,
    bDescriptorType: 1,
    bcdUSB: 512,
    bDeviceClass: 0,
    bDeviceSubClass: 0,
    bDeviceProtocol: 0,
    bMaxPacketSize0: 64,
    idVendor: 6353,
    idProduct: 20193,
    bcdDevice: 1088,
    iManufacturer: 1,
    iProduct: 2,
    iSerialNumber: 3,
    bNumConfigurations: 1
  },
  portNumbers: [ 1 ]
}

[thegecko]

The legacy API returns the raw USB data which doesn't undertake further blocking calls to obtain the strings (e.g. serial number, device name, etc.).

You can get these in a manner of ways:

• Use the legacy device you got from getDeviceList() or your legacy event and call getStringDescriptor for each of the string IDs
• Wrap the legacy device into a webusb compatible device which pre-populates these strings
• Use the WebUSB interface to request a device or register for events:

import { WebUSB } from 'usb';

(async () => {
    const customWebUSB = new WebUSB({
        // Bypass checking for authorised devices
        allowAllDevices: true
    });

    customWebUSB.addEventListener('connect', event => console.log(event.device);
})();

[thegecko]

On Windows, libusb doesn't always accurately return serial numbers. We have found certain devices with double-byte characters which don't work with just node-usb and we've used an amalgamation of both libraries to resolve this. This issue is a bit more specific and would need a public use case for investigation.

I have just confirmed this is no longer the case with the latest node-usb release which updates libusb, so the only difference between the libraries is native hotplug support on Windows.
It would be great to see the Windows detection code in this project added to node-usb or better still, fixed in libusb.

The reason I started using this project 6 years ago was because it was the only project that actually worked on Windows (and the other OS's). Although not my use case, it also seems like a lot of people use this with Electron so it would be good to have a compatible option there too.

node-usb now uses node-addon-api and is fully compatible with electron AFAIK. Example app here:

https://github.com/node-usb/node-usb-example-electron

The second criteria is documentation which looks to be available on https://github.com/node-usb/node-usb#usbdetection

Docs do exist. Making these better (especially as we introduce the v2 APIs) is next on my todo list.

A third criteria is it actually being able to be built and installed easily. This means prebuilds or ideally no C++ code necessary if that was even possible.

The recent move to prebuildify and prebuildify-cross creates NAPI v4 compatible binaries which don't require rebuilding across node or electron versions. Prebuilt binaries are shipped for the following architectures:

• android-arm
• android-arm64
• darwin-x64+arm64 (inc. M1)
• linux-arm (e.g. RPi)
• linux-arm64 (e.g. RPi 64)
• linux-ia32
• linux-x64
• win32-ia32
• win32-x64

Does node-usb-detection support any others?

If those can be addressed(which some already are), then I could see this being deprecated in favor of usb.

👍

[hrueger]

Hi all,
I just want to say that I'm now using @Julusian's rewrite.
I'm pulling it from my fork on GitHub because I had to fix an error with prebuilds not being available as well as an issue where the vendor id was used as the product id.

Apart from that, it works great and I'd really like to see this merged to this lib ;-)

Edit: I also find that the stopMonitoring() method is faster and more reliable now, however, this is just my subjective opinion as I could never really reproduce the bugs I had with that.

[thegecko]

I can look at writing a PR for node-usb bringing the windows code across, doing that means I can test just the one platform for memory leaks and segfaults.

@Julusian I've started to think about replacing the windows hotplug polling in node-usb with a native implementation. I'm not a C/C++ guru so wondered if you had started this at all? My first step would be to take the Windows parts of your NAPI port in this PR...

[Julusian]

I can look at writing a PR for node-usb bringing the windows code across, doing that means I can test just the one platform for memory leaks and segfaults.

@Julusian I've started to think about replacing the windows hotplug polling in node-usb with a native implementation. I'm not a C/C++ guru so wondered if you had started this at all? My first step would be to take the Windows parts of your NAPI port in this PR...

@thegecko I haven't started on anything. Its on my todo list and I would really like for it to be done, but windows is my least favourite platform to write c++ on so its been hard to motivate getting started

[thegecko]

Moved node-usb discussion to node-usb/node-usb#488

[mildsunrise]

Since this PR also mentions making node-usb-detection context-aware, cross-linking to the equivalent node-usb issue: node-usb/node-usb#491.

[sagotch]

@Julusian I am not sure to understand the state and the future of this PR.

It is marked as draft, and the only expected issue seems to be

I am aware of a segfault on mac that occurs when multiple worker_threads are constanting starting and stopping monitoring.

Is it still draft because of the lack of reviews or do you plan to continue working on this PR?

I saw your latest efforts was to merge things into node-usb. If you manage to do it, would it means that node-usb would supersede node-usb-detection? In that case do you have guidelines in order to move from node-usb-detection to node-usb?

Context: my goal is only to use https://github.com/LedgerHQ/ledger-live/tree/develop/libs/ledgerjs/packages/hw-transport-node-hid-singleton in an electron14+ app. I saw this: cryptogarageinc/ledger-liquid-lib@25e57a1 but do not know if it is the way to achieve this or not.

Thank you.

cc @thegecko (and please excuse me if this cc is not relevant)

[Julusian]

@sagotch

I do not expect this PR to make any more progress, the current aim is to merge the relevant portions of this into node-usb. I have left it open for visibility until node-usb is capable of proper windows hotplug detection.

Once node-usb/node-usb#524 is merged and released, then some documentation can be written and this library can be deprecated

[thegecko]

As @Julusian mentioned, some docs have now been added to node-usb:

https://github.com/node-usb/node-usb#migrating-from-node-usb-detection

[Julusian]

node-usb now has a release which provides native hotplug detection on all platforms supported by usb-detection (and some extra ones)
It is also node-api based and context-aware, so I think it best to recommend use of that library instead.

As such, I am closing this PR in favour of #169

[hrueger]

Hi @Julusian,
sorry for pinging you again ;-)
Because of node-usb/node-usb#546 I can't migrate to node-usb yet. I'm using my fork of your feat/napi branch which is working great.
However, I can't build for MacOS with electron universal, because the generated binary is not in mach-o format... electron/universal#41 (comment)

Could you give me a hint what build config to change in order to create a mach-o file for MacOS?

Best regards

[hrueger]

Never mind. It seems like @electron/universal cannot process multi-arch Mach-O binaries and thinks that they are not in Mach-O format.