rust-embedded / embedded-hal

A Hardware Abstraction Layer (HAL) for embedded systems
Apache License 2.0
1.95k stars 197 forks source link

Add asynchronous trait for CAN #520

Closed ptpaterson closed 10 months ago

ptpaterson commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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