perf(display): dirty tracking in update_display + plugin FPS declaration - #406
Merged
Conversation
update_display now skips SetImage+SwapOnVSync when the frame is byte-identical to the last pushed one (adler32 digest) AND brightness is unchanged — brightness is part of the digest, and set_brightness additionally resets it, so a dim-schedule change can never be skipped. clear() resets the digest (it writes to the matrix directly). Skipping a swap is hardware-safe: the panel refreshes the current frame from the driver's own thread; swaps only change content. Kill switch: display.dirty_tracking: false restores always-push. display_controller's high-FPS decision gains a precedence step: a plugin exposing needs_high_fps is honored first (so static-image can declare False for still PNGs and stop burning a 125fps loop on them); static-image without the attribute keeps its historical forced high-FPS (GIF back-compat); scrolling logic is otherwise unchanged. Verified with 7 tests against the real DisplayManager on RGBMatrixEmulator (identical-frame skip, pixel-change push, clear and brightness invalidation, snapshot-through-skip, kill switch) plus the 202-test display/controller/vegas suites, and a clean devpi deploy. Audit: every SetImage/SwapOnVSync/Clear/brightness call site is inside display_manager — no external writer can bypass the digest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds configurable framebuffer dirty tracking to skip unchanged hardware pushes while preserving snapshots, and updates per-mode rendering to honor plugins’ ChangesDisplay performance controls
High-FPS mode selection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DisplayManager
participant RGBMatrixEmulator
participant SnapshotFile
DisplayManager->>DisplayManager: Compute canvas and brightness digest
DisplayManager->>SnapshotFile: Write snapshot when due
DisplayManager->>RGBMatrixEmulator: SwapOnVSync when the digest changed
DisplayManager->>DisplayManager: Cache the pushed digest
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 5 |
| 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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 2087-2088: Update the FPS debug log in the controller’s FPS-check
flow to use structured context, add the stable “[DisplayController]” source
prefix, and include the plugin identifier alongside active_mode and
needs_high_fps. Reuse the existing plugin identity available in that flow rather
than introducing a new identifier.
In `@src/display_manager.py`:
- Around line 548-560: In the dirty-tracking digest logic, replace the broad
Exception handler around self.matrix.brightness with the specific exception type
used by the established get_brightness() and set_brightness() patterns, while
preserving the None fallback when the brightness property cannot be read.
- Around line 548-560: Serialize the entire update_display() operation with the
manager’s existing synchronization primitive, including the digest check, canvas
swap, and push/update work, so concurrent callers cannot race or both pass the
_last_pushed_digest check. Ensure every caller path into update_display() uses
the same lock and preserve the existing early-return behavior for unchanged
frames.
In `@test/test_display_dirty_tracking.py`:
- Around line 25-39: Reset DisplayManager singleton state during teardown for
both the dm fixture and test_config_flag_wires_through. Add cleanup that sets
DisplayManager._instance to None and DisplayManager._initialized to False after
each use, ensuring no configured instance leaks into later test modules.
- Around line 96-102: Update test_snapshot_still_written_on_skip to perform an
initial update followed by a second update that meets the skip conditions, then
assert the snapshot exists after the skipped push. Preserve the existing
snapshot path setup and drawing behavior while ensuring the test explicitly
exercises the skipped hardware-update path in update_display.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d2e2ea0-21a1-4eed-8799-03520f733c94
📒 Files selected for processing (3)
src/display_controller.pysrc/display_manager.pytest/test_display_dirty_tracking.py
…log fixes CodeRabbit review on #406, verified against current code: - update_display() can genuinely be called from background threads (some sports base classes call it directly from inside update() for an immediate "live" refresh), not just the render loop — confirmed via the existing follower-mode gating wrapper in display_controller.py, which exists specifically because "background plugin threads" can reach it. Without a lock, two callers could both pass the digest check before either writes _last_pushed_digest back, causing a redundant push, or interleave the offscreen/current canvas swap. Added self._update_lock (RLock, in case of re-entrant callers) around the full method body so every call site is automatically covered — no caller changes needed. (No prior lock existed to reuse on DisplayManager; this adds one.) - Narrowed the brightness-read exception handler to AttributeError, matching the established pattern in get_brightness()/set_brightness() — a getattr() with a default already swallows AttributeError, so the only case this guards is the property getter itself raising, and the established pattern treats that as an expected, specific failure mode rather than something to blanket-catch. - FPS-check debug log now includes the plugin_id already in scope (previously only active_mode) and a "[DisplayController]" prefix for grep-ability, matching the sibling log two lines below it. - test_display_dirty_tracking.py: dm fixture and test_config_flag_wires_through now reset the DisplayManager singleton on teardown, matching the pattern test_display_manager.py already uses elsewhere in the same file family. - test_snapshot_still_written_on_skip previously only exercised the non-skip (push) path despite its name; now performs a second update that meets the skip conditions (identical frame) and asserts the snapshot is still written even though the panel push itself is skipped. All 7 dirty-tracking tests pass, plus the full display_manager/ display_controller/vegas suite (140 passed). Full repo suite has only the 5 known pre-existing failures (double-sided config x2, state_reconciliation x2, and test_circuit_breaker's conftest.py mock signature drift — the latter fixed in #400, which this branch's base predates).
Owner
Author
|
Addressed the CodeRabbit findings, verified against current code: Fixed:
All 7 dirty-tracking tests pass, plus the full display_manager/display_controller/vegas suite (140 passed). Full repo suite shows only the 5 known pre-existing failures unrelated to this change. |
# Conflicts: # src/display_manager.py
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
PR 4 of the performance series.
update_displaypushed every frame to the panel unconditionally — static content re-pushed every second, high-FPS loops re-pushed 125×/sec between actual scroll steps, andstatic-imagewas hardcoded into the 125fps loop even for non-animated PNGs.SetImage/SwapOnVSync. Hardware-safe: the driver refreshes the panel from its own thread; swaps only change content.set_brightnessresets it (dim schedules can't be skipped);clear()resets it (writes to the matrix directly). Audit confirmed every panel-write call site lives indisplay_manager— nothing can bypass the digest.display.dirty_tracking: falserestores byte-exact old behavior.needs_high_fps; static-image without it keeps its historical forced high-FPS (GIF back-compat). Monorepo follow-up will have static-image declare based on whether the loaded asset is animated.Verification & honest numbers
DisplayManageron RGBMatrixEmulator (skip/push/clear/brightness/snapshot/kill-switch) + 202-test display/controller/vegas suites.🤖 Generated with Claude Code
https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam
Summary by CodeRabbit
New Features
Bug Fixes
Tests