Conversation
📝 WalkthroughWalkthroughAdds per-segment masks: mask state and lifecycle in Segment, filesystem loading/parsing of segmaskN.json, UI controls and JSON (de)serialization, render-time gating to skip masked pixels, and enumeration/init hooks to register available masks. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Frontend UI
participant App as WLED App
participant Seg as Segment
participant FS as Filesystem
participant Render as Renderer
UI->>App: requestJson setMask(seg, id)
App->>Seg: deserializeSegment() with "mask"
Seg->>Seg: setMask(maskId)
Seg->>FS: open segmask{maskId}.json
FS-->>Seg: return JSON (w,h,inv,bitarray)
Seg->>Seg: parse, alloc _mask, set _maskValid
Seg-->>App: updated segment state
App-->>UI: update UI
Note over Render,Seg: During render loop
Render->>Seg: setPixelColorXY(x,y,color)
Seg->>Seg: maskAllowsXY(x,y)?
alt allowed
Seg->>Seg: apply color
else masked
Seg-->>Seg: skip pixel
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/FX_fcn.cpp (1)
94-103: Mask reload path missing after copy/assignment.Copy ctor/assignment null out
_mask(and_maskValid) but preservemaskId/maskInvertvia memcpy. When rendering,maskAllows()checksif (!_mask || !_maskValid) return true;, so copied segments silently skip mask application even thoughmaskIdremains set. There is no automatic reload path—the effect must manually callsetMask(maskId)after copying, ormaskIdshould be cleared.
🤖 Fix all issues with AI agents
In `@wled00/FX_fcn.cpp`:
- Around line 253-260: _maskValid is only set once when loading the mask and can
become stale if virtual geometry changes; update the validity check by
recomputing it whenever geometry may change (for example at start of setUp() and
startFrame() or immediately before any mask application routine). Locate where
_maskValid is used (e.g., in FX_fcn::_maskValid checks and any applyMask/paint
routines) and replace the single-time assignment with a runtime check that
compares _maskW/_maskH to calc_virtualWidth()/calc_virtualHeight() (or reassign
_maskValid at the start of setUp()/startFrame()) so the mask validity reflects
current virtual geometry. Ensure no behavioral change besides recomputing
validity.
- Around line 150-176: The clearMask()/setMask() flow can free or replace _mask
while maskAllows() may be reading it on the other core; to fix, serialize the
pointer swap/free with the renderer by acquiring the same mutex used by the
render loop (e.g., segmentMux) around the critical section in Segment::clearMask
and Segment::setMask (before freeing or assigning _mask and updating
_maskLen/_maskW/_maskH/_maskValid/maskId) or alternatively wait for the strip to
be idle using the existing idle/wait API before changing the buffer; locate and
protect the critical sections in Segment::clearMask, Segment::setMask and
anywhere maskAllows reads _mask to prevent use-after-free.
🧹 Nitpick comments (2)
wled00/json.cpp (1)
364-372: Prefer an explicit clear path whenmaskis 0.
This makes intent obvious and avoids any unintended file load ifsetMask(0)isn’t a no-op.♻️ Proposed tweak
if (elem.containsKey("mask")) { // WLEDMM segment mask id int maskVal = elem["mask"] | 0; if (maskVal < 0) maskVal = 0; uint8_t maskId = constrain(maskVal, 0, WLED_MAX_SEGMASKS-1); - if (maskId != seg.maskId || (maskId != 0 && !seg.hasMask())) seg.setMask(maskId); + if (maskId == 0) { + if (seg.hasMask()) seg.clearMask(); + } else if (maskId != seg.maskId || !seg.hasMask()) { + seg.setMask(maskId); + } } if (elem.containsKey("minv")) { // WLEDMM segment mask invert seg.maskInvert = elem["minv"] | seg.maskInvert; }wled00/FX.h (1)
725-741: Consider adding[[gnu::hot]]attribute for hot-path optimization.The mask accessor logic is correct:
- Fail-open design (returns
truewhen mask is absent/invalid) prevents black pixels on error- Bounds checking prevents buffer overread
- Bit-packing/unpacking is efficient and correct (LSB-first)
Since
maskAllows()andmaskAllowsXY()are called for every pixel during rendering, consider adding the[[gnu::hot]]attribute (likeprogress()at line 692 andcurrentBri()at line 700) to hint the compiler for optimization.♻️ Suggested optimization
- inline bool hasMask(void) const { return _mask != nullptr; } // WLEDMM - inline bool maskAllows(uint16_t i) const { // WLEDMM + [[gnu::hot]] inline bool hasMask(void) const { return _mask != nullptr; } // WLEDMM + [[gnu::hot]] inline bool maskAllows(uint16_t i) const { // WLEDMM if (!_mask || !_maskValid) return true; if (size_t(i) >= _maskLen) return false; // WLEDMM: bit-packed mask (LSB-first): byte = i>>3, bit = i&7 bool bit = (_mask[i >> 3] >> (i & 7)) & 0x01; return maskInvert ? !bit : bit; } - inline bool maskAllowsXY(int x, int y) const { // WLEDMM + [[gnu::hot]] inline bool maskAllowsXY(int x, int y) const { // WLEDMM if (!_mask || !_maskValid) return true; if (x < 0 || y < 0) return false; size_t idx = size_t(x) + (size_t(y) * _maskW); if (idx >= _maskLen) return false; // WLEDMM: row-major (x + y*w), bit-packed mask (LSB-first in each byte) bool bit = (_mask[idx >> 3] >> (idx & 7)) & 0x01; return maskInvert ? !bit : bit; }
…with lambda; Remove gnu::hot flags on inline function definitions
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/FX_fcn.cpp (1)
94-103: Reset mask ID and invert flag when clearing mask buffer on segment copy.After
memcpy, both the copy constructor and copy assignment operator null_maskand reset mask buffer state, but leavemaskIdandmaskInvertfrom the source. This causes the copied segment to report a mask ID that has no actual buffer applied. Reset these to matchclearMask()behavior:Fix for both locations
_maskW = 0; // WLEDMM _maskH = 0; // WLEDMM _maskValid = false; // WLEDMM + maskId = 0; // WLEDMM + maskInvert = false; // WLEDMM
…in copy constructor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@wled00/FX_fcn.cpp`:
- Around line 171-176: Segment::setMask currently calls clearMask() but proceeds
even if clearMask failed to release the existing mask, which can leak the old
buffer; after calling clearMask() in Segment::setMask, check the _mask member
and abort (return false) if it is still non-null to avoid overwriting/leaking
the previous buffer—use the existing Segment::clearMask() call, then immediately
guard with a check on _mask before continuing (also preserve the existing id==0
behavior).
| bool Segment::setMask(uint8_t id) { // WLEDMM | ||
| clearMask(); | ||
| if (id >= WLED_MAX_SEGMASKS) { | ||
| return false; | ||
| } | ||
| if (id == 0) return true; |
There was a problem hiding this comment.
Abort mask load if clearMask() couldn't clear.
If clearMask() fails to acquire segmentMux, _mask can remain non-null; setMask() can then overwrite it, leaking the old buffer. Add a guard to bail out when _mask is still set.
🛠️ Suggested guard
bool Segment::setMask(uint8_t id) { // WLEDMM
clearMask();
+ if (_mask) {
+ DEBUG_PRINTLN(F("Segment::setMask: clearMask failed, aborting."));
+ return false;
+ }
if (id >= WLED_MAX_SEGMASKS) {
return false;
}🤖 Prompt for AI Agents
In `@wled00/FX_fcn.cpp` around lines 171 - 176, Segment::setMask currently calls
clearMask() but proceeds even if clearMask failed to release the existing mask,
which can leak the old buffer; after calling clearMask() in Segment::setMask,
check the _mask member and abort (return false) if it is still non-null to avoid
overwriting/leaking the previous buffer—use the existing Segment::clearMask()
call, then immediately guard with a check on _mask before continuing (also
preserve the existing id==0 behavior).
|
@sayhiben I really like the overall idea 😃 its a nice way to handle overlaying properly, and it optimizes pixel drawing by skipping masked out pixels at the same time 👍 |
|
@softhack007 Thanks for the thorough review! I'll work through comments today |
…when mask is not used
…py/move constructors
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@wled00/FX_fcn.cpp`:
- Around line 330-334: Direct free(_mask) is unsafe because maskAllows() reads
_mask without mutex protection; replace this with a protected pointer swap (or
call clearMask()) so readers never see a freed pointer. Either call clearMask()
from here, or replicate its sequence: esp32SemTake(&segmentMux) to lock, oldMask
= _mask; _mask = nullptr; esp32SemGive(&segmentMux) to unlock, then call
strip_wait_until_idle("Segment::operator= mask cleanup") if still desired and
free(oldMask). Also remove or update the misleading comment about deadlocks and
avoid freeing while still holding the mutex.
♻️ Duplicate comments (1)
wled00/FX_fcn.cpp (1)
171-176: Add guard to prevent memory leak ifclearMask()fails.If
clearMask()fails to acquiresegmentMux,_maskremains non-null. Proceeding with the load would overwrite the pointer and leak the old buffer. This was flagged in a previous review but doesn't appear to be addressed.🛠️ Suggested fix
bool Segment::setMask(uint8_t id) { // WLEDMM clearMask(); + if (_mask) { + DEBUG_PRINTLN(F("Segment::setMask: clearMask failed, aborting.")); + return false; + } if (id >= WLED_MAX_SEGMASKS) { return false; } if (id == 0) return true;
🧹 Nitpick comments (1)
wled00/FX_fcn.cpp (1)
1188-1189: Consider moving mask gate to the 1D-only path for efficiency.The mask check at line 1188 runs for all segments including 2D, but 2D segments already have mask gating in
setPixelColorXY_*functions. While the check is cheap (bit lookup with early_maskValidcheck), placing it in the 1D-only else branch (around line 1361+) would avoid redundant checks for 2D paths.A previous reviewer suggested: "Instead of doing
if !is2D() && !maskAllows(..)here, it's better to put it into theelsebranch for line 1005."♻️ Alternative placement
Move the mask gate to the 1D path only (after line 1371):
} `#endif` + + if (_maskValid && !maskAllows(i)) return; // WLEDMM mask gate for 1D segments if (ledsrgb) ledsrgb[i] = col;And remove from line 1188-1189.
| if (_mask) { // WLEDMM free mask buffer directly to avoid deadlocks | ||
| strip_wait_until_idle("Segment::operator= mask cleanup"); // WLEDMM avoid freeing while renderer is active | ||
| free(_mask); | ||
| _mask = nullptr; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Examine clearMask, maskAllows, and segmentMux usage
rg -n "clearMask\(\)|maskAllows|segmentMux" wled00/FX_fcn.cpp wled00/FX.h --context=3 | head -100Repository: MoonModules/WLED-MM
Length of output: 5748
🏁 Script executed:
#!/bin/bash
# Find the clearMask function definition
rg -n "void clearMask|void.*clearMask" wled00/FX_fcn.cpp wled00/FX.h -A 10Repository: MoonModules/WLED-MM
Length of output: 1448
🏁 Script executed:
#!/bin/bash
# Find maskAllows function definition
rg -n "maskAllows" wled00/FX_fcn.cpp wled00/FX.h -B 2 -A 8Repository: MoonModules/WLED-MM
Length of output: 1891
Use clearMask() or acquire segmentMux to protect pointer swap before freeing.
The direct free(_mask) is unsafe because maskAllows() (called from rendering tasks on other cores) reads _mask without mutex protection. While strip_wait_until_idle() ensures the renderer is idle now, it doesn't prevent the next render cycle from starting immediately after.
Compare with clearMask() (lines 152–169), which correctly:
- Acquires
segmentMuxwithesp32SemTake() - Swaps the pointer atomically (
oldMask = _mask; _mask = nullptr) - Releases the mutex
- Frees the old buffer after releasing the lock
Either call clearMask() directly or follow this same pattern to ensure the pointer swap is serialized with reader tasks. The comment about deadlocks is misleading—heap operations are thread-safe; the issue is protecting the pointer change itself.
🤖 Prompt for AI Agents
In `@wled00/FX_fcn.cpp` around lines 330 - 334, Direct free(_mask) is unsafe
because maskAllows() reads _mask without mutex protection; replace this with a
protected pointer swap (or call clearMask()) so readers never see a freed
pointer. Either call clearMask() from here, or replicate its sequence:
esp32SemTake(&segmentMux) to lock, oldMask = _mask; _mask = nullptr;
esp32SemGive(&segmentMux) to unlock, then call
strip_wait_until_idle("Segment::operator= mask cleanup") if still desired and
free(oldMask). Also remove or update the misleading comment about deadlocks and
avoid freeing while still holding the mutex.
There was a problem hiding this comment.
@softhack007 I believe I've hit all of your other suggestions at this point, but this quirk is tripping me up a little. Do you have any recommendations?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
@sayhiben i'll use your PR as a testcase for our new AI review rules ;-) it also means your PR is still on my list 😊 |
|
@coderabbitai please re-review this PR. Use the new review instructions and coding guidelines (.coderabbit.yaml) that were just added in the main mdev branch. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainThe PR description images show:
Let me fetch the current CodeRabbit configuration and the PR code changes to perform a thorough re-review. 🏁 Script executed: Length of output: 3804 🛠 Debug Information Gathered Good, the 🏁 Script executed: Length of output: 30727 Good. Now let me fetch the actual PR diff and key changed files. 🏁 Script executed: Length of output: 25336
Overall AssessmentThe approach is solid — bit-packed masks, ledmap-style streaming parse, 🔴 High — Undefined Behaviour in
|
|
@sayhiben wow that review result is better than I thought 😀 . BTW, you can always ask @coderabbitai for help or explanations on its comments, you can even discuss. The AI we use has become quite "communicative"👍 |
|
Tip For best results, initiate chat on the files or code changes. The PR description images show:
To summarize the priority order for
Feel free to ask me to clarify any of the above, show a more complete diff for a fix, or explain the reasoning — happy to dig into any of them! 🙂 🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes. The PR description images show: (1) a pixel-art LED matrix mounted in a laser-cut wooden frame displaying a colourful character; (2) the WLED Segments UI with the new "Mask 🌜" dropdown (set to
Feel free to ask me to explain any of them in more detail! ✏️ Learnings added
🧠 Learnings used |
2 participants
Overview
I'm designing a fancy sign for a craft fair booth with an engraved sheet of acrylic in front. In order to properly light it, I would like to define segments for various shapes in the engraved sheet such that I can apply different effects to each area. Currently, support for this is limited to ledmaps (only one across all segments), jMaps (1D -> 2D translation only), and grouping (only works for specific designs)
This PR adds support for clipping masks per segment, defined in virtual LED space. This enables overlapping segments to render distinct effects/palettes while preserving normal segment geometry and mappings.
Summary of changes
How it works
Segmask.json
Masks are defined by 1/0 values in a list similar to ledmapN.json files:
{"w":16,"h":32,"mask":[0,0,0,1,1,1,0,0,0,0,...,0,1,1],"inv":false}Other Notes
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
✏️ Tip: You can customize this high-level summary in your review settings.