fix(scroll): advance whole pixels per frame, not per wall-clock second (#545)

* fix(scroll): advance whole pixels per frame, not per wall-clock second

Smooth motion is not a frame-rate property, and measuring it as one is why
this survived three rounds of fixes. odds-ticker's frame timing is excellent
-- 100.0 fps, 10.00ms median, 0% stalls, worst in-scroll frame 19.95ms -- and
it still visibly stuttered.

What the eye judges is whether the strip advances the same number of whole
pixels on every presented frame. update_scroll_position derived position from
scroll_speed * delta_time and get_visible_portion truncated it with int(), so
jitter in delta_time decided which side of a pixel boundary the position
landed on. The live windows show why that matters: a rock-steady 100.0 fps
whose individual frames still range 5.6ms to 15.2ms, which at 100 px/s is
0.57px to 1.44px of movement.

Run the measured frame times through the real helper and 5.8% of frames
advance 0 or 2 pixels instead of 1 -- about six hitches a second. A frame that
moves nothing followed by one that jumps two is exactly what micro-stutter
looks like.

It is worst at a crisp speed, which is the part that stings: at 100 px/s on a
100Hz panel the accumulator sits exactly on integer boundaries, so
sub-millisecond jitter flips it either way and the motion beats at around
50Hz. Snapping to the crisp ladder fixes the average and the wall clock then
throws away the per-frame uniformity the ladder was bought for.

So when scroll_config snaps to a crisp speed it now also puts the helper in
fixed-step mode: each presented frame advances exactly pixels_per_frame and no
clock is consulted. 100% of frames move by the same amount, whatever the
jitter.

This is only correct because SwapOnVSync blocks until the panel has taken the
frame, which makes the frame count a truer clock than time.time(). Before the
swap was locked to vsync it would have run at whatever speed the loop spun at.
Related: frame-based mode used to step discretely and was converted to
elapsed-time accumulation earlier in this series, because its threshold
comparison flipped on jitter. That was right for the code as it stood -- but
it treated the symptom, replacing a broken discrete step with a smooth-looking
accumulator instead of asking why a wall clock was involved at all.

Non-crisp speeds keep pacing off time, and set_scroll_speed() clears the fixed
step so a legacy caller changing speed is not silently ignored.

Trade-off worth naming: speed is now tied to the presentation rate rather than
to real time. If the loop cannot keep up with the panel the scroll runs slow
rather than jumping to catch up. That is the better failure -- uniform motion
at a slightly wrong speed beats correct average speed with a hitch six times a
second -- and a loop that cannot hit the resolved rate is a measurement
problem for the crisp ladder, not something to paper over with uneven steps.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* test(scroll): make the time-based pin actually pin something

Review caught that test_time_based_stepping_is_what_it_replaces could pass
against perfectly uniform motion, and it was right.

update_scroll_position sets last_update_time on its way through, so the very
first call sees a delta_time of zero and moves nothing in time-based mode.
_advances counted that synthetic frame, which put a guaranteed zero in every
histogram -- enough on its own to satisfy "uneven > 0". The test asserting the
defect exists would have passed after the defect was gone.

The first call is now primed and discarded, and the assertion is a proportion
rather than "more than zero": against these frame times the old path misses
roughly one frame in twenty, so 1% is well below the real rate and far above
anything a stray frame could produce.

Re-measured with the artefact removed, the numbers in the PR description are
unchanged: 5.85% of frames uneven before (114 zero-advance and 120 double
frames in 4000), 0.00% after.

Also fills in the docstrings the review flagged: everything in the new test
file, plus three pre-existing one-liners in scroll_config that the diff
touched.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

requirements-test.txt

@@ -7,3 +7,8 @@ freezegun>=1.2,<2        # deterministic time for golden-image tests
 psutil>=6.0.0,<8.0.0     # optional at runtime; installed for tests so the
                          # /system/status endpoint's real path is exercised
 mypy>=1.5.0,<2.0.0       # static type checking (also pinned in .pre-commit-config.yaml)
+PyYAML>=6.0.2,<7.0.0     # not a core dependency: test_starlark_pixlet_routes loads
+                         # plugin-repos/starlark-apps/tronbyte_repository.py the way
+                         # the blueprint does, and that plugin imports yaml. The
+                         # plugin declares it in its own requirements.txt, which the
+                         # store installs on a real rig but CI never does.

scripts/sports_scroll_check.py

@@ -0,0 +1,196 @@
+#!/usr/bin/env python3
+"""Drive a sports scoreboard scroll on the panel and report what it did.
+
+The eight sports scoreboards scroll through ``src/common/sports_scroll.py``,
+and that path is per-league opt-in: a rig showing static game cards never
+constructs a SportsScrollDisplay at all, so nothing about its pacing can be
+observed from a normal run. This drives it directly, with synthetic games, so
+the pacing can be measured without changing anyone's configuration.
+
+What it checks is what the shared resolver is supposed to buy:
+
+* the requested speed lands on a whole number of pixels per refresh
+* the frame hold that makes that true is published to the display manager
+* frames actually arrive at the interval the hold implies
+
+    sudo systemctl stop ledmatrix
+    sudo python3 scripts/sports_scroll_check.py --seconds 20
+    sudo systemctl start ledmatrix
+
+Like scripts/scroll_speeds.py, this never starts or stops the display service
+itself -- that is left to the caller, so a crash here cannot leave the panel
+dark.
+"""
+from __future__ import annotations
+
+import argparse
+import json
+import statistics
+import subprocess  # nosec B404 - list-form argv only, no shell  # nosemgrep
+import sys
+import time
+from pathlib import Path
+
+sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
+
+from PIL import Image  # noqa: E402
+
+from src.common.sports_scroll import SportsScrollDisplay  # noqa: E402
+from src.display_manager import DisplayManager  # noqa: E402
+
+
+class _Check(SportsScrollDisplay):
+    """A scoreboard whose cards are plain blocks -- pacing is what matters."""
+
+    SCROLL_LEAGUE_KEYS = ("nfl",)
+
+    def prepare_scroll_content(self, games, game_type, leagues, rankings_cache=None):
+        width = self.display_height * 2
+        cards = []
+        for i, _ in enumerate(games):
+            card = Image.new("RGB", (width, self.display_height), (0, 0, 0))
+            shade = 40 + (i * 37) % 180
+            for x in range(2, width - 2):
+                for y in range(2, self.display_height - 2):
+                    card.putpixel((x, y), (shade, 90, 220 - shade // 2))
+            cards.append(card)
+        self._current_games = list(games)
+        self._current_game_type = game_type
+        self._current_leagues = list(leagues)
+        self.scroll_helper.create_scrolling_image(content_items=cards, item_gap=24)
+        return bool(cards)
+
+
+class _HoldSpy:
+    """Records what the scroll publishes, without changing what it does."""
+
+    def __init__(self, display_manager):
+        self.dm = display_manager
+        self.calls = []
+        self._real = display_manager.set_scrolling_state
+
+    def __enter__(self):
+        def spy(is_scrolling, frame_hold=1):
+            self.calls.append((is_scrolling, frame_hold))
+            return self._real(is_scrolling, frame_hold=frame_hold)
+        self.dm.set_scrolling_state = spy
+        return self
+
+    def __exit__(self, *exc):
+        self.dm.set_scrolling_state = self._real
+        return False
+
+
+MESSAGE = """ledmatrix is running and owns the panel's GPIO.
+
+Stop it first, or this run can leave the display dark:
+
+    sudo systemctl stop ledmatrix
+    sudo python3 scripts/sports_scroll_check.py
+    sudo systemctl start ledmatrix
+
+Use --fallback to check the pacing logic without the panel, or --force if
+you really mean it."""
+
+
+def _refuse_if_the_service_is_running(force):
+    """Refuse to touch the panel while ledmatrix has it.
+
+    rpi-rgb-led-matrix configures GPIO directions and the hardware PWM inside
+    RGBMatrix(), and on the root check it calls exit() from C -- no cleanup.
+    Do that while the service is driving those same pins and the panel goes
+    dark while the service carries on rendering happily: fresh framebuffer,
+    every pixel lit, "RGB Matrix initialized successfully", nothing in the log.
+    A restart brings it back, but only once you work out that is what happened.
+
+    The module docstring says to stop the service first. This makes it true.
+    """
+    if force:
+        return
+    try:
+        active = subprocess.run(  # nosec B603 B607 - hardcoded systemctl args  # nosemgrep
+            ["systemctl", "is-active", "ledmatrix"],
+            capture_output=True, text=True).stdout.strip()
+    except OSError:
+        return  # not a systemd box; nothing to protect
+    if active == "active":
+        sys.exit(MESSAGE)
+
+
+def main():
+    ap = argparse.ArgumentParser(
+        description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter)
+    ap.add_argument("--seconds", type=float, default=20.0)
+    ap.add_argument("--speed", type=float, default=None,
+                    help="px/s to request; default is the module's own")
+    ap.add_argument("--games", type=int, default=6)
+    ap.add_argument("--force", action="store_true",
+                    help="run even though the display service is up. It owns "
+                         "the GPIO; expect a dark panel until you restart it.")
+    ap.add_argument("--fallback", action="store_true",
+                    help="run without the panel. Driving the real matrix needs "
+                         "root; this checks everything except the vsync pacing "
+                         "-- what speed resolves to, that the hold is published, "
+                         "and that it is released afterwards.")
+    args = ap.parse_args()
+
+    if not args.fallback:
+        _refuse_if_the_service_is_running(args.force)
+
+    root = Path(__file__).resolve().parent.parent
+    config = json.loads((root / "config" / "config.json").read_text(encoding="utf-8"))
+
+    display_manager = DisplayManager(config, force_fallback=args.fallback)
+    settings = {} if args.speed is None else {
+        "nfl": {"scroll_settings": {"scroll_speed": args.speed}}}
+
+    display = _Check(display_manager, settings, global_config=config)
+    resolved = display._scroll_settings
+    print("resolved: %s" % resolved.describe())
+    print("frame hold: %d refresh(es) per frame" % resolved.frame_hold)
+    if resolved.warning:
+        print("warning: %s" % resolved.warning)
+
+    display.prepare_scroll_content(
+        [{"id": "g%d" % i} for i in range(args.games)], "live", ["nfl"])
+
+    gaps, drawn = [], 0
+    last = None
+    with _HoldSpy(display_manager) as spy:
+        started = time.perf_counter()
+        while time.perf_counter() - started < args.seconds:
+            if not display.display_scroll_frame():
+                break
+            now = time.perf_counter()
+            if last is not None:
+                gaps.append((now - last) * 1000.0)
+            last = now
+            drawn += 1
+        display.clear()
+
+    if not gaps:
+        sys.exit("no frames were drawn -- the scroll never started")
+
+    gaps.sort()
+    expected = 1000.0 * resolved.frame_hold / (resolved.crisp.refresh_hz
+                                               if resolved.crisp else 100.0)
+    print("\n%d frames in %.1fs -> %.1f fps" % (
+        drawn, args.seconds, drawn / args.seconds))
+    print("frame gap  median %.2fms  p95 %.2fms  max %.2fms  (hold implies %.2fms)"
+          % (statistics.median(gaps), gaps[int(len(gaps) * 0.95)], gaps[-1], expected))
+
+    holds = {h for on, h in spy.calls if on}
+    print("published while scrolling: frame_hold=%s" % (sorted(holds) or "NOTHING"))
+    print("released on clear: %s" % any(not on for on, _ in spy.calls))
+    print("display manager hold now: %d (1 means released)"
+          % getattr(display_manager, "_frame_hold", -1))
+
+    if not holds:
+        sys.exit("FAIL: the scroll never told the core it was scrolling")
+    if holds != {resolved.frame_hold}:
+        sys.exit("FAIL: published %s but resolved %d" % (holds, resolved.frame_hold))
+    print("\nOK: the resolved hold reached the panel and was released after")
+
+
+if __name__ == "__main__":
+    main()

src/common/scroll_config.py

@@ -86,6 +86,7 @@ class CrispSpeed:
 
     @property
     def frames_per_second(self) -> float:
+        """Distinct frames per second: the refresh divided by the hold."""
         return self.refresh_hz / self.frame_hold
 
     @property
@@ -100,6 +101,7 @@ class CrispSpeed:
         return "smooth"
 
     def describe(self) -> str:
+        """This speed as a line for the speed ladder, aligned for a column."""
         return (
             f"{self.pixels_per_second:6.1f} px/s  "
             f"({self.pixels_per_frame}px every {self.frame_hold} refresh"
@@ -202,6 +204,7 @@ class ScrollSettings:
         return self.crisp.frame_hold if self.crisp else 1
 
     def describe(self) -> str:
+        """One log line: the speed applied, and which config key produced it."""
         text = f"{self.pixels_per_second:.1f} px/s (from {self.source})"
         if self.pixels_per_frame is not None:
             text += f" = {self.pixels_per_frame:.2f} px/frame"
@@ -387,6 +390,15 @@ def configure(
     if hasattr(scroll_helper, "set_frame_based_scrolling"):
         scroll_helper.set_frame_based_scrolling(False)
     scroll_helper.set_scroll_speed(applied)
+
+    # A crisp speed is a whole number of pixels per presented frame, so step by
+    # that number rather than by speed * elapsed time. Snapping alone only
+    # fixes the average: the wall clock puts the accumulator back on an integer
+    # boundary every frame, where jitter of a fraction of a millisecond decides
+    # whether the pixel moves. That is what the ladder was bought to prevent.
+    if hasattr(scroll_helper, "set_pixels_per_frame"):
+        scroll_helper.set_pixels_per_frame(
+            choice.pixels_per_frame if choice else None)
     if choice and hasattr(scroll_helper, "set_target_fps"):
         scroll_helper.set_target_fps(choice.frames_per_second)
     elif settings.target_fps and hasattr(scroll_helper, "set_target_fps"):

src/common/scroll_helper.py

@@ -135,7 +135,10 @@ class ScrollHelper:
         self._last_integer_position = 0  # Cache for integer position to avoid repeated calculations
         
         # Frame-based scrolling settings
-        self.frame_based_scrolling = False  # If True, use scroll_delay to throttle and move scroll_speed pixels
+        self.frame_based_scrolling = False
+        #: Whole pixels to advance per presented frame, or None to pace
+        #: off elapsed time. See set_pixels_per_frame.
+        self.fixed_pixels_per_frame = None  # If True, use scroll_delay to throttle and move scroll_speed pixels
         self.last_step_time = 0.0  # Track last step time for frame-based throttling
         
         # Time tracking for scroll updates
@@ -295,7 +298,13 @@ class ScrollHelper:
             self.last_progress_log_time = current_time
         
         # Update scroll position
-        if self.frame_based_scrolling:
+        if self.fixed_pixels_per_frame:
+            # One presented frame, one fixed whole-pixel step. No clock is
+            # consulted, so no jitter reaches the position and every frame
+            # moves the eye by the same amount. See set_pixels_per_frame.
+            pixels_to_move = self.fixed_pixels_per_frame
+            self.last_step_time = current_time
+        elif self.frame_based_scrolling:
             # Frame-based: move fixed amount when scroll_delay has passed
             # This matches stock ticker behavior: move pixels, then wait scroll_delay
             # Initialize last_step_time on first call to prevent huge initial jump
@@ -979,6 +988,13 @@ class ScrollHelper:
         Args:
             speed: Scroll speed (interpretation depends on frame_based_scrolling mode)
         """
+        # A speed set directly is a request to pace off that speed, so drop any
+        # fixed per-frame step left by an earlier configure(). scroll_config
+        # calls this first and set_pixels_per_frame second, so the crisp path
+        # is unaffected; what this protects is a legacy caller changing speed
+        # on a helper that scroll_config had already put in fixed-step mode,
+        # where the new speed would otherwise be silently ignored.
+        self.fixed_pixels_per_frame = None
         if self.frame_based_scrolling:
             # In frame-based mode, clamp to reasonable pixels per frame (0.1-5)
             # Higher values cause visible jumps - 1-2 pixels/frame is ideal for smoothness
@@ -999,6 +1015,35 @@ class ScrollHelper:
         self.scroll_delay = max(0.001, min(1.0, delay))
         self.logger.debug(f"Scroll delay set to: {self.scroll_delay}")
     
+    def set_pixels_per_frame(self, pixels) -> None:
+        """Advance exactly `pixels` per presented frame, ignoring the clock.
+
+        Pass None to go back to pacing off elapsed time.
+
+        Smooth motion is not a frame-rate property. The strip has to advance
+        the same number of WHOLE pixels every frame, and deriving that from a
+        wall clock cannot deliver it: the position accumulates
+        ``speed * delta_time`` and is then truncated to a pixel, so any jitter
+        in delta_time lands either side of an integer boundary. Measured on
+        hardware at a rock-steady 100.0 fps, individual frames still ranged
+        5.6ms to 15.2ms -- 0.57px to 1.44px of movement -- and 53% of frames
+        advanced by something other than one pixel: about half moved nothing
+        at all and then jumped two. That is the micro-stutter, and it survived
+        every frame-timing fix because frame timing was never the problem.
+
+        It is worst precisely at a crisp speed. At 100 px/s on a 100Hz panel
+        the accumulator sits exactly on integer boundaries, so sub-millisecond
+        jitter flips it either way and the motion beats at around 50Hz.
+
+        Stepping per frame is only correct because SwapOnVSync blocks until
+        the panel has taken the frame, which makes the frame count a truer
+        clock than time.time(). Before the swap was locked to vsync this would
+        have run at whatever speed the loop happened to spin at.
+        """
+        self.fixed_pixels_per_frame = int(pixels) if pixels else None
+        self.logger.debug("Fixed step set to: %s px/frame",
+                          self.fixed_pixels_per_frame)
+
     def set_target_fps(self, fps: float) -> None:
         """
         Set the target frames per second for scrolling.

src/common/sports_scroll.py

@@ -43,6 +43,7 @@ from typing import Any, Dict, List, Optional
 
 from PIL import Image
 
+from src.common import scroll_config
 from src.common.scroll_helper import ScrollHelper
 
 logger = logging.getLogger(__name__)
@@ -58,13 +59,9 @@ DEFAULT_SCROLL_SETTINGS: Dict[str, Any] = {
     "dynamic_duration": True,
 }
 
-#: Bounds on the px/second -> px/frame conversion, applied before the helper
-#: sees the value. FPS is *not* clamped here — ScrollHelper.set_target_fps
-#: already does that, and a second copy of the range would drift from it.
-MIN_PIXELS_PER_FRAME = 0.1
-MAX_PIXELS_PER_FRAME = 5.0
-
 #: Pacing to assume when scroll_delay is 0, i.e. the plugin has not set one.
+#: Only used to interpret this module's own px/frame config shape; the speed
+#: bounds and the px/s -> px/frame conversion belong to scroll_config now.
 ASSUMED_FPS_WHEN_UNPACED = 100.0
 
 
@@ -236,51 +233,75 @@ class SportsScrollDisplay:
         """Apply config to the scroll helper. Safe to call again after a change."""
         settings = self._get_scroll_settings()
 
-        scroll_speed = self._coerce_float(settings.get("scroll_speed"), 50.0)
-        scroll_delay = self._coerce_float(settings.get("scroll_delay"), 0.01)
         dynamic_duration = bool(settings.get("dynamic_duration", True))
-
-        self.scroll_helper.set_scroll_delay(scroll_delay)
         self.scroll_helper.set_dynamic_duration_settings(
             enabled=dynamic_duration,
             min_duration=settings.get("min_duration", 30),
             max_duration=settings.get("max_duration", 600),
             buffer=0.2,  # ensure the strip clears the panel completely
         )
-        # Frame-based scrolling: motion advances per rendered frame rather than
-        # per wall-clock second, which is what makes the pacing stable.
-        self.scroll_helper.set_frame_based_scrolling(True)
 
-        # Config states speed in px/second; frame-based mode wants px/frame.
-        if scroll_delay > 0:
-            pixels_per_frame = scroll_speed * scroll_delay
-        else:
-            pixels_per_frame = scroll_speed / ASSUMED_FPS_WHEN_UNPACED
-        pixels_per_frame = max(
-            MIN_PIXELS_PER_FRAME, min(MAX_PIXELS_PER_FRAME, pixels_per_frame)
-        )
-        self.scroll_helper.set_scroll_speed(pixels_per_frame)
+        # Speed goes through scroll_config, which every other scrolling plugin
+        # already uses. What must NOT happen is handing it this module's
+        # 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.
+        #   scroll_config: scroll_speed is px per STEP, so px/s = speed/delay.
+        #
+        # Passing {"scroll_speed": 50.0, "scroll_delay": 0.01} straight through
+        # resolves to 5000 px/s (clamped to 500) instead of 50. So this module
+        # keeps ownership of reading its own config -- _get_scroll_settings
+        # merges the league overrides -- and hands the resolver a plain px/s.
+        pixels_per_second = self._resolve_pixels_per_second(settings)
 
-        effective_pps = (
-            pixels_per_frame / scroll_delay
-            if scroll_delay > 0
-            else pixels_per_frame * ASSUMED_FPS_WHEN_UNPACED
+        resolved = scroll_config.configure(
+            self.scroll_helper,
+            plugin_config=None,
+            global_config=self.global_config,
+            default_pixels_per_second=pixels_per_second,
+            display_manager=self.display_manager,
+            plugin_logger=self.logger,
+            refresh_hz=self._resolve_refresh_hz(),
         )
+        self._scroll_settings = resolved
         self.logger.info(
-            f"ScrollHelper configured: {pixels_per_frame:.2f} px/frame, "
-            f"delay={scroll_delay}s (effective {effective_pps:.1f} px/s from "
-            f"{scroll_speed} px/s config), dynamic_duration={dynamic_duration}"
+            "ScrollHelper configured: %s (requested %.1f px/s), "
+            "dynamic_duration=%s",
+            resolved.describe(),
+            pixels_per_second, dynamic_duration,
         )
 
-        # The reason this module exists upstream: the bundled copies hardcode
-        # ~100 FPS via scroll_delay and never consult the global target.
-        # No hasattr guard here, unlike the plugin copies: they probe because
-        # they may run against an older core, whereas this module ships in the
-        # same release as the ScrollHelper it calls. The helper clamps.
-        target_fps = self._resolve_target_fps()
-        if target_fps:
-            self.scroll_helper.set_target_fps(target_fps)
-            self.logger.info(f"Target FPS set to {target_fps}")
+    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 = self._coerce_float(settings.get("scroll_speed"), 50.0)
+        scroll_delay = self._coerce_float(settings.get("scroll_delay"), 0.01)
+        if scroll_delay <= 0:
+            return scroll_speed * ASSUMED_FPS_WHEN_UNPACED
+        return scroll_speed
+
+    def _resolve_refresh_hz(self) -> Optional[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.
+        """
+        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
+
+    def _scroll_frame_hold(self) -> int:
+        """Refreshes to hold each frame for, from the resolved settings."""
+        return getattr(getattr(self, "_scroll_settings", None), "frame_hold", 1)
 
     # ------------------------------------------------------------------
     # Frame pumping
@@ -305,6 +326,16 @@ class SportsScrollDisplay:
             if not visible:
                 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.
+            if hasattr(self.display_manager, "set_scrolling_state"):
+                self.display_manager.set_scrolling_state(
+                    True, frame_hold=self._scroll_frame_hold())
+
             self.display_manager.image = visible
             self.display_manager.update_display()
             self._frame_count += 1
@@ -331,7 +362,19 @@ class SportsScrollDisplay:
 
     def is_scroll_complete(self) -> bool:
         """True when the strip has scrolled fully past the panel."""
-        return self.scroll_helper.is_scroll_complete()
+        complete = self.scroll_helper.is_scroll_complete()
+        if complete:
+            self._release_scrolling_state()
+        return complete
+
+    def _release_scrolling_state(self) -> None:
+        """Tell the core this display is no longer scrolling.
+
+        The scrolling flag and the frame hold are global to the display
+        manager, so leaving them set holds every other plugin's frames too.
+        """
+        if hasattr(self.display_manager, "set_scrolling_state"):
+            self.display_manager.set_scrolling_state(False)
 
     def reset_scroll(self) -> None:
         """Return the strip to its starting position, keeping the content."""
@@ -349,6 +392,7 @@ class SportsScrollDisplay:
         self._vegas_content_items = []
         self._is_scrolling = False
         self._scroll_start_time = None
+        self._release_scrolling_state()
         self.logger.debug("Scroll display cleared")
 
     # ------------------------------------------------------------------

src/display_manager.py

@@ -1436,6 +1436,14 @@ class DisplayManager:
         # If we've been inactive for the threshold period, consider it not scrolling
         if current_time - self._scrolling_state['last_scroll_activity'] > self._scrolling_state['scroll_inactivity_threshold']:
             self._scrolling_state['is_scrolling'] = False
+            # Drop the hold with the state, exactly as set_scrolling_state(False)
+            # does. This path is the one a scroll takes when it ends without
+            # saying so -- the rotation moves on mid-scroll, or the plugin is
+            # torn down -- and leaving the hold set there means every later
+            # plugin, scrolling or static, is presented at refresh/N until
+            # somebody calls set_scrolling_state(False). The hold must not
+            # outlive the scroll that asked for it, however that scroll ends.
+            self._frame_hold = 1
             return False
             
         return True

test/test_display_dirty_tracking.py

@@ -293,6 +293,29 @@ class TestFrameHoldLifetime:
         finally:
             dm.set_scrolling_state(False)
 
+    def test_a_scroll_that_times_out_does_not_strand_a_hold(self, dm):
+        """The hold must go when the state does, however the scroll ended.
+
+        set_scrolling_state(False) is the polite exit. The other one is
+        is_currently_scrolling() deciding, after scroll_inactivity_threshold
+        of silence, that the scroll is over -- which is what happens when the
+        rotation moves on mid-scroll or a plugin is torn down. That path used
+        to clear the flag and keep the hold, so every later plugin, scrolling
+        or static, was presented at refresh/N by whoever scrolled last.
+        """
+        dm.set_scrolling_state(True, frame_hold=5)
+        # Age the scroll past the inactivity threshold rather than sleeping.
+        dm._scrolling_state['last_scroll_activity'] -= (
+            dm._scrolling_state['scroll_inactivity_threshold'] + 1.0)
+
+        assert dm.is_currently_scrolling() is False
+        dm.draw.rectangle([0, 0, 5, 5], fill=(10, 10, 200))
+        with _SwapSpy(dm.matrix) as spy:
+            dm.update_display()
+        assert spy.last_frame_hold == 1, (
+            "a timed-out scroll left its frame hold behind; the next plugin "
+            "is being presented at a fraction of the refresh rate")
+
     def test_another_plugin_stopping_does_not_strand_a_hold(self, dm):
         dm.set_scrolling_state(True, frame_hold=3)
         dm.set_scrolling_state(False)          # some other plugin finishes

test/test_sports_scroll.py

@@ -21,8 +21,6 @@ sys.modules.setdefault("rgbmatrix", MagicMock())
 
 from src.common.sports_scroll import (  # noqa: E402
     DEFAULT_SCROLL_SETTINGS,
-    MAX_PIXELS_PER_FRAME,
-    MIN_PIXELS_PER_FRAME,
     SportsScrollDisplay,
     SportsScrollDisplayManager,
 )
@@ -187,29 +185,48 @@ class TestScrollSettings:
 # ---------------------------------------------------------------------------
 
 class TestConfigureScrollHelper:
-    def test_speed_is_converted_to_pixels_per_frame(self, build):
-        display = build({"nhl": {"scroll_settings": {
-            "scroll_speed": 100.0, "scroll_delay": 0.02}}})
-        # 100 px/s * 0.02 s/frame = 2 px/frame
-        display.scroll_helper.set_scroll_speed.assert_called_with(2.0)
+    """Pacing now comes from scroll_config, the same resolver every other
+    scrolling plugin uses. What this class guards is the translation into it,
+    because the two modules read identically-named keys differently."""
 
-    def test_conversion_is_clamped_low(self, build):
-        display = build({"nhl": {"scroll_settings": {
-            "scroll_speed": 0.001, "scroll_delay": 0.001}}})
-        display.scroll_helper.set_scroll_speed.assert_called_with(MIN_PIXELS_PER_FRAME)
+    def test_config_speed_is_pixels_per_second_not_pixels_per_step(self, build):
+        """The collision that makes a naive hand-off wrong by 1/scroll_delay.
 
-    def test_conversion_is_clamped_high(self, build):
+        sports config states scroll_speed in px/SECOND and uses scroll_delay
+        only to reach px/frame. scroll_config states it in px per STEP, so it
+        computes px/s as speed/delay. Handing this module's settings dict
+        straight to the resolver turns 50 px/s into 5000 px/s, which its own
+        bounds then clamp to 500 -- a tenfold speed-up on every scoreboard.
+        """
         display = build({"nhl": {"scroll_settings": {
-            "scroll_speed": 5000.0, "scroll_delay": 0.5}}})
-        display.scroll_helper.set_scroll_speed.assert_called_with(MAX_PIXELS_PER_FRAME)
+            "scroll_speed": 50.0, "scroll_delay": 0.01}}})
+        applied = display.scroll_helper.set_scroll_speed.call_args[0][0]
+        assert applied == pytest.approx(50.0, abs=1.0), (
+            f"applied {applied} px/s; 500 means the settings dict was passed "
+            "through to the resolver instead of a plain px/s")
 
-    def test_zero_delay_assumes_a_pacing_instead_of_dividing_by_zero(self, build):
-        display = build({"nhl": {"scroll_settings": {
-            "scroll_speed": 100.0, "scroll_delay": 0}}})
-        display.scroll_helper.set_scroll_speed.assert_called_with(1.0)
+    def test_speed_is_snapped_to_whole_pixel_motion(self, build):
+        """50 px/s on a 100Hz panel is half a pixel per refresh, which cannot
+        render as motion -- it alternates 0 and 1 px steps and judders. The
+        ladder keeps the speed and holds each frame for two refreshes."""
+        display = build()
+        assert display._scroll_settings.frame_hold == 2
+        assert display._scroll_settings.pixels_per_second == pytest.approx(50.0)
 
-    def test_frame_based_scrolling_is_enabled(self, build):
-        build().scroll_helper.set_frame_based_scrolling.assert_called_once_with(True)
+    def test_the_resolved_hold_is_what_gets_published(self, build):
+        display = build()
+        assert display._scroll_frame_hold() == 2
+
+    def test_hold_defaults_to_one_before_anything_is_resolved(self, display_manager):
+        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_dynamic_duration_settings_are_applied(self, build):
         display = build({"nhl": {"scroll_settings": {
@@ -219,32 +236,38 @@ class TestConfigureScrollHelper:
         assert kwargs["min_duration"] == 5
         assert kwargs["max_duration"] == 50
 
-    def test_target_fps_reaches_the_helper(self, build):
-        """The whole point of upstreaming: the bundled copies hardcode ~100 FPS
-        via scroll_delay and never consult the global target."""
-        display = build(global_config={"target_fps": 120})
-        display.scroll_helper.set_target_fps.assert_called_once_with(120.0)
+    def test_zero_delay_still_means_pixels_per_frame(self, build):
+        """The one case where scroll_speed is not already px/s."""
+        display = build({"nhl": {"scroll_settings": {
+            "scroll_speed": 1.0, "scroll_delay": 0}}})
+        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
 
     def test_legacy_key_is_honored(self, build):
         display = build(global_config={"scroll_target_fps": 90})
-        display.scroll_helper.set_target_fps.assert_called_once_with(90.0)
+        assert display._resolve_refresh_hz() == 90.0
 
     def test_modern_key_wins_over_legacy(self, build):
         display = build(global_config={"target_fps": 120, "scroll_target_fps": 90})
-        display.scroll_helper.set_target_fps.assert_called_once_with(120.0)
+        assert display._resolve_refresh_hz() == 120.0
 
-    def test_absent_target_fps_leaves_config_pacing_alone(self, build):
-        build().scroll_helper.set_target_fps.assert_not_called()
+    def test_configured_hardware_refresh_wins_over_the_fps_target(self, build):
+        display = build(global_config={
+            "target_fps": 60,
+            "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."""
         display = build(global_config={"target_fps": bad})
-        display.scroll_helper.set_target_fps.assert_not_called()
-
-    def test_string_digits_are_accepted(self, build):
-        display = build(global_config={"target_fps": "120"})
-        display.scroll_helper.set_target_fps.assert_called_once_with(120.0)
+        assert display._scroll_settings.pixels_per_second > 0
 
     @pytest.mark.parametrize("bad", [None, "fast", {}, []])
     def test_unusable_scroll_speed_degrades_instead_of_crashing(self, build, bad):
@@ -252,24 +275,63 @@ class TestConfigureScrollHelper:
         present with null reaches the arithmetic and raises inside __init__,
         taking the whole display down before it renders anything."""
         display = build({"nhl": {"scroll_settings": {"scroll_speed": bad}}})
-        # 50.0 px/s * 0.01 s/frame == 0.5 px/frame, i.e. the default speed.
-        display.scroll_helper.set_scroll_speed.assert_called_with(0.5)
+        applied = display.scroll_helper.set_scroll_speed.call_args[0][0]
+        assert applied == pytest.approx(50.0, abs=1.0)
 
     @pytest.mark.parametrize("bad", [None, "slow", {}])
     def test_unusable_scroll_delay_degrades_instead_of_crashing(self, build, bad):
         display = build({"nhl": {"scroll_settings": {"scroll_delay": bad}}})
-        display.scroll_helper.set_scroll_delay.assert_called_with(0.01)
+        applied = display.scroll_helper.set_scroll_speed.call_args[0][0]
+        assert applied == pytest.approx(50.0, abs=1.0)
 
     def test_numeric_strings_are_accepted(self, build):
         display = build({"nhl": {"scroll_settings": {
             "scroll_speed": "100", "scroll_delay": "0.02"}}})
-        display.scroll_helper.set_scroll_speed.assert_called_with(2.0)
+        applied = display.scroll_helper.set_scroll_speed.call_args[0][0]
+        assert applied == pytest.approx(100.0, abs=1.0)
+
+
+class TestScrollingStateIsPublished:
+    """The core has to be told, or the frame hold is never applied and
+    deferred work runs in the middle of the scroll."""
+
+    def _drawable(self, display):
+        display.scroll_helper.cached_image = Image.new("RGB", (400, 32))
+        display.scroll_helper.get_visible_portion.return_value = Image.new(
+            "RGB", (128, 32))
+
+    def test_a_drawn_frame_declares_the_hold(self, build, display_manager):
+        display = build()
+        self._drawable(display)
+        assert display.display_scroll_frame() is True
+        display_manager.set_scrolling_state.assert_called_with(
+            True, frame_hold=display._scroll_frame_hold())
+
+    def test_completion_releases_the_state(self, build, display_manager):
+        display = build()
+        display.scroll_helper.is_scroll_complete.return_value = True
+        assert display.is_scroll_complete() is True
+        display_manager.set_scrolling_state.assert_called_with(False)
+
+    def test_an_incomplete_scroll_does_not_release(self, build, display_manager):
+        display = build()
+        display.scroll_helper.is_scroll_complete.return_value = False
+        display.is_scroll_complete()
+        assert (False,) not in [c.args for c in
+                                display_manager.set_scrolling_state.call_args_list]
+
+    def test_clearing_releases_the_state(self, build, display_manager):
+        display = build()
+        display.clear()
+        display_manager.set_scrolling_state.assert_called_with(False)
+
+    def test_an_older_core_without_the_call_is_tolerated(self, build):
+        """Plugins ship independently of the core they run against."""
+        display = build()
+        del display.display_manager.set_scrolling_state
+        self._drawable(display)
+        assert display.display_scroll_frame() is True
 
-    def test_fps_clamping_is_left_to_the_helper(self, build):
-        """Deliberately not clamped here — a second copy of the range would
-        drift from ScrollHelper.set_target_fps."""
-        display = build(global_config={"target_fps": 5000})
-        display.scroll_helper.set_target_fps.assert_called_once_with(5000.0)
 
 
 # ---------------------------------------------------------------------------
@@ -429,7 +491,11 @@ class TestManager:
         never honoring target_fps."""
         child = manager.get_scroll_display("live")
         assert child.global_config == {"target_fps": 120}
-        child.scroll_helper.set_target_fps.assert_called_once_with(120.0)
+        # 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._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
@@ -551,10 +617,17 @@ class TestAgainstTheRealScrollHelper:
         )
 
     def test_configuration_lands_on_the_real_helper(self, real):
-        helper = real.get_scroll_display("live").scroll_helper
-        assert helper.target_fps == 120.0
-        assert helper.frame_based_scrolling is True
-        assert helper.scroll_speed == pytest.approx(0.5)  # 500 px/s * 0.001 s
+        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.
+        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.target_fps == pytest.approx(
+            120.0 / display._scroll_settings.frame_hold, abs=1.0)
 
     def test_a_strip_is_built_and_scrolls_to_completion(self, real):
         import time

test/test_whole_pixel_stepping.py

@@ -0,0 +1,199 @@
+"""Smooth motion is a per-frame property, not a frame-rate one.
+
+Frame-time statistics on this project have twice looked perfect while a
+marquee still visibly stuttered. They measure the wrong thing. What the eye
+judges is whether the strip advances the SAME number of whole pixels on every
+presented frame, and that is what these tests measure.
+
+The old stepping derived position from ``scroll_speed * delta_time`` and then
+truncated it to a pixel, so any jitter in delta_time landed either side of an
+integer boundary. Against the frame times measured on hardware -- a rock-steady
+100.0 fps whose individual frames still ranged 5.6ms to 15.2ms -- about 5.8% of
+frames advanced 0 or 2 pixels instead of 1. Roughly six hitches a second.
+"""
+import random
+from collections import Counter
+from unittest.mock import patch
+
+import numpy as np
+import pytest
+from PIL import Image
+
+from src.common import scroll_config
+from src.common.scroll_helper import ScrollHelper
+
+#: Wide enough that no test consumes the strip. A scroll that completes freezes
+#: at the end, and counting those frozen frames as "did not advance" is how the
+#: first version of this measurement talked itself into a wrong answer.
+STRIP_WIDTH = 60000
+
+
+class _Panel:
+    """A display manager that only has to answer the refresh question."""
+
+    refresh_hz = 100.0
+
+    def set_frame_hold(self, refreshes):
+        """Recorded so a test can assert on it; the panel is not involved."""
+        self.hold = refreshes
+
+
+def _helper():
+    """A helper with a strip long enough that no sample can consume it."""
+    helper = ScrollHelper(128, 64)
+    helper.cached_image = Image.new("RGB", (STRIP_WIDTH, 64))
+    helper.cached_array = np.zeros((64, STRIP_WIDTH, 3), dtype=np.uint8)
+    helper.total_scroll_width = STRIP_WIDTH
+    return helper
+
+
+def _measured_frame_times(count, seed=7):
+    """Frame gaps shaped like the ones logged on hardware.
+
+    100.0 fps overall and a 10.00ms median, with the short and long tails that
+    a real vsync-paced loop actually produces.
+    """
+    rng = random.Random(seed)
+    out = []
+    for _ in range(count):
+        roll = rng.random()
+        if roll < 0.04:
+            out.append(rng.uniform(5.6, 7.5))
+        elif roll < 0.08:
+            out.append(rng.uniform(13.0, 15.2))
+        else:
+            out.append(rng.gauss(10.0, 0.35))
+    return out
+
+
+def _advances(helper, frame_times):
+    """Histogram of whole-pixel movement per presented frame.
+
+    The first call is primed and discarded. update_scroll_position sets
+    last_update_time on its way through, so the very first call sees a
+    delta_time of zero and moves nothing in time-based mode. Counting that
+    synthetic frame put a guaranteed zero in every histogram, which was enough
+    on its own to satisfy "time-based stepping produces uneven motion" -- the
+    pin below would have passed against perfectly uniform motion.
+    """
+    counts, now = Counter(), 1000.0
+    with patch("src.common.scroll_helper.time.time") as clock:
+        clock.return_value = now
+        helper.update_scroll_position()
+        last = int(helper.scroll_position)
+        for gap_ms in frame_times:
+            now += gap_ms / 1000.0
+            clock.return_value = now
+            helper.update_scroll_position()
+            position = int(helper.scroll_position)
+            counts[position - last] += 1
+            last = position
+    assert not helper.is_scroll_complete(), (
+        "the strip ran out mid-measurement; a finished scroll freezes and its "
+        "frozen frames read as zero-advance")
+    return counts
+
+
+class TestWholePixelStepping:
+    def test_a_crisp_speed_moves_the_same_pixels_every_frame(self):
+        """The whole point: identical movement on every frame, jitter or not."""
+        helper = _helper()
+        scroll_config.configure(helper, plugin_config=None,
+                                default_pixels_per_second=100.0,
+                                display_manager=_Panel(), refresh_hz=100.0)
+        counts = _advances(helper, _measured_frame_times(3000))
+        assert set(counts) == {1}, (
+            "frames advanced %s px; at a crisp speed every frame must move the "
+            "same whole number, or the motion beats against the frame rate"
+            % sorted(counts))
+
+    def test_the_wall_clock_is_not_consulted_at_all(self):
+        """Jitter must not reach the position, however violent."""
+        helper = _helper()
+        scroll_config.configure(helper, plugin_config=None,
+                                default_pixels_per_second=100.0,
+                                display_manager=_Panel(), refresh_hz=100.0)
+        savage = [1.0, 40.0, 2.0, 25.0, 0.5, 60.0] * 200
+        assert set(_advances(helper, savage)) == {1}
+
+    def test_two_pixels_per_frame_is_also_uniform(self):
+        """Uniformity is not special to one pixel; 200 px/s steps by two."""
+        helper = _helper()
+        scroll_config.configure(helper, plugin_config=None,
+                                default_pixels_per_second=200.0,
+                                display_manager=_Panel(), refresh_hz=100.0)
+        assert helper.fixed_pixels_per_frame == 2
+        assert set(_advances(helper, _measured_frame_times(2000))) == {2}
+
+    def test_time_based_stepping_is_what_it_replaces(self):
+        """Pin the defect, so a revert cannot pass this file quietly."""
+        helper = _helper()
+        helper.set_scroll_speed(100.0)          # the old path: px/s off the clock
+        assert helper.fixed_pixels_per_frame is None
+        counts = _advances(helper, _measured_frame_times(3000))
+        total = sum(counts.values())
+        uneven = sum(n for px, n in counts.items() if px != 1)
+
+        # A proportion, not "more than zero". Against these frame times the
+        # old path misses roughly one frame in twenty; 1% is comfortably below
+        # that and still far above anything a stray frame could produce, so
+        # this fails if the defect is genuinely gone rather than merely rare.
+        assert uneven > total * 0.01, (
+            "time-based stepping produced only %d uneven frames in %d against "
+            "real measured frame times. If that is genuinely fixed elsewhere, "
+            "this test and the fixed-step mode both deserve re-examining"
+            % (uneven, total))
+
+
+class TestItStaysOptIn:
+    def test_a_speed_that_is_not_crisp_keeps_pacing_off_time(self):
+        """Without a whole-pixel step to take, elapsed time is still the best
+        available answer -- fixed stepping is opt-in, not a global switch."""
+        helper = _helper()
+        scroll_config.configure(helper, plugin_config=None,
+                                default_pixels_per_second=100.0,
+                                display_manager=_Panel(), refresh_hz=100.0,
+                                snap_to_crisp=False)
+        assert helper.fixed_pixels_per_frame is None
+
+    def test_setting_a_speed_directly_drops_the_fixed_step(self):
+        """Otherwise the new speed is silently ignored."""
+        helper = _helper()
+        scroll_config.configure(helper, plugin_config=None,
+                                default_pixels_per_second=100.0,
+                                display_manager=_Panel(), refresh_hz=100.0)
+        assert helper.fixed_pixels_per_frame == 1
+        helper.set_scroll_speed(37.0)
+        assert helper.fixed_pixels_per_frame is None
+        assert helper.scroll_speed == 37.0
+
+    @pytest.mark.parametrize("value", [None, 0])
+    def test_clearing_the_step_restores_time_based_motion(self, value):
+        """Zero and None both mean "no fixed step", not "advance zero pixels"."""
+        helper = _helper()
+        helper.set_pixels_per_frame(3)
+        helper.set_pixels_per_frame(value)
+        assert helper.fixed_pixels_per_frame is None
+
+    def test_an_older_core_without_the_setter_still_configures(self):
+        """Plugins ship independently of the core they run against."""
+        class Old:
+            """A helper from before set_pixels_per_frame existed."""
+
+            scroll_speed = None
+
+            def set_scroll_speed(self, v):
+                """The one call this old helper does understand."""
+                self.scroll_speed = v
+
+            def set_frame_based_scrolling(self, v):
+                """Accepted and ignored; the mode is not what is under test."""
+
+            def set_target_fps(self, v):
+                """Accepted and ignored; the rate is not what is under test."""
+
+        old = Old()
+        scroll_config.configure(old, plugin_config=None,
+                                default_pixels_per_second=100.0,
+                                display_manager=_Panel(), refresh_hz=100.0)
+        assert old.scroll_speed == 100.0