rust-embedded / embedded-hal

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

io-async: fix warnings for latest nightly (1.75.0) #515

Closed rmsyn closed 11 months ago

rmsyn commented 11 months ago

Fixes cargo check warnings for the latest nightly version (1.75.0).

Recommended public API signatures for async trait functions has changed to recommending the desugared version for compatibility with different async runtimes.

Dirbaio commented 11 months ago

The warning against using async fn in trait definitions is because many async runtimes in non-embedded land default to requiring Send futures.

There's no way to express that with the async fn syntax, it is only possible with the -> impl Future<..> + Send syntax. This is why the warning recommends doing that. If you write your trait with async fn (or with -> impl Future your trait won't be usable with these runtimes. Only -> impl Future + Send is.

In the embedded case, all existing runtimes and HALs don't require Send futures (and are unlikely to require it, you rarely have multicore and when you do you dedicate each core to concrete tasks, instead of running a single multi-core executor). So in this case we should just allow the lint with #[allow(async_fn_in_trait)]

rmsyn commented 11 months ago

In the embedded case, all existing runtimes and HALs don't require Send futures (and are unlikely to require it, you rarely have multicore and when you do you dedicate each core to concrete tasks, instead of running a single multi-core executor). So in this case we should just allow the lint with #[allow(async_fn_in_trait)]

Ok, I finished with the remaining traits using the rewrite to the desugared version. I can change to use the #[allow(async_fn_trait)]. I'm also currently writing a HAL for a multicore platform, would these traits be useful in something like an RTOS that wanted to take advantage of multiple cores? (I can also rewrite with the + Send in the function signatures).

I think leaving it with the desugared version also might be better for any future changes that want to add the + Send trait, even if we leave it without it for now. Since this is functionally equivalent without + Send to the sugared async fn version.

Dirbaio commented 11 months ago

adding + Send breaks compatibility with all existing embedded async HALs, that's not an option.

Currently, All embedded HALs are in the "HAL not implementing Send" category (embassy-nrf, embassy-stm32, embassy-rp, espxxx-hal). All embedded executors are in the "Executor not requiring Send" category. (embassy-executor, RTIC, edge-executor, lilos).

This is an overview of the compatibility we get depending on the choice in the trait declaration:

Trait Executor requiring Send Executor not requiring Send HAL implementing Send HAL not implementing Send
async fn :warning: :green_circle: :green_circle: :green_circle:
fn -> impl Future :warning: :green_circle: :green_circle: :green_circle:
fn -> impl Future + Send :green_circle: :green_circle: :green_circle: :x:

(note that async fn and fn -> impl Future are exactly equivalent, the 1st desugars to the latter)

:green_circle: = works :x: = doesn't work :warning: = works for generic code that doesn't spawn, works for code that spawns with concrete types. Fails only with generic code that does spawn. Example:

trait Foo {
    async fn foo(self); // future may or may not be Send.
}

struct MyFoo;
impl Foo for MyFoo { 
    async fn foo(self) {
        // future that *does* implement Send.
    }
}

async fn use<T: Foo>(foo: T) {
    // works: nothing here requires `Send`, even if you do end up running this
    // code in an executor that does require `Send`.
    foo.foo().await;
}

fn spawn_concrete_type(foo: MyFoo) {
    // works: the compiler can see `t.foo()` is Send even if the signature for `foo()`
    // doesn't require `+ Send`, because it knows the concrete type.
    tokio::spawn(t.foo()) 
}

fn spawn_generic<T: Foo>(t: T) {
    // Doesn't work: the future for `t.foo()` may or may not be `Send` depending on `T`.
    tokio::spawn(t.foo()) 
}

So, first of all: there's no advantage in switching from async fn to -> impl Future (what this PR does), they're exactly equivalent.

About compat with executors that require Send, we can either:

  1. Do nothing, stay with async fn.
  2. Add a 2nd set of traits SendI2c, SendSpiBus, SendSpiDevice etc that are the same, but with -> impl Future + Send.

I prefer doing 1, because:

rmsyn commented 11 months ago

Do nothing, stay with async fn.

After reading your write-up, I agree. I'm new to async stuff, and was just going off the warning messages.

Once RTN lands, the Send* traits will no longer be idiomatic, but we won't be able to remove them them because it'll be a breaking change.

That completely makes sense, I was not aware of RTN.

I can make the changes to use the #[allow(async_fn_trait)] to fix the CI builds.

rmsyn commented 11 months ago

Looks like stable builds are failing for an unknown lint failure. Not sure how to block this out with conditional compilation without bringing in a new dependency.

Dirbaio commented 11 months ago

you can add a single #![allow(async_fn_in_trait)] at the top of lib.rs to silence the lint for the whole crate.

clippy and rustdoc fail becuase you have to update nightly here https://github.com/rust-embedded/embedded-hal/blob/master/.github/workflows/clippy.yml#L18 https://github.com/rust-embedded/embedded-hal/blob/master/.github/workflows/rustdoc.yml#L16