Fix/gc9a01 spiffs shim - #5795
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a GC9A01 TFT display usermod with rendering, sleep, clock, configuration, and rotary encoder support. Adds SPIFFS compatibility shims, PlatformIO include paths, TFT dependency metadata, and setup documentation. ChangesGC9A01 display integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RotaryEncoderUI
participant WLED
participant UsermodGC9A01Display
participant TFT_eSPI
RotaryEncoderUI->>WLED: Change display control state
RotaryEncoderUI->>UsermodGC9A01Display: Wake or show overlay
UsermodGC9A01Display->>WLED: Read current state
UsermodGC9A01Display->>TFT_eSPI: Render updated display
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (9)
usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp (5)
693-694: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
snprintfinstead ofsprintf.The current values cannot overflow
timeStr. The bound is still not expressed in the code. Static analysis flags the call.♻️ Proposed change
char timeStr[10]; - sprintf(timeStr, "%02d:%02d", hrs, mins); + snprintf(timeStr, sizeof(timeStr), "%02d:%02d", hrs, mins);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp` around lines 693 - 694, Replace the sprintf call formatting timeStr in the displayed time construction with snprintf, passing the timeStr buffer size to express the bound while preserving the existing "%02d:%02d" output.Source: Linters/SAST tools
503-503: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
knownMode >= 0check is always true.
knownModeisuint8_t. The comparison is redundant and produces a compiler warning with-Wtype-limits. Line 514 also compares a signedintwith the unsignedeffectName.length().♻️ Proposed change
- } else if (knownMode < strip.getModeCount() && knownMode >= 0) { + } else if (knownMode < strip.getModeCount()) {- for (int i = 0; i < effectName.length(); i++) { + for (unsigned i = 0; i < effectName.length(); i++) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp` at line 503, Update the conditional containing knownMode in the display mode handling to remove the redundant knownMode >= 0 check, since knownMode is uint8_t, while preserving the strip.getModeCount() upper-bound validation. Also adjust the comparison near the effectName.length() check to use compatible signed or unsigned types and eliminate the -Wtype-limits warning.
182-182: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftReduce full-screen clears and
Stringallocations in the redraw path.
drawMainInterface()runs up to once per second. It starts withtft.fillScreen(TFT_BLACK)and then redraws every element. The user sees flicker on each update. The function also builds severalStringobjects (overlayTitle,effectName,cleanName,brightStr) on every call. Repeated heap allocation and growth fragments the heap on ESP32.Redraw only the regions that changed. Build text into fixed
charbuffers instead ofString.As per path instructions (docs/cpp.instructions.md): "For display redraw loops, precompute invariants, avoid repeated allocations/String growth, and keep hot-path data in appropriate memory."
Also applies to: 190-198, 500-532
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp` at line 182, Update drawMainInterface() to remove the per-frame tft.fillScreen(TFT_BLACK) full-screen clear and redraw only UI regions whose values changed, preserving unchanged display content to prevent flicker. Replace the per-call String variables overlayTitle, effectName, cleanName, and brightStr with appropriately sized fixed char buffers, formatting text without heap allocation or growth. Precompute invariant display data outside the redraw hot path where practical.Source: Path instructions
627-640: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse an RGB565 bitmap and a bulk display write for startup/logo flash.
epd_bitmap_is a 120x120unsigned longPROGMEM array, using 14,400 × 4 bytes despite the display usingcolor565output. Convert it to astatic const uint16_t ... PROGMEMarray and updatedrawWLEDLogo()so it needs onlyLOGO_SIZE * LOGO_SIZEelements and transmits one packed color sequence fortft.setAddrWindow().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp` around lines 627 - 640, Update epd_bitmap_ to a static const uint16_t PROGMEM RGB565 array, preserving the 120×120 logo dimensions and packed color values. In drawWLEDLogo(), iterate over LOGO_SIZE * LOGO_SIZE elements, read each 16-bit value from PROGMEM, and send the contiguous RGB565 buffer through one setAddrWindow() bulk display write instead of converting and calling pushColor() per pixel.
943-956: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused GC9A01 members and stubs.
getEncoderModeName(),drawCurrentModeIndicator(),drawModeOverlay(),needsRedraw,knownMinute,knownHour,knownPowerState, andsetBrightness(uint8_t)are no longer read. Remove them from the header and implementation to avoid dead code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp` around lines 943 - 956, Remove the unused getEncoderModeName(), drawCurrentModeIndicator(), and drawModeOverlay() definitions from the GC9A01 display implementation, and remove their declarations plus needsRedraw, knownMinute, knownHour, knownPowerState, and setBrightness(uint8_t) from the corresponding class header. Update any related initialization or references so no dead declarations or definitions remain.Source: Path instructions
usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp (2)
1040-1042: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the remaining
sprintfcalls withsnprintf.These five calls write into 64-byte buffers. The current values cannot overflow them. The bound is still not expressed in the code, and static analysis reports each call.
Apply the same change in each location, for example:
snprintf(lineBuffer, sizeof(lineBuffer), "Custom%d: %d", par, val);Also applies to: 1134-1136, 1181-1183, 1230-1232, 1277-1279
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp` around lines 1040 - 1042, Replace all five sprintf calls writing to lineBuffer in the relevant display-update code paths with snprintf, passing sizeof(lineBuffer) as the buffer size while preserving each existing format string and arguments.Source: Linters/SAST tools
825-832: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated GC9A01 wake block into one helper.
The identical five-line pattern appears in ten functions:
if (gc9a01Display && gc9a01Display->wakeDisplay()) { gc9a01Display->redraw(true); return; } if (gc9a01Display) gc9a01Display->updateRedrawTime();Each copy must stay in sync. A single private helper that returns a bool removes the duplication for both display types.
♻️ Proposed helper
// returns true if the caller must discard the input (a display was woken) bool RotaryEncoderUIUsermod::prepareDisplays() { `#ifdef` USERMOD_FOUR_LINE_DISPLAY if (display && display->wakeDisplay()) { display->redraw(true); return true; } if (display) display->updateRedrawTime(); `#endif` `#ifdef` USERMOD_GC9A01_DISPLAY if (gc9a01Display && gc9a01Display->wakeDisplay()) { gc9a01Display->redraw(true); return true; } if (gc9a01Display) gc9a01Display->updateRedrawTime(); `#endif` return false; }Each change function then starts with
if (prepareDisplays()) return;.Also applies to: 869-875, 907-914, 951-958, 996-1003, 1058-1065, 1103-1110, 1151-1158, 1198-1205, 1248-1255
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp` around lines 825 - 832, Extract the duplicated display-wake handling into a private RotaryEncoderUIUsermod::prepareDisplays() helper that processes both display types, redraws any woken display, updates redraw timing otherwise, and returns whether input should be discarded. Replace the repeated blocks in all listed change functions with an early `if (prepareDisplays()) return;` while preserving the existing conditional compilation behavior.usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.h (1)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused GC9A01 pin aliases.
GC9A01_CS_PIN,GC9A01_DC_PIN, andGC9A01_RST_PINare defined but never referenced by the usermod.TFT_eSPIusesTFT_CS,TFT_DC, andTFT_RSTfor display pins, and the#ifndeffallbacks do not configure the driver after#include <TFT_eSPI.h>.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.h` at line 13, Remove the unused GC9A01 pin alias definitions, including GC9A01_CS_PIN, GC9A01_DC_PIN, and GC9A01_RST_PIN, from the display usermod; retain TFT_eSPI’s TFT_CS, TFT_DC, and TFT_RST configuration unchanged.usermods/usermod_v2_gc9a01_display/readme.md (1)
74-78: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMove vendor-specific identity overrides out of the generic setup.
If this README targets general WLED users, remove these hardcoded values or move them to a clearly marked board-specific section. The block sets
WLED_BRAND,WLED_PRODUCT_NAME,WLED_REPO,SERVERNAME, andWLED_RELEASE_NAMEto the author's product and fork.Custom brand and product values can change API and MQTT identity and may affect client recognition. (github.com)
As per path instructions, AI-generated configuration changes that are unrelated to the PR objective must be identified.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/usermod_v2_gc9a01_display/readme.md` around lines 74 - 78, Remove the vendor-specific identity overrides from the generic setup instructions, including WLED_BRAND, WLED_PRODUCT_NAME, WLED_REPO, SERVERNAME, and WLED_RELEASE_NAME, or relocate them into a clearly labeled board-specific configuration section.Sources: Path instructions, MCP tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@include/SPIFFS.h`:
- Around line 5-12: Update the SPIFFS definitions in include/SPIFFS.h lines 5-12
and lib/compat_spiffs/include/SPIFFS.h lines 5-12 so SPIFFS aliases or
references the mounted global LittleFS instance used by begin(), rather than
creating a separate fs::LittleFSFS object; alternatively, ensure this exact shim
object is mounted before compatibility-library file operations.
In `@usermods/usermod_v2_gc9a01_display/readme.md`:
- Line 3: Update the opening description to replace “fully-featured” with “fully
featured,” preserving the rest of the sentence unchanged.
- Line 73: Remove WLED_DISABLE_BROWNOUT_DET from the generic setup instructions
in the readme. Document it only in the board- or environment-specific setup
section for known power-supply issues, including a warning that it disables the
ESP32 brownout detector.
In `@usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp`:
- Around line 895-903: Replace FPSTR() uses on the GC9A01 configuration key
literals with shared static const char PROGMEM arrays, declared once in the
usermod and reused by readFromConfig(), addToConfig(), and appendConfigData().
Update the affected enabled, sleepMode, clockMode, and clock12hour key accesses
consistently while preserving existing configuration behavior.
- Line 70: Resolve the mismatch between the lastRedraw sentinel behavior and
setup initialization: choose whether the display sleeps after boot or waits for
first interaction. For sleep-after-boot, remove the ULONG_MAX sentinel usage and
corresponding lastRedraw != ULONG_MAX guard; for wait-for-interaction, remove
the lastRedraw = millis() assignment in setup(). Update the related comments to
describe the selected behavior accurately.
- Around line 62-63: Replace absolute timestamp comparisons in the overlay
timeout and update scheduling logic, including the blocks around
activeOverlayMode and the referenced redraw paths, with unsigned elapsed-time
subtraction comparisons. Update checks involving now, overlayUntil, nextUpdate,
and millis() so behavior remains correct across millis() rollover, preserving
the existing expiration and redraw conditions.
- Around line 912-918: Update readFromConfig() so it stores the backlight value
without calling setBacklight() before setup, and return the presence check for
the newest configuration key instead of true. Add the requested initDone member,
set it at the end of setup(), and have initDisplay() apply the stored backlight
only after TFT_BL has been configured as an output.
- Around line 341-343: Fix the note-symbol detection in the mode-name handling
by checking the raw byte value as unsigned rather than comparing the signed
result of String::charAt() to 127. In the condition around modeName, require
length greater than 1 before reading index 1, while preserving the existing
substring(5) removal behavior.
In `@usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.h`:
- Around line 26-28: Remove the fallback TFT_BL definition and guard all
backlight pin operations in initDisplay() and related code so they compile and
run only when TFT_eSPI provides TFT_BL. In setup(), register the defined
backlight pin with PinManager using allocatePin(TFT_BL, true,
PinOwner::UM_Unspecified) before configuring or driving it.
- Around line 30-32: Add USERMOD_ID_GC9A01_DISPLAY with value 59 to the shared
usermod ID definitions in const.h, then remove the local `#ifndef/`#define
fallback from the usermod_v2_gc9a01_display header so it uses the centralized
definition.
In
`@usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp`:
- Around line 678-686: The GC9A01-specific reset block around select_state
currently treats the 750 ms overlay expiry as selection expiry, so an untouched
Effect selection falls back to brightness. Replace this dependency with a
dedicated rotary-user-mod inactivity timeout that starts or refreshes when
changeState() selects a state and only resets select_state after the intended
timeout; alternatively, update changeState() so the overlay duration matches
that selection timeout. Preserve the existing asleep-display reset behavior.
- Around line 850-852: Replace sprintf in the brightness display code with
snprintf, passing the actual brightnessStr capacity (sizeof(brightnessStr)) as
the bound while preserving the existing format and overlay behavior.
---
Nitpick comments:
In `@usermods/usermod_v2_gc9a01_display/readme.md`:
- Around line 74-78: Remove the vendor-specific identity overrides from the
generic setup instructions, including WLED_BRAND, WLED_PRODUCT_NAME, WLED_REPO,
SERVERNAME, and WLED_RELEASE_NAME, or relocate them into a clearly labeled
board-specific configuration section.
In `@usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp`:
- Around line 693-694: Replace the sprintf call formatting timeStr in the
displayed time construction with snprintf, passing the timeStr buffer size to
express the bound while preserving the existing "%02d:%02d" output.
- Line 503: Update the conditional containing knownMode in the display mode
handling to remove the redundant knownMode >= 0 check, since knownMode is
uint8_t, while preserving the strip.getModeCount() upper-bound validation. Also
adjust the comparison near the effectName.length() check to use compatible
signed or unsigned types and eliminate the -Wtype-limits warning.
- Line 182: Update drawMainInterface() to remove the per-frame
tft.fillScreen(TFT_BLACK) full-screen clear and redraw only UI regions whose
values changed, preserving unchanged display content to prevent flicker. Replace
the per-call String variables overlayTitle, effectName, cleanName, and brightStr
with appropriately sized fixed char buffers, formatting text without heap
allocation or growth. Precompute invariant display data outside the redraw hot
path where practical.
- Around line 627-640: Update epd_bitmap_ to a static const uint16_t PROGMEM
RGB565 array, preserving the 120×120 logo dimensions and packed color values. In
drawWLEDLogo(), iterate over LOGO_SIZE * LOGO_SIZE elements, read each 16-bit
value from PROGMEM, and send the contiguous RGB565 buffer through one
setAddrWindow() bulk display write instead of converting and calling pushColor()
per pixel.
- Around line 943-956: Remove the unused getEncoderModeName(),
drawCurrentModeIndicator(), and drawModeOverlay() definitions from the GC9A01
display implementation, and remove their declarations plus needsRedraw,
knownMinute, knownHour, knownPowerState, and setBrightness(uint8_t) from the
corresponding class header. Update any related initialization or references so
no dead declarations or definitions remain.
In `@usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.h`:
- Line 13: Remove the unused GC9A01 pin alias definitions, including
GC9A01_CS_PIN, GC9A01_DC_PIN, and GC9A01_RST_PIN, from the display usermod;
retain TFT_eSPI’s TFT_CS, TFT_DC, and TFT_RST configuration unchanged.
In
`@usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp`:
- Around line 1040-1042: Replace all five sprintf calls writing to lineBuffer in
the relevant display-update code paths with snprintf, passing sizeof(lineBuffer)
as the buffer size while preserving each existing format string and arguments.
- Around line 825-832: Extract the duplicated display-wake handling into a
private RotaryEncoderUIUsermod::prepareDisplays() helper that processes both
display types, redraws any woken display, updates redraw timing otherwise, and
returns whether input should be discarded. Replace the repeated blocks in all
listed change functions with an early `if (prepareDisplays()) return;` while
preserving the existing conditional compilation behavior.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: ee3f313a-dad1-4705-83e5-9ede182e133d
📒 Files selected for processing (10)
.gitignoreinclude/SPIFFS.hlib/compat_spiffs/include/SPIFFS.husermods/platformio_override.usermods.iniusermods/usermod_v2_gc9a01_display/library.jsonusermods/usermod_v2_gc9a01_display/logo_data.husermods/usermod_v2_gc9a01_display/readme.mdusermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cppusermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.husermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp
| // Compatibility shim for libraries that still include SPIFFS.h. | ||
| // WLED uses LittleFS for filesystem access, so we map the legacy SPIFFS | ||
| // symbol to LittleFS to keep TFT_eSPI and similar libraries buildable. | ||
| namespace fs { | ||
| using SPIFFSFS = LittleFSFS; | ||
| } | ||
|
|
||
| inline fs::LittleFSFS SPIFFS; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repository files matching SPIFFS.h and LittleFS references =="
git ls-files | rg '(^|/)(SPIFFS\.h|.*LittleFS.*)|platform\.txt|platform\.properties|_fs.*' || true
echo
echo "== include/SPIFFS.h =="
if [ -f include/SPIFFS.h ]; then cat -n include/SPIFFS.h; fi
echo
echo "== lib/compat_spiffs/include/SPIFFS.h =="
if [ -f lib/compat_spiffs/include/SPIFFS.h ]; then cat -n lib/compat_spiffs/include/SPIFFS.h; fi
echo
echo "== references to SPIFFS outside compatibility headers =="
rg -n '\bSPIFFS\b|SPIFFSFS|LittleFSFS|LittleFS\.(begin|format|mkdir|open|read|write|remove|rename|listDir)|LittleFSFS\s+LittleFS|inline\s+fs::LittleFSFS\s+LittleFS' .Repository: wled/WLED
Length of output: 2431
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== inspect likely LittleFS declarations/usages =="
rg -n "fs::LittleFSFS|LittleFSFS\s+(LittleFS|SPIFFS)|LittleFSFS LittleFS|LittleFSFS\s*LittleFS|LittleFSFS\\* LittleFS|LittleFSFS\\*SPIFFS|\\bLittleFS\\.begin\\(" -g '!lib/compat_spiffs' .Repository: wled/WLED
Length of output: 249
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for p in [Path("include/SPIFFS.h"), Path("lib/compat_spiffs/include/SPIFFS.h")]:
if p.exists():
txt = p.read_text()
print(f"{p}: bytes={len(txt)} lines={len(txt.splitlines())} contains inline fs::LittleFSFS SPIFFS: {bool('inline fs::LittleFSFS SPIFFS;' in txt)} exists variable SPIFFS: {bool('\\bSPIFFS\\b' in txt)}")
for i, line in enumerate(txt.splitlines(), 1):
if "LittleFS" in line or "SPIFFS" in line:
print(f" {i}: {line}")
PYRepository: wled/WLED
Length of output: 649
🌐 Web query:
Arduino ESP32 LittleFS.h class LittleFSFS begin method inline global LittleFS instance
💡 Result:
In the ESP32 Arduino LittleFS library, LittleFSFS::begin(...) is declared as a bool member function and the global Arduino instance is named LittleFS (of type fs::LittleFSFS). [1][2] 1) LittleFSFS class and begin signature (Arduino-ESP32) - The class is declared as class LittleFSFS: public FS, with begin(...) as: - bool begin(bool formatOnFail = false, const char * basePath = "/littlefs", uint8_t maxOpenFiles = 10, const char * partitionLabel = "spiffs"); This is shown in the header. [2] 2) Implementation details of LittleFSFS::begin - The begin implementation: - Stores partitionLabel_ (allocating a copy when partitionLabel is provided). [3] - Checks esp_littlefs_mounted(partitionLabel_) and returns true if already mounted. [3] - Builds an esp_vfs_littlefs_conf_t with.base_path and.partition_label and calls esp_vfs_littlefs_register(&conf). [3] - If esp_vfs_littlefs_register fails with ESP_FAIL and formatOnFail is true, it calls format and then retries esp_vfs_littlefs_register. [3] - On success it sets the mountpoint to basePath via _impl->mountpoint(basePath) and returns true. [3] - Returns false on mount/register failure. [3] 3) Global LittleFS instance (what you call in sketches) - The header declares an extern global instance: - extern fs::LittleFSFS LittleFS; so sketches use LittleFS.begin(...). [2] 4) How begin is called in practice (example sketch) - In the LittleFS_test.ino example, setup calls begin on the global LittleFS instance, e.g.: - if (!LittleFS.begin(FORMAT_LITTLEFS_IF_FAILED)) {... } and in the TWOPART case it calls begin(...) with arguments including basePath and partition label. [4] Example usage pattern (Arduino sketch) - Include the header and call the global instance: - #include <LittleFS.h> - if (!LittleFS.begin(/formatOnFail/ true)) { /handle error/ } This matches the example’s use of the global LittleFS instance. [4]
Citations:
- 1: https://github.com/espressif/arduino-esp32/blob/7ff99fc4/libraries/LittleFS/src/LittleFS.h
- 2: https://github.com/espressif/arduino-esp32/blob/master/libraries/LittleFS/src/LittleFS.h
- 3: https://github.com/espressif/arduino-esp32/blob/7ff99fc4/libraries/LittleFS/src/LittleFS.cpp
- 4: https://github.com/espressif/arduino-esp32/blob/master/libraries/LittleFS/examples/LITTLEFS_test/LITTLEFS_test.ino
Route the SPIFFS compatibility shim to the mounted WLED LittleFS instance.
begin() mounts the LittleFS global, but these headers define a separate SPIFFS object. The extra object stays unmounted, so compatibility libraries can fail on file operations even after the mounted LittleFS filesystem appears ready. Keep both compatibility headers’ SPIFFS definition as an alias or external reference to the same mounted LittleFS instance, or mount this exact shim object before use.
📍 Affects 2 files
include/SPIFFS.h#L5-L12(this comment)lib/compat_spiffs/include/SPIFFS.h#L5-L12
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@include/SPIFFS.h` around lines 5 - 12, Update the SPIFFS definitions in
include/SPIFFS.h lines 5-12 and lib/compat_spiffs/include/SPIFFS.h lines 5-12 so
SPIFFS aliases or references the mounted global LittleFS instance used by
begin(), rather than creating a separate fs::LittleFSFS object; alternatively,
ensure this exact shim object is mounted before compatibility-library file
operations.
Source: MCP tools
| @@ -0,0 +1,120 @@ | |||
| # GC9A01 Display Usermod | |||
|
|
|||
| A fully-featured WLED usermod providing comprehensive visual interface on GC9A01 240x240 round TFT displays with complete rotary encoder integration. | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use “fully featured”.
Replace fully-featured with fully featured in the opening sentence.
🧰 Tools
🪛 LanguageTool
[grammar] ~3-~3: Ensure spelling is correct
Context: ... Display Usermod A fully-featured WLED usermod providing comprehensive visual interface on GC9A0...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@usermods/usermod_v2_gc9a01_display/readme.md` at line 3, Update the opening
description to replace “fully-featured” with “fully featured,” preserving the
rest of the sentence unchanged.
Source: Linters/SAST tools
| build_flags = ${common.build_flags} ${esp32_idf_V5.build_flags} | ||
| -Iinclude | ||
| -D WLED_DEBUG | ||
| -D WLED_DISABLE_BROWNOUT_DET |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg 'usermods/.*/readme\.md|usermods/.*/.*\.md|usermods/usermod_v2_gc9a01_display/readme.md|CHANGELOG.md|platformio|wled\.h|Brownout|BROWNOUT|DISABLE_BROWNOUT|brownout' || true
echo
echo "Target README context:"
if [ -f usermods/usermod_v2_gc9a01_display/readme.md ]; then
nl -ba usermods/usermod_v2_gc9a01_display/readme.md | sed -n '1,130p'
fi
echo
echo "Repository usages and docs for brownout flag:"
rg -n "WLED_DISABLE_BROWNOUT_DET|DISABLE_BROWNOUT|brownout|BROWNOUT" . -S || trueRepository: wled/WLED
Length of output: 3322
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Target README around setup block:"
awk 'NR>=1 && NR<=130 { printf "%5d\t%s\n", NR, $0 }' usermods/usermod_v2_gc9a01_display/readme.md
echo
echo "Definition/usages of brownout flag:"
rg -n "WLED_DISABLE_BROWNOUT_DET|DEFAULT_BROWNOUT|DISABLE_BROWN" -S . || true
echo
echo "Relevant wled.h snippets:"
rg -n -C 3 "WLED_DISABLE_BROWNOUT_DET|DEFAULT_BROWNOUT|brownout|BROWNOUT" wled00/wled.h platformio*.ini platformio_override.sample.ini usermods/platformio_override.usermods.ini CHANGELOG.md -S || trueRepository: wled/WLED
Length of output: 11970
Do not include WLED_DISABLE_BROWNOUT_DET in the generic setup.
This flag disables the ESP32 brownout detector and is off by default. Add it only for environments with known power-supply issues, and include a board-specific warning.
Proposed fix
- -D WLED_DISABLE_BROWNOUT_DET📝 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.
| -D WLED_DISABLE_BROWNOUT_DET |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@usermods/usermod_v2_gc9a01_display/readme.md` at line 73, Remove
WLED_DISABLE_BROWNOUT_DET from the generic setup instructions in the readme.
Document it only in the board- or environment-specific setup section for known
power-supply issues, including a warning that it disables the ESP32 brownout
detector.
Sources: Path instructions, MCP tools
| if (activeOverlayMode >= 0) { | ||
| if (now >= overlayUntil) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle millis() rollover in the timeout comparisons.
The code compares absolute timestamps: now >= overlayUntil, now < nextUpdate, and millis() < overlayUntil. millis() wraps to 0 after about 49.7 days. After a wrap, overlayUntil and nextUpdate stay large. The overlay then never expires and loop() stops calling redraw().
Compare the difference instead. The subtraction of unsigned values is rollover-safe.
🐛 Proposed fix pattern
- if (now >= overlayUntil) {
+ if ((long)(now - overlayUntil) >= 0) {- if (now < nextUpdate) return;
+ if ((long)(now - nextUpdate) < 0) return; bool UsermodGC9A01Display::isOverlayActive() {
- return (activeOverlayMode >= 0 && millis() < overlayUntil);
+ return (activeOverlayMode >= 0 && (long)(millis() - overlayUntil) < 0);
}
int UsermodGC9A01Display::getActiveOverlayMode() {
- if (millis() >= overlayUntil) {
+ if ((long)(millis() - overlayUntil) >= 0) {
return -1; // Overlay has expired
}
return activeOverlayMode;
}Also applies to: 857-859, 958-967
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp` around
lines 62 - 63, Replace absolute timestamp comparisons in the overlay timeout and
update scheduling logic, including the blocks around activeOverlayMode and the
referenced redraw paths, with unsigned elapsed-time subtraction comparisons.
Update checks involving now, overlayUntil, nextUpdate, and millis() so behavior
remains correct across millis() rollover, preserving the existing expiration and
redraw conditions.
| overlayUntil = 0; | ||
| forceRedraw = true; | ||
| needRedraw = true; // Force redraw to show main screen | ||
| lastRedraw = now; // Reset sleep timer when returning to main screen |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The lastRedraw sentinel comment does not match the behavior.
The header states that lastRedraw = ULONG_MAX makes "the sleep timer start after first interaction". setup() overwrites lastRedraw with millis() at line 845. The sentinel is therefore never active at boot, and the display sleeps after displayTimeout without any interaction.
Decide on one behavior. If the display must sleep after boot, remove the sentinel and the lastRedraw != ULONG_MAX guard. If it must wait for an interaction, remove the assignment in setup().
As per path instructions: "VERIFY comments match code behavior - AI frequently generates plausible but incorrect comments."
Also applies to: 150-150, 845-845
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp` at line 70,
Resolve the mismatch between the lastRedraw sentinel behavior and setup
initialization: choose whether the display sleeps after boot or waits for first
interaction. For sleep-after-boot, remove the ULONG_MAX sentinel usage and
corresponding lastRedraw != ULONG_MAX guard; for wait-for-interaction, remove
the lastRedraw = millis() assignment in setup(). Update the related comments to
describe the selected behavior accurately.
Source: Path instructions
| // Load backlight setting (0-100% range) | ||
| uint8_t newBacklight = top[FPSTR("backlight")] | backlight; | ||
| if (newBacklight != backlight) { | ||
| setBacklight(newBacklight); | ||
| } | ||
|
|
||
| return true; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not drive the backlight pin from readFromConfig(), and report missing keys.
Two problems exist here.
First, WLED calls readFromConfig() before setup(). setBacklight() calls analogWrite(TFT_BL, pwm) at that time. initDisplay() has not yet run pinMode(TFT_BL, OUTPUT). Store the value and apply it in initDisplay().
Second, the function returns true unconditionally. WLED uses the return value to decide whether the stored configuration is complete. If a key is missing after an upgrade, the config is not re-saved with defaults. Return the presence check of the newest key.
🐛 Proposed fix
// Load backlight setting (0-100% range)
- uint8_t newBacklight = top[FPSTR("backlight")] | backlight;
- if (newBacklight != backlight) {
- setBacklight(newBacklight);
- }
-
- return true;
+ uint8_t newBacklight = top[FPSTR(_gcBacklight)] | backlight;
+ if (initDone && newBacklight != backlight) setBacklight(newBacklight);
+ else backlight = min(newBacklight, (uint8_t)100);
+
+ return !top[FPSTR(_gcBacklight)].isNull();initDone needs to be added as a member and set at the end of setup().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.cpp` around
lines 912 - 918, Update readFromConfig() so it stores the backlight value
without calling setBacklight() before setup, and return the presence check for
the newest configuration key instead of true. Add the requested initDone member,
set it at the end of setup(), and have initDisplay() apply the stored backlight
only after TFT_BL has been configured as an output.
| #ifndef TFT_BL | ||
| #define TFT_BL 4 // Default fallback if not defined by TFT_eSPI | ||
| #endif |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not force a default backlight pin.
If TFT_eSPI does not define TFT_BL, the code still defines it as GPIO4. initDisplay() then calls pinMode(TFT_BL, OUTPUT) and digitalWrite(TFT_BL, HIGH) on a pin that the user did not assign. GPIO4 may be used by LED output or another usermod.
The usermod also never registers the pin with PinManager. WLED cannot detect the conflict.
Remove the fallback and guard all backlight code with the real TFT_BL definition. Allocate the pin with PinManager::allocatePin(TFT_BL, true, PinOwner::UM_Unspecified) in setup().
🐛 Proposed fix
-// Use TFT_eSPI's backlight pin definition
-#ifndef TFT_BL
- `#define` TFT_BL 4 // Default fallback if not defined by TFT_eSPI
-#endif
+// Backlight pin comes from the TFT_eSPI configuration only.
+// If TFT_BL is not defined, backlight control is disabled.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.h` around lines
26 - 28, Remove the fallback TFT_BL definition and guard all backlight pin
operations in initDisplay() and related code so they compile and run only when
TFT_eSPI provides TFT_BL. In setup(), register the defined backlight pin with
PinManager using allocatePin(TFT_BL, true, PinOwner::UM_Unspecified) before
configuring or driving it.
| #ifndef USERMOD_ID_GC9A01_DISPLAY | ||
| #define USERMOD_ID_GC9A01_DISPLAY 59 // Use the official ID from const.h | ||
| #endif |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the usermod ID is declared in const.h and whether 59 is already used.
fd -t f 'const.h' wled00 --exec rg -n 'USERMOD_ID_' {} \; | rg -n '\b59\b|GC9A01'Repository: wled/WLED
Length of output: 147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant files =="
fd -t f 'const.h|usermod_v2_gc9a01_display.h|UsermodManager|rotary encoder' . | sed -n '1,120p'
echo
echo "== const.h usermod IDs relevant section =="
if [ -f wled00/const.h ]; then
rg -n "USERMOD_ID_|USERMOD_COUNT|MAX_USERMODS|UsermodManager|usermod" wled00/const.h | sed -n '1,220p'
else
echo "wled00/const.h not found"
fi
echo
echo "== all repository references to USERMOD_ID_GC9A01_DISPLAY and ID 59 =="
rg -n 'USERMOD_ID_GC9A01_DISPLAY|\bUSERMOD_ID_[A-Za-z0-9_]*\s+\{\s*59\s*\}|\bUSERMOD_ID_[A-Za-z0-9_]*\s+(=|:|,)\s*59\b|\b59\b' . \
--glob '!build/**' --glob '!bin/**' --glob '!docs/**' | sed -n '1,240p'
echo
echo "== usermod files =="
git ls-files | rg 'usermods/.*usermod_v2_gc9a01_display|wled00/const.h|usermod.*\.h|UsermodManager' | sed -n '1,200p'Repository: wled/WLED
Length of output: 38066
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== UsermodManager declarations/definitions =="
rg -n "class UsermodManager|lookup\\(|find\\(|getId\\(|_usermods|UMData|getUMData" wled00 | sed -n '1,260p'
echo
echo "== IDs around USERMOD_ID_USER_FX and next available IDs =="
sed -n '248,258p' wled00/const.h
echo
echo "== usermod getId returns and const.h mapping =="
python3 - <<'PY'
import pathlib, re
const = pathlib.Path('wled00/const.h').read_text()
m = re.findall(r'^`#define`\s+(USERMOD_ID_[A-Za-z0-9_]+)\s+(\d+)\s*//\s*Usermod "(.*?)"',$ const, re.M)
ids = {macro:int(value) for macro, value, _ in m}
rev = {value:macro for macro, value in m}
print("USERMOD_ID_GC9A01_DISPLAY in const.h:", "USERMOD_ID_GC9A01_DISPLAY" in ids)
print("ID 59 used by const.h usermod:", rev.get(59))
for p in pathlib.Path('usermods').rglob('*.h'):
text = p.read_text(errors='ignore')
if 'getId()' in text and re.search(r'return\s+USERMOD_ID_', text):
for macro in re.findall(r'USERMOD_ID_[A-Za-z0-9_]+', text):
print(f"{p}: {macro}", "const.h value=", ids.get(macro, "missing"))
PYRepository: wled/WLED
Length of output: 14512
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== UsermodManager relevant source =="
sed -n '24,92p' wled00/um_manager.cpp
sed -n '368,390p' wled00/fcn_declare.h
sed -n '398,428p' wled00/fcn_declare.h
echo
echo "== const.h IDs section =="
sed -n '246,257p' wled00/const.h
echo
echo "== parse USERMOD IDs and usermod references =="
python3 - <<'PY'
import pathlib, re
usermod_re = re.compile(r'^`#define`\s+(USERMOD_ID_[A-Za-z0-9_]+)\s+(\d+)\s*//\s*Usermod "(.*?)"')
ids = {}
for line in pathlib.Path('wled00/const.h').read_text().splitlines():
m = usermod_re.match(line)
if m:
base = int(m.group(2))
mac = m.group(1)
ids[mac] = base
ids[base] = mac
for name in ['USERMOD_ID_GC9A01_DISPLAY', 59]:
print("USERMOD_ID_GC9A01_DISPLAY defined?", "USERMOD_ID_GC9A01_DISPLAY" in ids)
print("ID 59 base const macro", ids.get(59))
roots = ['usermods', 'wled00']
for root in roots:
print(f"\nReferences to USERMOD_ID_GC9A01_DISPLAY under {root}:")
for p in pathlib.Path(root).rglob('*'):
if p.is_file() and p.suffix in {'.h','.hpp','.cpp','.ino'}:
txt = p.read_text(errors='ignore')
if 'USERMOD_ID_GC9A01_DISPLAY' in txt:
for i, line in enumerate(txt.splitlines(), 1):
if 'USERMOD_ID_GC9A01_DISPLAY' in line:
print(f"{p}:{i}: {line.strip()}")
PYRepository: wled/WLED
Length of output: 8972
Add USERMOD_ID_GC9A01_DISPLAY to wled00/const.h and remove the local fallback.
It is only defined under usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.h, while other usermods register their official IDs in wled00/const.h. ID 59 is not currently used by any registered usermod, but the local fallback does not create a shared official entry.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@usermods/usermod_v2_gc9a01_display/usermod_v2_gc9a01_display.h` around lines
30 - 32, Add USERMOD_ID_GC9A01_DISPLAY with value 59 to the shared usermod ID
definitions in const.h, then remove the local `#ifndef/`#define fallback from the
usermod_v2_gc9a01_display header so it uses the centralized definition.
Source: Coding guidelines
| // Check if GC9A01 overlay has expired or display is asleep and reset to brightness mode (state 0) | ||
| #ifdef USERMOD_GC9A01_DISPLAY | ||
| if (gc9a01Display != nullptr && select_state > 0) { | ||
| if (!gc9a01Display->isOverlayActive() || gc9a01Display->isDisplayAsleep()) { | ||
| // Overlay has expired or display is asleep, return to brightness mode | ||
| select_state = 0; | ||
| } | ||
| } | ||
| #endif |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The state reset can discard a state selection after 750 ms.
changeState() calls gc9a01Display->overlay(stateName, 750, glyph). isOverlayActive() returns true only while millis() < overlayUntil. This block runs every 2 ms.
If the user presses the button to select "Effect" and does not rotate the encoder within 750 ms, select_state resets to 0. The next rotation changes brightness instead of the effect. Users of the Four Line Display do not experience this reset.
updateRedrawTime() extends overlayUntil by 3000 ms, but only after the first rotation.
Use a dedicated inactivity timer in the rotary usermod, or align the overlay duration in changeState() with the intended selection timeout.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp`
around lines 678 - 686, The GC9A01-specific reset block around select_state
currently treats the 750 ms overlay expiry as selection expiry, so an untouched
Effect selection falls back to brightness. Replace this dependency with a
dedicated rotary-user-mod inactivity timeout that starts or refreshes when
changeState() selects a state and only resets select_state after the intended
timeout; alternatively, update changeState() so the overlay duration matches
that selection timeout. Preserve the existing asleep-display reset behavior.
| char brightnessStr[16]; | ||
| sprintf(brightnessStr, "Brightness %d%%", (bri * 100) / 255); | ||
| gc9a01Display->overlay(brightnessStr, 500, 10); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
brightnessStr has no spare capacity.
"Brightness 100%" is 15 characters. With the terminator it needs exactly 16 bytes. The buffer is 16 bytes. Any change to the format string overflows the buffer.
Use snprintf and state the bound.
🐛 Proposed fix
- char brightnessStr[16];
- sprintf(brightnessStr, "Brightness %d%%", (bri * 100) / 255);
+ char brightnessStr[20];
+ snprintf(brightnessStr, sizeof(brightnessStr), "Brightness %d%%", (bri * 100) / 255);📝 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.
| char brightnessStr[16]; | |
| sprintf(brightnessStr, "Brightness %d%%", (bri * 100) / 255); | |
| gc9a01Display->overlay(brightnessStr, 500, 10); | |
| char brightnessStr[20]; | |
| snprintf(brightnessStr, sizeof(brightnessStr), "Brightness %d%%", (bri * 100) / 255); | |
| gc9a01Display->overlay(brightnessStr, 500, 10); |
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 850-850: Use of an unbounded buffer function that can overflow the destination; use a size-bounded equivalent (fgets, strncpy/strlcpy, strncat/strlcat, snprintf).
Context: sprintf(brightnessStr, "Brightness %d%%", (bri * 100) / 255)
Note: [CWE-120] Buffer Copy without Checking Size of Input ('Classic Buffer Overflow').
(dangerous-buffer-functions-cpp)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cpp`
around lines 850 - 852, Replace sprintf in the brightness display code with
snprintf, passing the actual brightnessStr capacity (sizeof(brightnessStr)) as
the bound while preserving the existing format and overlay behavior.
Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/compat_spiffs/src/SPIFFS.h`:
- Line 9: Indent the SPIFFSFS alias declaration by two spaces to match the
required C++ header namespace style.
- Line 12: Update the SPIFFS compatibility alias to reuse the existing LittleFS
singleton instead of constructing a new LittleFSFS instance; preserve the legacy
SPIFFS symbol while ensuring filesystem lifecycle calls operate on the same
LittleFS object.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: 88bed582-2911-4652-84b2-dfb1c324dac3
📒 Files selected for processing (1)
lib/compat_spiffs/src/SPIFFS.h
| // Placed in lib/compat_spiffs/src so PlatformIO adds it to the include path | ||
| // automatically when scanning libraries. | ||
| namespace fs { | ||
| using SPIFFSFS = LittleFSFS; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Apply the required two-space indentation.
Line 9 places the namespace member at column 1.
-using SPIFFSFS = LittleFSFS;
+ using SPIFFSFS = LittleFSFS;As per path instructions, C++ headers must use 2-space indentation and match the existing C++ style.
📝 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.
| using SPIFFSFS = LittleFSFS; | |
| using SPIFFSFS = LittleFSFS; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/compat_spiffs/src/SPIFFS.h` at line 9, Indent the SPIFFSFS alias
declaration by two spaces to match the required C++ header namespace style.
Source: Path instructions
| using SPIFFSFS = LittleFSFS; | ||
| } | ||
|
|
||
| inline fs::LittleFSFS SPIFFS; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --glob '*.{cpp,h,hpp,ino}' \
'\b(SPIFFS|LittleFS)[[:space:]]*\.[[:space:]]*(begin|end|open|format|exists|remove|rename|mkdir|rmdir)[[:space:]]*\(' .Repository: wled/WLED
Length of output: 147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)SPIFFS\.h$|LittleFS|WLED\.ino$' || true
echo "== SPIFFS.h if present =="
if [ -f lib/compat_spiffs/src/SPIFFS.h ]; then
cat -n lib/compat_spiffs/src/SPIFFS.h
fi
echo "== references to SPIFFS/LittleFS symbols =="
rg -n '\b(SPIFFS|LittleFS)\b' -g '*.{cpp,h,hpp,ino,c}' . || true
echo "== filesystem lifecycle references =="
rg -n '\b(SPIFFS|LittleFS)[[:space:]]*\.[[:space:]]*(begin|end|open|format|exists|remove|rename|mkdir|rmdir|fs|size|free)\s*\(' . || trueRepository: wled/WLED
Length of output: 2499
🌐 Web query:
Arduino ESP32 LittleFS LittleFSFS begin mount partition returns true mounted without assigning mount point
💡 Result:
In the Arduino ESP32 environment, the LittleFS begin() method provides default values for its parameters, which explains why it can return true even when you do not explicitly assign a mount point [1][2]. The signature of the begin method is typically: bool begin(bool formatOnFail = false, const char *basePath = "/littlefs", uint8_t maxOpenFiles = 10, const char *partitionLabel = "littlefs") When you call LittleFS.begin() without arguments: 1. It uses the default base path (mount point) of "/littlefs" [1]. 2. It attempts to mount the partition labeled "littlefs" [1]. If your call returns true, the filesystem has been successfully mounted at the default location (/littlefs). If you were expecting it to mount elsewhere or if you have a custom partition setup, you must explicitly provide the basePath and partitionLabel parameters [1][3][4]. Key considerations: - Default Behavior: If you do not specify parameters, the library defaults to "/littlefs" as the mount point [1]. This is why code often works "without" visible assignment; it is assigned automatically by the library's default arguments [2]. - Partition Table: LittleFS requires a partition of the correct type (often defined as spiffs or littlefs in your partitions.csv file) [5][6][7]. If the library cannot find a partition with the specified label (defaulting to "littlefs"), mounting will fail and return false [1][4]. - Legacy/Third-Party Libraries: Note that if you are using the older lorol/LITTLEFS library instead of the built-in core library, some behaviors regarding default parameters and labels may vary, though the core espressif/arduino-esp32 implementation follows the standard described above [1][8][9]. To ensure explicit control, always define your parameters: if (!LittleFS.begin(true, "/your_mount_point", 10, "your_partition_label")) { ... } [4]
Citations:
- 1: https://github.com/espressif/arduino-esp32/blob/7ff99fc4/libraries/LittleFS/src/LittleFS.cpp
- 2: https://randomnerdtutorials.com/esp32-write-data-littlefs-arduino/
- 3: https://github.com/espressif/arduino-esp32/blob/master/libraries/LittleFS/examples/LITTLEFS_test/LITTLEFS_test.ino
- 4: https://community.platformio.org/t/how-to-mount-littlefs-to-esp32-wroom-chip-using-platformio/45161
- 5: https://forum.arduino.cc/t/esp32-littlefs-mounting-failure/1447713
- 6: Clarify the way LittleFS is meant to be used with partition schemes. espressif/arduino-esp32#10253
- 7: Unable to create a LittleFS partition with default or custom ESP32 partition schemes when 32MB flash installed espressif/arduino-esp32#8362
- 8: https://github.com/lorol/LITTLEFS/
- 9: https://github.com/lorol/LITTLEFS/blob/1.0.5/README.md
🌐 Web query:
espressif arduino-esp32 LittleFS.cpp LittleFSFS begin _mount return true
💡 Result:
In the espressif/arduino-esp32 framework, the LittleFSFS::begin method returns true when the LittleFS partition is successfully mounted or if it is already mounted [1][2]. The specific implementation logic in LittleFS.cpp functions as follows: 1. Check for existing mount: The method first calls esp_littlefs_mounted(partitionLabel_) [1][2]. If this returns true, the filesystem is already accessible, and the function immediately returns true [1][2]. 2. Registration: If not already mounted, it prepares an esp_vfs_littlefs_conf_t configuration structure and attempts to register the filesystem using esp_vfs_littlefs_register(&conf) [1][2]. 3. Optional Formatting: If the registration fails (ESP_FAIL) and formatOnFail is set to true, it attempts to format the partition [1][2]. If formatting succeeds, it tries to register the filesystem again [1][2]. 4. Success Criteria: If registration is successful (err == ESP_OK), the method sets the internal mount point path via _impl->mountpoint(basePath) and returns true [1][2]. If the registration fails and cannot be recovered by formatting, it logs an error and returns false [1][2]. The return value indicates that the VFS (Virtual File System) layer has successfully associated the LittleFS partition with the specified base path [1][2].
Citations:
- 1: https://github.com/espressif/arduino-esp32/blob/7ff99fc4/libraries/LittleFS/src/LittleFS.cpp
- 2: https://github.com/lorol/LITTLEFS/blob/master/src/LITTLEFS.cpp
🌐 Web query:
espressif arduino-esp32 LittleFS.cpp LittleFSFS end esp_littlefs_unmount unregister partition _impl mountpoint
💡 Result:
In the arduino-esp32 framework, the LittleFS.end method is responsible for properly unmounting and unregistering the LittleFS filesystem from the Virtual File System (VFS) layer [1]. When LittleFS.end is called, the following process occurs: 1. Verification: It checks if the filesystem is currently mounted by calling esp_littlefs_mounted(partitionLabel_) [1][2]. 2. Unregistration: If mounted, it calls esp_vfs_littlefs_unregister(partitionLabel_) to unregister the partition [1][2]. 3. VFS Update: Internally, esp_vfs_littlefs_unregister handles unmounting the underlying filesystem and freeing the associated resources [3][4]. 4. Cleanup: After successfully unregistering, it calls _impl->mountpoint(NULL) to clear the mountpoint reference in the filesystem implementation wrapper [1][2]. This ensures that the partition is no longer associated with a VFS path, allowing it to be re-mounted later if needed [1][4].
Citations:
- 1: https://github.com/espressif/arduino-esp32/blob/7ff99fc4/libraries/LittleFS/src/LittleFS.cpp
- 2: https://git.liberatedsystems.co.uk/jacob.eva/arduino-esp32/src/commit/f3dca15a6f9e79b360e0166637ebbd105dfc8379/libraries/LITTLEFS/src/LITTLEFS.cpp
- 3: https://github.com/lorol/LITTLEFS/blob/master/src/esp_littlefs.c
- 4: https://github.com/lorol/LITTLEFS/blob/master/src/esp_littlefs.h
🌐 Web query:
espressif arduino-esp32 LittleFS.cpp LittleFSFS end existing mount another begin false
💡 Result:
If you’re referring to the Arduino-ESP32 LittleFS implementation, the behavior is: 1) LittleFSFS::begin will do nothing harmful if the same partition is already mounted. - In LittleFSFS::begin, arduino-esp32 checks esp_littlefs_mounted(partitionLabel_) and, if already mounted, logs “LittleFS Already Mounted!” and returns true immediately [1]. - That means you do NOT need (and should not do) an explicit “end; begin;” just to re-mount the same partition; begin is already idempotent for an already-mounted partition [1]. 2) LittleFSFS::end unmounts only if the partition is currently mounted. - end checks esp_littlefs_mounted(partitionLabel_) and, if mounted, calls esp_vfs_littlefs_unregister(partitionLabel_) and then clears the stored mountpoint [1]. 3) “end existing mount another begin false” / “begin false” in practice: - The “begin” parameter “formatOnFail” controls only whether formatting is attempted if mount fails; it is set via LittleFSFS::begin(bool formatOnFail,...) and the code passes.format_if_mount_failed = false in the esp_vfs_littlefs_conf_t, but then conditionally retries with format only when formatOnFail is true [1]. - Therefore, calling LittleFS.begin(false) (i.e., formatOnFail=false) will NOT format on mount failure; mount will fail and return false instead [1]. 4) Mounting a different LittleFS partition (different partitionLabel) can coexist with “another begin”. - The implementation supports specifying a partitionLabel; begin stores/updates partitionLabel_ and uses it for the mounted-check and registration [1]. - The official LittleFS_test example shows mounting a second partition by calling LittleFS.begin(..., basePath, maxOpenFiles, "part2") and then LittleFS.end when done, before using the first partition again [2]. What to do in your situation - If you just want to ensure a single mount: call LittleFS.begin(false) once at startup. If it’s already mounted, begin will just return true [1]. - If you need to switch to a different partition/basePath: call LittleFS.end first (so the previous partition is unregistered), then call LittleFS.begin(false, newBasePath, newMaxOpenFiles, newPartitionLabel) [1][2]. - If begin(false) fails: because formatOnFail is false, it will not format; you must either repair/format externally or call begin(true) / format intentionally [1]. Code sketch (switch partitions) - end previous mount, then mount another without formatting on fail: LittleFS.end; bool ok = LittleFS.begin(false, "/lfs2", 5, "part2"); If you tell me the exact call sequence you’re using (the basePath + partitionLabel values and whether you expect re-mounting same vs different partition), I can map it precisely to the code paths above.
Citations:
- 1: https://github.com/espressif/arduino-esp32/blob/7ff99fc4/libraries/LittleFS/src/LittleFS.cpp
- 2: https://github.com/espressif/arduino-esp32/blob/master/libraries/LittleFS/examples/LITTLEFS_test/LITTLEFS_test.ino
Reuse the existing LittleFS singleton.
This compatibility shim currently constructs a duplicate LittleFSFS wrapper with default mount parameters. A legacy SPIFFS shim is useful, but avoid exposing a second instance that can interfere with the same LittleFS partition when filesystem lifecycle calls are used.
Suggested fix
-inline fs::LittleFSFS SPIFFS;
+#define SPIFFS LittleFS📝 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.
| inline fs::LittleFSFS SPIFFS; | |
| `#define` SPIFFS LittleFS |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/compat_spiffs/src/SPIFFS.h` at line 12, Update the SPIFFS compatibility
alias to reuse the existing LittleFS singleton instead of constructing a new
LittleFSFS instance; preserve the legacy SPIFFS symbol while ensuring filesystem
lifecycle calls operate on the same LittleFS object.
1 participant
Summary by CodeRabbit
New Features
Documentation