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

Remove USE_POLL #507

Merged
merged 1 commit into from Apr 24, 2022
Merged

Remove USE_POLL #507

merged 1 commit into from Apr 24, 2022

Conversation

thegecko
Copy link
Member

Changes

  • In preparation for making this addon context aware, plan to drop some complexity as I'm not aware this is used any longer.

Checklist

Have you...

  • Tested the change acts as expected
  • Checked the change doesn't remove or change existing functionality
  • Considered how the feature impacts the product and tested around it (not just the happy paths)
  • Where possible, executed the hardware tests on multiple operating systems

@thegecko thegecko merged commit 774c698 into master Apr 24, 2022
@thegecko thegecko deleted the drop-use-poll branch April 24, 2022 14:43
@mildsunrise
Copy link
Collaborator

it's a bit sad that due to one platform (Windows) we'll need an additional thread + completion queue on platforms that support efficient integration with the event loop... but hotplug isn't a very critical functionality so I'm good with dropping efficiency if it reduces complexity.

@thegecko
Copy link
Member Author

thegecko commented Apr 25, 2022

To be honest, it wasn't clear to me what this did, but as it's currently ignored at build time it seemed like an unnecessary overhead to maintain.

What is it on Windows which isn't supported?

@kevinmehall
Copy link
Contributor

On Unix, libusb can return a set of file descriptors that can be polled for events by Node's own libuv event loop. This isn't supported on Windows, so the cross-platform way is to run a thread that loops on libusb's blocking libusb_handle_events and have the callbacks on that thread wake the Node event loop. This is for transfer completion events too, not just hotplug.

Looks like I had it briefly turned on for Linux / Mac in 2013 and then reverted due to issues I never got around to debugging. Removing is probably the right call.

@thegecko
Copy link
Member Author

Thanks for the detail @kevinmehall

@mildsunrise
Copy link
Collaborator

ah true, I forgot this was also for real USB I/O as well...

hmm, in that case I'd like to implement it someday, for I/O heavy apps I believe the extra efficieny is worth the added complexity (and the theoretical consequence for ABI stability is much more lax in practice).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants