Repository navigation
Esp32: place tcpip_kick_thread() in IRAM - #3044
Merged
Merged
Conversation
system_os_post() is IRAM_ATTR and documented as ISR-safe, and is used from IRAM interrupt handlers via System.queueCallback() (e.g. the GPIO interrupt processor, or LEDC fade-end callbacks, whose ISR IDF always registers with ESP_INTR_FLAG_IRAM). However, it calls tcpip_kick_thread(), which was not marked IRAM_ATTR. If such an interrupt fires while the flash cache is disabled (during a flash write/erase, e.g. OTA or filesystem writes), fetching tcpip_kick_thread() from flash faults. Observed on ESP32-C3 (debug build) as "Illegal instruction" with MEPC in tcpip_kick_thread() and RA in system_os_post(), called from ledc_fade_isr. Why this is rarely seen: - The interrupt must coincide with a cache-disabled window, so it needs both frequent IRAM interrupts that post to the task queue and flash writes at the same time. - When the compiler inlines tcpip_kick_thread() into system_os_post(), the code ends up in IRAM anyway. In a debug (-Og) build it is emitted as a separate function in flash (.flash.text). The function only uses ISR-safe FreeRTOS/lwIP calls, so marking it IRAM_ATTR is sufficient. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PR Summary
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
another corner case:
I had issues with OTA failing while Esp32HardwarePWM was running.
HardwarePWM uses end-of-fade interrupts to enqueue new fades using
System.queueCallback().During OTA, I understand that flash cache is disabled and thus only routines explicitly marked
IRAM_ATTRis placed in IRAM. If a fade end happens within this no-cache window, this chain falls through totcpip_kick_thread()which is not in IRAM.Below is the analysis that copilot has provided, it looks consistent and the below fix adds IRAM_ATTR for
tcp_kick_thread().==== Copilot analysis below ====
system_os_post() is IRAM_ATTR and documented as ISR-safe, and is used from IRAM interrupt handlers via System.queueCallback() (e.g. the GPIO interrupt processor, or LEDC fade-end callbacks, whose ISR IDF always registers with ESP_INTR_FLAG_IRAM). However, it calls tcpip_kick_thread(), which was not marked IRAM_ATTR.
If such an interrupt fires while the flash cache is disabled (during a flash write/erase, e.g. OTA or filesystem writes), fetching tcpip_kick_thread() from flash faults. Observed on ESP32-C3 (debug build) as "Illegal instruction" with MEPC in tcpip_kick_thread() and RA in system_os_post(), called from ledc_fade_isr.
Why this is rarely seen:
The function only uses ISR-safe FreeRTOS/lwIP calls, so marking it IRAM_ATTR is sufficient.