prep71 #106
prep71 #106
Conversation
Docs ==== - Installer updates Frontend ======= - page: add Control Backend ======= - Module: updatePending not static (WIP), updateMutex
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Docs & Frontend docs/gettingstarted/installer.md, interface/src/routes/+page.svelte |
Expanded installer hardware options (added ESP32-S3 n16r8 and ESP32-P4-Nano, updated LED counts/images) and added a new "Control" step (step 5) to the landing/setup page. |
Module state & concurrency src/MoonBase/Module.h, src/MoonBase/Module.cpp |
Converted updatePending from a static member to a per-instance field; changed updateMutex initialization to lazy creation (initialize on demand); added UpdatedItem ModuleState::updatedItem and Char<20> ModuleState::updateOriginId static members. |
Parlio / driver layout src/MoonLight/Nodes/Drivers/parlio.cpp |
Increased first_index_per_output array size from 16 to SOC_PARLIO_TX_UNIT_MAX_DATA_WIDTH; added a compile-time static_assert; fixed a loop boundary (i < outputs) and replaced Serial logging with ESP_LOGD/EXT_LOGD. |
Logging & formatting sweep src/MoonBase/Modules/ModuleIO.h, src/MoonBase/Nodes.cpp, src/MoonBase/Utilities.cpp, src/MoonBase/NodeManager.h, src/MoonLight/Layers/VirtualLayer.cpp, src/MoonLight/Modules/ModuleDrivers.h, src/MoonLight/Modules/ModuleEffects.h, src/MoonLight/Modules/ModuleLightsControl.h, src/MoonLight/Nodes/Drivers/D_Infrared.h, src/MoonLight/Nodes/Effects/E_MoonLight.h, src/MoonLight/Nodes/Effects/E_WLED.h, src/MoonLight/Nodes/Modifiers/M_MoonLight.h |
Numerous log changes: level adjustments (EXT_LOGD ↔ EXT_LOGI), removal of trailing newlines in debug strings, replacing Serial.printf with logging macros, conditional guarding for per-pin logs, and several debug lines commented out or removed. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
- Module: support button, new rows, nested tables #1 — Related edits to
ModuleState/UpdatedItemand update-tracking state. - Board presets ESP32-P4 #72 — Changes to
parlio.cpp(array sizing, setup logic, logging) overlap with this PR. - v0.7.0 preparations #76 — Modifications to
NodeManager::checkAndAlloc<T>and related logging behavior.
Suggested labels
enhancement
Poem
🐰 Hops and hums, my whiskers twitch,
Logs trimmed tidy, code less glitch.
Arrays stretched wide, new boards in sight,
States asleep until the light.
A little rabbit cheers tonight! 🥕
🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (1 warning, 1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 12.12% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. | |
| Title check | ❓ Inconclusive | The title 'prep71' is vague and does not convey meaningful information about the changeset. It appears to be a version number or internal label rather than a descriptive summary of the changes. | Use a descriptive title that summarizes the main change, such as 'Refactor module state management and update logging semantics' or similar, to clearly communicate the purpose of this pull request. |
✅ Passed checks (1 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
✏️ 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/MoonLight/Nodes/Effects/E_MoonLight.h (1)
1092-1172: Fix printf specifiers in debug logs to match argument types (avoid UB with varargs).At least
nearestDistisunsigned, but is logged with%d(Line 1118). On some toolchains/printf implementations, mismatched varargs types can produce incorrect output or undefined behavior.Proposed fix
- if (debugPrint) EXT_LOGD(ML_TAG, " %d, %d, %d, Not Mapped! Nearest: %d, %d, %d dist: %d diff: %d", newPos.x, newPos.y, newPos.z, nearestMapped.x, nearestMapped.y, nearestMapped.z, nearestDist, diff); + if (debugPrint) EXT_LOGD(ML_TAG, + " %d, %d, %d, Not Mapped! Nearest: %d, %d, %d dist: %u diff: %d", + newPos.x, newPos.y, newPos.z, + nearestMapped.x, nearestMapped.y, nearestMapped.z, + (unsigned)nearestDist, diff);src/MoonLight/Nodes/Drivers/parlio.cpp (1)
292-365: Fix off-by-one condition and document the IRAM_ATTR logging concern.
- Line 359:
i <= outputsis an off-by-one error. Since the prior condition already handlesi >= outputs, the else branch will still match wheni == outputs. Usei < outputsfor correct array bounds.- Lines 354/362:
EXT_LOGDcalls inshow_parlio()(markedIRAM_ATTR) invokepcTaskGetName()anduxTaskGetStackHighWaterMark(), which are unsafe if called from ISR or with flash cache disabled. Mitigated by the setup conditional block (setup rarely re-triggers), but should still be documented or moved outsideIRAM_ATTRif setup can occur on hot paths on your targets.Off-by-one fix
- } else if (i <= outputs) { + } else if (i < outputs) { if (parlio_config.data_gpio_nums[i] == -1) status = "[missing]"; }
🤖 Fix all issues with AI agents
In `@docs/gettingstarted/installer.md`:
- Line 16: The phrase "plug and play device" should be written as the compound
modifier "plug-and-play device"; update the sentence "Recommended plug and play
device to get started with, up to 4096 LEDs: QuinLED-dig2go" to use
"plug-and-play" as the hyphenated adjective modifying "device" so it reads
"Recommended plug-and-play device to get started with, up to 4096 LEDs:
QuinLED-dig2go".
In `@src/MoonBase/Module.cpp`:
- Around line 38-42: The static shared state (ModuleState::updateOriginId and
ModuleState::updatedItem) is causing data races because ModuleState::update()
writes them without synchronization while onUpdate handlers read them; change
these static members into instance members on ModuleState (e.g., move
updateOriginId and updatedItem into the ModuleState object and remove the static
declarations), update ModuleState::update() to write to
this->updateOriginId/this->updatedItem, and adjust all callers (onUpdate
handlers in ModuleIO.h, Module.h and any postUpdate()/postUpdateMutex usage) to
read the per-instance fields or accept the originId/updatedItem as parameters
instead of accessing globals; ensure updateMutex either becomes per-instance or
remains static but protects only truly-shared resources.
In `@src/MoonBase/Module.h`:
- Around line 53-60: Move updateMutex creation to immediate member
initialization to match project conventions used in PhysicalLayer (e.g.,
initialize updateMutex = xSemaphoreCreateMutex() at declaration inside
ModuleState) instead of lazy-creating it in the ModuleState() constructor; also
remove the volatile qualifier from updatePending since the mutex provides the
required synchronization. Update any constructor logging accordingly (remove the
lazy-create branch) and ensure the member declarations reference the
newly-initialized updateMutex and the non-volatile updatePending.
In `@src/MoonBase/Modules/ModuleIO.h`:
- Around line 798-806: The code calls batteryService->updateVoltage(volts) even
when current_board_id is neither board_SE16V1 nor board_LightCrafter16 leaving
volts at 0; change the logic in the block containing current_board_id, volts,
and updateVoltage so that for unknown boards you either early-return/skip
calling batteryService->updateVoltage (preferred) or set a documented default
divider before calling updateVoltage; ensure you also handle
voltage_readout_current_adc_attenuation by only calling
adc_get_adjusted_gain(...) when you actually computed volts for a supported
board (symbols: current_board_id, board_SE16V1, board_LightCrafter16, volts,
batteryService->updateVoltage, voltage_readout_current_adc_attenuation,
adc_get_adjusted_gain).
In `@src/MoonBase/Utilities.cpp`:
- Around line 64-85: The copyFile function is using the wrong log tag and log
level: replace ML_TAG with the module's MB_TAG in all EXT_LOG* calls inside
copyFile and change the failure logs from EXT_LOGD to EXT_LOGE for hard errors
(failed open/mkdir/open dst) so errors are properly reported; ensure you update
every log invocation in copyFile (the calls that report "Failed to open source
file", "Failed to create destination directory", and "Failed to open destination
file") to use MB_TAG and EXT_LOGE.
🧹 Nitpick comments (9)
src/MoonBase/NodeManager.h (1)
84-91: Keep behavior, but don’t leave commented-out logs behindThe logic/behavior in
checkAndAlloc<T>()is unchanged and looks fine; removing the noisy allocation log matches the PR’s logging cleanup. Consider deleting the commented-outEXT_LOGDline entirely (or guarding it with a logging/feature macro) to avoid accumulating dead code.interface/src/routes/+page.svelte (1)
38-42: Nice addition—consider list semantics + small whitespace cleanup
- Line 39 has a trailing space and the standalone
5line-break may render slightly oddly depending on CSS/whitespace handling.- Optional: these “steps” are an ordered sequence; consider switching the whole block to
<ol><li>for better semantics/a11y (and consistent spacing), and optionally adddata-sveltekit-preload-data="hover"on internal links for snappier navigation.src/MoonLight/Nodes/Drivers/D_Infrared.h (1)
286-292: Good log cleanup; consider replacingif (false)with a flag (#if/ config) for optional NEC dumps.Removing the trailing
\r\navoids double-newlines / inconsistent formatting when the logger already terminates lines. Since this block is currently dead, consider switching to a compile-time or runtime toggle so it can be enabled intentionally without editing code.Proposed tweak (compile-time guard)
- if (false) { + `#if` defined(ML_DEBUG_IR_NEC_FRAME) && ML_DEBUG_IR_NEC_FRAME EXT_LOGD(IR_DRIVER_TAG, "NEC frame start---"); for (size_t i = 0; i < symbol_num; i++) { EXT_LOGI(IR_DRIVER_TAG, "{%d:%d},{%d:%d}", rmt_nec_symbols[i].level0, rmt_nec_symbols[i].duration0, rmt_nec_symbols[i].level1, rmt_nec_symbols[i].duration1); } EXT_LOGI(IR_DRIVER_TAG, "---NEC frame end:"); - } + `#endif`docs/gettingstarted/installer.md (1)
24-24: Remove excessive punctuation for professional tone.The double exclamation marks (!!) make the documentation appear unprofessional. Consider using a single exclamation mark or removing it entirely.
✨ Suggested refinement
-Recommended state-of-the-art DIY board, up to 16K - 98K LEDs !!: [ESP32-P4-Nano](https://www.waveshare.com/esp32-p4-nano.htm){:target="_blank"} +Recommended state-of-the-art DIY board, up to 16K - 98K LEDs: [ESP32-P4-Nano](https://www.waveshare.com/esp32-p4-nano.htm){:target="_blank"}src/MoonLight/Modules/ModuleDrivers.h (2)
54-65: Pin logging guard usesledsPerPin[i]afterledPinscompaction—double-check array alignment.
layerP.ledPinsis compacted, butlayerP.ledsPerPinis not compacted here. IfledsPerPinis indexed by the original pin slot, this log line can become misleading (or worse, read unrelated sentinel values). If they’re intended to stay parallel, consider compactingledsPerPinin the same pass or logging based on the pre-compaction index.
158-166: INFO-level “Add … (p:%p pr:%d)” may be noisy; consider keeping it at DEBUG/VERBOSE.If nodes can be added dynamically (or frequently), raising this to
EXT_LOGIcould spam logs on embedded targets. If it’s truly “startup only,” it’s fine—otherwise considerEXT_LOGD/EXT_LOGVor a feature flag.src/MoonLight/Nodes/Effects/E_MoonLight.h (1)
1206-1245: UnconditionalEXT_LOGDinsettingUpParticles()may be too chatty in normal use.If this runs often (e.g., UI tweaking “number of Particles”/“barriers”), consider
EXT_LOGVor gating behind a debug flag to keep logs usable on-device.src/MoonLight/Nodes/Drivers/parlio.cpp (1)
14-30: Good: array resized to SOC macro; add a compile-time guard for existing bit-width assumptions.Resizing
first_index_per_outputtoSOC_PARLIO_TX_UNIT_MAX_DATA_WIDTHmatches the later loops that iterate to that bound, which is a solid safety fix.However, this file still assumes “small” widths in multiple places (e.g.,
pin_bit = (1 << pin)andbit_widthcapped at 16). Consider documenting/enforcing that assumption with a compile-time assert.Suggested guard (example)
`#include` "soc/soc_caps.h" // for SOC_PARLIO_SUPPORTED +#ifdef SOC_PARLIO_SUPPORTED +static_assert(SOC_PARLIO_TX_UNIT_MAX_DATA_WIDTH <= 16, "parlio.cpp assumes max data width <= 16 (packing/bit_width/bit shifts)."); +#endifsrc/MoonBase/Modules/ModuleIO.h (1)
607-660:readPins()ethernet config:EXT_LOGImay be too chatty for a hot path.If
readPins()runs frequently (it looks tied to an update handler), logging “Try to configure ethernet” at info level could spam logs and add overhead; consider gating (only log when pins changed / when config is complete / once per boot) or keeping it at debug. Also, minor type-safety: the lambda takesuint8_t gpioand assigns intoint8_t&; it’s safe for ESP32-S3 GPIO ranges, but makinggpioanint(orint8_t) would avoid accidental narrowing if reused elsewhere.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
docs/gettingstarted/installer.mdinterface/src/routes/+page.sveltelib/framework/WWWData.hsrc/MoonBase/Module.cppsrc/MoonBase/Module.hsrc/MoonBase/Modules/ModuleIO.hsrc/MoonBase/NodeManager.hsrc/MoonBase/Nodes.cppsrc/MoonBase/Utilities.cppsrc/MoonLight/Layers/VirtualLayer.cppsrc/MoonLight/Modules/ModuleDrivers.hsrc/MoonLight/Modules/ModuleEffects.hsrc/MoonLight/Modules/ModuleLightsControl.hsrc/MoonLight/Nodes/Drivers/D_Infrared.hsrc/MoonLight/Nodes/Drivers/parlio.cppsrc/MoonLight/Nodes/Effects/E_MoonLight.hsrc/MoonLight/Nodes/Effects/E_WLED.hsrc/MoonLight/Nodes/Modifiers/M_MoonLight.h
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Modify C++ backend code in the `src/` and `lib/` directories for ESP32 development
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/interface/src/routes/**/*.svelte : Create Svelte routes in `interface/src/routes/` for different pages of the web application
Applied to files:
interface/src/routes/+page.svelte
📚 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/Modules/ModuleLightsControl.hsrc/MoonLight/Nodes/Effects/E_MoonLight.hsrc/MoonLight/Nodes/Drivers/parlio.cppsrc/MoonLight/Modules/ModuleDrivers.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/Modules/ModuleLightsControl.hsrc/MoonLight/Layers/VirtualLayer.cppsrc/MoonLight/Nodes/Drivers/parlio.cppsrc/MoonLight/Modules/ModuleDrivers.h
📚 Learning: 2026-01-06T15:24:11.096Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 93
File: src/MoonLight/Layers/VirtualLayer.cpp:164-171
Timestamp: 2026-01-06T15:24:11.096Z
Learning: In VirtualLayer.cpp (setLight and getLight), the unmapped path intentionally disables bounds checking when indexV >= mappingTableSize or oneToOneMapping is true to optimize performance for very large LED counts (100K+). This is a deliberate design decision; only rely on this behavior if you can guarantee calls stay within bounds. Document this contract and avoid adding defensive checks in the hot path. Use explicit bounds checks in all other code paths or when the input cannot be guaranteed to be in-range.
Applied to files:
src/MoonLight/Layers/VirtualLayer.cpp
📚 Learning: 2025-12-07T14:16:03.228Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-07T14:16:03.228Z
Learning: VirtualLayer mappingTableSize is uint16_t with a maximum of 65535 LEDs. If a user defines a space where size.x * size.y * size.z exceeds 65535, the code accepts the overflow and performs mapping partially. This is an explicit design decision balancing performance and memory constraints.
Applied to files:
src/MoonLight/Layers/VirtualLayer.cppsrc/MoonLight/Nodes/Drivers/parlio.cppsrc/MoonLight/Modules/ModuleDrivers.h
📚 Learning: 2025-12-07T14:16:03.228Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-07T14:16:03.228Z
Learning: In VirtualLayer, mappingTableSize should always be 0 when mappingTable is nullptr. This invariant is maintained by developer responsibility: initial state sets both together, realloc success updates both together, realloc failure preserves both together. For performance reasons, no redundant null checks like `if (mappingTable && indexV < mappingTableSize)` are added, as the size check alone is sufficient when the invariant is maintained.
Applied to files:
src/MoonLight/Layers/VirtualLayer.cpp
📚 Learning: 2026-01-13T08:32:51.345Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 104
File: src/MoonLight/Nodes/Drivers/parlio.cpp:409-414
Timestamp: 2026-01-13T08:32:51.345Z
Learning: In src/MoonLight/Nodes/Drivers/parlio.cpp, show_parlio assumes 'components' (color channels per pixel: 3 for RGB, 4 for RGBW) is never 0 due to the API contract. Do not add defensive checks for this parameter in performance-critical paths; rely on caller responsibility as per design. This guideline applies to this file and similar performance-critical code where API contracts guarantee input validity; ensure any related tests/usage documents reflect this assumption.
Applied to files:
src/MoonLight/Layers/VirtualLayer.cppsrc/MoonLight/Nodes/Drivers/parlio.cpp
📚 Learning: 2026-01-13T15:18:36.992Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 104
File: src/MoonLight/Layers/PhysicalLayer.cpp:145-147
Timestamp: 2026-01-13T15:18:36.992Z
Learning: Ensure in code reviews that the three mutexes (effectsMutex, driversMutex, layerMutex) are created successfully at initialization time (checks in constructors log errors if xSemaphoreCreateMutex returns nullptr). Do not add runtime null checks for these mutex pointers after initialization, as the design treats allocation as an invariant. This pattern should be verified across MoonLight PhysicalLayer.cpp and MoonBase Nodes headers; consider documenting the invariant and ensuring no code path dereferences null mutex pointers. If extending, apply to all similar critical resources initialized during boot.
Applied to files:
src/MoonLight/Layers/VirtualLayer.cppsrc/MoonLight/Nodes/Drivers/parlio.cpp
📚 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-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Modify C++ backend code in the `src/` and `lib/` directories for ESP32 development
Applied to files:
src/MoonLight/Nodes/Drivers/parlio.cpp
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/lib/framework/**/*.{h,cpp} : Leverage the framework services in `lib/framework/` for WiFi management, MQTT, NTP, security, firmware updates, and system utilities
Applied to files:
src/MoonLight/Nodes/Drivers/parlio.cpp
🧬 Code graph analysis (2)
src/MoonLight/Modules/ModuleLightsControl.h (1)
src/MoonBase/Utilities.cpp (2)
isInPSRAM(103-126)isInPSRAM(103-103)
src/MoonLight/Modules/ModuleDrivers.h (1)
src/MoonBase/Utilities.cpp (2)
isInPSRAM(103-126)isInPSRAM(103-103)
🪛 LanguageTool
docs/gettingstarted/installer.md
[grammar] ~16-~16: Use a hyphen to join words.
Context: ...pare an ESP32 device
Recommended plug and play device to get started with, up ...
(QB_NEW_EN_HYPHEN)
[grammar] ~16-~16: Use a hyphen to join words.
Context: ... an ESP32 device
Recommended plug and play device to get started with, up to 4...
(QB_NEW_EN_HYPHEN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (15)
src/MoonBase/Nodes.cpp (2)
22-22: LGTM!Commenting out this debug statement is consistent with the other debug logs in this function and aligns with the PR's goal of reducing log verbosity.
545-545: LGTM!Demoting
setLightPresetlogging from INFO to DEBUG is appropriate—this detailed configuration output is useful for debugging but not essential for standard operation. Consistent with the logging at Line 408.docs/gettingstarted/installer.md (1)
26-26: Image file exists. The referenced file is present atdocs/firmware/installer/images/esp32-p4-nano.jpg.src/MoonLight/Nodes/Modifiers/M_MoonLight.h (1)
191-193: LGTM!Reformatting of commented debug log lines improves readability. No functional impact since these remain commented out.
src/MoonLight/Nodes/Effects/E_WLED.h (1)
380-380: LGTM!Commented debug log formatting adjustment. No runtime impact.
src/MoonLight/Modules/ModuleEffects.h (1)
266-266: LGTM!Elevating node creation logs to INFO level with "Add " prefix provides better visibility for tracking effect/modifier lifecycle. Useful for debugging node setup issues in production.
src/MoonLight/Modules/ModuleLightsControl.h (1)
74-76: LGTM!Demoting memory layout and PSRAM diagnostic logs from INFO to DEBUG appropriately reduces log noise during normal operation while keeping these details available for debugging.
src/MoonLight/Layers/VirtualLayer.cpp (3)
111-111: LGTM!Cosmetic adjustment to commented log line.
451-451: LGTM!Cosmetic adjustment to commented log line.
455-455: Verify the log tag change from ML_TAG to MB_TAG.The log level elevation to INFO is appropriate for the mapping summary. However, the tag changed from
ML_TAG(MoonLight) toMB_TAG(MoonBase). SinceVirtualLayeris a MoonLight-specific component (located insrc/MoonLight/Layers/), usingMB_TAGmay be inconsistent with the module's namespace.If this was intentional (e.g., for log filtering purposes), please disregard. Otherwise, consider keeping
ML_TAGfor consistency with other logs in this file.src/MoonLight/Nodes/Effects/E_MoonLight.h (2)
518-526: No actionable change here (commented-out debug log formatting).
1270-1285: Comment-only change; nothing to review.src/MoonLight/Nodes/Drivers/parlio.cpp (1)
463-467: Timer log: format looks fine; just keep it off the critical path in release builds.
EXT_LOGD(ML_TAG, "Parallel IO for %u pixels took %lu micros at %u FPS.", ...)looks consistent with prior printf-style logs; the modulo gating helps. If you ship builds where debug logs are enabled, consider ensuringPARLIO_TIMERis off by default to avoid jitter.src/MoonBase/Modules/ModuleIO.h (2)
678-751: RS485 pin collection + UART config changes look fine.
815-821: Battery current branch formatting-only change looks OK.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // shared static variables | ||
| SemaphoreHandle_t ModuleState::updateMutex = xSemaphoreCreateMutex(); | ||
| SemaphoreHandle_t ModuleState::updateMutex = nullptr; | ||
| UpdatedItem ModuleState::updatedItem; | ||
| bool ModuleState::updatePending = false; | ||
| Char<20> ModuleState::updateOriginId; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file structure and the specific lines mentioned
cd src/MoonBase && wc -l Module.cppRepository: MoonModules/MoonLight
Length of output: 79
🏁 Script executed:
# Read the problematic code sections
cat -n Module.cpp | sed -n '35,45p'Repository: MoonModules/MoonLight
Length of output: 107
🏁 Script executed:
# Also check lines 274-279
cat -n Module.cpp | sed -n '270,280p'Repository: MoonModules/MoonLight
Length of output: 107
🏁 Script executed:
# Let's see the class definition and understand ModuleState
rg "class ModuleState" -A 30Repository: MoonModules/MoonLight
Length of output: 1787
🏁 Script executed:
# Look for ModuleState::update method
rg "ModuleState::update" -B 2 -A 15Repository: MoonModules/MoonLight
Length of output: 25907
🏁 Script executed:
# Search for usage of updatedItem and updateOriginId
rg "updatedItem|updateOriginId" -nRepository: MoonModules/MoonLight
Length of output: 26488
🏁 Script executed:
# Check lines 274-279 to see what else the review comment applies to
head -n 285 src/MoonBase/Module.cpp | tail -n 20Repository: MoonModules/MoonLight
Length of output: 866
🏁 Script executed:
# Look at the full postUpdate implementation to understand synchronization
rg "void postUpdate" -B 2 -A 20Repository: MoonModules/MoonLight
Length of output: 1524
🏁 Script executed:
# Check for concurrent access patterns to updateOriginId
rg "updateOriginId" -B 1 -A 1 | head -100Repository: MoonModules/MoonLight
Length of output: 2904
Static state members should be synchronized or converted to instance variables to prevent data races across concurrent module updates.
The static updateOriginId is written in ModuleState::update() (line 277) without mutex protection, yet read from multiple onUpdate() handlers across modules (e.g., ModuleIO.h, Module.h) also without synchronization. This creates a data race when updates from http/websocket/sveltekit contexts overlap. Unlike updatedItem, which is guarded by updateMutex in postUpdate(), the static updateOriginId slot is accessed unsynchronized and shared across all ModuleState instances.
Consider making these instance members rather than static to eliminate the shared mutable state:
- Store
updateOriginIdandupdatedItemperModuleStateinstance - Pass
state.updateOriginIdto handlers instead of accessing a global slot
This also applies to lines 274–316, where the ModuleState::update() method assigns to both shared static members.
🤖 Prompt for AI Agents
In `@src/MoonBase/Module.cpp` around lines 38 - 42, The static shared state
(ModuleState::updateOriginId and ModuleState::updatedItem) is causing data races
because ModuleState::update() writes them without synchronization while onUpdate
handlers read them; change these static members into instance members on
ModuleState (e.g., move updateOriginId and updatedItem into the ModuleState
object and remove the static declarations), update ModuleState::update() to
write to this->updateOriginId/this->updatedItem, and adjust all callers
(onUpdate handlers in ModuleIO.h, Module.h and any postUpdate()/postUpdateMutex
usage) to read the per-instance fields or accept the originId/updatedItem as
parameters instead of accessing globals; ensure updateMutex either becomes
per-instance or remains static but protects only truly-shared resources.
| static UpdatedItem updatedItem; | ||
| static SemaphoreHandle_t updateMutex; | ||
| static bool updatePending; | ||
| volatile bool updatePending = false; | ||
|
|
||
| static Char<20> updateOriginId; // static, written by ModuleState::update, no mutex needed as written by one process at a time (http mostly, sveltekit sometimes recursively) | ||
|
|
||
| ModuleState() { | ||
| EXT_LOGD(MB_TAG, "ModuleState constructor"); | ||
|
|
||
| if (updateMutex == nullptr) { | ||
| EXT_LOGE(MB_TAG, "Failed to create updateMutex"); | ||
| } | ||
| if (!updateMutex) { | ||
| EXT_LOGD(MB_TAG, "creating updateMutex"); | ||
| updateMutex = xSemaphoreCreateMutex(); | ||
| if (!updateMutex) EXT_LOGE(MB_TAG, "Failed to create updateMutex"); | ||
| } |
There was a problem hiding this comment.
Critical: static updatedItem + per-instance updatePending can corrupt cross-module updates.
Scenario: Module A sets A.updatePending=true and writes ModuleState::updatedItem. Before A’s loop consumes it, Module B posts an update and overwrites the same static updatedItem. Now A still sees updatePending==true but processes B’s item (or vice versa). This can also drop intermediate updates.
Strongly suggest making the queued state per ModuleState instance (preferred), e.g.:
- make
UpdatedItem updatedItem;,SemaphoreHandle_t updateMutex;, andChar<20> updateOriginId;non-static members, and inModuleState::update(...)setstate.updateOriginId = originId;, or - if you really want one global queue, then the queued item must include module identity and the consumer must dispatch appropriately (and you likely want a small queue, not a single slot).
Proposed direction (per-instance storage)
- static UpdatedItem updatedItem;
- static SemaphoreHandle_t updateMutex;
- volatile bool updatePending = false;
- static Char<20> updateOriginId;
+ UpdatedItem updatedItem;
+ SemaphoreHandle_t updateMutex = nullptr;
+ bool updatePending = false;
+ Char<20> updateOriginId;(And update corresponding definitions/uses in src/MoonBase/Module.cpp.)
Also applies to: 112-141
| ModuleState() { | ||
| EXT_LOGD(MB_TAG, "ModuleState constructor"); | ||
|
|
||
| if (updateMutex == nullptr) { | ||
| EXT_LOGE(MB_TAG, "Failed to create updateMutex"); | ||
| } | ||
| if (!updateMutex) { | ||
| EXT_LOGD(MB_TAG, "creating updateMutex"); | ||
| updateMutex = xSemaphoreCreateMutex(); | ||
| if (!updateMutex) EXT_LOGE(MB_TAG, "Failed to create updateMutex"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine Module.h to understand updateMutex declaration and ModuleState structure
fd -t f "Module.h" --exec cat -n {} +Repository: MoonModules/MoonLight
Length of output: 14144
🏁 Script executed:
# Search for ModuleState class definition and updateMutex declaration
rg -A 20 "class ModuleState" src/MoonBase/Repository: MoonModules/MoonLight
Length of output: 1225
🏁 Script executed:
# Check how many ModuleState instances are created and in what contexts
rg "ModuleState" src/ -t cpp -t h --context 2Repository: MoonModules/MoonLight
Length of output: 21114
🏁 Script executed:
# Compare with PhysicalLayer and Nodes.h to understand the project's mutex pattern
fd -t f "PhysicalLayer.cpp" "Nodes.h" --exec echo "File: {}" \; -exec head -100 {} \;Repository: MoonModules/MoonLight
Length of output: 307
🏁 Script executed:
# Search for Module instantiation/creation patterns
rg "new Module|Module(" src/ -t cpp -t h --context 2 | head -100Repository: MoonModules/MoonLight
Length of output: 151
🏁 Script executed:
# Check Module.cpp constructor to understand instantiation context
cat src/MoonBase/Module.cpp | head -150Repository: MoonModules/MoonLight
Length of output: 6645
🏁 Script executed:
# Compare updateMutex pattern with other mutexes in project (effectsMutex, driversMutex, etc.)
rg "xSemaphoreCreateMutex" src/ -t cpp -t h -B 3 -A 3Repository: MoonModules/MoonLight
Length of output: 1316
🏁 Script executed:
# Check main.cpp to see how Module instances are created
rg "Module" src/main.cpp -A 2 -B 2 | head -80Repository: MoonModules/MoonLight
Length of output: 3210
🏁 Script executed:
# Look for any dynamic Module instantiation or creation from tasks
rg "new Module|new ModuleChannels|new ModuleLights" src/ -t cpp -t hRepository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
# Compare PhysicalLayer mutex initialization style with ModuleState
rg "SemaphoreHandle_t.*xSemaphoreCreateMutex" src/ -B 2 -A 2Repository: MoonModules/MoonLight
Length of output: 727
Mutex initialization pattern differs from project conventions; consider aligning with PhysicalLayer's immediate creation approach.
The lazy initialization of updateMutex in the ModuleState constructor is inconsistent with the project's established pattern. PhysicalLayer and other project code create mutexes immediately at member declaration (e.g., SemaphoreHandle_t effectsMutex = xSemaphoreCreateMutex();), not lazily. While module instances are currently global static objects initialized serially at boot before tasks start (making a race condition theoretical rather than practical), the lazy pattern should be aligned with project conventions by creating updateMutex at declaration time instead.
Also, volatile on updatePending adds no correctness benefit; the mutex alone provides necessary synchronization.
🤖 Prompt for AI Agents
In `@src/MoonBase/Module.h` around lines 53 - 60, Move updateMutex creation to
immediate member initialization to match project conventions used in
PhysicalLayer (e.g., initialize updateMutex = xSemaphoreCreateMutex() at
declaration inside ModuleState) instead of lazy-creating it in the ModuleState()
constructor; also remove the volatile qualifier from updatePending since the
mutex provides the required synchronization. Update any constructor logging
accordingly (remove the lazy-create branch) and ensure the member declarations
reference the newly-initialized updateMutex and the non-volatile updatePending.
| 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); | ||
| voltage_readout_current_adc_attenuation = adc_get_adjusted_gain(voltage_readout_current_adc_attenuation, adc_mv_vinput); | ||
| } |
There was a problem hiding this comment.
Battery voltage: avoid pushing 0V for unsupported current_board_id.
Right now volts stays 0 unless the board is board_SE16V1 or board_LightCrafter16, but updateVoltage(volts) is still called. Consider either:
else return;/ skipupdateVoltagefor unknown boards, or- compute a reasonable default divider for other boards (if applicable).
🤖 Prompt for AI Agents
In `@src/MoonBase/Modules/ModuleIO.h` around lines 798 - 806, The code calls
batteryService->updateVoltage(volts) even when current_board_id is neither
board_SE16V1 nor board_LightCrafter16 leaving volts at 0; change the logic in
the block containing current_board_id, volts, and updateVoltage so that for
unknown boards you either early-return/skip calling
batteryService->updateVoltage (preferred) or set a documented default divider
before calling updateVoltage; ensure you also handle
voltage_readout_current_adc_attenuation by only calling
adc_get_adjusted_gain(...) when you actually computed volts for a supported
board (symbols: current_board_id, board_SE16V1, board_LightCrafter16, volts,
batteryService->updateVoltage, voltage_readout_current_adc_attenuation,
adc_get_adjusted_gain).
|
regarding static SemaphoreHandle_t updateMutex: I tried initializing it immediately at member declaration but then the logging showed it was nullptr at every ModuleState instantiation - I was surprised but to avoid that I added the lazy initialization which is now only done once. Can you analyze why initializing it immediately at member declaration did not work for me? regarding bool updatePending = false;: I tried to make it static based on your earlier advice but then the whole update mechanism did not work anymore. Can you analyze why that was the case and provide a solution to make it static while update keeps working |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 7294 🏁 Script executed: Length of output: 18808 🏁 Script executed: Length of output: 1471 🏁 Script executed: Length of output: 3076
Issue 1: Static updateMutex initialization at declarationThe reason Why it was nullptr:
Since neither is ready during static initialization, Your lazy initialization is correct: Issue 2: Making updatePending static broke the update mechanismWhy it failed:
Current design pattern:
Solution to make updatePending work correctlyTo make Option A: Queue-based updates (recommended) // In Module.h
class ModuleState {
public:
static std::queue<UpdatedItem> updateQueue;
static SemaphoreHandle_t updateMutex;
void postUpdate(const UpdatedItem& updatedItem) {
if (xSemaphoreTake(updateMutex, portMAX_DELAY) == pdTRUE) {
updateQueue.push(updatedItem);
xSemaphoreGive(updateMutex);
}
}
void getUpdate() {
if (xSemaphoreTake(updateMutex, 0) == pdTRUE) {
if (!updateQueue.empty()) {
UpdatedItem localCopy = updateQueue.front();
updateQueue.pop();
xSemaphoreGive(updateMutex);
if (processUpdatedItem) processUpdatedItem(localCopy);
return;
}
xSemaphoreGive(updateMutex);
}
}
};Option B: Per-module tracking with map // In Module.h
class ModuleState {
public:
static std::map<ModuleState*, UpdatedItem> pendingUpdates;
static SemaphoreHandle_t updateMutex;
void postUpdate(const UpdatedItem& updatedItem) {
if (xSemaphoreTake(updateMutex, portMAX_DELAY) == pdTRUE) {
pendingUpdates[this] = updatedItem;
xSemaphoreGive(updateMutex);
}
}
void getUpdate() {
if (xSemaphoreTake(updateMutex, 0) == pdTRUE) {
auto it = pendingUpdates.find(this);
if (it != pendingUpdates.end()) {
UpdatedItem localCopy = it->second;
pendingUpdates.erase(it);
xSemaphoreGive(updateMutex);
if (processUpdatedItem) processUpdatedItem(localCopy);
return;
}
xSemaphoreGive(updateMutex);
}
}
};Why current design is pragmatic:
Unless you have a strong reason to make 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/gettingstarted/installer.md`:
- Line 24: Update the inline text "Recommended state-of-the-art DIY board, up to
16K - 98K LEDs !: [ESP32-P4-Nano](...)" by removing the extra space before the
exclamation mark and normalizing the range notation to use an en-dash (e.g.,
"16K–98K"); ensure the final text reads like "Recommended state-of-the-art DIY
board, up to 16K–98K LEDs!: [ESP32-P4-Nano](...)" so the punctuation and range
formatting are consistent.
♻️ Duplicate comments (2)
src/MoonBase/Modules/ModuleIO.h (1)
798-806: Formatting changes look fine.The reformatting of the voltage calculation logic is acceptable. Note that the pre-existing concern about pushing
0Vfor unsupportedcurrent_board_idvalues (flagged in previous reviews) remains unaddressed — this PR only reformats the code without changing the logic.src/MoonBase/Module.h (1)
47-51: StaticupdatedItemwith per-instanceupdatePendingstill permits cross-module race.This concern was raised in a previous review. The scenario remains: Module A posts an update (sets
updatePending=true, writes to staticupdatedItem), Module B posts before A consumes (overwrites staticupdatedItem), and A processes B's data.Per the PR discussion, the recommendation was to either keep everything per-instance or use a thread-safe queue. The current hybrid approach has inherent timing risks between independent module loops.
🧹 Nitpick comments (2)
src/MoonLight/Nodes/Drivers/parlio.cpp (1)
278-282: Consistent logging in commented code.While this block is commented out, updating to
EXT_LOGDmaintains consistency if it's ever re-enabled. Consider removing dead code entirely if it's not expected to be used.src/MoonBase/Utilities.cpp (1)
90-95: Consider consistent error logging for write failure.This failure path still uses
Serial.printlnwhile the other three error paths now useEXT_LOGE(MB_TAG, ...). For consistency, consider updating this as well.Proposed fix
if (dst.write(buf, n) != n) { - Serial.println("Write failed!"); + EXT_LOGE(MB_TAG, "Failed to write to destination file: %s", dstPath); src.close(); dst.close(); return false; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
docs/gettingstarted/installer.mdinterface/src/routes/+page.sveltelib/framework/WWWData.hsrc/MoonBase/Module.hsrc/MoonBase/Modules/ModuleIO.hsrc/MoonBase/NodeManager.hsrc/MoonBase/Nodes.cppsrc/MoonBase/Utilities.cppsrc/MoonLight/Nodes/Drivers/D_Infrared.hsrc/MoonLight/Nodes/Drivers/parlio.cppsrc/MoonLight/Nodes/Effects/E_MoonLight.h
💤 Files with no reviewable changes (1)
- src/MoonBase/NodeManager.h
✅ Files skipped from review due to trivial changes (1)
- src/MoonLight/Nodes/Drivers/D_Infrared.h
🚧 Files skipped from review as they are similar to previous changes (1)
- interface/src/routes/+page.svelte
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2026-01-13T15:18:48.740Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 104
File: src/MoonLight/Layers/PhysicalLayer.cpp:145-147
Timestamp: 2026-01-13T15:18:48.740Z
Learning: In MoonLight's PhysicalLayer and Node architecture (src/MoonLight/Layers/PhysicalLayer.cpp, src/MoonBase/Nodes.h): effectsMutex, driversMutex, and layerMutex are mandatory for system operation. These mutexes are validated at creation time (constructor checks log errors if xSemaphoreCreateMutex returns nullptr). Runtime null checks on these mutex pointers are intentionally omitted—the design treats successful mutex allocation as an invariant after initialization, placing responsibility on the programmer to ensure proper allocation. This is a performance-oriented "fail-fast at boot" approach rather than defensive runtime checking.
<!-- </add_learning>
Applied to files:
src/MoonBase/Module.h
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Modify C++ backend code in the `src/` and `lib/` directories for ESP32 development
Applied to files:
src/MoonLight/Nodes/Drivers/parlio.cpp
📚 Learning: 2025-11-29T16:17:30.619Z
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/src/**/*.{h,cpp} : Extend core functionality by implementing custom services in C++ that integrate with MQTT, the web interface, and device logic (e.g., LightMqttSettingsService, LightStateService)
Applied to files:
src/MoonLight/Nodes/Drivers/parlio.cpp
📚 Learning: 2026-01-13T08:32:51.345Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 104
File: src/MoonLight/Nodes/Drivers/parlio.cpp:409-414
Timestamp: 2026-01-13T08:32:51.345Z
Learning: In src/MoonLight/Nodes/Drivers/parlio.cpp, show_parlio assumes 'components' (color channels per pixel: 3 for RGB, 4 for RGBW) is never 0 due to the API contract. Do not add defensive checks for this parameter in performance-critical paths; rely on caller responsibility as per design. This guideline applies to this file and similar performance-critical code where API contracts guarantee input validity; ensure any related tests/usage documents reflect this assumption.
Applied to files:
src/MoonLight/Nodes/Drivers/parlio.cpp
📚 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/Drivers/parlio.cpp
📚 Learning: 2025-12-07T14:16:03.228Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-07T14:16:03.228Z
Learning: VirtualLayer mappingTableSize is uint16_t with a maximum of 65535 LEDs. If a user defines a space where size.x * size.y * size.z exceeds 65535, the code accepts the overflow and performs mapping partially. This is an explicit design decision balancing performance and memory constraints.
Applied to files:
src/MoonLight/Nodes/Drivers/parlio.cpp
📚 Learning: 2025-12-25T13:31:32.367Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-25T13:31:32.367Z
Learning: For ESP32 LED drivers using DMA, I2S, LCD, or PARLIO: always pin driver tasks to Core 1 (APP_CPU), not Core 0. WiFi/BT runs at priority ~23 on Core 0 and would constantly preempt lower-priority driver tasks, breaking LED timing requirements. ESP-IDF explicitly recommends display/DMA processing on Core 1 to avoid WiFi interference.
Applied to files:
src/MoonLight/Nodes/Drivers/parlio.cpp
📚 Learning: 2026-01-13T15:18:36.992Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 104
File: src/MoonLight/Layers/PhysicalLayer.cpp:145-147
Timestamp: 2026-01-13T15:18:36.992Z
Learning: Ensure in code reviews that the three mutexes (effectsMutex, driversMutex, layerMutex) are created successfully at initialization time (checks in constructors log errors if xSemaphoreCreateMutex returns nullptr). Do not add runtime null checks for these mutex pointers after initialization, as the design treats allocation as an invariant. This pattern should be verified across MoonLight PhysicalLayer.cpp and MoonBase Nodes headers; consider documenting the invariant and ensuring no code path dereferences null mutex pointers. If extending, apply to all similar critical resources initialized during boot.
Applied to files:
src/MoonLight/Nodes/Drivers/parlio.cpp
📚 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: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/MoonBase/Nodes.cpp (1)
src/MoonBase/Nodes.h (1)
requestMappings(215-224)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (19)
src/MoonBase/Nodes.cpp (3)
22-22: LGTM - Debug log appropriately disabled.Commenting out this verbose debug log reduces noise while keeping it available for future debugging if needed. Consistent with other commented debug logs in this function.
289-289: LGTM - Minor comment refinement.Reducing the emphasis from
!!!to!in the comment is a trivial cleanup. The note about script loop interaction remains as a useful reminder for future consideration.
545-545: LGTM - Log level appropriately reduced.Changing from
EXT_LOGItoEXT_LOGDforsetLightPresetis reasonable since this message is primarily useful during debugging rather than for regular operational visibility. This reduces log noise in normal operation.src/MoonBase/Modules/ModuleIO.h (4)
608-613: LGTM!The RS485 pin variable declarations are properly initialized with
UINT8_MAXas sentinel values. The log level elevation toEXT_LOGIfor ethernet configuration aligns with the PR's broader logging adjustments.
623-628: LGTM!The parameter type change from
uint8_ttoint8_taligns with the target field types (int8_t& target) and the-1sentinel value convention used by the ethernet settings. ESP32 GPIO numbers are well within the signed range, so this is a safe and appropriate type correction.
716-750: LGTM!The UART configuration and demo sensor code formatting changes are clean. The demo code remains properly guarded behind
DEMOCODE_FOR_SHT30_SENSOR.
815-820: LGTM!Current calculation formatting changes maintain the original logic.
src/MoonLight/Nodes/Drivers/parlio.cpp (4)
14-18: Good defensive programming with static_assert.The compile-time assertion validates the assumption that
SOC_PARLIO_TX_UNIT_MAX_DATA_WIDTH <= 16, which is critical for the bit packing logic inprocess_*bit()functions. This prevents silent failures on future hardware with wider data widths.
27-29: Array size now correctly tied to hardware capability.Using
SOC_PARLIO_TX_UNIT_MAX_DATA_WIDTHinstead of a hardcoded16ensures the array size matches the hardware's actual capability. This aligns well with the static_assert guard at line 18 and the clamping logic at line 292.
354-364: Critical off-by-one fix and ISR-safe logging.The change from
i <= outputstoi < outputs(line 359) fixes an off-by-one boundary error—outputsis a count, so valid indices are0tooutputs-1. The switch toESP_LOGDwith the ISR-safety comment is appropriate for code that may be called from timing-sensitive contexts.
463-467: Verify logging consistency with earlier ISR-safety rationale.Lines 354 and 362 explicitly use
ESP_LOGDwith comments noting "do not use EXT_LOG, unsafe if called from ISR", yet this line usesEXT_LOGD. While this code executes after the critical DMA transmission section and may be safe, the inconsistency warrants verification thatEXT_LOGDis appropriate here.src/MoonLight/Nodes/Effects/E_MoonLight.h (3)
524-524: LGTM!Commented-out debug log formatting adjustment has no impact.
1093-1168: LGTM!Debug log formatting changes in
updatePositionandDraware consistent. Format specifiers are appropriate for their argument types, and all statements are properly guarded by thedebugPrintflag.
1207-1284: LGTM!Debug log formatting changes in
settingUpParticlesand the commented-out log inloopare straightforward cosmetic adjustments.docs/gettingstarted/installer.md (2)
16-16: Previous hyphenation issue successfully resolved.The compound modifier "plug-and-play" is now correctly hyphenated.
20-20: Both external product links have been verified and are currently valid and accessible:
- AliExpress ESP32-S3 link (
https://s.click.aliexpress.com/e/_DBAtJ2H): Returns HTTP 302 redirect (normal for affiliate links) and resolves to the correct product page with HTTP 200 status.- Waveshare ESP32-P4-Nano link (
https://www.waveshare.com/esp32-p4-nano.htm): Returns HTTP 200 and is directly accessible.Both links point to the correct products as described in the documentation.
src/MoonBase/Utilities.cpp (1)
64-85: LGTM! Logging changes address prior feedback correctly.The error logging now uses the correct tag (
MB_TAG) and appropriate log level (EXT_LOGE) for hard failures. This is consistent with the rest of the MoonBase module.src/MoonBase/Module.h (2)
53-61: Lazy mutex initialization is correct for FreeRTOS lifecycle.The guarded lazy creation in the constructor is appropriate here. As discussed in the PR comments,
xSemaphoreCreateMutex()cannot be called at static initialization time (before FreeRTOS scheduler is ready) without returningnullptr. The constructor runs after RTOS initialization, making this the correct approach.The added debug logging on Line 57 improves diagnosability.
49-49: Per-instance flag and removal ofvolatileare correct.Making
updatePendingper-instance ensures each module tracks its own pending state independently, as discussed in the PR. Thevolatilequalifier was unnecessary since the mutex provides the required memory ordering guarantees.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
|
||
| {: style="width:100px"} | ||
|
|
||
| Recommended state-of-the-art DIY board, up to 16K - 98K LEDs !: [ESP32-P4-Nano](https://www.waveshare.com/esp32-p4-nano.htm){:target="_blank"} |
There was a problem hiding this comment.
Fix punctuation and range formatting.
The line contains two minor typographical issues:
- Remove the space before the exclamation mark
- Use consistent range notation without spaces (either "16K–98K" with en-dash or "16K-98K" with hyphen)
📝 Proposed fix
-Recommended state-of-the-art DIY board, up to 16K - 98K LEDs !: [ESP32-P4-Nano](https://www.waveshare.com/esp32-p4-nano.htm){:target="_blank"}
+Recommended state-of-the-art DIY board, up to 16K–98K LEDs: [ESP32-P4-Nano](https://www.waveshare.com/esp32-p4-nano.htm){:target="_blank"}📝 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.
| Recommended state-of-the-art DIY board, up to 16K - 98K LEDs !: [ESP32-P4-Nano](https://www.waveshare.com/esp32-p4-nano.htm){:target="_blank"} | |
| Recommended state-of-the-art DIY board, up to 16K–98K LEDs: [ESP32-P4-Nano](https://www.waveshare.com/esp32-p4-nano.htm){:target="_blank"} |
🤖 Prompt for AI Agents
In `@docs/gettingstarted/installer.md` at line 24, Update the inline text
"Recommended state-of-the-art DIY board, up to 16K - 98K LEDs !:
[ESP32-P4-Nano](...)" by removing the extra space before the exclamation mark
and normalizing the range notation to use an en-dash (e.g., "16K–98K"); ensure
the final text reads like "Recommended state-of-the-art DIY board, up to 16K–98K
LEDs!: [ESP32-P4-Nano](...)" so the punctuation and range formatting are
consistent.
Summary by CodeRabbit
New Features
Documentation
Refactor
Style
✏️ Tip: You can customize this high-level summary in your review settings.