Add new feature: mirrored and crc protected configuration files#5421
Closed
wled-install wants to merge 1 commit intowled:mainfrom
Closed
Add new feature: mirrored and crc protected configuration files#5421wled-install wants to merge 1 commit intowled:mainfrom
wled-install wants to merge 1 commit intowled:mainfrom
Conversation
Contributor
WalkthroughThis pull request introduces an optional CRC-based config integrity mechanism for WLED configuration files. It adds three new files (cfg_crc.h, cfg_crc.cpp, and integration in wled_server.cpp) and modifies cfg.cpp to validate and select between main and backup configs based on CRC validity, with fallback logic for defaults. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use OpenGrep to find security vulnerabilities and bugs across 17+ programming languages.OpenGrep is compatible with Semgrep configurations. Add an |
Member
|
Does this also work with the config backup added in version (0).16? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/cfg.cpp (1)
788-798:⚠️ Potential issue | 🔴 CriticalDon't short-circuit mirror recovery before
deserializeConfigFromFS().
wled00/wled.cpp:476-483still runsverifyConfig()/restoreConfig()before the new CRC selection logic. With CRC enabled,verifyConfig()only checks/cfg.jsonandrestoreConfig()now always returnsfalse, so a broken or missing primary falls intoresetConfig()and removes/cfg_backup.jsonbefore Lines 872-883 can recover from it. That turns a recoverable mirror failure into a factory reset.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/cfg.cpp` around lines 788 - 798, The current CRC-enabled guard causes restoreConfig() to always return false and verifyConfig() to only check s_cfg_json, which lets resetConfig() delete the backup before deserializeConfigFromFS() can recover it; update restoreConfig() (and/or verifyConfig()) so that when WLED_ENABLE_CRC_FOR_CONFIG is defined it does not short-circuit—either call the CRC-aware deserializeConfigFromFS() or attempt restoreFile(s_cfg_json) and then fallback to restoreFile(s_cfg_backup) (or validate both via the CRC logic) so a broken primary can still be recovered from the mirror instead of triggering resetConfig(); ensure you reference restoreConfig(), verifyConfig(), deserializeConfigFromFS(), resetConfig(), s_cfg_json and the backup file symbol when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/cfg_crc.cpp`:
- Around line 39-50: In loadCRC replace the unsafe use of File::parseInt()
(which returns a signed long) with reading the file contents into a buffer and
converting with strtoul (or parsing a fixed-width hex) to safely produce a
uint32_t; locate the loadCRC function in cfg_crc.cpp (references:
WLED_FS.exists, File f, f.parseInt()) and change the logic to read the file into
a null-terminated char array, call strtoul(buf, &endptr, 10 or 16) to obtain the
value, validate endptr and errno for conversion errors/overflow, assign to crc,
close the file, and return false on any read/parse error.
In `@wled00/cfg_crc.h`:
- Around line 5-7: The crc32_file API currently returns a uint32_t and uses
UINT32_MAX as an error sentinel which collides with a valid CRC; change the API
to return success separately (e.g., change crc32_file(const char* path) to bool
crc32_file(const char* path, uint32_t &outCrc)) and adjust callers (notably the
code in wled00/cfg.cpp that compares the raw crc to stored sidecar) to use the
boolean success result before using outCrc; ensure saveCRC/loadCRC signatures
remain consistent with this pattern and propagate clear failure handling instead
of relying on 0xFFFFFFFF as an error value.
In `@wled00/cfg.cpp`:
- Around line 879-883: The restore logic sets loadFile and writes the CRC before
verifying the repair succeeded; change the !validMain && validBackup branch (and
the analogous wsec branch) to first attempt the repair via
copyFile(s_cfg_backup_json, s_cfg_json) and check its return/errno, only on
success set loadFile = s_cfg_json and call saveCRC(s_cfg_crc_json, crcBackup);
if the copy fails keep loadFile pointing to the known-good backup
(s_cfg_backup_json or the wsec backup) and do NOT overwrite the CRC sidecar so
the boot will continue from the good mirror.
- Around line 891-898: The code clears pDoc when loadFile is null but doesn't
mark that defaults need persisting; update the logic around needsSave in the
block using pDoc and deserializeConfig(root, true) so that needsSave = needsSave
|| !cfgExists (or equivalently set needsSave true when loadFile is null / no cfg
exists). Locate the usage of loadFile, pDoc, deserializeConfig and adjust the
returned needsSave to include the "no config present" condition so defaults
(cfg.json/wsec.json and CRC sidecars) are written on first boot or after both
copies are rejected.
In `@wled00/wled_server.cpp`:
- Around line 226-245: The upload finalizer uses substring checks on the
variable filename (e.g., filename.indexOf("cfg.json") and
filename.indexOf(F("cfg.json"))), which unintentionally matches sidecar files
like /cfg.json.crc and triggers CRC recalculation and reboot; change the logic
to compare the normalized uploaded name exactly (normalize filename to its
basename or strip leading slash and then use equality checks against "cfg.json"
and "wsec.json") so only exact matches trigger the crc32_file/saveCRC/copyFile
steps and the doReboot/request->send call.
---
Outside diff comments:
In `@wled00/cfg.cpp`:
- Around line 788-798: The current CRC-enabled guard causes restoreConfig() to
always return false and verifyConfig() to only check s_cfg_json, which lets
resetConfig() delete the backup before deserializeConfigFromFS() can recover it;
update restoreConfig() (and/or verifyConfig()) so that when
WLED_ENABLE_CRC_FOR_CONFIG is defined it does not short-circuit—either call the
CRC-aware deserializeConfigFromFS() or attempt restoreFile(s_cfg_json) and then
fallback to restoreFile(s_cfg_backup) (or validate both via the CRC logic) so a
broken primary can still be recovered from the mirror instead of triggering
resetConfig(); ensure you reference restoreConfig(), verifyConfig(),
deserializeConfigFromFS(), resetConfig(), s_cfg_json and the backup file symbol
when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5a154887-6e99-40d0-8ebc-6ae58c544a6c
📒 Files selected for processing (4)
wled00/cfg.cppwled00/cfg_crc.cppwled00/cfg_crc.hwled00/wled_server.cpp
Comment on lines
+39
to
+50
| bool loadCRC(const char* path, uint32_t &crc) | ||
| { | ||
| if (!WLED_FS.exists(path)) | ||
| return false; | ||
|
|
||
| File f = WLED_FS.open(path, "r"); | ||
| if (!f) | ||
| return false; | ||
|
|
||
| crc = f.parseInt(); | ||
| f.close(); | ||
| return true; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the return type and overflow behavior of Arduino Stream::parseInt(), and is it safe for parsing decimal uint32_t values up to 4294967295?
💡 Result:
- Return type:
Stream::parseInt()returns along. (github.com) - Overflow behavior: The implementation accumulates into a signed
longwithvalue = value * 10 + digit;and does not perform any overflow/range checking. If the parsed number exceeds whatlongcan hold, the intermediate signed arithmetic overflows (behavior is not defined by C/C++), so the result is not reliable/portable. (github.com) - Is it safe for decimal
uint32_tup to4294967295? No. On the vast majority of Arduino cores,longis 32-bit signed, so the maximum representable value is 2,147,483,647, far below 4,294,967,295; parsing values aboveLONG_MAXwill overflow during accumulation. (github.com)
If you need to parse full-range uint32_t, read into a buffer/string and use strtoul() (base 10) with an explicit range check against UINT32_MAX (and check that the entire token was consumed).
Citations:
- 1: https://github.com/arduino/ArduinoCore-API/blob/master/api/Stream.cpp
- 2: https://github.com/arduino/ArduinoCore-API/blob/master/api/Stream.cpp
- 3: https://github.com/arduino/ArduinoCore-API/blob/master/api/Stream.cpp
Don't parse CRC32 values with Stream::parseInt().
parseInt() returns a signed long (max ~2.1 billion), but valid CRC32 values reach 4,294,967,295. Values above LONG_MAX overflow with undefined behavior during parsing. Read the file into a buffer and use strtoul() instead, or store as fixed-width hex.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/cfg_crc.cpp` around lines 39 - 50, In loadCRC replace the unsafe use
of File::parseInt() (which returns a signed long) with reading the file contents
into a buffer and converting with strtoul (or parsing a fixed-width hex) to
safely produce a uint32_t; locate the loadCRC function in cfg_crc.cpp
(references: WLED_FS.exists, File f, f.parseInt()) and change the logic to read
the file into a null-terminated char array, call strtoul(buf, &endptr, 10 or 16)
to obtain the value, validate endptr and errno for conversion errors/overflow,
assign to crc, close the file, and return false on any read/parse error.
Member
There was a problem hiding this comment.
as the value is written with printf(), typically you would read values back in using scanf(). Does the "file" object have something like a scanf method?
Contributor
There was a problem hiding this comment.
@softhack007, great question! The Arduino File object (which inherits from Stream) does not have a scanf-like method. The Arduino Stream API provides methods like parseInt(), parseFloat(), find(), findUntil(), and timeout-related helpers, but nothing analogous to scanf.
The three practical alternatives for this use case are:
Option 1: readString() + strtoul() (keep text/decimal format)
This is the safest drop-in fix for the current decimal text format while avoiding parseInt()'s signed-long overflow problem:
bool loadCRC(const char* path, uint32_t &crc)
{
if (!WLED_FS.exists(path))
return false;
File f = WLED_FS.open(path, "r");
if (!f)
return false;
String s = f.readString();
f.close();
if (s.length() == 0)
return false;
char *endptr = nullptr;
unsigned long val = strtoul(s.c_str(), &endptr, 10);
if (endptr == s.c_str()) // no digits consumed
return false;
crc = (uint32_t)val;
return true;
}Option 2: Store/read as binary (4 bytes) — most compact and robust
Avoids text parsing entirely. Write side becomes f.write((uint8_t*)&crc, 4) and read side:
bool loadCRC(const char* path, uint32_t &crc)
{
if (!WLED_FS.exists(path))
return false;
File f = WLED_FS.open(path, "r");
if (!f)
return false;
bool ok = (f.read((uint8_t*)&crc, sizeof(crc)) == sizeof(crc));
f.close();
return ok;
}Option 3: Store as fixed-width hex, parse with strtoul(..., 16)
f.printf("%08" PRIX32, crc) on write, then strtoul(s.c_str(), nullptr, 16) on read. Arguably easier to inspect manually than binary, and strtoul handles the full uint32_t range safely with either base.
Summary: No scanf equivalent exists on File/Stream. The binary approach (Option 2) is the most straightforward — no text formatting, no parsing ambiguity, and the CRC files are internal/opaque anyway. If human-readability is preferred, Option 1 or 3 with strtoul is the safe choice.
🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.
Learnt from: DedeHai
Repo: wled/WLED PR: 4793
File: wled00/file.cpp:481-513
Timestamp: 2025-07-31T18:21:49.868Z
Learning: In WLED, essential configuration files that require backup have short, controlled names (like "/cfg.json", "/presets.json") that are well within a 32-character buffer limit. The file naming is controlled by developers, making buffer overflow in backup filename construction highly unlikely.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, file operations (especially file.open()) should be checked to ensure they respect LittleFS filename limitations. The default LittleFS filename limit is 255 bytes (LFS_NAME_MAX). Reviews should assume default WLED configuration defines and not extreme edge-case values (e.g., WLED_MAX_SEGNAME_LEN = 512 would not be standard). File paths should be validated to stay within the 255-byte limit.
Learnt from: softhack007
Repo: wled/WLED PR: 5276
File: wled00/file.cpp:0-0
Timestamp: 2026-01-05T18:35:03.147Z
Learning: In WLED's file.cpp, the static File object `f` cannot be nullptr (it's not a pointer). The `if (f)` check only tests whether the file handle is open, and this check is already performed inside the `f.close()` function. Therefore, there is no need for an explicit `if (f)` check before calling `f.close()` - double-checking is redundant.
Learnt from: DedeHai
Repo: wled/WLED PR: 4956
File: wled00/data/edit.htm:213-238
Timestamp: 2025-09-25T03:51:29.606Z
Learning: For WLED file editor: Path handling can be simplified since the system operates in root directory context, complex path traversal protection is not needed.
Learnt from: DedeHai
Repo: wled/WLED PR: 4956
File: wled00/data/edit.htm:295-303
Timestamp: 2025-09-24T18:52:34.117Z
Learning: WLED file system stores all files in the root directory only (flat structure), so file paths don't have subdirectories. When working with WLED file editors, using just the filename (name) rather than full path is correct since there are no nested directories.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-08T00:57:36.134Z
Learning: In WLED (wled00/cfg.cpp), `deserializeConfigFromFS()` only sets `needsSave=true` (triggering a write of cfg.json) when usermod settings are present in the JSON (lines 758-761). On a fresh install with no usermods, `needsSave=false` so `serializeConfigToFS()` is never called at first boot. Correct defaults are never written to cfg.json until the user manually saves. Structural fix: set `needsSave = needsSave || !cfgExists` where `cfgExists = WLED_FS.exists(s_cfg_json)` checked before reading the file.
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/** : For web UI changes, edit files only under wled00/data (not firmware or generated files)
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-02-11T17:17:04.657Z
Learning: In WLED code reviews, when C-style functions or variables are added or modified, check if they are globally visible (not static). If the function/variable is not declared in any global header file (like fcn_declare.h), suggest either: (1) making it static (visible only within the translation unit) with a forward declaration if needed, or (2) adding a comment to clarify that the function/variable is intentionally global. This helps reduce global namespace pollution and enables better compiler optimizations.
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR `#4798`, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-02-11T11:14:24.395Z
Learning: In WLED code reviews, verify that all global variables in .cpp files are properly declared: check that they are defined via .h file or via "extern" declaration, verify that declarations are not duplicated, and ensure they are not implicitly declared as "int" (which can happen when the type is omitted).
Learnt from: netmindz
Repo: wled/WLED PR: 5093
File: wled00/util.cpp:1159-1182
Timestamp: 2025-11-20T00:04:04.829Z
Learning: In WLED PR `#5093`, the deviceId feature is designed for opt-in usage reporting that tracks only version/upgrade information (non-behavioral data), not user activity patterns. The deterministic salt approach (MAC + "WLED" + chip model/revision) is acceptable for this limited use case, as correlating MAC addresses to version history represents minimal privacy risk compared to behavioral tracking.
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
Comment on lines
+5
to
+7
| uint32_t crc32_file(const char* path); | ||
| bool saveCRC(const char* path, uint32_t crc); | ||
| bool loadCRC(const char* path, uint32_t &crc); |
Contributor
There was a problem hiding this comment.
Return CRC read success separately from the checksum value.
crc32_file() is declared as a plain uint32_t, but the implementation uses UINT32_MAX as an error sentinel. That is unsafe because 0xFFFFFFFF is also a legitimate CRC32 value, and callers in wled00/cfg.cpp compare the raw result directly to the stored sidecar. A missing/unreadable file can therefore be treated as valid if a failed save/copy ever persisted the sentinel.
🔧 API shape that avoids value-sentinel bugs
-uint32_t crc32_file(const char* path);
+bool crc32_file(const char* path, uint32_t& crc);📝 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
| uint32_t crc32_file(const char* path); | |
| bool saveCRC(const char* path, uint32_t crc); | |
| bool loadCRC(const char* path, uint32_t &crc); | |
| bool crc32_file(const char* path, uint32_t& crc); | |
| bool saveCRC(const char* path, uint32_t crc); | |
| bool loadCRC(const char* path, uint32_t &crc); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/cfg_crc.h` around lines 5 - 7, The crc32_file API currently returns a
uint32_t and uses UINT32_MAX as an error sentinel which collides with a valid
CRC; change the API to return success separately (e.g., change crc32_file(const
char* path) to bool crc32_file(const char* path, uint32_t &outCrc)) and adjust
callers (notably the code in wled00/cfg.cpp that compares the raw crc to stored
sidecar) to use the boolean success result before using outCrc; ensure
saveCRC/loadCRC signatures remain consistent with this pattern and propagate
clear failure handling instead of relying on 0xFFFFFFFF as an error value.
Member
There was a problem hiding this comment.
is declared as a plain
uint32_t, but the implementation usesUINT32_MAXas an error sentinel.
I agree with the bunny - all values of uint32_t are possible CRC results.
You could use a dedicated bool (return value) to flag errors in all CRC related functions.
Another possibility (hack alert) is to cut out the highest bit of the CRC (i.e. crc & 0xEFFFFFFF) so that the highest bit is available for error indication. Another (hack!) option is to use max(CRC -1, 0) as the CRC value, leaving UINT32_MAX for error indication. Both hacks will require careful handling in all places where CRC is computed or checked.
Contributor
There was a problem hiding this comment.
@softhack007, thanks for weighing in! Agreed on all counts.
The bool-return approach is clearly the right fix here. It:
- Loses no CRC value-space (no false-positive or false-negative risk).
- Keeps the pattern consistent with
saveCRC/loadCRCwhich already returnbool. - Makes error-handling at the call site explicit and compiler-checkable.
The two hack alternatives have real downsides:
| Approach | Problem |
|---|---|
crc & 0xEFFFFFFF |
Reduces effective CRC width by 1 bit everywhere; every compute/store/compare site must apply the mask consistently — easy to miss one. |
max(CRC - 1, 0) |
Silently maps both 0x00000000 and 0x00000001 to 0x00000000, so two distinct files with those CRCs would appear identical — a false-match bug. |
The recommended signature (already in the suggestion above) aligns the whole API cleanly:
bool crc32_file(const char* path, uint32_t& outCrc);
bool saveCRC(const char* path, uint32_t crc);
bool loadCRC(const char* path, uint32_t& crc);Callers in wled00/cfg.cpp would then gate any CRC comparison on the returned bool, eliminating the sentinel issue entirely.
🧠 Learnings used
Learnt from: DedeHai
Repo: wled/WLED PR: 4793
File: wled00/file.cpp:481-513
Timestamp: 2025-07-31T18:21:49.868Z
Learning: In WLED, essential configuration files that require backup have short, controlled names (like "/cfg.json", "/presets.json") that are well within a 32-character buffer limit. The file naming is controlled by developers, making buffer overflow in backup filename construction highly unlikely.
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/html_*.h : DO NOT edit generated embedded web header files (wled00/html_*.h)
Learnt from: softhack007
Repo: wled/WLED PR: 5381
File: wled00/data/update.htm:2-2
Timestamp: 2026-02-19T13:24:28.983Z
Learning: In WLED, generated embedded header files (html_*.h, js_iro.h, etc.) are NOT stored in the repository and have not been for over 2 years. These files are automatically regenerated during the build process from the source files in wled00/data/. Only the source .htm, .html, .css, and .js files in wled00/data/ should be committed when making web UI changes.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, file operations (especially file.open()) should be checked to ensure they respect LittleFS filename limitations. The default LittleFS filename limit is 255 bytes (LFS_NAME_MAX). Reviews should assume default WLED configuration defines and not extreme edge-case values (e.g., WLED_MAX_SEGNAME_LEN = 512 would not be standard). File paths should be validated to stay within the 255-byte limit.
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Learnt from: willmmiles
Repo: wled/WLED PR: 4859
File: usermods/udp_name_sync/udp_name_sync.cpp:62-70
Timestamp: 2025-08-21T00:00:17.196Z
Learning: In WLED codebase, UDP packet processing code must never rely on null termination being present on the wire and must always validate packet size first. The codebase already has some vulnerabilities where malformed data can crash the software, so avoiding introduction of new ones is a priority.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5293
File: usermods/user_fx/user_fx.cpp:167-171
Timestamp: 2026-01-13T03:07:20.560Z
Learning: In WLED effects (usermods/user_fx/user_fx.cpp and similar), when storing future timestamps, assignments like `stored_time = now + delay` are correct and handle uint32_t wraparound properly. Only comparison statements need to use subtraction-based elapsed-time checks (e.g., `(now - stored_time) >= delay`). Do not flag assignment statements as needing fixes for wraparound handling.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-02-11T11:14:24.395Z
Learning: In WLED code reviews, verify that all global variables in .cpp files are properly declared: check that they are defined via .h file or via "extern" declaration, verify that declarations are not duplicated, and ensure they are not implicitly declared as "int" (which can happen when the type is omitted).
Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-01-03T14:38:09.949Z
Learning: In WLED code reviews, check that FastLED trigonometric function aliases are not used. Replace `sin8()` and `cos8()` with `sin8_t()` and `cos8_t()`. Replace `sin16()` and `cos16()` with `sin16_t()` and `cos16_t()`. For float versions, replace `sinf()` and `cosf()` with `sin_approx()` and `cos_approx()`. See wled_math.cpp for the proper WLED implementations.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-02-11T17:17:04.657Z
Learning: In WLED code reviews, when C-style functions or variables are added or modified, check if they are globally visible (not static). If the function/variable is not declared in any global header file (like fcn_declare.h), suggest either: (1) making it static (visible only within the translation unit) with a forward declaration if needed, or (2) adding a comment to clarify that the function/variable is intentionally global. This helps reduce global namespace pollution and enables better compiler optimizations.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-08T00:57:36.134Z
Learning: In WLED (wled00/cfg.cpp), `deserializeConfigFromFS()` only sets `needsSave=true` (triggering a write of cfg.json) when usermod settings are present in the JSON (lines 758-761). On a fresh install with no usermods, `needsSave=false` so `serializeConfigToFS()` is never called at first boot. Correct defaults are never written to cfg.json until the user manually saves. Structural fix: set `needsSave = needsSave || !cfgExists` where `cfgExists = WLED_FS.exists(s_cfg_json)` checked before reading the file.
Learnt from: netmindz
Repo: wled/WLED PR: 5093
File: wled00/util.cpp:1159-1182
Timestamp: 2025-11-20T00:04:04.829Z
Learning: In WLED PR `#5093`, the deviceId feature is designed for opt-in usage reporting that tracks only version/upgrade information (non-behavioral data), not user activity patterns. The deterministic salt approach (MAC + "WLED" + chip model/revision) is acceptable for this limited use case, as correlating MAC addresses to version history represents minimal privacy risk compared to behavioral tracking.
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
Comment on lines
+879
to
+883
| } else if (!validMain && validBackup) { | ||
| DEBUG_PRINTLN(F("Main config invalid, restoring from backup...")); | ||
| copyFile(s_cfg_backup_json, s_cfg_json); | ||
| saveCRC(s_cfg_crc_json, crcBackup); | ||
| loadFile = s_cfg_json; |
Contributor
There was a problem hiding this comment.
Keep booting from the good mirror if the repair write fails.
On the !validMain && validBackup path, you switch loadFile back to the primary and overwrite its CRC sidecar before checking whether copyFile() actually repaired it. If that write fails, boot falls straight back to the corrupted main file instead of using the known-good backup. The same problem exists in the wsec branch at Lines 1397-1400.
🩹 Safer restore pattern
} else if (!validMain && validBackup) {
DEBUG_PRINTLN(F("Main config invalid, restoring from backup..."));
- copyFile(s_cfg_backup_json, s_cfg_json);
- saveCRC(s_cfg_crc_json, crcBackup);
- loadFile = s_cfg_json;
+ if (copyFile(s_cfg_backup_json, s_cfg_json) && saveCRC(s_cfg_crc_json, crcBackup)) {
+ loadFile = s_cfg_json;
+ } else {
+ loadFile = s_cfg_backup_json;
+ }
}Also applies to: 1397-1400
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/cfg.cpp` around lines 879 - 883, The restore logic sets loadFile and
writes the CRC before verifying the repair succeeded; change the !validMain &&
validBackup branch (and the analogous wsec branch) to first attempt the repair
via copyFile(s_cfg_backup_json, s_cfg_json) and check its return/errno, only on
success set loadFile = s_cfg_json and call saveCRC(s_cfg_crc_json, crcBackup);
if the copy fails keep loadFile pointing to the known-good backup
(s_cfg_backup_json or the wsec backup) and do NOT overwrite the CRC sidecar so
the boot will continue from the good mirror.
Comment on lines
+891
to
898
| if (loadFile) | ||
| success = readObjectFromFile(loadFile, nullptr, pDoc); | ||
| else | ||
| pDoc->clear(); | ||
| JsonObject root = pDoc->as<JsonObject>(); | ||
| bool needsSave = deserializeConfig(root, true); | ||
| releaseJSONBufferLock(); | ||
|
|
||
| return needsSave; |
Contributor
There was a problem hiding this comment.
Persist defaults when no valid config copy exists.
When loadFile is null, this path clears the document and keeps needsSave whatever deserializeConfig() returns. On first boot with CRC enabled, or after both copies are rejected, that means /cfg.json, /wsec.json, and the CRC sidecars are never recreated until the user manually saves once.
💾 Minimal fix
JsonObject root = pDoc->as<JsonObject>();
bool needsSave = deserializeConfig(root, true);
+ if (!loadFile) needsSave = true;
releaseJSONBufferLock();
return needsSave;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/cfg.cpp` around lines 891 - 898, The code clears pDoc when loadFile is
null but doesn't mark that defaults need persisting; update the logic around
needsSave in the block using pDoc and deserializeConfig(root, true) so that
needsSave = needsSave || !cfgExists (or equivalently set needsSave true when
loadFile is null / no cfg exists). Locate the usage of loadFile, pDoc,
deserializeConfig and adjust the returned needsSave to include the "no config
present" condition so defaults (cfg.json/wsec.json and CRC sidecars) are written
on first boot or after both copies are rejected.
Comment on lines
+226
to
245
| #ifdef WLED_ENABLE_CRC_FOR_CONFIG | ||
| if (filename.indexOf("cfg.json") >= 0) { | ||
| uint32_t crcMain = crc32_file("/cfg.json"); | ||
| saveCRC("/cfg.json.crc", crcMain); | ||
| copyFile("/cfg.json", "/cfg_backup.json"); | ||
| uint32_t crcBackup = crc32_file("/cfg_backup.json"); | ||
| saveCRC("/cfg_backup.json.crc", crcBackup); | ||
| } | ||
| if (filename.indexOf("wsec.json") >= 0) { | ||
| uint32_t crcMain = crc32_file("/wsec.json"); | ||
| saveCRC("/wsec.json.crc", crcMain); | ||
| copyFile("/wsec.json", "/wsec_backup.json"); | ||
| uint32_t crcBackup = crc32_file("/wsec_backup.json"); | ||
| saveCRC("/wsec_backup.json.crc", crcBackup); | ||
| } | ||
| #endif | ||
|
|
||
| if (filename.indexOf(F("cfg.json")) >= 0) { // check for filename with or without slash | ||
| doReboot = true; | ||
| request->send(200, FPSTR(CONTENT_TYPE_PLAIN), F("Config restore ok.\nRebooting...")); |
Contributor
There was a problem hiding this comment.
Use exact filename matches in the upload finalizer.
The new sidecar files this PR adds (/cfg.json.crc and /wsec.json.crc) both satisfy these indexOf(...json) checks. Uploading one of those files through /upload will recalculate live config CRCs and, for cfg.json, even trigger the config-restore reboot although the primary JSON was never uploaded.
🎯 Match the normalized upload name exactly
if (isFinal) {
request->_tempFile.close();
+ String normalizedName = filename;
+ if (!normalizedName.startsWith("/")) normalizedName = '/' + normalizedName;
`#ifdef` WLED_ENABLE_CRC_FOR_CONFIG
- if (filename.indexOf("cfg.json") >= 0) {
+ if (normalizedName == F("/cfg.json")) {
uint32_t crcMain = crc32_file("/cfg.json");
saveCRC("/cfg.json.crc", crcMain);
copyFile("/cfg.json", "/cfg_backup.json");
uint32_t crcBackup = crc32_file("/cfg_backup.json");
saveCRC("/cfg_backup.json.crc", crcBackup);
}
- if (filename.indexOf("wsec.json") >= 0) {
+ if (normalizedName == F("/wsec.json")) {
uint32_t crcMain = crc32_file("/wsec.json");
saveCRC("/wsec.json.crc", crcMain);
copyFile("/wsec.json", "/wsec_backup.json");
uint32_t crcBackup = crc32_file("/wsec_backup.json");
saveCRC("/wsec_backup.json.crc", crcBackup);
}
`#endif`
- if (filename.indexOf(F("cfg.json")) >= 0) { // check for filename with or without slash
+ if (normalizedName == F("/cfg.json")) {
doReboot = true;
request->send(200, FPSTR(CONTENT_TYPE_PLAIN), F("Config restore ok.\nRebooting..."));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/wled_server.cpp` around lines 226 - 245, The upload finalizer uses
substring checks on the variable filename (e.g., filename.indexOf("cfg.json")
and filename.indexOf(F("cfg.json"))), which unintentionally matches sidecar
files like /cfg.json.crc and triggers CRC recalculation and reboot; change the
logic to compare the normalized uploaded name exactly (normalize filename to its
basename or strip leading slash and then use equality checks against "cfg.json"
and "wsec.json") so only exact matches trigger the crc32_file/saveCRC/copyFile
steps and the doReboot/request->send call.
Member
There was a problem hiding this comment.
you could use different names for the CRC files, for example cfg.crc or cfg_json.crc - this would avoid double file types (.json.crc) and will also prevent false matches in the server upload endpoint.
Contributor
There was a problem hiding this comment.
@softhack007, that's an elegant root-cause fix. Renaming the CRC sidecar files to drop the .json component (e.g., cfg.crc, cfg_backup.crc, wsec.crc, wsec_backup.crc) would:
- Eliminate false matches — none of these names contain
cfg.jsonorwsec.jsonas a substring, so the existingindexOfchecks inhandleUploadwon't trip on them. - Remove the awkward double extension —
.json.crcreads as if the CRC file itself is a JSON variant. - Simplify the directory-listing guard — the
WLED_ENABLE_CRC_FOR_CONFIGblock in thelisthandler could also match on thes_wsecprefix (.crcextension can't masquerade as.json), keeping the filter logic leaner.
The full set of affected names across cfg.cpp, cfg_crc.h/cpp, and wled_server.cpp would be:
| Current | Suggested |
|---|---|
/cfg.json.crc |
/cfg.crc |
/cfg_backup.json.crc |
/cfg_backup.crc |
/wsec.json.crc |
/wsec.crc |
/wsec_backup.json.crc |
/wsec_backup.crc |
This approach is cleaner than patching the indexOf comparisons with exact-equality checks, since it prevents the ambiguity from arising in the first place.
🧠 Learnings used
Learnt from: DedeHai
Repo: wled/WLED PR: 4793
File: wled00/file.cpp:481-513
Timestamp: 2025-07-31T18:21:49.868Z
Learning: In WLED, essential configuration files that require backup have short, controlled names (like "/cfg.json", "/presets.json") that are well within a 32-character buffer limit. The file naming is controlled by developers, making buffer overflow in backup filename construction highly unlikely.
Learnt from: DedeHai
Repo: wled/WLED PR: 5116
File: wled00/data/index.js:3474-3484
Timestamp: 2025-11-27T13:54:01.277Z
Learning: In WLED, the server-side upload handler automatically normalizes file paths by prepending a leading slash if missing (`if (finalname.charAt(0) != '/') { finalname = '/' + finalname; }`). This means client-side code uploading files (e.g., via `/upload` endpoint) can use paths with or without leading slashes—both will resolve to the same location.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.
Learnt from: DedeHai
Repo: wled/WLED PR: 5404
File: wled00/wled_server.cpp:224-224
Timestamp: 2026-03-05T15:48:29.319Z
Learning: In wled00/wled_server.cpp, updateFSInfo() updates the informational fields fsBytesUsed/fsBytesTotal used for user display. Do not require synchronization/locking around these calls, even from async request handlers, because (1) the values are not required to be perfectly accurate, and (2) simultaneous multi-file delete/add ops are highly unlikely on this embedded IoT platform. This guidance is actionable for code reviews of this file and similar informational update paths; if in future these values become critical, consider adding proper synchronization or atomic access.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-08T00:57:36.134Z
Learning: In WLED (wled00/cfg.cpp), `deserializeConfigFromFS()` only sets `needsSave=true` (triggering a write of cfg.json) when usermod settings are present in the JSON (lines 758-761). On a fresh install with no usermods, `needsSave=false` so `serializeConfigToFS()` is never called at first boot. Correct defaults are never written to cfg.json until the user manually saves. Structural fix: set `needsSave = needsSave || !cfgExists` where `cfgExists = WLED_FS.exists(s_cfg_json)` checked before reading the file.
Learnt from: DedeHai
Repo: wled/WLED PR: 4956
File: wled00/data/edit.htm:213-222
Timestamp: 2025-09-25T05:52:09.044Z
Learning: In the WLED file editor (wled00/data/edit.htm), file renaming upon upload is not supported. The filename field is used for other purposes like creating new files or saving edited content, but uploaded files keep their original names.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, file operations (especially file.open()) should be checked to ensure they respect LittleFS filename limitations. The default LittleFS filename limit is 255 bytes (LFS_NAME_MAX). Reviews should assume default WLED configuration defines and not extreme edge-case values (e.g., WLED_MAX_SEGNAME_LEN = 512 would not be standard). File paths should be validated to stay within the 255-byte limit.
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/** : For web UI changes, edit files only under wled00/data (not firmware or generated files)
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-08T00:57:36.134Z
Learning: In WLED (wled00/cfg.cpp), `deserializeConfig()` is called with an empty JsonObject when cfg.json doesn't exist on fresh install. Any value read without the ArduinoJSON `|` fallback operator or CJSON macro will override correct constructor defaults with 0/null. Known affected values: `strip.setTargetFps(hw_led["fps"])` at line 179 (sets FPS to 0/unlimited instead of WLED_FPS=42). Fix: `strip.setTargetFps(hw_led["fps"] | WLED_FPS)`. The gamma issue (gammaCorrectCol/Bri) had the same root cause and was fixed in commit d1d9dec402 (Jan 2026) using inline `| default` fallbacks.
Learnt from: softhack007
Repo: wled/WLED PR: 5381
File: wled00/data/update.htm:2-2
Timestamp: 2026-02-19T13:24:28.983Z
Learning: In WLED, generated embedded header files (html_*.h, js_iro.h, etc.) are NOT stored in the repository and have not been for over 2 years. These files are automatically regenerated during the build process from the source files in wled00/data/. Only the source .htm, .html, .css, and .js files in wled00/data/ should be committed when making web UI changes.
Learnt from: softhack007
Repo: wled/WLED PR: 5107
File: boards/adafruit_matrixportal_esp32s3.json:1-66
Timestamp: 2025-11-22T20:33:14.840Z
Learning: In WLED, board JSON files in the boards/ directory may reference partition files that don't exist or have slightly different names than actual files in tools/. This is intentional because platformio.ini or platformio_override.sample.ini explicitly override the partition file using the board_build.partitions setting, which takes precedence over the board JSON partition reference. This pattern exists in multiple local board definitions and should not be flagged as an issue.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-02-11T17:17:04.657Z
Learning: In WLED code reviews, when C-style functions or variables are added or modified, check if they are globally visible (not static). If the function/variable is not declared in any global header file (like fcn_declare.h), suggest either: (1) making it static (visible only within the translation unit) with a forward declaration if needed, or (2) adding a comment to clarify that the function/variable is intentionally global. This helps reduce global namespace pollution and enables better compiler optimizations.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR `#4798`, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: netmindz
Repo: wled/WLED PR: 5093
File: wled00/util.cpp:1159-1182
Timestamp: 2025-11-20T00:04:04.829Z
Learning: In WLED PR `#5093`, the deviceId feature is designed for opt-in usage reporting that tracks only version/upgrade information (non-behavioral data), not user activity patterns. The deterministic salt approach (MAC + "WLED" + chip model/revision) is acceptable for this limited use case, as correlating MAC addresses to version history represents minimal privacy risk compared to behavioral tracking.
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
Member
|
@wled-install I'm wondering if you have experienced problems where the LittleFS built-in auto-recovery did not help? The new CRC check does add an extra level of protection, because LittleFS has error prevention, but no error correction on the data level. In case of bit-flip errors on the flash itself, your proposed recovery mechanism should help. Also in case of file corruption caused by WLED itself (serialization to file happens in several steps), a backup file is a good idea. And integrity checking by CRC is always a good idea 😃 . Edit: @DedeHai I'm wondering if this mechanism could be merged with the existing backup/recovery feature in version 16.0 ? Edit2: to protect against "power loss while serializing/writing cfg.son" errors, a typical strategy is to first write everything to a temporary file ( https://github.com/littlefs-project/littlefs/tree/master?tab=readme-ov-file#littlefs
https://github.com/littlefs-project/littlefs/blob/master/DESIGN.md
|
Collaborator
the current backup mechanism goes like this:
a CRC would add an additional safety - the question is do we need it and what scenarios can it catch that we can not catch without using CRC |
Member
@DedeHai I think a CRC would indeed catch more corruptions on the bit level, like a bit-flip (aging or flash wear) in the middle of a file. JSON tags might change ("leds" -> "lfds"), or strings like wifi name could be affected ("myWiFi" -> "wyWify"). Edit: afaik LittleFS also does "some" CRC checking (not sure about details), however it cannot provide a recovery. |
Collaborator
|
@softhack007 the CRC - as it is implemented in this PR - is almost useless to catch bit flips. all it will catch is if a file gets corrupted due to flash wear as it calcualtes the crc from the file that was written. so it does not add anything of value compared to my current implementation. It would need to store the file in ram, then calculate the CRC, write it to a file. |
Member
@DedeHai good catch :-) You're right, we have to consider the write operation as "untrusted" here, so the CRC should be calculated from the bytes as they are written, not from the resulting file after write. |
Contributor
Author
What would you (the main WLED developers) suggest?
P.S. The broader goal is also to add additional protection for OTA partitions and NVS to reduce the risk of corruption (100% protection is obviously not possible, especially with ESP32; bootloader itself will be always a weak point, but probability decreases). This would likely be implemented separately via a customized bootloader and only for controllers with larger flash sizes. The aim is mainly to minimize flash corruption risks during operation caused by cosmic radiation (especially at higher altitudes), bugs or random errors that might lead to overwrite flash regions, etc. Risks during configuration can be easily handled by the operator. |
Member
Do I need to add the ISS to the list of countries to the usage tracker? We do have users in Antarctica 😀 |
Contributor
Author
no, not that high 😀 |
Contributor
Author
|
I am closing this PR because it is not the best option to prevent data corruption during operation, as originally intended. Another PR will follow that addresses this more appropriately. This approach is not suitable for preventing data corruption during flash writes. |
Member
|
@coderabbitai lets try something new 😃 that's very common in safety critical domains. Please prepare a software FMEA (table), based on the previous discussions and common knowledge related to this PR. The feared event is "loss/corruption of cfg.json or wsec.json". Failure modes should include external errors (flash bit-flips and multi bit upsets, power loss during flash writing), and software related errors (incomplete file writes, flash write errors, filesystem full, read errors due to broken JSON). Include common cause/common mode failures, but limit your analysis to single failure and disregards multiple failures without common cause/common mode. List the compensating provisions that were suggested and assess their effectiveness. If you are not sure about failure end effects, leave the table cell empty for further discussion. A comprehensive overview of failure modes (related to the feared event) is the most important of your task. If possible, evaluate failure mode for "credibility" considering the normal use of WLED. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
Software FMEA — Feared Event: Loss/Corruption of
|
| # | Failure Mode | Root Cause | End Effect (if unmitigated) | LittleFS built-in | WLED v0.16 backup + verifyConfig() |
This PR: post-write CRC + backup | Proposed: in-write CRC | Proposed: write-to-temp + rename | Credibility | Notes |
|---|---|---|---|---|---|---|---|---|---|---|
| E1 | Single-bit flip in stored file data (SEU) | Cosmic ray / alpha particle hitting NOR flash cell | Silent content corruption (e.g. WiFi SSID corrupted, numeric value wrong) | ❌ No detection — littlefs by itself does not provide ECC; CRC only covers metadata, not file data | ❌ verifyConfig() checks JSON structure only — a flipped bit inside a JSON value (e.g. "myssid"→"mysssd") passes as valid JSON |
✅ Detects: CRC mismatch triggers restore from backup (if backup not also corrupted) — but CRC was computed after write, so it reflects the written state correctly for this scenario | ✅ Same detection effectiveness as post-write CRC for this scenario (bit-flip occurs after write) | N/A — does not help post-write | Low | NOR flash is inherently more resistant to SEU than SRAM/NAND. Intel showed electronic failure from cosmic rays increased at elevated altitudes. At ground level, probability is very low for a ~4–10 kB file. However, both files could share the same flash block — see C1. |
| E2 | Multi-bit upset (MBU) in stored file data | Cosmic ray / heavy ion striking multiple cells | Content corruption or completely unreadable file | ❌ No LittleFS correction | ✅ verifyConfig() may catch if JSON becomes structurally invalid; otherwise fails silently |
✅ CRC mismatch detected; restore from backup (common-mode risk: see C1) | ✅ Same | N/A | Very Low | MBU is rarer than single-bit SEU on NOR flash at ground level. |
| E3 | Flash block wear-out / stuck bits | End-of-life erase cycles exhausted on a block | Read-back returns incorrect data; file content corrupted or unreadable | Bad block detection and dynamic wear leveling provide a best-effort solution for avoiding early filesystem death due to wear. LittleFS remaps bad blocks. | ✅ Structural JSON corruption caught by verifyConfig(); content corruption may be silent |
✅ CRC detects divergence; restore from backup — but if wear-out is systematic on this flash region, backup may be at risk too | ✅ Marginally better | N/A | Very Low | ESP32 flash can handle 100,000 cycles at minimum; adding inefficiencies of the FS wears it out somewhat earlier, but that's about the size of magnitude of lifetime expected. Config writes in WLED are extremely infrequent, so wear-out of the config region in normal use is implausible. |
| E4 | Power loss during flash erase phase of a write | Abrupt power cut at exactly the wrong moment | Partially erased block — LittleFS CoW prevents this propagating to committed file | LittleFS is designed to handle random power failures; all file operations have strong copy-on-write guarantees and if power is lost, the filesystem will fall back to the last known good state. | ✅ Previous valid file is still accessible | ✅ Both files remain intact from before the write | N/A | ✅ Rename is atomic in LittleFS — all POSIX operations such as remove and rename are atomic, even in event of power-loss. No additional benefit over CoW for erase interruption | Medium | Power loss is a credible event for DIY WLED installs (no UPS). LittleFS CoW already handles this well at the erase level. |
| E5 | Power loss during flash program (write) phase | Abrupt power cut during data programming | Partially written page | LittleFS falls back to the last known good state. New (partial) data is not committed; old version survives. | ✅ Old version survives; recovered on next boot | ✅ CRC on the new file would not be updated (write was incomplete), so primary CRC mismatch → restore from backup. But backup = old version, so no data loss beyond the aborted write | ✅ More reliable detection of partially-committed CRC file | ✅✅ Strongest mitigation: an atomic rename commits either the old or new version atomically, eliminating the partial-write window entirely | Medium | Most credible power-loss scenario for WLED. LittleFS CoW already mitigates well; atomic rename + CRC further reduces residual risk. |
Software Failure Modes
| # | Failure Mode | Root Cause | End Effect (if unmitigated) | LittleFS built-in | WLED v0.16 backup + verifyConfig() |
This PR: post-write CRC + backup | Proposed: in-write CRC | Proposed: write-to-temp + rename | Credibility | Notes |
|---|---|---|---|---|---|---|---|---|---|---|
| S1 | Incomplete JSON write (truncated file) | Heap exhaustion, stack overflow, or exception mid-serialization | Truncated/partial JSON in cfg.json | ❌ LittleFS faithfully stores whatever was written | ✅ verifyConfig() detects truncated JSON (invalid structure); restores from backup |
✅ CRC mismatch (CRC was computed on truncated content from file, but backup holds previous good version) — However: if CRC is computed from the truncated file after write, CRC will match the truncated content, so it would NOT detect this! The backup (previous version) would then only be used on next corruption, not this one. DedeHai's comment is directly relevant here. | ✅✅ CRC computed during write catches truncation because final byte count is known at write time | ✅✅ Temp file holds truncated content; rename only occurs after successful close; if close fails → old file untouched | Medium | This is a key gap in this PR as discussed: all data is read back and checked for integrity by LittleFS at block level only — content validity is the application's responsibility. The backup (verifyConfig()) approach is effective here. |
| S2 | Flash write driver returns error (undetected) | SPI flash communication error; driver bug | Silently written incorrect data | ❌ LittleFS may or may not detect (depends on read-back verify) | ✅ In-write CRC would catch divergence between what was sent and what is stored | N/A | Low | The prog function may return LFS_ERR_CORRUPT if implementation can detect corrupt blocks; the wear leveling reads all data back and checks for integrity. LittleFS does perform read-back verification at the block level, providing some protection. | ||
| S3 | Filesystem full — write fails entirely | LittleFS partition exhausted | New config not saved; old config retained (if file wasn't truncated first) | ❌ Write fails with error; LittleFS does not auto-recover | N/A | ✅✅ Temp file write fails; original file untouched; rename never called | Low–Medium | WLED's LittleFS partition is small (~1–2 MB typically). If user uploads many files (e.g. via OTA or the file editor), the partition could fill up. Writing backup + CRC files (this PR) increases the risk of hitting this limit. | ||
| S4 | WLED serialization bug — incorrect JSON content written | Bug in serializeConfig() generating wrong values |
Valid JSON but with wrong values silently stored | ❌ Not a filesystem-level issue | ❌ verifyConfig() checks structure, not semantic correctness; backup also holds wrong data after one write cycle |
❌ CRC matches incorrect-but-valid JSON; no detection | ❌ Same | ❌ Same | Low | No file-integrity mechanism can detect semantic/logic errors in serialization. A regression test would be needed. |
| S5 | Deserialization error / broken JSON at read | FS returns garbled data on read; prior silent corruption manifests | Wrong config loaded; WLED starts with defaults or bad config | ❌ No LittleFS content protection | ✅ verifyConfig() detects bad JSON; restores from backup |
✅ CRC check at boot detects mismatch; switches to backup | ✅ Same | N/A | Low | Boot-time CRC check (this PR) adds value over WLED v0.16 backup by catching silent corruption that verifyConfig() misses (e.g. valid JSON with wrong values). |
| S6 | Backup write fails silently after primary write succeeds | Heap/stack error during backup write; flash write error | Backup out of date or corrupt; primary is good | ❌ Not detected | ❌ Not detected — verifyConfig() only checks on load |
N/A | N/A | Low | As flagged in the PR: unchecked return values of backup write calls. Stale backup is still better than no backup. |
Common Cause / Common Mode Failures
| # | Failure Mode | Root Cause | End Effect | Mitigations | Credibility | Notes |
|---|---|---|---|---|---|---|
| C1 | Both cfg.json and its backup reside on the same flash erase block | LittleFS dynamic wear-leveling may co-locate small files | A single block-level corruption or wear-out event affects both copies simultaneously | ❌ No application-level control over block placement in LittleFS | Low | LittleFS dynamic wear leveling makes this unpredictable. For files as small as cfg.json, there is a non-negligible chance both the main file and its backup (plus CRC files) land in the same or adjacent blocks. A placement guarantee would require a dedicated partition or static block mapping. |
| C2 | OTA firmware update inadvertently erases the LittleFS partition | Incorrect partition table; flash tool error; wrong OTA image | All config files lost simultaneously | ❌ No protection within LittleFS | ❌ Both primary and backup lost | ❌ Same |
| C3 | WLED factory reset / resetConfig() |
Intentional or accidental call to resetConfig() |
All config files including backups and CRCs deleted | N/A — intentional operation | ❌ Backup also wiped | ❌ CRC files also wiped (handled in PR) |
| C4 | LittleFS metadata corruption (directory/inode layer) | Power loss at specific metadata update timing; NOR flash read disturbance | Files may become inaccessible even if data blocks are intact | Atomicity requires redundancy and error detection; LittleFS uses a 32-bit CRC for error detection of metadata. Metadata pairs are CRC-protected and redundant. | ❌ Both files equally affected |
Summary: Effectiveness of Compensating Provisions
| Provision | Detects bit-flip (post-write) | Detects power loss corruption | Detects incomplete write | Detects FS full | Detects semantic error | Flash overhead | Implementation complexity |
|---|---|---|---|---|---|---|---|
| LittleFS CoW (built-in) | ❌ | ✅ (filesystem level) | ✅ (old version retained) | ❌ | ❌ | None | None |
WLED v0.16 backup + verifyConfig() |
❌ (structural only) | ✅ | ✅ (structural JSON check) | ❌ | 1× config size | Low | |
| This PR: post-write CRC + backup | ✅ | ✅ | ❌ (gap: CRC matches truncated content) | ❌ | 2× config + 2 CRC files | Medium | |
| Proposed: in-write CRC (CRC computed during write) | ✅ | ✅ | ✅ | ❌ | Same as PR | Medium-High | |
| Proposed: write-to-temp + atomic rename | ❌ | ✅✅ (strongest) | ✅✅ | ✅ (temp write fails, original intact) | ❌ | 1× config transient | Low |
Key Observations from the FMEA
-
The most credible failure mode for typical WLED home use is power loss during a write (E4/E5) and incomplete writes due to software errors (S1). LittleFS CoW already handles E4/E5 well. S1 (heap/exception during serialization) is the most practically impactful gap.
-
The critical gap in this PR (as DedeHai correctly identified): because the CRC is computed by reading back the written file rather than from the write buffer, it cannot detect an incomplete or silently-corrupted write (S1, S2). The CRC simply reflects whatever was stored, valid or not.
-
The
write-to-temp + renamestrategy (suggested by@softhack007) is the most effective single mitigation for S1, S3, E4, and E5, with minimal overhead and leverages LittleFS's own atomic rename guarantee. -
Bit-flip SEU (E1) on NOR flash at ground level is of very low credibility for a home LED controller, though it becomes more relevant at altitude or in industrial environments as noted by
@wled-install. -
Common-cause failure (C1) — both primary and backup co-located in the same flash block — is a non-obvious residual risk that no application-level solution fully eliminates without partition-level control.
-
Combining the existing WLED v0.16 backup +
verifyConfig()with the atomic rename strategy would address the highest-credibility failure modes with modest complexity.
🧠 Learnings used
Learnt from: wled-install
Repo: wled/WLED PR: 5426
File: wled00/safe_boot_functions.cpp:97-98
Timestamp: 2026-03-15T12:16:37.099Z
Learning: In wled00/safe_boot_functions.cpp (WLED SafeBootloader integration), the return values of `esp_partition_erase_range()` and `esp_partition_write()` in `update_ota_crc()` and `update_spiffs_crc()` are intentionally not checked. If these operations fail and an incorrect or corrupted CRC ends up stored, the SafeBootloader detects the CRC mismatch on boot and automatically restores data from the other (backup) partition. This is a deliberate design decision relying on the SafeBootloader's dual-partition recovery mechanism, consistent with the same reasoning applied to unchecked `esp_partition_read()` in `calc_crc()`.
Learnt from: wled-install
Repo: wled/WLED PR: 5426
File: wled00/safe_boot_functions.cpp:142-143
Timestamp: 2026-03-15T12:16:51.045Z
Learning: In wled00/safe_boot_functions.cpp (WLED SafeBootloader integration), the return values of `esp_partition_erase_range()` and `esp_partition_write()` inside `update_spiffs_crc()` are intentionally not checked. If these operations fail and an incorrect CRC is stored, the SafeBootloader detects the CRC mismatch and automatically restores data from the other (backup) partition. This is a deliberate design decision relying on the SafeBootloader's dual-partition recovery mechanism, consistent with the same rationale applied to `esp_partition_read()` in `calc_crc()`.
Learnt from: wled-install
Repo: wled/WLED PR: 5426
File: wled00/safe_boot_functions.cpp:32-32
Timestamp: 2026-03-15T12:15:58.551Z
Learning: In wled00/safe_boot_functions.cpp (WLED SafeBootloader integration), the return value of `esp_partition_read()` inside `calc_crc()` is intentionally not checked. If a read error causes an incorrect CRC to be written, the SafeBootloader detects the CRC mismatch and automatically restores data from the other (backup) partition. This is a deliberate design decision relying on the SafeBootloader's dual-partition recovery mechanism.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.
Learnt from: DedeHai
Repo: wled/WLED PR: 4793
File: wled00/file.cpp:481-513
Timestamp: 2025-07-31T18:21:49.868Z
Learning: In WLED, essential configuration files that require backup have short, controlled names (like "/cfg.json", "/presets.json") that are well within a 32-character buffer limit. The file naming is controlled by developers, making buffer overflow in backup filename construction highly unlikely.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, file operations (especially file.open()) should be checked to ensure they respect LittleFS filename limitations. The default LittleFS filename limit is 255 bytes (LFS_NAME_MAX). Reviews should assume default WLED configuration defines and not extreme edge-case values (e.g., WLED_MAX_SEGNAME_LEN = 512 would not be standard). File paths should be validated to stay within the 255-byte limit.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-08T00:57:36.134Z
Learning: In WLED (wled00/cfg.cpp), `deserializeConfig()` is called with an empty JsonObject when cfg.json doesn't exist on fresh install. Any value read without the ArduinoJSON `|` fallback operator or CJSON macro will override correct constructor defaults with 0/null. Known affected values: `strip.setTargetFps(hw_led["fps"])` at line 179 (sets FPS to 0/unlimited instead of WLED_FPS=42). Fix: `strip.setTargetFps(hw_led["fps"] | WLED_FPS)`. The gamma issue (gammaCorrectCol/Bri) had the same root cause and was fixed in commit d1d9dec402 (Jan 2026) using inline `| default` fallbacks.
Learnt from: DedeHai
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1176-1187
Timestamp: 2025-09-16T18:08:42.848Z
Learning: In WLED finalizeInit() bus creation (wled00/FX_fcn.cpp), intentionally allowing memory overruns when bus configurations exceed MAX_LED_MEMORY is a deliberate design choice. The trade-off prioritizes creating buses with reduced LED counts over completely failing to create buses, which would cause no LED output and UI failures. This approach forces users to update configurations after migrating to version 0.16 while maintaining basic functionality.
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR `#4798`, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-08T00:57:36.134Z
Learning: In WLED (wled00/cfg.cpp), `deserializeConfigFromFS()` only sets `needsSave=true` (triggering a write of cfg.json) when usermod settings are present in the JSON (lines 758-761). On a fresh install with no usermods, `needsSave=false` so `serializeConfigToFS()` is never called at first boot. Correct defaults are never written to cfg.json until the user manually saves. Structural fix: set `needsSave = needsSave || !cfgExists` where `cfgExists = WLED_FS.exists(s_cfg_json)` checked before reading the file.
Learnt from: DedeHai
Repo: wled/WLED PR: 4939
File: wled00/data/index.js:2406-2413
Timestamp: 2025-09-28T09:53:42.670Z
Learning: In WLED, code paths that appear unreachable may actually be safety fallbacks for low-heap conditions. These fallbacks are intentional defensive programming measures to ensure UI functionality is preserved when system resources are constrained.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Learnt from: willmmiles
Repo: wled/WLED PR: 4859
File: usermods/udp_name_sync/udp_name_sync.cpp:62-70
Timestamp: 2025-08-21T00:00:17.196Z
Learning: In WLED codebase, UDP packet processing code must never rely on null termination being present on the wire and must always validate packet size first. The codebase already has some vulnerabilities where malformed data can crash the software, so avoiding introduction of new ones is a priority.
Learnt from: netmindz
Repo: wled/WLED PR: 5093
File: wled00/util.cpp:1159-1182
Timestamp: 2025-11-20T00:04:04.829Z
Learning: In WLED PR `#5093`, the deviceId feature is designed for opt-in usage reporting that tracks only version/upgrade information (non-behavioral data), not user activity patterns. The deterministic salt approach (MAC + "WLED" + chip model/revision) is acceptable for this limited use case, as correlating MAC addresses to version history represents minimal privacy risk compared to behavioral tracking.
Member
|
@coderabbitai perfect 👍 You may add "safety engineer for embedded software" to your linked-in profile 😉 now |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
The discussion here was high quality and made the analysis much richer — DedeHai's sharp catch about the post-write CRC gap was the key insight that drove the summary conclusions. Happy to revisit or extend the table if the team pursues a revised approach (e.g. write-to-temp + rename + in-write CRC) in a follow-up PR. 🛡️ 🧠 Learnings used |
Member
|
@CodeRabbit for future reviews, I think this is great lessons learned. Please remember to suggest an FMEA analysis in case we have similar "is this worth doing" uncertainties in future reviews. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
🧠 Learnings used |
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
5 participants
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
This PR introduces an optional mechanism to protect WLED configuration files (cfg.json and wsec.json) against corruption due to flash errors or power loss.
The feature is disabled by default and can be enabled by defining:
WLED_ENABLE_CRC_FOR_CONFIGWhen enabled, the system maintains mirrored copies of each config with separate CRC files, providing self-healing at boot.
Details
Both cfg.json and wsec.json are mirrored: primary + backup copy.
Each config has a separate CRC file storing its checksum.
At boot:
Writes always update both copies and their CRCs in a controlled sequence to ensure power-loss safety.
Benefits
Detects random flash bit errors in configuration files
Recovers automatically from partial or corrupted writes
Improves robustness against power loss during configuration save
No behavioral changes when the feature is disabled
Notes
The implementation avoids modifying JSON structures or UI/API behavior.
Flash wear is slightly increased (two copies + CRCs), but config writes are rare, so this is negligible.
This feature is optional and fully backwards-compatible.
Summary by CodeRabbit
New Features