You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@plc4x.apache.org by cd...@apache.org on 2026/08/27 09:05:20 UTC
(plc4x) 01/01: fix(plc4j/eip): decompose tag paths once, make EipTag immutable
This is an automated email from the ASF dual-hosted git repository.
chrisdutz pushed a commit to branch feature/eip-tag-path-precompute
in repository https://gitbox.apache.org/repos/asf/plc4x.git
commit b5f6ea2eae499e17c4d0e8bc6b58cf2b2256fc72
Author: Christofer Dutz <ch...@c-ware.de>
AuthorDate: Thu Aug 27 11:04:56 2026 +0200
fix(plc4j/eip): decompose tag paths once, make EipTag immutable
---
RELEASE_NOTES | 8 ++
plc4go/internal/eip/Reader.go | 10 ++-
plc4go/internal/eip/Reader_test.go | 21 +++++
.../plc4x/java/eip/base/EipTcpConnection.java | 58 ++++++--------
.../org/apache/plc4x/java/eip/base/tag/EipTag.java | 91 ++++++++++++++++------
.../java/eip/base/tag/EipTagCoverageTest.java | 23 +++---
6 files changed, 142 insertions(+), 69 deletions(-)
diff --git a/RELEASE_NOTES b/RELEASE_NOTES
index f72a19c508..99964060e8 100644
--- a/RELEASE_NOTES
+++ b/RELEASE_NOTES
@@ -360,6 +360,14 @@ Incompatible changes
writers, PlcBYTE serializes as a bit string, and PlcTIME renders
as an ISO-8601 string like PlcLTIME. This aligns the Java and Go
renderings of the same value.
+- EtherNet/IP: EipTag is now immutable, matching every other
+ driver's tag in both bindings - it was the only tag class in
+ plc4j that exposed setters. setType(...) and setElementNb(...)
+ are gone; give the type and the element count to the
+ constructor instead. An element count below one is normalised
+ to one rather than kept, so a tag built as ":INT:0", or through
+ the (tag, type) constructor which used to leave the count at
+ zero, now reads one element instead of none.
Changed Maven Coordinates
-------------------------
diff --git a/plc4go/internal/eip/Reader.go b/plc4go/internal/eip/Reader.go
index 362cad23d5..977e37d70e 100644
--- a/plc4go/internal/eip/Reader.go
+++ b/plc4go/internal/eip/Reader.go
@@ -331,9 +331,12 @@ func ansiPad(identifier string) *uint8 {
return nil
}
+// resourceAddressPattern splits a tag address into the members it names. Compiled once: toAnsi
+// runs for every tag of every request, and compiling a pattern per call is not free.
+var resourceAddressPattern = regexp.MustCompile("([.\\[\\]])*([A-Za-z_0-9]+){1}")
+
func toAnsi(tag string) ([]byte, error) {
ctx := context.TODO()
- resourceAddressPattern := regexp.MustCompile("([.\\[\\]])*([A-Za-z_0-9]+){1}")
segments := make([]readWriteModel.PathSegment, 0)
lengthInBytes := uint16(0)
@@ -349,6 +352,11 @@ func toAnsi(tag string) ([]byte, error) {
if err != nil {
return nil, fmt.Errorf("error parsing address %s, identifier %s couldn't be parsed to an integer", tag, identifier)
}
+ // MemberID.instance is a uint 8 in the mspec, so it holds 0 to 255. Converting
+ // a larger index would wrap it silently and address the wrong element.
+ if numericIdentifier < 0 || numericIdentifier > 255 {
+ return nil, fmt.Errorf("error parsing address %s, index %d is out of range 0 to 255", tag, numericIdentifier)
+ }
newSegment = readWriteModel.NewLogicalSegment(readWriteModel.NewMemberID(0, uint8(numericIdentifier)))
} else {
newSegment = readWriteModel.NewDataSegment(readWriteModel.NewAnsiExtendedSymbolSegment(identifier, ansiPad(identifier)))
diff --git a/plc4go/internal/eip/Reader_test.go b/plc4go/internal/eip/Reader_test.go
index 487bc28cfa..0ee2ca17fe 100644
--- a/plc4go/internal/eip/Reader_test.go
+++ b/plc4go/internal/eip/Reader_test.go
@@ -151,3 +151,24 @@ func TestToAnsi(t *testing.T) {
})
}
}
+
+// An array index rides in a MemberID, whose instance field the mspec declares as uint 8, so it
+// holds 0 to 255. A larger index used to be converted with uint8(...), which wraps silently -
+// a[300] addressed element 44 and produced a request that looked entirely valid. The Java
+// driver rejects the same address when it serializes ("Value 300 is out of range for 8 bits"),
+// so this reports it too rather than reading the wrong element.
+func TestToAnsiRejectsOutOfRangeIndex(t *testing.T) {
+ for _, tag := range []string{"a[256]", "a[300]", "myArray[1000]"} {
+ t.Run(tag, func(t *testing.T) {
+ _, err := toAnsi(tag)
+ require.Error(t, err)
+ assert.Contains(t, err.Error(), "255")
+ })
+ }
+}
+
+func TestToAnsiAcceptsTheLargestIndex(t *testing.T) {
+ actual, err := toAnsi("a[255]")
+ require.NoError(t, err)
+ assert.Equal(t, []byte{0x91, 0x01, 0x61, 0x00, 0x28, 0xFF}, actual)
+}
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 ef167bdf57..eb7fda1706 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
@@ -53,8 +53,6 @@ import java.nio.charset.StandardCharsets;
import java.util.*;
import java.util.concurrent.*;
import java.util.concurrent.atomic.AtomicInteger;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
/**
* EtherNet/IP (CIP encapsulation) TCP connection — direct port of the legacy
@@ -555,7 +553,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()), elementCount(eipTag));
+ CipReadRequest req = new CipReadRequest(toAnsi(eipTag), eipTag.getElementNb());
CipUnconnectedRequest requestItem = new CipUnconnectedRequest(
classSegment, instanceSegment, req,
(byte) getConfiguration().getBackplane(),
@@ -606,7 +604,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()), elementCount(eipTag)));
+ requests.add(new CipReadRequest(toAnsi(eipTag), eipTag.getElementNb()));
} catch (BufferException e) {
return CompletableFuture.failedFuture(new PlcRuntimeException("Failed to read field", e));
}
@@ -649,7 +647,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()), elementCount(eipTag)));
+ requests.add(new CipReadRequest(toAnsi(eipTag), eipTag.getElementNb()));
} catch (BufferException e) {
return CompletableFuture.failedFuture(new PlcRuntimeException("Failed to read field", e));
}
@@ -758,12 +756,11 @@ public class EipTcpConnection extends PollingSubscriptionConnectionBase<EIPConfi
for (String fieldName : sendableTagNames(request)) {
EipTag field = (EipTag) request.getTag(fieldName);
PlcValue value = request.getPlcValue(fieldName);
- int elements = Math.max(field.getElementNb(), 1);
CompletableFuture<Void> tagFuture = chain.thenComposeAsync(v -> executeThrottled(() -> {
try {
byte[] data = encodeValue(value, field.getType());
CipWriteRequest writeReq = new CipWriteRequest(
- toAnsi(field.getTag()), field.getType(), elements, data);
+ toAnsi(field), field.getType(), field.getElementNb(), data);
CipUnconnectedRequest requestItem = new CipUnconnectedRequest(
classSegment, instanceSegment, writeReq,
(byte) getConfiguration().getBackplane(),
@@ -808,10 +805,9 @@ public class EipTcpConnection extends PollingSubscriptionConnectionBase<EIPConfi
for (String fieldName : sendableTagNames(request)) {
EipTag field = (EipTag) request.getTag(fieldName);
PlcValue value = request.getPlcValue(fieldName);
- int elements = Math.max(field.getElementNb(), 1);
try {
byte[] data = encodeValue(value, field.getType());
- items.add(new CipWriteRequest(toAnsi(field.getTag()), field.getType(), elements, data));
+ items.add(new CipWriteRequest(toAnsi(field), field.getType(), field.getElementNb(), data));
} catch (BufferException | PlcInvalidTagException e) {
return CompletableFuture.failedFuture(
new PlcRuntimeException("Failed to write field '" + fieldName + "'", e));
@@ -858,10 +854,9 @@ public class EipTcpConnection extends PollingSubscriptionConnectionBase<EIPConfi
for (String fieldName : sendableTagNames(request)) {
EipTag field = (EipTag) request.getTag(fieldName);
PlcValue value = request.getPlcValue(fieldName);
- int elements = Math.max(field.getElementNb(), 1);
try {
byte[] data = encodeValue(value, field.getType());
- items.add(new CipWriteRequest(toAnsi(field.getTag()), field.getType(), elements, data));
+ items.add(new CipWriteRequest(toAnsi(field), field.getType(), field.getElementNb(), data));
} catch (BufferException | PlcInvalidTagException e) {
return CompletableFuture.failedFuture(
new PlcRuntimeException("Failed to write field '" + fieldName + "'", e));
@@ -909,23 +904,25 @@ public class EipTcpConnection extends PollingSubscriptionConnectionBase<EIPConfi
// Encoders / decoders
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
- private byte[] toAnsi(String tag) throws BufferException {
- Pattern resourcePattern = Pattern.compile("([.\\[\\]])*([A-Za-z_0-9]+)");
- Matcher matcher = resourcePattern.matcher(tag);
- List<PathSegment> segments = new LinkedList<>();
+ /**
+ * Serializes the CIP path of a tag: one segment per member, in the order the address names
+ * them. The address was decomposed when the tag was built, so this does no parsing.
+ */
+ private byte[] toAnsi(EipTag tag) throws BufferException {
+ List<EipTag.PathElement> elements = tag.getPathElements();
+ List<PathSegment> segments = new ArrayList<>(elements.size());
int lengthBytes = 0;
- while (matcher.find()) {
- String identifier = matcher.group(2);
- String qualifier = matcher.group(1);
- PathSegment newSegment;
- if ("[".equals(qualifier)) {
- newSegment = new LogicalSegment(new MemberID((byte) 0x00, Short.parseShort(identifier)));
+ for (EipTag.PathElement element : elements) {
+ PathSegment segment;
+ if (element instanceof EipTag.MemberElement member) {
+ segment = new LogicalSegment(new MemberID((byte) 0x00, member.index()));
} else {
- newSegment = new DataSegment(new AnsiExtendedSymbolSegment(identifier,
- (identifier.length() % 2 == 0) ? null : (short) 0));
+ String name = ((EipTag.SymbolElement) element).name();
+ segment = new DataSegment(new AnsiExtendedSymbolSegment(name,
+ (name.length() % 2 == 0) ? null : (short) 0));
}
- segments.add(newSegment);
- lengthBytes += newSegment.getLengthInBytes();
+ segments.add(segment);
+ lengthBytes += segment.getLengthInBytes();
}
WriteBufferByteBased buffer = messageCodec.createWriteBuffer(lengthBytes);
for (PathSegment segment : segments) {
@@ -1012,15 +1009,6 @@ public class EipTcpConnection extends PollingSubscriptionConnectionBase<EIPConfi
return values;
}
- /**
- * 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);
- }
-
/**
* Reinterprets a 64 bit two's complement value as the unsigned bit string it is on the wire.
* {@link PlcLWORD} validates against a range starting at zero, so a negative long - which is
@@ -1126,7 +1114,7 @@ public class EipTcpConnection extends PollingSubscriptionConnectionBase<EIPConfi
// Package-private and static so the decoding can be tested without a connection.
static PlcValue parsePlcValue(EipTag tag, byte[] rawData, CIPDataTypeCode type) {
ByteBuffer data = ByteBuffer.wrap(rawData).order(ByteOrder.LITTLE_ENDIAN);
- int nb = elementCount(tag);
+ int nb = tag.getElementNb();
TypeCodec codec = FIXED_SIZE_CODECS.get(type);
// Handle array reads.
diff --git a/plc4j/drivers/eip/src/main/java/org/apache/plc4x/java/eip/base/tag/EipTag.java b/plc4j/drivers/eip/src/main/java/org/apache/plc4x/java/eip/base/tag/EipTag.java
index ada548fc5c..fb9634b6c0 100644
--- a/plc4j/drivers/eip/src/main/java/org/apache/plc4x/java/eip/base/tag/EipTag.java
+++ b/plc4j/drivers/eip/src/main/java/org/apache/plc4x/java/eip/base/tag/EipTag.java
@@ -19,6 +19,7 @@
package org.apache.plc4x.java.eip.base.tag;
+import org.apache.plc4x.java.api.exceptions.PlcInvalidTagException;
import org.apache.plc4x.java.api.model.ArrayInfo;
import org.apache.plc4x.java.api.model.PlcTag;
import org.apache.plc4x.java.api.types.PlcValueType;
@@ -29,6 +30,8 @@ import org.apache.plc4x.java.spi.buffers.api.WriteBuffer;
import org.apache.plc4x.java.spi.buffers.api.exceptions.BufferException;
import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Collections;
import java.util.List;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
@@ -38,33 +41,78 @@ public class EipTag implements PlcTag, Serializable {
private static final Pattern ADDRESS_PATTERN =
Pattern.compile("^(?<tag>[%a-zA-Z_.0-9]+\\[?[0-9]*]?):?(?<dataType>[A-Z]*):?(?<elementNb>[0-9]*)");
+ /**
+ * Splits an address into the members it names. A qualifier of '[' introduces an array
+ * member, anything else another symbol; characters outside a member name (a leading '%')
+ * are skipped. Compiled once - a tag address is decomposed when the tag is built, not
+ * again for every request that uses it.
+ */
+ private static final Pattern PATH_PATTERN = Pattern.compile("([.\\[\\]])*([A-Za-z_0-9]+)");
+
private static final String GROUP_NAME_TAG = "tag";
private static final String GROUP_NAME_GROUP_NAME_ELEMENTS = "elementNb";
private static final String GROUP_NAME_TYPE = "dataType";
+ /** One step of the CIP path an address describes. */
+ public sealed interface PathElement permits SymbolElement, MemberElement {
+ }
+
+ /** A named symbol - a tag, or one member of a structured tag. */
+ public record SymbolElement(String name) implements PathElement {
+ }
+
+ /** An array member, addressed by index. */
+ public record MemberElement(short index) implements PathElement {
+ }
+
private final String tag;
- private CIPDataTypeCode type;
- private int elementNb;
+ private final CIPDataTypeCode type;
+ private final int elementNb;
+ private final List<PathElement> pathElements;
public EipTag(String tag) {
- this.tag = tag;
- this.elementNb = 1;
+ this(tag, null, 1);
}
public EipTag(String tag, int elementNb) {
- this.tag = tag;
- this.elementNb = elementNb;
+ this(tag, null, elementNb);
+ }
+
+ public EipTag(String tag, CIPDataTypeCode type) {
+ this(tag, type, 1);
}
public EipTag(String tag, CIPDataTypeCode type, int elementNb) {
this.tag = tag;
this.type = type;
- this.elementNb = elementNb;
+ // A request always asks for at least one element; normalising here keeps every use site
+ // from having to guard against a count of zero.
+ this.elementNb = Math.max(elementNb, 1);
+ this.pathElements = decomposePath(tag);
}
- public EipTag(String tag, CIPDataTypeCode type) {
- this.tag = tag;
- this.type = type;
+ private static List<PathElement> decomposePath(String tag) {
+ if (tag == null) {
+ return Collections.emptyList();
+ }
+ Matcher matcher = PATH_PATTERN.matcher(tag);
+ List<PathElement> elements = new ArrayList<>(2);
+ while (matcher.find()) {
+ String identifier = matcher.group(2);
+ if ("[".equals(matcher.group(1))) {
+ // MemberID.instance is a uint 8 in the mspec, so it holds 0 to 255. Reject a
+ // larger index here rather than letting it fail later during serialization.
+ int index = Integer.parseInt(identifier);
+ if (index > 255) {
+ throw new PlcInvalidTagException(String.format(
+ "Error parsing address %s, index %d is out of range 0 to 255", tag, index));
+ }
+ elements.add(new MemberElement((short) index));
+ } else {
+ elements.add(new SymbolElement(identifier));
+ }
+ }
+ return Collections.unmodifiableList(elements);
}
@Override
@@ -95,22 +143,23 @@ public class EipTag implements PlcTag, Serializable {
return type;
}
- public void setType(CIPDataTypeCode type) {
- this.type = type;
- }
-
public int getElementNb() {
return elementNb;
}
- public void setElementNb(int elementNb) {
- this.elementNb = elementNb;
- }
-
public String getTag() {
return tag;
}
+ /**
+ * The CIP path this address describes, one element per member, decomposed when the tag was
+ * built. A structured address yields one element per member - {@code a.b} is two symbols,
+ * not one symbol named "a.b" - because that is how a controller walks the path.
+ */
+ public List<PathElement> getPathElements() {
+ return pathElements;
+ }
+
public static boolean matches(String tagQuery) {
return ADDRESS_PATTERN.matcher(tagQuery).matches();
}
@@ -129,11 +178,7 @@ public class EipTag implements PlcTag, Serializable {
} else {
type = CIPDataTypeCode.DINT;
}
- if (nb != 0) {
- return new EipTag(tag, type, nb);
- } else {
- return new EipTag(tag, type);
- }
+ return new EipTag(tag, type, nb);
}
return null;
}
diff --git a/plc4j/drivers/eip/src/test/java/org/apache/plc4x/java/eip/base/tag/EipTagCoverageTest.java b/plc4j/drivers/eip/src/test/java/org/apache/plc4x/java/eip/base/tag/EipTagCoverageTest.java
index 7db670e3fc..3f432b12a4 100644
--- a/plc4j/drivers/eip/src/test/java/org/apache/plc4x/java/eip/base/tag/EipTagCoverageTest.java
+++ b/plc4j/drivers/eip/src/test/java/org/apache/plc4x/java/eip/base/tag/EipTagCoverageTest.java
@@ -48,9 +48,9 @@ class EipTagCoverageTest {
void ctorWithTypeOnly() {
EipTag tag = new EipTag("%A0", CIPDataTypeCode.INT);
assertThat(tag.getType()).isEqualTo(CIPDataTypeCode.INT);
- // No element-count constructor → default int zero (not the 1 the
- // single-arg ctor injects). Lock the surface in place.
- assertThat(tag.getElementNb()).isZero();
+ // Every constructor now normalises to at least one element. This used to leave the
+ // field at Java's default of zero, which is not a count any request can be made with.
+ assertThat(tag.getElementNb()).isEqualTo(1);
}
@Test
@@ -61,11 +61,13 @@ class EipTagCoverageTest {
assertThat(tag.getPlcValueType()).isEqualTo(PlcValueType.DINT);
}
+ /**
+ * The type and element count are given at construction; EipTag is immutable, like every
+ * other driver's tag. This used to go through setType/setElementNb.
+ */
@Test
- void setTypeAndSetElementNbRoundtrip() {
- EipTag tag = new EipTag("%A0");
- tag.setType(CIPDataTypeCode.REAL);
- tag.setElementNb(7);
+ void typeAndElementNbComeFromTheConstructor() {
+ EipTag tag = new EipTag("%A0", CIPDataTypeCode.REAL, 7);
assertThat(tag.getType()).isEqualTo(CIPDataTypeCode.REAL);
assertThat(tag.getElementNb()).isEqualTo(7);
}
@@ -88,11 +90,12 @@ class EipTagCoverageTest {
}
@Test
- void ofWithZeroElementsUsesTypeOnlyCtorPath() {
- // elementNb=0 selects the (tag, type) constructor branch.
+ void ofWithZeroElementsReadsOneElement() {
+ // An explicit count of zero used to survive into the tag; a request for zero elements
+ // is meaningless, so it is normalised to one like any other count below one.
EipTag tag = EipTag.of("%A0:INT:0");
assertThat(tag).isNotNull();
- assertThat(tag.getElementNb()).isZero();
+ assertThat(tag.getElementNb()).isEqualTo(1);
assertThat(tag.getType()).isEqualTo(CIPDataTypeCode.INT);
}