Skip to content
This repository has been archived by the owner on Jun 27, 2022. It is now read-only.

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

Merged
merged 17 commits into from
Jul 31, 2018
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/hw-transport-node-hid/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@
"license": "Apache-2.0",
"dependencies": {
"@ledgerhq/hw-transport": "^4.19.0",
"node-hid": "^0.7.2"
"node-hid": "^0.7.2",
"usb-detection": "^3.1.0"
},
"devDependencies": {
"flow-bin": "^0.68.0",
Expand Down
8 changes: 7 additions & 1 deletion packages/hw-transport-node-hid/src/TransportNodeHid.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ function defer<T>(): Defer<T> {

let listenDevicesPollingInterval = 500;
let listenDevicesPollingSkip = () => false;
let listenDevicesDebugMode = false;

/**
* node-hid Transport implementation
Expand Down Expand Up @@ -68,6 +69,10 @@ export default class TransportNodeHid extends Transport<string> {
listenDevicesPollingSkip = conditionToSkip;
};

static setListenDevicesDebugMode = (bool: boolean) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.*

listenDevicesDebugMode = bool;
};

/**
*/
static listen = (
Expand All @@ -85,7 +90,8 @@ export default class TransportNodeHid extends Transport<string> {
});
const { events, stop } = listenDevices(
listenDevicesPollingInterval,
listenDevicesPollingSkip
listenDevicesPollingSkip,
listenDevicesDebugMode
);

const onAdd = device => {
Expand Down
102 changes: 84 additions & 18 deletions packages/hw-transport-node-hid/src/listenDevices.js
Original file line number Diff line number Diff line change
@@ -1,21 +1,31 @@
// @flow

import EventEmitter from "events";
import usbDetect from "usb-detection";
import getDevices from "./getDevices";

const VENDOR_ID = 11415; // Ledger's Vendor ID for filtering
const MAX_ATTEMPTS = 10;

export default (
delay: number,
listenDevicesPollingSkip: () => boolean
listenDevicesPollingSkip: () => boolean,
debugMode: boolean
): {
events: EventEmitter,
stop: () => void
} => {
const events = new EventEmitter();
events.setMaxListeners(0);

let timeoutDetection;
let listDevices = getDevices();

const debug = (...args) => {
if (debugMode && args[0]) {
console.log("[listenDevices]", ...args);
}
};

const flatDevice = d => d.path;

const getFlatDevices = () => [
Expand All @@ -27,37 +37,93 @@ export default (

let lastDevices = getFlatDevices();

const checkDevices = () => {
const poll = (type, attempt = 1) => {
let changeFound = false;

if (!listenDevicesPollingSkip()) {
debug(`Polling for ${type} [attempt ${attempt}/${MAX_ATTEMPTS}]`);

const currentDevices = getFlatDevices();

const newDevices = currentDevices.filter(d => !lastDevices.includes(d));
const removeDevices = lastDevices.filter(
d => !currentDevices.includes(d)
);
if (type === "add") {
const newDevices = currentDevices.filter(d => !lastDevices.includes(d));

if (newDevices.length > 0) {
debug("New device found:", newDevices);

listDevices = getDevices();
events.emit("add", getDeviceByPaths(newDevices));

if (newDevices.length > 0) {
listDevices = getDevices();
events.emit("add", getDeviceByPaths(newDevices));
changeFound = true;
} else {
debug("No new device found");
}
}

if (removeDevices.length > 0) {
events.emit("remove", getDeviceByPaths(removeDevices));
listDevices = listDevices.filter(
d => !removeDevices.includes(flatDevice(d))
if (type === "remove") {
const removeDevices = lastDevices.filter(
d => !currentDevices.includes(d)
);

if (removeDevices.length > 0) {
debug("Removed device found:", removeDevices);

events.emit("remove", getDeviceByPaths(removeDevices));
listDevices = listDevices.filter(
d => !removeDevices.includes(flatDevice(d))
);

changeFound = true;
} else {
debug("No removed device found");
}
}

if (changeFound) {
lastDevices = currentDevices;
} else {
if (attempt < MAX_ATTEMPTS) {
const newDelay = delay * attempt;

debug(`Repolling ${type} in ${newDelay}ms`);

setTimeout(() => {
poll(type, attempt + 1);
}, newDelay);
} else {
debug(`Giving up after ${attempt} attempts`);
}
}
} else {
debug(`Polling skipped, retrying in ${delay}ms`);

lastDevices = currentDevices;
setTimeout(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

poll(type);
Copy link
Contributor

Choose a reason for hiding this comment

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

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 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 🤔

}, delay);
}
setTimeout(checkDevices, delay);
};

timeoutDetection = setTimeout(checkDevices, delay);
debug("Starting to monitor USB for ledger devices");
usbDetect.startMonitoring();

// Detect add
usbDetect.on(`add:${VENDOR_ID}`, device => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

👌

debug("Device add detected:", device.deviceName);
Copy link
Contributor

Choose a reason for hiding this comment

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

and this device don't have the path ? 😢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

🆗


poll("add");
});

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

poll("remove");
});

return {
stop: () => {
clearTimeout(timeoutDetection);
debug("Stopping USB monitoring");
usbDetect.stopMonitoring();
},
events
};
Expand Down