-
Notifications
You must be signed in to change notification settings - Fork 33
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
Support webusb as a backend #99
base: main
Are you sure you want to change the base?
Conversation
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.
A very nice start!
I haven't carefully reviewed the unsafe
, but it's not doing anything as tricky with pointers as the native backends do. You have the ability to keep data in a closure all the way through instead of needing to put everything behind a *mut c_void
.
// todo!() | ||
// } | ||
|
||
pub(crate) async fn claim_interface( |
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.
And that also means making some of the blocking methods async...
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.
Is that a problem? Or do you think we can get away with it? I can maybe move the future to a thread if it blocks too long (tokio does not like that but I also don't want to assume tokio).
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.
This applies to Device::open
, Device::claim_interface
, Device::set_configuration
, Device::reset
, Interface::set_alt_setting
, Interface::clear_halt
.
Some of these operations like set_configuration
, set_alt_setting
, and clear_halt
involve control transfers to the device, so can wait arbitrarily long and really should be async. The problem is that the OS APIs for all of these operations are blocking. As you mention, the "right" way to handle that is using something that runs the blocking operations as a threadpool, like smol's blocking
or tokio spawn_blocking
. Even without wasm, there could be nusb users that want that to avoid blocking their event loops.
However, the question then is which runtime, and I would like to make it possible to use nusb without needing to pull in either big async runtime for (non-wasm) users who only want blocking operations anyway. Could do Cargo feature flags, but that's a maintenance burden.
The other case is WebUSB operations like release_interface
and close
that are triggered by dropping nusb types, since Rust doesn't have async drop yet. Since Interface
and Device
are ref-counted and already wait for transfers to cancel, dropping one already doesn't guarantee that anything is closed right away, so I don't expect this to be a problem. Could offer an async release
method consuming self that waits for drop or errors if other references exist, if you for some reason want to be able to release an interface and then claim it again right away.
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.
Trying this out in #100. The idea is that IoAction::wait()
would be cfg'd out on wasm, and then you could trivially impl<T> IoAction for T where T: Future
.
src/platform/webusb/enumeration.rs
Outdated
manufacturer_string: device.manufacturer_name(), | ||
product_string: device.product_name(), | ||
serial_number: device.serial_number(), | ||
interfaces: { |
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.
It looks like you should be able to get this info from device.configuration.interfaces[].alternate
https://developer.mozilla.org/en-US/docs/Web/API/USBAlternateInterface
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.
The problem is that this call returns empty alternates, hence the manual shenanigans with reading the descriptors.
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.
Hmm. I try to avoid device IO in list_devices
because making requests to random devices can be slow, or even crash them. Less bad in WebUSB, though, because it's only devices selected at the browser prompt.
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.
Hmm I understand that concern. The question is how I get the respective webusb primitives to work then though :/
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.
I just tried it in Chrome on Linux from the JS console and get class/subclass/protocol but not the interface string descriptor. Is that what you're seeing, or is the interface entirely missing?
> d = await navigator.usb.requestDevice({filters: [{}]})
USBDevice {usbVersionMajor: 2, usbVersionMinor: 1, usbVersionSubminor: 0, deviceClass: 0, deviceSubclass: 0, …}
> d.configuration.interfaces[0].alternate
USBAlternateInterface {alternateSetting: 0, interfaceClass: 255, interfaceSubclass: 0, interfaceProtocol: 0, interfaceName: null, …}
That interface string descriptor is also missing on some driver installation configurations on Windows. Though you probably care about that string descriptor more than most because of CMSIS-DAP.
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.
Correct, that interface string descriptor is missing. For J-Link for example it's not relevant but the CMSIS-DAP spec requires us to check it.
let ep_type = self.ep_type; | ||
let endpoint_number = self.endpoint & (!0x80); | ||
let (mut data, len) = data.into_vec(); | ||
spawn_local(async move { |
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.
I need to look at how Rust async and the JS event loop interoperate, but turning the WebUSB promise into a future and spawning a task to await that future seems like extra steps. I think it should be possible to make a JS closure wrapping a Rust function that that copies the data into linear memory, marks the transfer as complete, and notifies the transfer's waker, basically in the place of the event thread that does that on native platforms.
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.
You will have to go through the future though to make this transfer happen. I am not sure which layer exactly you want to shed?
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.
So spawn_local
creates a task with a waker, and JsFuture::from
creates a JS callback that arranges for that waker to be woken when the JS promise completes. That waker then queues a poll of the task, where your code handles the transfer completion, and calls notify_completion
. The common code wakes another waker stored on the transfer (the waker of the task polling the future returned by nusb), which then queues the user's task.
The TransferInner / PlatformTransfer internals are kind of a mismatch here, because other nusb platforms implement their own event loop but JS comes with one. But if you ignore the event loop itself then notify_completion
is effectively a completion callback invoked by "platform code". You could use promise.then()
directly with a callback that calls notify_completion
instead of converting the promise to a JsFuture
. One wrinkle is that it needs both success and error callbacks, where JsFuture
uses Rc
to share state between the two.
Alternatively, I wonder if there would be a clean way to make the nusb returned future somehow poll the JsFuture, instead of reimplementing what JsFuture does, but I think that would be an invasive change to the cross-platform code.
Not essential and not even sure how much of a performance hit it is to have an extra queueMicrotask
hop back to JS, but just a thought.
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.
Ah, yeah, I did not consider the double spawn/waker. I will fix that :) it's bad enough to have so many microticks :D
Cargo.toml
Outdated
[target.'cfg(any(target_os="linux", target_os="android"))'.dependencies] | ||
rustix = { version = "0.38.17", features = ["fs", "event", "net"] } | ||
libc = "0.2.155" | ||
# [target.'cfg(all(any(target_os="linux", target_os="android"), not(target_arch="wasm32")))'.dependencies] |
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.
Even with the conditional it fails to compile. Not sure how to do this one.
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.
Ok consider me confused. Having this last time I tried made it break. It works fine now.
This is pretty far now @kevinmehall. It compiles on all systems and runs the tests and I implemented all Things that remain:
Maybe you have some further input/hints. I am not sure how to round it up. |
src/transfer/mod.rs
Outdated
web_sys::UsbTransferStatus::Ok => Ok(()), | ||
web_sys::UsbTransferStatus::Stall => Err(TransferError::Stall), | ||
web_sys::UsbTransferStatus::Babble => Err(TransferError::Unknown), | ||
_ => unreachable!(), |
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.
This should go in platform/webusb
, not here.
Weird that these are the only values defined. Specifically, what happens if you disconnect the device?
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.
Done :)
I am not sure what happens. I would expect it to stall.
self.waker.register(cx.waker()); | ||
match self.events.try_recv() { |
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.
Nothing ever wakes the waker, and the channel can't block, so how does this ever get notified of an event?
Seems like instead of the channel, you should be sharing an Arc
of the AtomicWaker
and a RefCell<VecDeque<HotplugEvent>>
.
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.
Ah yes indeed! I fixed the usage of the waker.
Why do you think the channel blocks? try_recv
and send
should both be nonblocking?
// todo!() | ||
// } | ||
|
||
pub(crate) async fn claim_interface( |
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.
Trying this out in #100. The idea is that IoAction::wait()
would be cfg'd out on wasm, and then you could trivially impl<T> IoAction for T where T: Future
.
Very rough first implementation of a webusb backend.
Flashing in https://github.com/probe-rs/probe-rs works.
While this works, it's really rough around the edges and straight up does not fully compile. Nevertheless I am already looking for feedback as some of the unsafe primitives of nusb made me struggle a bit so I am sure I did this the wrong way.
Closes #83