Add trailing strip support for sparse ledmaps#5745
Conversation
Walkthrough2D gap maps now load through direct file streaming. Custom LED-map parsing reallocates tables for sparse and trailing pixels, while segment validation uses the expanded mapped length and checks trailing 1D coordinates. ChangesCustom mapping and gap loading
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant setUpMatrix as WS2812FX::setUpMatrix
participant filesystem as WLED_FS
participant parser as readNextIntFromFile
participant gapTable
setUpMatrix->>filesystem: Open /2d-gaps.json
setUpMatrix->>parser: Read matrix gap integers
parser-->>setUpMatrix: Return parsed integer
setUpMatrix->>gapTable: Store constrained gap value
setUpMatrix->>filesystem: Close file
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🧹 Nitpick comments (1)
wled00/file.cpp (1)
366-407: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid, well-guarded parser — consider adding unit tests.
Logic checks out: overflow-safe saturation, correct consume/leave semantics for
,/], and graceful handling of malformed trailing characters. Given this new parser now backs both gap-file and ledmap loading (wled00/FX_2Dfcn.cpp,wled00/FX_fcn.cpp), a focused unit test covering leading whitespace, negative numbers, overflow, and malformed trailing chars would guard against regressions.🤖 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 `@wled00/file.cpp` around lines 366 - 407, Add focused unit tests for readNextIntFromFile covering leading whitespace, negative values, oversized-number saturation, and malformed trailing characters. Verify the parser’s comma consumption and array-terminator behavior while exercising the same loading paths used by gap files and ledmaps.
🤖 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 `@wled00/FX_fcn.cpp`:
- Around line 2227-2251: The 1D ledmap loader uses an overly strict token match
that rejects whitespace-formatted map arrays. In the 1D setup branch, update the
f.find check guarding map parsing to search for the same "\"map\"" token used by
the 2D path, while leaving the subsequent integer parsing unchanged.
---
Nitpick comments:
In `@wled00/file.cpp`:
- Around line 366-407: Add focused unit tests for readNextIntFromFile covering
leading whitespace, negative values, oversized-number saturation, and malformed
trailing characters. Verify the parser’s comma consumption and array-terminator
behavior while exercising the same loading paths used by gap files and ledmaps.
🪄 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: CHILL
Plan: Pro
Run ID: 46594de5-ded4-453c-b01d-2dc48a61ce5e
📒 Files selected for processing (5)
wled00/FX_2Dfcn.cppwled00/FX_fcn.cppwled00/const.hwled00/fcn_declare.hwled00/file.cpp
| } else { | ||
| // 1D set-up: allocate strip length and fill with entries from file | ||
| // partial maps leave indices beyond customMappingSize unmapped (-1) TODO: see note above about appending unmapped pixels | ||
| const unsigned mapSize = getLengthTotal(); | ||
| customMappingTable = static_cast<uint16_t*>(d_malloc(sizeof(uint16_t) * mapSize)); // prefer DRAM for speed | ||
|
|
||
| if (customMappingTable) { | ||
| memset(customMappingTable, 0xFF, sizeof(uint16_t) * mapSize); // pre-fill with "-1" i.e. unmapped pixel | ||
| DEBUG_PRINTF_P(PSTR("ledmap allocated: %uB\n"), sizeof(uint16_t)*mapSize); | ||
| File f = WLED_FS.open(fileName, "r"); | ||
| if (f && f.find("\"map\":[")) { | ||
| int value; | ||
| unsigned mapindex = 0; | ||
| while (mapindex < mapSize && readNextIntFromFile(f, value)) { | ||
| if (value < 0 || value >= _length) value = 0xFFFF; // prevent integer wrap around | ||
| customMappingTable[mapindex++] = (uint16_t)value; | ||
| } | ||
| customMappingSize = mapSize; | ||
| currentLedmap = n; | ||
| f.close(); | ||
| } | ||
| } else { | ||
| DEBUG_PRINTLN(F("ERROR LED map allocation error.")); | ||
| } | ||
| currentLedmap = n; | ||
| f.close(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the 2D branch's find() target and close() placement for comparison
rg -n 'f\.find\("\\"map' wled00/FX_fcn.cpp
rg -n 'f\.close\(\)' wled00/FX_fcn.cppRepository: wled/WLED
Length of output: 427
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant sections around both branches.
sed -n '2160,2255p' wled00/FX_fcn.cpp
printf '\n--- search for File/FS close patterns ---\n'
rg -n 'File f = WLED_FS\.open|\.close\(\); // all done, close the file|if \(f && f\.find\("\\"map"\\"\)\)' wled00 -g'*.cpp' -g'*.h'
printf '\n--- look for documentation about File destructor / close semantics ---\n'
rg -n 'destructor|close\(\)|File' docs wled00 -g'*.md' -g'*.cpp' -g'*.h' | sed -n '1,120p'Repository: wled/WLED
Length of output: 15449
🌐 Web query:
ESP32 Arduino FS File destructor close semantics File destructor closes handle automatically
💡 Result:
In the ESP32 Arduino framework, the fs::File class uses reference counting to manage its underlying file handle [1]. When a File object goes out of scope, its internal shared pointer (_p) is destroyed [1][2]. The underlying implementation class (e.g., VFSFileImpl or similar) is designed to release its resources—including closing the file handle—when its own destructor runs [1][3]. Consequently, when the last File object referencing a specific handle is destroyed, the file is automatically closed [1]. While this automatic mechanism exists, it is widely considered best practice in embedded development to explicitly call File::close() when you are finished with a file [1][4]. Explicitly calling close() ensures that: 1. Any buffered data is immediately flushed to the storage medium. 2. The file handle and associated memory are released immediately, rather than waiting for the object to go out of scope or for the reference count to reach zero [4]. 3. You avoid potential issues related to delayed resource reclamation in memory-constrained environments [4]. In summary, while the destructor provides a safety mechanism for automatic closure [1], relying on it is not a substitute for explicit resource management via close() in robust application code [4].
Citations:
- 1: Automatically close File once it is out-of-scope? espressif/arduino-esp32#7614
- 2: https://github.com/espressif/arduino-esp32/blob/7ff99fc4/libraries/FS/src/FS.h
- 3: https://github.com/espressif/arduino-esp32/blob/7ff99fc4/libraries/FS/src/vfs_api.cpp
- 4: Why can't you close the file handle opened with SPIFFS.open("/") without doing damage? espressif/arduino-esp32#4727
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- occurrences of the ledmap JSON key in the repo ---\n'
rg -n '"map"\s*:|\\"map\\":\[' wled00 docs -g'*.cpp' -g'*.h' -g'*.json' -g'*.md'
printf '\n--- surrounding code where ledmap files are written/serialized ---\n'
rg -n 'customMappingTable|customMappingSize|currentLedmap|ledmap|map\]' wled00 -g'*.cpp' -g'*.h' | sed -n '1,160p'Repository: wled/WLED
Length of output: 238
🌐 Web query:
Arduino FS File destructor close() automatically scope exit ESP32 ESP8266
💡 Result:
In the ESP32 and ESP8266 Arduino cores, the fs::File object is designed to manage its own lifecycle [1][2][3]. When a File object goes out of scope, its destructor is invoked, which eventually triggers the closure of the underlying file handle [1][2]. For the ESP32, the File class uses a reference-counted pointer to the underlying implementation class (FileImpl) [1]. When the File object is destroyed, the reference count decreases, and the implementation’s destructor handles the actual closing of the file [1][4]. For the ESP8266, the underlying implementation similarly ensures that files are closed when the File object is destroyed [2][3]. While official documentation explicitly lists close as a method to call [5], and doing so manually is considered good practice for predictable resource management, the architectural design of these cores ensures that files are not left permanently open (leaked) when the object goes out of scope [1][6][2]. In summary, while you are not strictly required to call close to prevent leaks, it remains a best practice to call close manually if you need to ensure the file is closed at a specific point in your code, such as before further processing the data or to immediately free memory resources [6][3][5].
Citations:
- 1: Automatically close File once it is out-of-scope? espressif/arduino-esp32#7614
- 2: close files esp8266/Arduino#5484
- 3: LittleFS and ArduinoJSON, why don't you close the file in the example? esp8266/Arduino#7280
- 4: https://github.com/espressif/arduino-esp32/blob/7ff99fc4/libraries/FS/src/vfs_api.cpp
- 5: https://arduino-esp8266.readthedocs.io/en/stable/filesystem.html
- 6: Why can't you close the file handle opened with SPIFFS.open("/") without doing damage? espressif/arduino-esp32#4727
Relax the 1D ledmap token match f.find("\"map\":[") is stricter than the 2D path and skips valid pretty-printed ledmap files with whitespace around the array; use the same f.find("\"map\"") check here.
🤖 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 `@wled00/FX_fcn.cpp` around lines 2227 - 2251, The 1D ledmap loader uses an
overly strict token match that rejects whitespace-formatted map arrays. In the
1D setup branch, update the f.find check guarding map parsing to search for the
same "\"map\"" token used by the 2D path, while leaving the subsequent integer
parsing unchanged.
1 participant
Rework on gap and ledmap build.
I also added a (brute-force) loop to map any unmapped physical pixels at the end but commented it out (also did not test that). Currently any unmapped pixel with stay unmapped (-1) - it really is a design choice whether pixels left unmapped should be added after the map or just left blank.
Tested both gap and ledmap and for the tests I did it works as expected (gap file, gap file with trailing pixels, full ledmap, full ledmap with trailing strip, sparse ledmap with trailing strip). There may be some edge cases I missed.
fixes #5529
Summary by CodeRabbit
New Features
Bug Fixes