diff --git a/src/common/scroll_config.py b/src/common/scroll_config.py index 75d232d9..2c11d5d0 100644 --- a/src/common/scroll_config.py +++ b/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: diff --git a/src/display_manager.py b/src/display_manager.py index c54b9772..ed0811a5 100644 --- a/src/display_manager.py +++ b/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}") diff --git a/test/test_display_dirty_tracking.py b/test/test_display_dirty_tracking.py index 0c63d5c3..cf33d12b 100644 --- a/test/test_display_dirty_tracking.py +++ b/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) diff --git a/test/test_scroll_config.py b/test/test_scroll_config.py index ca8f4614..0c7f7ff2 100644 --- a/test/test_scroll_config.py +++ b/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}}, - display_manager=dm) - assert dm.hold == 4, "25px/s at 100Hz is 1px every 4th refresh" + s = configure(FakeHelper(), {"display_options": {"scroll_pixels_per_second": 25.0}}, + display_manager=dm) + 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"