diff --git a/CHANGELOG.md b/CHANGELOG.md index 93ea830f..8cc51f4a 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/common/scroll_helper.py b/src/common/scroll_helper.py index e4da770d..b9caffeb 100644 --- a/src/common/scroll_helper.py +++ b/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 - # 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 @@ 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()) + + # 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) + # 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) - 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()) + return Image.frombytes('RGB', _size, self._frame_buffer.tobytes()) def calculate_dynamic_duration(self) -> int: """ diff --git a/src/display_controller.py b/src/display_controller.py index 8756518a..95234d2d 100644 --- a/src/display_controller.py +++ b/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'): - 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]: diff --git a/test/test_display_duration_not_a_number.py b/test/test_display_duration_not_a_number.py new file mode 100644 index 00000000..1d43497b --- /dev/null +++ b/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"] diff --git a/test/test_scroll_helper_narrow_strip.py b/test/test_scroll_helper_narrow_strip.py new file mode 100644 index 00000000..9373cb59 --- /dev/null +++ b/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()