feat: Bambu AMS tray dashboard on TFT display (#148)#149
Merged
Conversation
…ApplicationManager
📝 WalkthroughWalkthroughAdds a Bambu AMS tray dashboard feature: new tray data types, MQTT tray_update handling, TFT rendering, display integration, NVS persistence for cached dashboard state, a dashboard-revert timer on tag events, and a config toggle. Changes
Sequence Diagram(s)sequenceDiagram
actor MQTT as MQTT Broker
participant HAM as HomeAssistantManager
participant AppMgr as ApplicationManager
participant TFTMgr as TFTManager
participant TFT as TFT Display
MQTT->>HAM: tray_update command (JSON array)
HAM->>HAM: Parse JSON, extract trays & colors
HAM->>AppMgr: updateTrayDashboard(state)
AppMgr->>AppMgr: Persist state to NVS (non-test)
HAM->>AppMgr: Send TRAY_UPDATE message
AppMgr->>TFTMgr: showTrayDashboard(state)
TFTMgr->>TFT: Render via TFTDashboard
sequenceDiagram
participant Init as System Init
participant NVS as NVS Storage
participant AppMgr as ApplicationManager
participant LCD as Display
participant Timer as Revert Timer
Init->>AppMgr: begin()
AppMgr->>NVS: Load cached TrayDashboardState
alt cached data && bambu_dashboard enabled
AppMgr->>LCD: showTrayDashboard(cached_state)
else
AppMgr->>LCD: show "AMS Ready"
end
Note over Timer: Tag-scan event
AppMgr->>Timer: arm dashboardRevertAt (millis)
loop process messages
AppMgr->>AppMgr: if revert elapsed -> restore persisted tray state & show
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/WebServerManager.cpp (1)
764-783:⚠️ Potential issue | 🟡 MinorClear
bambu_dashboardwhen TFT is disabled.This handler persists the dashboard flag independently of
tft_enabled, so a directPOST /api/configcan save a configuration the device cannot render. Keep that invariant server-side instead of relying on the page behavior.Suggested fix
update.tft_enabled = doc["tft_enabled"] | (uint8_t)0; strncpy(update.tft_driver, doc["tft_driver"] | "st7789", sizeof(update.tft_driver) - 1); // TFT and LCD share GPIO 22/23 on WROOM — auto-disable LCD when TFT enabled if (update.tft_enabled && update.lcd_enabled) { update.lcd_enabled = 0; } @@ - update.bambu_dashboard = doc["bambu_dashboard"] | 0; + update.bambu_dashboard = update.tft_enabled + ? static_cast<uint8_t>(doc["bambu_dashboard"] | 0) + : static_cast<uint8_t>(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WebServerManager.cpp` around lines 764 - 783, The handler currently writes update.bambu_dashboard regardless of update.tft_enabled; add logic after parsing update.tft_enabled (and after the existing TFT/LCD mutual-disable block) to force update.bambu_dashboard = 0 when update.tft_enabled is false so the device never persists a dashboard flag it cannot render; locate the code that reads/writes update.tft_enabled and update.bambu_dashboard in WebServerManager.cpp and insert the conditional clearing there (use the existing update.* fields: update.tft_enabled and update.bambu_dashboard).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ApplicationManager.cpp`:
- Around line 1284-1286: Current code calls Preferences::putBytes every
TRAY_UPDATE which causes frequent flash writes; change this by adding a
debounce/compare strategy: keep a cached copy of the last-persisted
TrayDashboardState (e.g., lastSavedTrayDashboardState_) and a timestamp (e.g.,
lastTraySaveTs_), then in the TRAY_UPDATE handler (where trayDashboardState_ is
set) only call prefs.putBytes("tray_dash", &trayDashboardState_,
sizeof(TrayDashboardState)) when the new trayDashboardState_ differs from
lastSavedTrayDashboardState_ or when a configured minimum interval has elapsed
(e.g., save no more often than once every X seconds). Update
lastSavedTrayDashboardState_ and lastTraySaveTs_ when persisting and ensure
Preferences prefs.begin("spoolsense", false) is only called at save time; this
reduces writes by persisting on real changes or at a low cadence.
- Around line 161-175: The dashboard revert timing uses a direct >= comparison
with millis(), which breaks on rollover; replace the direct check "if (now >=
dashboardRevertAt_)" with a wrap-safe elapsed comparison like "if ((int32_t)(now
- dashboardRevertAt_) >= 0)" so the condition is correct across millis()
rollovers, and apply the same change to the other occurrences mentioned (the
checks around dashboardRevertAt_ at the other paths/locations and the
blank/generic tag paths) referencing dashboardRevertAt_, millis(),
display_->showTrayDashboard and trayDashboardState_ for locating the code.
- Around line 1282-1287: The persistence code in
ApplicationManager::handleTrayUpdate unconditionally uses Preferences which is
only available when not building NATIVE_TEST; wrap the entire Preferences usage
(the instantiation, begin/putBytes/end) in an `#ifndef` NATIVE_TEST / `#endif` block
like the read path does so native-test builds don't reference Preferences;
ensure you guard the block that writes trayDashboardState_ (the
prefs.begin("spoolsense", false); prefs.putBytes("tray_dash",
&trayDashboardState_, sizeof(TrayDashboardState)); prefs.end(); sequence) with
the same conditional used at the top of the file.
In `@src/ApplicationManager.h`:
- Around line 34-35: The queue currently enqueues only the TRAY_UPDATE token
while updateTrayDashboard() writes to trayDashboardState_, causing race/ordering
issues; modify AppMessage to include a TrayDashboardState payload (or
alternatively protect trayDashboardState_ with a mutex) and change
updateTrayDashboard() to enqueue an AppMessage containing the snapshot, and
update handleTrayUpdate() to consume that payload instead of reading
trayDashboardState_ directly (refer to TrayDashboardState, AppMessage,
updateTrayDashboard(), trayDashboardState_, TRAY_UPDATE, handleTrayUpdate()).
In `@src/ConfigurationManager.cpp`:
- Around line 252-255: Initialize _bambuDashboard from the DeviceConfig-derived
setting before applying NVS overrides: in the code path where
loadFromDeviceConfig() is used, seed _bambuDashboard with
cfg.peripherals.bambu_dashboard (or
DeviceConfig::cfg.peripherals.bambu_dashboard) prior to the
prefs.isKey(NVS_KEY_BAMBU_DASH) check so that first-boot defaults from
loadFromDeviceConfig() are respected and only overwritten when NVS contains an
explicit value via prefs.getBool(NVS_KEY_BAMBU_DASH, false).
In `@src/HomeAssistantManager.cpp`:
- Around line 931-936: The code currently calls
ApplicationManager::getInstance().updateTrayDashboard(...) then fire-and-forgets
ApplicationManager::getInstance().sendMessage(msg) and immediately publishes
success with publishCommandResponse(...); change this to check the return value
of sendMessage (which indicates enqueue success/backpressure) and only call
publishCommandResponse(command, true, ...) when sendMessage returns true; if
sendMessage fails, publishCommandResponse(command, false, /* optional error
payload */) so the caller sees the queue backpressure instead of a false
success. Reference updateTrayDashboard, sendMessage,
AppMessageType::TRAY_UPDATE, and publishCommandResponse in your change.
- Around line 890-896: The tray_update path is constrained by three buffer sizes
causing 6+ tray payloads to truncate and fail; update mqttCallback() to enlarge
its local payload buffer (replace char buf[384] with a larger size like 2048 to
avoid truncation), increase the MQTT client/PubSubClient buffer capacity to
match that larger payload (adjust the buffer size used when initializing or
configuring the MQTT client), and raise the StaticJsonDocument capacity used in
tray handling (change StaticJsonDocument<768> trayDoc to a larger capacity such
as StaticJsonDocument<2048>) so the deserializer can handle 6–16 tray JSON
payloads end-to-end (ensure all three sizes are consistent).
In `@src/TFTDashboard.cpp`:
- Around line 33-34: The label buffer is too small to hold tray IDs like
"T255"/"T256" plus the null terminator; change the fixed buffer used where label
and snprintf are in TFTDashboard.cpp (the variables label and the snprintf call
that formats with tray.tray_index/TrayData::tray_index) to a larger size (at
least 5 bytes) or use a small std::string/std::array to ensure space for "T###"
plus '\0', and keep using snprintf (or std::to_string + concatenation) so the
formatted tray id cannot be truncated; also optionally validate the tray index
range before formatting.
- Around line 62-101: The render function (TFTDashboard::render) uses hard-coded
240 and fixed center coordinates (120,120) which breaks layouts on non-240x240
displays; update TFTDashboard::render to derive widths/heights from the provided
sprite (use sprite->width() and sprite->height() for totalW/totalH), compute
cellW and cellH using totalW/totalH instead of 240, and center the "No Trays"
text using totalW/2 and totalH/2; ensure any placement logic that computes x/y
for renderCell and renderEmptyCell uses these derived totals so the grid scales
and centers correctly for any panel size.
---
Outside diff comments:
In `@src/WebServerManager.cpp`:
- Around line 764-783: The handler currently writes update.bambu_dashboard
regardless of update.tft_enabled; add logic after parsing update.tft_enabled
(and after the existing TFT/LCD mutual-disable block) to force
update.bambu_dashboard = 0 when update.tft_enabled is false so the device never
persists a dashboard flag it cannot render; locate the code that reads/writes
update.tft_enabled and update.bambu_dashboard in WebServerManager.cpp and insert
the conditional clearing there (use the existing update.* fields:
update.tft_enabled and update.bambu_dashboard).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: a854cf1a-a7ab-4cb4-aef9-1b557de8ed88
📒 Files selected for processing (15)
src/ApplicationManager.cppsrc/ApplicationManager.hsrc/ConfigHTML.hsrc/ConfigurationManager.cppsrc/ConfigurationManager.hsrc/DeviceConfig.cppsrc/DeviceConfig.hsrc/DisplayI.hsrc/HomeAssistantManager.cppsrc/TFTDashboard.cppsrc/TFTDashboard.hsrc/TFTManager.cppsrc/TFTManager.hsrc/TrayDashboardTypes.hsrc/WebServerManager.cpp
Comment on lines
+1284
to
+1286
| Preferences prefs; | ||
| prefs.begin("spoolsense", false); | ||
| prefs.putBytes("tray_dash", &trayDashboardState_, sizeof(TrayDashboardState)); |
There was a problem hiding this comment.
Debounce the tray-cache writes.
This writes the full dashboard blob on every TRAY_UPDATE. Because the payload includes tray weights, Home Assistant can drive this path many times during a print, turning a reboot cache into frequent flash writes. Persist only when the state actually changes and at a much lower cadence.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 1284-1284: variable 'prefs' is not initialized
(cppcoreguidelines-init-variables)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ApplicationManager.cpp` around lines 1284 - 1286, Current code calls
Preferences::putBytes every TRAY_UPDATE which causes frequent flash writes;
change this by adding a debounce/compare strategy: keep a cached copy of the
last-persisted TrayDashboardState (e.g., lastSavedTrayDashboardState_) and a
timestamp (e.g., lastTraySaveTs_), then in the TRAY_UPDATE handler (where
trayDashboardState_ is set) only call prefs.putBytes("tray_dash",
&trayDashboardState_, sizeof(TrayDashboardState)) when the new
trayDashboardState_ differs from lastSavedTrayDashboardState_ or when a
configured minimum interval has elapsed (e.g., save no more often than once
every X seconds). Update lastSavedTrayDashboardState_ and lastTraySaveTs_ when
persisting and ensure Preferences prefs.begin("spoolsense", false) is only
called at save time; this reduces writes by persisting on real changes or at a
low cadence.
Comment on lines
+34
to
35
| TRAY_UPDATE, | ||
| }; |
There was a problem hiding this comment.
Carry TrayDashboardState inside AppMessage.
The new flow writes trayDashboardState_ through updateTrayDashboard() and then enqueues only a TRAY_UPDATE token. That breaks the queue’s synchronization boundary: two queued updates can both render the last state written, and readers can observe a partially copied TrayDashboardState while another task is writing it. Put the snapshot on the message payload (or guard this member with a mutex) and let handleTrayUpdate() consume the queued data.
Also applies to: 185-186
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ApplicationManager.h` around lines 34 - 35, The queue currently enqueues
only the TRAY_UPDATE token while updateTrayDashboard() writes to
trayDashboardState_, causing race/ordering issues; modify AppMessage to include
a TrayDashboardState payload (or alternatively protect trayDashboardState_ with
a mutex) and change updateTrayDashboard() to enqueue an AppMessage containing
the snapshot, and update handleTrayUpdate() to consume that payload instead of
reading trayDashboardState_ directly (refer to TrayDashboardState, AppMessage,
updateTrayDashboard(), trayDashboardState_, TRAY_UPDATE, handleTrayUpdate()).
Comment on lines
+252
to
+255
| if (prefs.isKey(NVS_KEY_BAMBU_DASH)) { | ||
| _bambuDashboard = prefs.getBool(NVS_KEY_BAMBU_DASH, false); | ||
| anyOverride = true; | ||
| } |
There was a problem hiding this comment.
Seed _bambuDashboard from DeviceConfig before applying NVS overrides.
This override path only runs when "bambu_dash" already exists in NVS, but loadFromDeviceConfig() never initializes _bambuDashboard. On first boot, boards that enable the dashboard via cfg.peripherals.bambu_dashboard will still report it disabled until the setting is written once.
🔧 Suggested fix
void ConfigurationManager::loadFromDeviceConfig() {
const DeviceConfig& cfg = getDeviceConfig();
@@
_lcdEnabled = cfg.peripherals.lcd_enabled;
_ledEnabled = cfg.peripherals.status_led_enabled;
_keypadEnabled = cfg.peripherals.keypad_enabled;
+ _bambuDashboard = cfg.peripherals.bambu_dashboard;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ConfigurationManager.cpp` around lines 252 - 255, Initialize
_bambuDashboard from the DeviceConfig-derived setting before applying NVS
overrides: in the code path where loadFromDeviceConfig() is used, seed
_bambuDashboard with cfg.peripherals.bambu_dashboard (or
DeviceConfig::cfg.peripherals.bambu_dashboard) prior to the
prefs.isKey(NVS_KEY_BAMBU_DASH) check so that first-boot defaults from
loadFromDeviceConfig() are respected and only overwritten when NVS contains an
explicit value via prefs.getBool(NVS_KEY_BAMBU_DASH, false).
Comment on lines
+931
to
+936
| ApplicationManager::getInstance().updateTrayDashboard(state); | ||
| AppMessage msg = {}; | ||
| msg.type = AppMessageType::TRAY_UPDATE; | ||
| ApplicationManager::getInstance().sendMessage(msg); | ||
|
|
||
| publishCommandResponse(command, true, nullptr); |
There was a problem hiding this comment.
Propagate app-queue backpressure before returning success.
src/ApplicationManager.cpp only caches the state in updateTrayDashboard(). The render/persist work happens later when AppMessageType::TRAY_UPDATE is handled, so ignoring sendMessage() failures can silently drop the visible update while still publishing success.
Suggested fix
ApplicationManager::getInstance().updateTrayDashboard(state);
AppMessage msg = {};
msg.type = AppMessageType::TRAY_UPDATE;
- ApplicationManager::getInstance().sendMessage(msg);
+ if (!ApplicationManager::getInstance().sendMessage(msg, 50)) {
+ publishCommandResponse(command, false, "app_queue_full");
+ return;
+ }
publishCommandResponse(command, true, nullptr);
return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/HomeAssistantManager.cpp` around lines 931 - 936, The code currently
calls ApplicationManager::getInstance().updateTrayDashboard(...) then
fire-and-forgets ApplicationManager::getInstance().sendMessage(msg) and
immediately publishes success with publishCommandResponse(...); change this to
check the return value of sendMessage (which indicates enqueue
success/backpressure) and only call publishCommandResponse(command, true, ...)
when sendMessage returns true; if sendMessage fails,
publishCommandResponse(command, false, /* optional error payload */) so the
caller sees the queue backpressure instead of a false success. Reference
updateTrayDashboard, sendMessage, AppMessageType::TRAY_UPDATE, and
publishCommandResponse in your change.
Comment on lines
+62
to
+101
| void TFTDashboard::render(LGFX_Sprite* sprite, const TrayDashboardState& state) { | ||
| sprite->fillScreen(0x000000); | ||
|
|
||
| if (state.tray_count == 0) { | ||
| sprite->setTextColor(COLOR_EMPTY_TEXT); | ||
| sprite->setTextDatum(MC_DATUM); | ||
| sprite->setTextSize(2); | ||
| sprite->drawString("No Trays", 120, 120); | ||
| sprite->pushSprite(0, 0); | ||
| return; | ||
| } | ||
|
|
||
| // Determine grid dimensions | ||
| uint8_t cols, rows; | ||
| bool small; | ||
| if (state.tray_count <= 4) { | ||
| cols = 2; rows = 2; small = false; | ||
| } else if (state.tray_count <= 8) { | ||
| cols = 2; rows = 4; small = true; | ||
| } else { | ||
| cols = 4; rows = 4; small = true; | ||
| } | ||
|
|
||
| int cellW = (240 - (cols + 1) * CELL_GAP) / cols; | ||
| int cellH = (240 - (rows + 1) * CELL_GAP) / rows; | ||
|
|
||
| for (uint8_t i = 0; i < rows * cols; i++) { | ||
| uint8_t col = i % cols; | ||
| uint8_t row = i / cols; | ||
| int x = CELL_GAP + col * (cellW + CELL_GAP); | ||
| int y = CELL_GAP + row * (cellH + CELL_GAP); | ||
|
|
||
| if (i < state.tray_count && state.trays[i].populated) { | ||
| renderCell(sprite, x, y, cellW, cellH, state.trays[i], small); | ||
| } else { | ||
| renderEmptyCell(sprite, x, y, cellW, cellH, small); | ||
| } | ||
| } | ||
|
|
||
| sprite->pushSprite(0, 0); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Derive the layout from the sprite size instead of hard-coding 240.
This renderer centers "No Trays" at (120, 120) and computes every cell from 240, so it will be clipped or off-center on any supported panel that is not 240x240. Using sprite->width() / sprite->height() here keeps the dashboard aligned with whatever driver TFTManager instantiated.
♻️ Suggested refactor
void TFTDashboard::render(LGFX_Sprite* sprite, const TrayDashboardState& state) {
- sprite->fillScreen(0x000000);
+ sprite->fillScreen(0x000000);
+ const int screenW = sprite->width();
+ const int screenH = sprite->height();
if (state.tray_count == 0) {
sprite->setTextColor(COLOR_EMPTY_TEXT);
sprite->setTextDatum(MC_DATUM);
sprite->setTextSize(2);
- sprite->drawString("No Trays", 120, 120);
+ sprite->drawString("No Trays", screenW / 2, screenH / 2);
sprite->pushSprite(0, 0);
return;
}
@@
- int cellW = (240 - (cols + 1) * CELL_GAP) / cols;
- int cellH = (240 - (rows + 1) * CELL_GAP) / rows;
+ int cellW = (screenW - (cols + 1) * CELL_GAP) / cols;
+ int cellH = (screenH - (rows + 1) * CELL_GAP) / rows;📝 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
| void TFTDashboard::render(LGFX_Sprite* sprite, const TrayDashboardState& state) { | |
| sprite->fillScreen(0x000000); | |
| if (state.tray_count == 0) { | |
| sprite->setTextColor(COLOR_EMPTY_TEXT); | |
| sprite->setTextDatum(MC_DATUM); | |
| sprite->setTextSize(2); | |
| sprite->drawString("No Trays", 120, 120); | |
| sprite->pushSprite(0, 0); | |
| return; | |
| } | |
| // Determine grid dimensions | |
| uint8_t cols, rows; | |
| bool small; | |
| if (state.tray_count <= 4) { | |
| cols = 2; rows = 2; small = false; | |
| } else if (state.tray_count <= 8) { | |
| cols = 2; rows = 4; small = true; | |
| } else { | |
| cols = 4; rows = 4; small = true; | |
| } | |
| int cellW = (240 - (cols + 1) * CELL_GAP) / cols; | |
| int cellH = (240 - (rows + 1) * CELL_GAP) / rows; | |
| for (uint8_t i = 0; i < rows * cols; i++) { | |
| uint8_t col = i % cols; | |
| uint8_t row = i / cols; | |
| int x = CELL_GAP + col * (cellW + CELL_GAP); | |
| int y = CELL_GAP + row * (cellH + CELL_GAP); | |
| if (i < state.tray_count && state.trays[i].populated) { | |
| renderCell(sprite, x, y, cellW, cellH, state.trays[i], small); | |
| } else { | |
| renderEmptyCell(sprite, x, y, cellW, cellH, small); | |
| } | |
| } | |
| sprite->pushSprite(0, 0); | |
| void TFTDashboard::render(LGFX_Sprite* sprite, const TrayDashboardState& state) { | |
| sprite->fillScreen(0x000000); | |
| const int screenW = sprite->width(); | |
| const int screenH = sprite->height(); | |
| if (state.tray_count == 0) { | |
| sprite->setTextColor(COLOR_EMPTY_TEXT); | |
| sprite->setTextDatum(MC_DATUM); | |
| sprite->setTextSize(2); | |
| sprite->drawString("No Trays", screenW / 2, screenH / 2); | |
| sprite->pushSprite(0, 0); | |
| return; | |
| } | |
| // Determine grid dimensions | |
| uint8_t cols, rows; | |
| bool small; | |
| if (state.tray_count <= 4) { | |
| cols = 2; rows = 2; small = false; | |
| } else if (state.tray_count <= 8) { | |
| cols = 2; rows = 4; small = true; | |
| } else { | |
| cols = 4; rows = 4; small = true; | |
| } | |
| int cellW = (screenW - (cols + 1) * CELL_GAP) / cols; | |
| int cellH = (screenH - (rows + 1) * CELL_GAP) / rows; | |
| for (uint8_t i = 0; i < rows * cols; i++) { | |
| uint8_t col = i % cols; | |
| uint8_t row = i / cols; | |
| int x = CELL_GAP + col * (cellW + CELL_GAP); | |
| int y = CELL_GAP + row * (cellH + CELL_GAP); | |
| if (i < state.tray_count && state.trays[i].populated) { | |
| renderCell(sprite, x, y, cellW, cellH, state.trays[i], small); | |
| } else { | |
| renderEmptyCell(sprite, x, y, cellW, cellH, small); | |
| } | |
| } | |
| sprite->pushSprite(0, 0); | |
| } |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 62-62: method 'render' can be made static
(readability-convert-member-functions-to-static)
[warning] 75-75: multiple declarations in a single statement reduces readability
(readability-isolate-declaration)
[warning] 75-75: variable 'cols' is not initialized
(cppcoreguidelines-init-variables)
[warning] 75-75: variable 'rows' is not initialized
(cppcoreguidelines-init-variables)
[warning] 76-76: variable 'small' is not initialized
(cppcoreguidelines-init-variables)
[warning] 91-91: variable name 'x' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 92-92: variable name 'y' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 94-94: if with identical then and else branches
(bugprone-branch-clone)
[note] 96-96: else branch starts here
(clang)
🪛 Cppcheck (2.20.0)
[style] 62-62: The function 'render' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TFTDashboard.cpp` around lines 62 - 101, The render function
(TFTDashboard::render) uses hard-coded 240 and fixed center coordinates
(120,120) which breaks layouts on non-240x240 displays; update
TFTDashboard::render to derive widths/heights from the provided sprite (use
sprite->width() and sprite->height() for totalW/totalH), compute cellW and cellH
using totalW/totalH instead of 240, and center the "No Trays" text using
totalW/2 and totalH/2; ensure any placement logic that computes x/y for
renderCell and renderEmptyCell uses these derived totals so the grid scales and
centers correctly for any panel size.
- Guard handleTrayUpdate NVS writes with #ifndef NATIVE_TEST - Make dashboard revert timer millis() wrap-safe (store start, check elapsed) - Increase MQTT client buffer to 2048 and callback buffer to 1536 for 16-tray payloads - Increase StaticJsonDocument to 2048 for full tray array parsing - Widen tray label buffer from 4 to 6 chars for T10+ indices
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/HomeAssistantManager.cpp`:
- Around line 919-925: The color parsing code accepts non-hex strings (e.g.,
"GGGGGG") because it only checks length then calls strtoul; fix by validating
the 6-character string contains only [0-9a-fA-F] (e.g., using strspn or a small
loop) or by using strtoul with an endptr and verifying endptr points at the
string terminator before using the result; if validation fails, set a safe
default (e.g., 0x000000) or skip assigning to tray.color to avoid silently
treating invalid input as black. Ensure you update the logic around colorHex /
strtoul / tray.color to perform this validation and fallback.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 37e3f9f2-ccca-49a2-b427-5e030bc3ed91
📒 Files selected for processing (3)
src/ApplicationManager.cppsrc/HomeAssistantManager.cppsrc/TFTDashboard.cpp
Comment on lines
+919
to
+925
| const char* colorHex = obj["color"] | "333333"; | ||
| if (strlen(colorHex) == 6) { | ||
| uint32_t c = strtoul(colorHex, nullptr, 16); | ||
| tray.color[0] = (c >> 16) & 0xFF; | ||
| tray.color[1] = (c >> 8) & 0xFF; | ||
| tray.color[2] = c & 0xFF; | ||
| } |
There was a problem hiding this comment.
Color hex parsing silently accepts invalid input.
If colorHex contains non-hex characters (e.g., "GGGGGG"), strtoul will return 0 or a partial parse without error indication. Consider validating the hex string or documenting that invalid colors default to black.
🛡️ Optional validation
const char* colorHex = obj["color"] | "333333";
- if (strlen(colorHex) == 6) {
+ if (strlen(colorHex) == 6 &&
+ strspn(colorHex, "0123456789ABCDEFabcdef") == 6) {
uint32_t c = strtoul(colorHex, nullptr, 16);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/HomeAssistantManager.cpp` around lines 919 - 925, The color parsing code
accepts non-hex strings (e.g., "GGGGGG") because it only checks length then
calls strtoul; fix by validating the 6-character string contains only
[0-9a-fA-F] (e.g., using strspn or a small loop) or by using strtoul with an
endptr and verifying endptr points at the string terminator before using the
result; if validation fails, set a safe default (e.g., 0x000000) or skip
assigning to tray.color to avoid silently treating invalid input as black.
Ensure you update the logic around colorHex / strtoul / tray.color to perform
this validation and fallback.
7 tasks
11 tasks
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
1 participant
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.
Summary
cmd/tray_updateMQTT command with flat JSON array of tray statesDesign
DisplayI::showTrayDashboard()with default empty body (LCD gets no-op automatically)TFTDashboardhelper class isolates grid rendering from TFTManagerTRAY_UPDATEsignal message through existing AppMessage queueNew Files
src/TrayDashboardTypes.hsrc/TFTDashboard.h/.cppTest plan
cmd/tray_updatevia MQTT with 4 trays — verify 2x2 grid renders with correct colorsSummary by CodeRabbit
New Features
Behavior
Configuration