[caddx] Fix wrongly handled discovery, off by 1 errors (#9030)

* Corrected caddx discovery bug
* Maintenance

- Changed refresh logic for the things
- Code cleanup
- Corrected off by one errors
- Added initial tests for the message parsing
* Corrected ParameterizedTest
* Changed new Date().getTime to System.currentTimeMillis()

Signed-off-by: Georgios Moutsos <georgios.moutsos@gmail.com>

bundles/org.openhab.binding.caddx/README.md

@@ -67,6 +67,7 @@ The following table shows the available configuration parameters for each thing.
 | bridge    | `serialPort` - Serial port for the bridge - Required                                           |
 |           | `protocol` - Protocol used for the communication (Binary, Ascii) - Required - Default = Binary |
 |           | `baud` - Baud rate of the bridge - Required - Default = 9600                                   |
+|           | `maxZoneNumber` - Maximum zone number to be added during discovery - Required - Default = 16   |
 | partition | `partitionNumber` - Partition number (1-8) - Required                                          |
 | zone      | `zoneNumber` - Zone number (1-192) - Required                                                  |
 | keypad    | `keypadAddress` - Keypad address (192-255) - Required                                          |
@@ -219,7 +220,7 @@ Caddx Alarm things support a variety of channels as seen below in the following
 The following is an example of a things file (caddx.things):
 
 ```
-Bridge caddx:bridge:thebridge  "Bridge"                   [ protocol="Binary", serialPort="/dev/ttyUSB0", baudrate=38400 ] {
+Bridge caddx:bridge:thebridge  "Bridge"                   [ protocol="Binary", serialPort="/dev/ttyUSB0", baud=38400, maxZoneNumber=18 ] {
     Thing partition partition1 "Groundfloor alarm"        [ partitionNumber=1 ]
     Thing zone      zone1      "Livingroom motion sensor" [ zoneNumber=1 ]
     Thing zone      zone2      "Bedroom motion sensor"    [ zoneNumber=2 ]

bundles/org.openhab.binding.caddx/src/main/java/org/openhab/binding/caddx/internal/CaddxMessage.java

@@ -169,8 +169,14 @@ public class CaddxMessage {
         switch (caddxMessageType) {
             case ZONE_STATUS_REQUEST:
             case ZONE_STATUS_MESSAGE:
+                String zone;
+                try {
+                    zone = "" + (Integer.parseInt(getPropertyById("zone_number")) + 1);
+                } catch (NumberFormatException e) {
+                    zone = "";
+                }
                 sb.append(" [Zone: ");
-                sb.append(getPropertyById("zone_number"));
+                sb.append(zone);
                 sb.append("]");
                 break;
             case LOG_EVENT_REQUEST:
@@ -181,8 +187,14 @@ public class CaddxMessage {
                 break;
             case PARTITION_STATUS_REQUEST:
             case PARTITION_STATUS_MESSAGE:
+                String partition;
+                try {
+                    partition = "" + (Integer.parseInt(getPropertyById("partition_number")) + 1);
+                } catch (NumberFormatException e) {
+                    partition = "";
+                }
                 sb.append(" [Partition: ");
-                sb.append(getPropertyById("partition_number"));
+                sb.append(partition);
                 sb.append("]");
                 break;
             default:

bundles/org.openhab.binding.caddx/src/main/java/org/openhab/binding/caddx/internal/CaddxProperty.java

@@ -83,7 +83,7 @@ public class CaddxProperty {
 
                 return Integer.toString(val);
             case STRING:
-                byte[] str = Arrays.copyOfRange(message, byteFrom - 1, byteFrom + byteLength);
+                byte[] str = Arrays.copyOfRange(message, byteFrom - 1, byteFrom + byteLength - 1);
                 return mapCaddxString(new String(str, StandardCharsets.US_ASCII));
             case BIT:
                 return (((message[byteFrom - 1] & (1 << bitFrom)) > 0) ? "true" : "false");

bundles/org.openhab.binding.caddx/src/main/java/org/openhab/binding/caddx/internal/config/CaddxBridgeConfiguration.java

@@ -29,10 +29,12 @@ public class CaddxBridgeConfiguration {
     public static final String PROTOCOL = "protocol";
     public static final String SERIAL_PORT = "serialPort";
     public static final String BAUD = "baud";
+    public static final String MAX_ZONE_NUMBER = "maxZoneNumber";
 
     private CaddxProtocol protocol = CaddxProtocol.Binary;
     private @Nullable String serialPort;
     private int baudrate = 9600;
+    private int maxZoneNumber = 16;
 
     public CaddxProtocol getProtocol() {
         return protocol;
@@ -45,4 +47,8 @@ public class CaddxBridgeConfiguration {
     public int getBaudrate() {
         return baudrate;
     }
+
+    public int getMaxZoneNumber() {
+        return maxZoneNumber;
+    }
 }

bundles/org.openhab.binding.caddx/src/main/java/org/openhab/binding/caddx/internal/handler/CaddxBridgeHandler.java

@@ -64,9 +64,18 @@ public class CaddxBridgeHandler extends BaseBridgeHandler implements CaddxPanelL
     private final Logger logger = LoggerFactory.getLogger(CaddxBridgeHandler.class);
 
     static final byte[] DISCOVERY_PARTITION_STATUS_REQUEST_0 = { 0x26, 0x00 };
-    static final byte[] DISCOVERY_ZONES_SNAPSHOT_REQUEST_00 = { 0x25, 0x00 };
+    static final byte[] DISCOVERY_ZONES_SNAPSHOT_REQUEST_00 = { 0x25, 0x00 }; // 1 - 16
-    static final byte[] DISCOVERY_ZONES_SNAPSHOT_REQUEST_10 = { 0x25, 0x10 };
+    static final byte[] DISCOVERY_ZONES_SNAPSHOT_REQUEST_10 = { 0x25, 0x01 }; // 17 - 32
-    static final byte[] DISCOVERY_ZONES_SNAPSHOT_REQUEST_20 = { 0x25, 0x20 };
+    static final byte[] DISCOVERY_ZONES_SNAPSHOT_REQUEST_20 = { 0x25, 0x02 }; // 33 - 48
+    static final byte[] DISCOVERY_ZONES_SNAPSHOT_REQUEST_30 = { 0x25, 0x03 }; // 49 - 64
+    static final byte[] DISCOVERY_ZONES_SNAPSHOT_REQUEST_40 = { 0x25, 0x04 }; // 65 - 80
+    static final byte[] DISCOVERY_ZONES_SNAPSHOT_REQUEST_50 = { 0x25, 0x05 }; // 81 - 96
+    static final byte[] DISCOVERY_ZONES_SNAPSHOT_REQUEST_60 = { 0x25, 0x06 }; // 97 - 112
+    static final byte[] DISCOVERY_ZONES_SNAPSHOT_REQUEST_70 = { 0x25, 0x07 }; // 113 - 64
+    static final byte[] DISCOVERY_ZONES_SNAPSHOT_REQUEST_80 = { 0x25, 0x08 }; // 129 - 144
+    static final byte[] DISCOVERY_ZONES_SNAPSHOT_REQUEST_90 = { 0x25, 0x09 }; // 145 - 160
+    static final byte[] DISCOVERY_ZONES_SNAPSHOT_REQUEST_A0 = { 0x25, 0x0A }; // 161 - 176
+    static final byte[] DISCOVERY_ZONES_SNAPSHOT_REQUEST_B0 = { 0x25, 0x0B }; // 177 - 192
     static final byte[] DISCOVERY_PARTITIONS_SNAPSHOT_REQUEST = { 0x27 };
 
     private final SerialPortManager portManager;
@@ -74,6 +83,7 @@ public class CaddxBridgeHandler extends BaseBridgeHandler implements CaddxPanelL
     private CaddxProtocol protocol = CaddxProtocol.Binary;
     private String serialPortName = "";
     private int baudRate;
+    private int maxZoneNumber;
     private @Nullable CaddxCommunicator communicator = null;
 
     // Things served by the bridge
@@ -90,11 +100,6 @@ public class CaddxBridgeHandler extends BaseBridgeHandler implements CaddxPanelL
         this.discoveryService = discoveryService;
     }
 
-    /**
-     * Constructor.
-     *
-     * @param bridge
-     */
     public CaddxBridgeHandler(SerialPortManager portManager, Bridge bridge) {
         super(bridge);
 
@@ -113,6 +118,7 @@ public class CaddxBridgeHandler extends BaseBridgeHandler implements CaddxPanelL
         serialPortName = portName;
         protocol = configuration.getProtocol();
         baudRate = configuration.getBaudrate();
+        maxZoneNumber = configuration.getMaxZoneNumber();
         updateStatus(ThingStatus.OFFLINE);
 
         // create & start panel interface
@@ -132,10 +138,21 @@ public class CaddxBridgeHandler extends BaseBridgeHandler implements CaddxPanelL
         if (comm != null) {
             comm.addListener(this);
 
-            // Send discovery commands for the things
+            // Send discovery commands for the zones
             comm.transmit(new CaddxMessage(DISCOVERY_ZONES_SNAPSHOT_REQUEST_00, false));
             comm.transmit(new CaddxMessage(DISCOVERY_ZONES_SNAPSHOT_REQUEST_10, false));
             comm.transmit(new CaddxMessage(DISCOVERY_ZONES_SNAPSHOT_REQUEST_20, false));
+            comm.transmit(new CaddxMessage(DISCOVERY_ZONES_SNAPSHOT_REQUEST_30, false));
+            comm.transmit(new CaddxMessage(DISCOVERY_ZONES_SNAPSHOT_REQUEST_40, false));
+            comm.transmit(new CaddxMessage(DISCOVERY_ZONES_SNAPSHOT_REQUEST_50, false));
+            comm.transmit(new CaddxMessage(DISCOVERY_ZONES_SNAPSHOT_REQUEST_60, false));
+            comm.transmit(new CaddxMessage(DISCOVERY_ZONES_SNAPSHOT_REQUEST_70, false));
+            comm.transmit(new CaddxMessage(DISCOVERY_ZONES_SNAPSHOT_REQUEST_80, false));
+            comm.transmit(new CaddxMessage(DISCOVERY_ZONES_SNAPSHOT_REQUEST_90, false));
+            comm.transmit(new CaddxMessage(DISCOVERY_ZONES_SNAPSHOT_REQUEST_A0, false));
+            comm.transmit(new CaddxMessage(DISCOVERY_ZONES_SNAPSHOT_REQUEST_B0, false));
+
+            // Send discovery commands for the partitions
             comm.transmit(new CaddxMessage(DISCOVERY_PARTITION_STATUS_REQUEST_0, false));
             comm.transmit(new CaddxMessage(DISCOVERY_PARTITIONS_SNAPSHOT_REQUEST, false));
         }
@@ -348,9 +365,9 @@ public class CaddxBridgeHandler extends BaseBridgeHandler implements CaddxPanelL
                         }
                         break;
                     case ZONES_SNAPSHOT_MESSAGE:
-                        int zoneOffset = Integer.parseInt(caddxMessage.getPropertyById("zone_offset"));
+                        int zoneOffset = Integer.parseInt(caddxMessage.getPropertyById("zone_offset")) * 16;
                         for (int i = 1; i <= 16; i++) {
-                            if (caddxMessage.getPropertyById("zone_" + i + "_trouble").equals("false")) {
+                            if (zoneOffset + i <= maxZoneNumber) {
                                 thing = findThing(CaddxThingType.ZONE, null, zoneOffset + i, null);
                                 if (thing != null) {
                                     continue;
@@ -358,8 +375,6 @@ public class CaddxBridgeHandler extends BaseBridgeHandler implements CaddxPanelL
 
                                 event = new CaddxEvent(caddxMessage, null, zoneOffset + i, null);
                                 discoveryService.addThing(getThing(), CaddxThingType.ZONE, event);
-                            } else {
-                                logger.debug("troubled zone: {}", zoneOffset + i);
                             }
                         }
                         break;

bundles/org.openhab.binding.caddx/src/main/java/org/openhab/binding/caddx/internal/handler/ThingHandlerPanel.java

@@ -41,12 +41,8 @@ public class ThingHandlerPanel extends CaddxBaseThingHandler {
     private final Logger logger = LoggerFactory.getLogger(ThingHandlerPanel.class);
     private @Nullable HashMap<String, String> panelLogMessagesMap = null;
     private @Nullable String communicatorStackPointer = null;
+    private long lastRefreshTime = 0;
 
-    /**
-     * Constructor.
-     *
-     * @param thing
-     */
     public ThingHandlerPanel(Thing thing) {
         super(thing, CaddxThingType.PANEL);
     }
@@ -77,17 +73,19 @@ public class ThingHandlerPanel extends CaddxBaseThingHandler {
         }
 
         if (command instanceof RefreshType) {
-            if (CaddxBindingConstants.PANEL_FIRMWARE_VERSION.equals(channelUID.getId())) {
+            if (CaddxBindingConstants.PANEL_LOG_MESSAGE_N_0.equals(channelUID.getId())) {
-                cmd = CaddxBindingConstants.PANEL_INTERFACE_CONFIGURATION_REQUEST;
-                data = "";
-            } else if (CaddxBindingConstants.PANEL_LOG_MESSAGE_N_0.equals(channelUID.getId())) {
                 cmd = CaddxBindingConstants.PANEL_SYSTEM_STATUS_REQUEST;
                 data = "";
+            } else if (System.currentTimeMillis() - lastRefreshTime > 2000) {
+                // Refresh only if 2 seconds have passed from the last refresh
+                cmd = CaddxBindingConstants.PANEL_INTERFACE_CONFIGURATION_REQUEST;
+                data = "";
             } else {
                 return;
             }
 
             bridgeHandler.sendCommand(cmd, data);
+            lastRefreshTime = System.currentTimeMillis();
         } else {
             logger.debug("Unknown command {}", command);
         }

bundles/org.openhab.binding.caddx/src/main/java/org/openhab/binding/caddx/internal/handler/ThingHandlerPartition.java

@@ -37,12 +37,8 @@ import org.slf4j.LoggerFactory;
 public class ThingHandlerPartition extends CaddxBaseThingHandler {
 
     private final Logger logger = LoggerFactory.getLogger(ThingHandlerPartition.class);
+    private long lastRefreshTime = 0;
 
-    /**
-     * Constructor.
-     *
-     * @param thing
-     */
     public ThingHandlerPartition(Thing thing) {
         super(thing, CaddxThingType.PARTITION);
     }
@@ -69,12 +65,14 @@ public class ThingHandlerPartition extends CaddxBaseThingHandler {
         }
 
         if (command instanceof RefreshType) {
-            if (channelUID.getId().equals(CaddxBindingConstants.PARTITION_ARMED)) {
+            // Refresh only if 2 seconds have passed from the last refresh
+            if (System.currentTimeMillis() - lastRefreshTime > 2000) {
                 cmd = CaddxBindingConstants.PARTITION_STATUS_REQUEST;
                 data = String.format("%d", getPartitionNumber() - 1);
             } else {
                 return;
             }
+            lastRefreshTime = System.currentTimeMillis();
         } else if (channelUID.getId().equals(CaddxBindingConstants.PARTITION_SECONDARY_COMMAND)) {
             cmd = channelUID.getId();
             data = String.format("%s,%d", command.toString(), (1 << getPartitionNumber() - 1));

bundles/org.openhab.binding.caddx/src/main/java/org/openhab/binding/caddx/internal/handler/ThingHandlerZone.java

@@ -38,12 +38,8 @@ import org.slf4j.LoggerFactory;
 public class ThingHandlerZone extends CaddxBaseThingHandler {
 
     private final Logger logger = LoggerFactory.getLogger(ThingHandlerZone.class);
+    private long lastRefreshTime = 0;
 
-    /**
-     * Constructor.
-     *
-     * @param thing
-     */
     public ThingHandlerZone(Thing thing) {
         super(thing, CaddxThingType.ZONE);
     }
@@ -77,15 +73,17 @@ public class ThingHandlerZone extends CaddxBaseThingHandler {
         String data = null;
 
         if (command instanceof RefreshType) {
-            if (channelUID.getId().equals(CaddxBindingConstants.ZONE_FAULTED)) {
+            // Refresh only if 2 seconds have passed from the last refresh
+            if (System.currentTimeMillis() - lastRefreshTime > 2000) {
                 cmd1 = CaddxBindingConstants.ZONE_STATUS_REQUEST;
                 cmd2 = CaddxBindingConstants.ZONE_NAME_REQUEST;
                 data = String.format("%d", getZoneNumber() - 1);
             } else {
                 return;
             }
+            lastRefreshTime = System.currentTimeMillis();
         } else if (channelUID.getId().equals(CaddxBindingConstants.ZONE_BYPASSED)) {
-            cmd1 = channelUID.getId();
+            cmd1 = CaddxBindingConstants.ZONE_BYPASSED;
             cmd2 = CaddxBindingConstants.ZONE_STATUS_REQUEST;
             data = String.format("%d", getZoneNumber() - 1);
         } else {

bundles/org.openhab.binding.caddx/src/main/resources/OH-INF/thing/bridge.xml

@@ -54,6 +54,12 @@
 				</options>
 			</parameter>
 
+			<parameter name="maxZoneNumber" type="integer" required="true" min="1" max="192">
+				<label>Maximum Zone Number</label>
+				<description>The maximum zone number that should be auto-discovered</description>
+				<default>16</default>
+			</parameter>
+
 		</config-description>
 	</bridge-type>
 

bundles/org.openhab.binding.caddx/src/test/java/org/openhab/binding/caddx/internal/CaddxMessageReaderUtil.java

@@ -0,0 +1,63 @@
+/**
+ * Copyright (c) 2010-2020 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.caddx.internal;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.stream.Collectors;
+
+import org.eclipse.jdt.annotation.NonNullByDefault;
+import org.openhab.core.util.HexUtils;
+
+/**
+ * Util class to read test input messages.
+ *
+ * @author Georgios Moutsos - Initial contribution
+ */
+@NonNullByDefault
+public final class CaddxMessageReaderUtil {
+    private static final String MESSAGE_EXT = ".msg";
+
+    private CaddxMessageReaderUtil() {
+        // Util class
+    }
+
+    /**
+     * Reads the raw bytes of the message given the file relative to this package and returns the objects.
+     *
+     * @param messageName name of the telegram file to read
+     * @return The raw bytes of a telegram
+     */
+    public static byte[] readRawMessage(String messageName) {
+        try (InputStream is = CaddxMessageReaderUtil.class.getResourceAsStream(messageName + MESSAGE_EXT)) {
+            String hexString = new BufferedReader(new InputStreamReader(is)).lines().collect(Collectors.joining("\n"));
+
+            return HexUtils.hexToBytes(hexString, " ");
+        } catch (IOException e) {
+            throw new AssertionError("IOException reading message data: ", e);
+        }
+    }
+
+    /**
+     * Reads a message given the file relative to this package and returns the object.
+     *
+     * @param messageName name of the message file to read
+     * @return a CaddxMessage object
+     */
+    public static CaddxMessage readCaddxMessage(String messageName) {
+        byte[] bytes = readRawMessage(messageName);
+        return new CaddxMessage(bytes, true);
+    }
+}

bundles/org.openhab.binding.caddx/src/test/java/org/openhab/binding/caddx/internal/message/CaddxMessageParseTest.java

@@ -0,0 +1,53 @@
+/**
+ * Copyright (c) 2010-2020 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.caddx.internal.message;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+import java.util.Arrays;
+import java.util.List;
+
+import org.eclipse.jdt.annotation.NonNullByDefault;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
+import org.openhab.binding.caddx.internal.CaddxMessage;
+import org.openhab.binding.caddx.internal.CaddxMessageReaderUtil;
+
+/**
+ * Test class for CaddxMessage.
+ *
+ * @author Georgios Moutsos - Initial contribution
+ */
+@NonNullByDefault
+public class CaddxMessageParseTest {
+
+    // @formatter:off
+    public static final List<Object[]> data() {
+        return Arrays.asList(new Object[][] {
+            { "zone_status_message", "zone_number", "4", },
+            { "interface_configuration_message", "panel_firmware_version", "5.37", },
+            { "interface_configuration_message", "panel_interface_configuration_message", "true", },
+
+        });
+    }
+    // @formatter:on
+
+    @ParameterizedTest
+    @MethodSource("data")
+    public void testParsing(String messageName, String property, String value) {
+        CaddxMessage message = CaddxMessageReaderUtil.readCaddxMessage(messageName);
+
+        assertNotNull(message, "Should not be null");
+        assertEquals(value, message.getPropertyById(property), property + " should be: " + value);
+    }
+}

bundles/org.openhab.binding.caddx/src/test/resources/org/openhab/binding/caddx/internal/interface_configuration_message.msg

@@ -0,0 +1 @@
+01 35 2E 33 37 F2 0F FA 1F 57 F8 46 4D

bundles/org.openhab.binding.caddx/src/test/resources/org/openhab/binding/caddx/internal/zone_status_message.msg

@@ -0,0 +1 @@
+84 04 01 01 05 C4 00 00 5C F5