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

Bugfix/fix nano s enumeration bug #2822

Merged

Conversation

id-ms
Copy link
Contributor

@id-ms id-ms commented Sep 1, 2021

Goal was not able to communicate with the Ledger Nano S device.
The device has two HID occurrences on the OS and after the firmware upgrade, one of them became not responsive. That made goal to the hang.

  • fixing the interaction with the Ledger devices (S and X) on the Windows platform.

@nullun
Copy link
Contributor

nullun commented Sep 1, 2021

I just built and tested this and wanted to confirm it resolves the issues I was having here #2665

Thank you @algoidan 👍

@codecov-commenter
Copy link

codecov-commenter commented Sep 1, 2021

Codecov Report

Merging #2822 (356a4e2) into master (9f81a92) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2822   +/-   ##
=======================================
  Coverage   47.12%   47.12%           
=======================================
  Files         349      349           
  Lines       56430    56430           
=======================================
+ Hits        26590    26591    +1     
+ Misses      26860    26857    -3     
- Partials     2980     2982    +2     
Impacted Files Coverage Δ
crypto/merkletrie/trie.go 66.42% <0.00%> (-2.19%) ⬇️
crypto/merkletrie/node.go 91.62% <0.00%> (-1.87%) ⬇️
network/requestTracker.go 70.25% <0.00%> (-0.87%) ⬇️
catchup/service.go 69.35% <0.00%> (ø)
data/transactions/verify/txn.go 45.08% <0.00%> (ø)
ledger/acctupdates.go 62.80% <0.00%> (+0.25%) ⬆️
network/wsPeer.go 74.93% <0.00%> (+0.55%) ⬆️
ledger/blockqueue.go 85.05% <0.00%> (+2.87%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f81a92...356a4e2. Read the comment docs.

// The usb library adds one extra byte to the input passed to the USB device
// so the written bytes are larger than what we've send
// https://github.com/karalabe/hid/blob/9c14560f9ee858c43f40b5cd01392b167aacf4e8/hid_enabled.go#L167
if runtime.GOOS == "windows" && cc > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

consider refactoring so you would use ledger_windows.go for windows-specific logic.

go.sum Show resolved Hide resolved
Copy link
Contributor

@tsachiherman tsachiherman left a comment

Choose a reason for hiding this comment

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

looks good !

@tsachiherman tsachiherman merged commit b44e3d9 into algorand:master Sep 1, 2021
@id-ms id-ms deleted the bugfix/fix-NanoS-enumeration-bug branch November 3, 2021 08: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.

None yet

4 participants