-
-
Notifications
You must be signed in to change notification settings - Fork 721
BLE transport for trezorlib #4948
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
Conversation
|
| model | device_test | click_test | persistence_test |
|---|---|---|---|
| T2T1 | test(all) main(all) ![]() |
test(all) main(all) ![]() |
test(all) main(all) ![]() |
| T3B1 | test(all) main(all) ![]() |
test(all) main(all) ![]() |
test(all) main(all) ![]() |
| T3T1 | test(all) main(all) ![]() |
test(all) main(all) ![]() |
test(all) main(all) ![]() |
| T3W1 | test(all) main(all) ![]() |
test(all) main(all) ![]() |
test(all) main(all) ![]() |
Latest CI run: 17658231521
da3c742 to
e2f4edb
Compare
ea387e6 to
287c0a6
Compare
287c0a6 to
5ca3d38
Compare
romanz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK, thanks!
64286bb to
4ed1e0d
Compare
|
@matejcik want to take a quick look? There are some potentially intrusive changes (async, persistent subprocess), then again none of it gets run when bleak is not installed. |
matejcik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, very reasonable actually
|
I'll rebase to resolve the conficts. |
git range-diff output |
7be7463 to
713e512
Compare
|
Tested locally: # trezorctl ble connect
Scanning...
Found 1 BLE device(s)
Device: T3W1 (ABC), 11:22:33:44:55:66
Connecting to T3W1 (ABC)...
Connected
# TREZOR_PATH=ble:11:22:33:44:55:66 trezorctl ping 123
Please confirm action on your Trezor device.
123Sometimes it prints |
713e512 to
4eca838
Compare
|
Good point, that is annoying. In 4eca838 BLE is enabled only if:
Also rebased on top of the uv changes. Please take a look. |
romanz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Co-authored-by: tychovrahe <[email protected]>
[no changelog]
4eca838 to
d78ff8b
Compare




































Uses cross-platform bleak library (you need to run
poetry install). The library is async so we run it in a separate process and trezorlib talks to it using a pipe. It could potentially work on macOS but I haven't tested yet.Bleak has a particular way of working with BLE devices, namely that it cannot see devices that are already connected to the system. This could affect Suite interoperability and may have to be worked around in platform-specific ways. For trezorlib to see a connected device you have to manually disconnect it first. Trezorlib disconnects from a connected device automatically using
atexit.For pairing to work you need to have the system pairing dialog open, then run
trezorctl ble connect. In linux it would be possible to enter the pairing code through trezorlib, but this is IIUC impossible on macOS (see also hbldh/bleak#1100).If I remember correctly the original BLE support on which this is based was written by @TychoVrahe.