feat: TFT display support (ST7789 240x240) with DisplayI interface#52
Merged
Conversation
Add TFTManager with FreeRTOS task, full-screen 8-bit sprite for flicker-free rendering, spool graphic with filament color fill, weight bar, tag type icons, breathing animation for low spool, and screen timeout. WROOM pin mapping on VSPI (pins 22/23 freed from LCD). Mutually exclusive with LCD I2C on WROOM. New build target: esp32dev_tft. Regular builds unaffected.
Remove ENABLE_TFT compile-time guards — LovyanGFX always compiled in. TFT enabled via tft_on NVS key and web config toggle. Mutually exclusive with LCD at runtime (shared GPIO 22/23 on WROOM). 8-bit color depth sprite (57.6KB) instead of 16-bit (115KB) to preserve heap headroom. Both esp32dev and esp32s3zero compile clean.
…nt it ApplicationManager now holds a DisplayI* instead of LCDManager*. All 48 display calls go through the interface (showText, showText4, setScreenTimeoutMs). Either display can be swapped at runtime via NVS. TFT toggle added to web config page. Both targets compile clean.
PN5180 library now uses dedicated SPIClass(HSPI) instead of the global SPI object (VSPI). TFT stays on VSPI with GPIO 22/23. This prevents SPI bus contention that caused NFC read failures when both peripherals were on the same bus. DC pin moved from GPIO 0 (boot strapping) to GPIO 4 (freed from LED).
Add showSpool() to DisplayI — LCD renders text, TFT renders color spool graphic with weight bar. Spoolman synced handler re-shows spool data instead of overwriting with text. TFT stays on spool display after tag removal instead of flipping to status screen. Fix showText/showText4 to use status renderer not error renderer. Add 100ms delay + setBrightness in TFT begin() for cold boot. Hardware tested: OpenPrintTag, TigerTag, OpenTag3D, NFC+ all working.
TFT shows big tool number for keypad entry and checkmark/X graphic for write results. LCD shows text equivalents. Wired keypad digit display through DisplayI instead of showText4.
…_hex parsing
The JSON streaming parser consumed end_object tokens from nested
objects like vendor.extra:{}, losing track of nesting depth and
missing fields that came after (color_hex, density, diameter).
Now skips unknown nested objects/arrays entirely. Also removes
debug print and prevents smart tag display overwrite on Spoolman sync.
…d objects
- NFC+ page: single Extruder/Bed Temp fields (averaged from material DB min/max)
- register-uid handler: writes settings_extruder_temp and settings_bed_temp to Spoolman filament
- Spoolman JSON parser: skip unknown nested objects (extra:{}) that broke color_hex parsing after vendor object
- Smart tags: don't overwrite spool display on Spoolman sync (tag already has correct data)
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR adds ST7789 240x240 TFT display support alongside the existing LCD display, with mutual exclusion. A new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Main as main.cpp
participant Config as ConfigurationManager
participant AppMgr as ApplicationManager
participant TFT as TFTManager
participant LCD as LCDManager
participant PN5180 as PN5180
User->>Main: Power on
Main->>Config: Load config (isTftEnabled?)
alt TFT Enabled
Main->>TFT: begin()
Main->>TFT: startTask()
Main->>TFT: showBoot(version)
Main->>AppMgr: begin(DisplayI* = &tftManager)
AppMgr->>TFT: showText/showSpool via DisplayI
else LCD Enabled
Main->>LCD: begin()
Main->>AppMgr: begin(DisplayI* = &lcdManager)
AppMgr->>LCD: showText/showSpool via DisplayI
end
Main->>Main: initWiFi()
alt TFT Path
Main->>TFT: showWifiConnecting()
Main->>TFT: showWifiConnected(ip)
else LCD Path
Main->>LCD: updateScreen(...)
end
Main->>PN5180: Initialize with HSPI
Main->>TFT: showReady()
User->>PN5180: Scan tag
PN5180->>AppMgr: Tag detected
AppMgr->>AppMgr: handleSpoolDetected()
AppMgr->>TFT: showSpool(spool_data)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
platformio.ini (1)
29-43:⚠️ Potential issue | 🟠 MajorAdd the missing
esp32dev_tftenvironment.This file still only declares
esp32devandesp32s3zero, sopio run -e esp32dev_tftwill fail even though the PR describes that target as supported.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platformio.ini` around lines 29 - 43, Add the missing PlatformIO environment named esp32dev_tft by copying or creating an [env:esp32dev_tft] block similar to [env:esp32dev] (or the esp32s3zero block if specific flags are needed) so pio run -e esp32dev_tft succeeds; ensure it sets board = esp32dev (or the correct board identifier), includes any required board_build.* settings and the same build_flags/defines used by esp32dev_tft in the PR (refer to the existing env blocks and the symbol esp32dev_tft to place the new block consistently).src/main.cpp (2)
233-239:⚠️ Potential issue | 🟡 MinorNFC failure message not shown on TFT.
When TFT is enabled, NFC initialization failure only updates LCD. The TFT path should also display this critical error:
if (!NFCManager::getInstance().begin()) { Serial.println("NFCManager init failed - halting"); - if (config.isLcdEnabled()) { + if (config.isTftEnabled()) { + tftManager.showError("NFC FAILED"); + } else if (config.isLcdEnabled()) { lcdManager.updateScreen("NFC FAILED", ""); } while (1) { delay(1000); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.cpp` around lines 233 - 239, The NFC init failure only updates the LCD; add the TFT path so the error is shown on TFT as well by checking config.isTftEnabled() and calling the TFT manager display method before halting. In the block that handles if (!NFCManager::getInstance().begin()) add a conditional similar to the LCD one that invokes tftManager (or the project's TFT display API) to show "NFC FAILED" (e.g., tftManager.updateScreen or appropriate show/error method) so both lcdManager.updateScreen and the TFT call run when their respective config.isLcdEnabled() / config.isTftEnabled() flags are set, then proceed to the existing infinite delay.
118-125:⚠️ Potential issue | 🟡 MinorNTP failure message not shown on TFT.
When TFT is enabled, NTP failure silently logs to serial but doesn't update the display. Consider adding TFT handling for consistency:
if (!getLocalTime(&timeinfo)) { Serial.println("Failed to obtain time"); - if (config.isLcdEnabled()) { + if (config.isTftEnabled()) { + tftManager.showError("NTP FAILED"); + } else if (config.isLcdEnabled()) { lcdManager.updateScreen("NTP FAILED", ""); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.cpp` around lines 118 - 125, The NTP failure branch only updates the serial log and the LCD; add TFT handling so the failure is shown on the TFT as well: inside the if (!getLocalTime(&timeinfo)) block (the same place that calls lcdManager.updateScreen("NTP FAILED", "")), check the TFT enable flag (e.g., config.isTftEnabled()) and call the TFT display API (e.g., tftManager.updateScreen("NTP FAILED") or tftManager.drawText(...) depending on your TFT API) to render the error, ensuring you guard the call with the config check and that tftManager is initialized before use.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Line 32: Update the phrase in CLAUDE.md to hyphenate the compound adjective:
change "Spoolman style APIs" to "Spoolman-style APIs" so the compound adjective
is grammatically correct (look for the line containing "Spool Sync:
ApplicationManager triggers sync -> SpoolmanManager queues request ->
SpoolmanManager task -> HTTP requests to SpoolSense / Spoolman style APIs.").
- Line 1: Add single blank lines before and after each top-level heading so the
file complies with MD022; specifically insert a blank line above and below the
"# Overview:" heading and do the same for the other headings called out (the
headings at the positions noted: the ones around lines showing headings at 8,
20, 28, 35, 144, and 151) so each heading is separated by an empty line above
and below.
- Line 6: Update the sentence that currently reads "All config is compile-time
via UserConfig.h" to clarify that while most configuration comes from
UserConfig.h at compile time, several features are configurable at runtime (for
example the TFT display via the NVS key 'tft_on' and the web UI toggle), and
mention other runtime-configurable items such as LCD and keypad; reference
UserConfig.h, the NVS key 'tft_on', and the web UI toggle so readers can locate
the runtime controls.
In `@lib/PN5180/PN5180.cpp`:
- Around line 116-118: The wrapper that begins/ends SPI transactions (calls
pn5180_spi.beginTransaction, transceiveCommand(buf, 6),
pn5180_spi.endTransaction()) should propagate the boolean result of
transceiveCommand instead of always returning true; modify the wrapper function
(and the other similar transaction wrappers in PN5180.cpp) to capture the return
value of transceiveCommand(...) and return that value (ensuring endTransaction()
still runs), so BUSY/SPI timeouts are reported correctly.
In `@src/ApplicationManager.cpp`:
- Around line 356-359: The tag format mapping omits the "Bambu" case causing
tagType to fall back to 0; update the conditional block that checks s.tag_format
(the strcmp chain around ApplicationManager::... lines) to include an else if
(strcmp(s.tag_format, "Bambu") == 0) that sets spool.tagType = 4 so it matches
the DisplaySpoolData/DisplayI.h documentation; keep the existing
OpenPrintTag/TigerTag/OpenTag3D branches and default to 0 for unknown formats.
In `@src/ConfigurationManager.cpp`:
- Around line 196-199: loadFromDeviceConfig() must initialize _tftEnabled so the
NVS override has a sensible first-boot default; currently tft only gets set when
NVS key NVS_KEY_TFT_ON exists. Update loadFromDeviceConfig() (or the
device-config parsing path) to set _tftEnabled from the device/config
compile-time default (or the existing deviceConfig.tft/default field if
present), and keep the NVS read in the block using
prefs.getUChar(NVS_KEY_TFT_ON, _tftEnabled ? 1 : 0) to override it; ensure the
unique symbols touched are loadFromDeviceConfig(), _tftEnabled and
NVS_KEY_TFT_ON.
In `@src/TFTConfig.h`:
- Line 14: Update the inline comment that currently reads "WROOM — VSPI (bus
3): pins 18/23 freed from LCD when ENABLE_TFT replaces LCD" to the correct pin
numbers for the WROOM TFT: change 18 to 22 so it reads "pins 22/23", and note
that these correspond to LCD_SCL/LCD_SDA; the affected comment is the one
referencing ENABLE_TFT and VSPI (bus 3) in TFTConfig.h.
In `@src/TFTManager.cpp`:
- Around line 627-632: The helper function dimColor is defined but never used;
either remove the unused function dimColor(uint32_t, uint8_t) from
TFTManager.cpp to avoid dead code, or add a brief comment above dimColor
explaining its intended future use (e.g., kept for per-pixel brightness
adjustments vs. current setBrightness-based breathing animation) so reviewers
know it’s intentionally retained; make the change by editing the dimColor
declaration/definition accordingly.
- Around line 127-139: TFTManager::begin currently assumes resource allocations
always succeed; check the return values of _sprite.createSprite(...) and
xQueueCreate(...) and handle failures: after calling _sprite.createSprite(width,
height) verify it returned true/valid and if not log an error and set a flag
(e.g., _spriteAvailable = false) to skip sprite rendering, and after
xQueueCreate(4, sizeof(TFTMessage)) verify the returned handle is non-NULL and
if NULL log an error and set _messageQueue = NULL/_queueAvailable = false (or
abort initialization) so subsequent xQueueSend and sprite calls first check
these flags/handles and avoid dereferencing NULL; ensure _lastActivityMs is
still initialized and add any cleanup or safe-fail behavior so the rest of
TFTManager methods gracefully handle missing sprite/queue resources.
- Around line 298-306: Read the breathing state and related fields under the
same critical section used when they are modified: acquire the existing
mutex/critical region (the one used by setScreenTimeoutMs and other queue
handlers), copy _isBreathing, _breathBrightness, _breathDirection and
_breathColor into local variables, then release the lock and perform the
breathing math and _tft.setBrightness() using the locals; reference the members
_isBreathing, _breathBrightness, _breathDirection, _breathColor and the modifier
method setScreenTimeoutMs to locate the protecting synchronization primitive to
reuse for this read-copy-release pattern.
In `@src/TFTManager.h`:
- Around line 22-28: Replace the TAG_TYPE_* preprocessor macros with a scoped
typed enum (e.g., enum class TagType : int { Unknown = 0, OpenPrintTag = 1,
TigerTag = 2, OpenTag3D = 3, Bambu = 4, NfcPlain = 5 }) in TFTManager.h; update
any variables, function parameters, return types and switch/case logic that
currently use those macros or plain ints to use TagType (or cast where interop
is required) and adjust comparisons to use TagType::... to gain type safety and
avoid macro conflicts (search for uses of TAG_TYPE_UNKNOWN,
TAG_TYPE_OPENPRINTTAG, TAG_TYPE_TIGERTAG, TAG_TYPE_OPENTAG3D, TAG_TYPE_BAMBU,
TAG_TYPE_NFC_PLAIN and replace accordingly).
In `@src/WebServerManager.cpp`:
- Around line 555-558: After reading the JSON into update (the fields
update.lcd_enabled and update.tft_enabled), add a validation step that rejects
the request if both update.lcd_enabled and update.tft_enabled are true; return
an HTTP 4xx error (with a short message like "LCD and TFT cannot be enabled
simultaneously") and do not call the config persistence/commit routine (the
function that writes the updated config or calls save/persist). Place this check
immediately after the lines that set update.lcd_enabled/update.tft_enabled and
before any code that applies or saves the configuration so the impossible
hardware state is never persisted.
---
Outside diff comments:
In `@platformio.ini`:
- Around line 29-43: Add the missing PlatformIO environment named esp32dev_tft
by copying or creating an [env:esp32dev_tft] block similar to [env:esp32dev] (or
the esp32s3zero block if specific flags are needed) so pio run -e esp32dev_tft
succeeds; ensure it sets board = esp32dev (or the correct board identifier),
includes any required board_build.* settings and the same build_flags/defines
used by esp32dev_tft in the PR (refer to the existing env blocks and the symbol
esp32dev_tft to place the new block consistently).
In `@src/main.cpp`:
- Around line 233-239: The NFC init failure only updates the LCD; add the TFT
path so the error is shown on TFT as well by checking config.isTftEnabled() and
calling the TFT manager display method before halting. In the block that handles
if (!NFCManager::getInstance().begin()) add a conditional similar to the LCD one
that invokes tftManager (or the project's TFT display API) to show "NFC FAILED"
(e.g., tftManager.updateScreen or appropriate show/error method) so both
lcdManager.updateScreen and the TFT call run when their respective
config.isLcdEnabled() / config.isTftEnabled() flags are set, then proceed to the
existing infinite delay.
- Around line 118-125: The NTP failure branch only updates the serial log and
the LCD; add TFT handling so the failure is shown on the TFT as well: inside the
if (!getLocalTime(&timeinfo)) block (the same place that calls
lcdManager.updateScreen("NTP FAILED", "")), check the TFT enable flag (e.g.,
config.isTftEnabled()) and call the TFT display API (e.g.,
tftManager.updateScreen("NTP FAILED") or tftManager.drawText(...) depending on
your TFT API) to render the error, ensuring you guard the call with the config
check and that tftManager is initialized before use.
🪄 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: e8213ab8-7e30-438a-ae9a-1449736ed28f
📒 Files selected for processing (19)
CLAUDE.mdinclude/BoardPins.hlib/PN5180/PN5180.cppplatformio.inisrc/ApplicationManager.cppsrc/ApplicationManager.hsrc/ConfigHTML.hsrc/ConfigurationManager.cppsrc/ConfigurationManager.hsrc/DisplayI.hsrc/LCDManager.cppsrc/LCDManager.hsrc/SpoolmanManager.cppsrc/TFTConfig.hsrc/TFTManager.cppsrc/TFTManager.hsrc/UIDRegistrationHTML.hsrc/WebServerManager.cppsrc/main.cpp
| @@ -0,0 +1,167 @@ | |||
| # Overview: | |||
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding blank lines around headings for markdown compliance.
The markdown linter flagged that headings should be surrounded by blank lines (MD022). While this doesn't affect functionality, adding blank lines would improve readability and markdown standard compliance.
Also applies to: 8-8, 20-20, 28-28, 35-35, 144-144, 151-151
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 1-1: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` at line 1, Add single blank lines before and after each top-level
heading so the file complies with MD022; specifically insert a blank line above
and below the "# Overview:" heading and do the same for the other headings
called out (the headings at the positions noted: the ones around lines showing
headings at 8, 20, 28, 35, 144, and 151) so each heading is separated by an
empty line above and below.
| main.cpp: Initializes all managers, starts FreeRTOS tasks | ||
| ApplicationManager: Central state machine + message bus, receives events (print start, spool scan, etc.) via queue and coordinates responses. | ||
| NFC Stack: NFCManager -> Hardware NFC adapter (PN5180 today) -> tag protocol handler -> openprinttag_lib (for OpenPrintTag) or generic tag handler | ||
| Spool Sync: ApplicationManager triggers sync -> SpoolmanManager queues request -> SpoolmanManager task -> HTTP requests to SpoolSense / Spoolman style APIs. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional: hyphenate compound adjective.
The phrase "Spoolman style APIs" could be hyphenated as "Spoolman-style APIs" per grammar conventions for compound adjectives.
🧰 Tools
🪛 LanguageTool
[grammar] ~32-~32: Use a hyphen to join words.
Context: ...> HTTP requests to SpoolSense / Spoolman style APIs. Configuration: Compile-time ...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` at line 32, Update the phrase in CLAUDE.md to hyphenate the
compound adjective: change "Spoolman style APIs" to "Spoolman-style APIs" so the
compound adjective is grammatically correct (look for the line containing "Spool
Sync: ApplicationManager triggers sync -> SpoolmanManager queues request ->
SpoolmanManager task -> HTTP requests to SpoolSense / Spoolman style APIs.").
Comment on lines
+116
to
+118
| pn5180_spi.beginTransaction(PN5180_SPI_SETTINGS); | ||
| transceiveCommand(buf, 6); | ||
| SPI.endTransaction(); | ||
| pn5180_spi.endTransaction(); |
There was a problem hiding this comment.
Return the result of transceiveCommand() instead of always succeeding.
This wrapper still discards the bool from transceiveCommand() and returns true, so BUSY/SPI timeouts are silently reported as success. The same pattern is repeated in the other transaction wrappers below.
Possible fix
- pn5180_spi.beginTransaction(PN5180_SPI_SETTINGS);
- transceiveCommand(buf, 6);
- pn5180_spi.endTransaction();
-
- return true;
+ pn5180_spi.beginTransaction(PN5180_SPI_SETTINGS);
+ const bool ok = transceiveCommand(buf, 6);
+ pn5180_spi.endTransaction();
+ return ok;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/PN5180/PN5180.cpp` around lines 116 - 118, The wrapper that begins/ends
SPI transactions (calls pn5180_spi.beginTransaction, transceiveCommand(buf, 6),
pn5180_spi.endTransaction()) should propagate the boolean result of
transceiveCommand instead of always returning true; modify the wrapper function
(and the other similar transaction wrappers in PN5180.cpp) to capture the return
value of transceiveCommand(...) and return that value (ensuring endTransaction()
still runs), so BUSY/SPI timeouts are reported correctly.
Comment on lines
+356
to
+359
| if (strcmp(s.tag_format, "OpenPrintTag") == 0) spool.tagType = 1; | ||
| else if (strcmp(s.tag_format, "TigerTag") == 0) spool.tagType = 2; | ||
| else if (strcmp(s.tag_format, "OpenTag3D") == 0) spool.tagType = 3; | ||
| else spool.tagType = 0; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if Bambu tag format string exists elsewhere
rg -n '"Bambu"' --type cpp
rg -n 'tag_format.*Bambu|Bambu.*tag_format' --type cppRepository: SpoolSense/spoolsense_scanner
Length of output: 55
🏁 Script executed:
# Find DisplaySpoolData definition to verify the comment
rg -n "DisplaySpoolData" --type cpp --type h -A 20 | head -100Repository: SpoolSense/spoolsense_scanner
Length of output: 5833
🏁 Script executed:
# Find all tag_format comparisons to see what formats are actually supported
rg -n "tag_format" --type cpp --type h -B 2 -A 2Repository: SpoolSense/spoolsense_scanner
Length of output: 3193
🏁 Script executed:
# Search for all string literals that might be tag formats
rg -n "strcmp.*tag_format|tag_format.*strcmp" --type cpp --type h -A 1 -B 1Repository: SpoolSense/spoolsense_scanner
Length of output: 538
Add missing "Bambu" (type 4) mapping in tag format switch.
The DisplaySpoolData struct (DisplayI.h line 12) documents tagType 4 as "Bambu", but the mapping at lines 356-359 doesn't handle it, defaulting to 0 instead:
Diff
if (strcmp(s.tag_format, "OpenPrintTag") == 0) spool.tagType = 1;
else if (strcmp(s.tag_format, "TigerTag") == 0) spool.tagType = 2;
else if (strcmp(s.tag_format, "OpenTag3D") == 0) spool.tagType = 3;
+ else if (strcmp(s.tag_format, "Bambu") == 0) spool.tagType = 4;
else spool.tagType = 0;📝 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
| if (strcmp(s.tag_format, "OpenPrintTag") == 0) spool.tagType = 1; | |
| else if (strcmp(s.tag_format, "TigerTag") == 0) spool.tagType = 2; | |
| else if (strcmp(s.tag_format, "OpenTag3D") == 0) spool.tagType = 3; | |
| else spool.tagType = 0; | |
| if (strcmp(s.tag_format, "OpenPrintTag") == 0) spool.tagType = 1; | |
| else if (strcmp(s.tag_format, "TigerTag") == 0) spool.tagType = 2; | |
| else if (strcmp(s.tag_format, "OpenTag3D") == 0) spool.tagType = 3; | |
| else if (strcmp(s.tag_format, "Bambu") == 0) spool.tagType = 4; | |
| else spool.tagType = 0; |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 356-356: statement should be inside braces
(readability-braces-around-statements)
[warning] 357-357: statement should be inside braces
(readability-braces-around-statements)
[warning] 358-358: statement should be inside braces
(readability-braces-around-statements)
[warning] 359-359: statement should be inside braces
(readability-braces-around-statements)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ApplicationManager.cpp` around lines 356 - 359, The tag format mapping
omits the "Bambu" case causing tagType to fall back to 0; update the conditional
block that checks s.tag_format (the strcmp chain around ApplicationManager::...
lines) to include an else if (strcmp(s.tag_format, "Bambu") == 0) that sets
spool.tagType = 4 so it matches the DisplaySpoolData/DisplayI.h documentation;
keep the existing OpenPrintTag/TigerTag/OpenTag3D branches and default to 0 for
unknown formats.
Comment on lines
+196
to
+199
| if (prefs.isKey(NVS_KEY_TFT_ON)) { | ||
| _tftEnabled = prefs.getUChar(NVS_KEY_TFT_ON, _tftEnabled ? 1 : 0) != 0; | ||
| anyOverride = true; | ||
| } |
There was a problem hiding this comment.
tft_enabled never gets a first-boot default from loadFromDeviceConfig().
Unlike LCD/LED/keypad, this flag is only overridden from NVS here. On a blank partition _tftEnabled stays at the hard-coded false, so the new toggle will not follow the same compile-time peripheral-default path as the other hardware options.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ConfigurationManager.cpp` around lines 196 - 199, loadFromDeviceConfig()
must initialize _tftEnabled so the NVS override has a sensible first-boot
default; currently tft only gets set when NVS key NVS_KEY_TFT_ON exists. Update
loadFromDeviceConfig() (or the device-config parsing path) to set _tftEnabled
from the device/config compile-time default (or the existing
deviceConfig.tft/default field if present), and keep the NVS read in the block
using prefs.getUChar(NVS_KEY_TFT_ON, _tftEnabled ? 1 : 0) to override it; ensure
the unique symbols touched are loadFromDeviceConfig(), _tftEnabled and
NVS_KEY_TFT_ON.
Comment on lines
+127
to
+139
| void TFTManager::begin() { | ||
| _tft.init(); | ||
| delay(100); // Let display hardware stabilize after cold boot | ||
| _tft.setRotation(0); | ||
| _tft.setBrightness(255); | ||
| _tft.fillScreen(COLOR_BG); | ||
|
|
||
| _sprite.setColorDepth(8); // 8-bit = 57.6KB vs 115KB at 16-bit | ||
| _sprite.createSprite(_tft.width(), _tft.height()); | ||
|
|
||
| _messageQueue = xQueueCreate(4, sizeof(TFTMessage)); | ||
| _lastActivityMs = millis(); | ||
| } |
There was a problem hiding this comment.
Add error handling for resource allocation failures.
xQueueCreate and _sprite.createSprite() can fail under low memory conditions. Unchecked failures will cause null pointer dereferences in xQueueSend calls and sprite rendering.
🛡️ Proposed fix to add error checking
void TFTManager::begin() {
_tft.init();
delay(100); // Let display hardware stabilize after cold boot
_tft.setRotation(0);
_tft.setBrightness(255);
_tft.fillScreen(COLOR_BG);
_sprite.setColorDepth(8); // 8-bit = 57.6KB vs 115KB at 16-bit
- _sprite.createSprite(_tft.width(), _tft.height());
+ if (!_sprite.createSprite(_tft.width(), _tft.height())) {
+ Serial.println("TFTManager: Failed to create sprite");
+ return;
+ }
_messageQueue = xQueueCreate(4, sizeof(TFTMessage));
+ if (_messageQueue == nullptr) {
+ Serial.println("TFTManager: Failed to create message queue");
+ return;
+ }
_lastActivityMs = millis();
}📝 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 TFTManager::begin() { | |
| _tft.init(); | |
| delay(100); // Let display hardware stabilize after cold boot | |
| _tft.setRotation(0); | |
| _tft.setBrightness(255); | |
| _tft.fillScreen(COLOR_BG); | |
| _sprite.setColorDepth(8); // 8-bit = 57.6KB vs 115KB at 16-bit | |
| _sprite.createSprite(_tft.width(), _tft.height()); | |
| _messageQueue = xQueueCreate(4, sizeof(TFTMessage)); | |
| _lastActivityMs = millis(); | |
| } | |
| void TFTManager::begin() { | |
| _tft.init(); | |
| delay(100); // Let display hardware stabilize after cold boot | |
| _tft.setRotation(0); | |
| _tft.setBrightness(255); | |
| _tft.fillScreen(COLOR_BG); | |
| _sprite.setColorDepth(8); // 8-bit = 57.6KB vs 115KB at 16-bit | |
| if (!_sprite.createSprite(_tft.width(), _tft.height())) { | |
| Serial.println("TFTManager: Failed to create sprite"); | |
| return; | |
| } | |
| _messageQueue = xQueueCreate(4, sizeof(TFTMessage)); | |
| if (_messageQueue == nullptr) { | |
| Serial.println("TFTManager: Failed to create message queue"); | |
| return; | |
| } | |
| _lastActivityMs = millis(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TFTManager.cpp` around lines 127 - 139, TFTManager::begin currently
assumes resource allocations always succeed; check the return values of
_sprite.createSprite(...) and xQueueCreate(...) and handle failures: after
calling _sprite.createSprite(width, height) verify it returned true/valid and if
not log an error and set a flag (e.g., _spriteAvailable = false) to skip sprite
rendering, and after xQueueCreate(4, sizeof(TFTMessage)) verify the returned
handle is non-NULL and if NULL log an error and set _messageQueue =
NULL/_queueAvailable = false (or abort initialization) so subsequent xQueueSend
and sprite calls first check these flags/handles and avoid dereferencing NULL;
ensure _lastActivityMs is still initialized and add any cleanup or safe-fail
behavior so the rest of TFTManager methods gracefully handle missing
sprite/queue resources.
Comment on lines
+298
to
+306
| // Breathing animation tick | ||
| if (_isBreathing && (millis() - _lastBreathMs >= BREATH_STEP_MS)) { | ||
| _lastBreathMs = millis(); | ||
| _breathBrightness += _breathDirection * 3; | ||
| if (_breathBrightness <= 30) _breathDirection = 1; | ||
| if (_breathBrightness >= 255) _breathDirection = -1; | ||
| _breathBrightness = constrain(_breathBrightness, 30, 255); | ||
| _tft.setBrightness(_breathBrightness); | ||
| } |
There was a problem hiding this comment.
Data race on breathing animation state.
_isBreathing is set inside a critical section (line 256, 274-279) but read at line 299 without protection. Similarly, _breathBrightness, _breathDirection, and _breathColor are accessed here without synchronization, while _isBreathing can be modified concurrently by setScreenTimeoutMs or another queue message.
While this likely works in practice because only the TFT task reads these variables and modifications are atomic-ish on ESP32, it's technically a race condition.
🔒 Proposed fix to read _isBreathing atomically
// Breathing animation tick
- if (_isBreathing && (millis() - _lastBreathMs >= BREATH_STEP_MS)) {
+ bool breathing;
+ taskENTER_CRITICAL(&_stateMux);
+ breathing = _isBreathing;
+ taskEXIT_CRITICAL(&_stateMux);
+ if (breathing && (millis() - _lastBreathMs >= BREATH_STEP_MS)) {
_lastBreathMs = millis();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TFTManager.cpp` around lines 298 - 306, Read the breathing state and
related fields under the same critical section used when they are modified:
acquire the existing mutex/critical region (the one used by setScreenTimeoutMs
and other queue handlers), copy _isBreathing, _breathBrightness,
_breathDirection and _breathColor into local variables, then release the lock
and perform the breathing math and _tft.setBrightness() using the locals;
reference the members _isBreathing, _breathBrightness, _breathDirection,
_breathColor and the modifier method setScreenTimeoutMs to locate the protecting
synchronization primitive to reuse for this read-copy-release pattern.
Comment on lines
+627
to
+632
| uint32_t TFTManager::dimColor(uint32_t color, uint8_t brightness) { | ||
| uint8_t r = ((color >> 16) & 0xFF) * brightness / 255; | ||
| uint8_t g = ((color >> 8) & 0xFF) * brightness / 255; | ||
| uint8_t b = ((color) & 0xFF) * brightness / 255; | ||
| return ((uint32_t)r << 16) | ((uint32_t)g << 8) | b; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Note: dimColor helper is currently unused.
This function is defined but not called anywhere in the current implementation (breathing animation uses setBrightness instead). Consider removing if not planned for use, or adding a comment indicating future intent.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 627-627: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 627-627: 2 adjacent parameters of 'dimColor' of similar type ('int') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 627-627: the first parameter in the range is 'color'
(clang)
[note] 627-627: the last parameter in the range is 'brightness'
(clang)
[warning] 628-628: variable 'r' is not initialized
(cppcoreguidelines-init-variables)
[warning] 628-628: variable name 'r' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 629-629: variable 'g' is not initialized
(cppcoreguidelines-init-variables)
[warning] 629-629: variable name 'g' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 630-630: variable 'b' is not initialized
(cppcoreguidelines-init-variables)
[warning] 630-630: variable name 'b' is too short, expected at least 3 characters
(readability-identifier-length)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TFTManager.cpp` around lines 627 - 632, The helper function dimColor is
defined but never used; either remove the unused function dimColor(uint32_t,
uint8_t) from TFTManager.cpp to avoid dead code, or add a brief comment above
dimColor explaining its intended future use (e.g., kept for per-pixel brightness
adjustments vs. current setBrightness-based breathing animation) so reviewers
know it’s intentionally retained; make the change by editing the dimColor
declaration/definition accordingly.
Comment on lines
+22
to
+28
| // Tag type constants — one icon per type | ||
| #define TAG_TYPE_UNKNOWN 0 | ||
| #define TAG_TYPE_OPENPRINTTAG 1 | ||
| #define TAG_TYPE_TIGERTAG 2 | ||
| #define TAG_TYPE_OPENTAG3D 3 | ||
| #define TAG_TYPE_BAMBU 4 | ||
| #define TAG_TYPE_NFC_PLAIN 5 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using a typed enum for tag type constants.
The TAG_TYPE_* defines work but a scoped enum would provide type safety and avoid potential macro conflicts.
♻️ Optional refactor to typed enum
-// Tag type constants — one icon per type
-#define TAG_TYPE_UNKNOWN 0
-#define TAG_TYPE_OPENPRINTTAG 1
-#define TAG_TYPE_TIGERTAG 2
-#define TAG_TYPE_OPENTAG3D 3
-#define TAG_TYPE_BAMBU 4
-#define TAG_TYPE_NFC_PLAIN 5
+// Tag type constants — one icon per type
+enum class TagType : uint8_t {
+ Unknown = 0,
+ OpenPrintTag = 1,
+ TigerTag = 2,
+ OpenTag3D = 3,
+ Bambu = 4,
+ NfcPlain = 5
+};📝 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
| // Tag type constants — one icon per type | |
| #define TAG_TYPE_UNKNOWN 0 | |
| #define TAG_TYPE_OPENPRINTTAG 1 | |
| #define TAG_TYPE_TIGERTAG 2 | |
| #define TAG_TYPE_OPENTAG3D 3 | |
| #define TAG_TYPE_BAMBU 4 | |
| #define TAG_TYPE_NFC_PLAIN 5 | |
| // Tag type constants — one icon per type | |
| enum class TagType : uint8_t { | |
| Unknown = 0, | |
| OpenPrintTag = 1, | |
| TigerTag = 2, | |
| OpenTag3D = 3, | |
| Bambu = 4, | |
| NfcPlain = 5 | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TFTManager.h` around lines 22 - 28, Replace the TAG_TYPE_* preprocessor
macros with a scoped typed enum (e.g., enum class TagType : int { Unknown = 0,
OpenPrintTag = 1, TigerTag = 2, OpenTag3D = 3, Bambu = 4, NfcPlain = 5 }) in
TFTManager.h; update any variables, function parameters, return types and
switch/case logic that currently use those macros or plain ints to use TagType
(or cast where interop is required) and adjust comparisons to use TagType::...
to gain type safety and avoid macro conflicts (search for uses of
TAG_TYPE_UNKNOWN, TAG_TYPE_OPENPRINTTAG, TAG_TYPE_TIGERTAG, TAG_TYPE_OPENTAG3D,
TAG_TYPE_BAMBU, TAG_TYPE_NFC_PLAIN and replace accordingly).
Comment on lines
555
to
+558
| update.lcd_enabled = doc["lcd_enabled"] | (uint8_t)0; | ||
| update.led_enabled = doc["led_enabled"] | (uint8_t)0; | ||
| update.keypad_enabled = doc["keypad_enabled"] | (uint8_t)0; | ||
| update.tft_enabled = doc["tft_enabled"] | (uint8_t)0; |
There was a problem hiding this comment.
Reject the LCD+TFT combination before persisting config.
On ESP32-WROOM both displays share GPIO23/GPIO22, but this handler stores them as independent booleans. A single POST can therefore persist an impossible hardware state and leave startup behavior ambiguous.
Possible fix
update.lcd_enabled = doc["lcd_enabled"] | (uint8_t)0;
update.led_enabled = doc["led_enabled"] | (uint8_t)0;
update.keypad_enabled = doc["keypad_enabled"] | (uint8_t)0;
update.tft_enabled = doc["tft_enabled"] | (uint8_t)0;
+#ifndef BOARD_ESP32_S3
+ if (update.lcd_enabled && update.tft_enabled) {
+ sendError(400, "LCD and TFT cannot both be enabled on ESP32-WROOM");
+ return;
+ }
+#endif🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/WebServerManager.cpp` around lines 555 - 558, After reading the JSON into
update (the fields update.lcd_enabled and update.tft_enabled), add a validation
step that rejects the request if both update.lcd_enabled and update.tft_enabled
are true; return an HTTP 4xx error (with a short message like "LCD and TFT
cannot be enabled simultaneously") and do not call the config persistence/commit
routine (the function that writes the updated config or calls save/persist).
Place this check immediately after the lines that set
update.lcd_enabled/update.tft_enabled and before any code that applies or saves
the configuration so the impossible hardware state is never persisted.
- Pass initial_weight_g through SpoolmanSyncedPayload so NFC+ tags show the weight bar on TFT (was always 0, hiding the bar) - NFC+ registration: single extruder/bed temp fields (averaged from material DB min/max), written to Spoolman filament settings
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 686-697: Before painting UID-lookup results (when
msg.payload.spoolmanSynced.success && msg.payload.spoolmanSynced.is_uid_lookup),
verify the tag is still present and skip creating/showing the DisplaySpool if it
was removed or replaced; specifically, guard the block that builds DisplaySpool
and calls display_->showSpool with the same tagStillPresent (or equivalent)
check used for NFCManager so stale UID-lookup screens are not shown, and apply
the same guard to the other UID-lookup branch(s) that call display_->showSpool.
- Around line 449-455: In handleSpoolUpdated(), don’t leave the transient
"Syncing Spoolman" screen when no sync is actually enqueued: only call
display_->showText(line1, "Syncing Spoolman") if you will subsequently enqueue a
Spoolman sync; otherwise immediately set the terminal UI state (either call
display_->showText with the final/terminal second line or post the
SPOOLMAN_SYNCED event) so the "Syncing Spoolman" message is cleared; update both
the non-lookup success branch (where line1 is built using kgRemaining) and the
similar branch around the other occurrence to either enqueue the sync or
transition to SPOOLMAN_SYNCED/display the final screen.
- Around line 347-353: The code always formats s.primary_color into
spool.colorHex causing colorless spools to appear black; modify the spool
population logic in the block that handles SpoolDetectedPayload (variables: s,
DisplaySpoolData spool, spool.colorHex) to check s.has_color first and only
snprintf the hex when has_color is true, otherwise set spool.colorHex to an
explicit "no color" state (e.g., empty string or a sentinel value) so the
display can distinguish missing color from black.
In `@src/SpoolmanManager.cpp`:
- Around line 1129-1131: getSpoolDetails currently only copies the fallback
filament.weight when its token is "real" and ignores integer values, causing
msg.payload.spoolmanSynced.initial_weight_g to be zero when Spoolman emits 1000
instead of 1000.0; update getSpoolDetails to accept integer-valued weights for
the "real" token by detecting numeric integer values (or coercing
filament.weight to float) and always propagate a converted float into
details.initial_weight_g so that msg.payload.spoolmanSynced.initial_weight_g
gets the correct nonzero value even when filament.weight is an integer; touch
the code paths around getSpoolDetails, details.initial_weight_g, and where
filament.token == "real" to perform the conversion.
🪄 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: 4a8c4359-94c3-42ac-bd47-9a29d1582196
📒 Files selected for processing (3)
src/ApplicationManager.cppsrc/ApplicationManager.hsrc/SpoolmanManager.cpp
Comment on lines
+347
to
+353
| const auto& s = msg.payload.spoolDetected; | ||
| DisplaySpoolData spool{}; | ||
| strncpy(spool.brand, s.manufacturer, sizeof(spool.brand) - 1); | ||
| strncpy(spool.material, s.material_name, sizeof(spool.material) - 1); | ||
| snprintf(spool.colorHex, sizeof(spool.colorHex), "%02X%02X%02X", | ||
| s.primary_color[0], s.primary_color[1], s.primary_color[2]); | ||
| spool.remainingWeight = s.kg_remaining * 1000.0f; |
There was a problem hiding this comment.
Preserve the “no color” state in DisplaySpoolData.
SpoolDetectedPayload.has_color is what distinguishes “missing color” from an actual black spool, but this path always formats primary_color into a hex string. Colorless tags will render as black on TFT.
Suggested fix
DisplaySpoolData spool{};
strncpy(spool.brand, s.manufacturer, sizeof(spool.brand) - 1);
strncpy(spool.material, s.material_name, sizeof(spool.material) - 1);
- snprintf(spool.colorHex, sizeof(spool.colorHex), "%02X%02X%02X",
- s.primary_color[0], s.primary_color[1], s.primary_color[2]);
+ if (s.has_color) {
+ snprintf(spool.colorHex, sizeof(spool.colorHex), "%02X%02X%02X",
+ s.primary_color[0], s.primary_color[1], s.primary_color[2]);
+ } else {
+ spool.colorHex[0] = '\0';
+ }🧰 Tools
🪛 Clang (14.0.6)
[warning] 347-347: variable name 's' is too short, expected at least 3 characters
(readability-identifier-length)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ApplicationManager.cpp` around lines 347 - 353, The code always formats
s.primary_color into spool.colorHex causing colorless spools to appear black;
modify the spool population logic in the block that handles SpoolDetectedPayload
(variables: s, DisplaySpoolData spool, spool.colorHex) to check s.has_color
first and only snprintf the hex when has_color is true, otherwise set
spool.colorHex to an explicit "no color" state (e.g., empty string or a sentinel
value) so the display can distinguish missing color from black.
Comment on lines
449
to
455
| if (msg.payload.spoolUpdated.success) { | ||
| char line1[17]; | ||
| snprintf(line1, sizeof(line1), "Updated: %.0fg", | ||
| kgRemaining * 1000.0f); | ||
| if (spoolmanConfigured) { | ||
| lcdManager->updateScreen(line1, "Syncing Spoolman"); | ||
| display_->showText(line1, "Syncing Spoolman"); | ||
| // Type/Remain will be scheduled after SPOOLMAN_SYNCED |
There was a problem hiding this comment.
The post-update Spoolman UI state never reaches a terminal screen.
handleSpoolUpdated() enters “Syncing Spoolman” whenever Spoolman is configured, but actual syncs are only enqueued under tighter conditions later in the function, and the successful non-lookup branch here never clears that transient screen. The result is a stuck sync message both when no sync is sent and when the sync succeeds.
Also applies to: 698-699
🧰 Tools
🪛 Clang (14.0.6)
[note] 449-449: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 453-453: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[warning] 450-450: do not declare C-style arrays, use std::array<> instead
(modernize-avoid-c-arrays)
[warning] 452-452: floating point literal has suffix 'f', which is not uppercase
(readability-uppercase-literal-suffix)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ApplicationManager.cpp` around lines 449 - 455, In handleSpoolUpdated(),
don’t leave the transient "Syncing Spoolman" screen when no sync is actually
enqueued: only call display_->showText(line1, "Syncing Spoolman") if you will
subsequently enqueue a Spoolman sync; otherwise immediately set the terminal UI
state (either call display_->showText with the final/terminal second line or
post the SPOOLMAN_SYNCED event) so the "Syncing Spoolman" message is cleared;
update both the non-lookup success branch (where line1 is built using
kgRemaining) and the similar branch around the other occurrence to either
enqueue the sync or transition to SPOOLMAN_SYNCED/display the final screen.
Comment on lines
+686
to
+697
| if (msg.payload.spoolmanSynced.success && msg.payload.spoolmanSynced.is_uid_lookup) { | ||
| // UID lookup — show spool graphic with Spoolman data (tag had no data) | ||
| DisplaySpoolData spool{}; | ||
| strncpy(spool.brand, msg.payload.spoolmanSynced.manufacturer, sizeof(spool.brand) - 1); | ||
| strncpy(spool.material, materialName, sizeof(spool.material) - 1); | ||
| const char* colorSrc = msg.payload.spoolmanSynced.color_hex; | ||
| if (colorSrc[0] == '#') colorSrc++; | ||
| strncpy(spool.colorHex, colorSrc, sizeof(spool.colorHex) - 1); | ||
| spool.remainingWeight = kgRemaining * 1000.0f; | ||
| spool.totalWeight = msg.payload.spoolmanSynced.initial_weight_g; | ||
| spool.tagType = 5; | ||
| display_->showSpool(spool); |
There was a problem hiding this comment.
Ignore stale UID-lookup results after tag removal.
The tagStillPresent check above protects NFCManager, but these UID-lookup display branches still paint a result even if the tag was removed or replaced while the request was in flight. On TFT that stale screen can persist because the tag-removed path intentionally avoids restoring status.
Also applies to: 700-701
🧰 Tools
🪛 Clang (14.0.6)
[note] 686-686: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 686-686: +1
(clang)
[note] 692-692: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[warning] 692-692: statement should be inside braces
(readability-braces-around-statements)
[warning] 694-694: floating point literal has suffix 'f', which is not uppercase
(readability-uppercase-literal-suffix)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ApplicationManager.cpp` around lines 686 - 697, Before painting
UID-lookup results (when msg.payload.spoolmanSynced.success &&
msg.payload.spoolmanSynced.is_uid_lookup), verify the tag is still present and
skip creating/showing the DisplaySpool if it was removed or replaced;
specifically, guard the block that builds DisplaySpool and calls
display_->showSpool with the same tagStillPresent (or equivalent) check used for
NFCManager so stale UID-lookup screens are not shown, and apply the same guard
to the other UID-lookup branch(s) that call display_->showSpool.
Comment on lines
1129
to
+1131
| msg.payload.spoolmanSynced.spoolman_id = found ? details.spoolman_id : -1; | ||
| msg.payload.spoolmanSynced.kg_remaining = found ? details.remaining_weight_g / 1000.0f : 0.0f; | ||
| msg.payload.spoolmanSynced.initial_weight_g = found ? details.initial_weight_g : 0.0f; |
There was a problem hiding this comment.
Handle integer filament.weight before propagating initial_weight_g.
getSpoolDetails() still only copies the fallback filament.weight when the token is real. If Spoolman emits 1000 instead of 1000.0, UID lookups will still send 0 here and the TFT weight bar stays empty.
🧰 Tools
🪛 Clang (14.0.6)
[note] 1129-1129: +4, including nesting penalty of 3, nesting level increased to 4
(clang)
[note] 1130-1130: +4, including nesting penalty of 3, nesting level increased to 4
(clang)
[note] 1131-1131: +4, including nesting penalty of 3, nesting level increased to 4
(clang)
[warning] 1130-1130: floating point literal has suffix 'f', which is not uppercase
(readability-uppercase-literal-suffix)
[warning] 1130-1130: floating point literal has suffix 'f', which is not uppercase
(readability-uppercase-literal-suffix)
[warning] 1131-1131: floating point literal has suffix 'f', which is not uppercase
(readability-uppercase-literal-suffix)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SpoolmanManager.cpp` around lines 1129 - 1131, getSpoolDetails currently
only copies the fallback filament.weight when its token is "real" and ignores
integer values, causing msg.payload.spoolmanSynced.initial_weight_g to be zero
when Spoolman emits 1000 instead of 1000.0; update getSpoolDetails to accept
integer-valued weights for the "real" token by detecting numeric integer values
(or coercing filament.weight to float) and always propagate a converted float
into details.initial_weight_g so that
msg.payload.spoolmanSynced.initial_weight_g gets the correct nonzero value even
when filament.weight is an integer; touch the code paths around getSpoolDetails,
details.initial_weight_g, and where filament.token == "real" to perform the
conversion.
…pin comment - TFTManager::begin() checks createSprite and xQueueCreate return values - Auto-disable LCD when TFT enabled in config POST handler (shared GPIO 22/23) - Fix comment: pins 22/23, not 18/23
This was referenced Mar 30, 2026
6 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
Test plan (hardware verified on WROOM)
Summary by CodeRabbit
Release Notes
New Features
Configuration
Bug Fixes
Dependencies