- Notifications
You must be signed in to change notification settings - Fork8.2k
ports/esp32: Fix crash and improve timer interrupt allocation#17265
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Open
DvdGiessen wants to merge2 commits intomicropython:masterChoose a base branch
base:master
Could not load branches
Branch not found:{{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline, and old review comments may become outdated.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
2e0fb59 to2015bdaCompareesp_intr_free is not safe to call from the timer ISR because it requiresthe current task (the one the ISR interrupted) to be pinned to the samecore as the interrupt was allocated on. Merely disabling the ISR however issafe since that only requires that we're currently running on the same core(which the ISR always is), regardless of the current task.This was causing deadlocks in machine_uart when the ISR happened tointerrupt a task that was not pinned to a specific core.Signed-off-by: Daniël van de Giessen <daniel@dvdgiessen.nl>
If the interrupt is not freed but merely disabled, instead of reallocatingit every time the timer is enabled again we can instead just re-enable it.That means we're no longer setting the handler every time, and we need toensure it does not change. Doing so by adding an additional wrapperfunction does not only solve that problem, it also allows us to removesome code duplication and simplify how machine_uart uses the timer.Signed-off-by: Daniël van de Giessen <daniel@dvdgiessen.nl>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Labels
None yet
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
esp_intr_freeis not safe to call from the timer ISR because it requires thecurrent task (the one the ISR interrupted) to be pinned to the same core as the interrupt was allocated on. The ISR is of course guaranteed to interrupt a task on the same core, but it's not guaranteed that that task is also pinned to that core.This was causing a lockup followed by ISR watchdog timeout in the
machine_uartRXIDLE timer (which disables the timer from its own callback) when the ISR happened to interrupt a task that was not pinned to a specific core (for example for me if often hit the lwIP TCP/IP thread task).The first commit in this PR fixes that by merely disabling the interrupt, whichis safe to do from the ISR since that only requires that we're currently running on the same core (which the ISR always is), regardless of the current task.
Additionally, the second commit makes repeated disabling and enabling of the interrupt (such as the UART RXIDLE does) a bit more efficient by re-enabling instead of reallocating it. That also allowed to removing some code duplication and simplify how
machine_uartuses the timer.Testing
I've been using these fixes in combination with#17138 (which changed the PPP implementation to use that RXIDLE IRQ). Like that PR, I've been running this on three custom boards (1x original ESP32, 2x ESP32S3) for a few weeks at the moment of writing.