Backport of Bootloop detection & recovery (#4793) #4840
Conversation
* added boot loop detection and config backup * automatic OTA rollback if loading backup does not fix it * added new file handling functions * adding verification of json files, added config restore at bootup if broken * added function to compare contents of two files for future use (currently not used)
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds config backup/restore/verify/reset helpers, expands public file/JSON I/O APIs, introduces filesystem utilities (copy/backup/restore/validate/dump), implements bootloop detection and recovery logic, and wires these into boot and OTA flows. Setup now attempts config recovery on validation failure, and OTA unconditionally backs up config and reports bootloop status. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
still need to resolve the conflicts for V3 builds. edit: |
|
@willmmiles can your updates to main be easily ported to this by cherry picking? |
Can't reset the config if there's nothing to reset!
Use 'rst.cfg.json' instead of 'cfg.json.rst.json' for configs that were reset.
ESP8266 RTC RAM requires 32-bit accesses, but there's no need to jump through a bunch of functions for it. Use references to simplify access and harmonize the implementation with ESP32.
Separate the platform-specific code from the logic, so any future changes can be made in fewer places.
Don't treat consecutive but infrequent crashes as bootloops. The bootloop recovery actions only make sense when there is no opportunity for a user to reconfigure their system. Suggested by @coderabbitai
Any repeating crash that prevents a human from logging in and fixing the config should be treated as a boot loop. Increase the detection timeout, so anything that's fast enough to preclude a user fix will trigger the recovery behaviour.
|
Updates pushed. I haven't tested it yet, but there were no merge conflicts. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
wled00/fcn_declare.h (1)
416-419: Remove duplicate declarations.These declarations are identical to lines 411-414. As acknowledged in previous review comments, this is a merge artifact that should be removed.
Apply this diff to remove the duplicate:
- -void handleBootLoop(); // detect and handle bootloops -#ifndef ESP8266 -void bootloopCheckOTA(); // swap boot image if bootloop is detected instead of restoring config -#endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
wled00/cfg.cpp(2 hunks)wled00/fcn_declare.h(3 hunks)wled00/file.cpp(1 hunks)wled00/util.cpp(2 hunks)wled00/wled.cpp(2 hunks)wled00/wled_server.cpp(2 hunks)
🔇 Additional comments (2)
wled00/fcn_declare.h (2)
27-30: LGTM! New config management API declarations.These declarations appropriately support the bootloop detection and recovery feature.
125-129: LGTM! New filesystem utility declarations.These declarations appropriately expand the file I/O API to support config backup, restore, and validation operations.
| inline bool readObjectFromFileUsingId(const String &file, uint16_t id, JsonDocument* dest, const JsonDocument* filter = nullptr) { return readObjectFromFileUsingId(file.c_str(), id, dest); }; | ||
| inline bool readObjectFromFile(const String &file, const char* key, JsonDocument* dest, const JsonDocument* filter = nullptr) { return readObjectFromFile(file.c_str(), key, dest); }; |
There was a problem hiding this comment.
Critical: Filter parameter not forwarded to underlying functions.
The new filter parameter in the inline wrappers is declared but not passed through to the underlying function calls, silently discarding it.
Apply this diff to forward the filter parameter:
-inline bool readObjectFromFileUsingId(const String &file, uint16_t id, JsonDocument* dest, const JsonDocument* filter = nullptr) { return readObjectFromFileUsingId(file.c_str(), id, dest); };
-inline bool readObjectFromFile(const String &file, const char* key, JsonDocument* dest, const JsonDocument* filter = nullptr) { return readObjectFromFile(file.c_str(), key, dest); };
+inline bool readObjectFromFileUsingId(const String &file, uint16_t id, JsonDocument* dest, const JsonDocument* filter = nullptr) { return readObjectFromFileUsingId(file.c_str(), id, dest, filter); };
+inline bool readObjectFromFile(const String &file, const char* key, JsonDocument* dest, const JsonDocument* filter = nullptr) { return readObjectFromFile(file.c_str(), key, dest, filter); };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| inline bool readObjectFromFileUsingId(const String &file, uint16_t id, JsonDocument* dest, const JsonDocument* filter = nullptr) { return readObjectFromFileUsingId(file.c_str(), id, dest); }; | |
| inline bool readObjectFromFile(const String &file, const char* key, JsonDocument* dest, const JsonDocument* filter = nullptr) { return readObjectFromFile(file.c_str(), key, dest); }; | |
| inline bool readObjectFromFileUsingId(const String &file, uint16_t id, JsonDocument* dest, const JsonDocument* filter = nullptr) { | |
| return readObjectFromFileUsingId(file.c_str(), id, dest, filter); | |
| }; | |
| inline bool readObjectFromFile(const String &file, const char* key, JsonDocument* dest, const JsonDocument* filter = nullptr) { | |
| return readObjectFromFile(file.c_str(), key, dest, filter); | |
| }; |
🤖 Prompt for AI Agents
In wled00/fcn_declare.h around lines 123 to 124, the inline overloads declare a
filter parameter but do not pass it to the underlying function calls; update the
two inline functions to forward the filter argument to their respective
underlying functions (i.e., call readObjectFromFileUsingId(file.c_str(), id,
dest, filter) and readObjectFromFile(file.c_str(), key, dest, filter)) so the
filter is not dropped.
There was a problem hiding this comment.
this is not a change introduced in this PR
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the actual implementation of readObjectFromFileUsingId and readObjectFromFile in file.cpp
echo "=== Searching for readObjectFromFileUsingId implementations ==="
ast-grep --pattern 'bool readObjectFromFileUsingId($$$) {
$$$
}'
echo -e "\n=== Searching for readObjectFromFile implementations ==="
ast-grep --pattern 'bool readObjectFromFile($$$) {
$$$
}'
echo -e "\n=== Using ripgrep to find all declarations/definitions with filter parameter ==="
rg -n "readObjectFromFile.*filter" --type=cpp -C 2
Length of output: 1145
🏁 Script executed:
#!/bin/bash
# Check if the underlying function declarations have been updated elsewhere in this PR
echo "=== Check all readObjectFromFile declarations in fcn_declare.h ==="
rg -n "bool readObjectFromFile" wled00/fcn_declare.h
echo -e "\n=== Check file.cpp for function signatures ==="
rg -n "^bool readObjectFromFile" wled00/file.cpp -A 1
echo -e "\n=== Search for any changes to these function signatures in git history ==="
git log --oneline --all -20 | head -20
echo -e "\n=== Check current branch ==="
git branch --show-current
Length of output: 1976
🏁 Script executed:
#!/bin/bash
# Check the main branch to see if filter parameter exists there
echo "=== Fetch main branch to compare ==="
git fetch origin main 2>&1 | head -5
echo -e "\n=== Check if filter parameter exists on main branch ==="
git show origin/main:wled00/fcn_declare.h | grep -n -A 2 -B 2 "readObjectFromFile.*filter" || echo "No filter parameter found on main branch"
echo -e "\n=== Check the 0_15_x branch ==="
git show origin/0_15_x:wled00/fcn_declare.h | grep -n -A 2 -B 2 "readObjectFromFile.*filter" || echo "No filter parameter found on 0_15_x branch"
echo -e "\n=== Check what the inline wrappers look like on 0_15_x before this PR ==="
git show origin/0_15_x:wled00/fcn_declare.h | grep -n -A 1 "inline bool readObjectFromFile"
Length of output: 1705
🏁 Script executed:
#!/bin/bash
# Check if the underlying function declarations in THIS branch have filter parameter
echo "=== Current branch underlying function declarations ==="
sed -n '114,118p' wled00/fcn_declare.h
echo -e "\n=== Main branch underlying function declarations ==="
git show origin/main:wled00/fcn_declare.h | sed -n '94,97p'
echo -e "\n=== Check file.cpp implementations on main ==="
git show origin/main:wled00/file.cpp | grep -n "^bool readObjectFromFile" -A 1
Length of output: 1203
@DedeHai, you're correct that the bug of not forwarding the filter parameter exists on the main branch as well. However, there's an important difference:
On main branch:
- Underlying functions (
readObjectFromFileUsingIdandreadObjectFromFileat lines 96-97) do support thefilterparameter - The inline wrappers fail to forward it (a bug, but the code compiles)
On this branch (0_15_x_bootloopdetect):
- Underlying functions (lines 117-118) do not support the
filterparameter at all - The inline wrappers accept
filterbut cannot forward it because the underlying functions don't have that parameter
This appears to be an incomplete backport. The diff shows this PR adds the filter parameter to the inline wrappers, but the corresponding updates to the underlying function declarations and implementations in file.cpp weren't backported.
Options to resolve:
- Remove the
filterparameter from the inline wrappers in this PR (revert to match the underlying functions) - Complete the backport by also updating the underlying function declarations and implementations to support filtering
Which approach would you prefer for this backport?
🧬 Code Graph Analysis Results
-
File: wled00/fcn_declare.h, lines 9-16
- Full snippet:
void onAlexaChange(EspalexaDevice* dev);
- Full snippet:
-
File: wled00/fcn_declare.h, lines 17-24
- Full snippet:
void alexaInit();
void handleAlexa();
void onAlexaChange(EspalexaDevice* dev);
- Full snippet:
-
Note: There is a duplicate declaration of onAlexaChange(EspalexaDevice* dev) in this header (appearing twice) under the alexa.cpp section, which aligns with the user’s comment about a change not introduced in this PR.
🧠 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.
In my test on ESP32 the tracker startet out as a huge number, this brings it back on track
|
@netmindz I tested this and fixed an issue found on ESP32 on V3 in the latest commit. This is now ready to merge. Not sure what the rabbit is going on about... it may be right but it has nothing to do with this PR. |
|
Can you resolve the conflict please @DedeHai and are we then good to merge? |
4 participants
Summary by CodeRabbit
New Features
Bug Fixes