[modbus] Validate poller count and write count according to modbus sp…

…ec (#8370) * Related to discussion in #8228 * Documented where the limits come from. * Internet is full of interpretation and variations. We follow the spec. Signed-off-by: Sami Salonen <[email protected]>
openhab · Sep 12, 2020 · 314c283 · 314c283
1 parent d228f73
commit 314c283
Show file tree

Hide file tree

Showing 7 changed files with 238 additions and 36 deletions.
diff --git a/...ing.modbus/src/main/java/org/openhab/binding/modbus/handler/ModbusPollerThingHandler.java b/...ing.modbus/src/main/java/org/openhab/binding/modbus/handler/ModbusPollerThingHandler.java
@@ -12,10 +12,10 @@
  */
 package org.openhab.binding.modbus.handler;
 
-import java.util.Arrays;
 import java.util.List;
 import java.util.Optional;
 import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.stream.Collectors;
 
 import org.apache.commons.lang.StringUtils;
 import org.eclipse.jdt.annotation.NonNullByDefault;
@@ -36,6 +36,7 @@
 import org.openhab.io.transport.modbus.AsyncModbusFailure;
 import org.openhab.io.transport.modbus.AsyncModbusReadResult;
 import org.openhab.io.transport.modbus.ModbusCommunicationInterface;
+import org.openhab.io.transport.modbus.ModbusConstants;
 import org.openhab.io.transport.modbus.ModbusFailureCallback;
 import org.openhab.io.transport.modbus.ModbusReadCallback;
 import org.openhab.io.transport.modbus.ModbusReadFunctionCode;
@@ -150,33 +151,6 @@ public void resetCache() {
         }
     }
 
-    /**
-     * Immutable {@link ModbusReadRequestBlueprint} to read from endpoint represented by this Poller's bridge
-     *
-     * @author Sami Salonen
-     *
-     */
-    private static class ModbusPollerReadRequest extends ModbusReadRequestBlueprint {
-
-        private static ModbusReadFunctionCode getFunctionCode(@Nullable String type) {
-            if (!ModbusBindingConstantsInternal.READ_FUNCTION_CODES.containsKey(type)) {
-                Object[] acceptedTypes = ModbusBindingConstantsInternal.READ_FUNCTION_CODES.keySet().toArray();
-                Arrays.sort(acceptedTypes);
-                throw new IllegalArgumentException(
-                        String.format("No function code found for type='%s'. Was expecting one of: %s", type,
-                                StringUtils.join(acceptedTypes, ", ")));
-            }
-            ModbusReadFunctionCode functionCode = ModbusBindingConstantsInternal.READ_FUNCTION_CODES.get(type);
-            return functionCode;
-        }
-
-        public ModbusPollerReadRequest(ModbusPollerConfiguration config,
-                ModbusEndpointThingHandler slaveEndpointThingHandler) throws EndpointNotInitializedException {
-            super(slaveEndpointThingHandler.getSlaveId(), getFunctionCode(config.getType()), config.getStart(),
-                    config.getLength(), config.getMaxTries());
-        }
-    }
-
     /**
      * Immutable data object to cache the results of a poll request
      */
@@ -204,6 +178,9 @@ public String toString() {
 
     private final Logger logger = LoggerFactory.getLogger(ModbusPollerThingHandler.class);
 
+    private final static List<String> SORTED_READ_FUNCTION_CODES = ModbusBindingConstantsInternal.READ_FUNCTION_CODES
+            .keySet().stream().sorted().collect(Collectors.toList());
+
     private @NonNullByDefault({}) ModbusPollerConfiguration config;
     private long cacheMillis;
     private volatile @Nullable PollTask pollTask;
@@ -214,6 +191,8 @@ public String toString() {
 
     private ReadCallbackDelegator callbackDelegator = new ReadCallbackDelegator();
 
+    private @Nullable ModbusReadFunctionCode functionCode;
+
     public ModbusPollerThingHandler(Bridge bridge) {
         super(bridge);
     }
@@ -258,10 +237,39 @@ public synchronized void initialize() {
         }
         this.callbackDelegator.resetCache();
         comms = null;
+        request = null;
         disposed = false;
         logger.trace("Initializing {} from status {}", this.getThing().getUID(), this.getThing().getStatus());
         try {
             config = getConfigAs(ModbusPollerConfiguration.class);
+            String type = config.getType();
+            if (!ModbusBindingConstantsInternal.READ_FUNCTION_CODES.containsKey(type)) {
+                updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR,
+                        String.format("No function code found for type='%s'. Was expecting one of: %s", type,
+                                StringUtils.join(SORTED_READ_FUNCTION_CODES, ", ")));
+                return;
+            }
+            functionCode = ModbusBindingConstantsInternal.READ_FUNCTION_CODES.get(type);
+            switch (functionCode) {
+                case READ_INPUT_REGISTERS:
+                case READ_MULTIPLE_REGISTERS:
+                    if (config.getLength() > ModbusConstants.MAX_REGISTERS_READ_COUNT) {
+                        updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, String.format(
+                                "Maximum of %d registers can be polled at once due to protocol limitations. Length %d is out of bounds.",
+                                ModbusConstants.MAX_REGISTERS_READ_COUNT, config.getLength()));
+                        return;
+                    }
+                    break;
+                case READ_COILS:
+                case READ_INPUT_DISCRETES:
+                    if (config.getLength() > ModbusConstants.MAX_BITS_READ_COUNT) {
+                        updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, String.format(
+                                "Maximum of %d coils/discrete inputs can be polled at once due to protocol limitations. Length %d is out of bounds.",
+                                ModbusConstants.MAX_BITS_READ_COUNT, config.getLength()));
+                        return;
+                    }
+                    break;
+            }
             cacheMillis = this.config.getCacheMillis();
             registerPollTask();
         } catch (EndpointNotInitializedException e) {
@@ -334,8 +342,13 @@ private synchronized void registerPollTask() throws EndpointNotInitializedExcept
             return;
         }
         this.comms = localComms;
+        ModbusReadFunctionCode localFunctionCode = functionCode;
+        if (localFunctionCode == null) {
+            return;
+        }
 
-        ModbusReadRequestBlueprint localRequest = new ModbusPollerReadRequest(config, slaveEndpointThingHandler);
+        ModbusReadRequestBlueprint localRequest = new ModbusReadRequestBlueprint(slaveEndpointThingHandler.getSlaveId(),
+                localFunctionCode, config.getStart(), config.getLength(), config.getMaxTries());
         this.request = localRequest;
 
         if (config.getRefresh() <= 0L) {

diff --git a/...bus/src/main/java/org/openhab/binding/modbus/internal/handler/ModbusDataThingHandler.java b/...bus/src/main/java/org/openhab/binding/modbus/internal/handler/ModbusDataThingHandler.java
@@ -334,9 +334,10 @@ public synchronized void initialize() {
             ModbusDataConfiguration localConfig = config = getConfigAs(ModbusDataConfiguration.class);
             updateUnchangedValuesEveryMillis = localConfig.getUpdateUnchangedValuesEveryMillis();
             Bridge bridge = getBridge();
-            if (bridge == null) {
-                logger.debug("Thing {} '{}' has no bridge", getThing().getUID(), getThing().getLabel());
-                updateStatusIfChanged(ThingStatus.OFFLINE, ThingStatusDetail.BRIDGE_OFFLINE, "No poller bridge");
+            if (bridge == null || !bridge.getStatus().equals(ThingStatus.ONLINE)) {
+                logger.debug("Thing {} '{}' has no bridge or it is not online", getThing().getUID(),
+                        getThing().getLabel());
+                updateStatusIfChanged(ThingStatus.OFFLINE, ThingStatusDetail.BRIDGE_OFFLINE, "No online bridge");
                 return;
             }
             BridgeHandler bridgeHandler = bridge.getHandler();
@@ -359,8 +360,9 @@ public synchronized void initialize() {
                 pollerHandler = localPollerHandler;
                 ModbusReadRequestBlueprint localReadRequest = localPollerHandler.getRequest();
                 if (localReadRequest == null) {
-                    logger.debug("Poller {} '{}' has no read request -- configuration is changing?", bridge.getUID(),
-                            bridge.getLabel());
+                    logger.debug(
+                            "Poller {} '{}' has no read request -- configuration is changing or bridge having invalid configuration?",
+                            bridge.getUID(), bridge.getLabel());
                     updateStatusIfChanged(ThingStatus.OFFLINE, ThingStatusDetail.BRIDGE_OFFLINE,
                             String.format("Poller %s '%s' has no poll task", bridge.getUID(), bridge.getLabel()));
                     return;

diff --git a/bundles/org.openhab.binding.modbus/src/main/resources/ESH-INF/thing/bridge-poller.xml b/bundles/org.openhab.binding.modbus/src/main/resources/ESH-INF/thing/bridge-poller.xml
@@ -26,7 +26,9 @@
 			</parameter>
 			<parameter name="length" type="integer" required="true">
 				<label>Length</label>
-				<description>Number of registers, coils or discrete inputs to read.</description>
+				<description><![CDATA[Number of registers, coils or discrete inputs to read.
+				<br />
+				<br />Maximum number of registers is 125 while 2000 is maximum for coils and discrete inputs.]]></description>
 			</parameter>
 			<parameter name="type" type="text" required="true">
 				<label>Type</label>

diff --git a/...ab.io.transport.modbus/src/main/java/org/openhab/io/transport/modbus/ModbusConstants.java b/...ab.io.transport.modbus/src/main/java/org/openhab/io/transport/modbus/ModbusConstants.java
@@ -21,6 +21,29 @@
 /**
  * Constants for Modbus transport
  *
+ * == Regarding maximum read and write limits ==
+ *
+ * Maximum number of registers that are allowed to be read.
+ *
+ * The Modbus protocol has many intepretation on maximum data size of messages. Good reference is here:
+ * https://wingpath.co.uk/manpage.php?product=modtest&page=message_limits.html
+ *
+ * We try to follow modern specification here (V1.1B3):
+ * https://modbus.org/docs/Modbus_Application_Protocol_V1_1b3.pdf. See section 4.1 Protocol Specification in the
+ * specification.
+ *
+ * According to V1.1B3, maximum size for PDU is 253 bytes, making maximum ADU size 256 (RTU) or 260 (TCP).
+ *
+ * In the spec section 6, one can see maximum values for read and write counts.
+ *
+ * Note that this is not the only interpretation -- some sources limit the ADU to 256 also with TCP.
+ * In some cases, slaves cannot take in so much data.
+ *
+ *
+ * Reads are limited by response PDU size.
+ * Writes (FC15 & FC16) are limited by write request ADU size.
+ *
+ *
  * @author Sami Salonen - Initial contribution
  *
  */
@@ -99,4 +122,25 @@ public String toString() {
                     .orElseThrow(() -> new IllegalArgumentException("Invalid valueType " + configValueType));
         }
     }
+
+    /**
+     * Maximum number of coils or discrete inputs that are allowed to be read.
+     * Limitation by Modbus protocol V1.1B3, 6.1 definition of Read Holding registers.
+     */
+    public static final int MAX_BITS_READ_COUNT = 2000;
+    /**
+     * Maximum number of registers that are allowed to be read.
+     * Limitation by Modbus protocol V1.1B3, 6.3 definition of Read Coils.
+     */
+    public static final int MAX_REGISTERS_READ_COUNT = 125;
+    /**
+     * Maximum number of coils or discrete inputs that are allowed to be written.
+     * Limitation by Modbus protocol V1.1B3, 6.11 definition of Write Multiple coils.
+     */
+    public static final int MAX_BITS_WRITE_COUNT = 1968;
+    /**
+     * Maximum number of registers that are allowed to be written.
+     * Limitation by Modbus protocol V1.1B3, 6.12 definition of Write Multiple registers.
+     */
+    public static final int MAX_REGISTERS_WRITE_COUNT = 123;
 }
diff --git a/....modbus/src/main/java/org/openhab/io/transport/modbus/json/WriteRequestJsonUtilities.java b/....modbus/src/main/java/org/openhab/io/transport/modbus/json/WriteRequestJsonUtilities.java
@@ -21,6 +21,7 @@
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
 import org.openhab.io.transport.modbus.BitArray;
+import org.openhab.io.transport.modbus.ModbusConstants;
 import org.openhab.io.transport.modbus.ModbusRegister;
 import org.openhab.io.transport.modbus.ModbusRegisterArray;
 import org.openhab.io.transport.modbus.ModbusWriteCoilRequestBlueprint;
@@ -84,7 +85,8 @@ private WriteRequestJsonUtilities() {
      * @param unitId unit id for the constructed {@link ModbusWriteRequestBlueprint}
      * @param jsonString json to be parsed in string format
      * @return collection of {@link ModbusWriteRequestBlueprint} representing the json
-     * @throws IllegalArgumentException in case of unexpected function codes
+     * @throws IllegalArgumentException in case of unexpected function codes, or too large payload exceeding modbus
+     *             protocol specification
      * @throws IllegalStateException in case of parsing errors and unexpected json structure
      *
      * @see WriteRequestJsonUtilities.JSON_FUNCTION_CODE
@@ -177,6 +179,10 @@ private static ModbusWriteRequestBlueprint constructBluerint(int unitId, @Nullab
             case WRITE_MULTIPLE_COILS:
                 if (valuesElem.size() == 0) {
                     throw new IllegalArgumentException("Must provide at least one coil");
+                } else if (valuesElem.size() > ModbusConstants.MAX_BITS_WRITE_COUNT) {
+                    throw new IllegalArgumentException(
+                            String.format("Trying to write too many coils (%d). Maximum is %s", valuesElem.size(),
+                                    ModbusConstants.MAX_BITS_WRITE_COUNT));
                 }
                 BitArray bits = new BitArray(valuesElem.size());
                 for (int i = 0; i < valuesElem.size(); i++) {
@@ -194,6 +200,10 @@ private static ModbusWriteRequestBlueprint constructBluerint(int unitId, @Nullab
                 ModbusRegister[] registers = new ModbusRegister[valuesElem.size()];
                 if (registers.length == 0) {
                     throw new IllegalArgumentException("Must provide at least one register");
+                } else if (valuesElem.size() > ModbusConstants.MAX_REGISTERS_WRITE_COUNT) {
+                    throw new IllegalArgumentException(
+                            String.format("Trying to write too many registers (%d). Maximum is %s", valuesElem.size(),
+                                    ModbusConstants.MAX_REGISTERS_WRITE_COUNT));
                 }
                 for (int i = 0; i < valuesElem.size(); i++) {
                     registers[i] = new ModbusRegister(valuesElem.get(i).getAsInt());

diff --git a/...bus/src/test/java/org/openhab/io/transport/modbus/test/WriteRequestJsonUtilitiesTest.java b/...bus/src/test/java/org/openhab/io/transport/modbus/test/WriteRequestJsonUtilitiesTest.java
@@ -15,17 +15,35 @@
 import static org.hamcrest.CoreMatchers.*;
 import static org.hamcrest.collection.IsArrayContainingInOrder.arrayContaining;
 import static org.junit.Assert.assertThat;
+import static org.openhab.io.transport.modbus.ModbusConstants.*;
 
+import java.util.Collection;
+import java.util.List;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+
+import org.eclipse.jdt.annotation.NonNull;
 import org.hamcrest.Matcher;
 import org.junit.Test;
 import org.openhab.io.transport.modbus.ModbusWriteFunctionCode;
+import org.openhab.io.transport.modbus.ModbusWriteRequestBlueprint;
 import org.openhab.io.transport.modbus.json.WriteRequestJsonUtilities;
 
 /**
  * @author Sami Salonen - Initial contribution
  */
 public class WriteRequestJsonUtilitiesTest {
 
+    private static List<String> MAX_REGISTERS = IntStream.range(0, MAX_REGISTERS_WRITE_COUNT).mapToObj(i -> "1")
+            .collect(Collectors.toList());
+    private static List<String> OVER_MAX_REGISTERS = IntStream.range(0, MAX_REGISTERS_WRITE_COUNT + 1)
+            .mapToObj(i -> "1").collect(Collectors.toList());
+
+    private static List<String> MAX_COILS = IntStream.range(0, MAX_BITS_WRITE_COUNT).mapToObj(i -> "1")
+            .collect(Collectors.toList());
+    private static List<String> OVER_MAX_COILS = IntStream.range(0, MAX_BITS_WRITE_COUNT + 1).mapToObj(i -> "1")
+            .collect(Collectors.toList());
+
     @Test
     public void testEmptyArray() {
         assertThat(WriteRequestJsonUtilities.fromJson(3, "[]").size(), is(equalTo(0)));
@@ -107,6 +125,25 @@ public void testFC16MultipleRegisters() {
                         ModbusWriteFunctionCode.WRITE_MULTIPLE_REGISTERS, 3, 4, 2)));
     }
 
+    @Test
+    public void testFC16MultipleRegistersMaxRegisters() {
+        Collection<@NonNull ModbusWriteRequestBlueprint> writes = WriteRequestJsonUtilities.fromJson(55, "[{"//
+                + "\"functionCode\": 16,"//
+                + "\"address\": 5412,"//
+                + "\"value\": [" + String.join(",", MAX_REGISTERS) + "]"//
+                + "}]");
+        assertThat(writes.size(), is(equalTo(1)));
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testFC16MultipleRegistersTooManyRegisters() {
+        WriteRequestJsonUtilities.fromJson(55, "[{"//
+                + "\"functionCode\": 16,"//
+                + "\"address\": 5412,"//
+                + "\"value\": [" + String.join(",", OVER_MAX_REGISTERS) + "]"//
+                + "}]");
+    }
+
     @SuppressWarnings({ "rawtypes", "unchecked" })
     @Test
     public void testFC5SingeCoil() {
@@ -152,6 +189,25 @@ public void testFC15MultipleCoils() {
                         ModbusWriteFunctionCode.WRITE_MULTIPLE_COILS, true, false, true)));
     }
 
+    @Test
+    public void testFC15MultipleCoilsMaxCoils() {
+        Collection<@NonNull ModbusWriteRequestBlueprint> writes = WriteRequestJsonUtilities.fromJson(55, "[{"//
+                + "\"functionCode\": 15,"//
+                + "\"address\": 5412,"//
+                + "\"value\": [" + String.join(",", MAX_COILS) + "]"//
+                + "}]");
+        assertThat(writes.size(), is(equalTo(1)));
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testFC15MultipleCoilsTooManyCoils() {
+        WriteRequestJsonUtilities.fromJson(55, "[{"//
+                + "\"functionCode\": 15,"//
+                + "\"address\": 5412,"//
+                + "\"value\": [" + String.join(",", OVER_MAX_COILS) + "]"//
+                + "}]");
+    }
+
     @Test(expected = IllegalStateException.class)
     public void testEmptyObject() {
         // we are expecting list, not object -> error