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

Devices enumeration: workaround for MacOS #6

Merged

Conversation

andreibancioiu
Copy link
Contributor

@andreibancioiu andreibancioiu commented May 12, 2023

On MacOS >= 13.3.1, device enumeration produces more records, e.g.

hid_device {'path': b'DevSrvsID:4294970322', 'vendor_id': 11415, 'product_id': 4117, 'serial_number': '0001', 'release_number': 513, 'manufacturer_string': 'Ledger', 'product_string': 'Nano S', 'usage_page': 65440, 'usage': 1, 'interface_number': 0}
hid_device {'path': b'DevSrvsID:4294970324', 'vendor_id': 11415, 'product_id': 4117, 'serial_number': '0001', 'release_number': 513, 'manufacturer_string': 'Ledger', 'product_string': 'Nano S', 'usage_page': 61904, 'usage': 1, 'interface_number': 0}

The order of the enumeration is not deterministic, and, sometimes, HID.enumerate_devices(self.vendor_id)[0] might pick an unusable record (not the actual device). In this PR, we implement the device picking logic as a new function, _decide_device_path, which handles the behavior of MacOS >= 13.3.1, as well.

The logic of HID.enumerate_devices() is left untouched - in case downstream applications were actually relying on the results being a collection of paths, instead of a single path.

How to test

Clone the repository and, while having a device connected (with the MultiversX app open - as an example), run the following command:

python3 -m ledgercomm.cli.send --hid stdin <<<ed02000000

It should work with no error (on all operating systems). Output should be similar to:

ledgercomm - DEBUG - => ed02000000
ledgercomm - DEBUG - <= 0102000100140000000000000000 9000

Extra references:

@andreibancioiu andreibancioiu marked this pull request as ready for review May 12, 2023 12:46
@lpascal-ledger lpascal-ledger changed the base branch from master to develop May 23, 2023 16:01
@lpascal-ledger
Copy link
Contributor

Thanks for the PR @andreibancioiu! We'll need some time to test it on a proper device, but it'll definitely be integrated then.

@lpascal-ledger lpascal-ledger changed the base branch from develop to master May 29, 2023 13:48
@fbeutin-ledger
Copy link
Contributor

Hello @andreibancioiu , sorry about this the CI is failing.. Can you take a quick look please? I'll do it next week otherwise

@andreibancioiu
Copy link
Contributor Author

andreibancioiu commented Jun 9, 2023

Hello @andreibancioiu , sorry about this the CI is failing.. Can you take a quick look please? I'll do it next week otherwise

Hello @fbeutin-ledger,

Thanks for the note 🙏

Fixed the linting issues:

  • I was using the deprecated LOG.warn() instead LOG.warning()
  • I was raising the base Exception at some point. Fixed by defining a custom error: CannotFindDeviceError

@fbeutin-ledger fbeutin-ledger changed the base branch from master to fbe/merge_mac_support June 14, 2023 08:20
@fbeutin-ledger fbeutin-ledger merged commit 1151a8e into LedgerHQ:fbe/merge_mac_support Jun 14, 2023
@fbeutin-ledger
Copy link
Contributor

Merged into LedgerHQ:fbe/merge_mac_support
I'll fix the lint there

@fbeutin-ledger
Copy link
Contributor

#14

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.

3 participants