Skip to content

Add new feature: mirrored and crc protected configuration files #5421

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wled-install wants to merge 1 commit into from
+278 −25
Closed

Add new feature: mirrored and crc protected configuration files #5421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
203 changes: 178 additions & 25 deletions wled00/cfg.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
#include "wled.h"
#include "wled_ethernet.h"

#ifdef WLED_ENABLE_CRC_FOR_CONFIG
#include "cfg_crc.h"
#endif
/*
* Serializes and parses the cfg.json and wsec.json settings files, stored in internal FS.
* The structure of the JSON is not to be considered an official API and may change without notice.
Expand All @@ -22,6 +24,7 @@
#define DEFAULT_LED_COLOR_ORDER COL_ORDER_GRB //default to GRB
#endif


static constexpr unsigned sumPinsRequired(const unsigned* current, size_t count) {
return (count > 0) ? (Bus::getNumberOfPins(*current) + sumPinsRequired(current+1,count-1)) : 0;
}
Expand Down Expand Up @@ -768,13 +771,26 @@ bool deserializeConfig(JsonObject doc, bool fromFS) {
}

static const char s_cfg_json[] PROGMEM = "/cfg.json";
static const char s_wsec_json[] PROGMEM = "/wsec.json";
#ifdef WLED_ENABLE_CRC_FOR_CONFIG
static const char s_cfg_backup_json[] PROGMEM = "/cfg_backup.json";
static const char s_cfg_crc_json[] PROGMEM = "/cfg.json.crc";
static const char s_cfg_crc_backup_json[] PROGMEM = "/cfg_backup.json.crc";
static const char s_wsec_backup_json[] PROGMEM = "/wsec_backup.json";
static const char s_wsec_crc_json[] PROGMEM = "/wsec.json.crc";
static const char s_wsec_crc_backup_json[] PROGMEM = "/wsec_backup.json.crc";
#endif

bool backupConfig() {
return backupFile(s_cfg_json);
}

bool restoreConfig() {
return restoreFile(s_cfg_json);
#ifdef WLED_ENABLE_CRC_FOR_CONFIG
return false;
#else
return restoreFile(s_cfg_json);
#endif
}

bool verifyConfig() {
Expand All @@ -785,53 +801,141 @@ bool configBackupExists() {
return checkBackupExists(s_cfg_json);
}


// rename config file and reboot
// if the cfg file doesn't exist, such as after a reset, do nothing
void resetConfig() {
#ifdef WLED_ENABLE_CRC_FOR_CONFIG
if (!WLED_FS.exists(s_cfg_json) && !WLED_FS.exists(s_cfg_backup_json))
return;
#else
if (!WLED_FS.exists(s_cfg_json))
return;
#endif
DEBUG_PRINTLN(F("Reset config"));
char backupname[32];
// rename main config
if (WLED_FS.exists(s_cfg_json)) {
DEBUG_PRINTLN(F("Reset config"));
char backupname[32];
snprintf_P(backupname, sizeof(backupname), PSTR("/rst.%s"), &s_cfg_json[1]);
WLED_FS.rename(s_cfg_json, backupname);
doReboot = true;
}
if (WLED_FS.exists(s_wsec_json))
WLED_FS.remove(s_wsec_json);

#ifdef WLED_ENABLE_CRC_FOR_CONFIG
//rename backup config
if (WLED_FS.exists(s_cfg_backup_json)) {
snprintf_P(backupname, sizeof(backupname), PSTR("/rst.%s"), &s_cfg_backup_json[1]);
WLED_FS.rename(s_cfg_backup_json, backupname);
}
//remove CRC files
if (WLED_FS.exists(s_cfg_crc_json))
WLED_FS.remove(s_cfg_crc_json);

if (WLED_FS.exists(s_cfg_crc_backup_json))
WLED_FS.remove(s_cfg_crc_backup_json);

if (WLED_FS.exists(s_wsec_backup_json))
WLED_FS.remove(s_wsec_backup_json);

if (WLED_FS.exists(s_wsec_crc_json))
WLED_FS.remove(s_wsec_crc_json);

if (WLED_FS.exists(s_wsec_crc_backup_json))
WLED_FS.remove(s_wsec_crc_backup_json);

#endif
doReboot = true;
}

bool deserializeConfigFromFS() {

[[maybe_unused]] bool success = deserializeConfigSec();

if (!requestJSONBufferLock(JSON_LOCK_CFG_DES)) return false;

DEBUG_PRINTLN(F("Reading settings from /cfg.json..."));

success = readObjectFromFile(s_cfg_json, nullptr, pDoc);

// NOTE: This routine deserializes *and* applies the configuration
// Therefore, must also initialize ethernet from this function
const char* loadFile = nullptr;

#ifdef WLED_ENABLE_CRC_FOR_CONFIG
uint32_t storedCRCMain = 0;
uint32_t storedCRCBackup = 0;

bool haveMainCRC = loadCRC(s_cfg_crc_json, storedCRCMain);
bool haveBackupCRC = loadCRC(s_cfg_crc_backup_json, storedCRCBackup);

uint32_t crcMain = crc32_file(s_cfg_json);
uint32_t crcBackup = crc32_file(s_cfg_backup_json);

bool validMain = haveMainCRC && (crcMain == storedCRCMain);
bool validBackup = haveBackupCRC && (crcBackup == storedCRCBackup);

if (validMain && validBackup) {
loadFile = s_cfg_json;
} else if (validMain && !validBackup) {
DEBUG_PRINTLN(F("Backup config invalid, restoring..."));
copyFile(s_cfg_json, s_cfg_backup_json);
saveCRC(s_cfg_crc_backup_json, crcMain);
loadFile = s_cfg_json;
} 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;
Comment on lines +879 to +883
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

} else {
DEBUG_PRINTLN(F("Both config files invalid! Using defaults."));
loadFile = nullptr; // signal no file
}
#else
loadFile = s_cfg_json;
#endif
if (loadFile)
success = readObjectFromFile(loadFile, nullptr, pDoc);
else
pDoc->clear();
JsonObject root = pDoc->as<JsonObject>();
bool needsSave = deserializeConfig(root, true);
releaseJSONBufferLock();

return needsSave;
Comment on lines +891 to 898
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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;
Based on learnings: `deserializeConfigFromFS()` only sets `needsSave=true` when usermod settings are present; structural fix: set `needsSave = needsSave || !cfgExists`.
🤖 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.

}

void serializeConfigToFS() {
serializeConfigSec();
backupConfig(); // backup before writing new config

DEBUG_PRINTLN(F("Writing settings to /cfg.json..."));

if (!requestJSONBufferLock(JSON_LOCK_CFG_SER)) return;

JsonObject root = pDoc->to<JsonObject>();

serializeConfig(root);

/*
* STEP 1
* write main config
*/
File f = WLED_FS.open(FPSTR(s_cfg_json), "w");
if (f) serializeJson(root, f);
f.close();
releaseJSONBufferLock();

if (f) {
serializeJson(root, f);
f.close();
#ifdef WLED_ENABLE_CRC_FOR_CONFIG
uint32_t crcMain = crc32_file(s_cfg_json);
saveCRC(s_cfg_crc_json, crcMain);
#endif
}
#ifdef WLED_ENABLE_CRC_FOR_CONFIG
/*
* STEP 2
* write backup config
*/
f = WLED_FS.open(FPSTR(s_cfg_backup_json), "w");

if (f) {
serializeJson(root, f);
f.close();
uint32_t crcBackup = crc32_file(s_cfg_backup_json);
saveCRC(s_cfg_crc_backup_json, crcBackup);
}
#endif
releaseJSONBufferLock();
configNeedsWrite = false;
}

Expand Down Expand Up @@ -1263,16 +1367,48 @@ void serializeConfig(JsonObject root) {
UsermodManager::addToConfig(usermods_settings);
}


static const char s_wsec_json[] PROGMEM = "/wsec.json";

//settings in /wsec.json, not accessible via webserver, for passwords and tokens
bool deserializeConfigSec() {
DEBUG_PRINTLN(F("Reading settings from /wsec.json..."));

if (!requestJSONBufferLock(JSON_LOCK_CFG_SEC_DES)) return false;
const char* loadFile = nullptr;

#ifdef WLED_ENABLE_CRC_FOR_CONFIG
uint32_t storedCRCMain = 0;
uint32_t storedCRCBackup = 0;

bool haveMainCRC = loadCRC(s_wsec_crc_json, storedCRCMain);
bool haveBackupCRC = loadCRC(s_wsec_crc_backup_json, storedCRCBackup);

uint32_t crcMain = crc32_file(s_wsec_json);
uint32_t crcBackup = crc32_file(s_wsec_backup_json);

bool validMain = haveMainCRC && (crcMain == storedCRCMain);
bool validBackup = haveBackupCRC && (crcBackup == storedCRCBackup);

bool success = readObjectFromFile(s_wsec_json, nullptr, pDoc);
if (validMain && validBackup) {
loadFile = s_wsec_json;
}
else if (validMain && !validBackup) {
copyFile(s_wsec_json, s_wsec_backup_json);
saveCRC(s_wsec_crc_backup_json, crcMain);
loadFile = s_wsec_json;
}
else if (!validMain && validBackup) {
copyFile(s_wsec_backup_json, s_wsec_json);
saveCRC(s_wsec_crc_json, crcBackup);
loadFile = s_wsec_json;
}
else {
DEBUG_PRINTLN(F("Both wsec files invalid"));
releaseJSONBufferLock();
return false;
}
#else
loadFile = s_wsec_json;
#endif

bool success = readObjectFromFile(loadFile, nullptr, pDoc);
if (!success) {
releaseJSONBufferLock();
return false;
Expand Down Expand Up @@ -1360,7 +1496,24 @@ void serializeConfigSec() {
#endif

File f = WLED_FS.open(FPSTR(s_wsec_json), "w");
if (f) serializeJson(root, f);
f.close();

if (f) {
serializeJson(root, f);
f.close();

#ifdef WLED_ENABLE_CRC_FOR_CONFIG
uint32_t crcMain = crc32_file(s_wsec_json);
saveCRC(s_wsec_crc_json, crcMain);
#endif
}
#ifdef WLED_ENABLE_CRC_FOR_CONFIG
f = WLED_FS.open(FPSTR(s_wsec_backup_json), "w");
if (f) {
serializeJson(root, f);
f.close();
uint32_t crcBackup = crc32_file(s_wsec_backup_json);
saveCRC(s_wsec_crc_backup_json, crcBackup);
}
#endif
releaseJSONBufferLock();
}
53 changes: 53 additions & 0 deletions wled00/cfg_crc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
#include "wled.h"

#ifdef WLED_ENABLE_CRC_FOR_CONFIG

static uint32_t crc32_update(uint32_t crc, uint8_t data)
{
crc ^= data;
for (uint8_t i = 0; i < 8; i++) {
if (crc & 1) crc = (crc >> 1) ^ 0xEDB88320;
else crc >>= 1;
}
return crc;
}

uint32_t crc32_file(const char* path)
{
File f = WLED_FS.open(path, "r");
if (!f) return UINT32_MAX;

uint32_t crc = 0xFFFFFFFF;

while (f.available())
crc = crc32_update(crc, f.read());

f.close();
return ~crc;
}

bool saveCRC(const char* path, uint32_t crc)
{
File f = WLED_FS.open(path, "w");
if (!f) return false;

f.printf("%lu", (unsigned long)crc);
f.close();
return true;
}

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;
Comment on lines +39 to +50
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 12, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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 a long. (github.com)
  • Overflow behavior: The implementation accumulates into a signed long with value = value * 10 + digit; and does not perform any overflow/range checking. If the parsed number exceeds what long can 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_t up to 4294967295? No. On the vast majority of Arduino cores, long is 32-bit signed, so the maximum representable value is 2,147,483,647, far below 4,294,967,295; parsing values above LONG_MAX will 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:

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.

Copy link
Member

@softhack007 softhack007 Mar 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@coderabbitai coderabbitai bot Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

}

#endif
8 changes: 8 additions & 0 deletions wled00/cfg_crc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
#pragma once

#ifdef WLED_ENABLE_CRC_FOR_CONFIG
static uint32_t crc32_update(uint32_t crc, uint8_t data);
uint32_t crc32_file(const char* path);
bool saveCRC(const char* path, uint32_t crc);
bool loadCRC(const char* path, uint32_t &crc);
Comment on lines +5 to +7
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 12, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.

Copy link
Member

@softhack007 softhack007 Mar 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is declared as a plain uint32_t, but the implementation uses UINT32_MAX as 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.

Copy link
Contributor

@coderabbitai coderabbitai bot Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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/loadCRC which already return bool.
  • 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).

#endif
Loading
Loading