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.

Sign up for GitHub

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

GPIO: prevent woken task interrupting GPIO handler #2838

Merged
merged 2 commits into from
Dec 19, 2024
Merged

GPIO: prevent woken task interrupting GPIO handler #2838

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a3ecf89
Clear interrupt bit before allowing waker to wake a task
bugadani Dec 18, 2024
fd7d9d4
Add test
bugadani Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions esp-hal/src/gpio/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -930,8 +930,11 @@ fn handle_pin_interrupts(user_handler: fn()) {
intr_bits -= 1 << pin_pos;

let pin_nr = pin_pos as u8 + bank.offset();
asynch::PIN_WAKERS[pin_nr as usize].wake();
set_int_enable(pin_nr, Some(0), 0, false);

crate::interrupt::free(|| {
asynch::PIN_WAKERS[pin_nr as usize].wake();
Copy link
Collaborator

@Dominaezzz Dominaezzz Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an interesting problem here. Suppose you have two higher priority tasks.

  • GPIO interrupt priority is lowest.
  • Task A priority is higher.
  • Task B priority is highest.

Task A & B are waiting for gpio events on separate pins. Say task A is waiting for GPIO2 and task B is waiting for GPIO3.

If both GPIO pin events trigger at the same time, task A is woken up first because it's listening to a low number GPIO pin, even though it has a lower priority than task B. Ideally task B should've been woken up first.

I suppose you can call this WAD but it's a bit strange.
Ideally all the wakers should be woken up before any task can resume but that might be asking too much.

EDIT: This problem existed before this PR of course, just thought I'd mention it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, I guess. We can extend the interrupt-free context to improve this. But we can't solve it fully when the GPIO handler has a low priority: GPIO3 can be toggled at any time, even immediately after we have read the pin states, and then the same situation ends up happening.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole situation where the handler is below the executors is weird enough, and workaroundable enough for me to think it's not worth spending much time on.

set_int_enable(pin_nr, Some(0), 0, false);
});
Comment on lines +933 to +937
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you change your mind? This doesn't match the PR title.
I was expecting you to swap the order of waking and interrupt clearing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it depends how you look at it, the task is waken immediately, or effectively at the end of the interrupt-free section. I'm not sure that swapping the order would be correct.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The infinite loop is caused because the waker is woken before the interrupts are cleared right? Swapping the order seems correct to me in that case. "Don't wake up the task until the pin's interrupt has been completely serviced"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it might be safe enough, but I'm not confident enough.

}

bank.write_interrupt_status_clear(intrs);
Expand Down
1 change: 1 addition & 0 deletions esp-hal/src/interrupt/riscv.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
//! interrupt15() => Priority::Priority15
//! ```

pub(crate) use esp_riscv_rt::riscv::interrupt::free;
pub use esp_riscv_rt::TrapFrame;
use riscv::register::{mcause, mtvec};

Expand Down
1 change: 1 addition & 0 deletions esp-hal/src/interrupt/xtensa.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
//! Interrupt handling

use xtensa_lx::interrupt;
pub(crate) use xtensa_lx::interrupt::free;
use xtensa_lx_rt::exception::Context;

pub use self::vectored::*;
Expand Down
2 changes: 1 addition & 1 deletion esp-hal/src/uart.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -904,7 +904,7 @@ where
cfg_if::cfg_if! {
if #[cfg(esp32)] {
// https://docs.espressif.com/projects/esp-chip-errata/en/latest/esp32/03-errata-description/esp32/cpu-subsequent-access-halted-when-get-interrupted.html
xtensa_lx::interrupt::free(|| {
crate::interrupt::free(|| {
*byte = fifo.read().rxfifo_rd_byte().bits();
});
} else {
Expand Down
29 changes: 29 additions & 0 deletions hil-test/tests/gpio.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,4 +399,33 @@ mod tests {

_ = Input::new(pin, Pull::Down);
}

#[test]
fn interrupt_executor_is_not_frozen(ctx: Context) {
use esp_hal::interrupt::{software::SoftwareInterrupt, Priority};
use esp_hal_embassy::InterruptExecutor;
use static_cell::StaticCell;

static INTERRUPT_EXECUTOR: StaticCell<InterruptExecutor<1>> = StaticCell::new();
let interrupt_executor = INTERRUPT_EXECUTOR.init(InterruptExecutor::new(unsafe {
SoftwareInterrupt::<1>::steal()
}));

let spawner = interrupt_executor.start(Priority::max());

spawner.must_spawn(test_task(ctx.test_gpio1.degrade()));

#[embassy_executor::task]
async fn test_task(pin: AnyPin) {
let mut pin = Input::new(pin, Pull::Down);

// This line must return, even if the executor
// is running at a higher priority than the GPIO handler.
pin.wait_for_low().await;

embedded_test::export::check_outcome(());
}

loop {}
}
}
Loading