refactor(sports): put the scoreboards on the shared scroll resolver

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/sports_scroll.py

@@ -43,6 +43,7 @@ class HockeyScrollDisplayManager(SportsScrollDisplayManager):
 
 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 @@ class HockeyScrollDisplayManager(SportsScrollDisplayManager):
  "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 @@ def _configure_scroll_helper(self) -> None:
  """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)
-
- effective_pps = (
- pixels_per_frame / scroll_delay
- if scroll_delay > 0
- else pixels_per_frame * ASSUMED_FPS_WHEN_UNPACED
+ # 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)
+
+ 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 @@ def display_scroll_frame(self) -> bool:
  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 @@ def _log_scroll_progress(self) -> None:
 
  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 @@ def clear(self) -> None:
  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 @@ def is_currently_scrolling(self) -> bool:
  # 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