Improve frame pacing and RGBW LED output#45
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
Funkelfetisch
changed the title
![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/platform/esp32/platform_esp32_rmt.cpp (1)
189-199: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDon't disable a timed-out RMT transfer
MatchrmtWs2812Wait()here: only callrmt_disable()ifrmt_tx_wait_all_done()succeeds. If the wait times out, the transfer may still be active and disabling it can still trip the classic-ESP32 panic; leaking the channel on teardown is safer than forcing the disable.🤖 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/platform/esp32/platform_esp32_rmt.cpp` around lines 189 - 199, Update rmtWs2812Deinit to check the return value of rmt_tx_wait_all_done before disabling the channel. Only set busy false and call rmt_disable/rmt_del_channel after a successful wait; when the wait times out, leave the active channel allocated rather than forcing teardown, matching rmtWs2812Wait behavior.
🤖 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/core/FileManagerModule.cpp`:
- Around line 23-26: Move the filesystem usage calls from onBuildControls() into
the existing throttled refresh path, using cached totalBytes_ and usedBytes_
when rebuilding controls. Keep onBuildControls() limited to rebinding control
descriptors, and ensure the usage gauge is refreshed from the cached or
periodically updated values without triggering a LittleFS scan on control
writes.
In `@src/core/Scheduler.cpp`:
- Around line 65-82: Refactor Scheduler::tick() so frame-rate limiting never
blocks the shared scheduler loop: remove the delayMs()/yield() wait loop and use
an elapsed-time gate that skips rendering until the next frame deadline while
still allowing loop20ms(), loop1s(), HTTP, and network work to run. Preserve
targetFps_ pacing and update lastFrameStartUs_ only when a frame is actually
processed.
---
Outside diff comments:
In `@src/platform/esp32/platform_esp32_rmt.cpp`:
- Around line 189-199: Update rmtWs2812Deinit to check the return value of
rmt_tx_wait_all_done before disabling the channel. Only set busy false and call
rmt_disable/rmt_del_channel after a successful wait; when the wait times out,
leave the active channel allocated rather than forcing teardown, matching
rmtWs2812Wait behavior.
🪄 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
Run ID: b16d3a37-5d2f-400e-b0b1-f775bf50a12d
📒 Files selected for processing (18)
docs/moonmodules/light/drivers.mdsrc/core/FileManagerModule.cppsrc/core/FileManagerModule.hsrc/core/Scheduler.cppsrc/core/Scheduler.hsrc/core/SystemModule.hsrc/light/drivers/Correction.hsrc/light/drivers/Drivers.hsrc/light/drivers/HueDriver.hsrc/light/drivers/LedDriverConfig.hsrc/light/drivers/NetworkSendDriver.hsrc/light/drivers/ParallelLedDriver.hsrc/light/drivers/RmtLedDriver.hsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_esp32_rmt.cppsrc/platform/platform.htest/unit/light/unit_Correction.cpptest/unit/light/unit_RmtLedEncoder.cpp
| // reports 0). loop1s refreshes the used value sparingly; the total is fixed. | ||
| totalBytes_ = static_cast<uint32_t>(platform::filesystemTotal()); | ||
| usedBytes_ = static_cast<uint32_t>(platform::filesystemUsed()); | ||
| lastUsageRefreshMs_ = platform::millis(); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep run --pattern 'void rebuildControls() { $$$ }' --lang cpp src/core
rg -n -A5 'void rebuildControls' src/coreRepository: MoonModules/projectMM
Length of output: 474
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the module and the throttled refresh path.
ast-grep outline src/core/FileManagerModule.cpp --view expanded
printf '\n--- FileManagerModule.cpp (relevant ranges) ---\n'
sed -n '1,140p' src/core/FileManagerModule.cpp
printf '\n--- MoonModule.h rebuildControls context ---\n'
sed -n '145,175p' src/core/MoonModule.h
printf '\n--- Search for filesystemUsed / kUsageRefreshMs ---\n'
rg -n -A3 -B3 'filesystemUsed|kUsageRefreshMs|lastUsageRefreshMs_' src/coreRepository: MoonModules/projectMM
Length of output: 10001
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- control rebuild triggers ---'
rg -n -A6 -B6 'controlChangeTriggersBuildState|rebuildControls\(|setControl\(' src/core
printf '\n%s\n' '--- FileManagerModule control-change hooks ---'
rg -n -A4 -B4 'show hidden|controlChangeTriggersBuildState|onUpdate|onBuildControls' src/core/FileManagerModule.*
printf '\n%s\n' '--- Scheduler / HTTP control write path ---'
rg -n -A8 -B8 'buildState\(|rebuildControls\(|setControl\(' src/core/Scheduler.* src/core/HttpServerModule.*Repository: MoonModules/projectMM
Length of output: 50377
Move the filesystem usage scan out of onBuildControls(). rebuildControls() re-runs this method on control writes, so toggling show hidden still pays the full LittleFS scan on the single-threaded tick. Keep onBuildControls() to rebinding descriptors only, and refresh the gauge from the throttled path or cached state.
🧰 Tools
🪛 Cppcheck (2.21.0)
[style] 26-26: The function 'parseDottedQuad' is never used.
(unusedFunction)
🤖 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/core/FileManagerModule.cpp` around lines 23 - 26, Move the filesystem
usage calls from onBuildControls() into the existing throttled refresh path,
using cached totalBytes_ and usedBytes_ when rebuilding controls. Keep
onBuildControls() limited to rebinding control descriptors, and ensure the usage
gauge is refreshed from the cached or periodically updated values without
triggering a LittleFS scan on control writes.
| void Scheduler::tick() { | ||
| uint32_t now = platform::millis(); | ||
| uint32_t tickStart = platform::micros(); | ||
|
|
||
| if (lastFrameStartUs_ != 0) { | ||
| const uint32_t targetFrameUs = 1000000u / targetFps_; | ||
| const uint32_t dueUs = lastFrameStartUs_ + targetFrameUs; | ||
| for (;;) { | ||
| const uint32_t nowUs = platform::micros(); | ||
| if (static_cast<int32_t>(nowUs - dueUs) >= 0) break; | ||
| const uint32_t remainingUs = dueUs - nowUs; | ||
| if (remainingUs >= 2000) platform::delayMs(remainingUs / 1000); | ||
| else platform::yield(); | ||
| } | ||
| } | ||
| lastFrameStartUs_ = platform::micros(); | ||
| const uint32_t workStartUs = lastFrameStartUs_; | ||
| uint32_t now = platform::millis(); | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect scheduler and platform yield/delay semantics from the repository.
git ls-files | rg '^(src/core/Scheduler\.cpp|src/core/Scheduler\.h|src/core/platform|src/platform|docs|README\.md)$' || true
echo '--- Scheduler.cpp ---'
cat -n src/core/Scheduler.cpp | sed -n '1,220p'
echo '--- Scheduler.h ---'
cat -n src/core/Scheduler.h | sed -n '1,260p'
echo '--- platform yield/delay references ---'
rg -n "yield\(|delayMs\(|micros\(|loop20ms|loop1s|HTTP|Network|fpsCap|targetFps_" src README.md docs -S
echo '--- platform implementation files ---'
fd -t f '.*(platform|yield|delay).*' src . || trueRepository: MoonModules/projectMM
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find platform abstraction definitions and any desktop-specific yield behavior.
rg -n "namespace platform|platform::yield|yield\s*\(\)|delayMs\s*\(" src -S
echo '--- file inventory for platform-related code ---'
git ls-files | rg '^src/.*/.*platform.*|^src/.*/.*yield.*|^src/.*/.*delay.*|^src/.*/.*micros.*|^src/.*/.*millis.*' || trueRepository: MoonModules/projectMM
Length of output: 1822
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the current Scheduler implementation and platform abstraction docs.
sed -n '1,220p' src/core/Scheduler.cpp | cat -n
echo '---'
sed -n '1,260p' src/core/Scheduler.h | cat -n
echo '---'
rg -n "loop1s|loop20ms|HTTP|Network|fpsCap|targetFps_|yield\(|delayMs\(" src README.md docs -SRepository: MoonModules/projectMM
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the actual desktop and ESP32 platform yield/delay implementations.
echo '--- src/platform/desktop/platform_desktop.cpp (yield/delay) ---'
sed -n '170,195p' src/platform/desktop/platform_desktop.cpp | cat -n
echo '--- src/platform/esp32/platform_esp32.cpp (yield/delay) ---'
sed -n '160,180p' src/platform/esp32/platform_esp32.cpp | cat -n
echo '--- src/platform/platform.h (yield/delay declaration) ---'
sed -n '35,50p' src/platform/platform.h | cat -nRepository: MoonModules/projectMM
Length of output: 3494
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find whether the scheduler documents a blocking/pacing trade-off already, and
# whether low-fps pacing is intended to preserve timing metrics rather than
# service other work.
rg -n "fpsCap|targetFps|frame pacing|yielding to other tasks|single render loop|non-blocking|blocking sleep|hot path" README.md docs src/core/Scheduler.* src/platform/platform.h -SRepository: MoonModules/projectMM
Length of output: 47824
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check how Scheduler::tick() is driven in main().
sed -n '500,555p' src/main.cpp | cat -nRepository: MoonModules/projectMM
Length of output: 3137
src/core/Scheduler.cpp:65-82 — Keep the frame-cap gate non-blocking
delayMs() / yield() here blocks the single shared scheduler loop, so lowering targetFps also delays loop20ms() / loop1s() work (HTTP, network polling, reconnects) by up to one frame interval (~100 ms at the 10 fps floor). On desktop, yield() is a no-op, so the last <2 ms of each wait spins on micros().
Split render pacing from service cadence or switch to an elapsed-time gate.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 65-65: function 'tick' has cognitive complexity of 40 (threshold 25)
(readability-function-cognitive-complexity)
[note] 68-68: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 71-71: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 73-73: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 75-75: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 76-76: +1, nesting level increased to 3
(clang)
[warning] 69-69: integer literal has suffix 'u', which is not uppercase
(readability-uppercase-literal-suffix)
[warning] 73-73: statement should be inside braces
(readability-braces-around-statements)
[warning] 75-75: statement should be inside braces
(readability-braces-around-statements)
[warning] 76-76: statement should be inside braces
(readability-braces-around-statements)
🪛 Cppcheck (2.21.0)
[style] 65-65: The function 'tick' is never used.
(unusedFunction)
[style] 68-68: The function 'setTargetFps' is never used.
(unusedFunction)
[style] 73-73: The function 'targetFps' is never used.
(unusedFunction)
[style] 78-78: The function 'tickTimeUs' is never used.
(unusedFunction)
[style] 79-79: The function 'workTimeUs' is never used.
(unusedFunction)
[style] 80-80: The function 'maxWorkTimeUs' is never used.
(unusedFunction)
[style] 81-81: The function 'maxFrameTimeUs' is never used.
(unusedFunction)
[style] 82-82: The function 'fps' is never used.
(unusedFunction)
🤖 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/core/Scheduler.cpp` around lines 65 - 82, Refactor Scheduler::tick() so
frame-rate limiting never blocks the shared scheduler loop: remove the
delayMs()/yield() wait loop and use an elapsed-time gate that skips rendering
until the next frame deadline while still allowing loop20ms(), loop1s(), HTTP,
and network work to run. Preserve targetFps_ pacing and update lastFrameStartUs_
only when a frame is actually processed.
Source: Coding guidelines
|
thx @Funkelfetisch for your PR's they are all 3 very useful and I am currently processing them, and will reply to all 3 soon |
|
AI update: Quick update — some of this PR now overlaps with work that just landed on The RGBW-preset part is largely supersededWe just built a reusable light-preset library ( 
What's still open from the RGBW side of your PR: nothing blocking — if there's a specific fixture wiring you need that isn't a built-in, it's now a one-row custom preset in the UI (or a built-in we can add). If your RMT change assumed the old 8-preset The two parts still genuinely yours
What you can do in the meantime
Thanks — the RMT direction is the piece we'd most like to see land next. |
Caches filesystem usage in the FileManager 1s loop, makes Scheduler frame pacing deadline-based instead of blocking in the hot path, and keeps RMT deinit from disabling a channel while a timed-out transfer may still be active. Checks: check_specs.py; git diff --check; build_esp32.py --firmware esp32s3-n16r8 (projectMM.bin 0x17f4a0, total image 1569836 bytes). Notes: RGBW preset overlap with upstream LightPresetsModule remains a PR-scope split/rebase item; this commit addresses the concrete review fixes without reshaping that scope.
|
Hi FunkelFetish, you kept my quite busy this weekend ;-), thx for your contribution as it helped me a lot improving things, including running lots of leds on lots of pins (well only 8, but 16 is next) Here a more detailed answer from one of my coworkers ;-) AI update: Follow-up on my earlier note. This PR was genuinely useful — arguably more than a straight merge would have been. It pushed me into a deep dive on LED encode performance and the render loop, which surfaced the real bottleneck and led to concrete improvements (below). The code itself isn't merging, but the PR did its job: it pointed at the right questions and got the work moving. Per part: Frame pacing — decided againstMeasured on real hardware; it's not the direction here.
Non-blocking RMT — already on
|
|
haha always happy to destroy weekends xD |
2 participants
Summary
Validation
git diff --check origin/main..HEADmoondeck/check/check_specs.pymoondeck/check/check_platform_boundary.pymoondeck/build/build_esp32.py --firmware esp32s3-n16r8esp32s3-n16r8image:0x17f4a0, 63% app partition freeNot Run
Summary by CodeRabbit
New Features
Bug Fixes
Documentation