embassy-rs / embassy

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

Potential Logic Issue for AVR Executor #3472

Closed carloskiki closed 3 weeks ago

carloskiki commented 3 weeks ago

For the record, I was reading the code to try to understand how it works, and I may be totally wrong. I am no async/threading expert.

For the AVR arch, the Executor implementation looks like this:

pub fn run(&'static mut self, init: impl FnOnce(Spawner)) -> ! {
    init(self.inner.spawner());

    loop {
        unsafe {
            avr_device::interrupt::disable();
            if !SIGNAL_WORK_THREAD_MODE.swap(false, Ordering::SeqCst) {
                avr_device::interrupt::enable(); // vvvvvvv
                avr_device::asm::sleep();        // ^^^^^^^
            } else {
                avr_device::interrupt::enable();
                self.inner.poll();
            }
        }
    }
}

What happens if an interrupt (which could I presume call __pender) occurs right after they are enabled, but right before the executor goes to sleep? (see code comments).

I believe the thread could not wake up until there is another interrupt, which could be an arbitrary amount of time in the future.

Line Link: https://github.com/embassy-rs/embassy/blob/10c9fbcc99b564d8ece88b32835dbc78a4269b34/embassy-executor/src/arch/avr.rs#L62

carloskiki commented 3 weeks ago

Nevermind, I have read the avr docs, and understood that sei (interrupt enable) guarantees that the next instruction is executed before an interrupt can occur.