Ensure xPortGetFreeHeapSize reports DRAM

cores/esp8266/Arduino.h

@@ -33,6 +33,7 @@ extern "C" {
 #include <string.h>
 #include <math.h>
 
+#include "umm_malloc/umm_malloc_cfgport.h"
 #include "stdlib_noniso.h"
 #include "binary.h"
 #include "esp8266_peri.h"
@@ -311,7 +312,8 @@ void configTime(const char* tz, String server1,
 #endif
 
 #ifdef DEBUG_ESP_OOM
-// reinclude *alloc redefinition because of <cstdlib> undefining them
-// this is mandatory for allowing OOM *alloc definitions in .ino files
-#include "umm_malloc/umm_malloc_cfg.h"
+// Position *alloc redefinition at the end of Arduino.h because <cstdlib> would
+// have undefined them. Mandatory for supporting OOM and other debug alloc
+// definitions in .ino files
+#include "heap_api_debug.h"
 #endif

cores/esp8266/Esp-frag.cpp

@@ -19,10 +19,10 @@
  */
 
 #include "umm_malloc/umm_malloc.h"
-#include "umm_malloc/umm_malloc_cfg.h"
 #include "coredecls.h"
 #include "Esp.h"
 
+#if defined(UMM_INFO)
 void EspClass::getHeapStats(uint32_t* hfree, uint32_t* hmax, uint8_t* hfrag)
 {
     // L2 / Euclidean norm of free block sizes.
@@ -60,3 +60,4 @@ uint8_t EspClass::getHeapFragmentation()
 {
   return (uint8_t)umm_fragmentation_metric();
 }
+#endif

cores/esp8266/Esp.cpp

@@ -219,13 +219,15 @@ uint16_t EspClass::getVcc(void)
 
 uint32_t EspClass::getFreeHeap(void)
 {
-    return system_get_free_heap_size();
+    return umm_free_heap_size_lw();
 }
 
+#if defined(UMM_INFO)
 uint32_t EspClass::getMaxFreeBlockSize(void)
 {
     return umm_max_block_size();
 }
+#endif
 
 uint32_t EspClass::getFreeContStack()
 {

cores/esp8266/Esp.h

@@ -114,11 +114,12 @@ class EspClass {
         static uint32_t getChipId();
 
         static uint32_t getFreeHeap();
+#if defined(UMM_INFO)
         static uint32_t getMaxFreeBlockSize();
         static uint8_t getHeapFragmentation(); // in %
         static void getHeapStats(uint32_t* free = nullptr, uint16_t* max = nullptr, uint8_t* frag = nullptr) __attribute__((deprecated("Use 'uint32_t*' on max, 2nd argument")));
         static void getHeapStats(uint32_t* free = nullptr, uint32_t* max = nullptr, uint8_t* frag = nullptr);
-
+#endif
         static uint32_t getFreeContStack();
         static void resetFreeContStack();
 

cores/esp8266/heap.cpp

@@ -11,7 +11,7 @@ extern "C" size_t umm_umul_sat(const size_t a, const size_t b);
 extern "C" void z2EapFree(void *ptr, const char* file, int line) __attribute__((weak, alias("vPortFree"), nothrow));
 // I don't understand all the compiler noise around this alias.
 // Adding "__attribute__ ((nothrow))" seems to resolve the issue.
-// This may be relevant: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81824 
+// This may be relevant: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81824
 
 // Need FORCE_ALWAYS_INLINE to put HeapSelect class constructor/deconstructor in IRAM
 #define FORCE_ALWAYS_INLINE_HEAP_SELECT
@@ -342,8 +342,10 @@ size_t IRAM_ATTR xPortWantedSizeAlign(size_t size)
 
 void system_show_malloc(void)
 {
+#ifdef UMM_INFO
     HeapSelectDram ephemeral;
     umm_info(NULL, true);
+#endif
 }
 
 /*

cores/esp8266/heap_api_debug.h

@@ -0,0 +1,74 @@
+/*
+ * Issolated heap debug helper code from from umm_malloc/umm_malloc_cfg.h.
+ * Updated umm_malloc/umm_malloc.h and Arduino.h to reference.
+ * No #ifdef fenceing was used before. From its previous location, this content
+ * was reassert multiple times through Arduino.h. In case there are legacy
+ * projects that depend on the previous unfenced behavior, no fencing has been
+ * added.
+ */
+
+/*
+ * *alloc redefinition - Included from Arduino.h for DEBUG_ESP_OOM support.
+ *
+ * It can also be directly include by the sketch for UMM_POISON_CHECK or
+ * UMM_POISON_CHECK_LITE builds to get more info about the caller when they
+ * report on a fail.
+ */
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#ifdef DEBUG_ESP_OOM
+#define MEMLEAK_DEBUG
+
+#include "umm_malloc/umm_malloc_cfg.h"
+
+#include <pgmspace.h>
+// Reuse pvPort* calls, since they already support passing location information.
+// Specifically the debug version (heap_...) that does not force DRAM heap.
+void *IRAM_ATTR heap_pvPortMalloc(size_t size, const char *file, int line);
+void *IRAM_ATTR heap_pvPortCalloc(size_t count, size_t size, const char *file, int line);
+void *IRAM_ATTR heap_pvPortRealloc(void *ptr, size_t size, const char *file, int line);
+void *IRAM_ATTR heap_pvPortZalloc(size_t size, const char *file, int line);
+void IRAM_ATTR heap_vPortFree(void *ptr, const char *file, int line);
+
+#define malloc(s) ({ static const char mem_debug_file[] PROGMEM STORE_ATTR = __FILE__; heap_pvPortMalloc(s, mem_debug_file, __LINE__); })
+#define calloc(n,s) ({ static const char mem_debug_file[] PROGMEM STORE_ATTR = __FILE__; heap_pvPortCalloc(n, s, mem_debug_file, __LINE__); })
+#define realloc(p,s) ({ static const char mem_debug_file[] PROGMEM STORE_ATTR = __FILE__; heap_pvPortRealloc(p, s, mem_debug_file, __LINE__); })
+
+#if defined(UMM_POISON_CHECK) || defined(UMM_POISON_CHECK_LITE)
+#define dbg_heap_free(p) ({ static const char mem_debug_file[] PROGMEM STORE_ATTR = __FILE__; heap_vPortFree(p, mem_debug_file, __LINE__); })
+#else
+#define dbg_heap_free(p) free(p)
+#endif
+
+#elif defined(UMM_POISON_CHECK) || defined(UMM_POISON_CHECK_LITE) // #elif for #ifdef DEBUG_ESP_OOM
+#include <pgmspace.h>
+void *IRAM_ATTR heap_pvPortRealloc(void *ptr, size_t size, const char *file, int line);
+#define realloc(p,s) ({ static const char mem_debug_file[] PROGMEM STORE_ATTR = __FILE__; heap_pvPortRealloc(p, s, mem_debug_file, __LINE__); })
+
+void IRAM_ATTR heap_vPortFree(void *ptr, const char *file, int line);
+// C - to be discussed
+/*
+  Problem, I would like to report the file and line number with the umm poison
+  event as close as possible to the event. The #define method works for malloc,
+  calloc, and realloc those names are not as generic as free. A #define free
+  captures too much. Classes with methods called free are included :(
+  Inline functions would report the address of the inline function in the .h
+  not where they are called.
+
+  Anybody know a trick to make this work?
+
+  Create dbg_heap_free() as an alternative for free() when you need a little
+  more help in debugging the more challenging problems.
+*/
+#define dbg_heap_free(p) ({ static const char mem_debug_file[] PROGMEM STORE_ATTR = __FILE__; heap_vPortFree(p, mem_debug_file, __LINE__); })
+
+#else
+#define dbg_heap_free(p) free(p)
+#endif /* DEBUG_ESP_OOM */
+
+#ifdef __cplusplus
+}
+#endif

cores/esp8266/mmu_iram.h

@@ -56,7 +56,8 @@ extern "C" {
  #define DEV_DEBUG_PRINT
  */
 
-#if defined(DEV_DEBUG_PRINT) || defined(DEBUG_ESP_MMU)
+#if (defined(DEV_DEBUG_PRINT) || defined(DEBUG_ESP_MMU)) && !defined(HOST_MOCK)
+// Errors follow when `#include <esp8266_peri.h>` is present when running CI HOST
 #include <esp8266_peri.h>
 
 #define DBG_MMU_FLUSH(a) while((USS(a) >> USTXC) & 0xff) {}

cores/esp8266/umm_malloc/Notes.h

@@ -276,4 +276,70 @@ Enhancement ideas:
   #endif
   ```
 */
+
+/*
+  Sep 26, 2022
+
+  History/Overview
+
+  ESP.getFreeHeap() needs a function it can call for free Heap size. The legacy
+  method was the SDK function `system_get_free_heap_size()` which is in IRAM.
+
+  `system_get_free_heap_size()` calls `xPortGetFreeHeapSize()` to get free heap
+  size. Our old legacy implementation used umm_info(), employing a
+  time-consuming method for getting free Heap size and runs with interrupts
+  blocked.
+
+  Later we used a distributed method that maintained the free heap size with
+  each malloc API call that changed the Heap. (enabled by build option UMM_STATS
+  or UMM_STATS_FULL) We used an internally function `umm_free_heap_size_lw()` to
+  report free heap size. We satisfied the requirements for
+  `xPortGetFreeHeapSize()` with an alias to `umm_free_heap_size_lw()`
+  in replacement for the legacy umm_info() call wrapper.
+
+  The upstream umm_malloc later implemented a similar method enabled by build
+  option UMM_INLINE_METRICS and introduced the function `umm_free_heap_size()`.
+
+  The NONOS SDK alloc request must use the DRAM Heap. Need to Ensure DRAM Heap
+  results when multiple Heap support is enabled. Since the SDK uses portable
+  malloc calls pvPortMalloc, ... we leveraged that for a solution - force
+  pvPortMalloc, ... APIs to serve DRAM only.
+
+  In an oversight, `xPortGetFreeHeapSize()` was left reporting the results for
+  the current heap selection via `umm_free_heap_size_lw()`. Thus, if an SDK
+  function like os_printf_plus were called when the current heap selection was
+  IRAM, it would get the results for the IRAM Heap. Then would receive DRAM with
+  an alloc request. However, when the free IRAM size is too small, it would
+  skip the Heap alloc request and use stack space.
+
+  Solution
+
+  The resolution is to rely on build UMM_STATS(default) or UMM_STATS_FULL for
+  free heap size information. When not available in the build, fallback to the
+  upstream umm_malloc's `umm_free_heap_size()` and require the build option
+  UMM_INLINE_METRICS. Otherwise, fail the build.
+
+  Use function name `umm_free_heap_size_lw()` to support external request for
+  current heap size. When build options result in fallback using umm_info.c,
+  ensure UMM_INLINE_METRICS enabled and alias to `umm_free_heap_size()`.
+
+  For the multiple Heap case, `xPortGetFreeHeapSize()` becomes a unique function
+  and reports only DRAM free heap size. Now `system_get_free_heap_size()` will
+  always report DRAM free Heap size. This might be a breaking change.
+
+  Specifics:
+
+  * Support `umm_free_heap_size_lw()` as an `extern`.
+
+  * When the build options UMM_STATS/UMM_STATS_FULL are not used, fallback to
+    the upstream umm_malloc's `umm_free_heap_size()` function in umm_info.c
+      * require the UMM_INLINE_METRICS build option.
+      * assign `umm_free_heap_size_lw()` as an alias to `umm_free_heap_size()`
+
+  * `xPortGetFreeHeapSize()`
+      * For single heap builds, alias to `umm_free_heap_size_lw()`
+      * For multiple Heaps builds, add a dedicated function that always reports
+        DRAM results.
+
+*/
 #endif

cores/esp8266/umm_malloc/umm_info.c

@@ -174,6 +174,14 @@ size_t umm_free_heap_size_core(umm_heap_context_t *_context) {
     return (size_t)_context->info.freeBlocks * sizeof(umm_block);
 }
 
+/*
+  When used as the fallback option for supporting exported function
+  `umm_free_heap_size_lw()`, the build option UMM_INLINE_METRICS is required.
+  Otherwise, umm_info() would be used to complete the operation, which uses a
+  time-consuming method for getting free Heap and runs with interrupts off,
+  which can negatively impact WiFi operations. Also, it cannot support calls
+  from ISRs, `umm_info()` runs from flash.
+*/
 size_t umm_free_heap_size(void) {
     #ifndef UMM_INLINE_METRICS
     umm_info(NULL, false);

cores/esp8266/umm_malloc/umm_local.c

@@ -161,35 +161,97 @@ void umm_poison_free_fl(void *ptr, const char *file, int line) {
 /* ------------------------------------------------------------------------ */
 
 #if defined(UMM_STATS) || defined(UMM_STATS_FULL) || defined(UMM_INFO)
+/*
+  For internal, mainly used by UMM_STATS_FULL; exported so external components
+  can perform Heap related calculations.
+*/
 size_t umm_block_size(void) {
     return sizeof(umm_block);
 }
 #endif
 
+/*
+  Need to expose a function to support getting the current free heap size.
+  Export `size_t umm_free_heap_size_lw(void)` for this purpose.
+  Used by ESP.getFreeHeap().
+
+  For an expanded discussion see Notes.h, entry dated "Sep 26, 2022"
+*/
 #if defined(UMM_STATS) || defined(UMM_STATS_FULL)
-// Keep complete call path in IRAM
+/*
+  Default build option to support export.
+
+  Keep complete call path in IRAM.
+*/
 size_t umm_free_heap_size_lw(void) {
     UMM_CHECK_INITIALIZED();
 
     umm_heap_context_t *_context = umm_get_current_heap();
     return (size_t)_context->UMM_FREE_BLOCKS * sizeof(umm_block);
 }
-#endif
 
+#elif defined(UMM_INLINE_METRICS)
 /*
-  I assume xPortGetFreeHeapSize needs to be in IRAM. Since
-  system_get_free_heap_size is in IRAM. Which would mean, umm_free_heap_size()
-  in flash, was not a safe alternative for returning the same information.
+  For the fallback option using `size_t umm_free_heap_size(void)`, we must have
+  the UMM_INLINE_METRICS build option enabled to support free heap size
+  reporting without the use of `umm_info()`.
 */
+size_t umm_free_heap_size_lw(void) __attribute__ ((alias("umm_free_heap_size")));
+
+#else
+/*
+  We require a resource to track and report free Heap size with low overhead.
+  For an expanded discussion see Notes.h, entry dated "Sep 26, 2022"
+*/
+#error UMM_INLINE_METRICS, UMM_STATS, or UMM_STATS_FULL needs to be defined.
+#endif
+
 #if defined(UMM_STATS) || defined(UMM_STATS_FULL)
-size_t xPortGetFreeHeapSize(void) __attribute__ ((alias("umm_free_heap_size_lw")));
+size_t umm_free_heap_size_core_lw(umm_heap_context_t *_context) {
+    return (size_t)_context->UMM_FREE_BLOCKS * sizeof(umm_block);
+}
+
 #elif defined(UMM_INFO)
-#ifndef UMM_INLINE_METRICS
-#warning "No ISR safe function available to implement xPortGetFreeHeapSize()"
+// Backfill support for umm_free_heap_size_core_lw()
+size_t umm_free_heap_size_core_lw(umm_heap_context_t *_context) __attribute__ ((alias("umm_free_heap_size_core")));
 #endif
+
+/*
+  This API is called by `system_get_free_heap_size()` which is in IRAM. Driving
+  the assumption the callee may be in an ISR or Cache_Read_Disable state. Use
+  IRAM to ensure that the complete call chain is in IRAM.
+
+  To satisfy this requirement, we need UMM_STATS... or UMM_INLINE_METRICS
+  defined. These support an always available without intense computation
+  free-Heap value.
+
+  Like the other vPort... APIs used by the SDK, this must always report on the
+  DRAM Heap not the current Heap.
+*/
+#if (UMM_NUM_HEAPS == 1)
+// Reduce IRAM usage for the single Heap case
+#if defined(UMM_STATS) || defined(UMM_STATS_FULL)
+size_t xPortGetFreeHeapSize(void) __attribute__ ((alias("umm_free_heap_size_lw")));
+#else
 size_t xPortGetFreeHeapSize(void) __attribute__ ((alias("umm_free_heap_size")));
 #endif
 
+#else
+size_t xPortGetFreeHeapSize(void) {
+    #if defined(UMM_STATS) || defined(UMM_STATS_FULL) || defined(UMM_INLINE_METRICS)
+    UMM_CHECK_INITIALIZED();
+    umm_heap_context_t *_context = umm_get_heap_by_id(UMM_HEAP_DRAM);
+
+    return umm_free_heap_size_core_lw(_context);
+    #else
+    // At this time, this build path is not reachable. In case things change,
+    // keep build check.
+    // Not in IRAM, umm_info() would have been used to complete this operation.
+    #error "No ISR safe function available to implement xPortGetFreeHeapSize()"
+    #endif
+}
+#endif
+
 #if defined(UMM_STATS) || defined(UMM_STATS_FULL)
 void umm_print_stats(int force) {
     umm_heap_context_t *_context = umm_get_current_heap();

cores/esp8266/umm_malloc/umm_malloc.h

@@ -10,8 +10,10 @@
 
 #include <stdint.h>
 
-// C This include is not in upstream
+// C These includes are not in the upstream
 #include "umm_malloc_cfg.h"   /* user-dependent */
+#include <osapi.h>
+#include <heap_api_debug.h>
 
 #ifdef __cplusplus
 extern "C" {