and mutex for all

revised mutex and critical section handling for segment drawing
- simplified code with macros (no more #if ESP32 ... #endif)
- remove some critical sections (prevents interrupt stalling)
- added mutex to functions that change the list of segments
- added mutexes to all (potential) background drawing code
- use recursive mutexes to prevent accidental self-locking of tasks

wled00/FX_fcn.cpp

@@ -117,17 +117,11 @@ void Segment::allocLeds() {
   if ((size > 0) && (!ledsrgb || size > ledsrgbSize)) {    //softhack dont allocate zero bytes
     DEBUG_PRINTF("allocLeds (%d,%d to %d,%d), %u from %u\n", start, startY, stop, stopY, size, ledsrgb?ledsrgbSize:0);
 
-    #if defined(ARDUINO_ARCH_ESP32) // WLEDMM protect against parallel access
+    CRGB* oldLedsRgb = ledsrgb; // WLEDMM this makes the re-allocation an anmost-atomic and more threadsafe operation
-    portENTER_CRITICAL(&s_wled_strip_mux);
-    #endif
-    if (ledsrgb) free(ledsrgb);   // we need a bigger buffer, so free the old one first
     ledsrgb = nullptr;
-    ledsrgb = (CRGB*)calloc(size, 1);
+    if (oldLedsRgb) free(oldLedsRgb);   // we need a bigger buffer, so free the old one first
+    ledsrgb = (CRGB*)calloc(size, 1);   // WLEDMM This is an OS call, so we should not wrap it in portEnterCRITICAL
     ledsrgbSize = ledsrgb?size:0;
-    #if defined(ARDUINO_ARCH_ESP32) // WLEDMM protect against parallel access
-    portEXIT_CRITICAL(&s_wled_strip_mux);
-    #endif
-
     if (ledsrgb == nullptr) {
       USER_PRINTLN("allocLeds failed!!");
       errorFlag = ERR_LOW_BUF; // WLEDMM raise errorflag
@@ -1877,15 +1871,16 @@ void WS2812FX::finalizeInit(void)
     // this is a critical section that will be removed with PR #278 which removes _globalLeds
     // problem: suspendStripService provides interlocking, but there’s a window before service() observes it, 
     //          and ESP32 is dual-core. A critical section closes that window so the pointer swap is atomic across cores.
+    CRGB* oldGLeds = Segment::_globalLeds;
 #if defined(ARDUINO_ARCH_ESP32)
     portENTER_CRITICAL(&s_wled_strip_mux);
 #endif
-    free(Segment::_globalLeds);
     Segment::_globalLeds = nullptr;
-    purgeSegments(true);   // WLEDMM moved here, because it seems to improve stability.
 #if defined(ARDUINO_ARCH_ESP32)
     portEXIT_CRITICAL(&s_wled_strip_mux);
 #endif
+    free(oldGLeds);
+    purgeSegments(true);   // WLEDMM moved here, because it seems to improve stability.
   }
   if (useLedsArray && getLengthTotal()>0) { // WLEDMM avoid malloc(0)
     size_t arrSize = sizeof(CRGB) * getLengthTotal();
@@ -1994,9 +1989,10 @@ void WS2812FX::service() {
         // effect blending (execute previous effect)
         // actual code may be a bit more involved as effects have runtime data including allocated memory
         //if (seg.transitional && seg._modeP) (*_mode[seg._modeP])(progress());
-        #if defined(ARDUINO_ARCH_ESP32) // WLEDMM protect against parallel drawing
+
-        if (xSemaphoreTake(busDrawMux, 200) != pdTRUE) { delay(1); continue;}      // WLEDMM first acquire draw mutex
+        // WLEDMM protect against parallel drawing
-        #endif
+        if (esp32SemTake(busDrawMux, 200) != pdTRUE) { delay(1); continue;}      // WLEDMM first acquire draw mutex
+
         frameDelay = (*_mode[seg.currentMode(seg.mode)])();
 
         if (frameDelay < speedLimit) frameDelay = FRAMETIME;                    // WLEDMM limit effects that want to go faster than target FPS
@@ -2007,9 +2003,8 @@ void WS2812FX::service() {
 
         seg.lastBri = seg.currentBri(seg.on ? seg.opacity:0);                   // WLEDMM remember for next time
         seg.handleTransition();
-        #if defined(ARDUINO_ARCH_ESP32)
+
-        xSemaphoreGive(busDrawMux); // WLEDMM unlock mutex
+        esp32SemGive(busDrawMux); // WLEDMM unlock mutex
-        #endif
       }
 
       seg.next_time = nowUp + frameDelay;
@@ -2150,13 +2145,10 @@ void WS2812FX::show(void) {
   // all of the data has been sent.
   // See https://github.com/Makuna/NeoPixelBus/wiki/ESP32-NeoMethods#neoesp32rmt-methods
 
-  #if defined(ARDUINO_ARCH_ESP32) // WLEDMM protect against parallel access
+  // WLEDMM protect against parallel access
-  if (xSemaphoreTake(busDrawMux, 200) != pdTRUE) { delay(1); return;}      // WLEDMM first acquire drawing permission (mutex), wait max 200ms
+  if (esp32SemTake(busDrawMux, 200) != pdTRUE) { delay(1); return;}      // WLEDMM first acquire drawing permission (mutex), wait max 200ms
-  #endif
   busses.show();
-  #if defined(ARDUINO_ARCH_ESP32)
+  esp32SemGive(busDrawMux);                                              // WLEDMM return permissions
-  xSemaphoreGive(busDrawMux);                                              // WLEDMM return permissions
-  #endif
 
   unsigned long diff = showNow - _lastShow;
   uint16_t fpsCurr = 200;
@@ -2355,15 +2347,19 @@ void WS2812FX::purgeSegments(bool force) {
   // remove all inactive segments (from the back)
   int deleted = 0;
   if (_segments.size() <= 1) return;
-  for (size_t i = _segments.size()-1; i > 0; i--)
+  // WLEDMM protect against parallel access while drawing
+  if (esp32SemTake(busDrawMux, 300) != pdTRUE) return;
+
+  for (size_t i = _segments.size()-1; i > 0; i--) {
     if (_segments[i].stop == 0 || force) {
       deleted++;
       _segments.erase(_segments.begin() + i);
-    }
+  } }
   if (deleted) {
     _segments.shrink_to_fit();
     /*if (_mainSegment >= _segments.size())*/ setMainSegmentId(0);
   }
+  esp32SemGive(busDrawMux);
 }
 
 Segment& WS2812FX::getSegment(uint8_t id) {
@@ -2391,6 +2387,9 @@ void WS2812FX::restartRuntime(bool doReset) {
 void WS2812FX::resetSegments(bool boundsOnly) { //WLEDMM add boundsonly
   DEBUG_PRINTF("resetSegments %d %dx%d\n", boundsOnly, Segment::maxWidth, Segment::maxHeight);
   if (!boundsOnly) {
+    // WLEDMM protect against parallel access while drawing
+    if (esp32SemTake(busDrawMux, portMAX_DELAY) != pdTRUE) return;  // warning: this waits until the mutex becomes availeable, no timeout
+
     _segments.clear(); // destructs all Segment as part of clearing
     #ifndef WLED_DISABLE_2D
     segment seg = isMatrix ? Segment(0, Segment::maxWidth, 0, Segment::maxHeight) : Segment(0, _length);
@@ -2399,6 +2398,7 @@ void WS2812FX::resetSegments(bool boundsOnly) { //WLEDMM add boundsonly
     #endif
     _segments.push_back(seg);
     _mainSegment = 0;
+    esp32SemGive(busDrawMux);
   } else { //WLEDMM boundsonly
     for (segment &seg : _segments) {
       #ifndef WLED_DISABLE_2D
@@ -2417,6 +2417,9 @@ void WS2812FX::resetSegments(bool boundsOnly) { //WLEDMM add boundsonly
 
 void WS2812FX::makeAutoSegments(bool forceReset) {
   if (autoSegments) { //make one segment per bus
+    // WLEDMM protect against parallel access while drawing
+    if (esp32SemTake(busDrawMux, portMAX_DELAY) != pdTRUE) return;  // warning: this waits until the mutex becomes availeable, no timeout
+
     uint16_t segStarts[MAX_NUM_SEGMENTS] = {0};
     uint16_t segStops [MAX_NUM_SEGMENTS] = {0};
     size_t s = 0;
@@ -2465,7 +2468,7 @@ void WS2812FX::makeAutoSegments(bool forceReset) {
     for (size_t i = 1; i < s; i++) {
       _segments.push_back(Segment(segStarts[i], segStops[i]));
     }
-
+    esp32SemGive(busDrawMux);
   } else {
 
     if (forceReset || getSegmentsNum() == 0) resetSegments();
@@ -2491,9 +2494,9 @@ void WS2812FX::makeAutoSegments(bool forceReset) {
 }
 
 void WS2812FX::fixInvalidSegments() {
-  #if defined(ARDUINO_ARCH_ESP32)          // WLEDMM protect against parallel access
+  // WLEDMM protect against parallel access while drawing
-  portENTER_CRITICAL(&s_wled_strip_mux);   // this locks out all parallel tasks, including PSRAM and flash filesystem access
+  if (esp32SemTake(busDrawMux, portMAX_DELAY) != pdTRUE) return;  // warning: this waits until the mutex becomes availeable, no timeout
-  #endif
+
   //make sure no segment is longer than total (sanity check)
   for (size_t i = getSegmentsNum()-1; i > 0; i--) {
     if (isMatrix) {
@@ -2513,15 +2516,13 @@ void WS2812FX::fixInvalidSegments() {
       if (_segments[i].stop  >  _length) _segments[i].stop = _length;
     }
   }
+  esp32SemGive(busDrawMux);  // give back the lock now, so purgeSegments can acquire it again
+
   // if any segments were deleted free memory
   purgeSegments();
   // this is always called as the last step after finalizeInit(), update covered bus types
   for (segment &seg : _segments)
     seg.refreshLightCapabilities();
-
-  #if defined(ARDUINO_ARCH_ESP32)
-  portEXIT_CRITICAL(&s_wled_strip_mux);
-  #endif
 }
 
 //true if all segments align with a bus, or if a segment covers the total length

wled00/e131.cpp

@@ -55,20 +55,16 @@ void handleDDPPacket(e131_packet_t* p) {
   realtimeLock(realtimeTimeoutMs, REALTIME_MODE_DDP);
 
   if (!realtimeOverride || (realtimeMode && useMainSegmentOnly)) {
-    #if defined(ARDUINO_ARCH_ESP32)
     // WLEDMM acquire drawing permission (wait max 200ms) before setting pixels
-    if (xSemaphoreTake(busDrawMux, 200) == pdTRUE) { 
+    if (esp32SemTake(busDrawMux, 200) == pdTRUE) { 
-    #endif
       for (uint16_t i = start; i < stop; i++) {
         setRealtimePixel(i, data[c], data[c+1], data[c+2], ddpChannelsPerLed >3 ? data[c+3] : 0);
         c += ddpChannelsPerLed;
         pixels++;
       }
       packets ++;
-    #if defined(ARDUINO_ARCH_ESP32)
+      esp32SemGive(busDrawMux);                                  // WLEDMM release drawing permissions
-      xSemaphoreGive(busDrawMux);                                  // WLEDMM release drawing permissions
     }
-    #endif
   }
 
   bool push = p->flags & DDP_PUSH_FLAG;
@@ -195,8 +191,11 @@ void handleDMXData(uint16_t uni, uint16_t dmxChannels, uint8_t* e131_data, uint8
       if (realtimeOverride && !(realtimeMode && useMainSegmentOnly)) return;
 
       wChannel = (availDMXLen > 3) ? e131_data[dataOffset+3] : 0;
-      for (uint16_t i = 0; i < totalLen; i++)
+      if (esp32SemTake(busDrawMux, 200) == pdTRUE) { // WLEDMM acquire drawing permission (wait max 200ms) before setting pixels
-        setRealtimePixel(i, e131_data[dataOffset+0], e131_data[dataOffset+1], e131_data[dataOffset+2], wChannel);
+        for (uint16_t i = 0; i < totalLen; i++)
+          setRealtimePixel(i, e131_data[dataOffset+0], e131_data[dataOffset+1], e131_data[dataOffset+2], wChannel);
+        esp32SemGive(busDrawMux);
+      }
       break;
 
     case DMX_MODE_SINGLE_DRGB:  // 4 channel: [Dimmer,R,G,B]
@@ -211,9 +210,11 @@ void handleDMXData(uint16_t uni, uint16_t dmxChannels, uint8_t* e131_data, uint8
         bri = e131_data[dataOffset+0];
         strip.setBrightness(bri, true);
       }
-
+      if (esp32SemTake(busDrawMux, 200) == pdTRUE) { // WLEDMM acquire drawing permission (wait max 200ms) before setting pixels
-      for (uint16_t i = 0; i < totalLen; i++)
+        for (uint16_t i = 0; i < totalLen; i++)
-        setRealtimePixel(i, e131_data[dataOffset+1], e131_data[dataOffset+2], e131_data[dataOffset+3], wChannel);
+          setRealtimePixel(i, e131_data[dataOffset+1], e131_data[dataOffset+2], e131_data[dataOffset+3], wChannel);
+        esp32SemGive(busDrawMux);
+      }
       break;
 
     case DMX_MODE_PRESET:       // 2 channel: [Dimmer,Preset]
@@ -357,16 +358,19 @@ void handleDMXData(uint16_t uni, uint16_t dmxChannels, uint8_t* e131_data, uint8
           }
         }
 
-        if (!is4Chan) {
+        if (esp32SemTake(busDrawMux, 200) == pdTRUE) { // WLEDMM acquire drawing permission (wait max 200ms) before setting pixels
-          for (uint16_t i = previousLeds; i < ledsTotal; i++) {
+          if (!is4Chan) {
-            setRealtimePixel(i, e131_data[dmxOffset], e131_data[dmxOffset+1], e131_data[dmxOffset+2], 0);
+            for (uint16_t i = previousLeds; i < ledsTotal; i++) {
-            dmxOffset+=3;
+              setRealtimePixel(i, e131_data[dmxOffset], e131_data[dmxOffset+1], e131_data[dmxOffset+2], 0);
-          }
+              dmxOffset+=3;
-        } else {
+            }
-          for (uint16_t i = previousLeds; i < ledsTotal; i++) {
+          } else {
-            setRealtimePixel(i, e131_data[dmxOffset], e131_data[dmxOffset+1], e131_data[dmxOffset+2], e131_data[dmxOffset+3]);
+            for (uint16_t i = previousLeds; i < ledsTotal; i++) {
-            dmxOffset+=4;
+              setRealtimePixel(i, e131_data[dmxOffset], e131_data[dmxOffset+1], e131_data[dmxOffset+2], e131_data[dmxOffset+3]);
+              dmxOffset+=4;
+            }
           }
+          esp32SemGive(busDrawMux);
         }
         break;
       }

wled00/json.cpp

@@ -353,8 +353,9 @@ bool deserializeSegment(JsonObject elem, byte it, byte presetId)
       USER_PRINTLN(F("deserializeSegment() image: strip is still drawing effects."));
       strip.waitUntilIdle();
     }
+    // WLEDMM protect against parallel drawing
+    if (esp32SemTake(busDrawMux, 250) == pdTRUE) {      // WLEDMM first acquire draw mutex, start of critical section
     seg.startFrame();
-    // WLEDMM end
 
     // set brightness immediately and disable transition
     transitionDelayTemp = 0;
@@ -404,6 +405,9 @@ bool deserializeSegment(JsonObject elem, byte it, byte presetId)
         set = 0;
       }
     }
+    esp32SemGive(busDrawMux); // release lock
+    } // end of critical section
+
     seg.map1D2D = oldMap1D2D; // restore mapping
     strip.trigger(); // force segment update
     suspendStripService = oldLock; // restore previous lock status

wled00/wled.cpp

@@ -477,8 +477,8 @@ void WLED::setup()
   init_math();  // WLEDMM: pre-calculate some lookup tables
 
   #ifdef ARDUINO_ARCH_ESP32
-  busDrawMux = xSemaphoreCreateBinary();      // WLEDMM prevent concurrent running of strip.show and strip.service
+  busDrawMux = xSemaphoreCreateRecursiveMutex();       // WLEDMM prevent concurrent running of strip.show and strip.service
-  xSemaphoreGive(busDrawMux);                 // init semaphores to initially allow drawing
+  xSemaphoreGiveRecursive(busDrawMux);                 // init semaphores to initially allow drawing
   #endif
 
   #ifdef ARDUINO_ARCH_ESP32
@@ -1103,16 +1103,16 @@ bool WLED::initEthernet()
 
 void WLED::initConnection()
 {
+  #ifdef WLED_ENABLE_WEBSOCKETS
+  ws.onEvent(wsEvent);
+  #endif
+
 #ifdef ARDUINO_ARCH_ESP32
   unsigned long t_wait = millis();
   while(strip.isUpdating() && (millis() - t_wait < 86)) delay(1); // WLEDMM try to catch a moment when strip is idle
   //if (strip.isUpdating()) USER_PRINTLN("WLED::initConnection: strip still updating.");
 #endif
 
-  #ifdef WLED_ENABLE_WEBSOCKETS
-  ws.onEvent(wsEvent);
-  #endif
-
   WiFi.disconnect(true);        // close old connections
 #ifdef ESP8266
   WiFi.setPhyMode(force802_3g ? WIFI_PHY_MODE_11G : WIFI_PHY_MODE_11N);

wled00/wled.h

@@ -766,8 +766,21 @@ WLED_GLOBAL volatile uint8_t loadedLedmap _INIT(0);         // WLEDMM default 0
 WLED_GLOBAL volatile bool suspendStripService _INIT(false); // WLEDMM temporarily prevent running strip.service, when strip or segments are "under update" and inconsistent
 WLED_GLOBAL volatile bool OTAisRunning _INIT(false);        // WLEDMM temporarily stop led updates during OTA
 
+// WLEDMM prevent concurrent strip.show() and strip.service() -> for DDP over ws, and other background tasks
 #ifdef ARDUINO_ARCH_ESP32
-WLED_GLOBAL SemaphoreHandle_t busDrawMux _INIT(nullptr);    // WLEDMM prevent concurrent strip.show() and strip.service() -> for DDP over ws
+WLED_GLOBAL SemaphoreHandle_t busDrawMux _INIT(nullptr);
+#define esp32SemTake(mux,timeout) xSemaphoreTakeRecursive(mux, timeout) // convenience macro that expands to xSemaphoreTakeRecursive
+#define esp32SemGive(mux)  xSemaphoreGiveRecursive(mux)                 // convenience macro that expands to xSemaphoreGiveRecursive
+#else
+// dummy for 8266
+#ifndef pdTRUE
+#define pdTRUE 1
+#endif
+#ifndef portMAX_DELAY
+#define portMAX_DELAY UINT32_MAX
+#endif
+#define esp32SemTake(mux,timeout) (pdTRUE)
+#define esp32SemGive(mux)
 #endif
 
 #ifndef ESP8266