Only poll USB devices list when a change is detected #191

MortalKastor · 2018-07-24T17:07:08Z

No description provided.

gre

good workaround for now. I wish we could find a clean native solution that have everything out of the box.

not sure if all my comments make sense. this is for sure a tricky topic, and also that will need to test intensively, with multiple devices and on all OSs

gre · 2018-07-24T17:33:03Z

packages/hw-transport-node-hid/src/TransportNodeHid.js

@@ -68,6 +69,10 @@ export default class TransportNodeHid extends Transport<string> {
    listenDevicesPollingSkip = conditionToSkip;
  };

+  static setListenDevicesDebugMode = (bool: boolean) => {


it would be better if we would allow to set a log function instead. I moved to this paradigm in ledgerjs recently too, because on Ledger Live we use a custom logger, which is not console.*

gre · 2018-07-24T17:35:32Z

packages/hw-transport-node-hid/src/listenDevices.js

+  usbDetect.startMonitoring();
+
+  // Detect add
+  usbDetect.on(`add:${VENDOR_ID}`, device => {


https://github.com/LedgerHQ/ledgerjs/blob/master/packages/hw-transport-node-hid/src/isLedgerDevice.js

maybe we should filter like it was here?

This is a pre-filter, taking advantage of the vendor ID filtering offered by usb-detection, our isLedgerDevice is used by getDevices afterwards

gre · 2018-07-24T17:36:35Z

packages/hw-transport-node-hid/src/listenDevices.js

+
+  // Detect add
+  usbDetect.on(`add:${VENDOR_ID}`, device => {
+    debug("Device add detected:", device.deviceName);


and this device don't have the path ? 😢

No, it doesn't 😭 MadLittleMods/node-usb-detection#19

gre · 2018-07-24T17:38:16Z

packages/hw-transport-node-hid/src/listenDevices.js


-      lastDevices = currentDevices;
+      setTimeout(() => {


this timeout does not get cleared if the whole listen is stop()-ed

gre · 2018-07-24T17:38:58Z

packages/hw-transport-node-hid/src/listenDevices.js


-      lastDevices = currentDevices;
+      setTimeout(() => {
+        poll(type);


we have to poll multiple times because event happen too soon? Is it always the case, if so it would be great to estimate a minimal time and do an initial timeout to likely never have to poll twice. 🤔

I think it would maybe preferable to refactor things using lodash debounce . each time there are remove/add events you just "push to 50ms later" the re-poll() , and the else case also is about triggering a poll().

e.g.

poll = debounce(() => { }, delay)

not sure what delay will be , if it make sense to be the same 1s that was before 🤔

we have to poll multiple times because event happen too soon?

From my tests I have to poll a second time (after 500ms) only for device removal on macOS and Windows, but I have no idea how much this behavior is consistent from machine to machine 🤔

gre

lil detail but rest LGTM! we'll give some try asap

gre · 2018-07-25T18:27:23Z

packages/hw-transport-node-hid/package.json

@@ -26,7 +26,9 @@
  "license": "Apache-2.0",
  "dependencies": {
    "@ledgerhq/hw-transport": "^4.19.0",
-    "node-hid": "^0.7.2"
+    "lodash.debounce": "^4.0.8",


it's a lil detail, but I would prefer we depend on lodash and doing import debounce from "lodash/debounce" , like on live

gre · 2018-07-25T18:28:31Z

packages/hw-transport-node-hid/src/listenDevices.js

+  usbDetect.startMonitoring();
+
+  // Detect add
+  usbDetect.on(`add:${VENDOR_ID}`, device => {


gre · 2018-07-25T18:28:40Z

packages/hw-transport-node-hid/src/listenDevices.js

+
+  // Detect add
+  usbDetect.on(`add:${VENDOR_ID}`, device => {
+    debug("Device add detected:", device.deviceName);


gre · 2018-07-25T18:41:21Z

approach i suggest to take:

let's do a beta release from this branch (before merge) – see the main README on how to do
we stress test it directly on ledger live (on 3 OS s, multiple devices, using a non dev build too)
if all is fine, we can merge

gre · 2018-07-26T19:08:42Z

if usb appears to be a pain to setup for windows, i suggest we go back to usb-detect, but manually manage the global event listening. for now, we could just listen always, because in LL we do this anwyay. just need to emit the events in some event emitter maybe, so each listen() can independently be created and unsubscribed.

gre · 2018-07-30T14:46:06Z

packages/hw-transport-node-hid/src/isLedgerDevice.js

-export default d =>
-  (["win32", "darwin"].includes(process.platform)
-    ? d.usagePage === 0xffa0
-    : d.interface === 0) &&


^ this wasn't a problem on linux? to filter the potential multi devices?

My bad.
I restored the filtering.

My usagePage is 0xf1d0, which means that devices aren't returned by that function. Is that normal?

0xF1D0 is specific to FIDO (👀) U2F which is, indeed, purposefully filtered out so we don't list devices with the U2F app open, or a currency app with Browser Support set to on.

Oh, so I need to disable browser support when using hw-transport-node-hid, I see! Sorry, the documentation is a bit scarce and I'm a newbie 😉. Thanks!

gre

🚀

gre · 2018-07-30T17:31:35Z

let's all test together one more time on Linux, Windows and Mac and in Production build mode

gre · 2018-07-31T09:06:54Z

let's rename setListenDevicesPollingInterval to setListenDevicesDebounce

MortalKastor and others added 6 commits July 23, 2018 15:51

usb-detection package added

33a2dee

debug flag setter for listenDevices

4442a98

Only poll when a device add or removal is detected

f070025

Factorize add and remove polling

8566412

Limit polling attempts per change detection

9483bd9

Merge branch 'master' into usb-detect

199a2ee

gre reviewed Jul 24, 2018

View reviewed changes

MortalKastor added 3 commits July 25, 2018 16:42

Switch to lodash debounce

9b5451b

Ability to set custom debug log function

3b897f3

Flow fixes

83f13c3

MortalKastor requested a review from gre July 25, 2018 16:02

gre approved these changes Jul 25, 2018

View reviewed changes

MortalKastor added the HODL Don't merge! label Jul 26, 2018

MortalKastor self-assigned this Jul 26, 2018

MortalKastor added 5 commits July 26, 2018 11:15

Merge branch 'master' into usb-detect

c862d38

Change depedency to complete lodash

b98dfe6

Switched to "usb" package for its better listener handling

c440e5e

Improved debounce loading

64dad39

Merge branch 'master' into usb-detect

ae6f5c5

MortalKastor force-pushed the usb-detect branch from c425acc to ae6f5c5 Compare July 26, 2018 16:11

This was referenced Jul 29, 2018

USB performance LedgerHQ/ledger-live-desktop#1322

Closed

use usb events instead of polling HID.devices() LedgerHQ/ledger-live-desktop#1058

Closed

Use native HID API to filter for Ledger devices

666f6ba

gre approved these changes Jul 30, 2018

View reviewed changes

gre reviewed Jul 30, 2018

View reviewed changes

Restore interface filtering

96c1e35

gre approved these changes Jul 30, 2018

View reviewed changes

setListenDevicesPollingInterval renamed to reflect new behavior

a1987cb

MortalKastor mentioned this pull request Jul 31, 2018

Use new USB device detection from ledgerjs LedgerHQ/ledger-live-desktop#1340

Merged

gre merged commit 067be87 into LedgerHQ:master Jul 31, 2018

MortalKastor removed the HODL Don't merge! label Jul 31, 2018

filoozom mentioned this pull request Aug 7, 2018

[TransportNodeHid] getDevices looks for potentially wrong usagePage #199

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only poll USB devices list when a change is detected #191

Only poll USB devices list when a change is detected #191

MortalKastor commented Jul 24, 2018

gre left a comment

gre Jul 24, 2018

gre Jul 24, 2018

MortalKastor Jul 25, 2018

gre Jul 25, 2018

gre Jul 24, 2018

MortalKastor Jul 25, 2018

gre Jul 25, 2018

gre Jul 24, 2018

gre Jul 24, 2018

MortalKastor Jul 25, 2018

gre left a comment

gre Jul 25, 2018

gre Jul 25, 2018

gre Jul 25, 2018

gre commented Jul 25, 2018

gre commented Jul 26, 2018

gre Jul 30, 2018

MortalKastor Jul 30, 2018

filoozom Aug 7, 2018

MortalKastor Aug 7, 2018

filoozom Aug 7, 2018

gre left a comment

gre commented Jul 30, 2018

gre commented Jul 31, 2018

Only poll USB devices list when a change is detected #191

Only poll USB devices list when a change is detected #191

Conversation

MortalKastor commented Jul 24, 2018

gre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gre commented Jul 25, 2018

gre commented Jul 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gre left a comment

Choose a reason for hiding this comment

gre commented Jul 30, 2018

gre commented Jul 31, 2018