[modbus] gracefully handle unexpected slave responses (openhab#7160)

* [modbus] Handle various invalid/illegal slave responses gracefully

- slave responds with too little data. Instead of crashing later when
  using the data, abort immediately.
- slave responds with function code which does not match the request.
  This is illegal according to protocol

Since we have seen both of the errors in the wild it is better to handle
them gracefully, and print out good error message for the user.

In addition, we now catch all Exceptions in the runnable that polls the
data. This is to cover any unexpected / unknown errors that might arise.
Now that we catch the error, the polling does not get halted (behaviour
of the scheduler we use) but instead it will be retried again.

Obviously copy-pasted from jamod (the underlying modbus library)

Signed-off-by: Sami Salonen <ssalonen@gmail.com>

bundles/org.openhab.io.transport.modbus/src/main/java/org/openhab/io/transport/modbus/ModbusReadRequestBlueprint.java

@@ -35,11 +35,8 @@ public interface ModbusReadRequestBlueprint extends ModbusRequestBlueprint {
     public int getReference();
 
     /**
-     * Returns the length of the data appended
-     * after the protocol header.
-     * <p>
+     * Returns the number of registers/coils/discrete inputs
      *
-     * @return the data length as <tt>int</tt>.
      */
     public int getDataLength();
 

bundles/org.openhab.io.transport.modbus/src/main/java/org/openhab/io/transport/modbus/ModbusUnexpectedResponseFunctionCodeException.java

@@ -0,0 +1,49 @@
+/**
+ * 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.io.transport.modbus;
+
+import org.eclipse.jdt.annotation.NonNullByDefault;
+
+/**
+ * Exception representing situation where function code of the response does not match request
+ *
+ * @author Sami Salonen - Initial contribution
+ *
+ */
+@NonNullByDefault
+public class ModbusUnexpectedResponseFunctionCodeException extends ModbusTransportException {
+
+    private static final long serialVersionUID = 1109165449703638949L;
+    private int requestFunctionCode;
+    private int responseFunctionCode;
+
+    public ModbusUnexpectedResponseFunctionCodeException(int requestFunctionCode, int responseFunctionCode) {
+        this.requestFunctionCode = requestFunctionCode;
+        this.responseFunctionCode = responseFunctionCode;
+
+    }
+
+    @Override
+    public String getMessage() {
+        return String.format("Function code of request (%d) does not equal response (%d)", requestFunctionCode,
+                responseFunctionCode);
+    }
+
+    @Override
+    public String toString() {
+        return String.format(
+                "ModbusUnexpectedResponseFunctionCodeException(requestFunctionCode=%d, responseFunctionCode=%d)",
+                requestFunctionCode, responseFunctionCode);
+    }
+
+}

bundles/org.openhab.io.transport.modbus/src/main/java/org/openhab/io/transport/modbus/ModbusUnexpectedResponseSizeException.java

@@ -0,0 +1,48 @@
+/**
+ * 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.io.transport.modbus;
+
+import org.eclipse.jdt.annotation.NonNullByDefault;
+
+/**
+ * Exception representing situation where data length of the response does not match request
+ *
+ * @author Sami Salonen - Initial contribution
+ *
+ */
+@NonNullByDefault
+public class ModbusUnexpectedResponseSizeException extends ModbusTransportException {
+
+    private static final long serialVersionUID = 2460907938819984483L;
+    private int requestSize;
+    private int responseSize;
+
+    public ModbusUnexpectedResponseSizeException(int requestSize, int responseSize) {
+        this.requestSize = requestSize;
+        this.responseSize = responseSize;
+
+    }
+
+    @Override
+    public String getMessage() {
+        return String.format("Data length of the request (%d) does not equal response (%d). Slave response is invalid.",
+                requestSize, responseSize);
+    }
+
+    @Override
+    public String toString() {
+        return String.format("ModbusUnexpectedResponseSizeException(requestFunctionCode=%d, responseFunctionCode=%d)",
+                requestSize, responseSize);
+    }
+
+}

bundles/org.openhab.io.transport.modbus/src/main/java/org/openhab/io/transport/modbus/ModbusUnexpectedTransactionIdException.java

@@ -35,7 +35,8 @@ public ModbusUnexpectedTransactionIdException(int requestId, int responseId) {
 
     @Override
     public String getMessage() {
-        return String.format("Transaction id of response (%d) does not equal request (%d)", requestId, responseId);
+        return String.format("Transaction id of request (%d) does not equal response (%d). Slave response is invalid.",
+                requestId, responseId);
     }
 
     @Override

bundles/org.openhab.io.transport.modbus/src/main/java/org/openhab/io/transport/modbus/internal/ModbusLibraryWrapper.java

@@ -255,42 +255,72 @@ public static Register[] convertRegisters(ModbusRegisterArray arr) {
                 .collect(Collectors.toList()).toArray(new Register[0]);
     }
 
+    /**
+     * Get number of bits/registers/discrete inputs in the request.
+     *
+     *
+     * @param response
+     * @param request
+     * @return
+     */
+    public static int getNumberOfItemsInResponse(ModbusResponse response, ModbusReadRequestBlueprint request) {
+        // jamod library seems to be a bit buggy when it comes number of coils/discrete inputs in the response. Some
+        // of the methods such as ReadCoilsResponse.getBitCount() are returning wrong values.
+        //
+        // This is the reason we use a bit more verbose way to get the number of items in the response.
+        final int responseCount;
+        if (request.getFunctionCode() == ModbusReadFunctionCode.READ_COILS) {
+            responseCount = ((ReadCoilsResponse) response).getCoils().size();
+        } else if (request.getFunctionCode() == ModbusReadFunctionCode.READ_INPUT_DISCRETES) {
+            responseCount = ((ReadInputDiscretesResponse) response).getDiscretes().size();
+        } else if (request.getFunctionCode() == ModbusReadFunctionCode.READ_MULTIPLE_REGISTERS) {
+            responseCount = ((ReadMultipleRegistersResponse) response).getRegisters().length;
+        } else if (request.getFunctionCode() == ModbusReadFunctionCode.READ_INPUT_REGISTERS) {
+            responseCount = ((ReadInputRegistersResponse) response).getRegisters().length;
+        } else {
+            throw new IllegalArgumentException(String.format("Unexpected function code %s", request.getFunctionCode()));
+        }
+        return responseCount;
+    }
+
     /**
      * Invoke callback with the data received
      *
      * @param message original request
      * @param callback callback for read
      * @param response Modbus library response object
      */
-    public static void invokeCallbackWithResponse(ModbusReadRequestBlueprint message, ModbusReadCallback callback,
+    public static void invokeCallbackWithResponse(ModbusReadRequestBlueprint request, ModbusReadCallback callback,
             ModbusResponse response) {
         try {
-            getLogger().trace("Calling read response callback {} for request {}. Response was {}", callback, message,
+            getLogger().trace("Calling read response callback {} for request {}. Response was {}", callback, request,
                     response);
-            // jamod library seems to be a bit buggy when it comes number of coils in the response, so we use
-            // minimum(request, response). The underlying reason is that BitVector.createBitVector initializes the
-            // vector
-            // with too many bits as size
-            if (message.getFunctionCode() == ModbusReadFunctionCode.READ_COILS) {
+            // The number of coils/discrete inputs received in response are always in the multiples of 8
+            // bits.
+            // So even if querying 5 bits, you will actually get 8 bits. Here we wrap the data in
+            // BitArrayWrappingBitVector
+            // with will validate that the consumer is not accessing the "invalid" bits of the response.
+            int dataItemsInResponse = getNumberOfItemsInResponse(response, request);
+            if (request.getFunctionCode() == ModbusReadFunctionCode.READ_COILS) {
                 BitVector bits = ((ReadCoilsResponse) response).getCoils();
-                callback.onBits(message,
-                        new BitArrayWrappingBitVector(bits, Math.min(bits.size(), message.getDataLength())));
-            } else if (message.getFunctionCode() == ModbusReadFunctionCode.READ_INPUT_DISCRETES) {
+                callback.onBits(request,
+                        new BitArrayWrappingBitVector(bits, Math.min(dataItemsInResponse, request.getDataLength())));
+            } else if (request.getFunctionCode() == ModbusReadFunctionCode.READ_INPUT_DISCRETES) {
                 BitVector bits = ((ReadInputDiscretesResponse) response).getDiscretes();
-                callback.onBits(message,
-                        new BitArrayWrappingBitVector(bits, Math.min(bits.size(), message.getDataLength())));
-            } else if (message.getFunctionCode() == ModbusReadFunctionCode.READ_MULTIPLE_REGISTERS) {
-                callback.onRegisters(message, new RegisterArrayWrappingInputRegister(
+                callback.onBits(request,
+                        new BitArrayWrappingBitVector(bits, Math.min(dataItemsInResponse, request.getDataLength())));
+            } else if (request.getFunctionCode() == ModbusReadFunctionCode.READ_MULTIPLE_REGISTERS) {
+                callback.onRegisters(request, new RegisterArrayWrappingInputRegister(
                         ((ReadMultipleRegistersResponse) response).getRegisters()));
-            } else if (message.getFunctionCode() == ModbusReadFunctionCode.READ_INPUT_REGISTERS) {
-                callback.onRegisters(message,
+            } else if (request.getFunctionCode() == ModbusReadFunctionCode.READ_INPUT_REGISTERS) {
+                callback.onRegisters(request,
                         new RegisterArrayWrappingInputRegister(((ReadInputRegistersResponse) response).getRegisters()));
             } else {
                 throw new IllegalArgumentException(
-                        String.format("Unexpected function code %s", message.getFunctionCode()));
+                        String.format("Unexpected function code %s", request.getFunctionCode()));
             }
         } finally {
-            getLogger().trace("Called read response callback {} for request {}. Response was {}", callback, message,
+            getLogger().trace("Called read response callback {} for request {}. Response was {}", callback, request,
                     response);
         }
     }