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
commit 6a70419b8a9f2b6ea33591b39bd3f66954d7c37a Author: Christofer Dutz <[email protected]> AuthorDate: Fri Aug 14 10:52:46 2026 +0200 fix(plc4j/modbus): Fixed an issue with optimized modbus reads that the handling of coils only returned the first value. Fixes: #2060 --- .../modbus/base/optimizer/ModbusReadOptimizer.java | 28 +++++-- .../base/optimizer/ModbusReadOptimizerTest.java | 96 ++++++++++++++++++++++ 2 files changed, 118 insertions(+), 6 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 67f02d5966..57e880ebb5 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 @@ -32,6 +32,7 @@ import org.apache.plc4x.java.spi.buffers.bytebased.WithByteBasedOption; import org.apache.plc4x.java.spi.drivers.messages.items.DefaultPlcResponseItem; 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; import java.util.*; @@ -121,12 +122,27 @@ public class ModbusReadOptimizer { try { if (blockTag instanceof ModbusTagCoil || blockTag instanceof ModbusTagDiscreteInput) { - // Coils/discrete inputs: bit-level extraction - int bitPosition = originalTag.getAddress() - blockTag.getAddress(); - int bytePosition = bitPosition / 8; - int bitPositionInByte = bitPosition % 8; - boolean isBitSet = (blockData[bytePosition] & (1 << bitPositionInByte)) != 0; - result.put(tagName, new DefaultPlcResponseItem<>(PlcResponseCode.OK, new PlcBOOL(isBitSet))); + // Coils/discrete inputs: bit-level extraction. An array tag (BOOL[n]) occupies + // n consecutive coils - the block read already covers all of them (see + // optimizeCoils), so every element has to be extracted, not just the first. + int firstBitPosition = originalTag.getAddress() - blockTag.getAddress(); + int numberOfElements = originalTag.getNumberOfElements(); + int lastBytePosition = (firstBitPosition + numberOfElements - 1) / 8; + if (lastBytePosition >= blockData.length) { + // The device returned fewer coils than we asked for. + result.put(tagName, new DefaultPlcResponseItem<>(PlcResponseCode.INTERNAL_ERROR, null)); + continue; + } + List<PlcValue> values = new ArrayList<>(numberOfElements); + for (int i = 0; i < numberOfElements; i++) { + int bitPosition = firstBitPosition + i; + int bytePosition = bitPosition / 8; + int bitPositionInByte = bitPosition % 8; + boolean isBitSet = (blockData[bytePosition] & (1 << bitPositionInByte)) != 0; + values.add(new PlcBOOL(isBitSet)); + } + PlcValue plcValue = numberOfElements == 1 ? values.getFirst() : new PlcList(values); + result.put(tagName, new DefaultPlcResponseItem<>(PlcResponseCode.OK, plcValue)); } else { // Registers: byte-level extraction int byteOffset = (originalTag.getAddress() - blockTag.getAddress()) * 2; 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 1a17cdb00d..b230c5558d 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 @@ -21,8 +21,13 @@ package org.apache.plc4x.java.modbus.base.optimizer; 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.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.drivers.messages.items.PlcResponseItem; +import org.apache.plc4x.java.spi.values.PlcBOOL; +import org.apache.plc4x.java.spi.values.PlcList; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; @@ -32,6 +37,7 @@ import org.opentest4j.AssertionFailedError; import java.util.Collections; import java.util.LinkedHashMap; import java.util.List; +import java.util.Map; import java.util.stream.Stream; import static org.junit.jupiter.api.Assertions.*; @@ -243,6 +249,96 @@ class ModbusReadOptimizerTest { }); } + /** + * 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. + */ + @Test + void coilArrayReturnsAllElements() { + // 8 coils starting at address 0, response bits (LSB first): 1,0,0,0,1,1,0,1 + Map<String, PlcResponseItem<PlcValue>> response = + splitSingleRead(new ModbusTagCoil(0, 8, ModbusDataType.BOOL, Collections.emptyMap()), + new byte[]{(byte) 0b10110001}); + + PlcValue value = response.get("tag0").getValue(); + assertInstanceOf(PlcList.class, value); + assertEquals(8, value.getLength()); + boolean[] expected = {true, false, false, false, true, true, false, true}; + for (int i = 0; i < expected.length; i++) { + assertEquals(expected[i], value.getIndex(i).getBoolean(), "bit " + i); + } + } + + /** + * An array sitting at an offset inside the block must be read from the right bit position and + * keep reading into the following byte when it spans a byte boundary. + */ + @Test + void coilArrayCrossingAByteBoundary() { + // A scalar coil at address 0 makes the block start there, so the array at address 6 + // really does start at bit 6 of the first byte and runs into the second one. + LinkedHashMap<String, ModbusTag> tags = new LinkedHashMap<>(); + tags.put("scalar", new ModbusTagCoil(0, 1, ModbusDataType.BOOL, Collections.emptyMap())); + tags.put("array", new ModbusTagCoil(6, 4, ModbusDataType.BOOL, Collections.emptyMap())); + + // bits 6,7 of byte 0 = 0,1 ; bits 0,1 of byte 1 = 0,1 + Map<String, PlcResponseItem<PlcValue>> response = + splitRead(tags, new byte[]{(byte) 0b10000000, (byte) 0b00000010}); + + PlcValue value = response.get("array").getValue(); + assertEquals(4, value.getLength()); + assertFalse(value.getIndex(0).getBoolean()); + assertTrue(value.getIndex(1).getBoolean()); + assertFalse(value.getIndex(2).getBoolean()); + assertTrue(value.getIndex(3).getBoolean()); + } + + /** + * A scalar coil must keep returning a plain PlcBOOL rather than a single-element list. + */ + @Test + void singleCoilStillReturnsAScalar() { + Map<String, PlcResponseItem<PlcValue>> response = + splitSingleRead(new ModbusTagCoil(0, 1, ModbusDataType.BOOL, Collections.emptyMap()), + new byte[]{(byte) 0b00000001}); + + PlcValue value = response.get("tag0").getValue(); + assertInstanceOf(PlcBOOL.class, value); + assertTrue(value.getBoolean()); + } + + /** + * A device answering with fewer coils than requested must not blow up the whole response. + */ + @Test + void shortCoilResponseYieldsAnError() { + Map<String, PlcResponseItem<PlcValue>> response = + splitSingleRead(new ModbusTagCoil(0, 16, ModbusDataType.BOOL, Collections.emptyMap()), + new byte[]{(byte) 0b00000001}); + + assertEquals(PlcResponseCode.INTERNAL_ERROR, response.get("tag0").getResponseCode()); + } + + /** + * Runs a single tag through the optimizer and splits the given raw response for it. + */ + private Map<String, PlcResponseItem<PlcValue>> splitSingleRead(ModbusTag tag, byte[] blockData) { + LinkedHashMap<String, ModbusTag> tagMap = new LinkedHashMap<>(); + tagMap.put("tag0", tag); + return splitRead(tagMap, blockData); + } + + /** + * Runs the given tags through the optimizer and splits the given raw response for the + * single block read they are expected to produce. + */ + private Map<String, PlcResponseItem<PlcValue>> splitRead(LinkedHashMap<String, ModbusTag> tags, byte[] blockData) { + ModbusReadOptimizer optimizer = new ModbusReadOptimizer(2000, 125, ModbusByteOrder.BIG_ENDIAN); + List<ModbusReadOptimizer.OptimizedRead> optimizedReads = optimizer.optimizeReads(tags); + assertEquals(1, optimizedReads.size()); + return optimizer.splitResponse(optimizedReads.getFirst(), PlcResponseCode.OK, blockData); + } + void processReadRequest(ModbusTag[] tags, CheckResult check) { ModbusReadOptimizer optimizer = new ModbusReadOptimizer(2000, 125, ModbusByteOrder.BIG_ENDIAN); LinkedHashMap<String, ModbusTag> tagMap = new LinkedHashMap<>();
