[danfossairunit] Fix network reliability issues and setting of all ch…

…annel values to zero (openhab#11172) * Fix Potential null pointer accesses * Added constants for TCP port and poll interval in seconds. * Extract interface from DanfossAirUnitCommunicationController. * Remove unused constants which seems to be left-overs from skeleton. * Added constant for discovery timeout value for readability. * Created handler subfolder for DanfossAirUnitHandler (like discovery) for consistency with other bindings. * Handle lost connection gracefully without updating all channels to zero. * Fix infinitly blocking network calls preventing proper error handling. * Fix thing status being reset to ONLINE after failing to update all channels. * Fix error handling when receiving invalid manual fan step. Fixes openhab#11167 Fixes openhab#11188 Signed-off-by: Jacob Laursen <[email protected]>
bosch-io · Sep 27, 2021 · 68d33ae · 68d33ae
1 parent d1f7029
commit 68d33ae
Show file tree

Hide file tree

Showing 8 changed files with 299 additions and 97 deletions.
diff --git a/...it/src/main/java/org/openhab/binding/danfossairunit/internal/CommunicationController.java b/...it/src/main/java/org/openhab/binding/danfossairunit/internal/CommunicationController.java
@@ -0,0 +1,33 @@
+/**
+ * Copyright (c) 2010-2021 Contributors to the openHAB project
+ *
+ * See the NOTICE file(s) distributed with this work for additional
+ * information.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License 2.0 which is available at
+ * http://www.eclipse.org/legal/epl-2.0
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ */
+package org.openhab.binding.danfossairunit.internal;
+
+import java.io.IOException;
+
+import org.eclipse.jdt.annotation.NonNullByDefault;
+
+/**
+ * This interface defines a communication controller that can be used to send requests to the Danfoss Air Unit.
+ * 
+ * @author Jacob Laursen - Refactoring, bugfixes and enhancements
+ */
+@NonNullByDefault
+public interface CommunicationController {
+    void connect() throws IOException;
+
+    void disconnect();
+
+    byte[] sendRobustRequest(byte[] operation, byte[] register) throws IOException;
+
+    byte[] sendRobustRequest(byte[] operation, byte[] register, byte[] value) throws IOException;
+}
diff --git a/...fossairunit/src/main/java/org/openhab/binding/danfossairunit/internal/DanfossAirUnit.java b/...fossairunit/src/main/java/org/openhab/binding/danfossairunit/internal/DanfossAirUnit.java
@@ -17,7 +17,6 @@
 import java.io.IOException;
 import java.math.BigDecimal;
 import java.math.RoundingMode;
-import java.net.InetAddress;
 import java.nio.charset.StandardCharsets;
 import java.time.DateTimeException;
 import java.time.ZoneId;
@@ -43,30 +42,22 @@
  * 
  * @author Ralf Duckstein - Initial contribution
  * @author Robert Bach - heavy refactorings
+ * @author Jacob Laursen - Refactoring, bugfixes and enhancements
  */
 
-@SuppressWarnings("SameParameterValue")
 @NonNullByDefault
 public class DanfossAirUnit {
 
-    private final DanfossAirUnitCommunicationController communicationController;
+    private final CommunicationController communicationController;
 
-    public DanfossAirUnit(InetAddress inetAddr, int port) {
-        this.communicationController = new DanfossAirUnitCommunicationController(inetAddr, port);
-    }
-
-    public void cleanUp() {
-        this.communicationController.disconnect();
+    public DanfossAirUnit(CommunicationController communicationController) {
+        this.communicationController = communicationController;
     }
 
     private boolean getBoolean(byte[] operation, byte[] register) throws IOException {
         return communicationController.sendRobustRequest(operation, register)[0] != 0;
     }
 
-    private void setSetting(byte[] register, boolean value) throws IOException {
-        setSetting(register, value ? (byte) 1 : (byte) 0);
-    }
-
     private short getWord(byte[] operation, byte[] register) throws IOException {
         byte[] resultBytes = communicationController.sendRobustRequest(operation, register);
         return (short) ((resultBytes[0] << 8) | (resultBytes[1] & 0xFF));
@@ -87,14 +78,6 @@ private void set(byte[] operation, byte[] register, byte value) throws IOExcepti
         communicationController.sendRobustRequest(operation, register, valueArray);
     }
 
-    private void set(byte[] operation, byte[] register, short value) throws IOException {
-        communicationController.sendRobustRequest(operation, register, shortToBytes(value));
-    }
-
-    private byte[] shortToBytes(short s) {
-        return new byte[] { (byte) ((s & 0xFF00) >> 8), (byte) (s & 0x00FF) };
-    }
-
     private short getShort(byte[] operation, byte[] register) throws IOException {
         byte[] result = communicationController.sendRobustRequest(operation, register);
         return (short) ((result[0] << 8) + (result[1] & 0xff));
@@ -141,14 +124,6 @@ private static float asPercentByte(byte b) {
         return f * 100 / 255;
     }
 
-    private void setSetting(byte[] register, short value) throws IOException {
-        byte[] valueArray = new byte[2];
-        valueArray[0] = (byte) (value >> 8);
-        valueArray[1] = (byte) value;
-
-        communicationController.sendRobustRequest(REGISTER_1_WRITE, register, valueArray);
-    }
-
     public String getUnitName() throws IOException {
         return getString(REGISTER_1_READ, UNIT_NAME);
     }
@@ -161,8 +136,12 @@ public StringType getMode() throws IOException {
         return new StringType(Mode.values()[getByte(REGISTER_1_READ, MODE)].name());
     }
 
-    public PercentType getManualFanStep() throws IOException {
-        return new PercentType(BigDecimal.valueOf(getByte(REGISTER_1_READ, MANUAL_FAN_SPEED_STEP) * 10));
+    public PercentType getManualFanStep() throws IOException, UnexpectedResponseValueException {
+        byte value = getByte(REGISTER_1_READ, MANUAL_FAN_SPEED_STEP);
+        if (value < 0 || value > 10) {
+            throw new UnexpectedResponseValueException(String.format("Invalid fan step: %d", value));
+        }
+        return new PercentType(BigDecimal.valueOf(value * 10));
     }
 
     public DecimalType getSupplyFanSpeed() throws IOException {

diff --git a/...main/java/org/openhab/binding/danfossairunit/internal/DanfossAirUnitBindingConstants.java b/...main/java/org/openhab/binding/danfossairunit/internal/DanfossAirUnitBindingConstants.java
@@ -30,12 +30,6 @@ public class DanfossAirUnitBindingConstants {
 
     public static String BINDING_ID = "danfossairunit";
 
-    // List of all Thing Type UIDs
-    public static ThingTypeUID THING_TYPE_SAMPLE = new ThingTypeUID(BINDING_ID, "sample");
-
-    // List of all Channel ids
-    public static String CHANNEL_1 = "channel1";
-
     // The only thing type UIDs
     public static ThingTypeUID THING_TYPE_AIRUNIT = new ThingTypeUID(BINDING_ID, "airunit");
 

diff --git a/...va/org/openhab/binding/danfossairunit/internal/DanfossAirUnitCommunicationController.java b/...va/org/openhab/binding/danfossairunit/internal/DanfossAirUnitCommunicationController.java
@@ -30,19 +30,22 @@
  * The {@link DanfossAirUnitCommunicationController} class does the actual network communication with the air unit.
  *
  * @author Robert Bach - initial contribution
+ * @author Jacob Laursen - Refactoring, bugfixes and enhancements
  */
 
 @NonNullByDefault
-public class DanfossAirUnitCommunicationController {
+public class DanfossAirUnitCommunicationController implements CommunicationController {
+
+    private static final int SOCKET_TIMEOUT_MILLISECONDS = 5_000;
 
     private final Logger logger = LoggerFactory.getLogger(DanfossAirUnitCommunicationController.class);
 
     private final InetAddress inetAddr;
     private final int port;
     private boolean connected = false;
     private @Nullable Socket socket;
-    private @Nullable OutputStream oStream;
-    private @Nullable InputStream iStream;
+    private @Nullable OutputStream outputStream;
+    private @Nullable InputStream inputStream;
 
     public DanfossAirUnitCommunicationController(InetAddress inetAddr, int port) {
         this.inetAddr = inetAddr;
@@ -53,9 +56,11 @@ public synchronized void connect() throws IOException {
         if (connected) {
             return;
         }
-        socket = new Socket(inetAddr, port);
-        oStream = socket.getOutputStream();
-        iStream = socket.getInputStream();
+        Socket localSocket = new Socket(inetAddr, port);
+        localSocket.setSoTimeout(SOCKET_TIMEOUT_MILLISECONDS);
+        this.outputStream = localSocket.getOutputStream();
+        this.inputStream = localSocket.getInputStream();
+        this.socket = localSocket;
         connected = true;
     }
 
@@ -64,15 +69,16 @@ public synchronized void disconnect() {
             return;
         }
         try {
-            if (socket != null) {
-                socket.close();
+            Socket localSocket = this.socket;
+            if (localSocket != null) {
+                localSocket.close();
             }
         } catch (IOException ioe) {
             logger.debug("Connection to air unit could not be closed gracefully. {}", ioe.getMessage());
         } finally {
-            socket = null;
-            iStream = null;
-            oStream = null;
+            this.socket = null;
+            this.inputStream = null;
+            this.outputStream = null;
         }
         connected = false;
     }
@@ -98,21 +104,27 @@ public synchronized byte[] sendRobustRequest(byte[] operation, byte[] register,
     }
 
     private synchronized byte[] sendRequestInternal(byte[] request) throws IOException {
+        OutputStream localOutputStream = this.outputStream;
 
-        if (oStream == null) {
+        if (localOutputStream == null) {
             throw new IOException(
                     String.format("Output stream is null while sending request: %s", Arrays.toString(request)));
         }
-        oStream.write(request);
-        oStream.flush();
+        localOutputStream.write(request);
+        localOutputStream.flush();
 
         byte[] result = new byte[63];
-        if (iStream == null) {
+        InputStream localInputStream = this.inputStream;
+        if (localInputStream == null) {
             throw new IOException(
                     String.format("Input stream is null while sending request: %s", Arrays.toString(request)));
         }
-        // noinspection ResultOfMethodCallIgnored
-        iStream.read(result, 0, 63);
+
+        int bytesRead = localInputStream.read(result, 0, 63);
+        if (bytesRead < 63) {
+            throw new IOException(String.format(
+                    "Error reading from stream, read returned %d as number of bytes read into the buffer", bytesRead));
+        }
 
         return result;
     }

diff --git a/....danfossairunit/src/main/java/org/openhab/binding/danfossairunit/internal/ValueCache.java b/....danfossairunit/src/main/java/org/openhab/binding/danfossairunit/internal/ValueCache.java
@@ -57,7 +57,6 @@ public boolean updateValue(String channelId, State newState) {
         return writeToCache;
     }
 
-    @NonNullByDefault
     private static class StateWithTimestamp {
         State state;
         long timestamp;

diff --git a/...org/openhab/binding/danfossairunit/internal/discovery/DanfossAirUnitDiscoveryService.java b/...org/openhab/binding/danfossairunit/internal/discovery/DanfossAirUnitDiscoveryService.java
@@ -51,11 +51,12 @@ public class DanfossAirUnitDiscoveryService extends AbstractDiscoveryService {
     private static final int BROADCAST_PORT = 30045;
     private static final byte[] DISCOVER_SEND = { 0x0c, 0x00, 0x30, 0x00, 0x11, 0x00, 0x12, 0x00, 0x13 };
     private static final byte[] DISCOVER_RECEIVE = { 0x0d, 0x00, 0x07, 0x00, 0x02, 0x02, 0x00 };
+    private static final int TIMEOUT_IN_SECONDS = 15;
 
     private final Logger logger = LoggerFactory.getLogger(DanfossAirUnitDiscoveryService.class);
 
     public DanfossAirUnitDiscoveryService() {
-        super(SUPPORTED_THING_TYPES_UIDS, 15, true);
+        super(SUPPORTED_THING_TYPES_UIDS, TIMEOUT_IN_SECONDS, true);
     }
 
     @Override