From fbf55d5886a4a6440645acd2768a5d1a4ecd93e5 Mon Sep 17 00:00:00 2001 From: Sami Salonen Date: Wed, 17 Feb 2021 12:41:40 +0200 Subject: [PATCH] [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 --- bundles/org.openhab.binding.modbus/README.md | 2 +- .../config/ModbusSerialConfiguration.java | 16 ++++----- .../config/ModbusTcpConfiguration.java | 8 ++--- .../AbstractModbusEndpointThingHandler.java | 2 +- .../itest.bndrun | 2 -- .../modbus/tests/AbstractModbusOSGiTest.java | 2 +- .../tests/ModbusTcpThingHandlerTest.java | 33 ++++++++++++++++++- 7 files changed, 47 insertions(+), 18 deletions(-) diff --git a/bundles/org.openhab.binding.modbus/README.md b/bundles/org.openhab.binding.modbus/README.md index c6f561aa4..ae30bbe53 100644 --- 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 32804d38c..8e81973a0 100644 --- 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 28ff1d2a2..f565f2082 100644 --- 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 ceb2e7f8f..35e6dd1c6 100644 --- 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 addedItems = new HashSet<>(); private Set addedThings = new HashSet<>(); private Set 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 67c590ae8..25a8a03e6 100644 --- 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