(plc4j/eip): Optimize EipTcpConnection::toAnsi by andvasp · Pull Request #2716 · apache/plc4x

andvasp · 2026-08-26T13:55:56Z

This PR remove the use of Pattern.compile from EipTcpConnection::toAnsi to optimize a method that is called for every request and for each tag address.

The part responsible for identifying the array index (CIP member) has been moved to the EipTag constructor. This avoids reprocessing the tag address on every request.

A simpler optimization would be to make the resourcePattern static:

static Pattern resourcePattern = Pattern.compile("([.\\[\\]])*([A-Za-z_0-9]+)");

However, this would still require processing the tag address on every request.

The pattern above also supports identifying more than one member, while EipTag.ADDRESS_PATTERN only allows a single index. Therefore, this difference has no impact on the new implementation.

EipTag is now immutable, which also makes it more suitable for future Valhalla optimizations.

Additionally, EipTag::getElementNb() is now enforced to be greater than 1, removing the need for the corresponding check in EipTcpConnection.

Moving the index identification to EipTag also has the additional benefit of making this information available for other use cases, such as DWORD-type processing.

The EipTag.type continues to be possible to be null but in case of passing null on constructor. Would it not be better to do it DINT?

andvasp · 2026-08-26T13:58:16Z

Hi @chrisdutz !

Can you review this PR and if possible accept it before release 1.0?

sruehl · 2026-08-26T14:03:03Z

there are conflicts btw, maybe rebase it

Copilot

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

This PR optimizes EtherNet/IP tag path encoding by removing per-call regex compilation and shifting array-index parsing into EipTag, making tags immutable and normalizing element counts.

Changes:

Made EipTag immutable, added support for a parsed array index, and enforced elementNb >= 1.
Updated EipTcpConnection to encode ANSI paths using parsed EipTag fields (tag + optional member index) instead of regex parsing.
Updated and expanded unit tests to reflect the new parsing/immutability behavior and round-trip address formatting.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
plc4j/drivers/eip/src/main/java/org/apache/plc4x/java/eip/base/tag/EipTag.java	Parses/stores optional array index, enforces element constraints, updates address formatting, and removes mutators.
plc4j/drivers/eip/src/main/java/org/apache/plc4x/java/eip/base/EipTcpConnection.java	Reworks `toAnsi` to use `EipTag` directly and removes element-count helper.
plc4j/drivers/eip/src/test/java/org/apache/plc4x/java/eip/base/tag/EipTagTest.java	Adjusts parsing expectations to validate new `index` field and updated tag/index split.
plc4j/drivers/eip/src/test/java/org/apache/plc4x/java/eip/base/tag/EipTagCoverageTest.java	Updates ctor/of() coverage for index defaulting, element defaulting, and address round-trips with index.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

andvasp · 2026-08-26T15:45:37Z

+            int nb = elementsString.isEmpty() ? 1 : Integer.parseInt(elementsString);
+
+            String indexString = matcher.group(GROUP_NAME_INDEX);
+            Short index = indexString != null ? Short.parseShort(indexString) : null;


NumberFormatException will not be thrown. Currently it accepts the address "A[]" considering index null.

Should this address be accepted considering index null?

What do you think @sruehl ?

@chrisdutz is the expert for Arrays using tags.

The current develop version will parse the tag " A0[]:DINT:3" but will throw an exception on toAnsi.

Compared with modbus, the regex will reject the array index but as the array is at the end of the tag, it will not impact the final capture. Then 9845:BOOL(1)[] is valid.

Modubs:

ADDRESS_PATTERN = (?<address>\d{1,9})(:(?<datatype>[a-zA-Z_]+)(\((?<stringLength>\d{1,5})\))?)?(\[(?<quantity>\d{1,5})])?

It matches but does not capture the quantity group.

This PR is considering a valid address where index = null but I believe it would be better to reject this address.

andvasp · 2026-08-26T15:27:57Z

+    public EipTag(String tag, Short index, CIPDataTypeCode type, int elementNb) {
        this.tag = tag;
+        this.index = index != null ? (short)Math.max(0, index) : null;
        this.type = type;
+        this.elementNb = Math.max(1, elementNb);


Yes. I agree but I did not want to change the previous behavior where it does not check for invalid value.

What do you think @sruehl ?

Another point here if we should assign DINT to type since this is the default data type.

andvasp · 2026-08-26T15:19:14Z

    public String getAddressString() {
        // Mirrors the format ADDRESS_PATTERN accepts, so of(getAddressString()) round-trips:
-        //   tag[:dataType[:elementNb]]
+        //   tag[index]:dataType:elementNb
        StringBuilder sb = new StringBuilder(tag);
+        if (index != null) {
+            sb.append('[').append(index).append(']');
+        }
        if (type != null) {
            sb.append(':').append(type.name());
-            if (elementNb > 1) {
-                sb.append(':').append(elementNb);
-            }
+        }
+        if (elementNb > 1) {
+            sb.append(':').append(elementNb);
        }


The problem here is that "[ ]" is used on the address. How to use it as optional and array? Like below?
tag[[index]][:dataType][:elementNb]

andvasp · 2026-08-26T14:43:34Z

there are conflicts btw, maybe rebase it
Thank you @sruehl .

That is one doubt that I have. If I can do a rebase or if I should merge develop here.

…mize this method that is called in each request for each tag address. I transfered part that identifies the index of the array to be done on the EipTag constructor. Enforce EipTag::getElementNb() to be greater than 1 removing the need to check it in EipTcpConnection.

sruehl · 2026-08-26T14:54:36Z

it is your fork you should be able to do whatever you like, commits will be squashed at the end anyway but this way we don't have those bogus merge commits in the PR set

sruehl · 2026-08-26T14:55:01Z

did you see the review comments?

andvasp · 2026-08-26T14:57:22Z

did you see the review comments?

Yes. I was rebasing. I will comment than now.

chrisdutz · 2026-08-27T06:33:04Z

So this sort of adds two index access parts to the address ... the "index" and the "elementNb" ... it seems as if the elementNb part sort of got obsolete.

Admittedly looking at the old and the new pattern I'm not really happy with any of them.

I did create a thread on dev@plc4x.apache.org last friday explicitly addressing this, however I didn't get even a single reply. (correction ... I got a response, but my mail client seems to have decided to keep all comments from that user from me)

My proposal there is here:
https://lists.apache.org/thread/vhhorv701nb8b7w2zr0oydn0npmcvc9x

andvasp · 2026-08-27T13:48:58Z

So this sort of adds two index access parts to the address ... the "index" and the "elementNb" ... it seems as if the elementNb part sort of got obsolete.

Admittedly looking at the old and the new pattern I'm not really happy with any of them.

But here we are not creating a new pattern, just splitting the variable tag and the index. What we would like to know if an empty array "%A[]" should be accepted as not having index or should throw an exception.

However, it does not matter any more because I noticed that you create your own version.

I did create a thread on dev@plc4x.apache.org last friday explicitly addressing this, however I didn't get even a single reply. (correction ... I got a response, but my mail client seems to have decided to keep all comments from that user from me)

My proposal there is here: https://lists.apache.org/thread/vhhorv701nb8b7w2zr0oydn0npmcvc9x

I like your proposal a lot, especially because it applies the same pattern for all drivers. Considering this, ideally would be good to be applied before release 1.0 to avoid break changes.

sruehl · 2026-08-27T13:59:37Z

so we can close this?

chrisdutz · 2026-08-27T14:15:49Z

Yeah ... it's superseeded by the "sligltly bigger" #2719

andvasp · 2026-08-27T15:43:03Z

Yeah ... it's superseeded by the "sligltly bigger" #2719

Sorry but I do not think it was superseded by it.. It was by b5f6ea2.

Here I am identifying the index on an existing pattern as a mechanism to use on toAnsi() eliminating the need of a regex.

sruehl requested review from chrisdutz and a balanced review from Copilot August 26, 2026 14:02

Copilot AI reviewed Aug 26, 2026

View reviewed changes

Copilot started reviewing on behalf of sruehl August 26, 2026 14:19 View session

andvasp force-pushed the feat/plc4j-eip-optimize-toAnsi branch from 68352d5 to bfd6294 Compare August 26, 2026 14:53

sruehl closed this Aug 27, 2026

Uh oh!

Conversation

andvasp commented Aug 26, 2026

Uh oh!

andvasp commented Aug 26, 2026

Uh oh!

sruehl commented Aug 26, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

andvasp Aug 26, 2026

Choose a reason for hiding this comment

Uh oh!

sruehl Aug 26, 2026

Choose a reason for hiding this comment

Uh oh!

andvasp Aug 26, 2026

Choose a reason for hiding this comment

Modubs:

Uh oh!

Uh oh!

andvasp Aug 26, 2026

Choose a reason for hiding this comment

Uh oh!

andvasp Aug 26, 2026

Choose a reason for hiding this comment

Uh oh!

andvasp commented Aug 26, 2026

Uh oh!

sruehl commented Aug 26, 2026

Uh oh!

sruehl commented Aug 26, 2026

Uh oh!

andvasp commented Aug 26, 2026

Uh oh!

chrisdutz commented Aug 27, 2026

Uh oh!

andvasp commented Aug 27, 2026

Uh oh!

sruehl commented Aug 27, 2026

Uh oh!

chrisdutz commented Aug 27, 2026

Uh oh!

andvasp commented Aug 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants