diff --git a/repeater/config.py b/repeater/config.py index d6af428..2e544c2 100644 --- a/repeater/config.py +++ b/repeater/config.py @@ -1312,12 +1312,19 @@ def build_metering_profiles(config: dict) -> list: def _default_radio_id(config: dict, radio_ids: list) -> Optional[str]: - """The radio a live config save actually retunes. + """The radio a live config save retunes, read from a config mapping. - Mirrors build_radio_stack's TX default -- ``fabric.default_radio`` when it - names a radio that exists, else the first configured one. A default naming - a radio that is not there cannot be the one that got retuned, so it falls - back rather than freezing every entry. + Follows build_radio_stack's TX default -- ``fabric.default_radio``, else the + first configured radio -- with one deliberate difference: a default naming a + radio that is not in ``radio_ids`` falls back to the first instead of being + returned as-is. build_radio_stack hands an unknown id straight to FabricRadio + and the node fails to start, so that config never reaches a status publish; + here the fallback only has to avoid freezing every entry. + + Callers reporting on a *running* node should capture this at boot rather + than recompute it: ``fabric.default_radio`` is restart-required, so a live + edit changes this answer while the running Fabric keeps its original + default. See ``capture_radio_status_baseline``. """ fabric_cfg = config.get("fabric") if isinstance(config.get("fabric"), dict) else {} default_radio = fabric_cfg.get("default_radio") or fabric_cfg.get("default_radio_id") @@ -1326,7 +1333,35 @@ def _default_radio_id(config: dict, radio_ids: list) -> Optional[str]: return radio_ids[0] if radio_ids else None -def build_radio_status_entries(config: dict, *, applied: Optional[dict] = None) -> list: +def capture_radio_status_baseline(config: dict) -> tuple: + """The radio map and default radio id as built, for a reporter to hold. + + Call this once, from boot config, before anything can edit it. Returns + ``(applied, default_radio_id)`` to hand back to + ``build_radio_status_entries`` on every later build. + + Both halves have to be captured together and early. The map is what + non-default radios keep until they are really reconfigured, and the default + id is which entry is allowed to move -- and that id is itself restart-required, + so reading it from live config would let a pending ``fabric.default_radio`` + edit refresh the radio that was not retuned while freezing the one that was, + reporting both radios wrongly where rebuilding everything got one right. + + Returns ``({}, None)`` if the config cannot be read, which leaves the caller + rebuilding from config exactly as it did before. + """ + try: + entries = build_radio_status_entries(config) + except Exception as exc: # pragma: no cover - reporting must not break startup + logger.debug("Could not capture the radio status baseline: %s", exc) + return {}, None + applied = {entry["id"]: entry for entry in entries} + return applied, _default_radio_id(config, list(applied)) + + +def build_radio_status_entries( + config: dict, *, applied: Optional[dict] = None, default_radio_id: Optional[str] = None +) -> list: """Map every radio id to its air settings, for the MQTT status message. Observers join this against the ``rx_radio_id`` / ``tx_radio_ids`` fields @@ -1347,9 +1382,12 @@ def build_radio_status_entries(config: dict, *, applied: Optional[dict] = None) SX1262 holds -9..22 dBm). ``applied`` maps radio id to the entry built when that radio was last really - configured, and makes the map survive a live save. Only the default radio is - retuned without a restart: a change to any other sits in ``radios[]`` marked - restart-required. Because ``radios[]`` entries inherit the top-level + configured, and ``default_radio_id`` says which entry a live save is allowed + to move. Both come from ``capture_radio_status_baseline`` at boot; passing + ``applied`` without ``default_radio_id`` falls back to reading the default + from ``config``, which is only right if nothing has edited it yet. Only the + default radio is retuned without a restart: a change to any other sits in + ``radios[]`` marked restart-required. Because ``radios[]`` entries inherit the top-level ``radio`` block key by key, a save that edits the default radio also moves the *configured* values of every radio whose entry omits that key, so rebuilding all of them from config would publish a bandwidth the hardware @@ -1387,7 +1425,9 @@ def build_radio_status_entries(config: dict, *, applied: Optional[dict] = None) entries.append(entry) if applied: - default_id = _default_radio_id(config, [entry["id"] for entry in entries]) + default_id = default_radio_id + if default_id is None: + default_id = _default_radio_id(config, [entry["id"] for entry in entries]) entries = [ entry if entry["id"] == default_id else applied.get(entry["id"], entry) for entry in entries diff --git a/repeater/data_acquisition/mqtt_handler.py b/repeater/data_acquisition/mqtt_handler.py index e835acf..7e81185 100644 --- a/repeater/data_acquisition/mqtt_handler.py +++ b/repeater/data_acquisition/mqtt_handler.py @@ -669,10 +669,16 @@ class MeshCoreToMqttPusher: self._shutdown_requested = False self._lock = threading.Lock() self._connect_timers: List[threading.Timer] = [] - # The radio map as built when the radios were last really configured. - # Seeded on the first build, which is this process's start -- see - # _radio_map for why a later rebuild must not replace it wholesale. - self._applied_radio_map: Optional[dict] = None + # The radio map and default radio id as the daemon built them, captured + # here rather than on the first publish: that publish waits for a broker + # to connect, and a node whose MQTT is briefly unreachable can take a + # live radio save first, which would bake the pending settings in as + # though they were what the hardware got. See _radio_map. + from ..config import capture_radio_status_baseline + + self._applied_radio_map, self._applied_default_radio_id = capture_radio_status_baseline( + config + ) # Initialize brokers list mqtt_brokers_config = config.get("mqtt_brokers", {}) @@ -1051,25 +1057,32 @@ class MeshCoreToMqttPusher: ``radio`` field already says everything, so the map is published only when there is genuinely more than one band to tell apart. - Rebuilt per status publish rather than cached at construction, because - air settings can be changed from the web UI while the node runs. Only - the default radio is retuned by such a save, though, and radios[] entries + Rebuilt per status publish rather than cached whole, because the default + radio's air settings can be changed from the web UI while the node runs. + Only that radio is retuned by such a save, though, and radios[] entries inherit the top-level ``radio`` block key by key -- so a rebuild that took every entry from the current config would report a new bandwidth for radios still transmitting on the old one, and observers would - attribute their packets to a band they are not on. The first build is - kept and handed back in as ``applied`` so every non-default radio keeps - what it was really configured with until the service restarts. + attribute their packets to a band they are not on. The baseline captured + at construction is handed back in, so every non-default radio keeps what + it was really configured with until the service restarts. + + The default radio id comes from that same baseline. ``fabric.default_radio`` + is itself restart-required, so reading it from the live config would let + a pending change refresh the radio that was not retuned and freeze the + one that was. """ from ..config import build_radio_status_entries try: - entries = build_radio_status_entries(self.config, applied=self._applied_radio_map) + entries = build_radio_status_entries( + self.config, + applied=self._applied_radio_map, + default_radio_id=self._applied_default_radio_id, + ) except Exception as exc: # pragma: no cover - reporting must not break status logger.debug(f"Could not build radio map for status: {exc}") return [] - if self._applied_radio_map is None and entries: - self._applied_radio_map = {entry["id"]: entry for entry in entries} return entries if len(entries) > 1 else [] def publish(self, subtopic: str, payload: dict, retain: bool = False, qos: int = 0): diff --git a/tests/test_observer_radio_attribution.py b/tests/test_observer_radio_attribution.py index cdbd33a..a4ebe9e 100644 --- a/tests/test_observer_radio_attribution.py +++ b/tests/test_observer_radio_attribution.py @@ -18,7 +18,12 @@ from unittest.mock import MagicMock, patch import pytest -from repeater.config import build_radio_status_entries, get_node_info, get_radio_for_board +from repeater.config import ( + build_radio_status_entries, + capture_radio_status_baseline, + get_node_info, + get_radio_for_board, +) from repeater.data_acquisition.mqtt_handler import MeshCoreToMqttPusher from repeater.data_acquisition.storage_utils import PacketRecord @@ -503,8 +508,14 @@ def test_applied_map_holds_non_default_radios_across_a_live_edit(): def test_applied_map_is_ignored_for_a_radio_the_config_has_just_added(): - """A radio with no applied entry has no band to preserve, so it reports - what it is configured with.""" + """A radio with no applied entry reports what it is configured with. + + Adding a radio is restart-required, so until then it has no hardware on the + air at all and arguably should not be published. This pins the behaviour + that was already there -- the map has always been built from the current + radios[] list -- rather than endorsing it; fixing it means publishing the + boot topology, which is a change of its own. + """ config = { "radio_type": "sx1262", "radio": LOCAL_RADIO["radio"], @@ -565,3 +576,77 @@ def test_published_status_holds_a_non_retuned_radio_across_a_live_edit(): "local": "869.618,500.0,8,8", # retuned "link": "864.2,62.5,8,8", # untouched, still reported as it is } + + +def test_a_live_save_before_the_first_publish_does_not_poison_the_baseline(): + """The first status publish waits for a broker to connect. A node whose MQTT + is briefly unreachable can take a radio save first, so a baseline captured + on that first publish would bake the pending settings in as though they were + what the hardware got.""" + config = _make_config( + radios=[copy.deepcopy(LOCAL_RADIO), {"id": "link", "radio": {"frequency": 864200000}}] + ) + config["radio"] = copy.deepcopy(LOCAL_RADIO["radio"]) + config["fabric"]["default_radio"] = "local" + + pusher = MeshCoreToMqttPusher(local_identity=_FakeIdentity("AB" * 32), config=config) + + # The save lands while MQTT is still down, before any status was published. + config["radios"][0]["radio"]["bandwidth"] = 500000 + config["radio"]["bandwidth"] = 500000 + + conn = pusher.connections[0] + captured = [] + conn._running = True + conn.client = MagicMock() + conn.client.publish = lambda topic, payload, retain=False, qos=0: captured.append(payload) + pusher.publish_status(state="online") + + by_id = {e["id"]: e["radio"] for e in json.loads(captured[0])["radios"]} + assert by_id["local"] == "869.618,500.0,8,8" # really retuned + assert by_id["link"] == "864.2,62.5,8,8" # never was + + +def test_a_pending_default_radio_change_does_not_move_which_entry_is_refreshed(): + """``fabric.default_radio`` is restart-required, so the running Fabric keeps + the default it booted with. Reading the default from live config would + refresh the radio that was not retuned and freeze the one that was -- + reporting both wrongly, where rebuilding everything at least got one right. + """ + config = { + "radio_type": "sx1262", + "radio": { + "frequency": 869618000, + "bandwidth": 62500, + "spreading_factor": 8, + "coding_rate": 8, + "preamble_length": 32, + }, + "fabric": {"default_radio": "link"}, + "radios": [ + {"id": "local", "radio": {"frequency": 869618000}}, + {"id": "link", "radio": {"frequency": 864200000}}, + ], + } + + applied, default_id = capture_radio_status_baseline(config) + assert default_id == "link" + + # A save retunes the running default (link), and an import queues a change + # of default that only takes effect on restart. + config["radios"][1]["radio"]["bandwidth"] = 500000 + config["radio"]["bandwidth"] = 500000 + config["fabric"]["default_radio"] = "local" + + by_id = { + e["id"]: e["radio"] + for e in build_radio_status_entries(config, applied=applied, default_radio_id=default_id) + } + + assert by_id["link"] == "864.2,500.0,8,8" # the radio that was retuned + assert by_id["local"] == "869.618,62.5,8,8" # the radio that was not + + # Following the pending default instead gets both radios wrong. + naive = {e["id"]: e["radio"] for e in build_radio_status_entries(config, applied=applied)} + assert naive["link"] == "864.2,62.5,8,8" + assert naive["local"] == "869.618,500.0,8,8"