From 7df63db744e158ec0a808ab7742f7d966e6fb8f1 Mon Sep 17 00:00:00 2001 From: Frank <91616163+softhack007@users.noreply.github.com> Date: Sat, 20 Dec 2025 17:13:52 +0100 Subject: [PATCH] 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 | 67 ++++++++++++++++++++++++----------------------- wled00/e131.cpp | 44 +++++++++++++++++-------------- wled00/json.cpp | 6 ++++- wled00/wled.cpp | 12 ++++----- wled00/wled.h | 15 ++++++++++- 5 files changed, 83 insertions(+), 61 deletions(-) diff --git a/wled00/FX_fcn.cpp b/wled00/FX_fcn.cpp index 5380a933..9d931dfb 100644 --- a/wled00/FX_fcn.cpp +++ b/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 - portENTER_CRITICAL(&s_wled_strip_mux); - #endif - if (ledsrgb) free(ledsrgb); // we need a bigger buffer, so free the old one first + CRGB* oldLedsRgb = ledsrgb; // WLEDMM this makes the re-allocation an anmost-atomic and more threadsafe operation 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 - #endif + + // WLEDMM protect against parallel drawing + 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 - #endif + + esp32SemGive(busDrawMux); // WLEDMM unlock mutex } 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 - if (xSemaphoreTake(busDrawMux, 200) != pdTRUE) { delay(1); return;} // WLEDMM first acquire drawing permission (mutex), wait max 200ms - #endif + // WLEDMM protect against parallel access + if (esp32SemTake(busDrawMux, 200) != pdTRUE) { delay(1); return;} // WLEDMM first acquire drawing permission (mutex), wait max 200ms busses.show(); - #if defined(ARDUINO_ARCH_ESP32) - xSemaphoreGive(busDrawMux); // WLEDMM return permissions - #endif + esp32SemGive(busDrawMux); // WLEDMM return permissions 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 - portENTER_CRITICAL(&s_wled_strip_mux); // this locks out all parallel tasks, including PSRAM and flash filesystem access - #endif + // WLEDMM protect against parallel access while drawing + if (esp32SemTake(busDrawMux, portMAX_DELAY) != pdTRUE) return; // warning: this waits until the mutex becomes availeable, no timeout + //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 diff --git a/wled00/e131.cpp b/wled00/e131.cpp index 42a66f01..6587c6e6 100644 --- a/wled00/e131.cpp +++ b/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) { - #endif + if (esp32SemTake(busDrawMux, 200) == pdTRUE) { 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) - xSemaphoreGive(busDrawMux); // WLEDMM release drawing permissions + esp32SemGive(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++) - setRealtimePixel(i, e131_data[dataOffset+0], e131_data[dataOffset+1], e131_data[dataOffset+2], wChannel); + if (esp32SemTake(busDrawMux, 200) == pdTRUE) { // WLEDMM acquire drawing permission (wait max 200ms) before setting pixels + 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); } - - for (uint16_t i = 0; i < totalLen; i++) - setRealtimePixel(i, e131_data[dataOffset+1], e131_data[dataOffset+2], e131_data[dataOffset+3], wChannel); + if (esp32SemTake(busDrawMux, 200) == pdTRUE) { // WLEDMM acquire drawing permission (wait max 200ms) before setting pixels + for (uint16_t i = 0; i < totalLen; i++) + 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) { - for (uint16_t i = previousLeds; i < ledsTotal; i++) { - 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++) { - setRealtimePixel(i, e131_data[dmxOffset], e131_data[dmxOffset+1], e131_data[dmxOffset+2], e131_data[dmxOffset+3]); - dmxOffset+=4; + if (esp32SemTake(busDrawMux, 200) == pdTRUE) { // WLEDMM acquire drawing permission (wait max 200ms) before setting pixels + if (!is4Chan) { + for (uint16_t i = previousLeds; i < ledsTotal; i++) { + 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++) { + setRealtimePixel(i, e131_data[dmxOffset], e131_data[dmxOffset+1], e131_data[dmxOffset+2], e131_data[dmxOffset+3]); + dmxOffset+=4; + } } + esp32SemGive(busDrawMux); } break; } diff --git a/wled00/json.cpp b/wled00/json.cpp index ed28f167..5c989b36 100644 --- a/wled00/json.cpp +++ b/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 diff --git a/wled00/wled.cpp b/wled00/wled.cpp index 0f467d93..6d148bb6 100644 --- a/wled00/wled.cpp +++ b/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 - xSemaphoreGive(busDrawMux); // init semaphores to initially allow drawing + busDrawMux = xSemaphoreCreateRecursiveMutex(); // WLEDMM prevent concurrent running of strip.show and strip.service + 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); diff --git a/wled00/wled.h b/wled00/wled.h index 03825ab0..247b55d5 100644 --- a/wled00/wled.h +++ b/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