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

* fix(display): a plugin duration that is not a number no longer stops the display

DisplayController._get_display_duration returned whatever the plugin's
get_display_duration() gave back. clock-simple, calendar and countdown
return their display_duration setting straight from config.json, so a
value saved as "20" or null reached _resolve_durations as a string or
None, and its `<= 0` check raised a TypeError. Nothing in the loop caught
it: run()'s outer handler logged "Unexpected error in display controller"
and cleanup() ended the service when that plugin's screen came up, and
systemd restarted it into the same crash.

The plugin's answer is now read as seconds: a finite number or a numeric
string is used (as BasePlugin.get_display_duration already accepts), a
number at or below zero still goes to _resolve_durations' 15 s rule, and
anything else -- None, a non-numeric string, a bool, NaN, infinity, or a
get_display_duration() that raises -- gets the 30 s a mode without a
plugin gets. The warning is logged once per plugin, not at every screen.

Tests: test/test_display_duration_not_a_number.py, including the real
run() on the run-loop harness, which returned at t=30 before the fix.

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

* fix(scroll): a strip narrower than the panel no longer raises on every frame

ScrollHelper._get_visible_portion_integer handled a frame that runs off
the end of the strip by copying the strip's tail and then the rest of the
frame from its head, which assumed the head was at least that wide. For a
strip narrower than the panel that raised "could not broadcast input
array" at every position, so get_visible_portion() never returned a frame
and the caller logged a traceback each frame. Vegas composes such a strip
(lead_in_width defaults to 0) when its content is narrower than the chain.

A wrapping frame is now taken column by column modulo the strip's width
(np.take, mode='wrap', into the reused frame buffer): the tail then the
head, as before, and a narrow strip repeated across the panel. The same
path takes a position before the start of the strip, whose [-n:m] slice
was empty and made frombytes raise; the integer and sub-pixel fast paths
now leave a negative start to it. A zero-width strip is still a black
frame.

Tests: test/test_scroll_helper_narrow_strip.py.

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

---------

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

CHANGELOG.md

@@ -531,6 +531,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,7 +561,7 @@ class ScrollHelper:
         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
@@ -569,9 +569,10 @@ class ScrollHelper:
             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
+            # One of the slices wraps (close to the end, or a strip narrower
-            # integer path handle that and pay the conversion. Continuous mode
+            # than the panel); let the integer path handle that and pay the
-            # extends the strip before reaching here, so this is the rare case.
+            # 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,32 +602,31 @@ class ScrollHelper:
         _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())
-        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 img_w == 0:
-            if width1 > 0:
+            self._frame_buffer[:] = 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,
+            # The frame runs off the strip, so it carries on from the head:
-                # clamped to available width (scroll_position should wrap before
+            # frame column j is strip column (start_x + j) modulo the strip's
-                # reaching this state in normal operation).
+            # width -- the tail and then the head, and a strip narrower than
-                available = min(self.display_width, img_w)
+            # the panel repeated across it. Copying the tail and then the rest
-                self._frame_buffer[:, :available] = self.cached_array[:, :available]
+            # of the frame from the head assumed the head was that wide, and
-                if available < self.display_width:
+            # raised at every position for a strip narrower than the panel
-                    self._frame_buffer[:, available:] = 0
+            # (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())
     

src/display_controller.py

@@ -25,11 +25,12 @@ import os
 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 @@ _MIN_INITIAL_UPDATE_TIMEOUT_SECONDS = 2.0
 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,6 +1354,12 @@ class DisplayController:
                         "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.
@@ -1347,6 +1367,17 @@ class DisplayController:
         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)
@@ -1354,8 +1385,22 @@ class DisplayController:
             return float(override)
 
         plugin_instance = self.plugin_modes.get(mode_key)
-        if plugin_instance is not None and hasattr(plugin_instance, 'get_display_duration'):
+        if plugin_instance is None or not hasattr(plugin_instance, 'get_display_duration'):
-            return 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"]

test/test_scroll_helper_narrow_strip.py

@@ -0,0 +1,112 @@
+"""
+ScrollHelper frames for a strip narrower than the panel, and other wraps.
+
+A frame that runs past the end of the strip continues from its head: column
+j of the frame is strip column (position + j) modulo the strip's width. The
+wrap path sliced the strip's tail and then "the rest of the frame" from its
+head, which assumed the head was at least that wide. For a strip narrower
+than the panel it raised ValueError at every position, so a narrow strip
+(Vegas composes one when its content is narrower than the chain, with its
+lead-in of 0) logged a traceback every frame instead of drawing.
+"""
+
+import numpy as np
+import pytest
+from PIL import Image
+
+from src.common.scroll_helper import ScrollHelper
+
+W, H = 128, 32
+
+
+def _strip(width, height=H):
+    """A strip whose every column is distinct: R and B are the column number."""
+    columns = np.arange(width)
+    pixels = np.zeros((height, width, 3), dtype=np.uint8)
+    pixels[:, :, 0] = columns % 256
+    pixels[:, :, 1] = 255 - (columns % 256)
+    pixels[:, :, 2] = columns // 256
+    return Image.fromarray(pixels, 'RGB')
+
+
+def _helper(strip_width, sub_pixel=False):
+    sh = ScrollHelper(W, H)
+    sh.set_scrolling_image(_strip(strip_width))
+    sh.sub_pixel_scrolling = sub_pixel
+    return sh
+
+
+def _frame(sh, position):
+    sh.scroll_position = position
+    frame = sh.get_visible_portion()
+    assert frame is not None and frame.size == (W, H) and frame.mode == 'RGB'
+    return np.asarray(frame)
+
+
+def _wrapped(sh, start):
+    """What the panel should show from ``start``: the strip, wrapping."""
+    return sh.cached_array[:, np.arange(start, start + W) % sh.cached_array.shape[1]]
+
+
+class TestNarrowStrip:
+    @pytest.mark.parametrize('strip_width', [1, 40, 50, W - 1])
+    @pytest.mark.parametrize('position', [0, 10, 39])
+    def test_frame_repeats_the_strip_across_the_panel(self, strip_width, position):
+        sh = _helper(strip_width)
+        position %= strip_width
+        assert np.array_equal(_frame(sh, position), _wrapped(sh, position))
+
+    def test_a_composed_strip_without_lead_in(self):
+        # How Vegas builds its strip: lead_gap=0 (vegas_scroll.lead_in_width).
+        sh = ScrollHelper(W, H)
+        sh.create_scrolling_image([_strip(40)], item_gap=0, element_gap=0, lead_gap=0)
+        assert sh.total_scroll_width == 40
+        for position in range(40):
+            assert np.array_equal(_frame(sh, position), _wrapped(sh, position))
+
+    def test_a_whole_pass_scrolls_without_raising(self):
+        sh = _helper(50)
+        sh.set_pixels_per_frame(3)
+        for _ in range(60):
+            sh.update_scroll_position()
+            assert sh.get_visible_portion().size == (W, H)
+
+    @pytest.mark.parametrize('position', [0.5, 10.25, 49.5])
+    def test_sub_pixel_blend_of_a_narrow_strip(self, position):
+        sh = _helper(50, sub_pixel=True)
+        frame = _frame(sh, position)
+        start = int(position)
+        near, far = _wrapped(sh, start), _wrapped(sh, start + 1)
+        lo, hi = np.minimum(near, far), np.maximum(near, far)
+        assert (frame >= lo).all() and (frame <= hi).all()
+
+
+class TestWrapOfAWideStrip:
+    """Unchanged: the tail, then the head."""
+
+    @pytest.mark.parametrize('position', [200 - W + 1, 150, 199])
+    def test_tail_then_head(self, position):
+        sh = _helper(200)
+        frame = _frame(sh, position)
+        tail = 200 - position
+        assert np.array_equal(frame[:, :tail], sh.cached_array[:, position:])
+        assert np.array_equal(frame[:, tail:], sh.cached_array[:, :W - tail])
+
+    def test_at_the_end_shows_the_head(self):
+        sh = _helper(200)
+        assert np.array_equal(_frame(sh, 200), sh.cached_array[:, :W])
+
+    def test_sub_pixel_at_the_last_column(self):
+        sh = _helper(200, sub_pixel=True)
+        assert _frame(sh, 199.5).shape == (H, W, 3)
+
+    def test_a_position_before_the_start_wraps_too(self):
+        # Slicing [-10:118] of the array was an empty slice: frombytes raised.
+        sh = _helper(200)
+        assert np.array_equal(_frame(sh, -10), _wrapped(sh, -10))
+
+
+def test_a_zero_width_strip_is_a_black_frame():
+    sh = ScrollHelper(W, H)
+    sh.set_scrolling_image(Image.new('RGB', (0, H)))
+    assert not _frame(sh, 0).any()