Skip to content
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

Don't require UsbBus to be Sync #149

Merged
merged 1 commit into from
Jul 30, 2024
Merged

Don't require UsbBus to be Sync #149

merged 1 commit into from
Jul 30, 2024

Conversation

dlaw
Copy link
Contributor

@dlaw dlaw commented Apr 28, 2024

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.

@eldruin
Copy link
Member

eldruin commented Jun 12, 2024

Thank you for the PR.
Maybe somebody like @ryan-summers or so can comment on this?

@ryan-summers
Copy link
Member

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?

If the UsbBus is not Sync, it can still be used to make a UsbDevice,
but that UsbDevice will not be Sync.  This is quite limiting (as it
mostly means that the USB device can't be accessed from interrupt
contexts), but it is entirely safe from the Rust safety perspective.
@dlaw
Copy link
Contributor Author

dlaw commented Jul 27, 2024

I rebased on the latest upstream commit and added a changelog entry. Cheers!

@ryan-summers
Copy link
Member

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.

@ryan-summers ryan-summers merged commit 09cbee9 into rust-embedded-community:master Jul 30, 2024
3 checks passed
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.

3 participants