From 82def5737fa2077aeff6da842cc096aafa727570 Mon Sep 17 00:00:00 2001 From: Timothy Allen Date: Sat, 10 Oct 2026 09:03:35 +0200 Subject: [PATCH] Select the bmspy-ups device from the configured devices bmspy-ups selected its device from the devices that had data at startup, and exited with status 0 if the selection failed. One failed first read ended the monitor or made it select the wrong device. The server has a new DEVICES command that lists the configured names, and client.list_devices calls it. bmspy-ups selects from that list and then waits for the first data of its device. A wrong or missing --device now exits with status 2. The service file does not restart on that status, because a restart cannot correct it. An older server ends on the unknown DEVICES command, so update the server first. --- bmspy-ups.service | 3 ++ bmspy/client.py | 6 ++++ bmspy/server.py | 4 +++ bmspy/ups.py | 34 ++++++++++++++++--- tests/test_client.py | 6 ++++ tests/test_server.py | 14 ++++++++ tests/test_ups.py | 79 +++++++++++++++++++++++++++++--------------- 7 files changed, 115 insertions(+), 31 deletions(-) diff --git a/bmspy-ups.service b/bmspy-ups.service index 43eeab8..412487d 100644 --- a/bmspy-ups.service +++ b/bmspy-ups.service @@ -5,9 +5,12 @@ Requires=bmspy-server.service [Service] Type=exec WorkingDirectory=/usr/local/bmspy +# Add --device NAME if bmspy-server reads more than one device. ExecStart=uv run --no-sync bmspy-ups RestartSec=5 Restart=on-failure +# Status 2 is a wrong option or device name. A restart cannot fix it. +RestartPreventExitStatus=2 [Install] WantedBy=default.target diff --git a/bmspy/client.py b/bmspy/client.py index 785ab54..bffa3de 100644 --- a/bmspy/client.py +++ b/bmspy/client.py @@ -125,6 +125,12 @@ def socket_comms(socket_path: str, request_data: dict[str, Any], debug: int = 0) return response_data +def list_devices(socket_path: str, client_name: str, debug: int = 0) -> list[str]: + """Return the names of all configured devices, including those without data.""" + data = socket_comms(socket_path, {"command": "DEVICES", "client": client_name}, debug) + return list(data["devices"]) + + def read_data(socket_path: str, client_name: str, ups: str | None = None, debug: int = 0) -> dict[str, UPS]: """Return {ups_name: UPS} for all devices, or just the named one.""" request: dict[str, Any] = {"command": "GET", "client": client_name} diff --git a/bmspy/server.py b/bmspy/server.py index 114772d..da276d2 100755 --- a/bmspy/server.py +++ b/bmspy/server.py @@ -433,6 +433,10 @@ def main(): debug, ) + case "DEVICES": + # Configured names, including devices that have no data yet. + send_response(connection, {"devices": list(ups_devices)}, client, debug) + case "GET": ups_filter = request_data.get("ups") targets = ( diff --git a/bmspy/ups.py b/bmspy/ups.py index 9033bed..05a2ea5 100644 --- a/bmspy/ups.py +++ b/bmspy/ups.py @@ -1,16 +1,20 @@ import argparse import atexit import os +import sys import time import smtplib import ssl import socket from collections import deque +from collections.abc import Collection from bmspy import client from bmspy.classes import UPS, BMSScalarField DAEMON_UPDATE_PERIOD = 30 +STARTUP_RETRY_PERIOD = 5 +USAGE_ERROR_STATUS = 2 scheduled_shutdown = False critical_sent = False @@ -26,9 +30,11 @@ def _get_field_value(device: UPS, field_name: str) -> float | None: return None -def _resolve_ups_device(data: dict[str, UPS], requested: str | None) -> str | None: +def _resolve_ups_device(data: Collection[str], requested: str | None) -> str | None: """Return the device name to monitor, or None and print an error if the selection is invalid. + *data* holds the available device names. A dict keyed by name also works. + Rules: - No devices → error. - requested is set and found → use it. @@ -58,6 +64,25 @@ def _resolve_ups_device(data: dict[str, UPS], requested: str | None) -> str | No return next(iter(data)) +def _wait_for_ups_device(socket_path: str, requested: str | None) -> str | None: + """Select the device to monitor, then wait until the server has data for it. + + The selection uses the configured device names, not the devices that have + data now. A device that misses its first read at startup then cannot change + the selection. A wrong or missing choice is final: it returns None. + """ + name = _resolve_ups_device(client.list_devices(socket_path, "ups"), requested) + if name is None: + return None + waiting = False + while name not in client.read_data(socket_path, "ups", ups=name): + if not waiting: + print("UPS '{}' has no data yet, waiting...".format(name)) + waiting = True + time.sleep(STARTUP_RETRY_PERIOD) + return name + + def handle_shutdown(action: str = "cancel", delay: int = 0, debug: int = 0) -> None: """Schedule or cancel a system shutdown; only schedules once per incident.""" global scheduled_shutdown @@ -224,10 +249,11 @@ def main() -> None: client.handle_registration(args.socket, "ups", debug) atexit.register(client.handle_registration, args.socket, "ups", debug) - initial_data = client.read_data(args.socket, "ups") - device_name = _resolve_ups_device(initial_data, args.device) + device_name = _wait_for_ups_device(args.socket, args.device) if device_name is None: - return + # A wrong or missing --device is a usage error, as in argparse. A retry + # cannot fix it, so the service file does not restart on this status. + sys.exit(USAGE_ERROR_STATUS) history = deque() while True: diff --git a/tests/test_client.py b/tests/test_client.py index e74ea0a..e2e7b4f 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -91,6 +91,12 @@ class TestReadData: call_args = mock_comms.call_args[0][1] assert call_args.get("ups") == "myups" + def test_list_devices_returns_configured_names(self): + from bmspy.client import list_devices + with patch("bmspy.client.socket_comms", return_value={"devices": ["network", "nas"]}) as mock_comms: + assert list_devices("/fake/socket", "test") == ["network", "nas"] + assert mock_comms.call_args[0][1] == {"command": "DEVICES", "client": "test"} + def test_no_ups_filter_not_in_request(self): with patch("bmspy.client.socket_comms") as mock_comms: mock_comms.return_value = {} diff --git a/tests/test_server.py b/tests/test_server.py index ad893f9..cd4efc8 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -445,6 +445,20 @@ class TestServerMain: finally: self._stop_server(sock_path, t) + def test_devices_command_lists_configured_devices(self, tmp_path): + """DEVICES names every configured device, through the real client.""" + from bmspy import client + + sock_path = str(tmp_path / "server_devices.sock") + ready = threading.Event() + t = self._make_server_thread(sock_path, ready, threading.Event()) + t.start() + ready.wait(timeout=2) + try: + assert client.list_devices(sock_path, "test") == ["ttyUSB0"] + finally: + self._stop_server(sock_path, t) + def test_deregister_command(self, tmp_path): """Test DEREGISTER command via server main().""" sock_path = str(tmp_path / "server_dereg.sock") diff --git a/tests/test_ups.py b/tests/test_ups.py index be39147..d7cc0ee 100644 --- a/tests/test_ups.py +++ b/tests/test_ups.py @@ -1,6 +1,6 @@ import pytest import smtplib -from unittest.mock import patch, MagicMock +from unittest.mock import call, patch, MagicMock import bmspy.ups as ups_mod from bmspy.classes import BMSScalarField, BMSMultiField, BMSInfoField, UPS @@ -24,6 +24,13 @@ def _reset_globals(): ups_mod.alert_sent = False +@pytest.fixture(autouse=True) +def _configured_devices(): + """The main() tests use one configured device unless they patch this themselves.""" + with patch("bmspy.ups.client.list_devices", return_value=["testups"]): + yield + + # --------------------------------------------------------------------------- # _get_field_value # --------------------------------------------------------------------------- @@ -569,31 +576,57 @@ class TestUpsMainDeviceSelection: def _make_data(self, name, current_amps=0.0, charge_ratio=0.8): return {name: _make_ups(current_amps, charge_ratio)} - def test_main_no_devices_returns_early(self, capsys): - """main() exits cleanly when no devices are found.""" + def test_main_waits_at_startup_until_the_device_has_data(self, capsys): + """A failed first read must not end the monitor: systemd would not restart it.""" with patch("sys.argv", ["bmspy-ups"]), \ - patch("bmspy.ups.client.read_data", return_value={}), \ - patch("bmspy.ups.client.handle_registration"): - ups_mod.main() - assert "no UPS" in capsys.readouterr().out + patch("bmspy.ups.client.list_devices", return_value=["ups1"]), \ + patch("bmspy.ups.client.read_data", + side_effect=[{}, {}, self._make_data("ups1"), StopIteration("done")]) as read_data, \ + patch("bmspy.ups.client.handle_registration"), \ + patch("bmspy.ups.time.sleep") as mock_sleep: + with pytest.raises(StopIteration): + ups_mod.main() + # Two empty answers, the answer with the device, then the first loop read. + assert [c.kwargs for c in read_data.call_args_list] == [{"ups": "ups1"}] * 4 + assert mock_sleep.call_args_list == [call(ups_mod.STARTUP_RETRY_PERIOD)] * 2 + # The reason is reported once, not on each retry. + assert capsys.readouterr().out.count("UPS 'ups1' has no data yet") == 1 - def test_main_multiple_devices_no_flag_returns_early(self, capsys): - """main() exits with an error when multiple devices exist and --device is not set.""" - two_devices = { - "ups1": _make_ups(0.0, 0.9), - "ups2": _make_ups(0.0, 0.8), - } - with patch("sys.argv", ["bmspy-ups"]), \ - patch("bmspy.ups.client.read_data", return_value=two_devices), \ + def test_main_selects_from_configured_devices_not_from_those_with_data(self): + """One device with data must not be picked when the server has several.""" + with patch("sys.argv", ["bmspy-ups", "--device", "ups2"]), \ + patch("bmspy.ups.client.list_devices", return_value=["ups1", "ups2"]), \ + patch("bmspy.ups.client.read_data", + side_effect=[{}, self._make_data("ups2"), StopIteration("done")]) as read_data, \ + patch("bmspy.ups.client.handle_registration"), \ + patch("bmspy.ups.time.sleep") as mock_sleep: + with pytest.raises(StopIteration): + ups_mod.main() + assert [c.kwargs for c in read_data.call_args_list] == [{"ups": "ups2"}] * 3 + assert mock_sleep.call_count == 1 + + @pytest.mark.parametrize("argv,expected", [ + ([], "multiple UPS devices found, specify one with --device: ups1, ups2"), + (["--device", "ghost"], "UPS 'ghost' not found. Available: ups1, ups2"), + ]) + def test_main_wrong_or_missing_device_option_is_an_error_exit(self, capsys, argv, expected): + """Only ups1 has data, but both are configured: do not guess, and do not wait.""" + with patch("sys.argv", ["bmspy-ups"] + argv), \ + patch("bmspy.ups.client.list_devices", return_value=["ups1", "ups2"]), \ + patch("bmspy.ups.client.read_data", return_value=self._make_data("ups1")) as read_data, \ patch("bmspy.ups.client.handle_registration"): - ups_mod.main() - out = capsys.readouterr().out - assert "ups1" in out or "ups2" in out + with pytest.raises(SystemExit) as exit_info: + ups_mod.main() + # The argparse usage status: systemd reports it and does not restart. + assert exit_info.value.code == ups_mod.USAGE_ERROR_STATUS == 2 + assert expected in capsys.readouterr().out + read_data.assert_not_called() def test_main_ups_flag_selects_device(self): """--device selects the correct device from multiple available ones.""" data_seq = [self._make_data("ups2")] * 6 - with patch("sys.argv", ["bmspy-ups", "--device", "ups2"]): + with patch("sys.argv", ["bmspy-ups", "--device", "ups2"]), \ + patch("bmspy.ups.client.list_devices", return_value=["ups1", "ups2"]): with pytest.raises(StopIteration): with patch("bmspy.ups.client.read_data", side_effect=[{"ups1": _make_ups(0.0, 0.9), "ups2": _make_ups(0.0, 0.8)}] @@ -619,14 +652,6 @@ class TestUpsMainDeviceSelection: ups_mod.main() assert read_data.call_args_list[1] == (("/tmp/test.sock", "ups"), {"ups": "testups"}) - def test_main_ups_flag_unknown_device_returns_early(self, capsys): - """--device with an unknown device name exits with an error.""" - with patch("sys.argv", ["bmspy-ups", "--device", "ghost"]), \ - patch("bmspy.ups.client.read_data", return_value={"ups1": _make_ups(0.0, 0.9)}), \ - patch("bmspy.ups.client.handle_registration"): - ups_mod.main() - assert "ghost" in capsys.readouterr().out - def test_main_device_disappears_in_loop_sleeps(self): """When the named device is missing from a loop response, main() sleeps and retries.""" initial = {"testups": _make_ups(0.0, 0.8)}