From 4e5f3f7ae32540242c52791669e9e9d7c2e12a8c Mon Sep 17 00:00:00 2001 From: Will Miles Date: Fri, 3 Apr 2026 11:40:08 -0400 Subject: [PATCH 1/3] Clean up setGeometry Reorganize setGeometry to perform all checks first before applying the values. This lets us accurately detect changes before starting a transition. --- wled00/FX_fcn.cpp | 130 ++++++++++++++++++++++++++-------------------- 1 file changed, 75 insertions(+), 55 deletions(-) diff --git a/wled00/FX_fcn.cpp b/wled00/FX_fcn.cpp index 34fde40058..39cb4cf2ca 100644 --- a/wled00/FX_fcn.cpp +++ b/wled00/FX_fcn.cpp @@ -458,83 +458,103 @@ void Segment::handleRandomPalette() { // strip must be suspended (strip.suspend()) before calling this function // this function may call fill() to clear pixels if spacing or mapping changed (which requires setting _vWidth, _vHeight, _vLength or beginDraw()) void Segment::setGeometry(uint16_t i1, uint16_t i2, uint8_t grp, uint8_t spc, uint16_t ofs, uint16_t i1Y, uint16_t i2Y, uint8_t m12) { - // return if neither bounds nor grouping have changed - bool boundsUnchanged = (start == i1 && stop == i2); + // Sanitise inputs + if (i2 <= i1) { // For any values, this means deactivate the segment; we check i2 before i1 for this case + i2 = 0; + } else { + // Clamp i2 to maximum length + if (i2 > Segment::maxWidth*Segment::maxHeight) { + i2 = MIN(i2,strip.getLengthTotal()); + } else if (i2 > Segment::maxWidth) { + i2 = Segment::maxWidth; + } else if (i2 < 1) { + i2 = 1; + } + } + // If i1 is invalid, use old value + // Valid range is inside maxWidth, or in trailing segment range + if ((i1 >= Segment::maxWidth) && (i1 < Segment::maxWidth*Segment::maxHeight || i1 >= strip.getLengthTotal())) { + i1 = start; + } + #ifndef WLED_DISABLE_2D - boundsUnchanged &= (startY == i1Y && stopY == i2Y); // 2D + if (Segment::maxHeight>1) { // 2D + if (i1Y >= Segment::maxHeight) i1Y = startY; + if (i2Y > Segment::maxHeight) { + i2Y = Segment::maxHeight; + } else if (i2Y < 1) { + i2Y = 1; + } + } else #endif - boundsUnchanged &= (grouping == grp && spacing == spc); // changing grouping and/or spacing changes virtual segment length (painting dimensions) + { + i1Y = 0; + i2Y = 1; + } - if (stop && (spc > 0 || m12 != map1D2D)) clear(); - if (grp) { // prevent assignment of 0 - grouping = grp; - spacing = spc; - } else { - grouping = 1; - spacing = 0; + if (grp == 0) { grp = 1; spc = 0; } // prevent assignment of 0 + if (ofs == UINT16_MAX) ofs = offset; // keep current setting if passed illegal value + m12 = constrain(m12, 0, 7); + + // Final safety check after all bounds adjustments + if ((i1 >= i2) || (i1Y >= i2Y)) { + i2 = 0; // disable segment } - if (ofs < UINT16_MAX) offset = ofs; - map1D2D = constrain(m12, 0, 7); + + // Inputs are ok, check if anything has changed + bool boundsUnchanged = (start == i1 && stop == i2) + #ifndef WLED_DISABLE_2D + && ((Segment::maxHeight <= 1) || (startY == i1Y && stopY == i2Y)) + #endif + && (grouping == grp) + && (spacing == spc) + && (offset == ofs) + && (m12 == map1D2D); if (boundsUnchanged) return; + DEBUG_PRINTF_P(PSTR("Segment geometry: (%d,%d),(%d,%d) -> (%d,%d),(%d,%d) [%d,%d]\n"), start, stop, startY, stopY, (int)i1, (int)i2, (int)i1Y, (int)i2Y, (int) grp, (int)spc); + unsigned oldLength = length(); - DEBUGFX_PRINTF_P(PSTR("Segment geometry: %d,%d -> %d,%d [%d,%d]\n"), (int)i1, (int)i2, (int)i1Y, (int)i2Y, (int)grp, (int)spc); + stateChanged = true; // send UDP/WS broadcast markForReset(); stopTransition(); // we can't use transition if segment dimensions changed stateChanged = true; // send UDP/WS broadcast - // apply change immediately - if (i2 <= i1) { //disable segment - #ifdef WLED_ENABLE_GIF - endImagePlayback(this); - #endif - deallocateData(); - p_free(pixels); - pixels = nullptr; - stop = 0; - return; - } - if (i1 < Segment::maxWidth || (i1 >= Segment::maxWidth*Segment::maxHeight && i1 < strip.getLengthTotal())) start = i1; // Segment::maxWidth equals strip.getLengthTotal() for 1D - stop = i2 > Segment::maxWidth*Segment::maxHeight && i1 >= Segment::maxWidth*Segment::maxHeight ? MIN(i2,strip.getLengthTotal()) : constrain(i2, 1, Segment::maxWidth); // check for 2D trailing strip - startY = 0; - stopY = 1; - #ifndef WLED_DISABLE_2D - if (Segment::maxHeight>1) { // 2D - if (i1Y < Segment::maxHeight) startY = i1Y; - stopY = constrain(i2Y, 1, Segment::maxHeight); - } - #endif - // safety check - if (start >= stop || startY >= stopY) { - #ifdef WLED_ENABLE_GIF - endImagePlayback(this); - #endif - deallocateData(); - p_free(pixels); - pixels = nullptr; - stop = 0; - return; - } - // allocate FX render buffer - if (length() != oldLength) { + // apply change + start = i1; + stop = i2; + startY = i1Y; + stopY = i2Y; + grouping = grp; + spacing = spc; + offset = ofs; + map1D2D = m12; + + // Cleanup check + auto newLength = length(); + if ((newLength > 0) && (newLength != oldLength)) { // allocate render buffer (always entire segment), prefer IRAM/PSRAM. Note: impact on FPS with PSRAM buffer is low (<2% with QSPI PSRAM) on S2/S3 p_free(pixels); pixels = static_cast(allocate_buffer(length() * sizeof(uint32_t), BFRALLOC_PREFER_PSRAM | BFRALLOC_NOBYTEACCESS)); if (!pixels) { DEBUGFX_PRINTLN(F("!!! Not enough RAM for pixel buffer !!!")); - #ifdef WLED_ENABLE_GIF - endImagePlayback(this); - #endif - deallocateData(); errorFlag = ERR_NORAM_PX; - stop = 0; - return; + stop = 0; // will fall through into disable check below } + } + if (length() == 0) { + #ifdef WLED_ENABLE_GIF + endImagePlayback(this); + #endif + deallocateData(); + p_free(pixels); + pixels = nullptr; + } else { + refreshLightCapabilities(); } - refreshLightCapabilities(); } From 73c22246bb07b35e7a6c011fe7d0e1830f5d628a Mon Sep 17 00:00:00 2001 From: Will Miles Date: Sat, 4 Jul 2026 23:25:46 +0000 Subject: [PATCH 2/3] setGeometry: Fix corner cases --- wled00/FX_fcn.cpp | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/wled00/FX_fcn.cpp b/wled00/FX_fcn.cpp index 39cb4cf2ca..033e0d52a1 100644 --- a/wled00/FX_fcn.cpp +++ b/wled00/FX_fcn.cpp @@ -458,28 +458,33 @@ void Segment::handleRandomPalette() { // strip must be suspended (strip.suspend()) before calling this function // this function may call fill() to clear pixels if spacing or mapping changed (which requires setting _vWidth, _vHeight, _vLength or beginDraw()) void Segment::setGeometry(uint16_t i1, uint16_t i2, uint8_t grp, uint8_t spc, uint16_t ofs, uint16_t i1Y, uint16_t i2Y, uint8_t m12) { - // Sanitise inputs + // Sanitise inputs if (i2 <= i1) { // For any values, this means deactivate the segment; we check i2 before i1 for this case i2 = 0; - } else { - // Clamp i2 to maximum length - if (i2 > Segment::maxWidth*Segment::maxHeight) { - i2 = MIN(i2,strip.getLengthTotal()); - } else if (i2 > Segment::maxWidth) { - i2 = Segment::maxWidth; - } else if (i2 < 1) { - i2 = 1; - } - } + } + // If i1 is invalid, use old value // Valid range is inside maxWidth, or in trailing segment range if ((i1 >= Segment::maxWidth) && (i1 < Segment::maxWidth*Segment::maxHeight || i1 >= strip.getLengthTotal())) { i1 = start; } + // Check i2 validity + if (i2 > 0) { + // Clamp i2 to maximum length + if ((i1 >= Segment::maxWidth*Segment::maxHeight) && (i2 >= Segment::maxWidth*Segment::maxHeight)) { + // Trailing strip after 2D + i2 = MIN(i2,strip.getLengthTotal()); + i1Y = 0; // 2D Y values are not used for trailing strip + i2Y = 1; + } else if (i2 > Segment::maxWidth) { + i2 = Segment::maxWidth; + } + } + #ifndef WLED_DISABLE_2D if (Segment::maxHeight>1) { // 2D - if (i1Y >= Segment::maxHeight) i1Y = startY; + if (i1Y >= Segment::maxHeight) i1Y = 0; // Unlike i1, doesn't inherit old value if (i2Y > Segment::maxHeight) { i2Y = Segment::maxHeight; } else if (i2Y < 1) { @@ -517,7 +522,6 @@ void Segment::setGeometry(uint16_t i1, uint16_t i2, uint8_t grp, uint8_t spc, ui unsigned oldLength = length(); - stateChanged = true; // send UDP/WS broadcast markForReset(); stopTransition(); // we can't use transition if segment dimensions changed stateChanged = true; // send UDP/WS broadcast @@ -536,6 +540,7 @@ void Segment::setGeometry(uint16_t i1, uint16_t i2, uint8_t grp, uint8_t spc, ui auto newLength = length(); if ((newLength > 0) && (newLength != oldLength)) { // allocate render buffer (always entire segment), prefer IRAM/PSRAM. Note: impact on FPS with PSRAM buffer is low (<2% with QSPI PSRAM) on S2/S3 + // Note we don't pass BFRALLOC_CLEAR as resetIfRequired() will initialize the buffer later p_free(pixels); pixels = static_cast(allocate_buffer(length() * sizeof(uint32_t), BFRALLOC_PREFER_PSRAM | BFRALLOC_NOBYTEACCESS)); if (!pixels) { From acb9487fdbde3d1c74ad8ad38f796c8ab7e3d5cb Mon Sep 17 00:00:00 2001 From: Will Miles Date: Mon, 6 Jul 2026 23:07:18 +0000 Subject: [PATCH 3/3] setGeometry: Update comment on invalid Y handling --- wled00/FX_fcn.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/wled00/FX_fcn.cpp b/wled00/FX_fcn.cpp index 033e0d52a1..5b923b1264 100644 --- a/wled00/FX_fcn.cpp +++ b/wled00/FX_fcn.cpp @@ -484,7 +484,11 @@ void Segment::setGeometry(uint16_t i1, uint16_t i2, uint8_t grp, uint8_t spc, ui #ifndef WLED_DISABLE_2D if (Segment::maxHeight>1) { // 2D - if (i1Y >= Segment::maxHeight) i1Y = 0; // Unlike i1, doesn't inherit old value + if (i1Y >= Segment::maxHeight) { + // Unlike i1 (X), Y values don't inherit old values if invalid + // This behaviour preserved for backwards compatibility + i1Y = 0; + } if (i2Y > Segment::maxHeight) { i2Y = Segment::maxHeight; } else if (i2Y < 1) {