Reorganize local enhancements

More comment cleanups
Added Status TODOs marks for upstream

cores/esp8266/umm_malloc/umm_info.c

@@ -157,18 +157,8 @@ void *umm_info( void *ptr, int force ) {
   }
   DBGLOG_FORCE( force, "+--------------------------------------------------------------+\n" );
 
-  DBGLOG_FORCE( force, "umm heap statistics:\n");
-  DBGLOG_FORCE( force,   "  Free Space        %5u\n", ummStats.free_blocks * sizeof(umm_block));
-#if defined(UMM_STATS_FULL)
-  DBGLOG_FORCE( force,   "  Low Watermark     %5u\n", ummStats.free_blocks_min * sizeof(umm_block));
-  DBGLOG_FORCE( force,   "  Low Watermark ISR %5u\n", ummStats.free_blocks_isr_min * sizeof(umm_block));
-  DBGLOG_FORCE( force,   "  MAX Alloc Request %5u\n", ummStats.alloc_max_size);
-  DBGLOG_FORCE( force,   "  OOM Count         %5u\n", ummStats.oom_count);
+  print_stats(force);
 #endif
-  DBGLOG_FORCE( force,   "  Size of umm_block %5u\n", sizeof(umm_block));
-  DBGLOG_FORCE( force, "+--------------------------------------------------------------+\n" );
-#endif
-
 
   /* Release the critical section... */
   UMM_CRITICAL_EXIT(id_info);
@@ -178,7 +168,7 @@ void *umm_info( void *ptr, int force ) {
 
 /* ------------------------------------------------------------------------ */
 
-size_t umm_free_heap_size_info( void ) {
+size_t umm_free_heap_size( void ) {
   umm_info(NULL, 0);
   return (size_t)ummHeapInfo.freeBlocks * sizeof(umm_block);
 }
@@ -189,22 +179,7 @@ size_t umm_max_block_size( void ) {
 }
 
 /* ------------------------------------------------------------------------ */
-#endif
 
-#if defined(UMM_STATS) || defined(UMM_INFO)
-size_t umm_block_size( void ) {
-  return sizeof(umm_block);
-}
-#endif
-
-#if defined(UMM_STATS) || defined(UMM_STATS_FULL)
-
-UMM_STATISTICS ummStats;
-
-// Complete call path in IRAM
-size_t umm_free_heap_size_lw( void ) {
-  return (size_t)ummStats.free_blocks * sizeof(umm_block);
-}
 #endif
 
 #endif  // defined(BUILD_UMM_MALLOC_C)

cores/esp8266/umm_malloc/umm_local.c

@@ -149,12 +149,51 @@ void umm_poison_free_fl(void *ptr, const char* file, int line) {
 }
 #endif
 
+/* ------------------------------------------------------------------------ */
+
+#if defined(UMM_STATS) || defined(UMM_STATS_FULL) || defined(UMM_INFO)
+size_t umm_block_size( void ) {
+  return sizeof(umm_block);
+}
+#endif
+
+#if defined(UMM_STATS) || defined(UMM_STATS_FULL)
+UMM_STATISTICS ummStats;
+
+// Keep complete call path in IRAM
+size_t umm_free_heap_size_lw( void ) {
+  return (size_t)ummStats.free_blocks * sizeof(umm_block);
+}
+#endif
+
+/*
+  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.
+*/
 #if defined(UMM_STATS) || defined(UMM_STATS_FULL)
-size_t umm_free_heap_size( void ) __attribute__ ((alias("umm_free_heap_size_lw")));
 size_t xPortGetFreeHeapSize(void) __attribute__ ((alias("umm_free_heap_size_lw")));
 #elif defined(UMM_INFO)
-size_t umm_free_heap_size( void ) __attribute__ ((alias("umm_free_heap_size_info")));
-size_t xPortGetFreeHeapSize(void) __attribute__ ((alias("umm_free_heap_size_info")));
+#warning "No ISR safe function available to implement xPortGetFreeHeapSize()"
+size_t xPortGetFreeHeapSize(void) __attribute__ ((alias("umm_free_heap_size")));
 #endif
 
+#if defined(UMM_STATS) || defined(UMM_STATS_FULL)
+void print_stats(int force) {
+  DBGLOG_FORCE( force, "umm heap statistics:\n");
+  DBGLOG_FORCE( force,   "  Free Space        %5u\n", ummStats.free_blocks * sizeof(umm_block));
+  DBGLOG_FORCE( force,   "  OOM Count         %5u\n", ummStats.oom_count);
+#if defined(UMM_STATS_FULL)
+  DBGLOG_FORCE( force,   "  Low Watermark     %5u\n", ummStats.free_blocks_min * sizeof(umm_block));
+  DBGLOG_FORCE( force,   "  Low Watermark ISR %5u\n", ummStats.free_blocks_isr_min * sizeof(umm_block));
+  DBGLOG_FORCE( force,   "  MAX Alloc Request %5u\n", ummStats.alloc_max_size);
+#endif
+  DBGLOG_FORCE( force,   "  Size of umm_block %5u\n", sizeof(umm_block));
+  DBGLOG_FORCE( force, "+--------------------------------------------------------------+\n" );
+}
 #endif
+
+
+#endif // BUILD_UMM_MALLOC_C
+
+

cores/esp8266/umm_malloc/umm_local.h

@@ -34,4 +34,8 @@
 static int check_poison_neighbors( unsigned short cur );
 #endif
 
+#if defined(UMM_STATS) || defined(UMM_STATS_FULL)
+void ICACHE_FLASH_ATTR print_stats(int force);
+#endif
+
 #endif

cores/esp8266/umm_malloc/umm_malloc.cpp

@@ -41,15 +41,10 @@
   * In umm_malloc.c
   *   Renamed to umm_malloc.cpp
   *
-  *   Added `extern "C" { ...b};` around code.
+  *   Added `extern "C" { ... };` around code.
   *
-  *   Surround DBGLOG_LEVEL with #ifndef... Now defined value in umm_malloc_cfg.h
-  *
-  *   umm_free() - moved critical section to start after safe calculations.
-  *
-  *   umm_malloc() - moved critical section to start after umm_blocks()
-  *   computations are based on constants that don't change, calling
-  *   argument excepted.
+  *   Surround DBGLOG_LEVEL with #ifndef... Define value of DBGLOG_LEVEL
+  *   from umm_malloc_cfg.h
   *
   *   umm_realloc() - Added UMM_CRITICAL_SUSPEND()/UMM_CRITICAL_RESUME() for
   *   when lightweight locks are available. eg. sti/cli. Single threaded
@@ -119,7 +114,7 @@
   *   add to list to report.
   */
  /*
-  * Current Deltas from the old umm_malloc
+  * New upstream umm_malloc feature delta's from the old umm_malloc we were using:
   *
   *   umm_posion check for a given *alloc - failure - no longer panics.
   *
@@ -132,8 +127,10 @@
   *   Defragmenting effect of realloc is gone. It now minimizes copy. This
   *   may have been an accident during code cleanup.
   *
+  * In one form or another these features have been restored in the
+  * reintegration of the upstream umm_malloc into the Arduino ESP8266 Core.
   */
-/* This block will be used for the PR description:
+/* This block will be used for the non-WIP PR description:
 
 This updates the heap management library, umm_malloc, to the current upstream
 version at https://github.com/rhempel/umm_malloc. Some reorganizing and new code

cores/esp8266/umm_malloc/umm_malloc_cfg.h

@@ -108,10 +108,8 @@ extern char _heap_start[];
   extern UMM_HEAP_INFO ummHeapInfo;
 
   void ICACHE_FLASH_ATTR *umm_info( void *ptr, int force );
-  size_t ICACHE_FLASH_ATTR umm_free_heap_size_info( void );
+  size_t ICACHE_FLASH_ATTR umm_free_heap_size( void );
   size_t ICACHE_FLASH_ATTR umm_max_block_size( void );
-  size_t ICACHE_FLASH_ATTR umm_block_size( void );
-
 #else
 #endif
 
@@ -127,13 +125,16 @@ extern char _heap_start[];
  *
  * UMM_INFO_PRINT is enabled as part of selecting `Debug port: "Serial" or
  * "Serial1"`. To make available all the time use '-D UMM_INFO_PRINT`.
+ *
+ * Status: Local Enhancement - addresses platform specific issue.
  */
-#if defined(DEBUG_ESP_PORT) && !defined(UMM_INFO_PRINT)
+#if defined(DEBUG_ESP_PORT) && !defined(UMM_INFO_PRINT) && defined(UMM_INFO)
 #define UMM_INFO_PRINT
 #endif
 
 /*
  * -D UMM_STATS :
+ * -D UMM_STATS_FULL
  *
  * This option provides a lightweight alternative to using `umm_info` just for
  * getting `umm_free_heap_size`.  With this option, a "free blocks" value is
@@ -144,18 +145,31 @@ extern char _heap_start[];
  * example is when an app closely monitors free heap to detect memory leaks. In
  * this case a single-core CPUs interrupt processing would have suffered the
  * most.
+ *
+ * UMM_STATS_FULL provides additional heap statistics. It can be used to gain
+ * additional insight into heap usage. This option would add an additional 132
+ * bytes of IRAM.
+ *
+ * Status: TODO: Needs to be proposed for upstream.
  */
 /*
 #define UMM_STATS
 #define UMM_STATS_FULL
  */
 
-#if defined(DEBUG_ESP_PORT) && !defined(UMM_STATS) && !defined(UMM_STATS_FULL)
-#define UMM_STATS_FULL
-#elif !defined(UMM_STATS) && !defined(UMM_STATS_FULL)
+/*
+ * For the ESP8266 we want at lest UMM_STATS built, so we have an ISR safe
+ * function to call for implementing xPortGetFreeHeapSize(), because umm_info()
+ * is in flash.
+ */
+#if !defined(UMM_STATS) && !defined(UMM_STATS_FULL)
 #define UMM_STATS
 #endif
 
+#if defined(UMM_STATS) && defined(UMM_STATS_FULL)
+#undef UMM_STATS
+#endif
+
 #if defined(UMM_STATS) || defined(UMM_STATS_FULL)
 
 typedef struct UMM_STATISTICS_t {
@@ -191,6 +205,10 @@ static inline size_t ICACHE_FLASH_ATTR umm_get_oom_count( void ) {
 #define STATS__OOM_UPDATE()          (void)0
 #endif
 
+#if defined(UMM_STATS) || defined(UMM_STATS_FULL) || defined(UMM_INFO)
+size_t ICACHE_FLASH_ATTR umm_block_size( void );
+#endif
+
 #ifdef UMM_STATS_FULL
 #define STATS__FREE_BLOCKS_MIN() \
 do { \
@@ -285,10 +303,6 @@ static inline size_t ICACHE_FLASH_ATTR umm_get_free_null_count( void ) {
 #define STATS__FREE_REQUEST(tag)          (void)0
 #endif
 
-#if defined(UMM_STATS) || defined(UMM_STATS_FULL) || defined(UMM_INFO)
-size_t umm_free_heap_size( void );
-#endif
-
 /*
   Per Devyte, the core currently doesn't support masking a specific interrupt
   level. That doesn't mean it can't be implemented, only that at this time
@@ -306,11 +320,12 @@ size_t umm_free_heap_size( void );
  *
  * Build option to collect timing usage data on critical section usage in
  * functions: info, malloc, realloc. Collects MIN, MAX, and number of time IRQs
- * were disabled at request time. Note, for realloc MAX disabled time will not
- * include the time from calling malloc and/or free when UMM_LIGHTWEIGHT_CPU is
- * defined. Examine code for specifics on what info is available and how to
- * access.
-*/
+ * were disabled at request time. Note, for realloc MAX disabled time will
+ * include the time spent in calling malloc and/or free. Examine code for
+ * specifics on what info is available and how to access.
+ *
+ * Status: TODO: Needs to be proposed for upstream.
+ */
 /*
 #define UMM_CRITICAL_METRICS
  */
@@ -402,11 +417,12 @@ static inline void _critical_exit(UMM_TIME_STAT *p, uint32_t *saved_ps) {
   * UMM_CRITICAL_SUSPEND and  UMM_CRITICAL_RESUME to the values of
   * UMM_CRITICAL_EXIT and UMM_CRITICAL_ENTRY.  These additional exit/entries
   * allow time to service interrupts during the reentrant sections of the code.
-  * Also, using these macros will relieve the nesting requirement.
   *
-  * These macros should probably not be used on multicore CPUs. Hardware locking
-  * methods sometimes carry higher overhead and may not be suitable for frequent
-  * calling.
+  * Performance may be impacked if used with multicore CPUs. The higher frquency
+  * of locking and unlocking may be an issue with locking methods that have a
+  * high overhead.
+  *
+  * Status: TODO: Needs to be proposed for upstream.
   */
 /*
  */
@@ -428,6 +444,10 @@ static inline void _critical_exit(UMM_TIME_STAT *p, uint32_t *saved_ps) {
  * shrinks an allocation, avoiding copy when possible. UMM_REALLOC_DEFRAG gives
  * priority with growing the revised allocation toward an adjacent hole in the
  * direction of the beginning of the heap when possible.
+ *
+ * Status: TODO: These are new options introduced to optionally restore the
+ * previous defrag propery of realloc. The issue has been raised in the upstream
+ * repo. No response at this time. Based on response, may propose for upstream.
  */
 /*
 #define UMM_REALLOC_MINIMIZE_COPY
@@ -503,6 +523,11 @@ static inline void _critical_exit(UMM_TIME_STAT *p, uint32_t *saved_ps) {
  * include its nearest allocated neighbors in the heap.
  * umm_malloc() will also checks the neighbors of the selected allocation
  * before use.
+ *
+ * Status: TODO?: UMM_POISON_CHECK_LITE is a new option. We could propose for
+ * upstream; however, the upstream version has much of the framework for calling
+ * poison check on each alloc call refactored out. Not sure how this will be
+ * received.
  */
 
 /*
@@ -571,6 +596,7 @@ static inline void _critical_exit(UMM_TIME_STAT *p, uint32_t *saved_ps) {
 // PROGMEM. Only the 1st parameter, fmt, is supported in PROGMEM.
 #define DBGLOG_FUNCTION(fmt, ...) ISR_PRINTF(fmt, ##__VA_ARGS__)
 #define DBGLOG_FUNCTION_P(fmt, ...) ISR_PRINTF_P(fmt, ##__VA_ARGS__)
+
 #else
 #define DBGLOG_FUNCTION(fmt, ...)   do { (void)fmt; } while(false)
 #define DBGLOG_FUNCTION_P(fmt, ...) do { (void)fmt; } while(false)
@@ -646,38 +672,14 @@ void* ICACHE_RAM_ATTR pvPortMalloc(size_t size, const char* file, int line);
 void* ICACHE_RAM_ATTR pvPortCalloc(size_t count, size_t size, const char* file, int line);
 void* ICACHE_RAM_ATTR pvPortRealloc(void *ptr, size_t size, const char* file, int line);
 void* ICACHE_RAM_ATTR pvPortZalloc(size_t size, const char* file, int line);
-void  ICACHE_RAM_ATTR vPortFree(void *ptr, const char* file, int line);
 #define malloc(s) ({ static const char mem_debug_file[] PROGMEM STORE_ATTR = __FILE__; pvPortMalloc(s, mem_debug_file, __LINE__); })
 #define calloc(n,s) ({ static const char mem_debug_file[] PROGMEM STORE_ATTR = __FILE__; pvPortCalloc(n, s, mem_debug_file, __LINE__); })
 #define realloc(p,s) ({ static const char mem_debug_file[] PROGMEM STORE_ATTR = __FILE__; pvPortRealloc(p, s, mem_debug_file, __LINE__); })
-  #if defined(UMM_POISON_CHECK) || defined(UMM_POISON_CHECK_LITE)
-    #if 0
-    #define free(p) ({ static const char mem_debug_file[] PROGMEM STORE_ATTR = __FILE__; vPortFree(p, mem_debug_file, __LINE__); })
-    #endif
-  #endif
 
 #elif defined(UMM_POISON_CHECK) || defined(UMM_POISON_CHECK_LITE)
 #include <pgmspace.h>
-
 void* ICACHE_RAM_ATTR pvPortRealloc(void *ptr, size_t size, const char* file, int line);
-void  ICACHE_RAM_ATTR vPortFree(void *ptr, const char* file, int line);
-
 #define realloc(p,s) ({ static const char mem_debug_file[] PROGMEM STORE_ATTR = __FILE__; pvPortRealloc(p, s, mem_debug_file, __LINE__); })
-#if 0
-//C
-/*
-  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?
-*/
-#define free(p) ({ static const char mem_debug_file[] PROGMEM STORE_ATTR = __FILE__; vPortFree(p, mem_debug_file, __LINE__); })
-#endif
-
 #endif /* DEBUG_ESP_OOM */
 
 #ifdef __cplusplus