From c64f74a611b34925630b2d8c1845858487c3d9ab Mon Sep 17 00:00:00 2001 From: Frank <91616163+softhack007@users.noreply.github.com> Date: Sat, 8 Jul 2023 22:40:43 +0200 Subject: [PATCH] fix for random crashes on changing presets (UI) Segments are created/deleted on-the-fly; it seems that "local leds" buffers did not follow properly. * revise segment copy/move constructors * de-optimize use of local leds (as they need to be re-allocated when segment size changes) * minor stability improvements --- wled00/FX.h | 6 ++-- wled00/FX_fcn.cpp | 72 +++++++++++++++++++++++++++++------------------ wled00/json.cpp | 6 ++++ wled00/ntp.cpp | 2 +- 4 files changed, 54 insertions(+), 32 deletions(-) diff --git a/wled00/FX.h b/wled00/FX.h index 9fcf3bc1..4354b3f9 100644 --- a/wled00/FX.h +++ b/wled00/FX.h @@ -389,7 +389,7 @@ typedef struct Segment { }; uint8_t startY; // start Y coodrinate 2D (top); there should be no more than 255 rows uint8_t stopY; // stop Y coordinate 2D (bottom); there should be no more than 255 rows - char *name; + char *name = nullptr; // WLEDMM initialize to nullptr // runtime data unsigned long next_time; // millis() of next update @@ -397,8 +397,8 @@ typedef struct Segment { uint32_t call; // call counter uint16_t aux0; // custom var uint16_t aux1; // custom var - byte* data; // effect data pointer - CRGB* ledsrgb; // local leds[] array (may be a pointer to global) //WLEDMM rename to ledsrgb to search on them (temp?) + byte* data = nullptr; // effect data pointer // WLEDMM initialize to nullptr + CRGB* ledsrgb = nullptr; // local leds[] array (may be a pointer to global) //WLEDMM rename to ledsrgb to search on them (temp?), and initialilize to nullptr size_t ledsrgbSize; //WLEDMM static CRGB *_globalLeds; // global leds[] array static uint16_t maxWidth, maxHeight; // these define matrix width & height (max. segment dimensions) diff --git a/wled00/FX_fcn.cpp b/wled00/FX_fcn.cpp index 391e5b81..5ecf66d2 100644 --- a/wled00/FX_fcn.cpp +++ b/wled00/FX_fcn.cpp @@ -92,7 +92,7 @@ CRGB *Segment::_globalLeds = nullptr; uint16_t Segment::maxWidth = DEFAULT_LED_COUNT; uint16_t Segment::maxHeight = 1; -// copy constructor +// copy constructor - creates a new segment by copy from orig, but does not copy buffers. Does not modify orig! Segment::Segment(const Segment &orig) { USER_PRINTLN(F("-- Copy segment constructor --")); memcpy((void*)this, (void*)&orig, sizeof(Segment)); //WLEDMM copy to this @@ -100,11 +100,12 @@ Segment::Segment(const Segment &orig) { data = nullptr; _dataLen = 0; _t = nullptr; - if (ledsrgb && !Segment::_globalLeds) ledsrgb = nullptr; //WLEDMM constructor so there was nothing. ledsrgb not freed as still used by orig! + if (ledsrgb && !Segment::_globalLeds) {ledsrgb = nullptr; ledsrgbSize = 0;} // WLEDMM if (orig.name) { name = new char[strlen(orig.name)+1]; if (name) strcpy(name, orig.name); } if (orig.data) { if (allocateData(orig._dataLen)) memcpy(data, orig.data, orig._dataLen); } if (orig._t) { _t = new Transition(orig._t->_dur, orig._t->_briT, orig._t->_cctT, orig._t->_colorT); } - if (orig.ledsrgb && !Segment::_globalLeds) { allocLeds(); if (ledsrgb) memcpy(ledsrgb, orig.ledsrgb, sizeof(CRGB)*length()); } + else markForReset(); // WLEDMM + // if (orig.ledsrgb && !Segment::_globalLeds) { allocLeds(); if (ledsrgb) memcpy(ledsrgb, orig.ledsrgb, sizeof(CRGB)*length()); } // WLEDMM jMap = nullptr; //WLEDMM jMap } @@ -120,16 +121,18 @@ void Segment::allocLeds() { } // softhack007 clean up buffer } if ((size > 0) && (!ledsrgb || size > ledsrgbSize)) { //softhack dont allocate zero bytes - USER_PRINTF("allocLeds %u from %u\n", size, ledsrgb?ledsrgbSize:0); - ledsrgb = (CRGB*)malloc(size); + USER_PRINTF("allocLeds (%d,%d to %d,%d), %u from %u\n", start, startY, stop, stopY, size, ledsrgb?ledsrgbSize:0); + if (ledsrgb) free(ledsrgb); // we need a bigger buffer, so free the old one first + ledsrgb = (CRGB*)calloc(size, 1); ledsrgbSize = ledsrgb?size:0; + if (ledsrgb == nullptr) USER_PRINTLN("allocLeds failed!!"); } else { USER_PRINTF("reuse Leds %u from %u\n", size, ledsrgb?ledsrgbSize:0); } } -// move constructor +// move constructor --> moves everything (including buffer) from orig to this Segment::Segment(Segment &&orig) noexcept { USER_PRINTLN(F("-- Move segment constructor --")); memcpy((void*)this, (void*)&orig, sizeof(Segment)); @@ -137,21 +140,21 @@ Segment::Segment(Segment &&orig) noexcept { orig.data = nullptr; orig._dataLen = 0; orig._t = nullptr; - orig.ledsrgb = nullptr; //WLEDMM: do not free as moved to here (constructor so there where no leds) - orig.jMap = nullptr; //WLEDMM jMap + orig.ledsrgb = nullptr; //WLEDMM + orig.ledsrgbSize = 0; // WLEDMM + orig.jMap = nullptr; //WLEDMM jMap } -// copy assignment +// copy assignment --> overwrite segment withg orig - deletes old buffers in "this", but does not change orig! Segment& Segment::operator= (const Segment &orig) { - USER_PRINTLN(F("-- Copying segment --")); + USER_PRINTLN(F("-- Copy-assignment segment --")); if (this != &orig) { // clean destination if (name) delete[] name; if (_t) delete _t; - // WLEDMM reuse leds instead of removing themn - // if (ledsrgb && !Segment::_globalLeds) free(ledsrgb); //WLEDMM: nullify below! CRGB* oldLeds = ledsrgb; size_t oldLedsSize = ledsrgbSize; + if (ledsrgb && !Segment::_globalLeds) free(ledsrgb); deallocateData(); // copy source memcpy((void*)this, (void*)&orig, sizeof(Segment)); @@ -160,20 +163,22 @@ Segment& Segment::operator= (const Segment &orig) { data = nullptr; _dataLen = 0; _t = nullptr; - if (!Segment::_globalLeds) {ledsrgb = oldLeds; ledsrgbSize = oldLedsSize;};// WLEDMM reuse leds instead of ledsrgb = nullptr; + //if (!Segment::_globalLeds) {ledsrgb = oldLeds; ledsrgbSize = oldLedsSize;}; // WLEDMM reuse leds instead of ledsrgb = nullptr; + if (!Segment::_globalLeds) {ledsrgb = nullptr; ledsrgbSize = 0;}; // WLEDMM copy has no buffers (yet) // copy source data if (orig.name) { name = new char[strlen(orig.name)+1]; if (name) strcpy(name, orig.name); } if (orig.data) { if (allocateData(orig._dataLen)) memcpy(data, orig.data, orig._dataLen); } if (orig._t) { _t = new Transition(orig._t->_dur, orig._t->_briT, orig._t->_cctT, orig._t->_colorT); } - if (orig.ledsrgb && !Segment::_globalLeds) { allocLeds(); if (ledsrgb) memcpy(ledsrgb, orig.ledsrgb, sizeof(CRGB)*length()); } + else markForReset(); // WLEDMM + //if (orig.ledsrgb && !Segment::_globalLeds) { allocLeds(); if (ledsrgb) memcpy(ledsrgb, orig.ledsrgb, sizeof(CRGB)*length()); } // WLEDMM don't copy old buffer jMap = nullptr; //WLEDMM jMap } return *this; } -// move assignment +// move assignment --> moves everything (including buffers) from "orig" to "this" Segment& Segment::operator= (Segment &&orig) noexcept { - USER_PRINTLN(F("-- Moving segment --")); + USER_PRINTLN(F("-- Move-assignment segment --")); if (this != &orig) { if (name) delete[] name; // free old name deallocateData(); // free old runtime data @@ -185,13 +190,18 @@ Segment& Segment::operator= (Segment &&orig) noexcept { orig._dataLen = 0; orig._t = nullptr; orig.ledsrgb = nullptr; //WLEDMM: do not free as moved to here + orig.ledsrgbSize = 0; //WLEDMM orig.jMap = nullptr; //WLEDMM jMap } return *this; } bool Segment::allocateData(size_t len) { - if (data && _dataLen == len) return true; //already allocated + if (data && _dataLen >= len) { + if (call == 0) memset(data, 0, len); // WLEDMM: clear data when SEGENV.call==0 + return true; //already allocated + } + //DEBUG_PRINTF("allocateData(%u) start %d, stop %d, vlen %d\n", len, start, stop, virtualLength()); deallocateData(); if (Segment::getUsedSegmentData() + len > MAX_SEGMENT_DATA) return false; //not enough memory // do not use SPI RAM on ESP32 since it is slow @@ -201,7 +211,7 @@ bool Segment::allocateData(size_t len) { //else //#endif data = (byte*) malloc(len); - if (!data) return false; //allocation failed + if (!data) { _dataLen = 0; return false;} //allocation failed // WLEDMM reset dataLen Segment::addUsedSegmentData(len); _dataLen = len; memset(data, 0, len); @@ -209,9 +219,10 @@ bool Segment::allocateData(size_t len) { } void Segment::deallocateData() { - if (!data) return; + if (!data) {_dataLen = 0; return;} // WLEDMM reset dataLen free(data); data = nullptr; + //DEBUG_PRINTLN("deallocateData() called free()."); Segment::addUsedSegmentData(-_dataLen); _dataLen = 0; } @@ -225,8 +236,7 @@ void Segment::deallocateData() { */ void Segment::resetIfRequired() { if (reset) { - //WLEDMM no need to free leds as we will reuse them - // if (ledsrgb && !Segment::_globalLeds) { free(ledsrgb); ledsrgb = nullptr; } + if (ledsrgb && !Segment::_globalLeds) { free(ledsrgb); ledsrgb = nullptr; ledsrgbSize=0;} // WLEDMM segment has changed, so we need a fresh buffer. if (transitional && _t) { transitional = false; delete _t; _t = nullptr; } deallocateData(); next_time = 0; step = 0; call = 0; aux0 = 0; aux1 = 0; @@ -236,19 +246,22 @@ void Segment::resetIfRequired() { void Segment::setUpLeds() { // deallocation happens in resetIfRequired() as it is called when segment changes or in destructor - if (Segment::_globalLeds) + if (Segment::_globalLeds) { #ifndef WLED_DISABLE_2D ledsrgb = &Segment::_globalLeds[start + startY*Segment::maxWidth]; + ledsrgbSize = length() * sizeof(CRGB); // also set this when using global leds. + //USER_PRINTF("\nsetUpLeds() Global LEDs: startX=%d stopx=%d startY=%d stopy=%d maxwidth=%d; length=%d, size=%d\n\n", start, stop, startY, stopY, Segment::maxWidth, length(), ledsrgbSize/3); #else leds = &Segment::_globalLeds[start]; #endif - else if ((ledsrgb == nullptr) && (length() > 0)) { //softhack007 quickfix - avoid malloc(0) which is undefined behaviour (should not happen, but i've seen it) + } else if (length() > 0) { //WLEDMM we always want a new buffer //softhack007 quickfix - avoid malloc(0) which is undefined behaviour (should not happen, but i've seen it) //#if defined(ARDUINO_ARCH_ESP32) && defined(BOARD_HAS_PSRAM) && defined(WLED_USE_PSRAM) //if (psramFound()) // ledsrgb = (CRGB*)ps_malloc(sizeof(CRGB)*length()); // softhack007 disabled; putting leds into psram leads to horrible slowdown on WROVER boards //else //#endif allocLeds(); //WLEDMM + //USER_PRINTF("\nsetUpLeds() local LEDs: startX=%d stopx=%d startY=%d stopy=%d maxwidth=%d; length=%d, size=%d\n\n", start, stop, startY, stopY, Segment::maxWidth, length(), ledsrgbSize/3); } } @@ -456,7 +469,7 @@ void Segment::setUp(uint16_t i1, uint16_t i2, uint8_t grp, uint8_t spc, uint16_t && (!grp || (grouping == grp && spacing == spc)) && (ofs == UINT16_MAX || ofs == offset)) return; - if (stop) fill(BLACK); //turn old segment range off + if (stop>start) fill(BLACK); //turn old segment range off // WLEDMM stop > start if (i2 <= i1) { //disable segment stop = 0; markForReset(); @@ -1049,8 +1062,9 @@ uint32_t Segment::getPixelColor(int i) i *= groupLength(); i += start; /* offset/phase */ - i += offset; + if (offset < INT16_MAX) i += offset; // WLEDMM if ((i >= stop) && (stop>0)) i -= length(); // WLEDMM avoid negative index (stop = 0 is a possible value) + if (i<0) i=0; // WLEDMM just to be 100% sure return strip.getPixelColor(i); } @@ -1540,7 +1554,7 @@ void WS2812FX::finalizeInit(void) //else //#endif if (arrSize > 0) Segment::_globalLeds = (CRGB*) malloc(arrSize); // WLEDMM avoid malloc(0) - if (Segment::_globalLeds != nullptr) memset(Segment::_globalLeds, 0, arrSize); // WLEDMM avoid dereferencing nullptr + if ((Segment::_globalLeds != nullptr) && (arrSize > 0)) memset(Segment::_globalLeds, 0, arrSize); // WLEDMM avoid dereferencing nullptr } //segments are created in makeAutoSegments(); @@ -1563,7 +1577,7 @@ void WS2812FX::waitUntilIdle(void) { delay(2); // Suspending for 1 tick (or more) gives other tasks a chance to run. //yield(); // seems to be a no-op on esp32 } while (isServicing() && (millis() - waitStarted < MAX_IDLE_WAIT_MS)); - USER_PRINTF("strip.waitUntilIdle(): strip %sidle after %d ms. (task %s)\n", isServicing()?"not ":"", int(millis() - waitStarted), pcTaskGetTaskName(NULL)); + USER_PRINTF("strip.waitUntilIdle(): strip %sidle after %d ms. (task %s with prio=%d)\n", isServicing()?"not ":"", int(millis() - waitStarted), pcTaskGetTaskName(NULL), uxTaskPriorityGet(NULL)); } return; #else @@ -1945,7 +1959,9 @@ void WS2812FX::purgeSegments(bool force) { } Segment& WS2812FX::getSegment(uint8_t id) { - return _segments[id >= _segments.size() ? getMainSegmentId() : id]; // vectors + uint8_t mainSegID = getMainSegmentId(); + if (mainSegID >= _segments.size()) mainSegID=0; // WLEDMM fallback in case that getMainSegmentId() is invalid + return _segments[id >= _segments.size() ? mainSegID : id]; // vectors } void WS2812FX::setSegment(uint8_t n, uint16_t i1, uint16_t i2, uint8_t grouping, uint8_t spacing, uint16_t offset, uint16_t startY, uint16_t stopY) { diff --git a/wled00/json.cpp b/wled00/json.cpp index e91b2f3a..168c1853 100644 --- a/wled00/json.cpp +++ b/wled00/json.cpp @@ -48,6 +48,7 @@ static bool inDeepCall = false; // WLEDMM needed so that recursive deserializeSegment() does not remove locks too early +// WLEDMM caution - this function may run outside of arduino loop context (async_tcp with priority=3) bool deserializeSegment(JsonObject elem, byte it, byte presetId) { const bool iAmGroot = !inDeepCall; // WLEDMM will only be true if this is the toplevel of the recursion. @@ -368,6 +369,7 @@ bool deserializeSegment(JsonObject elem, byte it, byte presetId) // deserializes WLED state (fileDoc points to doc object if called from web server) // presetId is non-0 if called from handlePreset() +// WLEDMM caution - this function may run outside of arduino loop context (async_tcp with priority=3) bool deserializeState(JsonObject root, byte callMode, byte presetId) { const bool iAmGroot = !inDeepCall; // WLEDMM will only be true if this is the toplevel of the recursion. @@ -419,6 +421,10 @@ bool deserializeState(JsonObject root, byte callMode, byte presetId) } } +#ifdef ARDUINO_ARCH_ESP32 + delay(2); // WLEDMM experimental - de-serialize takes time, so allow other tasks to run +#endif + // WLEDMM: before changing strip, make sure our strip is _not_ servicing effects in parallel suspendStripService = true; // temporarily lock out strip updates if (strip.isServicing()) { diff --git a/wled00/ntp.cpp b/wled00/ntp.cpp index c5393cde..c7b82bb8 100644 --- a/wled00/ntp.cpp +++ b/wled00/ntp.cpp @@ -228,7 +228,7 @@ bool checkNTPResponse() { ntpUdp.flush(); int cb = ntpUdp.parsePacket(); - if (!cb) return false; + if (!cb) {ntpUdp.flush(); return false;} // WLEDMM flush buffer uint32_t ntpPacketReceivedTime = millis(); DEBUG_PRINT(F("NTP recv, l="));