feat: take the SPI bus lock around our own transfers - #356
Merged
Conversation
thebentern
added a commit
to meshtastic/firmware
that referenced
this pull request
Jul 29, 2026
The tft task holds spiLock across the entire LVGL cycle - render, flush, SD map tile reads and panel sleep/wake commands included - and it must, since dropping the outer lock would leave those racing the radio. But on boards where the TFT, SD and LoRa radio share one SPI bus (T-Deck), the main loop then blocks behind a whole cycle's render+flush for every radio operation, felt as mesh RX/TX latency whenever the UI animates. Hand our spiLock to device-ui via setBusLock(). It releases and re-takes it between the pixel chunks of a redraw, so a blocked radio transaction can interleave and the main loop's worst-case wait drops to roughly one chunk instead of a whole cycle. The LVGL event registration lives in device-ui (meshtastic/device-ui#356) rather than here, so the firmware needs no LVGL headers: device-ui configures LVGL through private library.json flags that do not propagate to firmware translation units, and including an LVGL header here parses it under LVGL's default config instead - a struct/inline mismatch against the compiled library that corrupts lv_init() at boot. Requires the device-ui pin bump included here. Tested on T-Deck MUI: hook installs, boots clean, LoRa RX under an animating UI, no crashes.
Contributor
|
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:
📝 WalkthroughWalkthroughAdds a host-installable RAII SPI lock and applies it across display, touch, power, SD card, filesystem, log rotation, and security-key storage operations. ChangesSPI bus locking integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
include/graphics/DeviceScreen.h (1)
25-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude
<functional>inDeviceScreen.h.
DeviceScreen.hexposesstd::function<void()>in its public API but only gets<functional>viaDeviceGUI.h; add it directly so consumers includingDeviceScreen.halone are self-contained.🤖 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 `@include/graphics/DeviceScreen.h` around lines 25 - 27, Make DeviceScreen.h self-contained by directly including the standard header that defines std::function, which is used by DeviceScreen::setBusLock. Do not rely on the transitive inclusion through DeviceGUI.h.
🤖 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 `@source/graphics/DeviceGUI.cpp`:
- Around line 84-87: Update the event-hook registration in the getDisplay() path
to capture the lv_display_add_event_cb() result and handle a NULL descriptor.
Only emit the “Yielding host bus lock between flush chunks” debug log after
successful registration, and report registration failure through the existing
error-logging mechanism.
---
Nitpick comments:
In `@include/graphics/DeviceScreen.h`:
- Around line 25-27: Make DeviceScreen.h self-contained by directly including
the standard header that defines std::function, which is used by
DeviceScreen::setBusLock. Do not rely on the transitive inclusion through
DeviceGUI.h.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 581a3f3d-a531-439f-b221-a16fd9eb3758
📒 Files selected for processing (4)
include/graphics/DeviceGUI.hinclude/graphics/DeviceScreen.hsource/graphics/DeviceGUI.cppsource/graphics/DeviceScreen.cpp
Collaborator
|
I added another comment to 11278 with a nicer and more flexible interface ISpiLock which is added to DeviceGUI::create(): 
This interface can be easier passed around than a bunch of callback pointers. |
thebentern
force-pushed
the
spi-bus-lock-callback
branch
from
July 29, 2026 11:40
2243e49 to
bca0c94
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@include/graphics/driver/LGFXDriver.h`:
- Around line 391-394: Remove the long-lived ISpiLock::Guard bus from the
calibration flow around lgfx->calibrateTouch(). Update calibrateTouch() usage to
acquire the SPI lock only during actual touch-transfer operations, or invoke the
existing host-quiescing coordination mechanism for the prompt, ensuring shared
peripherals remain available while the user responds.
In `@source/graphics/common/SdCard.cpp`:
- Around line 255-259: Update SdFsCard::usedBytes() to avoid calling cardSize()
and freeBytes() while holding the non-recursive ISpiLock::Guard. Compute the
required card-size and free-space values directly under the existing guard, or
route through unlocked helper functions, while preserving the used-bytes result.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3635c567-fc28-47f8-bcd4-957d16f88de1
📒 Files selected for processing (7)
include/graphics/driver/LGFXDriver.hinclude/util/ISpiLock.hsource/graphics/TFT/TFTView_320x240.cppsource/graphics/common/SdCard.cppsource/graphics/map/SDCardService.cppsource/graphics/map/SdFatService.cppsource/util/ISpiLock.cpp
thebentern
added a commit
to meshtastic/firmware
that referenced
this pull request
Jul 29, 2026
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.
thebentern
changed the title
thebentern
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@include/graphics/DeviceScreen.h`:
- Around line 24-26: Resolve the overload ambiguity in DeviceScreen::create by
renaming the lock-only factory or changing it to accept a non-null lock
reference/wrapper, ensuring DeviceScreen::create(nullptr) selects the
DisplayDriverConfig overload unambiguously. Update corresponding declarations,
definitions, and call sites for the chosen API.
In `@source/graphics/DeviceScreen.cpp`:
- Around line 13-28: Update the ISpiLock::install calls used by all
DeviceScreen::create overloads so the process-wide SPI lock cannot be silently
replaced after initial installation. Preserve the first installed lock for
subsequent creates, and reject or otherwise explicitly handle attempts to
install a different or null lock while existing drivers may still use the
original.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dfdd1f2c-e7e8-4c9b-8eef-a2434d8d0e55
📒 Files selected for processing (4)
include/graphics/DeviceScreen.hinclude/util/ISpiLock.hsource/graphics/DeviceScreen.cppsource/graphics/common/SdCard.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- include/util/ISpiLock.h
thebentern
added a commit
to meshtastic/firmware
that referenced
this pull request
Jul 29, 2026
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.
thebentern
force-pushed
the
spi-bus-lock-callback
branch
from
July 29, 2026 12:51
7656ab9 to
c46b816
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/graphics/common/SdCard.cpp (1)
147-177: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftScope
ISpiLock::Guardto actual filesystem operations.These function-wide guards also cover parsing, set/string manipulation, serialization/deserialization, logging, and bookkeeping. Long map scans or log operations can monopolize the shared SPI bus and delay host-driven peripherals.
source/graphics/common/SdCard.cpp#L147-L177: guard individual directory/file operations; leave map-style processing outside the lock.source/graphics/common/SdCard.cpp#L304-L336: apply the same scoping to theSdFsCardbackend.source/util/LogRotate.cpp#L54-L84: guard file reads, including inside the deserialization callback, rather than the parser.source/util/LogRotate.cpp#L89-L109: guard open/write/close calls, not serialization or counter updates.source/util/LogRotate.cpp#L121-L157: guard individual cleanup operations, not the full directory walk and logging.🤖 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 `@source/graphics/common/SdCard.cpp` around lines 147 - 177, Restrict ISpiLock::Guard in source/graphics/common/SdCard.cpp lines 147-177 and 304-336 to the individual filesystem operations, leaving map-style parsing, set/string processing, logging, and bookkeeping outside the lock. Apply the same scoping in source/util/LogRotate.cpp lines 54-84 by guarding file reads, including reads in the deserialization callback, rather than the parser; in lines 89-109 guard only open/write/close calls; and in lines 121-157 guard each cleanup filesystem operation while leaving directory walking and logging unlocked.
🧹 Nitpick comments (1)
include/util/ISpiLock.h (1)
36-40: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake
yield()implement recursive release semantics.At Line 38, a single
unlock()only decrements recursive ownership for nestedGuards; it does not let another task acquire the underlying lock. Require implementations to release/reacquire their full recursive depth, rather than providing this incorrect default.Proposed fix
- virtual void yield(void) - { - unlock(); - lock(); - } + virtual void yield(void) = 0;🤖 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 `@include/util/ISpiLock.h` around lines 36 - 40, Update the default yield() implementation in ISpiLock to stop using a single unlock()/lock() pair; require each concrete implementation to release the underlying lock through its full recursive ownership depth, allow another task to acquire it, then reacquire that same depth. Preserve recursive ownership semantics for nested Guards.
🤖 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.
Outside diff comments:
In `@source/graphics/common/SdCard.cpp`:
- Around line 147-177: Restrict ISpiLock::Guard in
source/graphics/common/SdCard.cpp lines 147-177 and 304-336 to the individual
filesystem operations, leaving map-style parsing, set/string processing,
logging, and bookkeeping outside the lock. Apply the same scoping in
source/util/LogRotate.cpp lines 54-84 by guarding file reads, including reads in
the deserialization callback, rather than the parser; in lines 89-109 guard only
open/write/close calls; and in lines 121-157 guard each cleanup filesystem
operation while leaving directory walking and logging unlocked.
---
Nitpick comments:
In `@include/util/ISpiLock.h`:
- Around line 36-40: Update the default yield() implementation in ISpiLock to
stop using a single unlock()/lock() pair; require each concrete implementation
to release the underlying lock through its full recursive ownership depth, allow
another task to acquire it, then reacquire that same depth. Preserve recursive
ownership semantics for nested Guards.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f182ecf-92dc-46a0-8fc4-505edf1852b3
📒 Files selected for processing (11)
include/graphics/DeviceScreen.hinclude/graphics/driver/LGFXDriver.hinclude/util/ISpiLock.hsource/graphics/DeviceScreen.cppsource/graphics/TFT/TFTView_320x240.cppsource/graphics/common/SdCard.cppsource/graphics/map/SDCardService.cppsource/graphics/map/SdFatService.cppsource/util/FileLoader.cppsource/util/ISpiLock.cppsource/util/LogRotate.cpp
🚧 Files skipped from review as they are similar to previous changes (7)
- source/util/ISpiLock.cpp
- include/graphics/DeviceScreen.h
- source/graphics/TFT/TFTView_320x240.cpp
- source/graphics/map/SdFatService.cpp
- source/graphics/map/SDCardService.cpp
- source/graphics/DeviceScreen.cpp
- include/graphics/driver/LGFXDriver.h
thebentern
added a commit
to meshtastic/firmware
that referenced
this pull request
Jul 29, 2026
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.
Boards that share the display's SPI bus with other peripherals - an SD card, a LoRa radio - serialize access with a host-side lock. Meshtastic's firmware held that lock across the *entire* UI cycle, because it has no finer-grained way to know when we touch the bus. Most of a cycle is LVGL timer work and rendering into the draw buffer, which issues no SPI at all, so on T-Deck every radio operation on the host's main loop waited out a whole render+flush - tens to hundreds of milliseconds while the UI animates, felt as mesh RX/TX latency. Invert the ownership. ISpiLock is passed to DeviceScreen::create(); we take it only around real transfers, so the bus stays free during the CPU-only majority of a redraw and the host can stop holding it at all. Guarded: the flush (pushImage), SPI touch reads, panel init/fillScreen, printConfig's readCommand, touch calibration, the sleep/wake/power-save panel commands, every SD entry point (SdFsCard, the SD_MMC SDCard, both lv_fs tile services, TFTView's backup/restore), and the LittleFS paths in FileLoader and LogRotate - those last two share the filesystem object the host also serializes with this lock, and were only covered before by the coarse hold this removes. The interface is also held in a static, because two classes of call site cannot reach an instance member: the LVGL flush and touch callbacks are static and have no 'this', and the SD layer (sdCard, SDFs) is file-scope globals with no route to a DeviceGUI. No guard nests inside another: usedBytes() computes from cluster counts rather than calling cardSize()/freeBytes(), and LogRotate guards only its public entry points. Implementations should still be reentrant - the header says so - but nothing here depends on it. Nothing is guarded across DisplayDriver::view->sleep(), which suspends the SoC in light sleep, nor across controller->sendConfig() or messageAlert() in backup/restore - the former hands work to the radio, which needs this same bus from another task. Tested on T-Deck: flush, touch, panel init, SD detect, powersave sleep/wake, FileLoader's boot image and LogRotate's log directory all exercised; LoRa RX decoding with a live UI, no deadlocks, no watchdog resets.
thebentern
force-pushed
the
spi-bus-lock-callback
branch
from
July 29, 2026 13:13
c46b816 to
a93c1d3
Compare
thebentern
added a commit
to meshtastic/firmware
that referenced
this pull request
Jul 29, 2026
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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/util/LogRotate.cpp (1)
52-85: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
readNext()recurses while still holding its ownISpiLock::Guard, creating a nested acquisition.The
busguard taken at line 54 is still in scope when the failed-deserialize path recurses intoreadNext()at line 82, so the recursive call constructs a secondGuardon the same thread before the first is released. This is safe only if the host'sISpiLockis correctly reentrant — the same class of nesting the codebase deliberately eliminated inSdCard.cpp'susedBytes()rather than depend on. Converting the recursion to a loop removes the reliance on host reentrancy entirely.🔒 Proposed fix: replace recursion with a loop under the single guard
bool LogRotate::readNext(ILogEntry &entry) { ISpiLock::Guard bus; - if (!rootDir) { - rootDir = _fs.open(rootDirName); - if (!rootDir) - return false; - } - if (!currentFile) { - if (currentLogRead == 0 || currentLogRead > maxLogNum) - return false; - do { - currentFile = _fs.open(logFileName(currentLogRead), FILE_READ); - if (currentFile && currentFile.available()) { - break; - } - currentFile.close(); - currentLogRead++; - } while (currentLogRead <= maxLogNum); - if (!currentFile || !currentFile.available() || currentLogRead > maxLogNum) { - rootDir.close(); - return false; - } - ILOG_DEBUG("-> reading %s (%d bytes)", currentFile.name(), currentFile.size()); - } - - // elegant way to let the logentry do its work it knows best and pass just a temporary function for reading - if (!entry.deserialize([this](uint8_t *buf, size_t size) { return this->currentFile.read(buf, size); })) { - currentFile.close(); - currentLogRead++; - return readNext(entry); - } else - return true; + while (true) { + if (!rootDir) { + rootDir = _fs.open(rootDirName); + if (!rootDir) + return false; + } + if (!currentFile) { + if (currentLogRead == 0 || currentLogRead > maxLogNum) + return false; + do { + currentFile = _fs.open(logFileName(currentLogRead), FILE_READ); + if (currentFile && currentFile.available()) { + break; + } + currentFile.close(); + currentLogRead++; + } while (currentLogRead <= maxLogNum); + if (!currentFile || !currentFile.available() || currentLogRead > maxLogNum) { + rootDir.close(); + return false; + } + ILOG_DEBUG("-> reading %s (%d bytes)", currentFile.name(), currentFile.size()); + } + + // elegant way to let the logentry do its work it knows best and pass just a temporary function for reading + if (!entry.deserialize([this](uint8_t *buf, size_t size) { return this->currentFile.read(buf, size); })) { + currentFile.close(); + currentLogRead++; + continue; // retry with the next file under the same guard, no re-entry + } + return true; + } }Based on learnings, "Nested
ISpiLock::Guardusage must be safe for compliant hosts... avoid relying on nested guards only when the code can't guarantee this behavior."🤖 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 `@source/util/LogRotate.cpp` around lines 52 - 85, Update LogRotate::readNext to replace the recursive readNext call after deserialize failure with an iterative loop that continues opening and processing subsequent log files while the original ISpiLock::Guard remains active. Preserve the existing file-closing, currentLogRead increment, exhaustion handling, and successful deserialization behavior without acquiring a nested guard.Source: Learnings
🧹 Nitpick comments (1)
include/util/ISpiLock.h (1)
30-40: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDo not use this as an async-DMA yield primitive.
With one acquisition,
unlock()releases the bus while DMA may still be transferring; with nested reentrant acquisitions, it may not let a waiter proceed at all. Defer/remove this API until release is tied to confirmed transfer completion.🤖 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 `@include/util/ISpiLock.h` around lines 30 - 40, Remove or defer the virtual yield(void) API and its default unlock()/lock() implementation from ISpiLock; do not expose it as an async-DMA yield primitive until release can be tied to confirmed transfer completion.
🤖 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 `@include/util/ISpiLock.h`:
- Around line 42-49: Update the documentation for ISpiLock::install and
installed to state that the installed lock is retained as a raw global pointer
and must outlive all UI, LVGL, and SD activity, including every static callback
and guard use after installation. Do not imply that install takes ownership;
callers must provide storage with sufficient lifetime.
---
Outside diff comments:
In `@source/util/LogRotate.cpp`:
- Around line 52-85: Update LogRotate::readNext to replace the recursive
readNext call after deserialize failure with an iterative loop that continues
opening and processing subsequent log files while the original ISpiLock::Guard
remains active. Preserve the existing file-closing, currentLogRead increment,
exhaustion handling, and successful deserialization behavior without acquiring a
nested guard.
---
Nitpick comments:
In `@include/util/ISpiLock.h`:
- Around line 30-40: Remove or defer the virtual yield(void) API and its default
unlock()/lock() implementation from ISpiLock; do not expose it as an async-DMA
yield primitive until release can be tied to confirmed transfer completion.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 71eb49c6-6ae9-4b9e-a624-996ef57e79f2
📒 Files selected for processing (11)
include/graphics/DeviceScreen.hinclude/graphics/driver/LGFXDriver.hinclude/util/ISpiLock.hsource/graphics/DeviceScreen.cppsource/graphics/TFT/TFTView_320x240.cppsource/graphics/common/SdCard.cppsource/graphics/map/SDCardService.cppsource/graphics/map/SdFatService.cppsource/util/FileLoader.cppsource/util/ISpiLock.cppsource/util/LogRotate.cpp
Comment on lines
+42
to
+49
| /** | ||
| * Normally set through DeviceScreen::create(). Kept as a static holder because two | ||
| * classes of call site cannot reach an instance member: the LVGL flush and touch | ||
| * callbacks are static and have no `this`, and the SD layer (`sdCard`, `SDFs`) is | ||
| * file-scope globals with no route to a DeviceGUI. | ||
| */ | ||
| static void install(ISpiLock *lock); | ||
| static ISpiLock *installed(void); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Document the installed lock’s required lifetime.
install() retains this raw pointer globally and static callbacks dereference it after create() returns. A stack-scoped host lock would leave later guards with a dangling pointer. Require the lock to outlive all UI, LVGL, and SD activity, or give the registry ownership.
Proposed documentation
/**
* Normally set through DeviceScreen::create(). Kept as a static holder because two
* classes of call site cannot reach an instance member: the LVGL flush and touch
* callbacks are static and have no `this`, and the SD layer (`sdCard`, `SDFs`) is
* file-scope globals with no route to a DeviceGUI.
+ *
+ * The installed lock is borrowed. It must outlive every DeviceScreen and all guarded
+ * LVGL, filesystem, and SD activity.
*/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Normally set through DeviceScreen::create(). Kept as a static holder because two | |
| * classes of call site cannot reach an instance member: the LVGL flush and touch | |
| * callbacks are static and have no `this`, and the SD layer (`sdCard`, `SDFs`) is | |
| * file-scope globals with no route to a DeviceGUI. | |
| */ | |
| static void install(ISpiLock *lock); | |
| static ISpiLock *installed(void); | |
| /** | |
| * Normally set through DeviceScreen::create(). Kept as a static holder because two | |
| * classes of call site cannot reach an instance member: the LVGL flush and touch | |
| * callbacks are static and have no `this`, and the SD layer (`sdCard`, `SDFs`) is | |
| * file-scope globals with no route to a DeviceGUI. | |
| * | |
| * The installed lock is borrowed. It must outlive every DeviceScreen and all guarded | |
| * LVGL, filesystem, and SD activity. | |
| */ | |
| static void install(ISpiLock *lock); | |
| static ISpiLock *installed(void); |
🤖 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 `@include/util/ISpiLock.h` around lines 42 - 49, Update the documentation for
ISpiLock::install and installed to state that the installed lock is retained as
a raw global pointer and must outlive all UI, LVGL, and SD activity, including
every static callback and guard use after installation. Do not imply that
install takes ownership; callers must provide storage with sufficient lifetime.
mverch67
approved these changes
Jul 30, 2026
mverch67
left a comment
mverch67
left a comment
Collaborator
There was a problem hiding this comment.
I tested T-Deck with arduino-esp32 3.3.11 and when using the map scroll/zoom it was easy to crash (because of SX126x standby RadioLib err=-705).
Using this PR on top I wasn't able to reproduce the crash. So this PR should be merged as a precondition for arduino-esp 3.3.11 update.
Though, I'm not 100% sure if the lora assert is caused by 3.3.11 or the latest maps update. Anyway, this PR fixes it.
pull Bot
pushed a commit
to CrazyForks/firmware
that referenced
this pull request
Jul 31, 2026
…1278) 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. Co-authored-by: Manuel <71137295+mverch67@users.noreply.github.com>
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
2 participants
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.
Implements Option B from @mverch67's architectural analysis on meshtastic/firmware#11278, using the
ISpiLockinterface passed toDeviceScreen::create()as sketched there.Problem
Boards that share the display's SPI bus with other peripherals (SD card, LoRa radio) serialize access with a host-side lock. Meshtastic's firmware held that lock across the entire UI cycle, because it has no finer-grained way to know when we touch the bus. Most of a cycle is LVGL timer work and rendering into the draw buffer, which issues no SPI at all — so on T-Deck every radio operation on the host's main loop waited out a whole render+flush.
Change
ISpiLockis handed in viaDeviceScreen::create(); we take it only around real transfers, so the host can stop holding it entirely (firmware side: meshtastic/firmware#11278). When no lock is supplied the guards are no-ops, so behavior is unchanged for hosts that don't share a bus.Guarded: the flush (
pushImage), SPI touch reads, panelinit()/fillScreen,printConfig'sreadCommand, touch calibration, the sleep/wake/power-save panel commands, every SD entry point (SdFsCard, the SD_MMCSDCard, bothlv_fstile services,TFTView's backup/restore), and the LittleFS paths inFileLoaderandLogRotate.Those last two matter: they use the same filesystem object the host also serializes with this lock, and on the
task_handlerpath they were only covered incidentally by the coarse hold this removes.Design notes
The interface is also held in a static.
create()is the public spelling, but two classes of call site can't reach an instance member: the LVGL flush and touch callbacks arestaticwith nothis, and the SD layer (sdCard,SDFs) is file-scope globals with no route to aDeviceGUI.No guard nests inside another.
usedBytes()computes from cluster counts rather than callingcardSize()/freeBytes(), andLogRotateguards only its public entry points. Implementations should still be reentrant — the header says so — but nothing here depends on it.Never held across blocking non-SPI work: not across
DisplayDriver::view->sleep()(suspends the SoC), and not acrosscontroller->sendConfig()ormessageAlert()in backup/restore —sendConfig()hands work to the radio, which needs this same bus from another task.The one deliberate long hold is
calibrateTouch(), which blocks for user taps. LovyanGFX offers no hook to release mid-loop, dropping the guard would leave it the only unprotected SPI path, and it isn't a regression. In practice the branch is unreachable — every in-tree board ships stored calibration parameters, so the non-blocking path is taken.Not covered
If a board defines
LV_USE_FS_ARDUINO_SD(defaults to 0),MapTile::load()uses LVGL's own SD driver and bypasses these guards.Testing
T-Deck on hardware: flush, touch, panel init, SD detect, powersave sleep/wake,
FileLoader's boot image andLogRotate's log directory all exercised; LoRa RX decoding with a live UI (received and displayed a text message mid-run); no deadlocks and no watchdog resets — the latter being the signal a missed nesting case would produce.SD read/write paths are build-verified only — no card in the slot on my unit.
Summary by CodeRabbit
New Features
Bug Fixes