[modbus] fix defaults for tcp and serial things and some other minor …

…cleanup (openhab#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>

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

bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/internal/config/ModbusSerialConfiguration.java

@@ -24,19 +24,19 @@
 @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() {

bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/internal/config/ModbusTcpConfiguration.java

@@ -25,12 +25,12 @@
 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;
 

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;
 

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)',\

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

@@ -107,13 +107,13 @@ public void receive(Event event) {
  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<>();

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

@@ -14,7 +14,7 @@
 
 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 @@ private static BridgeBuilder createTcpThingBuilder(String id) {
 
  @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 void testInitializeAndSlaveEndpoint() throws EndpointNotInitializedExcept
 
  @Test
  public void testTwoDifferentEndpointWithDifferentParameters() {
+ // Real implementation needed to validate this behaviour
+ swapModbusManagerToReal();
  // thing1
  {
  Configuration thingConfig = new Configuration();
@@ -95,6 +98,18 @@ public void testTwoDifferentEndpointWithDifferentParameters() {
 
  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 void testTwoIdenticalEndpointWithDifferentParameters() {
  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