Board presets p4 #74
Board presets p4 #74
Conversation
Back-end ======== - IO Module : add Serg mini and universal shield - Lights control: MoonModules palette default - Parlio: parlio_config.data_gpio_nums: only defined pins
Front-end ======== - MultiInput: more space for numbers (Firefox compatibility) - Monitor: support nrOfChannels (RGB2040) Back-end ======== - Nodes: DriverNode: use header.lightPreset and add RGB2040 - Physical layer: header: add nrOfChannels and lightPreset - Virtual layer: set/getLight: check for lightPresets (RGB2040), fade*: use nrOfChannels
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Build & Config firmware/esp32-p4.ini, platformio.ini |
Commented out linker wrap -Wl,-wrap,esp_dma_capable_malloc in the ESP32-P4 Nano env and removed a stray blank line; updated APP_DATE build flag to "2025121511". |
Lights Header src/MoonLight/Layers/PhysicalLayer.h |
Added uint16_t nrOfChannels and uint8_t lightPreset to LightsHeader, reduced padding (fill[8] → fill[5]), and updated resetOffsets() to clear nrOfChannels (left lightPreset reset commented). |
Physical Layer Logic src/MoonLight/Layers/PhysicalLayer.cpp |
Account for RGB2040 in nextPin (double-count ledsPerPin when preset==RGB2040) and compute nrOfChannels = nrOfLights * channelsPerLight * (2 if RGB2040 else 1) in onLayoutPost; updated related logging. |
Virtual Layer src/MoonLight/Layers/VirtualLayer.h, src/MoonLight/Layers/VirtualLayer.cpp |
Renamed enum mapType → enum MapTypeEnum; added per-light RGB2040 remapping in setLight/getLight to skip empty channel ranges; adjusted fast-paths to use nrOfChannels / sizeof(CRGB) for 3-channel lights. |
Nodes & Presets src/MoonBase/Nodes.h, src/MoonBase/Nodes.cpp |
Source presets from layerP.lights.header.lightPreset, removed DriverNode::lightPreset member, added new preset identifiers (including RGB2040) and updated preset mapping/case handling and logging to use header field. |
Drivers & Outputs src/MoonLight/Nodes/Drivers/D_ArtnetOut.h, src/MoonLight/Nodes/Drivers/parlio.cpp |
Drivers now read header->lightPreset instead of a local variable; parlio.cpp guards data_gpio_nums initialization to set unused pins to -1; create_transposed_led_output_optimized signature updated to accept is_rgbw and RGB offsets. |
Module + IO Board Definitions src/MoonBase/Modules/ModuleIO.h, src/MoonLight/Modules/ModuleLightsControl.h, src/MoonLight/Modules/ModuleMoonLightInfo.h |
Refactored board pin mappings to flexible ledPins[] loops using std::size, added/adjusted many board-specific peripheral pins, changed default palette from 6→9, switched monitor emission length to use nrOfChannels, and exposed nrOfChannels in MoonLightInfo UI. |
UI / Monitor interface/src/routes/moonbase/monitor/Monitor.svelte, interface/src/lib/components/moonbase/MultiInput.svelte |
Added lightPreset and nrOfChannels state reads from header (bytes 32–34), refactored positions/channels parsing to iterate per-light and per-channel-group with RGB2040-aware skipping, and increased numeric input width by 1ch for number inputs. |
Sequence Diagram(s)
sequenceDiagram
autonumber
participant Driver as Driver (Node)
participant Header as LightsHeader (shared)
participant Physical as PhysicalLayer
participant Virtual as VirtualLayer
participant Monitor as Monitor UI
Driver->>Header: write lightPreset, nrOfChannels
Note right of Header: header updated by Driver
Driver->>Physical: configure pins/layout (uses lightPreset)
Physical->>Header: read lightPreset, compute nrOfChannels
Physical-->>Virtual: provide channel mapping / offsets
Virtual->>Header: read nrOfChannels for fast-paths
Monitor->>Header: read lightPreset, nrOfChannels (bytes 32–34)
Monitor->>Virtual: request positions/channels parsing
Virtual-->>Monitor: send remapped channel/position data
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
- Areas needing careful review:
VirtualLayer.cpp: RGB2040 index remapping correctness acrosssetLight/getLightand bulk operations.PhysicalLayer.h/cpp: struct packing/alignment after adding fields and correctness ofnrOfChannelscalculation.Monitor.svelte: byte offsets and parsing loops for positions and channels (endianness and offsets).ModuleIO.h: board pin mapping changes andstd::sizeiteration use.parlio.cppand callers: verify updatedcreate_transposed_led_output_optimizedsignature matches all call sites.
Possibly related PRs
- Board presets ESP32-P4 #72 — overlaps on
parliochanges and driver signature/handling edits. - add heap-optimization: request heap optimization review #62 — related changes to
PhysicalLayerheader/layout and channel accounting. - Board preset add presets #70 — related board pin mapping and
ModuleIOadjustments.
Suggested labels
enhancement
Poem
🐇 I hopped through headers, counted lanes,
Skipped the gaps where RGB2040 reigns,
Pins rearranged, presets reset,
Channels mapped — not one is let,
I twitch my nose, the colors play 🥕🎨
Pre-merge checks and finishing touches
❌ Failed checks (1 warning, 1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 14.81% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
|
| Title check | ❓ Inconclusive | The title 'Board presets p4' is vague and generic, using 'p4' without clear context about what the change actually accomplishes. While related to board configuration changes, it doesn't convey meaningful information about the substantial refactoring across multiple systems including light presets, channel handling, and API naming changes. | Consider a more descriptive title that captures the main changes, such as 'Refactor light presets and channel handling architecture' or 'Migrate to header-based light preset management'. |
✅ Passed checks (1 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
board-presets-p4
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: 3
🧹 Nitpick comments (6)
src/MoonBase/Modules/ModuleIO.h (3)
277-278: Use idiomatic array size calculation.The pattern
sizeof(ledPins)works foruint8_tarrays (sincesizeof(uint8_t) == 1), but it's non-idiomatic and fragile. If the array type changes (e.g., touint16_t), the iteration count becomes incorrect, leading to buffer overruns or incomplete iteration.Apply the standard idiom for calculating array element count:
- for (int i = 0; i < sizeof(ledPins); i++) pinAssigner.assignPin(ledPins[i], pin_LED); + for (int i = 0; i < sizeof(ledPins)/sizeof(ledPins[0]); i++) pinAssigner.assignPin(ledPins[i], pin_LED);Alternatively, if C++17 is available:
for (int i = 0; i < std::size(ledPins); i++) pinAssigner.assignPin(ledPins[i], pin_LED);Or use a range-based for loop for clarity:
for (uint8_t pin : ledPins) pinAssigner.assignPin(pin, pin_LED);Apply this pattern to all similar occurrences in the file (lines 278, 314, 328, 415, 427, 438, 455, 465, 470).
Also applies to: 313-314, 327-328, 414-415, 426-427, 437-438, 454-455, 464-465, 468-470
367-383: Consider completing the SergMiniShield board configuration.The configuration has several incomplete or commented elements:
- Line 370: Optional LED pin 17 is commented out
- Line 372: Button pin assignment is unclear (comment asks "which pin?")
These don't block functionality but may limit board capabilities. If hardware documentation is available, consider completing these assignments.
Do you want me to open an issue to track completing this board configuration once hardware details are confirmed?
407-407: Address the TODO for temperature pin.The comment indicates the temperature sensor pin assignment is incomplete. Consider verifying the correct pin from hardware documentation and completing this assignment.
Do you want me to open an issue to track this TODO item?
src/MoonLight/Layers/PhysicalLayer.h (1)
57-61: Header layout looks correct, but comment is misleading.The byte counting is correct: 35 bytes of data + 5 bytes fill = 40 bytes total. However, the comment
//->37 max (prime number)!!!appears to be outdated/incorrect — the fill brings the struct to 40 bytes, not 37.Consider updating the comment for clarity:
- uint8_t fill[5]; //->37 max (prime number)!!! needed so pack up until 40 + uint8_t fill[5]; // padding to align struct to 40 bytes totalsrc/MoonBase/Nodes.cpp (1)
515-523: RGB2040 preset configuration looks correct.Case 13 sets standard RGB offsets (0,1,2) with 3 channels per light. The distinction from case 0 (RGB) is that the preset value
13triggers special channel remapping logic elsewhere (e.g., in VirtualLayer.cpp for dual-channel-group handling per the AI summary).One minor observation: consider adding a brief comment explaining the RGB2040's unique dual-channel-group architecture to help future maintainers understand why this preset exists separately from RGB:
case 13: // curtain RGB2040 + // RGB2040 uses standard RGB offsets but has special channel remapping + // for dual-channel-group architecture (handled in VirtualLayer) header->channelsPerLight = 3; header->offsetRed = 0; header->offsetGreen = 1; header->offsetBlue = 2; break;src/MoonLight/Layers/PhysicalLayer.cpp (1)
187-187: RGB2040 channel doubling innextPin.The logic correctly doubles
ledsPerPinfor RGB2040 (preset 13) to account for the empty channel slots in the 20-39 range per group of 40. Consider extracting the magic number13to a named constant for clarity.+// At file or header level +constexpr uint8_t LIGHT_PRESET_RGB2040 = 13; - ledsPerPin[i] = (lights.header.nrOfLights - prevNrOfLights) * ((lights.header.lightPreset == 13) ? 2 : 1); // RGB2040 has empty channels + ledsPerPin[i] = (lights.header.nrOfLights - prevNrOfLights) * ((lights.header.lightPreset == LIGHT_PRESET_RGB2040) ? 2 : 1); // RGB2040 has empty channels
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
firmware/esp32-p4.ini(1 hunks)interface/src/lib/components/moonbase/MultiInput.svelte(1 hunks)interface/src/routes/moonbase/monitor/Monitor.svelte(4 hunks)platformio.ini(1 hunks)src/MoonBase/Modules/ModuleIO.h(8 hunks)src/MoonBase/Nodes.cpp(4 hunks)src/MoonBase/Nodes.h(0 hunks)src/MoonLight/Layers/PhysicalLayer.cpp(2 hunks)src/MoonLight/Layers/PhysicalLayer.h(3 hunks)src/MoonLight/Layers/VirtualLayer.cpp(6 hunks)src/MoonLight/Modules/ModuleLightsControl.h(2 hunks)src/MoonLight/Modules/ModuleMoonLightInfo.h(2 hunks)src/MoonLight/Nodes/Drivers/D_ArtnetOut.h(3 hunks)src/MoonLight/Nodes/Drivers/parlio.cpp(2 hunks)
💤 Files with no reviewable changes (1)
- src/MoonBase/Nodes.h
🧰 Additional context used
🧠 Learnings (4)
📚 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/Modules/ModuleMoonLightInfo.hsrc/MoonLight/Layers/VirtualLayer.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/**/factory_settings.ini : Configure default or factory-set ESP32 parameters in `factory_settings.ini`
Applied to files:
firmware/esp32-p4.ini
📚 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/**/platformio.ini : Use PlatformIO to build and upload the ESP32 backend C++ code
Applied to files:
firmware/esp32-p4.iniplatformio.ini
📚 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:
firmware/esp32-p4.ini
🧬 Code graph analysis (4)
src/MoonLight/Modules/ModuleMoonLightInfo.h (1)
src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)
src/MoonLight/Layers/PhysicalLayer.cpp (1)
docs/javascripts/moonlight.js (1)
i(107-107)
src/MoonLight/Layers/VirtualLayer.cpp (1)
src/MoonLight/Layers/VirtualLayer.h (1)
VirtualLayer(57-282)
src/MoonBase/Nodes.cpp (1)
src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)
⏰ 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 (26)
firmware/esp32-p4.ini (1)
28-30: The commented linker wrap flag references a non-existent__wrap_esp_dma_capable_malloc()function—remove or clarify the stale comment.The
-Wl,-wrap,esp_dma_capable_mallocflag on line 29 points to a wrapper function that does not exist anywhere in the codebase. This appears to be incomplete or aspirational code. Either remove the comment entirely, or if the PSRAM DMA buffer optimization for ESP-Hosted is still desired, implement the wrapper function and conditionally enable it with a documented build flag rather than leaving a dangling comment.Likely an incorrect or invalid review comment.
src/MoonBase/Modules/ModuleIO.h (1)
473-477: LGTM: Clean conditional pin selection for ESP32P4.The conditional compilation correctly selects pin 37 for ESP32P4 targets and falls back to pin 16 for other platforms. This is a clean implementation that maintains backward compatibility.
platformio.ini (1)
59-59: LGTM!Routine build date bump aligning with the RGB2040 feature additions in this PR.
src/MoonLight/Layers/PhysicalLayer.h (2)
81-84: LGTM!The decision to not reset
lightPreset(managed by Drivers) while resettingnrOfChannelsis reasonable and documented. The fill array is properly zeroed.
134-134: Minor formatting change.Whitespace adjustment in comment — no functional impact.
src/MoonLight/Nodes/Drivers/D_ArtnetOut.h (2)
125-128: LGTM!Correctly migrated from a local variable to
header->lightPreset, aligning with the header-driven preset management introduced in this PR. The RGBWYP (preset 9) special handling for mixed 4/6 channel lights is preserved.
134-134: Minor formatting cleanup.Whitespace adjustments in commented debug statements — no functional impact.
Also applies to: 148-148
src/MoonBase/Nodes.cpp (2)
348-362: LGTM!The control binding correctly uses
layerP.lights.header.lightPresetfor header-driven preset management. The new "RGB2040" option (preset 13) is properly added to the select control values.
411-411: LGTM!The switch statement now correctly uses
header->lightPresetinstead of a local variable, consistent with the header-driven architecture.interface/src/lib/components/moonbase/MultiInput.svelte (1)
184-196: LGTM!Minor width adjustment (+1ch) for number inputs. This provides slightly more breathing room for numeric values, which aligns with the PR's expanded channel/preset configurations that may involve larger numbers.
src/MoonLight/Modules/ModuleLightsControl.h (2)
121-121: Default palette changed to MoonModules (index 9).This is a simple preference change for the default palette. No functional concerns.
347-347: Updated to usenrOfChannelsfor monitor data emission.This change correctly aligns with the new header field that accounts for RGB2040's doubled channel count. The positions path at line 338 still correctly uses
nrOfLights * 3since positions are per-light, not per-channel.src/MoonLight/Modules/ModuleMoonLightInfo.h (2)
29-29: NewnrOfChannelscontrol added to UI definition.Consistent with the new header field. This provides visibility into the actual channel count (which differs from
nrOfLights * channelsPerLightfor RGB2040).
58-58: ExposesnrOfChannelsin the readHook.Correctly reads from the header field for UI display.
src/MoonLight/Layers/PhysicalLayer.cpp (1)
201-202:nrOfChannelscalculation and logging.The calculation correctly applies the RGB2040 multiplier. The log message now includes
nrOfChannelswhich aids debugging.src/MoonLight/Layers/VirtualLayer.cpp (5)
24-24: Constructor formatting change.Cosmetic change only.
139-143: RGB2040 channel remapping insetLight(m_oneLight case).The remapping formula
(indexP / 20 * 40 + indexP % 20)correctly skips the empty channel slots (20-39) in each group of 40 for RGB2040's dual-channel-group architecture.
149-151: RGB2040 channel remapping insetLight(m_moreLights case).Consistent with the m_oneLight case remapping.
262-262: Same concern:nrOfChannelsinfill_solid.Same issue as
fadeToBlackBy- verify whether passingnrOfChannels(byte count or doubled for RGB2040) to a function expecting LED count is intentional.
282-282: Same concern:nrOfChannelsinfill_rainbow.Same issue as above. The comment "RGB2040 has empty channels" suggests awareness, but the implementation may fill colors into empty channel slots.
src/MoonLight/Nodes/Drivers/parlio.cpp (2)
204-205: Clarifying comment for component processing order.Helpful documentation explaining how the RGB offsets map input array positions to output order (e.g., GRB -> 1,0,2).
298-298: Guard against out-of-bounds GPIO configuration.This fix correctly sets unused
data_gpio_numsentries to-1(GPIO_NUM_NC) wheni >= outputs, preventing potential undefined behavior from reading uninitializedparallelPinsvalues.interface/src/routes/moonbase/monitor/Monitor.svelte (4)
57-58: New state variables for RGB2040 support.
lightPresetdefaults to 2 (standard) andnrOfChannelsto 0. These will be populated from the header.
102-103: Updated log output includesnrOfChannels.Helpful for debugging.
110-113: Position loop refactored for clarity.The loop now explicitly iterates per light with
indexP * 3stride, making the intent clearer than the previous implicit 3-byte grouping.
75-80: Header parsing offsets are correct.The C++
LightsHeaderstruct confirmsnrOfChannelsas a uint16 at offset 32 andlightPresetas a uint8 at offset 34. The struct maintains proper alignment with a fill array to reach 40 bytes total. The JavaScript parsing code matches the C++ layout.
Front-End ======== - Monitor: lightPreset_RGB2040 tuning Back-End ======= - Nodes: add LightPresetsEnum - Virtual layer: optimise lightPreset_RGB2040 in set/getLight, fix fastled_fadeToBlackBy , fastled_fill_solid, fastled_fill_rainbow
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MoonBase/Nodes.cpp (1)
411-523: Add a default case to handle invalid preset values.The switch statement lacks a default case, which means invalid
lightPresetvalues will silently fall through without configuring any channel offsets. This could lead to undefined behavior or difficult-to-debug issues.Add a default case with error logging:
layer->layerP->lights.header.offsetZoom = 17; break; + default: + EXT_LOGE(ML_TAG, "Invalid lightPreset value: %d", header->lightPreset); + // Fall back to GRB (most common default) + header->lightPreset = lightPreset_GRB; + header->channelsPerLight = 3; + header->offsetRed = 1; + header->offsetGreen = 0; + header->offsetBlue = 2; + break; }
🧹 Nitpick comments (6)
src/MoonBase/Modules/ModuleIO.h (6)
19-63: Pin usage enum and UI options are aligned; consider reducing coupling
IO_PinUsageEnum(0–42) lines up 1:1 with the"usage"select options (indices 0–42), so persisted data keeps its meaning. This is good, but it’s also easy to break later by inserting/removing entries in only one place. Long term, consider driving both the enum and the UI labels from a single definition table to avoid silent mismatches.Also applies to: 141-186
66-85: Board enum ordering matchesboardPresetselect values
IO_BoardsEnumordering matches theboardPresetselect’s value order (0 = none/BUILD_TARGET, then Uno, Quad, Octa, 2Go, SergUni, SergMini, SE16, MHCD0, MHCP4Nano, YvesV48, TroyP4Nano, AtomS3, Luxceo, Cube). This preserves existing semantics for stored presets. Just keep in mind that inserting boards in the middle later will require updating either the enum ordering or the select value mapping to keep indices stable for already-saved configs.Also applies to: 106-123
277-278:std::sizeusage is good; alignethernetPinsloop and confirm C++17 supportUsing
std::size(ledPins)for the LED arrays is a nice improvement over rawsizeof. To keep things consistent and future‑proof (in caseethernetPins’ element type ever changes), I’d also switch that loop tostd::sizeand rely on C++17 standard library support:- uint8_t ethernetPins[6] = {28, 29, 30, 31, 34, 35}; - for (int i = 0; i < sizeof(ethernetPins); i++) pinAssigner.assignPin(ethernetPins[i], pin_Ethernet); // Ethernet Pins + uint8_t ethernetPins[] = {28, 29, 30, 31, 34, 35}; + for (int i = 0; i < std::size(ethernetPins); i++) + pinAssigner.assignPin(ethernetPins[i], pin_Ethernet); // Ethernet PinsAlso just double‑check that this TU (or a common header) includes the header that defines
std::size(typically<iterator>) and that you’re compiling as C++17 or newer.Also applies to: 313-314, 326-328, 414-416, 437-438, 464-465, 468-470, 454-455
368-384: SergMiniShield preset: no obvious pin conflicts; optional note on buttonsThe SergMiniShield preset now wires one LED pin, a relay‑lights‑on output, I2S pins, and I2C pins, with no reuse of the same GPIO for multiple roles. If this board is expected to expose a user button, you may eventually want to uncomment/add a
pin_Button_LightsOnmapping, but as-is there’s no functional issue.
411-433: MHCP4Nano:jumper1behavior and channel vs RS485 tradeoff look intentionalThe MHCP4Nano preset cleanly separates the two
jumper1modes:
jumper1on → 8 LED pins, plus dedicated RS485 and Exposed GPIOs.jumper1off → all 16 of those GPIOs are LED pins, with no RS485/Exposed usage.No pin conflicts are apparent. One design question (not a blocker): do you want
maxPowerto reflect the reduced number of LED channels in the 8‑channel (jumper on) mode, or is 100W an overall board/PSU cap regardless of how many channels are active?
436-455: TroyP4Nano mapping looks internally consistent; Ethernet pins marked explicitlyThe TroyP4Nano preset assigns 16 unique LED GPIOs, I2C, I2S, SDIO, reserved, Ethernet, and UART pins with no overlaps. Marking the RGMII/ETH pins via
pin_Ethernetis a nice touch. When you later extendreadPins()for non‑S3 targets, thesepin_Ethernetusages should give you a good hook for P4‑specific Ethernet init.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
interface/src/routes/moonbase/monitor/Monitor.svelte(4 hunks)platformio.ini(1 hunks)src/MoonBase/Modules/ModuleIO.h(10 hunks)src/MoonBase/Nodes.cpp(4 hunks)src/MoonBase/Nodes.h(1 hunks)src/MoonLight/Layers/PhysicalLayer.cpp(2 hunks)src/MoonLight/Layers/PhysicalLayer.h(4 hunks)src/MoonLight/Layers/VirtualLayer.cpp(6 hunks)src/MoonLight/Layers/VirtualLayer.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/MoonLight/Layers/PhysicalLayer.cpp
- src/MoonLight/Layers/VirtualLayer.cpp
- platformio.ini
🧰 Additional context used
🧠 Learnings (1)
📚 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.h
🧬 Code graph analysis (1)
src/MoonBase/Nodes.cpp (1)
src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)
⏰ 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 (16)
src/MoonLight/Layers/PhysicalLayer.h (1)
58-62: LGTM! Header fields added with proper alignment.The addition of
nrOfChannelsandlightPresetfields maintains the 40-byte struct alignment as required by Monitor.svelte. The placement ofnrOfChannelsat an even byte offset (32) ensures proper alignment, and the padding adjustment correctly preserves the total size.interface/src/routes/moonbase/monitor/Monitor.svelte (4)
57-62: LGTM! New header fields properly declared.The addition of
lightPresetandnrOfChannelsvariables along with thelightPreset_RGB2040constant aligns with the updated header structure in PhysicalLayer.h.
76-81: LGTM! Header parsing correctly reads new fields at documented offsets.The code correctly reads
nrOfChannelsfrom byte 32 andlightPresetfrom byte 34, matching the struct layout defined in PhysicalLayer.h.
111-114: LGTM! Position iteration now correctly handles per-light indexing.The change from iterating in blocks of 3 values to iterating per light with
indexP * 3stride correctly reflects that positions are stored as (x, y, z) triplets per light.
133-146: LGTM! RGB2040 handling addresses previous review comment.The computed
groupSize = 20 * channelsPerLightcorrectly replaces the previously hardcoded value of 60, making the RGB2040 skip logic adapt to different channel counts. This addresses the past review comment.src/MoonBase/Nodes.h (1)
21-37: LGTM! Comprehensive preset enumeration with clear naming.The
LightPresetsEnumprovides well-named constants for all supported light presets. The inclusion oflightPreset_countas the final entry enables validation and bounds checking. The move from a local member variable to a header-based state (as indicated in the summary) improves architectural consistency.src/MoonBase/Nodes.cpp (3)
348-362: LGTM! Preset control properly references header state.The change from a local
lightPresetvariable tolayerP.lights.header.lightPresetaligns with the header-based state management introduced in this PR. The control values match the newLightPresetsEnumentries.
468-475: LGTM! RGB2040 preset correctly configured.The RGB2040 preset uses standard RGB offsets (3 channels) with the comment correctly noting that special channel remapping is handled elsewhere in VirtualLayer. This aligns with the dual-channel-group architecture described in the PR summary.
496-509: LGTM! 32-channel moving head preset properly configured.The
lightPreset_MHBeTopper19x15W32case correctly configures all RGB offsets (RGB, RGB1, RGB2, RGB3) along with Pan, Tilt, Zoom, and Brightness for the 32-channel moving head fixture.src/MoonLight/Layers/VirtualLayer.h (1)
23-28: Verify that all references to the renamed enum have been updated.The enum
mapTypehas been renamed toMapTypeEnum. Ensure that all existing references throughout the codebase have been updated to use the new name. Search the codebase for any remaining occurrences ofmapTypeas a type reference to confirm the refactoring is complete.src/MoonBase/Modules/ModuleIO.h (6)
275-293: SE16V1 LED preset conversion toledPins[]loop looks correctThe SE16V1 preset now uses a
ledPins[]array with 16 distinct GPIOs and a loopedpin_LEDassignment; the remaining button, measurement, and SPI/PHY pins don’t overlap with those LED GPIOs. This reduces duplication without changing behavior.
312-331: QuinLED Dig Quad / Octa LED group refactor preserves semanticsFor DigQuadV3 and DigOctaV2, the new
ledPins[]arrays capture the same LED GPIO lists and drive them uniformly via the loop. No conflicting assignments are visible, and object["maxPower"] stays as before. The refactor looks behavior‑preserving.
385-407: SergUniShieldV5:jumper1-dependent LED routing looks consistentFor SergUniShieldV5, using
jumper1to choose between GPIO 1 vs 3 as the second LED pin (with GPIO 16 as the first) is consistent and doesn’t collide with button/relay/infrared/I2S/I2C mappings. Sincejumper1already triggers a preset rebuild inonUpdate, the runtime behavior should match the jumper state.
464-466: AtomS3 LED preset vialedPins[]is straightforwardThe AtomS3 preset now has a clear 4‑element LED group using the same
ledPins[]pattern as other boards. No other mappings in this branch, so there’s nothing conflicting.
467-471: Cube202010 LED preset conversion matches the existing intentCube202010 now uses a 10‑pin
ledPins[]array with a simple loop; the commented pin list below documents other candidate GPIOs without affecting behavior. Looks good.
471-477: Default preset: ESP32P4 override for LED pin is reasonableThe default preset keeps
maxPower = 10and now selects GPIO 37 for LED on ESP32P4 targets and 16 otherwise. This conditional keeps existing behavior for non‑P4 boards while making P4 defaults more sensible.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/MoonBase/Nodes.cpp (1)
483-522: Inconsistent header pointer usage in MH preset cases.The MH preset cases mix
header->andlayer->layerP->lights.header.access. While functionally equivalent (sinceheader = &layer->layerP->lights.header), this inconsistency reduces readability.Consider using the
header->pointer consistently throughout, matching the pattern in earlier cases:case lightPreset_MHBeeEyes150W15: - layer->layerP->lights.header.channelsPerLight = 15; + header->channelsPerLight = 15; header->offsetRed = 0; header->offsetGreen = 1; header->offsetBlue = 2; - layer->layerP->lights.header.offsetRGB = 10; - layer->layerP->lights.header.offsetPan = 0; - layer->layerP->lights.header.offsetTilt = 1; - layer->layerP->lights.header.offsetZoom = 7; - layer->layerP->lights.header.offsetBrightness = 8; - layer->layerP->lights.header.offsetGobo = 5; - layer->layerP->lights.header.offsetBrightness2 = 3; + header->offsetRGB = 10; + header->offsetPan = 0; + header->offsetTilt = 1; + header->offsetZoom = 7; + header->offsetBrightness = 8; + header->offsetGobo = 5; + header->offsetBrightness2 = 3; break;Apply similar changes to the other MH cases (lines 496-522).
src/MoonBase/Modules/ModuleIO.h (1)
277-278: Use ofstd::sizeon LED pin arrays looks good; just ensure C++17 and headersSwitching the LED pin loops to
uint8_t ledPins[]+for (int i = 0; i < std::size(ledPins); ++i)is a nice improvement oversizeofand keeps each board’s LED group within theindexcap (all ≤16, well under the 32 limit). I also checked for duplicate GPIOs within each board’s LED list and don’t see any collisions with other usages in the same board blocks.Only follow‑up: this header itself doesn’t include
<array>/<iterator>, so compilation depends on some transitive include supplyingstd::sizeand on the toolchain being C++17+. If that’s not already guaranteed project‑wide, consider:
- Adding the appropriate standard header in a common place included before
ModuleIO.h, or- Falling back to a small helper (e.g.,
template <typename T, size_t N> constexpr size_t countof(T (&)[N]) { return N; }).Also, for consistency, the
ethernetPinsloop on the Troy P4 Nano still usessizeof; you may optionally switch that tostd::size(ethernetPins)oncestd::sizeis guaranteed.// Optional consistency tweak inside the Troy P4 Nano block for (int i = 0; i < std::size(ethernetPins); i++) pinAssigner.assignPin(ethernetPins[i], pin_Ethernet);Also applies to: 313-315, 326-328, 347-348, 411-416, 425-427, 436-438, 464-466, 468-470
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/MoonBase/Modules/ModuleIO.h(11 hunks)src/MoonBase/Nodes.cpp(4 hunks)src/MoonLight/Layers/PhysicalLayer.h(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/MoonBase/Nodes.cpp (1)
src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)
⏰ 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 (10)
src/MoonLight/Layers/PhysicalLayer.h (2)
31-62: LGTM! Header struct changes are well-documented and alignment-aware.The new fields are correctly placed:
nrOfChannelsat an even offset (32) for properuint16_talignmentlightPresetinitialized toUINT8_MAXas a safe default (addresses past review)- Fill array adjusted to maintain 40-byte total
The comment on line 31 appropriately warns about alignment sensitivity for Monitor.svelte compatibility.
65-85: LGTM! Reset logic correctly differentiates between transient and driver-managed state.The asymmetric handling is appropriate:
nrOfChannelsis layout-derived (needs reset), whilelightPresetis a user/driver configuration that should persist.src/MoonBase/Nodes.cpp (4)
523-531: Default case write-back may cause unexpected UI behavior.When an invalid
lightPresetvalue is encountered, the code resets it tolightPreset_GRB. This write-back modifies the bound control value, which could surprise users if their selection appears to change unexpectedly.This is acceptable as a defensive fallback for corrupted state, but consider whether the UI should be notified to refresh the control value.
401-410: LGTM! Header-based lightPreset state management is cleaner.The refactor from local member variable to header-based state (
header->lightPreset) centralizes configuration and enables other components (VirtualLayer, D_ArtnetOut) to access the preset directly.
347-363: The enum ordering matches the control value order correctly. The LightPresetsEnum values (0-13) align perfectly with the addControlValue calls in DriverNode::setup(), and all cases are handled in the switch statement with a proper fallback to GRB.
468-475: The comment is accurate. VirtualLayer properly implements special RGB2040 channel remapping with multiple checks that apply index adjustment (indexP += (indexP / 20) * 20;) to skip the empty channel groups in the dual-channel-group architecture. No issues found.src/MoonBase/Modules/ModuleIO.h (4)
19-64: IO_PinUsageEnum andusageselect stay in lockstepThe
IO_PinUsageEnumordering (Unused → Reserved) still matches the"usage"select options and the values probed inreadPins()for SPI/PHY and other roles, so JSONusagevalues, UI labels, and runtime behaviour remain consistent. I don’t see mismatches or skipped indices here.Also applies to: 141-185, 571-589
66-85: Verify board enum ↔boardPresetvalue mapping, especially with new Serg boardsAdding
board_SergUniShieldV5/board_SergMiniShieldtoIO_BoardsEnumand exposing them in theboardPresetselect looks consistent, but this select appears to store raw board IDs. Please double‑check that:
- The numeric values used by
boardPresetstill align withIO_BoardsEnumafter inserting the Serg boards, and- Any persisted configs / headers that encode a board ID still decode to the intended board after this reordering.
If the select relies on implicit positional indexing, it may be safer to ensure
addControlValueexplicitly binds enum values rather than relying on order.Also applies to: 106-123
367-408: SergMiniShield / SergUniShieldV5 presets look self-consistentFor both SergMiniShield and SergUniShieldV5, the new mappings (LED on 16 plus optional second LED via
jumper1on 1 vs 3, dedicated Button_LightsOn, Relay_LightsOn, IR, I2S and I2C pins) avoid GPIO reuse within each board and fit under the per‑usageindexcap. Thejumper1handling for SergUniShieldV5 is also wired toonUpdate, so changes correctly trigger a preset rebuild.The commented placeholders for additional LED / button / temperature pins are harmless as comments; you can fill them in later without affecting current behaviour.
411-433: P4-related board presets (MHCP4Nano, TroyP4Nano, AtomS3, Cube202010, default) – mapping sanityThe new/updated presets for the P4‑oriented boards look coherent:
MHCP4Nano:
jumper1on: 8 LED outputs on{21,20,25,5,7,23,8,27}, separate RS485 pins{3,4,22,24}, and exposed{2,46,47,48}.jumper1off (default): those RS485/exposed pins are folded into the LED list for 16 total LED pins.- No GPIO is assigned two roles within either branch, and I2S pins
{33,26,32,36}don’t clash with LEDs/RS485/exposed.TroyP4Nano:
- 16 LED pins
{2,3,4,5,6,20,21,22,23,26,27,32,33,36,47,48}with no overlap with I2C, I2S, SDIO, reserved, Ethernet, or Serial pins.- The Ethernet group
{28,29,30,31,34,35}is clearly separated and tagged aspin_Ethernet, which is useful for documentation even if only SPI/PHY pins are currently consumed byreadPins().AtomS3 / Cube202010 / default:
- AtomS3’s four LED pins
{5,6,7,8}and Cube202010’s ten LEDs are unique within their blocks.- The
#ifdef CONFIG_IDF_TARGET_ESP32P4for the default board cleanly routes the single default LED to 37 on P4 and 16 otherwise.Functionally everything checks out from a mapping / duplication / index‑limit perspective. The only open item is hardware‑level: please confirm that the
jumper1semantics for MHCP4Nano (LED vs RS485/Exposed) and the chosen P4 default LED GPIO (37) match the actual board schematics.Also applies to: 436-456, 464-470, 473-477
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.