embassy-rs / embassy

Modern embedded framework, using Rust and async.
https://embassy.dev
Apache License 2.0
5.25k stars 724 forks source link

Creating generic tasks #1837

Closed cdunster closed 11 months ago

cdunster commented 1 year ago

First off, thank you all for the work on Embassy. For me, it is one of the most important Rust projects out there!

I am looking for a way to make a task generic over a GPIO, but truly generic so that it can be used on nRF and STM chips. So I would like something like:

#[embassy_executor::task]
async fn blinky_task<L: OutputPin>(mut led: L) {
    let mut ticker = Ticker::every(Duration::from_secs(1));
    loop {
        let _ = led.set_high();
        ticker.next().await;
        let _ = led.set_low();
        ticker.next().await;
    }
}

but this gives error: task functions must not be generic (props for the nice error message by the way).

The chip-specific crates provide a type for this very purpose so I could do:

#[embassy_executor::task]
async fn blinky_task(led: AnyPin) {
    let mut led = Output::new(led, Level::High, Speed::Low);
    let mut ticker = Ticker::every(Duration::from_secs(1));
    loop {
        let _ = led.set_high();
        ticker.next().await;
        let _ = led.set_low();
        ticker.next().await;
    }
}

But this is only generic over any GPIO from the chip-specific crate, so if I use embassy_stm32::gpio::AnyPin; then it only works on STM32 chips and vice versa if I use the embassy_nrf crate instead.

The only way that I have found to do this is to use Box:

#[embassy_executor::task]
async fn blinky_task(mut led: Box<dyn OutputPin<Error = Infallible>>) -> ! {...}

But using Box here is not really ideal and it adds the extra awkwardness that for unit tests I use the embedded-hal-mock crate where the OutputPin's error type is different and as I can't be generic over the error then I have to do something like:

#[cfg(not(test))]
type LedPinError = core::convert::Infallible;

#[cfg(test)]
pub type LedPinError = embedded_hal_mock::MockError;

#[embassy_executor::task]
async fn blinky_task(mut led: Box<dyn OutputPin<Error = LedPinError>>) -> ! {...}

So, my multi-part question is:

  1. Does anyone know how to simplify this code? :smile:
  2. Is it technically possible to make tasks generic? If so then maybe I could help.
  3. If the task function can't be generic then could it be possible to allow for associated function tasks so the type could be generic but the function is concretely implemented on that type.
  4. If the tasks can't be generic then could AnyPin be moved into the embassy-embedded-hal crate or something so that the type is the same across all the chip-specific crates and I could just use that?

Thank you for reading this and I hope someone can help.

coljnr9 commented 1 year ago

Not for all cases, but I use type_alias_impl_trait

I think something like this should work

type Led = impl OutputPin;

#[embassy_executor::task]
async fn blinky_task(mut led: Led) {
    let mut ticker = Ticker::every(Duration::from_secs(1));
    loop {
        let _ = led.set_high();
        ticker.next().await;
        let _ = led.set_low();
        ticker.next().await;
    }
}
cdunster commented 1 year ago

Hi @coljnr9, thanks for your reply. If I used impl in the type alias then I get:

error: unconstrained opaque type
5 | type Led = impl OutputPin;
  |            ^^^^^^^^^^^^^^
  = note: `Led` must be used in combination with a concrete type within the same module

So instead I tried using it directly:

#[embassy_executor::task]
async fn blinky_task(mut led: impl OutputPin + 'static) {
    let mut ticker = Ticker::every(Duration::from_secs(1));
    loop {
        let _ = led.set_high();
        ticker.next().await;
        let _ = led.set_low();
        ticker.next().await;
    }
}

But then I get the error:

error: type parameter `impl OutputPin + 'static` is part of concrete type but not used in parameter list for the `impl Trait` type alias
5 | #[embassy_executor::task]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^
  = note: this error originates in the attribute macro `embassy_executor::task` (in Nightly builds, run with -Z macro-backtrace for more info)
coljnr9 commented 1 year ago

I'm a bit new to Rust, so take w/ a grain of salt:

AFAIK

type Led = impl OutputPin;

gets constrained when you call the function with a concrete type, for me it's when I spawn the task from main()

This modified version of the STM32F7 blinky example compiles for me:

This was sensitive to the nightly compiler version I'm using nightly-2023-07-08

#![no_std]
#![no_main]
#![feature(type_alias_impl_trait)]

use defmt::*;
use embassy_executor::Spawner;
use embassy_stm32::gpio::{Level, Output, Speed};
use embassy_time::{Duration, Timer};
use embedded_hal::digital::v2::OutputPin;
use {defmt_rtt as _, panic_probe as _};

type Led = impl OutputPin;

#[embassy_executor::task]
async fn blink(mut led: Led) {
    loop {
        info!("high");
        let _ = led.set_high();
        Timer::after(Duration::from_millis(300)).await;

        info!("low");
        let _ = led.set_low();
        Timer::after(Duration::from_millis(300)).await;
    }
}

#[embassy_executor::main]
async fn main(spawner: Spawner) {
    let p = embassy_stm32::init(Default::default());
    info!("Hello World!");

    // Try compiling using a pin of another type type.

    // let led = Output::new(p.PB8, Level::High, Speed::Low);
    let led = Output::new(p.PB7, Level::High, Speed::Low);
    unwrap!(spawner.spawn(blink(led)));
}
Dirbaio commented 1 year ago

Keep in mind the only nightly version supported by Embassy is this one https://github.com/embassy-rs/embassy/blob/main/rust-toolchain.toml#L4. Use older ones at your own risk.

in recent nightlies, TAIT forces you to nest the functions due to https://github.com/rust-lang/rust/issues/107645. Now you have to do this:

#[embassy_executor::main]
async fn main(spawner: Spawner) {
    type Led = impl OutputPin;

    #[embassy_executor::task]
    async fn blink(mut led: Led) {
        loop {
            info!("high");
            let _ = led.set_high();
            Timer::after(Duration::from_millis(300)).await;

            info!("low");
            let _ = led.set_low();
            Timer::after(Duration::from_millis(300)).await;
        }
    }

    let p = embassy_stm32::init(Default::default());
    info!("Hello World!");

    // Try compiling using a pin of another type type.

    // let led = Output::new(p.PB8, Level::High, Speed::Low);
    let led = Output::new(p.PB7, Level::High, Speed::Low);
    unwrap!(spawner.spawn(blink(led)));
}
Dirbaio commented 1 year ago

(feel free to leave feedback in that Rust issue if you don't like it :smiling_face_with_tear: )

coljnr9 commented 1 year ago

Maybe just making more noise than necessary here, but something like this at least lets me keep task logic in separate modules.

#![no_std]
#![no_main]
#![feature(type_alias_impl_trait)]

use defmt::*;
use embassy_executor::Spawner;
use embassy_stm32::gpio::{Level, Output, Speed};
use embassy_time::{Duration, Timer};
use embedded_hal::digital::v2::OutputPin;
use {defmt_rtt as _, panic_probe as _};

mod blink_mod {
    use defmt::*;
    use embassy_time::{Duration, Timer};
    use embedded_hal::digital::v2::OutputPin;

    // Another option
    // pub async fn blink<T: OutputPin>(mut led: T) {
    pub async fn blink(mut led: impl OutputPin) {
        loop {
            info!("high");
            let _ = led.set_high();
            Timer::after(Duration::from_millis(300)).await;

            info!("low");
            let _ = led.set_low();
            Timer::after(Duration::from_millis(300)).await;
        }
    }
}

#[embassy_executor::main]
async fn main(spawner: Spawner) {
    let p = embassy_stm32::init(Default::default());
    info!("Hello World!");

    let led = Output::new(p.PD12, Level::High, Speed::Low);

    type Led = impl OutputPin;

    #[embassy_executor::task]
    async fn blink_outer(led: Led) {
        blink_mod::blink(led).await;
    }

    unwrap!(spawner.spawn(blink_outer(led)));
}

It's gross that the TAIT can't be defined in the blink_mod module, though (which I think is the breaking change discussed in that thread? It was bit over my head tbh)

Anyway, thanks for chiming in!

Dirbaio commented 1 year ago

It's gross that the TAIT can't be defined in the blink_mod module, though (which I think is the breaking change discussed in that thread? It was bit over my head tbh)

in this particular case, you already couldn't in previous nightlies. The TAIT defining scope was "the TAIT's parent module" before. You're trying to define it from main which is not within the parent module (which is mod blinky_mod)

In https://github.com/rust-lang/rust/issues/107645 the defining scope was changed to "any function within the TAIT's parent module that mentions the TAIT in arguments or return value, or mentions a struct that (possibly transitively) contains a field that mentions the TAIT in its type".

This is why you could previously put the two tasks side by side, but now you have to nest them: main doesn't mention the TAIT in args nor return type so it can't define it anymore.

I've always found the dependency on the module structure annoying, it sometimes forced you to artificially change the module structure to make the compiler happy. But at least it was simple. https://github.com/rust-lang/rust/issues/107645 made it more restricted so many use cases require more workaround, and the simplicity is gone :(

the final design is still up for discussion though...

Dirbaio commented 1 year ago

the alternative i'm proposing there is making the defining scope explicit, instead of making it implicit based on some heuristic rules. So you'd be able to do:

mod blink_mod {
    use defmt::*;
    use embassy_time::{Duration, Timer};
    use embedded_hal::digital::v2::OutputPin;

    pub type Led = impl OutputPin;

    #[embassy_executor::task]
    pub async fn blink(mut led: Led) {
        loop {
            info!("high");
            let _ = led.set_high();
            Timer::after(Duration::from_millis(300)).await;

            info!("low");
            let _ = led.set_low();
            Timer::after(Duration::from_millis(300)).await;
        }
    }
}

#[embassy_executor::main]
#[defines(blink_mod::Led)]  // <========== explicit defining scope
async fn main(spawner: Spawner) {
    let p = embassy_stm32::init(Default::default());
    info!("Hello World!");

    let led = Output::new(p.PD12, Level::High, Speed::Low);

    unwrap!(spawner.spawn(blink_mod::blink(led)));
}

so you can place things in the mod that makes most sense, and manually tell the compiler "this is the defining scope for X".

cdunster commented 11 months ago

Hi @coljnr9 and @Dirbaio, thanks for the help and explanation with this. I need to go through that thread that you linked so that I can better understand this but it seems that it's a known current limitation of the language instead of with Embassy, so I will close this issue :slightly_smiling_face:

For now, I just changed things around and managed to get the code working with mutable references instead of owned types. This means that things need to be static references which is not ideal and I need to specify the Error type which makes it not testable but I can get around this by creating a helper function that is testable and calling that from the task. I think this is fine anyway because having the loop {} in the task already makes it untestable.

#[embassy_executor::task]
async fn blinky_task(led: &'static mut dyn OutputPin<Error = Infallible>) {
    let mut ticker = Ticker::every(Duration::from_secs(1));
    loop {
        blink_once(led, &mut ticker).await;
    }
}

async fn blink_once<L: OutputPin + ?Sized>(led: &mut L, ticker: &mut Ticker) {
    let _ = led.set_high();
    ticker.next().await;
    let _ = led.set_low();
    ticker.next().await;
}

Thanks again!

eskorzon commented 10 months ago

@cdunster I was running into the no-generic tasks thing as well. I found that writing a macro to write the "blinky" function was the most straightforward and flexible thing to do.

macro_rules! blink_pin {
    ($name:ident, $pin:ty) => {
        #[embassy_executor::task]
        async fn $name(mut pin: $pin) {
            let mut ticker = Ticker::every(Duration::from_millis(100));
            loop {
                pin.set_high();
                ticker.next().await;

                pin.set_low();
                ticker.next().await;
            }
        }
    }
}

#[embassy_executor::main]
async fn main(spawner: Spawner) {
    let p = embassy_stm32::init(Default::default());

    blink_pin!(foo, Output<'static, peripherals::PB7>);
    unwrap!(spawner.spawn(foo(Output::new(p.PB7, Level::High, Speed::Low))));
}