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

Use the "generic-queue" from embassy-time instead of the "integrated-timers" from embassy-executor #1087

Closed
Fundevoge opened this issue Jan 17, 2024 · 4 comments

Comments

@Fundevoge
Copy link

#1037

The "integrated-timers" timer queue from embassy-executor does not work properly on esp32.
I added "generic-queue" from embassy-time and got a linker error.
As it turns out, this has to do with the "embassy-integrated-timers" default-feature of the esp32-hal.

With this default feature, embassy-executor creates the _embassy_time_schedule_wake function:

#[cfg(feature = "integrated-timers")]
embassy_time_queue_driver::timer_queue_impl!(static TIMER_QUEUE: TimerQueue = TimerQueue);

embassy-time provides an alternative timer_queue_impl behind "generic-queue":

#[cfg(feature = "generic-queue")]
embassy_time_queue_driver::timer_queue_impl!(static QUEUE: Queue = Queue::new());

But if both features are enabled, this duplicate declaration results in a nasty link-time error...

extern "Rust" {
    fn _embassy_time_schedule_wake(at: u64, waker: &Waker);
}

#[macro_export]
macro_rules! timer_queue_impl {
    (static $name:ident: $t: ty = $val:expr) => {
        static $name: $t = $val;

        #[no_mangle]
        fn _embassy_time_schedule_wake(at: u64, waker: &core::task::Waker) {
            <$t as $crate::TimerQueue>::schedule_wake(&$name, at, waker);
        }
    };
}

My proposed solution is to use the "generic-queue" from embassy-time instead of the "integrated-timers" from embassy-executor or to at least remove "embassy-integrated-timers" from the default features.

@bjoernQ
Copy link
Contributor

bjoernQ commented Jan 17, 2024

Running into linker errors is not nice.

We probably should improve the documentation regarding the features (i.e. explain that embassy-integrated-timers is a default feature and cannot be used together with embassy-generic-queue-*) and have a check in build.rs giving a nicer error if those features are activated concurrently

@Fundevoge
Copy link
Author

That sounds like a good idea. I am also a bit confused whether to add i.e. embassy-generic-queue-8 to esp32-hal or to esp-hal-common or to embassy-time as generic-queue-8, as they all seem to do about the same. Maybe also add documentation on that. If the feature is enabled in embassy-time directly, I am not sure if the build.rs of esp32-hal / esp-hal-common could see that and give an error.

@bjoernQ
Copy link
Contributor

bjoernQ commented Jan 17, 2024

Good point! Given with #1089 we'll probably remove the generic-queue-* features and let the user enable them on embassy-time we probably need to rethink this

At least we have an issue reminding us that we should do something about the ergonomics here 👍

@MabezDev
Copy link
Member

As part of #1196 we no longer enable integrated timers by default, leaving the end user with the choice of using generic queue or integrated timers.

@github-project-automation github-project-automation bot moved this from Todo to Done in esp-rs Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

3 participants