Add palettes and effects #110
Merged
Conversation
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
frontend ======= - Field renderer: add genPalPrev, add type palette (palette preview) backend ======= - Effects: add ColorTwinkle, Plasma, Julia, Police - WIP - Lights control: add WLED palettes and show previews
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Palette data src/MoonLight/Modules/palettes.h |
New file: 60+ gradient palettes as byte arrays, gGradientPalettes, palette_names, plus getGradientPalette(uint8_t) and getPaletteHexString(uint8_t) runtime helpers. Large static data blob to review. |
Lights control / UI defs src/MoonLight/Modules/ModuleLightsControl.h |
Switches hard-coded palettes to dynamic palette control; populates options from palette_names, exposes per-palette hex via getPaletteHexString, and loads palettes at runtime. |
Frontend palette preview & link rendering interface/src/lib/components/moonbase/FieldRenderer.svelte |
Adds genPalPrev() for CSS linear-gradient preview, introduces palette control UI with preview, simplifies IP/mDNS anchor rendering, and removes a select slot. |
Drivers: audio & infrared src/MoonLight/Nodes/Drivers/D_AudioSync.h, src/MoonLight/Nodes/Drivers/D_Infrared.h |
AudioSync: adds getCRGBForBand and getAudioPalette to generate FFT-driven palettes and load them onto layers. Infrared: increases max palette index bounds to cover larger palette set. |
Effects: registration & new effects src/MoonLight/Modules/ModuleEffects.h, src/MoonLight/Nodes/Effects/E_WLED.h |
Reorders StarSkyEffect registration/allocation; adds ColorTwinkleEffect, PlasmaEffect, JuliaEffect, PoliceEffect classes. Note: duplicate class declarations appear in E_WLED.h and need deduplication. |
Layer mapping correction & blur refactor src/MoonLight/Layers/VirtualLayer.h, src/MoonLight/Layers/VirtualLayer.cpp |
Adds VirtualLayer::presetCorrection(nrOfLights_t&) used for RGB2040 index mapping; refactors blur pipeline (blurRows/blurColumns) to be dimension-agnostic and updates calls. |
Module concurrency & updates src/MoonBase/Module.h, src/MoonBase/Module.cpp |
ModuleState::update early-returns on empty data; postUpdate/getUpdate switch to timed mutex attempts and a retry loop to avoid indefinite blocking and reduce critical-section work. |
Docs docs/moonbase/devices.md |
Minor wording edits: removed “(controls module)” and updated phrasing to access device “via its IP address”. |
Config platformio.ini |
Bump of APP_DATE build flag from 20260118 → 20260122. |
Sequence Diagram(s)
sequenceDiagram
actor User
participant UI as FieldRenderer.svelte
participant Control as ModuleLightsControl
participant Pal as palettes.h
participant Driver as AudioSyncDriver
participant Effect as WLED_Effect
User->>UI: select palette (name/index)
UI->>UI: genPalPrev(hex) -> render preview
UI->>Control: send palette selection
Control->>Pal: request palette bytes by index
Pal-->>Control: return gradient bytes or sentinel
Control->>Control: getGradientPalette() / getPaletteHexString()
Control->>Driver: load dynamic palette onto layer
Driver->>Effect: apply palette to layer
Effect->>Effect: render loop uses new palette
Effect-->>User: LEDs updated
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
- prep71 #106 — Similar changes to ModuleState update/mutex handling (postUpdate/getUpdate paths).
- V070 preps #79 — Overlapping frontend edits to
FieldRenderer.svelte(link and palette rendering changes). - Board preset add presets #70 — Related VirtualLayer mapping/blur API and palette handling changes.
Suggested labels
enhancement
Poem
🐰
I nibble gradients, byte by byte,
Sixty hues that glow at night,
New twinkles, plasma, Julia spins,
I hop and stash the color bins,
LEDs wake — a rabbit's delight!
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 32.76% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title 'Add palettes and effects' directly and clearly summarizes the main changes: introducing new color palettes (palettes.h with 60+ gradient definitions) and new effects (ColorTwinkleEffect, PlasmaEffect, JuliaEffect, PoliceEffect). |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
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 help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@docs/moonbase/devices.md`:
- Around line 9-10: The possessive "it's" is incorrect in the device
instructions; update the text "Click on IP to go to the device via it's IP
address" to use the possessive form "its" instead. Locate the sentence in
docs/moonbase/devices.md (the line containing "Click on IP to go to the device
via it's IP address") and replace "it's" with "its" so it reads "Click on IP to
go to the device via its IP address".
In `@src/MoonLight/Modules/ModuleLightsControl.h`:
- Around line 281-355: The loop that builds hexString for gGradientPalettes uses
a hard cap while (j < 100) which can truncate palettes—replace this fixed limit
with a safe bounds check tied to the actual palette byte length (or a defined
MAX_PALETTE_BYTES) and iterate in 4-byte steps until you encounter an index==255
termination; specifically update the loop that reads from gGradientPalettes[i]
(the variables palette and j) so it ensures (j + 4) <= palette_length (or <=
MAX_PALETTE_BYTES) before reading the next 4 bytes, append the 4 bytes per
entry, and break when palette[j] (the entry index) == 255 to avoid truncation.
- Around line 425-427: The fixed-size memcpy truncates palettes and drops the
0xFF terminator; change to copying from PROGMEM byte-by-byte using
pgm_read_byte() from gGradientPalettes[index] (index computed as
updatedItem.value.as<uint8_t>() - 11) into tcp until you encounter 0xFF or reach
the tcp buffer limit, then ensure tcp contains a valid 0xFF terminator before
calling layerP.palette.loadDynamicGradientPalette(tcp); also ensure tcp’s size
is sufficient for the largest expected palette and set the final byte to 0xFF if
the loop exits due to the size limit.
In `@src/MoonLight/Modules/palettes.h`:
- Around line 28-31: The header uses the Arduino type `byte` for `ib_jul01_gp`
but doesn't include Arduino.h, making compilation order-dependent; update the
header to be self-contained by adding a guarded include or fallback typedef:
check for a prior definition of `byte` (or for ARDUINO macro) and if missing
either include <Arduino.h> or typedef unsigned char byte so `const byte
ib_jul01_gp[]` has a defined type within this header.
In `@src/MoonLight/Nodes/Drivers/D_Infrared.h`:
- Around line 367-369: The clamp for palette increase in the combined_code ==
codePaletteInc branch currently uses MIN(state.data["palette"].as<uint8_t>() +
1, 8 + 3 + 61) which allows an out‑of‑range index (72); change the ceiling to
the last valid index by subtracting 1 (i.e. use (8 + 3 + 61) - 1 or, better, a
shared MAX_PALETTE_INDEX constant) so newState["palette"] is clamped to
0..(count-1); update the same logic location (the combined_code ==
codePaletteInc branch that sets newState["palette"]) and ensure any other uses
of the literal 8+3+61 are replaced with the shared constant or consistently
subtract 1.
In `@src/MoonLight/Nodes/Effects/E_WLED.h`:
- Around line 2143-2254: The Julia and Police effect loop() implementations are
empty causing no output; fix by implementing a minimal placeholder render in
each effect's loop (e.g., in the loop() method in E_WLED.h for Julia and
similarly for Police around the region noted at 2304-2308): call
SEGMENT.setUpLeds(), clear the frame with SEGMENT.fill(BLACK) (or BLACK with
desired brightness), then draw a simple deterministic indicator (for example a
single moving pixel or a low-cost color stripe using SEGMENT.setPixelColorXY or
SEGMENT.setPixelColor) based on strip.now or SEGMENT.custom1 so the effect
visibly updates, and return; alternatively, if you prefer not to ship a
placeholder, unregister/remove these effects from the registry until full ports
are implemented. Ensure you reference and modify the loop() methods for the
Julia and Police effect classes and use SEGMENT APIs (setUpLeds, fill,
setPixelColorXY) to avoid leaving previous frames visible.
- Around line 2010-2063: The loop() for this effect (the overridden loop() in
E_WLED.h / ColorTwinkle) is currently empty and leaves stale LEDs; add a minimal
placeholder: on first call (check SEGENV.call == 0) call SEGMENT.setUpLeds()
then clear the strip by setting every pixel to BLACK (use SEGLEN and
SEGMENT.setPixelColor), or alternatively call SEGMENT.fill(BLACK) to ensure no
stale output, and ensure any required SEGENV.allocateData(...) is invoked if the
effect expects per-pixel state (use SEGENV.allocateData with dataSize computed
from SEGLEN) so the effect doesn’t crash; keep the implementation tiny and
deterministic until the full twinkle logic is ported.
- Around line 2068-2109: PlasmaEffect currently advertises 3D via dim() but only
writes a single column in loop() (layer->setRGB(Coord3D(0, i), ...)), leaving
other LEDs untouched; either change dim() to return _1D to match current
rendering, or modify loop() to iterate X and Z (and optionally clear the layer
first) and call layer->setRGB(Coord3D(x, i, z), ColorFromPalette(...)) for each
coordinate so the effect covers the full 2D/3D plane; update the dim()
implementation or the loop() iteration accordingly in the PlasmaEffect class.
🧹 Nitpick comments (2)
src/MoonLight/Modules/ModuleEffects.h (1)
138-141: Keep WLED effects list alphabetical.The section is labeled “alphabetically” but the new effects are out of order. Reordering keeps UI and allocation lists consistent.
♻️ Suggested reorder
- addControlValue(control, getNameAndTags<ColorTwinkleEffect>()); - addControlValue(control, getNameAndTags<PlasmaEffect>()); - addControlValue(control, getNameAndTags<JuliaEffect>()); - addControlValue(control, getNameAndTags<PoliceEffect>()); + addControlValue(control, getNameAndTags<ColorTwinkleEffect>()); + addControlValue(control, getNameAndTags<JuliaEffect>()); + addControlValue(control, getNameAndTags<PlasmaEffect>()); + addControlValue(control, getNameAndTags<PoliceEffect>());- if (!node) node = checkAndAlloc<ColorTwinkleEffect>(name); - if (!node) node = checkAndAlloc<PlasmaEffect>(name); - if (!node) node = checkAndAlloc<JuliaEffect>(name); - if (!node) node = checkAndAlloc<PoliceEffect>(name); + if (!node) node = checkAndAlloc<ColorTwinkleEffect>(name); + if (!node) node = checkAndAlloc<JuliaEffect>(name); + if (!node) node = checkAndAlloc<PlasmaEffect>(name); + if (!node) node = checkAndAlloc<PoliceEffect>(name);Also applies to: 235-238
interface/src/lib/components/moonbase/FieldRenderer.svelte (1)
189-207: Move the inline<style>block to component scope.Embedding
<style>inside the template inserts a<style>element per row and bypasses Svelte’s scoped styling. Prefer a top-level<style>section.♻️ Suggested change (remove inline style)
- <style> - .palette-preview { - width: 250px; - height: 40px; - border: 1px solid `#ccc`; - border-radius: 3px; - } - </style>Add this at the bottom of the component instead:
<style> .palette-preview { width: 250px; height: 40px; border: 1px solid `#ccc`; border-radius: 3px; } </style>
Comment on lines
425
to
427
| byte tcp[76] = {255}; // WLEDMM: prevent out-of-range access in loadDynamicGradientPalette() | ||
| memcpy(tcp, (byte*)pgm_read_dword(&(gGradientPalettes[updatedItem.value.as<uint8_t>() - 11])), 72); | ||
| layerP.palette.loadDynamicGradientPalette(tcp); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find files related to palette handling
fd -t f "\.h$|\.cpp$" | xargs rg -l "loadDynamicGradientPalette|gGradientPalettes" | head -20Repository: MoonModules/MoonLight
Length of output: 141
🏁 Script executed:
# Search for loadDynamicGradientPalette implementation details
rg -A 10 "loadDynamicGradientPalette" --type cpp --type hRepository: MoonModules/MoonLight
Length of output: 1361
🏁 Script executed:
# Check gradient palette definitions and sizes
rg -B 2 -A 5 "gGradientPalettes" --type cpp --type h | head -50Repository: MoonModules/MoonLight
Length of output: 2130
🏁 Script executed:
# Look for FastLED palette structure in the codebase
rg "CRGBPalette" --type cpp --type h | head -20Repository: MoonModules/MoonLight
Length of output: 546
🏁 Script executed:
# Read the palettes.h file to see palette definitions
cat src/MoonLight/Modules/palettes.h | head -100Repository: MoonModules/MoonLight
Length of output: 4786
🏁 Script executed:
# Search for the actual gradient palette definitions
rg -A 2 "gp =" src/MoonLight/Modules/palettes.h | head -40Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
# Check what size gradients typically are by looking at their PROGMEM definitions
rg "PROGMEM|gp\[" src/MoonLight/Modules/palettes.h | head -20Repository: MoonModules/MoonLight
Length of output: 2895
🏁 Script executed:
# Count the actual byte size of all palette definitions
python3 << 'EOF'
import re
with open('src/MoonLight/Modules/palettes.h', 'r') as f:
content = f.read()
# Find all palette definitions
pattern = r'const byte (\w+)\[\] = \{([^}]+)\}'
matches = re.findall(pattern, content)
sizes = []
for name, data in matches:
# Count commas to get byte count
byte_count = len(data.split(','))
sizes.append((name, byte_count))
# Sort by size
sizes.sort(key=lambda x: x[1], reverse=True)
print("Top 15 largest palettes:")
for name, size in sizes[:15]:
print(f" {name}: {size} bytes")
print(f"\nTotal palettes: {len(sizes)}")
print(f"Max size: {max(s[1] for s in sizes)} bytes")
print(f"Min size: {min(s[1] for s in sizes)} bytes")
EOFRepository: MoonModules/MoonLight
Length of output: 151
🏁 Script executed:
# Check if there are any palettes >= 144 bytes
rg "const byte.*\[\] = \{" src/MoonLight/Modules/palettes.h | wc -lRepository: MoonModules/MoonLight
Length of output: 67
🏁 Script executed:
# Look for the loadDynamicGradientPalette implementation in FastLED
rg -B 5 -A 15 "loadDynamicGradientPalette" --type h | head -60Repository: MoonModules/MoonLight
Length of output: 2338
🏁 Script executed:
# Find the largest palette definitions
rg "const byte.*\[\] = \{" src/MoonLight/Modules/palettes.h -A 0 | while read line; do
name=$(echo "$line" | sed 's/.*const byte \([a-zA-Z_]*\).*/\1/')
# Extract just the declaration and count braces
size=$(rg "const byte $name\[\] = \{[^}]+\}" src/MoonLight/Modules/palettes.h | grep -o "{[^}]*}" | head -1 | tr ',' '\n' | wc -l)
echo "$size $name"
done | sort -rn | head -20Repository: MoonModules/MoonLight
Length of output: 592
🏁 Script executed:
# Search for documented sizes in the file
rg "Size: [0-9]+ bytes" src/MoonLight/Modules/palettes.h | sed 's/.*Size: \([0-9]*\).*/\1/' | sort -rn | uniqRepository: MoonModules/MoonLight
Length of output: 106
🏁 Script executed:
# Get specific info about the largest palettes mentioned
rg -B 3 "Size: [0-9]+ bytes" src/MoonLight/Modules/palettes.h | grep -E "(Size:|Gradient palette)" | paste - - | sort -t: -k2 -rn | head -20Repository: MoonModules/MoonLight
Length of output: 108
🏁 Script executed:
# Find which palette is 144 bytes
rg -B 5 "Size: 144 bytes" src/MoonLight/Modules/palettes.hRepository: MoonModules/MoonLight
Length of output: 351
🏁 Script executed:
# Get all palettes sorted by size to understand the distribution
rg -B 3 "Size: [0-9]+ bytes" src/MoonLight/Modules/palettes.h | rg "Size: [0-9]+" | sed 's/.*Size: \([0-9]*\).*/\1/' | sort -n | tail -20Repository: MoonModules/MoonLight
Length of output: 125
🏁 Script executed:
# Search for FastLED palette loading code in the repository
rg -i "loadDynamicGradientPalette|gradient.*palette" --type h --type cpp | grep -i "load\|terminator\|255" | head -20Repository: MoonModules/MoonLight
Length of output: 310
🏁 Script executed:
# Check the exact temperature_gp definition to understand its structure
rg -A 40 "Gradient palette.*temperature_gp" src/MoonLight/Modules/palettes.hRepository: MoonModules/MoonLight
Length of output: 1995
Copy gradient palettes completely until terminator, not with fixed byte limit.
The current code copies only 72 bytes, which truncates the temperature_gp palette (144 bytes) and removes its terminator (0xFF entry), causing loadDynamicGradientPalette() to read out-of-bounds. The buffer initialization byte tcp[76] = {255} only protects position 0; the rest is zero-initialized, not a valid terminator. Repack with a copy-until-terminator loop using pgm_read_byte() to safely handle palettes of any size up to the buffer limit.
🤖 Prompt for AI Agents
In `@src/MoonLight/Modules/ModuleLightsControl.h` around lines 425 - 427, The
fixed-size memcpy truncates palettes and drops the 0xFF terminator; change to
copying from PROGMEM byte-by-byte using pgm_read_byte() from
gGradientPalettes[index] (index computed as updatedItem.value.as<uint8_t>() -
11) into tcp until you encounter 0xFF or reach the tcp buffer limit, then ensure
tcp contains a valid 0xFF terminator before calling
layerP.palette.loadDynamicGradientPalette(tcp); also ensure tcp’s size is
sufficient for the largest expected palette and set the final byte to 0xFF if
the loop exits due to the size limit.
Comment on lines
+367
to
+369
| } else if (combined_code == codePaletteInc) { // palette increase | ||
| newState["palette"] = MIN(state.data["palette"].as<uint8_t>() + 1, 8 + 3 + 61); // 8 FastLED + 3 custom + 61 WLED palettes. to do: replace nr with max palette count | ||
| } else if (combined_code == codePaletteDec) { // palette decrease |
There was a problem hiding this comment.
Clamp max palette index to the last valid value.
Line 368 clamps to 72 (the palette count), but valid indices are 0–71. This allows palette=72, which will index past gGradientPalettes in ModuleLightsControl. Clamp to count − 1 (or a shared max constant).
🛠️ Proposed fix
- newState["palette"] = MIN(state.data["palette"].as<uint8_t>() + 1, 8 + 3 + 61); // 8 FastLED + 3 custom + 61 WLED palettes. to do: replace nr with max palette count
+ newState["palette"] = MIN(state.data["palette"].as<uint8_t>() + 1, (uint8_t)(8 + 3 + 61 - 1)); // max index = count-1📝 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
| } else if (combined_code == codePaletteInc) { // palette increase | |
| newState["palette"] = MIN(state.data["palette"].as<uint8_t>() + 1, 8 + 3 + 61); // 8 FastLED + 3 custom + 61 WLED palettes. to do: replace nr with max palette count | |
| } else if (combined_code == codePaletteDec) { // palette decrease | |
| } else if (combined_code == codePaletteInc) { // palette increase | |
| newState["palette"] = MIN(state.data["palette"].as<uint8_t>() + 1, (uint8_t)(8 + 3 + 61 - 1)); // max index = count-1 | |
| } else if (combined_code == codePaletteDec) { // palette decrease |
🤖 Prompt for AI Agents
In `@src/MoonLight/Nodes/Drivers/D_Infrared.h` around lines 367 - 369, The clamp
for palette increase in the combined_code == codePaletteInc branch currently
uses MIN(state.data["palette"].as<uint8_t>() + 1, 8 + 3 + 61) which allows an
out‑of‑range index (72); change the ceiling to the last valid index by
subtracting 1 (i.e. use (8 + 3 + 61) - 1 or, better, a shared MAX_PALETTE_INDEX
constant) so newState["palette"] is clamped to 0..(count-1); update the same
logic location (the combined_code == codePaletteInc branch that sets
newState["palette"]) and ensure any other uses of the literal 8+3+61 are
replaced with the shared constant or consistently subtract 1.
backend ======= - virtual layer: add presetCorrection hook in forEachLightIndex (for lightPreset_RGB2040) - AudioSync: implement audio reactive palettes
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MoonLight/Layers/VirtualLayer.h (1)
134-138: Apply presetCorrection in the unmapped path for RGB2040 support.The
presetCorrectionapplies an index adjustment specific to RGB2040 preset (indexP += (indexP / 20) * 20), but this is only called in them_oneLightandm_moreLightscases. WhenoneToOneMappingis true (direct virtual-to-physical index mapping), the unmapped path is taken and callscallback(indexV)directly, bypassing the correction.Since RGB2040 preset is orthogonal to mapping type, it can be used with regular layouts that maintain 1:1 mapping. In that scenario, the preset correction would not be applied, resulting in incorrect index handling for RGB2040 LEDs. Add
presetCorrection(indexV)before the callback in the unmapped path.
🧹 Nitpick comments (2)
src/MoonLight/Nodes/Drivers/D_AudioSync.h (1)
69-91: Consider initializingvalueor adding a default case for defensive coding.If
palis outside the 0-2 range,valuerelies on CRGB's default constructor (which initializes to black). While currently safe since callers only pass 0-2, explicit initialization would make the intent clearer and guard against future changes.♻️ Suggested improvement
CRGB getCRGBForBand(int x, uint8_t* fftResult, int pal) { - CRGB value; + CRGB value = CRGB::Black; CHSV hsv; if (pal == 0) { // bit hacky to use palette id here, but don't want to litter the code with lots of different methods. TODO: add enum for palette creation typesrc/MoonLight/Layers/VirtualLayer.cpp (1)
84-87: Consider adding a brief comment explaining the RGB2040 physical layout.The formula
indexP += (indexP / 20) * 20creates a sparse physical mapping where lights 0-19 stay at 0-19, lights 20-39 map to 40-59, etc. A short comment explaining why this layout is needed for RGB2040 would aid maintainability.📝 Suggested documentation
void VirtualLayer::presetCorrection(nrOfLights_t& indexP) const { + // RGB2040 has physical layout with alternating 20-light segments: + // virtual [0..19] -> physical [0..19], virtual [20..39] -> physical [40..59], etc. if (layerP->lights.header.lightPreset == lightPreset_RGB2040) indexP += (indexP / 20) * 20; }
back end ======== - Module: fix bug in post/getUpdate ! Avoid missing updates - Effects: Implement ColorTwinkle, Plasma, Julia, Police
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/MoonLight/Nodes/Effects/E_WLED.h`:
- Around line 2002-2049: The loop() method uses the data pointer (accessing
data[index] and bitRead/bitWrite) without checking for null; add a guard at the
start of E_WLED::loop() that returns immediately if data == nullptr or layer ==
nullptr or layer->nrOfLights == 0 to match other effects (e.g.,
BouncingBallsEffect/TetrixEffect); this prevents dereferencing data before it is
allocated in onSizeChanged() — locate the data accesses in loop(), add the
null-check and early return, and keep existing behavior otherwise.
🧹 Nitpick comments (3)
src/MoonBase/Module.cpp (2)
289-293: Consider removing commented-out debug code.This commented-out logging block adds noise without providing value. If debugging is needed in the future, it can be easily re-added or enabled via a debug flag.
🧹 Suggested removal
bool changed = state.checkReOrderSwap("", state.data, newData, updatedItem); - // if (originId != "devicesserver" && originId != "tasksserver") { - // String ss; - // serializeJson(newData, ss); - // EXT_LOGD(ML_TAG, "newData %s from %s", ss.c_str(), originId.c_str()); - // } - // EXT_LOGD(ML_TAG, "update isNew %d changed %d", isNew, changed);
279-310: Early guard for empty data is a good defensive addition.The logic correctly short-circuits when
newDatais empty. One minor suggestion: inverting the condition would reduce nesting depth and improve readability.♻️ Optional: flatten structure with early return
StateUpdateResult ModuleState::update(JsonObject& newData, ModuleState& state, const String& originId) { updateOriginId = originId; - if (newData.size() != 0) { // in case of empty file - - // check which controls have updated - if (newData != state.data) { - UpdatedItem updatedItem; - - bool changed = state.checkReOrderSwap("", state.data, newData, updatedItem); - - if (state.compareRecursive("", state.data, newData, updatedItem)) { - if (changed) EXT_LOGW(ML_TAG, "checkReOrderSwap changed, compareRecursive also changed? %s", originId.c_str()); - changed = true; - } - - return (changed) ? StateUpdateResult::CHANGED : StateUpdateResult::UNCHANGED; - } else - return StateUpdateResult::UNCHANGED; - } else { + if (newData.size() == 0) { EXT_LOGW(MB_TAG, "empty newData %s", originId.c_str()); return StateUpdateResult::UNCHANGED; } + + // check which controls have updated + if (newData == state.data) { + return StateUpdateResult::UNCHANGED; + } + + UpdatedItem updatedItem; + bool changed = state.checkReOrderSwap("", state.data, newData, updatedItem); + + if (state.compareRecursive("", state.data, newData, updatedItem)) { + if (changed) EXT_LOGW(ML_TAG, "checkReOrderSwap changed, compareRecursive also changed? %s", originId.c_str()); + changed = true; + } + + return changed ? StateUpdateResult::CHANGED : StateUpdateResult::UNCHANGED; }src/MoonLight/Nodes/Effects/E_WLED.h (1)
2237-2253: Variable shadowing: localwidthshadows memberwidth.Line 2242 declares a local
uint16_t widththat shadows the member variableuint8_t width. While the code works (the member is used on the right side before shadowing takes effect), this is confusing and error-prone for future maintenance.♻️ Rename local variable for clarity
void police_base(uint32_t color1, uint32_t color2) { uint16_t delay = 1 + (1000 / 40 << 3) / layer->size.y; // longer segments should change faster uint32_t it = pal::millis() / map(speed, 0, 255, delay << 4, delay); uint16_t offset = it % layer->size.y; - uint16_t width = ((layer->size.y * (width + 1)) >> 9); // max width is half the strip - if (!width) width = 1; - for (int i = 0; i < width; i++) { + uint16_t pixelWidth = ((layer->size.y * (width + 1)) >> 9); // max width is half the strip + if (!pixelWidth) pixelWidth = 1; + for (int i = 0; i < pixelWidth; i++) { uint16_t indexR = (offset + i) % layer->size.y; uint16_t indexB = (offset + i + (layer->size.y >> 1)) % layer->size.y;
Comment on lines
2002
to
2049
| void loop() override { | ||
| CRGB fastled_col, prev; | ||
| uint8_t brightness = 128; // strip.getBrightness() | ||
| fract8 fadeUpAmount = brightness > 28 ? 8 + (fadeSpeed >> 2) : 68 - brightness; | ||
| fract8 fadeDownAmount = brightness > 28 ? 8 + (fadeSpeed >> 3) : 68 - brightness; | ||
| for (uint16_t i = 0; i < layer->nrOfLights; i++) { | ||
| fastled_col = layer->getRGB(i); | ||
| prev = fastled_col; | ||
| uint16_t index = i >> 3; | ||
| uint8_t bitNum = i & 0x07; | ||
| bool fadeUp = bitRead(data[index], bitNum); | ||
|
|
||
| if (fadeUp) { | ||
| CRGB incrementalColor = fastled_col; | ||
| incrementalColor.nscale8_video(fadeUpAmount); | ||
| fastled_col += incrementalColor; | ||
|
|
||
| if (fastled_col.red == 255 || fastled_col.green == 255 || fastled_col.blue == 255) { | ||
| bitWrite(data[index], bitNum, false); | ||
| } | ||
| layer->setRGB(i, fastled_col); | ||
|
|
||
| if (layer->getRGB(i) == prev) { // fix "stuck" pixels | ||
| fastled_col += fastled_col; | ||
| layer->setRGB(i, fastled_col); | ||
| } | ||
| } else { | ||
| fastled_col.nscale8(255 - fadeDownAmount); | ||
| layer->setRGB(i, fastled_col); | ||
| } | ||
| } | ||
|
|
||
| for (uint16_t j = 0; j <= layer->nrOfLights / 50; j++) { | ||
| if (random8() <= spawnSpeed) { | ||
| for (uint8_t times = 0; times < 5; times++) { // attempt to spawn a new pixel 5 times | ||
| int i = random16(layer->nrOfLights); | ||
| if (layer->getRGB(i) == 0) { | ||
| fastled_col = ColorFromPalette(layerP.palette, random8(), 64, NOBLEND); | ||
| uint16_t index = i >> 3; | ||
| uint8_t bitNum = i & 0x07; | ||
| bitWrite(data[index], bitNum, true); | ||
| layer->setRGB(i, fastled_col); | ||
| break; // only spawn 1 new pixel per frame per 50 LEDs | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing null check for data pointer before use.
The loop() method accesses data[index] without first checking if data is nullptr. If allocation fails in onSizeChanged() or if loop() is called before onSizeChanged(), this will cause a crash. Other effects in this file guard against this (e.g., BouncingBallsEffect at line 38, TetrixEffect at line 991).
🐛 Proposed fix
void loop() override {
+ if (!data) return;
+
CRGB fastled_col, prev;
uint8_t brightness = 128; // strip.getBrightness()🤖 Prompt for AI Agents
In `@src/MoonLight/Nodes/Effects/E_WLED.h` around lines 2002 - 2049, The loop()
method uses the data pointer (accessing data[index] and bitRead/bitWrite)
without checking for null; add a guard at the start of E_WLED::loop() that
returns immediately if data == nullptr or layer == nullptr or layer->nrOfLights
== 0 to match other effects (e.g., BouncingBallsEffect/TetrixEffect); this
prevents dereferencing data before it is allocated in onSizeChanged() — locate
the data accesses in loop(), add the null-check and early return, and keep
existing behavior otherwise.
Collaborator
Author
|
@coderabbitai, can you review the update on postUpdate / getUpdate in latest commit, can you confirm it was faulty before and okay now? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 33 🏁 Script executed: Length of output: 239 🏁 Script executed: Length of output: 3256
The Problem in the Old CodeThe old // OLD CODE - BUGGY
if (xSemaphoreTake(updateMutex, portMAX_DELAY) == pdTRUE) {
this->updatedItem = updatedItem; // ⚠️ Overwrites without checking updatePending!
updatePending = true;
xSemaphoreGive(updateMutex);
}The bug: If a producer called How the New Code Fixes ItThe new implementation properly serializes updates: // NEW CODE - CORRECT
while (true) {
if (xSemaphoreTake(updateMutex, pdMS_TO_TICKS(10)) == pdTRUE) {
if (!updatePending) { // ✓ Only write when previous update consumed
this->updatedItem = updatedItem;
updatePending = true;
xSemaphoreGive(updateMutex);
break;
}
xSemaphoreGive(updateMutex);
}
vTaskDelay(pdMS_TO_TICKS(1)); // Wait and retry
}Key improvements:
The fix is correct and addresses a real concurrency issue. Well done on catching and fixing this! 👍 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/MoonLight/Nodes/Effects/E_WLED.h`:
- Around line 1999-2001: Add a destructor to class E_WLED that frees the heap
buffer allocated in onSizeChanged: if the member uint8_t* data is non-null, call
delete[] data, set data to nullptr and dataSize to 0; also consider marking copy
constructor/assignment as deleted or implement proper copy/move semantics to
avoid double-free/ownership bugs since data is a raw pointer.
♻️ Duplicate comments (1)
src/MoonLight/Modules/palettes.h (1)
25-31: Makebytedefinition self-contained.This header uses
byte(an Arduino-specific type alias foruint8_t) without includingArduino.h. This causes compilation failures in non-Arduino build environments, as confirmed by static analysis.🛠️ Proposed fix
`#ifndef` Palettes_h `#define` Palettes_h -// `#include` <Arduino.h> +#ifndef byte + `#include` <cstdint> + using byte = uint8_t; +#endif const byte ib_jul01_gp[] = {0, 194, 1, 1, 94, 1, 29, 18, 132, 57, 131, 28, 255, 113, 1, 1};
Comment on lines
+1999
to
+2001
| uint8_t* data = nullptr; | ||
| size_t dataSize = 0; | ||
|
|
There was a problem hiding this comment.
Missing destructor causes memory leak.
The data pointer allocated in onSizeChanged is never freed. Add a destructor to prevent memory leaks when the effect is destroyed.
🐛 Proposed fix
uint8_t* data = nullptr;
size_t dataSize = 0;
+ ~ColorTwinkleEffect() override {
+ if (data) freeMB(data, "data");
+ }
+
void onSizeChanged(const Coord3D& prevSize) override { reallocMB2<uint8_t>(data, dataSize, (layer->nrOfLights + 7) >> 3, "data"); }📝 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
| uint8_t* data = nullptr; | |
| size_t dataSize = 0; | |
| uint8_t* data = nullptr; | |
| size_t dataSize = 0; | |
| ~ColorTwinkleEffect() override { | |
| if (data) freeMB(data, "data"); | |
| } | |
| void onSizeChanged(const Coord3D& prevSize) override { reallocMB2<uint8_t>(data, dataSize, (layer->nrOfLights + 7) >> 3, "data"); } |
🤖 Prompt for AI Agents
In `@src/MoonLight/Nodes/Effects/E_WLED.h` around lines 1999 - 2001, Add a
destructor to class E_WLED that frees the heap buffer allocated in
onSizeChanged: if the member uint8_t* data is non-null, call delete[] data, set
data to nullptr and dataSize to 0; also consider marking copy
constructor/assignment as deleted or implement proper copy/move semantics to
avoid double-free/ownership bugs since data is a raw pointer.
Collaborator
Author
|
@coderabbitai, can you review the blur functions in virtual layer.h as they do not work as intended, eg blackhole calls blur2d but is not showing blur |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3731 🏁 Script executed: Length of output: 2258 🏁 Script executed: Length of output: 667 🏁 Script executed: Length of output: 7224 🏁 Script executed: Length of output: 6475 🏁 Script executed: Length of output: 4957 🏁 Script executed: Length of output: 2985 🏁 Script executed: Length of output: 3328 🏁 Script executed: Length of output: 882 🏁 Script executed: Length of output: 1076 🏁 Script executed: Length of output: 104 🏁 Script executed: Length of output: 1762
The ProblemLooking at lines 320-361 in VirtualLayer.h, both blur functions follow this pattern: for (uint16_t i = 0; i < width; i++) { // (or height for columns)
CRGB cur = getRGB(...);
CRGB part = cur;
part.nscale8(seep);
cur.nscale8(keep);
cur += carryover;
if (i) addRGB(..., part); // add to PREVIOUS pixel
setRGB(..., cur);
carryover = part; // save for next iteration
}
// ⚠️ Loop ends here - carryover is lost!After the loop completes,
The FixAdd the final carryover write after each inner loop: For blurRows (after line 341, before the closing for (uint16_t row = 0; row < height; row++) {
CRGB carryover = CRGB::Black;
for (uint16_t i = 0; i < width; i++) {
// ... existing code ...
carryover = part;
}
// Add this line:
addRGB(Coord3D(width - 1, row), carryover);
}For blurColumns (after line 360, before the closing for (uint16_t col = 0; col < width; ++col) {
CRGB carryover = CRGB::Black;
for (uint16_t i = 0; i < height; ++i) {
// ... existing code ...
carryover = part;
}
// Add this line:
addRGB(Coord3D(col, height - 1), carryover);
}This ensures the blur properly distributes across the entire matrix and should make the blur visible in Blackhole and other effects. Would you like me to create a code change plan to fix both functions? 🧠 Learnings used |
Collaborator
Author
|
@coderabbitai, that is better indeed , is it also possible to keep the original pixels as bright as they were and also the blur |
|
Tip For best results, initiate chat on the files or code changes.
For additive blur (bright originals + blur on top), we need to change the algorithm: Current Algorithm (Energy-Redistributing)keep = 255 - blur_amount; // 83 (~33%)
seep = blur_amount >> 1; // 86 (~34%)
cur.nscale8(keep); // ⚠️ Dims to 33%
cur += carryover;New Algorithm (Additive Blur)Remove the blurRows (around line 327-341): void blurRows(uint8_t blur_amount) {
uint8_t seep = blur_amount >> 1; // Neighbor contribution
// NO keep scaling - pixels stay 100% bright
for (uint16_t row = 0; row < height; row++) {
CRGB carryover = CRGB::Black;
for (uint16_t i = 0; i < width; i++) {
CRGB cur = getRGB(Coord3D(i, row));
CRGB part = cur;
part.nscale8(seep);
// DON'T scale cur - keep original brightness
cur += carryover;
if (i) addRGB(Coord3D(i - 1, row), part);
setRGB(Coord3D(i, row), cur);
carryover = part;
}
addRGB(Coord3D(width - 1, row), carryover); // Final carryover
}
}blurColumns (similar change around line 346-360): void blurColumns(uint8_t blur_amount) {
uint8_t seep = blur_amount >> 1;
for (uint16_t col = 0; col < width; ++col) {
CRGB carryover = CRGB::Black;
for (uint16_t i = 0; i < height; ++i) {
CRGB cur = getRGB(Coord3D(col, i));
CRGB part = cur;
part.nscale8(seep);
// DON'T scale cur
cur += carryover;
if (i) addRGB(Coord3D(col, i - 1), part);
setRGB(Coord3D(col, i), cur);
carryover = part;
}
addRGB(Coord3D(col, height - 1), carryover); // Final carryover
}
}This way:
The trade-off: brightness increases, so you may want to reduce 🧠 Learnings used💡 Use |
Collaborator
Author
|
@coderabbitai, if we don't scale cur , the whole screen fills up with solid color,if we scale cur, to much blur means nothing is shown. Is there a better algorithm which keeps the brightness not not fill up the whole screen ? |
|
Tip For best results, initiate chat on the files or code changes.
The Solution: Weighted Average Box BlurInstead of scaling then adding, compute a weighted average of each pixel with its neighbors: void blurRows(uint8_t blur_amount) {
if (blur_amount == 0) return;
// Convert blur_amount to a fraction (0-255 → 0.0-1.0)
uint8_t weight = blur_amount;
for (uint16_t row = 0; row < height; row++) {
CRGB prev = CRGB::Black;
CRGB cur = getRGB(Coord3D(0, row));
for (uint16_t i = 0; i < width; i++) {
CRGB next = (i < width - 1) ? getRGB(Coord3D(i + 1, row)) : CRGB::Black;
// Weighted average: center pixel + neighbors
CRGB result = cur; // Start with 100% current pixel
// Add weighted contribution from neighbors
CRGB neighborSum = prev;
neighborSum += next;
neighborSum.nscale8_video(weight); // Scale by blur_amount
// Blend: keep most of original, add scaled neighbors
result.nscale8_video(255 - weight);
result += neighborSum.nscale8_video(weight >> 1); // Divide by 2 (two neighbors)
setRGB(Coord3D(i, row), result);
prev = cur;
cur = next;
}
}
}Wait, that's still complex. Let me give you a simpler, energy-conserving approach that FastLED uses: Simpler Solution: Three-Tap Box Blurvoid blurRows(uint8_t blur_amount) {
if (blur_amount == 0) return;
uint8_t keep = 255 - blur_amount;
uint8_t seep = blur_amount >> 1; // Divide by 2 for two neighbors
for (uint16_t row = 0; row < height; row++) {
CRGB carryover = CRGB::Black;
for (uint16_t i = 0; i < width; i++) {
CRGB cur = getRGB(Coord3D(i, row));
// Scale down current pixel
CRGB part = cur;
part.nscale8(seep);
cur.nscale8(keep);
// Add carryover from previous pixel
cur += carryover;
// Write result
setRGB(Coord3D(i, row), cur);
// Save for next iteration
carryover = part;
}
// Wrap final carryover to first pixel to conserve energy
if (width > 1) {
addRGB(Coord3D(0, row), carryover);
}
}
}Key changes:
Apply the same pattern to This is energy-conserving: no brightness is lost or gained, it's just redistributed smoothly. The wrapping prevents the "disappearing" effect while the scaling prevents the "filling" effect. Would you like me to create a code change plan for both 🧠 Learnings used |
Collaborator
Author
|
@coderabbitai, for some reason class OctopusEffect is showing random pixels while it worked fine earlier, can you see what might have changed which caused this? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1032 🏁 Script executed: Length of output: 3938 🏁 Script executed: Length of output: 5135
The ProblemLooking at the code flow:
Result: Random pixel colors from uninitialized memory. The FixCall void onSizeChanged(const Coord3D& prevSize) override {
reallocMB2<Map_t>(rMap, rMapSize, layer->size.x * layer->size.y, "rMap");
if (rMap) setRMap(); // Initialize after allocation
}This ensures rMap is always properly initialized before Would you like me to create a code change plan to fix this? 🧠 Learnings used |
backend ======= - Virtual layer: blur tweaks - Lights control: use palette functions - palettes: byte to uint8_t, refactor combining all types and alphabetically
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MoonLight/Layers/VirtualLayer.h (1)
320-358: Guard blurRows/blurColumns against zero dimensions.If
size.xorsize.yis 0, the loops skip butaddRGB(size.x - 1, …)/addRGB(…, size.y - 1)underflows and writes out of bounds.🛠️ Suggested fix
void blurRows(fract8 blur_amount) { + if (size.x == 0 || size.y == 0) return; // blur rows same as columns, for irregular matrix uint8_t keep = 255 - blur_amount; uint8_t seep = blur_amount >> 1; @@ - addRGB(Coord3D(size.x - 1, row), carryover); + addRGB(Coord3D(size.x - 1, row), carryover); } } // blurColumns: perform a blur1d on each column of a rectangular matrix void blurColumns(fract8 blur_amount) { + if (size.x == 0 || size.y == 0) return; // blur columns uint8_t keep = 255 - blur_amount; uint8_t seep = blur_amount >> 1; @@ - addRGB(Coord3D(col, size.y - 1), carryover); + addRGB(Coord3D(col, size.y - 1), carryover); } }
🤖 Fix all issues with AI agents
In `@src/MoonLight/Modules/ModuleLightsControl.h`:
- Around line 331-334: The palette assignment trusts external input and can
index gGradientPalettes out of bounds via getGradientPalette(updatedItem.value);
validate or clamp updatedItem.value before calling getGradientPalette (or update
getGradientPalette to handle bounds) so you never read past gGradientPalettes;
on invalid index, choose a safe default palette (e.g., first palette) and then
assign to layerP.palette to avoid OOB reads when handling "palette" in the
updatedItem branch.
In `@src/MoonLight/Modules/palettes.h`:
- Around line 652-663: The code in the gpArray[0] == 255 branch uses a too-small
stack buffer (char buf[3]) for sprintf("%02x%02x%02x%02x"), causing stack
corruption; update the buffer to a safe size (at least 9 bytes for 8 hex chars +
NUL) and replace sprintf calls with snprintf(buf, sizeof(buf), ...) to avoid
overflow, or use a std::string/ostringstream to format into hexString directly;
adjust the formatting calls around buf, palette[], hexString and the final entry
to use the new safe approach.
♻️ Duplicate comments (4)
src/MoonLight/Modules/palettes.h (3)
24-28: Makepalettes.hself‑contained foruint8_t/byte.This header relies on
uint8_tandbytewithout including<cstdint>or<Arduino.h>, which breaks compilation when include order changes. Add a guarded include/typedef.🛠️ Suggested fix
`#ifndef` Palettes_h `#define` Palettes_h + +#if __has_include(<Arduino.h>) +#include <Arduino.h> +#else +#include <cstdint> +using byte = uint8_t; +#endif
638-641: Avoid truncating gradient palettes ingetGradientPalette().The fixed 72‑byte copy can truncate larger gradients (e.g., 144‑byte palettes), dropping the
0xFFterminator and causingloadDynamicGradientPalette()to read out‑of‑bounds.🛠️ Suggested fix
- } else { // gradient array palettes - byte tcp[76] = {255}; // WLEDMM: prevent out-of-range access in loadDynamicGradientPalette() - memcpy(tcp, (byte*)pgm_read_dword(&(gGradientPalettes[index])), 72); - palette.loadDynamicGradientPalette(tcp); - } + } else { // gradient array palettes + constexpr size_t kMaxPaletteBytes = 256; // adjust if you expect larger + byte tcp[kMaxPaletteBytes]; + size_t j = 0; + while (j + 3 < kMaxPaletteBytes) { + byte b = pgm_read_byte(((const byte*)pgm_read_dword(&(gGradientPalettes[index]))) + j); + tcp[j++] = b; + // copy 4‑byte entries + for (int k = 1; k < 4; k++) { + tcp[j] = pgm_read_byte(((const byte*)pgm_read_dword(&(gGradientPalettes[index]))) + j); + j++; + } + if (tcp[j - 4] == 255) break; + } + if (tcp[j - 4] != 255 && j + 4 <= kMaxPaletteBytes) tcp[j++] = 255; // ensure terminator + palette.loadDynamicGradientPalette(tcp); + }
667-675: Avoid the hard 100‑byte cap when building palette previews.Some gradients exceed 100 bytes; the fixed limit truncates them and yields incomplete previews. Use a safe upper bound and stop only at the 0xFF terminator.
🛠️ Suggested fix
- while (j < 100) { // Safety limit + const size_t kMaxPaletteBytes = 256; + while (j + 3 < kMaxPaletteBytes) { @@ - if (gpArray[j - 4] == 255) break; + if (gpArray[j - 4] == 255) break; }src/MoonLight/Nodes/Effects/E_WLED.h (1)
2060-2063: Add debug label tofreeMBcall for consistency with other destructors.The
freeMBsignature accepts an optional label parameter (const char* name = nullptr). All other effect destructors in this file provide a label for memory tracking. Align ColorTwinkleEffect with this pattern:~ColorTwinkleEffect() { freeMB(data, "data"); }
🧹 Nitpick comments (1)
src/MoonLight/Nodes/Effects/E_WLED.h (1)
2067-2093: Align PlasmaEffect::dim() with 2D/3D rendering.The loop writes across x and z for every y, so
_1Dunder‑represents the effect and can affect filtering/selection. Consider_2D(or_3Dif you want it available on volumetric layouts), or keep_1Dand only render a single line.🛠️ Possible change
- static uint8_t dim() { return _1D; } + static uint8_t dim() { return _2D; }
Collaborator
Author
|
@coderabbitai, is the blur1d() function redundant and can we use blurColumns() for it? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 4021
Key Difference
Current UsagesLooking at the codebase:
Why Both Are Needed
If you replaced So keep both functions — they're complementary, not redundant! 👍 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MoonLight/Layers/VirtualLayer.h (1)
298-313: Missing final carryover write causes incomplete blur.After the loop ends,
carryovercontains the last pixel'sseepcontribution but is never written. This loses the bottom pixel's blur contribution, weakening the overall effect.Proposed fix
setRGB(Coord3D(x, row), cur); carryover = part; } + if (size.y) addRGB(Coord3D(x, size.y - 1), carryover); }
🤖 Fix all issues with AI agents
In `@src/MoonLight/Layers/VirtualLayer.h`:
- Around line 320-338: In blurRows, the conditional that guards adding the
"part" to the previous column uses the wrong variable ("if (row)"), which causes
missed blur contributions and underflow when col == 0; change that check to "if
(col)" in the loop inside blurRows (the loop iterating uint16_t col) so
addRGB(Coord3D(col - 1, row), part) only runs when col > 0; verify the final
addRGB(Coord3D(size.x - 1, row), carryover) remains unchanged.
♻️ Duplicate comments (1)
src/MoonLight/Modules/palettes.h (1)
24-27: Add an explicit stdint/Arduino include souint8_tis defined.Clang reports unknown type name
uint8_tat Line 27; this header currently depends on include order. Add a standard include (or guardedArduino.h) to make it self-contained.🛠️ Proposed fix
`#ifndef` Palettes_h `#define` Palettes_h + +#include <stdint.h>
🧹 Nitpick comments (1)
src/MoonLight/Nodes/Effects/E_WLED.h (1)
2186-2219: Consider addingtaskYIELD()to prevent task starvation on large matrices.The nested loops iterate over every pixel (rows × cols), and each pixel runs an inner
whileloop up tomaxIterationstimes. For a 128×128 matrix withmaxIterations = 63, this can exceed 1 million operations per frame without yielding, potentially starving higher-priority tasks.♻️ Suggested fix
// Start y float y = ymin; for (int j = 0; j < rows; j++) { + taskYIELD(); // prevent task starvation on large matrices // Start x float x = xmin; for (int i = 0; i < cols; i++) {Based on learnings, the GameOfLifeEffect had similar issues with nested loops causing the effect to disappear when higher-priority tasks run.
Collaborator
Author
|
@coderabbitai, regarding learning
Update the learning to be more general, in your own words: checks and protections needs to be added for end user behavior, not for programmer behavior eg out of bounds checks, where out of bounds can be prevented by good programming , the reason is that this system is build for high performance and we don't want extra code for bad programming |
|
Tip For best results, initiate chat on the files or code changes.
New general principle: MoonLight adds input validation and bounds checking to protect against end-user behavior (invalid UI inputs, malformed network data, hardware failures), but intentionally omits defensive checks for programmer errors that can be prevented by correct coding (e.g., array out-of-bounds, invalid function arguments, null pointer dereferences). The system is built for high performance and will not add runtime overhead to catch programming mistakes. Programmers are responsible for writing correct code; the architecture trusts that internal function calls and data structures are used properly. This applies throughout the codebase and explains why you'll see validation at system boundaries (UI, network, hardware) but not in internal APIs and hot paths. 👍 ✏️ Learnings added
⛔ Learnings removed🧠 Learnings used |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.