Conversation
WalkthroughThis PR adds a Safe Boot feature that calculates and maintains CRC checksums for the SPIFFS filesystem partition. It introduces new utility functions and integrates CRC updates into key file operations (config serialization, file closing, and file copying) when the WLED_ENABLE_SAFE_BOOT flag is enabled. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
wled00/safe_boot_functions.h (1)
1-4: Minor typo in comment."Safe Bootbootloader" should be "Safe Bootloader".
-/* - This functions are used to use WLED together with "Safe Bootbootloader" +/* + * These functions are used to use WLED together with "Safe Bootloader"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/safe_boot_functions.h` around lines 1 - 4, Fix the typo in the file header comment in safe_boot_functions.h: change the phrase "Safe Bootbootloader" to "Safe Bootloader" in the top comment block so the description reads correctly; locate the comment block at the top of safe_boot_functions.h and update the text accordingly.wled00/ota_update.cpp (1)
100-102: Indentation inconsistency inside preprocessor block.The function call indentation doesn't match the surrounding code style. As per coding guidelines, use 2 spaces per indentation level.
`#ifdef` WLED_ENABLE_SAFE_BOOT - update_ota_crc(); + update_ota_crc(); `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/ota_update.cpp` around lines 100 - 102, The call to update_ota_crc() inside the `#ifdef` WLED_ENABLE_SAFE_BOOT block has incorrect indentation; adjust it to follow the project's 2-space-per-indent convention so the line with update_ota_crc(); is indented to match surrounding code (inside the same block) and keep the `#ifdef/`#endif aligned with other preprocessor directives; locate the block using the symbol update_ota_crc and the macro WLED_ENABLE_SAFE_BOOT and correct spacing only—no logic changes.wled00/cfg.cpp (1)
836-838: Indentation inconsistency inside preprocessor block.The function call uses 4-space indentation. As per coding guidelines, use 2 spaces per indentation level.
`#ifdef` WLED_ENABLE_SAFE_BOOT - update_spiffs_crc(); + update_spiffs_crc(); `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/cfg.cpp` around lines 836 - 838, The call inside the preprocessor block has a 4-space indent; change it to 2-space indentation to match project style. Locate the WLED_ENABLE_SAFE_BOOT block around the preprocessor directives and adjust the line with update_spiffs_crc() to use 2 spaces (e.g., " update_spiffs_crc();") and ensure the surrounding "#ifdef WLED_ENABLE_SAFE_BOOT" and "#endif" remain aligned with file convention.wled00/presets.cpp (1)
118-120: Indentation inconsistency inside preprocessor block.The function call uses 4-space indentation whereas the codebase guidelines specify 2 spaces per level. As per coding guidelines, C++ files should use 2-space indentation.
`#ifdef` WLED_ENABLE_SAFE_BOOT - update_spiffs_crc(); + update_spiffs_crc(); `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/presets.cpp` around lines 118 - 120, The call to update_spiffs_crc() inside the WLED_ENABLE_SAFE_BOOT preprocessor block is indented with 4 spaces; adjust it to 2-space indentation to match the project's C++ style guidelines—locate the block guarded by `#ifdef` WLED_ENABLE_SAFE_BOOT and change the whitespace before update_spiffs_crc(); to two spaces so it aligns with surrounding code.wled00/safe_boot_functions.cpp (2)
19-39: Makecalc_crcstatic to reduce global namespace pollution.The
calc_crcfunction is only used internally within this file and is not declared in the header. Making itstaticenables better compiler optimizations and prevents symbol conflicts. Based on learnings, C-style functions not declared in global headers should be made static.-uint32_t calc_crc(const esp_partition_t* part) +static uint32_t calc_crc(const esp_partition_t* part)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/safe_boot_functions.cpp` around lines 19 - 39, The calc_crc function is exposed as a global symbol but only used within this translation unit; make it static by adding the static storage-class specifier to its definition (i.e., change the function signature for calc_crc to be static uint32_t calc_crc(...)) so the symbol is internal to the file and allows better optimization and avoids name collisions; update any internal callers if needed (they should continue to call calc_crc as before).
12-17: Use English for code comments.Several comments are in German which reduces accessibility for international contributors:
- Line 16: "Größe SPIFFS / Backup" → "Size SPIFFS / Backup"
- Line 128: "prüfen ob identisch" → "check if identical"
- Line 133: "nichts zu tun" → "nothing to do"
Also applies to: 128-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/safe_boot_functions.cpp` around lines 12 - 17, Update all German inline comments to English: in the crc_group_t typedef update "Größe SPIFFS / Backup" to "Size SPIFFS / Backup" (referencing fields crc_app, crc_spiffs, size_spiffs), and update the comments around the later block where you have "prüfen ob identisch" and "nichts zu tun" to "check if identical" and "nothing to do" respectively; ensure the replaced comments preserve original intent and punctuation and are applied where the CRC/spiffs comparison logic is implemented so all code comments are in English.wled00/file.cpp (1)
485-487: Indentation inconsistency and brace placement.The
elseblock with braces on the same line is inconsistent with typical WLED code style. As per coding guidelines, use 2 spaces per indentation level.`#ifdef` WLED_ENABLE_SAFE_BOOT - else {update_spiffs_crc();} + else { + update_spiffs_crc(); + } `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/file.cpp` around lines 485 - 487, Fix the indentation and brace placement for the conditional guarded by WLED_ENABLE_SAFE_BOOT: move the else onto its own indented line and place the opening brace on the next line, use 2 spaces per indentation level, and ensure the call to update_spiffs_crc() and the closing brace are indented accordingly; update the block that references WLED_ENABLE_SAFE_BOOT and the update_spiffs_crc() call to match WLED coding style.
🤖 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/safe_boot_functions.cpp`:
- Around line 97-98: The calls to esp_partition_erase_range and
esp_partition_write (operating on crcpart with the stored buffer) lack error
checking; update safe_boot_functions.cpp to capture their esp_err_t returns,
check for != ESP_OK, and handle failures (log via your logger, return/propagate
an error code from the surrounding function or abort the CRC update, and avoid
leaving partial data). Additionally, after esp_partition_write, read back or
verify the written data (compare to stored) and treat mismatches as errors so
the caller can retry or fail safely.
- Line 32: The call to esp_partition_read is missing error checking; update the
code that calls esp_partition_read(part, offset, buf, toread) to capture its
return value, verify it equals ESP_OK (or appropriate success constant), and on
failure avoid using buf for CRC calculation—log/report the error (including
esp_err_to_name(err)), propagate/return an error code from the surrounding
function (or abort the safe-boot update flow) and ensure you do not write/store
an invalid CRC; reference esp_partition_read, variables part/offset/buf/toread
and the surrounding safe-boot CRC write path when implementing this handling.
- Around line 142-143: The esp_partition_erase_range and esp_partition_write
calls for the SPIFFS CRC partition (crcpart, 0, 4096 and
esp_partition_write(crcpart, 0, &stored, sizeof(stored))) lack return-value
checks; modify the code to capture their esp_err_t results, verify they equal
ESP_OK (similar to update_ota_crc()), and handle failures by logging the error
(include the returned error code and context) and returning/aborting
appropriately so failures do not go unnoticed when writing stored to crcpart.
---
Nitpick comments:
In `@wled00/cfg.cpp`:
- Around line 836-838: The call inside the preprocessor block has a 4-space
indent; change it to 2-space indentation to match project style. Locate the
WLED_ENABLE_SAFE_BOOT block around the preprocessor directives and adjust the
line with update_spiffs_crc() to use 2 spaces (e.g., " update_spiffs_crc();")
and ensure the surrounding "#ifdef WLED_ENABLE_SAFE_BOOT" and "#endif" remain
aligned with file convention.
In `@wled00/file.cpp`:
- Around line 485-487: Fix the indentation and brace placement for the
conditional guarded by WLED_ENABLE_SAFE_BOOT: move the else onto its own
indented line and place the opening brace on the next line, use 2 spaces per
indentation level, and ensure the call to update_spiffs_crc() and the closing
brace are indented accordingly; update the block that references
WLED_ENABLE_SAFE_BOOT and the update_spiffs_crc() call to match WLED coding
style.
In `@wled00/ota_update.cpp`:
- Around line 100-102: The call to update_ota_crc() inside the `#ifdef`
WLED_ENABLE_SAFE_BOOT block has incorrect indentation; adjust it to follow the
project's 2-space-per-indent convention so the line with update_ota_crc(); is
indented to match surrounding code (inside the same block) and keep the
`#ifdef/`#endif aligned with other preprocessor directives; locate the block using
the symbol update_ota_crc and the macro WLED_ENABLE_SAFE_BOOT and correct
spacing only—no logic changes.
In `@wled00/presets.cpp`:
- Around line 118-120: The call to update_spiffs_crc() inside the
WLED_ENABLE_SAFE_BOOT preprocessor block is indented with 4 spaces; adjust it to
2-space indentation to match the project's C++ style guidelines—locate the block
guarded by `#ifdef` WLED_ENABLE_SAFE_BOOT and change the whitespace before
update_spiffs_crc(); to two spaces so it aligns with surrounding code.
In `@wled00/safe_boot_functions.cpp`:
- Around line 19-39: The calc_crc function is exposed as a global symbol but
only used within this translation unit; make it static by adding the static
storage-class specifier to its definition (i.e., change the function signature
for calc_crc to be static uint32_t calc_crc(...)) so the symbol is internal to
the file and allows better optimization and avoids name collisions; update any
internal callers if needed (they should continue to call calc_crc as before).
- Around line 12-17: Update all German inline comments to English: in the
crc_group_t typedef update "Größe SPIFFS / Backup" to "Size SPIFFS / Backup"
(referencing fields crc_app, crc_spiffs, size_spiffs), and update the comments
around the later block where you have "prüfen ob identisch" and "nichts zu tun"
to "check if identical" and "nothing to do" respectively; ensure the replaced
comments preserve original intent and punctuation and are applied where the
CRC/spiffs comparison logic is implemented so all code comments are in English.
In `@wled00/safe_boot_functions.h`:
- Around line 1-4: Fix the typo in the file header comment in
safe_boot_functions.h: change the phrase "Safe Bootbootloader" to "Safe
Bootloader" in the top comment block so the description reads correctly; locate
the comment block at the top of safe_boot_functions.h and update the text
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d44fab80-1d22-43d3-a834-148e35c26da0
⛔ Files ignored due to path filters (1)
tools/WLED_ESP32_8MB_SafeBoot.csvis excluded by!**/*.csv
📒 Files selected for processing (6)
wled00/cfg.cppwled00/file.cppwled00/ota_update.cppwled00/presets.cppwled00/safe_boot_functions.cppwled00/safe_boot_functions.h
| #ifdef WLED_ENABLE_SAFE_BOOT | ||
| update_spiffs_crc(); | ||
| #endif |
There was a problem hiding this comment.
Critical performance concern: CRC calculation on every file close.
Calling update_spiffs_crc() after every closeFile() will cause significant performance degradation. Based on the implementation in wled00/safe_boot_functions.cpp:19-39, calc_crc() reads the entire SPIFFS partition in 4 KB chunks. For a typical 1-3 MB SPIFFS partition, this means reading megabytes of flash data on every single file close operation.
This will cause:
- Visible LED stuttering during file operations
- Slow preset saves and config writes
- Potential watchdog timeouts on larger partitions
Consider updating the CRC only at strategic points (config save, preset save completion) rather than on every file close, or deferring CRC updates to idle time.
- `#ifdef` WLED_ENABLE_SAFE_BOOT
- update_spiffs_crc();
- `#endif`
doCloseFile = false;
}The CRC updates in cfg.cpp, presets.cpp, and copyFile() should be sufficient for Safe Boot validation.
There was a problem hiding this comment.
Theoretically yes, practially it is fast enogh.
There was a problem hiding this comment.
@wled-install, fair enough — thanks for testing this on actual hardware. The CRC read-and-compute still happens on every close even when no update is needed, but if you've verified it doesn't cause noticeable lag or watchdog issues in practice (likely because ESP32 flash reads at ~40 MHz are fast relative to the partition size), that's a reasonable call to make. Acknowledged!
🧠 Learnings used
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/wled_server.cpp:237-262
Timestamp: 2025-11-08T08:25:18.724Z
Learning: In WLED's filesystem code (ESP8266/ESP32), when iterating directory entries with `openNextFile()`, explicit `close()` calls between iterations are not required. The File class assignment operator properly handles closing the previous file handle when reassigning (e.g., `rootfile = rootdir.openNextFile()`). Both explicit and implicit patterns exist in the codebase, but relying on assignment operator cleanup is valid.
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:35.387Z
Learning: In WLED (wled00/wled_server.cpp), `updateFSInfo()` updates `fsBytesUsed`/`fsBytesTotal` which are purely informational ("FYI") display values for the user. No synchronization or locking is required around these calls, even when invoked from async request handlers, because: (1) the values do not need to be 100% accurate, and (2) simultaneous multi-file delete/add operations are extremely improbable on this embedded IoT platform.
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), `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: 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: blazoncek
Repo: wled/WLED PR: 4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
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: 5048
File: wled00/wled.cpp:385-387
Timestamp: 2026-02-08T19:33:02.648Z
Learning: In WLED ESP32 code, when checking whether USB CDC serial initialization delay is needed, rely on the ARDUINO_USB_CDC_ON_BOOT flag rather than explicit CONFIG_IDF_TARGET checks, because not all development boards for chips with native USB capability (ESP32-C3/C5/C6/S2/S3/H2/P4) actually use the native USB—many have external UART bridge chips instead. The ARDUINO_USB_CDC_ON_BOOT flag correctly indicates the board's actual USB configuration.
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).
Learnt from: softhack007
Repo: wled/WLED PR: 4893
File: wled00/set.cpp:95-95
Timestamp: 2026-03-14T20:56:46.543Z
Learning: Guideline: Ensure WiFi hostname is set after WiFi.mode() but before WiFi.begin() to avoid default esp-XXXXXX hostname being used in DHCP. This ordering only takes effect after the STA interface exists (so avoid placing hostname setting before initConnection). In WLED, place the hostname configuration inside initConnection() (after WiFi.disconnect(true) and before WiFi.begin()) rather than in earlier boot code like deserializeConfig(). This rule should be applied in code reviews for WLED’s network initialization paths in wled00/*.cpp, and note that on ESP8266 the ordering is less strict but still desirable for consistency.
There was a problem hiding this comment.
Consider updating the CRC only at strategic points (config save, preset save completion) rather than on every file close, or deferring CRC updates to idle time.
@wled-install I still think this is a valid concern - especially when users have many leds, frequent flash file reads can lead to leds flickering, or effects slowing down randomly. Also there are boards with 16mb flash (9MB LittleFS) or even 32MB flash (24MB LittleFS) where it might actually cause a slow-down.
Not sure if you found all the file closing locations in the code - ledmaps/gapmaps, webserver up/download, and gif player do file access directly, without relying on file.cpp functions.
The first optimization could be to only update CRCs after file write closing, but not on file read closing.
Or - depending on how mission critical these checksums are - create a global flag crc_dirty and delay updating the checksums into the main loop, maybe only every 15 seconds.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
can you explain what it does and how it has an advantage over the bootloop detector? |
SafeBootloaderBootloader for the ESP32 that maintains mirrored OTA (ota_0 / ota_1) and SPIFFS partitions and restores them automatically if corruption is detected. It is intended to prevent corruption during operation (not 100%, but minimizes the risk), it does not help when corruption happens during intended flash write operations as it calculates crc from data already written. How It Works
Bootloop detector will not react if random changes lead to technically valid (but logiclaly potentially wrong) sw execution. SafeBootloader calculates checksum for entire partitions. |
I think such a crc check is already built into the bootloader and it will flag a corrupted ota partition as invalid and boot from the other one (if available). |
Good point, I checked and WLED bins actually have SHA-256 at the end. I thought they have just standard XOR signature for segments as SHA is actually optionally without secure boot activated. Ok, I can probably can use existing SHA. Mirroring OTA partitions is still useful; alternatively, the user could perform the OTA update twice to have identical software in both partitions, or initially flash the same software into both partitions. Headers are definately protected intrinsically. Partition with ota info data (not the app) is also protected intrinsically (should have two data records).
That way I only have to ensure bootloader and partition table integrity by external measures (if will be considered as necessary). |
|
Integrity of OTA partitions are now checked using standard SHA256 included. No need to update something additionally after OTA. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
wled00/safe_boot_functions.h (2)
2-3: Fix the SafeBootloader comment typo.The header comment currently says
Safe Bootbootloader; please correct the wording for readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/safe_boot_functions.h` around lines 2 - 3, Update the header comment in safe_boot_functions.h to correct the typo "Safe Bootbootloader" to "Safe Bootloader" (replace the exact phrase "Safe Bootbootloader") so the top-of-file description reads correctly; also ensure the surrounding sentence remains grammatically correct after this replacement.
9-12: Minimize header coupling by moving ESP includes to the.cppfile.This header only declares
update_spiffs_crc(). Keeping ESP-IDF includes insafe_boot_functions.cppreduces transitive dependencies for all includers.♻️ Suggested simplification
`#pragma` once `#ifdef` WLED_ENABLE_SAFE_BOOT -#include <esp_partition.h> -#include <esp_spi_flash.h> -#include <rom/crc.h> -#include <esp_ota_ops.h> bool update_spiffs_crc(); `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/safe_boot_functions.h` around lines 9 - 12, The header safe_boot_functions.h currently pulls in ESP-IDF headers; remove/includes of esp_partition.h, esp_spi_flash.h, rom/crc.h, and esp_ota_ops.h from the .h and instead include them only in safe_boot_functions.cpp where update_spiffs_crc() is defined; keep the header minimal by declaring only the update_spiffs_crc() prototype (and any standard includes it actually needs), and if any ESP types are required in the signature, replace them with forward-declared types or standard types in the header and use the real ESP headers in the .cpp implementation of update_spiffs_crc().wled00/safe_boot_functions.cpp (3)
20-21: Replace repeated4096literals with named constants.Using named constants here improves readability and avoids accidental drift between chunk size and erase size usage.
Based on learnings: "In WLED code reviews, when code is modified or added, look for 'magic numbers' ... suggest replacing them with defined constants when meaningful constants exist."♻️ Suggested cleanup
`#define` SPIFFS_CRC_MAGIC 0x53504653 +static constexpr size_t kCrcChunkSize = 4096; +static constexpr size_t kCrcEraseSize = 4096; @@ - const size_t bufsize = 4096; + const size_t bufsize = kCrcChunkSize; @@ - esp_partition_erase_range(crcpart, 0, 4096); + esp_partition_erase_range(crcpart, 0, kCrcEraseSize);Also applies to: 79-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/safe_boot_functions.cpp` around lines 20 - 21, Replace the repeated 4096 magic number with a named constant: declare a single constant (e.g., constexpr size_t BOOT_BUF_SIZE = 4096 or constexpr size_t CHUNK_SIZE = 4096) near the top of safe_boot_functions.cpp and use that constant for the bufsize declaration and the buf array (uint8_t buf[BOOT_BUF_SIZE]) and anywhere else the 4096 value is used (including the other occurrence around line 79) to keep chunk/erase sizes consistent and readable; update references to bufsize/buf to use the new constant name.
40-84: Consider documenting this approach with a short software FMEA.Given the reliability trade-off discussion in this PR, a compact FMEA in PR notes/docs would make the failure-mode coverage explicit and easier to evaluate.
Based on learnings: "When a PR discussion involves 'is this worth doing?' uncertainty about reliability/safety/data-integrity mechanisms, suggest performing a software FMEA."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/safe_boot_functions.cpp` around lines 40 - 84, Add a short software FMEA documenting the SPIFFS CRC update approach used by update_spiffs_crc (and the crc_group_t/CRC partition logic): list likely failure modes (e.g., esp_partition_find_first returns null, calc_crc mismatch, esp_partition_write or erase failures, power loss during update), describe effects (lost/mismatched CRC, boot failures, corrupted data), assign severity/occurrence/detectability and resulting risk priority, and note mitigations and detection controls (null checks and early returns, verify esp_partition_read/write return values, retries, journaling/atomic swap, CRC verification after write, power-loss handling) plus residual risk and recommended docs/tests; include this FMEA in the PR notes/docs so reviewers can evaluate reliability trade-offs.
18-18: Limitcalc_crcto translation-unit scope.
calc_crcis a local helper and can bestaticto avoid unnecessary global symbol exposure.Based on learnings: "In WLED code reviews, when C-style functions or variables are added or modified, check if they are globally visible (not static)... suggest making it static if not intended to be global."🔧 Suggested change
-uint32_t calc_crc(const esp_partition_t* part) +static uint32_t calc_crc(const esp_partition_t* part)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/safe_boot_functions.cpp` at line 18, Make the local helper function calc_crc have translation-unit scope by marking it static: change the function declaration/definition of calc_crc in safe_boot_functions.cpp to static uint32_t calc_crc(const esp_partition_t* part) and ensure there are no external references expecting a global symbol (if there are, either update callers or keep it non-static); this confines the symbol to the file and prevents unnecessary global exposure.
🤖 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/safe_boot_functions.cpp`:
- Around line 12-84: The code in crc_group_t, calc_crc, and update_spiffs_crc
uses 4-space indentation; reformat these blocks to use 2 spaces per indentation
level to match repository C++ style (apply to the whole function bodies and any
nested scopes, including the while loop in calc_crc and the if/else blocks in
update_spiffs_crc), keeping existing line breaks and logic intact and ensuring
alignment for statements like esp_partition_find_first,
esp_partition_read/write, esp_partition_erase_range and the DEBUG_PRINTLN calls.
---
Nitpick comments:
In `@wled00/safe_boot_functions.cpp`:
- Around line 20-21: Replace the repeated 4096 magic number with a named
constant: declare a single constant (e.g., constexpr size_t BOOT_BUF_SIZE = 4096
or constexpr size_t CHUNK_SIZE = 4096) near the top of safe_boot_functions.cpp
and use that constant for the bufsize declaration and the buf array (uint8_t
buf[BOOT_BUF_SIZE]) and anywhere else the 4096 value is used (including the
other occurrence around line 79) to keep chunk/erase sizes consistent and
readable; update references to bufsize/buf to use the new constant name.
- Around line 40-84: Add a short software FMEA documenting the SPIFFS CRC update
approach used by update_spiffs_crc (and the crc_group_t/CRC partition logic):
list likely failure modes (e.g., esp_partition_find_first returns null, calc_crc
mismatch, esp_partition_write or erase failures, power loss during update),
describe effects (lost/mismatched CRC, boot failures, corrupted data), assign
severity/occurrence/detectability and resulting risk priority, and note
mitigations and detection controls (null checks and early returns, verify
esp_partition_read/write return values, retries, journaling/atomic swap, CRC
verification after write, power-loss handling) plus residual risk and
recommended docs/tests; include this FMEA in the PR notes/docs so reviewers can
evaluate reliability trade-offs.
- Line 18: Make the local helper function calc_crc have translation-unit scope
by marking it static: change the function declaration/definition of calc_crc in
safe_boot_functions.cpp to static uint32_t calc_crc(const esp_partition_t* part)
and ensure there are no external references expecting a global symbol (if there
are, either update callers or keep it non-static); this confines the symbol to
the file and prevents unnecessary global exposure.
In `@wled00/safe_boot_functions.h`:
- Around line 2-3: Update the header comment in safe_boot_functions.h to correct
the typo "Safe Bootbootloader" to "Safe Bootloader" (replace the exact phrase
"Safe Bootbootloader") so the top-of-file description reads correctly; also
ensure the surrounding sentence remains grammatically correct after this
replacement.
- Around line 9-12: The header safe_boot_functions.h currently pulls in ESP-IDF
headers; remove/includes of esp_partition.h, esp_spi_flash.h, rom/crc.h, and
esp_ota_ops.h from the .h and instead include them only in
safe_boot_functions.cpp where update_spiffs_crc() is defined; keep the header
minimal by declaring only the update_spiffs_crc() prototype (and any standard
includes it actually needs), and if any ESP types are required in the signature,
replace them with forward-declared types or standard types in the header and use
the real ESP headers in the .cpp implementation of update_spiffs_crc().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e823f8cc-348d-4340-99f1-3bba20c8935d
📒 Files selected for processing (2)
wled00/safe_boot_functions.cppwled00/safe_boot_functions.h
| typedef struct { | ||
| uint32_t magic; | ||
| uint32_t crc_spiffs; // CRC SPIFFS / Backup | ||
| uint32_t size_spiffs; // Größe SPIFFS / Backup | ||
| } crc_group_t; | ||
|
|
||
| uint32_t calc_crc(const esp_partition_t* part) | ||
| { | ||
| const size_t bufsize = 4096; | ||
| uint8_t buf[bufsize]; | ||
|
|
||
| uint32_t crc = 0; | ||
| size_t offset = 0; | ||
|
|
||
| while (offset < part->size) { | ||
| size_t toread = bufsize; | ||
| if (offset + toread > part->size) | ||
| toread = part->size - offset; | ||
|
|
||
| esp_partition_read(part, offset, buf, toread); | ||
|
|
||
| crc = crc32_le(crc, buf, toread); | ||
| offset += toread; | ||
| } | ||
|
|
||
| return crc; | ||
| } | ||
|
|
||
| bool update_spiffs_crc() | ||
| { | ||
| const esp_partition_t* spiffs = | ||
| esp_partition_find_first( | ||
| ESP_PARTITION_TYPE_DATA, | ||
| ESP_PARTITION_SUBTYPE_DATA_SPIFFS, | ||
| "spiffs"); | ||
|
|
||
| const esp_partition_t* crcpart = | ||
| esp_partition_find_first( | ||
| ESP_PARTITION_TYPE_DATA, | ||
| static_cast<esp_partition_subtype_t>(0x41), // crc subtype | ||
| "crc0"); | ||
|
|
||
| if (!spiffs || !crcpart) { | ||
| DEBUG_PRINTLN(F("SPIFFS or CRC partition not found!")); | ||
| return false; | ||
| } | ||
|
|
||
|
|
||
| uint32_t crc = calc_crc(spiffs); | ||
|
|
||
| crc_group_t stored = {}; | ||
| esp_partition_read(crcpart, 0, &stored, sizeof(stored)); | ||
|
|
||
| // prüfen ob identisch | ||
| if (stored.magic == SPIFFS_CRC_MAGIC && | ||
| stored.crc_spiffs == crc && | ||
| stored.size_spiffs == spiffs->size) | ||
| { | ||
| // nichts zu tun | ||
| DEBUG_PRINTLN(F("SPIFFS CRC matches stored value, no update needed.")); | ||
| return true; | ||
| } | ||
|
|
||
| stored.magic = SPIFFS_CRC_MAGIC; | ||
| stored.crc_spiffs = crc; | ||
| stored.size_spiffs = spiffs->size; | ||
| DEBUG_PRINTLN(F("Updating SPIFFS CRC partition with new values.")); | ||
| esp_partition_erase_range(crcpart, 0, 4096); | ||
| esp_partition_write(crcpart, 0, &stored, sizeof(stored)); | ||
| DEBUG_PRINTLN(F("SPIFFS CRC partition updated.")); | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
Indentation does not follow the repository C++ style rule.
Line 12 onward uses 4-space indentation in nested scopes; this repository guideline requires 2 spaces per level.
As per coding guidelines **/*.{cpp,h,c}: Use spaces (2 per level) for indentation in C++ files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/safe_boot_functions.cpp` around lines 12 - 84, The code in
crc_group_t, calc_crc, and update_spiffs_crc uses 4-space indentation; reformat
these blocks to use 2 spaces per indentation level to match repository C++ style
(apply to the whole function bodies and any nested scopes, including the while
loop in calc_crc and the if/else blocks in update_spiffs_crc), keeping existing
line breaks and logic intact and ensuring alignment for statements like
esp_partition_find_first, esp_partition_read/write, esp_partition_erase_range
and the DEBUG_PRINTLN calls.
| crc_group_t stored = {}; | ||
| esp_partition_read(crcpart, 0, &stored, sizeof(stored)); | ||
|
|
||
| // prüfen ob identisch |
There was a problem hiding this comment.
minor: please translate german comments into english
| stored.crc_spiffs == crc && | ||
| stored.size_spiffs == spiffs->size) | ||
| { | ||
| // nichts zu tun |
There was a problem hiding this comment.
same minor comment here: german -> english please
|
I still fail to see what the code is trying to achieve. |
4 participants
Add option to use https://github.com/wled-install/SafeBootloader
If it is too specific, maybe just generic hooks can be implemented on relevant places as a part of usermod infrastructure and functionality can be then implemented as usermod.
Summary by CodeRabbit
Release Notes