Stop a Parlio resize from crashing the board, and tell a new device its own address - #64
Merged
Conversation
Driving a P4 past Parlio's frame limit panicked it with a cache error rather than degrading: the LED driver's completion interrupt lived in flash, and changing a control writes config to flash, which disables the cache. A DMA completion landing in that window fetched the handler from memory that was not reachable. Reported as issue #44 - "freezes above 900 LEDs per pin, needs a restart" - and reproduced on the bench. Performance: desktop 131 us/tick (7633 fps), esp32 2151 us/tick (464 fps). Light domain - PARLIO_TX/RX_ISR_CACHE_SAFE. Our own done-callback was already IRAM-resident; the IDF handler wrapping it was not. Verified by decoding the panic (pxPortGetCoprocArea via rtos_int_enter, with the flash driver mid-operation) and then re-running the exact sequence that crashed: repeated resizes across the limit, uptime climbing throughout. Only the S31 and P4 have Parlio; the option is inert on the classic and S3. - The frame-too-large path returned silently every tick, so the LEDs held their last frame while the UI stayed healthy. It now reports the ceiling in the unit the user sets - lights per pin - and a refused transmit counts as a dead frame, which the give-up detector was previously blind to (it only counted timeouts). Scripts/MoonDeck - lattice.mlv: three nested loops placing a 3D grid, from issue #37 - the z axis was always available to a layout, the shipped ones just pass 0. Bench-verified at 240 lights, 16x3x5. Three loops exceed Xtensa's registers, so it runs on P4/S31/desktop but not the S3; the script says so. Core - MoonLiveBuiltins_light.h used kArg3 without including the header that defines it - it compiled only because every consumer includes MoonLiveIr.h first. Docs/CI - The boot log told a device user to open http://localhost, which is their own machine. It now prints the interface address, and the AP line carries the address to open (issue #61). hostIp() is empty on ESP32 by design, so only the wrong line ever showed. - performance.md carried two errors in one cell: Parlio's cap is 65535 bytes per TRANSFER, not per lane, and an over-limit frame did not "fail with a loud status". Now the real ceilings: 898 lights/lane at 8 lanes RGB, 673 RGBW, halving at 16 lanes. - Backlog: a scripted layout loses its control values across a reboot. Persistence saves them; they load before the script compiles, so there is nothing to receive them yet. Compiling inside defineControls is NOT the fix - it re-seeds every control on a source change - and the entry records that so nobody retries it. Tests - Elapsed time survives a call: `t` is an argument register, so a callee may clobber it under the ABI. Pinned by running a script that calls before reading it, and checked by removing the register save and watching the test fail. Reviews - 🐇 CodeRabbit: the compile(source, table, sysvars) API bullet corrected, and the t-liveness test above added. Skipped the process-global random16/print-budget findings (a torn LCG read returns a different random number, which is the contract) and the print-queue request, which is already backlogged with that design. Flash: esp32 1762368, esp32s3-n16r8 1753264, esp32s31 2026016, esp32p4-eth 1604192, desktop 1137864. Tests: 1325 cases. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds a 3D MoonLive lattice layout and runtime coverage, improves Parlio capacity reporting and cache-safe operation, updates network startup messages, expands documentation, and refreshes repository health metrics. ChangesMoonLive runtime and layout
Parlio transfer handling
Network startup logging
Repository health records
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/performance.md`:
- Line 233: The performance documentation paragraph uses the outdated
executable-memory API name. Update the statement referencing
setDynamicBytes(codeLen()) to use the current binding,
setDynamicBytes(engine_.heapBytes()), while preserving the surrounding
explanation of dynamic memory and UI reporting.
- Line 235: Update the system-variable cost statement in the documentation to
distinguish binding layouts: effects refresh width/height/depth with three byte
stores per tick, modifiers also refresh x/y/z for six total, and layouts without
these variables should be described accordingly. Keep t identified as an
argument register with no additional store cost and retain the cold compile-path
note.
- Line 251: Update the Parlio performance row to clarify that the ~30,100 µs and
30 fps figure at 16,384 lights represents an over-limit refusal/dead-frame
measurement, not a successful transmission; alternatively replace it with
measurements from a within-limit light count. Keep the documented
898-lights-per-lane RGB ceiling and current driver behavior accurate.
In `@esp32/sdkconfig.defaults`:
- Around line 93-101: Update the ParlioState allocation and its semaphore
storage to use internal-RAM-capable allocation rather than ordinary new or
external-capable FreeRTOS allocation. Ensure the context used by parlioDoneCb
and the semaphore accessed by the cache-safe Parlio ISR are both placed in
internal RAM, while preserving their existing ownership and lifecycle behavior.
In `@moonlive/layouts/lattice.mlv`:
- Around line 1-15: Add or verify a regression test in the layout tests around
the existing lattice coverage in unit_MoonLiveLayout.cpp. Execute the lattice
script with dimensions 4×3×5 and assert it emits 60 coordinates, with x varying
fastest, y next, and five z-layers in order; preserve the expected addLight
coordinate sequence for this exact script.
In `@src/light/drivers/ParallelLedDriver.h`:
- Around line 735-736: Update tickRing() to treat a false return from
busTransmitRing() as a dead frame by incrementing deadFrames_ using the existing
bound, ensuring repeated refusals eventually trigger busGaveUp() instead of
retrying indefinitely on each render tick.
In `@src/main.cpp`:
- Around line 584-586: Update the HTTP server startup message in the surrounding
main flow to avoid claiming the network module will later report an address when
mm::platform::hostIp() is empty on desktop. Check the host-address availability
and print an “address unavailable” message for that case, while preserving the
existing address-reporting message when a usable address exists.
In `@test/scenarios/light/scenario_MoonLive_pipeline.json`:
- Line 131: Remove the timing-dependent assertion at
test/scenarios/light/scenario_MoonLive_pipeline.json lines 131-131 and replace
it with deterministic assertions, or move timing validation to benchmark-only
coverage. Update docs/performance.md lines 237-237 to document the
12-to-18-microsecond budget change and its rationale, or remove the statement
that existing scenario budgets are unchanged.
In `@test/unit/core/unit_moonlive_fill.cpp`:
- Around line 175-182: Update the test case “elapsed time survives a call that
happens before it is read” to avoid relying on evaluation order between setRGB
arguments: split the random16 call and the mod(t, 200) read into separate
statements while preserving the assertion that t retains the host-provided
elapsed value. Keep the test focused on verifying t survives the intervening
call.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6e7f4d83-638b-45f8-b4de-c8f5cb3b5a46
📒 Files selected for processing (14)
docs/backlog/backlog-light.mddocs/metrics/repo-health.jsondocs/metrics/repo-health.mddocs/moonmodules/light/MoonLiveEffect.mddocs/performance.mdesp32/sdkconfig.defaultsmoonlive/layouts/lattice.mlvsrc/core/NetworkModule.hsrc/light/drivers/ParallelLedDriver.hsrc/light/moonlive/MoonLiveLayout.hsrc/main.cpptest/scenarios/light/scenario_MoonLive_pipeline.jsontest/scenarios/light/scenario_perf_full.jsontest/unit/core/unit_moonlive_fill.cpp
| | Peripheral | Board | Pins used (8 lanes) | Result | Ceiling / bound | | ||
| |---|---|---|---|---| | ||
| | **Parlio** | ESP32-P4 (Waveshare P4-NANO) | `20,21,22,23,24,25,26,27` | `Drivers` tick ~30100 µs, fps 30 at 16384 lights (8 lanes, SWAR transpose) | 65535 bytes/lane single-shot = **897 RGB lights/lane**; an over-limit frame fails with a loud status | | ||
| | **Parlio** | ESP32-P4 (Waveshare P4-NANO) | `20,21,22,23,24,25,26,27` | `Drivers` tick ~30100 µs, fps 30 at 16384 lights (8 lanes, SWAR transpose) | Parlio's single-shot transfer caps at 65535 bytes TOTAL (not per lane), and a light costs `channels × 24 × slotBytes` — so the ceiling is **898 lights/lane at 8 lanes RGB**, 673 RGBW, and halves to 443/332 at 16 lanes (a 16-bit bus doubles `slotBytes`). Over that, the driver reports `too many lights per pin` and keeps running; lifting the ceiling is the [chunked-DMA work](backlog/backlog-light.md) (tier 1 → ~16-21K). | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clarify the over-limit Parlio result.
At 16,384 lights across 8 lanes, the row contains 2,048 lights per lane. This exceeds the stated RGB ceiling of 898 lights per lane. Do not present the 30,100 microsecond value as a successful 16,384-light transmission. Label it as an over-limit refusal/dead-frame measurement, or replace it with a within-limit result.
As per coding guidelines: “Documentation must describe the system as it currently exists.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/performance.md` at line 251, Update the Parlio performance row to
clarify that the ~30,100 µs and 30 fps figure at 16,384 lights represents an
over-limit refusal/dead-frame measurement, not a successful transmission;
alternatively replace it with measurements from a within-limit light count. Keep
the documented 898-lights-per-lane RGB ceiling and current driver behavior
accurate.
Source: Coding guidelines
Comment on lines
+1
to
+15
| // A 3D lattice: stacked layers of a grid, the primitive 3D space of LED strips. | ||
| // `z` is an ordinary axis to a layout -- the shipped 2D layouts simply pass 0 for it. | ||
| // Three nested loops need more registers than Xtensa has, so this runs on P4/S31/desktop | ||
| // but not the S3; two loops (grid.mlv) fit everywhere. | ||
| uint8_t cols = 4; // @control 1..32 | ||
| uint8_t rows = 3; // @control 1..32 | ||
| uint8_t layers = 5; // @control 1..32 | ||
|
|
||
| for (z = 0; z < layers; z = z + 1) { | ||
| for (y = 0; y < rows; y = y + 1) { | ||
| for (x = 0; x < cols; x = x + 1) { | ||
| addLight(x, y, z); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add or verify a 3D coordinate regression test.
This script must emit 60 coordinates for 4 × 3 × 5, with five z-layers and x varying fastest. Use test/unit/light/unit_MoonLiveLayout.cpp, Lines 38-52, to verify the count, coordinates, and ordering for this exact script.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@moonlive/layouts/lattice.mlv` around lines 1 - 15, Add or verify a regression
test in the layout tests around the existing lattice coverage in
unit_MoonLiveLayout.cpp. Execute the lattice script with dimensions 4×3×5 and
assert it emits 60 coordinates, with x varying fastest, y next, and five
z-layers in order; preserve the expected addLight coordinate sequence for this
exact script.
| "tick_us": [ | ||
| 5, | ||
| 12 | ||
| 18 |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Keep the performance contract and its documentation consistent.
The scenario now accepts 18 microseconds instead of 12, while the performance document says that existing scenario budgets were unchanged.
test/scenarios/light/scenario_MoonLive_pipeline.json#L131: Replace the timing gate with deterministic assertions, or move it to benchmark validation.docs/performance.md#L237: Record the changed 12-to-18-microsecond budget and its reason, or remove the unchanged-budget statement.
As per path instructions: “Tests should not depend on timing or network.” As per coding guidelines: “Documentation must describe the system as it currently exists.”
📍 Affects 2 files
test/scenarios/light/scenario_MoonLive_pipeline.json#L131-L131(this comment)docs/performance.md#L237-L237
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/scenarios/light/scenario_MoonLive_pipeline.json` at line 131, Remove the
timing-dependent assertion at
test/scenarios/light/scenario_MoonLive_pipeline.json lines 131-131 and replace
it with deterministic assertions, or move timing validation to benchmark-only
coverage. Update docs/performance.md lines 237-237 to document the
12-to-18-microsecond budget change and its rationale, or remove the statement
that existing scenario budgets are unchanged.
Sources: Coding guidelines, Path instructions
…each Making the Parlio interrupt cache-safe was only half the job. An ISR that runs with the flash cache disabled cannot read PSRAM, so everything it touches has to be internal - and two things still were not: the IDF driver's own transaction queues, and the alignment of the state struct our callback walks. Performance: desktop 128 us/tick (7812 fps), esp32 2151 us/tick (464 fps). Light domain - CONFIG_PARLIO_OBJ_CACHE_SAFE. The TX option places the HANDLER in IRAM but, unlike its RX twin, does not select it - so IDF's transaction queues kept MALLOC_CAP_DEFAULT and stayed PSRAM-eligible, and the same ISR dereferences them the moment our callback returns. An IRAM handler reading a PSRAM queue is the identical fault, one frame later. Found by review, confirmed in IDF's Kconfig. - ParlioState is allocated with heap_caps_aligned_alloc, not heap_caps_malloc. It holds 64-bit timestamps so it needs 8-byte alignment, and heap_caps_malloc promises only a word - a static_assert written to test that assumption failed, which is how this was caught rather than by reasoning about it. The assert stays, so a member needing more alignment fails loudly instead of landing askew. - tickRing's transmit had no failure branch, so a refused ring frame never counted toward the give-up detector. All three transmit paths now agree. Core - The boot log promised that "the network module reports the address" even on a desktop with no route, where nothing ever would. It states what is true instead. Tests - The elapsed-time test relied on argument evaluation order within one call. It happens to be left-to-right, but the test should not depend on that: two statements now, so the ordering is the language's. Re-checked by removing the register save and watching it fail. Docs/CI - performance.md said setDynamicBytes(codeLen()); the code reports heapBytes(), which also counts the control arena. And "three byte stores per tick" was the effect's view alone - a modifier writes six, on the cold path, and a layout writes none. Reviews - 👾 Reviewer on the allocation change: confirmed lifetime and leak-freedom on every path, vSemaphoreDelete on a static semaphore (a no-op, correct), the PSRAM DMA buffers (the ISR never touches them), and MALLOC_CAP_INTERNAL as the idiomatic construct. Its one finding is the OBJ_CACHE_SAFE gap above. - 🐇 CodeRabbit: 6 fixed, 2 skipped. The scenario "budget change" is inside an observed block, not a contract - that file asserts nothing, so there is no timing-dependent assertion to remove. A lattice coordinate-order test is reasonable but is new coverage, not a fix to this diff. Verified on the P4: Parlio drives through four resize cycles across the frame ceiling with uptime climbing throughout. Both Parlio-capable chips (S31, P4) rebuilt with the config. Flash: esp32 1762368, esp32s3-n16r8 1753232, esp32s31 2025600, esp32p4-eth 1604208, desktop 1137864. Tests: 1325 cases. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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/light/drivers/ParallelLedDriver.h (1)
582-584: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftReport the Parlio limit before bus initialization.
When a Parlio frame exceeds its transfer limit,
parlioWs2812Init()returnsfalse.reinit()then leavesinited_false, andtick()returns at Line 564. Lines 582-584 never callreportOverCapacity()for the rejected frame.Expose a backend maximum-transfer capacity and perform this check before
busInit(). Add a test that verifies the actionableledsPerPinstatus for a frame above the Parlio limit.Also applies to: 785-801
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/light/drivers/ParallelLedDriver.h` around lines 582 - 584, Expose the Parlio backend’s maximum transfer capacity and validate frameBytes_ against it in reinit() before busInit(), so oversized frames call reportOverCapacity(outCh) before parlioWs2812Init() can leave inited_ false. Apply the same pre-initialization handling to the related path around the second reported location, and add a test confirming an above-limit frame reports the actionable ledsPerPin status.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/performance.md`:
- Line 235: The performance documentation repeats the claim that t costs nothing
and overstates its costlessness. In the paragraph containing the duplicated “t
costs nothing at all” sentence, remove the duplicate and clarify that t adds no
arena byte store, while noting that a backend may still preserve it across
calls.
---
Outside diff comments:
In `@src/light/drivers/ParallelLedDriver.h`:
- Around line 582-584: Expose the Parlio backend’s maximum transfer capacity and
validate frameBytes_ against it in reinit() before busInit(), so oversized
frames call reportOverCapacity(outCh) before parlioWs2812Init() can leave
inited_ false. Apply the same pre-initialization handling to the related path
around the second reported location, and add a test confirming an above-limit
frame reports the actionable ledsPerPin status.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6b36081b-3171-4258-b2ec-31e6b6e62043
📒 Files selected for processing (8)
docs/metrics/repo-health.jsondocs/metrics/repo-health.mddocs/performance.mdesp32/sdkconfig.defaultssrc/light/drivers/ParallelLedDriver.hsrc/main.cppsrc/platform/esp32/platform_esp32_parlio.cpptest/unit/core/unit_moonlive_fill.cpp
A frame too large for Parlio's single transfer used to be discovered inside busInit, which failed and left the driver idle. The driver already had a seam for exactly this - the classic i80 declares a DMA budget that reinit() checks first - and Parlio simply never opted in. One override, and the refusal now happens with an actionable message instead of a failed init. Performance: desktop 129 us/tick (7751 fps), esp32 2151 us/tick (464 fps). Light domain - ParlioLedDriver declares dmaBudgetBytes() = 65535, the peripheral's hard single-transfer cap. reinit() checks it before busInit, so an over-limit configuration reports "frame 72KB over the bus 63KB: fewer lights/pin" and the device keeps running. - That status is now shared by two peripherals, so it no longer names i80. GCC caught the first wording overflowing the 64-byte fail buffer, which is the -Werror gate doing its job. Docs/CI - performance.md said `t` costs nothing. It adds no arena byte, but a callee may clobber an argument register under the ABI, so a backend preserves it across calls - the arm64 one stacks x3, and unit_moonlive_fill pins that a script reading `t` after a call still sees the host's value. Stated that way now. Reviews - 🐇 CodeRabbit, 2 findings: 1 fixed (the pre-init capacity check above), 1 partly - there is no duplicated sentence about `t` (one line, matched twice), but the claim that it is free was wrong and is corrected. Verified on the P4: 4096 lights over 4 lanes (1024/lane) refuses with the message above; spreading to 8 lanes drives all 4096; back to 4 refuses again. Uptime climbs throughout, so the status is not sticky and nothing wedges. All nine shipping ESP32 variants build - the ESP32 matrix only runs on main, so a break in a variant would otherwise surface after merge. Flash: esp32 1762368, esp32s3-n16r8 1753232, esp32s31 2025600, esp32p4-eth 1604272, desktop 1137928. Tests: 1325 cases. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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/light/drivers/ParallelLedDriver.h (1)
785-802: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReport the supported lights-per-pin limit before bus initialization.
reportOverCapacity()calculates the limit fromperipheral_->busCapacity(), but the DMA-budget branch runs afterdeinit()and beforebusInit(). That branch therefore emits only"frame %uKB over the bus %uKB: fewer lights/pin"and does not report the calculatedledsPerPinlimit required by Issue#44. The KB conversion can also make near-limit values display the same number.Pass the applicable DMA budget into the over-capacity calculation and report the exact supported lights-per-pin value on this path.
Also applies to: 1866-1869
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/light/drivers/ParallelLedDriver.h` around lines 785 - 802, Update reportOverCapacity() to accept the applicable DMA/bus budget as an argument and calculate the supported lights-per-pin limit from that value instead of peripheral_->busCapacity(). In the DMA-budget branch after deinit() and before busInit(), pass the computed budget to reportOverCapacity() so it emits the exact ledsPerPin limit rather than the existing KB-only message; preserve normal reporting for other paths by passing the peripheral capacity there.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/light/drivers/ParlioLedDriver.h`:
- Around line 79-83: Move the Parlio transfer limit out of
ParlioLedDriver::dmaBudgetBytes() and expose it from platform_esp32_parlio.cpp
through a platform accessor or constant derived from
PARLIO_LL_TX_MAX_BITS_PER_FRAME. Update dmaBudgetBytes() to use that
platform-provided value, preserving the byte-based limit and target-specific
behavior when SOC_PARLIO_SUPPORTED is enabled.
---
Outside diff comments:
In `@src/light/drivers/ParallelLedDriver.h`:
- Around line 785-802: Update reportOverCapacity() to accept the applicable
DMA/bus budget as an argument and calculate the supported lights-per-pin limit
from that value instead of peripheral_->busCapacity(). In the DMA-budget branch
after deinit() and before busInit(), pass the computed budget to
reportOverCapacity() so it emits the exact ledsPerPin limit rather than the
existing KB-only message; preserve normal reporting for other paths by passing
the peripheral capacity there.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3f42e1fb-5239-485a-8e4e-0150379c322e
📒 Files selected for processing (5)
docs/metrics/repo-health.jsondocs/metrics/repo-health.mddocs/performance.mdsrc/light/drivers/ParallelLedDriver.hsrc/light/drivers/ParlioLedDriver.h
Comment on lines
+79
to
+83
| // Parlio sends a frame in ONE transfer, and the peripheral caps that at 65535 bytes — a hard | ||
| // limit, unlike a memory budget that varies with the heap. Declaring it here lets reinit() | ||
| // refuse an oversized frame with an actionable status BEFORE busInit tries (and fails) to | ||
| // allocate it, which is what left the LEDs frozen with a healthy UI (issue #44). | ||
| size_t dmaBudgetBytes() const override { return 65535; } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -n -C 8 'PARLIO_LL_TX_MAX_BITS_PER_FRAME|parlioWs2812Init|dmaBudgetBytes' \
src/platform/esp32/platform_esp32_parlio.cpp \
src/light/drivers/ParlioLedDriver.h \
test/unit/light/unit_ParlioLedDriver.cppRepository: MoonModules/projectMM
Length of output: 10709
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- platform API declarations and implementation ---'
rg -n -C 6 'ParlioWs2812|parlioWs2812|kParlioMaxTransferBytes|PARLIO_LL_TX_MAX_BITS_PER_FRAME|SOC_PARLIO_SUPPORTED' \
src/platform src/light test/unit/light
printf '%s\n' '--- target and platform-selection references ---'
rg -n -C 4 'ParlioPeripheral|ParlioLedDriver|platform_esp32_parlio|SOC_PARLIO_SUPPORTED' \
. -g '!build' -g '!dist' -g '!node_modules'Repository: MoonModules/projectMM
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- dmaBudgetBytes contract and all implementations ---'
rg -n -C 10 'dmaBudgetBytes' src test -g '*.{h,hpp,c,cc,cpp}'
printf '%s\n' '--- driver construction and platform gating ---'
sed -n '1,135p' src/light/drivers/ParlioLedDriver.h
rg -n -C 8 'ParlioLedDriver|parlioLanes|dmaBudgetBytes\(\)|if constexpr' \
src/light src/platform test/unit/light -g '*.{h,hpp,c,cc,cpp}'
printf '%s\n' '--- exact platform constant and SDK macro references ---'
rg -n 'PARLIO_LL_TX_MAX_BITS_PER_FRAME|0x7FFFF|65535' . \
-g '*.{h,hpp,c,cc,cpp,ino}' -g '!build' -g '!dist' -g '!node_modules' \
| head -120Repository: MoonModules/projectMM
Length of output: 50378
🌐 Web query:
Espressif ESP-IDF PARLIO_LL_TX_MAX_BITS_PER_FRAME SOC_PARLIO_SUPPORTED target value
💡 Result:
In the Espressif ESP-IDF framework, SOC_PARLIO_SUPPORTED is a configuration macro defined in the soc_caps.h file for specific chips to indicate that the Parallel IO (PARLIO) peripheral is supported on that target [1][2][3][4]. When this macro is set to 1, the PARLIO driver features are enabled for the target [1][5]. PARLIO_LL_TX_MAX_BITS_PER_FRAME is a hardware abstraction layer (HAL) constant that defines the maximum number of bits per frame for the Parallel IO TX unit. In specific implementations (such as for the ESP32-C5), this value is defined as 0x7FFFF [6]. The ESP-IDF driver uses this constant to perform validation checks on transfer sizes. For example, when not using loop transmission, the driver verifies that the configured maximum transfer bits do not exceed PARLIO_LL_TX_MAX_BITS_PER_FRAME to ensure the hardware transfer limits are respected [7][8][9]. Key points regarding these values include: - SOC_PARLIO_SUPPORTED: Indicates hardware support for the PARLIO peripheral on a given chip [1][2]. - PARLIO_LL_TX_MAX_BITS_PER_FRAME: Represents the upper limit for transmission frame size in bits (e.g., 0x7FFFF) [6]. - Driver Validation: The ESP-IDF PARLIO driver uses these constants to enforce parameter limits, such as checking that requested transfer sizes are within valid ranges [10][7]. For information regarding a specific SoC, you should refer to the soc_caps.h file located in components/soc//include/soc/ within your ESP-IDF installation [2][3][4].
Citations:
- 1: https://github.com/espressif/esp-idf/blob/v6.0.2/components/esp_hal_parlio/include/hal/parlio_types.h
- 2: https://github.com/espressif/esp-idf/blob/v5.2.6/components/soc/esp32p4/include/soc/soc_caps.h
- 3: https://github.com/espressif/esp-idf/blob/12f36a02/components/soc/esp32c5/include/soc/soc_caps.h
- 4: https://github.com/espressif/esp-idf/blob/47faecc3/components/soc/esp32c6/include/soc/soc_caps.h
- 5: https://github.com/espressif/esp-idf/blob/7e3df61a/components/soc/esp32c5/include/soc/Kconfig.soc_caps.in
- 6: https://github.com/espressif/esp-idf/blob/master/components/esp_hal_parlio/esp32c5/include/hal/parlio_ll.h
- 7: https://code.brunner.ninja/espressif/esp-idf/commit/65074afc0d82094a58cdaeaebf01d908de34725c.patch
- 8: https://code.brunner.ninja/espressif/esp-idf/commit/8e591a2128294ee4f104183463b7bd722bc50e36.diff
- 9: https://code.brunner.ninja/espressif/esp-idf/commit/8e591a2128294ee4f104183463b7bd722bc50e36
- 10: espressif/esp-idf@3a9d082
Move the Parlio transfer limit into the platform layer.
ParlioLedDriver.h hard-codes 65535, while platform_esp32_parlio.cpp owns the transfer check. Add a platform accessor or constant derived from PARLIO_LL_TX_MAX_BITS_PER_FRAME, and use it for dmaBudgetBytes(). This keeps the limit correct for each target that enables SOC_PARLIO_SUPPORTED.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/light/drivers/ParlioLedDriver.h` around lines 79 - 83, Move the Parlio
transfer limit out of ParlioLedDriver::dmaBudgetBytes() and expose it from
platform_esp32_parlio.cpp through a platform accessor or constant derived from
PARLIO_LL_TX_MAX_BITS_PER_FRAME. Update dmaBudgetBytes() to use that
platform-provided value, preserving the byte-based limit and target-specific
behavior when SOC_PARLIO_SUPPORTED is enabled.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two reported bugs, both reproduced on the bench and fixed, plus the scripted 3D layout a third issue asked for.
Closes #44. Closes #61.
Partly addresses #37 — the scripted 3D layout lands here, but the z-position on
SingleRowLayout/SingleColumnLayoutdoes not, so that issue stays open for theLineLayoutconsolidation.The P4 crash (#44)
Driving Parlio past its frame limit panicked the board rather than degrading:
Decoding it pointed away from the LED driver entirely: the fault is inside FreeRTOS's interrupt entry with the flash driver mid-operation. The IDF Parlio completion ISR lives in flash; changing a control writes config to flash, which disables the cache, and a DMA completion landing in that window fetches the handler from memory that isn't reachable. Our own done-callback was already
IRAM_ATTR— the IDF handler wrapping it was not.Two
sdkconfiglines (PARLIO_TX/RX_ISR_CACHE_SAFE). Verified by re-running the exact sequence that crashed — repeated resizes across the limit — with uptime climbing throughout instead of resetting. Only the S31 and P4 have Parlio; the option is inert on the classic and S3.That also explains the reporter's "the web UI continues as if nothing went wrong": the crash is tied to reconfiguration, not to rendering.
The ceiling behind it, now reported
Parlio sends one frame per DMA transfer, capped at 65535 bytes, so a light costing
channels × 24 × slotBytesputs the limit at 898 lights/lane (8 lanes RGB), 673 RGBW, halving at 16 lanes. Bench-confirmed: 8 lanes × 512/lane drives fine, 4 lanes × 1024/lane does not.That path used to
returnsilently every tick — LEDs frozen on the last frame, UI healthy, no diagnostic. It now names the ceiling in the unit the user actually sets (lights per pin), and a refused transmit counts as a dead frame, which the existing give-up detector was blind to (it only counted timeouts).performance.mdwas wrong here in two ways and is corrected: the cap is per transfer, not per lane, and an over-limit frame did not "fail with a loud status".The boot log lied (#61)
A new device printed
HTTP server → http://localhost:80— which is the user's own machine, not the board.hostIp()is empty on ESP32 by design (the address belongs to NetworkModule, and no interface is up that early), so the follow-up line that would have printed the real address never appeared. Now it prints the interface address when known, and the AP line carries the address to open:A 3D scripted layout (#37)
lattice.mlv— three nested loops placing a 3D grid. The z axis was always available to a layout; the shipped ones simply pass 0. Bench-verified at240 lights · 16×3×5.Three nested loops exceed Xtensa's register budget, so it runs on P4/S31/desktop but not the S3. The script says so in its own comment; lifting that is the spilling work, on its own branch.
Also
MoonLiveBuiltins_light.husedkArg3without including the header defining it — it compiled only because every consumer includesMoonLiveIr.hfirst.t(an argument register a callee may legally clobber) survives a call, checked by removing the register save and watching it fail.defineControls) re-seeds every control on a source change; the entry records that so nobody retries it.Verification
1325 unit tests, 20 scenarios inside their existing contracts, GCC build clean. Bench: P4 (Parlio + LCD-MM + LCD-IDF compared at identical load), S31 and S3 firmware rebuilt with the config change.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests & Metrics