Fix bootloop if config is reset #4852
Conversation
Can't reset the config if there's nothing to reset!
WalkthroughAdds file-existence guarding and PROGMEM-safe backup path formatting for config/backups, validates JSON before creating backups, and persists bootloop action tracker to ESP8266 RTC memory after handling bootloop actions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
wled00/cfg.cpp (1)
794-799: Bounded/PROGMEM-safe string build and only reboot on successful rename.Two tweaks improve robustness:
- Build backupname safely (bounded, and PROGMEM-aware) to avoid accidental overruns and flash/RAM copy pitfalls.
- Only set doReboot when the rename actually succeeds (destination may already exist).
Apply this diff:
- char backupname[32]; - strcpy(backupname, s_cfg_json); - strcat(backupname, ".rst.json"); - WLED_FS.rename(s_cfg_json, backupname); - doReboot = true; + char backupname[32]; + // Build "/cfg.json.rst.json" safely; s_cfg_json is PROGMEM. + snprintf_P(backupname, sizeof(backupname), PSTR("%s.rst.json"), s_cfg_json); + if (WLED_FS.rename(s_cfg_json, backupname)) { + doReboot = true; + } else { + DEBUG_PRINTLN(F("Reset config: rename failed (dest exists?)")); + }wled00/util.cpp (1)
839-842: Minor consistency note: reset rename target differs from comment.The bootloop comment (Line 725) says “rename /cfg.json to /cfg.fault”, while resetConfig() currently renames to “/cfg.json.rst.json”. Not a functional blocker, but consider aligning the comment or the filename for clarity across the bootloop flow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
wled00/cfg.cpp(1 hunks)wled00/util.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: DedeHai
PR: wled/WLED#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.
📚 Learning: 2025-07-31T18:21:49.868Z
Learnt from: DedeHai
PR: wled/WLED#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.
Applied to files:
wled00/cfg.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
🔇 Additional comments (2)
wled00/cfg.cpp (1)
792-799: Good guard: avoids pointless reboot when no cfg.json is present.Early-returning unless the config file exists is the right fix to prevent bootloops on fresh/missing-config devices.
wled00/util.cpp (1)
839-842: LGTM: persist bootloop action on ESP8266 before restart.Writing bl_actiontracker back to RTC user memory fixes the state-loss bug on ESP8266 between reboots.
Use 'rst.cfg.json' instead of 'cfg.json.rst.json' for configs that were reset.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wled00/util.cpp (1)
724-726: Fix comment to match actual backup namingThe comment in util.cpp still refers to “/bak.cfg.json”, but the code uses the “/bkp.%s” format for backups. Update the comment at:
- wled00/util.cpp:724
Apply this diff:
-#define BOOTLOOP_ACTION_RESTORE 0 // default action: restore config from /bak.cfg.json +#define BOOTLOOP_ACTION_RESTORE 0 // default action: restore config from /bkp.cfg.json
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
wled00/cfg.cpp(1 hunks)wled00/file.cpp(4 hunks)wled00/util.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/cfg.cpp
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-31T18:21:49.868Z
Learnt from: DedeHai
PR: wled/WLED#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.
Applied to files:
wled00/file.cpp
🔇 Additional comments (3)
wled00/util.cpp (1)
839-841: ESP8266: Persisting bl_actiontracker to RTC memory — good fix.Writing the updated action tracker back to RTC before restart ensures the next boot executes the correct next-step action.
wled00/file.cpp (2)
568-571: PROGMEM-safe debug strings for JSON validation — LGTM.Switching to DEBUG_PRINTF_P with PSTR keeps strings in flash and avoids RAM overhead.
518-541: Increase backup buffer and verify callersThe 32-byte
backupnamerisks truncation with longer filenames. Please:
- Bump the buffer to at least 64 bytes in both
backupFileandrestoreFile.- Retain the PROGMEM format and
snprintf_P(..., filename+1).- Manually audit every
backupFile()/restoreFile()call to ensure the argument always starts with “/”, since you strip that leading slash.--- a/wled00/file.cpp @@ -522,7 +522,7 @@ bool backupFile(const char* filename) { - char backupname[32]; + char backupname[64]; --- a/wled00/file.cpp @@ -536,7 +536,7 @@ bool restoreFile(const char* filename) { - char backupname[32]; + char backupname[64];
|
Don't forget to cherry pick to 0_15_x too (if / when we back port this feature) |
A couple of small fixes for #4793:
validConfig()returns false every time. Fix this by disabling the reboot inresetConfig()if we already had no config.This is the most likely cause of #4848 - once an invalid config is reset, the device is stuck in a bootloop.
Summary by CodeRabbit
Bug Fixes
Improvements
Chores