esp-rs / esp-idf-hal

embedded-hal implementation for Rust on ESP32 and ESP-IDF
https://docs.esp-rs.org/esp-idf-hal/
Apache License 2.0
430 stars 170 forks source link

ResetReason not dealing with all reset alternatives panics when reason is esp_reset_reason_t_ESP_RST_USB #443

Closed punkto closed 1 month ago

punkto commented 1 month ago

On reset reason translation from IDF code to ResetReason struct at https://github.com/esp-rs/esp-idf-hal/blob/master/src/reset.rs#L34 the code panics if the reset reason is not in a subset of all reasons. In the bindings.rs generated from esp-idf-sys version "0.34.1" the reasons are:

#[doc = "!< Reset reason can not be determined"]
pub const esp_reset_reason_t_ESP_RST_UNKNOWN: esp_reset_reason_t = 0;
#[doc = "!< Reset due to power-on event"]
pub const esp_reset_reason_t_ESP_RST_POWERON: esp_reset_reason_t = 1;
#[doc = "!< Reset by external pin (not applicable for ESP32)"]
pub const esp_reset_reason_t_ESP_RST_EXT: esp_reset_reason_t = 2;
#[doc = "!< Software reset via esp_restart"]
pub const esp_reset_reason_t_ESP_RST_SW: esp_reset_reason_t = 3;
#[doc = "!< Software reset due to exception/panic"]
pub const esp_reset_reason_t_ESP_RST_PANIC: esp_reset_reason_t = 4;
#[doc = "!< Reset (software or hardware) due to interrupt watchdog"]
pub const esp_reset_reason_t_ESP_RST_INT_WDT: esp_reset_reason_t = 5;
#[doc = "!< Reset due to task watchdog"]
pub const esp_reset_reason_t_ESP_RST_TASK_WDT: esp_reset_reason_t = 6;
#[doc = "!< Reset due to other watchdogs"]
pub const esp_reset_reason_t_ESP_RST_WDT: esp_reset_reason_t = 7;
#[doc = "!< Reset after exiting deep sleep mode"]
pub const esp_reset_reason_t_ESP_RST_DEEPSLEEP: esp_reset_reason_t = 8;
#[doc = "!< Brownout reset (software or hardware)"]
pub const esp_reset_reason_t_ESP_RST_BROWNOUT: esp_reset_reason_t = 9;
#[doc = "!< Reset over SDIO"]
pub const esp_reset_reason_t_ESP_RST_SDIO: esp_reset_reason_t = 10;
#[doc = "!< Reset by USB peripheral"]
pub const esp_reset_reason_t_ESP_RST_USB: esp_reset_reason_t = 11;
#[doc = "!< Reset by JTAG"]
pub const esp_reset_reason_t_ESP_RST_JTAG: esp_reset_reason_t = 12;

Reasons with code greater than 10 are not included in the translation.

I have a board with an ESP32c3 module that on first boot after flashing it panics. The code is this (https://gitlab.com/scrobotics/optical-makerspace/dark-sky-meter-fw/-/blob/104-app-panics-just-on-first-boot/examples/panics_on_reset_reason.rs?ref_type=heads):

// This example demonstrates a panic that occurs when trying to get the reset reason.
use std::{thread, time};

use esp_idf_sys::{self as _, esp_reset_reason}; // If using the `binstart` feature of `esp-idf-sys`, always keep this module imported

fn main() {
    esp_idf_sys::link_patches();

    let reset_reason_code = unsafe { esp_reset_reason() };
    println!("Reset reason code: {:?}", reset_reason_code);

    let reset_reason = esp_idf_hal::reset::ResetReason::get(); // this panics on first run
    println!("Reset reason: {:?}", reset_reason);

    loop {
        thread::sleep(time::Duration::from_millis(1000)); // So the background task does not starve
    }
}

the output after flashing is:

I (24) boot: ESP-IDF v5.1.2-342-gbcf1645e44 2nd stage bootloader
I (24) boot: compile time Dec 12 2023 10:50:58
I (25) boot: chip revision: v0.4
I (29) boot.esp32c3: SPI Speed      : 40MHz
I (34) boot.esp32c3: SPI Mode       : DIO
I (38) boot.esp32c3: SPI Flash Size : 4MB
[...]

I (436) main_task: Started on CPU0
I (436) main_task: Calling app_main()
Reset reason code: 11
thread 'main' panicked at /home/jorgem/.cargo/registry/src/index.crates.io-6f17d22bba15001f/esp-idf-hal-0.43.1/src/reset.rs:47:18:
internal error: entered unreachable code
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

abort() was called at PC 0x420234a3 on core 0
0x420234a3 - panic_abort::__rust_start_panic::abort
    at /home/jorgem/.rustup/toolchains/nightly-2024-02-01-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/panic_abort/src/lib.rs:48
[...]

I (24) boot: ESP-IDF v5.1.2-342-gbcf1645e44 2nd stage bootloader
I (24) boot: compile time Dec 12 2023 10:50:58
I (24) boot: chip revision: v0.4
I (28) boot.esp32c3: SPI Speed      : 40MHz
I (33) boot.esp32c3: SPI Mode       : DIO
I (38) boot.esp32c3: SPI Flash Size : 4MB
[...]

I (436) main_task: Started on CPU0
I (436) main_task: Calling app_main()
Reset reason code: 4
Reset reason: Panic

I have also tested with an ESP32-C3-DevKit and the reset reason is different (maybe that is the reason because it has not been issued earlier):

I (32) boot: ESP-IDF v5.1.2-342-gbcf1645e44 2nd stage bootloader
I (32) boot: compile time Dec 12 2023 10:50:58
I (32) boot: chip revision: v0.3
[...]

I (444) main_task: Calling app_main()
Reset reason code: 1
Reset reason: PowerOn
Vollbrecht commented 1 month ago

Thanks for reporting!

Yeah that seams to not include all possibility's.

Are you interested in a PR to include the missing variants?

We only have to check that we don't include them for targets where a reset reason does not exist. If the underlying C type is the same for every target than its not a problem.

punkto commented 1 month ago

Hi @Vollbrecht , thanks for your support.

Before a PR, please let me se if I'm understanding what is going on. It seems that the IDF has a common esp_reset_reason_t for all the targets at https://github.com/espressif/esp-idf/blob/master/components/esp_system/include/esp_system.h#L24 :

typedef enum {
    ESP_RST_UNKNOWN,    //!< Reset reason can not be determined
    ESP_RST_POWERON,    //!< Reset due to power-on event
    ESP_RST_EXT,        //!< Reset by external pin (not applicable for ESP32)
    ESP_RST_SW,         //!< Software reset via esp_restart
    ESP_RST_PANIC,      //!< Software reset due to exception/panic
    ESP_RST_INT_WDT,    //!< Reset (software or hardware) due to interrupt watchdog
    ESP_RST_TASK_WDT,   //!< Reset due to task watchdog
    ESP_RST_WDT,        //!< Reset due to other watchdogs
    ESP_RST_DEEPSLEEP,  //!< Reset after exiting deep sleep mode
    ESP_RST_BROWNOUT,   //!< Brownout reset (software or hardware)
    ESP_RST_SDIO,       //!< Reset over SDIO
    ESP_RST_USB,        //!< Reset by USB peripheral
    ESP_RST_JTAG,       //!< Reset by JTAG
    ESP_RST_EFUSE,      //!< Reset due to efuse error
    ESP_RST_PWR_GLITCH, //!< Reset due to power glitch detected
    ESP_RST_CPU_LOCKUP, //!< Reset due to CPU lock up
} esp_reset_reason_t;

and the per target reasons (for example https://github.com/espressif/esp-idf/blob/v5.2.1/components/soc/esp32c2/include/soc/reset_reasons.h ) are boiled down to these at their respective get_reset_reason() like in https://github.com/espressif/esp-idf/blob/master/components/esp_system/port/soc/esp32c61/reset_reason.c#L19

If this is the case, we just need to add the last reasons to ResetReason and update the from method. That will work with all the targets as the c part is doing the per target part. Isn't it?

PS: Why are not ESP_RST_EFUSE, ESP_RST_PWR_GLITCH and ESP_RST_CPU_LOCKUP in bindings.rs ?

Vollbrecht commented 1 month ago

If i see it correctly they added most of the additions in ESP-IDF version v5.1 and onwards. They are here but for example not in v5.0 here

So we would simply need a conditional for all new options for v5.1 and onwards.

Vollbrecht commented 1 month ago

The condition can look something like


    #[cfg(not(any(
        esp_idf_version_major = "4",
        all(esp_idf_version_major = "5", esp_idf_version_minor = "0"),
    )))] // ESP-IDF 5.1 and later

    ``
punkto commented 1 month ago

Ah, and in v5.2.2 they added ESP_RST_EFUSE, ESP_RST_PWR_GLITCH or ESP_RST_CPU_LOCKUP, that answers my PS question.

Let me prepare a PR...