From 7b90759252c9aebf3d03aa573a71e1dbebf1293d Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Thu, 24 Sep 2026 17:32:29 -0400 Subject: [PATCH] fix: /errors stack traces, Wi-Fi disconnect and save, plugin fonts, API cache TTL (#636) * fix(errors): record the exception's own stack trace record_error() called traceback.format_exc(), which only sees an exception while its except block is running. plugin_executor records exceptions caught on a worker thread after that block has ended, so every trace on /errors read "NoneType: None". The trace is now built from the exception's __traceback__. The executor's log call had the same problem with exc_info=True and now passes the exception. record_error() also merged LEDMatrixError context into the caller's dict in place; it now works on a copy. Co-Authored-By: Claude Opus 5.5 * docs(wifi): point at configure_wifi_permissions.sh instead of a sudoers list The module docstring told users to grant NOPASSWD sudo on iptables and ip. configure_wifi_permissions.sh refuses those grants on purpose: a wildcard rule for either runs an arbitrary program as root. Point at the script and say why it leaves them out. Co-Authored-By: Claude Opus 5.5 * fix(wifi): disconnect finds the saved profile by SSID disconnect_from_network() asked `nmcli -f NAME,802-11-wireless.ssid connection show` for the profile to take down, but nmcli rejects that column for `connection show`, so the lookup always failed and only the device was disconnected. The per-profile lookup _connect_nmcli() already used is now _find_profile_for_ssid(), and both callers share it. It also splits terse output on the last colon and unescapes "\:", so a profile name containing a colon is found. Co-Authored-By: Claude Opus 5.5 * fix(wifi): write wifi_config.json atomically and report a failed save _save_config() opened the file for writing in place and swallowed any error, so a wifi_config.json left owned by root made the web toggle for auto-enabling AP mode report success while nothing was saved, and a crash mid-write could truncate the file. It now uses atomic_write_json, which also keeps the file's owner and shared group when root saves it, and returns False on failure. POST /wifi/ap/auto-enable answers 500 in that case. The file is now written with indent=4, like the other config files. Co-Authored-By: Claude Opus 5.5 * fix(fonts): resolve plugin:// fonts in the plugin's own directory FontManager looked for a plugin's bundled fonts under Path("plugins") / plugin_id: relative to the process cwd, and not the default install directory (plugin-repos/), so a manifest's plugin:// fonts never loaded. register_plugin_fonts() takes an optional plugin_dir, and PluginManager passes the directory it loaded the plugin from. Callers that omit it get a lookup in the configured plugin_system.plugins_directory, then plugins/, resolved against the install root. Co-Authored-By: Claude Opus 5.5 * fix(api-helper): cache responses for the requested cache_ttl APIHelper.get(cache_ttl=...) and set_cache(ttl=...) dropped the ttl on the claim that CacheManager does not support one, but CacheManager.set() takes a ttl, stores it with the entry, and both cache tiers honour it over a reader's max_age. Without it every response expired after the 300-second default read age, whatever the plugin asked for. The ttl is now passed through, and the cache read passes cache_ttl as max_age for entries written without one. The class docstring describes what the helper actually does. Co-Authored-By: Claude Opus 5.5 * fix(style): one scale range for the schema, element_scale and LogoHelper The generated Scale field allowed 0.1 to 10, element_style's reader capped at 10 with no floor, and LogoHelper accepted 0.05 to 8 and reset anything else to 1.0. A logo scale of 9, which the form accepts, drew at the shipped size. MIN_ELEMENT_SCALE / MAX_ELEMENT_SCALE (0.1, 10.0) in src.element_style are now the schema bounds and the clamp every reader applies through coerce_scale(): a positive number outside the range is clamped, and anything that is not a finite positive number means the default. That also stops element_scale() passing NaN through, since min(nan, 10.0) is nan. Co-Authored-By: Claude Opus 5.5 * fix(logos): placeholder lands at the requested path; empty logos list download_missing_logo() wrote its fallback placeholder to .png in the logo directory rather than to the logo_path the caller passed, so it could return True while nothing existed where the plugin looks (e.g. "TA&M.png" vs "TAANDM.png"). create_placeholder_logo() takes an optional filepath, and download_missing_logo passes the requested one. download_missing_logo_for_team() only caught KeyError, so a team whose "logos" list is empty raised IndexError; it now treats KeyError, IndexError and TypeError as "no logo URL". The placeholder is drawn with PLACEHOLDER_SIZE / PLACEHOLDER_BG, the constants is_placeholder_logo() recognises it by, instead of repeated literals. Co-Authored-By: Claude Opus 5.5 * fix(fonts): resolve bundled font paths against the install root TextHelper's default font_dir, the logo placeholder's font and FontManager's font_overrides.json were all relative to the process cwd, so a process started anywhere but the install root (the plugin safety harness, a manual run, a unit without WorkingDirectory) drew with PIL's default face and read no overrides. They now go through font_layout.resolve_asset_path; the overrides file sits in the install root's config/. The resolver docstrings described an order the code does not follow: resolve_asset_path never consults the cwd, and sports_shared's _resolve_font_path tries the cwd first. Both docstrings now say what the code does, and _resolve_font_path calls resolve_asset_path instead of probing FontManager for it. Co-Authored-By: Claude Opus 5.5 * fix(sync): the web UI reads the sync status file the display writes sync_manager writes its status to tempfile.gettempdir(), but GET /api/v3/sync/status read a hardcoded /tmp/led_matrix_sync_status.json and defaulted the port to a literal 5765. Wherever TMPDIR is set (or on any non-/tmp host) the page only ever showed "starting". The endpoint now uses sync_manager.STATUS_FILE and SYNC_PORT. Co-Authored-By: Claude Opus 5.5 * fix(http): the rankings resolver sends the project's User-Agent DynamicTeamResolver fetched ESPN rankings with a bare requests.get, so it sent python-requests' default User-Agent, which ESPN rejects; the AP_TOP_N favourites then resolved to nothing. It now sends DEFAULT_HTTP_HEADERS. BaseOddsManager carried its own copy of the User-Agent string and now uses the same shared headers (which also adds Accept-Language). Co-Authored-By: Claude Opus 5.5 * fix(backup): record the core release and read the configured plugin dir The manifest's ledmatrix_version came from a VERSION file that does not exist, then from .git/HEAD: a 12-character sha, or "ref: refs/he" when the branch's ref was packed. It is now src.__version__. list_installed_plugins() scanned a hardcoded plugin-repos/, so on an install whose plugin_system.plugins_directory points elsewhere, plugins missing from plugin_state.json were left out of the backup. It now reads the configured directory from config/config.json, defaulting to plugin-repos. Co-Authored-By: Claude Opus 5.5 * fix(startup): report a missing display section once A config without a display section produced three errors for the one problem ("Missing required configuration key: display", "Display configuration is missing or empty" and "Display configuration is missing"), and an empty one produced two. _validate_config now reports it once, as a missing key or an empty section, and _validate_display_config leaves it to that. The module docstring said the validator fails fast; nothing in the display service calls raise_on_errors(), so it now says the errors are reported and startup continues. Co-Authored-By: Claude Opus 5.5 * refactor(wifi): share the copied blocks and name the AP constants - _parse_nmcli_wifi_list() is the one parser behind _scan_nmcli and _scan_nmcli_cached. - _verify_connected(), _wait_for_device_idle(), _failsafe_ap() and _mark_forced() replace blocks that were pasted two or three times in the connect and enable-AP paths. The device-idle wait now checks before its first one-second sleep instead of after it. - _check_command() calls _find_command_path() instead of repeating it. - AP_IP, PORTAL_PORT, AP_PROFILE_NAME and AP_PROFILE_NAMES name values that were spelled out 14, 12, 8 and 2 times; the two deletion loops now walk the same tuple. The iwconfig status path compares the AP address exactly: startswith() also skipped 192.168.4.10-19. - Dropped a second WIFI.SIGNAL query that repeated the first, a no-op "if ssid: continue", the try/except around _connect_wpa_supplicant's constant return, and a second save of a scan scan_networks already saves. - _ensure_wifi_radio_enabled's docstring says it returns True when the radio state cannot be read at all. Co-Authored-By: Claude Opus 5.5 * refactor(config): drop dead branches and history comments in ConfigManager - The module docstring pointed plugin authors at update_plugin_config(), which does not exist; it now names save_config_atomic() and save_raw_file_content(). - load_config's FileNotFoundError handler tested the message for "config_secrets.json", but a missing secrets file is handled where it is read, so only config.json reaches it; the check is gone. - save_raw_file_content's `file_type == "main" or "secrets"` guard was always true (anything else raised earlier). - get_raw_file_content('secrets') already returns {} for a missing file, so the os.path.exists() in front of two calls to it is gone. - Comments that narrated earlier behaviour are rewritten as what the code does now. Co-Authored-By: Claude Opus 5.5 * refactor(background-data): present-tense comments, drop unused API - Comments that told the history of each fix (what "used to" happen, "the old per-delivery release") now state the invariant the code keeps. - get_statistics() no longer reports a constant 'queue_size': 0, and the uncalled clear_completed_requests() is gone (_cleanup_completed_requests does that job on every completion). Neither is referenced in core, the web UI or the plugin monorepo. shutdown_background_service() has no production caller either, but it is the only way to tear down the get_background_service() singleton, which the tests rely on, so it stays. Co-Authored-By: Claude Opus 5.5 * refactor(odds): drop the unread cache_ttl and merge the odds_data branches BaseOddsManager loaded base_odds_manager.cache_ttl from config and never used it: cached odds live for the update interval (get_odds' ttl=interval). No core or monorepo code reads the attribute, so it is gone along with its log line. The two consecutive `if odds_data:` blocks are one. Co-Authored-By: Claude Opus 5.5 * refactor(backup): one table for the single-file sections config, secrets, wifi and ytm_auth were each spelled out in create, preview, validate and restore. _SINGLE_FILE_SECTIONS lists them once, with the RestoreOptions flag that restores each, and all four walk it. Restore error messages keep their wording ("Failed to restore "). Co-Authored-By: Claude Opus 5.5 * refactor(fonts): drop FontManager's write-only state and duplicate logs - fonts_config, font_metadata and font_dependencies were written and never read; the performance_stats keys font_load_times, render_times, total_renders and the per-call "resolve" timings (_record_performance_metric) likewise. get_performance_stats() reads only the counters that remain. Nothing in core or the plugin monorepo references any of them. - A failed BDF load was logged twice, by _load_bdf_font and again by get_font; get_font's line is the one kept. - Removed "NEW:" and commented-out cozette entries, the "Copy font to assets/fonts" comment on code that copies nothing, and local imports of names the module already imports. The deprecated add_font() now resolves assets/fonts against the install root. The @deprecated methods stay. Co-Authored-By: Claude Opus 5.5 * refactor(text-helper): cache loaded fonts; drop the pre-textlength fallback TextHelper declared _font_cache, cleared it and reported its size, but never stored anything in it. load_fonts() now keeps each (file, size) it loads there, so clear_font_cache() and get_font_cache_stats() mean what they say and repeated load_fonts() calls reuse the fonts. get_text_width() no longer catches AttributeError for Pillow releases without ImageDraw.textlength; requirements.txt pins Pillow>=12.2. The class docstring describes what the helper does. Co-Authored-By: Claude Opus 5.5 * docs(common): fix wrong docstrings in api_helper, permission_utils, snapshot_policy - permission_utils called 0o2775 "sticky bit"; the 2 is setgid, which is what makes new files take the directory's group. - snapshot_policy pointed at web_interface/blueprints/api_v3.py, which is a package now; the health check is in api_v3/misc.py. - APIHelper.clear_cache() lost a history note and a fallback to a clear() method that neither CacheManager nor the testing MockCacheManager has. The session headers are built from DEFAULT_HTTP_HEADERS instead of a copy of them, and the module docstring says what the module offers. Co-Authored-By: Claude Opus 5.5 * docs(sports): present-tense comments in the shared scoreboard renderers - sports_scroll and sports_game_renderer comments that referred to "this PR", "the old flat 128px card" or what the renderer "previously" did now describe the current behaviour and its reason. - The block explaining why non-finite settings are rejected sat above _score_reserve_width; it describes _center_gap_width and now lives in it. - unshare_element_fonts wrapped its import of font_layout.load_truetype in an `except ImportError` that cannot fire inside core; the import stays at call time so tests can spy on the pinned loader. - sports_card docstrings that told the history of a fix say what the code does. Co-Authored-By: Claude Opus 5.5 * refactor(sports-shared): drop dead code, name the ESPN limit - _get_weeks_data asked for limit=1000, which fetch_espn_scoreboard clamps to ESPN_MAX_LIMIT anyway; it now names that constant. Its unused `immediate_events = []` is gone. - _get_season_schedule_dates() returned ("", "") and has no caller in core or the plugin monorepo. - _should_log keeps its warning_type parameter (part of the inherited signature, though nothing in core or the monorepo calls it) and its docstring says the cooldown is shared across types. - An unused ImageFont import is gone. Co-Authored-By: Claude Opus 5.5 * refactor(sync): one follower-mode switch, shared panel defaults - The class docstring said the leader sends PNG frames. Frames go over UDP as raw RGB; PNG is only the Vegas scroll image sent over TCP. It now describes both paths. - _enter_follower_mode() replaces the two copies of "note the leader, switch from standalone to follower, log, write status" in the frame and scroll-position handlers. - The rows/cols fallbacks use DEFAULT_ROWS / DEFAULT_COLS from src.display_geometry, as chain_length already did. Co-Authored-By: Claude Opus 5.5 * refactor(style): drop _layout_axis, name the layout group title - ElementStyleResolver._layout_axis() had no caller in core or the plugin monorepo. - _element_block_from_spec checked spec['size'] was a dict again after size_spec already had; it reads size_spec. - The "Layout Offsets" title written into three generated schema blocks is _LAYOUT_TITLE. Co-Authored-By: Claude Opus 5.5 * docs(logo-helper): say what the placeholder draws; name the 1.5 box factor - _create_placeholder_logo's docstring said it draws the team abbreviation; it draws an outlined grey box and nothing else. The docstring says so, and the "in a real implementation you'd want text" comments are gone. - The 1.5 x panel default logo box, written out six times, is DEFAULT_LOGO_BOX_FACTOR. - ImageDraw is imported with Image at the top of the module. Co-Authored-By: Claude Opus 5.5 * refactor(logos): drop dead code and a duplicate regex in logo_downloader - _SAFE_LEAGUE_CODE_RE was the same pattern as _SAFE_LEAGUE_RE; both checks use the one. - get_logo_filename_variations reassigned the TA&M case to the list it already had; the function returns the two names directly. - _get_team_name_variations() had no caller in core or the plugin monorepo. - fetch_single_team's docstring was copied from fetch_teams_data; a log message read "for{team_id}". Co-Authored-By: Claude Opus 5.5 * refactor: drop the Pillow<9.1 resample shim and a catch-and-reraise - adaptive_images fell back to Image.LANCZOS/NEAREST for Pillow < 9.1; requirements.txt pins Pillow>=12.2. RESAMPLE_LANCZOS and RESAMPLE_NEAREST keep their names (src.common re-exports them). - CacheManager.save_cache caught CacheError only to re-raise it; the disk write is now called directly, with the same result. Co-Authored-By: Claude Opus 5.5 * test(api-helper): stop the real CacheManager's cleanup thread The cache-lifetime tests built a CacheManager and left its cleanup thread's class-wide claim on the directory in place, which broke test_cache_cleanup_thread_ownership when it ran later in the session. The fixture now stops the thread on teardown. Co-Authored-By: Claude Opus 5.5 * docs(changelog): core-common Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- CHANGELOG.md | 12 + src/adaptive_images.py | 10 +- src/background_data_service.py | 81 +-- src/backup_manager.py | 177 +++--- src/base_odds_manager.py | 31 +- src/cache_manager.py | 10 +- src/common/api_helper.py | 82 +-- src/common/font_layout.py | 9 +- src/common/logo_helper.py | 85 ++- src/common/permission_utils.py | 12 +- src/common/snapshot_policy.py | 6 +- src/common/sports_card.py | 26 +- src/common/sports_game_renderer.py | 34 +- src/common/sports_scroll.py | 6 +- src/common/sports_shared.py | 52 +- src/common/sync_manager.py | 56 +- src/common/text_helper.py | 48 +- src/config_manager.py | 67 +-- src/dynamic_team_resolver.py | 6 +- src/element_style.py | 56 +- src/error_aggregator.py | 19 +- src/font_manager.py | 127 ++-- src/logo_downloader.py | 123 ++-- src/plugin_system/plugin_executor.py | 2 +- src/plugin_system/plugin_manager.py | 3 +- src/startup_validator.py | 26 +- src/wifi_manager.py | 613 +++++++++----------- test/test_api_helper.py | 55 +- test/test_api_v3_wifi_endpoints.py | 8 + test/test_background_data_service.py | 3 - test/test_backup_manager.py | 25 + test/test_base_odds_manager.py | 3 - test/test_element_visibility_align_scale.py | 32 + test/test_error_aggregator.py | 39 ++ test/test_font_manager.py | 38 ++ test/test_font_pixel_grid.py | 20 + test/test_http_headers.py | 22 + test/test_logo_downloader.py | 27 + test/test_plugin_loading_failures.py | 25 + test/test_startup_validator.py | 9 + test/test_sync_manager.py | 9 + test/test_text_helper.py | 22 +- test/test_wifi_manager_ap.py | 94 +++ web_interface/blueprints/api_v3/misc.py | 11 +- web_interface/blueprints/api_v3/wifi.py | 7 +- 45 files changed, 1197 insertions(+), 1031 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 90421aac..dbace593 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,18 @@ accepts both, but the store flags the old spelling as deprecated - `reload_plugin` reads the manifest from the plugin's discovered directory. - Removed: `last_display` from plugin state info and `get_last_display()` (nothing recorded them); `PluginOperationQueue`'s `history_file` and `lazy_load` arguments; and `data/plugin_operations.json`, which nothing read. +- Core service fixes: + - `/api/v3/errors` shows each exception's real stack trace instead of `NoneType: None`. + - Wi-Fi disconnect takes the saved connection profile down. + - `wifi_config.json` is written atomically, and a save that fails now gets a 500. + - `plugin://` fonts load from the plugin's own install directory. `FontManager.register_plugin_fonts()` takes an optional `plugin_dir`. + - `APIHelper` keeps cached responses for the `cache_ttl` it was given, instead of always 300 s. + - Logo scales from 0.1 to 10 are honoured everywhere; values outside that range are clamped. + - `LogoHelper` and `logo_downloader`: an empty ESPN logo list counts as a failed download, and the placeholder is written at the requested path. + - Bundled font paths no longer depend on the directory the process was started from. + - Backups record `src.__version__`. + - Removed: `BackgroundDataService`'s `queue_size` stat and `clear_completed_requests()`. + - The web service (`ledmatrix-web`) logs through `src.logging_config` like the display service, so `journalctl -p err -u ledmatrix-web` works. Successful GET/HEAD/OPTIONS requests (the UI's polling) are logged at DEBUG instead of diff --git a/src/adaptive_images.py b/src/adaptive_images.py index 72c0907a..c7a35a36 100644 --- a/src/adaptive_images.py +++ b/src/adaptive_images.py @@ -29,13 +29,9 @@ from typing import Any, Optional, Tuple from PIL import Image -# The one Pillow >= 9.1 compat shim (replaces the per-plugin copies). -try: - RESAMPLE_LANCZOS = Image.Resampling.LANCZOS - RESAMPLE_NEAREST = Image.Resampling.NEAREST -except AttributeError: # Pillow < 9.1 - RESAMPLE_LANCZOS = Image.LANCZOS - RESAMPLE_NEAREST = Image.NEAREST +# Re-exported by src.common for plugins, which import them from there. +RESAMPLE_LANCZOS = Image.Resampling.LANCZOS +RESAMPLE_NEAREST = Image.Resampling.NEAREST FIT_MODES = ("contain", "cover", "fill_height", "stretch") diff --git a/src/background_data_service.py b/src/background_data_service.py index a41aef45..15ac2b4b 100644 --- a/src/background_data_service.py +++ b/src/background_data_service.py @@ -131,19 +131,14 @@ class BackgroundDataService: # Thread management self.executor = ThreadPoolExecutor(max_workers=max_workers, thread_name_prefix="BackgroundData") - # cache_key -> request_id for fetches currently in flight. Submitting - # the same key twice used to start two identical fetches: request_id - # carries a millisecond timestamp, so every submit looked new, and - # active_requests is keyed by it rather than by what is being fetched. - # On a real board the season-schedule key is requested by both the - # Recent and the Upcoming manager, which miss the cache in the same - # millisecond and each download and parse the same payload. + # cache_key -> request_id for fetches currently in flight, so a second + # submit for the same key joins the running fetch instead of starting + # another. It is the normal case: a sport's Recent and Upcoming + # managers miss the cache for the same season schedule together. self._inflight_by_cache_key: Dict[str, str] = {} - # request_id was sport_year_milliseconds, which is not unique: two - # submits inside the same millisecond produced the SAME id, so one - # silently replaced the other in active_requests and completed_requests. - # Rare before, but dedupe hands this id back to every joiner as their - # handle for get_result(), so it has to be unique. A counter is enough. + # Makes every request_id unique. The id also carries a millisecond + # timestamp, but two submits can share a millisecond, and a joiner + # uses the id as its handle for get_result(). self._request_seq = itertools.count() self.active_requests: Dict[str, FetchRequest] = {} self.completed_requests: Dict[str, FetchResult] = {} @@ -186,9 +181,9 @@ class BackgroundDataService: This ensures Recent/Upcoming managers and background service use the same cache keys. """ - # Same format as CacheManager.generate_sport_cache_key(). This used to - # build a whole CacheManager to call it -- config load, cache-dir - # probing with test writes -- on every submit without a cache_key. + # Same format as CacheManager.generate_sport_cache_key(), built here + # rather than by constructing a CacheManager (config load, cache-dir + # probing) on every submit without a cache_key. if date_str is None: date_str = datetime.now(pytz.utc).strftime('%Y%m%d') return f"{sport}_{date_str}" @@ -331,10 +326,8 @@ class BackgroundDataService: try: with self._lock: - # A request cancelled while it sat in the executor queue must - # stay cancelled. Overwriting the status here undid the cancel - # outright: the worker went on to download, cache and call back - # for work the caller had already withdrawn. + # A request cancelled while it sat in the executor queue stays + # cancelled: no download, no cache write, no callback. if request.status == FetchStatus.CANCELLED: cancelled_before_start = True else: @@ -463,10 +456,9 @@ class BackgroundDataService: logger.error(f"Failed to fetch {request.sport} {request.year} data: {error_msg}") with self._lock: - # Don't relabel a cancelled request. The callback gate in the - # finally block only suppresses CANCELLED, so promoting it to - # FAILED here delivered an error callback for a fetch nobody - # was waiting on any more. + # A cancelled request stays CANCELLED even when its fetch + # failed: the finally block skips callbacks only for + # CANCELLED, and nobody is waiting on this fetch any more. if request.status != FetchStatus.CANCELLED: request.status = FetchStatus.FAILED request.error = error_msg @@ -526,20 +518,13 @@ class BackgroundDataService: except Exception as e: logger.error(f"Error in callback for request {request.id}: {e}") - # Released AFTER the loop, not inside it. Every callback here holds - # the same FetchResult, so releasing per-delivery handed the first - # one the data and every joiner `result.data is None` -- which is - # not a quiet degradation: they read `result.data.get('events')` and - # raise AttributeError, which this very loop catches and logs, so - # the symptom was one ERROR line and a manager that silently never - # got its schedule. Deduplication is the normal case, not a corner: - # a sport's recent, upcoming and live managers all ride one season - # fetch. + # Released after the loop, never inside it: every callback holds + # the same FetchResult (a sport's recent, upcoming and live + # managers usually share one fetch), so a release between + # deliveries would hand the later ones `result.data is None`. # - # Guarded on `callbacks`, because a request submitted without one - # has no other way to collect its payload than polling get_result(). - # The old per-delivery release got that right by accident: an empty - # list never entered the loop body. + # Only when there were callbacks: a request submitted without one + # collects its payload by polling get_result(). if callbacks: self._release_payload(result) request.result = None @@ -721,9 +706,6 @@ class BackgroundDataService: 'completed_requests_count': len(self.completed_requests), 'max_completed_requests': self._max_completed_requests, 'completed_requests_usage_percent': (len(self.completed_requests) / self._max_completed_requests * 100) if self._max_completed_requests > 0 else 0, - # Nothing is queued outside the executor; kept for callers - # that read the key. - 'queue_size': 0, 'last_cleanup': self._last_completed_requests_cleanup, 'cleanup_interval': self._completed_requests_cleanup_interval } @@ -793,27 +775,6 @@ class BackgroundDataService: return removed_count - def clear_completed_requests(self, older_than_hours: int = 24): - """ - Clear completed requests older than specified time. - - Args: - older_than_hours: Clear requests older than this many hours - """ - cutoff_time = time.time() - (older_than_hours * 3600) - - with self._lock: - to_remove = [] - for request_id, result in self.completed_requests.items(): - if result.completed_at < cutoff_time: - to_remove.append(request_id) - - for request_id in to_remove: - del self.completed_requests[request_id] - - if to_remove: - logger.info(f"Cleared {len(to_remove)} old completed requests") - def shutdown(self, wait: bool = True): """ Shutdown the background data service. diff --git a/src/backup_manager.py b/src/backup_manager.py index 13114114..490db4dd 100644 --- a/src/backup_manager.py +++ b/src/backup_manager.py @@ -83,14 +83,25 @@ BUNDLED_FONTS: frozenset[str] = frozenset({ _CONFIG_REL = Path("config/config.json") _SECRETS_REL = Path("config/config_secrets.json") _WIFI_REL = Path("config/wifi_config.json") -# Sits in config/ next to the three above and is pure user state — a -# YouTube Music session that has to be re-authenticated by hand if lost. -# It was omitted from backups, so a restore silently signed the user out. +# A YouTube Music session: pure user state that has to be re-authenticated by +# hand if lost, so a restore must bring it back. _YTM_REL = Path("config/ytm_auth.json") _FONTS_REL = Path("assets/fonts") _PLUGIN_UPLOADS_REL = Path("assets/plugins") _STATE_REL = Path("data/plugin_state.json") +#: The sections that are one file each: (section name, path, the +#: RestoreOptions flag that restores it). create, preview, validate and +#: restore all walk this table. ytm_auth follows restore_wifi: it is +#: device-local auth like the Wi-Fi settings, and a toggle of its own for one +#: file would be noise in the restore dialog. +_SINGLE_FILE_SECTIONS: Tuple[Tuple[str, Path, str], ...] = ( + ("config", _CONFIG_REL, "restore_config"), + ("secrets", _SECRETS_REL, "restore_secrets"), + ("wifi", _WIFI_REL, "restore_wifi"), + ("ytm_auth", _YTM_REL, "restore_wifi"), +) + MANIFEST_NAME = "manifest.json" PLUGINS_MANIFEST_NAME = "plugins.json" @@ -140,34 +151,18 @@ class RestoreResult: # --------------------------------------------------------------------------- -def _ledmatrix_version(project_root: Path) -> str: - """Best-effort version string for the current install.""" - version_file = project_root / "VERSION" - if version_file.exists(): - try: - return version_file.read_text(encoding="utf-8").strip() or "unknown" - except OSError: - pass - head_file = project_root / ".git" / "HEAD" - if head_file.exists(): - try: - head = head_file.read_text(encoding="utf-8").strip() - if head.startswith("ref: "): - ref = head[5:] - ref_path = project_root / ".git" / ref - if ref_path.exists(): - return ref_path.read_text(encoding="utf-8").strip()[:12] or "unknown" - return head[:12] or "unknown" - except OSError: - pass - return "unknown" +def _ledmatrix_version() -> str: + """The release of the running core (``src.__version__``), recorded in the + manifest so a restore can tell which release wrote the backup.""" + from src import __version__ + return __version__ -def _build_manifest(contents: List[str], project_root: Path) -> Dict[str, Any]: +def _build_manifest(contents: List[str]) -> Dict[str, Any]: return { "schema_version": SCHEMA_VERSION, "created_at": datetime.now(timezone.utc).isoformat().replace("+00:00", "Z"), - "ledmatrix_version": _ledmatrix_version(project_root), + "ledmatrix_version": _ledmatrix_version(), "hostname": socket.gethostname(), "contents": contents, } @@ -178,13 +173,34 @@ def _build_manifest(contents: List[str], project_root: Path) -> Dict[str, Any]: # --------------------------------------------------------------------------- +def _plugins_directory(project_root: Path) -> Path: + """The plugin install directory: ``plugin_system.plugins_directory`` from + config/config.json (relative to ``project_root`` unless absolute), or + ``plugin-repos`` when the config does not say or cannot be read.""" + configured: Any = None + try: + with (project_root / _CONFIG_REL).open("r", encoding="utf-8") as f: + config = json.load(f) + if isinstance(config, dict): + plugin_system = config.get("plugin_system") + if isinstance(plugin_system, dict): + configured = plugin_system.get("plugins_directory") + except (OSError, json.JSONDecodeError): + pass + if not isinstance(configured, str) or not configured.strip(): + configured = "plugin-repos" + path = Path(configured) + return path if path.is_absolute() else project_root / path + + def list_installed_plugins(project_root: Path) -> List[Dict[str, Any]]: """ Return a list of currently-installed plugins suitable for the backup manifest. Each entry has ``plugin_id`` and ``version``. - Reads ``data/plugin_state.json`` if present; otherwise walks the plugin - directory and reads each ``manifest.json``. + Reads ``data/plugin_state.json`` if present, then adds any plugin it + does not list from the ``manifest.json`` files in the configured plugin + directory (see :func:`_plugins_directory`). """ plugins: Dict[str, Dict[str, Any]] = {} @@ -206,8 +222,7 @@ def list_installed_plugins(project_root: Path) -> List[Dict[str, Any]]: except (OSError, json.JSONDecodeError) as e: logger.warning("Could not read plugin_state.json: %s", e) - # Fall back to scanning plugin-repos/ for manifests. - plugins_root = project_root / "plugin-repos" + plugins_root = _plugins_directory(project_root) if plugins_root.exists(): for entry in sorted(plugins_root.iterdir()): if not entry.is_dir(): @@ -298,19 +313,10 @@ def create_backup( tmp_path = zip_path.with_suffix(".zip.tmp") try: with zipfile.ZipFile(tmp_path, "w", compression=zipfile.ZIP_DEFLATED) as zf: - # Config files. - if (project_root / _CONFIG_REL).exists(): - zf.write(project_root / _CONFIG_REL, _CONFIG_REL.as_posix()) - contents.append("config") - if (project_root / _SECRETS_REL).exists(): - zf.write(project_root / _SECRETS_REL, _SECRETS_REL.as_posix()) - contents.append("secrets") - if (project_root / _WIFI_REL).exists(): - zf.write(project_root / _WIFI_REL, _WIFI_REL.as_posix()) - contents.append("wifi") - if (project_root / _YTM_REL).exists(): - zf.write(project_root / _YTM_REL, _YTM_REL.as_posix()) - contents.append("ytm_auth") + for section, rel, _flag in _SINGLE_FILE_SECTIONS: + if (project_root / rel).exists(): + zf.write(project_root / rel, rel.as_posix()) + contents.append(section) # User-uploaded fonts. user_fonts = iter_user_fonts(project_root) @@ -338,7 +344,7 @@ def create_backup( contents.append("plugins") # Manifest goes last so that `contents` reflects what we actually wrote. - manifest = _build_manifest(contents, project_root) + manifest = _build_manifest(contents) zf.writestr(MANIFEST_NAME, json.dumps(manifest, indent=2)) os.replace(tmp_path, zip_path) @@ -352,15 +358,16 @@ def create_backup( def preview_backup_contents(project_root: Path) -> Dict[str, Any]: """Return a summary of what ``create_backup`` would include.""" project_root = Path(project_root).resolve() - return { - "has_config": (project_root / _CONFIG_REL).exists(), - "has_secrets": (project_root / _SECRETS_REL).exists(), - "has_wifi": (project_root / _WIFI_REL).exists(), - "has_ytm_auth": (project_root / _YTM_REL).exists(), + preview: Dict[str, Any] = { + f"has_{section}": (project_root / rel).exists() + for section, rel, _flag in _SINGLE_FILE_SECTIONS + } + preview.update({ "user_fonts": [p.name for p in iter_user_fonts(project_root)], "plugin_uploads": len(iter_plugin_uploads(project_root)), "plugins": list_installed_plugins(project_root), - } + }) + return preview # --------------------------------------------------------------------------- @@ -431,15 +438,10 @@ def validate_backup(zip_path: Path) -> Tuple[bool, str, Dict[str, Any]]: {}, ) - detected: List[str] = [] - if _CONFIG_REL.as_posix() in names: - detected.append("config") - if _SECRETS_REL.as_posix() in names: - detected.append("secrets") - if _WIFI_REL.as_posix() in names: - detected.append("wifi") - if _YTM_REL.as_posix() in names: - detected.append("ytm_auth") + detected: List[str] = [ + section for section, rel, _flag in _SINGLE_FILE_SECTIONS + if rel.as_posix() in names + ] if any(n.startswith(_FONTS_REL.as_posix() + "/") for n in names): detected.append("fonts") if any( @@ -584,55 +586,18 @@ def restore_backup( result.errors.append("Failed to extract backup") return result - # Main config. - if options.restore_config and (tmp_dir / _CONFIG_REL).exists(): + for section, rel, flag in _SINGLE_FILE_SECTIONS: + if not (tmp_dir / rel).exists(): + continue + if not getattr(options, flag): + result.skipped.append(section) + continue try: - _copy_file(tmp_dir / _CONFIG_REL, project_root / _CONFIG_REL) - result.restored.append("config") + _copy_file(tmp_dir / rel, project_root / rel) + result.restored.append(section) except OSError as e: - logger.error("[Backup] Failed to restore config.json: %s", e, exc_info=True) - result.errors.append("Failed to restore config.json") - elif (tmp_dir / _CONFIG_REL).exists(): - result.skipped.append("config") - - # Secrets. - if options.restore_secrets and (tmp_dir / _SECRETS_REL).exists(): - try: - _copy_file(tmp_dir / _SECRETS_REL, project_root / _SECRETS_REL) - result.restored.append("secrets") - except OSError as e: - logger.error( - "[Backup] Failed to restore config_secrets.json: %s", e, exc_info=True - ) - result.errors.append("Failed to restore config_secrets.json") - elif (tmp_dir / _SECRETS_REL).exists(): - result.skipped.append("secrets") - - # WiFi. - if options.restore_wifi and (tmp_dir / _WIFI_REL).exists(): - try: - _copy_file(tmp_dir / _WIFI_REL, project_root / _WIFI_REL) - result.restored.append("wifi") - except OSError as e: - logger.error( - "[Backup] Failed to restore wifi_config.json: %s", e, exc_info=True - ) - result.errors.append("Failed to restore wifi_config.json") - elif (tmp_dir / _WIFI_REL).exists(): - result.skipped.append("wifi") - - # YouTube Music session. Follows restore_wifi rather than getting its - # own flag: it is device-local auth in the same sense, and a separate - # toggle for one file would be noise in the restore dialog. - if options.restore_wifi and (tmp_dir / _YTM_REL).exists(): - try: - _copy_file(tmp_dir / _YTM_REL, project_root / _YTM_REL) - result.restored.append("ytm_auth") - except OSError as e: - logger.error("[Backup] Failed to restore ytm_auth.json: %s", e, exc_info=True) - result.errors.append("Failed to restore ytm_auth.json") - elif (tmp_dir / _YTM_REL).exists(): - result.skipped.append("ytm_auth") + logger.error("[Backup] Failed to restore %s: %s", rel.name, e, exc_info=True) + result.errors.append(f"Failed to restore {rel.name}") # User fonts — skip anything that collides with a bundled font. tmp_fonts = tmp_dir / _FONTS_REL diff --git a/src/base_odds_manager.py b/src/base_odds_manager.py index efb10bba..548dcfba 100644 --- a/src/base_odds_manager.py +++ b/src/base_odds_manager.py @@ -18,6 +18,8 @@ import requests import json from typing import Dict, Any, Optional, List +from src.common.api_helper import DEFAULT_HTTP_HEADERS + class BaseOddsManager: """ @@ -45,22 +47,15 @@ class BaseOddsManager: self.logger = logging.getLogger(__name__) self.base_url = "https://sports.core.api.espn.com/v2/sports" - # This path used a bare requests.get, so it identified itself as - # python-requests/x.y -- the one thing ESPN is known to reject. Around - # 2026-08-04 it began 403ing browser strings and bare custom tokens - # alike; what it accepts is a token with a URL that says who is - # calling. Every other ESPN caller in the tree already sends this - # (src/common/api_helper.py); the odds path was simply missed, and it is the one whose failures cost - # the caller its whole update budget. + # Core's shared headers: ESPN rejects requests' default User-Agent + # (see api_helper.USER_AGENT), and a rejected odds request costs the + # calling plugin its update budget. # # Deliberately no retry adapter, unlike api_helper: retries multiply # request_timeout, which is set to 5s precisely to stay inside that # budget. One try, then the cooldown below. self.session = requests.Session() - self.session.headers.update({ - 'User-Agent': 'LEDMatrix/1.0 (+https://github.com/ChuckBuilds/LEDMatrix)', - 'Accept': 'application/json', - }) + self.session.headers.update(DEFAULT_HTTP_HEADERS) # Configuration with defaults self.update_interval = 3600 # 1 hour default @@ -72,7 +67,6 @@ class BaseOddsManager: self.request_timeout = 5 # Set when a request fails; until then, skip the network entirely. self._skip_network_until = 0.0 - self.cache_ttl = 1800 # 30 minutes default # Load configuration if available if config_manager: @@ -89,12 +83,10 @@ class BaseOddsManager: self.update_interval = odds_config.get('update_interval', self.update_interval) self.request_timeout = odds_config.get('timeout', self.request_timeout) - self.cache_ttl = odds_config.get('cache_ttl', self.cache_ttl) - + self.logger.debug(f"BaseOddsManager configuration loaded: " f"update_interval={self.update_interval}s, " - f"timeout={self.request_timeout}s, " - f"cache_ttl={self.cache_ttl}s") + f"timeout={self.request_timeout}s") except Exception as e: self.logger.warning(f"Failed to load BaseOddsManager configuration: {e}") @@ -172,15 +164,12 @@ class BaseOddsManager: odds_data = self._extract_espn_data(raw_data) if odds_data: self.logger.info(f"Successfully extracted odds data: {odds_data}") - else: - self.logger.debug("No odds data available for this game") - - if odds_data: self.cache_manager.set(cache_key, odds_data, ttl=interval) self.logger.info(f"Saved odds data to cache for {cache_key} with TTL {interval}s") else: self.logger.debug(f"No odds data available for {cache_key}") - # Cache the fact that no odds are available to avoid repeated API calls + # Cache the absence too, so the game is not re-requested + # on every update until the interval passes. self.cache_manager.set(cache_key, {"no_odds": True}, ttl=interval) return odds_data diff --git a/src/cache_manager.py b/src/cache_manager.py index d064a37f..ee4f46a6 100644 --- a/src/cache_manager.py +++ b/src/cache_manager.py @@ -32,7 +32,6 @@ from typing import Any, Dict, List, Optional import logging import threading import tempfile -from src.exceptions import CacheError from src.cache.memory_cache import MemoryCache, default_max_size from src.cache.disk_cache import DiskCache from src.cache.cache_strategy import CacheStrategy @@ -272,12 +271,9 @@ class CacheManager: # Update memory cache first self._memory_cache_component.set(key, data) - # Save to disk cache - try: - self._disk_cache_component.set(key, data) - except CacheError: - # Disk cache errors are already logged and raised by DiskCache - raise + # DiskCache logs a failed write and raises CacheError, which the + # caller gets as is. + self._disk_cache_component.set(key, data) def load_cache(self, key: str) -> Optional[Dict[str, Any]]: """Load data from cache with memory caching.""" diff --git a/src/common/api_helper.py b/src/common/api_helper.py index d025b378..2e5603f9 100644 --- a/src/common/api_helper.py +++ b/src/common/api_helper.py @@ -1,8 +1,9 @@ """ API Helper -Handles HTTP requests, caching, and ESPN API integration for LED matrix plugins. -Extracted from LEDMatrix core to provide reusable functionality for plugins. +HTTP requests, response caching and ESPN fetch helpers for plugins +(``from src.common import APIHelper``), plus the headers every core request +sends (:data:`USER_AGENT`, :data:`DEFAULT_HTTP_HEADERS`). """ import logging @@ -36,13 +37,20 @@ DEFAULT_HTTP_HEADERS: Mapping[str, str] = MappingProxyType({ class APIHelper: """ - Helper class for HTTP requests, caching, and ESPN API integration. - - Provides functionality for: - - HTTP requests with retry logic and timeouts - - Response caching with TTL support - - ESPN API integration for sports data - - Request rate limiting and throttling + HTTP requests with retries, response caching and ESPN helpers. + + - Requests go through one ``requests.Session`` that retries GET, HEAD + and OPTIONS on 429 and 5xx with exponential backoff, and sends + :data:`DEFAULT_HTTP_HEADERS`. + - Consecutive requests from one helper are spaced at least + ``set_rate_limit()`` seconds apart (1 second by default). A cache hit + does not count. + - With a ``cache_manager``, :meth:`get` caches the parsed JSON under + ``cache_key`` for ``cache_ttl`` seconds. The lifetime is stored with + the entry, so CacheManager honours it on every later read, whatever + max_age that read asks for. + - Failed requests are logged and return None; nothing here raises for a + network or HTTP error. """ def __init__(self, cache_manager=None, default_timeout: int = 30, @@ -73,13 +81,7 @@ class APIHelper: self.session.mount("https://", adapter) self.session.mount("http://", adapter) - # Default headers - self.session.headers.update({ - 'User-Agent': USER_AGENT, - 'Accept': 'application/json', - 'Accept-Language': 'en-US,en;q=0.9', - 'Connection': 'keep-alive' - }) + self.session.headers.update({**DEFAULT_HTTP_HEADERS, 'Connection': 'keep-alive'}) # Rate limiting self._last_request_time = 0 @@ -102,9 +104,8 @@ class APIHelper: Returns: Response data as dictionary or None if request fails """ - # Check cache first if cache_key and self.cache_manager: - cached = self._get_from_cache(cache_key) + cached = self._get_from_cache(cache_key, cache_ttl) if cached is not None: self.logger.debug(f"Using cached response for {cache_key}") return cached @@ -268,33 +269,31 @@ class APIHelper: Args: key: Cache key data: Data to cache - ttl: Time-to-live in seconds (ignored - CacheManager doesn't support TTL) + ttl: Seconds the entry stays valid. Stored with the entry, so + it applies to every later read of ``key``. """ - if self.cache_manager: - self.cache_manager.set(key, data) + self._set_cache(key, data, ttl) def get_cache(self, key: str) -> Optional[Any]: """ Get cached data. - + Args: key: Cache key - + Returns: - Cached data or None if not found + Cached data, or None if there is none or it has expired. An + entry written with a ttl (set_cache, get) expires after that ttl; + one written without expires after CacheManager's default max_age. """ - if self.cache_manager: - return self.cache_manager.get(key) - return None + return self._get_from_cache(key) def clear_cache(self, pattern: Optional[str] = None) -> None: """ Clear cache data. - Uses CacheManager's real surface (clear_cache / delete / - list_cache_files); safely no-ops on managers without it. The old - implementation guarded on a nonexistent ``clear`` method, so it - silently never cleared anything. + Uses CacheManager's clear_cache(), or list_cache_files() and delete() + for a pattern. A cache manager without those methods is left alone. Args: pattern: Optional substring to match cache keys; only matching @@ -315,21 +314,22 @@ class APIHelper: "cannot clear by pattern") elif hasattr(self.cache_manager, 'clear_cache'): self.cache_manager.clear_cache() - elif hasattr(self.cache_manager, 'clear'): - self.cache_manager.clear() else: self.logger.debug("Cache manager exposes no clear method; no-op") - def _get_from_cache(self, key: str) -> Optional[Any]: - """Get data from cache.""" - if self.cache_manager: + def _get_from_cache(self, key: str, max_age: Optional[int] = None) -> Optional[Any]: + """Cached data for ``key``, or None. ``max_age`` only matters for an + entry stored without a ttl; one stored with a ttl uses that.""" + if not self.cache_manager: + return None + if max_age is None: return self.cache_manager.get(key) - return None - - def _set_cache(self, key: str, data: Any, ttl: int) -> None: - """Set data in cache.""" + return self.cache_manager.get(key, max_age=max_age) + + def _set_cache(self, key: str, data: Any, ttl: Optional[int]) -> None: + """Store ``data`` under ``key`` for ``ttl`` seconds.""" if self.cache_manager: - self.cache_manager.set(key, data) + self.cache_manager.set(key, data, ttl=ttl) def _enforce_rate_limit(self) -> None: """Enforce rate limiting between requests.""" diff --git a/src/common/font_layout.py b/src/common/font_layout.py index 097e604a..9441b0ef 100644 --- a/src/common/font_layout.py +++ b/src/common/font_layout.py @@ -67,10 +67,11 @@ _INSTALL_ROOT = Path(__file__).resolve().parents[2] def resolve_asset_path(relative_path: str) -> str: """Resolve a repo-relative asset path independently of the process cwd. - Prefers the path as given — so an absolute path is returned untouched and - behaviour is unchanged wherever the cwd already happened to be the install - root — then the install root derived above, then the original string so a - caller that wants to raise and fall back still can. + In order: an absolute path that exists is returned untouched; otherwise + ``relative_path`` under the install root derived above, if that exists; + otherwise ``relative_path`` unchanged, so a caller that wants to raise + and fall back still can. The cwd is never consulted, so a relative path + means the same file whichever directory the process started in. Without the fallback, any process started outside the install root (the plugin safety harness, a manual ``python run.py`` from ``$HOME``, a unit diff --git a/src/common/logo_helper.py b/src/common/logo_helper.py index a574b89c..9ead2536 100644 --- a/src/common/logo_helper.py +++ b/src/common/logo_helper.py @@ -11,7 +11,7 @@ from pathlib import Path from typing import Dict, List, Optional, Union import requests -from PIL import Image +from PIL import Image, ImageDraw from src.common.api_helper import USER_AGENT from src.common.permission_utils import ( ensure_directory_permissions, @@ -32,35 +32,14 @@ from src.common.permission_utils import ( # trade for not re-warning about a file nobody is going to add. MISSING_LOGO_RECHECK_SECONDS = 3600.0 -#: Bounds on a user-supplied logo scale. Wide enough to be useful, closed -#: enough that a typo cannot ask for a 4000px image on a 64px panel. -MIN_LOGO_SCALE = 0.05 -MAX_LOGO_SCALE = 8.0 - - -def _usable_scale(scale) -> float: - """A scale that can be applied, or 1.0. - - Anything unusable -- None, a string, zero, a negative, NaN, infinity -- - means "as shipped", because the alternative is a blank panel from a - mistyped number. - """ - try: - value = float(scale) - except (TypeError, ValueError): - return 1.0 - if value != value or value in (float('inf'), float('-inf')): - return 1.0 - if value < MIN_LOGO_SCALE or value > MAX_LOGO_SCALE: - return 1.0 - return value - - - # Well above any real team logo; bounds what a remote URL can write to disk. # The cap for every logo download: src.logo_downloader.fetch_logo uses it too. MAX_LOGO_BYTES = 10 * 1024 * 1024 +#: A logo's default bounding box, as a multiple of the panel's width and +#: height, when the caller gives no max_width / max_height. +DEFAULT_LOGO_BOX_FACTOR = 1.5 + class LogoHelper: """ @@ -119,12 +98,16 @@ class LogoHelper: Args: team_abbr: Team abbreviation for caching logo_path: Path to the logo file - max_width: Maximum width (defaults to display_width * 1.5) - max_height: Maximum height (defaults to display_height * 1.5) + max_width: Maximum width (default display_width * + DEFAULT_LOGO_BOX_FACTOR) + max_height: Maximum height (default display_height * + DEFAULT_LOGO_BOX_FACTOR) scale: User's size multiplier for this image, from - ``customization.layout..scale``. 1.0 is untouched and - takes exactly the path it always did. Callers hold the config, - so they resolve the element name; this only applies the number. + ``customization.layout..scale``; 1.0 leaves the box + as is. Callers hold the config, so they resolve the element + name; this only applies the number, clamped to + src.element_style's MIN_ELEMENT_SCALE..MAX_ELEMENT_SCALE. + A value that is not a finite positive number means 1.0. Returns: PIL Image object or None if loading fails @@ -138,10 +121,13 @@ class LogoHelper: # key is size-qualified — a panel-size change must not return a # logo resized for the old dimensions. if max_width is None: - max_width = int(self.display_width * 1.5) + max_width = int(self.display_width * DEFAULT_LOGO_BOX_FACTOR) if max_height is None: - max_height = int(self.display_height * 1.5) - scale = _usable_scale(scale) + max_height = int(self.display_height * DEFAULT_LOGO_BOX_FACTOR) + # Imported here: src.element_style imports src.common (for bdf_font), + # whose __init__ imports this module. + from src.element_style import coerce_scale + scale = coerce_scale(scale, 1.0) if scale != 1.0: max_width = max(1, int(round(max_width * scale))) max_height = max(1, int(round(max_height * scale))) @@ -374,9 +360,9 @@ class LogoHelper: nobody asked to grow would change every existing render. """ if max_width is None: - max_width = int(self.display_width * 1.5) + max_width = int(self.display_width * DEFAULT_LOGO_BOX_FACTOR) if max_height is None: - max_height = int(self.display_height * 1.5) + max_height = int(self.display_height * DEFAULT_LOGO_BOX_FACTOR) # Only resize if necessary if logo.width <= max_width and logo.height <= max_height: @@ -429,31 +415,26 @@ class LogoHelper: max_width: Optional[int] = None, max_height: Optional[int] = None) -> Optional[Image.Image]: """ - Create a placeholder logo with team abbreviation. - + A stand-in for a logo that could not be loaded or downloaded: a + translucent grey box with a light outline, filling the logo box. + No text is drawn; ``team_abbr`` is only used in log messages. + Args: - team_abbr: Team abbreviation to display - max_width: Maximum width - max_height: Maximum height - + team_abbr: Team the placeholder stands in for + max_width: Width (default display_width * DEFAULT_LOGO_BOX_FACTOR) + max_height: Height (default display_height * DEFAULT_LOGO_BOX_FACTOR) + Returns: - PIL Image with placeholder logo + The RGBA placeholder, or None if it could not be created """ try: if max_width is None: - max_width = int(self.display_width * 1.5) + max_width = int(self.display_width * DEFAULT_LOGO_BOX_FACTOR) if max_height is None: - max_height = int(self.display_height * 1.5) + max_height = int(self.display_height * DEFAULT_LOGO_BOX_FACTOR) - # Create placeholder image placeholder = Image.new('RGBA', (max_width, max_height), (0, 0, 0, 0)) - - # This would require a font, so we'll create a simple colored rectangle - # In a real implementation, you'd want to add text rendering here - from PIL import ImageDraw draw = ImageDraw.Draw(placeholder) - - # Draw a simple rectangle with team abbreviation draw.rectangle([0, 0, max_width-1, max_height-1], fill=(100, 100, 100, 200), outline=(200, 200, 200, 255)) diff --git a/src/common/permission_utils.py b/src/common/permission_utils.py index f59b31cc..5e959577 100644 --- a/src/common/permission_utils.py +++ b/src/common/permission_utils.py @@ -241,7 +241,8 @@ def get_assets_dir_mode() -> int: Return permission mode for asset directories. Returns: - Permission mode: 0o2775 (rwxrwxr-x + sticky bit) for group-writable directories + Permission mode: 0o2775 (rwxrwsr-x): group-writable, and setgid so + entries created in it take the directory's group """ return 0o2775 # rwxrwsr-x (setgid + group writable) @@ -251,7 +252,8 @@ def get_config_dir_mode() -> int: Return permission mode for config directory. Returns: - Permission mode: 0o2775 (rwxrwxr-x + sticky bit) for group-writable directories + Permission mode: 0o2775 (rwxrwsr-x): group-writable, and setgid so + entries created in it take the directory's group """ return 0o2775 # rwxrwsr-x (setgid + group writable) @@ -271,7 +273,8 @@ def get_plugin_dir_mode() -> int: Return permission mode for plugin directories. Returns: - Permission mode: 0o2775 (rwxrwxr-x + sticky bit) for group-writable directories + Permission mode: 0o2775 (rwxrwsr-x): group-writable, and setgid so + entries created in it take the directory's group """ return 0o2775 # rwxrwsr-x (setgid + group writable) @@ -281,7 +284,8 @@ def get_cache_dir_mode() -> int: Return permission mode for cache directories. Returns: - Permission mode: 0o2775 (rwxrwxr-x + sticky bit) for group-writable cache directories + Permission mode: 0o2775 (rwxrwsr-x): group-writable, and setgid so + entries created in it take the directory's group """ return 0o2775 # rwxrwsr-x (setgid + group writable) diff --git a/src/common/snapshot_policy.py b/src/common/snapshot_policy.py index 20cc8b0d..b9399dd3 100644 --- a/src/common/snapshot_policy.py +++ b/src/common/snapshot_policy.py @@ -5,7 +5,7 @@ serves two consumers with different needs: - The web UI's live preview (SSE reader in web_interface/app.py) wants fresh frames — but only while a browser is actually watching. -- The health check (web_interface/blueprints/api_v3.py, hardware status) +- The health check (web_interface/blueprints/api_v3/misc.py, hardware status) uses the file's AGE as a liveness proxy: age >= 60s reads as degraded. PNG-encoding every frame at 5 fps forever — identical frames, no viewers — @@ -27,7 +27,7 @@ Policy: TOUCH_INTERVAL so the health check (60s threshold) never degrades. If any constant here changes, re-check the health threshold in -api_v3.py (get_hardware_status) — TOUCH_INTERVAL must stay well under it. +api_v3/misc.py (get_hardware_status) — TOUCH_INTERVAL must stay well under it. """ from enum import Enum @@ -37,7 +37,7 @@ VIEWER_INTERVAL = 0.2 # Snapshot cadence with no viewers — cheap freshness for page-open (seconds). IDLE_INTERVAL = 30.0 # Max age of the last write/touch before bumping mtime for the health -# check. MUST stay well under api_v3's 60s degraded threshold. +# check. MUST stay well under get_hardware_status's 60s degraded threshold. TOUCH_INTERVAL = 20.0 # A viewer marker older than this no longer counts as a live viewer. VIEWER_MARKER_FRESH_SEC = 5.0 diff --git a/src/common/sports_card.py b/src/common/sports_card.py index 2c5c88fb..3389f090 100644 --- a/src/common/sports_card.py +++ b/src/common/sports_card.py @@ -101,11 +101,10 @@ def element_color(config: Optional[Dict[str, Any]], element: str, mode: Optional[str] = None): """Per-element text colour from customization..text_color. - Delegated rather than reimplemented: there were two copies of this - read and three of the offset read, and the shared one also resolves - the element under the names plugins actually use (the layout block - says `score` where the style block says `score_text`) and honours a - per-mode override. Hex strings are still accepted. + Delegates to src.element_style.element_color, which also resolves the + element under the names plugins actually use (the layout block says + `score` where the style block says `score_text`) and honours a per-mode + override. Hex strings are accepted. """ from src.element_style import element_color as _shared return _shared(config, element, default, mode) @@ -125,12 +124,11 @@ def resolve_font_color(config: Optional[Dict[str, Any]], One object can legitimately belong to several elements -- a size resolver can land two of them on the same face, and a BDF face cannot be un-shared at all because ``freetype.Face`` objects cannot be rebuilt from a path. - Those draws used to go out white, which is how an element rendered in any - of the 32 shipped bitmap fonts could silently lose a colour the user had - set. So ambiguity is now narrowed before it is given up on: among the + Ambiguity is therefore narrowed before it is given up on: among the elements sharing a face, a single configured colour is the only thing the user can have meant, and several that agree mean the same thing. Only a - genuine disagreement falls back to *default*. + genuine disagreement falls back to *default* -- otherwise an element + drawn in any of the shipped bitmap fonts could lose a colour the user set. The element vocabulary is a parameter because the two callers disagree about it -- the mixin's map says ``team_text`` where this module's says @@ -483,7 +481,7 @@ def unshare_element_fonts(logger, fonts, element_for_font=None): with identical metrics, so nothing about the rendering changes; only the ability to tell two elements apart does. Faces that cannot be rebuilt (a BDF loaded through freetype.Face, anything without a usable - path) are left shared, and their draws stay white as before. + path) are left shared; resolve_font_color then picks their colour. *element_for_font* names the font keys to consider, in order (the first holder of a face keeps it); it defaults to this module's @@ -491,10 +489,8 @@ def unshare_element_fonts(logger, fonts, element_for_font=None): which names different keys -- see ``resolve_font_color`` for why the two vocabularies are kept apart. """ - try: - from src.common.font_layout import load_truetype as _load - except ImportError: # pragma: no cover - return fonts + # Looked up at call time so tests can spy on the pinned loader. + from src.common.font_layout import load_truetype if element_for_font is None: element_for_font = ELEMENT_FOR_FONT seen = {} @@ -509,7 +505,7 @@ def unshare_element_fonts(logger, fonts, element_for_font=None): if not path or not size: continue try: - fonts[key] = _load(path, size) + fonts[key] = load_truetype(path, size) except (OSError, ValueError, TypeError): logger.debug( "Could not un-share the %s face; it keeps the default colour", key) diff --git a/src/common/sports_game_renderer.py b/src/common/sports_game_renderer.py index c890e45c..60367542 100644 --- a/src/common/sports_game_renderer.py +++ b/src/common/sports_game_renderer.py @@ -56,21 +56,13 @@ class SportsGameRendererMixin: # ---- geometry ------------------------------------------------------ - # Non-finite settings are rejected before any int()/round(): "inf" reaches - # these from config as a float or a string, passes an `isinstance` plus - # `>= 0` check unharmed, and then raises OverflowError out of int() -- - # which the old `except (TypeError, ValueError)` did not catch, so it - # aborted the whole card render. Present in all eight plugins before this - # moved to the core; fixing it here fixes it in all eight. - def _score_reserve_width(self) -> int: - """Centre strip the score actually needs, measured rather than assumed. + """Centre strip the score needs: the width of _SCORE_PROBE in the + score font plus a gutter each side, or 0 if it cannot be measured. - The gap was derived from the card width alone (width x - CENTER_GAP_RATIO, clamped to CENTER_GAP_MAX_PX) while the score's size - comes from config and the element-style resolver. Nothing compared the - two, so any score wider than the clamp was drawn over the logos. - Measuring it keeps the strip wide enough for whatever font is in play. + Measured rather than derived from the card width, because the score's + size comes from config and the element-style resolver: a strip sized + from the width alone lets a large score run over the logos. """ try: probe = ImageDraw.Draw(Image.new("RGB", (4, 4))) @@ -87,6 +79,10 @@ class SportsGameRendererMixin: the card width between the configurable min and max. 0 restores edge-to-edge logos. """ + # Non-finite settings are rejected before any int()/round(): "inf" + # arrives from config as a float or a string, passes the isinstance + # and >= 0 checks, and int() then raises OverflowError, which would + # abort the whole card render. configured = self._scroll_card_option("center_gap") if (isinstance(configured, (int, float)) and math.isfinite(configured) and configured >= 0): @@ -110,10 +106,9 @@ class SportsGameRendererMixin: def _logo_slot_width(self) -> int: """Per-side logo slot, leaving the center gap clear. - No longer capped at display_height: the card is sized as two - full-height logos plus the measured gap, so what is left after the gap - is exactly the logo's share. The cap was what froze the logos at 46px - on the old flat 128px card. + Not capped at display_height: the card is sized as two full-height + logos plus the measured gap, so what is left after the gap is exactly + the logo's share. At least 8 px. """ available = (self.display_width - self._center_gap_width()) // 2 return max(8, available) @@ -130,9 +125,8 @@ class SportsGameRendererMixin: """X/Y nudge for one element, from customization.layout. Same block the full-screen scorebug reads (sports.py - _get_layout_offset), so a nudge configured in the web UI now moves - the element on the scroll/Vegas card too -- previously the schema - advertised these offsets but this renderer ignored them. + _get_layout_offset), so a nudge configured in the web UI moves the + element on the scroll/Vegas card as well as on the scorebug. """ from src.element_style import layout_offset return layout_offset(self.config, element, axis, default, diff --git a/src/common/sports_scroll.py b/src/common/sports_scroll.py index d32fa76a..85e8bd6a 100644 --- a/src/common/sports_scroll.py +++ b/src/common/sports_scroll.py @@ -487,9 +487,9 @@ class SportsScrollDisplayManager: ) except Exception: # prepare_scroll_content is subclass-implemented and builds cards - # straight from feed data, which is exactly where this PR's other - # crashes came from. One sport's bad payload must not take down the - # shared orchestration for the others. + # straight from feed data, so it can raise on a malformed payload. + # One sport's bad payload must not take down the shared + # orchestration for the others. self.logger.exception( "Error preparing scroll content for game_type=%s", game_type ) diff --git a/src/common/sports_shared.py b/src/common/sports_shared.py index e9112ec6..af025a73 100644 --- a/src/common/sports_shared.py +++ b/src/common/sports_shared.py @@ -97,11 +97,11 @@ from datetime import datetime, timedelta, timezone from typing import Any, ClassVar, Dict, List, Optional, Tuple import pytz -from src.common.espn_dates import fetch_espn_scoreboard +from src.common.espn_dates import ESPN_MAX_LIMIT, fetch_espn_scoreboard import requests -from PIL import Image, ImageDraw, ImageFont +from PIL import Image, ImageDraw from src.common import sports_card as _card -from src.common.font_layout import load_truetype +from src.common.font_layout import load_truetype, resolve_asset_path logger = logging.getLogger(__name__) @@ -132,36 +132,16 @@ def _resolve_font_path(path: str) -> str: load raises, the caller falls back, and the scoreboard renders in PIL's default face instead of the pixel font it was laid out for. - Resolution order matches the core's own resolver: the path as given - first, so behaviour is unchanged wherever it already worked and a - configured absolute path is returned untouched, then the core install - root, then the original string so callers still raise and fall back - exactly as they do today. + Resolution order: the path as given, relative to the cwd, when it + exists -- the order the scoreboards' own sports.py copies used, so a + process running from another checkout keeps that checkout's fonts -- + then :func:`src.common.font_layout.resolve_asset_path` (the install + root), which returns the original string when neither exists so callers + still raise and fall back. """ if os.path.exists(path): return path - try: - import src.font_manager as _core_fonts - - # The core grew this resolver in ChuckBuilds/LEDMatrix#425. Use it - # when it is there so both repos stay on one definition of "install - # root"; older cores fall through to the equivalent derivation below. - manager = getattr(_core_fonts, "FontManager", None) - resolver = getattr(manager, "_resolve_asset_path", None) - if resolver is not None: - resolved = resolver(path) - if resolved and os.path.exists(resolved): - return resolved - root = os.path.dirname(os.path.dirname(os.path.abspath(_core_fonts.__file__))) - candidate = os.path.join(root, path) - if os.path.exists(candidate): - return candidate - except (ImportError, AttributeError, OSError): - # No core on the path (standalone tooling), a core laid out - # differently, or an unreadable install. Returning the original keeps - # the caller's existing fallback intact. - return path - return path + return resolve_asset_path(path) class SportsCoreSharedMixin: @@ -201,9 +181,6 @@ class SportsCoreSharedMixin: #: How long to stay quiet between ranking-coverage warnings. _RANKING_COVERAGE_SECONDS: ClassVar[int] = 60 * 60 - def _get_season_schedule_dates(self) -> tuple[str, str]: - return "", "" - def _draw_scorebug_layout(self, game: Dict, force_clear: bool = False) -> None: """Placeholder draw method - subclasses should override.""" # This base method will be simple, subclasses provide specifics @@ -889,7 +866,10 @@ class SportsCoreSharedMixin: draw.text((x, y), text, font=font, fill=fill) def _should_log(self, warning_type: str, cooldown: int = 60) -> bool: - """Check if we should log a warning based on cooldown period.""" + """True at most once per ``cooldown`` seconds, for rate-limiting a + warning. The cooldown is shared by every warning on this manager: + ``warning_type`` is part of the signature scoreboards inherit, but + does not give each type its own cooldown.""" current_time = time.time() if current_time - self._last_warning_time > cooldown: self._last_warning_time = current_time @@ -904,8 +884,6 @@ class SportsCoreSharedMixin: try: # Fetch current week and next few days for immediate display now = datetime.now(pytz.utc) - immediate_events = [] - start_date = now - timedelta(days=self.schedule_lookback_days) end_date = now + timedelta(days=self.schedule_lookahead_days) date_str = f"{start_date.strftime('%Y%m%d')}-{end_date.strftime('%Y%m%d')}" @@ -913,7 +891,7 @@ class SportsCoreSharedMixin: data = fetch_espn_scoreboard( self.session, url, - params={"dates": date_str, "limit": 1000}, + params={"dates": date_str, "limit": ESPN_MAX_LIMIT}, headers=self.headers, timeout=10, logger=self.logger, diff --git a/src/common/sync_manager.py b/src/common/sync_manager.py index 3df6ca5c..bec3d7f0 100644 --- a/src/common/sync_manager.py +++ b/src/common/sync_manager.py @@ -32,7 +32,7 @@ from typing import Callable, Optional import numpy as np from PIL import Image -from src.display_geometry import DEFAULT_CHAIN_LENGTH +from src.display_geometry import DEFAULT_CHAIN_LENGTH, DEFAULT_COLS, DEFAULT_ROWS # Raw-frame wire format: 8-byte magic + 4-byte header + raw RGB pixels # Much faster than PNG: no encode/decode, negligible CPU, same UDP packet size @@ -75,9 +75,12 @@ class FollowerState(Enum): class DisplaySyncManager: """ Core sync manager. Instantiated by DisplayController based on config['sync']. - Leader sends compressed PNG frames to the follower after each render cycle. - Follower renders received frames; returns to own plugin stack when leader - goes offline. + + The leader sends each rendered frame to the follower over UDP as raw RGB + bytes (send_frame), and for Vegas scrolling sends the whole scroll image + once per cycle as a PNG over TCP on port + 1 (send_scroll_image), then + only the scroll position. The follower draws what it receives and goes + back to its own plugins when the leader stops sending. """ def __init__( @@ -192,8 +195,8 @@ class DisplaySyncManager: def _handle_hello(self, msg: dict, sender_ip: str) -> None: hw = self._hw_config - local_rows = hw.get("rows", 32) - local_cols = hw.get("cols", 64) + local_rows = hw.get("rows", DEFAULT_ROWS) + local_cols = hw.get("cols", DEFAULT_COLS) peer_rows = int(msg.get("rows", 0)) peer_cols = int(msg.get("cols", 0)) peer_chain = int(msg.get("chain", DEFAULT_CHAIN_LENGTH)) @@ -469,16 +472,23 @@ class DisplaySyncManager: """Record a decoded leader frame and enter follower mode if needed.""" with self._frame_lock: self._latest_frame = img + self._enter_follower_mode(sender_ip) + + def _enter_follower_mode(self, sender_ip: str) -> bool: + """Note that the leader at ``sender_ip`` just sent something, and + switch from standalone to follower mode if not already following. + Returns True if this call made the switch.""" self._last_leader_frame_time = time.time() self._leader_ip = sender_ip - - if self._follower_state == FollowerState.STANDALONE: - self._follower_state = FollowerState.FOLLOWER - self.logger.info( - "Sync: leader active at %s — switching to follower mode", - sender_ip, - ) - self.write_status_file() + if self._follower_state != FollowerState.STANDALONE: + return False + self._follower_state = FollowerState.FOLLOWER + self.logger.info( + "Sync: leader active at %s — switching to follower mode", + sender_ip, + ) + self.write_status_file() + return True def _follower_recv_loop(self) -> None: while self._running: @@ -559,15 +569,7 @@ class DisplaySyncManager: # back from. Treat it as malformed. raise ValueError(f"non-finite scroll x: {msg['x']!r}") self._latest_scroll_x = scroll_x - self._last_leader_frame_time = time.time() - self._leader_ip = sender_ip - if self._follower_state == FollowerState.STANDALONE: - self._follower_state = FollowerState.FOLLOWER - self.logger.info( - "Sync: leader active at %s — switching to follower mode", - sender_ip, - ) - self.write_status_file() + if self._enter_follower_mode(sender_ip): fire_new_cycle = True # build initial scroll image elif t == "nc": # Leader started a new scroll cycle — rebuild local image @@ -589,8 +591,8 @@ class DisplaySyncManager: hw = self._hw_config hello = json.dumps({ "t": "hello", - "rows": hw.get("rows", 32), - "cols": hw.get("cols", 64), + "rows": hw.get("rows", DEFAULT_ROWS), + "cols": hw.get("cols", DEFAULT_COLS), "chain": hw.get("chain_length", DEFAULT_CHAIN_LENGTH), }).encode("utf-8") heartbeat = json.dumps({"t": "hb"}).encode("utf-8") @@ -660,8 +662,8 @@ class DisplaySyncManager: base = { "role": self.role.value, "port": self.port, - "local_rows": hw.get("rows", 32), - "local_cols": hw.get("cols", 64), + "local_rows": hw.get("rows", DEFAULT_ROWS), + "local_cols": hw.get("cols", DEFAULT_COLS), "local_chain": hw.get("chain_length", DEFAULT_CHAIN_LENGTH), } diff --git a/src/common/text_helper.py b/src/common/text_helper.py index 703476eb..be27772c 100644 --- a/src/common/text_helper.py +++ b/src/common/text_helper.py @@ -10,7 +10,7 @@ from pathlib import Path from typing import Dict, List, Optional, Tuple, Union from PIL import Image, ImageDraw, ImageFont -from src.common.font_layout import load_truetype +from src.common.font_layout import load_truetype, resolve_asset_path # Shared throwaway draw surface for measuring text without a target canvas. _measure_draw = ImageDraw.Draw(Image.new("RGB", (1, 1))) @@ -18,13 +18,14 @@ _measure_draw = ImageDraw.Draw(Image.new("RGB", (1, 1))) class TextHelper: """ - Helper class for text rendering with outlines and font management. - - Provides functionality for: - - Loading and managing fonts - - Drawing text with outlines for better readability - - Calculating text dimensions and positioning - - Managing font resources + Font loading, outlined text and text measurement for plugins. + + - :meth:`load_fonts` loads TrueType fonts from ``font_dir`` (the install's + assets/fonts by default) with the layout engine pinned + (font_layout.load_truetype). Each (file, size) is loaded once per helper + and reused; a missing or unloadable file becomes PIL's default font. + - :meth:`draw_text_with_outline` and friends draw onto a caller's + ``ImageDraw``; the measuring methods need no canvas. """ def __init__(self, font_dir: Optional[Union[str, Path]] = None, @@ -33,22 +34,26 @@ class TextHelper: Initialize the TextHelper. Args: - font_dir: Directory containing font files (defaults to assets/fonts) + font_dir: Directory containing font files. Defaults to the + install's assets/fonts, whatever the process cwd is. logger: Optional logger instance """ self.logger = logger or logging.getLogger(__name__) - self.font_dir = Path(font_dir) if font_dir else Path("assets/fonts") + self.font_dir = Path(font_dir) if font_dir else Path(resolve_asset_path("assets/fonts")) + # ":" -> loaded font; see load_fonts. self._font_cache: Dict[str, ImageFont.ImageFont] = {} def load_fonts(self, font_config: Optional[Dict[str, Dict]] = None) -> Dict[str, ImageFont.ImageFont]: """ Load fonts for different text elements. - + Args: - font_config: Custom font configuration dictionary - + font_config: ``{name: {"file": , "size": }}``; + defaults to the scoreboard set in _get_default_font_config. + Returns: - Dictionary mapping font names to PIL ImageFont objects + Dictionary mapping font names to PIL ImageFont objects. A font + already loaded by this helper at the same size is reused. """ if font_config is None: font_config = self._get_default_font_config() @@ -61,9 +66,13 @@ class TextHelper: size = config['size'] if font_path.exists(): - font = load_truetype(str(font_path), size) + cache_key = f"{font_path}:{size}" + font = self._font_cache.get(cache_key) + if font is None: + font = load_truetype(str(font_path), size) + self._font_cache[cache_key] = font + self.logger.debug(f"Loaded font: {font_name} ({font_path}, size {size})") fonts[font_name] = font - self.logger.debug(f"Loaded font: {font_name} ({font_path}, size {size})") else: # Fallback to default font font = ImageFont.load_default() @@ -115,12 +124,7 @@ class TextHelper: Returns: Width in pixels """ - try: - return int(_measure_draw.textlength(text, font=font)) - except AttributeError: - # Fallback for older PIL versions - bbox = _measure_draw.textbbox((0, 0), text, font=font) - return bbox[2] - bbox[0] + return int(_measure_draw.textlength(text, font=font)) def get_text_height(self, text: str, font: ImageFont.ImageFont) -> int: """ diff --git a/src/config_manager.py b/src/config_manager.py index d9800d4d..292f6026 100644 --- a/src/config_manager.py +++ b/src/config_manager.py @@ -16,8 +16,10 @@ additionally keeps rotating backups in ``config/backups/``. Plugin configuration -------------------- Plugin configs are stored inside ``config.json`` under the plugin's ID key -and survive plugin reinstalls. Use :meth:`ConfigManager.update_plugin_config` -to write plugin settings; never write directly to the plugin directory. +and survive plugin reinstalls. Write them by saving the whole config with +:meth:`ConfigManager.save_config_atomic` (or +:meth:`ConfigManager.save_raw_file_content`); never write settings into the +plugin directory, which a reinstall deletes. Hot-reload ---------- @@ -127,13 +129,10 @@ class ConfigManager: # Update in-memory config if save was successful if result.status == SaveResultStatus.SUCCESS: self.config = new_config_data - # In-memory config now matches what was just written; refresh - # the load signature so the fast path stays valid. NOTE: the - # in-memory copy includes merged secrets; the on-disk file has - # them stripped — the fast path returning self.config preserves - # exactly the pre-cache behavior (load-after-save also returned - # the secret-merged self.config only after re-reading secrets; - # here secrets file is unchanged, so contents are equivalent). + # In-memory config now matches what was just written, so the + # load_config fast path may return it. It still carries the + # merged secrets that were stripped on disk; that matches a full + # reload, because the secrets file was not changed by the save. self._loaded_sig = self._files_signature() self.logger.info(f"Configuration successfully saved atomically to {os.path.abspath(self.config_path)}") elif result.status == SaveResultStatus.ROLLED_BACK: @@ -253,11 +252,11 @@ class ConfigManager: return self.config except FileNotFoundError as e: - if str(e).find('config_secrets.json') == -1: # Only raise if main config is missing - error_msg = f"Configuration file not found at {os.path.abspath(self.config_path)}" - self.logger.error(error_msg, exc_info=True) - raise ConfigError(error_msg, config_path=self.config_path) from e - return self.config + # Only config.json can get here: a missing or unreadable secrets + # file is handled where it is read. + error_msg = f"Configuration file not found at {os.path.abspath(self.config_path)}" + self.logger.error(error_msg, exc_info=True) + raise ConfigError(error_msg, config_path=self.config_path) from e except json.JSONDecodeError as e: error_msg = f"Error parsing configuration file {os.path.abspath(self.config_path)}" self.logger.error(error_msg, exc_info=True) @@ -320,10 +319,9 @@ class ConfigManager: A missing secrets file is fine (nothing to strip). But a file that EXISTS and cannot be read or parsed means stripping is impossible — and the in-memory config being saved has secrets deep-merged into it, - so proceeding would write them into config.json in plaintext. That - was the historical behavior; it is now a hard refusal. The save - raises so the caller (and user) fixes the secrets file instead of - silently leaking its contents into the world-readable main config. + so proceeding would write them into config.json in plaintext. The + save raises instead, so the caller (and user) fixes the secrets file + rather than leaking its contents into the world-readable main config. """ if not os.path.exists(self.secrets_path): return {} @@ -465,9 +463,8 @@ class ConfigManager: # Merge template defaults into current config self._merge_template_defaults(self.config, template_config) - # Save migrated config using atomic save to preserve permissions - # Use atomic save to preserve file permissions - # Note: save_config_atomic handles secrets internally + # save_config_atomic strips the merged secrets back out and + # keeps the file's owner and mode. result = self.save_config_atomic( new_config_data=self.config, create_backup=False, # Already created backup above @@ -600,20 +597,16 @@ class ConfigManager: self.logger.info(f"{file_type.capitalize()} configuration successfully saved to {os.path.abspath(path_to_save)}") - # If we just saved the main config or secrets, the merged self.config might be stale. - # Reload it to reflect the new state. - # Note: We wrap this in try-except because reload failures (e.g., migration errors) - # should not cause the save operation to fail - the file was saved successfully. - if file_type == "main" or file_type == "secrets": - try: - self.load_config() - except Exception as reload_error: - # Log the reload error but don't fail the save operation - # The file was saved successfully, reload is just for in-memory consistency - self.logger.warning( - f"Configuration file saved successfully, but reload failed: {reload_error}. " - f"The file on disk is valid, but in-memory config may be stale." - ) + # The merged self.config is now stale; reload it. A reload failure + # (a migration error, say) is logged, not raised: the file itself + # was saved. + try: + self.load_config() + except Exception as reload_error: + self.logger.warning( + f"Configuration file saved successfully, but reload failed: {reload_error}. " + f"The file on disk is valid, but in-memory config may be stale." + ) except PermissionError as e: # Provide helpful error message with fix instructions @@ -670,7 +663,7 @@ class ConfigManager: try: # Load current configs main_config = self.get_raw_file_content('main') - secrets_config = self.get_raw_file_content('secrets') if os.path.exists(self.secrets_path) else {} + secrets_config = self.get_raw_file_content('secrets') # {} when there is no file # Remove plugin from main config if plugin_id in main_config: @@ -703,7 +696,7 @@ class ConfigManager: try: # Load current configs main_config = self.get_raw_file_content('main') - secrets_config = self.get_raw_file_content('secrets') if os.path.exists(self.secrets_path) else {} + secrets_config = self.get_raw_file_content('secrets') # {} when there is no file valid_set = set(valid_plugin_ids) diff --git a/src/dynamic_team_resolver.py b/src/dynamic_team_resolver.py index 7cbe09cd..ef666a20 100644 --- a/src/dynamic_team_resolver.py +++ b/src/dynamic_team_resolver.py @@ -21,6 +21,8 @@ import time import requests from typing import Dict, List +from src.common.api_helper import DEFAULT_HTTP_HEADERS + logger = logging.getLogger(__name__) class DynamicTeamResolver: @@ -141,7 +143,9 @@ class DynamicTeamResolver: self.logger.info("Fetching fresh NCAA Football rankings from ESPN API") rankings_url = "https://site.api.espn.com/apis/site/v2/sports/football/college-football/rankings" - response = requests.get(rankings_url, timeout=self.request_timeout) + # ESPN rejects requests' default User-Agent; see api_helper.USER_AGENT. + response = requests.get(rankings_url, headers=dict(DEFAULT_HTTP_HEADERS), + timeout=self.request_timeout) response.raise_for_status() data = response.json() diff --git a/src/element_style.py b/src/element_style.py index 50c45660..15e1af4b 100644 --- a/src/element_style.py +++ b/src/element_style.py @@ -90,6 +90,17 @@ def _cache_put(key: Tuple[str, int], value: Tuple[Any, int]) -> None: # Config keys a style element block carries, in schema/UI order. _STYLE_KEYS = ('font', 'font_size', 'text_color', 'visible', 'align') +# Title of every generated `layout` (x/y offset) group in the config form. +_LAYOUT_TITLE = 'Layout Offsets' + +#: Bounds on a user-set ``customization.layout..scale``. They are the +#: Scale field's minimum and maximum in the generated schema, and every reader +#: (coerce_scale, element_scale, LogoHelper.load_logo) clamps to them, so the +#: web form and the renderer agree. Below 0.1 a logo is a dot; ten times a +#: panel-sized box is already far off the panel. +MIN_ELEMENT_SCALE = 0.1 +MAX_ELEMENT_SCALE = 10.0 + @dataclass(frozen=True) class ElementStyle: @@ -301,7 +312,7 @@ def expand_style_elements(schema: Dict[str, Any]) -> Dict[str, Any]: if layout_props: layout = props.setdefault('layout', { 'type': 'object', - 'title': 'Layout Offsets', + 'title': _LAYOUT_TITLE, 'description': 'Pixel offsets applied to each element ' '(positive x moves right, positive y moves down)', 'x-advanced': True, @@ -345,16 +356,14 @@ def _element_block_from_spec(element_key: str, 'type': 'string', 'title': 'Font Family', 'x-advanced': True, - # The core already ships this widget and the config form already - # allowlists it; without the hint the field rendered as a bare - # text box the user had to type a filename into. + # The core's font picker; without the hint the form renders a + # bare text box the user has to type a filename into. 'x-widget': 'font-selector', } # A bitmap font ignores font_size and renders at its own baked-in # size, so the size ceiling has to be enforced when picking the # font, not when setting the size. - max_size = (size_spec or {}).get('max') if isinstance( - spec.get('size'), dict) else None + max_size = size_spec.get('max') if size_spec else None if isinstance(max_size, (int, float)): font_prop['x-options'] = {'maxFixedSize': max_size} if 'default' in font_spec: @@ -457,8 +466,8 @@ def _offset_block_from_spec(element_key: str, 'title': 'Scale', 'description': 'Size multiplier; 1 is the shipped size.', 'default': 1.0, - 'minimum': 0.1, - 'maximum': 10.0, + 'minimum': MIN_ELEMENT_SCALE, + 'maximum': MAX_ELEMENT_SCALE, 'x-advanced': True, } if isinstance(scale_spec, dict): @@ -539,7 +548,7 @@ def _modes_block(declaration: Dict[str, Any], if layout_props: element_props['layout'] = { 'type': 'object', - 'title': 'Layout Offsets', + 'title': _LAYOUT_TITLE, 'x-advanced': True, 'additionalProperties': False, 'properties': layout_props, @@ -680,7 +689,7 @@ def _modes_block_from_properties(props: Dict[str, Any], element_keys: list, if layout_props: element_props['layout'] = { 'type': 'object', - 'title': 'Layout Offsets', + 'title': _LAYOUT_TITLE, 'x-advanced': True, 'additionalProperties': False, 'properties': layout_props, @@ -1010,12 +1019,15 @@ def _coerce_align(value: Any) -> Optional[str]: return None -def _coerce_scale(value: Any, default: float) -> float: - """A positive size multiplier, or ``default``. +def coerce_scale(value: Any, default: float = 1.0) -> float: + """A usable size multiplier: ``value`` clamped to + [MIN_ELEMENT_SCALE, MAX_ELEMENT_SCALE], or ``default``. - Clamped rather than merely validated: a scale of 0 or a negative one is - a zero-or-inverted image, and the panel is 32 pixels tall -- a typo - should cost a wrong size, not a crash inside PIL. + ``default`` is returned for anything that is not a finite positive number + (None, a bool, a string, 0, a negative, NaN, infinity): those are typos, + and a typo should cost the shipped size, not a blank or inverted image or + a crash inside PIL. A positive number outside the range is a real request + for "smaller" or "bigger", so it is clamped rather than ignored. """ if isinstance(value, bool) or value is None: return default @@ -1023,9 +1035,9 @@ def _coerce_scale(value: Any, default: float) -> float: scale = float(value) except (TypeError, ValueError): return default - if scale <= 0: + if not math.isfinite(scale) or scale <= 0: return default - return min(scale, 10.0) + return min(max(scale, MIN_ELEMENT_SCALE), MAX_ELEMENT_SCALE) def _coerce_offset(value: Any, default: int, element_key: str, @@ -1194,7 +1206,7 @@ def element_scale(config: Any, element_key: str, default: float = 1.0, try: value = _element_field(config, element_key, 'scale', mode, in_layout=True) - return default if value is None else _coerce_scale(value, default) + return default if value is None else coerce_scale(value, default) except Exception as e: logger.warning("Error reading scale for %s: %s", element_key, e) return default @@ -1371,12 +1383,6 @@ class ElementStyleResolver: return {} return _lookup_element(block.get('layout'), element_key) - @classmethod - def _layout_axis(cls, block: Dict[str, Any], element_key: str, - axis: str) -> Any: - """``block['layout'][element][axis]``, or None if absent anywhere.""" - return cls._layout_element(block, element_key).get(axis) - # -- resolution internals ----------------------------------------------- @@ -1497,7 +1503,7 @@ class ElementStyleResolver: user_forced_color=bool(color_forced), visible=_coerce_bool(visible, True), align=_coerce_align(align), - scale=_coerce_scale(scale, 1.0), + scale=coerce_scale(scale, 1.0), ) def _classic_style(self, classic_font: str, classic_size: int, diff --git a/src/error_aggregator.py b/src/error_aggregator.py index 55fc7b1e..49b656a2 100644 --- a/src/error_aggregator.py +++ b/src/error_aggregator.py @@ -26,6 +26,19 @@ from src.exceptions import LEDMatrixError from src.redaction import redact_credentials +def _format_trace(error: BaseException) -> str: + """The traceback carried by ``error`` itself. + + Callers often record an exception after its ``except`` block has ended, + or from another thread than the one that raised it (plugin_executor runs + plugins on worker threads), where ``traceback.format_exc()`` has nothing + to report. The exception object keeps its own ``__traceback__``, so the + trace is built from that. An exception that was created but never raised + has no traceback, and the result is just its type and message. + """ + return "".join(traceback.format_exception(type(error), error, error.__traceback__)) + + @dataclass class ErrorRecord: """Record of a single error occurrence.""" @@ -145,8 +158,8 @@ class ErrorAggregator: with self._lock: error_type = type(error).__name__ - # Extract additional context from LEDMatrixError subclasses - error_context = context or {} + # A copy, so the caller's dict is not changed behind its back. + error_context = dict(context) if context else {} if isinstance(error, LEDMatrixError) and error.context: error_context.update(error.context) @@ -157,7 +170,7 @@ class ErrorAggregator: context=error_context, plugin_id=plugin_id, operation=operation, - stack_trace=traceback.format_exc() + stack_trace=_format_trace(error) ) # Add record (with size limit) diff --git a/src/font_manager.py b/src/font_manager.py index f013aecc..38f2f173 100644 --- a/src/font_manager.py +++ b/src/font_manager.py @@ -40,6 +40,11 @@ from pathlib import Path from PIL import ImageFont from src.common.bdf_font import load_bdf_face, read_bdf_native_size from src.common.font_layout import load_truetype, resolve_asset_path +from src.common.permission_utils import ( + ensure_directory_permissions, + get_assets_dir_mode, + get_config_dir_mode, +) from typing import Dict, Tuple, Optional, Union, Any, List from src.deprecation import deprecated @@ -57,7 +62,6 @@ class FontManager: def __init__(self, config: Dict[str, Any]): self.config = config - self.fonts_config = config.get("fonts", {}) # Font discovery and catalog self.font_catalog: Dict[str, str] = {} # family_name -> file_path @@ -73,10 +77,8 @@ class FontManager: # Plugin font management self.plugin_fonts: Dict[str, Dict[str, Any]] = {} # plugin_id -> font_manifest self.plugin_font_catalogs: Dict[str, Dict[str, str]] = {} # plugin_id -> {family_name -> file_path} - self.font_metadata: Dict[str, Dict[str, Any]] = {} # family_name -> metadata - self.font_dependencies: Dict[str, List[str]] = {} # family_name -> [required_families] - # Manager font registration - NEW for manager-centric model + # Fonts managers and plugins report using (register_manager_font). self.manager_fonts: Dict[str, Dict[str, Any]] = {} # manager_id -> {element_key: {family, size_px, color}} self.detected_fonts: Dict[str, Dict[str, Any]] = {} # element_key -> {family, size_px, color, manager_id, usage_count} # Bumped when a manager's registered families change (not when one @@ -88,13 +90,10 @@ class FontManager: self.temp_font_dir = Path(tempfile.gettempdir()) / "ledmatrix_fonts" self.temp_font_dir.mkdir(exist_ok=True) - # Performance monitoring + # Counters behind get_performance_stats(). self.performance_stats = { - "font_load_times": {}, "cache_hits": 0, "cache_misses": 0, - "render_times": {}, - "total_renders": 0, "failed_loads": 0, "start_time": time.time() } @@ -105,9 +104,6 @@ class FontManager: "four_by_six": "assets/fonts/4x6-font.ttf", "five_by_seven": "assets/fonts/5x7.bdf", "tom_thumb": "assets/fonts/tom-thumb.bdf" - # Note: cozette_bdf removed - font file not available - # To re-enable: download cozette.bdf from https://github.com/the-moonwitch/Cozette - # and add: "cozette_bdf": "assets/fonts/cozette.bdf" } # Size tokens for convenience @@ -116,7 +112,10 @@ class FontManager: } # Font overrides storage (for manual overrides) - self.font_overrides_file = "config/font_overrides.json" + # Under the install root's config/ (which always exists), not the + # cwd: the file itself may not exist yet, and resolve_asset_path + # hands back a missing path unchanged. + self.font_overrides_file = os.path.join(resolve_asset_path("config"), "font_overrides.json") self.font_overrides: Dict[str, Dict[str, Any]] = {} # Bumped whenever cached font objects are invalidated, so holders of @@ -128,7 +127,6 @@ class FontManager: def reload_config(self, new_config: Dict[str, Any]): """Reload configuration and refresh font catalog.""" self.config = new_config - self.fonts_config = new_config.get("fonts", {}) self.font_cache.clear() # Clear cache to force reload self.metrics_cache.clear() # Clear metrics cache self.cache_generation += 1 @@ -136,7 +134,6 @@ class FontManager: logger.info("FontManager configuration reloaded successfully") # ==================== Manager Font Registration ==================== - # NEW: Support for managers to register their font choices dynamically def register_manager_font(self, manager_id: str, element_key: str, family: str, size_px: int, color: Optional[Tuple[int, int, int]] = None): @@ -209,16 +206,23 @@ class FontManager: # ==================== Plugin Font Management ==================== - def register_plugin_fonts(self, plugin_id: str, font_manifest: Dict[str, Any]) -> bool: + def register_plugin_fonts(self, plugin_id: str, font_manifest: Dict[str, Any], + plugin_dir: Optional[Union[str, Path]] = None) -> bool: """ Register fonts for a specific plugin. Args: plugin_id: Unique identifier for the plugin - font_manifest: Font manifest from plugin's manifest.json + font_manifest: The ``fonts`` block of the plugin's manifest.json + plugin_dir: The plugin's directory, which ``plugin://`` sources + are relative to. PluginManager passes the directory it loaded + the plugin from. When omitted, the plugin is looked up in the + configured ``plugin_system.plugins_directory`` and then in + ``plugins/``. Returns: - True if registration successful, False otherwise + True if the manifest was valid (individual fonts that fail to load + are logged and skipped), False otherwise """ try: # Validate font manifest structure @@ -235,7 +239,7 @@ class FontManager: # Process font definitions fonts = font_manifest.get("fonts", []) for font_def in fonts: - if self._register_plugin_font(plugin_id, font_def): + if self._register_plugin_font(plugin_id, font_def, plugin_dir): logger.info(f"Successfully registered font {font_def.get('family')} for plugin {plugin_id}") logger.info(f"Registered {len(fonts)} fonts for plugin {plugin_id}") @@ -270,7 +274,8 @@ class FontManager: return True - def _register_plugin_font(self, plugin_id: str, font_def: Dict[str, Any]) -> bool: + def _register_plugin_font(self, plugin_id: str, font_def: Dict[str, Any], + plugin_dir: Optional[Union[str, Path]] = None) -> bool: """Register a single font from a plugin.""" try: family = font_def["family"] @@ -284,7 +289,7 @@ class FontManager: elif source.startswith("plugin://"): # Relative to plugin directory relative_path = source.replace("plugin://", "") - font_path = self._resolve_plugin_font_path(plugin_id, relative_path) + font_path = self._resolve_plugin_font_path(plugin_id, relative_path, plugin_dir) else: # Absolute or relative path font_path = source @@ -298,14 +303,6 @@ class FontManager: self.plugin_font_catalogs[plugin_id][family] = font_path self.font_catalog[namespaced_family] = font_path - # Store metadata - if "metadata" in font_def: - self.font_metadata[namespaced_family] = font_def["metadata"] - - # Store dependencies - if "dependencies" in font_def: - self.font_dependencies[namespaced_family] = font_def["dependencies"] - logger.info(f"Registered plugin font: {namespaced_family} -> {font_path}") return True @@ -367,11 +364,15 @@ class FontManager: return '.zip' return '.ttf' # default - def _resolve_plugin_font_path(self, plugin_id: str, relative_path: str) -> Optional[str]: - """Resolve a plugin-relative font path.""" - # Assume plugins are in a 'plugins' directory - plugin_dir = Path("plugins") / plugin_id - font_path = plugin_dir / relative_path + def _resolve_plugin_font_path(self, plugin_id: str, relative_path: str, + plugin_dir: Optional[Union[str, Path]] = None) -> Optional[str]: + """Resolve a ``plugin://`` font path against the plugin's directory.""" + if plugin_dir is None: + plugin_dir = self._find_plugin_dir(plugin_id) + if plugin_dir is None: + logger.error(f"Plugin font {relative_path}: directory for plugin {plugin_id} not found") + return None + font_path = Path(plugin_dir) / relative_path if font_path.exists(): return str(font_path) @@ -379,6 +380,18 @@ class FontManager: logger.error(f"Plugin font not found: {font_path}") return None + def _find_plugin_dir(self, plugin_id: str) -> Optional[Path]: + """The installed directory of ``plugin_id``, for callers of + register_plugin_fonts that do not pass one: the configured plugins + directory (relative paths are relative to the install root), then the + legacy ``plugins/`` directory.""" + # Imported here: src.plugin_system's package import loads PluginManager. + from src.plugin_system.plugin_dirs import resolve_plugin_dir + + configured = (self.config.get("plugin_system") or {}).get("plugins_directory") or "plugin-repos" + search_dirs = [Path(resolve_asset_path(configured)), Path(resolve_asset_path("plugins"))] + return resolve_plugin_dir(plugin_id, search_dirs, prefix=True) + @deprecated("3.7.0") def unregister_plugin_fonts(self, plugin_id: str) -> bool: """Unregister all fonts for a plugin.""" @@ -390,8 +403,6 @@ class FontManager: namespaced_family = f"{plugin_id}::{family}" if namespaced_family in self.font_catalog: del self.font_catalog[namespaced_family] - if namespaced_family in self.font_metadata: - del self.font_metadata[namespaced_family] del self.plugin_font_catalogs[plugin_id] @@ -442,8 +453,6 @@ class FontManager: Returns: Resolved font object """ - start_time = time.time() - try: # Check for manual overrides first if element_key in self.font_overrides: @@ -460,14 +469,7 @@ class FontManager: if plugin_id in self.plugin_font_catalogs and family in self.plugin_font_catalogs[plugin_id]: family = f"{plugin_id}::{family}" - # Get the font - font = self.get_font(family, size_px) - - # Record performance - duration = time.time() - start_time - self._record_performance_metric("resolve", f"{family}_{size_px}", duration) - - return font + return self.get_font(family, size_px) except Exception as e: logger.error(f"Error resolving font for {element_key}: {e}", exc_info=True) @@ -491,7 +493,6 @@ class FontManager: return self.font_cache[cache_key] self.performance_stats["cache_misses"] += 1 - start_time = time.time() # Load font font_path = self.font_catalog.get(family) @@ -506,15 +507,13 @@ class FontManager: else: font = load_truetype(font_path, size_px) except Exception as e: + # The one log line for a failed load: _load_bdf_font lets + # its error propagate to here. logger.error(f"Error loading font {font_path}: {e}") self.performance_stats["failed_loads"] += 1 font = ImageFont.load_default() - # Cache and record performance self.font_cache[cache_key] = font - duration = time.time() - start_time - self.performance_stats["font_load_times"][cache_key] = duration - return font def _load_bdf_font(self, font_path: str, size_px: int) -> freetype.Face: @@ -524,11 +523,7 @@ class FontManager: rather than failing over to PIL's default font, a different typeface (see :func:`src.common.bdf_font.load_bdf_face`). """ - try: - return load_bdf_face(font_path, size_px)[0] - except Exception as e: - logger.error(f"Error loading BDF font {font_path}: {e}") - raise + return load_bdf_face(font_path, size_px)[0] def get_native_bdf_size(self, family: str) -> Optional[int]: """The one true pixel size of a BDF family in the catalog, or None @@ -723,11 +718,6 @@ class FontManager: def _save_overrides(self): """Save current font overrides to file.""" try: - from pathlib import Path - from src.common.permission_utils import ( - ensure_directory_permissions, - get_config_dir_mode - ) font_overrides_path = Path(self.font_overrides_file) ensure_directory_permissions(font_overrides_path.parent, get_config_dir_mode()) with open(self.font_overrides_file, 'w') as f: @@ -754,12 +744,6 @@ class FontManager: """Get available size tokens.""" return self.size_tokens.copy() - def _record_performance_metric(self, operation: str, font_key: str, duration: float): - """Record a performance metric.""" - if operation not in self.performance_stats: - self.performance_stats[operation] = {} - self.performance_stats[operation][font_key] = duration - @deprecated("3.7.0") def get_performance_stats(self) -> Dict[str, Any]: """Get performance statistics.""" @@ -789,7 +773,8 @@ class FontManager: @deprecated("3.7.0") def add_font(self, font_file_path: str, family_name: str) -> bool: - """Add a new font to the catalog.""" + """Add ``font_file_path`` to the catalog as ``family_name``. The file + stays where it is; only assets/fonts is created if it is missing.""" try: # Validate font file if not os.path.exists(font_file_path): @@ -801,13 +786,7 @@ class FontManager: logger.warning(f"Font family '{family_name}' already exists") return False - # Copy font to assets/fonts directory - from pathlib import Path - from src.common.permission_utils import ( - ensure_directory_permissions, - get_assets_dir_mode - ) - fonts_dir = Path("assets/fonts") + fonts_dir = Path(resolve_asset_path("assets/fonts")) ensure_directory_permissions(fonts_dir, get_assets_dir_mode()) # Add to catalog diff --git a/src/logo_downloader.py b/src/logo_downloader.py index c74096df..c042188a 100644 --- a/src/logo_downloader.py +++ b/src/logo_downloader.py @@ -16,7 +16,7 @@ import json from typing import Dict, List, Optional, Tuple from pathlib import Path from PIL import Image, ImageDraw, ImageFont, UnidentifiedImageError -from src.common.font_layout import load_truetype +from src.common.font_layout import load_truetype, resolve_asset_path from PIL.PngImagePlugin import PngInfo from requests.adapters import HTTPAdapter from urllib3.util.retry import Retry @@ -370,22 +370,15 @@ class LogoDownloader: @staticmethod def get_logo_filename_variations(abbreviation: str) -> list: - """Get possible filename variations for a team abbreviation.""" - variations = [] + """Filenames a logo for ``abbreviation`` may be stored under: the + upper-cased abbreviation as given, then its normalize_abbreviation() + form (``TA&M.png``, then ``TAANDM.png``).""" original = abbreviation.upper() normalized = LogoDownloader.normalize_abbreviation(abbreviation) - - # Add original and normalized versions - variations.extend([f"{original}.png", f"{normalized}.png"]) - - # Special handling for known cases - if original == 'TA&M': - # TA&M has a file named TA&M.png, but normalize creates TAANDM.png - variations = [f"{original}.png", f"{normalized}.png"] - - return variations + return [f"{original}.png", f"{normalized}.png"] - # Allowlist for league names used in filesystem paths: alphanumerics, underscores, dashes only + # Allowlist for a league name or code that goes into a filesystem path or + # an ESPN URL: lower-case alphanumerics, underscores and dashes only. _SAFE_LEAGUE_RE = re.compile(r'^[a-z0-9_-]+$') def get_logo_directory(self, league: str) -> str: @@ -462,15 +455,12 @@ class LogoDownloader: logger.error(f"Unexpected error downloading logo for {team_abbreviation}: {e}") return False - # Allowlist for the league_code segment interpolated into ESPN API URLs - _SAFE_LEAGUE_CODE_RE = re.compile(r'^[a-z0-9_-]+$') - def _resolve_api_url(self, league: str) -> Optional[str]: """Resolve the ESPN API teams URL for a league, with dynamic fallback for custom soccer leagues.""" api_url = self.API_ENDPOINTS.get(league) if not api_url and league.startswith('soccer_'): league_code = league[len('soccer_'):] - if not self._SAFE_LEAGUE_CODE_RE.match(league_code): + if not self._SAFE_LEAGUE_RE.match(league_code): logger.warning(f"Rejecting unsafe league_code for ESPN URL construction: {league_code!r}") return None api_url = f'https://site.api.espn.com/apis/site/v2/sports/soccer/{league_code}/teams' @@ -501,7 +491,8 @@ class LogoDownloader: return None def fetch_single_team(self, league: str, team_id: str) -> Optional[Dict]: - """Fetch team data from ESPN API for a specific league.""" + """Fetch one team's record (``/``) from the + ESPN API; None on any request or parse failure.""" api_url = self._resolve_api_url(league) if not api_url: logger.error(f"No API endpoint configured for league: {league}") @@ -520,7 +511,7 @@ class LogoDownloader: logger.error(f"Error fetching team data for {team_id} in {league}: {e}") return None except json.JSONDecodeError as e: - logger.error(f"Error parsing JSON response for{team_id} in {league}: {e}") + logger.error(f"Error parsing JSON response for {team_id} in {league}: {e}") return None def extract_teams_from_data(self, data: Dict, league: str) -> List[Dict[str, str]]: @@ -626,42 +617,6 @@ class LogoDownloader: # Default to FBS for unknown conferences return 'FBS' - def _get_team_name_variations(self, abbreviation: str) -> List[str]: - """Generate common variations of a team abbreviation for matching.""" - variations = set() - abbr = abbreviation.upper() - variations.add(abbr) - - # Add normalized version - variations.add(self.normalize_abbreviation(abbr)) - - # Common substitutions - substitutions = { - '&': ['AND', 'A'], - 'A&M': ['TAMU', 'TA&M', 'TEXASAM'], - 'STATE': ['ST', 'ST.'], - 'UNIVERSITY': ['U', 'UNIV'], - 'COLLEGE': ['C', 'COL'], - 'TECHNICAL': ['TECH', 'T'], - 'NORTHERN': ['NORTH', 'N'], - 'SOUTHERN': ['SOUTH', 'S'], - 'EASTERN': ['EAST', 'E'], - 'WESTERN': ['WEST', 'W'] - } - - # Apply substitutions - for original, replacements in substitutions.items(): - if original in abbr: - for replacement in replacements: - variations.add(abbr.replace(original, replacement)) - variations.add(abbr.replace(original, '')) # Remove the word entirely - - # Add common abbreviations for Texas A&M - if 'A&M' in abbr or 'TAMU' in abbr: - variations.update(['TAMU', 'TA&M', 'TEXASAM', 'TEXAS_A&M', 'TEXAS_AM']) - - return list(variations) - def download_missing_logos_for_league(self, league: str, force_download: bool = False) -> Tuple[int, int]: """Download missing logos for a specific league.""" logger.info(f"Starting logo download for league: {league}") @@ -794,7 +749,9 @@ class LogoDownloader: return False try: logo_url = data["team"]["logos"][0]["href"] - except KeyError: + except (KeyError, IndexError, TypeError): + # A team without logos comes back with an empty list. + logger.debug(f"No logo URL for team {team_id} in {league}") return False # Download the logo success = self.download_logo(logo_url, logo_path, team_abbreviation) @@ -824,24 +781,39 @@ class LogoDownloader: logger.info(f"Overall logo download results: {total_downloaded} downloaded, {total_failed} failed") return results - def create_placeholder_logo(self, team_abbreviation: str, logo_dir: str) -> bool: - """Create a placeholder logo when real logo cannot be downloaded.""" + def create_placeholder_logo(self, team_abbreviation: str, logo_dir: str, + filepath: Optional[Path] = None) -> bool: + """Write a grey placeholder with the team abbreviation on it, for when + the real logo cannot be downloaded. + + Args: + team_abbreviation: Drawn on the placeholder. + logo_dir: Directory for the file when ``filepath`` is not given; + the file is then ``.png``. + filepath: The exact path to write, which is where the caller will + look for the logo. ``logo_dir`` is ignored when it is given. + + Returns: + True if the placeholder was written, False otherwise (logged). + """ + if filepath is not None: + filepath = Path(filepath) + logo_dir = str(filepath.parent) try: - # Ensure the logo directory exists if not self.ensure_logo_directory(logo_dir): logger.error(f"Failed to create logo directory: {logo_dir}") return False - - filename = f"{self.normalize_abbreviation(team_abbreviation)}.png" - filepath = Path(logo_dir) / filename - # Create a simple placeholder logo - logo = Image.new('RGBA', (64, 64), (100, 100, 100, 255)) # Gray background + if filepath is None: + filename = f"{self.normalize_abbreviation(team_abbreviation)}.png" + filepath = Path(logo_dir) / filename + + logo = Image.new('RGBA', PLACEHOLDER_SIZE, PLACEHOLDER_BG) draw = ImageDraw.Draw(logo) # Try to load a font, fallback to default try: - font = load_truetype("assets/fonts/PressStart2P-Regular.ttf", 12) + font = load_truetype(resolve_asset_path("assets/fonts/PressStart2P-Regular.ttf"), 12) except (OSError, IOError): try: font = ImageFont.load_default() @@ -855,8 +827,8 @@ class LogoDownloader: bbox = draw.textbbox((0, 0), text, font=font) text_width = bbox[2] - bbox[0] text_height = bbox[3] - bbox[1] - x = (64 - text_width) // 2 - y = (64 - text_height) // 2 + x = (PLACEHOLDER_SIZE[0] - text_width) // 2 + y = (PLACEHOLDER_SIZE[1] - text_height) // 2 draw.text((x, y), text, font=font, fill=(255, 255, 255, 255)) else: # Fallback without font @@ -963,14 +935,19 @@ def download_missing_logo(league: str, team_id: str, team_abbreviation: str, log Convenience function to download a missing team logo. Args: - team_abbreviation: Team abbreviation (e.g., 'UGA', 'BAMA', 'TA&M') league: League identifier (e.g., 'ncaa_fb', 'nfl') - logo_path: Full path to where the logo should be saved + team_id: ESPN team id, used to look up the logo URL when + ``logo_url`` is not given + team_abbreviation: Team abbreviation (e.g., 'UGA', 'BAMA', 'TA&M') + logo_path: Where the logo (or placeholder) is written; relative paths + are relative to the install root logo_url: Optional direct URL to the logo create_placeholder: Whether to create a placeholder if download fails Returns: - True if logo exists or was successfully downloaded, False otherwise + True if a logo or placeholder is at ``logo_path`` afterwards (it was + already there, was downloaded, or a placeholder was written), + False otherwise. """ downloader = shared_downloader() @@ -1011,7 +988,7 @@ def download_missing_logo(league: str, team_id: str, team_abbreviation: str, log time.sleep(0.1) # Small delay if not success and create_placeholder: logger.info(f"Creating placeholder logo for {team_abbreviation}") - success = downloader.create_placeholder_logo(team_abbreviation, logo_dir) + success = downloader.create_placeholder_logo(team_abbreviation, logo_dir, filepath=filepath) return success success = downloader.download_missing_logo_for_team(league, team_id, team_abbreviation, logo_path) @@ -1019,7 +996,7 @@ def download_missing_logo(league: str, team_id: str, team_abbreviation: str, log if not success and create_placeholder: logger.info(f"Creating placeholder logo for {team_abbreviation}") # Create placeholder as fallback - success = downloader.create_placeholder_logo(team_abbreviation, logo_dir) + success = downloader.create_placeholder_logo(team_abbreviation, logo_dir, filepath=filepath) if success: logger.info(f"Successfully handled logo for {team_abbreviation}") diff --git a/src/plugin_system/plugin_executor.py b/src/plugin_system/plugin_executor.py index 6446fca0..a2014145 100644 --- a/src/plugin_system/plugin_executor.py +++ b/src/plugin_system/plugin_executor.py @@ -93,7 +93,7 @@ class PluginExecutor: if result_container['exception']: error = result_container['exception'] error_msg = f"{plugin_context} operation failed: {error}" - self.logger.error(error_msg, exc_info=True) + self.logger.error(error_msg, exc_info=error) record_error(error, plugin_id=plugin_id, operation="execute") raise PluginError(error_msg, plugin_id=plugin_id) from error diff --git a/src/plugin_system/plugin_manager.py b/src/plugin_system/plugin_manager.py index 97aef580..90d7bc67 100644 --- a/src/plugin_system/plugin_manager.py +++ b/src/plugin_system/plugin_manager.py @@ -394,7 +394,8 @@ class PluginManager: self.font_manager, 'register_plugin_fonts' ): try: - self.font_manager.register_plugin_fonts(plugin_id, font_manifest) + self.font_manager.register_plugin_fonts( + plugin_id, font_manifest, plugin_dir=plugin_dir) except Exception as e: self.logger.warning( "Failed to register fonts for plugin %s: %s", plugin_id, e diff --git a/src/startup_validator.py b/src/startup_validator.py index 954c751f..14c4c227 100644 --- a/src/startup_validator.py +++ b/src/startup_validator.py @@ -1,8 +1,14 @@ """ Startup Validator -Validates system configuration, plugins, and dependencies on startup. -Fails fast with clear error messages to prevent runtime issues. +Checks configuration, the cache directory, plugins and the installed systemd +units when the display service starts, and reports what it finds. + +validate_all() never raises: it returns (is_valid, errors, warnings) and +DisplayController logs them. Startup continues either way, so a problem found +here shows up in the log rather than stopping the display. raise_on_errors() +turns the errors into exceptions for a caller that does want to stop; the +display service does not call it. """ import os @@ -180,17 +186,16 @@ class StartupValidator: try: config = self.config_manager.load_config() - # Check for required top-level keys required_keys = ['display', 'timezone'] for key in required_keys: if key not in config: self.errors.append(f"Missing required configuration key: {key}") - - # Validate display configuration - display_config = config.get('display', {}) - if not display_config: - self.errors.append("Display configuration is missing or empty") - + + # A missing display section is reported once, above, and an empty + # one here; _validate_display_config leaves both to this method. + if 'display' in config and not config['display']: + self.errors.append("Display configuration is empty") + except ConfigError as e: self.errors.append(f"Configuration error: {e}") except Exception as e: @@ -247,8 +252,7 @@ class StartupValidator: display_config = config.get('display', {}) if not display_config: - self.errors.append("Display configuration is missing") - return + return # reported by _validate_config hardware_config = display_config.get('hardware', {}) if not hardware_config: diff --git a/src/wifi_manager.py b/src/wifi_manager.py index 9e3dc0fb..86dde71f 100644 --- a/src/wifi_manager.py +++ b/src/wifi_manager.py @@ -9,24 +9,18 @@ Tested and optimized for: - Raspberry Pi OS Bookworm (Debian 12) with NetworkManager - Raspberry Pi 3B+, 4, 5 with built-in WiFi -Sudoers Requirements: - The following sudoers entries are required for passwordless operation. - Add to /etc/sudoers.d/ledmatrix_wifi: - - ledpi ALL=(ALL) NOPASSWD: /usr/bin/nmcli - ledpi ALL=(ALL) NOPASSWD: /usr/bin/systemctl start hostapd - ledpi ALL=(ALL) NOPASSWD: /usr/bin/systemctl stop hostapd - ledpi ALL=(ALL) NOPASSWD: /usr/bin/systemctl start dnsmasq - ledpi ALL=(ALL) NOPASSWD: /usr/bin/systemctl stop dnsmasq - ledpi ALL=(ALL) NOPASSWD: /usr/bin/systemctl restart NetworkManager - ledpi ALL=(ALL) NOPASSWD: /usr/sbin/ip - ledpi ALL=(ALL) NOPASSWD: /sbin/ip - ledpi ALL=(ALL) NOPASSWD: /usr/sbin/rfkill - ledpi ALL=(ALL) NOPASSWD: /usr/sbin/iptables - ledpi ALL=(ALL) NOPASSWD: /usr/sbin/sysctl - ledpi ALL=(ALL) NOPASSWD: /usr/bin/cp /tmp/hostapd.conf /etc/hostapd/hostapd.conf - ledpi ALL=(ALL) NOPASSWD: /usr/bin/cp /tmp/dnsmasq.conf /etc/dnsmasq.d/ledmatrix-captive.conf - ledpi ALL=(ALL) NOPASSWD: /usr/bin/rm -f /etc/dnsmasq.d/ledmatrix-captive.conf +Privileges: + The web interface runs as an unprivileged user and reaches nmcli, + systemctl, sysctl, nft and rfkill through exact-command sudo rules. + scripts/install/configure_wifi_permissions.sh writes those rules (and a + PolicyKit rule for NetworkManager); first_time_install.sh runs it. Use + that script rather than granting commands by hand. It deliberately + grants neither ``iptables`` nor ``ip``: their rules take a live interface + name, so they would need a wildcard, and ``iptables --modprobe=`` + and ``ip netns exec`` both run an arbitrary program as root. The code + paths that call them with sudo therefore only work where the user has + broader sudo rights (a stock Raspberry Pi image grants the default user + blanket NOPASSWD). """ import subprocess @@ -39,6 +33,8 @@ from pathlib import Path from typing import Any, Dict, List, Optional, Tuple from dataclasses import dataclass +from src.config_manager_atomic import atomic_write_json + logger = logging.getLogger(__name__) # Path for storing WiFi configuration (will be set dynamically) @@ -78,6 +74,22 @@ DNSMASQ_SERVICE = "dnsmasq" DEFAULT_AP_SSID = "LEDMatrix-Setup" DEFAULT_AP_CHANNEL = 7 +#: The access point's own address. Clients get 192.168.4.2-20 from dnsmasq +#: (hostapd mode) and every DNS name resolves here, which is what makes phones +#: show the captive-portal page. +AP_IP = "192.168.4.1" + +#: The web interface's port. The captive portal redirects port 80 to it. +PORTAL_PORT = 5000 + +#: The NetworkManager profile this module creates for the access point. +AP_PROFILE_NAME = "LEDMatrix-Setup-AP" + +#: AP profiles taken down and deleted before a new one is created and when AP +#: mode ends: ours, NetworkManager's default hotspot name, and an older name. +#: Deleted by name only, never by SSID, so a saved home network is never hit. +AP_PROFILE_NAMES = (AP_PROFILE_NAME, "Hotspot", "TickerSetup-AP") + # LED status message file (for display_controller integration) LED_STATUS_FILE = None # Will be set dynamically @@ -198,30 +210,8 @@ class WiFiManager: logger.debug(f"Could not clear LED status message: {e}") def _check_command(self, command: str) -> bool: - """Check if a command is available""" - try: - # First try 'which' command - result = subprocess.run( - ["which", command], - capture_output=True, - timeout=2 - ) - if result.returncode == 0: - return True - - # Check common sbin paths (not in standard user PATH) - sbin_paths = [ - f"/usr/sbin/{command}", - f"/sbin/{command}", - f"/usr/local/sbin/{command}" - ] - for path in sbin_paths: - if os.path.isfile(path) and os.access(path, os.X_OK): - return True - - return False - except (subprocess.TimeoutExpired, subprocess.SubprocessError, OSError): - return False + """Whether ``command`` is installed (see _find_command_path).""" + return self._find_command_path(command) is not None def _find_command_path(self, command: str) -> Optional[str]: """ @@ -321,14 +311,22 @@ class WiFiManager: del self.config["saved_networks"] self._save_config() - def _save_config(self): - """Save WiFi configuration to file""" + def _save_config(self) -> bool: + """Write ``self.config`` to ``self.config_path``. + + The write is atomic and keeps the file's owner and shared group (see + atomic_write_json), so a save by the root display service does not + lock the web user out of the file. Returns False when the file could + not be written, for example when an older root-run save left it + owned by root; the in-memory config is kept either way. + """ try: - with open(self.config_path, 'w') as f: - json.dump(self.config, f, indent=2) - logger.info(f"Saved WiFi config to {self.config_path}") - except Exception as e: - logger.error(f"Failed to save WiFi config: {e}") + atomic_write_json(self.config_path, self.config) + except (OSError, TypeError, ValueError) as e: + logger.error(f"Failed to save WiFi config to {self.config_path}: {e}") + return False + logger.info(f"Saved WiFi config to {self.config_path}") + return True def get_wifi_status(self) -> WiFiStatus: """ @@ -402,8 +400,6 @@ class WiFiManager: for line in result.stdout.strip().split('\n'): if '802-11-wireless.ssid:' in line: ssid = line.split(':', 1)[1].strip() - if ssid: - continue elif 'WIFI.SIGNAL:' in line: try: signal = int(line.split(':', 1)[1].strip()) @@ -425,24 +421,7 @@ class WiFiManager: ssid = parts[1].strip() if ssid: break - - # Fallback: Get signal strength if not already retrieved - if signal == 0 and wlan_device: - result = subprocess.run( - ["nmcli", "-t", "-f", "WIFI.SIGNAL", "device", "show", wlan_device], - capture_output=True, - text=True, - timeout=5 - ) - if result.returncode == 0: - for line in result.stdout.strip().split('\n'): - if 'WIFI.SIGNAL:' in line: - try: - signal = int(line.split(':', 1)[1].strip()) - break - except (ValueError, IndexError): - pass - + # Get IP address if connected if wifi_connected and wlan_device: result = subprocess.run( @@ -536,7 +515,7 @@ class WiFiManager: if result.returncode == 0: ips = result.stdout.strip().split() for ip in ips: - if not ip.startswith('192.168.4.1'): # Exclude AP IP + if ip != AP_IP: ip_address = ip break @@ -778,13 +757,13 @@ class WiFiManager: if subprocess.run( ["sudo", iptables, "-t", "nat", "-C", "PREROUTING", "-i", self._wifi_interface, "-p", "tcp", "--dport", "80", - "-j", "REDIRECT", "--to-port", "5000"], + "-j", "REDIRECT", "--to-port", str(PORTAL_PORT)], capture_output=True, timeout=5 ).returncode != 0: r = subprocess.run( ["sudo", iptables, "-t", "nat", "-A", "PREROUTING", "-i", self._wifi_interface, "-p", "tcp", "--dport", "80", - "-j", "REDIRECT", "--to-port", "5000"], + "-j", "REDIRECT", "--to-port", str(PORTAL_PORT)], capture_output=True, text=True, timeout=5 ) if r.returncode != 0: @@ -794,12 +773,12 @@ class WiFiManager: if subprocess.run( ["sudo", iptables, "-C", "INPUT", - "-i", self._wifi_interface, "-p", "tcp", "--dport", "5000", "-j", "ACCEPT"], + "-i", self._wifi_interface, "-p", "tcp", "--dport", str(PORTAL_PORT), "-j", "ACCEPT"], capture_output=True, timeout=5 ).returncode != 0: r = subprocess.run( ["sudo", iptables, "-A", "INPUT", - "-i", self._wifi_interface, "-p", "tcp", "--dport", "5000", "-j", "ACCEPT"], + "-i", self._wifi_interface, "-p", "tcp", "--dport", str(PORTAL_PORT), "-j", "ACCEPT"], capture_output=True, text=True, timeout=5 ) if r.returncode != 0: @@ -808,7 +787,7 @@ class WiFiManager: return False self._redirect_backend = "iptables" - logger.info("iptables: port 80→5000 redirect rules added") + logger.info(f"iptables: port 80→{PORTAL_PORT} redirect rules added") return True def _setup_iptables_redirect_nftables(self, nft: str) -> bool: @@ -819,7 +798,7 @@ class WiFiManager: ["sudo", nft, "add", "chain", "ip", "ledmatrix", "prerouting", "{", "type", "nat", "hook", "prerouting", "priority", "-100", ";", "}"], ["sudo", nft, "add", "rule", "ip", "ledmatrix", "prerouting", - "iif", self._wifi_interface, "tcp", "dport", "80", "redirect", "to", ":5000"], + "iif", self._wifi_interface, "tcp", "dport", "80", "redirect", "to", f":{PORTAL_PORT}"], ] for cmd in cmds: r = subprocess.run(cmd, capture_output=True, text=True, timeout=5) @@ -832,7 +811,7 @@ class WiFiManager: logger.debug(f"nft cmd non-zero (may already exist): {r.stderr.strip()}") self._redirect_backend = "nftables" - logger.info("nftables: port 80→5000 redirect rule added") + logger.info(f"nftables: port 80→{PORTAL_PORT} redirect rule added") return True def _teardown_iptables_redirect(self) -> None: @@ -847,12 +826,12 @@ class WiFiManager: subprocess.run( ["sudo", iptables, "-t", "nat", "-D", "PREROUTING", "-i", self._wifi_interface, "-p", "tcp", "--dport", "80", - "-j", "REDIRECT", "--to-port", "5000"], + "-j", "REDIRECT", "--to-port", str(PORTAL_PORT)], capture_output=True, timeout=5 ) subprocess.run( ["sudo", iptables, "-D", "INPUT", - "-i", self._wifi_interface, "-p", "tcp", "--dport", "5000", + "-i", self._wifi_interface, "-p", "tcp", "--dport", str(PORTAL_PORT), "-j", "ACCEPT"], capture_output=True, timeout=5 ) @@ -887,7 +866,7 @@ class WiFiManager: except Exception as e: logger.warning(f"Could not tear down port redirect: {e}") - def _write_nm_dnsmasq_captive_conf(self, ap_ip: str = "192.168.4.1") -> None: + def _write_nm_dnsmasq_captive_conf(self, ap_ip: str = AP_IP) -> None: """ Write the NM dnsmasq-shared.d drop-in that makes NM's built-in dnsmasq resolve every hostname to the AP IP. This triggers the OS captive-portal @@ -1026,39 +1005,53 @@ class WiFiManager: ) if result.returncode != 0: return [] - - seen_ssids = set() - for line in result.stdout.strip().split('\n'): - if not line or ':' not in line: - continue - parts = line.split(':') - if len(parts) >= 3: - ssid = parts[0].strip() - if not ssid or ssid in seen_ssids: - continue - seen_ssids.add(ssid) - try: - signal = int(parts[1].strip()) - security = parts[2].strip() if len(parts) > 2 else "open" - frequency_str = parts[3].strip() if len(parts) > 3 else "0" - frequency_str = frequency_str.replace(" MHz", "").replace("MHz", "").strip() - frequency = float(frequency_str) if frequency_str else 0.0 - if "WPA3" in security: - sec_type = "wpa3" - elif "WPA2" in security: - sec_type = "wpa2" - elif "WPA" in security: - sec_type = "wpa" - else: - sec_type = "open" - networks.append(WiFiNetwork(ssid=ssid, signal=signal, security=sec_type, frequency=frequency)) - except (ValueError, IndexError): - continue - networks.sort(key=lambda x: x.signal, reverse=True) + networks = self._parse_nmcli_wifi_list(result.stdout) except Exception as e: logger.debug(f"nmcli cached list failed: {e}") return networks + @staticmethod + def _parse_nmcli_wifi_list(stdout: str) -> List[WiFiNetwork]: + """Parse ``nmcli -t -f SSID,SIGNAL,SECURITY,FREQ device wifi list``. + + One entry per SSID (the first line seen for it; hidden networks with + an empty SSID are skipped), security reduced to wpa3/wpa2/wpa/open, + sorted strongest first. Unparseable lines are skipped. + """ + networks = [] + seen_ssids = set() + for line in stdout.strip().split('\n'): + if not line or ':' not in line: + continue + parts = line.split(':') + if len(parts) < 3: + continue + ssid = parts[0].strip() + if not ssid or ssid in seen_ssids: + continue + seen_ssids.add(ssid) + try: + signal = int(parts[1].strip()) + security = parts[2].strip() + frequency_str = parts[3].strip() if len(parts) > 3 else "0" + frequency_str = frequency_str.replace(" MHz", "").replace("MHz", "").strip() + frequency = float(frequency_str) if frequency_str else 0.0 + except (ValueError, IndexError) as e: + logger.debug(f"Skipping network line due to parsing error: {line[:50]}... Error: {e}") + continue + if "WPA3" in security: + sec_type = "wpa3" + elif "WPA2" in security: + sec_type = "wpa2" + elif "WPA" in security: + sec_type = "wpa" + else: + sec_type = "open" + networks.append(WiFiNetwork(ssid=ssid, signal=signal, security=sec_type, + frequency=frequency)) + networks.sort(key=lambda x: x.signal, reverse=True) + return networks + def _save_cached_scan(self, networks: List[WiFiNetwork]) -> None: """Save scan results to a cache file for use during AP mode.""" try: @@ -1088,7 +1081,6 @@ class WiFiManager: def _scan_nmcli(self) -> List[WiFiNetwork]: """Scan networks using nmcli""" - networks = [] try: # Trigger scan subprocess.run( @@ -1108,52 +1100,7 @@ class WiFiManager: if result.returncode != 0: return [] - - seen_ssids = set() - for line in result.stdout.strip().split('\n'): - if not line or ':' not in line: - continue - - parts = line.split(':') - if len(parts) >= 3: - ssid = parts[0].strip() - if not ssid or ssid in seen_ssids: - continue - - seen_ssids.add(ssid) - - try: - signal = int(parts[1].strip()) - security = parts[2].strip() if len(parts) > 2 else "open" - - # Parse frequency - strip " MHz" if present - frequency_str = parts[3].strip() if len(parts) > 3 else "0" - frequency_str = frequency_str.replace(" MHz", "").replace("MHz", "").strip() - frequency = float(frequency_str) if frequency_str else 0.0 - - # Normalize security type - if "WPA3" in security: - sec_type = "wpa3" - elif "WPA2" in security: - sec_type = "wpa2" - elif "WPA" in security: - sec_type = "wpa" - else: - sec_type = "open" - - networks.append(WiFiNetwork( - ssid=ssid, - signal=signal, - security=sec_type, - frequency=frequency - )) - except (ValueError, IndexError) as e: - logger.debug(f"Skipping network line due to parsing error: {line[:50]}... Error: {e}") - continue - - # Sort by signal strength - networks.sort(key=lambda x: x.signal, reverse=True) - return networks + return self._parse_nmcli_wifi_list(result.stdout) except Exception as e: logger.error(f"Error scanning with nmcli: {e}") return [] @@ -1357,27 +1304,7 @@ class WiFiManager: disconnect_success, disconnect_msg = self.disconnect_from_network(skip_ap_check=True) if disconnect_success: logger.info(f"Disconnected from {original_ssid}: {disconnect_msg}") - # Wait for device to be ready for new connection - # Check device state before proceeding - max_wait = 5 - wait_count = 0 - while wait_count < max_wait: - time.sleep(1) - result = subprocess.run( - ["nmcli", "-t", "-f", "STATE", "device", "status", self._wifi_interface], - capture_output=True, - text=True, - timeout=5 - ) - if result.returncode == 0: - state = result.stdout.strip().split(':')[-1] if ':' in result.stdout else result.stdout.strip() - # Device is ready if it's disconnected or unavailable (not connecting/connected) - if state in ["disconnected", "unavailable", "unmanaged"]: - logger.info(f"Device ready for new connection (state: {state})") - break - wait_count += 1 - - if wait_count >= max_wait: + if not self._wait_for_device_idle(5): logger.warning("Device may not be ready, but proceeding with connection attempt") else: logger.warning(f"Failed to disconnect from {original_ssid}: {disconnect_msg}") @@ -1403,26 +1330,15 @@ class WiFiManager: return False, f"Failed to connect to {ssid}, restored {original_ssid}" else: logger.error(f"Failed to restore original connection: {original_ssid}") - # Trigger AP mode as last resort - self._show_led_message("Enabling AP mode...", duration=5) - ap_success, ap_msg = self.enable_ap_mode(force=True) - if ap_success: - logger.info("AP mode enabled as failsafe") - return False, "Connection failed and restoration failed. AP mode enabled." - else: - logger.error(f"Failed to enable AP mode: {ap_msg}") - return False, f"Connection failed, restoration failed, and AP mode failed: {ap_msg}" + return self._failsafe_ap( + "Connection failed and restoration failed. AP mode enabled.", + "Connection failed, restoration failed, and AP mode failed") # If connection failed and no original connection to restore, enable AP mode elif not success: logger.warning(f"Connection to {ssid} failed and no original connection to restore") - self._show_led_message("Enabling AP mode...", duration=5) - ap_success, ap_msg = self.enable_ap_mode(force=True) - if ap_success: - logger.info("AP mode enabled as failsafe") - return False, "Connection failed. AP mode enabled." - else: - return False, f"Connection failed and AP mode failed: {ap_msg}" + return self._failsafe_ap("Connection failed. AP mode enabled.", + "Connection failed and AP mode failed") return success, message else: @@ -1443,6 +1359,22 @@ class WiFiManager: logger.error("Last-resort AP mode enable failed in recovery path: %s", ap_error, exc_info=True) return False, str(e) + def _failsafe_ap(self, enabled_msg: str, failed_msg: str) -> Tuple[bool, str]: + """Force the setup AP up after a connect that left no working network, + so the user can still reach the device. + + Returns the (False, message) result for connect_to_network: + ``enabled_msg`` when the AP came up, else ``failed_msg`` plus the + reason it did not. + """ + self._show_led_message("Enabling AP mode...", duration=5) + ap_success, ap_msg = self.enable_ap_mode(force=True) + if ap_success: + logger.info("AP mode enabled as failsafe") + return False, enabled_msg + logger.error(f"Failed to enable AP mode: {ap_msg}") + return False, f"{failed_msg}: {ap_msg}" + def _restore_original_connection(self, connection_name: str, ssid: str) -> bool: """ Restore a previously active WiFi connection. @@ -1496,68 +1428,96 @@ class WiFiManager: logger.error(f"Error restoring connection: {e}") return False + def _find_profile_for_ssid(self, ssid: str) -> Optional[str]: + """Name of the saved NetworkManager profile for ``ssid``, or None. + + ``802-11-wireless.ssid`` is not a column ``nmcli connection show`` + can list, so this lists the Wi-Fi profiles and asks each one for its + SSID. A profile named after the SSID is the fallback, for when the + listing fails. + """ + list_result = subprocess.run( # nosec B603 B607 - fixed args, no user input + ["nmcli", "-t", "-f", "NAME,TYPE", "connection", "show"], + capture_output=True, text=True, timeout=5 + ) + if list_result.returncode == 0: + for line in list_result.stdout.strip().split('\n'): + # Terse output escapes a colon inside a field as "\:". TYPE + # never contains one, so the last colon ends the name. + conn_name, sep, conn_type = line.rpartition(':') + if not sep or conn_type.strip() != '802-11-wireless': + continue + conn_name = conn_name.replace('\\:', ':').replace('\\\\', '\\') + ssid_r = subprocess.run( # nosec B603 B607 - conn_name from nmcli output, not user input + ["nmcli", "-g", "802-11-wireless.ssid", "connection", "show", conn_name], + capture_output=True, text=True, timeout=5 + ) + if ssid_r.returncode == 0 and ssid_r.stdout.strip() == ssid: + return conn_name + + direct_check = subprocess.run( # nosec B603 B607 - list args, no shell + ["nmcli", "connection", "show", ssid], + capture_output=True, text=True, timeout=5 + ) + if direct_check.returncode == 0: + return ssid + return None + + def _wait_for_device_idle(self, attempts: int) -> bool: + """Poll the Wi-Fi device, once a second for up to ``attempts`` checks, + until it is disconnected, unavailable or unmanaged: a profile + activated while the device is still connecting or tearing down an + old link can fail. True if it went idle, False on timeout.""" + for attempt in range(attempts): + result = subprocess.run( + ["nmcli", "-t", "-f", "STATE", "device", "status", self._wifi_interface], + capture_output=True, + text=True, + timeout=5 + ) + if result.returncode == 0: + state = result.stdout.strip().split(':')[-1] + if state in ("disconnected", "unavailable", "unmanaged"): + logger.debug(f"Wi-Fi device idle (state: {state})") + return True + if attempt < attempts - 1: + time.sleep(1) + return False + + def _verify_connected(self, ssid: str, attempts: int = 5, delay: float = 2.0, + stop_on_other_network: bool = False) -> Optional[WiFiStatus]: + """Wait for the device to report a connection to ``ssid``. + + nmcli returns before DHCP finishes, so the status is polled every + ``delay`` seconds, up to ``attempts`` times. Returns that status, or + None if it never showed ``ssid``. With ``stop_on_other_network`` a + connection to a different SSID ends the wait at once as a failure. + """ + for _ in range(attempts): + time.sleep(delay) + status = self.get_wifi_status() + if not status.connected: + continue + if status.ssid == ssid: + return status + if stop_on_other_network and status.ssid: + logger.warning(f"Connected to wrong network: {status.ssid} instead of {ssid}") + return None + return None + def _connect_nmcli(self, ssid: str, password: str) -> Tuple[bool, str]: """Connect using nmcli""" try: # Show LED message self._show_led_message(f"Connecting to {ssid}...", duration=10) - # Find existing NM connection for this SSID. - # 802-11-wireless.ssid is not a valid column in 'nmcli connection show', - # so list all wifi connections then query each one's SSID individually. - list_result = subprocess.run( # nosec B603 B607 - fixed args, no user input - ["nmcli", "-t", "-f", "NAME,TYPE", "connection", "show"], - capture_output=True, text=True, timeout=5 - ) - existing_conn_name = None - if list_result.returncode == 0: - for line in list_result.stdout.strip().split('\n'): - if ':' not in line: - continue - parts = line.split(':') - if len(parts) < 2 or parts[1].strip() != '802-11-wireless': - continue - conn_name = parts[0].strip() - ssid_r = subprocess.run( # nosec B603 B607 - conn_name from nmcli output, not user input - ["nmcli", "-g", "802-11-wireless.ssid", "connection", "show", conn_name], - capture_output=True, text=True, timeout=5 - ) - if ssid_r.returncode == 0 and ssid_r.stdout.strip() == ssid: - existing_conn_name = conn_name - break - - # Also try direct lookup by SSID (in case connection name matches SSID) - if not existing_conn_name: - direct_check = subprocess.run( - ["nmcli", "connection", "show", ssid], - capture_output=True, - text=True, - timeout=5 - ) - if direct_check.returncode == 0: - existing_conn_name = ssid - + existing_conn_name = self._find_profile_for_ssid(ssid) if existing_conn_name: # Connection exists, try to activate it first (faster and more reliable) logger.info(f"Found existing connection for {ssid}, activating...") - # Ensure device is ready before activating - # Wait for device to be in disconnected/unavailable state - max_wait = 3 - for wait_attempt in range(max_wait): - device_result = subprocess.run( - ["nmcli", "-t", "-f", "STATE", "device", "status", self._wifi_interface], - capture_output=True, - text=True, - timeout=5 - ) - if device_result.returncode == 0: - state = device_result.stdout.strip().split(':')[-1] if ':' in device_result.stdout else device_result.stdout.strip() - if state in ["disconnected", "unavailable", "unmanaged"]: - break - if wait_attempt < max_wait - 1: - time.sleep(1) - + self._wait_for_device_idle(3) + result = subprocess.run( ["nmcli", "connection", "up", existing_conn_name], capture_output=True, @@ -1566,19 +1526,8 @@ class WiFiManager: ) if result.returncode == 0: - # Wait longer for connection to stabilize and verify multiple times - max_verification_attempts = 5 - verification_delay = 2 - connected = False - - for attempt in range(max_verification_attempts): - time.sleep(verification_delay) - status = self.get_wifi_status() - if status.connected and status.ssid == ssid: - connected = True - break - - if connected: + status = self._verify_connected(ssid) + if status is not None: ip = status.ip_address or "Unknown" self._show_led_message(f"Connected! {ip}", duration=5) logger.info(f"Successfully connected to {ssid} with IP {ip}") @@ -1605,25 +1554,8 @@ class WiFiManager: ) if result.returncode == 0: - # Wait longer for connection to stabilize and verify multiple times - max_verification_attempts = 5 - verification_delay = 2 - connected = False - - for attempt in range(max_verification_attempts): - time.sleep(verification_delay) - status = self.get_wifi_status() - if status.connected: - # Verify we're connected to the correct SSID - if status.ssid == ssid: - connected = True - break - elif status.ssid: - # Connected to different network - this is a failure - logger.warning(f"Connected to wrong network: {status.ssid} instead of {ssid}") - break - - if connected: + status = self._verify_connected(ssid, stop_on_other_network=True) + if status is not None: ip = status.ip_address or "Unknown" self._show_led_message(f"Connected! {ip}", duration=5) logger.info(f"Successfully connected to {ssid} with IP {ip}") @@ -1717,14 +1649,10 @@ class WiFiManager: return any(ind in lower for ind in indicators) def _connect_wpa_supplicant(self, ssid: str, password: str) -> Tuple[bool, str]: - """Connect using wpa_supplicant (fallback)""" - try: - # This would require modifying /etc/wpa_supplicant/wpa_supplicant.conf - # For now, return not implemented - return False, "wpa_supplicant connection not yet implemented. Please use NetworkManager (nmcli)." - except Exception as e: - logger.error(f"Error connecting with wpa_supplicant: {e}") - return False, str(e) + """Without NetworkManager there is no supported way to connect: doing it + through wpa_supplicant would mean editing its config file, which is + not implemented. Always returns (False, reason).""" + return False, "wpa_supplicant connection not yet implemented. Please use NetworkManager (nmcli)." def disconnect_from_network(self, skip_ap_check: bool = False) -> Tuple[bool, str]: """ @@ -1745,33 +1673,18 @@ class WiFiManager: # Disconnect using nmcli if self.has_nmcli: - # Try to disconnect the specific connection first (more reliable) + # Take the profile down first, then the device, so the + # device ends up disconnected even when no profile is found. if status.ssid: - # Find the connection name for this SSID - conn_result = subprocess.run( - ["nmcli", "-t", "-f", "NAME,802-11-wireless.ssid", "connection", "show"], - capture_output=True, - text=True, - timeout=5 - ) - if conn_result.returncode == 0: - for line in conn_result.stdout.strip().split('\n'): - if ':' in line: - parts = line.split(':') - if len(parts) >= 2: - conn_name = parts[0].strip() - conn_ssid = parts[1].strip() if len(parts) > 1 else "" - if conn_ssid == status.ssid: - # Disconnect this specific connection - subprocess.run( - ["nmcli", "connection", "down", conn_name], - capture_output=True, - timeout=10 - ) - logger.info(f"Disconnected connection {conn_name} for {status.ssid}") - break - - # Also disconnect the device to ensure clean state + conn_name = self._find_profile_for_ssid(status.ssid) + if conn_name: + subprocess.run( # nosec B603 B607 - list args, no shell + ["nmcli", "connection", "down", conn_name], + capture_output=True, + timeout=10 + ) + logger.info(f"Disconnected connection {conn_name} for {status.ssid}") + result = subprocess.run( ["nmcli", "device", "disconnect", self._wifi_interface], capture_output=True, @@ -1814,7 +1727,11 @@ class WiFiManager: max_retries: Maximum number of retry attempts to enable WiFi radio Returns: - True if WiFi is enabled or was successfully enabled, False otherwise + True if the radio is enabled or was enabled here. Also True when + the state could not be checked at all (nmcli or rfkill raised on + every attempt): callers go ahead rather than refusing to act on a + radio that is probably fine. False only when the radio was seen + disabled or blocked and could not be turned on. """ for attempt in range(max_retries): try: @@ -2062,11 +1979,7 @@ class WiFiManager: if result[0]: self._ap_enabled_at = time.time() if force: - try: - self._FORCE_AP_FLAG_PATH.touch() - logger.debug(f"Force-AP flag created: {self._FORCE_AP_FLAG_PATH}") - except OSError as exc: - logger.warning(f"Failed to create force-AP flag {self._FORCE_AP_FLAG_PATH}: {exc}") + self._mark_forced() return result # Fallback to nmcli hotspot (simpler, no captive portal) @@ -2077,11 +1990,7 @@ class WiFiManager: if result[0]: self._ap_enabled_at = time.time() if force: - try: - self._FORCE_AP_FLAG_PATH.touch() - logger.debug(f"Force-AP flag created: {self._FORCE_AP_FLAG_PATH}") - except OSError as exc: - logger.warning(f"Failed to create force-AP flag {self._FORCE_AP_FLAG_PATH}: {exc}") + self._mark_forced() return result return False, "No WiFi tools available (nmcli, hostapd, or dnsmasq required)" @@ -2089,6 +1998,15 @@ class WiFiManager: logger.error(f"Error in enable_ap_mode: {e}") return False, str(e) + def _mark_forced(self) -> None: + """Record that AP mode was forced on, so the periodic check leaves it + up even when Ethernet is connected (see _manage_ap_mode).""" + try: + self._FORCE_AP_FLAG_PATH.touch() + logger.debug(f"Force-AP flag created: {self._FORCE_AP_FLAG_PATH}") + except OSError as exc: + logger.warning(f"Failed to create force-AP flag {self._FORCE_AP_FLAG_PATH}: {exc}") + def _enable_ap_mode_hostapd(self) -> Tuple[bool, str]: """Enable AP mode using hostapd and dnsmasq (captive portal)""" try: @@ -2115,7 +2033,7 @@ class WiFiManager: timeout=10 ) subprocess.run( - ["sudo", "ip", "addr", "add", "192.168.4.1/24", "dev", self._wifi_interface], + ["sudo", "ip", "addr", "add", f"{AP_IP}/24", "dev", self._wifi_interface], capture_output=True, timeout=10 ) @@ -2124,7 +2042,7 @@ class WiFiManager: capture_output=True, timeout=10 ) - logger.info(f"Configured {self._wifi_interface} with IP 192.168.4.1 for AP mode") + logger.info(f"Configured {self._wifi_interface} with IP {AP_IP} for AP mode") except (subprocess.TimeoutExpired, subprocess.SubprocessError, OSError) as e: logger.warning(f"Error setting up {self._wifi_interface} IP: {e}") @@ -2168,7 +2086,7 @@ class WiFiManager: # Use the validated SSID so the displayed name matches what hostapd broadcast ap_ssid, _ = self._validate_ap_config() self._show_led_message( - f"WiFi Setup\n{ap_ssid}\nNo password\n192.168.4.1:5000", duration=10 + f"WiFi Setup\n{ap_ssid}\nNo password\n{AP_IP}:{PORTAL_PORT}", duration=10 ) return True, "AP mode enabled" except Exception as e: @@ -2198,7 +2116,7 @@ class WiFiManager: # Delete only the specific application-managed AP profiles by name. # Never delete by SSID — that would destroy a user's saved home network. - for conn_name in ["Hotspot", "LEDMatrix-Setup-AP", "TickerSetup-AP"]: + for conn_name in AP_PROFILE_NAMES: subprocess.run(["nmcli", "connection", "down", conn_name], capture_output=True, timeout=5) subprocess.run(["nmcli", "connection", "delete", conn_name], @@ -2215,14 +2133,14 @@ class WiFiManager: cmd = [ "nmcli", "connection", "add", "type", "wifi", - "con-name", "LEDMatrix-Setup-AP", + "con-name", AP_PROFILE_NAME, "ifname", self._wifi_interface, "ssid", ap_ssid, "802-11-wireless.mode", "ap", "802-11-wireless.band", "bg", # 2.4 GHz for maximum compatibility "802-11-wireless.channel", str(ap_channel), "ipv4.method", "shared", - "ipv4.addresses", "192.168.4.1/24", + "ipv4.addresses", f"{AP_IP}/24", # No 802-11-wireless-security section → open network ] @@ -2247,14 +2165,14 @@ class WiFiManager: logger.info("AP connection profile created, bringing it up...") up_result = subprocess.run( - ["nmcli", "connection", "up", "LEDMatrix-Setup-AP"], + ["nmcli", "connection", "up", AP_PROFILE_NAME], capture_output=True, text=True, timeout=20 ) if up_result.returncode != 0: error_msg = up_result.stderr.strip() or up_result.stdout.strip() logger.error(f"Failed to bring up AP connection: {error_msg}") self._remove_nm_dnsmasq_captive_conf() - subprocess.run(["nmcli", "connection", "delete", "LEDMatrix-Setup-AP"], + subprocess.run(["nmcli", "connection", "delete", AP_PROFILE_NAME], capture_output=True, timeout=10) self._show_led_message("AP mode failed", duration=5) return False, f"Failed to start AP: {error_msg}" @@ -2266,9 +2184,9 @@ class WiFiManager: if not self._setup_iptables_redirect(): logger.error("Captive-portal redirect setup failed; rolling back AP profile") self._remove_nm_dnsmasq_captive_conf() - subprocess.run(["nmcli", "connection", "down", "LEDMatrix-Setup-AP"], + subprocess.run(["nmcli", "connection", "down", AP_PROFILE_NAME], capture_output=True, timeout=10) - subprocess.run(["nmcli", "connection", "delete", "LEDMatrix-Setup-AP"], + subprocess.run(["nmcli", "connection", "delete", AP_PROFILE_NAME], capture_output=True, timeout=10) self._clear_led_message() return False, "AP started but captive-portal redirect setup failed" @@ -2282,17 +2200,17 @@ class WiFiManager: logger.debug(f"AP verification attempt {_attempt + 1}/5 not yet active, waiting 2s") time.sleep(2) if status.get('active'): - ip = status.get('ip', '192.168.4.1') + ip = status.get('ip', AP_IP) logger.info(f"AP mode confirmed active at {ip} (open network, no password)") - self._show_led_message(f"WiFi Setup\n{ap_ssid}\nNo password\n{ip}:5000", duration=10) - return True, f"AP mode enabled (open network) - Access at {ip}:5000" + self._show_led_message(f"WiFi Setup\n{ap_ssid}\nNo password\n{ip}:{PORTAL_PORT}", duration=10) + return True, f"AP mode enabled (open network) - Access at {ip}:{PORTAL_PORT}" else: logger.error("AP mode started but not verified by status check — rolling back") self._teardown_iptables_redirect() self._remove_nm_dnsmasq_captive_conf() - subprocess.run(["nmcli", "connection", "down", "LEDMatrix-Setup-AP"], + subprocess.run(["nmcli", "connection", "down", AP_PROFILE_NAME], capture_output=True, timeout=10) - subprocess.run(["nmcli", "connection", "delete", "LEDMatrix-Setup-AP"], + subprocess.run(["nmcli", "connection", "delete", AP_PROFILE_NAME], capture_output=True, timeout=10) self._clear_led_message() return False, "AP mode started but verification failed" @@ -2326,9 +2244,9 @@ class WiFiManager: conn_name = parts[0].strip() conn_type = parts[1].strip().lower() # Match our known AP profile name OR the legacy nmcli hotspot type - if conn_name == "LEDMatrix-Setup-AP" or 'hotspot' in conn_type: + if conn_name == AP_PROFILE_NAME or 'hotspot' in conn_type: # Get actual IP address (may be 192.168.4.1 or 10.42.0.1 depending on config) - ip = '192.168.4.1' + ip = AP_IP interface = parts[2] if len(parts) > 2 else self._wifi_interface try: ip_result = subprocess.run( @@ -2399,7 +2317,7 @@ class WiFiManager: ) else: # Disable nmcli hotspot mode (fallback) - for conn_name in ["LEDMatrix-Setup-AP", "Hotspot", "TickerSetup-AP"]: + for conn_name in AP_PROFILE_NAMES: subprocess.run( ["nmcli", "connection", "down", conn_name], capture_output=True, @@ -2428,7 +2346,7 @@ class WiFiManager: # Clean up WiFi interface IP configuration subprocess.run( - ["sudo", "ip", "addr", "del", "192.168.4.1/24", "dev", self._wifi_interface], + ["sudo", "ip", "addr", "del", f"{AP_IP}/24", "dev", self._wifi_interface], capture_output=True, timeout=10 ) @@ -2541,13 +2459,13 @@ ignore_broadcast_ssid=0 dhcp-range=192.168.4.2,192.168.4.20,255.255.255.0,24h # Captive portal: Redirect all DNS queries to Pi -address=/#/192.168.4.1 +address=/#/{AP_IP} # Captive portal detection endpoints -address=/captive.apple.com/192.168.4.1 -address=/connectivitycheck.gstatic.com/192.168.4.1 -address=/www.msftconnecttest.com/192.168.4.1 -address=/detectportal.firefox.com/192.168.4.1 +address=/captive.apple.com/{AP_IP} +address=/connectivitycheck.gstatic.com/{AP_IP} +address=/www.msftconnecttest.com/{AP_IP} +address=/detectportal.firefox.com/{AP_IP} """ # Write config (requires sudo) @@ -2651,9 +2569,10 @@ address=/detectportal.firefox.com/192.168.4.1 # Pre-cache a WiFi scan so the captive portal can show networks try: logger.info("Running pre-AP WiFi scan for captive portal cache...") + # AP mode is not up yet, so this is a live scan, and + # scan_networks saves its result for the portal. networks, _cached = self.scan_networks(allow_cached=False) if networks: - self._save_cached_scan(networks) logger.info(f"Cached {len(networks)} networks for captive portal") except Exception as scan_err: logger.debug(f"Pre-AP scan failed (non-critical): {scan_err}") diff --git a/test/test_api_helper.py b/test/test_api_helper.py index 55449766..6df0960d 100644 --- a/test/test_api_helper.py +++ b/test/test_api_helper.py @@ -92,7 +92,7 @@ class TestGet: helper.session.get.assert_not_called() rate_spy.assert_not_called() - def test_cache_miss_fetches_and_caches_without_ttl(self, helper, cache): + def test_cache_miss_fetches_and_caches_with_ttl(self, helper, cache): cache.get.return_value = None helper.session.get = Mock(return_value=_make_response({'a': 1})) @@ -100,9 +100,47 @@ class TestGet: cache_ttl=999) assert result == {'a': 1} - # Pin the ttl-dropped contract: CacheManager.set is called with - # (key, data) only — the cache_ttl argument is discarded. - cache.set.assert_called_once_with('k', {'a': 1}) + cache.set.assert_called_once_with('k', {'a': 1}, ttl=999) + + def test_set_cache_passes_ttl(self, helper, cache): + helper.set_cache('k', {'a': 1}, ttl=42) + cache.set.assert_called_once_with('k', {'a': 1}, ttl=42) + + +class TestCacheLifetimeWithRealCacheManager: + """cache_ttl decides how long a response is reused, in both directions: + past CacheManager's 300-second default read age, and not beyond it.""" + + @pytest.fixture + def real_cache(self, tmp_path): + from unittest.mock import patch + from src.cache_manager import CacheManager + with patch('src.cache_manager.CacheManager._get_writable_cache_dir', + return_value=str(tmp_path)): + cache = CacheManager() + yield cache + # Releases the class-wide cleanup-thread claim on this directory, + # which would otherwise leak into test_cache_cleanup_thread_ownership. + cache.stop_cleanup_thread() + + def _fetch_twice(self, real_cache, monkeypatch, ttl, elapsed): + helper = APIHelper(cache_manager=real_cache) + helper.set_rate_limit(0) + helper.session.get = Mock(side_effect=[_make_response({'n': 1}), + _make_response({'n': 2})]) + now = [1_000_000.0] + monkeypatch.setattr('src.cache.memory_cache.time.time', lambda: now[0]) + monkeypatch.setattr('src.cache.disk_cache.time.time', lambda: now[0]) + monkeypatch.setattr('src.cache_manager.time.time', lambda: now[0]) + helper.get('https://example.com/api', cache_key='lifetime_test', cache_ttl=ttl) + now[0] += elapsed + return helper.get('https://example.com/api', cache_key='lifetime_test', cache_ttl=ttl) + + def test_long_ttl_outlives_the_default_read_age(self, real_cache, monkeypatch): + assert self._fetch_twice(real_cache, monkeypatch, ttl=3600, elapsed=1000) == {'n': 1} + + def test_short_ttl_expires(self, real_cache, monkeypatch): + assert self._fetch_twice(real_cache, monkeypatch, ttl=60, elapsed=120) == {'n': 2} def test_request_exception_returns_none_and_caches_nothing( self, helper, cache): @@ -223,15 +261,6 @@ class TestClearCache: manager.clear_cache.assert_called_once_with() - def test_no_pattern_falls_back_to_clear(self): - manager = types.SimpleNamespace(clear=Mock()) - helper = APIHelper(cache_manager=manager) - helper.set_rate_limit(0) - - helper.clear_cache() - - manager.clear.assert_called_once_with() - def test_no_pattern_manager_without_any_clear_is_noop(self): helper = APIHelper(cache_manager=object()) helper.set_rate_limit(0) diff --git a/test/test_api_v3_wifi_endpoints.py b/test/test_api_v3_wifi_endpoints.py index 2f1df946..38814fb4 100644 --- a/test/test_api_v3_wifi_endpoints.py +++ b/test/test_api_v3_wifi_endpoints.py @@ -408,6 +408,14 @@ class TestAutoEnableApMode: assert response.status_code == 400 assert "auto_enable_ap_mode" not in wifi_manager.config + def test_a_failed_save_is_reported(self, api_v3_client, wifi_manager): + # wifi_config.json left owned by root is the usual cause. + wifi_manager.config = {} + wifi_manager._save_config.return_value = False + response = api_v3_client.post(self.URL, json={"auto_enable_ap_mode": False}) + assert response.status_code == 500 + assert response.get_json()["status"] == "error" + class TestRadioEnabledAndForceAcceptIntegers: """`{"enabled": 1}` / `{"enabled": 0}` used to be mishandled: the old diff --git a/test/test_background_data_service.py b/test/test_background_data_service.py index ef066d7c..ed16bb1a 100644 --- a/test/test_background_data_service.py +++ b/test/test_background_data_service.py @@ -362,6 +362,3 @@ class TestPriorityIsAcceptedAndIgnored: rid = service.submit_fetch_request( "nfl", 2026, "http://example.invalid/x", cache_key="k", priority=5) assert service.get_result(rid).cached is True - - def test_statistics_still_report_an_empty_queue(self, service): - assert service.get_statistics()["queue_size"] == 0 diff --git a/test/test_backup_manager.py b/test/test_backup_manager.py index fb603d43..0097c213 100644 --- a/test/test_backup_manager.py +++ b/test/test_backup_manager.py @@ -180,6 +180,31 @@ def test_create_backup_manifest(project: Path, tmp_path: Path) -> None: assert set(manifest["contents"]) >= {"config", "secrets", "wifi", "fonts", "plugin_uploads", "plugins"} +def test_manifest_version_is_the_core_release(project: Path, tmp_path: Path) -> None: + """Not a git sha or a truncated "ref: refs/he..." read from .git/HEAD.""" + from src import __version__ + git = project / ".git" + git.mkdir() + (git / "HEAD").write_text("ref: refs/heads/some-branch-that-is-not-there\n", encoding="utf-8") + zip_path = create_backup(project, output_dir=tmp_path / "exports") + with zipfile.ZipFile(zip_path) as zf: + manifest = json.loads(zf.read("manifest.json")) + assert manifest["ledmatrix_version"] == __version__ + + +def test_installed_plugins_come_from_the_configured_directory(tmp_path: Path) -> None: + root = tmp_path / "proj" + (root / "config").mkdir(parents=True) + (root / "config" / "config.json").write_text( + json.dumps({"plugin_system": {"plugins_directory": "plugins"}}), encoding="utf-8") + plugin_dir = root / "plugins" / "dev-plugin" + plugin_dir.mkdir(parents=True) + (plugin_dir / "manifest.json").write_text( + json.dumps({"id": "dev-plugin", "version": "0.3.0"}), encoding="utf-8") + + assert [p["plugin_id"] for p in list_installed_plugins(root)] == ["dev-plugin"] + + # --------------------------------------------------------------------------- # Validate # --------------------------------------------------------------------------- diff --git a/test/test_base_odds_manager.py b/test/test_base_odds_manager.py index e53a5036..ad7cec4b 100644 --- a/test/test_base_odds_manager.py +++ b/test/test_base_odds_manager.py @@ -342,7 +342,6 @@ class TestLoadConfiguration: 'base_odds_manager': { 'update_interval': 100, 'timeout': 5, - 'cache_ttl': 42, } } @@ -352,7 +351,6 @@ class TestLoadConfiguration: # Key/attr mismatch pin: the config key is 'timeout' but the # attribute is request_timeout. assert manager.request_timeout == 5 - assert manager.cache_ttl == 42 def test_get_config_raising_keeps_defaults(self, cache_manager): config_manager = MagicMock() @@ -362,4 +360,3 @@ class TestLoadConfiguration: assert manager.update_interval == 3600 assert manager.request_timeout == 5 - assert manager.cache_ttl == 1800 diff --git a/test/test_element_visibility_align_scale.py b/test/test_element_visibility_align_scale.py index 171f9f4e..3209bad4 100644 --- a/test/test_element_visibility_align_scale.py +++ b/test/test_element_visibility_align_scale.py @@ -185,3 +185,35 @@ class TestLogoScale: def test_an_unusable_scale_is_ignored(self, logo, bad): helper = LogoHelper(display_width=64, display_height=32) assert helper.load_logo("AAA", logo, 32, 32, scale=bad).size == (32, 32) + + def test_a_scale_the_schema_allows_is_applied(self, logo): + """The Scale field's maximum is honoured, not reset to 1.0.""" + from src.element_style import MAX_ELEMENT_SCALE + helper = LogoHelper(display_width=64, display_height=32) + big = helper.load_logo("AAA", logo, 4, 4, scale=MAX_ELEMENT_SCALE) + assert big.size == (40, 40) + + def test_a_scale_beyond_the_range_is_clamped(self, logo): + from src.element_style import MAX_ELEMENT_SCALE, MIN_ELEMENT_SCALE + helper = LogoHelper(display_width=64, display_height=32) + assert helper.load_logo("AAA", logo, 4, 4, scale=MAX_ELEMENT_SCALE * 3).size == (40, 40) + assert helper.load_logo("AAA", logo, 40, 40, scale=MIN_ELEMENT_SCALE / 2).size == (4, 4) + + +class TestScaleCoercion: + """One range for the schema, element_scale and LogoHelper.""" + + def test_schema_bounds_are_the_clamp_bounds(self): + from src.element_style import (MAX_ELEMENT_SCALE, MIN_ELEMENT_SCALE, + _offset_block_from_spec) + prop = _offset_block_from_spec("home_logo", {"scale": True})["properties"]["scale"] + assert (prop["minimum"], prop["maximum"]) == (MIN_ELEMENT_SCALE, MAX_ELEMENT_SCALE) + + @pytest.mark.parametrize("raw,expected", [ + (0.5, 0.5), (25, 10.0), (0.01, 0.1), + (0, 1.0), (-2, 1.0), ("x", 1.0), (True, 1.0), + (float("nan"), 1.0), (float("inf"), 1.0), + ]) + def test_element_scale_clamps_and_rejects(self, raw, expected): + cfg = {"customization": {"layout": {"home_logo": {"scale": raw}}}} + assert element_scale(cfg, "home_logo") == expected diff --git a/test/test_error_aggregator.py b/test/test_error_aggregator.py index 84bb89e3..2d8ca40e 100644 --- a/test/test_error_aggregator.py +++ b/test/test_error_aggregator.py @@ -124,6 +124,45 @@ class TestErrorRecording: assert aggregator._plugin_error_counts["plugin-a"]["ValueError"] == 2 assert aggregator._plugin_error_counts["plugin-b"]["ValueError"] == 1 + def test_stack_trace_recorded_outside_except_block(self): + """The trace comes from the exception, not from the handler in progress. + + plugin_executor records exceptions caught on a worker thread after + its except block has ended, where format_exc() only says + "NoneType: None". + """ + def failing_plugin_update(): + raise ValueError("boom") + + caught = [] + + def worker(): + try: + failing_plugin_update() + except ValueError as e: + caught.append(e) + + thread = threading.Thread(target=worker) + thread.start() + thread.join() + + record = ErrorAggregator().record_error(caught[0], plugin_id="p") + + assert "NoneType: None" not in record.stack_trace + assert "failing_plugin_update" in record.stack_trace + assert "ValueError: boom" in record.stack_trace + + def test_record_error_leaves_caller_context_unchanged(self): + """LEDMatrixError context is merged into a copy of the caller's dict.""" + context = {"caller": "value"} + error = PluginError("failed", plugin_id="p", context={"extra": 1}) + + record = ErrorAggregator().record_error(error, context=context) + + assert context == {"caller": "value"} + assert record.context["caller"] == "value" + assert record.context["extra"] == 1 + class TestPatternDetection: """Test error pattern detection.""" diff --git a/test/test_font_manager.py b/test/test_font_manager.py index 232361ce..e9e80c7b 100644 --- a/test/test_font_manager.py +++ b/test/test_font_manager.py @@ -8,10 +8,14 @@ test here asserts observable behavior: returned font types, cache identity, fallback selection, and BDF native-size reading. """ +import json +import shutil + import freetype import pytest from PIL import ImageFont +from src.common.font_layout import resolve_asset_path from src.font_manager import FontManager @@ -132,3 +136,37 @@ class TestCacheLifecycle: fm.reload_config({}) assert fm.cache_generation == gen_before + 1 assert not fm.font_cache + + +class TestPluginFonts: + """plugin:// sources resolve against the plugin's own directory, which + by default lives under plugin-repos/, not a cwd-relative plugins/.""" + + MANIFEST = {"fonts": [{"family": "bundled", "source": "plugin://fonts/Bundled.ttf"}]} + + @staticmethod + def _plugin_with_font(root, name="my-plugin"): + plugin_dir = root / name + (plugin_dir / "fonts").mkdir(parents=True) + (plugin_dir / "manifest.json").write_text(json.dumps({"id": "my-plugin"})) + shutil.copy(resolve_asset_path("assets/fonts/PressStart2P-Regular.ttf"), + plugin_dir / "fonts" / "Bundled.ttf") + return plugin_dir + + def test_font_resolves_under_the_given_plugin_dir(self, fm, tmp_path): + plugin_dir = self._plugin_with_font(tmp_path / "plugin-repos") + + assert fm.register_plugin_fonts("my-plugin", self.MANIFEST, plugin_dir=plugin_dir) + + assert fm.font_catalog["my-plugin::bundled"] == str(plugin_dir / "fonts" / "Bundled.ttf") + font = fm.resolve_font("x.y", "bundled", 8, plugin_id="my-plugin") + assert isinstance(font, ImageFont.FreeTypeFont) + + def test_without_a_plugin_dir_the_configured_directory_is_searched(self, tmp_path): + plugins_root = tmp_path / "installed" + plugin_dir = self._plugin_with_font(plugins_root, name="ledmatrix-my-plugin") + fm = FontManager({"plugin_system": {"plugins_directory": str(plugins_root)}}) + + assert fm.register_plugin_fonts("my-plugin", self.MANIFEST) + + assert fm.font_catalog["my-plugin::bundled"] == str(plugin_dir / "fonts" / "Bundled.ttf") diff --git a/test/test_font_pixel_grid.py b/test/test_font_pixel_grid.py index 3889d91a..2ff72c0f 100644 --- a/test/test_font_pixel_grid.py +++ b/test/test_font_pixel_grid.py @@ -133,6 +133,26 @@ class TestAssetPathsIgnoreTheWorkingDirectory: rel = f"assets/fonts/{FOUR_BY_SIX}" assert FontManager._resolve_asset_path(rel) == resolve_asset_path(rel) + def test_font_overrides_file_lives_in_the_install_config(self, tmp_path, monkeypatch): + from src.font_manager import FontManager + monkeypatch.chdir(tmp_path) + fm = FontManager({}) + assert fm.font_overrides_file == str(PROJECT_ROOT / "config" / "font_overrides.json") + + def test_logo_placeholder_draws_with_the_bundled_font(self, tmp_path, monkeypatch): + import src.logo_downloader as logo_downloader + from src.logo_downloader import LogoDownloader + loaded = [] + + def spy(font, size, **kwargs): + loaded.append(font) + return load_truetype(font, size, **kwargs) + + monkeypatch.chdir(tmp_path) + monkeypatch.setattr(logo_downloader, "load_truetype", spy) + assert LogoDownloader().create_placeholder_logo("AB", str(tmp_path)) + assert loaded == [str(PROJECT_ROOT / "assets" / "fonts" / PRESS_START)] + class TestTheHarnessForkAgreesWithTheCore: """The divergence that let the wrong rendering be blessed as golden. diff --git a/test/test_http_headers.py b/test/test_http_headers.py index bc5836a1..bc4f55c6 100644 --- a/test/test_http_headers.py +++ b/test/test_http_headers.py @@ -78,3 +78,25 @@ class TestBackgroundDataServiceHeaders: assert 'yourusername' not in str(headers) finally: service.shutdown(wait=False) + + +class TestResolverHeaders: + def test_dynamic_team_resolver_sends_the_user_agent(self): + from unittest.mock import patch + from src.dynamic_team_resolver import DynamicTeamResolver + DynamicTeamResolver._rankings_cache = {} + DynamicTeamResolver._cache_timestamp = 0 + try: + with patch('src.dynamic_team_resolver.requests.get', + side_effect=RuntimeError("stop")) as get: + DynamicTeamResolver().resolve_teams(["AP_TOP_5"]) + assert get.call_args.kwargs['headers']['User-Agent'] == USER_AGENT + finally: + DynamicTeamResolver._rankings_cache = {} + DynamicTeamResolver._cache_timestamp = 0 + + def test_odds_manager_uses_the_shared_headers(self): + from src.base_odds_manager import BaseOddsManager + headers = BaseOddsManager(MagicMock()).session.headers + for name, value in DEFAULT_HTTP_HEADERS.items(): + assert headers[name] == value diff --git a/test/test_logo_downloader.py b/test/test_logo_downloader.py index a303b087..581997bd 100644 --- a/test/test_logo_downloader.py +++ b/test/test_logo_downloader.py @@ -357,6 +357,33 @@ class TestRefreshPlaceholderTimestamp: assert refresh_placeholder_timestamp(tmp_path / "nope.png") is False +class TestFailurePaths: + def test_a_team_without_logos_is_a_failed_download(self, tmp_path): + downloader = LogoDownloader() + with patch.object(downloader, "fetch_single_team", + return_value={"team": {"logos": []}}): + assert downloader.download_missing_logo_for_team( + "nfl", "1", "XYZ", tmp_path / "XYZ.png") is False + + def test_placeholder_is_written_where_the_caller_looks(self, tmp_path): + """A path that is not .png (the plugin's own + file naming, or an abbreviation normalize_abbreviation rewrites) + still ends up holding the placeholder, so True means it exists.""" + logo_path = tmp_path / "TA&M.png" + with patch.object(LogoDownloader, "download_logo", return_value=False): + assert download_missing_logo( + "ncaa_fb", "245", "TA&M", logo_path, + logo_url="http://example/tamu.png") is True + assert is_placeholder_logo(logo_path) + assert not (tmp_path / "TAANDM.png").exists() + + def test_placeholder_uses_the_placeholder_geometry(self, tmp_path): + assert LogoDownloader().create_placeholder_logo("AB", str(tmp_path)) + with Image.open(tmp_path / "AB.png") as img: + assert img.size == PLACEHOLDER_SIZE + assert img.convert("RGBA").getpixel((0, 0)) == PLACEHOLDER_BG + + # --------------------------------------------------------------------------- # download_logo: the download the scoreboard plugins actually use # diff --git a/test/test_plugin_loading_failures.py b/test/test_plugin_loading_failures.py index 0bb3e0d7..8035ac3a 100644 --- a/test/test_plugin_loading_failures.py +++ b/test/test_plugin_loading_failures.py @@ -294,6 +294,31 @@ class TestValidateConfigFailure: assert result is False +class TestPluginFontRegistration: + """A manifest's fonts block is registered against the directory the + plugin was loaded from, which plugin:// font sources are relative to.""" + + def test_plugin_dir_is_passed_to_the_font_manager(self, temp_plugin_dir, mock_managers): + plugin_dir = temp_plugin_dir / "test-plugin" + plugin_dir.mkdir() + fonts = {"fonts": [{"family": "f", "source": "plugin://f.ttf"}]} + manifest = {"id": "test-plugin", "name": "Test Plugin", + "entry_point": "manager.py", "class_name": "TestPlugin", + "fonts": fonts} + + with patch('src.common.permission_utils.ensure_directory_permissions'): + manager = PluginManager(plugins_dir=str(temp_plugin_dir), **mock_managers) + manager.plugin_manifests["test-plugin"] = manifest + with patch.object(manager.plugin_loader, 'load_plugin', + return_value=(MagicMock(), MagicMock())): + with patch.object(manager.plugin_loader, 'find_plugin_directory', + return_value=plugin_dir): + manager.load_plugin("test-plugin") + + mock_managers["font_manager"].register_plugin_fonts.assert_called_once_with( + "test-plugin", fonts, plugin_dir=plugin_dir) + + class TestPluginStateOnFailure: """Test that plugin state is correctly set on various failures.""" diff --git a/test/test_startup_validator.py b/test/test_startup_validator.py index 43cc2f38..2bda887f 100644 --- a/test/test_startup_validator.py +++ b/test/test_startup_validator.py @@ -63,6 +63,15 @@ class TestValidateConfig: assert "Missing required configuration key: display" in errors assert "Missing required configuration key: timezone" in errors + @pytest.mark.parametrize("config,expected", [ + ({'timezone': 'UTC'}, "Missing required configuration key: display"), + ({'display': {}, 'timezone': 'UTC'}, "Display configuration is empty"), + ]) + def test_a_missing_display_section_is_reported_once(self, good_cache, config, expected): + validator = StartupValidator(make_config_manager(config)) + _, errors, _ = validator.validate_all() + assert errors == [expected] + def test_config_error_does_not_propagate(self, good_cache): mgr = make_config_manager(GOOD_CONFIG) mgr.load_config.side_effect = ConfigError("bad json") diff --git a/test/test_sync_manager.py b/test/test_sync_manager.py index ced77d4d..da9b52d2 100644 --- a/test/test_sync_manager.py +++ b/test/test_sync_manager.py @@ -38,6 +38,7 @@ import numpy as np import pytest from PIL import Image +from test._api_v3_test_helpers import api_v3_client, api_v3_module # noqa: F401 from src.common import sync_manager from src.common.sync_manager import ( DisplaySyncManager, @@ -878,6 +879,14 @@ class TestWriteStatusFile: mgr.write_status_file() # must not raise assert mgr.logger.debug.called + def test_web_status_endpoint_reads_the_file_that_was_written(self, api_v3_client): + """GET /sync/status reads STATUS_FILE, which lives under + tempfile.gettempdir() -- not always /tmp.""" + mgr = make_manager(role=SyncRole.LEADER) + mgr.write_status_file() + response = api_v3_client.get("/api/v3/sync/status") + assert response.get_json()["data"]["role"] == "leader" + class TestStop: def _stub_with_sockets(self): diff --git a/test/test_text_helper.py b/test/test_text_helper.py index 1e143ddf..0a585e42 100644 --- a/test/test_text_helper.py +++ b/test/test_text_helper.py @@ -24,10 +24,13 @@ class TestTextHelper: assert th.font_dir == tmp_path assert th._font_cache == {} - def test_init_default_font_dir(self): - """Test TextHelper initialization with default font directory.""" + def test_init_default_font_dir(self, tmp_path, monkeypatch): + """The default is the install's assets/fonts, not a cwd-relative path.""" + from pathlib import Path + monkeypatch.chdir(tmp_path) th = TextHelper() - assert th.font_dir == pytest.importorskip("pathlib").Path("assets/fonts") + assert th.font_dir == Path(__file__).resolve().parents[1] / "assets" / "fonts" + assert isinstance(th.load_fonts()["score"], ImageFont.FreeTypeFont) @patch('PIL.ImageFont.truetype') @patch('PIL.ImageFont.load_default') @@ -123,6 +126,17 @@ class TestTextHelper: def test_get_default_font_config(self, text_helper): """Test getting default font configuration.""" config = text_helper._get_default_font_config() - + assert isinstance(config, dict) assert len(config) > 0 + + def test_each_font_file_and_size_is_loaded_once(self): + th = TextHelper() + first = th.load_fonts() + second = th.load_fonts() + # Six names, three (file, size) pairs: PressStart2P at 10 and 8, 4x6 at 6. + assert first["score"] is second["score"] is first["rank"] + assert th.get_font_cache_stats()["cached_fonts"] == 3 + th.clear_font_cache() + assert th.get_font_cache_stats()["cached_fonts"] == 0 + assert th.load_fonts()["score"] is not first["score"] diff --git a/test/test_wifi_manager_ap.py b/test/test_wifi_manager_ap.py index 82e75625..fbcee733 100644 --- a/test/test_wifi_manager_ap.py +++ b/test/test_wifi_manager_ap.py @@ -369,6 +369,26 @@ def test_default_config_has_no_saved_networks(tmp_path: Path) -> None: assert "saved_networks" not in json.loads(config_path.read_text()) +@pytest.mark.unit +def test_save_config_reports_a_failed_write(manager: WiFiManager, tmp_path: Path) -> None: + # A directory where the file should be: every write to it fails, as one + # to a root-owned wifi_config.json does for the web user. + blocked = tmp_path / "blocked.json" + blocked.mkdir() + manager.config_path = blocked + + assert manager._save_config() is False + assert list(tmp_path.glob(".blocked.json.tmp.*")) == [] + + +@pytest.mark.unit +def test_save_config_round_trips(manager: WiFiManager) -> None: + manager.config["auto_enable_ap_mode"] = False + + assert manager._save_config() is True + assert json.loads(manager.config_path.read_text())["auto_enable_ap_mode"] is False + + @pytest.mark.unit def test_connecting_does_not_store_the_password(manager: WiFiManager) -> None: commands = [] @@ -389,3 +409,77 @@ def test_connecting_does_not_store_the_password(manager: WiFiManager) -> None: "the new-connection path was not reached" assert "hunter22" not in json.dumps(manager.config) assert "hunter22" not in manager.config_path.read_text() + + +# --------------------------------------------------------------------------- +# 7. Disconnect takes the saved profile down +# --------------------------------------------------------------------------- + +def _profile_nmcli(profiles: dict): + """A fake subprocess.run for `nmcli connection show` over ``profiles`` + (profile name -> SSID), recording every command it is given.""" + commands = [] + + def fake_run(cmd, *args, **kwargs): + commands.append(cmd) + if cmd == ["nmcli", "-t", "-f", "NAME,TYPE", "connection", "show"]: + lines = [name.replace(":", "\\:") + ":802-11-wireless" for name in profiles] + lines.append("Wired connection 1:802-3-ethernet") + return _ok(stdout="\n".join(lines) + "\n") + if cmd[:4] == ["nmcli", "-g", "802-11-wireless.ssid", "connection"]: + return _ok(stdout=profiles.get(cmd[-1], "") + "\n") + if cmd[:3] == ["nmcli", "connection", "show"]: + return _ok() if cmd[3] in profiles else _fail() + # nmcli rejects 802-11-wireless.ssid as a `connection show -f` column. + if "802-11-wireless.ssid" in cmd: + return _fail(stderr="Error: invalid field '802-11-wireless.ssid'") + return _ok() + + return fake_run, commands + + +@pytest.mark.unit +def test_find_profile_for_ssid_matches_by_ssid_not_name(manager: WiFiManager) -> None: + fake_run, _ = _profile_nmcli({"home: upstairs": "HomeNet", "Office": "OfficeNet"}) + with patch("src.wifi_manager.subprocess.run", side_effect=fake_run): + assert manager._find_profile_for_ssid("HomeNet") == "home: upstairs" + assert manager._find_profile_for_ssid("OfficeNet") == "Office" + assert manager._find_profile_for_ssid("Elsewhere") is None + + +@pytest.mark.unit +def test_disconnect_takes_the_profile_down(manager: WiFiManager) -> None: + from src.wifi_manager import WiFiStatus + + fake_run, commands = _profile_nmcli({"Home profile": "HomeNet"}) + with patch("src.wifi_manager.subprocess.run", side_effect=fake_run), \ + patch("src.wifi_manager.time.sleep"), \ + patch.object(manager, "get_wifi_status", + return_value=WiFiStatus(connected=True, ssid="HomeNet")): + ok, _ = manager.disconnect_from_network(skip_ap_check=True) + + assert ok + assert ["nmcli", "connection", "down", "Home profile"] in commands + assert ["nmcli", "device", "disconnect", "wlan0"] in commands + + +# --------------------------------------------------------------------------- +# 8. The nmcli Wi-Fi list parser both scan paths share +# --------------------------------------------------------------------------- + +@pytest.mark.unit +def test_nmcli_wifi_list_parsing() -> None: + out = ( + "HomeNet:40:WPA2:2437 MHz\n" + "Cafe:80::5180 MHz\n" + "HomeNet:90:WPA2:5180 MHz\n" # duplicate SSID: first line wins + ":70:WPA2:2412 MHz\n" # hidden network + "Broken:notanumber:WPA2:2412 MHz\n" + "Modern:60:WPA3 SAE:5745 MHz\n" + ) + networks = WiFiManager._parse_nmcli_wifi_list(out) + assert [(n.ssid, n.signal, n.security, n.frequency) for n in networks] == [ + ("Cafe", 80, "open", 5180.0), + ("Modern", 60, "wpa3", 5745.0), + ("HomeNet", 40, "wpa2", 2437.0), + ] diff --git a/web_interface/blueprints/api_v3/misc.py b/web_interface/blueprints/api_v3/misc.py index 0f50f490..0f4df30e 100644 --- a/web_interface/blueprints/api_v3/misc.py +++ b/web_interface/blueprints/api_v3/misc.py @@ -14,6 +14,7 @@ from web_interface.blueprints.api_v3 import ( subprocess, success_response, tempfile, ) from src.common.path_safety import safe_path_component +from src.common import sync_manager as _sync from src import error_aggregator as _errors import web_interface.blueprints.api_v3 as _pkg # Read through the module rather than bound by value: tests patch these @@ -190,21 +191,21 @@ def get_logs(): @api_v3.route('/sync/status', methods=['GET']) def get_sync_status(): """Return live multi-display sync status written by the display process.""" - import os as _os - status_file = "/tmp/led_matrix_sync_status.json" + # The display process writes this file; read it where it is written. + status_file = _sync.STATUS_FILE # Also surface config so the UI can show the configured role even before # the display process has written a status file. cfg_role = "standalone" - cfg_port = 5765 + cfg_port = _sync.SYNC_PORT if api_v3.config_manager: try: cfg = api_v3.config_manager.load_config().get("sync", {}) cfg_role = cfg.get("role", "standalone") - cfg_port = int(cfg.get("port", 5765)) + cfg_port = int(cfg.get("port", _sync.SYNC_PORT)) except Exception: pass - if _os.path.exists(status_file): + if os.path.exists(status_file): try: with open(status_file) as f: live = json.load(f) diff --git a/web_interface/blueprints/api_v3/wifi.py b/web_interface/blueprints/api_v3/wifi.py index 0f03a878..7700ea82 100644 --- a/web_interface/blueprints/api_v3/wifi.py +++ b/web_interface/blueprints/api_v3/wifi.py @@ -372,7 +372,12 @@ def set_auto_enable_ap_mode(): wifi_manager = WiFiManager() wifi_manager.config["auto_enable_ap_mode"] = auto_enable - wifi_manager._save_config() + if not wifi_manager._save_config(): + return jsonify({ + 'status': 'error', + 'message': (f'Could not save the setting to {wifi_manager.config_path}; ' + 'check that the web interface user can write it.'), + }), 500 return jsonify({ 'status': 'success',