diff --git a/CHANGELOG.md b/CHANGELOG.md index 9257449f..b74c8f26 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -885,6 +885,18 @@ policies are unchanged. a runtime publisher that stops still goes `stale`, and a subscription that goes quiet still falls back to the cache. The cache path's 120 s rule is unchanged. +- A plugin that pauses the Vegas scroll gets its pause when its display + duration is not a plain number. Several plugins (clock-simple, calendar, + countdown) return `display_duration` as it is in config.json, so a value + saved as `"20"` or `null` (the raw config editor, a hand edit) reached the + pause as a string or None; comparing it with the clock raised, and the + plugin flashed up and the scroll went straight on, at every one of its + turns. `inf` held the pause until something interrupted it, and 0, a + negative number or NaN ended it at once. The pause now reads the duration + as the rotation does (`finite_seconds()` in `base_plugin`): a numeric + string counts, anything else that is not a finite number (or a + `get_display_duration()` that raises) pauses for 30 s, and a number at or + below zero for 15 s, with one warning per plugin. ### Scrolling diff --git a/src/display_controller.py b/src/display_controller.py index 95234d2d..ead80499 100644 --- a/src/display_controller.py +++ b/src/display_controller.py @@ -25,7 +25,6 @@ import os import inspect import signal import json -import math import threading import types from collections import deque @@ -57,6 +56,7 @@ from src.ipc.contract import ( PluginReloadResult, ) from src.ipc.server import ControlServer, QueuedCommand, StateHub, start_control_server +from src.plugin_system.base_plugin import finite_seconds from src.vegas_mode.render_pipeline import SYNC_SEND_INTERVAL # Get logger with consistent configuration @@ -90,19 +90,6 @@ _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. @@ -1392,7 +1379,7 @@ class DisplayController: except Exception as err: # pylint: disable=broad-except problem = f"get_display_duration() raised {type(err).__name__}: {err}" else: - seconds = _finite_seconds(value) + seconds = finite_seconds(value) if seconds is not None: return seconds problem = f"display duration {value!r} is not a number" diff --git a/src/plugin_system/base_plugin.py b/src/plugin_system/base_plugin.py index 54314cd9..5f7a4ea0 100644 --- a/src/plugin_system/base_plugin.py +++ b/src/plugin_system/base_plugin.py @@ -11,6 +11,7 @@ Stability: Stable - maintains backward compatibility from abc import ABC, abstractmethod from enum import Enum from typing import Dict, Any, Optional, List +import math import os import sys from src.deprecation import deprecated, warn_deprecated @@ -240,6 +241,26 @@ def resolve_vegas_participation(plugin: Any, plugin_id: Optional[str] = None) -> return legacy_vegas_participation(plugin) +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. + + How the core reads a plugin's get_display_duration() -- the rotation + (DisplayController._get_display_duration) and the Vegas static pause -- + which several plugins answer straight from config.json, so a value saved + as "20" or null arrives as a string or None. A number at or below zero is + returned as it is; each caller has its own rule for that. + """ + 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 BasePlugin(ABC): """ Base class that all plugins must inherit from. diff --git a/src/vegas_mode/coordinator.py b/src/vegas_mode/coordinator.py index 8eb0f0ab..62825526 100644 --- a/src/vegas_mode/coordinator.py +++ b/src/vegas_mode/coordinator.py @@ -18,10 +18,11 @@ import math import sys import time import threading -from typing import Optional, Dict, Any, List, Callable, TYPE_CHECKING +from typing import Optional, Dict, Any, FrozenSet, List, Callable, TYPE_CHECKING from src import display_watchdog from src.common import render_gate +from src.plugin_system.base_plugin import finite_seconds from src.vegas_mode.config import VegasModeConfig from src.vegas_mode.elements import LiveEpochs from src.vegas_mode.plugin_adapter import PluginAdapter @@ -53,6 +54,14 @@ _FPS_HEARTBEAT_INTERVAL = 300.0 #: every plugin. Game state doesn't change within a quarter second. _LIVE_PRIORITY_CHECK_INTERVAL = 0.25 +#: Seconds a static pause shows a plugin whose display duration can't be +#: used, as long as the rotation shows it: 30 when get_display_duration() +#: raises or answers something that is not a number +#: (DisplayController._get_display_duration), 15 when it answers a number at +#: or below zero (DisplayController._resolve_durations). +_UNREADABLE_DURATION = 30.0 +_NOT_POSITIVE_DURATION = 15.0 + def _percentile(ordered: List[float], fraction: float) -> float: """Nearest-rank percentile of an already-sorted list. @@ -92,6 +101,9 @@ class VegasModeCoordinator: _live_reason: Optional[str] = None # Set only while Vegas has changed the GIL switch interval; read with getattr. _saved_switch_interval: Optional[float] + #: Plugins already warned about a display duration the pause can't use, + #: so a bad setting logs once, not at every turn. Replaced, not mutated. + _duration_warned: FrozenSet[str] = frozenset() def __init__( self, @@ -1010,7 +1022,7 @@ class VegasModeCoordinator: # Wait for the plugin's display duration. Monotonic, like the # iteration clock: an NTP step on an RTC-less Pi would otherwise # end the pause at once or stretch it by the correction. - duration = plugin.get_display_duration() + duration = self._static_pause_duration(plugin) start = time.monotonic() while time.monotonic() - start < duration: @@ -1046,6 +1058,42 @@ class VegasModeCoordinator: return True + def _static_pause_duration(self, plugin: 'BasePlugin') -> float: + """Seconds a static pause shows ``plugin``: its display duration, + read the way the rotation reads it. + + Several plugins return their display_duration setting straight from + config.json, so one saved as "20" or null came back as a string or + None; comparing it with the clock raised, and the pause's broad + except ended the pause at every one of the plugin's turns. inf + paused until something interrupted it, and NaN, False, 0 or a + negative number ended the pause at once. A numeric string counts + (finite_seconds); anything else, or a raise, gets + _UNREADABLE_DURATION, and a number at or below zero + _NOT_POSITIVE_DURATION, logged once per plugin. + """ + try: + value = plugin.get_display_duration() + except Exception as err: # pylint: disable=broad-except + problem = f"get_display_duration() raised {type(err).__name__}: {err}" + seconds = _UNREADABLE_DURATION + else: + seconds = finite_seconds(value) + if seconds is not None and seconds > 0: + return seconds + if seconds is None: + problem = f"display duration {value!r} is not a number" + seconds = _UNREADABLE_DURATION + else: + problem = f"display duration {value!r} is not above zero" + seconds = _NOT_POSITIVE_DURATION + plugin_id = plugin.plugin_id + if plugin_id not in self._duration_warned: + self._duration_warned = self._duration_warned | {plugin_id} + logger.warning("[%s] %s; its static pause lasts %.0fs (logged once)", + plugin_id, problem, seconds) + return seconds + def _end_static_pause(self) -> None: """End static pause and restore scroll state.""" should_resume_scrolling = False diff --git a/test/test_vegas_static_mode.py b/test/test_vegas_static_mode.py index 2d3f6679..3bfe1962 100644 --- a/test/test_vegas_static_mode.py +++ b/test/test_vegas_static_mode.py @@ -219,7 +219,8 @@ class TestCoordinatorStaticPause: def _plugin(self): plugin = MagicMock() plugin.plugin_id = 'clock' - plugin.get_display_duration.return_value = 0 + # A moment: zero would pause 15 s, as the rotation shows it. + plugin.get_display_duration.return_value = 0.01 return plugin def test_trigger_comes_from_the_pipeline(self): diff --git a/test/test_vegas_static_pause_duration.py b/test/test_vegas_static_pause_duration.py new file mode 100644 index 00000000..957e45f7 --- /dev/null +++ b/test/test_vegas_static_pause_duration.py @@ -0,0 +1,197 @@ +"""A Vegas static pause lasts as long as the rotation shows the plugin. + +The pause asked the plugin for get_display_duration() and compared the +answer with the clock. Several plugins (clock-simple, calendar, countdown) +return their display_duration setting as it is in config.json, so one saved +as "20" or null -- the raw config editor, a hand edit -- reached that +comparison as a string or None. The TypeError went to the pause's broad +except, which ended the pause: the plugin flashed up and the scroll went on, +at every one of its turns. inf paused until something interrupted it, and +NaN, False, 0 or a negative number ended the pause at once. + +The pause now reads the answer the way the rotation does since #739, with +the same helper (base_plugin.finite_seconds): a numeric string counts; +anything else that is not a finite number, or a raise, gets the rotation's +30 s; a number at or below zero gets its 15 s. +""" + +import logging +import os +import threading +from types import SimpleNamespace +from unittest.mock import MagicMock + +os.environ.setdefault("EMULATOR", "true") + +import pytest + +from src.vegas_mode import coordinator + +NOT_NUMBERS = [None, '', 'twenty', True, False, float('nan'), float('inf'), + 'inf', '1e400', [20], {'seconds': 20}] +NOT_ABOVE_ZERO = [0, -5, '-5', '0'] +NUMBERS = [('20', 20.0), (' 7.5 ', 7.5), (12, 12.0), (12.5, 12.5)] + + +class FakeClock: + """time.monotonic/time.sleep for the pause loop: sleeping moves the clock.""" + + #: A pause still going after this long never ends (inf did that). + LIMIT = 3600.0 + + def __init__(self): + self.now = 0.0 + + def monotonic(self): + return self.now + + def sleep(self, seconds): + self.now += seconds + if self.now > self.LIMIT: + raise RuntimeError("the static pause never ended") + + +@pytest.fixture +def clock(monkeypatch): + fake = FakeClock() + monkeypatch.setattr(coordinator, 'time', fake) + return fake + + +def _plugin(duration, plugin_id='clock-simple'): + plugin = MagicMock() + plugin.plugin_id = plugin_id + plugin.get_display_duration.return_value = duration + return plugin + + +def _coord(*plugins): + coord = coordinator.VegasModeCoordinator.__new__(coordinator.VegasModeCoordinator) + coord.render_pipeline = MagicMock() + coord.render_pipeline.get_scroll_position.return_value = 0 + coord.display_manager = MagicMock() + locks = {plugin.plugin_id: threading.Lock() for plugin in plugins} + coord.plugin_manager = SimpleNamespace(get_plugin_lock=locks.__getitem__) + coord._state_lock = threading.Lock() + coord._static_pause_active = False + coord._saved_scroll_position = None + coord._should_stop = False + coord._live_priority_active = False + coord._live_priority_check = None + coord._interrupt_check = None + coord.stats = {'static_pauses': 0} + return coord + + +def _pause(coord, plugin, clock): + """One static pause: (whether it completed, how long it lasted).""" + start = clock.now + completed = coord._handle_static_pause(plugin) + return completed, clock.now - start + + +class TestPauseLength: + @pytest.mark.parametrize('value, seconds', NUMBERS) + def test_numbers_and_numeric_strings_are_used(self, clock, value, seconds): + plugin = _plugin(value) + completed, lasted = _pause(_coord(plugin), plugin, clock) + assert completed is True + assert lasted == pytest.approx(seconds, abs=0.15) + + @pytest.mark.parametrize('value', NOT_NUMBERS, ids=repr) + def test_anything_but_a_finite_number_pauses_for_30s(self, clock, value): + plugin = _plugin(value) + completed, lasted = _pause(_coord(plugin), plugin, clock) + assert completed is True + assert lasted == pytest.approx(30.0, abs=0.15) + plugin.display.assert_called_once_with(force_clear=True) + + @pytest.mark.parametrize('value', NOT_ABOVE_ZERO, ids=repr) + def test_a_number_not_above_zero_pauses_for_15s(self, clock, value): + plugin = _plugin(value) + completed, lasted = _pause(_coord(plugin), plugin, clock) + assert completed is True + assert lasted == pytest.approx(15.0, abs=0.15) + + def test_a_raising_get_display_duration_pauses_for_30s(self, clock): + plugin = _plugin(None) + plugin.get_display_duration.side_effect = KeyError('display_duration') + completed, lasted = _pause(_coord(plugin), plugin, clock) + assert completed is True + assert lasted == pytest.approx(30.0, abs=0.15) + + def test_a_good_value_after_a_bad_one_is_used(self, clock): + plugin = _plugin(None) + coord = _coord(plugin) + assert _pause(coord, plugin, clock)[1] == pytest.approx(30.0, abs=0.15) + plugin.get_display_duration.return_value = 45 + assert _pause(coord, plugin, clock)[1] == pytest.approx(45.0, abs=0.15) + + def test_the_pause_can_still_be_interrupted(self, clock): + plugin = _plugin('twenty') + coord = _coord(plugin) + coord._interrupt_check = lambda: clock.now >= 5 + completed, lasted = _pause(coord, plugin, clock) + assert completed is False + assert lasted == pytest.approx(5.0, abs=0.15) + + +class TestWarning: + def test_logged_once_per_plugin(self, clock, caplog): + clock_plugin = _plugin('twenty') + calendar = _plugin(None, plugin_id='calendar') + coord = _coord(clock_plugin, calendar) + with caplog.at_level(logging.WARNING, logger='src.vegas_mode.coordinator'): + for _ in range(3): + for plugin in (clock_plugin, calendar): + coord._handle_static_pause(plugin) + warnings = [r.getMessage() for r in caplog.records + if 'display duration' in r.getMessage()] + assert len(warnings) == 2 + assert any('clock-simple' in m and "'twenty'" in m for m in warnings) + assert any('calendar' in m and 'None' in m for m in warnings) + + +class TestFiniteSeconds: + """The shared rule: what counts as a number of seconds.""" + + @pytest.mark.parametrize('value, seconds', NUMBERS + [(0, 0.0), ('-5', -5.0)]) + def test_numbers_and_numeric_strings(self, value, seconds): + from src.plugin_system.base_plugin import finite_seconds + result = finite_seconds(value) + assert result == seconds and isinstance(result, float) + + @pytest.mark.parametrize('value', NOT_NUMBERS + [pytest.param(10 ** 400, id='10**400')], + ids=repr) + def test_anything_else_is_none(self, value): + from src.plugin_system.base_plugin import finite_seconds + assert finite_seconds(value) is None + + +def _rotation_seconds(plugin): + """How long the rotation shows ``plugin`` (no dynamic duration, no + Rotation & Durations override): the two calls run() makes for a screen. + """ + from src.display_controller import DisplayController + dc = object.__new__(DisplayController) + dc.config = {} + dc.plugin_modes = {'mode': plugin} + return dc._resolve_durations(plugin, 'mode', dc._get_display_duration('mode'), False)[1] + + +class TestSameAsTheRotation: + """The pause and the rotation share finite_seconds; this pins their + fallbacks (30 s, 15 s) to each other too.""" + + @pytest.mark.parametrize('value', [value for value, _ in NUMBERS] + + NOT_NUMBERS + NOT_ABOVE_ZERO, ids=repr) + def test_the_pause_lasts_as_long_as_the_rotation_shows_it(self, clock, value): + plugin = _plugin(value) + expected = _rotation_seconds(plugin) + assert _pause(_coord(plugin), plugin, clock)[1] == pytest.approx(expected, abs=0.15) + + def test_a_raise_too(self, clock): + plugin = _plugin(None) + plugin.get_display_duration.side_effect = KeyError('display_duration') + expected = _rotation_seconds(plugin) + assert _pause(_coord(plugin), plugin, clock)[1] == pytest.approx(expected, abs=0.15)