new mapping mode "Waterfall" #197
Conversation
up to 30% faster, and prevents interference with other segments.
|
note to myself: don't run waterfall when receiving realtime pixels (DDP, Art-Net, etc)
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughIntroduces a new 2D mapping option (M12_sWaterfall) and per-segment flag (_is2Deffect) to improve 2D effect handling. Adds waterfall mapping logic, optimizes rendering paths with reallyIs2D gating, and includes bounds-checking for the new mapping value. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as FX Renderer
participant State as Segment::startFrame()
participant Logic as FX_fcn (Rendering)
participant Buffer as Pixel Buffer
Renderer->>State: Update segment frame
State->>State: Detect 2D mode & mapping
State->>State: Set _is2Deffect flag
Renderer->>Logic: Render frame
Logic->>Logic: Compute reallyIs2D = _is2Deffect || (is2D && !(M12_pBar+reverse))
alt reallyIs2D == true
Logic->>Buffer: Apply 2D rendering path
else reallyIs2D == false (e.g., Waterfall)
Logic->>Buffer: Apply optimized 1D/Waterfall path
end
Logic->>Logic: Handle M12_pBar+reverse → M12_sWaterfall mapping
Renderer->>Buffer: Display result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The changes introduce heterogeneous modifications across four files with varying complexity: a new flag and enum value (straightforward), state computation logic with pattern detection (moderate), extensive reallyIs2D gating applied across multiple rendering functions with conditional mapping transformations (dense), and bounds-checking logic (simple). The FX_fcn.cpp modifications require careful review of the new reallyIs2D condition application across different rendering paths. Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (5)
wled00/FX.h (1)
428-429: _is2Deffect flag addition looks good; add brief doc on lifecycle.Flag is consumed to gate 2D paths. Add a short comment that it’s computed in
Segment::startFrame()and not persisted across frames to avoid misuse.wled00/json.cpp (2)
168-169: Normalization hides Waterfall (8) by clamping to Bar (1).Clamping
map1D2D > 7toM12_pBarmeans a caller sending8(Waterfall) silently becomes Bar, relying onreverseto recreate Waterfall. Consider:
- If keeping aliasing: explicitly map
8toM12_pBarand setrev=true(if safe), or- Return a 400/validation error for unsupported values to avoid silent behavior changes.
At minimum, add a comment here noting Waterfall is represented as Bar+reverse.
331-332: Same clamp applied for per‑LED image path; mirror the Waterfall alias note.Replicate the explanatory comment or alias handling here too for consistency with segment deserialization.
wled00/FX_2Dfcn.cpp (1)
270-279: _is2Deffect detection logic OK; cache getModeData() once.Multiple
strstr(strip.getModeData(mode), ...)calls per frame. Cache pointer once before checks to trim flash reads. Minor but cheap win.wled00/FX_fcn.cpp (1)
1445-1466: Gating 2D ops with reallyIs2D avoids wasted 2D work for Waterfall.Solid refactor; reduces unnecessary 2D processing.
Minor: hoist
reallyIs2Dinto a small inline helper to avoid repeating the expression.Also applies to: 1503-1543, 1547-1551, 1574-1586
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
wled00/FX.h(2 hunks)wled00/FX_2Dfcn.cpp(1 hunks)wled00/FX_fcn.cpp(12 hunks)wled00/json.cpp(2 hunks)
🔇 Additional comments (4)
wled00/FX_fcn.cpp (4)
646-664: Aliasing Bar+reverse to Waterfall is clean; LGTM.Local
map1d2dalias keeps storage ≤7 while enabling distinct handling in switches.
876-883: virtualLength accounts for Waterfall; LGTM.Waterfall length = virtualWidth so only top row is written; consistent with copy‑down in startFrame.
970-974: Waterfall setPixelColor writes only first row; matches design.Good separation of concerns with copy‑down in
startFrame().Please confirm this still looks correct when
transpose/reverse_yare set (top row remains the “source” after transforms).
1246-1253: Waterfall getPixelColor returns first‑row color; LGTM.Consistent with write path.
| // WLEDMM Waterfall mapping | ||
| if (!_is2Deffect && (map1D2D == M12_pBar) && reverse) { // WLEDMM Waterfall = bar + reverse | ||
| if (!ledsrgb || (ledsrgbSize == 0)) setUpLeds(); // create segment buffer if not set up already ==> 30% faster, and prevents interference with other segments. | ||
| for (unsigned myRow = _2dHeight-1; myRow > 0; myRow--) | ||
| for (unsigned myCol = 0; myCol < _2dWidth; myCol++) { | ||
| setPixelColorXY(myCol, myRow, getPixelColorXY(int(myCol), int(myRow)-1)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Waterfall runs even when frozen or under realtime control; skip copy in those cases.
startFrame() is called before effect execution and even when seg.freeze is true. The Waterfall copy down (writing all rows) will still mutate pixels during DDP/Art‑Net, conflicting with external control.
Guard the copy with freeze/realtime checks and ensure height≥2.
Apply this diff:
- // WLEDMM Waterfall mapping
- if (!_is2Deffect && (map1D2D == M12_pBar) && reverse) { // WLEDMM Waterfall = bar + reverse
- if (!ledsrgb || (ledsrgbSize == 0)) setUpLeds(); // create segment buffer if not set up already ==> 30% faster, and prevents interference with other segments.
- for (unsigned myRow = _2dHeight-1; myRow > 0; myRow--)
- for (unsigned myCol = 0; myCol < _2dWidth; myCol++) {
- setPixelColorXY(myCol, myRow, getPixelColorXY(int(myCol), int(myRow)-1));
- }
- }
+ // WLEDMM Waterfall mapping (alias: Bar + reverse). Skip under external control or freeze.
+ if (!_is2Deffect && (map1D2D == M12_pBar) && reverse) {
+ // External control guards
+ bool allUnderExt = (realtimeMode && !realtimeOverride && !useMainSegmentOnly);
+ bool mainUnderExt = (realtimeMode && !realtimeOverride && useMainSegmentOnly
+ && (strip.getCurrSegmentId() == strip.getMainSegmentId()));
+ if (!freeze && !(allUnderExt || mainUnderExt) && _2dHeight >= 2) {
+ if (!ledsrgb || (ledsrgbSize == 0)) setUpLeds(); // ensure local buffer
+ for (unsigned myRow = _2dHeight - 1; myRow > 0; myRow--) {
+ for (unsigned myCol = 0; myCol < _2dWidth; myCol++) {
+ setPixelColorXY(myCol, myRow, getPixelColorXY(int(myCol), int(myRow) - 1));
+ }
+ }
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In wled00/FX_2Dfcn.cpp around lines 280 to 287, the Waterfall copy still mutates
pixels during freeze or realtime external control and can run when height < 2;
update the conditional so the row-copy runs only when not frozen, not in
realtime mode, and when _2dHeight >= 2 (e.g. add checks like && !seg.freeze &&
!realtime && (_2dHeight >= 2) to the existing if), leaving the setUpLeds() call
and inner loops unchanged so they are skipped when those guards are true.
| M12_sPinwheel = 7, //WLEDMM Pinwheel | ||
| M12_sWaterfall = 8 //WLEDMM Waterfall | ||
| } mapping1D2D_t; |
There was a problem hiding this comment.
Waterfall value (8) can’t be stored in 3‑bit map1D2D; clarify intent or widen field.
map1D2D is 3 bits (values 0–7). Adding M12_sWaterfall = 8 makes it unrepresentable in the bitfield and JSON will clamp it back to ≤7. Either:
- Treat Waterfall strictly as an alias (don’t attempt to serialize/store 8; document this), or
- Repack
optionsto givemap1D2D4 bits and update serialization accordingly.
Right now this is easy to misconfigure and silently falls back. Recommend adding a comment/static_assert to make this explicit, or widening when feasible.
🤖 Prompt for AI Agents
In wled00/FX.h around lines 350 to 352, the enum assigns M12_sWaterfall = 8 but
map1D2D is stored in a 3-bit field (0–7), so value 8 cannot be represented and
will be clamped; fix by either treating Waterfall as an alias (change its value
to an existing representable value and add a comment documenting it is an alias
and must not be serialized as 8) or by widening the bitfield to 4 bits (repack
the surrounding options, update any serialization/JSON read/write code to use
the new bit width, and add a static_assert ensuring the enum max fits the
bitfield). Ensure you add a clear comment/static_assert so future changes cannot
silently overflow the field.
There was a problem hiding this comment.
@coderabbitai you are right, this part is still temporary and i'm looking for a better solution. do you have an elegant solution to extend map1D2D to 4 bit or more, while not breaking other segment handling functions like Segment::differs() ?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the Segment::differs() implementation
echo "=== Segment::differs() implementation ==="
ast-grep --pattern $'uint8_t differs($$$) const {
$$$
}'
echo -e "\n=== Alternative search for differs ==="
rg -nP -A 20 'uint8_t\s+Segment::differs|uint8_t\s+differs\s*\(' --type=cpp
Length of output: 3196
🏁 Script executed:
#!/bin/bash
# Search for segment serialization/deserialization in JSON
echo "=== Segment serialization (serializeSegment) ==="
rg -nP -B 3 -A 15 'serializeSegment' --type=cpp | head -100
echo -e "\n=== Segment deserialization (deserializeSegment) ==="
rg -nP -B 3 -A 15 'deserializeSegment' --type=cpp | head -100
Length of output: 12522
🏁 Script executed:
#!/bin/bash
# Find usage of the 'set' field in segments
echo "=== Usage of segment 'set' field ==="
rg -nP '\bset\s*[=:]|\.set\b|\->set\b|segment\.set|SEGMENT\.set' --type=cpp -C 2 | head -150
echo -e "\n=== Bitfield 'set' definition ==="
rg -nP 'uint8_t\s+set\s*:\s*2' --type=cpp -C 3
Length of output: 10714
🏁 Script executed:
#!/bin/bash
# Check for any hardcoded size assumptions about options
echo "=== Options size assumptions ==="
rg -nP 'sizeof.*options|options.*16|uint16_t\s+options' --type=cpp -C 2 | head -80
Length of output: 363
🏁 Script executed:
#!/bin/bash
# Find where map1D2D is read/written in JSON
echo "=== map1D2D in JSON handling ==="
rg -nP 'map1D2D|"m12"|m12' wled00/json.cpp -C 3 | head -120
Length of output: 2390
@softhack007, I have an elegant solution for you! Here's my recommendation to extend map1D2D to 4 bits:
Recommended Solution: Expand options from uint16_t to uint32_t
This is the most future-proof approach and gives you 16 additional bits for future segment features.
Changes needed:
1. In wled00/FX.h (around line 365):
union {
uint32_t options; //bit pattern: msb first: [... future bits ...] set:2, soundSim:1, mapping:4, transposed, mirrorY, reverseY, transitional, freeze, reset, mirrored, on, reverse, selected
struct {
bool selected : 1; // 0 : selected
bool reverse : 1; // 1 : reversed
bool on : 1; // 2 : is On
bool mirror : 1; // 3 : mirrored
bool freeze : 1; // 4 : paused/frozen
bool reset : 1; // 5 : indicates that Segment runtime requires reset
bool transitional: 1; // 6 : transitional (there is transition occuring)
bool reverse_y : 1; // 7 : reversed Y (2D)
bool mirror_y : 1; // 8 : mirrored Y (2D)
bool transpose : 1; // 9 : transposed (2D, swapped X & Y)
uint8_t map1D2D : 4; // 10-13 : mapping for 1D effect on 2D (0-15 supported)
uint8_t soundSim : 1; // 14 : 0-1 sound simulation types
uint8_t set : 2; // 15-16 : 0-3 UI segment sets/groups
uint32_t reserved : 15; // 17-31 : reserved for future use
};
};2. In wled00/FX_fcn.cpp (around line 1370-1371):
Update the Segment::differs() mask:
//bit pattern: (msb first) set:2, sound:1, mapping:4, transposed, mirrorY, reverseY, [transitional, reset,] paused, mirrored, on, reverse, [selected]
if ((options & 0b00000000000000011111111110011110U) != (b.options & 0b00000000000000011111111110011110U)) d |= SEG_DIFFERS_OPT;3. In wled00/json.cpp (around line 178 and 331):
Update the constraint to allow values 0-15:
seg.map1D2D = constrain(map1D2D, 0, 15);And remove the clamping at lines 168 and 331:
// Remove: if (map1D2D > 7) map1D2D = M12_pBar;Why this solution is elegant:
- Future-proof: Gives you 15 reserved bits for future segment features
- Minimal breaking changes: JSON serialization already uses named fields, so no changes needed there
- Clean: No aliases or workarounds needed
- Memory impact: Only +2 bytes per segment (52 → 54 bytes), which is acceptable
- No functional breakage:
differs()still works correctly with updated mask
Testing checklist:
- ✅ Verify segment serialization/deserialization still works
- ✅ Test that
differs()correctly detects option changes - ✅ Confirm M12_sWaterfall (8) can now be stored and retrieved
- ✅ Check memory usage is acceptable (should be ~64 bytes more for 32 segments)
This approach won't break any existing segment handling functions—you just need to update the differs() mask and the constraint values. The JSON code already handles this dynamically through field names.
There was a problem hiding this comment.
@coderabbitai very good, thank you bunny,
There was a problem hiding this comment.
@softhack007, you're very welcome! 🐰
Happy to help! If you run into any issues during implementation or need further assistance with the waterfall mapping mode, feel free to reach out. Good luck with the PR! 🌊
1 participant
Waterfall mapping takes a 1D effect on the top row, and copies is down one row in each step.
The code still needs some rework:
examples
"Blurz" looks like comets
"BPM" creates waves
Summary by CodeRabbit
Release Notes
New Features
Improvements