fix(firmware/web): /profiles page reliability + perf for large libraries#716
Open
FR4NKLN wants to merge 9 commits into
Open
fix(firmware/web): /profiles page reliability + perf for large libraries#716FR4NKLN wants to merge 9 commits into
FR4NKLN wants to merge 9 commits into
Conversation
…ousel
Two coupled changes to DefaultUI's favorited-profile handling that fix
post-bulk-import select lag AND the coex_enable boot loop at 50+ profiles.
Incremental mutators replace per-event reloadProfiles(). The four
profiles:profile:* handlers previously each set profileLoaded=0 and
forced profileLoopTask to re-read every favorited profile from SPIFFS
on its next tick — O(N) SPIFFS opens per change, ~1.5-4 s of contention
at 50 favorites. New onProfileSelected / onProfileFavorited /
onProfileUnfavorited / onProfileSaved touch only the affected entry,
O(1) per change. The cold-start full-reload path is preserved as a
safety net when the cache hasn't completed its initial build yet.
Lazy-load carousel decouples cache size from favorite count.
favoritedProfiles was a std::vector<Profile> with the default
(internal-heap) allocator, populated for every favorited entry at boot.
At 50+ Pro profiles this consumed ~100 KB of the ~320 KB internal RAM
budget — enough to starve the WiFi/BLE coexistence allocator that
coex_enable needs during NimBLEDevice::init after WiFi connect, causing
a boot loop with the backtrace:
coex_core_enable -> coex_enable -> esp_bt_controller_enable
-> NimBLEDevice::init -> Controller::setupBluetooth
favoritedProfileIds stays the full list (cheap — just Strings). The
parallel favoritedProfiles vector now holds a warm window of
PROFILE_CACHE_RADIUS=1 around currentProfileIdx; entries outside it are
default-constructed placeholders (no String/phases heap). onNextProfile
and onPreviousProfile synchronously load the new edge so the LVGL
effect renders populated data on the next frame; loopProfiles
maintains the window every 25 ms thereafter and evicts entries that
drift outside it. Internal-heap usage is now O(cache window), not
O(favorites).
…dProfile doc Three firmware defenses and one frontend resilience change that together close the failure modes PR jniebuhr#710 set out to address but whose implementations did not actually land in the merged diff. ws.makeBuffer() null-check at three call sites (handleProfileRequest + two req:history paths). PR jniebuhr#710's description claimed this guard shipped; verifying against origin/master, it did not. Without it the next line null-derefs when internal heap can't satisfy a 25-50 KB buffer allocation against post-broadcast fragmentation — crashes the WS task and disconnects every client. Per-client status-broadcast backlog guard. Also documented in PR jniebuhr#710 but not implemented — only setCloseClientOnQueueFull(false) landed. The guard skips clients whose per-client send queue is >= 8 deep, so 500ms status ticks no longer pile up behind a larger in-flight response and overflow the queue. PSRAM-back the per-call JsonDocument in ProfileManager::loadProfile. Each call previously allocated a fresh 6-12 KB node pool from the ~320 KB internal heap and released it on scope exit; for req:profiles:list that's N round-trips through internal heap per request, contributing to the fragmentation jniebuhr#710 set out to address. ESP32-S3 boards have 8 MB PSRAM otherwise idle. Frontend: try/catch + Array.isArray() guard around loadProfiles() in ProfileList/index.jsx, plus a red error banner with Retry button when the request fails. Previously a timed-out or undefined response sent setProfiles(undefined), which then crashed .map() and the ErrorBoundary swallowed the page — the dreaded infinite spinner.
Building the full profile-list JSON involves N synchronous SPIFFS opens
and ArduinoJson parses. At 50+ Pro profiles that is 5+ seconds of work,
which when run inside the AsyncTCP WS_EVT_DATA callback exceeds the
async_tcp task watchdog and reboots the device. Verified via serial:
E (75555) task_wdt: Task watchdog got triggered.
E (75555) task_wdt: - async_tcp (CPU 1)
Adds a ProfileListJob{clientId, rid, minimal} struct and a depth-4
FreeRTOS queue. handleProfileRequest for req:profiles:list now
allocates a job, xQueueSend's it, and returns immediately — AsyncTCP
unblocks in microseconds.
A dedicated WebUI::profileList task (core 0, priority 1, 6 KB stack)
drains the queue and runs buildAndSendProfileList, which performs the
SPIFFS loop and JSON build off the AsyncTCP critical path and sends
the response via ws.text(clientId, buffer). AsyncWebSocket's send
methods are queue-safe across tasks per its design.
A vTaskDelay(1) yield between SPIFFS reads is preserved — even though
the worker is off AsyncTCP, the SPIFFS mutex is still shared with the
static-file handler and the on-device profileLoopTask, and yielding
lets them interleave.
All other req:profiles:* types (load, save, select, favorite,
unfavorite, reorder) stay inline in handleProfileRequest — they're
single-file or metadata-only operations well below the watchdog
threshold.
GitHubOTA::checkForUpdates() does synchronous HTTPS to github.com. On a flaky network (DNS retry / SSL handshake stall) HTTPClient can hold for 30+ seconds. That call ran on WebUIPlugin::loop() on the Arduino main loop; the 500 ms status broadcast and every other plugin loop sat behind it for the duration, making the dashboard appear dead even though AsyncTCP itself was fine. Adds volatile bool otaCheckInProgress and a static otaCheckTask function. WebUIPlugin::loop() no longer calls checkForUpdates() directly — when the 5-minute interval fires, it spawns a one-shot 8 KB-stack task and returns. The task does the HTTPS, fires the ota:update:status event, calls updateOTAStatus, clears the in-progress flag, and vTaskDelete(NULL)s itself. otaCheckInProgress prevents overlapping spawns when a previous check is still hung on the network past the interval.
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR offloads profile-list generation and OTA checks to FreeRTOS worker tasks, implements a bounded lazy cache for favorited profiles in the UI with incremental event handlers, guards WebSocket buffer allocation against null, and adds frontend error tracking with retry. ChangesProfile and WebUI improvements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Per SonarQube `javascript:S6582`. The new form is equivalent in behavior (falls back to "Request failed" if `err` is undefined or `err.message` is falsy) and more concise. Remaining SQ flags on this PR are FreeRTOS-mandated patterns (`void *` task argument signatures and `new`/`delete` for heap-allocated job structs passed through `xQueueSend`); refactoring those would obscure intent without meaningfully improving safety.
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
Contributor
Author
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
jniebuhr
reviewed
May 29, 2026
| plugin->buildAndSendProfileList(*job); | ||
| delete job; | ||
| job = nullptr; | ||
| } |
Owner
There was a problem hiding this comment.
shouldn't there be some delay? even just a small one?
Contributor
Author
There was a problem hiding this comment.
portMAX_DELAY blocks the task rather than polling — xQueueReceive suspends it
(Blocked state, zero CPU) until a job is posted and FreeRTOS wakes it on the
xQueueSend. So it's not a busy loop — a delay would only add latency before the
list builds. Standard blocking-consumer pattern — so no sleep needed here
| // the user navigates the carousel to this entry. Avoids a SPIFFS read | ||
| // on the event-firing task (usually AsyncTCP) for an entry the user | ||
| // may never look at. | ||
| favoritedProfileIds.emplace_back(id); |
Owner
There was a problem hiding this comment.
This could change the order of profiles on the screen
Contributor
Author
There was a problem hiding this comment.
Good catch — fixed. onProfileFavorited no longer appends at the tail; it derives
the insert index from the new favorite's position in getFavoritedProfiles()
(which is profileOrder sorted) — discounting the selected profile pinned at index
0 — so the entry follows the configured order instead of jumping to the end.
Verified on-device: with a profile placed at profileOrder index 2 it inserted at
carousel index 2 rather than the tail (index 32 of 33). Pushed as 7e2358b.
FR4NKLN
added a commit
to FR4NKLN/gaggimate
that referenced
this pull request
May 29, 2026
The per-client send-queue backlog skip in the status broadcast is being handled separately as part of a broadcast-layer refactor (extracted broadcast method + shared-buffer textAll). Revert the status loop back to ws.textAll(statusDoc.as<String>()) so this PR stays focused on the profile-page reliability fixes. The makeBuffer null-checks and the PSRAM-backed loadProfile from the same defenses change are retained.
…el index onProfileFavorited appended the new favorite at the tail of the carousel, so a freshly-favorited profile jumped to the end instead of slotting into its configured profileOrder position. Derive the carousel index from the profile's position in getFavoritedProfiles() (already profileOrder-sorted), discounting the selected profile pinned at index 0. The Profile struct stays a lazy placeholder loaded on demand, so there is still no SPIFFS read on the event-firing task.
PSRAM-backing of ProfileManager::loadProfile's JsonDocument is now landed upstream in jniebuhr#723 (GM-7). Revert our copy so this PR no longer touches ProfileManager.cpp and merges cleanly onto current master.
![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: 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 `@src/display/ui/default/DefaultUI.cpp`:
- Around line 412-423: The code calls favoritedProfileIds.front() without
checking emptiness (used in the block computing selectedId and insertAt), which
can UB if onProfileUnfavorited() left favoritedProfileIds empty; fix by
early-checking favoritedProfileIds.empty() before using front() and handling
that case by rebuilding the carousel state or resetting selection (e.g., call
the routine that rebuilds the favorites carousel or clear selection and set
insertAt = 0), otherwise proceed with the existing logic that computes
selectedId, selectedBefore, sortedPos, sorted, and insertAt; ensure the
empty-case avoids using front() and inserts id correctly.
- Around line 400-424: DefaultUI::onProfileFavorited currently calls
favoritedProfileIds.front() without checking empty and the
favoritedProfileIds/favoritedProfiles vectors are mutated concurrently by
onProfileUnfavorited and loopProfiles (which runs in another FreeRTOS task) via
PluginManager::trigger, so add synchronization and a safety check: introduce a
mutex (e.g., std::mutex favoritedMutex) protecting both favoritedProfileIds and
favoritedProfiles, wrap all accesses/mutations in DefaultUI::onProfileFavorited,
DefaultUI::onProfileUnfavorited and DefaultUI::loopProfiles with a
std::lock_guard on that mutex, and before using favoritedProfileIds.front()
ensure favoritedProfileIds.empty() is false (handle empty by inserting at 0).
Ensure locks are held consistently and briefly to avoid deadlocks and that
insert/erase operations use the guarded vectors only.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0899ab9c-ab5c-4235-b204-56e73494b4c3
📒 Files selected for processing (1)
src/display/ui/default/DefaultUI.cpp
onProfileUnfavorited can erase the last entry, leaving favoritedProfileIds empty. The ordering fix's front() call would then deref an empty vector. Add favoritedProfileIds.empty() to the early guard so an empty cache defers to the cold-start rebuild (same recovery path as the !profileLoaded case).
|
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.
What & why
Severity: high. Once the favorited library is large — which happens fast,
since every profile is auto-favorited on import — both the
/profilespage andthe on-device carousel become unreliable. Several failure modes converge around
30+ profiles: a
coex_enableboot-loop, anasync_tcpwatchdog reboot when/profilesloads, a long dashboard freeze during the periodic OTA check onflaky networks, laggy selects after bulk saves, and a spinner-forever state on a
transient WS allocation failure. This PR scales both paths and addresses each.
Firmware
Lazy-load the on-device carousel (
DefaultUI). Cold-start builds thefavorited-ID list (cheap strings) but defers loading the
Profilestructs;only a small window around the displayed entry is resident at any time.
Loading every favorited profile synchronously put 100 KB+ on the internal
heap, starving the Wi-Fi/BLE coexistence allocator during Bluetooth bring-up
(
coex_enableaborts → boot loop). Lazy-load bounds the resident setregardless of library size.
O(1) incremental cache mutators for select / favorite / unfavorite / save,
replacing a blanket invalidation that re-read every favorited profile from
SPIFFS on the profile-loop task after each change.
Newly-favorited profiles keep their configured order.
onProfileFavoritedinserts at the carousel index derived from
getFavoritedProfiles()(alreadyprofileOrder-sorted), discounting the selected entry pinned at index 0,instead of appending at the tail — so favoriting no longer reorders the
carousel. (Addresses review feedback.)
Offload
req:profiles:listto a dedicated worker task (WebUIPlugin).Building a large list on the AsyncTCP task stalled the WebSocket and tripped
the
async_tcptask watchdog; it now runs on its own FreeRTOS task fed by aqueue, so AsyncTCP returns immediately.
Null-check
ws.makeBuffer()at all four call sites — the buffer allocatesfrom internal heap and can fail under fragmentation; results were previously
dereferenced unconditionally.
Run the OTA update check on a one-shot task so a slow HTTPS check can't
block the main loop and stall the status broadcast.
Web
/profilesload-error handling. The list request is wrapped in try/catch;on failure the page shows an error alert with a Retry button instead of a
permanent spinner, and non-array responses are handled defensively.
Test plan
masterwith a large auto-favorited library — nocoex_enableabort; Wi-Fi, controller link (protocol match), and webserverall come up
profileOrderslot in the carousel,not the tail
/profilesloads without an infinite spinner; a timeout / non-2xx surfacesthe Retry banner
req:profiles:listbuild runs off the AsyncTCP task (worker queue) — noasync_tcpwatchdog rebootevent)
Notes
ProfileManager::loadProfilePSRAM-backing an earlier revision carried isdropped — feat(display): route remaining JsonDocuments to PSRAM (GM-7) #723 lands the same change.
Summary by CodeRabbit
Performance
Improvements