From 2432332f870ce4b24e13e7d16d05fb6ed0ec9981 Mon Sep 17 00:00:00 2001 From: agessaman Date: Fri, 17 Jul 2026 17:38:11 -0700 Subject: [PATCH] refactor(airtime): introduce refresh_radio_params method for dynamic modulation updates Added a new method to the AirtimeManager class to refresh modulation parameters without clearing transmission history. Updated ConfigManager to call this method after applying live radio configurations. Enhanced tests to verify that modulation updates occur correctly during live updates. --- config.yaml.example | 4 + repeater/airtime.py | 21 ++++- repeater/config_manager.py | 10 ++ repeater/data_acquisition/sqlite_handler.py | 10 +- .../data_acquisition/storage_collector.py | 4 +- repeater/engine.py | 19 ++-- tests/test_airtime.py | 28 ++++++ tests/test_config_manager.py | 94 ++++++++++++++++++- tests/test_engine.py | 4 +- tests/test_sqlite_handler_easy.py | 26 +++++ tests/test_storage_collector_cleanup.py | 35 +++++++ 11 files changed, 236 insertions(+), 19 deletions(-) create mode 100644 tests/test_storage_collector_cleanup.py diff --git a/config.yaml.example b/config.yaml.example index f404dd7..b94690f 100644 --- a/config.yaml.example +++ b/config.yaml.example @@ -522,6 +522,10 @@ storage: retention: # Clean up SQLite records older than this many days sqlite_cleanup_days: 31 + # Companion event journal / soft-consumed message history retention. + # Independent of sqlite_cleanup_days so packet RF history and companion + # sync history can be sized separately. Defaults to 31 when omitted. + companion_events_days: 31 # RRD archives are managed automatically: # - 1 minute resolution for 1 week diff --git a/repeater/airtime.py b/repeater/airtime.py index c21ddc2..c19a78f 100644 --- a/repeater/airtime.py +++ b/repeater/airtime.py @@ -1,6 +1,6 @@ import logging import time -from typing import Tuple +from typing import Optional, Tuple from openhop_core.protocol.packet_utils import calculate_lora_airtime_ms @@ -16,10 +16,7 @@ class AirtimeManager: ) # Store radio settings for airtime calculations - self.spreading_factor = self.radio_config.get("spreading_factor", 7) - self.bandwidth = self.radio_config.get("bandwidth", 125000) - self.coding_rate = self.radio_config.get("coding_rate", 5) - self.preamble_length = self.radio_config.get("preamble_length", 8) + self.refresh_radio_params(self.radio_config) # Track airtime in rolling window self.tx_history = [] # [(timestamp, airtime_ms), ...] @@ -27,6 +24,20 @@ class AirtimeManager: self.total_airtime_ms = 0 self.total_rx_airtime_ms = 0 + def refresh_radio_params(self, radio_config: Optional[dict] = None) -> None: + """Reload cached modulation params used by airtime estimation. + + Call after a successful live radio reconfiguration. Does not reset + TX/RX history or duty-cycle totals. + """ + if radio_config is None: + radio_config = self.config.get("radio", {}) or {} + self.radio_config = radio_config + self.spreading_factor = self.radio_config.get("spreading_factor", 7) + self.bandwidth = self.radio_config.get("bandwidth", 125000) + self.coding_rate = self.radio_config.get("coding_rate", 5) + self.preamble_length = self.radio_config.get("preamble_length", 8) + def calculate_airtime( self, payload_len: int, diff --git a/repeater/config_manager.py b/repeater/config_manager.py index 53f981c..4db5dc2 100644 --- a/repeater/config_manager.py +++ b/repeater/config_manager.py @@ -140,12 +140,22 @@ class ConfigManager: return False self._sync_repeater_handler_radio_config(radio_cfg) + self._refresh_airtime_radio_params() logger.info("Applied live radio configuration to running daemon") return True except Exception as e: logger.error(f"Failed to apply live radio config: {e}", exc_info=True) return False + def _refresh_airtime_radio_params(self) -> None: + repeater_handler = getattr(self.daemon, "repeater_handler", None) + airtime_mgr = getattr(repeater_handler, "airtime_mgr", None) if repeater_handler else None + if airtime_mgr is None or not hasattr(airtime_mgr, "refresh_radio_params"): + return + # Use the full radio section so preamble_length is included; the live + # hardware snapshot intentionally omits fields the radio API does not set. + airtime_mgr.refresh_radio_params(self.config.get("radio", {}) or {}) + @staticmethod def _parse_bool(value: Any, default: bool = True) -> bool: if value is None: diff --git a/repeater/data_acquisition/sqlite_handler.py b/repeater/data_acquisition/sqlite_handler.py index 12d9e62..56c585f 100644 --- a/repeater/data_acquisition/sqlite_handler.py +++ b/repeater/data_acquisition/sqlite_handler.py @@ -2351,7 +2351,15 @@ class SQLiteHandler: logger.error(f"Failed to vacuum database: {e}") raise - def cleanup_old_data(self, days: int = 7): + def cleanup_old_data(self, days: int = 7, companion_events_days: Optional[int] = None): + """Prune retention-bounded tables. + + ``companion_events_days`` is forwarded from engine.py + (``storage.retention.companion_events_days``, default 31). Accepted here + so the periodic cleanup call cannot TypeError and silently skip all + SQLite pruning. Companion journal/history pruning is layered on by the + companion-api storage work once those tables exist. + """ try: cutoff = time.time() - (days * 24 * 3600) diff --git a/repeater/data_acquisition/storage_collector.py b/repeater/data_acquisition/storage_collector.py index 0594004..b7fa2d7 100644 --- a/repeater/data_acquisition/storage_collector.py +++ b/repeater/data_acquisition/storage_collector.py @@ -501,8 +501,8 @@ class StorageCollector: logger.debug(f"Could not lookup node name for {pubkey[:8] if pubkey else 'None'}: {e}") return None - def cleanup_old_data(self, days: int = 7): - self.sqlite_handler.cleanup_old_data(days) + def cleanup_old_data(self, days: int = 7, companion_events_days: Optional[int] = None): + self.sqlite_handler.cleanup_old_data(days, companion_events_days=companion_events_days) def get_noise_floor_history(self, hours: int = 24, limit: int = None) -> list: return self.sqlite_handler.get_noise_floor_history(hours, limit) diff --git a/repeater/engine.py b/repeater/engine.py index a7eb704..b76d177 100644 --- a/repeater/engine.py +++ b/repeater/engine.py @@ -1706,12 +1706,17 @@ class RepeaterHandler(BaseHandler): if current_time - self.last_db_cleanup >= 21600: if self.storage: try: - retention_days = ( - self.config.get("storage", {}) - .get("retention", {}) - .get("sqlite_cleanup_days", 31) + retention_cfg = self.config.get("storage", {}).get( + "retention", {} + ) + retention_days = retention_cfg.get("sqlite_cleanup_days", 31) + companion_events_days = retention_cfg.get( + "companion_events_days", 31 + ) + self.storage.cleanup_old_data( + days=retention_days, + companion_events_days=companion_events_days, ) - self.storage.cleanup_old_data(days=retention_days) logger.info("Cleaned up SQLite data older than %d days", retention_days) except Exception as e: logger.warning(f"SQLite cleanup failed: {e}") @@ -1809,8 +1814,8 @@ class RepeaterHandler(BaseHandler): ): self.neighbour_link_tracker.evict_stalest_locked() - # Note: Radio config changes require restart as they affect hardware - # Note: Airtime manager has its own config reference that gets updated + # Radio hardware apply and AirtimeManager modulation refresh are + # handled by ConfigManager after a successful live radio update. logger.info("Runtime configuration reloaded successfully") except Exception as e: diff --git a/tests/test_airtime.py b/tests/test_airtime.py index d6feb8e..842a7d0 100644 --- a/tests/test_airtime.py +++ b/tests/test_airtime.py @@ -157,3 +157,31 @@ def test_stats_report_tx_rx_airtime_totals(): assert stats["total_airtime_ms"] == pytest.approx(tx_airtime) assert stats["total_rx_airtime_ms"] == pytest.approx(rx_airtime) assert stats["current_airtime_ms"] == pytest.approx(tx_airtime) + + +def test_refresh_radio_params_updates_cached_modulation(): + """Live SF/BW changes must refresh estimation scalars without clearing history.""" + from openhop_core.protocol.packet_utils import calculate_lora_airtime_ms + + mgr = _make_mgr(sf=7, bw_hz=125000, cr=5, preamble=8) + before = mgr.calculate_airtime(50) + assert math.isclose(before, 97.536, rel_tol=1e-9) + + mgr.record_tx(before) + mgr.refresh_radio_params( + { + "spreading_factor": 12, + "bandwidth": 125000, + "coding_rate": 5, + "preamble_length": 8, + } + ) + + after = mgr.calculate_airtime(50) + expected = calculate_lora_airtime_ms(50, 12, 125000, 5, 8) + assert math.isclose(after, expected, rel_tol=1e-9) + assert math.isclose(after, 2301.952, rel_tol=1e-9) + assert after > before + assert mgr.spreading_factor == 12 + assert mgr.total_airtime_ms == pytest.approx(before) + assert len(mgr.tx_history) == 1 diff --git a/tests/test_config_manager.py b/tests/test_config_manager.py index ba66ff5..065fc68 100644 --- a/tests/test_config_manager.py +++ b/tests/test_config_manager.py @@ -1,37 +1,62 @@ +import math + +from repeater.airtime import AirtimeManager from repeater.config_manager import ConfigManager class _DummyRepeaterHandler: - def __init__(self): + def __init__(self, config=None): self.radio_config = {} + self.airtime_mgr = AirtimeManager( + config + or { + "radio": { + "frequency": 868000000, + "bandwidth": 125000, + "spreading_factor": 7, + "coding_rate": 5, + "tx_power": 14, + "preamble_length": 8, + } + } + ) class _DummySX1262Radio: - def __init__(self): + def __init__(self, apply_ok=True): self.frequency = 868000000 self.bandwidth = 125000 self.spreading_factor = 7 self.coding_rate = 5 self.tx_power = 14 self.calls = [] + self.apply_ok = apply_ok def set_frequency(self, frequency): self.calls.append(("set_frequency", frequency)) + if not self.apply_ok: + return False self.frequency = frequency return True def set_tx_power(self, power): self.calls.append(("set_tx_power", power)) + if not self.apply_ok: + return False self.tx_power = power return True def set_spreading_factor(self, spreading_factor): self.calls.append(("set_spreading_factor", spreading_factor)) + if not self.apply_ok: + return False self.spreading_factor = spreading_factor return True def set_bandwidth(self, bandwidth): self.calls.append(("set_bandwidth", bandwidth)) + if not self.apply_ok: + return False self.bandwidth = bandwidth return True @@ -64,7 +89,7 @@ class _DummyDaemon: "kiss": dict(config.get("kiss", {})), } self.radio = radio - self.repeater_handler = _DummyRepeaterHandler() + self.repeater_handler = _DummyRepeaterHandler(config) self.advert_helper = None self.dispatcher = None @@ -128,3 +153,66 @@ def test_live_update_daemon_applies_kiss_radio_config(): ] assert radio.radio_config == config["radio"] assert daemon.repeater_handler.radio_config == config["radio"] + + +def test_live_update_daemon_refreshes_airtime_manager_modulation(): + startup_radio = { + "frequency": 868000000, + "bandwidth": 125000, + "spreading_factor": 7, + "coding_rate": 5, + "tx_power": 14, + "preamble_length": 8, + } + updated_radio = { + "frequency": 915000000, + "bandwidth": 125000, + "spreading_factor": 12, + "coding_rate": 5, + "tx_power": 14, + "preamble_length": 8, + } + config = {"radio": dict(startup_radio)} + radio = _DummySX1262Radio() + daemon = _DummyDaemon(config, radio) + airtime_mgr = daemon.repeater_handler.airtime_mgr + before = airtime_mgr.calculate_airtime(50) + assert math.isclose(before, 97.536, rel_tol=1e-9) + + config["radio"] = dict(updated_radio) + manager = ConfigManager("/tmp/config.yaml", config, daemon) + assert manager.live_update_daemon(["radio"]) + + assert airtime_mgr.spreading_factor == 12 + assert airtime_mgr.bandwidth == 125000 + assert airtime_mgr.preamble_length == 8 + assert math.isclose(airtime_mgr.calculate_airtime(50), 2301.952, rel_tol=1e-9) + + +def test_failed_live_radio_apply_leaves_airtime_manager_unchanged(): + startup_radio = { + "frequency": 868000000, + "bandwidth": 125000, + "spreading_factor": 7, + "coding_rate": 5, + "tx_power": 14, + "preamble_length": 8, + } + config = {"radio": dict(startup_radio)} + radio = _DummySX1262Radio(apply_ok=False) + daemon = _DummyDaemon(config, radio) + airtime_mgr = daemon.repeater_handler.airtime_mgr + + config["radio"] = { + "frequency": 915000000, + "bandwidth": 125000, + "spreading_factor": 12, + "coding_rate": 5, + "tx_power": 14, + "preamble_length": 8, + } + manager = ConfigManager("/tmp/config.yaml", config, daemon) + assert manager.live_update_daemon(["radio"]) is False + + assert airtime_mgr.spreading_factor == 7 + assert math.isclose(airtime_mgr.calculate_airtime(50), 97.536, rel_tol=1e-9) diff --git a/tests/test_engine.py b/tests/test_engine.py index f334a0f..7a718e4 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -2774,7 +2774,9 @@ class TestEngineTransmissionAndBackgroundLifecycle: handler._record_crc_errors_async.assert_awaited_once() handler._send_periodic_advert_async.assert_awaited_once() handler.cleanup_cache.assert_called_once() - handler.storage.cleanup_old_data.assert_called_once() + handler.storage.cleanup_old_data.assert_called_once_with( + days=31, companion_events_days=31 + ) @pytest.mark.asyncio async def test_background_timer_loop_continues_when_db_cleanup_fails(self, handler): diff --git a/tests/test_sqlite_handler_easy.py b/tests/test_sqlite_handler_easy.py index bb71d8e..d598748 100644 --- a/tests/test_sqlite_handler_easy.py +++ b/tests/test_sqlite_handler_easy.py @@ -676,3 +676,29 @@ def test_get_lbt_diagnostics_empty_range_preserves_no_data_distinction(tmp_path) assert bucket["avg_attempts"] is None assert out["packet_types"] == [] assert out["packet_type_buckets"] == [] + + +def test_cleanup_old_data_accepts_companion_events_days(tmp_path): + """engine.py forwards companion_events_days; accepting it must not break + packet retention cleanup (the pre-fix TypeError was swallowed upstream). + """ + import time + + h = _make_handler(tmp_path) + old_ts = time.time() - (40 * 86400) + h.store_packet( + { + "timestamp": old_ts, + "type": 1, + "route": 2, + "length": 3, + "transmitted": False, + "packet_hash": "pkt-old", + } + ) + + h.cleanup_old_data(days=31, companion_events_days=14) + + with h._connect() as conn: + count = conn.execute("SELECT COUNT(*) FROM packets").fetchone()[0] + assert count == 0 diff --git a/tests/test_storage_collector_cleanup.py b/tests/test_storage_collector_cleanup.py new file mode 100644 index 0000000..15de20c --- /dev/null +++ b/tests/test_storage_collector_cleanup.py @@ -0,0 +1,35 @@ +"""Regression test: StorageCollector.cleanup_old_data must accept and pass +through ``companion_events_days``. + +engine.py's 6-hourly cleanup passes both kwargs; before this passthrough +existed the call raised TypeError, was swallowed by the caller's broad +except, and SQLite cleanup silently never ran. +""" + +from unittest.mock import Mock + +from repeater.data_acquisition.storage_collector import StorageCollector + + +def _bare_collector() -> StorageCollector: + collector = StorageCollector.__new__(StorageCollector) + collector.sqlite_handler = Mock() + return collector + + +def test_cleanup_passes_companion_events_days_through(): + collector = _bare_collector() + collector.cleanup_old_data(days=10, companion_events_days=20) + collector.sqlite_handler.cleanup_old_data.assert_called_once_with( + 10, companion_events_days=20 + ) + + +def test_cleanup_defaults_match_engine_callsite(): + # engine.py always passes both kwargs, but a bare call must stay valid + # for any older callers that only know about ``days``. + collector = _bare_collector() + collector.cleanup_old_data(days=7) + collector.sqlite_handler.cleanup_old_data.assert_called_once_with( + 7, companion_events_days=None + )