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

Add asynchronous trait for CAN #520

Closed

Conversation

ptpaterson
Copy link

Async fn in trait is stabilized in nightly 1.75, which means folks can opt in to async when using nightly, but can continue to use blocking and nb without it.

fixes: #468

I'm bad at names. I don't know if there is a better convention to have an async mod alongside nb and blocking.

I've implemented these signatures in my esp32-c3 driver's inherent implementation and is working wonderfully. I would love to have this be supported by embedded-hal, so I can make the CANOpen implementation I am working on generic over the async trait and to share.

@ptpaterson ptpaterson requested a review from a team as a code owner November 13, 2023 14:18
@ptpaterson
Copy link
Author

Ah. Okay clippy is failing and saying add `#![feature(async_fn_in_trait)]` to the crate attributes to enable. The point of this PR is not to require that for everyone.

We could tuck this under embedded-hal-async but that defeats the purpose of the separate embedded-can crate.

Is the team amenable to changes that don't 100% pass clippy tests?

(Rustdoc test fails, too, but that one's not me 😄)

@Dirbaio
Copy link
Member

Dirbaio commented Nov 13, 2023

Currently, CI checks embedded-can builds with latest stable Rust, which is 1.73 right now. There's a few options:

  • Wait for 1.75 before merging, which will be released 28th December.
  • Merge now, add a nightly feature to gate the async trait, and make CI try stable with the feature disabled, and nightly with the feature enabled. This allows releasing before 1.75. We can remove the nightly feature gate later.
  • Merge now, Make CI check only nightly for now, this forces waiting for 1.75 before releasing.

Also, one thing about the trait design I'm not sure about: the current design means you can't transmit while waiting for a receive or vice versa because both methods are on the same instance and take &mut self. Perhaps it could make sense to have split traits CanReceive, CanTransmit, so you can "split" the CAN peripheral, similar to embedded-io-async. This'd allow the user to separate CAN handling in 2 tasks for example, one for receive and one for transmit.

Interestingly this is not an issue with the nb trait because the user can poll alternatingly for transmit and receive, effectively "waiting" for both at the same time.

The blocking trait has the same issue, but if the user is using blocking I'd say it's fine to assume they don't care about concurrency at all... Unless they're using a stackful (non-async) RTOS, but those are rare in Rust. Perhaps it makes sense to split the blocking trait too?

@ptpaterson
Copy link
Author

Wait for 1.75 before merging, which will be released 28th December.

Oh! that's much sooner than I thought because I didn't pay attention to the schedule. Waiting is cool with me, rather than fight with other things.

Regarding mutable references, I had copied the signatures over from blocking but in my esp32 implementation I was able to only require a shared reference, because I don't need a mutable reference to the peripheral (esp-hal-common Can implementation doesn't technically need it) in order to write to the registers. My async version is in a local copy of the hal, not in user code. Though, to be honest, ever since I did that, I've been looking at it and wondering how safe it is. I image other implementations would not be so lucky and need a mutable reference to the peripheral.

I looked to embassy for some guidance in implementation, and do note that CanRx and CanTx are separate structs containing RefCell-wrapped instances.
https://github.com/embassy-rs/embassy/blob/ea99671729be91b63156097b01128c3ea6f74a75/embassy-stm32/src/can/bxcan.rs#L472-L474

It seems like a reasonable path to split the traits based on that precedent. I'll also say that as I am working on app code, I am definitely relying on my current implementation to only require a shared reference. It now seems likely I would be in trouble (have to use some unsafe lines) if a mutable reference is required. But I don't feel knowledgeable enough to argue stringly either way.

@Dirbaio
Copy link
Member

Dirbaio commented Nov 13, 2023

The problem with shared references is it allows doing arbitrarily many rx's or tx's concurrently, which is harder to support and doesn't have many use cases in the first place.

@ptpaterson
Copy link
Author

One mutable reference for RX and one mutable reference for TX makes sense then.

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.

embedded-can async traits
2 participants