Fix screen CS crosstalk window and add IRAM_ATTR to button ISRs#336
Open
Bullpuph123 wants to merge 1 commit into
Open
Fix screen CS crosstalk window and add IRAM_ATTR to button ISRs#336Bullpuph123 wants to merge 1 commit into
Bullpuph123 wants to merge 1 commit into
Conversation
selectScreen wrote all CS pins in a single pass, which could transiently select two panels at once and leak SPI data to the wrong orb. Now all screens are deselected first, then only the target is selected, with a bounds check on the screen index. Button ISRs run from GPIO interrupts and must be in IRAM: without IRAM_ATTR they can crash the ESP32 with a cache-disabled exception if the interrupt fires during a flash write (NVS/LittleFS). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
4 tasks
There was a problem hiding this comment.
Pull request overview
This PR improves ESP32 firmware reliability by (1) preventing transient SPI chip-select overlap when switching between TFT panels and (2) ensuring GPIO button interrupt handlers execute from IRAM to avoid crashes during flash cache-disabled windows.
Changes:
- Updated
ScreenManager::selectScreen()to deselect all CS lines first, validate the requested screen index, and only then assert the target CS (with rotation-aware mapping). - Marked button ISR entrypoints (
MainHelperISR wrappers andButton::isrButtonChange()) withIRAM_ATTRand documented the rationale (flash write / cache-disabled crash avoidance).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| firmware/src/core/utils/MainHelper.cpp | Marks the attachInterrupt ISR wrappers as IRAM_ATTR and clarifies ISR/IRAM requirements in comments. |
| firmware/src/core/screenmanager/ScreenManager.cpp | Makes screen selection a two-step “deselect-all then select-one” operation and adds an invalid-index guard + warning log. |
| firmware/src/core/button/Button.cpp | Marks the button edge handler as IRAM_ATTR so the ISR path runs from IRAM during flash cache-disabled periods. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ericthelin
approved these changes
Jul 16, 2026
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.
Split from #330 (part 3 of 3), rebased onto dev as requested.
Summary
Button::isrButtonChange()and the MainHelper ISR wrappers run from GPIO interrupts but weren't placed in IRAM. If the interrupt fires while flash cache is disabled (NVS/LittleFS write), the ESP32 crashes with aCache disabled but cached memory region accessedexception.millis()anddigitalRead()are IRAM-safe, so marking the handlers is sufficient.Test plan
pio run -e infoorbs-v0_3🤖 Generated with Claude Code