Conversation
Delete common/ui.c (1647 lines) — manifest UI is now the only UI path. Changes: - Delete common/ui.c entirely - Remove USE_MANIFEST option from CMakeLists.txt - bridge_client.c: remove dispatch macros (UI_SET_STATUS etc.), call manifest_ui_* directly; remove dead code (now_playing_state, fetch_now_playing, ui_update_cb, post_ui_update, default_now_playing) - manifest_ui.c: remove #if USE_MANIFEST guard around shim section, remove unused shims (ui_init, ui_set_status, ui_set_artwork, etc.), keep implementations for functions still called externally (ui_loop_iter, ui_set_controls_visible, OTA, battery) - app_main.c: call manifest_ui_* directly (no conditional) - main_idf.c: call manifest_ui_init() directly, replace ui_update() calls with ui_set_message() - platform_input_idf.c: call manifest_ui_* directly (no conditional) - ui.h: trim to shared types + functions still called from external files (ui_loop_iter, ui_set_message, ui_set_zone_name, settings, OTA, controls, network status, battery) - Update comments in manifest_ui.c/h to remove ui.c references Closes #132
📝 WalkthroughWalkthroughLegacy LVGL-based UI and all USE_MANIFEST compile-time branches were removed; the codebase now always uses the manifest-driven UI (manifest_ui_*). Legacy Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Main
participant Bridge as Bridge Client
participant Platform as Platform Input
participant ManifestUI as manifest_ui
App->>ManifestUI: manifest_ui_init()
Platform->>ManifestUI: input callbacks (set via manifest bridge)
Platform->>Bridge: send input events / selection changes
Bridge->>ManifestUI: manifest_ui_set_zone_name(...)
Bridge->>ManifestUI: manifest_ui_set_message(...) / manifest_ui_set_network_status(...)
ManifestUI-->>App: render/update (manifest-driven)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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 |
Firmware PreviewFlash this PR's firmware directly in your browser: Updated: 2026-02-25T09:21:04.275Z |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (3)
common/bridge_client.c (2)
993-1002: 🛠️ Refactor suggestion | 🟠 MajorInconsistent UI dispatch: direct
manifest_ui_set_*calls here vspost_ui_*wrappers elsewhere in the same function.Within
bridge_poll_thread, Line 993 callsmanifest_ui_set_network_status(NULL)directly (correct single-post), while Line 992 usespost_ui_message("Bridge: Connected")(double-post). Similarly, Line 1002 callsmanifest_ui_set_zone_name(zone_name_copy)directly but other zone name updates usepost_ui_zone_name. This inconsistency reinforces the need to standardize on directmanifest_ui_set_*calls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/bridge_client.c` around lines 993 - 1002, In bridge_poll_thread, fix the inconsistent UI dispatch by using the direct manifest_ui_set_* calls everywhere in this function instead of mixing post_ui_* wrappers: replace occurrences of post_ui_message("Bridge: Connected") and any post_ui_zone_name(...) calls with the equivalent manifest_ui_set_network_status(...) / manifest_ui_set_zone_name(...) invocations (or remove redundant double-posts), and ensure you still copy the zone name into zone_name_copy under lock before calling manifest_ui_set_zone_name(zone_name_copy) so thread-safety and single direct UI updates (manifest_ui_set_network_status, manifest_ui_set_zone_name) are consistent.
166-191:⚠️ Potential issue | 🟠 MajorDouble-posting: callbacks already run on the UI thread but call
manifest_ui_set_*which posts to the UI thread again.
ui_status_cb,ui_message_cb, andui_zone_name_cbare dispatched viaplatform_task_post_to_ui, so they already execute on the UI thread. But callingmanifest_ui_set_status/manifest_ui_set_message/manifest_ui_set_zone_namefrom these callbacks enqueues another post to the UI thread (with an extrastrdupfor the string variants). This means:
- Extra latency — the actual LVGL label update is deferred to the next
platform_task_run_pending()cycle.- Extra allocations —
manifest_ui_set_messagedoesstrdup, thenui_cb_set_messagedoesfreeon the original copy, while the second copy lives until processed.Since
manifest_ui_set_*already handles thread-safe posting internally, the simplest fix is to eliminate thepost_ui_*wrappers and callmanifest_ui_set_*directly from the poll thread:♻️ Proposed fix — remove double-posting helpers
-static void ui_status_cb(void *arg) { - bool *online = arg; - if (!online) { - return; - } - manifest_ui_set_status(*online); - free(online); -} - -static void ui_message_cb(void *arg) { - char *msg = arg; - if (!msg) { - return; - } - manifest_ui_set_message(msg); - free(msg); -} - -static void ui_zone_name_cb(void *arg) { - char *name = arg; - if (!name) { - return; - } - manifest_ui_set_zone_name(name); - free(name); -}Then replace all
post_ui_message(...)calls withmanifest_ui_set_message(...),post_ui_zone_name(...)withmanifest_ui_set_zone_name(...), andpost_ui_status(...)withmanifest_ui_set_status(...). Thepost_ui_status_copy,post_ui_message_copy,post_ui_zone_name_copyhelpers and their forward declarations can also be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/bridge_client.c` around lines 166 - 191, The callbacks ui_status_cb, ui_message_cb, and ui_zone_name_cb are double-posting to the UI thread: they run on the UI thread already (via platform_task_post_to_ui) and then call post_ui_* wrappers which post again and strdup strings, causing extra latency and allocations; replace uses of post_ui_message/post_ui_zone_name/post_ui_status with direct calls to manifest_ui_set_message/manifest_ui_set_zone_name/manifest_ui_set_status, remove the post_ui_* and post_ui_*_copy helpers and their forward declarations, and update the callbacks (ui_status_cb, ui_message_cb, ui_zone_name_cb) to call the manifest_ui_set_* functions directly while preserving correct ownership/free semantics (do not strdup before calling manifest functions; free the original buffer only if the manifest functions duplicate it).common/manifest_ui.c (1)
1702-1705:⚠️ Potential issue | 🟠 MajorRemove the redundant
lv_task_handler()call; in LVGL 9.x it's only a compatibility alias.
lv_task_handler()is the legacy v7 name for tasks, kept as a wrapper via the v8 API mapping header for backward compatibility. In LVGL 9.x,lv_timer_handler()is the actual handler andlv_task_handler()merely aliases it. Calling both causes the same handler logic to run twice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/manifest_ui.c` around lines 1702 - 1705, The ui_loop_iter function currently calls lv_task_handler() which is a legacy alias to lv_timer_handler() in LVGL 9.x causing duplicate handler execution; remove the lv_task_handler() call from ui_loop_iter and leave platform_task_run_pending() and lv_timer_handler() only so the timer/task handling runs once (update the ui_loop_iter function to drop the lv_task_handler() invocation).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@common/manifest_ui.c`:
- Around line 1642-1650: The bridge callbacks (ui_message_cb, ui_zone_name_cb,
ui_status_cb) are double-posting because they call manifest_ui_set_* (which
strdup + platform_task_post_to_ui) after already being invoked on the UI thread
by post_ui_message/post_ui_zone_name/post_ui_status; fix by making the callbacks
call a non-posting, UI-thread-only setter (create/used functions named
manifest_ui_apply_message, manifest_ui_apply_zone_name,
manifest_ui_apply_network_status or similar) that update state directly without
strdup/platform_task_post_to_ui, and update manifest implementation to add these
direct setters (or add a boolean parameter to manifest_ui_set_* to skip posting)
so that ui_set_message/ui_set_zone_name/ui_set_network_status still post when
called from non-UI threads but bridge_client.c callbacks call the new
non-posting variants.
---
Outside diff comments:
In `@common/bridge_client.c`:
- Around line 993-1002: In bridge_poll_thread, fix the inconsistent UI dispatch
by using the direct manifest_ui_set_* calls everywhere in this function instead
of mixing post_ui_* wrappers: replace occurrences of post_ui_message("Bridge:
Connected") and any post_ui_zone_name(...) calls with the equivalent
manifest_ui_set_network_status(...) / manifest_ui_set_zone_name(...) invocations
(or remove redundant double-posts), and ensure you still copy the zone name into
zone_name_copy under lock before calling
manifest_ui_set_zone_name(zone_name_copy) so thread-safety and single direct UI
updates (manifest_ui_set_network_status, manifest_ui_set_zone_name) are
consistent.
- Around line 166-191: The callbacks ui_status_cb, ui_message_cb, and
ui_zone_name_cb are double-posting to the UI thread: they run on the UI thread
already (via platform_task_post_to_ui) and then call post_ui_* wrappers which
post again and strdup strings, causing extra latency and allocations; replace
uses of post_ui_message/post_ui_zone_name/post_ui_status with direct calls to
manifest_ui_set_message/manifest_ui_set_zone_name/manifest_ui_set_status, remove
the post_ui_* and post_ui_*_copy helpers and their forward declarations, and
update the callbacks (ui_status_cb, ui_message_cb, ui_zone_name_cb) to call the
manifest_ui_set_* functions directly while preserving correct ownership/free
semantics (do not strdup before calling manifest functions; free the original
buffer only if the manifest functions duplicate it).
In `@common/manifest_ui.c`:
- Around line 1702-1705: The ui_loop_iter function currently calls
lv_task_handler() which is a legacy alias to lv_timer_handler() in LVGL 9.x
causing duplicate handler execution; remove the lv_task_handler() call from
ui_loop_iter and leave platform_task_run_pending() and lv_timer_handler() only
so the timer/task handling runs once (update the ui_loop_iter function to drop
the lv_task_handler() invocation).
|
Verify each finding against the current code and only fix it if needed. In |
- Remove double-posting: post_ui_* helpers did strdup+post, then callbacks called manifest_ui_set_* which did strdup+post again. Replace all post_ui_* calls with direct manifest_ui_set_* calls (which already handle thread-safe posting internally). - Remove redundant lv_task_handler() call (alias for lv_timer_handler in LVGL 9.x, was causing duplicate handler execution).
|
Fixed in bff1870:
|
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 (2)
common/manifest_ui.c (2)
1580-1593:⚠️ Potential issue | 🟡 MinorFallback path in
manifest_ui_hide_zone_pickerleaveszone_labelpermanently hidden.When
s_mgr.manifest.nav.default_screenis not found in the nav order, the function falls through toshow_screen(0)without restoring thezone_labelthatmanifest_ui_show_zone_pickerhid. This is a latent edge case (misconfigured manifest), but the fix is trivial.🛡️ Proposed fix
} - show_screen(0); + show_screen(0); + if (s_chrome.zone_label) + lv_obj_clear_flag(s_chrome.zone_label, LV_OBJ_FLAG_HIDDEN); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/manifest_ui.c` around lines 1580 - 1593, manifest_ui_hide_zone_picker can fall through to the fallback show_screen(0) without restoring s_chrome.zone_label; ensure the zone label is unhidden in all exit paths by calling lv_obj_clear_flag(s_chrome.zone_label, LV_OBJ_FLAG_HIDDEN) before or immediately after the fallback show_screen(0) (so both the matched-nav branch and the fallback restore the label); reference manifest_ui_hide_zone_picker, s_chrome.zone_label, show_screen, and LV_OBJ_FLAG_HIDDEN.
1404-1406:⚠️ Potential issue | 🟡 Minor
lv_timer_createreturn value not checked — potential null dereference.If
lv_timer_createfails (OOM),s_msg_timerisNULLand the subsequentlv_timer_set_repeat_count(NULL, 1)call dereferences it, crashing. Additionally, if the timer creation fails, the status bar message would never be auto-cleared.The codebase already handles this correctly elsewhere (e.g.,
battery_timercreation in the same file checks the return value before use).🛡️ Proposed fix
s_msg_timer = lv_timer_create(msg_timer_cb, 3000, NULL); + if (s_msg_timer) { lv_timer_set_repeat_count(s_msg_timer, 1); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/manifest_ui.c` around lines 1404 - 1406, lv_timer_create can return NULL on failure and s_msg_timer is dereferenced immediately; update the block that calls lv_timer_create(msg_timer_cb, 3000, NULL) to check the return value (s_msg_timer) before calling lv_timer_set_repeat_count(s_msg_timer, 1) and any other timer operations; if s_msg_timer is NULL, avoid dereferencing and ensure the status-bar auto-clear behavior still occurs (e.g., skip timer setup or implement a safe fallback path), referencing s_msg_timer, lv_timer_create, msg_timer_cb, and lv_timer_set_repeat_count to locate the code to change.
🧹 Nitpick comments (2)
common/bridge_client.c (2)
1135-1168: Dead code: zone-list arrays andline1_msg/line2_msgare never consumed after manifest migration.Two orphaned cleanup artifacts:
UI_INPUT_MENUbranch (lines 1135–1167):names[],ids[],selected,count, and theback_name/back_id/settings_name/settings_idstatics are all populated — including an unnecessarylock_state()/unlock_state()cycle — but none are passed tomanifest_ui_show_zone_picker(), which takes no arguments. The manifest's own list screen contains the zone data.Error paths (lines ~936–1014):
line1_msgandline2_msgare computed viasnprintfthroughout the error-handling branches but onlystatus_msgis forwarded tomanifest_ui_set_network_status().Both patterns are remnants from the old dual-path code that fed explicit arrays into the legacy zone picker. The entire dead computation can be removed.
♻️ Proposed fix for the UI_INPUT_MENU branch
if (event == UI_INPUT_MENU) { - const char *names[MAX_ZONES + 3]; /* +3 for Back, Settings, margin */ - const char *ids[MAX_ZONES + 3]; - static const char *back_name = "Back"; - static const char *back_id = ZONE_ID_BACK; - static const char *settings_name = "Settings"; - static const char *settings_id = ZONE_ID_SETTINGS; - int selected = 1; /* Default to first zone after Back */ - int count = 0; - - /* Add Back as first option */ - names[count] = back_name; - ids[count] = back_id; - count++; - - lock_state(); - if (s_state.zone_count > 0) { - for (int i = 0; i < s_state.zone_count && count < MAX_ZONES + 2; ++i) { - names[count] = s_state.zones[i].name; - ids[count] = s_state.zones[i].id; - if (strcmp(s_state.zones[i].id, s_state.cfg.zone_id) == 0) { - selected = count; - } - count++; - } - } - unlock_state(); - - /* Add Settings as last option */ - names[count] = settings_name; - ids[count] = settings_id; - count++; - manifest_ui_show_zone_picker(); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/bridge_client.c` around lines 1135 - 1168, The UI_INPUT_MENU branch contains dead setup code: remove the unused arrays/names/ids, the back_/settings_ statics, selected/count variables and the lock_state()/unlock_state() block that populates them (the code that builds names[]/ids[] and updates selected) since manifest_ui_show_zone_picker() takes no arguments; simply call manifest_ui_show_zone_picker() in that branch. Likewise, remove the unused snprintf computations that build line1_msg and line2_msg in the error-handling paths (they are never passed to manifest_ui_set_network_status()); only keep status_msg and calls that actually affect the manifest API. Ensure no remaining references to names/ids/line1_msg/line2_msg remain in common/bridge_client.c and adjust includes/variables if they become unused.
560-568: Duplicate null/error check — second block is dead code.Lines 565–568 are an exact copy of lines 560–563; none of
ret,resp, orresp_lenchange between the two checks. Remove the duplicate block.♻️ Proposed fix
if (ret != 0 || !resp || resp_len == 0) { platform_http_free(resp); return NULL; } - - if (ret != 0 || !resp || resp_len == 0) { - platform_http_free(resp); - return NULL; - } - manifest_t *m = malloc(sizeof(manifest_t));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/bridge_client.c` around lines 560 - 568, There is a duplicated error/null-check block that repeats "if (ret != 0 || !resp || resp_len == 0) { platform_http_free(resp); return NULL; }" — remove the second (redundant) occurrence so the function only performs the check once; locate the duplicate by searching for the conditional using variables ret, resp, resp_len and the call to platform_http_free, delete the repeated block and keep a single, well-placed check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@common/manifest_ui.c`:
- Around line 1580-1593: manifest_ui_hide_zone_picker can fall through to the
fallback show_screen(0) without restoring s_chrome.zone_label; ensure the zone
label is unhidden in all exit paths by calling
lv_obj_clear_flag(s_chrome.zone_label, LV_OBJ_FLAG_HIDDEN) before or immediately
after the fallback show_screen(0) (so both the matched-nav branch and the
fallback restore the label); reference manifest_ui_hide_zone_picker,
s_chrome.zone_label, show_screen, and LV_OBJ_FLAG_HIDDEN.
- Around line 1404-1406: lv_timer_create can return NULL on failure and
s_msg_timer is dereferenced immediately; update the block that calls
lv_timer_create(msg_timer_cb, 3000, NULL) to check the return value
(s_msg_timer) before calling lv_timer_set_repeat_count(s_msg_timer, 1) and any
other timer operations; if s_msg_timer is NULL, avoid dereferencing and ensure
the status-bar auto-clear behavior still occurs (e.g., skip timer setup or
implement a safe fallback path), referencing s_msg_timer, lv_timer_create,
msg_timer_cb, and lv_timer_set_repeat_count to locate the code to change.
---
Duplicate comments:
In `@common/manifest_ui.c`:
- Around line 1642-1650: Previous double-posting issue is resolved; verify that
the thin wrappers ui_set_message, ui_set_zone_name, and ui_set_network_status
simply forward to manifest_ui_set_message / manifest_ui_set_zone_name /
manifest_ui_set_network_status and are only used from non-UI threads
(captive_portal.c, display_sleep.c, main_idf.c), while bridge_client.c calls
manifest_ui_set_* directly from its poll thread; no code change required unless
a duplicate platform_task_post_to_ui is later introduced—if so, remove the
redundant post from either the wrapper (ui_set_*) or the callee to ensure a
single post-to-UI path.
---
Nitpick comments:
In `@common/bridge_client.c`:
- Around line 1135-1168: The UI_INPUT_MENU branch contains dead setup code:
remove the unused arrays/names/ids, the back_/settings_ statics, selected/count
variables and the lock_state()/unlock_state() block that populates them (the
code that builds names[]/ids[] and updates selected) since
manifest_ui_show_zone_picker() takes no arguments; simply call
manifest_ui_show_zone_picker() in that branch. Likewise, remove the unused
snprintf computations that build line1_msg and line2_msg in the error-handling
paths (they are never passed to manifest_ui_set_network_status()); only keep
status_msg and calls that actually affect the manifest API. Ensure no remaining
references to names/ids/line1_msg/line2_msg remain in common/bridge_client.c and
adjust includes/variables if they become unused.
- Around line 560-568: There is a duplicated error/null-check block that repeats
"if (ret != 0 || !resp || resp_len == 0) { platform_http_free(resp); return
NULL; }" — remove the second (redundant) occurrence so the function only
performs the check once; locate the duplicate by searching for the conditional
using variables ret, resp, resp_len and the call to platform_http_free, delete
the repeated block and keep a single, well-placed check.
|
Resolve conflicts |
- Fix manifest_ui_hide_zone_picker fallback: restore zone_label visibility when default_screen not found in nav order - Guard lv_timer_create return: check for NULL before calling lv_timer_set_repeat_count to prevent null dereference on OOM - Remove dead code in UI_INPUT_MENU branch: arrays/names/ids/selected/count were populated but never consumed by manifest_ui_show_zone_picker() - Remove dead line1_msg/line2_msg variables: formatted but never used, only status_msg is passed to manifest_ui_set_network_status() - Remove duplicate null/error check in fetch_manifest()
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 (2)
common/manifest_ui.c (1)
1570-1598:⚠️ Potential issue | 🟡 MinorInconsistent
s_chrome.zone_labelNULL guard across zone-picker show/hide paths.Three call sites with different behavior:
Location Guard? manifest_ui_show_zone_picker, line 1576 (lv_obj_add_flag)❌ None manifest_ui_hide_zone_pickerearly-return path, line 1590 (lv_obj_clear_flag)❌ None manifest_ui_hide_zone_pickerfallback path, lines 1596-1597✅ if (s_chrome.zone_label)
s_chrome.zone_labelis only set whenbuild_chromeexecutes successfully (i.e., whenlv_screen_active()returns non-NULL inmanifest_ui_init). In any degenerate boot scenario wheremanifest_ui_initreturns early, lines 1576 and 1590 will crash with a null dereference.🛡️ Proposed fix — add guards to the two unprotected sites
void manifest_ui_show_zone_picker(void) { for (int i = 0; i < s_mgr.manifest.nav.count; i++) { int idx = find_screen_index(s_mgr.manifest.nav.order[i]); if (idx >= 0 && s_mgr.manifest.screens[idx].type == SCREEN_TYPE_LIST) { show_screen(i); - lv_obj_add_flag(s_chrome.zone_label, LV_OBJ_FLAG_HIDDEN); + if (s_chrome.zone_label) + lv_obj_add_flag(s_chrome.zone_label, LV_OBJ_FLAG_HIDDEN); return; } } } void manifest_ui_hide_zone_picker(void) { int def = find_screen_index(s_mgr.manifest.nav.default_screen); if (def >= 0) { for (int i = 0; i < s_mgr.manifest.nav.count; i++) { if (find_screen_index(s_mgr.manifest.nav.order[i]) == def) { show_screen(i); - lv_obj_clear_flag(s_chrome.zone_label, LV_OBJ_FLAG_HIDDEN); + if (s_chrome.zone_label) + lv_obj_clear_flag(s_chrome.zone_label, LV_OBJ_FLAG_HIDDEN); return; } } } show_screen(0); if (s_chrome.zone_label) lv_obj_clear_flag(s_chrome.zone_label, LV_OBJ_FLAG_HIDDEN); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/manifest_ui.c` around lines 1570 - 1598, Add null checks for s_chrome.zone_label before calling LVGL flag functions to avoid dereferencing a NULL when build_chrome/manifest_ui_init didn’t set it: in manifest_ui_show_zone_picker wrap the lv_obj_add_flag(s_chrome.zone_label, LV_OBJ_FLAG_HIDDEN) call with an if (s_chrome.zone_label) guard, and in manifest_ui_hide_zone_picker wrap the early-return-path lv_obj_clear_flag(s_chrome.zone_label, LV_OBJ_FLAG_HIDDEN) call with the same guard; keep the existing guarded fallback path untouched.common/bridge_client.c (1)
599-607:⚠️ Potential issue | 🟠 MajorBlocking HTTP download on the UI thread — freezes LVGL for the duration of the artwork fetch.
ui_manifest_cbruns on the UI thread (posted viaplatform_task_post_to_ui). Callingmanifest_ui_set_artworkhere issues a synchronousplatform_http_get_imagerequest (100 KB RGB565 at ESP32 WiFi rates ≈ 150–800 ms) before returning. During that windowlv_timer_handler()is not ticked: arcs stop animating, touch events queue up unprocessed, and the display appears frozen.The artwork fetch should be performed in the bridge polling thread (or a dedicated task), with only the final LVGL widget update (
lv_image_set_src/lv_obj_clear_flag) posted to the UI thread.💡 Sketch of the fix
In
bridge_client.c— kick off artwork download in the polling thread (already off the UI thread):static void ui_manifest_cb(void *arg) { manifest_t *m = arg; if (!m) return; s_last_known_volume = m->fast.volume; s_last_known_volume_min = m->fast.volume_min; s_last_known_volume_max = m->fast.volume_max; s_last_known_volume_step = m->fast.volume_step; manifest_ui_update(m); - // Fetch artwork whenever screens are updated (SHA changed) - for (int i = 0; i < m->screen_count; i++) { - if (m->screens[i].type == SCREEN_TYPE_MEDIA) { - const char *url = m->screens[i].data.media.image_url; - if (url[0]) { - manifest_ui_set_artwork(url); - } - break; - } - } free(m); }In
bridge_poll_thread— afterpost_manifest_update, fetch artwork on the poll thread and post only the LVGL update:// After post_manifest_update(manifest): if (ok) { for (int i = 0; i < manifest->screen_count; i++) { if (manifest->screens[i].type == SCREEN_TYPE_MEDIA) { const char *url = manifest->screens[i].data.media.image_url; if (url[0]) manifest_ui_set_artwork(url); // HTTP on poll thread, LVGL update posted internally break; } } }
manifest_ui_set_artworkalready posts the finallv_image_set_srcstep to the UI thread viaplatform_task_post_to_ui(or can be made to do so); only theplatform_http_get_imagecall needs to move off the UI thread.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/bridge_client.c` around lines 599 - 607, The manifest artwork download is blocking the UI because ui_manifest_cb calls manifest_ui_set_artwork (which performs platform_http_get_image) on the UI thread; move the HTTP fetch off the UI thread by invoking platform_http_get_image from the bridge_poll_thread (or polling task) after post_manifest_update succeeds, and keep only the LVGL update (lv_image_set_src / lv_obj_clear_flag) posted back to the UI via platform_task_post_to_ui inside manifest_ui_set_artwork; update ui_manifest_cb to only detect the media URL and defer the fetch to bridge_poll_thread (or call a new helper from the poll thread) so platform_http_get_image is never run on the UI thread.
🧹 Nitpick comments (1)
common/bridge_client.c (1)
1253-1293:manifest_ui_set_network_statuscalled whiles_state_lockis held — fragile lock ordering.Lines 1275 and 1290 (and line 1080 in
bridge_client_handle_input) callmanifest_ui_set_network_statusfrom insidelock_state(). That function callsstrdup(heap allocation under a lock) followed byplatform_task_post_to_ui, which acquiress_ui_mutex.This creates a consistent lock-ordering chain
s_state_lock → s_ui_mutex. No current UI callback acquiress_state_lock, so there is no deadlock today. However:
- Heap allocation while holding a mutex inflates worst-case hold time.
- Any future UI callback that calls
lock_state()would immediately deadlock.Prefer releasing the lock before posting to the UI thread:
♻️ Proposed refactor for
bridge_client_set_network_readyvoid bridge_client_set_network_ready(bool ready) { s_network_ready = ready; + const char *net_status_msg = NULL; lock_state(); if (ready) { ... if (s_device_state != DEVICE_STATE_OPERATIONAL) { ... s_device_state = DEVICE_STATE_CONNECTED; - manifest_ui_set_network_status("Loading zones..."); + net_status_msg = "Loading zones..."; } ... } else { ... if (new_state == DEVICE_STATE_RECONNECTING) { - manifest_ui_set_network_status("Reconnecting..."); + net_status_msg = "Reconnecting..."; } } unlock_state(); + + if (net_status_msg) + manifest_ui_set_network_status(net_status_msg); }Apply the same pattern to the
lock_state()block inbridge_client_handle_input(line 1080).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/bridge_client.c` around lines 1253 - 1293, The code calls manifest_ui_set_network_status while holding s_state_lock (via lock_state()), causing heap allocation and cross-lock ordering with s_ui_mutex; fix bridge_client_set_network_ready (and the call site in bridge_client_handle_input) by preparing the desired status text while holding the state lock (e.g., choose an immutable const char* or copy a small enum/value), then call unlock_state() before invoking manifest_ui_set_network_status so the strdup/allocation and platform_task_post_to_ui happen without s_state_lock held, preserving lock ordering and minimizing time holding the mutex.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@common/bridge_client.c`:
- Around line 599-607: The manifest artwork download is blocking the UI because
ui_manifest_cb calls manifest_ui_set_artwork (which performs
platform_http_get_image) on the UI thread; move the HTTP fetch off the UI thread
by invoking platform_http_get_image from the bridge_poll_thread (or polling
task) after post_manifest_update succeeds, and keep only the LVGL update
(lv_image_set_src / lv_obj_clear_flag) posted back to the UI via
platform_task_post_to_ui inside manifest_ui_set_artwork; update ui_manifest_cb
to only detect the media URL and defer the fetch to bridge_poll_thread (or call
a new helper from the poll thread) so platform_http_get_image is never run on
the UI thread.
In `@common/manifest_ui.c`:
- Around line 1570-1598: Add null checks for s_chrome.zone_label before calling
LVGL flag functions to avoid dereferencing a NULL when
build_chrome/manifest_ui_init didn’t set it: in manifest_ui_show_zone_picker
wrap the lv_obj_add_flag(s_chrome.zone_label, LV_OBJ_FLAG_HIDDEN) call with an
if (s_chrome.zone_label) guard, and in manifest_ui_hide_zone_picker wrap the
early-return-path lv_obj_clear_flag(s_chrome.zone_label, LV_OBJ_FLAG_HIDDEN)
call with the same guard; keep the existing guarded fallback path untouched.
---
Nitpick comments:
In `@common/bridge_client.c`:
- Around line 1253-1293: The code calls manifest_ui_set_network_status while
holding s_state_lock (via lock_state()), causing heap allocation and cross-lock
ordering with s_ui_mutex; fix bridge_client_set_network_ready (and the call site
in bridge_client_handle_input) by preparing the desired status text while
holding the state lock (e.g., choose an immutable const char* or copy a small
enum/value), then call unlock_state() before invoking
manifest_ui_set_network_status so the strdup/allocation and
platform_task_post_to_ui happen without s_state_lock held, preserving lock
ordering and minimizing time holding the mutex.
# Conflicts: # common/app_main.c # common/bridge_client.c # common/ui.c
There was a problem hiding this comment.
♻️ Duplicate comments (1)
common/bridge_client.c (1)
1198-1278:⚠️ Potential issue | 🟠 MajorRemove the
post_ui_*callback layer in the manifest branch to prevent double-posting.The manifest path still calls
post_ui_status/post_ui_message/post_ui_zone_name, and those callbacks invokemanifest_ui_set_*(which already doesstrdup + platform_task_post_to_ui). This reintroduces double queueing and extra heap churn for every status/message update.Suggested fix direction
- post_ui_status(ok); + manifest_ui_set_status(ok); - post_ui_zone_name(label_copy); + manifest_ui_set_zone_name(label_copy); - post_ui_message("Loading zone..."); + manifest_ui_set_message("Loading zone...");Then remove
post_ui_*andui_*_cbhelpers that only forward intomanifest_ui_set_*.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/bridge_client.c` around lines 1198 - 1278, The manifest branch is still invoking the indirect post_ui_* callbacks which forward into manifest_ui_set_* causing double-queueing and redundant strdup; remove the post_ui_status/post_ui_message/post_ui_zone_name calls in the manifest path and call manifest_ui_set_status/manifest_ui_set_message/manifest_ui_set_zone_name directly (preserve existing ownership semantics and strdup behavior), and delete the now-unneeded post_ui_* helper functions and any ui_*_cb wrappers that merely forwarded into manifest_ui_set_* to eliminate the extra allocation and task post.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@common/bridge_client.c`:
- Around line 1198-1278: The manifest branch is still invoking the indirect
post_ui_* callbacks which forward into manifest_ui_set_* causing double-queueing
and redundant strdup; remove the
post_ui_status/post_ui_message/post_ui_zone_name calls in the manifest path and
call manifest_ui_set_status/manifest_ui_set_message/manifest_ui_set_zone_name
directly (preserve existing ownership semantics and strdup behavior), and delete
the now-unneeded post_ui_* helper functions and any ui_*_cb wrappers that merely
forwarded into manifest_ui_set_* to eliminate the extra allocation and task
post.
1 participant
Closes #132
Summary
Removes the legacy
ui.cimplementation (1647 lines) and allUSE_MANIFESTconditional compilation. The manifest-driven UI is now the only UI path.Changes
Deleted
common/ui.c— entire legacy UI implementationModified
common/bridge_client.c— Removed dispatch macros (UI_SET_STATUS, UI_UPDATE, etc.), now calls manifest_ui_* directly. Removed dead code: struct now_playing_state, fetch_now_playing(), ui_update_cb(), post_ui_update(), default_now_playing().common/manifest_ui.c— Removed #if USE_MANIFEST guard around shim section. Removed unused shims (ui_init, ui_set_status, ui_set_artwork, ui_dispatch_input, ui_handle_volume_rotation, zone picker shims, ui_test_pattern, ui_set_progress). Kept implementations for functions still called externally (ui_loop_iter, ui_set_controls_visible, OTA functions, battery, message/zone/network).common/manifest_ui.h— Updated comments to remove USE_MANIFEST/ui.c references.common/ui.h— Trimmed to shared types (ui_input_event_t, ui_input_cb_t) + functions still called from external files.common/app_main.c— Calls manifest_ui_* directly (no conditional).idf_app/main/main_idf.c— Calls manifest_ui_init() directly. Replaced ui_update() calls with ui_set_message().idf_app/main/platform_input_idf.c— Calls manifest_ui_* directly (no conditional).idf_app/main/CMakeLists.txt— Removed USE_MANIFEST option and conditional block.Verification
Note
Some documentation files (docs/) still reference ui.c — these should be updated separately.
Summary by CodeRabbit