Board presets more presets #71
Conversation
Back-end ======== - ModuleIO: Octo -> Octa, Add Dig2Go, set index to 1 - Light control: rename pinRelayBrightness and pinToggleOnOff to pinRelayLightsOn and pinButtonLightsOn. readPins()/onUpdate: digitalWrite relay - Audio Sync: refactor no connection, set to 0 - Infrared: increase to 10 presets - Virtual layer: nrOfLights is max mapped
|
Caution Review failedThe pull request is closed. Note
|
| Cohort / File(s) | Summary |
|---|---|
Docs & Guides docs/moonlight/drivers.md, docs/gettingstarted/installer.md, docs/gettingstarted/installation.md, docs/develop/development.md, docs/moonbase/inputoutput.md |
Wording and structure updates: Art‑Net In/Out and Light preset reorganized; new ESPConnect flashing guide and updated flash command/baud/firmware paths; installer table adds Dig2Go and images; preset renaming to "Max Power in Watts" and other UI text tweaks. |
Board Presets & IO src/MoonBase/Modules/ModuleIO.h, docs/gettingstarted/installer.md |
Enum rename board_QuinLEDDigOctoV2 → board_QuinLEDDigOctaV2; added board_QuinLEDDig2Go and UI preset; setBoardPresetDefaults uses pin index default 1 (not 0); added Dig2Go/Octa pin mappings and maxPower adjustments; onUpdate rebuild on jumper1 change; safer ethernet-pin reading with validation and logging. |
Art‑Net / DDP Input src/MoonLight/Nodes/Drivers/D_ArtnetIn.h |
Removed ddpUdp; added universeMin/universeMax; new packet loop with start/stop listening, header/opcode validation, universe filtering, DDP and Art‑Net parsing, startPixel/universe→offset math with clamping, and per-layer or direct writes. |
Lights Control Pin Renames src/MoonLight/Modules/ModuleLightsControl.h |
Renamed pinRelayBrightness → pinRelayLightsOn and pinToggleOnOff → pinButtonLightsOn; readPins configures modes and initial states; onUpdate/loop/debounce and monitor logic updated to use new pins with validation and logs. |
GPIO Validation & Small Driver Edits src/MoonLight/Nodes/Drivers/D_Infrared.h, src/MoonLight/Modules/ModuleDrivers.h, src/MoonLight/Nodes/Drivers/D_AudioSync.h |
Added GPIO validity checks when reading GPIO values (with logging); Infrared palette bounds tightened using MIN/MAX to max 11; AudioSync name/log string changed to "Audio Sync" and network-stop clears sharedData only when previously init. |
Layer Indexing & Metadata src/MoonLight/Layers/VirtualLayer.cpp, src/MoonLight/Modules/ModuleEffects.h, src/MoonLight/Modules/ModuleChannels.h, src/MoonLight/Modules/ModuleMoonLightInfo.h |
VirtualLayer::addLight updates nrOfLights via max(current, indexV+1); various module setup loops shifted to 1-based layer indexing; ModuleMoonLightInfo adds numeric layer field populated as index+1. |
Ethernet Settings Types lib/framework/EthernetSettingsService.h |
Tightened Ethernet GPIO variable types under ESP32S3 from unsigned/int to int8_t and switched sentinel for invalid SCK to -1. |
Minor Cleanups src/MoonLight/Modules/ModuleDrivers.h, src/MoonLight/Modules/ModuleEffects.h |
Small formatting/whitespace removals and minor begin() default adjustments to align with 1-based layers. |
Sequence Diagram(s)
mermaid
sequenceDiagram
participant Net as Network (UDP)
participant ArtIn as ArtNetInDriver
participant LayerMgr as Layer Manager / VirtualLayer
participant LEDs as LED Output
Note over Net,ArtIn: UDP packets arrive (Art‑Net or DDP)
Net->>ArtIn: receivePacket(buf)
ArtIn->>ArtIn: validate header & opcode
alt universe ∈ [universeMin..universeMax]
ArtIn->>ArtIn: compute startPixel from universe+offset, clamp length
alt view == 0 (direct)
ArtIn->>LEDs: write channel bytes directly
else view != 0 (layered)
ArtIn->>LayerMgr: setLight(pixel,data,layer=view)
LayerMgr->>LEDs: apply aggregated writes
end
else out of range
ArtIn->>ArtIn: ignore packet
end
Note over ArtIn: on IP change → stop/listen toggles
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
- Pay special attention to:
src/MoonLight/Nodes/Drivers/D_ArtnetIn.h(parsing, endianness, removed ddpUdp),src/MoonBase/Modules/ModuleIO.h(enum rename, default pin index, jumper rebuild), andsrc/MoonLight/Modules/ModuleLightsControl.h(pin rename propagation). - Also check EthernetSettingsService type changes and places that rely on sentinel values for invalid GPIOs.
Possibly related PRs
- Board preset add presets #70 — Overlaps ModuleIO changes: preset wiring and ethernet pin handling updates.
- Board presets more presets #71 — Overlapping edits to ArtNetIn, VirtualLayer, ModuleLightsControl and other modules.
- Board presets tuning #66 — Related to ModuleLightsControl pin renames and pin-handling updates.
Suggested labels
enhancement
Poem
🐰 I hopped through code with twitchy toes,
New boards and pins where daylight glows,
Packets parsed and layers now one‑based—
I wiggle my nose and tidy the trace,
A rabbit's joy: bright lights in rows.
Pre-merge checks and finishing touches
❌ Failed checks (2 warnings)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check | The title "Board presets more presets" does not clearly convey the actual scope of changes, which span multiple systems including Art-Net drivers, firmware versioning, board enumeration, layer handling, and documentation updates. | Use a more descriptive title that reflects the primary changes, such as "Add QuinLED Dig 2Go support and expand board presets" or "Extend board presets and improve Art-Net driver configuration." | |
| Docstring Coverage | Docstring coverage is 10.34% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
✅ Passed checks (1 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
📜 Recent review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/framework/EthernetSettingsService.h(1 hunks)src/MoonBase/Modules/ModuleIO.h(6 hunks)src/MoonLight/Nodes/Drivers/D_ArtnetIn.h(3 hunks)
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: 0
🧹 Nitpick comments (4)
src/MoonLight/Nodes/Drivers/D_Infrared.h (1)
355-358: Consider using a shared constant for palette count.The hardcoded
10matches the current palette count inModuleLightsControl.h, but the TODO comment is now stale (references8). To avoid future sync issues, consider defining a shared constant (e.g.,MAX_PALETTE_INDEX) that both files reference.docs/moonlight/drivers.md (1)
36-36: Naming consistency update looks good. Consider adding alt text for accessibility.The "Audio Sync" naming is now consistent with the driver code. However, static analysis flagged a missing
altattribute on the image. For accessibility, consider:-| Audio Sync | <img width="100" src="https://github.com/user-attachments/assets/bfedf80b-6596-41e7-a563-ba7dd58cc476"/> | No controls | ... +| Audio Sync | <img width="100" alt="Audio Sync preview" src="https://github.com/user-attachments/assets/bfedf80b-6596-41e7-a563-ba7dd58cc476"/> | No controls | ...src/MoonLight/Modules/ModuleLightsControl.h (2)
80-83: Minor: Add spacing around ternary operator for consistency.- digitalWrite(pinRelayLightsOn, newBri>0?HIGH:LOW); + digitalWrite(pinRelayLightsOn, newBri > 0 ? HIGH : LOW);
157-159: Same spacing suggestion applies here.- digitalWrite(pinRelayLightsOn, newBri>0?HIGH:LOW); + digitalWrite(pinRelayLightsOn, newBri > 0 ? HIGH : LOW);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/moonlight/drivers.md(1 hunks)src/MoonBase/Modules/ModuleIO.h(4 hunks)src/MoonLight/Layers/VirtualLayer.cpp(1 hunks)src/MoonLight/Modules/ModuleDrivers.h(0 hunks)src/MoonLight/Modules/ModuleEffects.h(0 hunks)src/MoonLight/Modules/ModuleLightsControl.h(6 hunks)src/MoonLight/Nodes/Drivers/D_AudioSync.h(3 hunks)src/MoonLight/Nodes/Drivers/D_Infrared.h(1 hunks)
💤 Files with no reviewable changes (2)
- src/MoonLight/Modules/ModuleDrivers.h
- src/MoonLight/Modules/ModuleEffects.h
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-07T14:16:03.203Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-07T14:16:03.203Z
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: 2025-12-07T14:16:03.203Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-07T14:16:03.203Z
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.cpp
🪛 markdownlint-cli2 (0.18.1)
docs/moonlight/drivers.md
36-36: Images should have alternate text (alt text)
(MD045, no-alt-text)
36-36: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
⏰ 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 (8)
src/MoonLight/Nodes/Drivers/D_AudioSync.h (2)
29-35: Good resilience improvement.Zeroing the shared audio data on network disconnect prevents stale audio values from affecting reactive effects. The cleanup order (data zeroing before
init = false) is correct.
19-19: LGTM!Naming consistency update aligns with documentation.
src/MoonBase/Modules/ModuleIO.h (3)
69-70: LGTM!The
Octo→Octatypo fix and newboard_QuinLEDDig2Goenum addition are well-positioned.
330-343: New QuinLED Dig 2Go board preset looks correct.The pin assignments appear reasonable for this ESP32-based board with:
- Button, IR, LED, Relay, I2S audio, I2C, and exposed GPIO pins configured
- Commented-out MCLK suggests it may be unused on this board
235-235: Default index change from 0 to 1 is correct and aligns with the intended 1-based indexing scheme.The codebase validates pin indices with
index >= 1 && index <= 20(ModuleDrivers.h:47) and converts them to 0-based array indices usingindex-1(ModuleDrivers.h:48). Setting the default to 1 ensures newly added pins are valid by default and properly indexed. The previous default of 0 would have failed validation and caused pins not to be initialized.src/MoonLight/Modules/ModuleLightsControl.h (2)
28-29: Good semantic rename for clarity.The new names
pinRelayLightsOnandpinButtonLightsOnbetter describe the purpose of these pins compared to the previous generic names.
310-322: Button handling logic is correct.The debounce implementation properly handles the
INPUT_PULLUPconfiguration with LOW-active button press detection.src/MoonLight/Layers/VirtualLayer.cpp (1)
339-339: LGTM! Robust light-count tracking.Using
MAX(nrOfLights, indexV + 1)ensures the light count grows monotonically and correctly handles lights added in non-sequential order. This is more robust than unconditional assignment.
|
@coderabbitai, can you do a review of D_ArtnetIn.h. If I test it with Touchdesigner sending out a range of LEDs only the first LED lights up. Could you specifically look if the code is indeed accepting a while range of led values instead of only the first? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 40 🏁 Script executed: Length of output: 5887 @ewowi, I found the issue! The problem is that the code extracts the universe number but never uses it to calculate the LED offset. This means all Art-Net universes are writing to the same LEDs starting at position 0, so only the first LED(s) light up. Here's the problematic section in uint16_t universe = header->universe; // Universe is extracted...
uint16_t dataLength = (header->length >> 8) | (header->length << 8);
uint8_t* dmxData = packetBuffer + sizeof(ArtNetHeader);
int numPixels = min((uint16_t)(dataLength / layerP.lights.header.channelsPerLight), (uint16_t)(layerP.lights.header.nrOfLights));
for (int i = 0; i < numPixels; i++) {
// ...but the loop always writes starting at index 0!
memcpy(&layerP.lights.channels[i * layerP.lights.header.channelsPerLight], &dmxData[i * layerP.lights.header.channelsPerLight], layerP.lights.header.channelsPerLight);
}The Fix: You need to calculate a starting pixel offset based on the universe number: void handleArtNet() {
int packetSize = artnetUdp.parsePacket();
if (packetSize >= sizeof(ArtNetHeader)) {
artnetUdp.read(packetBuffer, packetSize);
if (memcmp(packetBuffer, "Art-Net", 7) == 0) {
ArtNetHeader* header = (ArtNetHeader*)packetBuffer;
uint16_t opcode = header->opcode;
if (opcode == 0x5000) {
uint16_t universe = header->universe;
uint16_t dataLength = (header->length >> 8) | (header->length << 8);
uint8_t* dmxData = packetBuffer + sizeof(ArtNetHeader);
// Calculate starting LED position based on universe
// Each Art-Net universe supports up to 512 DMX channels
int startPixel = universe * (512 / layerP.lights.header.channelsPerLight);
int numPixels = min((uint16_t)(dataLength / layerP.lights.header.channelsPerLight),
(uint16_t)(layerP.lights.header.nrOfLights - startPixel));
// Write to the correct offset
for (int i = 0; i < numPixels; i++) {
int ledIndex = startPixel + i;
if (ledIndex < layerP.lights.header.nrOfLights) {
memcpy(&layerP.lights.channels[ledIndex * layerP.lights.header.channelsPerLight],
&dmxData[i * layerP.lights.header.channelsPerLight],
layerP.lights.header.channelsPerLight);
}
}
}
}
}
}Additional Issue: The |
Installer: add Dig2Go Back-end ======== - Lights control: store nrOfPalettes - (virtual) Layers starting with 1 - Info: add layer number - Artnet in: layer start with 1, include universe - Infrared: use nrOfPalettes
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MoonLight/Nodes/Drivers/D_ArtnetIn.h (1)
30-44: Theviewcontrol is defined but never used.The
viewvariable and its UI controls (lines 36-44) are set up to allow selecting between the Physical layer and virtual Layers 1, 2, etc., but the actual packet handlers (handleArtNet()andhandleDDP()) always write tolayerP.lights.channelsregardless of theviewsetting.If layer selection is intended functionality, the handlers should be updated to respect the
viewsetting. For example, inhandleArtNet():// Write to the correct offset for (int i = 0; i < numPixels; i++) { int ledIndex = startPixel + i; if (ledIndex < layerP.lights.header.nrOfLights) { - memcpy(&layerP.lights.channels[ledIndex * layerP.lights.header.channelsPerLight], &dmxData[i * layerP.lights.header.channelsPerLight], layerP.lights.header.channelsPerLight); + if (view == 0) { + memcpy(&layerP.lights.channels[ledIndex * layerP.lights.header.channelsPerLight], &dmxData[i * layerP.lights.header.channelsPerLight], layerP.lights.header.channelsPerLight); + } else { + layerP.layers[view - 1]->setLight(ledIndex, &dmxData[i * layerP.lights.header.channelsPerLight], 0, layerP.lights.header.channelsPerLight); + } } }Similar changes would be needed for
handleDDP(). Would you like me to generate a complete implementation or open an issue to track this enhancement?
🧹 Nitpick comments (1)
src/MoonLight/Modules/ModuleLightsControl.h (1)
94-94: Consider moving member variable to conventional location.While functional, declaring
nrOfPalettesbetween methods is unconventional. Member variables are typically grouped together (e.g., near lines 28-29 with the other public members) for better code organization.Consider moving this declaration:
uint8_t pinRelayLightsOn = UINT8_MAX; uint8_t pinButtonLightsOn = UINT8_MAX; + uint8_t nrOfPalettes = 0; ModuleLightsControl(PsychicHttpServer* server, ESP32SvelteKit* sveltekit, FileManager* fileManager, ModuleIO* moduleIO) : Module("lightscontrol", server, sveltekit) {And remove from line 94.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/gettingstarted/installer.md(1 hunks)src/MoonLight/Modules/ModuleChannels.h(1 hunks)src/MoonLight/Modules/ModuleEffects.h(1 hunks)src/MoonLight/Modules/ModuleLightsControl.h(7 hunks)src/MoonLight/Modules/ModuleMoonLightInfo.h(3 hunks)src/MoonLight/Nodes/Drivers/D_ArtnetIn.h(3 hunks)src/MoonLight/Nodes/Drivers/D_Infrared.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/MoonLight/Nodes/Drivers/D_Infrared.h
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-07T14:16:03.203Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-07T14:16:03.203Z
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/D_ArtnetIn.h
🧬 Code graph analysis (3)
src/MoonLight/Modules/ModuleEffects.h (1)
docs/javascripts/moonlight.js (1)
i(107-107)
src/MoonLight/Modules/ModuleMoonLightInfo.h (1)
src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)
src/MoonLight/Modules/ModuleChannels.h (1)
docs/javascripts/moonlight.js (1)
i(107-107)
🪛 LanguageTool
docs/gettingstarted/installer.md
[style] ~15-~15: Using many exclamation marks might seem excessive (in this case: 21 exclamation marks for a text that’s 7894 characters long)
Context: ...-|----------------------|
| esp32-d0 | 
{: style="width:100px"} | | Dig Uno:
Dig Quad:
Dig2Go:
| esp32-d0-16mb | 
{: style="width:100px"} | | Dig Octa:
Serg ESP32{:target="_blank"} and Shield/i/93057/products/2021-08-14T14%3A44%3A14.418Z-shield_v3-1.jpg?1628927139)
| esp32-s3-devkitc-1-n8r8v | 
{: style="width:100px"} | | SE-16p
{: style="width:100px"} |
| esp32-s3-devkitc-1-n16r8v | {: style="width:100px"} | | Ali*{:target="_blank"} |
| esp32-s3-atoms3r | 
{: style="width:100px"} | | M5Stack store{:target="_blank"} |
| esp32-s3-zero-n4r2 | 
{: style="width:100px"} | | Ali*{:target="_blank"} |
| esp32-p4-nano | 
{: style="width:100px"} | esp32-c6-firmware-update *
| Waveshare{:target="_blank"}
ESP32-P4 shield:
| esp32-d0-wrover | 
{: style="width:100px"} | | Ali*{:target="_blank"} |
| esp32-d0-wrover-moonbase | {: style="width:100px"} | | Ali*{:target="_blank"} |
| esp32-d0-moonbase | {: style="width:100px"} | | |
| esp32-p4-olimex 🚧 | 
{: style="width:100px"} | | Olimex{:target="_blank"} |
| esp32-c3-devkitm-1 | 
{: style="width:100px"} | | Ali*{:target="_blank"} |
| esp32-c3-supermini | 
{: style="width:100px"} | | Ali*{:target="_blank"} |
| others | 
⏰ 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 (11)
docs/gettingstarted/installer.md (1)
15-15: LGTM!The Dig2Go board preset addition is well-formatted and consistent with the existing table entries. The documentation properly reflects the new board support added in the codebase.
src/MoonLight/Modules/ModuleLightsControl.h (4)
28-29: LGTM!The pin renames improve semantic clarity.
pinRelayLightsOnandpinButtonLightsOnmore accurately describe their purpose compared to the previous names.
74-88: LGTM!The
readPins()updates correctly implement the renamed pins with proper GPIO configuration (OUTPUT for relay, INPUT_PULLUP for button) and appropriate initialization logic.
126-126: LGTM!Correctly captures the number of palette options for use in palette-bound operations elsewhere in the codebase.
159-161: LGTM!All remaining updates consistently apply the pin renames throughout the relay control logic, button debouncing, comments, and logging. The implementations are correct and maintain proper functionality.
Also applies to: 260-260, 312-325, 335-335
src/MoonLight/Modules/ModuleEffects.h (1)
63-64: LGTM: 1-based layer indexing implemented consistently.The shift from 0-based to 1-based layer numbering improves user-facing clarity. This change aligns with similar updates in ModuleChannels.h (line 32) and ModuleMoonLightInfo.h (lines 37, 84) throughout the PR.
src/MoonLight/Modules/ModuleChannels.h (1)
32-38: LGTM: Layer naming now 1-based.Layer names now start at "Layer 1" instead of "Layer 0", improving user experience. This change is consistent with the 1-based layer indexing implemented across ModuleEffects.h and ModuleMoonLightInfo.h.
src/MoonLight/Modules/ModuleMoonLightInfo.h (2)
37-37: LGTM: Layer identifier field added.The new "layer" field provides a 1-based layer identifier in the info display, consistent with the coordinated shift to 1-based layer numbering throughout this PR.
84-84: LGTM: Layer field correctly populated with 1-based index.The layer identifier is correctly populated with
index + 1, establishing the 1-based layer numbering convention for user-facing displays.src/MoonLight/Nodes/Drivers/D_ArtnetIn.h (2)
38-44: LGTM: Layer naming aligns with 1-based convention.The layer naming now starts at "Layer 1", consistent with the coordinated 1-based layer numbering changes across ModuleEffects.h, ModuleChannels.h, and ModuleMoonLightInfo.h.
121-131: Loop bounds check prevents buffer overflow, but calculation uses unsafe casting pattern.The code works due to the defensive bounds check at line 129, but the
numPixelscalculation at line 124 has a problematic pattern: whenstartPixel >= nrOfLights, the subtraction(nrOfLights - startPixel)produces a negative value that gets cast touint16_t, wrapping to a very large positive number. While the subsequent loop check prevents actual buffer overflow, this should be improved:// Better approach: if (startPixel < layerP.lights.header.nrOfLights) { int numPixels = min((uint16_t)(dataLength / layerP.lights.header.channelsPerLight), (uint16_t)(layerP.lights.header.nrOfLights - startPixel)); // ... loop code ... }This validates
startPixelbefore attempting the subtraction, eliminating the unsafe cast and making the intent clearer.⛔ Skipped due to learnings
Learnt from: ewowi Repo: MoonModules/MoonLight PR: 0 File: :0-0 Timestamp: 2025-12-07T14:16:03.203Z 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.
|
When testing artnet in, I only receive a few packages after booting the device, see the log below. Is there an error in the artnet receiving protocol? e.g. do I need to send a confirm reply message back? I receive artnet data from touch designer, if I send it to a hardware artnet controller it runs fine, but sending it to my code, only processing a few packages. [ 6418][I][WiFiStatus.cpp:48] onStationModeDisconnected(): [🐼] WiFi Disconnected. Reason code=201 (NO_AP_FOUND) No artnet packages received after this. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 6272 @ewowi, I found the issue! The problem is that your code only processes ONE packet per loop iteration, but Art-Net streams packets at high frequency (30-60+ packets per second). The UDP receive buffer fills up and then the operating system drops new packets. Here's the problem in your current code: void handleArtNet() {
int packetSize = artnetUdp.parsePacket(); // Only checks ONCE
if (packetSize >= sizeof(ArtNetHeader)) {
artnetUdp.read(packetBuffer, packetSize); // Reads ONE packet
// ... process packet ...
}
// Returns after processing just one packet!
}The Fix: You need to drain the UDP buffer by processing ALL available packets in each call: void handleArtNet() {
// Process ALL available packets in the buffer
while (int packetSize = artnetUdp.parsePacket()) {
if (packetSize >= sizeof(ArtNetHeader)) {
artnetUdp.read(packetBuffer, min(packetSize, (int)sizeof(packetBuffer)));
// Verify Art-Net packet
if (memcmp(packetBuffer, "Art-Net", 7) == 0) {
ArtNetHeader* header = (ArtNetHeader*)packetBuffer;
uint16_t opcode = header->opcode;
EXT_LOGD(ML_TAG, "size:%d universe:%d", packetSize, header->universe);
// Check if it's a DMX packet (opcode 0x5000)
if (opcode == 0x5000) {
uint16_t universe = header->universe;
uint16_t dataLength = (header->length >> 8) | (header->length << 8);
uint8_t* dmxData = packetBuffer + sizeof(ArtNetHeader);
int startPixel = universe * (512 / layerP.lights.header.channelsPerLight);
int numPixels = min((uint16_t)(dataLength / layerP.lights.header.channelsPerLight),
(uint16_t)(layerP.lights.header.nrOfLights - startPixel));
for (int i = 0; i < numPixels; i++) {
int ledIndex = startPixel + i;
if (ledIndex < layerP.lights.header.nrOfLights) {
memcpy(&layerP.lights.channels[ledIndex * layerP.lights.header.channelsPerLight],
&dmxData[i * layerP.lights.header.channelsPerLight],
layerP.lights.header.channelsPerLight);
}
}
}
}
}
}
}
void handleDDP() {
// Same fix - drain all packets
while (int packetSize = ddpUdp.parsePacket()) {
if (packetSize >= sizeof(DDPHeader)) {
ddpUdp.read(packetBuffer, min(packetSize, (int)sizeof(packetBuffer)));
DDPHeader* header = (DDPHeader*)packetBuffer;
bool pushFlag = (header->flags & 0x80) != 0;
uint8_t dataType = header->dataType;
uint32_t offset = (header->offset >> 24) | ((header->offset >> 8) & 0xFF00) |
((header->offset << 8) & 0xFF0000) | (header->offset << 24);
uint16_t dataLen = (header->dataLen >> 8) | (header->dataLen << 8);
if (dataType == 0x01) {
uint8_t* pixelData = packetBuffer + sizeof(DDPHeader);
int startPixel = offset / layerP.lights.header.channelsPerLight;
int numPixels = min((uint16_t)(dataLen / layerP.lights.header.channelsPerLight),
(uint16_t)(layerP.lights.header.nrOfLights - startPixel));
for (int i = 0; i < numPixels; i++) {
int ledIndex = startPixel + i;
if (ledIndex < layerP.lights.header.nrOfLights) {
memcpy(&layerP.lights.channels[ledIndex * layerP.lights.header.channelsPerLight],
&pixelData[i * layerP.lights.header.channelsPerLight],
layerP.lights.header.channelsPerLight);
}
}
}
}
}
}Why This Happens:
Why Hardware Controllers Work: Commercial Art-Net controllers are dedicated devices with optimized network stacks that prioritize UDP packet processing. They typically:
No Reply Needed: You're correct that Art-Net doesn't require ACK/reply messages - it's a one-way streaming protocol. The issue is purely about buffer management. |
Back-end ======== - Module IO: jumper1 triggers redefine - Art-Net in: process all packets, store in physical layer or virtual
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/MoonBase/Modules/ModuleIO.h (1)
454-458: State modification pattern inconsistency.Line 456 directly modifies
_state.data["modded"], while similar state changes elsewhere (line 463-464) use theupdate()method which ensures proper persistence and notifications.However, since
setBoardPresetDefaults()(triggered vianewBoardID) already setsmodded=falseat line 224 and callsupdate()at line 437, this direct assignment may be redundant. Consider either:
- Removing line 456 if the intent is to let
setBoardPresetDefaultshandle it, or- Using the
update()pattern if the immediate change is needed before the board preset rebuildsrc/MoonLight/Modules/ModuleLightsControl.h (1)
310-323: Consider more robust debounce pattern (optional).The current debounce implementation enforces a minimum time between accepted state changes, which works for most cases. However, a more robust pattern would verify that the button state remains stable for the debounce period before accepting the change.
Current pattern prevents rapid repeated triggers but could theoretically accept a bouncing state if timing aligns unfavorably. For a simple toggle use case, the existing implementation is adequate. If you encounter button behavior issues, consider this more robust pattern:
if (pinButtonLightsOn != UINT8_MAX) { int state = digitalRead(pinButtonLightsOn); // Reset debounce timer on any state change if (state != lastState) { lastDebounceTime = millis(); lastState = state; } // Accept stable state after debounce period static int acceptedState = HIGH; if ((millis() - lastDebounceTime) > debounceDelay && state != acceptedState) { acceptedState = state; // Trigger only on button press if (state == LOW) { JsonDocument doc; JsonObject newState = doc.to<JsonObject>(); newState["lightsOn"] = !_state.data["lightsOn"]; update(newState, ModuleState::update, _moduleName + "server"); } } }src/MoonLight/Nodes/Drivers/D_ArtnetIn.h (1)
97-135: Art‑Net universe mapping and packet draining are correct; consider a small defensive guardThe new
handleArtNet():
- Properly drains all pending UDP packets and reads each into
packetBuffer.- Verifies the Art‑Net ID and DMX opcode before touching data.
- Computes
startPixelfromuniverse * (512 / channelsPerLight)andnumPixelsfrom bothdataLengthand remaining LEDs, then iterates overnumPixelsand applies per‑pixel data either to the physical buffer (view == 0) or to the selected layer viasetLight().This should fix the “only first LED lights up” problem and correctly consume full DMX frames per universe.
One optional improvement: if
startPixel >= layerP.lights.header.nrOfLights, you could early‑out from the DMX section to avoid looping with no writes:- int startPixel = universe * (512 / layerP.lights.header.channelsPerLight); + int startPixel = universe * (512 / layerP.lights.header.channelsPerLight); + if (startPixel >= layerP.lights.header.nrOfLights) { + continue; // No LEDs mapped for this universe + }Not required for correctness because of the
ledIndex < nrOfLightscheck, but it keeps intent clear and avoids useless iterations.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/MoonBase/Modules/ModuleIO.h(5 hunks)src/MoonLight/Modules/ModuleLightsControl.h(6 hunks)src/MoonLight/Nodes/Drivers/D_ArtnetIn.h(2 hunks)src/MoonLight/Nodes/Drivers/D_Infrared.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/MoonLight/Nodes/Drivers/D_Infrared.h
⏰ 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 (6)
src/MoonBase/Modules/ModuleIO.h (3)
69-70: LGTM!The enum rename (Octo → Octa) correctly matches the QuinLED product naming, and the new
board_QuinLEDDig2Gois properly inserted with consistent ordering between the enum and UI dropdown values.
235-235: LGTM!Good fix. The previous default of
0was outside the valid range defined at line 187 (1-32). Setting it to1ensures pins start with a valid index value.
330-343: LGTM!The pin mappings for the QuinLED Dig 2Go board look well-structured with appropriate usage assignments.
Consider whether an explicit
object["maxPower"]value should be set for this board, similar to other QuinLED boards (Uno sets 50W, Quad sets 150W). Currently it will use the module default of 10W.src/MoonLight/Modules/ModuleLightsControl.h (2)
28-29: LGTM: Clearer naming convention.The renamed pin variables (
pinRelayLightsOnandpinButtonLightsOn) better describe their purpose compared to the previous names.
157-160: LGTM: Efficient relay state management.The code correctly checks if the brightness state has transitioned between zero and non-zero before updating the relay, avoiding unnecessary GPIO writes. The validation that
pinRelayLightsOnis configured before use is also appropriate.src/MoonLight/Nodes/Drivers/D_ArtnetIn.h (1)
30-44: View control and 1‑based layer labeling look consistentUsing
view = 0for the physical layer and starting the displayed layer index at 1 (with “Physical layer” as the first option) matches the laterview == 0/view - 1logic in the packet handlers. No issues here.
|
@coderabbitai, regarding the artnet-in remark you made:
I guess it's a necessary but not sufficient fix as it is still only showing a a few package and then nothing is received anymore |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 6275 @ewowi, you're absolutely right - the buffer drain fix is necessary but not sufficient. Looking at the current code and your logs, I can see several critical issues that would cause packets to stop after a few seconds: Critical Issues Found
Comprehensive FixHere's the corrected code with proper error handling and recovery: bool init = false;
unsigned long lastPacketTime = 0;
const unsigned long SOCKET_TIMEOUT = 5000; // Reset socket if no packets for 5 seconds
void loop() override {
// Check network connectivity
bool hasNetwork = WiFi.localIP() || ETH.localIP();
if (!hasNetwork) {
if (init) {
// Network lost - cleanup
if (ddp)
ddpUdp.stop();
else
artnetUdp.stop();
init = false;
EXT_LOGD(ML_TAG, "%s stopped (network lost)", ddp ? "DDP" : "Art-Net");
}
return;
}
if (!init) {
// Initialize UDP
bool success = ddp ? ddpUdp.begin(port) : artnetUdp.begin(port);
if (success) {
init = true;
lastPacketTime = millis();
EXT_LOGD(ML_TAG, "Listening for %s on port %d", ddp ? "DDP" : "Art-Net", port);
} else {
EXT_LOGE(ML_TAG, "Failed to start UDP on port %d", port);
return;
}
}
// Watchdog: restart socket if no packets received for too long
if (millis() - lastPacketTime > SOCKET_TIMEOUT && lastPacketTime != 0) {
EXT_LOGW(ML_TAG, "No packets received for %d seconds, restarting %s socket",
SOCKET_TIMEOUT / 1000, ddp ? "DDP" : "Art-Net");
if (ddp)
ddpUdp.stop();
else
artnetUdp.stop();
init = false;
return;
}
if (ddp)
handleDDP();
else
handleArtNet();
}
void handleArtNet() {
int packetsProcessed = 0;
// Process ALL available packets in the buffer
while (int packetSize = artnetUdp.parsePacket()) {
packetsProcessed++;
lastPacketTime = millis();
if (packetSize < sizeof(ArtNetHeader) || packetSize > sizeof(packetBuffer)) {
artnetUdp.flush();
continue;
}
artnetUdp.read(packetBuffer, packetSize);
// Verify Art-Net packet
if (memcmp(packetBuffer, "Art-Net", 7) != 0) {
continue;
}
ArtNetHeader* header = (ArtNetHeader*)packetBuffer;
// Check if it's a DMX packet (opcode 0x5000)
if (header->opcode != 0x5000) {
continue;
}
uint16_t universe = header->universe;
uint16_t dataLength = (header->length >> 8) | (header->length << 8);
// Validate data length
if (dataLength > packetSize - sizeof(ArtNetHeader)) {
continue;
}
uint8_t* dmxData = packetBuffer + sizeof(ArtNetHeader);
int startPixel = universe * (512 / layerP.lights.header.channelsPerLight);
if (startPixel >= layerP.lights.header.nrOfLights) {
continue; // Universe beyond our LED range
}
int numPixels = min((uint16_t)(dataLength / layerP.lights.header.channelsPerLight),
(uint16_t)(layerP.lights.header.nrOfLights - startPixel));
for (int i = 0; i < numPixels; i++) {
int ledIndex = startPixel + i;
if (ledIndex >= layerP.lights.header.nrOfLights) break;
if (view == 0) {
memcpy(&layerP.lights.channels[ledIndex * layerP.lights.header.channelsPerLight],
&dmxData[i * layerP.lights.header.channelsPerLight],
layerP.lights.header.channelsPerLight);
} else {
// Bounds check for layer access
size_t layerIndex = view - 1;
if (layerIndex < layerP.layers.size()) {
layerP.layers[layerIndex]->setLight(ledIndex,
&dmxData[i * layerP.lights.header.channelsPerLight],
0,
layerP.lights.header.channelsPerLight);
}
}
}
}
// Log only when packets were processed (reduced logging)
if (packetsProcessed > 0) {
EXT_LOGV(ML_TAG, "Processed %d Art-Net packets", packetsProcessed);
}
}
void handleDDP() {
int packetsProcessed = 0;
while (int packetSize = ddpUdp.parsePacket()) {
packetsProcessed++;
lastPacketTime = millis();
if (packetSize < sizeof(DDPHeader) || packetSize > sizeof(packetBuffer)) {
ddpUdp.flush();
continue;
}
ddpUdp.read(packetBuffer, packetSize);
DDPHeader* header = (DDPHeader*)packetBuffer;
if (header->dataType != 0x01) {
continue; // Not RGB data
}
uint32_t offset = (header->offset >> 24) | ((header->offset >> 8) & 0xFF00) |
((header->offset << 8) & 0xFF0000) | (header->offset << 24);
uint16_t dataLen = (header->dataLen >> 8) | (header->dataLen << 8);
// Validate data length
if (dataLen > packetSize - sizeof(DDPHeader)) {
continue;
}
uint8_t* pixelData = packetBuffer + sizeof(DDPHeader);
int startPixel = offset / layerP.lights.header.channelsPerLight;
if (startPixel >= layerP.lights.header.nrOfLights) {
continue;
}
int numPixels = min((uint16_t)(dataLen / layerP.lights.header.channelsPerLight),
(uint16_t)(layerP.lights.header.nrOfLights - startPixel));
for (int i = 0; i < numPixels; i++) {
int ledIndex = startPixel + i;
if (ledIndex >= layerP.lights.header.nrOfLights) break;
if (view == 0) {
memcpy(&layerP.lights.channels[ledIndex * layerP.lights.header.channelsPerLight],
&pixelData[i * layerP.lights.header.channelsPerLight],
layerP.lights.header.channelsPerLight);
} else {
size_t layerIndex = view - 1;
if (layerIndex < layerP.layers.size()) {
layerP.layers[layerIndex]->setLight(ledIndex,
&pixelData[i * layerP.lights.header.channelsPerLight],
0,
layerP.lights.header.channelsPerLight);
}
}
}
}
if (packetsProcessed > 0) {
EXT_LOGV(ML_TAG, "Processed %d DDP packets", packetsProcessed);
}
}Key Improvements
Test this and let me know if packets continue flowing! |
Back end ======== - IO (Ethernet), LightsControl(button and relay lightson), : Add pincheck in readPins - Artnet in: refactor and add artnetUDP.clear (fixed the bug)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/MoonLight/Nodes/Drivers/D_ArtnetIn.h (1)
145-171: Add layer bounds checking in DDP handler.The DDP implementation correctly addresses the previous double-write issue and mirrors the Art-Net view-based routing. However, the same layer bounds checking issue exists here.
Apply this diff to add bounds checking:
for (int i = 0; i < numPixels; i++) { int ledIndex = startPixel + i; if (ledIndex < layerP.lights.header.nrOfLights) { if (view == 0) { memcpy(&layerP.lights.channels[ledIndex * layerP.lights.header.channelsPerLight], &pixelData[i * layerP.lights.header.channelsPerLight], layerP.lights.header.channelsPerLight); - } else { + } else if (view - 1 < layerP.layers.size()) { layerP.layers[view - 1]->setLight(ledIndex, &pixelData[i * layerP.lights.header.channelsPerLight], 0, layerP.lights.header.channelsPerLight); + } else { + EXT_LOGE(ML_TAG, "Invalid layer view %d", view); } } }
🧹 Nitpick comments (3)
src/MoonLight/Nodes/Drivers/D_ArtnetIn.h (1)
72-86: Excellent: UDP buffer draining pattern implemented.The while-loop correctly drains all available packets, addressing the critical packet loss issue documented in the PR objectives. The bounds checking and error handling are appropriate.
Minor observation: The inner condition at line 78 (
if (packetSize >= sizeof(ArtNetHeader))) is redundant with the outer check at line 73, since packets smaller thansizeof(ArtNetHeader)are already skipped viacontinue. This doesn't affect correctness but could be simplified.Optional simplification:
while (int packetSize = artnetUdp.parsePacket()) { if (packetSize < sizeof(ArtNetHeader) || packetSize > sizeof(packetBuffer)) { artnetUdp.clear(); continue; } - if (packetSize >= sizeof(ArtNetHeader)) { - artnetUdp.read(packetBuffer, min(packetSize, (int)sizeof(packetBuffer))); + artnetUdp.read(packetBuffer, min(packetSize, (int)sizeof(packetBuffer))); - if (ddp) - handleDDP(); - else - handleArtNet(); - } + if (ddp) + handleDDP(); + else + handleArtNet(); }src/MoonBase/Modules/ModuleIO.h (2)
324-343: QuinLED Dig Octa V2 and new Dig 2Go presets look reasonable; consider explicitmaxPowerif knownThe Octa V2 preset and the new Dig 2Go preset mappings are straightforward and consistent with how other boards use
PinAssigner(usage groups will get 1‑based indices, multiplepin_Exposedpins will be indexed 1,2, …). Functionally this looks fine.If you know realistic power limits for these boards (similar to how SE16, Uno, Quad, Penta, etc. set
object["maxPower"]), consider adding an explicitmaxPoweroverride here as well; otherwise they’ll inherit the default 10 W in the finalelsebranch, which might be unnecessarily conservative for these larger boards.
494-538: GPIO validation for Ethernet pin discovery improves robustness; consider minor consolidationValidating
GPIO_IS_VALID_GPIO(gpio)before assigning the SPI and PHY pins toEthernetSettingsServiceis a solid safety improvement and the error logging will help diagnose misconfigured presets. Leaving the fields atUINT8_MAXif any pin is invalid ensuresinitEthernet()is only called when a complete, valid set is found.The repeated
if (usage == ...) { if (GPIO_IS_VALID_GPIO(...)) ... }pattern is fine as‑is, but if this grows further, you might consider a small helper (or aswitchonusage) to reduce duplication.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/MoonBase/Modules/ModuleIO.h(6 hunks)src/MoonLight/Modules/ModuleLightsControl.h(6 hunks)src/MoonLight/Nodes/Drivers/D_ArtnetIn.h(3 hunks)src/MoonLight/Nodes/Drivers/D_AudioSync.h(3 hunks)src/MoonLight/Nodes/Drivers/D_Infrared.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/MoonLight/Modules/ModuleLightsControl.h
⏰ 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 (11)
src/MoonLight/Nodes/Drivers/D_AudioSync.h (4)
19-19: LGTM: Display name updated for consistency.The space added to "Audio Sync" improves readability in the UI.
44-44: LGTM: Log message updated for consistency.The log message now matches the updated driver name "Audio Sync".
53-53: LGTM: Comment updated for consistency.The commented log example now uses "Audio Sync" consistently with the driver name.
29-37: Good improvement for network resilience.The new conditional cleanup properly resets shared audio data and the init flag on network disconnect, allowing clean reinitialization when connectivity is restored. This addresses the network state recovery issue mentioned in the PR objectives.
Two points to verify:
WLEDSync cleanup: Confirm that the
WLEDSynclibrary doesn't require an explicitstop()or cleanup method call before callingbegin()again on line 42. Some UDP-based libraries need to close sockets explicitly.Work in progress: The comment on line 28 says "WIP". Is further work planned for this network failure handling?
#!/bin/bash # Search for WLEDSync class definition and methods to check for cleanup/stop methods rg -n -C3 'class WLEDSync' --type=cpp --type=h rg -n '\b(stop|close|end|cleanup)\s*\(' --type=cpp --type=h -g '*WLED-sync*' -g '*WLEDSync*'src/MoonLight/Nodes/Drivers/D_ArtnetIn.h (3)
37-43: LGTM: 1-based layer indexing.The change to start layer naming at 1 aligns with the broader 1-based layer indexing approach documented in the AI summary and related module updates.
57-64: LGTM: Robust network connectivity handling.The network state check properly stops the UDP listener when connectivity is lost and allows it to restart when connectivity returns. This addresses the network recovery issues mentioned in the PR objectives.
113-171: Summary: Critical fixes successfully implemented.This update successfully addresses the major issues documented in the PR objectives:
✅ Universe handling fixed: The Art-Net handler now correctly computes
startPixelfrom the universe number, allowing multiple universes to map to different LED ranges.✅ Packet draining implemented: The while-loop pattern processes all available UDP packets per iteration, preventing buffer overflow and packet loss.
✅ Network recovery added: UDP socket is properly restarted when network connectivity is restored.
✅ View-based routing: Proper delegation to either physical buffer or layer surfaces based on the view selection.
The implementation aligns well with the comprehensive fixes suggested in the PR comment thread. Great work on addressing these critical networking issues!
src/MoonBase/Modules/ModuleIO.h (4)
65-84: Enum additions for OctaV2 and Dig2Go look consistent; verify external uses of the renamed constantRenaming to
board_QuinLEDDigOctaV2and insertingboard_QuinLEDDig2Goimmediately after keeps the numeric ordering stable and matches the UI options below. Just double‑check that all references to the oldboard_QuinLEDDigOctoV2name (if any remain in other files or build configs) have been updated so the build doesn’t break on stale enum names.
105-124: Board preset option for “QuinLED Dig 2Go” correctly aligned with enum orderAdding
"QuinLED Dig 2Go"directly after"QuinLED Dig Octa v2"keeps the select’s indices in sync with theIO_Boardsenum ordering. This should ensure thatboardPresetcontinues to map 1:1 toboard_QuinLEDDig2Gowithout extra migration logic.
221-237: Defaultindexinitialisation now matches control range; confirm no code relied onindex == 0Changing the reset path to
pin["index"] = 1;brings the default in line with the control’s 1–32 range and thePinAssignerbehavior (first assigned pin per usage gets index 1). Please just confirm there’s no downstream logic that usedindex == 0as a sentinel for “unassigned” pins, since unused pins will now have index 1 as well.
442-458: Rebuilding board preset onjumper1changes is sensible; side effects look controlledUsing the
jumper1update to force_state.data["modded"] = falseand queue anewBoardIDrebuild ensures the preset reflects the new jumper setting, which is important for boards like SE16 that repurpose pins. SincesetBoardPresetDefaultslater callsupdate(...), the UI should be brought back in sync withmodded=false, and the extranewBoardIDwrite won’t loop because theboardPresetbranch is gated onupdateOriginIdnot containing"server". This looks correct.
| void handleArtNet() { | ||
| // LightsHeader* header = &layerP.lights.header; | ||
|
|
||
| int packetSize = artnetUdp.parsePacket(); | ||
|
|
||
| if (packetSize >= sizeof(ArtNetHeader)) { | ||
| artnetUdp.read(packetBuffer, packetSize); | ||
|
|
||
| // Verify Art-Net packet | ||
| if (memcmp(packetBuffer, "Art-Net", 7) == 0) { | ||
| ArtNetHeader* header = (ArtNetHeader*)packetBuffer; | ||
| uint16_t opcode = header->opcode; | ||
| // | ||
| // EXT_LOGD(ML_TAG, "%d", header->universe); | ||
| // Verify Art-Net packet | ||
| if (memcmp(packetBuffer, "Art-Net", 7) == 0) { | ||
| ArtNetHeader* header = (ArtNetHeader*)packetBuffer; | ||
| uint16_t opcode = header->opcode; | ||
|
|
||
| // Check if it's a DMX packet (opcode 0x5000) | ||
| if (opcode == 0x5000) { | ||
| uint16_t universe = header->universe; | ||
| uint16_t dataLength = (header->length >> 8) | (header->length << 8); // Swap bytes | ||
| // EXT_LOGD(ML_TAG, "size:%d universe:%d", packetSize, header->universe); | ||
|
|
||
| // Process if it's our universe | ||
| // Check if it's a DMX packet (opcode 0x5000) | ||
| if (opcode == 0x5000) { | ||
| uint16_t universe = header->universe; | ||
| uint16_t dataLength = (header->length >> 8) | (header->length << 8); | ||
|
|
||
| // if (universe == artnetUniverse) { // all universes welcome | ||
| uint8_t* dmxData = packetBuffer + sizeof(ArtNetHeader); | ||
| uint8_t* dmxData = packetBuffer + sizeof(ArtNetHeader); | ||
|
|
||
| // Map DMX channels to LEDs (3 channels per LED: RGB) | ||
| int numPixels = min((uint16_t)(dataLength / layerP.lights.header.channelsPerLight), (uint16_t)(layerP.lights.header.nrOfLights)); | ||
| int startPixel = universe * (512 / layerP.lights.header.channelsPerLight); | ||
| int numPixels = min((uint16_t)(dataLength / layerP.lights.header.channelsPerLight), (uint16_t)(layerP.lights.header.nrOfLights - startPixel)); | ||
|
|
||
| for (int i = 0; i < numPixels; i++) { | ||
| memcpy(&layerP.lights.channels[i * layerP.lights.header.channelsPerLight], &dmxData[i * layerP.lights.header.channelsPerLight], layerP.lights.header.channelsPerLight); | ||
| for (int i = 0; i < numPixels; i++) { | ||
| int ledIndex = startPixel + i; | ||
| if (ledIndex < layerP.lights.header.nrOfLights) { | ||
| if (view == 0) { | ||
| memcpy(&layerP.lights.channels[ledIndex * layerP.lights.header.channelsPerLight], &dmxData[i * layerP.lights.header.channelsPerLight], layerP.lights.header.channelsPerLight); | ||
| } else { | ||
| layerP.layers[view - 1]->setLight(ledIndex, &dmxData[i * layerP.lights.header.channelsPerLight], 0, layerP.lights.header.channelsPerLight); | ||
| } | ||
| } | ||
|
|
||
| // FastLED.show(); | ||
| // Serial.println("Art-Net: " + String(numPixels) + " pixels updated"); | ||
| // } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Art-Net DMX protocol length field byte order specification
💡 Result:
The Art-Net length field is big-endian: the spec defines LengthHi then LengthLo (High byte first, Low byte second). [1]
Sources
- Art-Net 4 Protocol Release (packet definitions showing LengthHi / LengthLo and "High Byte. Low Byte."). [1]
🏁 Script executed:
# Find the ArtNetHeader struct definition
rg -t cpp "struct.*ArtNetHeader|class.*ArtNetHeader" -A 20Repository: MoonModules/MoonLight
Length of output: 1605
🏁 Script executed:
# Also check the layerP structure to understand the layers member
rg -t cpp "layerP\." -B 5 -A 2 | head -100Repository: MoonModules/MoonLight
Length of output: 5783
🏁 Script executed:
# Find layerP declaration and structure
rg -t cpp "layerP" --no-heading | grep -E "(extern|LayerProvider|struct|class)" | head -20Repository: MoonModules/MoonLight
Length of output: 491
🏁 Script executed:
# Find the LayerProvider or layers type definition
rg -t cpp "struct LayerProvider|class LayerProvider" -A 30Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
# Check what type layers is
rg -t cpp "layers\s*=" -B 3 -A 3Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
# Find PhysicalLayer class definition
rg -t cpp "class PhysicalLayer|struct PhysicalLayer" -A 50 | head -100Repository: MoonModules/MoonLight
Length of output: 3746
🏁 Script executed:
# Check where view parameter comes from in the driver context
rg -t cpp "view\s*=" -B 5 -A 2 | grep -A 7 -B 7 "D_ArtnetIn"Repository: MoonModules/MoonLight
Length of output: 2710
🏁 Script executed:
# Check the full handleArtNet function context around opcode check
sed -n '113,145p' src/MoonLight/Nodes/Drivers/D_ArtnetIn.hRepository: MoonModules/MoonLight
Length of output: 1470
🏁 Script executed:
# Check if there's any endian conversion utilities or preprocessor macros
rg -t cpp "endian|ntoh|hton" -B 2 -A 2Repository: MoonModules/MoonLight
Length of output: 47
Add bounds checking for layer access to prevent out-of-bounds vector access.
The view parameter can cause an out-of-bounds access when indexing into layerP.layers. Since view is set from external input without validation and layerP.layers is a dynamically-sized vector, accessing layerP.layers[view - 1] when view > layerP.layers.size() will crash the system.
Add the bounds check:
for (int i = 0; i < numPixels; i++) {
int ledIndex = startPixel + i;
if (ledIndex < layerP.lights.header.nrOfLights) {
if (view == 0) {
memcpy(&layerP.lights.channels[ledIndex * layerP.lights.header.channelsPerLight], &dmxData[i * layerP.lights.header.channelsPerLight], layerP.lights.header.channelsPerLight);
- } else {
+ } else if (view - 1 < layerP.layers.size()) {
layerP.layers[view - 1]->setLight(ledIndex, &dmxData[i * layerP.lights.header.channelsPerLight], 0, layerP.lights.header.channelsPerLight);
+ } else {
+ EXT_LOGE(ML_TAG, "Invalid layer view %d", view);
}
}
}🤖 Prompt for AI Agents
In src/MoonLight/Nodes/Drivers/D_ArtnetIn.h around lines 113-143, the code
indexes layerP.layers[view - 1] without validating that view is within [1,
layerP.layers.size()], risking out-of-bounds access; add a guard before using
layerP.layers[view - 1] that verifies view >= 1 and size_t(view - 1) <
layerP.layers.size() (and that the pointer at that index is non-null), and if
the check fails skip/continue handling that DMX packet (or fall back to the
view==0 path) to prevent crashes.
| } else if (combined_code == codePaletteInc) { // palette increase | ||
| newState["palette"] = MIN(state.data["palette"].as<uint8_t>() + 1, 11); // to do: replace 8 with max palette count | ||
| } else if (combined_code == codePaletteDec) { // palette decrease | ||
| newState["palette"] = MAX(state.data["palette"].as<uint8_t>() - 1, 0); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -nP --type=cpp --type=h 'palette\s*(count|size|max|len)|palettes\[' -C 3Repository: MoonModules/MoonLight
Length of output: 1053
🏁 Script executed:
# Also search for any constant definitions or enums related to palettes
rg -nP --type=cpp --type=h 'constexpr|const.*palette|enum.*[Pp]alette' -C 2Repository: MoonModules/MoonLight
Length of output: 3308
🏁 Script executed:
# Search in the D_Infrared.h file specifically for palette-related definitions
grep -n -i 'palette' src/MoonLight/Nodes/Drivers/D_Infrared.h | head -20Repository: MoonModules/MoonLight
Length of output: 801
🏁 Script executed:
# Search for palette.entries definition and size
rg -nP --type=cpp --type=h 'palette\s*\{|entries\s*\[|struct.*[Pp]alette' -C 5Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
# Search for CRGB array definitions that might define palette
rg -nP --type=cpp --type=h 'CRGB.*entries|entries.*CRGB' -C 2Repository: MoonModules/MoonLight
Length of output: 1056
🏁 Script executed:
# Look for any hex or numeric palette definitions
rg -nP --type=cpp --type=h '0x[0-9a-fA-F]{6}.*palette|palette.*\[' -C 2 | head -40Repository: MoonModules/MoonLight
Length of output: 3933
🏁 Script executed:
# Search for Layer or layerP structure definitions
rg -nP --type=cpp --type=h 'struct Layer|class Layer|typedef.*Layer' -C 3Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
# Search for palette structure definition
rg -nP --type=cpp --type=h 'struct.*[Pp]alette|palette\s*\{' -C 5Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
# Look for FastLED related includes and palette definitions
rg -nP --type=cpp --type=h 'CRGBPalette|#include.*palette' -C 2Repository: MoonModules/MoonLight
Length of output: 570
🏁 Script executed:
# Confirm CRGBPalette usage
rg -nP --type=cpp --type=h 'CRGBPalette'Repository: MoonModules/MoonLight
Length of output: 146
🏁 Script executed:
# Check if there are other palette sizes used
rg -nP --type=cpp --type=h 'CRGBPalette[0-9]'Repository: MoonModules/MoonLight
Length of output: 146
Fix the palette upper bound from 11 to 15 to match CRGBPalette16 size.
The codebase uses CRGBPalette16 which has 16 entries with valid indices 0-15. The current hardcoded upper bound of 11 restricts users to only 12 palettes when 16 are available. Replace the hardcoded value with a constant or use sizeof(layerP.palette.entries) / sizeof(CRGB) - 1 to match the actual palette size.
🤖 Prompt for AI Agents
In src/MoonLight/Nodes/Drivers/D_Infrared.h around lines 358 to 361, the upper
bound for palette indices is incorrectly hardcoded as 11; change it to match
CRGBPalette16 (valid indices 0–15) by replacing the 11 with 15 or, better,
compute it dynamically (e.g. sizeof(layerP.palette.entries)/sizeof(CRGB) - 1) so
the clamp reflects the actual palette size; update both the palette-increase and
any similar bounds checks accordingly.
Docs ==== - Add ESPConnect in installation - Add board presets in IO - Add Art-Net In in drivers Back-end ======== - IO: add maxPower for DigOcta and Dig2Go - readPins, only assign if valid(gpio) - Art-Net In: add universe Min/Max
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/MoonLight/Nodes/Drivers/D_ArtnetIn.h (1)
37-38: Control range exceeds variable type capacity.The control allows values up to 65538, but the underlying variables are
uint8_t(max 255). This inconsistency was addressed in the previous comment.
🧹 Nitpick comments (5)
docs/moonbase/inputoutput.md (1)
59-75: Board details expansion is comprehensive.The new QuinLed board images and SE16 v1 section provide clear installation guidance. Specific board selection instructions (esp32-d0 vs. esp32-d0-16mb), flash erase notes, and jumper configuration are actionable and helpful.
Minor style note: Line 66 uses "at the moment"—consider replacing with "currently" for conciseness.
Apply this diff to improve style:
-* On first install, erase flash first (Especially when other firmware like WLED was on it) as MoonLight uses a partition scheme with 3MB of flash (no ota at the moment). +* On first install, erase flash first (Especially when other firmware like WLED was on it) as MoonLight uses a partition scheme with 3MB of flash (currently no OTA support).docs/gettingstarted/installation.md (1)
181-181: Resolve the uncertainty marker for flash offset guidance.When flashing firmware, App0 and App1 are OTA partition slots—the bootloader executes the factory app unless it sees OTA data indicating which OTA image to boot. For direct flashing a non-webflash .bin file, App0 is the standard choice. Consider removing the "(Or App1?)" uncertainty and recommending App0 explicitly to reduce user confusion.
Apply this diff to clarify the guidance:
- * Files ending with .bin, without _webflash (e.g. MoonLight_esp32-d0_0-6-1.bin). Choose recommended offsets App0 (Or App1?). Only use if you previously had MoonLight installed on the device, so the partitions has already been set. These files can be found in [published releases](https://github.com/MoonModules/MoonLight/releases). + * Files ending with .bin, without _webflash (e.g. MoonLight_esp32-d0_0-6-1.bin). Choose recommended offsets App0. Only use if you previously had MoonLight installed on the device, so the partitions has already been set. These files can be found in [published releases](https://github.com/MoonModules/MoonLight/releases).docs/develop/development.md (1)
113-118: Update the alternative flashing example to match the primary version number.Line 113 shows version 0-6-1, but line 118 still references 0-6-0. For consistency and to avoid user confusion, update the alternative example to use the same version number as the primary example.
Apply this diff to align the version numbers:
- * use ./build/release/MoonLight_esp32-s3-devkitc-1-n16r8v_0-6-0.bin and address 0x10000 to flash only the MoonLight partition + * use ./build/release/MoonLight_esp32-s3-devkitc-1-n16r8v_0-6-1.bin and address 0x10000 to flash only the MoonLight partitionsrc/MoonBase/Modules/ModuleIO.h (1)
511-540: Consider extracting GPIO validation to reduce duplication.The GPIO validation pattern is repeated five times. While the current implementation is correct, extracting it into a helper lambda would improve maintainability:
ess->v_ETH_PHY_IRQ = UINT8_MAX; // if ethernet pins change // find the pins needed + auto assignIfValid = [](uint8_t gpio, uint8_t usage, uint8_t& target) { + if (GPIO_IS_VALID_GPIO(gpio)) + target = gpio; + else + EXT_LOGE(MB_TAG, "%d: gpio %d not valid", usage, gpio); + }; + for (JsonObject pinObject : _state.data["pins"].as<JsonArray>()) { uint8_t usage = pinObject["usage"]; uint8_t gpio = pinObject["GPIO"]; - if (usage == pin_SPI_SCK) { - if (GPIO_IS_VALID_GPIO(gpio)) - ess->v_ETH_SPI_SCK = gpio; - else - EXT_LOGE(MB_TAG, "%d: gpio %d not valid", usage, gpio); - } + if (usage == pin_SPI_SCK) + assignIfValid(gpio, usage, ess->v_ETH_SPI_SCK); // ... repeat for other pinssrc/MoonLight/Nodes/Drivers/D_ArtnetIn.h (1)
149-175: DDP handling correctly processes packets and fixed the double-write issue.The byte-order conversions are correct for the DDP protocol (big-endian). The double-write issue from past review comments has been addressed by using the same view-based routing pattern as Art-Net.
However:
- The
pushFlagextracted at line 152 is not used. Consider removing it or using it to batch updates.- The layer bounds checking issue (line 170:
layerP.layers[view - 1]) remains unaddressed, as noted in past review comments.If
pushFlagis intended for future use, add a TODO comment. Otherwise, remove it:DDPHeader* header = (DDPHeader*)packetBuffer; - bool pushFlag = (header->flags & 0x80) != 0; uint8_t dataType = header->dataType;Apply the same bounds check as suggested for Art-Net at line 170.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/develop/development.md(1 hunks)docs/gettingstarted/installation.md(1 hunks)docs/moonbase/inputoutput.md(3 hunks)docs/moonlight/drivers.md(2 hunks)src/MoonBase/Modules/ModuleIO.h(6 hunks)src/MoonLight/Modules/ModuleDrivers.h(1 hunks)src/MoonLight/Modules/ModuleLightsControl.h(6 hunks)src/MoonLight/Nodes/Drivers/D_ArtnetIn.h(3 hunks)src/MoonLight/Nodes/Drivers/D_Infrared.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/MoonLight/Nodes/Drivers/D_Infrared.h
- src/MoonLight/Modules/ModuleDrivers.h
- src/MoonLight/Modules/ModuleLightsControl.h
🧰 Additional context used
🧠 Learnings (2)
📚 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:
docs/develop/development.md
📚 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:
docs/develop/development.md
🪛 LanguageTool
docs/moonbase/inputoutput.md
[style] ~66-~66: For conciseness, consider replacing this expression with an adverb.
Context: ...tition scheme with 3MB of flash (no ota at the moment). * You might need to reset your router...
(AT_THE_MOMENT)
[style] ~68-~68: Using many exclamation marks might seem excessive (in this case: 6 exclamation marks for a text that’s 2517 characters long)
Context: ...he same MCU and no new IP is assigned. !!! Tip "Dig Uno USB" Remove fuse to co...
(EN_EXCESSIVE_EXCLAMATION)
🪛 markdownlint-cli2 (0.18.1)
docs/moonbase/inputoutput.md
62-62: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
⏰ 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 (17)
docs/moonbase/inputoutput.md (2)
14-14: Documentation improvement: "Max Power in Watts" clarity is good.The rename and expanded explanation (10W = 5V * 2A for USB compatibility) provides helpful context for default presets. Clear and actionable.
39-40: New pin usages properly documented.Addition of "Relay LightsOn" and SPI pins (SPI_SCK, SPI_MISO, SPI_MOSI, PHY_CS, PHY_IRQ) aligns with board preset expansions and is clearly linked to Light Control and S3 Ethernet.
docs/develop/development.md (1)
115-117: Helpful guidance improvements in flashing instructions.The addition of the port/file replacement note and baud rate explanation enhance clarity for users following these flashing instructions.
src/MoonBase/Modules/ModuleIO.h (6)
69-70: LGTM! Enum rename and new board addition.The typo fix (Octo→Octa) and new board enum member are correctly placed in the sequence.
111-111: LGTM! UI control value added for new board.The new control value correctly corresponds to the new board enum member.
235-235: Pin index default now aligns with control range.The default value now matches the minimum allowed value (1) defined in the "index" control at line 187. This change shifts pin indexing to 1-based.
324-330: LGTM! QuinLEDDigOctaV2 preset updated with maxPower.The maxPower calculation (10A × 8 outputs = 400W) is correctly documented and the pin assignments are preserved.
331-345: LGTM! New QuinLEDDig2Go board preset added.The pin assignments are appropriate for a USB-powered board (10W max). The I2S, I2C, and other peripheral pins are correctly assigned.
456-459: LGTM! Jumper1 changes now trigger preset rebuild.The logic correctly resets the
moddedflag and triggers a full board preset rebuild, which is necessary since jumper1 affects pin assignments (e.g., SE16V1 board at line 285).src/MoonLight/Nodes/Drivers/D_ArtnetIn.h (4)
41-47: LGTM! Layer indexing now starts at 1.Consistent with the 1-based indexing shift throughout the PR. View 0 remains the physical layer, and virtual layers are numbered starting from 1.
61-68: LGTM! Network connectivity check prevents unnecessary processing.The logic correctly stops UDP listening when both WiFi and Ethernet are offline, properly managing the
initflag to allow restart when connectivity returns.
70-74: LGTM! UDP initialization correctly gated by init flag.The initialization logic prevents redundant
begin()calls and provides useful logging.
115-147: Art-Net handling improved with universe filtering and verification.The new universe filtering logic is correct, and the byte-order swap at line 127 properly converts the big-endian Art-Net length field to little-endian. The startPixel calculation correctly accounts for the universe offset.
However, the layer bounds checking issue flagged in past review comments (line 140:
layerP.layers[view - 1]) remains unaddressed.As noted in the past review, add bounds checking before accessing
layerP.layers[view - 1]:if (view == 0) { memcpy(&layerP.lights.channels[ledIndex * layerP.lights.header.channelsPerLight], &dmxData[i * layerP.lights.header.channelsPerLight], layerP.lights.header.channelsPerLight); - } else { + } else if (view - 1 < layerP.layers.size()) { layerP.layers[view - 1]->setLight(ledIndex, &dmxData[i * layerP.lights.header.channelsPerLight], 0, layerP.lights.header.channelsPerLight); + } else { + EXT_LOGE(ML_TAG, "Invalid layer view %d", view); }Based on past review comments.
docs/moonlight/drivers.md (4)
33-40: LGTM! Documentation table updated with new Art-Net In driver.The table entries accurately reflect the code changes, clearly distinguishing between Art-Net In and Art-Net Out, and listing the correct controls for each.
47-47: LGTM! Max Power documentation updated to reference IO Module.Correctly reflects the move of Max Power configuration to board presets in the IO Module.
69-82: LGTM! Comprehensive Art-Net In documentation added.The section clearly explains all configuration options, the difference between physical and virtual layers, and includes a helpful tip about managing running effects.
83-83: LGTM! Section header clarified.Explicitly naming the section "Art-Net Out" improves documentation clarity and consistency.
| while (int packetSize = artnetUdp.parsePacket()) { | ||
| if (packetSize < sizeof(ArtNetHeader) || packetSize > sizeof(packetBuffer)) { | ||
| artnetUdp.clear(); // drains all available packets | ||
| continue; | ||
| } | ||
|
|
||
| artnetUdp.read(packetBuffer, min(packetSize, (int)sizeof(packetBuffer))); | ||
|
|
||
| if (ddp) | ||
| ddpUdp.begin(port); | ||
| handleDDP(); | ||
| else | ||
| artnetUdp.begin(port); | ||
| EXT_LOGD(ML_TAG, "Listening for %s on port %d", ddp ? "DDP" : "Art-Net", port); | ||
| init = true; | ||
| handleArtNet(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for WiFiUDP class definition and clear() method
find . -type f \( -name "*.h" -o -name "*.cpp" \) | xargs grep -l "WiFiUDP" | head -5Repository: MoonModules/MoonLight
Length of output: 146
🏁 Script executed:
# Also search for Arduino WiFiUDP documentation or local implementation
rg "class WiFiUDP" -A 30Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
# Check if this is using Arduino WiFiUDP library
rg "WiFiUDP" --type h --type cpp | head -20Repository: MoonModules/MoonLight
Length of output: 184
🌐 Web query:
Arduino WiFiUDP clear() method behavior documentation
💡 Result:
Short answer: There is no documented WiFiUDP::clear() method in the Arduino/ESP WiFiUDP APIs. The UDP API exposes parsePacket(), available(), read()/peek(), flush(), stop(), etc. — use flush() to discard unread bytes for the current packet or stop() to close the socket. [1][2]
Sources:
- ESP8266 WiFi library docs (WiFiUDP overview; no clear() listed). [1]
- WiFiUDP API docs (Ameba/Realtek/Arduino variants) showing flush(), parsePacket(), read(), stop(), etc. (flush = "Clear all the bytes ... not yet read"). [2]
🏁 Script executed:
# Check if there are any comments in the file about WiFiUDP behavior
cat -n src/MoonLight/Nodes/Drivers/D_ArtnetIn.h | head -100Repository: MoonModules/MoonLight
Length of output: 3659
Replace artnetUdp.clear() with artnetUdp.flush()—clear() does not exist in the WiFiUDP API.
The WiFiUDP class does not provide a clear() method. The standard Arduino WiFiUDP API provides flush() instead, which discards unread bytes from the current packet. Use flush() at line 78 to skip the invalid packet and continue processing subsequent packets:
while (int packetSize = artnetUdp.parsePacket()) {
if (packetSize < sizeof(ArtNetHeader) || packetSize > sizeof(packetBuffer)) {
- artnetUdp.clear(); // drains all available packets
+ artnetUdp.flush(); // skip this packet's unread bytes
continue;
}🤖 Prompt for AI Agents
In src/MoonLight/Nodes/Drivers/D_ArtnetIn.h around lines 76 to 88, the code
calls artnetUdp.clear() to discard an invalid packet but WiFiUDP does not
implement clear(); replace that call with artnetUdp.flush() to discard unread
bytes from the current packet and then continue processing; ensure the flush()
call is used in the same conditional branch before the continue so subsequent
packets are processed.
Back-End ======== - Ethernet: set pins to int8_t (to allow for -1) - Module - Art-Net-In: universe Min/Max to uint16_t, max 32K
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/framework/EthernetSettingsService.h (1)
115-115: Consider consistent sentinel values for "not configured" pins.Line 115 uses
INT8_MAX(127) as a sentinel for "not configured", while lines 122-123 use-1for "not wired". For consistency and clarity, consider using-1uniformly to indicate disabled/unconfigured pins unless there's a specific reason for the distinction.If the distinction is intentional (e.g., different validation logic for SPI vs PHY pins), consider adding a comment explaining why different sentinel values are used.
Also applies to: 122-123
src/MoonBase/Modules/ModuleIO.h (1)
501-537: GPIO validity checks improve robustness.The addition of
assignIfValidlambda and validity checks before assigning ethernet pins is excellent defensive programming. Invalid GPIOs now log errors instead of causing potential runtime issues. One minor improvement: consider logging the pin usage name (e.g., "SPI_SCK") instead of the numericusageenum value for clearer diagnostics.Apply this diff to improve error message clarity:
- auto assignIfValid = [](uint8_t gpio, uint8_t usage, int8_t& target) { + auto assignIfValid = [](uint8_t gpio, const char* usageName, int8_t& target) { if (GPIO_IS_VALID_GPIO(gpio)) target = gpio; else - EXT_LOGE(MB_TAG, "%d: gpio %d not valid", usage, gpio); + EXT_LOGE(MB_TAG, "%s: gpio %d not valid", usageName, gpio); }; // if ethernet pins change // find the pins needed for (JsonObject pinObject : _state.data["pins"].as<JsonArray>()) { uint8_t usage = pinObject["usage"]; uint8_t gpio = pinObject["GPIO"]; if (usage == pin_SPI_SCK) { - assignIfValid(gpio, usage, ess->v_ETH_SPI_SCK); + assignIfValid(gpio, "SPI_SCK", ess->v_ETH_SPI_SCK); } if (usage == pin_SPI_MISO) { - assignIfValid(gpio, usage, ess->v_ETH_SPI_MISO); + assignIfValid(gpio, "SPI_MISO", ess->v_ETH_SPI_MISO); } if (usage == pin_SPI_MOSI) { - assignIfValid(gpio, usage, ess->v_ETH_SPI_MOSI); + assignIfValid(gpio, "SPI_MOSI", ess->v_ETH_SPI_MOSI); } if (usage == pin_PHY_CS) { - assignIfValid(gpio, usage, ess->v_ETH_PHY_CS); + assignIfValid(gpio, "PHY_CS", ess->v_ETH_PHY_CS); } if (usage == pin_PHY_IRQ) { - assignIfValid(gpio, usage, ess->v_ETH_PHY_IRQ); + assignIfValid(gpio, "PHY_IRQ", ess->v_ETH_PHY_IRQ); } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/develop/development.md(1 hunks)docs/gettingstarted/installation.md(1 hunks)docs/moonbase/inputoutput.md(3 hunks)lib/framework/EthernetSettingsService.h(1 hunks)src/MoonBase/Modules/ModuleIO.h(6 hunks)src/MoonLight/Nodes/Drivers/D_ArtnetIn.h(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/develop/development.md
- src/MoonLight/Nodes/Drivers/D_ArtnetIn.h
- docs/gettingstarted/installation.md
🧰 Additional context used
🧠 Learnings (1)
📚 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:
lib/framework/EthernetSettingsService.h
🪛 LanguageTool
docs/moonbase/inputoutput.md
[style] ~68-~68: Using many exclamation marks might seem excessive (in this case: 6 exclamation marks for a text that’s 2521 characters long)
Context: ...he same MCU and no new IP is assigned. !!! Tip "Dig Uno USB" Remove fuse to co...
(EN_EXCESSIVE_EXCLAMATION)
⏰ 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 (12)
lib/framework/EthernetSettingsService.h (1)
121-123: Type narrowing frominttoint8_tintroduces API inconsistency with ESP32 ETH library.While the values (41, 2, 1, and -1 sentinel) fit within
int8_trange, the ESP32 ETH.begin() API expectsintparameters for pin numbers. Theint8_tvariables will be implicitly promoted tointat the call site (line 91 of EthernetSettingsService.cpp), which works but creates unnecessary type mismatch. For clarity and API alignment, consider reverting these tointto match the downstream ETH library signature, or document why the narrower type is intentional.Likely an incorrect or invalid review comment.
src/MoonBase/Modules/ModuleIO.h (6)
69-70: LGTM: Enum naming correction and new board addition.The rename from
board_QuinLEDDigOctoV2toboard_QuinLEDDigOctaV2fixes a typo to match the actual product name. Since enum values (numeric indices) remain unchanged, this is a safe refactor. The newboard_QuinLEDDig2Goenum entry is properly added in sequence.
111-111: UI string added for new board.The setup definition correctly includes the string label "QuinLED Dig 2Go" corresponding to the new enum entry. This maintains consistency between the enum and UI representation.
235-235: Shift to 1-based pin indexing.Changing the default
pin["index"]from 0 to 1 aligns with the UI control range (line 187:"number", 1, 32) and thePinAssignerlogic which startsargCounterat 1 (line 209). This shift to 1-based indexing is consistent across the module.
324-330: Octa board preset completed with power spec.The addition of
maxPower = 400(8 channels × 10A fuse × 5V = 400W) and the explicit pin assignments complete the Octa V2 preset. The power calculation is appropriate for 5V operation.
456-459: Jumper1 change discards user modifications.When
jumper1is updated, the code unconditionally setsmodded = falseand rebuilds the preset. This behavior will discard any custom pin modifications the user made. Verify this is the intended UX—users might expect jumper changes to preserve their customizations while only updating jumper-affected pins.
331-345: Verify Dig2Go pin assignments against official QuinLED hardware documentation.The new Dig2Go board preset includes pin assignments for button, infrared, LED, relay, I2S audio, I2C, and exposed pins. These GPIO assignments should be validated against the official QuinLED Dig2Go hardware schematic to ensure correctness, especially for bus-critical pins like I2S (pins 19, 4, 18) and I2C (pins 21, 22). Note that the I2S_MCLK pin is currently unassigned (commented with placeholder "xx"), which may also need confirmation.
docs/moonbase/inputoutput.md (5)
14-14: Max Power description clarified.The updated description now explains the default calculation (5V × 2A = 10W) and USB compatibility constraint, making the setting more understandable for users.
39-40: New supported features documented.The addition of "Relay LightsOn" and the S3 Ethernet pins (SPI_SCK, SPI_MISO, SPI_MOSI, PHY_CS, PHY_IRQ) aligns with the code changes in ModuleIO.h and provides users with clear documentation of available pin usages.
59-67: QuinLed installation guidance improved.The addition of board images and specific installer instructions (esp32-d0 for Dig 2Go/Uno/Quad, esp32-d0-16mb for Dig Octa) helps users select the correct firmware variant. The note about erasing flash on first install is important for users migrating from other firmware.
68-70: Static analysis false positive.The LanguageTool warning about excessive exclamation marks is a false positive. The
!!! Tipsyntax is valid MkDocs admonition syntax for creating styled tip boxes, not actual excessive punctuation.
74-75: SE16 board image added.The addition of the SE-16p board image provides visual reference consistent with the QuinLed boards section above.
| int8_t v_ETH_SPI_SCK = INT8_MAX; //42; v_ETH_SPI_SCK is check if configured, see configureNetwork and ModuleIO | ||
| int8_t v_ETH_SPI_MISO = 44; | ||
| int8_t v_ETH_SPI_MOSI = 43; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type=cpp 'v_ETH_SPI_(SCK|MISO|MOSI)\b' -C 3Repository: MoonModules/MoonLight
Length of output: 3863
🏁 Script executed:
rg -n --type=cpp 'assignIfValid' -B 5 -A 10Repository: MoonModules/MoonLight
Length of output: 3003
🏁 Script executed:
sed -n '85,95p' lib/framework/EthernetSettingsService.cppRepository: MoonModules/MoonLight
Length of output: 509
Fix sentinel value check in EthernetSettingsService.cpp line 88: change UINT8_MAX to INT8_MAX.
The type change from uint8_t to int8_t for the SPI pin variables is correct—it enables using INT8_MAX (127) as a "not configured" sentinel while safely accommodating valid ESP32-S3 GPIO values (0-48). However, the sentinel check in EthernetSettingsService.cpp line 88 was not updated:
if (v_ETH_SPI_SCK != UINT8_MAX) { // ❌ UINT8_MAX = 255, but v_ETH_SPI_SCK is int8_t initialized to INT8_MAX = 127This causes the condition to always be true (127 ≠ 255), triggering SPI initialization even when pins are not configured. Change to INT8_MAX to match the sentinel value used in the header and ModuleIO configuration logic.
The type compatibility for assignments is sound—assignIfValid validates GPIO values via GPIO_IS_VALID_GPIO() before converting from uint8_t to int8_t, ensuring only valid pins are assigned.
🤖 Prompt for AI Agents
In lib/framework/EthernetSettingsService.h around lines 115 to 117 and
corresponding EthernetSettingsService.cpp line 88, the sentinel check uses
UINT8_MAX but the SPI pin variables are int8_t initialized to INT8_MAX; update
the check to compare against INT8_MAX so unconfigured pins (127) are correctly
treated as not set. Modify the condition from comparing to UINT8_MAX to INT8_MAX
and ensure any other checks or assignments consistently use INT8_MAX as the "not
configured" sentinel for these int8_t SPI pin members.
Summary by CodeRabbit
New Features
Improvements
Updates
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.