Bootloop detection platform factoring #4853
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -720,125 +720,135 @@ void *realloc_malloc(void *ptr, size_t size) { | |
| // checks if the ESP reboots multiple times due to a crash or watchdog timeout | ||
| // if a bootloop is detected: restore settings from backup, then reset settings, then switch boot image (and repeat) | ||
|
|
||
| #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) | ||
|
|
||
| // Platform-agnostic abstraction | ||
| enum class ResetReason { | ||
| Power, | ||
| Software, | ||
| Crash, | ||
| Brownout | ||
| }; | ||
|
|
||
| #ifdef ESP8266 | ||
| // Place variables in RTC memory via references, since RTC memory is not exposed via the linker in the Non-OS SDK | ||
| // Use an offset of 32 as there's some hints that the first 128 bytes of "user" memory are used by the OTA system | ||
| // Ref: https://github.com/esp8266/Arduino/blob/78d0d0aceacc1553f45ad8154592b0af22d1eede/cores/esp8266/Esp.cpp#L168 | ||
| 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() { | ||
| uint32_t resetReason = system_get_rst_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; | ||
| } | ||
|
|
||
| static inline uint32_t getRtcMillis() { return system_get_rtc_time() / 160; }; // rtc ticks ~160000Hz | ||
|
|
||
| #else | ||
| // variables in RTC_NOINIT memory persist between reboots (but not on hardware reset) | ||
| RTC_NOINIT_ATTR static uint32_t bl_last_boottime; | ||
| RTC_NOINIT_ATTR static uint32_t bl_crashcounter; | ||
| RTC_NOINIT_ATTR static uint32_t bl_actiontracker; | ||
|
|
||
| static inline ResetReason rebootReason() { | ||
| esp_reset_reason_t reason = esp_reset_reason(); | ||
| if (reason == ESP_RST_BROWNOUT) return ResetReason::Brownout; | ||
| if (reason == ESP_RST_SW) return ResetReason::Software; | ||
| if (reason == ESP_RST_PANIC || reason == ESP_RST_WDT || reason == ESP_RST_INT_WDT || reason == ESP_RST_TASK_WDT) return ResetReason::Crash; | ||
| return ResetReason::Power; | ||
| } | ||
|
|
||
| #if ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(4, 4, 0) | ||
| static inline uint32_t getRtcMillis() { return esp_rtc_get_time_us() / 1000; } | ||
| #elif ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(3, 3, 0) | ||
| static inline uint32_t getRtcMillis() { return rtc_time_slowclk_to_us(rtc_time_get(), rtc_clk_slow_freq_get_hz()) / 1000; } | ||
| #endif | ||
|
|
||
| void bootloopCheckOTA() { bl_actiontracker = BOOTLOOP_ACTION_OTA; } // swap boot image if bootloop is detected instead of restoring config | ||
|
|
||
| #endif | ||
|
Comment on lines
779
to
781
Contributor
There was a problem hiding this comment. 🛠️ 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 */ }
+#endifAlso applies to: 841-851 🤖 Prompt for AI Agents
Member
Author
There was a problem hiding this comment. 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.
Contributor
There was a problem hiding this comment. @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
|
||
|
|
||
| // detect bootloop by checking the reset reason and the time since last boot | ||
| static bool detectBootLoop() { | ||
| uint32_t rtctime = getRtcMillis(); | ||
| bool result = false; | ||
|
|
||
| switch(rebootReason()) { | ||
| case ResetReason::Power: | ||
| bl_actiontracker = BOOTLOOP_ACTION_RESTORE; // init action tracker if not an intentional reboot (e.g. from OTA or bootloop handler) | ||
| // fall through | ||
| case ResetReason::Software: | ||
| // no crash detected, reset counter | ||
| bl_crashcounter = 0; | ||
| break; | ||
|
|
||
| case ResetReason::Crash: | ||
| { | ||
| 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; | ||
| } | ||
| } | ||
| break; | ||
| } | ||
|
|
||
| case ResetReason::Brownout: | ||
| // crash due to brownout can't be detected unless using flash memory to store bootloop variables | ||
| DEBUG_PRINTLN(F("brownout detected")); | ||
| //restoreConfig(); // TODO: blindly restoring config if brownout detected is a bad idea, need a better way (if at all) | ||
| break; | ||
| } | ||
|
|
||
| bl_last_boottime = rtctime; // store current runtime for next reboot | ||
|
|
||
| return result; | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| void handleBootLoop() { | ||
| DEBUG_PRINTF_P(PSTR("checking for bootloop: time %d, counter %d, action %d\n"), bl_last_boottime, bl_crashcounter, bl_actiontracker); | ||
| if (!detectBootLoop()) return; // no bootloop detected | ||
|
|
||
| switch(bl_actiontracker) { | ||
| case BOOTLOOP_ACTION_RESTORE: | ||
| restoreConfig(); | ||
| ++bl_actiontracker; | ||
| break; | ||
| case BOOTLOOP_ACTION_RESET: | ||
| resetConfig(); | ||
| ++bl_actiontracker; | ||
| break; | ||
| case BOOTLOOP_ACTION_OTA: | ||
| #ifndef ESP8266 | ||
| if(Update.canRollBack()) { | ||
| DEBUG_PRINTLN(F("Swapping boot partition...")); | ||
| Update.rollBack(); // swap boot partition | ||
| } | ||
| ++bl_actiontracker; | ||
| break; | ||
| #else | ||
| // fall through | ||
| #endif | ||
| case BOOTLOOP_ACTION_DUMP: | ||
| dumpFilesToSerial(); | ||
| break; | ||
| } | ||
|
|
||
| ESP.restart(); // restart cleanly and don't wait for another crash | ||
| } | ||
|
|
||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
🤖 Prompt for AI Agents