fix(scroll): tie the frame hold to the scroll, not the plugin

The hold applied in configure() never reached the panel. Plugins share one
display manager, and set_scrolling_state(False) -- fired whenever ANY other
plugin finishes its scroll -- reset the hold to 1. A hold set once at plugin
construction was therefore always gone by the time that plugin rendered.

The symptom was a log line that lied. ledmatrix-stocks reported

    Scroll configured: 50.0 px/s (1px every 2 refreshes = 50.0 fps, smooth)

while the panel measured 100.0 fps, median 10.00ms. Config, resolution and
snapping were all correct; only the pacing silently was not applied.

set_scrolling_state(is_scrolling, frame_hold=1) now carries it, so the hold
lives exactly as long as the scroll that asked for it. configure() reports the
value as ScrollSettings.frame_hold instead of applying it -- applying it behind
the caller's back could never have been right on a shared display manager.
Existing callers are unaffected; the default keeps one frame per refresh.

Verified on hardware: stocks at 50 px/s now measures

    50.0 fps over 251 frames | median 20.00ms p95 20.09ms | stalls 0 skips 0

20.00ms being exactly two refreshes, with the panel still refreshing at 100Hz
underneath so flicker is unchanged.

test_another_plugin_stopping_does_not_strand_a_hold pins the interaction that
broke this.

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

src/common/scroll_config.py

@@ -196,6 +196,11 @@ class ScrollSettings:
     #: What the config asked for, before snapping.
     requested_pixels_per_second: Optional[float] = None
 
+    @property
+    def frame_hold(self) -> int:
+        """Refreshes to hold each frame for; pass to set_scrolling_state()."""
+        return self.crisp.frame_hold if self.crisp else 1
+
     def describe(self) -> str:
         text = f"{self.pixels_per_second:.1f} px/s (from {self.source})"
         if self.pixels_per_frame is not None:
@@ -345,7 +350,13 @@ def configure(
         refresh_hz=refresh_hz,
     )
 
-    hz = _coerce(refresh_hz) or settings.target_fps or DEFAULT_REFRESH_HZ
+    # Refresh rate, most authoritative first: what the caller passed, then the
+    # display manager (which can see display.hardware; a plugin cannot), then
+    # whatever resolve() inferred, then the default.
+    hz = _coerce(refresh_hz)
+    if hz is None and display_manager is not None:
+        hz = _coerce(getattr(display_manager, "refresh_hz", None))
+    hz = hz or settings.target_fps or DEFAULT_REFRESH_HZ
     applied = settings.pixels_per_second
     choice = None
 
@@ -371,8 +382,11 @@ def configure(
     elif settings.target_fps and hasattr(scroll_helper, "set_target_fps"):
         scroll_helper.set_target_fps(settings.target_fps)
 
-    if choice and display_manager is not None and hasattr(display_manager, "set_frame_hold"):
-        display_manager.set_frame_hold(choice.frame_hold)
+    # Deliberately NOT applied here. The hold belongs to a scroll, not to a
+    # plugin's lifetime: plugins share one display manager, and one left set at
+    # construction is reset the moment any other plugin finishes scrolling.
+    # Callers pass settings.frame_hold to set_scrolling_state(True, ...) when
+    # they start scrolling. configure() only reports what is needed.
 
     if choice:
         requested = settings.requested_pixels_per_second
@@ -385,12 +399,10 @@ def configure(
         else:
             log.info("Scroll configured: %s (from %s)",
                      choice.describe(), settings.source)
-        if choice.frame_hold > 1 and (
-            display_manager is None or not hasattr(display_manager, "set_frame_hold")
-        ):
-            log.warning(
-                "Scroll wants a frame hold of %d but no display manager was "
-                "given to apply it; motion will use fractional pixels and judder",
+        if choice.frame_hold > 1:
+            log.debug(
+                "Scroll needs a frame hold of %d - pass settings.frame_hold to "
+                "display_manager.set_scrolling_state(True, ...) each scroll",
                 choice.frame_hold,
             )
     else:

src/display_manager.py

@@ -1301,6 +1301,26 @@ class DisplayManager:
         
         return dt.strftime(f"%b %-d{suffix}") 
 
+    @property
+    def refresh_hz(self) -> float:
+        """The panel's refresh rate in Hz, from the hardware config.
+
+        The authoritative place to ask, because a plugin only receives its own
+        config section and cannot see display.hardware. Scroll pacing needs
+        this: the speeds a panel can show in whole pixels are refresh_hz
+        divided by the frame hold, so getting it wrong silently produces
+        fractional-pixel motion. See src/common/scroll_config.py.
+
+        Note this is the configured *cap*, not necessarily what the panel
+        achieves -- scripts/scroll_speeds.py --measure reports the real rate.
+        """
+        hardware = (self.config.get('display') or {}).get('hardware') or {}
+        try:
+            value = float(hardware.get('limit_refresh_rate_hz') or 0)
+        except (TypeError, ValueError):
+            value = 0.0
+        return value if value > 0 else 100.0
+
     def set_frame_hold(self, refreshes: int) -> None:
         """Hold each pushed frame for this many panel refreshes (>=1).
 
@@ -1318,14 +1338,27 @@ class DisplayManager:
             return
         self._frame_hold = max(1, min(255, value))
 
-    def set_scrolling_state(self, is_scrolling: bool):
-        """Set the current scrolling state. Call this when a display starts/stops scrolling."""
+    def set_scrolling_state(self, is_scrolling: bool, frame_hold: int = 1):
+        """Set the current scrolling state, and this scroll's frame pacing.
+
+        Call this when a display starts or stops scrolling. ``frame_hold`` is
+        how many panel refreshes each frame is held for -- 2 gives one whole
+        pixel every second refresh, which is how a scroll runs at half the
+        refresh rate without fractional pixel positions.
+
+        The hold is set here rather than once at plugin construction because
+        it must not outlive the scroll that asked for it: plugins share one
+        display manager, so a hold left set by whoever scrolled last would
+        silently re-pace the next plugin. Passing it alongside the state makes
+        the lifetime exactly the scroll, and the default of 1 means any caller
+        that does not care gets a new frame every refresh.
+        """
         current_time = time.time()
         self._scrolling_state['is_scrolling'] = is_scrolling
         if is_scrolling:
             self._scrolling_state['last_scroll_activity'] = current_time
+            self.set_frame_hold(frame_hold)
         else:
-            # Whatever pacing the finished scroll asked for must not carry over.
             self._frame_hold = 1
         logger.debug(f"Scrolling state set to: {is_scrolling}")
 

test/test_display_dirty_tracking.py

@@ -272,3 +272,43 @@ class TestFrameHold:
     def test_unusable_holds_are_ignored_or_floored(self, dm, bad, expected):
         dm.set_frame_hold(bad)
         assert dm._frame_hold == expected
+
+
+class TestFrameHoldLifetime:
+    """The hold must last exactly as long as the scroll that asked for it.
+
+    Plugins share one display manager. A hold applied at plugin construction is
+    wiped the moment any *other* plugin finishes scrolling, so by the time the
+    first plugin renders it is back to one pixel per refresh -- the speed reads
+    correct in the log and is wrong on the panel.
+    """
+
+    def test_scrolling_state_carries_the_hold(self, dm):
+        dm.set_scrolling_state(True, frame_hold=4)
+        try:
+            dm.draw.rectangle([0, 0, 7, 7], fill=(10, 200, 10))
+            with _SwapSpy(dm.matrix) as spy:
+                dm.update_display()
+            assert spy.last_frame_hold == 4
+        finally:
+            dm.set_scrolling_state(False)
+
+    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
+        dm.set_scrolling_state(True)           # a plugin that wants no hold
+        try:
+            dm.draw.rectangle([0, 0, 6, 6], fill=(200, 10, 10))
+            with _SwapSpy(dm.matrix) as spy:
+                dm.update_display()
+            assert spy.last_frame_hold == 1
+        finally:
+            dm.set_scrolling_state(False)
+
+    def test_default_keeps_previous_behaviour(self, dm):
+        """Callers that never heard of frame holds get one frame per refresh."""
+        dm.set_scrolling_state(True)
+        try:
+            assert dm._frame_hold == 1
+        finally:
+            dm.set_scrolling_state(False)

test/test_scroll_config.py

@@ -217,24 +217,25 @@ class TestConfigure:
         assert settings.warning is None
         assert not [r for r in caplog.records if "judder" in r.getMessage()]
 
-    def test_warns_when_a_hold_is_needed_but_cannot_be_applied(self, caplog):
-        """Without a display manager the hold silently does not happen, and the
-        motion falls back to fractional pixels. That must not pass quietly."""
-        with caplog.at_level(logging.WARNING):
-            configure(FakeHelper(), {"display_options": {"scroll_pixels_per_second": 50.0}})
-        assert any("frame hold" in r.getMessage() for r in caplog.records)
-
-    def test_applies_the_frame_hold_to_the_display_manager(self):
+    def test_reports_the_hold_without_applying_it(self):
+        """The hold belongs to a scroll, not a plugin's lifetime -- one left set
+        at construction is wiped as soon as any other plugin stops scrolling.
+        configure() reports it; the caller passes it to set_scrolling_state."""
         dm = FakeDisplayManager()
-        configure(FakeHelper(), {"display_options": {"scroll_pixels_per_second": 25.0}},
+        s = configure(FakeHelper(), {"display_options": {"scroll_pixels_per_second": 25.0}},
                       display_manager=dm)
-        assert dm.hold == 4, "25px/s at 100Hz is 1px every 4th refresh"
+        assert s.frame_hold == 4, "25px/s at 100Hz is 1px every 4th refresh"
+        assert dm.hold is None, "must not apply the hold behind the caller's back"
 
     def test_full_speed_needs_no_hold(self):
-        dm = FakeDisplayManager()
-        configure(FakeHelper(), {"display_options": {"scroll_pixels_per_second": 100.0}},
-                  display_manager=dm)
-        assert dm.hold == 1
+        s = configure(FakeHelper(), {"display_options": {"scroll_pixels_per_second": 100.0}},
+                      display_manager=FakeDisplayManager())
+        assert s.frame_hold == 1
+
+    def test_frame_hold_is_one_when_snapping_is_off(self):
+        s = configure(FakeHelper(), {"display_options": {"scroll_pixels_per_second": 50.0}},
+                      snap_to_crisp=False)
+        assert s.frame_hold == 1
 
     def test_snaps_to_the_nearest_crisp_speed_and_reports_both(self):
         s = configure(FakeHelper(), {"display_options": {"scroll_pixels_per_second": 45.0}},
@@ -350,3 +351,40 @@ class TestSolveCrisp:
     def test_steppiness_labels_are_sane(self):
         assert solve_crisp(100, 100).steppiness == "smooth"
         assert crisp_ladder(100)[0].steppiness == "stepped"
+
+
+class TestRefreshFromDisplayManager:
+    """A plugin sees only its own config section, so the display manager is the
+    authoritative source for the panel's refresh rate."""
+
+    class DM:
+        def __init__(self, hz):
+            self.refresh_hz = hz
+            self.hold = None
+
+        def set_frame_hold(self, refreshes):
+            self.hold = refreshes
+
+    def test_uses_the_display_manager_rate(self):
+        dm = self.DM(60.0)
+        s = configure(FakeHelper(), {"display_options": {"scroll_pixels_per_second": 30.0}},
+                      display_manager=dm)
+        # 30px/s on a 60Hz panel is 1px every 2nd refresh, exactly.
+        assert s.pixels_per_second == pytest.approx(30.0)
+        assert s.frame_hold == 2
+
+    def test_explicit_refresh_hz_wins_over_the_display_manager(self):
+        dm = self.DM(60.0)
+        s = configure(FakeHelper(), {"display_options": {"scroll_pixels_per_second": 25.0}},
+                      display_manager=dm, refresh_hz=100)
+        assert s.frame_hold == 4, "25px/s at 100Hz is 1px every 4th refresh"
+
+    def test_missing_attribute_falls_back_to_the_default(self):
+        class Bare:
+            def set_frame_hold(self, refreshes):
+                self.hold = refreshes
+
+        bare = Bare()
+        s = configure(FakeHelper(), {"display_options": {"scroll_pixels_per_second": 50.0}},
+                      display_manager=bare)
+        assert s.frame_hold == 2, "assumed 100Hz"