-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Bootloop detection platform factoring #4853
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.
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
Bootloop detection platform factoring #4853
Conversation
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.
WalkthroughReworks bootloop handling in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (5)
wled00/util.cpp (5)
730-737
: Use a smaller underlying type for ResetReasonSaving RTC/DRAM bytes and keeping ABI clear helps; enum class ResetReason can use an 8-bit underlying type.
-enum class ResetReason { +enum class ResetReason : uint8_t { Power, Software, Crash, Brownout };
739-745
: ESP8266 RTC slots: nice move away from OTA area; add a no-op bootloopCheckOTA stub for symmetryGood call placing variables past the first 128B. To simplify call sites and avoid #ifdefs elsewhere, provide an ESP8266 no-op stub for bootloopCheckOTA.
static volatile uint32_t& bl_last_boottime = *(RTC_USER_MEM + 32); static volatile uint32_t& bl_crashcounter = *(RTC_USER_MEM + 33); static volatile uint32_t& bl_actiontracker = *(RTC_USER_MEM + 34); static inline ResetReason rebootReason() { rst_info* resetreason = system_get_rst_info(); if (resetreason->reason == REASON_EXCEPTION_RST || resetreason->reason == REASON_WDT_RST) return ResetReason::Crash; if (resetreason->reason == REASON_SOFT_RESTART) return ResetReason::Software; return ResetReason::Power; } -static inline uint32_t getRtcMillis() { return system_get_rtc_time() / 160; }; // rtc ticks ~160000Hz +static inline uint32_t getRtcMillis() { return system_get_rtc_time() / 160; } // rtc ticks ~160000Hz + +// Keep symbol available across platforms; ESP8266 has no rollback support. +static inline void bootloopCheckOTA() {} // no-op on ESP8266
753-754
: Remove superfluous trailing semicolonTiny nit: extraneous semicolon after function definition.
-static inline uint32_t getRtcMillis() { return system_get_rtc_time() / 160; }; // rtc ticks ~160000Hz +static inline uint32_t getRtcMillis() { return system_get_rtc_time() / 160; } // rtc ticks ~160000Hz
823-846
: Handle OTA rollback absence explicitly; sequencing LGTMThe action sequencing is clear. When rollback isn’t available, a log helps. Keep incrementing the action as you already do.
case BOOTLOOP_ACTION_OTA: #ifndef ESP8266 - if(Update.canRollBack()) { + if (Update.canRollBack()) { DEBUG_PRINTLN(F("Swapping boot partition...")); Update.rollBack(); // swap boot partition + } else { + DEBUG_PRINTLN(F("No rollback image available; continuing to next action")); } ++bl_actiontracker; break; #else // fall through #endif
723-729
: Backup filename migration verified
- No remaining references to
/bak.cfg.json
; all restore paths now use/bkp.cfg.json
.Optional refactor: use constexpr
- In wled00/util.cpp (lines 723–729), consider replacing the BOOTLOOP_* macros with
constexpr uint32_t
for improved type safety and scoping.
📜 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 (1)
wled00/util.cpp
(1 hunks)
🔇 Additional comments (4)
wled00/util.cpp (4)
761-767
: ESP32 reset reason mapping looks goodCovers PANIC and WDT classes and brownout distinctly; sensible defaults fall back to Power.
769-774
: RTC time abstraction for ESP32 is appropriateCorrectly picks esp_rtc_get_time_us() for IDF >= 4.4 and falls back to rtc_time_slowclk_to_us for 3.3.x.
820-822
: Debug line is helpful and safeGood to log the persisted values before detection. Note: value may be stale on cold power-on, but it’s only for diagnostics.
775-776
: bootloopCheckOTA is properly declared and guarded
- wled00/fcn_declare.h declares
void bootloopCheckOTA()
inside#ifndef ESP8266
- wled00/wled_server.cpp wraps the
bootloopCheckOTA()
call in#ifndef ESP8266
No further changes needed.
static inline ResetReason rebootReason() { | ||
rst_info* resetreason = system_get_rst_info(); | ||
if (resetreason->reason == REASON_EXCEPTION_RST || resetreason->reason == REASON_WDT_RST) return ResetReason::Crash; | ||
if (resetreason->reason == REASON_SOFT_RESTART) return ResetReason::Software; | ||
return ResetReason::Power; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Include soft WDT resets in Crash classification on ESP8266
Soft WDT resets are a common bootloop cause on 8266 and should be treated as Crash.
static inline ResetReason rebootReason() {
rst_info* resetreason = system_get_rst_info();
- if (resetreason->reason == REASON_EXCEPTION_RST || resetreason->reason == REASON_WDT_RST) return ResetReason::Crash;
+ if (resetreason->reason == REASON_EXCEPTION_RST || resetreason->reason == REASON_WDT_RST || resetreason->reason == REASON_SOFT_WDT_RST) return ResetReason::Crash;
if (resetreason->reason == REASON_SOFT_RESTART) return ResetReason::Software;
return ResetReason::Power;
}
📝 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.
static inline ResetReason rebootReason() { | |
rst_info* resetreason = system_get_rst_info(); | |
if (resetreason->reason == REASON_EXCEPTION_RST || resetreason->reason == REASON_WDT_RST) return ResetReason::Crash; | |
if (resetreason->reason == REASON_SOFT_RESTART) return ResetReason::Software; | |
return ResetReason::Power; | |
} | |
static inline ResetReason rebootReason() { | |
rst_info* resetreason = system_get_rst_info(); | |
if (resetreason->reason == REASON_EXCEPTION_RST | |
|| resetreason->reason == REASON_WDT_RST | |
|| resetreason->reason == REASON_SOFT_WDT_RST) | |
return ResetReason::Crash; | |
if (resetreason->reason == REASON_SOFT_RESTART) | |
return ResetReason::Software; | |
return ResetReason::Power; | |
} |
🤖 Prompt for AI Agents
In wled00/util.cpp around lines 746 to 752, the rebootReason() function
currently treats only REASON_EXCEPTION_RST and REASON_WDT_RST as Crash; update
it to also treat the soft WDT reset constant (e.g. REASON_SOFT_WDT_RST) as
Crash. Also defensively null-check the pointer returned from
system_get_rst_info() before accessing its fields to avoid potential crashes.
Modify the if conditions to include REASON_SOFT_WDT_RST in the Crash branch and
add a null check that returns a sensible default (e.g. Power) if
system_get_rst_info() returns nullptr.
Cant review in detail or test rn, but looks like solid changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (3)
wled00/util.cpp (3)
746-756
: Defensive null-check for system_get_rst_info()Add a null-check to avoid a rare deref crash in early-boot edge cases.
Apply:
-static inline ResetReason rebootReason() { - uint32_t resetReason = system_get_rst_info()->reason; +static inline ResetReason rebootReason() { + const rst_info* info = system_get_rst_info(); + if (!info) return ResetReason::Power; // defensive default + uint32_t resetReason = info->reason; if (resetReason == REASON_EXCEPTION_RST || resetReason == REASON_WDT_RST || resetReason == REASON_SOFT_WDT_RST) return ResetReason::Crash; if (resetReason == REASON_SOFT_RESTART) return ResetReason::Software; return ResetReason::Power; }
746-751
: Soft WDT is correctly classified as CrashGood catch including
REASON_SOFT_WDT_RST
.
797-809
: Reset crash counter when reboot interval is longWithout resetting on long intervals, non-consecutive short crashes can accumulate and falsely trigger bootloop detection.
Apply:
uint32_t rebootinterval = rtctime - bl_last_boottime; if (rebootinterval < BOOTLOOP_INTERVAL_MILLIS) { bl_crashcounter++; if (bl_crashcounter >= BOOTLOOP_THRESHOLD) { DEBUG_PRINTLN(F("!BOOTLOOP DETECTED!")); bl_crashcounter = 0; result = true; } - } + } else { + // Only consecutive short-interval crashes should count + bl_crashcounter = 0; + }
🧹 Nitpick comments (4)
wled00/util.cpp (4)
723-729
: Prefer constexpr over macros for type safety and scopeThese constants can be
constexpr
orstatic constexpr
to get type checking and scoped names.Apply:
-#define BOOTLOOP_INTERVAL_MILLIS 5000 // time limit between crashes: 5 seconds -#define BOOTLOOP_THRESHOLD 5 // number of consecutive crashes to trigger bootloop detection -#define BOOTLOOP_ACTION_RESTORE 0 // default action: restore config from /bkp.cfg.json -#define BOOTLOOP_ACTION_RESET 1 // if restore does not work, reset config (rename /cfg.json to /rst.cfg.json) -#define BOOTLOOP_ACTION_OTA 2 // swap the boot partition -#define BOOTLOOP_ACTION_DUMP 3 // nothing seems to help, dump files to serial and reboot (until hardware reset) +static constexpr uint32_t BOOTLOOP_INTERVAL_MILLIS = 5000; // time limit between crashes: 5 seconds +static constexpr uint8_t BOOTLOOP_THRESHOLD = 5; // number of consecutive crashes to trigger bootloop detection +static constexpr uint8_t BOOTLOOP_ACTION_RESTORE = 0; // default action: restore config from /bkp.cfg.json +static constexpr uint8_t BOOTLOOP_ACTION_RESET = 1; // if restore does not work, reset config (rename /cfg.json to /rst.cfg.json) +static constexpr uint8_t BOOTLOOP_ACTION_OTA = 2; // swap the boot partition +static constexpr uint8_t BOOTLOOP_ACTION_DUMP = 3; // nothing seems to help, dump files to serial and reboot (until hardware reset)
757-757
: Clarify RTC tick scalingThe 160 divider assumes ~160 kHz RTC ticks. Consider a brief comment with source or a named constant for maintainability.
-static inline uint32_t getRtcMillis() { return system_get_rtc_time() / 160; }; // rtc ticks ~160000Hz +// ESP8266 RTC runs at ~160 kHz; convert ticks to ms +static inline uint32_t getRtcMillis() { return system_get_rtc_time() / 160; }
811-816
: Brownout branch logs only; no state changeIntentional, but consider documenting why restoration/reset are skipped for brownouts to avoid future “helpful” changes.
824-851
: ESP8266 OTA action falls through to DUMP and does not advanceOn ESP8266,
BOOTLOOP_ACTION_OTA
falls through to DUMP (by design) and does not incrementbl_actiontracker
. This causes indefinite DUMP on subsequent detections. Verify this is the desired terminal state.If you want 8266 to advance to DUMP (3) for clearer telemetry, consider:
-#else - // fall through -#endif +#else + bl_actiontracker = BOOTLOOP_ACTION_DUMP; +#endif
📜 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 (1)
wled00/util.cpp
(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
wled00/util.cpp (2)
wled00/cfg.cpp (4)
restoreConfig
(781-783)restoreConfig
(781-781)resetConfig
(791-799)resetConfig
(791-791)wled00/file.cpp (2)
dumpFilesToSerial
(576-593)dumpFilesToSerial
(576-576)
⏰ 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). (13)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
🔇 Additional comments (7)
wled00/util.cpp (7)
730-737
: Enum for reset reasons looks goodClean, platform-agnostic categorization.
739-745
: RTC user memory offsets: confirm non-overlap across core versionsOffsets 32–34 (128–140 bytes) avoid the region hinted to be used by OTA. Please confirm this holds for all supported ESP8266 core versions to prevent subtle state corruption across OTA operations.
Would you like a script to scan your CI matrix (boards + cores) and grep the core sources to assert these offsets remain outside OTA’s usage?
765-771
: ESP32 reset reason mapping is sensibleCovers SW, brownout, and various WDTs/panics -> Crash. LGTM.
773-778
: RTC time source handling across IDF versions is correctAppropriate APIs and units conversion to ms.
789-796
: Reset on Power/Software is correctInitializing action on Power and clearing the crash counter on Software resets is the right behavior.
818-821
: Storing rtctime unconditionally is correctEnsures next-boot interval is computed regardless of reason.
779-781
: No unconditional ESP8266 calls to bootloopCheckOTAI’ve verified that both the declaration and definition of bootloopCheckOTA() are wrapped in #ifndef ESP8266 guards, and the sole call site in wled_server.cpp is likewise inside an #ifndef ESP8266 block. There are no unguarded calls on ESP8266 builds.
code looks good, do you want me to run any tests? |
Any testing is always welcome! I did validate that the variables worked properly with my ESP8266 test board last week. Unfortunately I blew up its voltage regulator last night - now it's somewhat frankensteined together with a different unit where the CPU is blown, so the soft reset doesn't work and it can only OTA until I swap the chips, so I don't want to boot loop it for the moment. New test board should arrive today. (So nice they're so inexpensive!) |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (2)
wled00/util.cpp (2)
797-813
: Crash counter reset on long intervals addresses false positives.This implements the earlier suggestion to track only consecutive short-interval crashes. Nice.
746-756
: Null-deref risk: system_get_rst_info() can be nullptr on ESP8266. Add a defensive check.Also keeps behavior consistent with the prior review request.
Apply:
-static inline ResetReason rebootReason() { - uint32_t resetReason = system_get_rst_info()->reason; +static inline ResetReason rebootReason() { + const rst_info* info = system_get_rst_info(); + if (!info) return ResetReason::Power; + uint32_t resetReason = info->reason; if (resetReason == REASON_EXCEPTION_RST || resetReason == REASON_WDT_RST || resetReason == REASON_SOFT_WDT_RST) return ResetReason::Crash; if (resetReason == REASON_SOFT_RESTART) return ResetReason::Software; return ResetReason::Power; }
🧹 Nitpick comments (5)
wled00/util.cpp (5)
739-745
: RTC_USER_MEM offsets: add a one-time init guard/magic to avoid garbage after power-on.Even though Power falls through to reset counters, stale/garbage values before first write can still leak into logs or transient logic. Consider reserving one word for a magic/version and zeroing the block on mismatch.
Example:
static volatile uint32_t& bl_last_boottime = *(RTC_USER_MEM + 32); static volatile uint32_t& bl_crashcounter = *(RTC_USER_MEM + 33); static volatile uint32_t& bl_actiontracker = *(RTC_USER_MEM + 34); +static volatile uint32_t& bl_magic = *(RTC_USER_MEM + 35); +constexpr uint32_t BL_MAGIC = 0x574C4544; // 'WLED' + +static inline void bl_rtc_init_if_needed() { + if (bl_magic != BL_MAGIC) { + bl_magic = BL_MAGIC; + bl_last_boottime = 0; + bl_crashcounter = 0; + bl_actiontracker = BOOTLOOP_ACTION_RESTORE; + } +}Call once at the start of detectBootLoop().
765-771
: ESP32 reset mapping looks right.Panic and WDTs grouped as Crash; SW as Software; Brownout separated. Consider explicitly handling ESP_RST_POWERON/DEEPSLEEP as Power for readability (current default already returns Power).
815-820
: Brownout path: consider telemetry and debounce.If brownouts are frequent, surfacing a counter (non-persistent) in DEBUG or info JSON can help users diagnose power issues. Optional: ignore repeats within N seconds to avoid log spam.
828-829
: Debug print uses old boottime value; consider including current rtctime too.Helps correlate decisions.
-DEBUG_PRINTF_P(PSTR("checking for bootloop: time %d, counter %d, action %d\n"), bl_last_boottime, bl_crashcounter, bl_actiontracker); +DEBUG_PRINTF_P(PSTR("checking for bootloop: prev_time %u, now %u, counter %u, action %u\n"), + bl_last_boottime, getRtcMillis(), bl_crashcounter, bl_actiontracker);
632-709
: Minor typos in comments: “alloactions” → “allocations”.Purely cosmetic; fix if you touch the file again.
📜 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 (1)
wled00/util.cpp
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/{*.cpp,!(html_*)*.h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ source and header files
Files:
wled00/util.cpp
🧬 Code graph analysis (1)
wled00/util.cpp (2)
wled00/cfg.cpp (4)
restoreConfig
(781-783)restoreConfig
(781-781)resetConfig
(791-799)resetConfig
(791-791)wled00/file.cpp (2)
dumpFilesToSerial
(576-593)dumpFilesToSerial
(576-576)
⏰ 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). (18)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp32_eth)
🔇 Additional comments (6)
wled00/util.cpp (6)
723-729
: Raising the interval to 2 minutes is sensible.Matches the earlier discussion about Wi‑Fi join latency; threshold of 5 keeps false positives low.
730-737
: Enum-based reason model improves clarity and portability.Nice factoring; keeps platform handling isolated.
757-757
: Verify 160 tick/ms divisor across SDKs/clock configs.On ESP8266, rtc tick rate can vary; please confirm 160 is correct for our target core versions or derive from calibration if available.
Would you like a small sketch to print getRtcMillis() vs millis() over 10s for a few boards?
773-777
: Time source selection is good.Using esp_rtc_get_time_us() (>=4.4) and slowclk conversion (>=3.3) makes this robust across IDF versions.
785-796
: detectBootLoop() structure is clean and side-effect order is correct.Grabs time, handles reason, updates boottime, returns result. Good.
Also applies to: 822-824
831-854
: No action needed — backup and restore filenames match. restoreFile and backupFile both use s_backup_fmt ("/bkp.%s") with s_cfg_json ("/cfg.json") to generate "/bkp.cfg.json" as documented.
void bootloopCheckOTA() { bl_actiontracker = BOOTLOOP_ACTION_OTA; } // swap boot image if bootloop is detected instead of restoring config | ||
|
||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Provide a no-op bootloopCheckOTA() on ESP8266 to avoid cross-platform call sites needing ifdefs.
Right now the symbol only exists on non-ESP8266. A stub keeps call sites clean.
Apply:
-void bootloopCheckOTA() { bl_actiontracker = BOOTLOOP_ACTION_OTA; } // swap boot image if bootloop is detected instead of restoring config
+#ifndef ESP8266
+void bootloopCheckOTA() { bl_actiontracker = BOOTLOOP_ACTION_OTA; } // swap boot image if bootloop is detected instead of restoring config
+#else
+void bootloopCheckOTA() { /* not supported on ESP8266 */ }
+#endif
Also applies to: 841-851
🤖 Prompt for AI Agents
In wled00/util.cpp around lines 779-781 (and also apply the same change for
lines 841-851), add a no-op stub implementation of bootloopCheckOTA() for the
ESP8266 build so the symbol always exists across platforms; specifically, wrap a
simple empty function definition (doing nothing) in the appropriate #ifdef or
#if defined(ESP8266) guard so call sites do not need platform-specific ifdefs
and the non-ESP8266 behavior (setting bl_actiontracker = BOOTLOOP_ACTION_OTA)
remains unchanged for other platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd considered it; honestly doing it that way would generally be my preferred style. Unfortunately, given that the body and caller are in separate translation units, the empty call cannot be inlined away, so the result would be a small waste of limited code memory bytes on ESP8266 systems for nothing but code aesthetic reasons. Given that the extra #ifdefs are already in the code, the best value is to leave them alone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@willmmiles That's a very valid point about the memory constraints on ESP8266 systems. You're absolutely right that the function call overhead would be wasteful when every byte of code memory counts, especially since the compiler can't inline away calls across translation units. The practical consideration of limited flash memory definitely outweighs the aesthetic benefits of a unified API in this case. Thanks for the explanation!
✏️ Learnings added
Learnt from: willmmiles
PR: wled/WLED#4853
File: wled00/util.cpp:779-781
Timestamp: 2025-08-29T01:34:34.314Z
Learning: On ESP8266 systems, avoid adding no-op stub functions across translation units due to limited code memory constraints, as the compiler cannot inline away the function calls, resulting in wasteful memory usage.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
This one's for you, @coderabbitai: |
Tip For best results, initiate chat on the files or code changes. @willmmiles Thank you for the clarification! You're absolutely right - I appreciate you explaining how ✏️ Learnings added
|
Tested on ESP8266 and C3, works as intended. I took the liberty to remove whitespaces and add a debug output if a crash is detected. |
damn, forgot to push the commit beofre merge :) ok, will add it to main directly |
Summary by CodeRabbit
New Features
Refactor