fix(startup): bound the initial plugin update so the panel lights sooner - #456
Merged
Conversation
DisplayController.__init__ calls _update_modules() once to populate
plugin data before the first frame. It walks every loaded plugin in
turn, and each update blocks the calling thread for up to the executor's
30s timeout, so the uncapped total is the sum of every slow plugin on
the system. The rig's own log:
Initial plugin update completed in 82.255 seconds
Initial plugin update completed in 55.123 seconds
Initial plugin update completed in 25.975 seconds
The panel shows nothing for all of it.
Nothing is lost by stopping early. A plugin that has never updated is
immediately due, so run_scheduled_updates() collects it seconds later --
with the display already running rather than blank.
A deadline alone was not enough: it is checked before each plugin, so
the last one to start could still block for the full 30s, and a 20s
budget produced a 31.8s pass on the rig. The remaining budget is now
passed down as that update's timeout too, with a floor so a plugin
starting on the last sliver is not handed ~0s and recorded as having
timed out for a slot it never had. Measured after: 20.006s.
Found while profiling a scroll freeze with py-spy, which caught the main
thread 9.34s inside execute_with_timeout's join. Worth being clear that
this is startup latency, not the recurring stutter -- _update_modules
has exactly one caller and runtime updates already run off the display
thread.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
|
Warning Review limit reached
Next review available in: 27 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR bounds initial plugin updates to a 20-second startup budget and adds deferred-plugin logging. It also detects a local IPv4 address and displays it in a centered, adaptive initialization banner. ChangesStartup plugin update budget
Initializing screen banner
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 17 |
| Duplication | 0 |
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.
That screen is what the panel holds for the whole initial plugin update, and on a headless Pi it is the only place the address appears without going looking for it -- so it now carries the address under "Initializing". The lookup connects a UDP socket, which sends no packets: it only asks the kernel which source address it would route from. That costs 0.03ms and works with the network down so long as a route exists. Deliberately not `hostname -I` plus a systemctl probe for AP mode, which is how the web launcher does it -- two subprocesses with multi-second timeouts, on the startup path this branch exists to shorten. Two things had to change for the address to be worth putting there. The text is now sized to fit rather than fixed at 8px: "Initializing" is 96px in PressStart2P, drawn at x=10, so it already ran off the side of a 64px panel before an address was added. It falls back to 4x6 where that does not fit, and both lines are centred. And the test pattern is punched out from behind the block, with the text drawn white rather than blue. The diagonal runs through the middle of the panel, which is exactly where this sits, and blue on black reads fine on a monitor but is marginal on a dim panel. An address that cannot be read off the wall is not worth showing. The rendering tests assert against pixels -- no green left behind the text at any supported size, enough lit pixels to be visible -- rather than against the geometry that produced them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
![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
🤖 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/display_controller.py`:
- Around line 867-876: Update the startup update flow around execute_update so a
plugin is deferred when the remaining deadline budget is below
_MIN_INITIAL_UPDATE_TIMEOUT_SECONDS; otherwise pass the exact remaining budget
as timeout without applying a minimum floor. Add a regression test covering a
plugin that begins with less than two seconds remaining and verifies it is
deferred.
In `@test/test_initializing_screen.py`:
- Line 104: Update the `_layout` assignment in the initializing-screen test to
bind the unused third return value as `_top` instead of `top`, while preserving
the existing `_font`, `widths`, and `bottom` bindings.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d095d58-4328-417a-8fa5-bce85f36499b
📒 Files selected for processing (4)
src/display_controller.pysrc/display_manager.pytest/test_initial_update_budget.pytest/test_initializing_screen.py
The test pattern lights one pure channel per element: red border, green diagonal, blue text. That is how a glance at the panel tells you whether led_rgb_sequence is right -- wire it BGR and the border comes up blue and the text red. Drawing the text white, as the previous commit did for contrast, lights all three channels and destroys the only blue reference on the screen. Reverted to blue, with the reason written down so it is not treated as a style preference again, and with tests that pin it: the text must be pure blue, nothing on the screen may be white, and all three primaries must be present. The punched-out backdrop stays. It only removes the diagonal from behind the glyphs, which costs nothing diagnostically -- the diagonal is still plainly visible across the rest of the panel -- and it is what makes the address readable at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
…p it The per-plugin timeout was clamped up to a floor, so a plugin that began with a sliver of budget left was granted the full floor and ran on past the deadline: a 20s budget could take 22. The floor existed to stop a plugin being handed a slot too short to use and then recorded as having timed out, which is a real concern, but clamping solved it by breaking the bound. Deferring solves both. Below the floor the plugin is left to the update tick, which was already the fate of everything after the deadline, so nothing new is lost -- a plugin that has never updated is immediately due. Above it, the timeout is the exact remainder, and the pass cannot outlast its deadline. Measured on the rig after the change: 20.002s, 5 plugins deferred. Also names an unused binding in the initializing-screen test. Both reported by CodeRabbit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
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
DisplayController.__init__calls_update_modules()once to populate plugin data before the first frame. It walks every loaded plugin in turn, and each update blocks the calling thread for up to the executor's 30s timeout — so the uncapped total is the sum of every slow plugin on the system.The controller's own log, from three boots of the dev rig:
The panel shows nothing for all of it.
Measured result
Why deferring is safe
A plugin that has never updated is immediately due, so
run_scheduled_updates()collects it seconds later — with the display already running instead of blank. The deferred plugins are named in the log rather than silently dropped.A deadline alone wasn't enough
It's checked before each plugin, so the last one to start could still block for the full 30s: a 20s budget produced a 31.8s pass on the rig. The remaining budget is now passed down as that update's timeout as well, with a floor (
_MIN_INITIAL_UPDATE_TIMEOUT_SECONDS) so a plugin starting on the last sliver isn't handed ~0s and recorded as having timed out for a slot it never really had.Scope — this is not the stutter
I found this while profiling the scroll freeze, and I want to be clear about what it is and isn't. py-spy caught the main thread 9.34s inside
execute_with_timeout'sjoin, and I initially described it as "a second freeze, longer than the first". It isn't:_update_moduleshas exactly one caller, in__init__, and runtime plugin updates already run off the display thread via the Vegas update tick. This is startup latency, not a recurring freeze.The recurring freeze is fixed separately in ledmatrix-plugins#272 (map tiles fetched on the render thread).
Tests
13 tests in
test/test_initial_update_budget.py: every plugin runs without a deadline; a passed deadline stops the pass; one slow plugin doesn't drag in the twenty behind it; the deadline is re-checked per plugin rather than once up front; the per-plugin timeout is capped by the remaining budget and never drops below the floor; the executor default is left alone when no deadline is given; deferred plugins are named in the log and nothing is logged when all of them ran; and no-plugin-manager / empty-plugin-set stay harmless.2757 passedon the full suite. The one failure,test_install_lowmem.py::TestDiskBackedTmpdir, is pre-existing onmainand environment-dependent.🤖 Generated with Claude Code
https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
Summary by CodeRabbit