From a9e1bd0b721203e5cfb5d611af62ae685589b890 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Mon, 14 Sep 2026 17:51:11 -0400 Subject: [PATCH] fix: address CodeRabbit review on #580 - Preview fallbacks (SSE stream and /display/current) use logical_size({}) (128x32, the shared default) instead of a hard-coded 128x64. - display_geometry treats a non-mapping display/hardware block as missing, so a malformed config.json falls back to defaults instead of raising AttributeError (which turned the Starlark render into an HTTP 500). - Docs: the static update interval falls back manifest -> plugin config -> 60s, in both the API reference and the architecture spec. Not taken: validating double_sided copies against chain_length/parallel. An orientation Rotate: or U-mapper pixel mapper decides which axis panels lie on, so the counts would reject working setups (the existing vertical-split test is one). Co-Authored-By: Claude Opus 5 --- docs/PLUGIN_API_REFERENCE.md | 5 +++-- docs/PLUGIN_ARCHITECTURE_SPEC.md | 3 ++- src/display_geometry.py | 19 +++++++++++++++---- test/test_display_geometry.py | 16 ++++++++++++++++ web_interface/app.py | 5 ++--- web_interface/blueprints/api_v3/display.py | 12 ++++-------- 6 files changed, 42 insertions(+), 18 deletions(-) diff --git a/docs/PLUGIN_API_REFERENCE.md b/docs/PLUGIN_API_REFERENCE.md index 5decc99e..0a98d0f1 100644 --- a/docs/PLUGIN_API_REFERENCE.md +++ b/docs/PLUGIN_API_REFERENCE.md @@ -38,8 +38,9 @@ self.enabled # Boolean enabled status Fetch/update data for this plugin. Called on the plugin's update interval: the value `get_update_interval()` returns when it returns a number, otherwise -the `update_interval` in the plugin's manifest (see -[`get_update_interval()`](#get_update_interval---optionalfloat) below). +the static interval: the `update_interval` in the plugin's manifest, else +`update_interval` in the plugin's section of `config.json`, else 60 seconds +(see [`get_update_interval()`](#get_update_interval---optionalfloat) below). **Example**: ```python diff --git a/docs/PLUGIN_ARCHITECTURE_SPEC.md b/docs/PLUGIN_ARCHITECTURE_SPEC.md index 5f7309f8..3c8ddb6f 100644 --- a/docs/PLUGIN_ARCHITECTURE_SPEC.md +++ b/docs/PLUGIN_ARCHITECTURE_SPEC.md @@ -190,7 +190,8 @@ class BasePlugin(ABC): """ Fetch/update data for this plugin. Called every get_update_interval() seconds when that returns a - number, otherwise every manifest update_interval seconds. + number, otherwise at the static interval: the manifest's + update_interval, else the plugin config's update_interval, else 60s. """ pass diff --git a/src/display_geometry.py b/src/display_geometry.py index ffc4250e..1c069911 100644 --- a/src/display_geometry.py +++ b/src/display_geometry.py @@ -23,9 +23,16 @@ DEFAULT_CHAIN_LENGTH = 2 DEFAULT_PARALLEL = 1 +def _display(config: Optional[Mapping[str, Any]]) -> Mapping[str, Any]: + # A hand-edited config.json can hold anything here; treat a non-mapping + # like a missing block so callers get the defaults, not AttributeError. + display = (config or {}).get('display') + return display if isinstance(display, Mapping) else {} + + def _hardware(config: Optional[Mapping[str, Any]]) -> Mapping[str, Any]: - display = (config or {}).get('display') or {} - return display.get('hardware') or {} + hw = _display(config).get('hardware') + return hw if isinstance(hw, Mapping) else {} def physical_size(config: Optional[Mapping[str, Any]]) -> Tuple[int, int]: @@ -54,6 +61,10 @@ def resolve_double_sided(physical_width: int, physical_height: int, config is logged and disabled rather than raised — a misconfigured panel should still light up. + Only pixels are checked, not whole panels: ``chain_length`` and + ``parallel`` don't say which axis a panel lies on once an orientation + ``Rotate:`` or U-mapper ``pixel_mapper_config`` rearranges the chain. + ``quiet`` suppresses the log lines, for callers that run on every web request and would otherwise repeat them on each poll. """ @@ -118,8 +129,8 @@ def logical_size(config: Optional[Mapping[str, Any]], ``DisplayManager.width``/``height`` give. """ width, height = physical_size(config) - display = (config or {}).get('display') or {} - ds = resolve_double_sided(width, height, display.get('double_sided') or {}, + ds = resolve_double_sided(width, height, + _display(config).get('double_sided') or {}, quiet=quiet) if ds is not None: return ds['logical_width'], ds['logical_height'] diff --git a/test/test_display_geometry.py b/test/test_display_geometry.py index 28b0df24..0d59b92e 100644 --- a/test/test_display_geometry.py +++ b/test/test_display_geometry.py @@ -71,6 +71,13 @@ def test_disabled_double_sided_is_ignored(): assert logical_size(cfg) == (256, 32) +@pytest.mark.parametrize('display', ['oops', ['a'], 1, {'hardware': 'oops'}, + {'hardware': ['a']}]) +def test_non_mapping_display_config_uses_the_defaults(display): + assert physical_size({'display': display}) == (128, 32) + assert logical_size({'display': display}) == (128, 32) + + @pytest.fixture def display_client(monkeypatch): from web_interface.blueprints.api_v3 import api_v3 @@ -103,6 +110,15 @@ def test_display_current_defaults_chain_length_like_display_manager(display_clie assert (data['width'], data['height']) == (64 * DEFAULT_CHAIN_LENGTH, 32) +def test_display_current_falls_back_to_the_shared_default(display_client): + client, config_manager = display_client + config_manager.load_config.side_effect = ValueError('unreadable') + + data = client.get('/api/v3/display/current').get_json()['data'] + + assert (data['width'], data['height']) == logical_size({}) == (128, 32) + + def test_preview_callers_do_not_rederive_the_size(): """Every preview/size caller goes through display_geometry, so none of them can drift back to a private chain_length default.""" diff --git a/web_interface/app.py b/web_interface/app.py index f4cca6d4..8bc50b16 100644 --- a/web_interface/app.py +++ b/web_interface/app.py @@ -750,12 +750,11 @@ def display_preview_generator(): # Get display dimensions from config: the logical size DisplayManager # renders at, so double-sided setups preview one screen + from src.display_geometry import logical_size try: - from src.display_geometry import logical_size width, height = logical_size(config_manager.load_config()) except (KeyError, TypeError, ValueError, AttributeError, ConfigError): - width = 128 - height = 64 + width, height = logical_size({}) while True: try: diff --git a/web_interface/blueprints/api_v3/display.py b/web_interface/blueprints/api_v3/display.py index 3351151d..afb2f5d9 100644 --- a/web_interface/blueprints/api_v3/display.py +++ b/web_interface/blueprints/api_v3/display.py @@ -27,16 +27,12 @@ def get_display_current(): # Get display dimensions from config: the logical size DisplayManager # renders at, so double-sided setups preview one screen + from src.display_geometry import logical_size try: - if api_v3.config_manager: - from src.display_geometry import logical_size - width, height = logical_size(api_v3.config_manager.load_config()) - else: - width = 128 - height = 64 + config = api_v3.config_manager.load_config() if api_v3.config_manager else {} + width, height = logical_size(config) except Exception: - width = 128 - height = 64 + width, height = logical_size({}) # Try to read snapshot file image_data = None