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