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:10:14 UTC
(plc4x) 02/05: fix(plc4j/modbus): two defects in the optimized read path
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 c68e15eca14057dfdd3308fdc934d56f0b027360
Author: Sebastian Rühl <sr...@apache.org>
AuthorDate: Fri Sep 11 16:55:59 2026 +0200
fix(plc4j/modbus): two defects in the optimized read path
Both found while porting this optimizer to plc4go, and both verified failing
first against the unfixed file.
1. Extended-register blocks were merged using maxRegistersPerRequest, the
plain-register ceiling of 125. FC 0x14 (Read File Record) cannot answer that
much: the response is function code (1) + response data length (1) + per item
[data length (1) + reference type (1) + 2 bytes per register], so 125
registers come back as 254 bytes, one past the 253-byte PDU limit -- and a
block straddling a 10000-register file boundary becomes two items and
overflows by more. The optimizer was manufacturing requests no device can
answer out of tags that were individually fine. Extended registers now get
their own ceiling of 123, the widest that fits both the one-item and the
two-item shape; a configured value below that still wins.
2. splitResponse sliced a member out of the block using getLengthBytes(), the
tag's packed payload length. For any tag whose payload is an ODD number of
bytes -- a scalar CHAR, a STRING of odd declared length -- the byte swap
applied for the *_BYTE_SWAP orders cannot move the trailing byte, while an
unoptimized read, handed whole registers, does move it. The same tag
therefore decoded a DIFFERENT value depending on whether it happened to be
merged, silently, under an OK response code. It now slices whole registers
via getLengthWords(), and a block too short for the member is reported as
INTERNAL_ERROR for that tag rather than throwing out of the response.
Tests: extended registers 125 apart must not merge while the same addresses as
holding registers still do; a 123-register span is still merged so the narrower
ceiling costs no round trip; every emitted block answers within 253 bytes,
including the straddling shape; and a CHAR and an odd-length STRING read through
the optimizer under BIG_ENDIAN_BYTE_SWAP decode the same value an unoptimized
read gives.
Adjacent defect NOT fixed here, flagged for a separate change: optimizeRegisters
computes a tag's span as ceil(dataTypeSize / 2) * numberOfElements and ignores
stringLength, so a lone STRING(5) tag reserves one register where it needs three
and the merged read under-requests.
---
.../modbus/base/optimizer/ModbusReadOptimizer.java | 57 ++++++-
.../base/optimizer/ModbusReadOptimizerTest.java | 188 +++++++++++++++++++++
2 files changed, 238 insertions(+), 7 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 99cea81aca..4b72c7bb4f 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
@@ -49,6 +49,20 @@ public class ModbusReadOptimizer {
private static final Logger LOGGER = LoggerFactory.getLogger(ModbusReadOptimizer.class);
+ /**
+ * The widest extended-register block whose response still fits the 253-byte Modbus PDU limit.
+ * <p>
+ * Extended registers travel over FC 0x14 (Read File Record), whose response carries a byte
+ * count and then per-item framing that the plain register function codes do not:
+ * 1 (function code) + 1 (response data length) + per item [1 (data length) + 1 (reference
+ * type) + 2 bytes per register]. A 125-register block therefore answers with
+ * 1 + 1 + 1 + 1 + 250 = 254 bytes, one past the limit. A block straddling a 10000-register
+ * file boundary is split into two items (see the extended-register handling in the
+ * connections) and answers with 1 + 1 + 2 * (1 + 1) + 250 = 256 bytes, overflowing by more.
+ * 123 registers is the widest that fits both shapes: 250 bytes as one item, 252 as two.
+ */
+ private static final int MAX_EXTENDED_REGISTERS_PER_REQUEST = 123;
+
private final int maxCoilsPerRequest;
private final int maxRegistersPerRequest;
private final ModbusByteOrder defaultByteOrder;
@@ -90,13 +104,13 @@ public class ModbusReadOptimizer {
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));
+ result.addAll(optimizeRegisters(group.getKey(), group.getValue(), maxRegistersPerRequest, ModbusReadOptimizer::createHoldingRegister));
}
for (Map.Entry<Short, TreeMap<String, ModbusTag>> group : inputRegisters.entrySet()) {
- result.addAll(optimizeRegisters(group.getKey(), group.getValue(), ModbusReadOptimizer::createInputRegister));
+ result.addAll(optimizeRegisters(group.getKey(), group.getValue(), maxRegistersPerRequest, ModbusReadOptimizer::createInputRegister));
}
for (Map.Entry<Short, TreeMap<String, ModbusTag>> group : extendedRegisters.entrySet()) {
- result.addAll(optimizeRegisters(group.getKey(), group.getValue(), ModbusReadOptimizer::createExtendedRegister));
+ result.addAll(optimizeRegisters(group.getKey(), group.getValue(), maxExtendedRegistersPerRequest(), ModbusReadOptimizer::createExtendedRegister));
}
for (Map.Entry<Short, TreeMap<String, ModbusTag>> group : discreteInputs.entrySet()) {
result.addAll(optimizeCoils(group.getKey(), group.getValue()));
@@ -158,7 +172,22 @@ public class ModbusReadOptimizer {
} else {
// Registers: byte-level extraction
int byteOffset = (originalTag.getAddress() - blockTag.getAddress()) * 2;
- int byteLength = originalTag.getLengthBytes();
+ // Whole REGISTERS, not the tag's packed byte length. The two differ for any
+ // type whose payload is an odd number of bytes - a scalar CHAR, a STRING of
+ // odd declared length - and the difference is not cosmetic: the byte swap
+ // below can only swap whole pairs, so an odd-length slice leaves its last
+ // byte unswapped while the unoptimized read, which is handed the device's
+ // whole registers, swaps it. Slicing to the payload length would therefore
+ // decode a DIFFERENT value than the same tag read on its own, silently and
+ // with an OK response code. The parser ignores the bytes it does not need,
+ // which is exactly what the unoptimized path already relies on, and the block
+ // covers the wider slice because its span reserves whole registers too.
+ int byteLength = originalTag.getLengthWords() * 2;
+ if (byteOffset + byteLength > blockData.length) {
+ // The device answered with fewer registers than we asked for.
+ result.put(tagName, new DefaultPlcResponseItem<>(PlcResponseCode.INTERNAL_ERROR, null));
+ continue;
+ }
byte[] tagData = new byte[byteLength];
System.arraycopy(blockData, byteOffset, tagData, 0, byteLength);
@@ -274,7 +303,21 @@ public class ModbusReadOptimizer {
return result;
}
- private List<OptimizedRead> optimizeRegisters(Short unitId, Map<String, ModbusTag> tagsByName, TagFactory tagFactory) {
+ /**
+ * The ceiling that applies to a block of extended registers: the configured register limit,
+ * capped at what an FC 0x14 response can carry back.
+ */
+ private int maxExtendedRegistersPerRequest() {
+ return Math.min(maxRegistersPerRequest, MAX_EXTENDED_REGISTERS_PER_REQUEST);
+ }
+
+ /**
+ * Cuts one group of register tags into blocks no wider than {@code maxPerRequest} registers.
+ * The ceiling is passed in rather than read off the field, because extended registers answer
+ * over a different function code and so tolerate a narrower block than the other two areas
+ * (see {@link #MAX_EXTENDED_REGISTERS_PER_REQUEST}).
+ */
+ private List<OptimizedRead> optimizeRegisters(Short unitId, Map<String, ModbusTag> tagsByName, int maxPerRequest, TagFactory tagFactory) {
List<Map.Entry<String, ModbusTag>> sorted = new ArrayList<>(tagsByName.entrySet());
sorted.sort(Comparator.comparingInt(e -> e.getValue().getAddress()));
@@ -292,7 +335,7 @@ public class ModbusReadOptimizer {
if (firstRegister == -1) {
firstRegister = tag.getAddress();
lastRegister = tagEnd;
- maxRegister = tag.getAddress() + maxRegistersPerRequest;
+ maxRegister = tag.getAddress() + maxPerRequest;
currentGroup.put(entry.getKey(), tag);
continue;
}
@@ -307,7 +350,7 @@ public class ModbusReadOptimizer {
currentGroup = new LinkedHashMap<>();
firstRegister = tag.getAddress();
lastRegister = tagEnd;
- maxRegister = tag.getAddress() + maxRegistersPerRequest;
+ maxRegister = tag.getAddress() + maxPerRequest;
} else {
lastRegister = Math.max(lastRegister, tagEnd);
}
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 0020ac10a0..6369f7f177 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
@@ -18,13 +18,17 @@
*/
package org.apache.plc4x.java.modbus.base.optimizer;
+import org.apache.plc4x.java.modbus.base.ModbusRegisterCodec;
import org.apache.plc4x.java.modbus.base.tag.ModbusTag;
import org.apache.plc4x.java.modbus.base.tag.ModbusTagCoil;
+import org.apache.plc4x.java.modbus.base.tag.ModbusTagExtendedRegister;
import org.apache.plc4x.java.modbus.base.tag.ModbusTagHoldingRegister;
import org.apache.plc4x.java.api.types.PlcResponseCode;
import org.apache.plc4x.java.api.value.PlcValue;
import org.apache.plc4x.java.modbus.readwrite.ModbusDataType;
import org.apache.plc4x.java.modbus.types.ModbusByteOrder;
+import org.apache.plc4x.java.spi.buffers.api.exceptions.BufferException;
+import org.apache.plc4x.java.spi.buffers.bytebased.ReadBufferByteBased;
import org.apache.plc4x.java.spi.drivers.messages.items.PlcResponseItem;
import org.apache.plc4x.java.spi.values.PlcBOOL;
import org.apache.plc4x.java.spi.values.PlcList;
@@ -333,6 +337,190 @@ class ModbusReadOptimizerTest {
assertEquals(PlcResponseCode.INTERNAL_ERROR, response.get("tag0").getResponseCode());
}
+ /** Selects the byte-swapping big endian order, which a tag may ask for by itself. */
+ private static final Map<String, String> BIG_ENDIAN_BYTE_SWAP =
+ Collections.singletonMap("byte-order", ModbusByteOrder.BIG_ENDIAN_BYTE_SWAP.name());
+
+ /**
+ * A CHAR occupies a whole register but carries one byte, so a slice cut to its payload length
+ * cannot be swapped in pairs - the byte would stay where it is, while the unoptimized read,
+ * which is handed the whole register, swaps it. Merging a tag into a block must not change the
+ * value it decodes to.
+ */
+ @Test
+ void oddLengthScalarIsSwappedTheSameWayAnUnoptimizedReadSwapsIt() throws BufferException {
+ ModbusTag tag = new ModbusTagHoldingRegister(1, 1, ModbusDataType.CHAR, BIG_ENDIAN_BYTE_SWAP);
+ byte[] registers = {'A', 'B'};
+
+ Map<String, PlcResponseItem<PlcValue>> response = splitSingleRead(tag, registers);
+
+ assertEquals(PlcResponseCode.OK, response.get("tag0").getResponseCode());
+ assertEquals("B", unoptimizedRead(tag, registers).getString());
+ assertEquals(unoptimizedRead(tag, registers).getString(), response.get("tag0").getValue().getString());
+ }
+
+ /**
+ * The same for a string of odd declared length: three characters sit in two registers, and the
+ * swap has to see both of them.
+ */
+ @Test
+ void oddLengthStringIsSwappedTheSameWayAnUnoptimizedReadSwapsIt() throws BufferException {
+ ModbusTag text = new ModbusTagHoldingRegister(1, 1, 3, ModbusDataType.STRING, BIG_ENDIAN_BYTE_SWAP);
+ // A second tag behind it, so the block really does cover both of the string's registers
+ // rather than the test handing over registers nobody asked for.
+ LinkedHashMap<String, ModbusTag> tags = new LinkedHashMap<>();
+ tags.put("text", text);
+ tags.put("guard", new ModbusTagHoldingRegister(3, 1, ModbusDataType.INT, BIG_ENDIAN_BYTE_SWAP));
+ byte[] blockData = {'A', 'B', 'C', 'D', 0x12, 0x34};
+
+ Map<String, PlcResponseItem<PlcValue>> response = splitRead(tags, blockData);
+
+ assertEquals(PlcResponseCode.OK, response.get("text").getResponseCode());
+ byte[] stringRegisters = {'A', 'B', 'C', 'D'};
+ assertEquals("BAD", unoptimizedRead(text, stringRegisters).getString());
+ assertEquals(unoptimizedRead(text, stringRegisters).getString(), response.get("text").getValue().getString());
+ }
+
+ /**
+ * Decodes the given registers for a single tag the way a read that was never merged into a
+ * block does (see the connections' {@code toPlcValue}): the whole registers the device
+ * answered with, swapped as one, then parsed.
+ */
+ private static PlcValue unoptimizedRead(ModbusTag tag, byte[] registers) throws BufferException {
+ ModbusByteOrder byteOrder = tag.getByteOrder();
+ byte[] data = registers;
+ if (byteOrder == ModbusByteOrder.BIG_ENDIAN_BYTE_SWAP || byteOrder == ModbusByteOrder.LITTLE_ENDIAN_BYTE_SWAP) {
+ data = byteSwap(data);
+ }
+ boolean bigEndian = (byteOrder == ModbusByteOrder.BIG_ENDIAN || byteOrder == ModbusByteOrder.BIG_ENDIAN_BYTE_SWAP);
+ return ModbusRegisterCodec.parse(new ReadBufferByteBased(data), tag.getDataType(),
+ tag.getNumberOfElements(), bigEndian, tag.getStringLength());
+ }
+
+ private static byte[] byteSwap(byte[] in) {
+ byte[] out = new byte[in.length];
+ for (int i = 0; i < out.length - 1; i += 2) {
+ out[i] = in[i + 1];
+ out[i + 1] = in[i];
+ }
+ if (in.length % 2 != 0) {
+ out[in.length - 1] = in[in.length - 1];
+ }
+ return out;
+ }
+
+ /** The largest PDU the Modbus specification allows, request or response. */
+ private static final int MAX_MODBUS_PDU_SIZE = 253;
+
+ /** The number of registers one extended-register file holds. */
+ private static final int EXTENDED_REGISTER_FILE_LENGTH = 10000;
+
+ /**
+ * An extended-register block is answered over FC 0x14 (Read File Record), whose per-item
+ * framing leaves room for fewer registers than FC 0x03 does. A 125-register block is one byte
+ * too wide for it, so two tags spanning that many registers must be read separately - even
+ * though the plain register areas happily merge them.
+ */
+ @Test
+ void extendedRegistersAreNotMergedWiderThanAReadFileRecordResponseFits() {
+ processReadRequest(new ModbusTag[]{
+ new ModbusTagExtendedRegister(1, 1, ModbusDataType.INT, Collections.emptyMap()),
+ new ModbusTagExtendedRegister(125, 1, ModbusDataType.INT, Collections.emptyMap())
+ },
+ optimizedReads -> {
+ assertEquals(2, optimizedReads.size());
+ ModbusTag first = optimizedReads.getFirst().mergedTag;
+ assertInstanceOf(ModbusTagExtendedRegister.class, first);
+ assertEquals(1, first.getAddress());
+ assertEquals(1, first.getNumberOfElements());
+ ModbusTag second = optimizedReads.get(1).mergedTag;
+ assertInstanceOf(ModbusTagExtendedRegister.class, second);
+ assertEquals(125, second.getAddress());
+ assertEquals(1, second.getNumberOfElements());
+ });
+
+ // The same two addresses in a holding register: the narrower ceiling is the extended
+ // registers' own, not a tightening of the limit every area is cut with.
+ processReadRequest(new ModbusTag[]{
+ new ModbusTagHoldingRegister(1, 1, ModbusDataType.INT, Collections.emptyMap()),
+ new ModbusTagHoldingRegister(125, 1, ModbusDataType.INT, Collections.emptyMap())
+ },
+ optimizedReads -> {
+ assertEquals(1, optimizedReads.size());
+ assertEquals(125, optimizedReads.getFirst().mergedTag.getNumberOfElements());
+ });
+ }
+
+ /**
+ * Extended registers that do fit stay merged - the ceiling must not cost a round trip it
+ * doesn't have to.
+ */
+ @Test
+ void extendedRegistersThatFitAreStillMerged() {
+ processReadRequest(new ModbusTag[]{
+ new ModbusTagExtendedRegister(1, 1, ModbusDataType.INT, Collections.emptyMap()),
+ new ModbusTagExtendedRegister(123, 1, ModbusDataType.INT, Collections.emptyMap())
+ },
+ optimizedReads -> {
+ assertEquals(1, optimizedReads.size());
+ ModbusTag mergedTag = optimizedReads.getFirst().mergedTag;
+ assertInstanceOf(ModbusTagExtendedRegister.class, mergedTag);
+ assertEquals(1, mergedTag.getAddress());
+ assertEquals(123, mergedTag.getNumberOfElements());
+ });
+ }
+
+ /**
+ * Whatever the optimizer emits for extended registers has to be answerable: no block may ask
+ * for more than an FC 0x14 response can carry back, including one that crosses a file boundary
+ * and is therefore answered as two items.
+ */
+ @Test
+ void everyExtendedRegisterBlockFitsTheModbusPduLimit() {
+ // Densely addressed tags, so every block is cut as wide as the optimizer will allow, both
+ // inside a single file and across the boundary between two of them.
+ LinkedHashMap<String, ModbusTag> tags = new LinkedHashMap<>();
+ for (int address = 1; address < 250; address++) {
+ tags.put("low" + address, new ModbusTagExtendedRegister(address, 1, ModbusDataType.INT, Collections.emptyMap()));
+ }
+ for (int address = 9900; address < 10200; address++) {
+ tags.put("boundary" + address, new ModbusTagExtendedRegister(address, 1, ModbusDataType.INT, Collections.emptyMap()));
+ }
+
+ ModbusReadOptimizer optimizer = new ModbusReadOptimizer(2000, 125, ModbusByteOrder.BIG_ENDIAN);
+ List<ModbusReadOptimizer.OptimizedRead> optimizedReads = optimizer.optimizeReads(tags);
+
+ assertFalse(optimizedReads.isEmpty());
+ boolean sawBlockCrossingAFileBoundary = false;
+ for (ModbusReadOptimizer.OptimizedRead optimizedRead : optimizedReads) {
+ ModbusTag block = optimizedRead.mergedTag;
+ sawBlockCrossingAFileBoundary |= itemsOf(block) > 1;
+ int pduSize = readFileRecordResponseSize(block);
+ assertTrue(pduSize <= MAX_MODBUS_PDU_SIZE,
+ "the block at " + block.getAddress() + " covering " + block.getNumberOfElements()
+ + " registers is answered with a " + pduSize + " byte PDU");
+ }
+ assertTrue(sawBlockCrossingAFileBoundary, "the two-item shape wasn't exercised");
+ }
+
+ /**
+ * The size of the FC 0x14 response answering the given extended-register block: function code
+ * (1) + response data length (1) + per item [data length (1) + reference type (1) + data].
+ */
+ private static int readFileRecordResponseSize(ModbusTag block) {
+ return 1 + 1 + itemsOf(block) * 2 + block.getNumberOfElements() * 2;
+ }
+
+ /**
+ * The number of items the response is split into: one per file the block reaches into, the
+ * way the connections group an extended-register read.
+ */
+ private static int itemsOf(ModbusTag block) {
+ int firstFile = block.getAddress() / EXTENDED_REGISTER_FILE_LENGTH;
+ int lastFile = (block.getAddress() + block.getNumberOfElements() - 1) / EXTENDED_REGISTER_FILE_LENGTH;
+ return lastFile - firstFile + 1;
+ }
+
/**
* Runs a single tag through the optimizer and splits the given raw response for it.
*/