Skip to content

Clean up setGeometry (main) #5713

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
willmmiles wants to merge 3 commits into
base: main
Choose a base branch
from
+84 −55
Open

Clean up setGeometry (main) #5713

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4e5f3f7
Clean up setGeometry
willmmiles Apr 3, 2026
73c2224
setGeometry: Fix corner cases
willmmiles Jul 4, 2026
acb9487
setGeometry: Update comment on invalid Y handling
willmmiles Jul 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
135 changes: 80 additions & 55 deletions wled00/FX_fcn.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,83 +458,108 @@ 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;
}

// 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
boundsUnchanged &= (startY == i1Y && stopY == i2Y); // 2D
if (Segment::maxHeight>1) { // 2D
if (i1Y >= Segment::maxHeight) i1Y = 0; // Unlike i1, doesn't inherit old value

@DedeHai DedeHai Jul 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you explain why one does inherit and the other does not? in what scenario does this even happen? I imagine if loading a preset with oob segment settings?

@willmmiles willmmiles Jul 5, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Historical compatibility, ie. that's what the code prior to this PR did. I agree it's a strange semantic. Like so many API quirks I imagine it was an early implementation decision that stuck around. The tweak here was because I misread the original implementation - in the original code, Y was always reset to 0 if invalid, while X keeps the old value.

I ended up with a little test script to validate that the API was consistent with main, except fixing the crash and notification bugs. Sadly there is one incompatibility I left in here as I judged it wasn't worth the effort to fix: if you have a system with a matrix + trailing strip, and you send an update to a segment that was in the trailing strip with a trailing-strip stop, but matrix start index; then the original code would disable the segment, but this code keeps the old start value.

eg. for a system with a 10x10 matrix followed by a 100 LED strip:
original segment: { "start": 120, "len": 60 } (stop = 180)
is sent a bad update: { "start": 2 } (unlike every other value, omitting "len" doesn't keep the length the same, it keeps the stop coord the same...!!!)

The old code would produce:
{ "start": 120, "len": 0 } (disabled)
while the new code will still land at:
{ "start": 120, "len": 60}

@DedeHai DedeHai Jul 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the explanation, makes sense to me. Matrix with trailing strip is a bit of a special case I assume not many people use.
could you please update the comment to // Unlike i1, doesn't inherit old value (legacy) for clarity?

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);
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
// Note we don't pass BFRALLOC_CLEAR as resetIfRequired() will initialize the buffer later
p_free(pixels);
pixels = static_cast<uint32_t*>(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();
}


Expand Down
Loading