From 4564dc172d2787c0161fee0f2236ed89b87907db Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Thu, 17 Sep 2026 08:49:48 -0400 Subject: [PATCH] fix(sports): scoreboard scroll speed no longer follows target_fps sports_scroll computed the crisp speed ladder against the global target_fps whenever limit_refresh_rate_hz was the 100 Hz default. Since frame-locked presentation (#545) the helper steps a fixed number of whole pixels per presented frame and the panel presents at its real refresh, so the General tab's "Scroll Frame Rate" became a speed multiplier: 60 ran a 50 px/s scoreboard at 100 px/s, 200 ran it at 25 px/s. The ladder now uses the display manager's refresh_hz, then display.hardware.limit_refresh_rate_hz, then the default. target_fps is not consulted. Docstrings now say scroll_delay is ignored for pacing (no behaviour change there) and describe the fixed-step model. Tests: replace the tests that pinned target_fps as the ladder refresh and described time-based stepping; assert speed independence from target_fps (unit and end-to-end presented px/s against the real helper), that the fixed per-frame step is applied, and that scroll_delay does not change speed. Co-Authored-By: Claude Opus 5 --- src/common/sports_scroll.py | 88 ++++++++++--------- test/test_sports_scroll.py | 168 ++++++++++++++++++++++++++---------- 2 files changed, 170 insertions(+), 86 deletions(-) diff --git a/src/common/sports_scroll.py b/src/common/sports_scroll.py index 0278a1d3..d32fa76a 100644 --- a/src/common/sports_scroll.py +++ b/src/common/sports_scroll.py @@ -18,10 +18,15 @@ Promoting the content layer would be exactly the mistake ``docs/SPORTS_UNIFICATION.md`` warns against — merging on the intuition that same-named methods are the same method. Same name, different job. -The one behavior this module adds over the plugin copies is native support for -``global_config['target_fps']``: the bundled copies hardcode ~100 FPS via -``scroll_delay=0.01`` and never consult the global smooth-scrolling target. A -plugin inheriting from here gets it for free. +What this module adds over the plugin copies is pacing through +:mod:`src.common.scroll_config`, the resolver every other scroller uses: the +configured px/s is snapped to a whole-pixel speed for the panel's refresh, the +helper steps a fixed number of pixels per presented frame, and each drawn frame +publishes the frame hold to the display manager. Speed depends only on +``scroll_speed`` and the panel refresh +(``display.hardware.limit_refresh_rate_hz``) -- not on the global +``target_fps``, and not on ``scroll_delay``, which is kept in the settings for +compatibility only. Usage:: @@ -94,9 +99,9 @@ class SportsScrollDisplay: :param display_manager: the core display manager :param config: the plugin's configuration :param custom_logger: the plugin's logger, so scroll lines are attributed - :param global_config: the LEDMatrix global config — the source of - ``target_fps``. Optional so an older caller that does not pass it - keeps working at the config-derived pacing. + :param global_config: the LEDMatrix global config, consulted for the + panel refresh when the display manager cannot report one. + Optional so an older caller that does not pass it keeps working. """ self.display_manager = display_manager self.config = config @@ -196,21 +201,6 @@ class SportsScrollDisplay: return {**settings, **override} return settings - def _resolve_target_fps(self) -> Optional[float]: - """The global smooth-scrolling FPS target, or None to keep config pacing. - - Coerced before use: a malformed value in the global config must degrade - to the existing ``scroll_delay`` pacing, never raise on a display path. - """ - raw = self.global_config.get("target_fps") or self.global_config.get( - "scroll_target_fps" - ) - try: - return float(raw) if raw is not None else None - except (TypeError, ValueError): - self.logger.debug("Ignoring unusable target_fps: %r", raw) - return None - def _coerce_float(self, value: Any, default: float) -> float: """A usable float from config, or ``default``. @@ -246,8 +236,8 @@ class SportsScrollDisplay: # settings dict: the two read the same key names with different # meanings, and the collision is a factor of 1/scroll_delay. # - # sports_scroll: scroll_speed is px/SECOND; scroll_delay is only the - # frame period used to reach px/frame. + # sports_scroll: scroll_speed is px/SECOND; scroll_delay is ignored + # for pacing (see _resolve_pixels_per_second). # scroll_config: scroll_speed is px per STEP, so px/s = speed/delay. # # Passing {"scroll_speed": 50.0, "scroll_delay": 0.01} straight through @@ -276,8 +266,13 @@ class SportsScrollDisplay: def _resolve_pixels_per_second(self, settings: Dict[str, Any]) -> float: """This module's config shape, expressed as plain pixels per second. - ``scroll_speed`` is already px/s here. ``scroll_delay`` only matters - when a caller supplied px/frame instead, which the 0 case covers. + ``scroll_speed`` is already px/s here, and it is the whole answer. + ``scroll_delay`` is kept in the settings for compatibility but is + ignored for pacing: the frame rate is the panel refresh divided by the + frame hold scroll_config picks, so no positive delay changes the speed. + The one exception is a delay of exactly 0 (below every scoreboard + schema's minimum), which is read as ``scroll_speed`` being px per + frame at ``ASSUMED_FPS_WHEN_UNPACED``. """ scroll_speed = self._coerce_float(settings.get("scroll_speed"), 50.0) scroll_delay = self._coerce_float(settings.get("scroll_delay"), 0.01) @@ -285,19 +280,29 @@ class SportsScrollDisplay: return scroll_speed * ASSUMED_FPS_WHEN_UNPACED return scroll_speed - def _resolve_refresh_hz(self) -> Optional[float]: + def _resolve_refresh_hz(self) -> float: """The panel refresh the crisp ladder should be computed against. - Prefers the configured hardware refresh. Falls back to the global - ``target_fps``/``scroll_target_fps`` this module has always honoured: - under the old model that key *was* the rate frames were presented at, - so it is the faithful translation for anyone who set it. Returning - None lets scroll_config apply its own default. + This has to be the rate frames are actually presented at, because the + helper advances a fixed number of whole pixels per presented frame and + the display manager holds each frame for ``frame_hold`` refreshes. A + ladder built for any other rate plays back at the wrong speed: built + for 60 Hz and shown on a 100 Hz panel, it runs 100/60 too fast. + + So it is the display manager's ``refresh_hz`` (what the panel is + driven at), then ``display.hardware.limit_refresh_rate_hz`` from the + global config, then scroll_config's default. The global ``target_fps`` + is deliberately NOT consulted: before frame-locked presentation it was + the rate frames were shown at, but now honouring it only turned the + General tab's "Scroll Frame Rate" into a scoreboard speed multiplier. """ - hardware = scroll_config.refresh_hz_from_config(self.global_config) - if hardware and hardware != scroll_config.DEFAULT_REFRESH_HZ: - return hardware - return self._resolve_target_fps() or hardware or None + reported = getattr(self.display_manager, "refresh_hz", None) + # A real number only: a stub or a mock answering for anything must not + # become the refresh rate. + if (isinstance(reported, (int, float)) and not isinstance(reported, bool) + and reported > 0): + return float(reported) + return scroll_config.refresh_hz_from_config(self.global_config) def _scroll_frame_hold(self) -> int: """Refreshes to hold each frame for, from the resolved settings.""" @@ -327,11 +332,12 @@ class SportsScrollDisplay: return False # Tell the core the panel is scrolling, and for how many - # refreshes to hold each frame. Without this the frame hold is - # never applied -- so a speed the ladder made crisp still presents - # a new frame every refresh and judders -- and, because deferred - # updates only run while nothing is scrolling, core would run - # blocking work in the middle of this scroll. + # refreshes to hold each frame. The helper advances a fixed number + # of whole pixels per presented frame, so without the hold a new + # frame is presented every refresh and the scroll runs frame_hold + # times too fast. And because deferred updates only run while + # nothing is scrolling, core would otherwise run blocking work in + # the middle of this scroll. if hasattr(self.display_manager, "set_scrolling_state"): self.display_manager.set_scrolling_state( True, frame_hold=self._scroll_frame_hold()) diff --git a/test/test_sports_scroll.py b/test/test_sports_scroll.py index 893dfa34..f6972883 100644 --- a/test/test_sports_scroll.py +++ b/test/test_sports_scroll.py @@ -6,8 +6,11 @@ is shared, the content layer is not. Two things are worth asserting beyond * ``prepare_scroll_content`` must stay an override point — a base class that quietly rendered *something* would let a plugin ship a blank scroll. -* ``target_fps`` must actually reach the helper. That is the whole reason this - module exists upstream; the bundled plugin copies hardcode ~100 FPS. +* Speed must depend only on ``scroll_speed`` and the panel refresh. The helper + steps a fixed number of whole pixels per presented frame and the panel + presents at its refresh divided by the frame hold, so a ladder computed for + any other rate -- the global ``target_fps`` used to be one -- plays back at + the wrong speed. """ import logging @@ -221,12 +224,14 @@ class TestConfigureScrollHelper: bare = SportsScrollDisplay.__new__(SportsScrollDisplay) assert SportsScrollDisplay._scroll_frame_hold(bare) == 1 - def test_stepping_is_time_based(self, build): - """Frame-based mode gated motion on a wall clock at 1/scroll_delay - steps, with scroll_delay set to the frame period -- putting the - comparison exactly on its own threshold, so it flipped on sub- - millisecond jitter.""" - build().scroll_helper.set_frame_based_scrolling.assert_called_once_with(False) + def test_helper_steps_whole_pixels_per_presented_frame(self, build): + """No wall clock: the helper advances the ladder's whole-pixel step on + every presented frame, and the frame hold sets how often that is.""" + display = build() + helper = display.scroll_helper + helper.set_frame_based_scrolling.assert_called_once_with(False) + helper.set_pixels_per_frame.assert_called_once_with( + display._scroll_settings.crisp.pixels_per_frame) def test_dynamic_duration_settings_are_applied(self, build): display = build({"nhl": {"scroll_settings": { @@ -243,31 +248,65 @@ class TestConfigureScrollHelper: applied = display.scroll_helper.set_scroll_speed.call_args[0][0] assert applied == pytest.approx(100.0, abs=1.0) - def test_global_target_fps_sets_the_refresh_the_ladder_uses(self, build): - """Under the old model this key was the rate frames were presented at, - so it is the faithful translation of it into a panel refresh.""" - display = build(global_config={"target_fps": 60}) - assert display._resolve_refresh_hz() == 60.0 + @pytest.mark.parametrize("global_config", [ + {}, + {"target_fps": 60}, + {"target_fps": 100}, + {"target_fps": 120}, + {"target_fps": 200}, + {"scroll_target_fps": 90}, + {"target_fps": 120, "scroll_target_fps": 90}, + ]) + def test_target_fps_does_not_change_scroll_speed(self, build, global_config): + """The General tab's "Scroll Frame Rate" is not a speed control. - def test_legacy_key_is_honored(self, build): - display = build(global_config={"scroll_target_fps": 90}) - assert display._resolve_refresh_hz() == 90.0 + It used to set the refresh the ladder was computed against, while the + panel kept presenting at its real 100Hz: 60 doubled every scoreboard's + speed and 200 halved it.""" + display = build(global_config=global_config) + assert display._resolve_refresh_hz() == 100.0 + assert display._scroll_settings.pixels_per_second == pytest.approx(50.0) + assert display._scroll_settings.frame_hold == 2 + assert display._scroll_settings.crisp.pixels_per_frame == 1 - def test_modern_key_wins_over_legacy(self, build): - display = build(global_config={"target_fps": 120, "scroll_target_fps": 90}) - assert display._resolve_refresh_hz() == 120.0 - - def test_configured_hardware_refresh_wins_over_the_fps_target(self, build): + def test_configured_hardware_refresh_sets_the_ladder(self, build): display = build(global_config={ "target_fps": 60, "display": {"hardware": {"limit_refresh_rate_hz": 120}}}) assert display._resolve_refresh_hz() == 120.0 + assert display._scroll_settings.crisp.refresh_hz == 120.0 + + def test_display_manager_refresh_wins_over_global_config(self, build, + display_manager): + """The display manager reports the rate the panel is driven at.""" + display_manager.refresh_hz = 60.0 + display = build(global_config={ + "target_fps": 200, + "display": {"hardware": {"limit_refresh_rate_hz": 120}}}) + assert display._resolve_refresh_hz() == 60.0 + + @pytest.mark.parametrize("reported", [True, 0, -5, "100", None]) + def test_unusable_display_manager_refresh_falls_back(self, build, + display_manager, + reported): + display_manager.refresh_hz = reported + display = build(global_config={ + "display": {"hardware": {"limit_refresh_rate_hz": 120}}}) + assert display._resolve_refresh_hz() == 120.0 @pytest.mark.parametrize("bad", ["fast", None, {}, [], "", 0]) - def test_unusable_target_fps_degrades_instead_of_raising(self, build, bad): - """A malformed global config must cost the FPS target, not the display.""" + def test_unusable_target_fps_is_harmless(self, build, bad): + """A malformed global config must not cost the display.""" display = build(global_config={"target_fps": bad}) - assert display._scroll_settings.pixels_per_second > 0 + assert display._scroll_settings.pixels_per_second == pytest.approx(50.0) + + @pytest.mark.parametrize("delay", [0.001, 0.01, 0.05, 0.1]) + def test_scroll_delay_is_ignored_for_pacing(self, build, delay): + """Kept in the settings for compatibility; it does not change speed.""" + display = build({"nhl": {"scroll_settings": { + "scroll_speed": 50.0, "scroll_delay": delay}}}) + assert display._scroll_settings.pixels_per_second == pytest.approx(50.0) + assert display._scroll_settings.frame_hold == 2 @pytest.mark.parametrize("bad", [None, "fast", {}, []]) def test_unusable_scroll_speed_degrades_instead_of_crashing(self, build, bad): @@ -475,7 +514,9 @@ class TestLifecycle: class TestManager: @pytest.fixture def manager(self, display_manager): - return _Manager(display_manager, {}, LOGGER, global_config={"target_fps": 120}) + return _Manager(display_manager, {}, LOGGER, global_config={ + "target_fps": 120, + "display": {"hardware": {"limit_refresh_rate_hz": 120}}}) def test_displays_are_created_lazily_and_reused(self, manager): first = manager.get_scroll_display("live") @@ -487,15 +528,13 @@ class TestManager: "recent") def test_global_config_is_threaded_to_children(self, manager): - """A missed hand-off here is exactly how the plugin copies ended up - never honoring target_fps.""" + """The child needs the global config to find the panel refresh when + the display manager cannot report one.""" child = manager.get_scroll_display("live") - assert child.global_config == {"target_fps": 120} - # The target is now the refresh the crisp ladder is computed against, - # so what proves the hand-off is that the child reasons about 120Hz -- - # not the presentation rate, which the chosen hold divides down. + assert child.global_config["target_fps"] == 120 + # What proves the hand-off is that the child reasons about the 120Hz + # hardware refresh from that config (target_fps plays no part). assert child._resolve_refresh_hz() == 120.0 - child.scroll_helper.set_target_fps.assert_called_once() def test_prepare_sets_the_active_type(self, manager): assert manager.prepare_and_display([{"id": "g1"}], "live", ["nhl"]) is True @@ -619,15 +658,17 @@ class TestAgainstTheRealScrollHelper: def test_configuration_lands_on_the_real_helper(self, real): display = real.get_scroll_display("live") helper = display.scroll_helper - # 500 px/s on a 120Hz panel snaps to a whole number of pixels per - # refresh, and the helper is driven in px/s in time-based mode. + # 500 px/s on the default 100Hz panel is a whole number of pixels per + # refresh, and the helper steps that many pixels per presented frame. assert helper.frame_based_scrolling is False assert helper.scroll_speed == pytest.approx( display._scroll_settings.pixels_per_second) assert helper.scroll_speed == pytest.approx(500.0, abs=25.0) - # target_fps is the presentation rate the chosen hold produces. + assert helper.fixed_pixels_per_frame == ( + display._scroll_settings.crisp.pixels_per_frame) + # target_fps is informational: the presentation rate the hold gives. assert helper.target_fps == pytest.approx( - 120.0 / display._scroll_settings.frame_hold, abs=1.0) + 100.0 / display._scroll_settings.frame_hold, abs=1.0) def test_a_strip_is_built_and_scrolls_to_completion(self, real): import time @@ -638,21 +679,58 @@ class TestAgainstTheRealScrollHelper: # 3 cards of 100px + gaps, so the strip is wider than the 128px panel. assert display.scroll_helper.cached_image.width > 128 - # Frame-based scrolling is wall-clock gated on scroll_delay, so the - # loop has to actually pass time rather than spin. + # The helper steps a fixed whole-pixel amount per presented frame and + # consults no clock, so the scroll completes in a bounded number of + # frames however fast this loop spins. deadline = time.time() + 20 - while not real.is_complete() and time.time() < deadline: + frames = 0 + while not real.is_complete() and time.time() < deadline and frames < 10000: real.display_frame() - time.sleep(0.0012) + frames += 1 - # Asserted separately so a host too slow to sustain the frame rate - # reports a timeout rather than looking like a scrolling defect. - assert time.time() < deadline, ( - "scroll did not finish within 20s — the host may be too slow to " - "sustain the configured frame rate") + assert time.time() < deadline, "scroll did not finish within 20s" assert real.is_complete() is True assert display.scroll_helper.scroll_position > 128 def test_dynamic_duration_is_a_real_number(self, real): real.prepare_and_display([{"id": "a"}], "live", ["nhl"]) assert real.get_scroll_display("live").get_dynamic_duration() > 0 + + @pytest.mark.parametrize("target_fps", [None, 60, 100, 200]) + def test_presented_speed_is_independent_of_target_fps(self, monkeypatch, + target_fps): + """End to end: the px/s the panel actually shows. + + The panel is simulated the way SwapOnVSync paces it: each drawn frame + occupies ``frame_hold`` refreshes of a 100Hz panel. Ten seconds of + refreshes must move the strip 500px at the default 50 px/s, whatever + the General tab's target_fps says.""" + from src.common.scroll_helper import ScrollHelper + monkeypatch.setattr("src.common.sports_scroll.ScrollHelper", ScrollHelper) + + class _Panel: + matrix = None + width, height = 128, 32 + refresh_hz = 100.0 + hold = 1 + image = None + + def set_scrolling_state(self, scrolling, frame_hold=1): + self.hold = frame_hold + + def update_display(self): + pass + + global_config = {"display": {"hardware": {"limit_refresh_rate_hz": 100}}} + if target_fps is not None: + global_config["target_fps"] = target_fps + panel = _Panel() + display = _RealDisplay(panel, {}, LOGGER, global_config=global_config) + display.scroll_helper.set_scrolling_image(Image.new("RGB", (5000, 32))) + + refreshes = 0 + while refreshes < 1000: # ten seconds at 100Hz + assert display.display_scroll_frame() is True + refreshes += panel.hold + moved = display.scroll_helper.total_distance_scrolled + assert moved / 10.0 == pytest.approx(50.0, abs=1.0)