rust-embedded-community / usb-device

Experimental device-side USB framework for microcontrollers in Rust.
MIT License
451 stars 76 forks source link

Don't require Sync for UsbBus. #79

Closed Dirbaio closed 2 years ago

Dirbaio commented 2 years ago

Currently UsbBus requires Sync and has &self receiver for all methods. This forces implementations to use global critical sections such as cortex_m::interrupt::free. This is a problem for a few reasons:

This PR fixes this by changing all the UsbBus methods to require &mut self, and passing exclusive borrows to the Bus around to all the places that need it.

Disadvantages::

Advantages:

nosync -Os:
    test bench_bulk_write ... ok  16 transfers of 65536 bytes in 1.933s -> 4.340Mbit/s
    test bench_bulk_read ... ok  16 transfers of 65536 bytes in 1.582s -> 5.304Mbit/s
nosync -O3:
    test bench_bulk_write ... ok  16 transfers of 65536 bytes in 1.906s -> 4.401Mbit/s
    test bench_bulk_read ... ok  16 transfers of 65536 bytes in 1.498s -> 5.601Mbit/s

master -Os:
    test bench_bulk_write ... ok  16 transfers of 65536 bytes in 1.929s -> 4.349Mbit/s
    test bench_bulk_read ... ok  16 transfers of 65536 bytes in 1.744s -> 4.811Mbit/s
master -O3:
    test bench_bulk_write ... ok  16 transfers of 65536 bytes in 1.846s -> 4.434Mbit/s
    test bench_bulk_read ... ok  16 transfers of 65536 bytes in 1.552s -> 5.406Mbit/s
Dirbaio commented 2 years ago

There was some discussion about this on the Matrix chat

TLDR is: the register layout of many USB peripherals allows concurrently reading/writing to different endpoints (it is the case on nRF, STM32 USBD and OTG), so it's nice to keep that so that different classes can concurrently read/write to different endpoints without locking.

The problem is the current trait still allows things like concurrently writing to the same endpoint twice, which it shouldn't.

I now think some solution like the one in the WIP branch endpoint-trait is better https://github.com/rust-embedded-community/usb-device/blob/endpoint-trait/src/usbcore.rs . Each endpoint is a separate struct, requires &mut self to read/write, but is Send, so you can read/write to different endpoints concurrently.

embassy-usb does it this way, the result is the whole USB stack (core+classes) is fully lock-free which makes it much faster.

I'm not going to keep working on this personally though, as embassy-usb fits my needs.

stappersg commented 2 years ago

Trivial update for the sole propuse that #89 gets an merge request is closed notification.