[modbus] fix defaults for tcp and serial things and some other minor cleanup (#10147)

* [modbus] More strict nullness. Remove apache.commons.lang from itests
* [modbus] Defaults for tcp and serial things according to docs
* [modbus] further explicit defaults
* [modbus] document default encoding for serial.
RTU is pretty much the only one used in the field.
Previous default was ascii implicitly.
* [modbus] verify defaults are used for undefined configuration parameters

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

diff --git a/bundles/org.openhab.binding.modbus/README.md b/bundles/org.openhab.binding.modbus/README.md
index c6f561aa443c10d0ff8fc235842a13415f70c137..ae30bbe53e4f204f8bfd9c426c3edc158210f9c3 100644 (file)
--- a/bundles/org.openhab.binding.modbus/README.md
+++ b/bundles/org.openhab.binding.modbus/README.md
@@ -146,7 +146,7 @@ Basic parameters
 | stopBits  | text    | ✓        |                    | Stop bits. Valid values are: `"1.0"`, `"1.5"`, `"2.0"`.                                                                                                                                                       |     |
 | parity    | text    | ✓        |                    | Parity. Valid values are: `"none"`, `"even"`, `"odd"`.                                                                                                                                                    |     |
 | dataBits  | integer | ✓        |                    | Data bits. Valid values are: `5`, `6`, `7` and `8`.                                                                                                                                                       |     |
-| encoding  | text    | ✓        |                    | Encoding. Valid values are: `"ascii"`, `"rtu"`, `"bin"`.                                                                                                                                                  |     |
+| encoding  | text    |          | `"rtu"`           | Encoding. Valid values are: `"ascii"`, `"rtu"`, `"bin"`.                                                                                                                                                  |     |
 | echo      | boolean |          | `false`            | Flag for setting the RS485 echo mode. This controls whether we should try to read back whatever we send on the line, before reading the response. Valid values are: `true`, `false`.                      |     |
 
 Advanced parameters

diff --git a/bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/internal/config/ModbusSerialConfiguration.java b/bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/internal/config/ModbusSerialConfiguration.java
index 32804d38c4ecc7eac9af3bf14beee353a66109e1..8e81973a0ec5201c9f40e7b42cad5408bdfd2067 100644 (file)
--- a/bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/internal/config/ModbusSerialConfiguration.java
+++ b/bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/internal/config/ModbusSerialConfiguration.java
@@ -24,19 +24,19 @@ import org.eclipse.jdt.annotation.Nullable;
 @NonNullByDefault
 public class ModbusSerialConfiguration {
     private @Nullable String port;
-    private int id;
+    private int id = 1;
     private int baud;
     private @Nullable String stopBits;
     private @Nullable String parity;
     private int dataBits;
-    private @Nullable String encoding;
+    private String encoding = "rtu";
     private boolean echo;
-    private int receiveTimeoutMillis;
-    private @Nullable String flowControlIn;
-    private @Nullable String flowControlOut;
-    private int timeBetweenTransactionsMillis;
-    private int connectMaxTries;
-    private int connectTimeoutMillis;
+    private int receiveTimeoutMillis = 1500;
+    private String flowControlIn = "none";
+    private String flowControlOut = "none";
+    private int timeBetweenTransactionsMillis = 35;
+    private int connectMaxTries = 1;
+    private int connectTimeoutMillis = 10_000;
     private boolean enableDiscovery;
 
     public @Nullable String getPort() {

diff --git a/bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/internal/config/ModbusTcpConfiguration.java b/bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/internal/config/ModbusTcpConfiguration.java
index 28ff1d2a2c99130a6bee3227815300498dc2b637..f565f2082a58f922d05554045c8fff3db77c6eed 100644 (file)
--- a/bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/internal/config/ModbusTcpConfiguration.java
+++ b/bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/internal/config/ModbusTcpConfiguration.java
@@ -25,12 +25,12 @@ import org.eclipse.jdt.annotation.Nullable;
 public class ModbusTcpConfiguration {
     private @Nullable String host;
     private int port;
-    private int id;
-    private int timeBetweenTransactionsMillis;
+    private int id = 1;
+    private int timeBetweenTransactionsMillis = 60;
     private int timeBetweenReconnectMillis;
-    private int connectMaxTries;
+    private int connectMaxTries = 1;
     private int reconnectAfterMillis;
-    private int connectTimeoutMillis;
+    private int connectTimeoutMillis = 10_000;
     private boolean enableDiscovery;
     private boolean rtuEncoded;
 

diff --git a/bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/internal/handler/AbstractModbusEndpointThingHandler.java b/bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/internal/handler/AbstractModbusEndpointThingHandler.java
index ceb2e7f8f5b47cb4e54c38bdc95af50859bfeb45..35e6dd1c605db98b8b7a0c89d62c5a4b29d1555d 100644 (file)
--- a/bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/internal/handler/AbstractModbusEndpointThingHandler.java
+++ b/bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/internal/handler/AbstractModbusEndpointThingHandler.java
@@ -45,7 +45,7 @@ public abstract class AbstractModbusEndpointThingHandler<E extends ModbusSlaveEn
     protected volatile @Nullable C config;
     protected volatile @Nullable E endpoint;
     protected ModbusManager modbusManager;
-    protected volatile @Nullable EndpointPoolConfiguration poolConfiguration;
+    protected volatile @NonNullByDefault({}) EndpointPoolConfiguration poolConfiguration;
     private final Logger logger = LoggerFactory.getLogger(AbstractModbusEndpointThingHandler.class);
     private @NonNullByDefault({}) ModbusCommunicationInterface comms;
 

diff --git a/itests/org.openhab.binding.modbus.tests/itest.bndrun b/itests/org.openhab.binding.modbus.tests/itest.bndrun
index d27327c3aec5a6899fd488c2b6caa14de295e6e9..37bb27f89b1f332b129199545619183abc25b681 100644 (file)
--- a/itests/org.openhab.binding.modbus.tests/itest.bndrun
+++ b/itests/org.openhab.binding.modbus.tests/itest.bndrun
@@ -27,8 +27,6 @@ Fragment-Host: org.openhab.binding.modbus
 #
 -runbundles: \
        javax.measure.unit-api;version='[1.0.0,1.0.1)',\
-       org.apache.commons.io;version='[2.2.0,2.2.1)',\
-       org.apache.commons.lang;version='[2.6.0,2.6.1)',\
        org.apache.felix.configadmin;version='[1.9.8,1.9.9)',\
        org.apache.felix.scr;version='[2.1.10,2.1.11)',\
        org.eclipse.equinox.event;version='[1.4.300,1.4.301)',\

diff --git a/itests/org.openhab.binding.modbus.tests/src/main/java/org/openhab/binding/modbus/tests/AbstractModbusOSGiTest.java b/itests/org.openhab.binding.modbus.tests/src/main/java/org/openhab/binding/modbus/tests/AbstractModbusOSGiTest.java
index 26516d9a41abf615fc7b7812bd3af8cb9cab4579..e0198a3f8d9ef395cb255ef09841f39411ac0a01 100644 (file)
--- a/itests/org.openhab.binding.modbus.tests/src/main/java/org/openhab/binding/modbus/tests/AbstractModbusOSGiTest.java
+++ b/itests/org.openhab.binding.modbus.tests/src/main/java/org/openhab/binding/modbus/tests/AbstractModbusOSGiTest.java
@@ -107,13 +107,13 @@ public abstract class AbstractModbusOSGiTest extends JavaOSGiTest {
     private final Logger logger = LoggerFactory.getLogger(AbstractModbusOSGiTest.class);
 
     protected @Mock @NonNullByDefault({}) ModbusManager mockedModbusManager;
+    protected @NonNullByDefault({}) ModbusManager realModbusManager;
     protected @NonNullByDefault({}) ManagedThingProvider thingProvider;
     protected @NonNullByDefault({}) ManagedItemProvider itemProvider;
     protected @NonNullByDefault({}) ManagedItemChannelLinkProvider itemChannelLinkProvider;
     protected @NonNullByDefault({}) ItemRegistry itemRegistry;
     protected @NonNullByDefault({}) CoreItemFactory coreItemFactory;
 
-    private @NonNullByDefault({}) ModbusManager realModbusManager;
     private Set<Item> addedItems = new HashSet<>();
     private Set<Thing> addedThings = new HashSet<>();
     private Set<ItemChannelLink> addedLinks = new HashSet<>();

diff --git a/itests/org.openhab.binding.modbus.tests/src/main/java/org/openhab/binding/modbus/tests/ModbusTcpThingHandlerTest.java b/itests/org.openhab.binding.modbus.tests/src/main/java/org/openhab/binding/modbus/tests/ModbusTcpThingHandlerTest.java
index 67c590ae82c2168d782375e78b68deadad845157..25a8a03e6541a9f72cb24eb3336becca4d2e8a80 100644 (file)
--- a/itests/org.openhab.binding.modbus.tests/src/main/java/org/openhab/binding/modbus/tests/ModbusTcpThingHandlerTest.java
+++ b/itests/org.openhab.binding.modbus.tests/src/main/java/org/openhab/binding/modbus/tests/ModbusTcpThingHandlerTest.java
@@ -14,7 +14,7 @@ package org.openhab.binding.modbus.tests;
 
 import static org.hamcrest.CoreMatchers.*;
 import static org.hamcrest.MatcherAssert.assertThat;
-import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.*;
 
 import java.util.Objects;
 
@@ -46,6 +46,7 @@ public class ModbusTcpThingHandlerTest extends AbstractModbusOSGiTest {
 
     @Test
     public void testInitializeAndSlaveEndpoint() throws EndpointNotInitializedException {
+        // Using mocked modbus manager
         Configuration thingConfig = new Configuration();
         thingConfig.put("host", "thisishost");
         thingConfig.put("port", 44);
@@ -81,6 +82,8 @@ public class ModbusTcpThingHandlerTest extends AbstractModbusOSGiTest {
 
     @Test
     public void testTwoDifferentEndpointWithDifferentParameters() {
+        // Real implementation needed to validate this behaviour
+        swapModbusManagerToReal();
         // thing1
         {
             Configuration thingConfig = new Configuration();
@@ -95,6 +98,18 @@ public class ModbusTcpThingHandlerTest extends AbstractModbusOSGiTest {
 
             ModbusTcpThingHandler thingHandler = (ModbusTcpThingHandler) thing.getHandler();
             assertNotNull(thingHandler);
+
+            EndpointPoolConfiguration expectedPoolConfiguration = new EndpointPoolConfiguration();
+            expectedPoolConfiguration.setConnectMaxTries(1);
+            expectedPoolConfiguration.setInterTransactionDelayMillis(1);
+
+            // defaults
+            expectedPoolConfiguration.setConnectTimeoutMillis(10_000);
+            expectedPoolConfiguration.setInterConnectDelayMillis(0);
+            expectedPoolConfiguration.setReconnectAfterMillis(0);
+
+            assertEquals(expectedPoolConfiguration, realModbusManager
+                    .getEndpointPoolConfiguration(new ModbusTCPSlaveEndpoint("thisishost", 44, false)));
         }
         {
             Configuration thingConfig = new Configuration();
@@ -145,6 +160,22 @@ public class ModbusTcpThingHandlerTest extends AbstractModbusOSGiTest {
             assertThat(thing.getStatus(), is(equalTo(ThingStatus.OFFLINE)));
             assertThat(thing.getStatusInfo().getStatusDetail(), is(equalTo(ThingStatusDetail.CONFIGURATION_ERROR)));
         }
+        {
+            //
+            // Ensure the right EndpointPoolConfiguration is still in place
+            //
+            EndpointPoolConfiguration expectedPoolConfiguration = new EndpointPoolConfiguration();
+            expectedPoolConfiguration.setConnectMaxTries(1);
+            expectedPoolConfiguration.setInterTransactionDelayMillis(1); // Note: not 100
+
+            // defaults
+            expectedPoolConfiguration.setConnectTimeoutMillis(10_000);
+            expectedPoolConfiguration.setInterConnectDelayMillis(0);
+            expectedPoolConfiguration.setReconnectAfterMillis(0);
+
+            assertEquals(expectedPoolConfiguration, realModbusManager
+                    .getEndpointPoolConfiguration(new ModbusTCPSlaveEndpoint("thisishost", 44, false)));
+        }
     }
 
     @Test