Merge pull request #5550 from TD-er/bugfix/crash_esp32s3_psram

[ESP32S3] Fix random crashes on ESP32S3 with PSRAM

platformio_core_defs.ini

@@ -191,7 +191,7 @@ extra_scripts             = ${esp82xx_common.extra_scripts}
 ;platform_packages           = framework-arduinoespressif32 @ https://github.com/Jason2866/esp32-arduino-lib-builder/releases/download/2904-2115-5.5/framework-arduinoespressif32-release_v5.5-f2a3fa2b.tar.xz
 
 platform                     = https://github.com/Jason2866/platform-espressif32.git#Arduino/IDF55_gcc152
-platform_packages            = framework-arduinoespressif32 @ https://github.com/Jason2866/esp32-arduino-lib-builder/releases/download/1205-1404-5.5/framework-arduinoespressif32-release_v5.5-65012b38.tar.xz
+platform_packages            = framework-arduinoespressif32 @ https://github.com/Jason2866/esp32-arduino-lib-builder/releases/download/1905-1208-5.5/framework-arduinoespressif32-release_v5.5-f3cdb9d0.tar.xz
 
 custom_remove_include       = true
 
@@ -242,6 +242,7 @@ build_flags                 = -DESP32_STAGE
                               -DNO_GLOBAL_OPENTHREADCLI
                               -DNO_GLOBAL_WIFIPROV
 ;                              -Wsuggest-override
+                              -mstrict-align
 
 lib_ignore                = BLE
                             Zigbee

src/src/Globals/RamTracker.cpp

@@ -2,45 +2,36 @@
 
 #ifndef BUILD_NO_RAM_TRACKER
 
-#if FEATURE_TIMING_STATS
-#include "../DataStructs/TimingStats.h"
-#endif
+# if FEATURE_TIMING_STATS
+#  include "../DataStructs/TimingStats.h"
+# endif
 
-#include "../ESPEasyCore/ESPEasy_Log.h"
+# include "../ESPEasyCore/ESPEasy_Log.h"
 
-#include "../Globals/Settings.h"
-#include "../Globals/Statistics.h"
+# include "../Globals/Settings.h"
+# include "../Globals/Statistics.h"
 
-#include "../Helpers/Memory.h"
-#include "../Helpers/Misc.h"
-#include "../Helpers/StringConverter.h"
+# include "../Helpers/Memory.h"
+# include "../Helpers/Misc.h"
+# include "../Helpers/StringConverter.h"
 
 RamTracker myRamTracker;
 
 /********************************************************************************************\
    Global convenience functions calling RamTracker
  \*********************************************************************************************/
+void checkRAMtoLog(void)                                               { myRamTracker.getTraceBuffer(); }
 
-void checkRAMtoLog(void){
-  myRamTracker.getTraceBuffer();
-}
+void checkRAM(const String& flashString, int a)                        { checkRAM(flashString, String(a)); }
 
-void checkRAM(const String &flashString, int a ) {
-  checkRAM(flashString, String(a));
-}
+void checkRAM(const __FlashStringHelper *flashString, int a)           { checkRAM(String(flashString), String(a)); }
 
-void checkRAM(const __FlashStringHelper * flashString, int a) {
-  checkRAM(String(flashString), String(a));
-}
+void checkRAM(const __FlashStringHelper *flashString, const String& a) { checkRAM(String(flashString), a); }
 
-
-void checkRAM(const __FlashStringHelper * flashString, const String& a) {
-  checkRAM(String(flashString), a);
-}
-
-void checkRAM(const __FlashStringHelper * flashString, const __FlashStringHelper * a) {
+void checkRAM(const __FlashStringHelper *flashString, const __FlashStringHelper *a) {
   checkRAM_values values;
-  if (!values.mustContinue()) return;
+
+  if (!values.mustContinue()) { return; }
   String s = flashString;
   s += F(" (");
   s += a;
@@ -48,9 +39,10 @@ void checkRAM(const __FlashStringHelper * flashString, const __FlashStringHelper
   checkRAM(values, s);
 }
 
-void checkRAM(const String& flashString, const String &a ) {
+void checkRAM(const String& flashString, const String& a) {
   checkRAM_values values;
-  if (!values.mustContinue()) return;
+
+  if (!values.mustContinue()) { return; }
   String s = flashString;
   s += F(" (");
   s += a;
@@ -58,42 +50,60 @@ void checkRAM(const String& flashString, const String &a ) {
   checkRAM(values, s);
 }
 
-void checkRAM(const __FlashStringHelper * descr ) {
+void checkRAM(const __FlashStringHelper *descr) {
   checkRAM_values values;
-  if (values.mustContinue()) 
-    checkRAM(values, String(descr));
+
+  if (values.mustContinue()) {
+    String s;
+    s = descr;
+    checkRAM(values, s);
+  }
 }
 
 void checkRAM_PluginCall_task(uint8_t taskIndex, uint8_t Function) {
   checkRAM_values values;
-  if (!values.mustContinue()) return;
+
+  if (!values.mustContinue()) { return; }
   String s = concat(F("PluginCall_task_"), taskIndex + 1);
 
   s += F(" (");
-  #if FEATURE_TIMING_STATS
+  # if FEATURE_TIMING_STATS
   s += getPluginFunctionName(Function);
-  #else // if FEATURE_TIMING_STATS
+  # else // if FEATURE_TIMING_STATS
   s += String(Function);
-  #endif // if FEATURE_TIMING_STATS
+  # endif // if FEATURE_TIMING_STATS
   s += ')';
   checkRAM(values, s);
 }
 
-void checkRAM(const String& descr ) {
+void checkRAM(const String& descr) {
   checkRAM_values values;
+
   checkRAM(values, descr);
 }
 
 checkRAM_values::checkRAM_values() {
   freeStack = getFreeStackWatermark();
-#ifdef ESP32
+# ifdef ESP32
   freeRAM = ESP.getMinFreeHeap();
-#else
+# else
   freeRAM = FreeMem();
-#endif
+# endif // ifdef ESP32
 }
 
 bool checkRAM_values::mustContinue() const {
+  # ifdef ESP32
+
+  if (xPortInIsrContext()) {
+    // When called from an ISR, you should not send out logs.
+    // Allocating memory from within an ISR is a big no-no.
+    // Also long-time blocking like sending logs (especially to a syslog server)
+    // is also really not a good idea from an ISR call.
+    return false;
+  }
+  # endif // ifdef ESP32
+
+
   // Here we simply check to see if it is desired to continue creating a description string.
   // When no description string is created, it would still be nice to get some idea of the lowest stack/ram while we're here.
   if (Settings.EnableRAMTracking()) {
@@ -112,20 +122,21 @@ bool checkRAM_values::mustContinue() const {
   return false;
 }
 
-void checkRAM(const checkRAM_values & values, const String& descr)
+void checkRAM(const checkRAM_values& values, const String& descr)
 {
-  if (Settings.EnableRAMTracking())
+  if (Settings.EnableRAMTracking()) {
     myRamTracker.registerRamState(descr);
+  }
 
   if (values.freeStack <= lowestFreeStack) {
-    lowestFreeStack = values.freeStack;
+    lowestFreeStack         = values.freeStack;
     lowestFreeStackfunction = descr;
   }
 
   if (values.freeRAM <= lowestRAM)
   {
-    lowestRAM = values.freeRAM;
-    lowestRAMfunction = std::move(descr);
+    lowestRAM         = values.freeRAM;
+    lowestRAMfunction = descr;
   }
 }
 
@@ -133,8 +144,6 @@ void checkRAM(const checkRAM_values & values, const String& descr)
    RamTracker class
  \*********************************************************************************************/
 
-
-
 // find highest the trace with the largest minimum memory (gets replaced by worse one)
 unsigned int RamTracker::bestCaseTrace(void) {
   unsigned int lowestMemoryInTrace      = 0;
@@ -159,24 +168,28 @@ RamTracker::RamTracker(void) {
     free_string(traces[i]);
     tracesMemory[i] = 0xffffffff; // init with best case memory values, so they get replaced if memory goes lower
   }
+  const auto freeHeap(ESP.getFreeHeap());
 
   for (int i = 0; i < TRACEENTRIES; i++) {
     nextAction[i]            = "startup";
-    nextActionStartMemory[i] = ESP.getFreeHeap(); // init with best case memory values, so they get replaced if memory goes lower
+    nextActionStartMemory[i] = freeHeap; // init with best case memory values, so they get replaced if memory goes lower
   }
 }
 
 void RamTracker::registerRamState(const String& s) {   // store function
+  if (writePtr >= TRACEENTRIES) {
+    writePtr = 0;                                      // inc write pointer and wrap around too.
+  }
   nextAction[writePtr]            = s;                 // name and mem
   nextActionStartMemory[writePtr] = ESP.getFreeHeap(); // in cyclic buffer.
   int bestCase = bestCaseTrace();                      // find best case memory trace
 
   if (ESP.getFreeHeap() < tracesMemory[bestCase]) {    // compare to current memory value
     free_string(traces[bestCase]);
-    readPtr          = writePtr + 1;                   // read out buffer, oldest value first
+    readPtr = writePtr + 1;                            // read out buffer, oldest value first
 
-    if (readPtr >= TRACEENTRIES) { 
-      readPtr = 0;        // read pointer wrap around
+    if (readPtr >= TRACEENTRIES) {
+      readPtr = 0;                                     // read pointer wrap around
     }
     tracesMemory[bestCase] = ESP.getFreeHeap();        // store new lowest value of that trace
 
@@ -197,9 +210,10 @@ void RamTracker::registerRamState(const String& s) {   // store function
   }
 }
 
- // return giant strings, one line per trace. Add stremToWeb method to avoid large strings.
+// return giant strings, one line per trace. Add stremToWeb method to avoid large strings.
 void RamTracker::getTraceBuffer() {
-#ifndef BUILD_NO_DEBUG
+# ifndef BUILD_NO_DEBUG
+
   if (Settings.EnableRAMTracking() && loglevelActiveFor(LOG_LEVEL_DEBUG_DEV)) {
     String retval = F("Memtrace\n");
 
@@ -213,21 +227,22 @@ void RamTracker::getTraceBuffer() {
       retval.clear();
     }
   }
-#endif // ifndef BUILD_NO_DEBUG
+# endif // ifndef BUILD_NO_DEBUG
 }
 
 #else // BUILD_NO_RAM_TRACKER
+
 /*
 
-void checkRAMtoLog(void) {}
+   void checkRAMtoLog(void) {}
 
-void checkRAM(const String& flashString,
+   void checkRAM(const String& flashString,
               int           a) {}
 
-void checkRAM(const String& flashString,
+   void checkRAM(const String& flashString,
               const String& a) {}
 
-void checkRAM(const String& descr) {}
-*/
+   void checkRAM(const String& descr) {}
+ */
 
-#endif // BUILD_NO_RAM_TRACKER
+#endif // BUILD_NO_RAM_TRACKER

src/src/Globals/RamTracker.h

@@ -16,11 +16,11 @@
 class RamTracker {
 private:
 
-  String traces[TRACES];                            // trace of latest memory checks
-  unsigned int tracesMemory[TRACES];                // lowest memory for that  trace
+  String traces[TRACES]{};                            // trace of latest memory checks
+  unsigned int tracesMemory[TRACES]{};                // lowest memory for that  trace
   unsigned int readPtr, writePtr;                   // pointer to cyclic buffer
-  String nextAction[TRACEENTRIES];                  // buffer to record the names of functions before they are transfered to a trace
-  unsigned int nextActionStartMemory[TRACEENTRIES]; // memory levels for the functions.
+  String nextAction[TRACEENTRIES]{};                  // buffer to record the names of functions before they are transfered to a trace
+  unsigned int nextActionStartMemory[TRACEENTRIES]{}; // memory levels for the functions.
 
   unsigned int bestCaseTrace(void);
 

src/src/Helpers/StringConverter.cpp

@@ -123,7 +123,7 @@ void free_string(String& str) {
 #if defined(ESP32) || defined(CORE_POST_3_0_0)
   // Use current implementation of String::copy as this
   // invalidates and thus deallocates current buffer
-  str = (const char*)nullptr;
+//  str = (const char*)nullptr;
   str = String(); // No idea why this is needed, without it, some ESP32's may bootloop
 #else
   str = String();