You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@plc4x.apache.org by sr...@apache.org on 2026/09/11 15:51:14 UTC
(plc4x) 01/03: fix(plc4j/modbus): size a merged block from the tag's own register count
This is an automated email from the ASF dual-hosted git repository.
sruehl pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/plc4x.git
commit 06c79686bf69fac8e1fbb0f8b547c272a2390485
Author: Sebastian Rühl <sr...@apache.org>
AuthorDate: Fri Sep 11 17:50:29 2026 +0200
fix(plc4j/modbus): size a merged block from the tag's own register count
optimizeRegisters derived a tag's span as ceil(dataTypeSize / 2) multiplied by
the element count, which ignores stringLength. getDataTypeSize() is 1 for STRING
and CHAR, so a lone STRING(5) tag reserved a single register where it occupies
three: the merged block then asked the device for less than the tag needs and
the tag decoded from registers that were never sent.
ModbusTag.getLengthWords() already computes this correctly -- it folds in the
element count and rounds the packed byte total up to whole registers -- and is
what splitResponse uses to slice the response back apart. Using it here makes
the two agree, which is the property that matters: the block a tag is placed in
must cover what splitting that block will later read out of it.
It is also tighter in the other direction, where the old formula over-reserved:
CHAR[3] occupies two registers, not three.
optimizeCoils had the same shape, deriving a span from the data type rather than
the tag. It could only over-reserve, never under-request, so it was not a decode
bug -- but the two branches now follow one rule, and a coil block matches what an
unoptimized read of the same tag would ask for.
Verified: ModbusReadOptimizerTest, 25 tests, 0 failures.
---
.../modbus/base/optimizer/ModbusReadOptimizer.java | 17 ++++--
.../base/optimizer/ModbusReadOptimizerTest.java | 63 ++++++++++++++++++++++
2 files changed, 76 insertions(+), 4 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 4b72c7bb4f..9271a27c3b 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
@@ -262,8 +262,12 @@ public class ModbusReadOptimizer {
for (Map.Entry<String, ModbusTag> entry : sorted) {
ModbusTag tag = entry.getValue();
- int sizeInCoils = tag.getDataType() == ModbusDataType.BOOL ? 1 : tag.getDataType().getDataTypeSize() * 8;
- int tagEnd = tag.getAddress() + (sizeInCoils * tag.getNumberOfElements());
+ // One coil per element, which is exactly the quantity an unoptimized read of the same
+ // tag puts into its request (see the connections' readRequestPdu). Reserving eight
+ // coils per byte of the data type instead would cover a tag that cannot be answered
+ // anyway - anything but BOOL is reported UNSUPPORTED in splitResponse - while pushing
+ // the block over a device's coil count and so failing the BOOL tags sharing it.
+ int tagEnd = tag.getAddress() + tag.getNumberOfElements();
if (firstAddress == -1) {
firstAddress = tag.getAddress();
@@ -329,8 +333,13 @@ public class ModbusReadOptimizer {
for (Map.Entry<String, ModbusTag> entry : sorted) {
ModbusTag tag = entry.getValue();
- int sizeInRegisters = (int) Math.ceil((double) tag.getDataType().getDataTypeSize() / 2);
- int tagEnd = tag.getAddress() + (sizeInRegisters * tag.getNumberOfElements());
+ // The tag's own span, which is the quantity an unoptimized read of it puts into its
+ // request and the width splitResponse slices back out. Deriving the span from the data
+ // type instead misses everything the type does not carry: a string's length lives in
+ // the address, and ModbusDataType.getDataTypeSize() answers 1 for STRING, so a lone
+ // STRING(5) reserved a single register while occupying three - the block then
+ // under-requested and the tag decoded registers the device never sent.
+ int tagEnd = tag.getAddress() + tag.getLengthWords();
if (firstRegister == -1) {
firstRegister = tag.getAddress();
diff --git a/plc4j/drivers/modbus/src/test/java/org/apache/plc4x/java/modbus/base/optimizer/ModbusReadOptimizerTest.java b/plc4j/drivers/modbus/src/test/java/org/apache/plc4x/java/modbus/base/optimizer/ModbusReadOptimizerTest.java
index 6369f7f177..d1bb6c102a 100644
--- a/plc4j/drivers/modbus/src/test/java/org/apache/plc4x/java/modbus/base/optimizer/ModbusReadOptimizerTest.java
+++ b/plc4j/drivers/modbus/src/test/java/org/apache/plc4x/java/modbus/base/optimizer/ModbusReadOptimizerTest.java
@@ -253,6 +253,69 @@ class ModbusReadOptimizerTest {
});
}
+ /**
+ * A string declares its length in the address, not in its data type - ModbusDataType answers 1
+ * for STRING - so a block cut to the type's size asks for a single register while the tag
+ * occupies three, and the tag then decodes registers the device never sent.
+ */
+ @Test
+ void aLoneStringIsReadWithItsWholeSpan() {
+ processReadRequest(new ModbusTag[]{
+ // Five characters, packed two to a register: three registers.
+ new ModbusTagHoldingRegister(1, 1, 5, ModbusDataType.STRING, Collections.emptyMap())
+ },
+ optimizedReads -> {
+ assertEquals(1, optimizedReads.size());
+ ModbusTag mergedTag = optimizedReads.getFirst().mergedTag;
+ assertInstanceOf(ModbusTagHoldingRegister.class, mergedTag);
+ assertEquals(1, mergedTag.getAddress());
+ // The quantity that goes on the wire (see the connections' readRequestPdu).
+ assertEquals(3, mergedTag.getLengthWords());
+ assertEquals(3, mergedTag.getNumberOfElements());
+ });
+ }
+
+ /**
+ * The same where the string is merged with a neighbour: the block ends where the string ends,
+ * so a span read off the data type cuts the block off inside the string.
+ */
+ @Test
+ void aStringMergedWithANeighbourKeepsItsWholeSpan() {
+ processReadRequest(new ModbusTag[]{
+ // The neighbour comes first, so the block's end is the string's own.
+ new ModbusTagHoldingRegister(1, 1, ModbusDataType.INT, Collections.emptyMap()),
+ new ModbusTagHoldingRegister(2, 1, 5, ModbusDataType.STRING, Collections.emptyMap())
+ },
+ optimizedReads -> {
+ assertEquals(1, optimizedReads.size());
+ ModbusTag mergedTag = optimizedReads.getFirst().mergedTag;
+ assertEquals(1, mergedTag.getAddress());
+ // One register for the neighbour plus the string's three.
+ assertEquals(4, mergedTag.getLengthWords());
+ assertEquals(4, mergedTag.getNumberOfElements());
+ });
+ }
+
+ /**
+ * A coil block is cut in coils, one per element - the quantity an unoptimized read of the tag
+ * asks for. A coil carries a single bit and anything wider is answered UNSUPPORTED, so a
+ * non-BOOL coil tag must not reserve one coil per bit of its data type: the block would reach
+ * past the coils the device has and fail the BOOL tags sharing it, for data nobody can decode.
+ */
+ @Test
+ void aNonBoolCoilDoesNotWidenTheBlock() {
+ processReadRequest(new ModbusTag[]{
+ new ModbusTagCoil(0, 1, ModbusDataType.INT, Collections.emptyMap())
+ },
+ optimizedReads -> {
+ assertEquals(1, optimizedReads.size());
+ ModbusTag mergedTag = optimizedReads.getFirst().mergedTag;
+ assertInstanceOf(ModbusTagCoil.class, mergedTag);
+ assertEquals(0, mergedTag.getAddress());
+ assertEquals(1, mergedTag.getNumberOfElements());
+ });
+ }
+
/**
* A coil array (BOOL[n]) has to yield all n values, not just the first one - see GH-2060.
* The block read already covers the whole array, so the data is on the wire either way.