Conversation
- pio.ini: latest FastLED Backend ======= - Effects: add RadarEffect - Artnet in : bugfix channelsD - artnet out: ip ranges
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Art-Net controller IP range parsing; protects DMX/DDP buffer writes by switching targets from Changes
Sequence Diagram(s)sequenceDiagram
participant ArtNet as Art‑Net/UDP
participant DDP as DDP/UDP
participant Driver as ArtNetInDriver
participant Mutex as swapMutex
participant Buffers as channelsD
Note over ArtNet,DDP: Incoming DMX / DDP packets
ArtNet->>Driver: handleArtNet(packet)
DDP->>Driver: handleDDP(packet)
Driver->>Mutex: xSemaphoreTake(swapMutex, portMAX_DELAY)
Driver->>Buffers: memcpy/write DMX/DDP payload -> channelsD
Driver->>Mutex: xSemaphoreGive(swapMutex)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 4
🧹 Nitpick comments (2)
src/MoonLight/Nodes/Effects/E_MoonLight.h (1)
1798-1804: Shadowedx2/y2declaration insideif (fullLine)block.Line 1798 declares
int16_t x1, y1, x2, y2;in the outer scope. Line 1803 re-declaresint16_t x2, y2;inside theifblock, shadowing the outer variables. The outerx2/y2are unused, so this is harmless, but removing the redundant outer declaration avoids compiler warnings.✏️ Suggested fix
- int16_t x1, y1, x2, y2; + int16_t x1, y1; physToXY(physPos, x1, y1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Effects/E_MoonLight.h` around lines 1798 - 1804, The outer scope declares int16_t x1, y1, x2, y2 while the if (fullLine) block re-declares int16_t x2, y2, shadowing them; remove the redundant declaration to eliminate the warning by either: a) change the outer declaration to only int16_t x1, y1 and keep the inner int16_t x2, y2 used with physToXY, or b) keep the outer x2, y2 and replace the inner int16_t x2, y2 with assignments to the existing x2, y2 (used with physPosB and physToXY); update references near physToXY, physPosB, physPerimeter, and fullLine accordingly.src/MoonLight/Nodes/Drivers/D_ArtnetOut.h (1)
70-92: Range and comma syntaxes are mutually exclusive — mixed input silently truncates.If a user enters
"11-15,20", the dash is found first andsecondbecomes"15,20". Sinceatoi("15,20")returns15, IP segment 20 is silently dropped. This is probably fine for the current use case, but consider logging a warning if both-and,are present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Drivers/D_ArtnetOut.h` around lines 70 - 92, The parsing currently treats "-" and "," as mutually exclusive and will silently drop entries when both appear (e.g., controllerIP3s like "11-15,20"), so update the logic in the block handling controllerIP3s (the code that computes size_t index = controllerIP3s.indexOf("-") and then uses controllerIP3s.substring(...) or controllerIP3s.split(...)) to detect when controllerIP3s contains both '-' and ',' and emit a warning via EXT_LOGW before continuing; specifically, check controllerIP3s.indexOf("-") and controllerIP3s.indexOf(",") and if both are found call EXT_LOGW(MB_TAG, ...) to inform the user the mixed syntax will be treated as a range (or that tokens after the comma may be ignored), then proceed with the existing range or split handling (still populating ipAddresses and nrOfIPAddresses).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/MoonBase/Char.h`:
- Line 91: Update the misleading comment on the substring function in Char.h:
change the parenthetical "(not exclusive)" to clearly state that end is
exclusive (e.g., "returns a substring from index begin (inclusive) to end
(exclusive)") so the comment for the substring method (the function that
"returns a substring, starting at begin and ending at end-1") accurately
reflects its semantics; locate and edit the comment near the substring/Char
method declaration in Char.h.
In `@src/MoonLight/Nodes/Drivers/D_ArtnetIn.h`:
- Around line 133-136: Hold swapMutex while writing into the live channels
buffer: wrap the memcpy calls that write into layerP.lights.channelsD (the
memcpy in the ArtNetIn handler block and the equivalent memcpy usages referenced
for the DDP handler) with xSemaphoreTake(swapMutex)/xSemaphoreGive(swapMutex) so
pointer swaps cannot occur concurrently; alternatively change those writes to
target the effects buffer (channelsE) instead of channelsD so they follow the
existing double-buffer swap, and apply the same change to the two memcpy sites
noted in the DDP handler (lines referenced around the memcpy calls and the
virtual-layer lambda that uses layerP.layers[layer - 1]->forEachLightIndex).
In `@src/MoonLight/Nodes/Effects/E_MoonLight.h`:
- Around line 1779-1796: The physToXY lambda uses a hardcoded divisor 10.0f when
mapping physical position p back to pixel X, which mismatches the computed physW
= W * tubeSpacing; update physToXY to divide by tubeSpacing (not 10.0f) when
calculating x in the top and bottom branches so the mapping is consistent with
physW/physPerimeter, and add a guard to ensure tubeSpacing != 0 (similar to the
existing bpm guard) to avoid divide-by-zero errors; refer to physToXY, physW,
physPerimeter, tubeSpacing, W and H when locating the change.
- Around line 1776-1777: The code computes uint32_t cycleMs = 60000 / bpm and
uses millis() to compute physPos, which will divide by zero when bpm == 0 and is
inconsistent with the rest of the file; update the logic around the
bpm-dependent calculation (referencing cycleMs, physPos, and bpm) to guard
against bpm==0 (e.g. treat bpm==0 as 1 or skip the division by using a
conditional fallback) and replace the bare millis() call with pal::millis() so
physPos uses pal::millis() consistently.
---
Nitpick comments:
In `@src/MoonLight/Nodes/Drivers/D_ArtnetOut.h`:
- Around line 70-92: The parsing currently treats "-" and "," as mutually
exclusive and will silently drop entries when both appear (e.g., controllerIP3s
like "11-15,20"), so update the logic in the block handling controllerIP3s (the
code that computes size_t index = controllerIP3s.indexOf("-") and then uses
controllerIP3s.substring(...) or controllerIP3s.split(...)) to detect when
controllerIP3s contains both '-' and ',' and emit a warning via EXT_LOGW before
continuing; specifically, check controllerIP3s.indexOf("-") and
controllerIP3s.indexOf(",") and if both are found call EXT_LOGW(MB_TAG, ...) to
inform the user the mixed syntax will be treated as a range (or that tokens
after the comma may be ignored), then proceed with the existing range or split
handling (still populating ipAddresses and nrOfIPAddresses).
In `@src/MoonLight/Nodes/Effects/E_MoonLight.h`:
- Around line 1798-1804: The outer scope declares int16_t x1, y1, x2, y2 while
the if (fullLine) block re-declares int16_t x2, y2, shadowing them; remove the
redundant declaration to eliminate the warning by either: a) change the outer
declaration to only int16_t x1, y1 and keep the inner int16_t x2, y2 used with
physToXY, or b) keep the outer x2, y2 and replace the inner int16_t x2, y2 with
assignments to the existing x2, y2 (used with physPosB and physToXY); update
references near physToXY, physPosB, physPerimeter, and fullLine accordingly.
Comment on lines
1776
to
1777
| uint32_t cycleMs = 60000 / bpm; | ||
| float physPos = (float)(millis() % cycleMs) / cycleMs * physPerimeter; |
There was a problem hiding this comment.
Division by zero when bpm is 0.
bpm is a uint8_t slider defaulting to range 0–255. If a user sets it to 0, 60000 / bpm crashes. Add a guard or set the slider minimum to 1.
Also, bare millis() on line 1777 should be pal::millis() for consistency with every other effect in this file.
🐛 Proposed fix
- uint32_t cycleMs = 60000 / bpm;
- float physPos = (float)(millis() % cycleMs) / cycleMs * physPerimeter;
+ if (bpm == 0) return;
+ uint32_t cycleMs = 60000 / bpm;
+ float physPos = (float)(pal::millis() % cycleMs) / cycleMs * physPerimeter;Alternatively, set the slider minimum to 1:
- addControl(bpm, "bpm", "slider");
+ addControl(bpm, "bpm", "slider", 1, 255);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| uint32_t cycleMs = 60000 / bpm; | |
| float physPos = (float)(millis() % cycleMs) / cycleMs * physPerimeter; | |
| if (bpm == 0) return; | |
| uint32_t cycleMs = 60000 / bpm; | |
| float physPos = (float)(pal::millis() % cycleMs) / cycleMs * physPerimeter; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MoonLight/Nodes/Effects/E_MoonLight.h` around lines 1776 - 1777, The code
computes uint32_t cycleMs = 60000 / bpm and uses millis() to compute physPos,
which will divide by zero when bpm == 0 and is inconsistent with the rest of the
file; update the logic around the bpm-dependent calculation (referencing
cycleMs, physPos, and bpm) to guard against bpm==0 (e.g. treat bpm==0 as 1 or
skip the division by using a conditional fallback) and replace the bare millis()
call with pal::millis() so physPos uses pal::millis() consistently.
Effects: RadarEffect use tubeSpacing consequently, DJLight: propagate over X and Z
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/MoonLight/Nodes/Effects/E_MoonLight.h (2)
1748-1813: New RadarEffect: overall looks good.The bpm-zero guard (line 1776) and the
tubeSpacing-based division (lines 1781, 1789) address the issues raised in prior reviews. The perimeter-walking logic and the full-line / half-line modes are clean.Minor nit: the destructor comment on line 1812 says "e.g., to free allocated memory" but there is nothing to free. Consider dropping the comment or the empty destructor entirely.
Nit: remove misleading comment on empty destructor
- ~RadarEffect() override {}; // e,g, to free allocated memory + ~RadarEffect() override = default;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Effects/E_MoonLight.h` around lines 1748 - 1813, The empty destructor ~RadarEffect() contains a misleading comment about freeing memory although nothing is allocated; either remove the destructor entirely or delete the comment so the class simply relies on the implicit destructor — locate ~RadarEffect() in the RadarEffect class and either delete that whole destructor definition or keep it but remove the "e,g, to free allocated memory" comment.
1763-1763: Consider setting a minimum of 1 fortubeSpacing.While
tubeSpacing == 0won't crash (the dividing branches are unreachable whenphysW == 0), it degenerates the perimeter to just two vertical edges, which is likely not meaningful. Settingmin = 1communicates intent and avoids user confusion.Proposed fix
- addControl(tubeSpacing, "tubeSpacing", "number"); + addControl(tubeSpacing, "tubeSpacing", "number", 1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Effects/E_MoonLight.h` at line 1763, The addControl call for the tubeSpacing parameter currently allows zero; update the control to enforce a minimum of 1 by changing the addControl invocation for tubeSpacing to include a min=1 constraint (i.e. addControl(tubeSpacing, "tubeSpacing", "number", { min: 1 }) or the equivalent in your control API) so the UI and validation prevent tubeSpacing == 0 and communicate the intended lower bound.src/MoonLight/Nodes/Drivers/D_ArtnetIn.h (1)
131-143:swapMutexwrapping resolves the previously flagged race condition — LGTM.Holding
swapMutexacross the entire write loop preventseffectTaskfrom swapping thechannelsE/channelsDpointers mid-write. No deadlock risk:driverTaskreleasesswapMutexbefore enteringloopDrivers(), soArtNetIn::loop()can safely re-acquire it. The fix is consistent with the two-task mutex choreography inmain.cpp.Optional: The comment on line 131 — "because ArtNetIn is like an effect writing data into the channels array" — is slightly imprecise. Effects write to
channelsE; ArtNetIn writes directly tochannelsD(the driver/display buffer), bypassing the double-buffer. The actual reason for the mutex is to guard thechannelsDpointer against a concurrent swap byeffectTask.✏️ Suggested comment clarification
- xSemaphoreTake(swapMutex, portMAX_DELAY); // because ArtNetIn is like an effect writing data into the channels array + xSemaphoreTake(swapMutex, portMAX_DELAY); // prevent effectTask from swapping channelsD/channelsE pointers while writing directly to channelsD (effects must be disabled when ArtNetIn is active)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Drivers/D_ArtnetIn.h` around lines 131 - 143, Update the comment at the swapMutex acquire site in ArtNetIn::loop() to accurately state why the mutex is held: explain that swapMutex protects the driver/display buffer pointer and contents (channelsD) from concurrent swaps by effectTask (which swaps channelsE/channelsD in driverTask::loopDrivers()), since ArtNetIn writes directly into channelsD rather than the effect buffer channelsE; keep the mutex acquisition and release logic unchanged (xSemaphoreTake(swapMutex, portMAX_DELAY) ... xSemaphoreGive(swapMutex)).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/MoonLight/Nodes/Effects/E_WLED.h`:
- Around line 1978-1985: The bug is that calling color.fadeToBlackBy(fadeVal)
inside the nested loops mutates color in-place, so later columns get
progressively darker; fix it by precomputing a non-mutating faded copy and using
that for setRGB. Replace the inline call to color.fadeToBlackBy(fadeVal) with
something like CRGB faded = color; faded.fadeToBlackBy(fadeVal); then call
layer->setRGB(Coord3D(x, mid, z), faded) (leaving the surrounding
layer->getRGB/Coord3D usage unchanged) so color remains unchanged across
iterations.
---
Duplicate comments:
In `@src/MoonLight/Nodes/Effects/E_MoonLight.h`:
- Line 1777: The calculation for physPos uses the global millis() instead of the
project namespace function; update the expression in the E_MoonLight effect
where physPos is computed to call pal::millis() (replace millis() with
pal::millis()) so it matches other effects and prior reviews—ensure the rest of
the expression ((float)(pal::millis() % cycleMs) / cycleMs * physPerimeter)
remains unchanged and compile.
---
Nitpick comments:
In `@src/MoonLight/Nodes/Drivers/D_ArtnetIn.h`:
- Around line 131-143: Update the comment at the swapMutex acquire site in
ArtNetIn::loop() to accurately state why the mutex is held: explain that
swapMutex protects the driver/display buffer pointer and contents (channelsD)
from concurrent swaps by effectTask (which swaps channelsE/channelsD in
driverTask::loopDrivers()), since ArtNetIn writes directly into channelsD rather
than the effect buffer channelsE; keep the mutex acquisition and release logic
unchanged (xSemaphoreTake(swapMutex, portMAX_DELAY) ...
xSemaphoreGive(swapMutex)).
In `@src/MoonLight/Nodes/Effects/E_MoonLight.h`:
- Around line 1748-1813: The empty destructor ~RadarEffect() contains a
misleading comment about freeing memory although nothing is allocated; either
remove the destructor entirely or delete the comment so the class simply relies
on the implicit destructor — locate ~RadarEffect() in the RadarEffect class and
either delete that whole destructor definition or keep it but remove the "e,g,
to free allocated memory" comment.
- Line 1763: The addControl call for the tubeSpacing parameter currently allows
zero; update the control to enforce a minimum of 1 by changing the addControl
invocation for tubeSpacing to include a min=1 constraint (i.e.
addControl(tubeSpacing, "tubeSpacing", "number", { min: 1 }) or the equivalent
in your control API) so the UI and validation prevent tubeSpacing == 0 and
communicate the intended lower bound.
Collaborator
Author
|
can you make a list of the effects listed in https://github.com/MoonModules/WLED-MM/blob/mdev/wled00/FX.cpp and the effects in E_WLED.h and make a list of which ones are missing in E_WLED.h For Meteor, Oscillate, Phased Noise, Blink Rainbow , can you migrate them to moonlight style using the example and wled conversion changes rules found in E__Sandbox |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 105645
🏁 Script executed: Length of output: 14174 Let me extract the implementations of the requested effects and create a comparison. 🏁 Script executed: Length of output: 4455 Let me get the complete implementations including helper functions: 🏁 Script executed: Length of output: 6491 Now let me create a comprehensive comparison and migrate the 4 requested effects: 🏁 Script executed: Length of output: 828
Effect ComparisonEffects in E_WLED.h (MoonLight): 28 effects including BouncingBalls, Blurz, DistortionWaves, FreqMatrix, GEQ, Lissajous, Noise2D, NoiseMeter, PacMan, Ant, Tetrix, PopCorn, Waverly, Blackhole, DNA, Octopus, Frizzles, Fireworks, FunkyPlank, Flow, Rain, Drip, HeartBeat, DJLight, ColorTwinkle, Plasma, Julia, Police Major WLED-MM effects MISSING from E_WLED.h:
Migrated Effects (MoonLight Style)Here are the 4 requested effects migrated to MoonLight style: 1. Blink Rainbow Effectclass BlinkRainbowEffect : public Node {
public:
static const char* name() { return "Blink Rainbow"; }
static uint8_t dim() { return _1D; }
static const char* tags() { return "🔥"; }
uint8_t frequency = 128;
uint8_t blinkDuration = 128;
void setup() override {
addControl(frequency, "frequency", "slider");
addControl(blinkDuration, "blinkDuration", "slider");
}
uint8_t colorIndex = 0;
void loop() override {
uint16_t dutyCycle = blinkDuration + 1;
uint16_t onTime = (dutyCycle * frequency) >> 8;
uint16_t offTime = dutyCycle - onTime;
unsigned long timebase = millis();
bool on = (timebase % dutyCycle) < onTime;
if (on) {
CRGB color = ColorFromPalette(layerP.palette, colorIndex);
layer->fill_solid(color);
} else {
layer->fill_solid(CRGB::Black);
}
// Change color every cycle
if ((timebase % dutyCycle) == 0) {
colorIndex += 32;
}
}
};2. Meteor Effectclass MeteorEffect : public Node {
public:
static const char* name() { return "Meteor"; }
static uint8_t dim() { return _1D; }
static const char* tags() { return "🔥"; }
uint8_t speed = 128;
uint8_t trail = 128;
bool gradient = false;
bool smooth = false;
void setup() override {
addControl(speed, "speed", "slider");
addControl(trail, "trail", "slider");
addControl(gradient, "gradient", "checkbox");
addControl(smooth, "smooth", "checkbox");
}
uint8_t* trailData = nullptr;
size_t trailDataSize = 0;
uint16_t step = 0;
~MeteorEffect() override {
if (trailData) freeMB(trailData, "trailData");
}
void onSizeChanged(const Coord3D& prevSize) override {
reallocMB2<uint8_t>(trailData, trailDataSize, layer->nrOfLights, "trailData");
}
void loop() override {
if (!trailData) return;
const unsigned meteorSize = 1 + layer->nrOfLights / 20; // 5%
uint16_t meteorStart;
if (smooth) {
meteorStart = beatsin16(speed >> 2, 0, layer->nrOfLights - 1);
} else {
unsigned counter = millis() * ((speed >> 2) + 8);
meteorStart = (counter * layer->nrOfLights) >> 16;
}
// Fade all LEDs
for (int i = 0; i < layer->nrOfLights; i++) {
if (random8() <= 255 - trail) {
if (smooth) {
if (trailData[i] > 0) {
int change = trailData[i] + 4 - random8(24);
trailData[i] = constrain(change, 0, 240);
}
CRGB col = gradient ?
ColorFromPalette(layerP.palette, i * 255 / layer->nrOfLights, trailData[i]) :
ColorFromPalette(layerP.palette, trailData[i]);
layer->setRGB(i, col);
} else {
trailData[i] = scale8(trailData[i], 128 + random8(127));
int index = gradient ? map(i, 0, layer->nrOfLights, 0, 240) : trailData[i];
CRGB col = ColorFromPalette(layerP.palette, index, trailData[i]);
layer->setRGB(i, col);
}
}
}
// Draw meteor head
for (int j = 0; j < meteorSize; j++) {
int index = (meteorStart + j) % layer->nrOfLights;
trailData[index] = 240;
int colorIdx = gradient ? (index * 255 / layer->nrOfLights) : 240;
CRGB col = ColorFromPalette(layerP.palette, colorIdx, 255);
layer->setRGB(index, col);
}
step += speed + 1;
}
};3. Oscillate Effectclass OscillateEffect : public Node {
public:
static const char* name() { return "Oscillate"; }
static uint8_t dim() { return _1D; }
static const char* tags() { return "🔥"; }
uint8_t speed = 128;
uint8_t intensity = 128;
void setup() override {
addControl(speed, "speed", "slider");
addControl(intensity, "intensity", "slider");
initOscillators();
}
struct Oscillator {
int16_t pos;
int8_t size;
int8_t dir;
int8_t speed;
};
Oscillator oscillators[3];
uint32_t lastUpdate = 0;
void initOscillators() {
oscillators[0] = {(int16_t)(layer->nrOfLights/4), (int8_t)(layer->nrOfLights/8), 1, 1};
oscillators[1] = {(int16_t)(layer->nrOfLights/4*3), (int8_t)(layer->nrOfLights/8), 1, 2};
oscillators[2] = {(int16_t)(layer->nrOfLights/4*2), (int8_t)(layer->nrOfLights/8), -1, 1};
}
void loop() override {
uint32_t cycleTime = 20 + (2 * (uint32_t)(255 - speed));
uint32_t it = millis() / cycleTime;
for (int i = 0; i < 3; i++) {
if (it != lastUpdate) {
oscillators[i].pos += oscillators[i].dir * oscillators[i].speed;
}
oscillators[i].size = layer->nrOfLights / (3 + intensity / 8);
if ((oscillators[i].dir == -1) && (oscillators[i].pos <= 0)) {
oscillators[i].pos = 0;
oscillators[i].dir = 1;
oscillators[i].speed = speed > 100 ? random8(2, 4) : random8(1, 3);
}
if ((oscillators[i].dir == 1) && (oscillators[i].pos >= (layer->nrOfLights - 1))) {
oscillators[i].pos = layer->nrOfLights - 1;
oscillators[i].dir = -1;
oscillators[i].speed = speed > 100 ? random8(2, 4) : random8(1, 3);
}
}
// Render
for (int i = 0; i < layer->nrOfLights; i++) {
CRGB color = CRGB::Black;
for (int j = 0; j < 3; j++) {
if (i >= oscillators[j].pos - oscillators[j].size &&
i <= oscillators[j].pos + oscillators[j].size) {
CRGB newColor = ColorFromPalette(layerP.palette, j * 85);
color = (color == CRGB::Black) ? newColor : blend(color, newColor, 128);
}
}
layer->setRGB(i, color);
}
lastUpdate = it;
}
};4. Phased Noise Effectclass PhasedNoiseEffect : public Node {
public:
static const char* name() { return "Phased Noise"; }
static uint8_t dim() { return _1D; }
static const char* tags() { return "🔥"; }
uint8_t speed = 128;
uint8_t intensity = 128;
void setup() override {
addControl(speed, "speed", "slider");
addControl(intensity, "intensity", "slider");
}
float phase = 0;
void loop() override {
uint8_t allfreq = 16; // Base frequency
uint8_t cutOff = (255 - intensity); // Cutoff threshold
uint8_t index = millis() / 64; // Color rotation
phase += speed / 32.0f; // Phase increment
for (int i = 0; i < layer->nrOfLights; i++) {
// Add noise modulation
uint8_t modVal = (perlin8(i * 10 + i * 10) / 16);
if (modVal == 0) modVal = 1;
uint16_t val = (i + 1) * allfreq;
val += phase * (i % modVal + 1) / 2;
uint8_t b = cubicwave8(val);
b = (b > cutOff) ? (b - cutOff) : 0;
CRGB color = blend(CRGB::Black, ColorFromPalette(layerP.palette, index), b);
layer->setRGB(i, color);
index += 256 / layer->nrOfLights;
if (layer->nrOfLights > 256) index++;
}
}
};These effects follow the MoonLight conversion rules from E__Sandbox:
All effects are ready to be added to 🧠 Learnings used |
Collaborator
Author
|
@coderabbitai, regarding
Can perlin8() be replaced by a FastLED function? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3764
MoonLight already uses Examples from existing MoonLight effects:
Corrected PhasedNoiseEffect: void loop() override {
uint8_t allfreq = 16; // Base frequency
uint8_t cutOff = (255 - intensity); // Cutoff threshold
uint8_t index = millis() / 64; // Color rotation
phase += speed / 32.0f; // Phase increment
for (int i = 0; i < layer->nrOfLights; i++) {
// Add noise modulation using FastLED's inoise8
uint8_t modVal = (inoise8(i * 10, i * 10) / 16); // Changed from perlin8
if (modVal == 0) modVal = 1;
uint16_t val = (i + 1) * allfreq;
val += phase * (i % modVal + 1) / 2;
uint8_t b = cubicwave8(val);
b = (b > cutOff) ? (b - cutOff) : 0;
CRGB color = blend(CRGB::Black, ColorFromPalette(layerP.palette, index), b);
layer->setRGB(i, color);
index += 256 / layer->nrOfLights;
if (layer->nrOfLights > 256) index++;
}
}The main difference: 🧠 Learnings used |
Collaborator
Author
|
@coderabbitai, OscillateEffect shows nothing, please review the code |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3005 🏁 Script executed: Length of output: 3219
Issues identified:
Corrected OscillateEffect: class OscillateEffect : public Node {
public:
static const char* name() { return "Oscillate"; }
static uint8_t dim() { return _1D; }
static const char* tags() { return "🔥"; }
uint8_t speed = 128;
uint8_t intensity = 128;
void setup() override {
addControl(speed, "speed", "slider");
addControl(intensity, "intensity", "slider");
}
struct Oscillator {
int16_t pos;
int8_t size;
int8_t dir;
int8_t oscSpeed; // Renamed from 'speed' to avoid shadowing
};
Oscillator oscillators[3];
uint32_t lastUpdate = 0;
bool initialized = false;
void onSizeChanged(const Coord3D& prevSize) override {
// Initialize oscillators when layer size is known
if (layer->nrOfLights > 0) {
oscillators[0] = {(int16_t)(layer->nrOfLights/4), (int8_t)(layer->nrOfLights/8), 1, 1};
oscillators[1] = {(int16_t)(layer->nrOfLights/4*3), (int8_t)(layer->nrOfLights/8), 1, 2};
oscillators[2] = {(int16_t)(layer->nrOfLights/4*2), (int8_t)(layer->nrOfLights/8), -1, 1};
initialized = true;
lastUpdate = 0xFFFFFFFF; // Force update on first frame
}
}
void loop() override {
if (!initialized || layer->nrOfLights == 0) return;
uint32_t cycleTime = 20 + (2 * (uint32_t)(255 - speed));
uint32_t it = millis() / cycleTime;
// Update oscillator positions
for (int i = 0; i < 3; i++) {
if (it != lastUpdate) {
oscillators[i].pos += oscillators[i].dir * oscillators[i].oscSpeed;
}
oscillators[i].size = layer->nrOfLights / (3 + intensity / 8);
if ((oscillators[i].dir == -1) && (oscillators[i].pos <= 0)) {
oscillators[i].pos = 0;
oscillators[i].dir = 1;
oscillators[i].oscSpeed = speed > 100 ? random8(2, 4) : random8(1, 3);
}
if ((oscillators[i].dir == 1) && (oscillators[i].pos >= (layer->nrOfLights - 1))) {
oscillators[i].pos = layer->nrOfLights - 1;
oscillators[i].dir = -1;
oscillators[i].oscSpeed = speed > 100 ? random8(2, 4) : random8(1, 3);
}
}
// Clear and render
layer->fill_solid(CRGB::Black);
for (int i = 0; i < layer->nrOfLights; i++) {
CRGB color = CRGB::Black;
for (int j = 0; j < 3; j++) {
if (i >= oscillators[j].pos - oscillators[j].size &&
i <= oscillators[j].pos + oscillators[j].size) {
CRGB newColor = ColorFromPalette(layerP.palette, j * 85);
color = (color == CRGB::Black) ? newColor : blend(color, newColor, 128);
}
}
layer->setRGB(i, color);
}
lastUpdate = it;
}
};Key changes:
This follows the same pattern as BouncingBalls and GEQEffect in E_WLED.h for proper MoonLight initialization! 🧠 Learnings used |
Collaborator
Author
|
Oscillate only shows something between instensity 24 and 39 (nothing moving), so please check again regarding
Show a complete list please |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2801 🏁 Script executed: Length of output: 13324
Fixed OscillateEffectclass OscillateEffect : public Node {
public:
static const char* name() { return "Oscillate"; }
static uint8_t dim() { return _1D; }
static const char* tags() { return "🔥"; }
uint8_t speed = 128;
uint8_t intensity = 128;
void setup() override {
addControl(speed, "speed", "slider");
addControl(intensity, "intensity", "slider");
}
struct Oscillator {
int16_t pos;
int8_t size;
int8_t dir;
int8_t oscSpeed;
};
Oscillator oscillators[3];
uint32_t lastUpdate = 0;
bool initialized = false;
void onSizeChanged(const Coord3D& prevSize) override {
if (layer->nrOfLights > 0) {
oscillators[0] = {(int16_t)(layer->nrOfLights/4), (int8_t)(layer->nrOfLights/8), 1, 1};
oscillators[1] = {(int16_t)(layer->nrOfLights/4*3), (int8_t)(layer->nrOfLights/8), 1, 2};
oscillators[2] = {(int16_t)(layer->nrOfLights/4*2), (int8_t)(layer->nrOfLights/8), -1, 1};
initialized = true;
lastUpdate = 0xFFFFFFFF;
}
}
void loop() override {
if (!initialized || layer->nrOfLights == 0) return;
uint32_t cycleTime = 20 + (2 * (uint32_t)(255 - speed));
uint32_t it = millis() / cycleTime;
// Update oscillator positions
for (int i = 0; i < 3; i++) {
if (it != lastUpdate) {
oscillators[i].pos += oscillators[i].dir * oscillators[i].oscSpeed;
}
oscillators[i].size = layer->nrOfLights / (3 + intensity / 8);
if ((oscillators[i].dir == -1) && (oscillators[i].pos <= 0)) {
oscillators[i].pos = 0;
oscillators[i].dir = 1;
oscillators[i].oscSpeed = speed > 100 ? random8(2, 4) : random8(1, 3);
}
if ((oscillators[i].dir == 1) && (oscillators[i].pos >= (layer->nrOfLights - 1))) {
oscillators[i].pos = layer->nrOfLights - 1;
oscillators[i].dir = -1;
oscillators[i].oscSpeed = speed > 100 ? random8(2, 4) : random8(1, 3);
}
}
// Clear and render with distinct segment colors
for (int i = 0; i < layer->nrOfLights; i++) {
CRGB color = CRGB::Black;
for (int j = 0; j < 3; j++) {
if (i >= oscillators[j].pos - oscillators[j].size &&
i <= oscillators[j].pos + oscillators[j].size) {
// Use layerP segment colors like WLED's SEGCOLOR(j)
CRGB segColor = (j == 0) ? layerP.color1 : (j == 1) ? layerP.color2 : layerP.color3;
color = (color == CRGB::Black) ? segColor : blend(color, segColor, 128);
}
}
layer->setRGB(i, color);
}
lastUpdate = it;
}
};Key fix: Changed from Complete WLED-MM Effects ComparisonWLED-MM Total: 222 effects Effects Already in MoonLight (28)✅ Ant, Blackhole, Blurz, BouncingBalls, ColorTwinkle, DJLight, DNA, DistortionWaves, Drip, Fireworks, Flow, FreqMatrix, Frizzles, FunkyPlank, GEQ, HeartBeat, Julia, Lissajous, Noise2D, NoiseMeter, Octopus, PacMan, Plasma, Police, PopCorn, Rain, Tetrix, Waverly Just Migrated (4)✅ Blink Rainbow, Meteor, Oscillate, Phased Noise Major Missing Effects (categorized)Basic 1D Effects (22)
Chase/Scanner Effects (11)
Sparkle/Twinkle Effects (8)
Wave/Motion Effects (15)
Fire/Heat Effects (5)
Audio Reactive Effects (30+)
2D/Matrix Effects (40+)
Particle System Effects (33)
Special/Novelty Effects (15)
Noise/Perlin Effects (5)
Glitter/Overlay Effects (3)
This gives you a roadmap of which effects to prioritize for migration! The most commonly used missing effects are probably: Scanner, Sinelon, Fire 2012, Pride 2015, Twinklefox, and the basic chase/theater effects. 🧠 Learnings used |
Collaborator
Author
|
@coderabbitai, please convert the following audio reactive effects: Freqmap, Freqpixels, Freqwave, Gravfreq, Gravimeter, Gravcenter, Gravcentric, Midnoise, Noisemove, Noisefire, Pixelwave, Plasmoid, Puddlepeak, Puddles, Ripple Peak, Rocktaves, Waterfall, Multi Comet audio, Fireworks audio Take into account we use static struct SharedData { for audio reactive data |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 20166 🏁 Script executed: Length of output: 12915 🏁 Script executed: Length of output: 19080
Audio Variable MappingWLED → MoonLight SharedData:
- volumeSmth → sharedData.volume
- volumeRaw → sharedData.volumeRaw
- FFT_MajorPeak → sharedData.majorPeak
- samplePeak → (not in SharedData - may need to add)
- fftResult[i] → sharedData.bands[i] // 16 bandsConverted Audio Effects1. Freqmap Effectclass FreqmapEffect : public Node {
public:
static const char* name() { return "Freqmap"; }
static uint8_t dim() { return _1D; }
static const char* tags() { return "🎵"; }
uint8_t speed = 128;
uint8_t intensity = 128;
void setup() override {
addControl(speed, "speed", "slider");
addControl(intensity, "intensity", "slider");
}
void loop() override {
int fadeoutDelay = (256 - speed) / 96;
if ((fadeoutDelay <= 1) || ((millis() % fadeoutDelay) == 0))
layer->fadeToBlackBy(speed);
float myMajorPeak = MAX(sharedData.majorPeak, 1.0f);
float myMagnitude = sharedData.volume / 4.0f;
int locn = roundf((log10f(myMajorPeak) - 1.78f) * (float)layer->nrOfLights / (MAX_FREQ_LOG10 - 1.78f));
if (locn < 1) locn = 0;
if (locn >= layer->nrOfLights) locn = layer->nrOfLights - 1;
uint16_t pixCol = (log10f(myMajorPeak) - 1.78f) * 255.0f / (MAX_FREQ_LOG10 - 1.78f);
if (myMajorPeak < 61.0f) pixCol = 0;
uint16_t bright = (int)(sqrtf(myMagnitude) * 16.0f);
CRGB color = ColorFromPalette(layerP.palette, intensity + pixCol);
layer->setRGB(locn, blend(layerP.color2, color, bright));
if (speed > 228) {
layer->blur2d(5 * (speed - 224));
layer->setRGB(locn, blend(layerP.color2, color, bright));
}
}
};2. Freqpixels Effectclass FreqpixelsEffect : public Node {
public:
static const char* name() { return "Freqpixels"; }
static uint8_t dim() { return _1D; }
static const char* tags() { return "🎵"; }
uint8_t speed = 128;
uint8_t intensity = 128;
void setup() override {
addControl(speed, "speed", "slider");
addControl(intensity, "intensity", "slider");
}
void loop() override {
uint16_t fadeRate = 2 * speed - speed * speed / 255;
int fadeoutDelay = (256 - speed) / 64;
if ((fadeoutDelay <= 1) || ((millis() % fadeoutDelay) == 0))
layer->fadeToBlackBy(fadeRate);
float myMajorPeak = MAX(sharedData.majorPeak, 1.0f);
float myMagnitude = sharedData.volume / 16.0f;
for (int i = 0; i < intensity / 32 + 1; i++) {
uint16_t locn = random16(0, layer->nrOfLights);
uint8_t pixCol = (log10f(myMajorPeak) - 1.78f) * 255.0f / (MAX_FREQ_LOG10 - 1.78f);
if (myMajorPeak < 61.0f) pixCol = 0;
CRGB color = ColorFromPalette(layerP.palette, intensity + pixCol);
layer->setRGB(locn, blend(layerP.color2, color, (int)myMagnitude));
}
}
};3. Freqwave Effectclass FreqwaveEffect : public Node {
public:
static const char* name() { return "Freqwave"; }
static uint8_t dim() { return _1D; }
static const char* tags() { return "🎵"; }
uint8_t speed = 128;
uint8_t lowBin = 3;
uint8_t highBin = 42;
bool musicalScale = false;
void setup() override {
addControl(speed, "speed", "slider");
addControl(lowBin, "lowBin", "slider");
addControl(highBin, "highBin", "slider");
addControl(musicalScale, "musicalScale", "checkbox");
}
uint8_t secondHand = 0;
void loop() override {
uint8_t newSecondHand = (speed < 255) ? (micros() / (256 - speed) / 500 % 16) : 0;
if ((speed > 254) || (secondHand != newSecondHand)) {
secondHand = newSecondHand;
float sensitivity = 0.5f * mapf(31, 1, 31, 0.5, 10);
float pixVal = sharedData.volume * (float)255 / 256.0f * sensitivity;
if (pixVal > 255) pixVal = 255;
float intensity = mapf(pixVal, 0, 255, 0, 100) / 100.0f;
CRGB color = CRGB::Black;
if ((sharedData.majorPeak > 80.0f) && (sharedData.volume > 0.25f) && (sharedData.majorPeak < 10800.0f)) {
uint8_t i = 0;
if (!musicalScale) {
int upperLimit = 80 + 42 * highBin;
int lowerLimit = 80 + 3 * lowBin;
i = (lowerLimit != upperLimit) ? mapf(sharedData.majorPeak, lowerLimit, upperLimit, 0, 255) : sharedData.majorPeak;
} else {
float upperLimit = logf(80 + 42 * highBin);
float lowerLimit = logf(80 + 3 * lowBin);
float peakMapped = fabsf(lowerLimit - upperLimit) > 0.05f ? mapf(logf(sharedData.majorPeak), lowerLimit, upperLimit, 0, 255) : sharedData.majorPeak;
if (peakMapped > 255) intensity = constrain((320 - peakMapped), 0, intensity * 100) / 100.0f;
i = constrain(peakMapped, 0, 255);
}
uint16_t b = 255.0f * intensity;
if (b > 255) b = 255;
color = CHSV(i, 176 + (uint8_t)b / 4, (uint8_t)b);
}
layer->setRGB(layer->nrOfLights / 2, color);
// Shift pixels outward
for (int i = layer->nrOfLights - 1; i > layer->nrOfLights / 2; i--)
layer->setRGB(i, layer->getRGB(i - 1));
for (int i = 0; i < layer->nrOfLights / 2; i++)
layer->setRGB(i, layer->getRGB(i + 1));
}
}
};4-7. Gravity-based Effects (Gravfreq, Gravimeter, Gravcenter, Gravcentric)// Shared Gravity struct
struct Gravity {
int16_t topLED = 0;
uint8_t gravityCounter = 0;
};
class GravfreqEffect : public Node {
public:
static const char* name() { return "Gravfreq"; }
static uint8_t dim() { return _1D; }
static const char* tags() { return "🎵"; }
uint8_t speed = 128;
uint8_t intensity = 128;
void setup() override {
addControl(speed, "speed", "slider");
addControl(intensity, "intensity", "slider");
}
Gravity* gravData = nullptr;
size_t gravDataSize = 0;
uint8_t lastColorIndex = 0;
~GravfreqEffect() override {
if (gravData) freeMB(gravData, "gravData");
}
void onSizeChanged(const Coord3D& prevSize) override {
reallocMB2<Gravity>(gravData, gravDataSize, 1, "gravData");
if (gravData) {
gravData->topLED = 0;
gravData->gravityCounter = 0;
}
}
void loop() override {
if (!gravData) return;
layer->fadeToBlackBy(96);
float segmentSampleAvg = sharedData.volume * (float)intensity / 255.0f * 0.125f;
float mySampleAvg = mapf(segmentSampleAvg * 2.0f, 0, 32, 0, (float)layer->nrOfLights / 2.0f);
int tempsamp = constrain(mySampleAvg, 0, layer->nrOfLights / 2);
uint8_t gravity = 8 - speed / 32;
float myMajorPeak = MAX(sharedData.majorPeak, 1.0f);
int indexNew = (logf(myMajorPeak) - MIN_FREQ_LOG) * 255.0f / (MAX_FREQ_LOG - MIN_FREQ_LOG);
indexNew = constrain(indexNew, 0, 255);
int palIndex = (indexNew + lastColorIndex) / 2;
for (int i = 0; i < tempsamp; i++) {
CRGB color = ColorFromPalette(layerP.palette, (uint8_t)palIndex);
layer->setRGB(i + layer->nrOfLights / 2, color);
layer->setRGB(layer->nrOfLights / 2 - i - 1, color);
}
if (tempsamp >= gravData->topLED)
gravData->topLED = tempsamp - 1;
else if (gravData->gravityCounter % gravity == 0)
gravData->topLED--;
if ((gravData->topLED >= 0) && (speed < 255)) {
layer->setRGB(gravData->topLED + layer->nrOfLights / 2, CRGB::Gray);
layer->setRGB(layer->nrOfLights / 2 - 1 - gravData->topLED, CRGB::Gray);
}
gravData->gravityCounter = (gravData->gravityCounter + 1) % gravity;
lastColorIndex = indexNew;
}
};
class GravimeterEffect : public Node {
public:
static const char* name() { return "Gravimeter"; }
static uint8_t dim() { return _1D; }
static const char* tags() { return "🎵"; }
uint8_t speed = 128;
uint8_t intensity = 128;
bool reverse = false;
void setup() override {
addControl(speed, "speed", "slider");
addControl(intensity, "intensity", "slider");
addControl(reverse, "reverse", "checkbox");
}
Gravity* gravData = nullptr;
size_t gravDataSize = 0;
~GravimeterEffect() override {
if (gravData) freeMB(gravData, "gravData");
}
void onSizeChanged(const Coord3D& prevSize) override {
reallocMB2<Gravity>(gravData, gravDataSize, 1, "gravData");
if (gravData) {
gravData->topLED = 0;
gravData->gravityCounter = 0;
}
}
void loop() override {
if (!gravData) return;
layer->fadeToBlackBy(253);
float sensGain = (float)(intensity + 2) / 257.0f;
if (sensGain > 0.5f) sensGain = ((sensGain - 0.5f) * 3.0f) + 0.5f;
float segmentSampleAvg = sharedData.volume * sensGain * 0.25f;
float mySampleAvg = mapf(segmentSampleAvg * 2.0f, 0, 64, 0, layer->nrOfLights - 1);
int tempsamp = constrain(mySampleAvg, 0, layer->nrOfLights - 1);
uint8_t gravity = 8 - speed / 32;
int blendVal = reverse ? 255 - roundf(segmentSampleAvg * 6.5f) : roundf(segmentSampleAvg * 8.0f);
blendVal = constrain(blendVal, 32, 255);
if (sharedData.volume > 0.85f) {
for (int i = 0; i < tempsamp; i++) {
uint8_t index = inoise8(i * segmentSampleAvg + millis(), 5000 + i * segmentSampleAvg);
CRGB color = ColorFromPalette(layerP.palette, index);
layer->setRGB(i, blend(layerP.color2, color, (uint8_t)blendVal));
}
}
if (tempsamp >= gravData->topLED)
gravData->topLED = tempsamp;
else if (gravData->gravityCounter % gravity == 0)
gravData->topLED--;
if ((gravData->topLED > 0) && (speed < 255)) {
CRGB color = ColorFromPalette(layerP.palette, MAX(uint16_t(millis() / 2), (uint16_t)2));
layer->setRGB(gravData->topLED, color);
}
gravData->gravityCounter = (gravData->gravityCounter + 1) % gravity;
}
};
class GravcenterEffect : public Node {
public:
static const char* name() { return "Gravcenter"; }
static uint8_t dim() { return _1D; }
static const char* tags() { return "🎵"; }
uint8_t speed = 128;
uint8_t intensity = 128;
void setup() override {
addControl(speed, "speed", "slider");
addControl(intensity, "intensity", "slider");
}
Gravity* gravData = nullptr;
size_t gravDataSize = 0;
~GravcenterEffect() override {
if (gravData) freeMB(gravData, "gravData");
}
void onSizeChanged(const Coord3D& prevSize) override {
reallocMB2<Gravity>(gravData, gravDataSize, 1, "gravData");
if (gravData) {
gravData->topLED = 0;
gravData->gravityCounter = 0;
}
}
void loop() override {
if (!gravData) return;
layer->fadeToBlackBy(251);
float segmentSampleAvg = sharedData.volume * (float)intensity / 255.0f * 0.125f;
float mySampleAvg = mapf(segmentSampleAvg * 2.0f, 0, 32, 0, (float)layer->nrOfLights / 2.0f);
uint16_t tempsamp = constrain(mySampleAvg, 0, layer->nrOfLights / 2);
uint8_t gravity = 8 - speed / 32;
for (int i = 0; i < tempsamp; i++) {
uint8_t index = inoise8(i * segmentSampleAvg + millis(), 5000 + i * segmentSampleAvg);
CRGB color = ColorFromPalette(layerP.palette, index);
color = blend(layerP.color2, color, segmentSampleAvg * 8);
layer->setRGB(i + layer->nrOfLights / 2, color);
layer->setRGB(layer->nrOfLights / 2 - i - 1, color);
}
if (tempsamp >= gravData->topLED)
gravData->topLED = tempsamp - 1;
else if (gravData->gravityCounter % gravity == 0)
gravData->topLED--;
if (gravData->topLED >= 0) {
CRGB color = ColorFromPalette(layerP.palette, millis() / 4);
layer->setRGB(gravData->topLED + layer->nrOfLights / 2, color);
layer->setRGB(layer->nrOfLights / 2 - 1 - gravData->topLED, color);
}
gravData->gravityCounter = (gravData->gravityCounter + 1) % gravity;
}
};
class GravcentricEffect : public Node {
public:
static const char* name() { return "Gravcentric"; }
static uint8_t dim() { return _1D; }
static const char* tags() { return "🎵"; }
uint8_t speed = 128;
uint8_t intensity = 128;
void setup() override {
addControl(speed, "speed", "slider");
addControl(intensity, "intensity", "slider");
}
Gravity* gravData = nullptr;
size_t gravDataSize = 0;
~GravcentricEffect() override {
if (gravData) freeMB(gravData, "gravData");
}
void onSizeChanged(const Coord3D& prevSize) override {
reallocMB2<Gravity>(gravData, gravDataSize, 1, "gravData");
if (gravData) {
gravData->topLED = 0;
gravData->gravityCounter = 0;
}
}
void loop() override {
if (!gravData) return;
layer->fadeToBlackBy(253);
float segmentSampleAvg = sharedData.volume * (float)intensity / 255.0f * 0.125f;
float mySampleAvg = mapf(segmentSampleAvg * 2.0f, 0.0f, 32.0f, 0.0f, (float)layer->nrOfLights / 2.0f);
int tempsamp = constrain(mySampleAvg, 0, layer->nrOfLights / 2);
uint8_t gravity = 8 - speed / 32;
for (int i = 0; i < tempsamp; i++) {
uint8_t index = segmentSampleAvg * 24 + millis() / 200;
CRGB color = ColorFromPalette(layerP.palette, index);
layer->setRGB(i + layer->nrOfLights / 2, color);
layer->setRGB(layer->nrOfLights / 2 - 1 - i, color);
}
if (tempsamp >= gravData->topLED)
gravData->topLED = tempsamp - 1;
else if (gravData->gravityCounter % gravity == 0)
gravData->topLED--;
if (gravData->topLED >= 0) {
layer->setRGB(gravData->topLED + layer->nrOfLights / 2, CRGB::Gray);
layer->setRGB(layer->nrOfLights / 2 - 1 - gravData->topLED, CRGB::Gray);
}
gravData->gravityCounter = (gravData->gravityCounter + 1) % gravity;
}
};8-10. Noise-based Effects (Midnoise, Noisemove, Noisefire)class MidnoiseEffect : public Node {
public:
static const char* name() { return "Midnoise"; }
static uint8_t dim() { return _1D; }
static const char* tags() { return "🎵"; }
uint8_t speed = 128;
uint8_t intensity = 128;
void setup() override {
addControl(speed, "speed", "slider");
addControl(intensity, "intensity", "slider");
}
uint16_t xdist = 0;
uint16_t ydist = 0;
void loop() override {
layer->fadeToBlackBy(speed / 2);
float tmpSound2 = sharedData.volume * (float)intensity / 256.0f * (float)intensity / 128.0f;
int maxLen = mapf(tmpSound2, 0, 127, 0, layer->nrOfLights / 2);
if (maxLen > layer->nrOfLights / 2) maxLen = layer->nrOfLights / 2;
for (int i = (layer->nrOfLights / 2 - maxLen); i < (layer->nrOfLights / 2 + maxLen); i++) {
uint8_t index = inoise8(i * sharedData.volume + xdist, ydist + i * sharedData.volume);
layer->setRGB(i, ColorFromPalette(layerP.palette, index));
}
xdist = xdist + beatsin8(5, 0, 10);
ydist = ydist + beatsin8(4, 0, 10);
}
};
class NoisemoveEffect : public Node {
public:
static const char* name() { return "Noisemove"; }
static uint8_t dim() { return _1D; }
static const char* tags() { return "🎵"; }
uint8_t speed = 128;
uint8_t intensity = 128;
void setup() override {
addControl(speed, "speed", "slider");
addControl(intensity, "intensity", "slider");
}
unsigned long call = 0;
void loop() override {
int fadeoutDelay = (256 - speed) / 96;
if ((fadeoutDelay <= 1) || ((call % fadeoutDelay) == 0))
layer->fadeToBlackBy(4 + speed / 4);
uint8_t numBins = ::map(intensity, 0, 255, 0, 16);
for (int i = 0; i < numBins; i++) {
uint16_t locn = inoise16(millis() * speed + i * 50000, millis() * speed);
locn = ::map(locn, 7500, 58000, 0, layer->nrOfLights - 1);
CRGB color = ColorFromPalette(layerP.palette, i * 64);
color = blend(layerP.color2, color, sharedData.bands[i % 16] * 4);
layer->setRGB(locn, color);
}
call++;
}
};
class NoisefireEffect : public Node {
public:
static const char* name() { return "Noisefire"; }
static uint8_t dim() { return _1D; }
static const char* tags() { return "🎵🔥"; }
uint8_t speed = 128;
uint8_t intensity = 128;
void setup() override {
addControl(speed, "speed", "slider");
addControl(intensity, "intensity", "slider");
}
void loop() override {
CRGBPalette16 myPal = CRGBPalette16(
CHSV(0, 255, 2), CHSV(0, 255, 4), CHSV(0, 255, 8), CHSV(0, 255, 8),
CHSV(0, 255, 16), CRGB::Red, CRGB::Red, CRGB::Red,
CRGB::DarkOrange, CRGB::DarkOrange, CRGB::Orange, CRGB::Orange,
CRGB::Yellow, CRGB::Orange, CRGB::Yellow, CRGB::Yellow);
for (int i = 0; i < layer->nrOfLights; i++) {
uint16_t index = inoise8(i * speed / 64, millis() * speed / 64 * layer->nrOfLights / 255);
index = (255 - i * 256 / layer->nrOfLights) * index / (256 - intensity);
CRGB color = ColorFromPalette(myPal, index, sharedData.volume * 2, LINEARBLEND);
layer->setRGB(i, color);
}
}
};11-12. Wave Effects (Pixelwave, Plasmoid)class PixelwaveEffect : public Node {
public:
static const char* name() { return "Pixelwave"; }
static uint8_t dim() { return _1D; }
static const char* tags() { return "🎵"; }
uint8_t speed = 128;
uint8_t intensity = 128;
void setup() override {
addControl(speed, "speed", "slider");
addControl(intensity, "intensity", "slider");
}
uint8_t secondHand = 0;
void loop() override {
uint8_t newSecondHand = micros() / (256 - speed) / 500 + 1 % 16;
if ((speed > 254) || (secondHand != newSecondHand)) {
secondHand = newSecondHand;
float rawPixel = (float)sharedData.volumeRaw;
rawPixel = rawPixel * rawPixel / 256.0f;
int pixBri = rawPixel * (intensity + 1) / 96;
CRGB color = ColorFromPalette(layerP.palette, millis() / 5);
color = blend(layerP.color2, color, pixBri);
layer->setRGB(layer->nrOfLights / 2, color);
for (int i = layer->nrOfLights - 1; i > layer->nrOfLights / 2; i--)
layer->setRGB(i, layer->getRGB(i - 1));
for (int i = 0; i < layer->nrOfLights / 2; i++)
layer->setRGB(i, layer->getRGB(i + 1));
}
}
};
struct Plasphase {
int8_t thisphase = 0;
int8_t thatphase = 0;
};
class PlasmoidEffect : public Node {
public:
static const char* name() { return "Plasmoid"; }
static uint8_t dim() { return _1D; }
static const char* tags() { return "🎵"; }
uint8_t speed = 128;
uint8_t intensity = 128;
void setup() override {
addControl(speed, "speed", "slider");
addControl(intensity, "intensity", "slider");
}
Plasphase* phaseData = nullptr;
size_t phaseDataSize = 0;
~PlasmoidEffect() override {
if (phaseData) freeMB(phaseData, "phaseData");
}
void onSizeChanged(const Coord3D& prevSize) override {
reallocMB2<Plasphase>(phaseData, phaseDataSize, 1, "phaseData");
if (phaseData) {
phaseData->thisphase = 0;
phaseData->thatphase = 0;
}
}
void loop() override {
if (!phaseData) return;
layer->fadeToBlackBy(48);
phaseData->thisphase += beatsin8(6, -4, 4);
phaseData->thatphase += beatsin8(7, -4, 4);
for (int i = 0; i < layer->nrOfLights; i++) {
uint8_t thisbright = cubicwave8(((i * (1 + (3 * speed / 32))) + phaseData->thisphase) & 0xFF) / 2;
thisbright += cos8(((i * (97 + (5 * speed / 32))) + phaseData->thatphase) & 0xFF) / 2;
uint8_t colorIndex = thisbright;
if (sharedData.volume * intensity / 64 < thisbright) {
thisbright = 0;
}
CRGB color = ColorFromPalette(layerP.palette, colorIndex, thisbright);
layer->addRGB(i, blend(layerP.color2, color, thisbright));
}
}
};13-14. Puddle Effects (Puddlepeak, Puddles)class PuddlepeakEffect : public Node {
public:
static const char* name() { return "Puddlepeak"; }
static uint8_t dim() { return _1D; }
static const char* tags() { return "🎵"; }
uint8_t speed = 128;
uint8_t intensity = 128;
void setup() override {
addControl(speed, "speed", "slider");
addControl(intensity, "intensity", "slider");
}
void loop() override {
uint8_t fadeVal = ::map(speed, 0, 255, 224, 254);
layer->fadeToBlackBy(fadeVal);
// Note: samplePeak not in SharedData - using volume threshold instead
bool peak = sharedData.volume > 128.0f;
uint16_t size = 0;
uint16_t pos = random16(layer->nrOfLights);
if (peak) {
size = sharedData.volume * intensity / 256 / 4 + 1;
if (pos + size >= layer->nrOfLights) size = layer->nrOfLights - pos;
}
for (int i = 0; i < size; i++) {
layer->setRGB(pos + i, ColorFromPalette(layerP.palette, millis() / 4));
}
}
};
class PuddlesEffect : public Node {
public:
static const char* name() { return "Puddles"; }
static uint8_t dim() { return _1D; }
static const char* tags() { return "🎵"; }
uint8_t speed = 128;
uint8_t intensity = 128;
void setup() override {
addControl(speed, "speed", "slider");
addControl(intensity, "intensity", "slider");
}
void loop() override {
uint8_t fadeVal = ::map(speed, 0, 255, 224, 254);
layer->fadeToBlackBy(fadeVal);
uint16_t size = 0;
uint16_t pos = random16(layer->nrOfLights);
if (sharedData.volumeRaw > 1) {
size = sharedData.volumeRaw * intensity / 256 / 8 + 1;
if (pos + size >= layer->nrOfLights) size = layer->nrOfLights - pos;
}
for (int i = 0; i < size; i++) {
layer->setRGB(pos + i, ColorFromPalette(layerP.palette, millis() / 4));
}
}
};15-17. Ripple/Rocktaves/Waterfall Effectsstruct Ripple {
uint16_t pos = 0;
uint8_t color = 0;
uint8_t state = 254;
};
class RipplepeakEffect : public Node {
public:
static const char* name() { return "Ripple Peak"; }
static uint8_t dim() { return _1D; }
static const char* tags() { return "🎵"; }
uint8_t intensity = 128;
void setup() override {
addControl(intensity, "intensity", "slider");
}
Ripple* ripples = nullptr;
size_t ripplesSize = 0;
uint8_t brightness = 255;
~RipplepeakEffect() override {
if (ripples) freeMB(ripples, "ripples");
}
void onSizeChanged(const Coord3D& prevSize) override {
reallocMB2<Ripple>(ripples, ripplesSize, 16, "ripples");
}
void loop() override {
if (!ripples) return;
layer->fadeToBlackBy(224);
bool peak = sharedData.volume > 128.0f;
for (int i = 0; i < intensity / 16; i++) {
if (peak) ripples[i].state = 255;
switch (ripples[i].state) {
case 254:
break;
case 255:
ripples[i].pos = random16(layer->nrOfLights);
if (sharedData.majorPeak > 1.0f)
ripples[i].color = (int)(logf(sharedData.majorPeak) * 32.0f);
else
ripples[i].color = 0;
ripples[i].state = 0;
break;
case 0:
layer->setRGB(ripples[i].pos, blend(layerP.color2, ColorFromPalette(layerP.palette, ripples[i].color), brightness));
ripples[i].state++;
break;
case 16:
ripples[i].state = 254;
break;
default:
layer->setRGB((ripples[i].pos + ripples[i].state + layer->nrOfLights) % layer->nrOfLights,
blend(layerP.color2, ColorFromPalette(layerP.palette, ripples[i].color), brightness / ripples[i].state * 2));
layer->setRGB((ripples[i].pos - ripples[i].state + layer->nrOfLights) % layer->nrOfLights,
blend(layerP.color2, ColorFromPalette(layerP.palette, ripples[i].color), brightness / ripples[i].state * 2));
ripples[i].state++;
break;
}
}
}
};
class RocktavesEffect : public Node {
public:
static const char* name() { return "Rocktaves"; }
static uint8_t dim() { return _1D; }
static const char* tags() { return "🎵"; }
void setup() override {
}
void loop() override {
layer->fadeToBlackBy(16);
float frTemp = sharedData.majorPeak;
uint8_t octCount = 0;
uint8_t volTemp = 0;
float myMagnitude = sharedData.volume / 16.0f;
volTemp = 32.0f + myMagnitude * 1.5f;
if (myMagnitude < 48) volTemp = 0;
if (myMagnitude > 144) volTemp = 255;
while (frTemp > 249) {
octCount++;
frTemp = frTemp / 2;
}
frTemp -= 132;
frTemp = fabsf(frTemp * 2.1f);
uint16_t i = ::map(beatsin8(8 + octCount * 4, 0, 255, 0, octCount * 8), 0, 255, 0, layer->nrOfLights - 1);
i = constrain(i, 0, layer->nrOfLights - 1);
layer->addRGB(i, blend(layerP.color2, ColorFromPalette(layerP.palette, (uint8_t)frTemp), volTemp));
}
};
class WaterfallEffect : public Node {
public:
static const char* name() { return "Waterfall"; }
static uint8_t dim() { return _1D; }
static const char* tags() { return "🎵"; }
uint8_t speed = 128;
uint8_t intensity = 128;
void setup() override {
addControl(speed, "speed", "slider");
addControl(intensity, "intensity", "slider");
}
uint8_t secondHand = 0;
void loop() override {
uint8_t newSecondHand = micros() / (256 - speed) / 500 + 1 % 16;
if ((speed > 254) || (secondHand != newSecondHand)) {
secondHand = newSecondHand;
float myMajorPeak = MAX(sharedData.majorPeak, 1.0f);
float myMagnitude = sharedData.volume / 8.0f;
uint8_t pixCol = (log10f(myMajorPeak) - 2.26f) * 150;
if (myMajorPeak < 182.0f) pixCol = 0;
for (int i = 0; i < layer->nrOfLights - 1; i++)
layer->setRGB(i, layer->getRGB(i + 1));
bool peak = sharedData.volume > 128.0f;
if (peak) {
layer->setRGB(layer->nrOfLights - 1, CHSV(92, 92, 92));
} else {
CRGB color = ColorFromPalette(layerP.palette, pixCol + intensity);
layer->setRGB(layer->nrOfLights - 1, blend(layerP.color2, color, (int)myMagnitude));
}
}
}
};Notes
All effects are ready to add to 🧠 Learnings used |
- Emoji tweaks - New effects: BlinkRainbow, Metor, Oscillate, PhasedNoise backend ======= - palettes: add singlecolors
Collaborator
Author
|
@coderabbitai review commit a2c47a4 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 24483 🏁 Script executed: Length of output: 28065
✅ Strengths1. New Effects Implementation (4 effects added)All four effects (BlinkRainbow, Meteor, Oscillate, PhasedNoise) are properly implemented with:
2. Audio Palette Index Fix ⭐- if (palette >= 10 && palette <= 12) { // Audio palettes
+ if (palette >= audioPaletteIndex && palette <= audioPaletteIndex + 2) {Perfect fix! The new single-color palettes shifted audio palette indices from 10-12 to 18-20. Using the constant prevents future breakage. 3. DJLightEffect Bug Fix 🐛→✅+ CRGB fadedColor = color;
+ fadedColor.fadeToBlackBy(fadeVal);
for (int x = 0; x < layer->size.x; x++) {
for (int z = 0; z < layer->size.z; z++) {
- layer->setRGB(Coord3D(x, mid, z), color.fadeToBlackBy(fadeVal));
+ layer->setRGB(Coord3D(x, mid, z), fadedColor);Excellent fix! 4. New Single-Color Palettes (8 added)Great addition for users who want solid colors. Orange uses 5. Emoji CleanupRemoved 🐺 from moving heads and standardized tags. Sandbox examples now use 🆕 instead of ⏳.
|
| Category | Changes |
|---|---|
| New Effects | BlinkRainbow, Meteor, Oscillate, PhasedNoise |
| New Palettes | Red, Green, Blue, Orange, Purple, Cyan, Warm/Cold White |
| Bug Fixes | DJLight fadeToBlackBy, Audio palette index |
| Refactoring | Emoji standardization, improved comments |
| Files Changed | 13 files (+377/-135 lines) |
🔧 Recommended Fix
Update OscillateEffect color selection:
for (int j = 0; j < 3; j++) {
if (i >= oscillators[j].pos - oscillators[j].size && i <= oscillators[j].pos + oscillators[j].size) {
- CRGB newColor = ColorFromPalette(layerP.palette, j * 85);
+ CRGB newColor = (j == 0) ? layerP.color1 : (j == 1) ? layerP.color2 : layerP.color3;
color = (color == CRGB::Black) ? newColor : blend(color, newColor, 128);
}
}This will use the same distinct segment colors as WLED's original implementation.
Overall: Solid commit with important fixes and great additions! 🎉 The audio palette index fix and DJLight bug fix are particularly valuable. Just need to fix OscillateEffect's color selection for proper visibility.
🧠 Learnings used
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-02-05T13:20:46.001Z
Learning: In MoonLight with FastLED 3.11 on ESP-IDF 5.x+ (Arduino core 3.x): FastLED defaults to RMT5 driver (wraps Espressif's led_strip component), which has known ISR handler lifecycle issues causing repeated "Handler detached" messages every ~500ms and severe performance degradation (e.g., 2 FPS). RMT5 is more prone to Wi-Fi/interrupt jitter than legacy RMT4. Solution: force legacy RMT4 by adding `-D FASTLED_RMT5=0` to build flags in platformio.ini, or switch to FastLED's parallel I/O drivers which don't use RMT. RMT4 and RMT5 cannot coexist in the same binary.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-18T12:49:07.297Z
Learning: In MoonLight channel reordering architecture: Keep rgbwBufferMapping in driver task (not effect task) because: (1) Driver has spare CPU cycles waiting for DMA/I2S/Art-Net timing; effect task is CPU-bound and starvation-prone. (2) Contiguous RGB writes in effects are cache-friendly; scattered writes at final offsets are cache-hostile for non-RGB orders (GRB, BGR, etc.). (3) forEachLightIndex already handles sparse mapping iteration; moving reordering to effects adds complexity without benefit since drivers must send full frames anyway (can't skip iteration). (4) rgbwBufferMapping does brightness/gamma LUT + white extraction + reordering—moving just reordering creates code duplication. (5) Effects should express color intent (CRGB), not hardware details (offsetRed/Green/Blue). Driver-side transformation maintains separation of concerns and task load balance for scaling to 98K+ pixels.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 121
File: src/MoonLight/Nodes/Drivers/D_FastLED.h:176-176
Timestamp: 2026-02-08T20:43:12.857Z
Learning: In MoonLight FastLED 3.11 driver (src/MoonLight/Nodes/Drivers/D_FastLED.h): Affinity is applied per-channel via fl::ChannelOptions.mAffinity passed to fl::ChannelConfig (not via FastLED.setExclusiveDriver). Keeping setExclusiveDriver commented out leaves all drivers available; FastLED then selects the appropriate driver for each channel based on the mAffinity preference ("RMT", "I2S", "SPI", "PARLIO", or "" for auto). This allows per-channel driver preferences rather than a global restriction.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-18T17:07:33.344Z
Learning: FastLED library does not support direct 4-byte-per-pixel (CRGBW) buffers. It always expects CRGB (3-byte) arrays and internally generates the W channel from RGB using .setRgbw() API. Casting a 4-byte RGBW buffer to CRGB* causes misalignment (W becomes next pixel's R). For MoonLight's FastLED driver with channelsPerLight=4: must either (1) repack RGB in-place before FastLED.show() then restore after, (2) use driver-specific channelsPerLight=3 layout, or (3) allocate temporary RGB-only buffer. Option 1 (in-place repacking) is best for ESP32-D0 memory constraints.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 84
File: src/MoonLight/Modules/ModuleLightsControl.h:367-379
Timestamp: 2025-12-28T10:28:24.157Z
Learning: In MoonLight PR `#84`: wrapping effectTask's layerP.loop() with monitorMutex while reading channelsE for monitor emission is thread-safe and prevents partial frames, but introduces serialization between effects rendering and monitor emission. Reading from channelsD (driver/front buffer) instead—with monitorMutex held only during the brief pointer swap—is superior because it maintains full parallelism (effects ║ monitor ║ driver), shows actual LED output, and minimizes mutex hold time.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-06T13:45:03.067Z
Learning: For MoonLight on ESP32 at extreme loads (e.g., 49K channels @ 100 fps): Task watchdog can fire when AppDrivers/AppEffects process many nodes without yielding. Even with vTaskDelay(1) at loop end, the node iteration for-loops (PhysicalLayer::loopDrivers line 108-111, PhysicalLayer::loop line 73-75) can take >5 seconds without yielding, starving IDLE task (priority 0). Solution: add taskYIELD() or vTaskDelay(1) inside the node/layer iteration loops to give IDLE task chances to run and reset watchdog. Lowering task priority won't help since IDLE is already at 0 (lowest).
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-15T12:38:33.598Z
Learning: In MoonLight GameOfLifeEffect (src/MoonLight/Nodes/Effects/E_MoonModules.h): The effect's nested loops (especially lines 257-323) iterate through all cells (e.g., 128×128 = 16,384) with nested neighbor checks (18 per cell = 294,912 operations) without yielding. This starves effectTask (priority 3) when higher-priority tasks like HTTP server (priority 5) run, causing the effect to disappear. UI actions (save, refresh) temporarily show one frame because HTTP completes and briefly releases CPU back to effectTask. Solution: Add taskYIELD() inside outer X loop to prevent task starvation. This is the same root cause as issue `#102` (Starsky glitches with many driver nodes).
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-25T13:34:40.097Z
Learning: For the MoonLight LED system: Effects task can run on Core 0 (PRO_CPU) since it only performs computation (calculating colors, writing to channels array) without timing-critical hardware interaction. WiFi preemption is acceptable for pure computation tasks. Only the driver task (using DMA, I2S, LCD, PARLIO) requires Core 1 (APP_CPU) for uninterrupted timing-critical hardware operations.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:212-221
Timestamp: 2026-01-06T15:24:29.770Z
Learning: In VirtualLayer::getLight (src/MoonLight/Layers/VirtualLayer.cpp), bounds checking for unmapped lights (the else branch around lines 212-221) is intentionally disabled for performance when driving 100K+ LEDs. The design places responsibility on effect developers to stay within maxChannels. This is an explicit performance vs safety tradeoff for extreme LED counts.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-15T15:13:55.631Z
Learning: In MoonLight effectTask (src/main.cpp): The memcpy(channelsE, channelsD) at line 137 happens AFTER releasing swapMutex, allowing driverTask to swap the channelsD/channelsE pointers mid-copy, causing buffer corruption. This specifically breaks effects like GameOfLife that use getRGB() to read previous-frame data for blending decisions. Other effects work because they only read current-frame data they just wrote. Fix: hold swapMutex during the entire memcpy operation to prevent pointer swap during copy.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-26T14:59:22.091Z
Learning: In MoonModules/MoonLight PR `#114`: ModuleDevices deadlock caused by: (1) StatefulService.h line 204-205 bug - after 500ms mutex timeout, continues WITHOUT acquiring mutex, causing race conditions and JSON corruption. (2) receiveUDP() while-loop processes own UDP broadcasts via loopback, creating circular call chain: sendUDP→broadcast→receiveUDP→updateDevices→update→handlers→sendUDP. (3) updateDevices() holds _accessMutex during expensive sort/JSON operations with 15 devices, can exceed 500ms with nested calls. Solution: Fix StatefulService to wait forever or reboot on timeout; skip own broadcasts by checking message.name == ownHostname; optimize updateDevices to sort outside mutex.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 104
File: src/MoonLight/Layers/PhysicalLayer.cpp:145-147
Timestamp: 2026-01-13T15:18:48.740Z
Learning: In MoonLight's PhysicalLayer and Node architecture (src/MoonLight/Layers/PhysicalLayer.cpp, src/MoonBase/Nodes.h): effectsMutex, driversMutex, and layerMutex are mandatory for system operation. These mutexes are validated at creation time (constructor checks log errors if xSemaphoreCreateMutex returns nullptr). Runtime null checks on these mutex pointers are intentionally omitted—the design treats successful mutex allocation as an invariant after initialization, placing responsibility on the programmer to ensure proper allocation. This is a performance-oriented "fail-fast at boot" approach rather than defensive runtime checking.
<!-- </add_learning>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/MoonLight/Nodes/Effects/E_MovingHeads.h (1)
77-77:⚠️ Potential issue | 🟡 Minor
addControl(range, "slider", "slider")— wrong controlnameargument.Both
Troy1MoveEffect::setup()(Line 77) andTroy2MoveEffect::setup()(Line 194) pass"slider"as the control's name, which should be"range". This means the persisted config key will be"slider"rather than"range", colliding with the type string and making therangecontrol effectively unrestorable from saved state.🐛 Proposed fix (applies to both lines 77 and 194)
- addControl(range, "slider", "slider"); + addControl(range, "range", "slider");Also applies to: 194-194
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Effects/E_MovingHeads.h` at line 77, In both Troy1MoveEffect::setup() and Troy2MoveEffect::setup() the addControl call uses the wrong control name ("slider"); change the second argument from "slider" to "range" so the call becomes addControl(range, "range", "slider") (preserving the control type "slider") — this ensures the persisted config key matches the control variable name `range` and avoids collisions with the type string.src/MoonLight/Nodes/Drivers/D_ArtnetIn.h (1)
33-33:⚠️ Potential issue | 🟡 MinorPort upper bound should be 65535, not 65538.
UDP port numbers are 16-bit (0–65535). The current max of 65538 allows invalid port values;
portisuint16_tso values above 65535 would silently wrap.Proposed fix
- addControl(port, "port", "number", 0, 65538); + addControl(port, "port", "number", 0, 65535);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Drivers/D_ArtnetIn.h` at line 33, The port control's upper bound is incorrect (65538) which permits invalid UDP port values; update the call to addControl for the "port" control in D_ArtnetIn (the line with addControl(port, "port", "number", 0, 65538)) to use 65535 as the max and ensure any related validation treats the value as uint16_t so values >65535 are rejected or clamped.
🧹 Nitpick comments (6)
src/MoonLight/Nodes/Effects/E_MovingHeads.h (1)
56-56: Dead code:closestColorIndex,colorwheelpalette, andfindClosestColorWheelIndex/colorDistanceare never called.
Troy1MoveEffect:closestColorIndex(Line 56),colorwheelpalette(Lines 58–67), andfindClosestColorWheelIndex/colorDistance(Lines 83–99) are declared/defined but never invoked inloop().Troy2MoveEffect:colorDistanceandfindClosestColorWheelIndex(Lines 200–214) duplicate the same unused logic.Consider removing these if the feature is not planned, or adding a
// TODOcomment if the palette-snapping feature is intended for a future control.Also applies to: 86-99, 200-214
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Effects/E_MovingHeads.h` at line 56, Remove or annotate the unused palette-snapping code: either delete the unused variables closestColorIndex and colorwheelpalette and the helper functions findClosestColorWheelIndex and colorDistance in the Troy1MoveEffect and Troy2MoveEffect implementations (they appear in the E_MovingHeads.h diff around the Troy1MoveEffect and Troy2MoveEffect blocks) or, if you intend to implement palette snapping later, add a clear TODO comment above each unused declaration (closestColorIndex, colorwheelpalette, findClosestColorWheelIndex, colorDistance) explaining the planned use; ensure no other code references these symbols before removal.src/MoonLight/Nodes/Drivers/D_ArtnetIn.h (1)
38-44: Loop variablelayershadows the member fieldlayer.The range-for variable
VirtualLayer* layeron line 39 shadows the memberuint8_t layerdeclared on line 26. The loop variable is effectively unused (onlyiis referenced). Rename it to avoid confusion.Proposed fix
uint8_t i = 1; // start with one - for (VirtualLayer* layer : layerP.layers) { + for (VirtualLayer* vLayer : layerP.layers) { Char<32> layerName; layerName.format("Layer %d", i); addControlValue(layerName.c_str()); i++; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Drivers/D_ArtnetIn.h` around lines 38 - 44, The loop variable VirtualLayer* layer in the range-for over layerP.layers shadows the member named layer; rename the loop variable (e.g., to VirtualLayer* vl or VirtualLayer* iterLayer) and update uses (layerName.format, addControlValue, i++) to use the new name so there is no shadowing and intent is clear; keep the rest of the loop logic (uint8_t i = 1, layerName, addControlValue) unchanged.src/MoonLight/Nodes/Drivers/D_AudioSync.h (1)
17-17:#defineleaks into every translation unit — prefer aconstexpr
#define audioPaletteIndex 18is global to the preprocessor and can clash with any identically named symbol in included code. A typedconstexprconstant scoped to the class or an anonymous namespace is safer and participates in type checking.♻️ Proposed change
- `#define` audioPaletteIndex 18 // see palettes.h + static constexpr uint8_t audioPaletteIndex = 18; // see palettes.hPlace it inside the
AudioSyncDriverclass body, or in an anonymous namespace if it needs to remain file-scoped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Drivers/D_AudioSync.h` at line 17, Replace the global preprocessor macro audioPaletteIndex with a typed constexpr to avoid leaking into other translation units: remove the `#define` and add a scoped constant (e.g., inside the AudioSyncDriver class as static constexpr int audioPaletteIndex = 18; or in an anonymous namespace as constexpr int audioPaletteIndex = 18;) and update any references to use that symbol; ensure the declaration's type matches usage and delete the original macro.src/MoonLight/Modules/palettes.h (1)
538-560: Consider astatic_assertto keeppalette_namesandgGradientPalettesin syncThe two arrays are currently both 79 entries (correct), but there is no compile-time guard. A future addition to one without the other would produce wrong UI names or an out-of-bounds index silently.
♻️ Proposed addition (after both arrays are closed)
+static_assert(sizeof(gGradientPalettes) / sizeof(gGradientPalettes[0]) == + sizeof(palette_names) / sizeof(palette_names[0]), + "gGradientPalettes and palette_names must have the same number of entries");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Modules/palettes.h` around lines 538 - 560, The arrays palette_names and gGradientPalettes can drift out of sync; add a compile-time guard by inserting a static_assert after both arrays are defined that compares their element counts (e.g. static_assert((sizeof(palette_names)/sizeof(palette_names[0])) == (sizeof(gGradientPalettes)/sizeof(gGradientPalettes[0])), "palette_names and gGradientPalettes must have the same number of entries"); this ensures any future change to one without the other fails to compile and points to palette_names and gGradientPalettes as the mismatch.src/MoonLight/Nodes/Effects/E_MoonLight.h (1)
1812-1812: Empty destructor is unnecessary.
~RadarEffect()has no allocated resources to free. The comment says "e.g. to free allocated memory" but there's nothing to free. Consider removing it to reduce noise — the compiler-generated destructor suffices.Proposed fix
- ~RadarEffect() override {}; // e,g, to free allocated memory🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Effects/E_MoonLight.h` at line 1812, The explicit empty destructor ~RadarEffect() in class RadarEffect is unnecessary since there are no resources to free; remove the destructor declaration (~RadarEffect() override {}; ) so the compiler-generated destructor is used instead, and update any related comments (remove the "e.g. to free allocated memory" note) to avoid misleading callers and reduce noise.src/MoonLight/Modules/ModuleEffects.h (1)
148-151: New WLED effects break the stated alphabetical ordering.The comment on line 120 says "WLED effects, alphabetically", but BlinkRainbowEffect, MeteorEffect, OscillateEffect, and PhasedNoiseEffect are appended after WaverlyEffect instead of being inserted in their correct alphabetical positions. The same applies to lines 250–253 in
addNode. For consistency with the existing convention:
BlinkRainbowEffect→ afterBlackholeEffectMeteorEffect→ afterLissajousEffectOscillateEffect→ afterNoise2DEffectPhasedNoiseEffect→ afterPacManEffect🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Modules/ModuleEffects.h` around lines 148 - 151, The WLED effects were appended out of alphabetical order; reorder the addControlValue calls so BlinkRainbowEffect, MeteorEffect, OscillateEffect, and PhasedNoiseEffect are inserted at their correct alphabetical positions: move getNameAndTags<BlinkRainbowEffect>() to immediately after the addControlValue for BlackholeEffect, move getNameAndTags<MeteorEffect>() to immediately after LissajousEffect, move getNameAndTags<OscillateEffect>() to immediately after Noise2DEffect, and move getNameAndTags<PhasedNoiseEffect>() to immediately after PacManEffect; make the same corresponding reordering for the addNode block (the calls around lines ~250–253) so both addControlValue and addNode lists remain alphabetically ordered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/MoonLight/Modules/palettes.h`:
- Around line 609-610: The green palette uses CRGB::Green which is CRGB(0,128,0)
and renders half brightness; update the case 3 branch that assigns palette[i]
(the loop in the switch/case for case 3) to use a full-intensity green
(CRGB(0,255,0)) instead of CRGB::Green so the Green palette matches the
brightness of the other palettes.
- Around line 595-632: The switch over gpArray[2] that fills the palette[16]
entries (in the MoonLight sub-palette) lacks a default:, so out-of-range values
leave palette uninitialized; add a default: branch at the end of that switch
which fills palette[i] for i=0..15 with a safe fallback color (e.g., CRGB::White
or a neutral CRGB(0,0,0)) or re-use an existing case (such as MoonLight case 0)
to ensure palette is always fully initialized; update the switch containing
cases 0–9 to include this default branch so callers reading palette cannot
receive indeterminate data.
In `@src/MoonLight/Nodes/Drivers/D_ArtnetIn.h`:
- Around line 135-141: Add a runtime bounds check before indexing layerP.layers:
ensure layer > 0 and (size_t)(layer - 1) < layerP.layerCount (or layers.size())
and that layerP.layers[layer - 1] is non-null before calling forEachLightIndex
or any dereference. Apply the same guard in both handleArtNet (where the shown
memcpy/forEachLightIndex occurs) and handleDDP (lines referenced in the review)
to skip virtual-layer handling when the layer value is invalid to prevent
out‑of‑bounds access.
In `@src/MoonLight/Nodes/Effects/E_WLED.h`:
- Line 2765: The modVal calculation in PhasedNoiseEffect uses inoise8(i * 10 + i
* 10) which collapses to inoise8(i * 20) (1D noise); change this to the 2D call
used in WLED by passing two coordinates (e.g., inoise8(i * 10, i * 10)) so
modVal uses 2D Perlin noise for richer spatial variation; update the expression
where modVal is assigned (search for "modVal" and the call inoise8(i * 10 + i *
10) in PhasedNoiseEffect) to use the comma-separated args.
- Around line 2674-2679: The Oscillator.size field is too small (int8_t) and
overflows for large fixtures; update the Oscillator struct to use int16_t for
size (leave pos, dir, oscSpeed unchanged), update any assignments in loop()
(e.g., oscillators[i].size = layer->nrOfLights / (3 + intensity / 8)) and
onSizeChanged to cast/assign to int16_t instead of int8_t so large
layer->nrOfLights values do not wrap and the render range check in loop() works
correctly.
- Around line 2580-2583: BlinkRainbowEffect's color advance uses a millisecond
point-equality check (if ((timebase % dutyCycle) == 0)) which misses most cycle
boundaries because the loop runs every 25–40ms; change to detect cycle rollover
by adding a class member (e.g., int lastCycle initialized to -1 in the
BlinkRainbowEffect constructor) and in loop() compute currentCycle = timebase /
dutyCycle (integer division) and if (currentCycle != lastCycle) then increment
colorIndex by 32 and set lastCycle = currentCycle so the color advances exactly
once per dutyCycle rollover.
---
Outside diff comments:
In `@src/MoonLight/Nodes/Drivers/D_ArtnetIn.h`:
- Line 33: The port control's upper bound is incorrect (65538) which permits
invalid UDP port values; update the call to addControl for the "port" control in
D_ArtnetIn (the line with addControl(port, "port", "number", 0, 65538)) to use
65535 as the max and ensure any related validation treats the value as uint16_t
so values >65535 are rejected or clamped.
In `@src/MoonLight/Nodes/Effects/E_MovingHeads.h`:
- Line 77: In both Troy1MoveEffect::setup() and Troy2MoveEffect::setup() the
addControl call uses the wrong control name ("slider"); change the second
argument from "slider" to "range" so the call becomes addControl(range, "range",
"slider") (preserving the control type "slider") — this ensures the persisted
config key matches the control variable name `range` and avoids collisions with
the type string.
---
Duplicate comments:
In `@src/MoonLight/Nodes/Effects/E_MoonLight.h`:
- Around line 1748-1813: The RadarEffect::loop() uses millis() for timing which
is inconsistent with the rest of this file's platform abstraction; change the
timing call in RadarEffect::loop() to use pal::millis() (i.e., replace millis()
with pal::millis() where cycleMs and physPos are computed) so the effect uses
the same PAL timing API as other effects.
In `@src/MoonLight/Nodes/Effects/E_WLED.h`:
- Around line 1978-1987: The color.fadeToBlackBy(...) call was mutating `color`
each iteration; compute the faded color once and reuse it: create `CRGB
fadedColor = color;` then call `fadedColor.fadeToBlackBy(fadeVal);` before the
outer x/z loops and use `fadedColor` in the inner body (where
`layer->setRGB(Coord3D(x, mid, z), fadedColor)` is called) while leaving the
rest of the column-shift logic (`for` loops using `layer->getRGB` and
`layer->setRGB`) unchanged so every column gets the same faded value instead of
progressively darker values.
---
Nitpick comments:
In `@src/MoonLight/Modules/ModuleEffects.h`:
- Around line 148-151: The WLED effects were appended out of alphabetical order;
reorder the addControlValue calls so BlinkRainbowEffect, MeteorEffect,
OscillateEffect, and PhasedNoiseEffect are inserted at their correct
alphabetical positions: move getNameAndTags<BlinkRainbowEffect>() to immediately
after the addControlValue for BlackholeEffect, move
getNameAndTags<MeteorEffect>() to immediately after LissajousEffect, move
getNameAndTags<OscillateEffect>() to immediately after Noise2DEffect, and move
getNameAndTags<PhasedNoiseEffect>() to immediately after PacManEffect; make the
same corresponding reordering for the addNode block (the calls around lines
~250–253) so both addControlValue and addNode lists remain alphabetically
ordered.
In `@src/MoonLight/Modules/palettes.h`:
- Around line 538-560: The arrays palette_names and gGradientPalettes can drift
out of sync; add a compile-time guard by inserting a static_assert after both
arrays are defined that compares their element counts (e.g.
static_assert((sizeof(palette_names)/sizeof(palette_names[0])) ==
(sizeof(gGradientPalettes)/sizeof(gGradientPalettes[0])), "palette_names and
gGradientPalettes must have the same number of entries"); this ensures any
future change to one without the other fails to compile and points to
palette_names and gGradientPalettes as the mismatch.
In `@src/MoonLight/Nodes/Drivers/D_ArtnetIn.h`:
- Around line 38-44: The loop variable VirtualLayer* layer in the range-for over
layerP.layers shadows the member named layer; rename the loop variable (e.g., to
VirtualLayer* vl or VirtualLayer* iterLayer) and update uses (layerName.format,
addControlValue, i++) to use the new name so there is no shadowing and intent is
clear; keep the rest of the loop logic (uint8_t i = 1, layerName,
addControlValue) unchanged.
In `@src/MoonLight/Nodes/Drivers/D_AudioSync.h`:
- Line 17: Replace the global preprocessor macro audioPaletteIndex with a typed
constexpr to avoid leaking into other translation units: remove the `#define` and
add a scoped constant (e.g., inside the AudioSyncDriver class as static
constexpr int audioPaletteIndex = 18; or in an anonymous namespace as constexpr
int audioPaletteIndex = 18;) and update any references to use that symbol;
ensure the declaration's type matches usage and delete the original macro.
In `@src/MoonLight/Nodes/Effects/E_MoonLight.h`:
- Line 1812: The explicit empty destructor ~RadarEffect() in class RadarEffect
is unnecessary since there are no resources to free; remove the destructor
declaration (~RadarEffect() override {}; ) so the compiler-generated destructor
is used instead, and update any related comments (remove the "e.g. to free
allocated memory" note) to avoid misleading callers and reduce noise.
In `@src/MoonLight/Nodes/Effects/E_MovingHeads.h`:
- Line 56: Remove or annotate the unused palette-snapping code: either delete
the unused variables closestColorIndex and colorwheelpalette and the helper
functions findClosestColorWheelIndex and colorDistance in the Troy1MoveEffect
and Troy2MoveEffect implementations (they appear in the E_MovingHeads.h diff
around the Troy1MoveEffect and Troy2MoveEffect blocks) or, if you intend to
implement palette snapping later, add a clear TODO comment above each unused
declaration (closestColorIndex, colorwheelpalette, findClosestColorWheelIndex,
colorDistance) explaining the planned use; ensure no other code references these
symbols before removal.
Collaborator
Author
|
@coderabbitai, regarding
Please make a list of the new effects using And another list using I add and order alphabetically myself |
|
Tip For best results, initiate chat on the files or code changes.
List 1: For
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/MoonLight/Nodes/Effects/E_MovingHeads.h (1)
56-99: Remove dead commented-out code; rely on version control for history.~44 lines of commented-out code here (
closestColorIndex,colorwheelpalette,colorDistance,findClosestColorWheelIndex) and a similar ~16-line block inTroy2MoveEffect(lines 200–215). If this functionality is being retired, delete it outright—git preserves history. If re-enablement is planned, a brief// TODO:explaining the intent would be more informative than dormant code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Effects/E_MovingHeads.h` around lines 56 - 99, The file contains large blocks of dead commented-out code (closestColorIndex, colorwheelpalette, colorDistance, findClosestColorWheelIndex and the similar block in Troy2MoveEffect) that should be removed to keep the source clean; either delete those commented sections outright (relying on git for history) or replace each block with a single concise TODO comment referencing the feature (e.g., "TODO: restore color wheel matching if reintroduced") and remove all commented lines for the functions/variables named above to avoid clutter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/MoonLight/Modules/palettes.h`:
- Around line 624-626: The Purple palette entry (case 6) uses CRGB::Purple which
is half-brightness (128,0,128) and thus reads dim compared to full-intensity
colors; update the assignment in the case 6 block that sets palette[i] to use a
full-intensity purple/magenta value instead (e.g., CRGB(128,0,255) for a
perceptual violet or CRGB(255,0,255) for magenta) for all 16 entries so Purple
matches the brightness of Red/Blue/Cyan—mirror the same approach you applied
earlier when replacing CRGB::Green with CRGB(0,255,0).
In `@src/MoonLight/Nodes/Effects/E_MovingHeads.h`:
- Line 77: The range slider (added via addControl(range, "range", "slider")) can
be 0–255 which causes uint8_t wraparound when used as pan - range / pan + range
in beatsin8 calls; fix by preventing unsigned under/overflow before calling
beatsin8: in MovingHeadsEffect (where pan and range are used around lines
111–123), Troy2MoveEffect and WowiMoveEffect, compute safe int32_t low =
(int)pan - (int)range; if (low < 0) low = 0; int32_t high = (int)pan +
(int)range; if (high > 255) high = 255; then pass (uint8_t)low and (uint8_t)high
to beatsin8 (ensuring lowest <= highest), or alternatively constrain the control
itself by setting addControl(range, "range", "slider") to a smaller max; apply
the same clamping logic to the beatsin8 call sites in Troy2MoveEffect and
WowiMoveEffect.
In `@src/MoonLight/Nodes/Effects/E_WLED.h`:
- Around line 3552-3571: The expression computing newSecondHand in loop() has
wrong operator precedence so % 16 applies only to 1; change the calculation of
newSecondHand (in E_WLED::loop(), variable newSecondHand) to apply the modulo to
the whole time-based value — i.e. parenthesize the time/division and +1 before
%16 so the result wraps 0–15 (use the form: ((micros() / (256 - speed) / 500) +
1) % 16).
- Around line 3297-3312: The computation for newSecondHand in loop() has
incorrect operator precedence so % 16 applies only to the literal 1; fix it by
parenthesizing the full time expression before applying % 16 (i.e. compute
(micros() / (256 - speed) / 500 + 1) % 16) so newSecondHand wraps 0–15 as
intended, leaving types (uint8_t newSecondHand, secondHand, speed) unchanged.
---
Duplicate comments:
In `@src/MoonLight/Nodes/Drivers/D_ArtnetIn.h`:
- Line 138: Both handleArtNet and handleDDP currently dereference
layerP.layers[layer - 1] without bounds checks; add a guard that validates layer
is >= 1 and (size_t)(layer - 1) < layerP.layers.size() before using
layerP.layers[layer - 1], and early-return or continue when the check fails.
Update the code around the lambda calls (the memcpy loop invoking
forEachLightIndex on layerP.layers[layer - 1]) to only run after this
validation; ensure you use size_t casting to avoid signed/unsigned comparison
issues and keep behavior consistent in both handleArtNet and handleDDP.
---
Nitpick comments:
In `@src/MoonLight/Nodes/Effects/E_MovingHeads.h`:
- Around line 56-99: The file contains large blocks of dead commented-out code
(closestColorIndex, colorwheelpalette, colorDistance, findClosestColorWheelIndex
and the similar block in Troy2MoveEffect) that should be removed to keep the
source clean; either delete those commented sections outright (relying on git
for history) or replace each block with a single concise TODO comment
referencing the feature (e.g., "TODO: restore color wheel matching if
reintroduced") and remove all commented lines for the functions/variables named
above to avoid clutter.
| addControl(colorwheelbrightness, "colorwheelbrightness", "slider"); // 0-7 for 8 colors in the colorwheel | ||
| addControl(autoMove, "autoMove", "checkbox"); | ||
| addControl(range, "slider", "slider"); | ||
| addControl(range, "range", "slider"); |
There was a problem hiding this comment.
range slider allows values that cause uint8_t overflow in beatsin8 calls.
addControl(range, "range", "slider") defaults to 0–255. The range value is used unsafely in expressions like pan - range and pan + range (lines 111–112, 122–123) where both operands are uint8_t. When range exceeds pan (or 255 - pan), unsigned wrap-around makes beatsin8's lowest > highest, producing erratic oscillation.
The same pattern exists in Troy2MoveEffect (line 194 → used at lines 229–230, 241–242) and WowiMoveEffect (line 311 → used at lines 317–318).
Consider constraining range or clamping at the call site:
Option A: Constrain the control max
- addControl(range, "range", "slider");
+ addControl(range, "range", "slider", 0, 127);Apply similarly at lines 194 and 311.
Option B: Clamp at the call sites
- uint8_t mypan = beatsin8(bpm, pan - range, pan + range, 0, sharedData.bands[0] / 2);
- uint8_t mytilt = beatsin8(bpm, tilt - range, tilt + range, 0, sharedData.bands[0] / 2);
+ uint8_t lo_pan = (range > pan) ? 0 : pan - range;
+ uint8_t hi_pan = (range > 255 - pan) ? 255 : pan + range;
+ uint8_t lo_tilt = (range > tilt) ? 0 : tilt - range;
+ uint8_t hi_tilt = (range > 255 - tilt) ? 255 : tilt + range;
+ uint8_t mypan = beatsin8(bpm, lo_pan, hi_pan, 0, sharedData.bands[0] / 2);
+ uint8_t mytilt = beatsin8(bpm, lo_tilt, hi_tilt, 0, sharedData.bands[0] / 2);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| addControl(range, "range", "slider"); | |
| addControl(range, "range", "slider", 0, 127); |
Suggested change
| addControl(range, "range", "slider"); | |
| uint8_t lo_pan = (range > pan) ? 0 : pan - range; | |
| uint8_t hi_pan = (range > 255 - pan) ? 255 : pan + range; | |
| uint8_t lo_tilt = (range > tilt) ? 0 : tilt - range; | |
| uint8_t hi_tilt = (range > 255 - tilt) ? 255 : tilt + range; | |
| uint8_t mypan = beatsin8(bpm, lo_pan, hi_pan, 0, sharedData.bands[0] / 2); | |
| uint8_t mytilt = beatsin8(bpm, lo_tilt, hi_tilt, 0, sharedData.bands[0] / 2); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MoonLight/Nodes/Effects/E_MovingHeads.h` at line 77, The range slider
(added via addControl(range, "range", "slider")) can be 0–255 which causes
uint8_t wraparound when used as pan - range / pan + range in beatsin8 calls; fix
by preventing unsigned under/overflow before calling beatsin8: in
MovingHeadsEffect (where pan and range are used around lines 111–123),
Troy2MoveEffect and WowiMoveEffect, compute safe int32_t low = (int)pan -
(int)range; if (low < 0) low = 0; int32_t high = (int)pan + (int)range; if (high
> 255) high = 255; then pass (uint8_t)low and (uint8_t)high to beatsin8
(ensuring lowest <= highest), or alternatively constrain the control itself by
setting addControl(range, "range", "slider") to a smaller max; apply the same
clamping logic to the beatsin8 call sites in Troy2MoveEffect and WowiMoveEffect.
Collaborator
Author
|
@coderabbitai, in M_MoonLight.h I have the CircleModifier, which is a migration of WLED_MM code which does drawArc(vW/2, vH/2, i/2, col); for each pixel WLED_MM also has Block: int centerX = (vW+1)/2 - 1; Can you create a class for M_MoonLight.h called BlockModifier which makes a block from a 1D effect? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 16156
// Takes the y dimension from the layout (1D effect) and turn it into concentric square blocks in 2D.
class BlockModifier : public Node {
public:
static const char* name() { return "Block"; }
static uint8_t dim() { return _2D; } // 1D to 2D
static const char* tags() { return "💎"; }
Coord3D modifierSize;
bool hasModifier() const override { return true; }
void modifySize() override {
modifierSize = layer->size;
modifyPosition(layer->size); // modify the virtual size as x, 0, 0
// change the size to be one bigger in each dimension
layer->size.x++;
layer->size.y++;
layer->size.z++;
}
void modifyPosition(Coord3D& position) override {
// Calculate the distance from center using Chebyshev distance (max of abs differences)
int centerX = (modifierSize.x + 1) / 2 - 1;
int centerY = (modifierSize.y + 1) / 2 - 1;
int dx = abs((int)position.x - centerX);
int dy = abs((int)position.y - centerY);
// Block distance is the maximum of the two deltas (creates square rings)
int distance = MAX(dx, dy);
position.x = 0;
position.y = distance;
position.z = 0;
}
};Key differences from CircleModifier:
How it works:
Example: With a 7×7 grid, the block distances look like: This matches the WLED_MM Block behavior where each pixel of a 1D effect expands as a square outline from the center! 🎨 🧠 Learnings used |
backend ======= - effect moves, renames - palette purple fix - Add block modifier
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/MoonLight/Modules/palettes.h (1)
595-597: FastLEDdefaultreturnsRainbowStripeColors_p; MoonLightdefaultreturnsCRGB::Black— inconsistent fallbacks.Both defaults are unreachable in practice (the encoding strictly determines
gpArray[2]), but the two branches handle the same "unknown sentinel" scenario differently. Consider aligning them.♻️ Suggested change for consistency
- default: - palette = RainbowStripeColors_p; - break; + default: // unreachable — all FastLED sub-palette indices are fixed at compile time + palette = CloudColors_p; + break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Modules/palettes.h` around lines 595 - 597, The switch/default handling for the unknown sentinel is inconsistent between FastLED and MoonLight — update the default branch in palettes.h so it matches the other implementation: pick one consistent fallback (either CRGB::Black or RainbowStripeColors_p) and replace the current default that assigns RainbowStripeColors_p so both branches use the same sentinel fallback; modify the default case that sets palette (and any use of gpArray[2] as the encoding sentinel) to assign the chosen consistent value (reference symbols: palette, RainbowStripeColors_p, CRGB::Black, gpArray[2]).src/MoonLight/Nodes/Effects/E_MovingHeads.h (1)
56-67: Optional: drop the commented-out color helpers to reduce clutter.
If these helpers are no longer needed, removing them keeps the effect blocks leaner.Also applies to: 82-99, 196-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Effects/E_MovingHeads.h` around lines 56 - 67, Remove the obsolete commented-out helpers (the commented declaration closestColorIndex and the colorwheelpalette vector) to reduce clutter in E_MovingHeads; search for the same commented blocks (similar commented colorwheelpalette and indices) that appear later in the file (the other occurrences you noted) and delete those commented blocks as well so the header contains only active declarations such as any real enums, structs or function prototypes for the moving head effect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/MoonLight/Modules/palettes.h`:
- Around line 467-474: Persisted user palette indices will silently map to
different palettes after shifting the audio palettes to indices 18–20; add a
migration step when loading saved palette preferences that translates legacy
indices 10–12 to the new audioPaletteIndex values (use D_AudioSync.h's static
constexpr uint8_t audioPaletteIndex and the existing palette - audioPaletteIndex
pattern to detect/compute offsets), update storage write-back so migrated prefs
are persisted, and ensure any code that reads palette counts/reserved ranges
(e.g., the comment/limit in D_Infrared.h around line 368) is revised to reflect
the new total palette count or enforced with runtime bounds checks to prevent
out-of-range loads.
In `@src/MoonLight/Nodes/Effects/E_MovingHeads.h`:
- Around line 363-365: The tilt value tilt2 is being mapped with panMin/panMax
instead of the tilt range, so tilt sliders don't apply correctly; update the
mapping for tilt2 (the beatsin8 call that assigns to tilt2 in E_MovingHeads) to
use tiltMin and tiltMax instead of panMin and panMax, keeping the same beatsin8
parameters (bandSpeed[band] check, panBPM fallback, 0..255 input range and the
0..64 offset) so the tilt mapping uses the correct tilt range.
---
Duplicate comments:
In `@src/MoonLight/Nodes/Effects/E_MovingHeads.h`:
- Line 191: The addControl call for the "range" slider is malformed and the
value isn't clamped, allowing pan ± range to wrap uint8; fix both occurrences
(the addControl invocation at the E_MovingHeads class where addControl(range,
"range", "slider") is used and the duplicate at the other location) by replacing
the call with a proper min/max signature (e.g. addControl(range, "range",
"slider", 0, 128)) and then clamp the runtime value wherever pan is computed
(the pan calculation code that applies pan ± range) using std::clamp(range, 0,
128) (or equivalent) to ensure range cannot exceed the safe maximum and prevent
uint8 wraparound.
---
Nitpick comments:
In `@src/MoonLight/Modules/palettes.h`:
- Around line 595-597: The switch/default handling for the unknown sentinel is
inconsistent between FastLED and MoonLight — update the default branch in
palettes.h so it matches the other implementation: pick one consistent fallback
(either CRGB::Black or RainbowStripeColors_p) and replace the current default
that assigns RainbowStripeColors_p so both branches use the same sentinel
fallback; modify the default case that sets palette (and any use of gpArray[2]
as the encoding sentinel) to assign the chosen consistent value (reference
symbols: palette, RainbowStripeColors_p, CRGB::Black, gpArray[2]).
In `@src/MoonLight/Nodes/Effects/E_MovingHeads.h`:
- Around line 56-67: Remove the obsolete commented-out helpers (the commented
declaration closestColorIndex and the colorwheelpalette vector) to reduce
clutter in E_MovingHeads; search for the same commented blocks (similar
commented colorwheelpalette and indices) that appear later in the file (the
other occurrences you noted) and delete those commented blocks as well so the
header contains only active declarations such as any real enums, structs or
function prototypes for the moving head effect.
backend ======= - wifi settings: add WiFi.setScanMethod (bugfix) - pio.ini: latest FastLED
Collaborator
Author
|
@coderabbitai, regarding
Can you please review my comment, as I don;t think this is the case and can you also review 33cbc29 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 15271
Substring Comment Analysis ✅Your comment is technically accurate: // returns a substring, starting at begin and ending at end-1 (not exclusive)What it means:
The implementation confirms this: strlcpy(sub.s, s + begin, end - begin + 1);
Example: However, the phrasing could be clearer. Standard convention would be: // returns a substring from begin (inclusive) to end (exclusive)This immediately tells developers that Commit 33cbc29 Review ✅Excellent commit! Here's the breakdown: ✅ WiFi Bugfix (Critical)+ WiFi.setScanMethod(WIFI_ALL_CHANNEL_SCAN);From WLED-MM bugfix: ensures all channels are scanned and strongest signal is used. Great catch importing this fix! ✅ Palette Fixes
✅ Code Quality Improvements
|
Firmware ======== - pio.ini: add compiler optimizations backend ======= - main: Serial init wait
Collaborator
Author
|
@coderabbitai, please review 0502dac |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 5340
✅ Strengths1. Compiler Optimizations (platformio.ini) ⭐⭐⭐Excellent additions with thorough documentation:
The decision to skip 2. Char.h Comment Fix ✅- // returns a substring, starting at begin and ending at end-1 (not exclusive)
+ // returns a substring from begin (inclusive) to end (exclusive)Perfect! Much clearer and follows standard substring documentation convention. 🐛 Critical IssuesSerial Initialization Logic Bug (main.cpp)Problem 1: Printf will never execute for (int i = 0; i < 5; i++) {
if (!Serial) delay(300); // ← Only delays if Serial NOT ready
if (Serial) Serial.printf("Serial init wait %d", i * 300); // ← Only prints if Serial IS ready
}This logic is backwards! When Problem 2: Missing newline Serial.printf("Serial init wait %d", i * 300); // No \nIf this somehow prints multiple times, output will be concatenated: Problem 3: Removed important USB CDC workaround -#if ARDUINO_USB_CDC_ON_BOOT && (defined(CONFIG_IDF_TARGET_ESP32S3) || ...)
- Serial.setTxTimeoutMs(0); // avoid "hung devices" when USB_CDC is enabled
-#endifThis was specifically added to prevent the issue documented in arduino-esp32#9043. Removing it might reintroduce hung devices on S3/C3/C6/P4 with USB CDC. 🔧 Recommended FixesFix 1: Correct Serial Wait LogicSerial.begin(SERIAL_BAUD_RATE);
// Wait up to 1500ms for Serial to be ready
for (int i = 0; i < 5 && !Serial; i++) {
delay(300);
}
if (Serial) {
Serial.printf("Serial initialized\n");
Serial.flush();
} else {
// Serial not available - continue anyway (headless mode)
}Fix 2: Restore USB CDC Workaround (if needed)`#if` ARDUINO_USB_CDC_ON_BOOT && (defined(CONFIG_IDF_TARGET_ESP32S3) || defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32C6) || defined(CONFIG_IDF_TARGET_ESP32P4))
Serial.setTxTimeoutMs(0); // Avoid hung devices; see arduino-esp32#9043
`#endif`Or, if you've tested and confirmed the wait loop solves the problem without 📊 Summary
Overall: Great optimizations, but Serial init needs immediate fix before merge! 🔧 🧠 Learnings used |
Collaborator
Author
|
@coderabbitai, regarding
I don't think this is true, the idea is that the delay gives the system time to make it true, please verify. I will revert the removal of the setTxTimeoutMs(0), and add Serial.setDebugOutput(true); after Serial.flush(). Will add in next commit. With this in mind as I am about to merge this PR, please do a thorough review of everything done in this PR, and add a list of things not done and future follow up actions. |
|
Tip For best results, initiate chat on the files or code changes.
Serial Init Logic - CORRECT ✅for (int i = 0; i < 5; i++) {
if (!Serial) delay(300); // Wait if not ready
if (Serial) Serial.printf(...); // Check again AFTER delay
}Flow:
The two Your plan sounds perfect:
Comprehensive PR
|
Collaborator
Author
|
@coderabbitai, in ModuleLightsControl.h, probably onUpdate(), when rebooting it looks like it is taking the last saved presets.list instead of doing a rescan of which preset files are available in /.config/presets (building the actual list). The last should be taken, / regarded as the truth, always |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 27342
The Problem 🐛State Persistence vs Filesystem Scan Race Condition:
Root Cause
Recommended SolutionsOption 1: Prevent preset.list from being saved ⭐ (Best)Mark Option 2: Force filesystem as truth on every state loadModify the state loading logic to always call void begin() override {
Module::begin(); // Load saved state
setPresetsFromFolder(); // Filesystem is truth - ALWAYS rescan after loading state
// ... rest of begin()
}This already exists at line 71, but you may need to ensure the Option 3: Clear preset.list before loading stateIn control["default"]["list"].to<JsonArray>(); // ← This gets saved!
// Add a property to indicate this should not be persisted:
control["persist"] = false; // if framework supports itOption 4: Debug the update() callIn update([&](ModuleState& state) {
return StateUpdateResult::CHANGED;
}, _moduleName);But it's updating the already-loaded state. You may need to force a replace operation rather than a merge: update([&](ModuleState& state) {
state.data["preset"]["list"] = _state.data["preset"]["list"]; // Explicitly set
return StateUpdateResult::CHANGED;
}, _moduleName);Quick Fix to TestAdd logging to verify the issue: void begin() override {
Module::begin();
EXT_LOGI(ML_TAG, "Preset list BEFORE scan: %s",
_state.data["preset"]["list"].as<String>().c_str());
setPresetsFromFolder(); // Scan filesystem
EXT_LOGI(ML_TAG, "Preset list AFTER scan: %s",
_state.data["preset"]["list"].as<String>().c_str());
}If the AFTER log shows the correct list but the UI still shows the old list, then the issue is in how the state is communicated to the UI after boot. Which solution do you prefer? I recommend Option 1 (mark as non-persistent) if the framework supports it, or Option 2 (ensure setPresetsFromFolder always wins) as a guaranteed fix. 🧠 Learnings used |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
✅ Major Accomplishments
✅ BlinkRainbowEffect
✅ MeteorEffect
✅ OscillateEffect (fixed with layerP.color1/2/3)
✅ PhasedNoiseEffect (using inoise8)
All follow proper MoonLight patterns (setup/loop/onSizeChanged, memory management, palette usage).
✅ Freqmap, Freqpixels, Freqwave
✅ Gravfreq, Gravimeter, Gravcenter, Gravcentric (with Gravity struct)
✅ Midnoise, Noisemove, Noisefire
✅ Pixelwave, Plasmoid (with Plasphase struct)
✅ Puddlepeak, Puddles
✅ Ripplepeak (with Ripple struct)
✅ Rocktaves, Waterfall
All properly mapped to SharedData (bands[], volume, volumeRaw, majorPeak).
✅ 8 single-color palettes (Red, Green, Blue, Orange, Purple, Cyan, Warm White, Cold White)
✅ Audio palette index fix (audioPaletteIndex constant instead of hardcoded 10-12)
✅ Random palette default changed from duplicate RainbowStripe to PartyColors
✅ IR remote palette count updated (8 + 8 + 3 + 61)
✅ DJLight fadeToBlackBy (compute once, not in nested loops)
✅ AmbientMove tilt mapping (was using pan range)
✅ WiFi scan method (WIFI_ALL_CHANNEL_SCAN for strongest signal)
✅ BlockModifier (Chebyshev distance for square rings)
✅ Art-Net Out IP range support (e.g., "192.168.1.100-110")
✅ -ffunction-sections + -fdata-sections + -Wl,--gc-sections (dead code elimination)
✅ -fno-exceptions (10-20% flash savings)
✅ Kept -Os (avoids Xtensa literal pool overflow)
✅ Outstanding documentation of trade-offs
✅ NULL → nullptr conversions
✅ [[noreturn]] attributes on task functions
✅ Tag cleanup (🐺→removed, 🆕 for sandbox examples, standardized emoji)
✅ Improved comments (substring, indexOf)
✅ FastLED library update
Summary by CodeRabbit
New Features
Appearance / Tags
Palettes
Documentation