This is an automated email from the ASF dual-hosted git repository.
chrisdutz pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/plc4x.git
The following commit(s) were added to refs/heads/develop by this push:
new ec721f4ad4 fix(plc4j/modbus): Fixed an issue with optimized modbus
reads that the unit-id was dropped in the process.
ec721f4ad4 is described below
commit ec721f4ad4a83bcb8e4da40e429816538bce8f7b
Author: Christofer Dutz <[email protected]>
AuthorDate: Wed Aug 12 15:42:52 2026 +0200
fix(plc4j/modbus): Fixed an issue with optimized modbus reads that the
unit-id was dropped in the process.
Fixes: #2686
---
.../modbus/base/optimizer/ModbusReadOptimizer.java | 100 ++++++++++++++-------
...mizerTest.java => ModbusReadOptimizerTest.java} | 78 +++++++++++++++-
.../rtu/ModbusRtuConnectionRequestChainTest.java | 45 ++++++++++
3 files changed, 190 insertions(+), 33 deletions(-)
diff --git
a/plc4j/drivers/modbus/src/main/java/org/apache/plc4x/java/modbus/base/optimizer/ModbusReadOptimizer.java
b/plc4j/drivers/modbus/src/main/java/org/apache/plc4x/java/modbus/base/optimizer/ModbusReadOptimizer.java
index ab31210aa0..67f02d5966 100644
---
a/plc4j/drivers/modbus/src/main/java/org/apache/plc4x/java/modbus/base/optimizer/ModbusReadOptimizer.java
+++
b/plc4j/drivers/modbus/src/main/java/org/apache/plc4x/java/modbus/base/optimizer/ModbusReadOptimizer.java
@@ -55,33 +55,47 @@ public class ModbusReadOptimizer {
}
/**
- * Groups tags by type and merges adjacent ones into optimized block reads.
+ * Groups tags by type and unit-id and merges adjacent ones into optimized
block reads.
* Returns a list of OptimizedRead objects, each containing a merged tag
to read
* and the original tag names it covers.
+ * <p>
+ * A block read is addressed to exactly one unit, so tags carrying
different unit-ids must
+ * never end up in the same request, and the unit-id of a group has to be
passed on to the
+ * merged tag (otherwise the connection would fall back to its default
unit-id).
*/
public List<OptimizedRead> optimizeReads(Map<String, ModbusTag>
tagsByName) {
- // Sort tags by type
- TreeMap<String, ModbusTag> coils = new TreeMap<>();
- TreeMap<String, ModbusTag> holdingRegisters = new TreeMap<>();
- TreeMap<String, ModbusTag> inputRegisters = new TreeMap<>();
- TreeMap<String, ModbusTag> extendedRegisters = new TreeMap<>();
- TreeMap<String, ModbusTag> discreteInputs = new TreeMap<>();
+ // Sort tags by type and unit-id
+ Map<Short, TreeMap<String, ModbusTag>> coils = newUnitIdGroups();
+ Map<Short, TreeMap<String, ModbusTag>> holdingRegisters =
newUnitIdGroups();
+ Map<Short, TreeMap<String, ModbusTag>> inputRegisters =
newUnitIdGroups();
+ Map<Short, TreeMap<String, ModbusTag>> extendedRegisters =
newUnitIdGroups();
+ Map<Short, TreeMap<String, ModbusTag>> discreteInputs =
newUnitIdGroups();
for (Map.Entry<String, ModbusTag> entry : tagsByName.entrySet()) {
ModbusTag tag = entry.getValue();
- if (tag instanceof ModbusTagCoil) coils.put(entry.getKey(), tag);
- else if (tag instanceof ModbusTagHoldingRegister)
holdingRegisters.put(entry.getKey(), tag);
- else if (tag instanceof ModbusTagInputRegister)
inputRegisters.put(entry.getKey(), tag);
- else if (tag instanceof ModbusTagExtendedRegister)
extendedRegisters.put(entry.getKey(), tag);
- else if (tag instanceof ModbusTagDiscreteInput)
discreteInputs.put(entry.getKey(), tag);
+ if (tag instanceof ModbusTagCoil) addToUnitIdGroup(coils, entry);
+ else if (tag instanceof ModbusTagHoldingRegister)
addToUnitIdGroup(holdingRegisters, entry);
+ else if (tag instanceof ModbusTagInputRegister)
addToUnitIdGroup(inputRegisters, entry);
+ else if (tag instanceof ModbusTagExtendedRegister)
addToUnitIdGroup(extendedRegisters, entry);
+ else if (tag instanceof ModbusTagDiscreteInput)
addToUnitIdGroup(discreteInputs, entry);
}
List<OptimizedRead> result = new ArrayList<>();
- if (!coils.isEmpty()) result.addAll(optimizeCoils(coils));
- if (!holdingRegisters.isEmpty())
result.addAll(optimizeRegisters(holdingRegisters,
ModbusReadOptimizer::createHoldingRegister));
- if (!inputRegisters.isEmpty())
result.addAll(optimizeRegisters(inputRegisters,
ModbusReadOptimizer::createInputRegister));
- if (!extendedRegisters.isEmpty())
result.addAll(optimizeRegisters(extendedRegisters,
ModbusReadOptimizer::createExtendedRegister));
- if (!discreteInputs.isEmpty())
result.addAll(optimizeCoils(discreteInputs));
+ for (Map.Entry<Short, TreeMap<String, ModbusTag>> group :
coils.entrySet()) {
+ result.addAll(optimizeCoils(group.getKey(), group.getValue()));
+ }
+ for (Map.Entry<Short, TreeMap<String, ModbusTag>> group :
holdingRegisters.entrySet()) {
+ result.addAll(optimizeRegisters(group.getKey(), group.getValue(),
ModbusReadOptimizer::createHoldingRegister));
+ }
+ for (Map.Entry<Short, TreeMap<String, ModbusTag>> group :
inputRegisters.entrySet()) {
+ result.addAll(optimizeRegisters(group.getKey(), group.getValue(),
ModbusReadOptimizer::createInputRegister));
+ }
+ for (Map.Entry<Short, TreeMap<String, ModbusTag>> group :
extendedRegisters.entrySet()) {
+ result.addAll(optimizeRegisters(group.getKey(), group.getValue(),
ModbusReadOptimizer::createExtendedRegister));
+ }
+ for (Map.Entry<Short, TreeMap<String, ModbusTag>> group :
discreteInputs.entrySet()) {
+ result.addAll(optimizeCoils(group.getKey(), group.getValue()));
+ }
return result;
}
@@ -156,7 +170,29 @@ public class ModbusReadOptimizer {
// Internal
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
- private List<OptimizedRead> optimizeCoils(Map<String, ModbusTag>
tagsByName) {
+ /**
+ * Creates a map for grouping tags by their unit-id. Tags without an
explicit unit-id (which
+ * will be read using the connection's default unit-id) are grouped under
the {@code null} key.
+ */
+ private static Map<Short, TreeMap<String, ModbusTag>> newUnitIdGroups() {
+ return new TreeMap<>(Comparator.nullsFirst(Comparator.naturalOrder()));
+ }
+
+ private static void addToUnitIdGroup(Map<Short, TreeMap<String,
ModbusTag>> groups,
+ Map.Entry<String, ModbusTag> entry) {
+ groups.computeIfAbsent(entry.getValue().getUnitId(), unitId -> new
TreeMap<>())
+ .put(entry.getKey(), entry.getValue());
+ }
+
+ /**
+ * Builds the config of a merged tag, so that the unit-id of the tags it
was built from is
+ * used when sending the request.
+ */
+ private static Map<String, String> mergedTagConfig(Short unitId) {
+ return unitId == null ? Collections.emptyMap() :
Collections.singletonMap("unit-id", unitId.toString());
+ }
+
+ private List<OptimizedRead> optimizeCoils(Short unitId, Map<String,
ModbusTag> tagsByName) {
// Sort by address
List<Map.Entry<String, ModbusTag>> sorted = new
ArrayList<>(tagsByName.entrySet());
sorted.sort(Comparator.comparingInt(e -> e.getValue().getAddress()));
@@ -184,8 +220,8 @@ public class ModbusReadOptimizer {
// Finish current group
boolean isDiscreteInput =
currentGroup.values().iterator().next() instanceof ModbusTagDiscreteInput;
ModbusTag mergedTag = isDiscreteInput
- ? new ModbusTagDiscreteInput(firstAddress, lastAddress -
firstAddress, ModbusDataType.BYTE, Collections.emptyMap())
- : new ModbusTagCoil(firstAddress, lastAddress -
firstAddress, ModbusDataType.BYTE, Collections.emptyMap());
+ ? new ModbusTagDiscreteInput(firstAddress, lastAddress -
firstAddress, ModbusDataType.BYTE, mergedTagConfig(unitId))
+ : new ModbusTagCoil(firstAddress, lastAddress -
firstAddress, ModbusDataType.BYTE, mergedTagConfig(unitId));
result.add(new OptimizedRead(mergedTag, currentGroup));
// Start new group
@@ -203,14 +239,14 @@ public class ModbusReadOptimizer {
if (!currentGroup.isEmpty()) {
boolean isDiscreteInput = currentGroup.values().iterator().next()
instanceof ModbusTagDiscreteInput;
ModbusTag mergedTag = isDiscreteInput
- ? new ModbusTagDiscreteInput(firstAddress, lastAddress -
firstAddress, ModbusDataType.BYTE, Collections.emptyMap())
- : new ModbusTagCoil(firstAddress, lastAddress - firstAddress,
ModbusDataType.BYTE, Collections.emptyMap());
+ ? new ModbusTagDiscreteInput(firstAddress, lastAddress -
firstAddress, ModbusDataType.BYTE, mergedTagConfig(unitId))
+ : new ModbusTagCoil(firstAddress, lastAddress - firstAddress,
ModbusDataType.BYTE, mergedTagConfig(unitId));
result.add(new OptimizedRead(mergedTag, currentGroup));
}
return result;
}
- private List<OptimizedRead> optimizeRegisters(Map<String, ModbusTag>
tagsByName, TagFactory tagFactory) {
+ private List<OptimizedRead> optimizeRegisters(Short unitId, Map<String,
ModbusTag> tagsByName, TagFactory tagFactory) {
List<Map.Entry<String, ModbusTag>> sorted = new
ArrayList<>(tagsByName.entrySet());
sorted.sort(Comparator.comparingInt(e -> e.getValue().getAddress()));
@@ -236,7 +272,7 @@ public class ModbusReadOptimizer {
if (tagEnd > maxRegister) {
// Finish current group
result.add(new OptimizedRead(
- tagFactory.createTag(firstRegister, lastRegister -
firstRegister, ModbusDataType.WORD),
+ tagFactory.createTag(firstRegister, lastRegister -
firstRegister, ModbusDataType.WORD, mergedTagConfig(unitId)),
currentGroup));
// Start new group
@@ -252,22 +288,22 @@ public class ModbusReadOptimizer {
if (!currentGroup.isEmpty()) {
result.add(new OptimizedRead(
- tagFactory.createTag(firstRegister, lastRegister -
firstRegister, ModbusDataType.WORD),
+ tagFactory.createTag(firstRegister, lastRegister -
firstRegister, ModbusDataType.WORD, mergedTagConfig(unitId)),
currentGroup));
}
return result;
}
- private static ModbusTag createHoldingRegister(int address, int count,
ModbusDataType dataType) {
- return new ModbusTagHoldingRegister(address, count, dataType,
Collections.emptyMap());
+ private static ModbusTag createHoldingRegister(int address, int count,
ModbusDataType dataType, Map<String, String> config) {
+ return new ModbusTagHoldingRegister(address, count, dataType, config);
}
- private static ModbusTag createInputRegister(int address, int count,
ModbusDataType dataType) {
- return new ModbusTagInputRegister(address, count, dataType,
Collections.emptyMap());
+ private static ModbusTag createInputRegister(int address, int count,
ModbusDataType dataType, Map<String, String> config) {
+ return new ModbusTagInputRegister(address, count, dataType, config);
}
- private static ModbusTag createExtendedRegister(int address, int count,
ModbusDataType dataType) {
- return new ModbusTagExtendedRegister(address, count, dataType,
Collections.emptyMap());
+ private static ModbusTag createExtendedRegister(int address, int count,
ModbusDataType dataType, Map<String, String> config) {
+ return new ModbusTagExtendedRegister(address, count, dataType, config);
}
private static byte[] byteSwap(byte[] in) {
@@ -297,7 +333,7 @@ public class ModbusReadOptimizer {
@FunctionalInterface
private interface TagFactory {
- ModbusTag createTag(int address, int count, ModbusDataType dataType);
+ ModbusTag createTag(int address, int count, ModbusDataType dataType,
Map<String, String> config);
}
}
diff --git
a/plc4j/drivers/modbus/src/test/java/org/apache/plc4x/java/modbus/base/optimizer/ModbusOptimizerTest.java
b/plc4j/drivers/modbus/src/test/java/org/apache/plc4x/java/modbus/base/optimizer/ModbusReadOptimizerTest.java
similarity index 70%
rename from
plc4j/drivers/modbus/src/test/java/org/apache/plc4x/java/modbus/base/optimizer/ModbusOptimizerTest.java
rename to
plc4j/drivers/modbus/src/test/java/org/apache/plc4x/java/modbus/base/optimizer/ModbusReadOptimizerTest.java
index 2e923a69e4..1a17cdb00d 100644
---
a/plc4j/drivers/modbus/src/test/java/org/apache/plc4x/java/modbus/base/optimizer/ModbusOptimizerTest.java
+++
b/plc4j/drivers/modbus/src/test/java/org/apache/plc4x/java/modbus/base/optimizer/ModbusReadOptimizerTest.java
@@ -23,6 +23,7 @@ import org.apache.plc4x.java.modbus.base.tag.ModbusTagCoil;
import org.apache.plc4x.java.modbus.base.tag.ModbusTagHoldingRegister;
import org.apache.plc4x.java.modbus.readwrite.ModbusDataType;
import org.apache.plc4x.java.modbus.types.ModbusByteOrder;
+import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;
@@ -35,7 +36,7 @@ import java.util.stream.Stream;
import static org.junit.jupiter.api.Assertions.*;
-class ModbusOptimizerTest {
+class ModbusReadOptimizerTest {
private static Stream<Arguments> coilInputData() {
return Stream.of(
@@ -167,6 +168,81 @@ class ModbusOptimizerTest {
processReadRequest(tags, check);
}
+ /**
+ * Tags addressing different units may never be merged into a single block
read, and the
+ * unit-id of a group has to be carried over to the merged tag - otherwise
the connection
+ * would silently fall back to its default unit-id (see GitHub issue
#2686).
+ */
+ @Test
+ void registersWithDifferentUnitIdsAreNotMerged() {
+ processReadRequest(new ModbusTag[]{
+ new ModbusTagHoldingRegister(0, 1, ModbusDataType.INT,
Collections.singletonMap("unit-id", "2")),
+ new ModbusTagHoldingRegister(1, 1, ModbusDataType.INT,
Collections.singletonMap("unit-id", "3"))
+ },
+ optimizedReads -> {
+ assertEquals(2, optimizedReads.size());
+ // The addresses would have been adjacent, so without the
unit-id they'd be one block.
+ ModbusTag first = optimizedReads.getFirst().mergedTag;
+ assertEquals((short) 2, first.getUnitId());
+ assertEquals(0, first.getAddress());
+ assertEquals(1, first.getNumberOfElements());
+ ModbusTag second = optimizedReads.get(1).mergedTag;
+ assertEquals((short) 3, second.getUnitId());
+ assertEquals(1, second.getAddress());
+ assertEquals(1, second.getNumberOfElements());
+ });
+ }
+
+ @Test
+ void coilsWithDifferentUnitIdsAreNotMerged() {
+ processReadRequest(new ModbusTag[]{
+ new ModbusTagCoil(0, 1, ModbusDataType.BOOL,
Collections.singletonMap("unit-id", "2")),
+ new ModbusTagCoil(1, 1, ModbusDataType.BOOL,
Collections.singletonMap("unit-id", "3"))
+ },
+ optimizedReads -> {
+ assertEquals(2, optimizedReads.size());
+ assertEquals((short) 2,
optimizedReads.getFirst().mergedTag.getUnitId());
+ assertEquals((short) 3,
optimizedReads.get(1).mergedTag.getUnitId());
+ });
+ }
+
+ /**
+ * Tags of the same unit are still merged, and the merged tag keeps the
unit-id.
+ */
+ @Test
+ void registersWithSameUnitIdAreMergedKeepingTheUnitId() {
+ processReadRequest(new ModbusTag[]{
+ new ModbusTagHoldingRegister(0, 1, ModbusDataType.INT,
Collections.singletonMap("unit-id", "7")),
+ new ModbusTagHoldingRegister(1, 1, ModbusDataType.INT,
Collections.singletonMap("unit-id", "7"))
+ },
+ optimizedReads -> {
+ assertEquals(1, optimizedReads.size());
+ ModbusTag mergedTag = optimizedReads.getFirst().mergedTag;
+ assertEquals((short) 7, mergedTag.getUnitId());
+ assertEquals(0, mergedTag.getAddress());
+ assertEquals(2, mergedTag.getNumberOfElements());
+ });
+ }
+
+ /**
+ * Tags with an explicit unit-id must not be mixed with tags that use the
connection default.
+ */
+ @Test
+ void tagsWithoutUnitIdAreNotMergedWithTagsHavingOne() {
+ processReadRequest(new ModbusTag[]{
+ new ModbusTagHoldingRegister(0, 1, ModbusDataType.INT,
Collections.emptyMap()),
+ new ModbusTagHoldingRegister(1, 1, ModbusDataType.INT,
Collections.singletonMap("unit-id", "2"))
+ },
+ optimizedReads -> {
+ assertEquals(2, optimizedReads.size());
+ // Tags without a unit-id are grouped first and keep using the
connection default.
+ assertNull(optimizedReads.getFirst().mergedTag.getUnitId());
+ assertEquals(0,
optimizedReads.getFirst().mergedTag.getAddress());
+ assertEquals((short) 2,
optimizedReads.get(1).mergedTag.getUnitId());
+ assertEquals(1, optimizedReads.get(1).mergedTag.getAddress());
+ });
+ }
+
void processReadRequest(ModbusTag[] tags, CheckResult check) {
ModbusReadOptimizer optimizer = new ModbusReadOptimizer(2000, 125,
ModbusByteOrder.BIG_ENDIAN);
LinkedHashMap<String, ModbusTag> tagMap = new LinkedHashMap<>();
diff --git
a/plc4j/drivers/modbus/src/test/java/org/apache/plc4x/java/modbus/rtu/ModbusRtuConnectionRequestChainTest.java
b/plc4j/drivers/modbus/src/test/java/org/apache/plc4x/java/modbus/rtu/ModbusRtuConnectionRequestChainTest.java
index 3a93031132..3767db619e 100644
---
a/plc4j/drivers/modbus/src/test/java/org/apache/plc4x/java/modbus/rtu/ModbusRtuConnectionRequestChainTest.java
+++
b/plc4j/drivers/modbus/src/test/java/org/apache/plc4x/java/modbus/rtu/ModbusRtuConnectionRequestChainTest.java
@@ -39,6 +39,7 @@ import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ExecutionException;
+import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
@@ -215,6 +216,43 @@ class ModbusRtuConnectionRequestChainTest {
"a request that died in the queue must never reach the wire");
}
+ /**
+ * A tag-level unit-id must end up on the wire instead of the connection
default,
+ * for reads as well as writes (GitHub issue #2686). The connection
default here is 1,
+ * so a frame addressed to unit 7 can only come from the tag.
+ */
+ @Test
+ void writeRequestUsesTheTagUnitIdOnTheWire() throws Exception {
+ ScriptedAsyncTransport transport = new ScriptedAsyncTransport();
+ ModbusRtuConnection connection = newConnectedConnection(transport);
+
+ connection.writeRequestBuilder()
+ .addTagAddress("value", "4x00001:INT{unit-id: 7}", 42)
+ .build()
+ .execute();
+
+ awaitTrue(() -> transport.writeCount() == 1, 2, TimeUnit.SECONDS);
+ assertEquals(7, transport.lastWrittenFrame()[0] & 0xFF,
+ "write must be addressed to the tag's unit id, not the connection
default");
+ }
+
+ @Test
+ void readRequestUsesTheTagUnitIdOnTheWire() throws Exception {
+ ScriptedAsyncTransport transport = new ScriptedAsyncTransport();
+ ModbusRtuConnection connection = newConnectedConnection(transport);
+
+ connection.readRequestBuilder()
+ .addTagAddress("value", "4x00001:INT{unit-id: 7}")
+ .build()
+ .execute();
+
+ // Pre-fix the optimizer dropped the unit-id when building the
block-read tag,
+ // so this frame went out addressed to the connection default (1).
+ awaitTrue(() -> transport.writeCount() == 1, 2, TimeUnit.SECONDS);
+ assertEquals(7, transport.lastWrittenFrame()[0] & 0xFF,
+ "read must be addressed to the tag's unit id, not the connection
default");
+ }
+
/**
* Builds a connection wired to the given fake transport, and drives it
* through the same construction/connect path as {@code
ModbusRtuConnectionTest}.
@@ -281,6 +319,7 @@ class ModbusRtuConnectionRequestChainTest {
private int readPosition;
private boolean open = true;
private final AtomicInteger writeCount = new AtomicInteger();
+ private final List<byte[]> writtenFrames = new
CopyOnWriteArrayList<>();
private final AtomicBoolean failNextWrite = new AtomicBoolean(false);
private final AtomicBoolean failAllWrites = new AtomicBoolean(false);
private final AtomicReference<Runnable> dataListener = new
AtomicReference<>();
@@ -293,6 +332,11 @@ class ModbusRtuConnectionRequestChainTest {
return writeCount.get();
}
+ /** The bytes of the last frame handed to the transport (an RTU ADU
starts with the unit id). */
+ byte[] lastWrittenFrame() {
+ return writtenFrames.isEmpty() ? null :
writtenFrames.get(writtenFrames.size() - 1);
+ }
+
void failNextWrite() {
failNextWrite.set(true);
}
@@ -349,6 +393,7 @@ class ModbusRtuConnectionRequestChainTest {
@Override
public void write(byte[] bytes) throws TransportException {
writeCount.incrementAndGet();
+ writtenFrames.add(bytes.clone());
if (failAllWrites.get() || failNextWrite.compareAndSet(true,
false)) {
throw new TransportException("scripted write failure");
}
