usb-device
usb-device copied to clipboard
Don't require UsbBus to be Sync
Current behavior:
- UsbBus is Sync --> the resulting UsbDevice is Sync.
- UsbBus is not Sync --> compiler error due to unsatisfied trait bound
Proposed new behavior:
- UsbBus is Sync --> the resulting UsbDevice is Sync.
- UsbBus is not Sync --> the resulting UsbDevice is not Sync
In other words, it is safe to allow a non-Sync UsbBus because Rust will NOT derive Sync for the UsbDevice in this case.
Why is this useful?
I have a hard-real-time firmware application which uses the USB interface to provide secondary (non-real-time) diagnostics. I poll the UsbDevice in my main loop, while all my other real-time stuff is handled in various interrupts. I do not use USB interrupts and I never access the UsbDevice from an interrupt context.
Unfortunately, making the UsbBus Sync requires the use of critical sections which damage the real-time interrupt timing. Eliminating the critical sections is totally safe in this situation, and solves my timing issues, but doing so means that the UsbBus cannot safely be marked as Sync.
- For more context see https://github.com/stm32-rs/stm32-usbd/issues/32
- For an implemented example of the UsbBus changes for non-Sync usage, see https://github.com/dlaw/stm32-usbd-no-cs
- If accepted, this PR would allow me to eliminate the following highly unsound
impl Sync: https://github.com/dlaw/stm32-usbd-no-cs/commit/7df1814aacca9c82c3d325276f290bdd5afa5696#diff-b7dd6c6dc06c9c5da00a0c048df395e5fed282392d70a222b1d0e34706d9baf3R26
Thank you for the PR. Maybe somebody like @ryan-summers or so can comment on this?
Sorry for the delay, I've had very little free time until recently. I looked into the request and this seems sound to me. I don't see an explicit reason why we are requiring the UsbBus to be Sync, and the compiler should handle deriving these traits for us automagically, so I don't see a strict need to require this.
@mvirkkunen As the original author, do you see anything problematic with relaxing this bound?
@dlaw Would you mind adding a CHANGELOG.md entry for this update as well?
I rebased on the latest upstream commit and added a changelog entry. Cheers!
In light of not having a response here, I'm going to opt with accepting this and seeing if we discover any issues as we go along.