feat(scroll): report a panel that cannot reach its refresh cap, and suggest one it can hold - #759
Open
ChuckBuilds wants to merge 1 commit into
Open
ChuckBuilds wants to merge 1 commit into
ChuckBuilds wants to merge 1 commit into
Conversation
…uggest one it can hold
Scroll speeds are solved against display.hardware.limit_refresh_rate_hz,
which is only a ceiling. A panel that cannot reach it still moves whole
pixels per frame, but every scroll runs slow by the shortfall and the
"smooth" ladder is the cap's, not the panel's. A user rig (Pi 4, 2x128x64,
adafruit-hat-pwm, pwm_bits 9, gpio_slowdown 5) measured 107.6-113.1 Hz under
a 120 Hz cap: 60 px/s ran at 55, and nothing said why.
- scroll_config: refresh_shortfall() (more than 3% under the planned rate),
holdable_cap() (a multiple of 10, 5% under the measurement, since the
measurement is the fast end of an uncapped panel's drift), and
describe_refresh_shortfall().
- FrameTimingRecorder.plan_refresh(): once the measured period has held for
three trusted windows, a shortfall is logged once as a warning naming the
cap to use. DisplayManager calls it only for a real panel, not the
emulator or the fallback canvas. The stats file records
planned_refresh_hz (additive).
- GET /api/v3/config/refresh-rate, plus a hint under the Display tab's
Limit Refresh Rate field with a button that fills in the suggested cap.
- _panel_refresh_hz (behind the Vegas slider's advice) ignores a measurement
written under a different cap, so a changed cap stops being advised from
the old rate before the display restarts.
Verified on ledpi with a temporary 200 Hz cap: the warning logged about a
minute after the restart ("about 132 Hz ... Set Limit Refresh Rate to
120 Hz"), the endpoint returned the same shortfall, and the Display tab
showed the hint; its button filled in 120. ledpi was restored afterwards.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 42 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (11)
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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 27 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
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.
Summary
Scroll speeds are worked out against
display.hardware.limit_refresh_rate_hz, but that setting is only a ceiling. A panel that can't reach it still moves whole pixels per frame. What goes wrong:The display already measures the real refresh rate (
measured_refresh_hzin the frame-stats file), and the Vegas slider's advice already uses it. Only the display itself never compared it with the cap.Found on a user rig (Pi 4, 2×128×64,
adafruit-hat-pwm,pwm_bits 9,gpio_slowdown 5): it measured 107.6–113.1 Hz under a 120 Hz cap, so 60 px/s ran at 55. Capping at 100 Hz fixed it, but finding that took hand-reading the frame timings.Changes
Shortfall helpers in
src/common/scroll_config.py:refresh_shortfall(measured, planned): reports a panel that measures more than 3% below the rate speeds are planned for.holdable_cap(measured): suggests a multiple of 10 at least 5% under the measurement. The measurement is the fast end of an uncapped panel's drift, so a cap inside that band wouldn't hold.describe_refresh_shortfall(): the log line.One warning from the display:
FrameTimingRecorder.plan_refresh()checks the measured rate once it has held for 3 windows of scrolling, and logs a single warning naming the cap to use.DisplayManagercalls it only for a real panel. The emulator and the fallback canvas pace themselves, so the check would be noise there.Web UI:
GET /api/v3/config/refresh-rate.Stale measurements:
planned_refresh_hz(additive)._panel_refresh_hz, which feeds the Vegas slider's advice, ignores a measurement written under a different cap. After a cap change, the slider no longer advises from the old rate until the display restarts.Docs and changelog: a "A panel that cannot reach its cap" section in
docs/SCROLL_PERFORMANCE.md, plus a CHANGELOG entry.Deliberately not done: switching speeds over to the measured rate automatically. Plugins set their speeds at startup, before any measurement exists. And an uncapped panel's rate drifts, so the fix is a cap it can hold, not a moving target.
Test plan
TestRefreshShortfall(7 tests), recorder warn-once / no warning at the cap / no warning without a planned rate / snapshot field, andtest_api_v3_refresh_rate.py(5 tests).test/web_interface, display manager, scan order, frame timing, scroll config and scroll speeds suites: same 11 pre-existing Windows failures asmain(starlark pixlet routes and others), none new.node test/js/run_all.js: all suites passed.Merge note
#756 (3.8.1 release prep) also edits the spot right under
## Unreleasedin the CHANGELOG. Whichever merges second needsmainmerged in, and this branch's entry then goes in the new empty Unreleased section.🤖 Generated with Claude Code