GPIO: prevent woken task interrupting GPIO handler #2838
Conversation
|
|
||
| crate::interrupt::free(|| { | ||
| asynch::PIN_WAKERS[pin_nr as usize].wake(); | ||
| set_int_enable(pin_nr, Some(0), 0, false); | ||
| }); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
I guess it might be safe enough, but I'm not confident enough.
bugadani
changed the title
bugadani
changed the title
| set_int_enable(pin_nr, Some(0), 0, false); | ||
|
|
||
| crate::interrupt::free(|| { | ||
| asynch::PIN_WAKERS[pin_nr as usize].wake(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This PR fixes an issue when InterruptExecutor uses async GPIO operations, but the GPIO handler runs on a lower priority. The issue caused the interrupt executor to enter an infinite loop.