Skip to content

Support webusb as a backend #99

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Yatekii
Copy link

@Yatekii Yatekii commented Dec 22, 2024

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

Copy link
Owner

@kevinmehall kevinmehall left a 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(
Copy link
Owner

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...

Copy link
Author

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).

Copy link
Owner

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.

Copy link
Owner

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.

manufacturer_string: device.manufacturer_name(),
product_string: device.product_name(),
serial_number: device.serial_number(),
interfaces: {
Copy link
Owner

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

Copy link
Author

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.

Copy link
Owner

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.

Copy link
Author

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 :/

Copy link
Owner

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.

Copy link
Author

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 {
Copy link
Owner

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.

Copy link
Author

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?

Copy link
Owner

@kevinmehall kevinmehall Dec 23, 2024

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.

Copy link
Author

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]
Copy link
Author

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.

Copy link
Author

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.

@Yatekii
Copy link
Author

Yatekii commented Dec 27, 2024

This is pretty far now @kevinmehall. It compiles on all systems and runs the tests and I implemented all todo!()s.

Things that remain:

  • Defer calls to sync functions in async context. It is unclear how to do this. Essentially: Support webusb as a backend #99 (comment)
    • Device::open
    • Device::claim_interface
    • Device::set_configuration
    • Device::reset
    • Interface::set_alt_setting
    • Interface::clear_halt
  • Remove the fake Sync/Send impl for the WebUsbDevice handle. Internally the UsbDevice has an *mut u8 which is not Send/Sync and I don't know how to change this. Maybe we really need the unsafe impl Send/Sync. Essentially: Support webusb as a backend #99 (comment)
  • Double defer of transfers with two wakers can be simplified. Could also be merged as is and improved later. probe-rs where I want to use this needs async closure stabilization though so this has no hurry at least until 1.85. Essentially: Support webusb as a backend #99 (comment)
  • Device creation does IO calls which is not preferred. Can be merged as is but can potentially be improved. Essentially: Support webusb as a backend #99 (comment)

Maybe you have some further input/hints. I am not sure how to round it up.

web_sys::UsbTransferStatus::Ok => Ok(()),
web_sys::UsbTransferStatus::Stall => Err(TransferError::Stall),
web_sys::UsbTransferStatus::Babble => Err(TransferError::Unknown),
_ => unreachable!(),
Copy link
Owner

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?

Copy link
Author

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.

Comment on lines +82 to +93
self.waker.register(cx.waker());
match self.events.try_recv() {
Copy link
Owner

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>>.

Copy link
Author

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(
Copy link
Owner

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.

@Yatekii Yatekii force-pushed the task/webusb branch 2 times, most recently from dad0884 to 97a49a6 Compare May 19, 2025 18:11
@Yatekii
Copy link
Author

Yatekii commented May 19, 2025

I finally rebased this badboi. Probably highly buggy. I will test it the coming days :) I am still terrible at all the unsafe stuff. I need a way to make the WebUsb device handle Send/Sync. It has an internal *mut u8 ptr. I guess just warping it in a uncloneable newtype struct that is Send is enough? And then I wrap that in an Arc<Mutex<>>. How I sync creation of the device wrapper is also an open question but I guess a global list with open devices works :) I am not sure if WebUSB even remembers open devices and only lets you open once. I'll check that too.

I am pretty sure I did most of the things wrong in one or the other way (for example returning the device descriptor) but I'll hopefully clear that up :)

@Yatekii Yatekii force-pushed the task/webusb branch 4 times, most recently from 367769c to 1701b7f Compare May 19, 2025 20:45
@kevinmehall
Copy link
Owner

I need a way to make the WebUsb device handle Send/Sync

I think the correct way is that the types that wrap WebUsb objects just aren't Send/Sync. That is, we should cfg out the tests checking for those traits when compiling for wasm. There's a good reason that JsValue and related are not Send: they're indexes into a JS-side table of object references, and while the wasm linear memory is shared with web workers, that table is not. So you simply can't create a JsValue on one thread and use it on another.

If that becomes painful for cross-platform use we could consider something like wgpu's fragile-send-sync-non-atomic-wasm feature that makes them Send/Sync but only when atomics are disabled, meaning it can't be used in multi-threaded wasm and those traits are meaningless. But I'd expect wasm atomics and multithreading to become increasingly popular.

Comment on lines +433 to +436
/// A queue of pending transfers, expected to complete in order
pending: VecDeque<Pending<super::TransferData>>,

idle_transfer: Option<Idle<super::TransferData>>,
Copy link
Owner

Choose a reason for hiding this comment

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

There's no requirement to use crate::transfer::internal and it might be easier and safer not to for webusb. That module is common code for platforms where a transfer is handed off to the OS APIs with a *mut c_void context pointer and a callback that needs to trigger the waker. web_sys already gives you a Future, so I don't think there's any need for that.

I'd just make pending a VecDeque of a new struct containing the JsFuture and the buffer, and poll_next_complete could poll the future from the head of the list. Hopefully no unsafe code needed, except maybe to copy to the uninitialized buffer for IN transfers.

Comment on lines -206 to +213
warn!("invalid device descriptor bLength");
warn!("invalid config descriptor bLength. expected {DESCRIPTOR_LEN_CONFIGURATION}, got {}", buf[0]);
Copy link
Owner

Choose a reason for hiding this comment

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

This is validating a device descriptor, not a config descriptor.

@Yatekii
Copy link
Author

Yatekii commented May 20, 2025

I think we could make a MaybeSend and a MaybeSync but I must say, being Send/Sync is definitely convenient for users of nusb. I see why it's dangerous to assume Send/Sync ofc, but I thought we might be able to make it work with proper synchronization. The UsbDevice pointer is guaranteed to always point to the same object according to spec, so it seems like we could use that.

@kevinmehall
Copy link
Owner

It's not a synchronization issue, but a limitation of how Wasm to JS interop works. If I understand correctly, it will either panic or call the method on the wrong JS object if used from a different thread. I think even in pure JS it's not possible because the WebUSB types are not a transferable object so they exist only in one JS heap.

One option would be to proxy all the calls through a dedicated web worker or the main thread, but that would get complicated and probably require the user to do additional work to set things up.

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.

WebUSB backend
2 participants