From 508c24c5e3c6394a3bda02aa4711b3225bc5f57e Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 17 Sep 2026 08:17:11 -0500 Subject: [PATCH] [esp32][rp2] Print the previous boot crash report before the logger reads it (#19351) --- esphome/components/esp32/crash_handler.cpp | 14 +++++++++----- esphome/components/esp32/crash_handler.h | 7 +------ esphome/components/esp32/hal.cpp | 8 -------- esphome/components/rp2/crash_handler.cpp | 22 +++++++++++++++++----- esphome/components/rp2/crash_handler.h | 3 ++- esphome/core/application.h | 4 ++-- 6 files changed, 31 insertions(+), 27 deletions(-) diff --git a/esphome/components/esp32/crash_handler.cpp b/esphome/components/esp32/crash_handler.cpp index 6f65243aaa0..b72a2777c7a 100644 --- a/esphome/components/esp32/crash_handler.cpp +++ b/esphome/components/esp32/crash_handler.cpp @@ -173,7 +173,10 @@ static const char *const TAG = "esp32.crash"; // NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables) static uint32_t s_current_build_time = static_cast(ESPHOME_BUILD_TIME); -void crash_handler_read_and_clear() { +// Validate the NOINIT record. Runs on every has_data() call; re-running is +// harmless and the magic is left alone so the record survives an OTA +// rollback reboot, crash_handler_clear() drops it once an API client has it. +static void read_crash_data() { if (s_raw_crash_data.magic == CRASH_MAGIC && s_raw_crash_data.version == CRASH_DATA_VERSION) { s_crash_data_valid = true; // Clamp counts to prevent out-of-bounds reads from corrupt .noinit data @@ -194,11 +197,12 @@ void crash_handler_read_and_clear() { s_raw_crash_data.other_reg_frame_count = s_raw_crash_data.other_backtrace_count; #endif } - // Don't clear magic here — crash data must survive OTA rollback reboots. - // Magic is cleared by crash_handler_clear() after an API client receives the data. } -bool crash_handler_has_data() { return s_crash_data_valid; } +bool crash_handler_has_data() { + read_crash_data(); + return s_crash_data_valid; +} void crash_handler_clear() { // Only clear the magic so data doesn't survive the next reboot. @@ -426,7 +430,7 @@ static void log_foreign_addresses() { // crashes again during boot, and allowing the CLI's process_stacktrace to match // and decode each address individually. void crash_handler_log() { - if (!s_crash_data_valid) + if (!crash_handler_has_data()) return; ESP_LOGE(TAG, "*** CRASH DETECTED ON PREVIOUS BOOT ***"); diff --git a/esphome/components/esp32/crash_handler.h b/esphome/components/esp32/crash_handler.h index c5e7d145ece..314be80314c 100644 --- a/esphome/components/esp32/crash_handler.h +++ b/esphome/components/esp32/crash_handler.h @@ -4,11 +4,6 @@ namespace esphome::esp32 { -/// Read and validate crash data from NOINIT memory. -/// Does not clear the magic marker — call crash_handler_clear() after -/// the data has been delivered to an API client so it survives OTA rollback reboots. -void crash_handler_read_and_clear(); - /// Log crash data if a crash was detected on previous boot. void crash_handler_log(); @@ -16,7 +11,7 @@ void crash_handler_log(); /// Call after the data has been delivered to an API client. void crash_handler_clear(); -/// Returns true if crash data was found this boot. +/// Returns true if crash data was found this boot, reading it first if needed. bool crash_handler_has_data(); } // namespace esphome::esp32 diff --git a/esphome/components/esp32/hal.cpp b/esphome/components/esp32/hal.cpp index f6199d557f3..199cb89f516 100644 --- a/esphome/components/esp32/hal.cpp +++ b/esphome/components/esp32/hal.cpp @@ -1,9 +1,6 @@ #ifdef USE_ESP32 -// defines.h must come before crash_handler.h so USE_ESP32_CRASH_HANDLER is set -// before crash_handler.h's #ifdef-guarded namespace block is parsed. #include "esphome/core/defines.h" -#include "crash_handler.h" #include "esphome/core/hal.h" #include @@ -45,11 +42,6 @@ void arch_restart() { } void arch_init() { -#ifdef USE_ESP32_CRASH_HANDLER - // Read crash data from previous boot before anything else - esp32::crash_handler_read_and_clear(); -#endif - // Enable the task watchdog only on the loop task (from which we're currently running) esp_task_wdt_add(nullptr); diff --git a/esphome/components/rp2/crash_handler.cpp b/esphome/components/rp2/crash_handler.cpp index a0fea216371..9bcdc8bee4e 100644 --- a/esphome/components/rp2/crash_handler.cpp +++ b/esphome/components/rp2/crash_handler.cpp @@ -55,8 +55,7 @@ namespace esphome::rp2 { static const char *const TAG = "rp2.crash"; -// Placed in .noinit so BSS zero-init cannot race with crash_handler_read_and_clear(). -// The valid field is explicitly cleared in crash_handler_read_and_clear() instead. +// Filled from the watchdog scratch registers on the first read. static struct CrashData { bool valid; uint32_t pc; @@ -64,11 +63,24 @@ static struct CrashData { uint32_t sp; uint32_t backtrace[MAX_BACKTRACE]; uint8_t backtrace_count; -} s_crash_data __attribute__((section(".noinit"))); // NOLINT(cppcoreguidelines-avoid-non-const-global-variables) +} s_crash_data; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables) -bool crash_handler_has_data() { return s_crash_data.valid; } +// Logger::pre_setup() logs the record before App.pre_setup() reaches +// arch_init(), so the first caller reads it and later calls are no-ops. +// The read clears the scratch registers, so it must not run twice, and +// arch_init() keeps its call so the read precedes watchdog_enable(), which +// overwrites scratch[4]. +static bool s_crash_data_read = false; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables) + +bool crash_handler_has_data() { + crash_handler_read_and_clear(); + return s_crash_data.valid; +} void crash_handler_read_and_clear() { + if (s_crash_data_read) + return; + s_crash_data_read = true; s_crash_data.valid = false; uint32_t magic = watchdog_hw->scratch[0]; if ((magic & 0xFFFF0000) == CRASH_MAGIC_SENTINEL && (magic & 0xFFFF) == CRASH_DATA_VERSION) { @@ -97,7 +109,7 @@ void crash_handler_read_and_clear() { // the device crashes again during boot, and allowing the CLI's process_stacktrace // to match and decode each address individually. void crash_handler_log() { - if (!s_crash_data.valid) + if (!crash_handler_has_data()) return; ESP_LOGE(TAG, "*** CRASH DETECTED ON PREVIOUS BOOT ***"); diff --git a/esphome/components/rp2/crash_handler.h b/esphome/components/rp2/crash_handler.h index 8c43d9fd3b0..3aec80b63b2 100644 --- a/esphome/components/rp2/crash_handler.h +++ b/esphome/components/rp2/crash_handler.h @@ -9,12 +9,13 @@ namespace esphome::rp2 { /// Read crash data from watchdog scratch registers and clear them. +/// Only the first call reads; later calls are no-ops. void crash_handler_read_and_clear(); /// Log crash data if a crash was detected on previous boot. void crash_handler_log(); -/// Returns true if crash data was found this boot. +/// Returns true if crash data was found this boot, reading it first if needed. bool crash_handler_has_data(); } // namespace esphome::rp2 diff --git a/esphome/core/application.h b/esphome/core/application.h index f1cf6fcca02..8ed4c09096a 100644 --- a/esphome/core/application.h +++ b/esphome/core/application.h @@ -67,7 +67,7 @@ static constexpr uint32_t TEARDOWN_TIMEOUT_REBOOT_MS = 1000; // 1 second for qu class Application { public: #ifdef ESPHOME_NAME_ADD_MAC_SUFFIX - // Called before Logger::pre_setup() — must not log (global_logger is not yet set). + // Runs after Logger::pre_setup() (emitted at EARLY_INIT priority), so the app name is not set yet there. /// Pre-setup with MAC suffix: overwrites placeholder in mutable static buffers with actual MAC. void pre_setup(char *name, size_t name_len, char *friendly_name, size_t friendly_name_len) { arch_init(); @@ -87,7 +87,7 @@ class Application { this->friendly_name_ = StringRef(friendly_name, friendly_name_len); } #else - // Called before Logger::pre_setup() — must not log (global_logger is not yet set). + // Runs after Logger::pre_setup() (emitted at EARLY_INIT priority), so the app name is not set yet there. /// Pre-setup without MAC suffix: StringRef points directly at const string literals in flash. void pre_setup(const char *name, size_t name_len, const char *friendly_name, size_t friendly_name_len) { arch_init();