Fix screen CS crosstalk window and add IRAM_ATTR to button ISRs #336

Open

Bullpuph123 wants to merge 1 commit from Bullpuph123/fix/screenmanager-isr into dev

Bullpuph123 commented

2026-07-15 00:47:29 +00:00

(Migrated from github.com)

Split from #330 (part 3 of 3), rebased onto dev as requested.

Summary

selectScreen: deselect-all-first: the previous single-pass loop wrote CS pins in order, which can transiently leave both the old and new panel selected, letting SPI data leak to the wrong orb during the transition (observed on hardware as stray pixels appearing on a different orb). Now all CS lines are raised first, then only the target is lowered. Also adds a bounds check on the screen index (logs a warning and leaves everything deselected instead of driving a wrong/out-of-range CS pin), and hoists the ConfigManager read out of the loop.
IRAM_ATTR on button ISRs: 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 a Cache disabled but cached memory region accessed exception. millis() and digitalRead() are IRAM-safe, so marking the handlers is sufficient.

Test plan

Builds with pio run -e infoorbs-v0_3
Press buttons during a config save (flash write) — no crash
Verify widgets still render to the correct orbs with orbRotation 0-3

🤖 Generated with Claude Code

Split from #330 (part 3 of 3), rebased onto dev as requested. ## Summary - **selectScreen: deselect-all-first**: the previous single-pass loop wrote CS pins in order, which can transiently leave both the old and new panel selected, letting SPI data leak to the wrong orb during the transition (observed on hardware as stray pixels appearing on a different orb). Now all CS lines are raised first, then only the target is lowered. Also adds a bounds check on the screen index (logs a warning and leaves everything deselected instead of driving a wrong/out-of-range CS pin), and hoists the ConfigManager read out of the loop. - **IRAM_ATTR on button ISRs**: `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 a `Cache disabled but cached memory region accessed` exception. `millis()` and `digitalRead()` are IRAM-safe, so marking the handlers is sufficient. ## Test plan - [x] Builds with `pio run -e infoorbs-v0_3` - [x] Press buttons during a config save (flash write) — no crash - [x] Verify widgets still render to the correct orbs with orbRotation 0-3 🤖 Generated with [Claude Code](https://claude.com/claude-code)

copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed

2026-07-15 23:43:47 +00:00

copilot-pull-request-reviewer[bot] (Migrated from github.com) left a 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 (MainHelper ISR wrappers and Button::isrButtonChange()) with IRAM_ATTR and 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.

## 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 (`MainHelper` ISR wrappers and `Button::isrButtonChange()`) with `IRAM_ATTR` and 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. | --- 💡 <a href="/brettdottech/info-orbs/new/dev?filename=.github/instructions/*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.

ericthelin (Migrated from github.com) approved these changes

2026-07-16 00:40:38 +00:00

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin Bullpuph123/fix/screenmanager-isr:Bullpuph123/fix/screenmanager-isr

git switch Bullpuph123/fix/screenmanager-isr

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch dev

git merge --no-ff Bullpuph123/fix/screenmanager-isr

git switch Bullpuph123/fix/screenmanager-isr

git rebase dev

git switch dev

git merge --ff-only Bullpuph123/fix/screenmanager-isr

git switch Bullpuph123/fix/screenmanager-isr

git rebase dev

git switch dev

git merge --no-ff Bullpuph123/fix/screenmanager-isr

git switch dev

git merge --squash Bullpuph123/fix/screenmanager-isr