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

BLE support #25

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

BLE support #25

wants to merge 6 commits into from

Conversation

yhql
Copy link
Contributor

@yhql yhql commented May 24, 2022

This PR introduces support for BLE APDU exchanges, using bleak.

When enumerating devices, it scans connected BLE devices and keeps the ones whose name start with "Nano X", which is the default naming.
It does not handle pairing to a device.

This change was tested on Windows 10.

A second commit also displays some information about the currently connected device while in verbose mode.

Co-authored-by: @greenknot

@jibeee
Copy link
Contributor

jibeee commented Jul 1, 2022

BLE enumeration is time consuming, so every ledgerctl command becomes slow.
Here is a simple test I made under Windows:

  • Without BLE:
PS C:\Users\jb\Documents\ledgerctl> Measure-Command { python .\ledgerctl.py info }


Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 0
Milliseconds      : 870
Ticks             : 8707504
TotalDays         : 1,00781296296296E-05
TotalHours        : 0,000241875111111111
TotalMinutes      : 0,0145125066666667
TotalSeconds      : 0,8707504
TotalMilliseconds : 870,7504
  • With BLE (no paired device):
PS C:\Users\jb\Documents\ledgerctl> Measure-Command { python .\ledgerctl.py info }


Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 2
Milliseconds      : 652
Ticks             : 26526521
TotalDays         : 3,07019918981481E-05
TotalHours        : 0,000736847805555556
TotalMinutes      : 0,0442108683333333
TotalSeconds      : 2,6526521
TotalMilliseconds : 2652,6521

I think BLE, as for TCP, should be disabled by default. It could be enabled with an environment variable.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 1, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@yhql
Copy link
Contributor Author

yhql commented Jul 1, 2022

@jibeee should be fixed by commit c8aba20

@codecov-commenter
Copy link

codecov-commenter commented Jul 1, 2022

Codecov Report

Patch and project coverage have no change.

Comparison is base (fe73e30) 0.00% compared to head (3ba98a8) 0.00%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #25    +/-   ##
========================================
  Coverage     0.00%   0.00%            
========================================
  Files           20      21     +1     
  Lines         1252    1359   +107     
========================================
- Misses        1252    1359   +107     
Impacted Files Coverage Δ
ledgerwallet/client.py 0.00% <0.00%> (ø)
ledgerwallet/transport/__init__.py 0.00% <0.00%> (ø)
ledgerwallet/transport/ble.py 0.00% <0.00%> (ø)
ledgerwallet/transport/hid.py 0.00% <0.00%> (ø)
ledgerwallet/transport/tcp.py 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@lpascal-ledger
Copy link
Contributor

What's the state of this PR?

@yhql
Copy link
Contributor Author

yhql commented Mar 2, 2023

First comments from @jibeee were fixed, but it's definitely not super stable and needs more field testing.

@lpascal-ledger
Copy link
Contributor

And a rebase :3

@yhql yhql force-pushed the ble_support branch 2 times, most recently from 5fc73f6 to 913cd1f Compare March 3, 2023 09:25
@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@lpascal-ledger lpascal-ledger changed the base branch from develop to master May 29, 2023 13:45
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.

4 participants