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 dd9667b5031f1bb7787a7289bb159936d6f682f5 Author: Christofer Dutz <[email protected]> AuthorDate: Fri Aug 14 11:18:48 2026 +0200 fix(plc4j/eip): Fixed an issue that generally prevented the reading of arrays. fixes: #1008 --- .../plc4x/java/eip/base/EipTcpConnection.java | 48 +++++++++-- .../plc4x/java/eip/base/EipArrayReadTest.java | 99 ++++++++++++++++++++++ .../apache/plc4x/java/eip/base/EipDockerIT.java | 38 +++++++++ 3 files changed, 180 insertions(+), 5 deletions(-) diff --git a/plc4j/drivers/eip/src/main/java/org/apache/plc4x/java/eip/base/EipTcpConnection.java b/plc4j/drivers/eip/src/main/java/org/apache/plc4x/java/eip/base/EipTcpConnection.java index 94e160fe92..629f6dfda6 100644 --- a/plc4j/drivers/eip/src/main/java/org/apache/plc4x/java/eip/base/EipTcpConnection.java +++ b/plc4j/drivers/eip/src/main/java/org/apache/plc4x/java/eip/base/EipTcpConnection.java @@ -444,7 +444,7 @@ public class EipTcpConnection extends PollingSubscriptionConnectionBase<EIPConfi EipTag eipTag = (EipTag) request.getTag(tagName); CompletableFuture<Void> tagFuture = chain.thenComposeAsync(v -> executeThrottled(() -> { try { - CipReadRequest req = new CipReadRequest(toAnsi(eipTag.getTag()), 1); + CipReadRequest req = new CipReadRequest(toAnsi(eipTag.getTag()), elementCount(eipTag)); CipUnconnectedRequest requestItem = new CipUnconnectedRequest( classSegment, instanceSegment, req, (byte) getConfiguration().getBackplane(), @@ -495,7 +495,7 @@ public class EipTcpConnection extends PollingSubscriptionConnectionBase<EIPConfi for (String tagName : sendableTagNames(request)) { EipTag eipTag = (EipTag) request.getTag(tagName); try { - requests.add(new CipReadRequest(toAnsi(eipTag.getTag()), 1)); + requests.add(new CipReadRequest(toAnsi(eipTag.getTag()), elementCount(eipTag))); } catch (BufferException e) { return CompletableFuture.failedFuture(new PlcRuntimeException("Failed to read field", e)); } @@ -538,7 +538,7 @@ public class EipTcpConnection extends PollingSubscriptionConnectionBase<EIPConfi for (String tagName : sendableTagNames(request)) { EipTag eipTag = (EipTag) request.getTag(tagName); try { - requests.add(new CipReadRequest(toAnsi(eipTag.getTag()), 1)); + requests.add(new CipReadRequest(toAnsi(eipTag.getTag()), elementCount(eipTag))); } catch (BufferException e) { return CompletableFuture.failedFuture(new PlcRuntimeException("Failed to read field", e)); } @@ -835,6 +835,9 @@ public class EipTcpConnection extends PollingSubscriptionConnectionBase<EIPConfi PlcValue plcValue = null; if (code == PlcResponseCode.OK) { plcValue = parsePlcValue(tag, resp.getData().getData(), resp.getData().getDataType()); + if (plcValue == null) { + code = PlcResponseCode.INTERNAL_ERROR; + } } values.put(tagName, new DefaultPlcResponseItem<>(code, plcValue)); } else if (p instanceof MultipleServiceResponse responses) { @@ -866,6 +869,9 @@ public class EipTcpConnection extends PollingSubscriptionConnectionBase<EIPConfi PlcValue plcValue = null; if (code == PlcResponseCode.OK) { plcValue = parsePlcValue(tag, rr.getData().getData(), rr.getData().getDataType()); + if (plcValue == null) { + code = PlcResponseCode.INTERNAL_ERROR; + } } values.put(tagName, new DefaultPlcResponseItem<>(code, plcValue)); } @@ -884,17 +890,49 @@ public class EipTcpConnection extends PollingSubscriptionConnectionBase<EIPConfi PlcValue plcValue = null; if (code == PlcResponseCode.OK) { plcValue = parsePlcValue((EipTag) tag, resp.getData().getData(), resp.getData().getDataType()); + if (plcValue == null) { + // Undecodable payload (unsupported type or a reply shorter than the requested + // number of elements) - don't report that as a successful read. + code = PlcResponseCode.INTERNAL_ERROR; + } } values.put(tagName, new DefaultPlcResponseItem<>(code, plcValue)); return values; } - private PlcValue parsePlcValue(EipTag tag, byte[] rawData, CIPDataTypeCode type) { + /** + * Number of elements to ask the device for. An array tag ({@code %tag[0]:DINT:8}) has to + * request all of its elements - see GH-1008 - otherwise the device returns a single element + * and the decoder below has nothing to read the remaining ones from. + */ + private static int elementCount(EipTag tag) { + return Math.max(tag.getElementNb(), 1); + } + + /** Whether the type is stored as a flat sequence of equally sized elements. */ + private static boolean isFixedSize(CIPDataTypeCode type) { + return switch (type) { + case SINT, INT, DINT, LINT, REAL, LREAL, BOOL -> true; + default -> false; + }; + } + + // Package-private and static so the decoding can be tested without a connection. + static PlcValue parsePlcValue(EipTag tag, byte[] rawData, CIPDataTypeCode type) { final int STRING_LEN_OFFSET = 2; final int STRING_DATA_OFFSET = 6; ByteBuffer data = ByteBuffer.wrap(rawData).order(ByteOrder.LITTLE_ENDIAN); - int nb = tag.getElementNb(); + int nb = elementCount(tag); if (nb > 1) { + // Never read past what the device actually sent - a short reply must not blow up + // the whole response with an IndexOutOfBoundsException. Only the fixed-size types + // below are laid out element by element; STRING/STRUCTURED carry their own length. + int elementSize = isFixedSize(type) ? type.getSize() : 0; + if (elementSize > 0 && rawData.length < nb * elementSize) { + LOGGER.warn("Device returned {} bytes for tag '{}', expected {} for {} elements of {}.", + rawData.length, tag.getTag(), nb * elementSize, nb, type); + return null; + } List<PlcValue> list = new ArrayList<>(); int index = 0; for (int i = 0; i < nb; i++) { diff --git a/plc4j/drivers/eip/src/test/java/org/apache/plc4x/java/eip/base/EipArrayReadTest.java b/plc4j/drivers/eip/src/test/java/org/apache/plc4x/java/eip/base/EipArrayReadTest.java new file mode 100644 index 0000000000..3395a1b190 --- /dev/null +++ b/plc4j/drivers/eip/src/test/java/org/apache/plc4x/java/eip/base/EipArrayReadTest.java @@ -0,0 +1,99 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.plc4x.java.eip.base; + +import org.apache.plc4x.java.api.value.PlcValue; +import org.apache.plc4x.java.eip.base.tag.EipTag; +import org.apache.plc4x.java.eip.readwrite.CIPDataTypeCode; +import org.apache.plc4x.java.spi.values.PlcDINT; +import org.apache.plc4x.java.spi.values.PlcList; +import org.junit.jupiter.api.Test; + +import java.nio.ByteBuffer; +import java.nio.ByteOrder; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * Decoding of multi-element (array) reads - see GH-1008. The driver used to request a single + * element while decoding {@code elementNb} of them, which either misdecoded the value or threw + * an IndexOutOfBoundsException out of the response handler. + */ +class EipArrayReadTest { + + @Test + void arrayOfDintsIsFullyDecoded() { + EipTag tag = EipTag.of("%N40[0]:DINT:8"); + assertNotNull(tag); + assertEquals(8, tag.getElementNb()); + + byte[] raw = dints(1, 2, 3, 4, 5, 6, 7, 8); + PlcValue value = EipTcpConnection.parsePlcValue(tag, raw, CIPDataTypeCode.DINT); + + assertInstanceOf(PlcList.class, value); + assertEquals(8, value.getLength()); + for (int i = 0; i < 8; i++) { + assertEquals(i + 1, value.getIndex(i).getInt(), "element " + i); + } + } + + @Test + void scalarStillDecodesToAScalar() { + EipTag tag = EipTag.of("%N40:DINT"); + PlcValue value = EipTcpConnection.parsePlcValue(tag, dints(42), CIPDataTypeCode.DINT); + + assertInstanceOf(PlcDINT.class, value); + assertEquals(42, value.getInt()); + } + + /** + * The reporter's crash: a reply carrying fewer elements than the tag declares must not throw. + */ + @Test + void shortReplyIsReportedInsteadOfThrowing() { + EipTag tag = EipTag.of("%N40[0]:DINT:8"); + + // Only one element's worth of data for an 8-element tag. + assertNull(EipTcpConnection.parsePlcValue(tag, dints(1), CIPDataTypeCode.DINT)); + } + + @Test + void arrayOfIntsIsFullyDecoded() { + EipTag tag = EipTag.of("%N40[0]:INT:4"); + + ByteBuffer buffer = ByteBuffer.allocate(8).order(ByteOrder.LITTLE_ENDIAN); + for (short s : new short[]{10, 20, 30, 40}) { + buffer.putShort(s); + } + PlcValue value = EipTcpConnection.parsePlcValue(tag, buffer.array(), CIPDataTypeCode.INT); + + assertNotNull(value); + assertEquals(4, value.getLength()); + assertEquals(10, value.getIndex(0).getInt()); + assertEquals(40, value.getIndex(3).getInt()); + } + + private static byte[] dints(int... values) { + ByteBuffer buffer = ByteBuffer.allocate(values.length * 4).order(ByteOrder.LITTLE_ENDIAN); + for (int value : values) { + buffer.putInt(value); + } + return buffer.array(); + } +} diff --git a/plc4j/drivers/eip/src/test/java/org/apache/plc4x/java/eip/base/EipDockerIT.java b/plc4j/drivers/eip/src/test/java/org/apache/plc4x/java/eip/base/EipDockerIT.java index 13e106750b..02af9f0366 100644 --- a/plc4j/drivers/eip/src/test/java/org/apache/plc4x/java/eip/base/EipDockerIT.java +++ b/plc4j/drivers/eip/src/test/java/org/apache/plc4x/java/eip/base/EipDockerIT.java @@ -25,6 +25,7 @@ import org.apache.plc4x.java.api.messages.PlcReadResponse; import org.apache.plc4x.java.api.messages.PlcWriteRequest; import org.apache.plc4x.java.api.messages.PlcWriteResponse; import org.apache.plc4x.java.api.types.PlcResponseCode; +import org.apache.plc4x.java.api.value.PlcValue; import org.junit.jupiter.api.*; import org.testcontainers.containers.GenericContainer; import org.testcontainers.containers.wait.strategy.Wait; @@ -230,4 +231,41 @@ public class EipDockerIT { } } + /** + * Reading several array elements at once - see GH-1008. The driver used to ask the device + * for a single element while decoding as many as the tag declared, so this returned either + * a wrong value or an IndexOutOfBoundsException. + * + * <p>The element count has to reach the wire in the {@code CipReadRequest}, which is what + * separates this from {@link #writeReadArrayElement()} - that one reads a single element + * and passes either way. + */ + @Test + void readMultipleArrayElements() throws Exception { + try (PlcConnection connection = new DefaultPlcDriverManager().getConnection(connectionUrl)) { + // Seed the first four elements with recognisable values. + PlcWriteRequest.Builder writeBuilder = connection.writeRequestBuilder(); + for (int i = 0; i < 4; i++) { + writeBuilder.addTagAddress("e" + i, "hurz_DINT_ARR[" + i + "]:DINT", 0x1000 + i); + } + PlcWriteResponse writeResponse = writeBuilder.build().execute().get(); + for (int i = 0; i < 4; i++) { + assertEquals(PlcResponseCode.OK, writeResponse.getResponseCode("e" + i)); + } + + // Read all four back with a single array read. + PlcReadRequest readRequest = connection.readRequestBuilder() + .addTagAddress("arr", "hurz_DINT_ARR[0]:DINT:4") + .build(); + PlcReadResponse readResponse = readRequest.execute().get(); + + assertEquals(PlcResponseCode.OK, readResponse.getResponseCode("arr")); + PlcValue value = readResponse.getPlcValue("arr"); + assertEquals(4, value.getLength()); + for (int i = 0; i < 4; i++) { + assertEquals(0x1000 + i, value.getIndex(i).getInt(), "element " + i); + } + } + } + }
