diff --git a/BlocksScreen/BlocksScreen.py b/BlocksScreen/BlocksScreen.py index 9cc399e7..0b425d28 100644 --- a/BlocksScreen/BlocksScreen.py +++ b/BlocksScreen/BlocksScreen.py @@ -1,5 +1,6 @@ import logging import os +import shlex import shutil import signal import socket @@ -181,6 +182,30 @@ def initialize_conf_manager() -> None: ) +def _seed_variables(main_window: MainWindow, manager: DeviceConfigManager) -> None: + """SAVE_VARIABLE repo keys Klipper lacks: a file append is lost on its next save.""" + defaults = manager.variable_defaults() + sent: set[str] = set() + + def _on_update(status: dict) -> None: + live = status.get("variables") + if live is None: + return + for key in defaults.keys() - live.keys() - sent: + sent.add(key) + logging.info("Seeding Klipper variable %s = %s", key, defaults[key]) + main_window.run_gcode_signal.emit( + f"SAVE_VARIABLE VARIABLE={key} VALUE={shlex.quote(defaults[key])}" + ) + + def _on_state(_state: str) -> None: + # Emits are change-only and a fast restart emits just "ready": clear on any. + sent.clear() + + main_window.printer.save_variables_update.connect(_on_update) + main_window.ws.klippy_state_signal.connect(_on_state) + + def _sd_notify(msg: str) -> None: sock_path = os.environ.get("NOTIFY_SOCKET", "") if not sock_path: @@ -321,6 +346,7 @@ def eventFilter(self, a0: QtCore.QObject | None, a1: QtCore.QEvent | None) -> bo # Klipper may have started before the boot sync rewrote # printer.cfg; restarted once Moonraker is up and no job runs. main_window.request_klipper_restart() + _seed_variables(main_window, conf_man) # Live side of the device templates: printer.cfg follows devices # plugged in while running (after the user's OK for templates that # ask for it), and Klipper is restarted to load it. diff --git a/BlocksScreen/lib/network/worker.py b/BlocksScreen/lib/network/worker.py index aa995a69..351e4ba0 100644 --- a/BlocksScreen/lib/network/worker.py +++ b/BlocksScreen/lib/network/worker.py @@ -360,6 +360,8 @@ async def _async_initialize(self) -> None: return self._running = True + # The UI has no networking toggle, so `nmcli networking off` would lock the user out. + await self._ensure_networking_enabled() await self._detect_interfaces() if await self._is_ethernet_connected(): diff --git a/BlocksScreen/logger.py b/BlocksScreen/logger.py index 7b3b7d50..aedfa4f3 100644 --- a/BlocksScreen/logger.py +++ b/BlocksScreen/logger.py @@ -349,7 +349,12 @@ def _install(self) -> None: """Install exception hooks.""" # Setup faulthandler for C-level crashes (segfaults, etc.) try: - self._fault_file = open(self._fault_log_path, "w") + # Append: the systemd restart right after a SEGV would otherwise wipe its trace + self._fault_file = open(self._fault_log_path, "a") + self._fault_file.write( + f"--- run started {datetime.now().isoformat()} pid {os.getpid()} ---\n" + ) + self._fault_file.flush() faulthandler.enable(file=self._fault_file, all_threads=True) # Also dump traceback on SIGUSR1 (useful for debugging hangs) @@ -558,11 +563,7 @@ def uninstall(self) -> None: if self._original_threading_excepthook and hasattr(threading, "excepthook"): threading.excepthook = self._original_threading_excepthook - if self._fault_file: - try: - self._fault_file.close() - except OSError: - pass # File already closed; nothing to recover + # Kept open: faulthandler writes teardown SEGVs here; a closed fd gets reused. CrashHandler._installed = False CrashHandler._instance = None diff --git a/BlocksScreen/tools/configuration_manager/configurator.py b/BlocksScreen/tools/configuration_manager/configurator.py index 410a1a89..c47b679a 100644 --- a/BlocksScreen/tools/configuration_manager/configurator.py +++ b/BlocksScreen/tools/configuration_manager/configurator.py @@ -1,4 +1,5 @@ #!/usr/bin/python3 +import ast import configparser import hashlib import logging @@ -238,6 +239,8 @@ def __init__(self, config) -> None: self.config_fi_name, self.config_fi_relpath = self._build_file_index( self.config_dir ) + # Set by sync(): a Klipper that refused the old variables.cfg must restart. + self.variables_repaired = False self.sync() @@ -658,9 +661,13 @@ def _cpy_cfg_files(self) -> None: # Files are different so they need merging ok = False + if f == "variables.cfg": + # Klipper rewrites it from memory: new keys go in via SAVE_VARIABLE + self.variables_repaired = self._repair_variables(target) + continue if f == "printer.cfg": ok = self.merge_cfg(src_file, target, marker=SV_CONFIG_MARKER) - elif f in ["variables.cfg", "BlocksScreen.cfg"]: + elif f == "BlocksScreen.cfg": ok = self.merge_cfg(src_file, target) else: ok = copy_file_simple(src_file, target) @@ -671,6 +678,70 @@ def _cpy_cfg_files(self) -> None: except Exception as e: _logger.info("Caught exception while cpy files: %s" % e) + def _repair_variables(self, target: pathlib.Path) -> bool: + """Rewrite a variables.cfg save_variables cannot load, in its own format.""" + strict = configparser.ConfigParser() + try: + # Same checks as save_variables.loadVariables, which fails Klippy's config + strict.read(target, encoding="utf-8") + if strict.has_section("Variables"): + for _, raw in strict.items("Variables"): + ast.literal_eval(raw) + return False + except (configparser.Error, ValueError, SyntaxError, TypeError) as e: + _logger.warning("variables.cfg unloadable by Klipper, repairing: %s", e) + loose = configparser.ConfigParser(strict=False, interpolation=None) + try: + loose.read(target, encoding="utf-8") + except (configparser.Error, ValueError) as e: + _logger.error("variables.cfg beyond repair, left as is: %s", e) + return False + keep = {} + for key, raw in ( + loose.items("Variables") if loose.has_section("Variables") else [] + ): + try: + # Loose read skips interpolation: '%%' is Klipper's escaped '%', kept raw + if "%" in raw.replace("%%", ""): + raise ValueError("'%' breaks save_variables' parser") + ast.literal_eval(raw.replace("%%", "%")) + except (ValueError, SyntaxError, TypeError) as e: + _logger.warning("Dropping variables.cfg %s = %r: %s", key, raw, e) + continue + keep[key] = raw + backup = self.backup_dir / f"variables.cfg.{_timestamp()}" + shutil.copy2(target, backup) + lines = [f"{k} = {v}\n" for k, v in sorted(keep.items())] + atomic_write_text(target, "[Variables]\n" + "".join(lines) + "\n") + _logger.info("Repaired variables.cfg, original kept at %s", backup) + return True + + def variable_defaults(self) -> dict[str, str]: + """Repo variables.cfg keys Klipper's SAVE_VARIABLE can take, as raw literals.""" + src_files = self.repo_fi_name.get("variables.cfg", []) + if not src_files: + return {} + cfg = configparser.ConfigParser(interpolation=None) + try: + cfg.read(min(src_files, key=lambda p: len(p.parents)), encoding="utf-8") + except configparser.Error as e: + _logger.error("Unreadable repo variables.cfg: %s", e) + return {} + if not cfg.has_section("Variables"): + return {} + defaults = {} + for key, raw in cfg.items("Variables"): + # HH owns mmu_*; '%' breaks save_variables' parser, ';' is a gcode comment + if key.startswith("mmu_") or "%" in raw or ";" in raw: + continue + try: + ast.literal_eval(raw) + except (ValueError, SyntaxError): + _logger.warning("variables.cfg %s is not a literal: %r", key, raw) + continue + defaults[key] = raw + return defaults + def cmp_file(self, src, target) -> bool: src = pathlib.Path(src) target = pathlib.Path(target) diff --git a/BlocksScreen/tools/configuration_manager/device_configurator.py b/BlocksScreen/tools/configuration_manager/device_configurator.py index c0c53492..d326710f 100644 --- a/BlocksScreen/tools/configuration_manager/device_configurator.py +++ b/BlocksScreen/tools/configuration_manager/device_configurator.py @@ -99,8 +99,8 @@ def __init__( self._consent = consent if consent is not None else DeviceConsent() self._devices: list[Device] = [] self._merging: str | None = None - # Set by sync(): printer.cfg differs from what it was before this - # boot's sync, so a Klipper that already loaded it must restart. + # Set by sync(): printer.cfg changed or variables.cfg was repaired during + # this boot's sync, so a Klipper that already loaded them must restart. self.boot_changed = False _logger.info( "Device profiles: %s", ", ".join(p.name for p in self._profiles) or "none" @@ -165,6 +165,8 @@ def sync(self) -> None: self.boot_changed = self._read_printer_cfg() != before if self.boot_changed: _logger.info("printer.cfg changed during boot sync") + if self.variables_repaired: + self.boot_changed = True def merge_cfg(self, src_file, target_file, marker="") -> bool: """ConfigManager.merge_cfg, remembering which file is being merged""" diff --git a/tests/network/test_worker_unit.py b/tests/network/test_worker_unit.py index ff4e4ad5..f87f2e9d 100644 --- a/tests/network/test_worker_unit.py +++ b/tests/network/test_worker_unit.py @@ -3145,7 +3145,11 @@ async def _test(): class TestAsyncInitializeFull: def test_happy_path_full_init(self, qapp): w = _make(qapp, running=False) - w._detect_interfaces = AsyncMock() + order: list[str] = [] + w._ensure_networking_enabled = AsyncMock( + side_effect=lambda: order.append("enable") or True + ) + w._detect_interfaces = AsyncMock(side_effect=lambda: order.append("detect")) w._ensure_wired_autoconnect = AsyncMock() w._is_ethernet_connected = AsyncMock(return_value=False) w._activate_saved_vlans = AsyncMock() @@ -3162,7 +3166,11 @@ def test_happy_path_full_init(self, qapp): _run(w._async_initialize()) assert w._running is True + # Boot clears `nmcli networking off`, not just the Wi-Fi preflight. + w._ensure_networking_enabled.assert_awaited_once() w._detect_interfaces.assert_awaited_once() + # Detection against a disabled NM finds no devices, so enabling must come first. + assert order == ["enable", "detect"] # Boot must not re-arm: NM's latch is how "ethernet off" survives a reboot. w._ensure_wired_autoconnect.assert_not_awaited() w._start_signal_listeners.assert_awaited_once() diff --git a/tests/util/test_configurator_unit.py b/tests/util/test_configurator_unit.py index 5e8d523d..64d7284c 100644 --- a/tests/util/test_configurator_unit.py +++ b/tests/util/test_configurator_unit.py @@ -890,17 +890,106 @@ def test_printer_cfg_differs_merge_called(self, tmp_path): assert SV_CONFIG_MARKER in merged assert "#*# user: saved" in merged - def test_variables_cfg_differs_merge_called(self, tmp_path): + def test_existing_variables_cfg_left_to_klipper(self, tmp_path): mgr = _make_manager(tmp_path, cpy_files=["variables.cfg"]) src = _write(mgr.repo / "variables.cfg", "[g]\nopt_a: repo\nopt_b: repo\n") tgt = _write(mgr.config_dir / "variables.cfg", "[g]\nopt_a: existing\n") mgr.repo_fi_name = {"variables.cfg": [src]} mgr.config_fi_name = {"variables.cfg": [tgt]} mgr._cpy_cfg_files() - result = configparser.ConfigParser() - result.read_string(tgt.read_text()) - assert result.get("g", "opt_a") == "existing" - assert result.get("g", "opt_b") == "repo" + # Klipper's next SAVE_VARIABLE would drop an append; the seeder adds keys instead + assert tgt.read_text() == "[g]\nopt_a: existing\n" + + def test_unloadable_variables_cfg_repaired(self, tmp_path): + mgr = _make_manager(tmp_path, cpy_files=["variables.cfg"]) + src = _write(mgr.repo / "variables.cfg", "[Variables]\na = 1\n") + tgt = _write( + mgr.config_dir / "variables.cfg", + "[Variables]\nb = 2\na = 3\n[Variables]\nc = 'x'\n", + ) + mgr.repo_fi_name = {"variables.cfg": [src]} + mgr.config_fi_name = {"variables.cfg": [tgt]} + mgr._cpy_cfg_files() + assert tgt.read_text() == "[Variables]\na = 3\nb = 2\nc = 'x'\n\n" + + +class TestRepairVariables: + BROKEN = "[Variables]\nb = 2\n[Variables]\nb = 5\npct = '50%'\nbare = PLA\n" + + def test_loadable_file_untouched(self, tmp_path): + mgr = _make_manager(tmp_path) + text = "[Variables]\nb = 2\nok = '50%%'\n" + tgt = _write(mgr.config_dir / "variables.cfg", text) + assert mgr._repair_variables(tgt) is False + assert tgt.read_text() == text + assert not list(mgr.backup_dir.iterdir()) + + def test_rewrites_as_klipper_loads_it(self, tmp_path): + mgr = _make_manager(tmp_path) + tgt = _write(mgr.config_dir / "variables.cfg", self.BROKEN) + assert mgr._repair_variables(tgt) is True + # Later duplicate wins; values Klipper's parser rejects are dropped + assert tgt.read_text() == "[Variables]\nb = 5\n\n" + assert mgr._repair_variables(tgt) is False + + def test_original_backed_up(self, tmp_path): + mgr = _make_manager(tmp_path) + tgt = _write(mgr.config_dir / "variables.cfg", self.BROKEN) + mgr._repair_variables(tgt) + (backup,) = mgr.backup_dir.iterdir() + assert backup.read_text() == self.BROKEN + + def test_unparseable_file_left_as_is(self, tmp_path): + mgr = _make_manager(tmp_path) + tgt = _write(mgr.config_dir / "variables.cfg", "a = 1\n") + assert mgr._repair_variables(tgt) is False + assert tgt.read_text() == "a = 1\n" + + def test_escaped_percent_kept(self, tmp_path): + mgr = _make_manager(tmp_path) + text = "[Variables]\nbare = PLA\nok = '50%%'\n" + tgt = _write(mgr.config_dir / "variables.cfg", text) + assert mgr._repair_variables(tgt) is True + assert tgt.read_text() == "[Variables]\nok = '50%%'\n\n" + assert mgr._repair_variables(tgt) is False + + def test_failed_repair_keeps_original(self, tmp_path, monkeypatch): + mgr = _make_manager(tmp_path) + tgt = _write(mgr.config_dir / "variables.cfg", self.BROKEN) + + def power_cut(*_): + raise OSError("power cut") + + monkeypatch.setattr(os, "replace", power_cut) + with pytest.raises(OSError): + mgr._repair_variables(tgt) + assert tgt.read_text() == self.BROKEN + assert not list(mgr.config_dir.glob(".variables.cfg.*")) + + +class TestVariableDefaults: + def _mgr(self, tmp_path, text: str) -> ConfigManager: + mgr = _make_manager(tmp_path) + mgr.repo_fi_name = {"variables.cfg": [_write(mgr.repo / "variables.cfg", text)]} + return mgr + + def test_returns_literal_keys(self, tmp_path): + mgr = self._mgr(tmp_path, "[Variables]\netemp=0\nprintcore_type: 'Standard'\n") + assert mgr.variable_defaults() == {"etemp": "0", "printcore_type": "'Standard'"} + + def test_skips_hh_owned_and_unsendable_values(self, tmp_path): + mgr = self._mgr( + tmp_path, + "[Variables]\nmmu_state_gate_selected = 0\npct = '50%'\n" + "semi = 'a;b'\nbare = PLA\nok = True\n", + ) + assert mgr.variable_defaults() == {"ok": "True"} + + def test_no_repo_file_or_section(self, tmp_path): + mgr = _make_manager(tmp_path) + mgr.repo_fi_name = {} + assert mgr.variable_defaults() == {} + assert self._mgr(tmp_path, "[Other]\na = 1\n").variable_defaults() == {} def test_seed_only_file_kept_when_present(self, tmp_path): mgr = _make_manager(tmp_path, cpy_files=["moonraker.conf"]) diff --git a/tests/util/test_device_configurator_unit.py b/tests/util/test_device_configurator_unit.py index fb0377fe..ed3de69b 100644 --- a/tests/util/test_device_configurator_unit.py +++ b/tests/util/test_device_configurator_unit.py @@ -237,6 +237,14 @@ def test_amu_removed_while_off(self, dirs, monkeypatch): _boot(dirs, monkeypatch, [AMU]) assert _boot(dirs, monkeypatch, []).boot_changed is True + def test_variables_cfg_repaired(self, dirs, monkeypatch): + _, config_dir, _ = dirs + _boot(dirs, monkeypatch, [AMU]) + # Klipper already refused this file, only a restart loads the repair + (config_dir / "variables.cfg").write_text("[Variables]\na = 1\nbare = PLA\n") + assert _boot(dirs, monkeypatch, [AMU]).boot_changed is True + assert _boot(dirs, monkeypatch, [AMU]).boot_changed is False + class TestScanDevices: def test_by_id_listing_fills_in_what_the_daemon_missed(self, monkeypatch): diff --git a/tests/util/test_fault_log_unit.py b/tests/util/test_fault_log_unit.py new file mode 100644 index 00000000..23d90d5c --- /dev/null +++ b/tests/util/test_fault_log_unit.py @@ -0,0 +1,47 @@ +"""CrashHandler fault log: a SEGV after shutdown still lands in the fault log.""" + +import subprocess +import sys +import textwrap +from pathlib import Path + +_REPO = Path(__file__).resolve().parents[2] + +_CHILD = textwrap.dedent( + """ + import faulthandler, sys + from BlocksScreen.logger import CrashHandler + fault, other = sys.argv[1], sys.argv[2] + CrashHandler.install(crash_log_path=fault + ".crash", fault_log_path=fault) + CrashHandler._instance.uninstall() + with open(other, "w") as f: + f.write("unrelated\\n") + f.flush() + faulthandler._sigsegv() + """ +) + + +def _crash_after_uninstall(tmp_path: Path) -> tuple[str, str]: + fault, other = tmp_path / "fault.log", tmp_path / "other.log" + fault.write_text("previous run\n") + proc = subprocess.run( + [sys.executable, "-c", _CHILD, str(fault), str(other)], + cwd=_REPO, + capture_output=True, + timeout=20, + check=False, + ) + assert proc.returncode != 0 + return fault.read_text(), other.read_text() + + +def test_segv_after_uninstall_goes_to_fault_log(tmp_path): + fault, other = _crash_after_uninstall(tmp_path) + assert "Segmentation fault" in fault + assert "Segmentation fault" not in other + + +def test_fault_log_keeps_previous_runs_and_marks_each_start(tmp_path): + fault, _ = _crash_after_uninstall(tmp_path) + assert fault.startswith("previous run\n--- run started ") diff --git a/tests/widgets/test_seed_variables.py b/tests/widgets/test_seed_variables.py new file mode 100644 index 00000000..b9662675 --- /dev/null +++ b/tests/widgets/test_seed_variables.py @@ -0,0 +1,74 @@ +"""_seed_variables: repo variables.cfg keys reach Klipper via SAVE_VARIABLE, once each.""" + +import ast +import shlex +from unittest.mock import MagicMock + +import pytest + +from .test_sigterm_shutdown import _load_entry_module + + +@pytest.fixture(scope="module") +def entry_mod(): + return _load_entry_module() + + +def _window(entry_mod, defaults: dict[str, str]) -> MagicMock: + window = MagicMock() + manager = MagicMock() + manager.variable_defaults.return_value = defaults + entry_mod._seed_variables(window, manager) + return window + + +def _seed(entry_mod, defaults: dict[str, str]): + window = _window(entry_mod, defaults) + on_update = window.printer.save_variables_update.connect.call_args.args[0] + return on_update, window.run_gcode_signal.emit + + +def test_only_missing_keys_are_saved(entry_mod): + on_update, emit = _seed(entry_mod, {"etemp": "0", "printcore_type": "'Standard'"}) + on_update({"variables": {"etemp": 5}}) + emit.assert_called_once_with( + "SAVE_VARIABLE VARIABLE=printcore_type VALUE=" + shlex.quote("'Standard'") + ) + + +def test_value_survives_klipper_param_parsing(entry_mod): + on_update, emit = _seed(entry_mod, {"material_type": "'PLA #1'"}) + on_update({"variables": {}}) + # Klipper splits extended params with posix shlex and '#;' commenters + lex = shlex.shlex(emit.call_args.args[0].split(" ", 1)[1], posix=True) + lex.whitespace_split = True + lex.commenters = "#;" + params = dict(arg.split("=", 1) for arg in lex) + assert ast.literal_eval(params["VALUE"]) == "PLA #1" + + +def test_key_is_sent_once_even_before_klipper_reports_it(entry_mod): + on_update, emit = _seed(entry_mod, {"heating": "False"}) + on_update({"variables": {}}) + on_update({"variables": {"etemp": 0}}) + assert emit.call_count == 1 + + +def test_unsaved_key_is_resent_after_klippy_restart(entry_mod): + window = _window(entry_mod, {"heating": "False"}) + on_update = window.printer.save_variables_update.connect.call_args.args[0] + on_state = window.ws.klippy_state_signal.connect.call_args.args[0] + emit = window.run_gcode_signal.emit + on_update({"variables": {}}) + on_update({"variables": {}}) + assert emit.call_count == 1 + # Fast restart: disconnect resets the stored state, so only "ready" is emitted + on_state("ready") + on_update({"variables": {}}) + assert emit.call_count == 2 + + +def test_update_without_variables_is_ignored(entry_mod): + on_update, emit = _seed(entry_mod, {"heating": "False"}) + on_update({}) + emit.assert_not_called()