Memfault Firmware SDK 0.39.1 (Build 1436)

Memfault Inc · Memfault Inc · commit b315d889ab12 · 2023-02-07T23:47:26.000Z
diff --git a/CHANGES.md b/CHANGES.md
@@ -1,3 +1,20 @@
+### Changes between Memfault SDK 0.39.1 and SDK 0.38.0 - Feb 3, 2023
+
+#### :rocket: New Features
+
+- **Experimental**
+  - CMSIS-Pack support
+  - Out Of Memory reboot reason added
+
+#### :chart_with_upwards_trend: Improvements
+
+- ESP-IDF:
+  - The default implementation of `memfault_platform_coredump_get_regions` is
+    changed to collect the current active stack, .bss, .data, and .heap regions.
+    Additionally if you are using ESP-IDF >= 4.4.0, the SDK will prioritize
+    collecting FreeRTOS regions containing task TCB and stack data.
+  - Assert coredumps are now labeled with the Assert reason
+
 ### Changes between Memfault SDK 0.39.0 and SDK 0.38.0 - Feb 3, 2023
 
 #### :boom: Breaking Changes
diff --git a/VERSION b/VERSION
@@ -1,2 +1,2 @@
-BUILD ID: 1415
-GIT COMMIT: cb7a3096e
+BUILD ID: 1436
+GIT COMMIT: 38142ae37
diff --git a/components/include/memfault/core/reboot_reason_types.h b/components/include/memfault/core/reboot_reason_types.h
@@ -50,7 +50,7 @@ typedef enum MfltResetReason {
   kMfltRebootReason_WatchdogDeprecated = 0x8002,
 
   kMfltRebootReason_BrownOutReset = 0x8003,
-  kMfltRebootReason_Nmi = 0x8004, // Non-Maskable Interrupt
+  kMfltRebootReason_Nmi = 0x8004,  // Non-Maskable Interrupt
 
   // More details about nomenclature in https://mflt.io/root-cause-watchdogs
   kMfltRebootReason_HardwareWatchdog = 0x8005,
@@ -68,6 +68,9 @@ typedef enum MfltResetReason {
   // to a previous version was initiated
   kMfltRebootReason_FirmwareUpdateError = 0x8009,
 
+  // A software reset triggered due to a dynamic memory (heap) allocation failure.
+  kMfltRebootReason_OutOfMemory = 0x800A,
+
   // Resets from Arm Faults
   kMfltRebootReason_BusFault = 0x9100,
   kMfltRebootReason_MemFault = 0x9200,
diff --git a/components/include/memfault/panics/arch/xtensa/xtensa.h b/components/include/memfault/panics/arch/xtensa/xtensa.h
@@ -11,6 +11,7 @@
 #include <stdint.h>
 
 #include "memfault/core/compiler.h"
+#include "memfault/core/reboot_reason_types.h"
 
 #ifdef __cplusplus
 extern "C" {
@@ -43,6 +44,12 @@ MEMFAULT_PACKED_STRUCT MfltRegState {
   uint32_t excvaddr;
 };
 
+//! Called by platform assertion handlers to save info before triggering fault handling
+//!
+//! @param pc Pointer to address of assertion location
+//! @param lr Pointer to return address of assertion location
+//! @param reason Reboot reason for the assertion
+void memfault_xtensa_fault_handling_assert(void *pc, void *lr, eMemfaultRebootReason reason);
 
 #ifdef __cplusplus
 }
diff --git a/components/include/memfault/version.h b/components/include/memfault/version.h
@@ -19,7 +19,7 @@ typedef struct {
   uint8_t patch;
 } sMfltSdkVersion;
 
-#define MEMFAULT_SDK_VERSION   { .major = 0, .minor = 39, .patch = 0 }
+#define MEMFAULT_SDK_VERSION   { .major = 0, .minor = 39, .patch = 1 }
 
 #ifdef __cplusplus
 }
diff --git a/components/panics/src/memfault_fault_handling_xtensa.c b/components/panics/src/memfault_fault_handling_xtensa.c
@@ -22,6 +22,15 @@ const sMfltCoredumpRegion *memfault_coredump_get_arch_regions(size_t *num_region
 
 static eMemfaultRebootReason s_crash_reason = kMfltRebootReason_Unknown;
 
+void memfault_xtensa_fault_handling_assert(void *pc, void *lr, eMemfaultRebootReason reason) {
+  sMfltRebootTrackingRegInfo info = {
+    .pc = (uint32_t)pc,
+    .lr = (uint32_t)lr,
+  };
+  s_crash_reason = reason;
+  memfault_reboot_tracking_mark_reset_imminent(s_crash_reason, &info);
+}
+
 void memfault_fault_handler(const sMfltRegState *regs, eMemfaultRebootReason reason) {
   if (s_crash_reason == kMfltRebootReason_Unknown) {
     sMfltRebootTrackingRegInfo info = {
diff --git a/ports/esp_idf/memfault/CMakeLists.txt b/ports/esp_idf/memfault/CMakeLists.txt
@@ -52,6 +52,28 @@ list(APPEND MEMFAULT_COMPONENTS_INC_FOLDERS
    $ENV{MEMFAULT_PLATFORM_EXTRA_INCLUDES}
 )
 
+# For version >= 4.4.0, we can collect smaller coredumps by default
+# by prioritizing active stack and FreeRTOS regions first. ESP-IDF <= 4.4.0
+# uses a simpler scheme collecting all of DRAM. See
+# common/memfault_platform_coredump.c for more info.
+# Note: CMake does not short-circuit logic statements, nested ifs required
+# Note: ENV{IDF_VERSION} added in esp-idf 4.4.0
+if (DEFINED ENV{IDF_VERSION})
+  if ($ENV{IDF_VERSION} VERSION_GREATER_EQUAL "4.4.0")
+    list(APPEND MEMFAULT_COMPONENTS_SRCS
+      ${MEMFAULT_SDK_ROOT}/ports/freertos/src/memfault_freertos_ram_regions.c
+    )
+
+    list(APPEND MEMFAULT_COMPONENTS_INC_FOLDERS
+      ${MEMFAULT_SDK_ROOT}/ports/include/
+    )
+
+    # Add a linker fragment to place FreeRTOS timers and task objects in the same area of dram0.bss
+    set(COMPONENT_ADD_LDFRAGMENTS "${MEMFAULT_ESP_IDF_PORT_COMMON}/memfault_esp_freertos.lf")
+  endif()
+endif()
+
+
 # Register Memfault SDK Component
 set(COMPONENT_SRCS ${MEMFAULT_COMPONENTS_SRCS})
 set(COMPONENT_ADD_INCLUDEDIRS ${MEMFAULT_COMPONENTS_INC_FOLDERS})
@@ -94,3 +116,30 @@ target_link_libraries(
     INTERFACE
     -T ${compact_log_linker_script}
 )
+
+# Link required libraries and add compiler flags to enable FreeRTOS region collection
+# in >= 4.4.0. Note: CMake does not short-circuit logic statements, nested ifs required
+# Note: ENV{IDF_VERSION} added in esp-idf 4.4.0
+if(DEFINED ENV{IDF_VERSION})
+  if ($ENV{IDF_VERSION} VERSION_GREATER_EQUAL "4.4.0")
+    # Policy change requires CMake v3.13+
+    cmake_minimum_required(VERSION 3.13)
+
+    # First set new policy for target_link_libraries, this resolves a warning when using on
+    # targets not created in this directory
+    cmake_policy(SET CMP0079 NEW)
+
+    # Get the name of the ESP FreeRTOS target/library
+    idf_component_get_property(freertos_lib freertos COMPONENT_LIB)
+
+    # Link this component to FreeRTOS, use INTERFACE because we're only sharing headers
+    target_link_libraries(${freertos_lib} INTERFACE ${this_component})
+
+    # Lastly ensure that our FreeRTOS trace hooks are defined first by adding this
+    # compile option to the FreeRTOS target to include with all source
+    # This method is an alternative to #include within FreeRTOSConfig.h which esp-idf
+    # makes very difficult to do.
+    get_filename_component(freertos_trace_header ${MEMFAULT_SDK_ROOT}/ports/include/memfault/ports/freertos_trace.h ABSOLUTE)
+    target_compile_options(${freertos_lib} INTERFACE -include ${freertos_trace_header})
+  endif()
+endif()
diff --git a/ports/esp_idf/memfault/common/memfault_esp_freertos.lf b/ports/esp_idf/memfault/common/memfault_esp_freertos.lf
@@ -0,0 +1,45 @@
+# This linker fragment should yield a similar snippet in
+# <build_dir>/esp-idf/esp_system/ld/sections.ld
+# The important lines are EXCLUDE_FILE and the symbols surrounding the
+# task and timer rules
+#/* Shared RAM */
+#  .dram0.bss (NOLOAD) :
+#  {
+#    . = ALIGN (8);
+#    _bss_start = ABSOLUTE(.);
+#
+#    ...
+#    *(EXCLUDE_FILE(*libfreertos.a:tasks.* *libfreertos.a:timers.*) .bss EXCLUDE_FILE(*libfreertos.a:tasks.* *libfreertos.a:timers.*) .bss.*)
+#    *(EXCLUDE_FILE(*libfreertos.a:tasks.* *libfreertos.a:timers.*) COMMON)
+#    ...
+#
+#    ...
+#    _memfault_capture_tasks_start = ABSOLUTE(.);
+#    *libfreertos.a:tasks.*(.bss .bss.* COMMON)
+#    _memfault_capture_tasks_end = ABSOLUTE(.);
+#    _memfault_capture_timers_start = ABSOLUTE(.);
+#    *libfreertos.a:timers.*(.bss .bss.* COMMON)
+#    _memfault_capture_timers_end = ABSOLUTE(.);
+#    ...
+#  } > dram0_0_seg
+
+# Create a scheme to describe input sections to an output section
+[scheme:memfault]
+entries:
+    common -> dram0_bss
+    bss -> dram0_bss
+
+# Create a mapping using the defined scheme. Add linker symbols around
+# to capture start and end of task and timer location.
+# Note: ldgen does not properly generate rules for section fragments that include multiple
+# input sections. To work around this, we duplicate a rule for both bss and common section
+# fragments
+[mapping:memfault_freertos]
+archive: libfreertos.a
+entries:
+    timers (memfault);
+        bss-> dram0_bss SURROUND(memfault_timers_bss),
+        common -> dram0_bss SURROUND(memfault_timers_common)
+    tasks (memfault);
+        bss -> dram0_bss SURROUND(memfault_tasks_bss),
+        common -> dram0_bss SURROUND(memfault_tasks_common)
diff --git a/ports/esp_idf/memfault/common/memfault_fault_handler.c b/ports/esp_idf/memfault/common/memfault_fault_handler.c
@@ -21,9 +21,13 @@
 
 // Note: The esp-idf implements abort which will invoke the esp-idf coredump handler as well as a
 // chip reboot so we just piggback off of that
-void memfault_fault_handling_assert(void *pc, void *lr) { abort(); }
+void memfault_fault_handling_assert(void *pc, void *lr) {
+  memfault_xtensa_fault_handling_assert(pc, lr, kMfltRebootReason_Assert);
+  abort();
+}
 
 void memfault_fault_handling_assert_extra(void *pc, void *lr, sMemfaultAssertInfo *extra_info) {
+  memfault_xtensa_fault_handling_assert(pc, lr, extra_info->assert_reason);
   abort();
 }
 
diff --git a/ports/esp_idf/memfault/common/memfault_platform_coredump.c b/ports/esp_idf/memfault/common/memfault_platform_coredump.c
@@ -30,6 +30,11 @@
 #include "memfault/esp_port/uart.h"
 #include "memfault/util/crc16_ccitt.h"
 
+// Needed for >= v4.4.0 default coredump collection
+#if ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(4, 4, 0)
+  #include "memfault/ports/freertos_coredump.h"
+#endif
+
 // Factor out issues with Espressif's ESP32 to ESP conversion in sdkconfig
 #define COREDUMPS_ENABLED \
   (CONFIG_ESP32_ENABLE_COREDUMP || CONFIG_ESP_COREDUMP_ENABLE)
@@ -53,13 +58,26 @@
   esp_partition_find_first(ESP_PARTITION_TYPE_DATA, ESP_PARTITION_SUBTYPE_DATA_COREDUMP, NULL)
 #endif
 
+#if ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(4, 4, 0)
+  // Memory regions used for esp-idf >= 4.4.0
+  // Active stack (1) + task/timer and bss/common regions (4) +
+  // freertos tasks (MEMFAULT_PLATFORM_MAX_TASK_REGIONS) + bss(1) + data(1) + heap(1)
+  #define MEMFAULT_ESP_PORT_NUM_REGIONS (1 + 4 + MEMFAULT_PLATFORM_MAX_TASK_REGIONS + 1 + 1 + 1)
+#else  // ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(4, 4, 0)
+  // Memory regions for esp-idf < 4.4.0
+  // Active stack (1) + bss(1) + data(1) + heap(1)
+  #define MEMFAULT_ESP_PORT_NUM_REGIONS (1 + 1 + 1 + 1)
+#endif  // ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(4, 4, 0)
+
 typedef struct {
   uint32_t magic;
   esp_partition_t partition;
   uint32_t crc;
 } sEspIdfCoredumpPartitionInfo;
 
 static sEspIdfCoredumpPartitionInfo s_esp32_coredump_partition_info;
+static const uintptr_t esp32_dram_start_addr = SOC_DRAM_LOW;
+static const uintptr_t esp32_dram_end_addr = SOC_DRAM_HIGH;
 
 static uint32_t prv_get_partition_info_crc(void) {
   return memfault_crc16_ccitt_compute(MEMFAULT_CRC16_CCITT_INITIAL_VALUE,
@@ -75,42 +93,121 @@ static const esp_partition_t *prv_get_core_partition(void) {
   return &s_esp32_coredump_partition_info.partition;
 }
 
-//! By default, we attempt to collect all of internal RAM as part of a Coredump
+// We use two different default coredump collection methods
+// due to differences in esp-idf versions. The following helper
+// is only used for esp-idf >= 4.4.0
+#if ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(4, 4, 0)
+//! Helper function to get bss and common sections of task and timer objects
+size_t prv_get_freertos_bss_common(sMfltCoredumpRegion *regions, size_t num_regions) {
+  if (regions == NULL || num_regions == 0) {
+    return 0;
+  }
+
+  size_t region_index = 0;
+  extern uint32_t _memfault_timers_bss_start;
+  extern uint32_t _memfault_timers_bss_end;
+  extern uint32_t _memfault_timers_common_start;
+  extern uint32_t _memfault_timers_common_end;
+  extern uint32_t _memfault_tasks_bss_start;
+  extern uint32_t _memfault_tasks_bss_end;
+  extern uint32_t _memfault_tasks_common_start;
+  extern uint32_t _memfault_tasks_common_end;
+
+  // ldgen has a bug that does not exclude rules matching multiple input sections at the
+  // same time. To work around this, we instead emit a symbol for each section we're attempting
+  // to collect. This means 8 symbols (tasks/timers + bss/common). If this is ever fixed we
+  // can remove the need to collect 4 separate regions.
+  size_t timers_bss_length =
+    (uintptr_t)&_memfault_timers_bss_end - (uintptr_t)&_memfault_timers_bss_start;
+  size_t timers_common_length =
+    (uintptr_t)&_memfault_timers_common_end - (uintptr_t)&_memfault_timers_common_start;
+  size_t tasks_bss_length =
+    (uintptr_t)&_memfault_tasks_bss_end - (uintptr_t)&_memfault_tasks_bss_start;
+  size_t tasks_common_length =
+    (uintptr_t)&_memfault_tasks_common_end - (uintptr_t)&_memfault_tasks_common_start;
+
+  regions[region_index++] =
+    MEMFAULT_COREDUMP_MEMORY_REGION_INIT(&_memfault_timers_bss_start, timers_bss_length);
+  regions[region_index++] =
+    MEMFAULT_COREDUMP_MEMORY_REGION_INIT(&_memfault_timers_common_start, timers_common_length);
+  regions[region_index++] =
+    MEMFAULT_COREDUMP_MEMORY_REGION_INIT(&_memfault_tasks_bss_start, tasks_bss_length);
+  regions[region_index++] =
+    MEMFAULT_COREDUMP_MEMORY_REGION_INIT(&_memfault_tasks_common_start, tasks_common_length);
+
+  return region_index;
+}
+#endif  // ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(4, 4, 0)
+
+//! Simple implementation to ensure address is in SRAM range.
+//!
+//! @note The function is intentionally defined as weak so someone can
+//! easily override the port defaults by re-defining a non-weak version of
+//! the function in another file
+MEMFAULT_WEAK size_t memfault_platform_sanitize_address_range(void *start_addr,
+                                                              size_t desired_size) {
+  const uintptr_t start_addr_int = (uintptr_t)start_addr;
+  const uintptr_t end_addr_int = start_addr_int + desired_size;
+
+  if ((start_addr_int < esp32_dram_start_addr) || (start_addr_int > esp32_dram_end_addr)) {
+    return 0;
+  }
+
+  if (end_addr_int > esp32_dram_end_addr) {
+    return esp32_dram_end_addr - start_addr_int;
+  }
+
+  return desired_size;
+}
+
+//! By default we prioritize collecting active stack, bss, data, and heap.
+//!
+//! In esp-idf >= 4.4.0, we additionally collect bss and stack regions for
+//! FreeRTOS tasks.
 //!
 //! @note The function is intentionally defined as weak so someone can
 //! easily override the port defaults by re-defining a non-weak version of
 //! the function in another file
 MEMFAULT_WEAK
 const sMfltCoredumpRegion *memfault_platform_coredump_get_regions(
-    const sCoredumpCrashInfo *crash_info, size_t *num_regions) {
-  static sMfltCoredumpRegion s_coredump_regions[1];
-
-  // The ESP32S2 + S3 have a different memory map than the ESP32; IRAM and DRAM
-  // share the same pysical SRAM, but are mapped at different addresses. We need
-  // to account for the placement of IRAM data, which offsets the start of
-  // placed DRAM.
-  //
-  // https://www.espressif.com/sites/default/files/documentation/esp32-s2_technical_reference_manual_en.pdf#subsubsection.3.3.2
-  // https://www.espressif.com/sites/default/files/documentation/esp32-s3_technical_reference_manual_en.pdf#subsubsection.4.3.2
-#if defined(CONFIG_IDF_TARGET_ESP32S3) || defined(CONFIG_IDF_TARGET_ESP32S2)
-  // use this symbol defined by the linker to find the start of DRAM
+  const sCoredumpCrashInfo *crash_info, size_t *num_regions) {
+  static sMfltCoredumpRegion s_coredump_regions[MEMFAULT_ESP_PORT_NUM_REGIONS];
+  int region_idx = 0;
+
+  const size_t stack_size = memfault_platform_sanitize_address_range(
+    crash_info->stack_address, MEMFAULT_PLATFORM_ACTIVE_STACK_SIZE_TO_COLLECT);
+
+  // First, capture the active stack
+  s_coredump_regions[region_idx++] =
+    MEMFAULT_COREDUMP_MEMORY_REGION_INIT(crash_info->stack_address, stack_size);
+
+  // Second, capture the task regions, if esp-idf >= 4.4.0
+#if ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(4, 4, 0)
+  region_idx += memfault_freertos_get_task_regions(
+    &s_coredump_regions[region_idx], MEMFAULT_ARRAY_SIZE(s_coredump_regions) - region_idx);
+
+  // Third, capture the FreeRTOS-specific bss regions
+  region_idx += prv_get_freertos_bss_common(&s_coredump_regions[region_idx],
+                                            MEMFAULT_ARRAY_SIZE(s_coredump_regions) - region_idx);
+#endif  // ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(4, 4, 0)
+
+  // Next, capture all of .bss + .data that we can fit.
   extern uint32_t _data_start;
-  const uint32_t esp32_dram_start_addr = (uint32_t)&_data_start;
-#else
-  const uint32_t esp32_dram_start_addr = SOC_DMA_LOW;
-#endif
+  extern uint32_t _data_end;
+  extern uint32_t _bss_start;
+  extern uint32_t _bss_end;
+  extern uint32_t _heap_start;
 
-  size_t dram_collection_len = SOC_DMA_HIGH - esp32_dram_start_addr;
-  const esp_partition_t *core_part = prv_get_core_partition();
-  if (core_part != NULL) {
-    // NB: Leave some space in storage for other regions collected by the SDK
-    dram_collection_len = MEMFAULT_MIN((core_part->size * 7) / 8,
-                                       dram_collection_len);
-  }
+  s_coredump_regions[region_idx++] = MEMFAULT_COREDUMP_MEMORY_REGION_INIT(
+    &_bss_start, (uint32_t)((uintptr_t)&_bss_end - (uintptr_t)&_bss_start));
+  s_coredump_regions[region_idx++] = MEMFAULT_COREDUMP_MEMORY_REGION_INIT(
+    &_data_start, (uint32_t)((uintptr_t)&_data_end - (uintptr_t)&_data_start));
+  // Finally, capture as much of the heap as we can fit
+  s_coredump_regions[region_idx++] = MEMFAULT_COREDUMP_MEMORY_REGION_INIT(
+    &_heap_start, (uint32_t)(esp32_dram_end_addr - (uintptr_t)&_heap_start));
+
+  *num_regions = region_idx;
 
-  s_coredump_regions[0] = MEMFAULT_COREDUMP_MEMORY_REGION_INIT(
-      (void *)esp32_dram_start_addr, dram_collection_len);
-  *num_regions = MEMFAULT_ARRAY_SIZE(s_coredump_regions);
   return &s_coredump_regions[0];
 }
 
diff --git a/ports/freertos/src/memfault_freertos_ram_regions.c b/ports/freertos/src/memfault_freertos_ram_regions.c
diff --git a/scripts/cmsis_pack_bundle.py b/scripts/cmsis_pack_bundle.py

Original file line number	Diff line number	Diff line change
`@@ -19,7 +19,7 @@ typedef struct {`
`19`	`19`	`uint8_t patch;`
`20`	`20`	`} sMfltSdkVersion;`
`21`	`21`
`22`		`-#define MEMFAULT_SDK_VERSION { .major = 0, .minor = 39, .patch = 0 }`
	`22`	`+#define MEMFAULT_SDK_VERSION { .major = 0, .minor = 39, .patch = 1 }`
`23`	`23`
`24`	`24`	`#ifdef __cplusplus`
`25`	`25`	`}`