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.
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