Fix S31 ethernet transmit wedge; count every dropped frame; name the card vendors - #60
Merged
Conversation
An S31 streaming to panel cards degraded with uptime and then wedged entirely, recovering only on a reboot. Two sdkconfig lines fix it: the DMA buffer was smaller than an ethernet frame, and the transmit mutex was off while two tasks shared the interface. Bench: 19 000 refused frames and two wedges in 20 minutes, against a clean hour after. Performance: no tick-path change. The fix costs ~20 KB of internal DMA RAM on the S31 (both rings, unconditional). **Core** - ethSendFailCounts splits send failures by cause. esp_eth_transmit refuses a down link BEFORE touching the MAC (ESP_ERR_INVALID_STATE) and a full ring after (ESP_ERR_NO_MEM); one counter could not tell them apart, which is what made the bug unreadable for hours. Every failure here turned out to be the ring. - ethRestartTx re-runs link negotiation and resets the descriptor rings — the only way back from a wedge, since no ioctl writes the driver's link flag. **Light domain** - PanelCardDriver attempts recovery once per wedge and reports what happened. The wedge check now runs BEFORE the link check: a wedge is defined by sends failing, which is knowable whatever the link claims, and checking the link first made the branch unreachable wherever the platform reports no link. - The status line carries the split: "1000 Mbit - 3432 pkt/s, N lost (x link, y ring)". **Scripts/MoonDeck** - repo_health reads its delta baseline from git rather than the working tree, so repeated runs give identical deltas instead of comparing against the previous run, and the delta covers all pending changes. Carry-forward still reads the working tree — it wants the newest numbers, the delta wants the committed ones. **Tests** - 26 unit tests (was 21): the wedge detection, the once-per-wedge bound, a wedge that survives its restart, and the cause-split counter. **Docs/CI** - lessons.md: the descriptor arithmetic, the mutex race, and the two wrong turns — misreading a rate test as ruling out back-pressure, and one counter serving two faults. - The driver's catalog screenshot, regenerated against the fixed firmware. **Reviews** - 👾 the once-per-wedge bound reset on a merely-healthy link, so a surviving wedge could bounce the interface every ~20 s -> now gated on frames flowing. - 👾 ethRestartTx's return was dropped; a failed start left ethernet stopped and reported as an unplugged cable -> now its own error. - 👾 desktop ethSendFailCounts returned the streak, not a cumulative total -> fixed; the status would have read (0 link, 0 ring) while drops climbed. - 👾 repo_health carry-forward would revert fresh perf numbers to the committed ones -> delta and carry-forward now read from different sources. - 👾 a comment orphaned from its function by this change -> moved back. - 👾 statusBuf_ sized for the wrong line; truncation would cut the ring count -> 96 B. - 👾 "~46 KB" read as a saving when it is a total, and the size bump's own ~20 KB was unstated -> both corrected. - 👾 new behaviour unpinned -> tests added, which is what exposed the unreachable wedge branch above. Bench-isolated, so only what earns its place ships: buffer size and the mutex are both load-bearing (removing the mutex alone returned 3 000 drops in 4 minutes), while a 30-deep ring ran no cleaner than 10 and was reverted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 2
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/PanelCardDriver.h (1)
306-338: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReconcile
droppedwithlinkDown + ringFull; they track different scopes.
dropped(fromframesDroppedTotal_) only counts failures from the per-row send loop. It does not count failures from the brightness frame (lines 213-219) or the sync frame (lines 250-255).platform::ethSendFailCounts()counts every failedethSendRaw()call, including brightness and sync frames.The status line shows both numbers together:
"%u lost (%u link, %u ring)". Because the two counters have different scopes,linkDown + ringFullcan exceeddropped, for example "3 lost (5 link, 2 ring)". This misleads exactly the kind of troubleshooting this diagnostic is built for.Also,
if (dropped)gates the whole per-cause line. When only brightness/sync sends fail during a tick (all row sends succeed),droppedstays 0, so reallinkDown/ringFullincrements from that tick stay invisible in this status line.Count every
ethSendRaw()failure inframesDroppedTotal_, including brightness and sync frames, sodroppedandlinkDown + ringFulldescribe the same population. Alternatively, drop the separatedroppedlocal counter and gate/report directly fromlinkDown + ringFull.🐛 Proposed fix: count brightness/sync failures too
{ const size_t len = buildColorLightBrightnessPacket(packet_, kCardGain); // Sent TWICE, like the sync below. Card firmware v13+ acts on the second copy only; // older firmware ignores the duplicate, so sending both costs one frame and works on // every version rather than making the behaviour depend on a firmware probe we do not do. - if (platform::ethSendRaw(packet_, len)) framesSent_++; - if (platform::ethSendRaw(packet_, len)) framesSent_++; + if (platform::ethSendRaw(packet_, len)) framesSent_++; else framesDroppedTotal_++; + if (platform::ethSendRaw(packet_, len)) framesSent_++; else framesDroppedTotal_++; } ... if (anyRowSent) { const size_t len = buildColorLightSyncPacket(packet_, kCardGain); // Twice, for the same firmware reason as the brightness frame above. - if (platform::ethSendRaw(packet_, len)) framesSent_++; - if (platform::ethSendRaw(packet_, len)) framesSent_++; + if (platform::ethSendRaw(packet_, len)) framesSent_++; else framesDroppedTotal_++; + if (platform::ethSendRaw(packet_, len)) framesSent_++; else framesDroppedTotal_++; }🤖 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/PanelCardDriver.h` around lines 306 - 338, Update the send-failure accounting in the brightness and sync frame paths alongside the per-row send loop so every failed ethSendRaw() increments framesDroppedTotal_. Keep the dropped status gate and "%u lost (%u link, %u ring)" report in the status logic, ensuring dropped and the totals returned by platform::ethSendFailCounts() cover the same failures.
🤖 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 `@moondeck/check/repo_health.py`:
- Around line 221-228: Validate loaded snapshot data and each required section
in load_previous and load_working_tree, accepting only mappings and reporting
malformed JSON or wrong shapes visibly. Preserve the existing snapshot as the
baseline and prevent write and main from overwriting it until valid data is
available; use the working-tree fallback only when Git cannot provide HEAD.
Ensure merge_carry_forward receives mappings, and add regression tests covering
malformed JSON and valid JSON with invalid root or section shapes.
In `@src/platform/esp32/platform_esp32.cpp`:
- Around line 973-983: Update ethRestartTx() to check the return value of
esp_eth_stop() and avoid calling esp_eth_start() when stopping fails, returning
failure immediately. Ensure the stop/start handling has an explicit bounded
worst-case duration suitable for the MM_NONBLOCKING PanelCardDriver::tick1s()
path, and document that bound near ethRestartTx() or its call site.
---
Outside diff comments:
In `@src/light/drivers/PanelCardDriver.h`:
- Around line 306-338: Update the send-failure accounting in the brightness and
sync frame paths alongside the per-row send loop so every failed ethSendRaw()
increments framesDroppedTotal_. Keep the dropped status gate and "%u lost (%u
link, %u ring)" report in the status logic, ensuring dropped and the totals
returned by platform::ethSendFailCounts() cover the same failures.
🪄 Autofix (Beta)
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: 95af0a24-d922-4277-b5eb-a08200736de6
⛔ Files ignored due to path filters (1)
docs/assets/light/drivers/PanelCardDriver.pngis excluded by!**/*.png
📒 Files selected for processing (10)
docs/history/lessons.mddocs/metrics/repo-health.jsondocs/metrics/repo-health.mdesp32/sdkconfig.defaults.esp32s31moondeck/check/repo_health.pysrc/light/drivers/PanelCardDriver.hsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_esp32.cppsrc/platform/platform.htest/unit/light/unit_PanelCardDriver.cpp
Review findings on the panel-card work, plus a test that had been failing in CI
at random since it was written. The panel driver counted only its row frames as
dropped, so its status compared two different sets of frames and the comparison
meant nothing.
Performance: unchanged.
**Light domain**
- PanelCardDriver counts brightness and sync failures too. Only row failures were
counted, so four frames per tick failed invisibly and `dropped` described a
different set of frames than the platform's per-cause totals it sits beside.
**Core**
- ethRestartTx checks esp_eth_stop before starting: a failed stop leaves the
driver in a state we did not establish, and starting on top of that compounds
it. The caller already turns a false return into "restart the device".
- Documented why the stop/start is safe from the 1 Hz tick: register writes plus
an esp_timer stop/start, microseconds rather than the seconds negotiation takes,
and only ever in a wedged state where no frames are going out.
**Scripts/MoonDeck**
- repo_health validates the snapshot shape at load. merge_carry_forward does
`old.get(section, {})`, so a JSON list or scalar would raise and a section
holding a list would corrupt the merge. A rejection is announced rather than
read as "no previous numbers", which would report every metric as new and look
like a clean slate instead of a broken baseline.
**Tests**
- unit_DistortionWavesEffect freezes the clock. The effect's hue is the average of
two time-advanced sines and the test ticks once, so the phase was whatever the
process uptime happened to be — and at 2.28% of start times the two sampled
lights land on the same hue. Measured, and it matches the 2.3% predicted from
the sine pair. It had a ~1-in-44 chance of failing on any run, on any branch,
and the realtime sanitizer runs slowly enough to hit it. Same ClockGuard pattern
BouncingBallsEffect already uses. The other 15 time-driven effect tests were
checked: they assert time-invariant properties, so only this one was exposed.
- 9 Python regression tests for the snapshot validation, including a control that
a valid snapshot passes through untouched.
**Docs/CI**
- README credits: WLED and WLED-MM first (projectMM is born out of WLED, and every
part here is a MoonModule), and FPP as the inspiration for PanelCardDriver.
- README em-dashes removed (26), rewritten rather than substituted.
**Reviews**
- 🐇 brightness/sync failures were not counted -> fixed.
- 🐇 esp_eth_stop's return was ignored -> checked.
- 🐇 repo_health accepted any JSON shape -> validated, announced, and pinned by
tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ewowi
changed the title
Pre-merge review findings on the panel-card work. The recovery path was the weak spot: it blocked the render thread for up to 4 seconds against a comment claiming microseconds, a failed restart hid behind an unplugged-cable warning, and the tests that were supposed to pin the guard could not fail. Performance: no change to the steady-state tick. The recovery blocks up to ~4 s when it fires, which is now stated rather than mis-stated; it fires only in a wedged state where no frames are going out. **Core** - ethRestartTx: clear ethLinkUp_ only AFTER esp_eth_stop succeeds. Clearing first and then failing left transmit permanently refused with no event able to set the flag again, behind a "no ethernet link" warning. - A link-down refusal now increments the link counter. It was silent, so `dropped` and the per-cause totals beside it described different sets of frames. - Documented the real cost of ethRestartTx: esp_eth_start restarts autonegotiation, which polls the PHY with vTaskDelay(100 ms) up to autonego_timeout_ms (4000 by default, and a wedged link is down so it runs to the timeout). The previous comment claimed microseconds and no link poll, which was wrong on both counts. **Light domain** - A failed restart is latched, so the next tick cannot soften it to the link warning. It is the one state a user cannot resolve by plugging a cable back in. - PanelCardDriver's more-info section lists the other receiver-card vendors (NovaStar, ColorLight, Linsn, Mooncell, Huidu, DBstar, Xixun) with links, and invites contributions: a *Packet.h plus a kFormatOptions entry, with the window, correction, chunking and platform seam already shared. It also says what is NOT known — each vendor speaks its own proprietary L2 protocol, and whether another format fits this driver's row-plus-sync model is unverified. **Scripts/MoonDeck** - check_esp32_built asks ninja's dependency database what each firmware actually compiles. PanelCardDriver.h is gated out of the S3 build, so ninja correctly does nothing, the binary's timestamp never moves, and the gate reported it stale forever with no rebuild able to clear it. Any comment-only edit to that header would have blocked every future commit. **Tests** - The once-per-wedge tests were vacuous: the desktop restart clears the streak, so the wedge branch was never re-entered and they passed with the guard deleted. They now rebuild the streak between ticks, and the guard was mutation-tested (deleted -> fails, restored -> passes). - A test for the failed-restart path, reachable via a new setTestEthRestartFails hook. - Two tests for the git baseline, including one proving the delta comes from the commit rather than the working tree. - clearClaims also resets the platform's simulated-failure switches: they are process-global, so a test leaving one set poisoned the next, which is why one case passed alone and failed in sequence. **Docs/CI** - performance.md: re-measured on the fixed firmware, and it now records the DMA finding and its ~20 KB cost. Corrected a claim that ~32 FPS was the driver's ceiling; it is the render's, and a lighter effect mix measures ~56 FPS. - README: WLED-iOS alongside WLED-Android in the Moustachauve credit. - No em-dashes in any line this branch adds. **Reviews** - 👾 ethRestartTx blocks ~4 s on the render thread, comment said microseconds -> measured in the IDF source, comment corrected, trade stated. - 👾 a failed restart masqueraded as an unplugged cable -> ordering fixed, error latched. - 👾 the once-per-wedge tests could not fail -> rewritten and mutation-tested. - 👾 dropped vs the per-cause totals counted different frames -> link-down counted. - 👾 load_previous untested -> two tests with a tmp git repo. - 👾 the restart-failed branch was untestable -> test hook added. - 👾 new prose added em-dashes -> removed from every added line. - 👾 carried-forward flash figures for targets not rebuilt: accepted, that is the run-before-commit shape of the KPI gate rather than a branch defect. Bench: flashed to the S31 and ran past both historical failure points (wedges at 9:04 and 20:23 before the fix). 22+ minutes, zero drops, zero wedges, 4 251 packets/s against a 4 224 baseline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ewowi
changed the title
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
1 participant
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.
What this fixes
An ESP32-S31 streaming panel-card frames degraded with uptime and then wedged completely — every
esp_eth_transmitrefused, only a reboot recovering it. It read as a hardware fault for hours. It was two sdkconfig lines.The arithmetic that caused it.
CONFIG_ETH_DMA_BUFFER_SIZEdefaults to 512 B. A panel-card frame is 1512 B, so every frame consumed three descriptors — a 10-descriptor TX ring held ~3.3 frames while the driver fires 132 back-to-back. 1536 B (64-byte aligned) gives one descriptor per frame.The second half is a race.
CONFIG_ETH_TRANSMIT_MUTEXdefaults off, andmac->transmitadvances a shared descriptor pointer with no locking. With the eth netif up, lwIP and the render task both reach it.Only what earns its place ships. Buffer size and the mutex are both load-bearing — removing the mutex alone brought drops back even with 30 descriptors. A 30-deep ring ran no cleaner than 10, so it was reverted, saving ~30 KB of internal RAM. Net cost: ~20 KB (the size bump applies to both rings, and the RX half buys the panel driver nothing).
The diagnostic that found it
esp_eth_transmitrefuses a down link before touching the MAC (ESP_ERR_INVALID_STATE) and a full ring after (ESP_ERR_NO_MEM). We collapsed both into one bool, so "5.3 million drops" was unreadable — it could have been either fault. Splitting them settled it in one build: every failure wasring, none waslink.The driver's status carries it:
1000 Mbit - 3432 pkt/s, N lost (x link, y ring). Kept deliberately — panel-card tuning continues, and this is what makes it measurable.Recovery
ethRestartTx()(esp_eth_stop/start) re-runs negotiation and resets the descriptor rings — the only way back from a wedge, since no ioctl writes the driver's link flag. Attempted once per wedge, never repeatedly: a restart cannot fix an unplugged cable, and retrying would bounce the interface under a user reading the card. Bench-verified recovering twice, ~17 s each.Still shipped even though the config fix removes the wedges it was written for: it is insurance against a wedge from another cause, and it is now pinned by tests.
Also in this branch
DistortionWavesEffectasserts two distant lights differ, but its hue is the average of two time-advanced sines and the test ticks once, so the phase was whatever the process uptime happened to be. Measured: 2.28% of start times make the two sampled lights identical, matching the 2.3% predicted from the sine pair. That is ~1-in-44 on any CI run, on any branch, and it predates this work — the realtime sanitizer just runs slowly enough to hit it more often. Now freezes the clock, using the patternBouncingBallsEffectalready established. The other 15 time-driven effect tests were checked; they assert time-invariant properties.repo_health.pyreads its delta baseline from git, not the working tree. Repeated runs previously compared against each other, so a second run showed a delta of ~0 and the real change vanished. Deltas now cover all pending changes and are stable across runs. Carry-forward still reads the working tree — it wants the newest numbers, the delta wants the committed ones. Snapshot shape is validated, and a malformed baseline is announced rather than silently read as "no previous numbers".lessons.md— the descriptor arithmetic, the mutex race, and the two wrong turns that cost hours.Other card vendors
ColorLight is one of several receiver-card makers, so the driver is named for the category and
carries a
formatselector. The class documentation now listsNovaStar, ColorLight,
Linsn, Mooncell,
Huidu, DBstar and Xixun, and invites contributions: adding one is a
*Packet.hplus akFormatOptionsentry, since the window, correction, chunking and platform seamare shared, and the desktop capture path pins byte layouts with no hardware.
It also says what is not known: each vendor speaks its own proprietary L2 protocol, so a
ColorLight frame will not drive a NovaStar card, and whether another format fits this driver's
row-plus-sync model is unverified. The shape is an invitation, not a promise.
A gate bug found on the way
check_esp32_builtcompared the S3 binary against every source, including ones that firmwaredoes not compile.
PanelCardDriver.his gated out of the S3 build, so ninja correctly does nothing,the binary's timestamp never moves, and the gate reported it stale forever with no rebuild able to
clear it. Any comment-only edit to that header would have blocked every future commit.
It now asks ninja's own dependency database what each firmware compiles. Verified with two controls
that must fire: the S3 against a header it does compile, and the S31 against
PanelCardDriver.h.Both correctly report stale, so the check got more precise rather than merely quieter.
Reviewed
Three review rounds, 19 findings, all processed — including three real bugs in the recovery path, all in code written during review:
ethRestartTx's return value was dropped. A failedesp_eth_startleft Ethernet stopped forever while the card reported "no ethernet link", indistinguishable from an unplugged cable.droppeddescribed a different set of frames than the(link, ring)totals beside it, making the comparison meaningless.The pre-merge round found three more, all in the same recovery path:
ethRestartTxblocks the render thread for up to ~4 seconds, against a comment of mineclaiming microseconds.
esp_eth_startrestarts autonegotiation, which polls the PHY withvTaskDelay(100 ms)up toautonego_timeout_ms(4000 by default, and a wedged link is down so itruns to the timeout). Verified in the IDF source; the comment now states the real cost and why the
trade is acceptable.
ethLinkUp_was cleared beforeesp_eth_stop, so a failed stop left transmit permanently refused behind a "no ethernet link"warning. Ordering fixed, and the error is latched so the next tick cannot soften it.
branch was never re-entered and they passed with the guard deleted. Rewritten to rebuild the
streak, and the guard was mutation-tested: deleted → fails, restored → passes.
Writing the tests for "new behaviour unpinned" exposed a fourth: the wedge branch was unreachable on desktop, because the link check ran first and
ethLinkUp()is false there. That is why the recovery shipped untested. The fix is an ordering correction with its own justification — a wedge is defined by sends failing, which is knowable whatever the link claims.Verification
Honest limits
performance.mdrecords bench numbers rather than an asserted ceiling.🤖 Generated with Claude Code