[modbus] Validate poller count and write count according to modbus spec

Related to discussion in openhab#8228

Signed-off-by: Sami Salonen <[email protected]>

bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/handler/ModbusPollerThingHandler.java

@@ -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,12 @@ public String toString() {
 
     private final Logger logger = LoggerFactory.getLogger(ModbusPollerThingHandler.class);
 
+    private final static Object[] SORTED_READ_FUNCTION_CODES = ModbusBindingConstantsInternal.READ_FUNCTION_CODES
+            .keySet().toArray();
+    static {
+        Arrays.sort(SORTED_READ_FUNCTION_CODES);
+    }
+
     private @NonNullByDefault({}) ModbusPollerConfiguration config;
     private long cacheMillis;
     private volatile @Nullable PollTask pollTask;
@@ -214,6 +194,8 @@ public String toString() {
 
     private ReadCallbackDelegator callbackDelegator = new ReadCallbackDelegator();
 
+    private @Nullable ModbusReadFunctionCode functionCode;
+
     public ModbusPollerThingHandler(Bridge bridge) {
         super(bridge);
     }
@@ -258,10 +240,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 +345,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) {

bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/internal/handler/ModbusDataThingHandler.java

@@ -337,9 +337,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();
@@ -362,8 +363,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;

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>

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

@@ -99,4 +99,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
+     */
+    public static final int MAX_BITS_READ_COUNT = 2000;
+    /**
+     * Maximum number of registers that are allowed to be read.
+     * Limitation by Modbus protocol
+     */
+    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
+     */
+    public static final int MAX_BITS_WRITE_COUNT = 1968;
+    /**
+     * Maximum number of registers that are allowed to be written.
+     * Limitation by Modbus protocol
+     */
+    public static final int MAX_REGISTERS_WRITE_COUNT = 123;
 }

bundles/org.openhab.io.transport.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());

bundles/org.openhab.io.transport.modbus/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

itests/org.openhab.binding.modbus.tests/src/main/java/org/openhab/binding/modbus/tests/ModbusPollerThingHandlerTest.java

@@ -47,6 +47,7 @@
 import org.openhab.io.transport.modbus.AsyncModbusFailure;
 import org.openhab.io.transport.modbus.AsyncModbusReadResult;
 import org.openhab.io.transport.modbus.BitArray;
+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;
@@ -157,6 +158,80 @@ public void testInitializeNonPolling()
         verifyNoMoreInteractions(mockedModbusManager);
     }
 
+    private void testPollerLengthCheck(String type, int length, boolean expectedOnline) {
+        Configuration pollerConfig = new Configuration();
+        pollerConfig.put("refresh", 0L);
+        pollerConfig.put("start", 5);
+        pollerConfig.put("length", length);
+        pollerConfig.put("type", type);
+        poller = createPollerThingBuilder("poller").withConfiguration(pollerConfig).withBridge(endpoint.getUID())
+                .build();
+
+        addThing(poller);
+        assertThat(poller.getStatus(), is(equalTo(expectedOnline ? ThingStatus.ONLINE : ThingStatus.OFFLINE)));
+        if (!expectedOnline) {
+            assertThat(poller.getStatusInfo().getStatusDetail(), is(equalTo(ThingStatusDetail.CONFIGURATION_ERROR)));
+        }
+
+        verifyEndpointBasicInitInteraction();
+        verifyNoMoreInteractions(mockedModbusManager);
+    }
+
+    @Test
+    public void testPollerWithMaxRegisters()
+            throws IllegalArgumentException, IllegalAccessException, NoSuchFieldException, SecurityException {
+        testPollerLengthCheck(ModbusBindingConstantsInternal.READ_TYPE_HOLDING_REGISTER,
+                ModbusConstants.MAX_REGISTERS_READ_COUNT, true);
+    }
+
+    @Test
+    public void testPollerLengthOutOfBoundsWithRegisters()
+            throws IllegalArgumentException, IllegalAccessException, NoSuchFieldException, SecurityException {
+        testPollerLengthCheck(ModbusBindingConstantsInternal.READ_TYPE_HOLDING_REGISTER,
+                ModbusConstants.MAX_REGISTERS_READ_COUNT + 1, false);
+    }
+
+    @Test
+    public void testPollerWithMaxInputRegisters()
+            throws IllegalArgumentException, IllegalAccessException, NoSuchFieldException, SecurityException {
+        testPollerLengthCheck(ModbusBindingConstantsInternal.READ_TYPE_INPUT_REGISTER,
+                ModbusConstants.MAX_REGISTERS_READ_COUNT, true);
+    }
+
+    @Test
+    public void testPollerLengthOutOfBoundsWithInputRegisters()
+            throws IllegalArgumentException, IllegalAccessException, NoSuchFieldException, SecurityException {
+        testPollerLengthCheck(ModbusBindingConstantsInternal.READ_TYPE_INPUT_REGISTER,
+                ModbusConstants.MAX_REGISTERS_READ_COUNT + 1, false);
+    }
+
+    @Test
+    public void testPollerWithMaxCoils()
+            throws IllegalArgumentException, IllegalAccessException, NoSuchFieldException, SecurityException {
+        testPollerLengthCheck(ModbusBindingConstantsInternal.READ_TYPE_COIL, ModbusConstants.MAX_BITS_READ_COUNT, true);
+    }
+
+    @Test
+    public void testPollerLengthOutOfBoundsWithCoils()
+            throws IllegalArgumentException, IllegalAccessException, NoSuchFieldException, SecurityException {
+        testPollerLengthCheck(ModbusBindingConstantsInternal.READ_TYPE_COIL, ModbusConstants.MAX_BITS_READ_COUNT + 1,
+                false);
+    }
+
+    @Test
+    public void testPollerWithMaxDiscreteInput()
+            throws IllegalArgumentException, IllegalAccessException, NoSuchFieldException, SecurityException {
+        testPollerLengthCheck(ModbusBindingConstantsInternal.READ_TYPE_DISCRETE_INPUT,
+                ModbusConstants.MAX_BITS_READ_COUNT, true);
+    }
+
+    @Test
+    public void testPollerLengthOutOfBoundsWithDiscreteInput()
+            throws IllegalArgumentException, IllegalAccessException, NoSuchFieldException, SecurityException {
+        testPollerLengthCheck(ModbusBindingConstantsInternal.READ_TYPE_DISCRETE_INPUT,
+                ModbusConstants.MAX_BITS_READ_COUNT + 1, false);
+    }
+
     public void testPollingGeneric(String type, ModbusReadFunctionCode expectedFunctionCode)
             throws IllegalArgumentException, IllegalAccessException, NoSuchFieldException, SecurityException {
         PollTask pollTask = Mockito.mock(PollTask.class);