Where should embedded-hal-async traits be implemented?

[MabezDev]

As of 7c9adcb the feature/embassy branch implements embassy::time::Driver & embedded_hal_async::digital::Wait inside a new crate, esp-hal-async. Although this attempt has been successful I don't know if it's the approach we wish to move forward with.

Problem 1, the async traits cannot be implemented directly

Due to rust's orphan rule, we cannot implement traits directly on the esp-hal types, instead, a wrapper type is needed. My GPIO implementation is similar to @bjoernQ's in his experiments repo, but I chose to implement Deref to call methods on the original pin, instead of converting the pin into an async pin. The long-term plan would be to re-export all pins as AsyncPin's so no conversion needs to be done. This brings me to the second problem.

Problem 2, embedded_hal_async traits should suppliment embedded_hal traits

So, Gpio's are now wrapped in AsyncPin, what happens to esp-hal? None of the peripheral constructors work unless we implement every single trait implemented on the original pin on the AsyncPin (e.g

esp-hal/esp-hal-common/src/spi.rs

Line 55 in 7c9adcb

pub fn new<SCK: OutputPin, MOSI: OutputPin, MISO: InputPin, CS: OutputPin>(

). This isn't a huge deal, but will slow down compile times.

It's important to mention here that @bjoernQ's extension trait method doesn't fail here, but gets a bit odd with other peripherals. For instance, it's common in the r-e ecosystem to use into_XXX methods on gpio pins, but what about spi? You'd create a normal spi instance then call into_async on it? It could work I suppose.

Problem 3, which hal to use?

I'm living in the future where I may want to use both blocking and async APIs. You may think this is rare, but I imagine it could be quite common if you are running an RTOS you may want to block on a thread for one specific peripheral, then in another thread multiplex a few peripherals handling into one task to save stack space (perhaps these other peripherals are used less frequently). Another example would be RTIC, I know we don't have a port right now, but RTIC can block in the idle thread, and be async in the rest of the tasks.

The question is right now, what's the point of esp-hal-async if it exposes both blocking and async APIs? Why not just cut out the middle-man and have esp-hal have blocking and async implementations?

[bjoernQ]

At least in my head something like .to_async() wasn't really a problem but I admit I haven't thought it completely to the end.

All those points are totally valid and to be hones I don't have much pure technical reasons for not implementing the async-traits in the base HALs however I have / had some doubts

• hard dependencies on embassy: we can and probably should make this an opt-in / non-default feature but it looks like there is no way to implement things by "just" implementing some traits but it seems that embassy is quite intrusive, i.e. it seems to be not possible to just implement traits like from embedded-hal-async but we need to interact with the executor - that is apparently a problem right now in Async-Rust in general ... but maybe there would be a way to make the implementation at least mostly agnostic over the async-runtime
  • embassy isn't published on crates.io and apparently won't be in near future: that will block us from publishing to crates.io
• instable API: both embedded-hal-async and also embassy are far from defining a stable API which calls for trouble
  • we also have some support for eh1 which suffers from similar problems but it seems it's stabilizing now a bit
  • making all this (like the eh1 support) an optional feature and taking care to really not pulling anything of it in otherwise probably solves this - also pinning the versions will help
• feature-creep: I'd love to support many runtimes in future (even RTIC - I never liked it but maybe people would like something like that😄 ) but if support for each runtime adds a ton of code and dependencies to the HAL we should choose carefully ... if we can do things on top of the HAL in a separate support crate it's less of a problem

I'm not totally against adding async support to the HAL(s) but while there are hard technical reasons for doing that, we should be at least be aware of the down-sides.

For me the biggest thing is that it currently doesn't seem to be possible to implement async-traits that are completely agnostic over the runtime (but maybe I'm wrong - I wish I am)

Most of these things are not pure technical issues but gut feelings to be honest - and even the hard-facts like depending on git-dependencies might vanish in future

I also admit that currently there isn't any other serious embedded async runtime on the horizon so being agnostic over the async-runtime is maybe more a philosophical thing currently - but maybe there will be other promising async-runtimes in the future

[ivmarkov]

I also admit that currently there isn't any other serious embedded async runtime on the horizon so being agnostic over the async-runtime is maybe more a philosophical thing currently - but maybe there will be other promising async-runtimes in the future

I feel strongly for staying executor agnostic.

• For example, you can use smol-rs's async-task executor (or my ISR-friendly adaptation of it). It is no_std as well, but does require alloc when spawning a task. Which might be a good compromise for some folks
• Or you can use futures / futures-lite "give me one giant future to execute" mini-"executors". Or roll your own one btw, as I did.

In general: given that executors are such a modular thing, it would be a shame sacrificing this and binding ourselves to just one implementation, even if this implementation is damn great. :) Just because of the WakerRegistration hiccup.

[bjoernQ]

One thing that came to my mind after looking into the async GPIO code: There we only depend on AtomicWaker so if that is really all we need for many/most of the implementations we could maybe abstract that away and have it in the HALs while still having an embassy-hal which adds the things that are more coupled to embassy (including the glue code between our AtomicWaker abstraction and embassy::waitqueue::AtomicWaker)

Really don't know if that is sane - also makes things way more complicated

[Dirbaio]

it seems that embassy is quite intrusive, i.e. it seems to be not possible to just implement traits like from embedded-hal-async but we need to interact with the executor

That's not true! :) You can impl the embedded-hal-async traits (gpio, uart, spi, i2c..) without depending on embassy at all, just sticking to the standard Waker, and it'll work on the Embassy executor (and all other executors).

(The reason you might want to use embassy::waitqueue::{WakerRegistration, AtomicWaker} is because it has optimized wakers, which are 1 word instead of 2 and don't need virtual calls on wake which is slightly faster. But it's not required to make a HAL that can run on the Embassy executor.)

instable API: both embedded-hal-async and also embassy are far from defining a stable API which calls for trouble

embedded-hal-async requires nightly and will continue to do so until Rust gets stable "async fn in traits" support. To workaround this, I suggest you do like the embassy HALs do, which is to have both inherent methods, and feature-gated trait impls.

• inherent methods: https://github.com/embassy-rs/embassy/blob/master/embassy-nrf/src/spim.rs#L337
• async trait impl: https://github.com/embassy-rs/embassy/blob/master/embassy-nrf/src/spim.rs#L501

This way you can use the inherent methods in Rust stable, and you only need Rust nightly if you need the traits (for HAL-independent drivers).

what's the point of esp-hal-async if it exposes both blocking and async APIs? Why not just cut out the middle-man and have esp-hal have blocking and async implementations?

I suggest you impl both in the same HAL, yep. If you do separate crates you'll have to duplicate lots of code (or expose lots of inner guts from one to use in the other). The resulting API is less usable too because duplicate types.

Another advantage of single-HAL is you can mix blocking+async on the same peripheral instance. For example, if you're using an SPI ethernet chip, you'll be doing tiny 1-byte register access, and big chonky 1500-byte packet transfers. Async-yielding for the 1-byte transfers is not worth it because it's just a handful of clock cycles (the async context switch will be more than that) but it is for the big packet transfers, so if you mix you can get a faster+smaller driver.

[bjoernQ]

Thanks for the clarification! Really helpful

Besides my philosophic / esoteric concerns the one remaining thing is still that for publishing on crates.io we cannot have git-dependencies - any roadmap on embassy side in this regard? We certainly don't need everything on crates.io

And totally unrelated but since you are here: any plans to make embassy-net depend on a recent and published smoltcp version?

[Dirbaio]

any roadmap on embassy side in this regard

coming soon, the main blocker is docs now. Anyway, you don't have to depend on embassy at all, so it shouldn't be blocking releasing esp-hal(-async) on crates.io.

depend on a recent and published smoltcp version

there's a few open issues/prs I'd like to tackle before smoltcp 0.9... but time is scarce 🥲

[MabezDev]

Besides my philosophic / esoteric concerns the one remaining thing is still that for publishing on crates.io we cannot have git-dependencies - any roadmap on embassy side in this regard? We certainly don't need everything on crates.io

We could put embassy behind an unstable cfg (different from a feature to avoid semver guarantees, this is what tokio does) and declare specifically embassy support as unstable. I would also say before we merge anything async-related, let's get our first release out the door and build on from there.

For me the biggest thing is that it currently doesn't seem to be possible to implement async-traits that are completely agnostic over the runtime (but maybe I'm wrong - I wish I am)

To follow on from @Dirbaio's comment, you're right that we do use embassy::waitqueue::AtomicWaker, but to be executor agnostic we could copy & paste the agnostic waker here and be done with it (we could instead export the optimized waker for embassy if the embassy feature was enabled). I remember reading in the embassy matrix (I think @ivmarkov was asking) about splitting out some of the utility parts like the waker (only the agnostic one) and mutex into a separate crate but I don't know if that's a sure thing or not. I believe that with this change the current GPIO driver would be completely executor agnostic because at that point we are only relying on what's in the core library. This is also why in the initial impl, embassy is its own module and gpio is completely seperate from it.

Another advantage of single-HAL is you can mix blocking+async on the same peripheral instance. For example, if you're using an SPI ethernet chip, you'll be doing tiny 1-byte register access, and big chonky 1500-byte packet transfers. Async-yielding for the 1-byte transfers is not worth it because it's just a handful of clock cycles (the async context switch will be more than that) but it is for the big packet transfers, so if you mix you can get a faster+smaller driver.

This is a use-case I hadn't considered, thanks!

[bjoernQ]

sounds good

This is also why in the initial impl, embassy is its own module and gpio is completely seperate from it.

I thought it's like that and now it's confirmed

[Dirbaio]

We could put embassy behind an unstable cfg (different from a feature to avoid semver guarantees, this is what tokio does)

Please don't use cfgs for enabling unstable stuff, I hate this trend. It's extremely annoying

• cfgs don't solve the semver issues. You can get a broken build after cargo update no matter whether you're using cfgs or feature.
• The issue cfgs are trying to solve is "a library might enable unstable features, making the user use unstable stuff without being aware", but IMO if a feature is named unstable-whatever it's quite obvious a lib shouldn't enable it on its own. Libs simply shouldn't do that, and it's not a problem in practice IMO.
• they require special command line args to cargo build (or special stuff in .cargo/config which has its own issues, like it silently doesn't apply if you build from another working directory)
• It rebuilds absolutely everything if you change them, vs just the affected crates
• There's stuff you can do with features that you can't with cfgs:
  • Lib A feature unstable-bar enabling lib B feature unstable-foo
  • Lib A featureunstable-embassy enabling the optional dep embassy.

The standard way of allowing users to configure crates is Cargo features. Forcing nonstandard flags onto the user to use your lib because a very theoretical "what if" concern is quite arrogant.

[MabezDev]

I agree, let's not do that. I've already been burned by tokio in MabezDev/wokwi-server#9 & esp-rs/esp-idf-template#37 (comment) 🥲. I think a standard feature prefixed by unstable- is enough.

[ivmarkov]

For me the biggest thing is that it currently doesn't seem to be possible to implement async-traits that are completely agnostic over the runtime (but maybe I'm wrong - I wish I am)

To follow on from @Dirbaio's comment, you're right that we do use embassy::waitqueue::AtomicWaker, but to be executor agnostic we could copy & paste the agnostic waker here and be done with it (we could instead export the optimized waker for embassy if the embassy feature was enabled). I remember reading in the embassy matrix (I think @ivmarkov was asking) about splitting out some of the utility parts like the waker (only the agnostic one) and mutex into a separate crate but I don't know if that's a sure thing or not. I believe that with this change the current GPIO driver would be completely executor agnostic because at that point we are only relying on what's in the core library. This is also why in the initial impl, embassy is its own module and gpio is completely seperate from it.

I suggest we start by copy-pasting the agnostic WakerRegistration stuff but in future we might need something more complex:

• Ideally, user should be able to choose between WakerRegistration and MultiWakerRegistration (@Dirbaio - why is WakerRegistration not implemented as pub type WakerRegistration = MultiWakerRegistration<1>?; also I find it a bit odd that one is consuming 100% CPU on waker re-registration overflow, and the other just fails the op; unifying the behavior would simplify things for the user)
• Ideally, the waker registration impl should be should be pluggable; there are ideas how to do it, but no consensus yet

Another advantage of single-HAL is you can mix blocking+async on the same peripheral instance. For example, if you're using an SPI ethernet chip, you'll be doing tiny 1-byte register access, and big chonky 1500-byte packet transfers. Async-yielding for the 1-byte transfers is not worth it because it's just a handful of clock cycles (the async context switch will be more than that) but it is for the big packet transfers, so if you mix you can get a faster+smaller driver.

This is a use-case I hadn't considered, thanks!

If we go single-HAL (which I would also prefer), can we use asynch as the module name(s) for the async traits? That would be symmetric with embedded-io from Embassy and our own embedded-svc and esp-idf-svc

@Dirbaio :
How about publishing embassy on crates.io with all async stuff hidden behind an async feature, just like embedded-io?
The async feature in embedded-io also needs nightly, but this has not stopped us from publishing, right?

[Dirbaio]

@Dirbaio - why is WakerRegistration not implemented as pub type WakerRegistration = MultiWakerRegistration<1>?; also I find it a bit odd that one is consuming 100% CPU on waker re-registration overflow, and the other just fails the op; unifying the behavior would simplify things for the user

Agreed!

@Dirbaio How about publishing embassy on crates.io with all async stuff hidden behind an async feature,

in embedded-io it's because async traits require nightly, but that's not the case for the async stuff in embassy, so I'm not sure why'd we want this?

[ivmarkov]

@Dirbaio How about publishing embassy on crates.io with all async stuff hidden behind an async feature,

in embedded-io it's because async traits require nightly, but that's not the case for the async stuff in embassy, so I'm not sure why'd we want this?

But then why are we shying away from publishing embassy on crates.io?

embassy isn't published on crates.io and apparently won't be in near future: that will block us from publishing to crates.io

I just don't get this! ^^^
It would be so much easier to consume!

[MabezDev]

One thing I hadn't considered is that the async driver compiled in esp-hal proper uses the peripheral interrupt, making it unavailable if a user doesn't want to use async at all, or they want to get an interrupt for a different event. In the GPIO example, the GPIO interrupt is 'taken' by the async driver and if a user tries to declare it for their own use they'll get a linker error.

I guess this is where the blocking and async paradigms clash, the only way I can think about handling this would:

Forwarding the actual interrupt handler to a user interrupt handler i.e

ISR Pending -> Vectoring handler -> vectored interrupt (#[interrupt]) -> User vectored interrupt

Inside the vectored interrupt part is where the async bits will be managed.

But I really don't like this at all, we have to duplicate handlers, and users are confused as to which handler to declare. It just becomes a nightmare. Any thoughts on how to manage this?

[bjoernQ]

While I also don't like it, I currently also don't see any alternative.

Maybe it's a necessary trade-off when user's want to make use of the async model - especially if they want to mix it with the traditional model. As long as it doesn't affect pure non-async code it might be something that just needs to be properly documented 🤷‍♂️

[MabezDev]

To summarize my thoughts from the esp-rs community meeting:

async & Interrupt driven are two separate paradigms. For every interrupt event, we should provide an async way to access them when in async mode. Doing it this way will force us into an async feature, but I don't think that's the end of the world. All the async feature will do is bind the interrupt handlers to do the waking.