MUI: stop holding the SPI bus across the whole UI cycle - #11278
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTFT setup adds a reentrant SPI lock, delegates task-handler locking to device-ui, passes the lock through both display creation paths, and updates the pinned device-ui dependency revision. ChangesTFT bus coordination
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/graphics/tftSetup.cpp (1)
17-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce these explanatory comment blocks.
The new comments substantially exceed the project’s one- or two-line limit. Keep only the non-obvious rationale; the function name and registration site cover the rest.
Proposed simplification
-// device-ui configures LVGL through its private build flags (-Iinclude -// -DLV_CONF_INCLUDE_SIMPLE in its library.json), which do not propagate to firmware -// translation units. Point LVGL at that same lv_conf.h explicitly before including any -// LVGL header here: parsing them under LVGL's default config instead produces struct -// and inline-function definitions that mismatch the compiled library (an ODR -// violation), which corrupts lv_init() at boot. +// Use the same LVGL configuration as the device-ui library. -/** - * Give waiting SPI users a window before each chunk of pixels is pushed. - * - * ... - */ +// Let waiters acquire the shared SPI bus between flush chunks.As per coding guidelines, “Keep code comments minimal—one or two lines maximum—and comment only when the reason is not obvious.”
Also applies to: 40-58, 166-168
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/graphics/tftSetup.cpp` around lines 17 - 22, Shorten the explanatory comments in the LVGL setup and the additional locations at lines 40–58 and 166–168 to no more than one or two lines each. Retain only the non-obvious rationale, specifically the need to use the matching LVGL configuration, and remove details already evident from the function names or registration code.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/graphics/tftSetup.cpp`:
- Around line 17-22: Shorten the explanatory comments in the LVGL setup and the
additional locations at lines 40–58 and 166–168 to no more than one or two lines
each. Retain only the non-obvious rationale, specifically the need to use the
matching LVGL configuration, and remove details already evident from the
function names or registration code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 78f95766-d180-4ad4-8262-5fc27aa5dd7c
📒 Files selected for processing (1)
src/graphics/tftSetup.cpp
⚡ Try this PR in the Web Flasher
Warning This is an automated, unreviewed CI test build. Back up your device configuration Supported boards built by this PR (31)
Build artifacts expire on 2026-08-29. Updated for |
There was a problem hiding this comment.
From software architecture point of view it would be better to provide a callback std::function to DeviceGUI (either as parameter to init() or separate method) that is called by the lvgl library in case of LV_EVENT_FLUSH_START.
This way you can keep all lvgl related code out of the firmware and achieve exactly the same behavior.
Edit: providing the possibility for device-ui to acquire a lock is also required when accessing further SPI devices, not only TFT display as instructed by LVGL but also SD card access.
Architectural Analysis: SPI Lock SynchronizationCurrent ProblemThe
Phases 1 and 2 can account for the majority of the cycle time, unnecessarily blocking the LoRa thread from the SPI bus for tens to hundreds of milliseconds. Option A: Keep current firmware lock + add yield callbackThe most natural hook is Why it's insufficient:
Option B: Explicit |
| Location | Change |
|---|---|
| DeviceGUI / DisplayDriver | Accept and store ISpiLock* |
| Display flush callback | Wrap with lock() / unlock() |
| SD card driver (any SPI I/O) | Wrap each operation with lock() / unlock() |
| tftSetup.cpp | Remove lock from task loop; pass ISpiLock to DeviceScreen::create() |
Scope assessment: The changes are localized to driver-level call sites — not pervasive. The ISpiLock interface is 2–3 virtual methods. The flush callback wrapping is a single location per display driver. SD card wrapping is however many spi_transfer call sites exist in device-ui.
Does the gain justify the changes?
Yes, clearly — particularly in Meshtastic's case:
- LoRa receive latency is the primary concern. The LoRa interrupt handler needs SPI access to read the received packet. Every millisecond the SPI bus is blocked by LVGL rendering increases the risk of buffer overflow in the radio FIFO. A 240×320 TFT full redraw can hold the bus for 30–80ms.
- The firmware/task separation is already established. The SPI bus sharing contract is already encoded in
spiLock. Option B formalizes that contract at the correct level of granularity. yield()becomes an optional additive optimization on top of Option B — useful if you later add DMA-based async flushing (the display driver signals flush complete asynchronously, device-ui can release the lock while DMA runs). It's not a prerequisite.
Recommendation
Implement Option B. Option A is a partial workaround that doesn't address SD card I/O and provides negligible benefit for the display case without async DMA. The ISpiLock interface is small, the changes in device-ui are localized, and the architectural result is correct: the SPI bus is contended only during actual transfers, giving LoRa the bus during the much longer LVGL rendering window.
194f40a to
c21f7ee
Compare
|
Good call — done in meshtastic/device-ui#356, and this PR now just hands the lock over via That turned out to matter for more than layering: device-ui configures LVGL through private On the edit re: acquiring the lock for SD access — I exposed Note this PR now needs #356 to land first; the pin here points at the branch commit and I'll bump it to the merged master hash. Tested on T-Deck: hook installs, boots clean, LoRa RX under an animating UI, no crashes. |
c21f7ee to
e477e2e
Compare
thebentern
changed the title
|
Agreed on Option B — rebuilt both PRs that way. The yield version is gone; the firmware no longer holds the lock at all, and device-ui takes it around actual transfers (meshtastic/device-ui#356). You were right about SD being the deciding factor. One correction on the display case though, since it affects how you weigh the interim: the yield wasn't microsecond-scale. The tft task is pinned to core 0 and loopTask runs on core 1, so giving the semaphore lets a blocked loopTask acquire it and run its whole transaction on its own core while the tft task's re-take blocks behind it. Moot now, but the real reason Option A was insufficient is the one you gave second — the lock stayed held through the CPU-only render phase, and SD was untouched. Two things from implementing it that are worth your eyes: The lock holder has to be static, not a member. Your table had Reentrancy is mandatory, and that's on the host. Also: nothing is held across Sites guarded: flush, SPI touch reads, panel init/fillScreen, Tested on T-Deck: flush, touch, init, SD detect, powersave sleep and wake all exercised, LoRa RX decoding under a live UI, no deadlocks or watchdog resets. SD read/write is build-verified only — no card in my slot, so |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/graphics/tftSetup.cpp (2)
32-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComment block exceeds the project's minimal-comment guideline.
This 21-line multi-paragraph explanation conflicts with the guideline restricting comments to one or two lines, reserved for non-obvious reasoning. Consider trimming this to a short pointer (e.g., 1-2 lines noting reentrancy is required because device-ui nests lock acquisitions) and moving the detailed rationale to the PR description or a design doc.
As per coding guidelines, "Keep code comments minimal—one or two lines maximum—and comment only when the reason is not obvious; do not restate straightforward code or add multi-paragraph explanatory blocks."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/graphics/tftSetup.cpp` around lines 32 - 52, Trim the large comment block above the SPI lock handling to one or two lines. Retain only the non-obvious requirement that reentrancy is needed because device-ui can nest lock acquisitions, and remove the detailed performance, ownership, and implementation rationale from the source comment.Source: Coding guidelines
72-82: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider an
assert()on the depth invariant before decrementing.If
unlock()is ever called without a matchinglock()(a bug elsewhere),--depthon an already-zerouint32_twraps toUINT32_MAX, silently masking the bug and leaving the lock permanently held from that thread's perspective. Anassert(depth > 0)before the decrement would surface such misuse immediately during development.As per coding guidelines, "Use
assert()for invariants that should never fail."🛡️ Proposed fix
void unlock(void) override { `#ifdef` HAS_FREE_RTOS + assert(depth > 0); if (--depth == 0) { owner = nullptr; spiLock->unlock(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/graphics/tftSetup.cpp` around lines 72 - 82, In the HAS_FREE_RTOS branch of unlock(), validate the depth invariant with assert(depth > 0) before decrementing depth. Keep the existing owner reset and spiLock->unlock() behavior unchanged when the resulting depth reaches zero.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/graphics/tftSetup.cpp`:
- Around line 56-82: Update ReentrantSpiLock’s non-HAS_FREE_RTOS lock and unlock
paths to track an owner and recursion depth, matching the existing reentrant
behavior without FreeRTOS task handles. Use a portable owner representation and
ensure owner/depth state is initialized, incremented for nested acquisitions,
and cleared only when the outermost unlock releases spiLock.
---
Nitpick comments:
In `@src/graphics/tftSetup.cpp`:
- Around line 32-52: Trim the large comment block above the SPI lock handling to
one or two lines. Retain only the non-obvious requirement that reentrancy is
needed because device-ui can nest lock acquisitions, and remove the detailed
performance, ownership, and implementation rationale from the source comment.
- Around line 72-82: In the HAS_FREE_RTOS branch of unlock(), validate the depth
invariant with assert(depth > 0) before decrementing depth. Keep the existing
owner reset and spiLock->unlock() behavior unchanged when the resulting depth
reaches zero.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 974a120c-6e6f-44c3-b284-abe8a622220c
📒 Files selected for processing (2)
platformio.inisrc/graphics/tftSetup.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- platformio.ini
e477e2e to
d581388
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/graphics/tftSetup.cpp (3)
71-77: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard against
depthunderflow inunlock().If
unlock()is ever called without a matchinglock()(e.g. a device-ui guard bug or double-unlock on an error path),--depthwrapsuint32_ttoUINT32_MAX, silently skipping the release branch. The design happens to self-heal on the next fresh acquisition (since the fast path also checksowner == self), but this mismatch goes completely undetected — exactly the kind of invariant anassert()should catch during development.As per coding guidelines, "Use assert() for invariants that should never fail."
🛡️ Proposed fix
void unlock(void) override { + assert(depth > 0 && owner == currentThread()); if (--depth == 0) { owner = ThreadId(); spiLock->unlock(); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/graphics/tftSetup.cpp` around lines 71 - 77, Guard the depth invariant in unlock() by asserting that depth is greater than zero before decrementing it. Keep the existing owner reset and spiLock release behavior when the decremented depth reaches zero.Source: Coding guidelines
35-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim the class doc comment to fit the comment-length guideline.
This 20-line, multi-paragraph explanatory block (history, rationale, and synchronization proof) exceeds the repo's stated comment style. Condense to the essential "why" (reentrancy needed because device-ui guards per-method while
spiLockis a plain binary semaphore) and drop the historical narrative/paragraph breaks.As per coding guidelines, "Keep code comments minimal—one or two lines maximum—and comment only when the reason is not obvious; do not restate straightforward code or add multi-paragraph explanatory blocks."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/graphics/tftSetup.cpp` around lines 35 - 55, Trim the class documentation above the spiLock implementation to one or two lines stating only that reentrancy is required because device-ui guards individual methods while spiLock is a plain binary semaphore. Remove the historical context, performance rationale, and synchronization proof without changing the implementation.Source: Coding guidelines
59-92: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winVerify plain
owner/depthreads are safe across ESP32-S3's two cores.
lock()'s fast path readsowner/depthwithout holdingspiLock, whileunlock()writes them with only the doc comment's informal reasoning as protection ("only the owner writes... so no additional synchronization is needed"). T-Deck's ESP32-S3 is a genuine dual-core part, andtft_task_handleris pinned to core 0 (xTaskCreatePinnedToCore(..., 0)at line 198) while other callers of the sharedreentrantSpiLock/spiLock(e.g. LoRa RX callers per the PR description) likely run on the other core — so this is real cross-core shared state, not just same-core reordering. Per the C++ memory model this is a data race (UB) even if the current owner-check logic tends to fail safe in practice.Consider making
owneranddepthstd::atomicfor defense-in-depth. Note:std::atomic<TaskHandle_t>(a pointer) is fine, butstd::atomic<std::thread::id>is not portably guaranteed to compile since the standard does not guaranteestd::thread::idis trivially copyable (LWG issue 1277) — verify this compiles for the Portduino/native target before adopting it there, or use a raw integer/pointer-based thread identifier instead ofstd::thread::idfor that branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/graphics/tftSetup.cpp` around lines 59 - 92, Make the shared owner and depth state in the reentrant lock implementation race-free across cores. Update lock() and unlock() to use atomic-compatible storage and operations for both fields, while preserving recursive ownership and release behavior; use an identifier representation that supports portable atomic storage in the non-FreeRTOS currentThread() branch, and verify the HAS_FREE_RTOS and Portduino/native builds compile.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/graphics/tftSetup.cpp`:
- Around line 71-77: Guard the depth invariant in unlock() by asserting that
depth is greater than zero before decrementing it. Keep the existing owner reset
and spiLock release behavior when the decremented depth reaches zero.
- Around line 35-55: Trim the class documentation above the spiLock
implementation to one or two lines stating only that reentrancy is required
because device-ui guards individual methods while spiLock is a plain binary
semaphore. Remove the historical context, performance rationale, and
synchronization proof without changing the implementation.
- Around line 59-92: Make the shared owner and depth state in the reentrant lock
implementation race-free across cores. Update lock() and unlock() to use
atomic-compatible storage and operations for both fields, while preserving
recursive ownership and release behavior; use an identifier representation that
supports portable atomic storage in the non-FreeRTOS currentThread() branch, and
verify the HAS_FREE_RTOS and Portduino/native builds compile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 029c7d35-85d1-4911-9f3b-5075e920e8ac
📒 Files selected for processing (2)
platformio.inisrc/graphics/tftSetup.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- platformio.ini
d581388 to
318e86f
Compare
tft_task_handler held spiLock for the entire LVGL cycle. Most of that cycle is timer work and rendering into the draw buffer, which issues no SPI at all - but on boards where the TFT, SD card and LoRa radio share one bus (T-Deck), every radio operation on the main loop still waited it out. That is tens to hundreds of milliseconds whenever the UI animates, felt as mesh RX/TX latency. device-ui now takes the lock around its own transfers instead (meshtastic/device-ui#356), so the coarse hold here can go and the bus is contended only during real traffic. Lend it spiLock through a reentrant adapter. device-ui nests its guards - SdFsCard::usedBytes() calls cardSize() and freeBytes(), each of which takes the lock - while spiLock is a plain binary semaphore that would self-deadlock on the second take, so track the owning task and only touch the underlying lock on the outermost acquire. Requires the device-ui pin bump included here. Tested on T-Deck: flush, touch, panel init, SD detect, powersave sleep and wake all exercised; LoRa RX decoding under a live UI, no deadlocks, no watchdog resets. Also builds seeed-sensecap-indicator-tft.
318e86f to
6d0a289
Compare
2 participants
Implements Option B from @mverch67's architectural analysis. Paired with meshtastic/device-ui#356.
Problem
tft_task_handlerheldspiLockfor the entire LVGL cycle. Most of that cycle is timer work and rendering into the draw buffer, which issues no SPI at all — but on boards where the TFT, SD card and LoRa radio share one bus (T-Deck: GPIO 40/41), every radio operation on the main loop still waited it out. That's tens to hundreds of milliseconds whenever the UI animates, felt as mesh RX/TX latency.Change
device-ui now takes the lock around its own transfers, so the coarse hold here goes away entirely and the bus is contended only during real traffic.
The firmware lends
spiLockthrough a reentrant adapter. device-ui nests its guards —SdFsCard::usedBytes()callscardSize()andfreeBytes(), each of which takes the lock — whilespiLockis a plain binary semaphore (xSemaphoreCreateBinary) that would self-deadlock on the second take. The adapter tracks the owning task and only touches the underlying lock on the outermost acquire.Requires the device-ui pin bump included here; #356 needs to land first and then this pin moves to the merged master hash.
Testing
T-Deck on hardware: flush, touch, panel init, SD detect, powersave sleep and wake all exercised; LoRa RX decoding under a live UI (received and displayed a text message mid-run); no deadlocks and no watchdog resets over sustained monitoring — the latter being the signal a missed nesting case would produce. Builds
t-deck-tftandseeed-sensecap-indicator-tft.SD read/write paths are build-verified only — no card in the slot on my unit.
Summary by CodeRabbit