starsky effect #96
Merged
starsky effect #96
+112
−6
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
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Module Integration src/MoonLight/Modules/ModuleEffects.h |
Inserts StarSkyEffect into public node ordering and allocation sequences (placed after FireEffect) so it appears in selection and instantiation flows. |
New Effect Implementation src/MoonLight/Nodes/Effects/E_MoonLight.h |
Adds StarSkyEffect class: public identifiers (name, dim, tags), controls (star_speed, star_fill_ratio, usePalette), lifecycle methods (setup, onSizeChanged, onUpdate, loop), per-star buffers (nb_stars, stars_fade_dir, stars_indexes, stars_brightness, stars_colors), setup_animation() and resetStars() for allocation/cleanup, destructor, and runtime animation/brightness logic. |
I/O & Board Handling src/MoonBase/Modules/ModuleIO.h |
Adds uint8_t current_board_id member; setBoardPresetDefaults stores active board ID; voltage and current calculations branch by board type (SE16V1 vs LightCrafter16) with distinct ADC-to-voltage/current formulas and gating behavior. |
Sequence Diagram(s)
sequenceDiagram
actor User
participant ModuleEffects
participant StarSkyEffect
participant Allocator
participant LEDRenderer
User->>ModuleEffects: select "Star Sky"
ModuleEffects->>StarSkyEffect: instantiate node
StarSkyEffect->>Allocator: allocate per-star buffers (setup_animation)
Allocator-->>StarSkyEffect: return pointers / status
StarSkyEffect->>StarSkyEffect: initialize stars and state
loop every frame
StarSkyEffect->>StarSkyEffect: update star positions, brightness, fade directions
StarSkyEffect->>LEDRenderer: write computed colors to LED buffer
LEDRenderer-->>StarSkyEffect: frame rendered
end
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
- 1D effects x to y-axis conversion #81 — Modifies
E_MoonLight.heffects classes and axis handling; likely to conflict or overlap withStarSkyEffectadditions.
Poem
🐇✨ I count the bytes where starlight stays,
Buffers hum softly in semaphore bays.
I plant bright dots across the velvet sweep,
Hop, blink, buffer — awake while others sleep.
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 20.00% 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 'starsky effect' accurately matches the main changes, which introduce a new StarSkyEffect class and integrate it into the MoonLight effects system. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
📜 Recent review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/MoonLight/Nodes/Effects/E_MoonLight.h
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
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: 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: 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: 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.
📚 Learning: 2026-01-06T15:24:29.770Z
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.
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
📚 Learning: 2025-12-18T15:15:57.828Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-18T15:15:57.828Z
Learning: In DripEffect (E_WLED.h), dropsSize must be set to layer->size.y (not layer->size.x) in onSizeChanged(), because the drops array is allocated as [layer->size.y][maxNumDrops]. Using layer->size.x causes out-of-bounds access when iterating over y-coordinates.
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
📚 Learning: 2026-01-06T13:45:03.067Z
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).
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
📚 Learning: 2026-01-06T15:24:17.118Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:164-171
Timestamp: 2026-01-06T15:24:17.118Z
Learning: In VirtualLayer::setLight and VirtualLayer::getLight (src/MoonLight/Layers/VirtualLayer.cpp), bounds checking in the unmapped path (when indexV >= mappingTableSize or oneToOneMapping is true) is intentionally disabled for performance when driving very large LED counts (100K+). Effect developers are responsible for ensuring their code stays within bounds. This is an explicit design decision prioritizing performance over defensive checks in the hot path.
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
📚 Learning: 2025-12-28T10:28:24.157Z
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.
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
📚 Learning: 2025-12-18T15:59:30.556Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-18T15:59:30.556Z
Learning: In DripEffect (E_WLED.h), the bounce fails because the damped velocity is too weak to keep the drop at pos > 0 for multiple frames. When the drop returns to pos <= 0 while still in the bouncing state (colIndex == bouncing), it immediately resets to init (line 1663-1664) before any visible bounce occurs. The bounce velocity must be strong enough (divide by 1.1 or less, or use minimal damping like *= 0.9) so the drop stays airborne long enough for a visible bounce arc before the eventual second ground hit triggers the reset.
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
📚 Learning: 2025-12-25T13:34:40.097Z
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.
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
🧬 Code graph analysis (1)
src/MoonLight/Nodes/Effects/E_MoonLight.h (3)
src/MoonBase/Nodes.h (2)
Node(67-182)Node(238-238)src/MoonBase/Module.cpp (2)
addControl(353-362)addControl(353-353)src/MoonLight/Nodes/Effects/E_MoonModules.h (2)
onSizeChanged(183-199)onUpdate(120-124)
🔇 Additional comments (8)
src/MoonLight/Nodes/Effects/E_MoonLight.h (8)
41-46: LGTM: Class declaration follows framework conventions.The static metadata methods are properly defined for effect registration.
48-51: LGTM: Control variables with sensible defaults.The defaults provide a reasonable starting point for the effect.
53-58: LGTM: Proper deferred initialization.Correctly defers
setup_animation()toonSizeChanged()when the layer is guaranteed to be available. This addresses part of the initialization timing concerns from the crash analysis.
60-66: LGTM: Safe buffer initialization.All pointers initialized to
nullptrandnb_starsto 0, enabling the guard checks inloop()to detect uninitialized state correctly.
67-79: LGTM: Proper resource cleanup.The destructor and
resetStars()correctly free all allocated buffers and reset state. The pattern of checking before freeing and nulling after is defensive and correct.
103-106: LGTM: Appropriate triggers for re-initialization.Correctly calls
setup_animation()when layer size changes or whenstar_fill_ratioorusePalettecontrols change, as these affect buffer allocation and star count.
81-101: Allocation failure handling is correct and properly implemented.The allocation checks at lines 88-92 correctly validate all
allocMBreturn values and handle failures by callingresetStars()and returning early. The framework guarantees non-zero layer dimensions (initialized to prevent division-by-zero), so the divisions at line 112 inloop()are safe.
108-136: Guard is comprehensive, but division-by-zero and unmapped fixture concerns require verification.The pointer guard at line 109 is good, but the 1D-to-3D index conversion at line 112 divides by
layer->size.xandlayer->size.ywithout validating they are non-zero. If either dimension is 0, this causes a crash. Other effects in the codebase (e.g., E_MovingHeads.h, VirtualLayer::XYZUnModified) also perform arithmetic on size dimensions without guards, suggesting the framework may guarantee size > 0, but this assumption should be verified.Additionally,
stars_indexes[i]is bounded bylayer->nrOfLights(line 95, 127), but if the layer has unmapped LEDs or a non-rectangular shape, the computed position may exceed layer bounds. Per the framework design, effect developers are responsible for staying within bounds, but confirm that this effect is only used with fully-mapped rectangular fixtures, or add an out-of-bounds check after line 112 if complex geometries are intended to be supported.
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.
Author
|
@CodeRabbit: getting the following error: Guru Meditation Error: Core 1 panic'ed (StoreProhibited). Exception was unhandled. Core 1 register dump: Backtrace: 0x40056f9e:0x3fceb360 0x4206d9e9:0x3fceb370 0x42044501:0x3fceb3c0 0x4207bf33:0x3fceb400 0x4208fe83:0x3fceb460 0x420867dc:0x3fceb490 0x4209c1c3:0x3fceb500 0x4203b1fa:0x3fceb5e0 0x4203b209:0x3fceb600 0x4203b2eb:0x3fceb620 0x4203b35a:0x3fceb640 0x4203de15:0x3fceb660 0x4203dfcb:0x3fceb8d0 0x4203e523:0x3fcebb40 0x4217b48b:0x3fcebc00 0x4203b6ae:0x3fcebc20 0x4203ee25:0x3fcebc40 0x4203eeb5:0x3fcebd40 0x42084fc1:0x3fcebda0 0x42085071:0x3fcebde0 0x4209f84d:0x3fcebe20 0x420abd46:0x3fcebe80 0x4037fd89:0x3fcebea0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @src/MoonLight/Nodes/Effects/E_MoonLight.h:
- Around line 68-74: The per-star debug log in StarSkyEffect is too verbose;
replace or remove the EXT_LOGD call that logs each star's index/brightness/fade
(the log using stars_indexes[i], start_brightness, stars_fade_dir[i]) so it
doesn't flood logs — either change it to a verbose-level log (EXT_LOGV) for
per-star details or drop it entirely and keep only a single summary EXT_LOGD
after the loop that reports nb_stars and layer->nrOfLights; update the logging
around the loop that assigns stars_indexes[], stars_fade_dir[], and calls
layer->setRGB(...) accordingly.
- Around line 87-104: In loop20ms(), avoid uint8_t wraparound when changing
brightness: read current = layer->getRGB(stars_indexes[i]).r into a
wider/temporary variable or use the uint8_t but perform bounds checks before
doing +1/−1; compute new_brightness by clamping (e.g., if fading up and current
< 255 then new = current + 1 else set fade direction to 0 and new = 255;
similarly for fading down) and then call layer->setRGB(...) with new_brightness;
update references to stars_fade_dir, stars_indexes and nb_stars accordingly so
the boundary tests occur before setRGB to prevent overflow/underflow.
- Line 79: The onUpdate() method currently has the wrong signature so it never
overrides Node::onUpdate(const Char<20>& oldValue, const JsonObject& control)
and therefore changes to controls like slowness or star_fill_ratio won't call
setup_animation(); fix by replacing the no-arg onUpdate() with the correct
override signature (Node::onUpdate(const Char<20>& oldValue, const JsonObject&
control)) and call setup_animation() inside it when relevant controls changed
(check oldValue or inspect control), or if re-initialization on control change
is not desired simply remove the incorrect onUpdate() method.
- Around line 66-67: allocMB calls assigning stars_indexes and stars_fade_dir
can return null; after the allocations in the E_MoonLight initialization (where
allocMB<uint16_t>(nb_stars) and allocMB<uint8_t>(nb_stars) are called) add null
checks for both stars_indexes and stars_fade_dir and handle failure immediately
(e.g., free any previously allocated memory, log/error out, and return an error
or false from the initializer). Ensure you reference and clean up the other
member(s) if one allocation succeeds and the other fails to avoid leaks.
- Line 65: The star density calc using nb_stars = ((uint32_t)star_fill_ratio *
(uint32_t)layer->nrOfLights)/10000 + 1 produces unintuitive low densities;
update the divisor to a more intuitive scale (e.g., 100 or 1000) and revisit the
+1 offset so the slider maps sensibly to percentage-like behavior — replace the
10000 divisor with your chosen scale (100 or 1000) in the nb_stars computation
and remove or adjust the +1 if it causes an unwanted minimum of 1 star, ensuring
nb_stars scales proportionally with star_fill_ratio and layer->nrOfLights.
🧹 Nitpick comments (1)
src/MoonLight/Nodes/Effects/E_MoonLight.h (1)
69-75: Consider checking for duplicate star positions.Multiple stars can currently occupy the same pixel index (line 70), which may cause them to interfere with each other during brightness updates. If this is unintentional, add duplicate checking during initialization.
💡 Example approach
std::set<uint16_t> used_positions; for (uint32_t i = 0; i < nb_stars; i++) { uint16_t pos; int attempts = 0; do { pos = random16(layer->nrOfLights); attempts++; } while (used_positions.count(pos) && attempts < 100); stars_indexes[i] = pos; used_positions.insert(pos); // ... rest of initialization }Note: This requires including
<set>and may impact performance for large star counts.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/MoonLight/Modules/ModuleEffects.hsrc/MoonLight/Nodes/Effects/E_MoonLight.h
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
🧬 Code graph analysis (2)
src/MoonLight/Nodes/Effects/E_MoonLight.h (3)
src/MoonBase/Nodes.h (2)
Node(67-182)Node(238-238)src/MoonLight/Layers/PhysicalLayer.cpp (2)
loop20ms(78-83)loop20ms(78-78)src/MoonLight/Layers/VirtualLayer.cpp (2)
loop20ms(71-75)loop20ms(71-71)
src/MoonLight/Modules/ModuleEffects.h (1)
src/MoonLight/Nodes/Effects/E_MoonLight.h (1)
StarSkyEffect(42-105)
🔇 Additional comments (1)
src/MoonLight/Modules/ModuleEffects.h (1)
87-87: LGTM! StarSkyEffect registration is properly positioned.The effect is correctly added to both the control list and allocation sequence, positioned after FireEffect as intended.
Also applies to: 180-180
| freeMB(stars_fade_dir); | ||
| if (layer->nrOfLights == 0) { return; } | ||
| layer->fill_solid(CRGB::Black); | ||
| nb_stars = ((uint32_t)star_fill_ratio * (uint32_t)layer->nrOfLights)/10000 + 1; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file in question
head -n 100 src/MoonLight/Nodes/Effects/E_MoonLight.h | tail -n 50Repository: MoonModules/MoonLight
Length of output: 2153
🏁 Script executed:
# Check the full context around line 65
sed -n '50,80p' src/MoonLight/Nodes/Effects/E_MoonLight.hRepository: MoonModules/MoonLight
Length of output: 1300
🏁 Script executed:
# Search for star_fill_ratio definition and usage
rg "star_fill_ratio" -B 3 -A 3 -iRepository: MoonModules/MoonLight
Length of output: 1703
🏁 Script executed:
# Search for any documentation or comments about stars
rg "nb_stars|star.*ratio" -B 2 -A 2 -iRepository: MoonModules/MoonLight
Length of output: 2248
🏁 Script executed:
# Search for addControl implementation to understand slider constraints
rg "addControl.*slider" -B 2 -A 5Repository: MoonModules/MoonLight
Length of output: 50377
🏁 Script executed:
# Look for tests or examples of StarSkyEffect
fd -e "test\|spec\|example" -i | xargs rg "StarSky|star_fill" 2>/dev/null || echo "No test files found"Repository: MoonModules/MoonLight
Length of output: 1040
🏁 Script executed:
# Search for any configuration files or examples that use this effect
rg "star.*fill|StarSky" -t json -t yaml -t txt -t mdRepository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
# Check if there are any comments or documentation in the header file about the star effect
sed -n '1,100p' src/MoonLight/Nodes/Effects/E_MoonLight.hRepository: MoonModules/MoonLight
Length of output: 3825
🏁 Script executed:
# Check if there's documentation or pattern for default slider ranges
rg "addControl.*slider.*\)" -A 1 src/MoonLight/Nodes/Effects/E_MoonLight.h | head -40Repository: MoonModules/MoonLight
Length of output: 1385
🏁 Script executed:
# Look for any test or config files that show star_fill_ratio usage
find . -type f \( -name "*test*" -o -name "*config*" -o -name "*example*" \) -exec rg -l "star" {} \;Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
# Check git history or comments for context on the divisor choice
rg "10000|star_fill" --type-listRepository: MoonModules/MoonLight
Length of output: 454
🏁 Script executed:
# Look for any related proportional calculations in the codebase to understand the pattern
rg "/10000|/1000|/100" src/MoonLight/Nodes/Effects/ -B 2 -A 2 | head -50Repository: MoonModules/MoonLight
Length of output: 1347
Revisit the star fill ratio calculation for intuitive percentage-based control.
The control labeled "star fill" currently uses a divisor of 10000, producing only 1 star at the default value (ratio=1) and ~26 stars at maximum (ratio=255) for a 1000-LED setup. This results in 0.1–2.6% star density, making the slider feel unintuitive despite the "star fill" label suggesting a percentage-based control.
Consider changing the divisor to 100 or 1000 to align with user expectations: a divisor of 100 would yield 10 stars at default and 2550 at max (more reasonable coverage), while 1000 would yield 1 star at default but 255 at max, providing better granularity across the 0–255 slider range.
🤖 Prompt for AI Agents
In @src/MoonLight/Nodes/Effects/E_MoonLight.h at line 65, The star density calc
using nb_stars = ((uint32_t)star_fill_ratio * (uint32_t)layer->nrOfLights)/10000
+ 1 produces unintuitive low densities; update the divisor to a more intuitive
scale (e.g., 100 or 1000) and revisit the +1 offset so the slider maps sensibly
to percentage-like behavior — replace the 10000 divisor with your chosen scale
(100 or 1000) in the nb_stars computation and remove or adjust the +1 if it
causes an unwanted minimum of 1 star, ensuring nb_stars scales proportionally
with star_fill_ratio and layer->nrOfLights.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 65631 🏁 Script executed: Length of output: 9526 🏁 Script executed: Length of output: 1954 🏁 Script executed: Length of output: 8174 🏁 Script executed: Length of output: 291 🏁 Script executed: Length of output: 2971 🏁 Script executed: Length of output: 8099 🏁 Script executed: Length of output: 22123 🏁 Script executed: Length of output: 2702 🏁 Script executed: Length of output: 3487 🏁 Script executed: Length of output: 4183 @limpkin, I've identified the root cause of your crash. The issue is a null pointer dereference due to failed memory allocation. Root Cause AnalysisThe crash occurs at line 167 in memcpy(&layerP->lights.channelsE[index], channels, length);The
Looking at the backtrace, Issues Found1. Primary Issue: No null checks after memory allocation failsIn 2. Secondary Issue: StarSkyEffect doesn't validate allocationsLines 66-67 in your effect: stars_indexes = allocMB<uint16_t>(nb_stars);
stars_fade_dir = allocMB<uint8_t>(nb_stars);These allocations can fail and return null, but the code immediately uses them without checking. Recommended FixAdd null pointer checks in void setup_animation() {
freeMB(stars_indexes);
freeMB(stars_fade_dir);
// Check if layer is properly initialized
if (!layer || !layer->layerP || !layer->layerP->lights.channelsE) {
EXT_LOGE(ML_TAG, "StarSkyEffect: layer not properly initialized");
return;
}
if (layer->nrOfLights == 0) { return; }
layer->fill_solid(CRGB::Black);
nb_stars = ((uint32_t)star_fill_ratio * (uint32_t)layer->nrOfLights)/10000 + 1;
stars_indexes = allocMB<uint16_t>(nb_stars);
stars_fade_dir = allocMB<uint8_t>(nb_stars);
// Check if allocations succeeded
if (!stars_indexes || !stars_fade_dir) {
EXT_LOGE(ML_TAG, "StarSkyEffect: Failed to allocate memory for %d stars", nb_stars);
freeMB(stars_indexes);
freeMB(stars_fade_dir);
nb_stars = 0;
return;
}
EXT_LOGD(ML_TAG, "StarSkyEffect: %d stars added for a total of %d pixels", nb_stars, layer->nrOfLights);
for (uint32_t i = 0; i < nb_stars; i++) {
stars_indexes[i] = random16(layer->nrOfLights);
stars_fade_dir[i] = random8(2);
uint8_t start_brightness = random8();
EXT_LOGD(ML_TAG, "StarSkyEffect: using pixel #%d, start brightness %d, fade dir %d", stars_indexes[i], start_brightness, stars_fade_dir[i]);
layer->setRGB(stars_indexes[i], CRGB(start_brightness, start_brightness, start_brightness));
}
}Also add a guard in void loop20ms() override {
if (!stars_indexes || !stars_fade_dir || nb_stars == 0) return;
if (local_clock++ == slowness) {
// ... rest of the codeWhy This HappensYour ESP32 is running out of memory during initialization. The error suggests you may have:
Check your memory availability and LED configuration to ensure you have enough RAM/PSRAM for your setup. 🧠 Learnings used |
Author
|
@ewowi we're good for merge, AI isn't very useful here |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/MoonLight/Nodes/Effects/E_MoonLight.h:
- Around line 62-67: setup_animation() frees the star buffers but can return
early when layer->nrOfLights == 0, leaving nb_stars and star pointer variables
(stars_indexes, stars_fade_dir, stars_brightness) in a stale state and causing
potential use-after-free in loop20ms(); after calling freeMB(...) ensure you set
nb_stars = 0 and explicitly set stars_indexes, stars_fade_dir, and
stars_brightness to nullptr before any early return, and apply the same
nulling/nb_stars reset to the other similar cleanup block covering the 100-117
region so no freed pointers or stale counts remain referenced later.
- Around line 91-93: The onUpdate method in E_MoonLight is missing the override
specifier, which can allow silent signature drift; locate the onUpdate method
that takes a Char<20> and a JsonObject and add the override specifier to its
declaration (making it consistent with onSizeChanged) while keeping the body
that calls setup_animation().
- Around line 100-117: In loop20ms(), the increment/decrement of
stars_brightness (used in stars_brightness[i] = stars_brightness[i] ± 1) can
wrap uint8_t and break the stars_fade_dir checks; fix by using saturating
arithmetic or explicit clamps when updating brightness in the loop (e.g.,
replace direct +/- with qadd8/qsub8 or check bounds before modifying) and ensure
after updating you set stars_fade_dir[i] when brightness hits 0 or UINT8_MAX
(and still allow the random flip). Update references to stars_brightness,
stars_fade_dir, and loop20ms to use the saturating operation so no
overflow/underflow occurs.
🧹 Nitpick comments (1)
src/MoonLight/Nodes/Effects/E_MoonLight.h (1)
68-71: “star fill” control range vsnb_starsmath is unclear
nb_stars = star_fill_ratio * nrOfLights / 10000 + 1makesstar filleffectively basis-points (and forces at least 1 star even if set to 0). If the UI intends percent, this should likely be/ 100and slider max 100; if it intends “density”, consider renaming and/or setting explicit min/max.Also applies to: 94-98
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/MoonLight/Nodes/Effects/E_MoonLight.h
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
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: 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: 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.
📚 Learning: 2026-01-06T15:24:29.770Z
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.
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
📚 Learning: 2026-01-06T13:45:03.067Z
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).
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
📚 Learning: 2025-12-18T15:15:57.828Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-18T15:15:57.828Z
Learning: In DripEffect (E_WLED.h), dropsSize must be set to layer->size.y (not layer->size.x) in onSizeChanged(), because the drops array is allocated as [layer->size.y][maxNumDrops]. Using layer->size.x causes out-of-bounds access when iterating over y-coordinates.
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
📚 Learning: 2026-01-06T15:24:17.118Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:164-171
Timestamp: 2026-01-06T15:24:17.118Z
Learning: In VirtualLayer::setLight and VirtualLayer::getLight (src/MoonLight/Layers/VirtualLayer.cpp), bounds checking in the unmapped path (when indexV >= mappingTableSize or oneToOneMapping is true) is intentionally disabled for performance when driving very large LED counts (100K+). Effect developers are responsible for ensuring their code stays within bounds. This is an explicit design decision prioritizing performance over defensive checks in the hot path.
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
📚 Learning: 2025-12-28T10:28:24.157Z
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.
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
📚 Learning: 2025-12-18T15:59:30.556Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-18T15:59:30.556Z
Learning: In DripEffect (E_WLED.h), the bounce fails because the damped velocity is too weak to keep the drop at pos > 0 for multiple frames. When the drop returns to pos <= 0 while still in the bouncing state (colIndex == bouncing), it immediately resets to init (line 1663-1664) before any visible bounce occurs. The bounce velocity must be strong enough (divide by 1.1 or less, or use minimal damping like *= 0.9) so the drop stays airborne long enough for a visible bounce arc before the eventual second ground hit triggers the reset.
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
🧬 Code graph analysis (1)
src/MoonLight/Nodes/Effects/E_MoonLight.h (4)
src/MoonBase/Nodes.h (2)
Node(67-182)Node(238-238)src/MoonLight/Nodes/Effects/E_MoonModules.h (2)
onSizeChanged(183-199)onUpdate(120-124)src/MoonBase/Nodes.cpp (2)
onUpdate(401-565)onUpdate(401-401)src/MoonLight/Layers/VirtualLayer.cpp (2)
loop20ms(71-75)loop20ms(71-71)
🔇 Additional comments (1)
src/MoonLight/Nodes/Effects/E_MoonLight.h (1)
62-68: [Rewritten review comment]
[Classification tag]
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/MoonLight/Nodes/Effects/E_MoonLight.h:
- Around line 78-100: setup_animation() takes swapMutex via xSemaphoreTake but
returns early when layer is null or empty without releasing it; before the "if
(!layer || layer->nrOfLights == 0) return;" add xSemaphoreGive(swapMutex) (or
restructure to only take the mutex after the null/empty check) so the semaphore
is always released on every return path; also verify other early-return paths
(allocation failures) already call xSemaphoreGive(swapMutex) as shown.
- Around line 111-112: loop20ms currently checks nb_stars, stars_indexes,
stars_fade_dir and stars_brightness but does not verify that layer is non-null;
add a guard at the start of E_MoonLight::loop20ms to return early if layer is
null (in addition to the existing pointer checks) so subsequent accesses to
layer (used later in loop20ms and set up by setup_animation()) cannot
dereference a null pointer.
- Around line 119-134: The loop handling star fades overflows/underflows because
it increments/decrements stars_brightness[i] before checking bounds; fix by
checking brightness limits before arithmetic in the loop in E_MoonLight (use the
stars_brightness[], stars_fade_dir[], stars_indexes[], layer->setRGB(),
random8(), random16(), layer->nrOfLights symbols). Specifically: when
stars_fade_dir[i] == 1, if stars_brightness[i] == UINT8_MAX then set fade_dir to
0 (and write max brightness) else increment and write; when stars_fade_dir[i] ==
0, if stars_brightness[i] == 0 then pick a new stars_indexes[i], set fade_dir to
1 (and write zero brightness) else decrement and write; preserve the random8()
chance to flip but apply it after safely updating without causing wraparound.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/MoonLight/Nodes/Effects/E_MoonLight.h
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
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: 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.
📚 Learning: 2025-12-28T10:28:24.157Z
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.
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
📚 Learning: 2026-01-06T15:24:29.770Z
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.
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
📚 Learning: 2026-01-06T13:45:03.067Z
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).
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
📚 Learning: 2025-12-18T15:15:57.828Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-18T15:15:57.828Z
Learning: In DripEffect (E_WLED.h), dropsSize must be set to layer->size.y (not layer->size.x) in onSizeChanged(), because the drops array is allocated as [layer->size.y][maxNumDrops]. Using layer->size.x causes out-of-bounds access when iterating over y-coordinates.
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
📚 Learning: 2026-01-06T15:24:17.118Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:164-171
Timestamp: 2026-01-06T15:24:17.118Z
Learning: In VirtualLayer::setLight and VirtualLayer::getLight (src/MoonLight/Layers/VirtualLayer.cpp), bounds checking in the unmapped path (when indexV >= mappingTableSize or oneToOneMapping is true) is intentionally disabled for performance when driving very large LED counts (100K+). Effect developers are responsible for ensuring their code stays within bounds. This is an explicit design decision prioritizing performance over defensive checks in the hot path.
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
🧬 Code graph analysis (1)
src/MoonLight/Nodes/Effects/E_MoonLight.h (2)
src/MoonBase/Nodes.h (2)
Node(67-182)Node(238-238)src/MoonLight/Nodes/Effects/E_MoonModules.h (2)
onSizeChanged(183-199)onUpdate(120-124)
🔇 Additional comments (3)
src/MoonLight/Nodes/Effects/E_MoonLight.h (3)
58-65: Constructor and destructor properly initialize and clean up resources.The mutex initialization and cleanup pattern is correct. Note that
xSemaphoreCreateMutex()creates a mutex in the "given" (unlocked) state by default, so line 60'sxSemaphoreGive()is redundant but harmless.
67-76: LGTM! Proper cleanup pattern.The cleanup correctly frees all allocated buffers, nullifies pointers, and resets state. This prevents use-after-free and ensures loop20ms() will skip rendering when nb_stars is 0.
102-109: LGTM! Proper lifecycle management.The effect correctly re-initializes on size changes and when the "star fill" parameter changes. The filtering in
onUpdateprevents unnecessary re-initialization.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/MoonLight/Nodes/Effects/E_MoonLight.h:
- Around line 58-61: StarSkyEffect's constructor calls xSemaphoreCreateMutex()
without checking for NULL and then unconditionally calls
xSemaphoreGive(swapMutex); update the constructor to check the return of
xSemaphoreCreateMutex(), assign swapMutex to NULL on failure, and only call
xSemaphoreGive(swapMutex) when swapMutex is non-NULL; also add an appropriate
failure handling path (e.g., log an error via your logger, set an error flag, or
assert) so the code never dereferences a NULL swapMutex.
- Around line 78-100: The function setup_animation acquires swapMutex with
xSemaphoreTake but can return early when (!layer || layer->nrOfLights == 0)
without releasing it, causing a deadlock; fix by ensuring the mutex is released
before any early return (e.g., call xSemaphoreGive(swapMutex) immediately before
the return in setup_animation) or alternatively move the (!layer ||
layer->nrOfLights == 0) check to before xSemaphoreTake to avoid acquiring the
mutex unnecessarily; update setup_animation to always pair
xSemaphoreTake(swapMutex) with a matching xSemaphoreGive(swapMutex) on all
control paths.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/MoonLight/Nodes/Effects/E_MoonLight.h
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
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: 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: 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: 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.
📚 Learning: 2025-12-28T10:28:24.157Z
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.
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
📚 Learning: 2026-01-06T15:24:29.770Z
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.
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
📚 Learning: 2025-12-18T15:15:57.828Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-18T15:15:57.828Z
Learning: In DripEffect (E_WLED.h), dropsSize must be set to layer->size.y (not layer->size.x) in onSizeChanged(), because the drops array is allocated as [layer->size.y][maxNumDrops]. Using layer->size.x causes out-of-bounds access when iterating over y-coordinates.
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
📚 Learning: 2026-01-06T13:45:03.067Z
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).
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
📚 Learning: 2026-01-06T15:24:17.118Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:164-171
Timestamp: 2026-01-06T15:24:17.118Z
Learning: In VirtualLayer::setLight and VirtualLayer::getLight (src/MoonLight/Layers/VirtualLayer.cpp), bounds checking in the unmapped path (when indexV >= mappingTableSize or oneToOneMapping is true) is intentionally disabled for performance when driving very large LED counts (100K+). Effect developers are responsible for ensuring their code stays within bounds. This is an explicit design decision prioritizing performance over defensive checks in the hot path.
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
📚 Learning: 2025-12-18T15:59:30.556Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-18T15:59:30.556Z
Learning: In DripEffect (E_WLED.h), the bounce fails because the damped velocity is too weak to keep the drop at pos > 0 for multiple frames. When the drop returns to pos <= 0 while still in the bouncing state (colIndex == bouncing), it immediately resets to init (line 1663-1664) before any visible bounce occurs. The bounce velocity must be strong enough (divide by 1.1 or less, or use minimal damping like *= 0.9) so the drop stays airborne long enough for a visible bounce arc before the eventual second ground hit triggers the reset.
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
🔇 Additional comments (5)
src/MoonLight/Nodes/Effects/E_MoonLight.h (5)
67-76: LGTM!The cleanup logic correctly frees all allocated buffers, nullifies pointers, and resets state variables.
102-103: LGTM!Both methods correctly trigger animation restart on relevant changes. The conditional check in
onUpdateensuressetup_animationis only called when the "star fill" control changes.
105-109: LGTM!Standard setup pattern: registers UI controls and initializes the animation state.
86-91: Good allocation error handling.The memory allocation checks on line 86, combined with proper cleanup via
resetStars()and mutex release, correctly handle allocation failures. This prevents the effect from running with partially-allocated buffers.Based on PR objectives' recommendation to check allocMB return values.
111-138: Validate layer and physical buffers before calling setRGB.Based on the PR objectives, the reported crash occurs when
layer->setRGB()is invoked whilelayer->layerP->lights.channelsEis null. The guard on line 112 checks effect-local state but doesn't verify that the underlying physical layer buffers are allocated and ready.According to the crash analysis in the PR discussion, memory allocation failures can leave
channelsEnull while the code continues execution, and setup ordering may cause this method to run before physical buffers are allocated.🛡️ Proposed defensive checks
void loop20ms() override { if (nb_stars == 0 || !stars_indexes || !stars_fade_dir || !stars_brightness) return; + if (!layer || !layer->layerP || !layer->layerP->lights.channelsE) { + EXT_LOGE(ML_TAG, "StarSkyEffect: layer infrastructure not ready"); + return; + } if (local_clock++ == slowness) { xSemaphoreTake(swapMutex, portMAX_DELAY);Based on PR objectives and crash analysis.
⛔ Skipped due to learnings
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: 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: 93 File: src/MoonLight/Layers/VirtualLayer.cpp:164-171 Timestamp: 2026-01-06T15:24:17.118Z Learning: In VirtualLayer::setLight and VirtualLayer::getLight (src/MoonLight/Layers/VirtualLayer.cpp), bounds checking in the unmapped path (when indexV >= mappingTableSize or oneToOneMapping is true) is intentionally disabled for performance when driving very large LED counts (100K+). Effect developers are responsible for ensuring their code stays within bounds. This is an explicit design decision prioritizing performance over defensive checks in the hot path.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: 2025-12-18T15:15:57.828Z Learning: In DripEffect (E_WLED.h), dropsSize must be set to layer->size.y (not layer->size.x) in onSizeChanged(), because the drops array is allocated as [layer->size.y][maxNumDrops]. Using layer->size.x causes out-of-bounds access when iterating over y-coordinates.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.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @src/MoonBase/Modules/ModuleIO.h:
- Around line 809-817: current_board_id is accessed from multiple tasks (written
in setBoardPresetDefaults() called from loop() and read in loop1s()), causing a
data race; change current_board_id to an atomic type (e.g.,
std::atomic<uint8_t>) or protect all accesses with a mutex/semaphore: update the
declaration of current_board_id to std::atomic<uint8_t> (include <atomic>),
replace direct reads/writes in setBoardPresetDefaults() and loop1s() with atomic
load/store operations (or wrap reads/writes in a mutex lock/unlock if you prefer
locking), and ensure any other places that read or write current_board_id use
the same atomic/mutex pattern so visibility and atomicity are guaranteed across
tasks.
- Around line 798-801: The code currently sets volts=0 unless current_board_id
matches board_SE16V1 or board_LightCrafter16, which causes a silent 0V update
via batteryService->updateVoltage(volts); add a default branch that either logs
a warning (including current_board_id and raw adc_mv_vinput) or skips calling
batteryService->updateVoltage when the board is unsupported, and update the
comment next to the LightCrafter16 calculation to reflect the correct divider
ratio (10k/1.43k) and the formula using adc_mv_vinput to avoid the misleading
"1k43/10k" text; reference variables: current_board_id, board_SE16V1,
board_LightCrafter16, adc_mv_vinput, volts, batteryService->updateVoltage.
In @src/MoonLight/Nodes/Effects/E_MoonLight.h:
- Around line 78-104: In setup_animation(), handle the case where swapMutex may
be nullptr by only calling xSemaphoreTake(swapMutex, ...) and
xSemaphoreGive(swapMutex) when swapMutex != nullptr; if swapMutex is nullptr,
run the existing logic unlocked (still call resetStars(), perform allocMB for
stars_indexes/stars_fade_dir/stars_brightness, check for nulls and call
resetStars() before returning). Ensure you do not call xSemaphoreGive unless you
successfully called xSemaphoreTake, and keep the early returns consistent (i.e.,
if you return due to layer==nullptr or allocation failure, only give the
semaphore when it was taken).
- Around line 48-61: The constructor for StarSkyEffect incorrectly calls
xSemaphoreGive() after xSemaphoreCreateMutex() and doesn't check for allocation
failure, and the mutex is never freed; remove the immediate xSemaphoreGive()
call, check the result of xSemaphoreCreateMutex() and handle nullptr (e.g., set
swapMutex to nullptr and avoid using it or assert/log), and add a destructor
that calls vSemaphoreDelete(swapMutex) only if swapMutex is non-null to avoid
leaks and crashes when OOM; update any code paths that call
xSemaphoreTake/xSemaphoreGive to handle a nullptr swapMutex safely.
🧹 Nitpick comments (2)
src/MoonBase/Modules/ModuleIO.h (1)
810-813: Consider extracting magic numbers to named constants.The ADC conversion formulas contain several magic numbers (330, 250, 50.00, 37.75) that would benefit from descriptive names for maintainability.
Suggested refactor
At class or file scope:
// SE16V1 current sensing constants static constexpr uint32_t SE16V1_CURRENT_QUIESCENT_MV = 250; static constexpr float SE16V1_CURRENT_MV_PER_MA = 0.02f; // 50 mA/mV = 1/0.02 mV/mA // LightCrafter16 current sensing constants static constexpr uint32_t LC16_CURRENT_QUIESCENT_MV = 330; static constexpr float LC16_CURRENT_MV_PER_MA = 0.0265f; // 37.75 mA/mV // Threshold for valid current reading static constexpr uint32_t CURRENT_SENSE_THRESHOLD_MV = 330;Then in the calculation:
-if (adc_mv_cinput > 330) +if (adc_mv_cinput > CURRENT_SENSE_THRESHOLD_MV) { - if (current_board_id == board_SE16V1) { batteryService->updateCurrent((((float)(adc_mv_cinput)-250) * 50.00) / 1000); } - else if (current_board_id == board_LightCrafter16) { batteryService->updateCurrent((((float)(adc_mv_cinput)-330) * 37.75) / 1000); } + if (current_board_id == board_SE16V1) { + batteryService->updateCurrent(((float)(adc_mv_cinput - SE16V1_CURRENT_QUIESCENT_MV)) / SE16V1_CURRENT_MV_PER_MA / 1000); + } + else if (current_board_id == board_LightCrafter16) { + batteryService->updateCurrent(((float)(adc_mv_cinput - LC16_CURRENT_QUIESCENT_MV)) / LC16_CURRENT_MV_PER_MA / 1000); + } }This makes the hardware parameters explicit and easier to update if board revisions change.
src/MoonLight/Nodes/Effects/E_MoonLight.h (1)
106-113: Set explicit bounds forstar fillslider and clarify density scaling logic.
star_fill_ratiois auint8_tslider with no explicit bounds inaddControl(), which is inconsistent with other slider controls in the codebase (e.g.,addControl(width, "width", "slider", 1, 20)). The formula(star_fill_ratio * nrOfLights) / 10000implements an undocumented density scale. Consider:
- Adding bounds to the
addControl()call to match the codebase convention and limit the range (e.g.,addControl(star_fill_ratio, "star fill", "slider", 0, 255))- Documenting the intended scale (appears to be "stars as percentage density: 0-255 represents ~0-2.55% of pixels")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/MoonBase/Modules/ModuleIO.hsrc/MoonLight/Nodes/Effects/E_MoonLight.h
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
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: 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: 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.
📚 Learning: 2025-12-28T10:28:24.157Z
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.
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
📚 Learning: 2026-01-06T15:24:29.770Z
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.
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
📚 Learning: 2025-12-18T15:15:57.828Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-18T15:15:57.828Z
Learning: In DripEffect (E_WLED.h), dropsSize must be set to layer->size.y (not layer->size.x) in onSizeChanged(), because the drops array is allocated as [layer->size.y][maxNumDrops]. Using layer->size.x causes out-of-bounds access when iterating over y-coordinates.
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
📚 Learning: 2026-01-06T13:45:03.067Z
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).
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
📚 Learning: 2026-01-06T15:24:17.118Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:164-171
Timestamp: 2026-01-06T15:24:17.118Z
Learning: In VirtualLayer::setLight and VirtualLayer::getLight (src/MoonLight/Layers/VirtualLayer.cpp), bounds checking in the unmapped path (when indexV >= mappingTableSize or oneToOneMapping is true) is intentionally disabled for performance when driving very large LED counts (100K+). Effect developers are responsible for ensuring their code stays within bounds. This is an explicit design decision prioritizing performance over defensive checks in the hot path.
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
📚 Learning: 2025-12-18T15:59:30.556Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-18T15:59:30.556Z
Learning: In DripEffect (E_WLED.h), the bounce fails because the damped velocity is too weak to keep the drop at pos > 0 for multiple frames. When the drop returns to pos <= 0 while still in the bouncing state (colIndex == bouncing), it immediately resets to init (line 1663-1664) before any visible bounce occurs. The bounce velocity must be strong enough (divide by 1.1 or less, or use minimal damping like *= 0.9) so the drop stays airborne long enough for a visible bounce arc before the eventual second ground hit triggers the reset.
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
📚 Learning: 2025-12-25T13:34:40.097Z
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.
Applied to files:
src/MoonLight/Nodes/Effects/E_MoonLight.h
🧬 Code graph analysis (1)
src/MoonLight/Nodes/Effects/E_MoonLight.h (2)
src/MoonBase/Nodes.h (2)
Node(67-182)Node(238-238)src/MoonLight/Layers/VirtualLayer.cpp (2)
loop20ms(71-75)loop20ms(71-71)
🔇 Additional comments (3)
src/MoonBase/Modules/ModuleIO.h (2)
241-241: LGTM: Board ID tracking initialized correctly.The board ID is properly stored when presets are applied, enabling board-specific ADC conversions downstream.
824-824: Field declaration looks correct.The default value
UINT8_MAXappropriately indicates an uninitialized state. However, ensure thread-safe access as flagged in the earlier comment regarding lines 809-817.src/MoonLight/Nodes/Effects/E_MoonLight.h (1)
115-142: The proposedchannelsEguard is unnecessary and conflicts with the codebase design.
channelsEallocation is guaranteed before effects run (PhysicalLayer::setup() completes during global setup() before the effect loop starts). More importantly, the intentional omission of null checks in VirtualLayer::setLight() is a performance requirement for 100K+ LED counts—bounds checking is disabled by design (not a safety gap). Adding this guard contradicts that documented tradeoff.The
swapMutexnull-check has some merit as defensive coding (xSemaphoreCreateMutex() can fail), but the effect's local swapMutex is a separate concern from the PR objectives. If mutex creation fails, the effect should be disabled upstream, not silently skipped at runtime.Likely an incorrect or invalid review comment.
Comment on lines
+798
to
801
| float volts = 0; | ||
| if (current_board_id == board_SE16V1) { volts = ((float)adc_mv_vinput) * 2 / 1000; } // /2 resistor divider | ||
| else if (current_board_id == board_LightCrafter16) { volts = ((float)adc_mv_vinput) * 11.43 / (1.43 * 1000); } // 1k43/10k resistor divider | ||
| batteryService->updateVoltage(volts); |
There was a problem hiding this comment.
Add default case or log warning for unsupported boards.
If current_board_id is neither board_SE16V1 nor board_LightCrafter16, volts remains 0 and updateVoltage(0) is called. This silently reports 0V for boards with voltage sensing pins configured but no conversion formula implemented, which could mask configuration issues.
Additionally, the comment on Line 800 states "1k43/10k resistor divider" but the calculation 11.43 / (1.43 * 1000) suggests a 10k/1.43k divider ratio.
Suggested improvements
Consider one of these approaches:
Option 1: Log a warning for unsupported boards
float volts = 0;
if (current_board_id == board_SE16V1) { volts = ((float)adc_mv_vinput) * 2 / 1000; } // /2 resistor divider
-else if (current_board_id == board_LightCrafter16) { volts = ((float)adc_mv_vinput) * 11.43 / (1.43 * 1000); } // 1k43/10k resistor divider
+else if (current_board_id == board_LightCrafter16) { volts = ((float)adc_mv_vinput) * 11.43 / (1.43 * 1000); } // 10k/1.43k resistor divider
+else {
+ EXT_LOGW(MB_TAG, "No voltage conversion for board_id %d", current_board_id);
+}
batteryService->updateVoltage(volts);Option 2: Skip the update entirely if unsupported
float volts = 0;
+bool voltage_calculated = false;
-if (current_board_id == board_SE16V1) { volts = ((float)adc_mv_vinput) * 2 / 1000; } // /2 resistor divider
-else if (current_board_id == board_LightCrafter16) { volts = ((float)adc_mv_vinput) * 11.43 / (1.43 * 1000); } // 1k43/10k resistor divider
+if (current_board_id == board_SE16V1) {
+ volts = ((float)adc_mv_vinput) * 2 / 1000;
+ voltage_calculated = true;
+} else if (current_board_id == board_LightCrafter16) {
+ volts = ((float)adc_mv_vinput) * 11.43 / (1.43 * 1000); // 10k/1.43k resistor divider
+ voltage_calculated = true;
+}
+if (voltage_calculated) {
-batteryService->updateVoltage(volts);
+ batteryService->updateVoltage(volts);
+}📝 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
| float volts = 0; | |
| if (current_board_id == board_SE16V1) { volts = ((float)adc_mv_vinput) * 2 / 1000; } // /2 resistor divider | |
| else if (current_board_id == board_LightCrafter16) { volts = ((float)adc_mv_vinput) * 11.43 / (1.43 * 1000); } // 1k43/10k resistor divider | |
| batteryService->updateVoltage(volts); | |
| float volts = 0; | |
| if (current_board_id == board_SE16V1) { volts = ((float)adc_mv_vinput) * 2 / 1000; } // /2 resistor divider | |
| else if (current_board_id == board_LightCrafter16) { volts = ((float)adc_mv_vinput) * 11.43 / (1.43 * 1000); } // 10k/1.43k resistor divider | |
| else { | |
| EXT_LOGW(MB_TAG, "No voltage conversion for board_id %d", current_board_id); | |
| } | |
| batteryService->updateVoltage(volts); |
Suggested change
| float volts = 0; | |
| if (current_board_id == board_SE16V1) { volts = ((float)adc_mv_vinput) * 2 / 1000; } // /2 resistor divider | |
| else if (current_board_id == board_LightCrafter16) { volts = ((float)adc_mv_vinput) * 11.43 / (1.43 * 1000); } // 1k43/10k resistor divider | |
| batteryService->updateVoltage(volts); | |
| float volts = 0; | |
| bool voltage_calculated = false; | |
| if (current_board_id == board_SE16V1) { | |
| volts = ((float)adc_mv_vinput) * 2 / 1000; | |
| voltage_calculated = true; | |
| } else if (current_board_id == board_LightCrafter16) { | |
| volts = ((float)adc_mv_vinput) * 11.43 / (1.43 * 1000); // 10k/1.43k resistor divider | |
| voltage_calculated = true; | |
| } | |
| if (voltage_calculated) { | |
| batteryService->updateVoltage(volts); | |
| } |
🤖 Prompt for AI Agents
In @src/MoonBase/Modules/ModuleIO.h around lines 798 - 801, The code currently
sets volts=0 unless current_board_id matches board_SE16V1 or
board_LightCrafter16, which causes a silent 0V update via
batteryService->updateVoltage(volts); add a default branch that either logs a
warning (including current_board_id and raw adc_mv_vinput) or skips calling
batteryService->updateVoltage when the board is unsupported, and update the
comment next to the LightCrafter16 calculation to reflect the correct divider
ratio (10k/1.43k) and the formula using adc_mv_vinput to avoid the misleading
"1k43/10k" text; reference variables: current_board_id, board_SE16V1,
board_LightCrafter16, adc_mv_vinput, volts, batteryService->updateVoltage.
Comment on lines
+809
to
817
| if ((current_board_id == board_SE16V1) || (current_board_id == board_LightCrafter16)) { | ||
| if (adc_mv_cinput > 330) // datasheet quiescent output voltage of 0.5V, which is ~330mV after the 10k/5k1 voltage divider. Ideally, this value should be measured at boot when nothing is displayed on the LEDs | ||
| { | ||
| if (current_board_id == board_SE16V1) { batteryService->updateCurrent((((float)(adc_mv_cinput)-250) * 50.00) / 1000); } // 40mV / A with a /2 resistor divider, so a 50mA/mV | ||
| else if (current_board_id == board_LightCrafter16) { batteryService->updateCurrent((((float)(adc_mv_cinput)-330) * 37.75) / 1000); } // 40mV / A with a 10k/5k1 resistor divider, so a 37.75mA/mV | ||
| } else { | ||
| batteryService->updateCurrent(0); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential race condition: unsynchronized access to current_board_id.
current_board_id is written in setBoardPresetDefaults() (called from loop() running in the sveltekit task, line 602) and read here in loop1s() (likely a different task). Without synchronization, this creates a data race where:
loop1s()might readcurrent_board_idwhile it's being updatedloop1s()might read stale values due to cache coherency- If
loop1s()runs beforesetBoardPresetDefaults()is called,current_board_idwill beUINT8_MAXand current updates will be skipped silently
Recommended fix: Add volatile qualifier or mutex protection
Option 1: Use volatile for simple visibility (minimal fix)
-uint8_t current_board_id = UINT8_MAX;
+volatile uint8_t current_board_id = UINT8_MAX;Option 2: Use atomic access (preferred for ESP-IDF)
+#include <atomic>
private:
ESP32SvelteKit* _sveltekit;
- uint8_t current_board_id = UINT8_MAX;
+ std::atomic<uint8_t> current_board_id{UINT8_MAX};Note: volatile provides visibility but not atomicity. For uint8_t on ESP32, reads/writes are atomic, so volatile should suffice. However, std::atomic is clearer about intent.
Based on learnings, MoonLight tasks can run on different cores with preemption, making thread-safe access patterns critical.
🤖 Prompt for AI Agents
In @src/MoonBase/Modules/ModuleIO.h around lines 809 - 817, current_board_id is
accessed from multiple tasks (written in setBoardPresetDefaults() called from
loop() and read in loop1s()), causing a data race; change current_board_id to an
atomic type (e.g., std::atomic<uint8_t>) or protect all accesses with a
mutex/semaphore: update the declaration of current_board_id to
std::atomic<uint8_t> (include <atomic>), replace direct reads/writes in
setBoardPresetDefaults() and loop1s() with atomic load/store operations (or wrap
reads/writes in a mutex lock/unlock if you prefer locking), and ensure any other
places that read or write current_board_id use the same atomic/mutex pattern so
visibility and atomicity are guaranteed across tasks.
This was referenced Jan 11, 2026
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
✏️ Tip: You can customize this high-level summary in your review settings.