esp-rs / esp-hal

no_std Hardware Abstraction Layers for ESP32 microcontrollers
https://docs.esp-rs.org/esp-hal/
Apache License 2.0
764 stars 214 forks source link

Unexpected behaviour of `ram(rtc_fast, persistent)` #2516

Open jukrut opened 1 week ago

jukrut commented 1 week ago

Bug description

I cloned the esp-hal repo and ran the ram.rs example with a little adjustment:

-    loop {
-        function_in_ram();
-        delay.delay(1.secs());
-    }
+    let timer = TimerWakeupSource::new(Duration::from_secs(1));
+    println!("sleeping!");
+    delay.delay_millis(100);
+    rtc.sleep_deep(&[&timer]);

and either I misunderstand ram(rtc_fast, persistent) or something somewhere doesn't work like it should :)

running it and grep for SOME_PERS...

SOME_PERSISTENT_DATA [0, 0]
SOME_PERSISTENT_DATA [0, 0]
SOME_PERSISTENT_DATA [ca, 27]
SOME_PERSISTENT_DATA [ca, 27]
SOME_PERSISTENT_DATA [ca, 27]
SOME_PERSISTENT_DATA [ca, 27]
SOME_PERSISTENT_DATA [8a, 27]
SOME_PERSISTENT_DATA [8a, 27]
SOME_PERSISTENT_DATA [8a, 23]
SOME_PERSISTENT_DATA [8a, 23]
SOME_PERSISTENT_DATA [8a, 27]
SOME_PERSISTENT_DATA [8a, 27]
SOME_PERSISTENT_DATA [8a, 27]
SOME_PERSISTENT_DATA [8a, 27]
SOME_PERSISTENT_DATA [8a, 25]
SOME_PERSISTENT_DATA [8a, 25]
SOME_PERSISTENT_DATA [8a, 27]
SOME_PERSISTENT_DATA [8a, 27]
SOME_PERSISTENT_DATA [8a, 27]
SOME_PERSISTENT_DATA [8a, 27]
SOME_PERSISTENT_DATA [ca, 23]
SOME_PERSISTENT_DATA [ca, 23]
SOME_PERSISTENT_DATA [8a, a7]

with this code (esp-idf-template):

//! This example demonstrates a how to ask for the reset reason and the wakeup reason.
use std::{thread, time};

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

#[link_section = ".rtc.data"]
static mut X: [i32; 10] = [0; 10];

fn main() -> anyhow::Result<()> {
    esp_idf_sys::link_patches();

    let wakeup_reason = WakeupReason::get();
    println!("Wakeup reason: {:?}", wakeup_reason);

    let reset_reason = ResetReason::get();
    println!("Reset reason: {:?}", reset_reason);

    thread::sleep(time::Duration::from_millis(1000));
    unsafe {
        println!("{}", X[0]);

        X[0] += 1;
        println!("{}", X[0]);
    }

    let sleep_micros = 2_000_000;
    unsafe {
        esp_idf_sys::esp_sleep_enable_timer_wakeup(sleep_micros);

        println!("Going to deep sleep {} seconds", sleep_micros / 1_000_000);
        esp_idf_sys::esp_deep_sleep_start();
        // Software reset!
    }
}

it works as expected.

To Reproduce

  1. fresh install (optional)
  2. run example ram.rs (with diff from above)

Expected behavior

data is persisted and updated (incremented in this special case)

Environment

bjoernQ commented 1 week ago

This works fine on at least S3 and C6

Seems like on ESP32 we power down RTC ram while we don't do it for S3/C6 by default for deep-sleep

However, it's already user configurable. This should make it work for ESP32:

    let timer = TimerWakeupSource::new(core::time::Duration::from_secs(1));
    println!("sleeping!");
    delay.delay_millis(100);
    let mut cfg = RtcSleepConfig::deep();
    cfg.set_rtc_fastmem_pd_en(false);
    rtc.sleep(&cfg, &[&timer]);

We probably want to use similar defaults for different chips. (So not closing this now)

jukrut commented 1 week ago

Thx I can confirm it works now as expected.

Sounds very good to have that the default in the future.

bugadani commented 1 week ago

My €.02:

I don't think we can win here. On one hand, we can optimize for lowest power by default, then we'll have questions like this. We can improve documentation I guess to make the situation better. On the other hand, if we optimize for defaults that don't break users' expectations, we'll end up with questions like "why does deep-sleep consume too much power?". Again, documentation could help with that, too.

But what even are defaults the users want? What features do we want to keep enabled to satisfy everybody? I think esp-hal should aim for lowest power by default, then allow users to turn back on what they need.

MabezDev commented 1 week ago

I think esp-hal should aim for lowest power by default, then allow users to turn back on what they need.

In general what we do specifically is less important to me, but being consistent is. With that said I agree with this, we should default to lowest power.

bjoernQ commented 1 week ago

And probably we should improve documentation (i.e. state what deep and light-sleep defaults are)

jukrut commented 3 days ago

I totally agree. It can't be perfect for all situations. It's probably a question of target audience..

The only problem I had was : that the example code didn't work.

If there would be a deep sleep example without persistence and lower power defaults or low power optimization and a example with persistence that had the other settings.

of course documentation would also be nice... but for me a example says more then 1000 words.

In my xp as a developer with other tooling, libs or so: I am happy if the defaults work and I had to further optimize for e.g. performance or low power..