diff --git a/.gitignore b/.gitignore index 74169ae..8e4eec0 100644 --- a/.gitignore +++ b/.gitignore @@ -30,4 +30,12 @@ Thumbs.db htmlcov/ # Logs -*.log \ No newline at end of file +*.log + +# Agent tooling +docs/superpowers/ + +# Local reference docs (fetched, not tracked) +docs/reference/ +.claude/ +CLAUDE.md diff --git a/alpaca/device.py b/alpaca/device.py index e1b956e..6a72751 100644 --- a/alpaca/device.py +++ b/alpaca/device.py @@ -68,10 +68,11 @@ def __init__(self, alpaca_config: AlpacaConfig, detect_config: DetectConfig): self.detection_thread: Optional[threading.Thread] = None self.stop_detection = threading.Event() - # Setup MQTT client first - self.mqtt_client = self._setup_mqtt() + # Setup MQTT client first. ha_discovery must exist before _setup_mqtt() + # because the on_connect callback fires from the network thread and reads it. self.ha_discovery = None - + self.mqtt_client = self._setup_mqtt() + # Pre-load cloud detector at startup logger.info("Pre-loading ML model...") self.cloud_detector = CloudDetector(self.detect_config, mqtt_client=self.mqtt_client) @@ -216,7 +217,24 @@ def _setup_mqtt(self): if self.detect_config.mqtt_discovery_mode == 'homeassistant': availability_topic = f"{self.detect_config.mqtt_discovery_prefix}/sensor/clouddetect_{self.detect_config.device_id}/availability" client.will_set(availability_topic, "offline", retain=True) - + + # Republish discovery and availability on every connect, not just the + # first. loop_start() reconnects silently after a dropped session, and + # the broker publishes the retained "offline" will when that happens. + # Without this the device stays unavailable in Home Assistant forever + # even though state topics keep updating. + # Signature accepts paho 1.x (client, userdata, flags, rc) and + # 2.x (..., reason_code, properties). + def on_connect(client, userdata, flags, rc, properties=None): + if rc != 0: + logger.error(f"MQTT connect failed with code {rc}") + return + if self.ha_discovery: + self.ha_discovery.publish_discovery_configs() + logger.info("Republished HA discovery and availability after connect") + + client.on_connect = on_connect + try: client.connect(self.detect_config.broker, self.detect_config.port) client.loop_start() @@ -423,11 +441,12 @@ def disconnect(self, client_ip: str = None, client_id: int = None): logger.warning(f"Attempted to disconnect unknown client: {client_ip} (ID: {client_id})") def is_safe(self) -> bool: - """Determine if conditions are safe based on latest detection""" - # Safety Fail-safe: Always return False if not connected - if not self.is_connected: - return False - + """Debounced safety state of the sky, independent of ASCOM client connections. + + The Alpaca routes apply the per-client Connected gate themselves + (see routes/api.py client_is_safe). Everything else (external REST + API, dashboard, MQTT) reports this value directly. + """ with self.detection_lock: return self._stable_safe_state @@ -478,8 +497,3 @@ def get_connected_clients_info(self) -> List[Dict[str, Any]]: "duration_seconds": (now - conn_time).total_seconds() }) return clients - - def get_device_state(self) -> list: - """Get current operational state""" - is_safe_val = self.is_safe() - return [{"Name": "IsSafe", "Value": is_safe_val}] diff --git a/alpaca/routes/api.py b/alpaca/routes/api.py index cce5875..066b491 100644 --- a/alpaca/routes/api.py +++ b/alpaca/routes/api.py @@ -53,6 +53,19 @@ def endpoint(device_number: int): )) return endpoint +def client_is_safe() -> bool: + """IsSafe as reported to the calling Alpaca client. + + Alpaca tracks Connected per client (IP + ClientID). A client that has not + connected is told False, matching the pre-connection behaviour ASCOM + clients have always seen from this driver. The detector itself keeps + running; monitor.is_safe() carries the real state for non-Alpaca consumers. + """ + client_id, _ = monitor.get_client_params() + if not monitor.is_client_connected(request.remote_addr, client_id): + return False + return monitor.is_safe() + @api_bp.route('/v1/safetymonitor//issafe', methods=['GET']) def get_issafe(device_number: int): """Get safety status""" @@ -61,7 +74,7 @@ def get_issafe(device_number: int): return error_response _, client_tx_id = monitor.get_client_params() return jsonify(monitor.create_response( - value=monitor.is_safe(), + value=client_is_safe(), client_transaction_id=client_tx_id )) @@ -186,7 +199,9 @@ def get_description(device_number: int): @api_bp.route('/v1/safetymonitor//devicestate', methods=['GET']) def get_devicestate(device_number: int): - return create_simple_get_endpoint(monitor.get_device_state)(device_number) + return create_simple_get_endpoint( + lambda: [{"Name": "IsSafe", "Value": client_is_safe()}] + )(device_number) @api_bp.route('/v1/safetymonitor//driverinfo', methods=['GET']) def get_driverinfo(device_number: int): diff --git a/detect.py b/detect.py index 39e68c0..8d7649b 100755 --- a/detect.py +++ b/detect.py @@ -190,9 +190,11 @@ def __init__(self, config: Config, mqtt_client=None): self.model = self._load_model() self.class_names = self._load_class_names() self.session = requests.Session() # Reuse HTTP connections - self.mqtt_client = mqtt_client if mqtt_client is not None else self._setup_mqtt() + # ha_discovery must exist before _setup_mqtt() because the on_connect + # callback fires from the network thread and reads it. self.ha_discovery = None - + self.mqtt_client = mqtt_client if mqtt_client is not None else self._setup_mqtt() + # Initialize HA discovery if enabled if self.mqtt_client and self.config.mqtt_discovery_mode == 'homeassistant': self.ha_discovery = HADiscoveryManager(self.config, self.mqtt_client) @@ -230,7 +232,22 @@ def _setup_mqtt(self): if self.config.mqtt_discovery_mode == 'homeassistant': availability_topic = f"{self.config.mqtt_discovery_prefix}/sensor/clouddetect_{self.config.device_id}/availability" client.will_set(availability_topic, "offline", retain=True) - + + # Republish discovery and availability on every connect, not just the + # first. loop_start() reconnects silently after a dropped session, and + # the broker publishes the retained "offline" will when that happens. + # Signature accepts paho 1.x (client, userdata, flags, rc) and + # 2.x (..., reason_code, properties). + def on_connect(client, userdata, flags, rc, properties=None): + if rc != 0: + logger.error(f"MQTT connect failed with code {rc}") + return + if self.ha_discovery: + self.ha_discovery.publish_discovery_configs() + logger.info("Republished HA discovery and availability after connect") + + client.on_connect = on_connect + try: client.connect(self.config.broker, self.config.port) # Start background network loop to handle keepalive pings and reconnections diff --git a/docs/EXTERNAL_API.md b/docs/EXTERNAL_API.md index a2c3635..00bc22a 100644 --- a/docs/EXTERNAL_API.md +++ b/docs/EXTERNAL_API.md @@ -114,7 +114,7 @@ Returns current safety status and latest detection results. | Field | Type | Description | |-------|------|-------------| -| `is_safe` | boolean | Current safety state | +| `is_safe` | boolean | Debounced detector state. Does not depend on any ASCOM client being connected. | | `safety_status` | string | "Safe" or "Unsafe" | | `detection.class_name` | string | Detected condition (Clear, Wisps, Mostly Cloudy, etc.) | | `detection.confidence_score` | number | ML model confidence (0.0 - 1.0) | diff --git a/templates/setup.html b/templates/setup.html index 454c518..c6e2521 100644 --- a/templates/setup.html +++ b/templates/setup.html @@ -780,8 +780,8 @@

☁️ SimpleCloudDetect

- ASCOM Status - ⓘ + Safety State + ⓘ
{{ ascom_safe_status }}
diff --git a/tests/test_issafe_gate.py b/tests/test_issafe_gate.py new file mode 100644 index 0000000..3b38432 --- /dev/null +++ b/tests/test_issafe_gate.py @@ -0,0 +1,80 @@ +"""IsSafe is gated per Alpaca client; the external API reports raw detector state.""" +import threading +from types import SimpleNamespace + +import pytest +from flask import Flask, request + +from alpaca.routes.api import api_bp, init_api +from alpaca.routes.external_api import external_api_bp, init_external_api + + +class FakeMonitor: + def __init__(self, safe=True): + self.alpaca_config = SimpleNamespace(device_number=0) + self._safe = safe + self.connected = set() + self.detection_lock = threading.Lock() + self.latest_detection = None + + def get_client_params(self): + return int(request.args.get("ClientID", 0)), int(request.args.get("ClientTransactionID", 0)) + + def register_heartbeat(self, ip, client_id): + pass + + def create_response(self, value=None, error_number=0, error_message="", client_transaction_id=0): + return { + "Value": value, + "ErrorNumber": error_number, + "ErrorMessage": error_message, + "ClientTransactionID": client_transaction_id, + } + + def is_client_connected(self, ip, client_id): + return (ip, client_id) in self.connected + + def is_safe(self): + return self._safe + + +@pytest.fixture +def harness(): + monitor = FakeMonitor(safe=True) + app = Flask(__name__) + init_api(monitor) + init_external_api(monitor) + app.register_blueprint(api_bp, url_prefix="/api") + app.register_blueprint(external_api_bp) + return app.test_client(), monitor + + +def test_issafe_false_for_unconnected_client(harness): + client, _ = harness + resp = client.get("/api/v1/safetymonitor/0/issafe?ClientID=5") + assert resp.get_json()["Value"] is False + + +def test_issafe_true_for_connected_client_only(harness): + client, monitor = harness + monitor.connected.add(("127.0.0.1", 5)) + assert client.get("/api/v1/safetymonitor/0/issafe?ClientID=5").get_json()["Value"] is True + assert client.get("/api/v1/safetymonitor/0/issafe?ClientID=6").get_json()["Value"] is False + + +def test_devicestate_uses_same_gate(harness): + client, monitor = harness + assert client.get("/api/v1/safetymonitor/0/devicestate?ClientID=5").get_json()["Value"] == [ + {"Name": "IsSafe", "Value": False} + ] + monitor.connected.add(("127.0.0.1", 5)) + assert client.get("/api/v1/safetymonitor/0/devicestate?ClientID=5").get_json()["Value"] == [ + {"Name": "IsSafe", "Value": True} + ] + + +def test_external_status_ignores_client_connections(harness): + client, _ = harness + data = client.get("/api/ext/v1/status").get_json() + assert data["is_safe"] is True + assert data["safety_status"] == "Safe" diff --git a/tests/test_mqtt_reconnect.py b/tests/test_mqtt_reconnect.py new file mode 100644 index 0000000..426f197 --- /dev/null +++ b/tests/test_mqtt_reconnect.py @@ -0,0 +1,102 @@ +""" +Regression test: MQTT availability must be restored after a reconnect. + +The broker publishes the retained "offline" Last Will when a session drops. +paho's loop_start() reconnects silently, so without an on_connect handler that +republishes discovery, the device stays permanently unavailable in Home +Assistant while its state topics keep updating. +""" +import sys +import types +import unittest +from unittest import mock + +# detect.py imports TensorFlow at module level. Stub it so this test runs +# without the ML stack installed. Everything else is a real dependency. +if 'tensorflow' not in sys.modules: + keras_models = types.ModuleType('tensorflow.keras.models') + keras_models.load_model = lambda *a, **k: None + keras = types.ModuleType('tensorflow.keras') + keras.models = keras_models + tf = types.ModuleType('tensorflow') + tf.keras = keras + sys.modules['tensorflow'] = tf + sys.modules['tensorflow.keras'] = keras + sys.modules['tensorflow.keras.models'] = keras_models + sys.modules['keras'] = keras + sys.modules['keras.models'] = keras_models + +sys.path.insert(0, '.') + +from alpaca.device import AlpacaSafetyMonitor # noqa: E402 + + +class FakeConfig: + broker = '192.168.1.250' + port = 1883 + mqtt_username = None + mqtt_password = None + mqtt_discovery_mode = 'homeassistant' + mqtt_discovery_prefix = 'homeassistant' + device_id = 'testdevice' + + +class FakeMonitor: + """Minimal stand-in so _setup_mqtt can run without loading the ML model.""" + + def __init__(self): + self.detect_config = FakeConfig() + self.ha_discovery = None + + +class MqttReconnectTest(unittest.TestCase): + + def _setup(self): + monitor = FakeMonitor() + fake_client = mock.MagicMock() + with mock.patch('alpaca.device.mqtt.Client', return_value=fake_client): + AlpacaSafetyMonitor._setup_mqtt(monitor) + return monitor, fake_client + + def test_last_will_is_offline_and_retained(self): + _, client = self._setup() + client.will_set.assert_called_once_with( + 'homeassistant/sensor/clouddetect_testdevice/availability', + 'offline', + retain=True, + ) + + def test_reconnect_republishes_discovery(self): + monitor, client = self._setup() + monitor.ha_discovery = mock.MagicMock() + + client.on_connect(client, None, {}, 0) + + monitor.ha_discovery.publish_discovery_configs.assert_called_once() + + def test_failed_connect_does_not_republish(self): + monitor, client = self._setup() + monitor.ha_discovery = mock.MagicMock() + + client.on_connect(client, None, {}, 5) # 5 = not authorised + + monitor.ha_discovery.publish_discovery_configs.assert_not_called() + + def test_first_connect_before_discovery_exists_does_not_raise(self): + # on_connect fires from the network thread during _setup_mqtt, before + # ha_discovery is assigned. It must not raise there. + _, client = self._setup() + client.on_connect(client, None, {}, 0) + + def test_paho2_signature_accepted(self): + # paho 2.x calls on_connect with an extra properties argument. + monitor, client = self._setup() + monitor.ha_discovery = mock.MagicMock() + + client.on_connect(client, None, {}, 0, None) + + monitor.ha_discovery.publish_discovery_configs.assert_called_once() + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/verify_multi_client.py b/tests/verify_multi_client.py index d8b9917..2f4e7c0 100644 --- a/tests/verify_multi_client.py +++ b/tests/verify_multi_client.py @@ -20,9 +20,9 @@ def check_connected(): log(f"Error checking connected status: {e}") return None -def check_issafe(): +def check_issafe(client_id=0): try: - resp = requests.get(f"{BASE_URL}/issafe") + resp = requests.get(f"{BASE_URL}/issafe", params={'ClientID': client_id}) resp.raise_for_status() return resp.json()["Value"] except Exception as e: @@ -139,19 +139,31 @@ def run_tests(): log("Scenario B Passed") - # Scenario C: Fail-safe - log("--- Scenario C: Fail-safe ---") + # Scenario C: Per-client IsSafe gate + log("--- Scenario C: Per-client IsSafe gate ---") # Ensure fully disconnected if check_connected(): log("FAIL: System should be disconnected") sys.exit(1) - - is_safe = check_issafe() - log(f"Is Safe (while disconnected)? {is_safe}") - if is_safe: - log("FAIL: Should report Unsafe (False) when disconnected") + + log("Connecting Client A (ID 1)...") + connect(1) + + is_safe_other = check_issafe(client_id=99) + log(f"Is Safe for unconnected Client 99? {is_safe_other}") + if is_safe_other: + log("FAIL: A client that never connected must see IsSafe=False") sys.exit(1) - + + is_safe_a = check_issafe(client_id=1) + log(f"Is Safe for connected Client 1? {is_safe_a}") + if is_safe_a is None: + log("FAIL: Connected client could not read IsSafe") + sys.exit(1) + + log("Disconnecting Client A...") + disconnect(1) + log("Scenario C Passed") log("ALL TESTS PASSED")