[nikohomecontrol] Reconnection logic (#12885)

* Clear handler references when disposing handler
* Improved handling when connection lost
* Fix refresh interval property definition
* Unset event handler when actions removed from NHC
* Startup and connection attempt improvements
* Unset action references
* Reconnection logic cleanup
* Cleanup bridge online setting
* Also synchronize scheduleRestartCommunication

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>

bundles/org.openhab.binding.nikohomecontrol/src/main/java/org/openhab/binding/nikohomecontrol/internal/handler/NikoHomeControlActionHandler.java

@@ -278,6 +278,19 @@ public class NikoHomeControlActionHandler extends BaseThingHandler implements Nh
         initialized = true;
     }
 
+    @Override
+    public void dispose() {
+        NikoHomeControlCommunication nhcComm = getCommunication(getBridgeHandler());
+        if (nhcComm != null) {
+            NhcAction action = nhcComm.getActions().get(actionId);
+            if (action != null) {
+                action.unsetEventHandler();
+            }
+        }
+        nhcAction = null;
+        super.dispose();
+    }
+
     private void updateProperties(NhcAction nhcAction) {
         Map<String, String> properties = new HashMap<>();
 
@@ -347,7 +360,7 @@ public class NikoHomeControlActionHandler extends BaseThingHandler implements Nh
     private void restartCommunication(NikoHomeControlCommunication nhcComm) {
         // We lost connection but the connection object is there, so was correctly started.
         // Try to restart communication.
-        nhcComm.restartCommunication();
+        nhcComm.scheduleRestartCommunication();
         // If still not active, take thing offline and return.
         if (!nhcComm.communicationActive()) {
             updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR,

bundles/org.openhab.binding.nikohomecontrol/src/main/java/org/openhab/binding/nikohomecontrol/internal/handler/NikoHomeControlBridgeHandler.java

@@ -79,6 +79,10 @@ public abstract class NikoHomeControlBridgeHandler extends BaseBridgeHandler imp
 
         scheduler.submit(() -> {
             comm.startCommunication();
+
+            int refreshInterval = getConfig().as(NikoHomeControlBridgeConfig.class).refresh;
+            setupRefreshTimer(refreshInterval);
+
             if (!comm.communicationActive()) {
                 bridgeOffline();
                 return;
@@ -87,9 +91,6 @@ public abstract class NikoHomeControlBridgeHandler extends BaseBridgeHandler imp
             updateProperties();
 
             updateStatus(ThingStatus.ONLINE);
-
-            int refreshInterval = getConfig().as(NikoHomeControlBridgeConfig.class).refresh;
-            setupRefreshTimer(refreshInterval);
         });
     }
 
@@ -153,11 +154,6 @@ public abstract class NikoHomeControlBridgeHandler extends BaseBridgeHandler imp
     @Override
     public void controllerOnline() {
         bridgeOnline();
-
-        int refreshInterval = getConfig().as(NikoHomeControlBridgeConfig.class).refresh;
-        if (refreshTimer == null) {
-            setupRefreshTimer(refreshInterval);
-        }
     }
 
     /**
@@ -178,6 +174,7 @@ public abstract class NikoHomeControlBridgeHandler extends BaseBridgeHandler imp
             comm.stopCommunication();
         }
         nhcComm = null;
+        super.dispose();
     }
 
     @Override

bundles/org.openhab.binding.nikohomecontrol/src/main/java/org/openhab/binding/nikohomecontrol/internal/handler/NikoHomeControlEnergyMeterHandler.java

@@ -149,10 +149,15 @@ public class NikoHomeControlEnergyMeterHandler extends BaseThingHandler implemen
     @Override
     public void dispose() {
         NikoHomeControlCommunication nhcComm = getCommunication(getBridgeHandler());
-
         if (nhcComm != null) {
             nhcComm.stopEnergyMeter(energyMeterId);
+            NhcEnergyMeter energyMeter = nhcComm.getEnergyMeters().get(energyMeterId);
+            if (energyMeter != null) {
+                energyMeter.unsetEventHandler();
+            }
         }
+        nhcEnergyMeter = null;
+        super.dispose();
     }
 
     private void updateProperties(NhcEnergyMeter nhcEnergyMeter) {
@@ -236,7 +241,7 @@ public class NikoHomeControlEnergyMeterHandler extends BaseThingHandler implemen
     private void restartCommunication(NikoHomeControlCommunication nhcComm) {
         // We lost connection but the connection object is there, so was correctly started.
         // Try to restart communication.
-        nhcComm.restartCommunication();
+        nhcComm.scheduleRestartCommunication();
         // If still not active, take thing offline and return.
         if (!nhcComm.communicationActive()) {
             updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR,

bundles/org.openhab.binding.nikohomecontrol/src/main/java/org/openhab/binding/nikohomecontrol/internal/handler/NikoHomeControlThermostatHandler.java

@@ -221,6 +221,19 @@ public class NikoHomeControlThermostatHandler extends BaseThingHandler implement
         initialized = true;
     }
 
+    @Override
+    public void dispose() {
+        NikoHomeControlCommunication nhcComm = getCommunication(getBridgeHandler());
+        if (nhcComm != null) {
+            NhcThermostat thermostat = nhcComm.getThermostats().get(thermostatId);
+            if (thermostat != null) {
+                thermostat.unsetEventHandler();
+            }
+        }
+        nhcThermostat = null;
+        super.dispose();
+    }
+
     private void updateProperties(NhcThermostat nhcThermostat) {
         Map<String, String> properties = new HashMap<>();
 
@@ -311,7 +324,7 @@ public class NikoHomeControlThermostatHandler extends BaseThingHandler implement
     private void restartCommunication(NikoHomeControlCommunication nhcComm) {
         // We lost connection but the connection object is there, so was correctly started.
         // Try to restart communication.
-        nhcComm.restartCommunication();
+        nhcComm.scheduleRestartCommunication();
         // If still not active, take thing offline and return.
         if (!nhcComm.communicationActive()) {
             updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR,

bundles/org.openhab.binding.nikohomecontrol/src/main/java/org/openhab/binding/nikohomecontrol/internal/protocol/NhcAction.java

@@ -65,6 +65,15 @@ public abstract class NhcAction {
         this.eventHandler = eventHandler;
     }
 
+    /**
+     * This method should be called when an object implementing the {@NhcActionEvent} interface is disposed.
+     * It resets the reference, so no updates go to the handler anymore.
+     *
+     */
+    public void unsetEventHandler() {
+        this.eventHandler = null;
+    }
+
     /**
      * Get id of action.
      *
@@ -189,6 +198,7 @@ public abstract class NhcAction {
         NhcActionEvent eventHandler = this.eventHandler;
         if (eventHandler != null) {
             eventHandler.actionRemoved();
+            unsetEventHandler();
         }
     }
 

bundles/org.openhab.binding.nikohomecontrol/src/main/java/org/openhab/binding/nikohomecontrol/internal/protocol/NhcControllerEvent.java

@@ -92,7 +92,7 @@ public interface NhcControllerEvent {
     public void noticeEvent(String noticeText);
 
     /**
-     * This method is called when properties are updated from from the Niko Home Control controller.
+     * This method is called when properties are updated from the Niko Home Control controller.
      */
     public void updatePropertiesEvent();
 }

bundles/org.openhab.binding.nikohomecontrol/src/main/java/org/openhab/binding/nikohomecontrol/internal/protocol/NhcEnergyMeter.java

@@ -71,6 +71,7 @@ public abstract class NhcEnergyMeter {
         NhcEnergyMeterEvent eventHandler = this.eventHandler;
         if (eventHandler != null) {
             eventHandler.energyMeterRemoved();
+            unsetEventHandler();
         }
     }
 
@@ -85,6 +86,15 @@ public abstract class NhcEnergyMeter {
         this.eventHandler = eventHandler;
     }
 
+    /**
+     * This method should be called when an object implementing the {@NhcEnergyMeterEvent} interface is disposed.
+     * It resets the reference, so no updates go to the handler anymore.
+     *
+     */
+    public void unsetEventHandler() {
+        this.eventHandler = null;
+    }
+
     /**
      * Get id of meter.
      *

bundles/org.openhab.binding.nikohomecontrol/src/main/java/org/openhab/binding/nikohomecontrol/internal/protocol/NhcThermostat.java

@@ -119,6 +119,7 @@ public abstract class NhcThermostat {
         NhcThermostatEvent eventHandler = this.eventHandler;
         if (eventHandler != null) {
             eventHandler.thermostatRemoved();
+            unsetEventHandler();
         }
     }
 
@@ -141,6 +142,15 @@ public abstract class NhcThermostat {
         this.eventHandler = eventHandler;
     }
 
+    /**
+     * This method should be called when an object implementing the {@NhcThermostatEvent} interface is disposed.
+     * It resets the reference, so no updates go to the handler anymore.
+     *
+     */
+    public void unsetEventHandler() {
+        this.eventHandler = null;
+    }
+
     /**
      * Get id of the thermostat.
      *

bundles/org.openhab.binding.nikohomecontrol/src/main/java/org/openhab/binding/nikohomecontrol/internal/protocol/NikoHomeControlCommunication.java

@@ -14,8 +14,12 @@ package org.openhab.binding.nikohomecontrol.internal.protocol;
 
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
+import org.eclipse.jdt.annotation.Nullable;
 import org.openhab.binding.nikohomecontrol.internal.protocol.nhc1.NikoHomeControlCommunication1;
 import org.openhab.binding.nikohomecontrol.internal.protocol.nhc2.NikoHomeControlCommunication2;
 import org.slf4j.Logger;
@@ -45,8 +49,16 @@ public abstract class NikoHomeControlCommunication {
 
     protected final NhcControllerEvent handler;
 
-    protected NikoHomeControlCommunication(NhcControllerEvent handler) {
+    protected final ScheduledExecutorService scheduler;
+
+    // restart attempts
+    private volatile int delay = 0;
+    private volatile int attempt = 0;
+    protected @Nullable ScheduledFuture<?> scheduledRestart = null;
+
+    protected NikoHomeControlCommunication(NhcControllerEvent handler, ScheduledExecutorService scheduler) {
         this.handler = handler;
+        this.scheduler = scheduler;
     }
 
     /**
@@ -57,21 +69,69 @@ public abstract class NikoHomeControlCommunication {
     /**
      * Stop Communication with Niko Home Control system.
      */
-    public abstract void stopCommunication();
+    public void stopCommunication() {
+        stopScheduledRestart();
+
+        resetCommunication();
+    }
+
+    /**
+     * Stop Communication with Niko Home Control system, but keep reconnection attempts going.
+     */
+    public abstract void resetCommunication();
+
+    protected synchronized void stopScheduledRestart() {
+        ScheduledFuture<?> future = scheduledRestart;
+        if (future != null) {
+            future.cancel(true);
+        }
+        scheduledRestart = null;
+        delay = 0;
+        attempt = 0;
+    }
 
     /**
      * Close and restart communication with Niko Home Control system.
      */
     public synchronized void restartCommunication() {
-        stopCommunication();
+        resetCommunication();
 
         logger.debug("restart communication from thread {}", Thread.currentThread().getId());
 
         startCommunication();
     }
 
+    private synchronized void checkAndRestartCommunication() {
+        restartCommunication();
+
+        // Try again if it didn't succeed
+        if (!communicationActive()) {
+            attempt++;
+            delay = ((attempt <= 5) ? 30 : 60);
+            logger.debug("schedule communication restart in {} seconds", delay);
+            scheduledRestart = scheduler.schedule(this::checkAndRestartCommunication, delay, TimeUnit.SECONDS);
+        } else {
+            stopScheduledRestart();
+        }
+    }
+
     /**
-     * Method to check if communication with Niko Home Control is active.
+     * Close and restart communication with Niko Home Control system. This method will keep doing multiple reconnection
+     * attempts, starting immediately, then 5 times with 30 second intervals and every minute thereafter until the
+     * connection is re-established.
+     */
+    public synchronized void scheduleRestartCommunication() {
+        // Don't do this if we already scheduled to restart
+        if (scheduledRestart == null) {
+            delay = 0;
+            attempt = 0;
+            scheduledRestart = scheduler.schedule(this::checkAndRestartCommunication, 0, TimeUnit.SECONDS);
+        }
+    }
+
+    /**
+     * Method to check if communication with Niko Home Control is active. This method can be blocking for max 5s to wait
+     * for completion of startup.
      *
      * @return True if active
      */

bundles/org.openhab.binding.nikohomecontrol/src/main/java/org/openhab/binding/nikohomecontrol/internal/protocol/nhc1/NikoHomeControlCommunication1.java

@@ -70,8 +70,6 @@ public class NikoHomeControlCommunication1 extends NikoHomeControlCommunication
     private volatile boolean listenerStopped;
     private volatile boolean nhcEventsRunning;
 
-    private ScheduledExecutorService scheduler;
-
     // We keep only 2 gson adapters used to serialize and deserialize all messages sent and received
     protected final Gson gsonOut = new Gson();
     protected Gson gsonIn;
@@ -83,8 +81,7 @@ public class NikoHomeControlCommunication1 extends NikoHomeControlCommunication
      */
     public NikoHomeControlCommunication1(NhcControllerEvent handler, ScheduledExecutorService scheduler,
             String eventThreadName) {
-        super(handler);
-        this.scheduler = scheduler;
+        super(handler, scheduler);
         this.eventThreadName = eventThreadName;
 
         // When we set up this object, we want to get the proper gson adapter set up once
@@ -121,9 +118,12 @@ public class NikoHomeControlCommunication1 extends NikoHomeControlCommunication
             // IP-interface.
             (new Thread(this::runNhcEvents, eventThreadName)).start();
 
-        } catch (IOException | InterruptedException e) {
-            stopCommunication();
+            handler.controllerOnline();
+        } catch (InterruptedException e) {
             handler.controllerOffline("@text/offline.communication-error");
+        } catch (IOException e) {
+            handler.controllerOffline("@text/offline.communication-error");
+            scheduleRestartCommunication();
         }
     }
 
@@ -132,7 +132,7 @@ public class NikoHomeControlCommunication1 extends NikoHomeControlCommunication
      *
      */
     @Override
-    public synchronized void stopCommunication() {
+    public synchronized void resetCommunication() {
         listenerStopped = true;
 
         Socket socket = nhcSocket;
@@ -181,7 +181,7 @@ public class NikoHomeControlCommunication1 extends NikoHomeControlCommunication
                 // this is a socket error, not a communication stop triggered from outside this runnable
                 logger.debug("IO error in listener");
                 // the IO has stopped working, so we need to close cleanly and try to restart
-                restartCommunication();
+                scheduleRestartCommunication();
                 return;
             }
         } finally {
@@ -213,10 +213,12 @@ public class NikoHomeControlCommunication1 extends NikoHomeControlCommunication
         sendAndReadMessage("getalarms");
     }
 
-    @SuppressWarnings("null")
     private void sendAndReadMessage(String command) throws IOException {
-        sendMessage(new NhcMessageCmd1(command));
-        readMessage(nhcIn.readLine());
+        BufferedReader in = nhcIn;
+        if (in != null) {
+            sendMessage(new NhcMessageCmd1(command));
+            readMessage(in.readLine());
+        }
     }
 
     /**
@@ -224,19 +226,23 @@ public class NikoHomeControlCommunication1 extends NikoHomeControlCommunication
      *
      * @param nhcMessage
      */
-    @SuppressWarnings("null")
     private synchronized void sendMessage(Object nhcMessage) {
         String json = gsonOut.toJson(nhcMessage);
-        logger.debug("send json {}", json);
-        nhcOut.println(json);
-        if (nhcOut.checkError()) {
-            logger.debug("error sending message, trying to restart communication");
-            restartCommunication();
-            // retry sending after restart
-            logger.debug("resend json {}", json);
-            nhcOut.println(json);
-            if (nhcOut.checkError()) {
-                handler.controllerOffline("@text/offline.communication-error");
+        PrintWriter out = nhcOut;
+        if (out != null) {
+            logger.debug("send json {}", json);
+            out.println(json);
+            if (out.checkError()) {
+                logger.debug("error sending message, trying to restart communication");
+                restartCommunication();
+                // retry sending after restart
+                logger.debug("resend json {}", json);
+                out.println(json);
+                if (out.checkError()) {
+                    handler.controllerOffline("@text/offline.communication-error");
+                    // Keep on trying to restart, but don't send message anymore
+                    scheduleRestartCommunication();
+                }
             }
         }
     }

bundles/org.openhab.binding.nikohomecontrol/src/main/java/org/openhab/binding/nikohomecontrol/internal/protocol/nhc2/NhcMqttConnection2.java

@@ -39,6 +39,7 @@ import org.openhab.core.io.transport.mqtt.MqttConnectionObserver;
 import org.openhab.core.io.transport.mqtt.MqttConnectionState;
 import org.openhab.core.io.transport.mqtt.MqttException;
 import org.openhab.core.io.transport.mqtt.MqttMessageSubscriber;
+import org.openhab.core.io.transport.mqtt.reconnect.AbstractReconnectStrategy;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -162,6 +163,13 @@ public class NhcMqttConnection2 implements MqttActionCallback {
         connection.setTrustManagers(trustManagers);
         connection.setCredentials(profile, token);
         connection.setQos(1);
+
+        // Don't use the transport periodic reconnect strategy. It doesn't restart the initialization when the
+        // connection is lost and creates extra threads that do not get cleaned up. Just stop it.
+        AbstractReconnectStrategy reconnectStrategy = connection.getReconnectStrategy();
+        if (reconnectStrategy != null) {
+            reconnectStrategy.stop();
+        }
         return connection;
     }
 

bundles/org.openhab.binding.nikohomecontrol/src/main/java/org/openhab/binding/nikohomecontrol/internal/protocol/nhc2/NikoHomeControlCommunication2.java

@@ -82,10 +82,9 @@ public class NikoHomeControlCommunication2 extends NikoHomeControlCommunication
     private volatile @Nullable NhcSystemInfo2 nhcSystemInfo;
     private volatile @Nullable NhcTimeInfo2 nhcTimeInfo;
 
+    private volatile boolean initStarted = false;
     private volatile @Nullable CompletableFuture<Boolean> communicationStarted;
 
-    private ScheduledExecutorService scheduler;
-
     private final Gson gson = new GsonBuilder().setFieldNamingPolicy(FieldNamingPolicy.UPPER_CAMEL_CASE).create();
 
     /**
@@ -98,13 +97,13 @@ public class NikoHomeControlCommunication2 extends NikoHomeControlCommunication
      */
     public NikoHomeControlCommunication2(NhcControllerEvent handler, String clientId,
             ScheduledExecutorService scheduler) throws CertificateException {
-        super(handler);
+        super(handler, scheduler);
         mqttConnection = new NhcMqttConnection2(clientId, this, this);
-        this.scheduler = scheduler;
     }
 
     @Override
     public synchronized void startCommunication() {
+        initStarted = false;
         communicationStarted = new CompletableFuture<>();
 
         InetAddress addr = handler.getAddr();
@@ -128,20 +127,22 @@ public class NikoHomeControlCommunication2 extends NikoHomeControlCommunication
 
         try {
             mqttConnection.startConnection(addrString, port, profile, token);
-            initialize();
         } catch (MqttException e) {
             logger.debug("error in mqtt communication");
-            stopCommunication();
+            handler.controllerOffline("@text/offline.communication-error");
+            scheduleRestartCommunication();
         }
     }
 
     @Override
-    public synchronized void stopCommunication() {
+    public synchronized void resetCommunication() {
         CompletableFuture<Boolean> started = communicationStarted;
         if (started != null) {
             started.complete(false);
         }
         communicationStarted = null;
+        initStarted = false;
+
         mqttConnection.stopConnection();
     }
 
@@ -165,25 +166,33 @@ public class NikoHomeControlCommunication2 extends NikoHomeControlCommunication
      * messages.
      *
      */
-    private void initialize() throws MqttException {
+    private synchronized void initialize() {
+        initStarted = true;
+
         NhcMessage2 message = new NhcMessage2();
 
-        message.method = "systeminfo.publish";
-        mqttConnection.connectionPublish(profile + "/system/cmd", gson.toJson(message));
+        try {
+            message.method = "systeminfo.publish";
+            mqttConnection.connectionPublish(profile + "/system/cmd", gson.toJson(message));
 
-        message.method = "services.list";
-        mqttConnection.connectionPublish(profile + "/authentication/cmd", gson.toJson(message));
+            message.method = "services.list";
+            mqttConnection.connectionPublish(profile + "/authentication/cmd", gson.toJson(message));
 
-        message.method = "devices.list";
-        mqttConnection.connectionPublish(profile + "/control/devices/cmd", gson.toJson(message));
+            message.method = "devices.list";
+            mqttConnection.connectionPublish(profile + "/control/devices/cmd", gson.toJson(message));
 
-        message.method = "notifications.list";
-        mqttConnection.connectionPublish(profile + "/notification/cmd", gson.toJson(message));
+            message.method = "notifications.list";
+            mqttConnection.connectionPublish(profile + "/notification/cmd", gson.toJson(message));
+        } catch (MqttException e) {
+            initStarted = false;
+            logger.debug("error in mqtt communication during initialization");
+            resetCommunication();
+        }
     }
 
     private void connectionLost(String message) {
         logger.debug("connection lost");
-        stopCommunication();
+        resetCommunication();
         handler.controllerOffline(message);
     }
 
@@ -832,6 +841,8 @@ public class NikoHomeControlCommunication2 extends NikoHomeControlCommunication
             if (!communicationActive()) {
                 message = (message != null) ? message : "@text/offline.communication-error";
                 connectionLost(message);
+                // Keep on trying to restart, but don't send message anymore
+                scheduleRestartCommunication();
             }
         }
     }
@@ -894,19 +905,19 @@ public class NikoHomeControlCommunication2 extends NikoHomeControlCommunication
 
     @Override
     public void connectionStateChanged(MqttConnectionState state, @Nullable Throwable error) {
-        if (error != null) {
-            logger.debug("Connection state: {}, error", state, error);
-            String message = error.getLocalizedMessage();
-            message = (message != null) ? message : "@text/offline.communication-error";
-            if (!MqttConnectionState.CONNECTING.equals(state)) {
-                // This is a connection loss, try to restart
-                restartCommunication();
-            }
-            if (!communicationActive()) {
+        // do in separate thread as this method needs to return early
+        scheduler.submit(() -> {
+            if (error != null) {
+                logger.debug("Connection state: {}, error", state, error);
+                String localizedMessage = error.getLocalizedMessage();
+                String message = (localizedMessage != null) ? localizedMessage : "@text/offline.communication-error";
                 connectionLost(message);
+                scheduleRestartCommunication();
+            } else if ((state == MqttConnectionState.CONNECTED) && !initStarted) {
+                initialize();
+            } else {
+                logger.trace("Connection state: {}", state);
             }
-        } else {
-            logger.trace("Connection state: {}", state);
-        }
+        });
     }
 }

bundles/org.openhab.binding.nikohomecontrol/src/main/resources/OH-INF/i18n/nikohomecontrol.properties

@@ -24,8 +24,8 @@ bridge2ConfigPasswordLabel = API Token
 bridge2ConfigPasswordDescription = Token for Niko Home Control II hobby API, should not be empty. This token will have to be renewed after expiration (1 year after creation)
 
 bridgeConfigRefreshLabel = Refresh Interval
-bridgeConfigRefreshDescription = Refresh interval for connection with Niko Home Control IP-interface (min), default 300. If set to 0 or left empty, no refresh will be scheduled
-bridge2ConfigRefreshDescription = Refresh interval for connection with Connected Controller or Wireless Smart Hub (min), default 300. If set to 0 or left empty, no refresh will be scheduled
+bridgeConfigRefreshDescription = Refresh interval for connection with Niko Home Control IP-interface (min), default 300. If set to 0, no refresh will be scheduled
+bridge2ConfigRefreshDescription = Refresh interval for connection with Connected Controller or Wireless Smart Hub (min), default 300. If set to 0, no refresh will be scheduled
 
 # thing types
 pushButtonLabel = Pushbutton