use RTC_DATA_ATTR; address review comments

Author: dhalbert

ports/esp32s2/common-hal/alarm/SleepMemory.c

@@ -30,33 +30,37 @@
 #include "py/runtime.h"
 #include "common-hal/alarm/SleepMemory.h"
 
+#include "esp_log.h"
 #include "esp_sleep.h"
 
+// Data storage for singleton instance of SleepMemory.
+// Might be RTC_SLOW_MEM or RTC_FAST_MEM, depending on setting of CONFIG_ESP32S2_RTCDATA_IN_FAST_MEM.
+static RTC_DATA_ATTR uint8_t _sleep_mem[SLEEP_MEMORY_LENGTH];
+
 void alarm_sleep_memory_reset(void) {
-    // Power RTC slow memory during deep sleep
-    esp_sleep_pd_config(ESP_PD_DOMAIN_RTC_SLOW_MEM, ESP_PD_OPTION_ON);
+    // ESP-IDF build system takes care of doing esp_sleep_pd_config() or the equivalentwith
+    // the correct settings, depending on which RTC mem we are using.
+    // https://docs.espressif.com/projects/esp-idf/en/latest/esp32s2/api-reference/system/sleep_modes.html#power-down-of-rtc-peripherals-and-memories
 }
 
 uint32_t common_hal_alarm_sleep_memory_get_length(alarm_sleep_memory_obj_t *self) {
-    return SLEEP_MEMORY_LENGTH;
+    return sizeof(_sleep_mem);
 }
 
-bool common_hal_alarm_sleep_memory_set_bytes(alarm_sleep_memory_obj_t *self,
-        uint32_t start_index, uint8_t* values, uint32_t len) {
+bool common_hal_alarm_sleep_memory_set_bytes(alarm_sleep_memory_obj_t *self, uint32_t start_index, const uint8_t* values, uint32_t len) {
 
-    if (start_index + len > SLEEP_MEMORY_LENGTH) {
+    if (start_index + len > sizeof(_sleep_mem)) {
         return false;
     }
 
-    memcpy((uint8_t *) (SLEEP_MEMORY_BASE + start_index), values, len);
+    memcpy((uint8_t *) (_sleep_mem + start_index), values, len);
     return true;
 }
 
-void common_hal_alarm_sleep_memory_get_bytes(alarm_sleep_memory_obj_t *self,
-        uint32_t start_index, uint32_t len, uint8_t* values) {
+void common_hal_alarm_sleep_memory_get_bytes(alarm_sleep_memory_obj_t *self, uint32_t start_index, uint8_t* values, uint32_t len) {
 
-    if (start_index + len > SLEEP_MEMORY_LENGTH) {
+    if (start_index + len > sizeof(_sleep_mem)) {
         return;
     }
-    memcpy(values, (uint8_t *) (SLEEP_MEMORY_BASE + start_index), len);
+    memcpy(values, (uint8_t *) (_sleep_mem + start_index), len);
 }

ports/esp32s2/common-hal/alarm/SleepMemory.h

@@ -34,23 +34,15 @@
 // RTC registers: There are a few 32-bit registers maintained during deep sleep.
 // We are already using one for saving sleep information during deep sleep.
 //
-// RTC Fast Memory: 8kB, also used for deep-sleep power on stub, and for heap
-// during normal operation if CONFIG_ESP32S2_ALLOW_RTC_FAST_MEM_AS_HEAP is set.
-// Power-on during deep sleep must be enabled.
-// I experimented with using RTC Fast Memory. It seemed to work, but occasionally,
-// got smashed for unknown reasons.
-// Base of RTC Fast memory on the data bus is 0x3FF9E000. The address is different on the instruction bus.
-//
+// RTC Fast Memory: 8kB, also used for deep-sleep power-on stub.
 // RTC Slow Memory: 8kB, also used for the ULP (tiny co-processor available during sleep).
-// Less likely to be used by ESP-IDF.
-// Since we may want to use the ULP in the future, we will use the upper half
-// of Slow Memory and reserve the lower half for ULP.
-// From ulp.h:
-// #define RTC_SLOW_MEM ((uint32_t*) 0x50000000)       /*!< RTC slow memory, 8k size */
+//
+// The ESP-IDF build system takes care of the power management of these regions.
+// RTC_DATA_ATTR will allocate storage in RTC_SLOW_MEM unless CONFIG_ESP32S2_RTCDATA_IN_FAST_MEM
+// is set. Any memory not allocated by us can be used by the ESP-IDF for heap or other purposes.
 
-// Upper half of RTC_SLOW_MEM.
+// Use half of RTC_SLOW_MEM or RTC_FAST_MEM.
 #define SLEEP_MEMORY_LENGTH (4096)
-#define SLEEP_MEMORY_BASE (0x50000000 + 4096)
 
 typedef struct {
     mp_obj_base_t base;

shared-bindings/alarm/SleepMemory.c

@@ -54,26 +54,6 @@
 //|         ...
 //|
 
-//|     def __bool__(self) -> bool:
-//|         """``sleep_memory`` is ``True`` if its length is greater than zero.
-//|         This is an easy way to check for its existence.
-//|         """
-//|         ...
-//|
-//|     def __len__(self) -> int:
-//|         """Return the length. This is used by (`len`)"""
-//|         ...
-//|
-STATIC mp_obj_t alarm_sleep_memory_unary_op(mp_unary_op_t op, mp_obj_t self_in) {
-    alarm_sleep_memory_obj_t *self = MP_OBJ_TO_PTR(self_in);
-    uint16_t len = common_hal_alarm_sleep_memory_get_length(self);
-    switch (op) {
-        case MP_UNARY_OP_BOOL: return mp_obj_new_bool(len != 0);
-        case MP_UNARY_OP_LEN: return MP_OBJ_NEW_SMALL_INT(len);
-        default: return MP_OBJ_NULL; // op not supported
-    }
-}
-
 STATIC const mp_rom_map_elem_t alarm_sleep_memory_locals_dict_table[] = {
 };
 
@@ -131,7 +111,7 @@ STATIC mp_obj_t alarm_sleep_memory_subscr(mp_obj_t self_in, mp_obj_t index_in, m
                 }
 
                 if (!common_hal_alarm_sleep_memory_set_bytes(self, slice.start, src_items, src_len)) {
-                    mp_raise_RuntimeError(translate("Unable to write to nvm."));
+                    mp_raise_RuntimeError(translate("Unable to write to sleep_memory."));
                 }
                 return mp_const_none;
                 #else
@@ -141,7 +121,7 @@ STATIC mp_obj_t alarm_sleep_memory_subscr(mp_obj_t self_in, mp_obj_t index_in, m
                 // Read slice.
                 size_t len = slice.stop - slice.start;
                 uint8_t *items = m_new(uint8_t, len);
-                common_hal_alarm_sleep_memory_get_bytes(self, slice.start, len, items);
+                common_hal_alarm_sleep_memory_get_bytes(self, slice.start, items, len);
                 return mp_obj_new_bytearray_by_ref(len, items);
             }
 #endif
@@ -152,7 +132,7 @@ STATIC mp_obj_t alarm_sleep_memory_subscr(mp_obj_t self_in, mp_obj_t index_in, m
             if (value == MP_OBJ_SENTINEL) {
                 // load
                 uint8_t value_out;
-                common_hal_alarm_sleep_memory_get_bytes(self, index, 1, &value_out);
+                common_hal_alarm_sleep_memory_get_bytes(self, index, &value_out, 1);
                 return MP_OBJ_NEW_SMALL_INT(value_out);
             } else {
                 // store
@@ -162,7 +142,7 @@ STATIC mp_obj_t alarm_sleep_memory_subscr(mp_obj_t self_in, mp_obj_t index_in, m
                 }
                 uint8_t short_value = byte_value;
                 if (!common_hal_alarm_sleep_memory_set_bytes(self, index, &short_value, 1)) {
-                    mp_raise_RuntimeError(translate("Unable to write to nvm."));
+                    mp_raise_RuntimeError(translate("Unable to write to sleep_memory."));
                 }
                 return mp_const_none;
             }
@@ -174,7 +154,6 @@ const mp_obj_type_t alarm_sleep_memory_type = {
     { &mp_type_type },
     .name = MP_QSTR_SleepMemory,
     .subscr = alarm_sleep_memory_subscr,
-    .unary_op = alarm_sleep_memory_unary_op,
     .print = NULL,
     .locals_dict = (mp_obj_t)&alarm_sleep_memory_locals_dict,
 };

shared-bindings/alarm/SleepMemory.h

@@ -34,11 +34,7 @@ extern const mp_obj_type_t alarm_sleep_memory_type;
 
 uint32_t common_hal_alarm_sleep_memory_get_length(alarm_sleep_memory_obj_t *self);
 
-bool common_hal_alarm_sleep_memory_set_bytes(alarm_sleep_memory_obj_t *self,
-    uint32_t start_index, uint8_t* values, uint32_t len);
-// len and values are intentionally swapped to signify values is an output and
-// also leverage the compiler to validate uses are expected.
-void common_hal_alarm_sleep_memory_get_bytes(alarm_sleep_memory_obj_t *self,
-    uint32_t start_index, uint32_t len, uint8_t* values);
+bool common_hal_alarm_sleep_memory_set_bytes(alarm_sleep_memory_obj_t *self, uint32_t start_index, const uint8_t* values, uint32_t len);
+void common_hal_alarm_sleep_memory_get_bytes(alarm_sleep_memory_obj_t *self, uint32_t start_index, uint8_t* values, uint32_t len);
 
 #endif  // MICROPY_INCLUDED_SHARED_BINDINGS_ALARM_SLEEPMEMORY_H