Filter out non-APDU interfaces in UsbTransport and other changes related to issue #12 - #14
Conversation
| edition = "2021" | ||
| license = "Apache-2.0" | ||
|
|
||
| [features] |
There was a problem hiding this comment.
FYI: changes in this file are only needed to be able to override the backend from command line when building ledger-cli (e.g. cargo run --no-default-features --features transport_usb_hidraw --bin ledger-cli -- list)
| transport_ble = [ "btleplug" ] | ||
|
|
||
| # Switch libusb backends, `libusb` works better with WSL so we're using that by default | ||
| # Switch libusb backends, `libusb` works better with WSL and plain Linux so we're using that by default |
There was a problem hiding this comment.
FYI: I had a situation on my Ubuntu where HidApi::devices_list only returned the FIDO interface and not the APDU one when using the hidraw backend, but it returned both interfaces when using libusb. The issue was solved by physically disconnecting and reconnecting the device. So it looks like hidraw is unreliable in general and not only with WSL.
…s UnknownModel, ApplicationLoaded; add .vscode to .gitignore
118d970 to
6c116c1
Compare
…!Send" and "impl Send" for UsbTransport/UsbDevice.
6c116c1 to
36673bb
Compare
yogh333
left a comment
There was a problem hiding this comment.
LGTM, could you please run clippy in order to make tCI happy? Thanks
Done |
|
Are there any plans to merge this? |
|
I've pushed one more commit, making futures returned by Also, Also, I've updated min Rust version to 1.85, because it's what one of dependencies needs. |
Edit: initially I wanted to create several PRs on top of each other, but it seems that GitHub can't do that when PRs come from a fork. So I've put all my changes into a single PR.
I have 4 commits here:
"Filter out non-APDU interfaces in UsbTransport".
This addresses the "USB interface selection" part of A few issues related to device selection #12.
"Improve BLE device discovery, support new models".
This addresses the "Bluetooth device selection" part of A few issues related to device selection #12. Also, Flex and NanoGen5 were added to the list of models.
I've only checked BT connectivity on Nano Gen 5, but since the corresponding UUIDs come from Ledger Live source code, I'm pretty sure it should work for other models too.
Though additional testing won't hurt of course.
"Replace Error::Unknown with more specific errors".
Not related to A few issues related to device selection #12. It's just that Error::Unknown is way too vague, so I've added a few more specific variants instead. Also, a couple of Error variants were unused, so I removed them.
"UsbTransport and UsbDevice are no longer Send. Remove explicit "impl !Send" and "impl Send" for UsbTransport/UsbDevice".
This addresses the last item in A few issues related to device selection #12 - UsbTransport/UsbDevice not being Send but being marked as Send anyway.
As I've said in the issue, IMO UsbTransport/UsbDevice should be Send. But since I haven't got a reply to that, I decided to go the other way and actually make them non-Send.
So,
when the Send bound on returned futures is not needed, async functions are declared directly and the corresponding warningledger-libno longer uses theasync-traitcrate; instead,("use of async fn in public traits is discouraged as auto trait bounds cannot be specified") is silenced via
#[allow(async_fn_in_trait)].If the Send bound is needed, then the more verbose syntax
fn foo() -> impl Future<...> + Sendis used.Note that there is also the trait-variant crate whose
makemacro allows to have both the normal "async fn" syntax in traits and the Send bound on the futures.But it looks like the crate is unmaintained and
makewas producing some weird errors in cases when a trait method had a body. So I guess the verbose syntax is the way.I also had to introduce an additional trait
NonSendExchange, which is likeExchangebut without theSendbound. It's needed becauseUsbDevicecan't implementExchange.Also, just in case, I bumped minimum hidapi version to "2.6" (the previously referenced v2.1 required that only one instance of HidApi can exist at time, this restriction was lifted in later versions).
P.S.
async-traitis still used in other places. Probably I should remove it there too, for consistency?