Improve Safety State Reporting and MQTT Reconnect Availability

.gitignore

@@ -30,4 +30,12 @@ Thumbs.db
 htmlcov/
 
 # Logs
-*.log
+*.log
+
+# Agent tooling
+docs/superpowers/
+
+# Local reference docs (fetched, not tracked)
+docs/reference/
+.claude/
+CLAUDE.md

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}]

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/<int:device_number>/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/<int:device_number>/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/<int:device_number>/driverinfo', methods=['GET'])
 def get_driverinfo(device_number: int):

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

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) |

templates/setup.html

@@ -780,8 +780,8 @@ <h1>☁️ Simple<span class="highlight">Cloud</span>Detect</h1>
                                     </div>
                                     <div class="detection-item">
                                         <div class="detection-label">
-                                            ASCOM Status
-                                            <span class="info-tooltip" data-tooltip="Reports UNSAFE when no clients are connected. Cloud detection is always active in 'Safety History' panel.">ⓘ</span>
+                                            Safety State
+                                            <span class="info-tooltip" data-tooltip="Debounced detector state, also served by the external API and MQTT. ASCOM clients receive this value only while connected.">ⓘ</span>
                                         </div>
                                         <div class="detection-value" style="color: {{ ascom_safe_color }};">{{ ascom_safe_status }}</div>
                                     </div>

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"

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()