fix(display): a non-numeric plugin duration no longer stops the display; narrow scroll strips no longer raise

CHANGELOG.md

@@ -514,6 +514,25 @@ policies are unchanged.
   for the plugin is refused (`plugin-reloading`), and a config reconcile
   neither loads it twice nor unloads it mid-load. A Vegas fetch that waited
   out a reload for the lock skips the old instance.
+- A plugin display duration that is not a number no longer stops the
+  display. Several plugins (clock-simple, calendar, countdown) return their
+  `display_duration` setting as it is in config.json, so a value saved as
+  `"20"` or `null` (the raw config editor, a hand edit) reached the run loop
+  as a string or None. Comparing it with 0 raised a TypeError that no
+  handler in the loop caught: the display service exited when that plugin's
+  screen came up, and systemd restarted it into the same crash. The
+  controller now reads the plugin's answer as a number: a numeric string
+  counts, and anything else (or a `get_display_duration()` that raises)
+  shows the mode for 30 s, with one warning per plugin.
+- A scroll strip narrower than the panel scrolls instead of raising on every
+  frame. When a frame ran off the end of the strip, `ScrollHelper` copied
+  the strip's tail and then the rest of the frame from its head, which
+  assumed the head was that wide; for a narrower strip that raised
+  `ValueError: could not broadcast` at every position, so nothing was drawn
+  and each frame logged a traceback. Vegas builds such a strip, with no
+  lead-in, when its content is narrower than the chain. A frame that runs
+  off the strip now continues from its head column by column, so a narrow
+  strip repeats across the panel; a wide strip wraps exactly as before.
 - The schedule-off blank and the WiFi notice no longer start with a
   scroller's leftovers. Both are drawn by the display controller rather than
   dispatched to a plugin, so #716's handover never reached them: drawn while

src/common/scroll_helper.py

@@ -561,17 +561,18 @@ def _blend_visible_portion(self, start_x: int, fractional: float) -> Image.Image
         width = self.display_width
         strip_width = self.cached_array.shape[1]
 
-        if start_x + width + 1 <= strip_width:
+        if 0 <= start_x and start_x + width + 1 <= strip_width:
             # Slice the backing array directly. Going via
             # _get_visible_portion_integer would build two PIL images only for
             # them to be converted straight back to arrays, which measured 15x
             # the cost of the integer path.
             near = self.cached_array[:, start_x:start_x + width]
             far = self.cached_array[:, start_x + 1:start_x + 1 + width]
         else:
-            # Close enough to the end that one of the slices wraps; let the
-            # integer path handle that and pay the conversion. Continuous mode
-            # extends the strip before reaching here, so this is the rare case.
+            # One of the slices wraps (close to the end, or a strip narrower
+            # than the panel); let the integer path handle that and pay the
+            # conversion. Continuous mode extends the strip before reaching
+            # here, so this is the rare case.
             near = np.asarray(
                 self._get_visible_portion_integer(start_x, start_x + width))
             far = np.asarray(
@@ -601,34 +602,33 @@ def _get_visible_portion_integer(self, start_x: int, end_x: int) -> Image.Image:
         _size = (self.display_width, self.display_height)
         img_w = self.cached_array.shape[1]
 
-        if end_x <= img_w:
+        if 0 <= start_x and end_x <= img_w:
             # Normal case: single contiguous slice (fastest path). tobytes()
             # on the column-slice view already returns C-order bytes, so
             # ascontiguousarray() first only added a second full-frame copy.
             return Image.frombytes(
                 'RGB', _size,
                 self.cached_array[:, start_x:end_x].tobytes())
+
+        # Ensure frame buffer is allocated for all non-simple paths
+        if self._frame_buffer is None or self._frame_buffer.shape != (self.display_height, self.display_width, 3):
+            self._frame_buffer = np.zeros((self.display_height, self.display_width, 3), dtype=np.uint8)
+
+        if img_w == 0:
+            self._frame_buffer[:] = 0
         else:
-            # Ensure frame buffer is allocated for all non-simple paths
-            if self._frame_buffer is None or self._frame_buffer.shape != (self.display_height, self.display_width, 3):
-                self._frame_buffer = np.zeros((self.display_height, self.display_width, 3), dtype=np.uint8)
-
-            width1 = img_w - start_x
-            if width1 > 0:
-                # Wrap-around: tail of image + head of image
-                self._frame_buffer[:, :width1] = self.cached_array[:, start_x:]
-                remaining_width = self.display_width - width1
-                self._frame_buffer[:, width1:] = self.cached_array[:, :remaining_width]
-            else:
-                # Edge case: start_x at or past image end — show from beginning,
-                # clamped to available width (scroll_position should wrap before
-                # reaching this state in normal operation).
-                available = min(self.display_width, img_w)
-                self._frame_buffer[:, :available] = self.cached_array[:, :available]
-                if available < self.display_width:
-                    self._frame_buffer[:, available:] = 0
-
-            return Image.frombytes('RGB', _size, self._frame_buffer.tobytes())
+            # The frame runs off the strip, so it carries on from the head:
+            # frame column j is strip column (start_x + j) modulo the strip's
+            # width -- the tail and then the head, and a strip narrower than
+            # the panel repeated across it. Copying the tail and then the rest
+            # of the frame from the head assumed the head was that wide, and
+            # raised at every position for a strip narrower than the panel
+            # (Vegas composes one, with no lead-in, when its content is
+            # narrower than the chain).
+            np.take(self.cached_array, np.arange(start_x, end_x), axis=1,
+                    mode='wrap', out=self._frame_buffer)
+
+        return Image.frombytes('RGB', _size, self._frame_buffer.tobytes())
 
     def calculate_dynamic_duration(self) -> int:
         """

src/display_controller.py

@@ -25,11 +25,12 @@
 import inspect
 import signal
 import json
+import math
 import threading
 import types
 from collections import deque
 from contextlib import contextmanager
-from typing import Dict, Any, List, Optional, Callable, Set, Tuple
+from typing import Dict, Any, FrozenSet, List, Optional, Callable, Set, Tuple
 from datetime import datetime
 from concurrent.futures import ThreadPoolExecutor, as_completed  # pylint: disable=no-name-in-module
 import pytz
@@ -89,6 +90,19 @@
 DEFAULT_DYNAMIC_DURATION_CAP = 180.0
 
 
+def _finite_seconds(value: Any) -> Optional[float]:
+    """``value`` as seconds when it is a finite number or a numeric string,
+    else None. A bool is not a number here, though it is an int: True would
+    read as a one-second screen."""
+    if isinstance(value, bool):
+        return None
+    try:
+        seconds = float(value)
+    except (TypeError, ValueError, OverflowError):
+        return None
+    return seconds if math.isfinite(seconds) else None
+
+
 class _PluginReloadJob:
     """A ``plugin.reload`` whose slow half runs off the render thread.
 
@@ -1340,22 +1354,53 @@ def _note_empty_pass(self) -> None:
                         "until one does", self.EMPTY_ROTATION_PAUSE)
         self._sleep_with_plugin_updates(self.EMPTY_ROTATION_PAUSE)
 
+    #: Plugins already warned about a display duration that is not a number,
+    #: so a bad setting logs once, not at every one of its screens. A
+    #: frozenset, replaced rather than mutated; class-level default for
+    #: controllers built without __init__ (tests).
+    _duration_warned: FrozenSet[str] = frozenset()
+
     def _get_display_duration(self, mode_key):
         """Seconds to show a mode: the Rotation & Durations page's value for it
         (display.display_durations), else the plugin's own duration.
 
         The saved value has to win. Every plugin inherits
         get_display_duration(), so checking the plugin first meant the page's
         values were never read.
+
+        The plugin's answer is checked here, not trusted. Several plugins
+        return their display_duration setting straight from config.json, so
+        one saved as "20" or null (the raw config editor, a hand edit) came
+        back as a string or None; _resolve_durations compared it with 0, and
+        the TypeError went past every handler in the loop and stopped the
+        display service, which systemd restarted into the same screen. A
+        numeric string counts, as in BasePlugin.get_display_duration; any
+        other value that is not a finite number, or a raise, gets the 30 s a
+        mode without a plugin gets. A number at or below zero is passed on:
+        _resolve_durations has its own rule for that.
         """
         display_durations = self.config.get('display', {}).get('display_durations', {}) or {}
         override = display_durations.get(mode_key)
         if isinstance(override, (int, float)) and not isinstance(override, bool) and override > 0:
             return float(override)
 
         plugin_instance = self.plugin_modes.get(mode_key)
-        if plugin_instance is not None and hasattr(plugin_instance, 'get_display_duration'):
-            return plugin_instance.get_display_duration()
+        if plugin_instance is None or not hasattr(plugin_instance, 'get_display_duration'):
+            return 30
+        try:
+            value = plugin_instance.get_display_duration()
+        except Exception as err:  # pylint: disable=broad-except
+            problem = f"get_display_duration() raised {type(err).__name__}: {err}"
+        else:
+            seconds = _finite_seconds(value)
+            if seconds is not None:
+                return seconds
+            problem = f"display duration {value!r} is not a number"
+        plugin_id = getattr(plugin_instance, 'plugin_id', None) or mode_key
+        if plugin_id not in self._duration_warned:
+            self._duration_warned = self._duration_warned | {plugin_id}
+            logger.warning("Plugin %s: %s; showing its modes for 30s (logged once)",
+                           plugin_id, problem)
         return 30
 
     def _get_global_dynamic_cap(self) -> Optional[float]:

test/test_display_duration_not_a_number.py

@@ -0,0 +1,112 @@
+"""A plugin duration that is not a number must not stop the display.
+
+Several plugins return their ``display_duration`` setting as it is in
+config.json (``return self.config.get('display_duration', 15.0)``), so a
+value saved as ``"20"`` or ``null`` -- from the raw config editor, or by
+hand -- reached run() as a string or None. _resolve_durations then compared
+it with 0, the TypeError went past every handler in the loop, and the
+display service exited; systemd restarted it into the same screen and the
+same crash.
+"""
+
+import logging
+import math
+import os
+from unittest.mock import MagicMock
+
+os.environ.setdefault("EMULATOR", "true")
+
+import pytest
+
+from src.display_controller import DisplayController
+from test._run_loop_harness import FakePlugin, RunLoopHarness
+
+
+def _controller(plugin_modes):
+    dc = object.__new__(DisplayController)
+    dc.config = {}
+    dc.plugin_modes = plugin_modes
+    return dc
+
+
+def _plugin(duration, plugin_id='clock-simple'):
+    plugin = MagicMock()
+    plugin.plugin_id = plugin_id
+    plugin.get_display_duration.return_value = duration
+    return plugin
+
+
+class TestPluginDurationIsCoerced:
+    @pytest.mark.parametrize('value, expected', [
+        ('20', 20.0), (' 7.5 ', 7.5), (12, 12.0), (12.5, 12.5)])
+    def test_numbers_and_numeric_strings_are_used(self, value, expected):
+        dc = _controller({'clock': _plugin(value)})
+        duration = dc._get_display_duration('clock')
+        assert duration == expected and isinstance(duration, float)
+
+    @pytest.mark.parametrize('value', [
+        None, '', 'twenty', True, False, float('nan'), float('inf'), 'inf',
+        [20], {'seconds': 20}])
+    def test_anything_but_a_finite_number_gets_the_default(self, value):
+        dc = _controller({'clock': _plugin(value)})
+        assert dc._get_display_duration('clock') == 30
+
+    @pytest.mark.parametrize('value', [0, -5, '-5', '0'])
+    def test_a_number_not_above_zero_still_gets_the_15s_rule(self, value):
+        """Unchanged: _resolve_durations turns it into 15 s, with its warning."""
+        plugin = _plugin(value)
+        dc = _controller({'clock': plugin})
+        base = dc._get_display_duration('clock')
+        assert dc._resolve_durations(plugin, 'clock', base, False)[1] == 15.0
+
+    def test_a_raising_get_display_duration_gets_the_default(self):
+        plugin = _plugin(None)
+        plugin.get_display_duration.side_effect = KeyError('display_duration')
+        assert _controller({'clock': plugin})._get_display_duration('clock') == 30
+
+    def test_the_result_feeds_resolve_durations(self):
+        """The two calls run() makes back to back, for one screen."""
+        plugin = _plugin('bad')
+        dc = _controller({'clock': plugin})
+        base = dc._get_display_duration('clock')
+        assert dc._resolve_durations(plugin, 'clock', base, False) == (30, 30)
+
+    def test_logged_once_per_plugin(self, caplog):
+        dc = _controller({'clock': _plugin('twenty'),
+                          'clock_big': _plugin('twenty'),
+                          'calendar': _plugin(None, plugin_id='calendar')})
+        # clock_big is a second mode of the same plugin.
+        dc.plugin_modes['clock_big'].plugin_id = 'clock-simple'
+        with caplog.at_level(logging.WARNING, logger='src.display_controller'):
+            for _ in range(3):
+                for mode in ('clock', 'clock_big', 'calendar'):
+                    dc._get_display_duration(mode)
+        warnings = [r for r in caplog.records if 'display duration' in r.getMessage()]
+        assert len(warnings) == 2
+        assert {'clock-simple', 'calendar'} == {
+            next(p for p in ('clock-simple', 'calendar') if p in r.getMessage())
+            for r in warnings}
+
+    def test_a_good_value_after_a_bad_one_is_used(self):
+        plugin = _plugin(None)
+        dc = _controller({'clock': plugin})
+        assert dc._get_display_duration('clock') == 30
+        plugin.get_display_duration.return_value = 45
+        assert dc._get_display_duration('clock') == 45.0
+
+
+class TestRunLoopSurvives:
+    """Through the real run() on the harness's fake clock."""
+
+    @pytest.mark.parametrize('duration, shown_for', [('20', 20.0), (None, 30.0),
+                                                     ('twenty', 30.0)])
+    def test_the_screen_runs_and_the_rotation_goes_on(self, tmp_path, duration, shown_for):
+        harness = RunLoopHarness(tmp_path, horizon=120)
+        harness.add_plugin(FakePlugin("weather", ["weather"], duration=30))
+        harness.add_plugin(FakePlugin("clock-simple", ["clock"], duration=duration))
+        # Before the fix run() returned at t=30, when the clock came up, and
+        # the harness raised "run() returned ... before the horizon".
+        rows = harness.run()["screens"]
+        clock = next(row for row in rows if row[1] == "clock")
+        assert math.isclose(clock[2], shown_for, abs_tol=1.0)
+        assert [row[1] for row in rows][:3] == ["weather", "clock", "weather"]