feat(plc4j/eip): Detect Connection Manager and Message using "Get Attribute Single" by andvasp · Pull Request #2674 · apache/plc4x

andvasp · 2026-08-05T20:21:37Z

Implement "Get Attribute Single" on EIP protocol for cases where "Get Attribute All" is not supported by the device to address #2135.

… cases where "Get Attribute All" is not supported by the device.

chrisdutz · 2026-08-05T21:40:45Z

+            if (!(response instanceof CipRRData rr) || rr.getStatus() != CIPStatus.Success.getValue() ||
+                !(rr.getTypeIds().get(1) instanceof UnConnectedDataItem di && di.getService() instanceof GetAttributeAllResponse gar)) {
+                return CompletableFuture.completedFuture(null);
            }
            if (gar.getStatus() == CIPStatus.ServiceNotSupported.getValue()) {
-                return;
+                return checkAttributesSingle();


Admittedly I find this particular part of code quite hard to read ... Could you please simplify this a bit?

Admittedly I'm a big fan of the "di.getService() instanceof GetAttributeAllResponse gar" notation saving myself the explicit cast, but I am super unhappy with the decision of the Java group in a negated form to make the variable available outside the if statement (Which you are using) ... it's just challenging from a maintenance perspective.

Also leaving a few comments here to what's happeing would be great. I know I didn't set a good example but I'm trying my best to leave more comments for my fellow maintainers if I think something's tricky to understand. I guess the problematic you're trying to solve would qualify for such a comment.

Admittedly I find this particular part of code quite hard to read ... Could you please simplify this a bit?

Admittedly I'm a big fan of the "di.getService() instanceof GetAttributeAllResponse gar" notation saving myself the explicit cast, but I am super unhappy with the decision of the Java group in a negated form to make the variable available outside the if statement (Which you are using) ... it's just challenging from a maintenance perspective.

Here I see 3 options, where I prefer the sequential other. What do you think?

Option 1:
if (!(response instanceof CipRRData rr) || rr.getStatus() != CIPStatus.Success.getValue() ||
!(rr.getTypeIds().get(1) instanceof UnConnectedDataItem di) ||
!(di.getService() instanceof GetAttributeAllResponse gar)) {
return CompletableFuture.completedFuture(null);
}

Option 2:
if (!(response instanceof CipRRData rr) || rr.getStatus() != CIPStatus.Success.getValue()) {
return CompletableFuture.completedFuture(null);
}
if (!(rr.getTypeIds().get(1) instanceof UnConnectedDataItem di) ||
!(di.getService() instanceof GetAttributeAllResponse gar)) {
return CompletableFuture.completedFuture(null);
}

Option 3:
if (!(response instanceof CipRRData rr) || rr.getStatus() != CIPStatus.Success.getValue()) {
return CompletableFuture.completedFuture(null);
}
UnConnectedDataItem dataItem = (UnConnectedDataItem) rr.getTypeIds().get(1);
if (!(dataItem.getService() instanceof GetAttributeAllResponse gar)) {
return CompletableFuture.completedFuture(null);
}

I'm more trying to wrap my head around what the code should do ...

If it's (not a CipRRData) or (it is and it's status is not success) or (it is and it's first type id is an UnConnectedDataItem) or (it is, it's first type id is a UnConnectedDataItem's service is a GetAttributeAllResponse ....

So much negation ... wouldn't it be an alternative to focus what we expect it to be?
Something like this?

if (response instanceof CipRRData rr && rr.getStatus() == CIPStatus.Success.getValue() && rr.getTypeIds().size() > 1 && rr.getTypeIds().get(1) instanceof UnConnectedDataItem di && serviceType.isInstance(di.getService())) { return serviceType.cast(di.getService()); } return null;

That I would instantly understand ;-)

Hi Chris!

I push a new version. Please check if it is better now.

andvasp · 2026-08-07T16:40:27Z

    }

-    private CompletableFuture<Void> checkAttributesSingle() {
+    private CipService getCipService(EipPacket response) {


I created this method to encapsulate the logic for get the CipService. I believe it could also be applied elsewhere, even though the logic isn't exactly the same.

Let me know what you think.

A simple getCipService would make me expect that it simply gets the CIP service ... here the method is actually extracting something if a very specific data-case is present or doesn't do anything if that's not the case.

Are we using this or could we use this in different places?

In the past we used a lot of "return null" methods and are more and more trying to use Optionals in Java ... I tink renaming it to something that indicates the fact that it's not just blindly accessing something, possibly something like:

Optional<CipService> extractCipService(EipPacket response)

Might not hide this detail?

A simple getCipService would make me expect that it simply gets the CIP service ... here the method is actually extracting something if a very specific data-case is present or doesn't do anything if that's not the case.

Yes. extractCipService is a better name. Actually, I thought about using it when thinking about possible solutions but when implementing it I forget to use it.

Are we using this or could we use this in different places?

I am using this method at 2 places. I found other places that could use it but as they do not have exactly the same logic, I prefer to not change them. But I believe would be good to use. Let me know what you think.

In the past we used a lot of "return null" methods and are more and more trying to use Optionals in Java ... I tink renaming it to something that indicates the fact that it's not just blindly accessing something, possibly something like:

Optional<CipService> extractCipService(EipPacket response)

I considered using Optional, but in this case I found it more verbose and as this method is used just internally in this class and with Pattern Matching, I prefer to not use Optional here. See the examples below and tell me what do you think.

if (service.isPresent() && service.get() instanceof GetAttributeSingleResponse gsr) // I do not like to use isPresent() and get() together but I believe is less verbose.

I considered create the method below but again I think is more verbose and it with extra comparations at Class methods.

<T> Optional<T> extractCipService(EipPacket response, Class<T> type)

What do you think about this? As I mentioned ... I'm not very fond of these cast variables outside of the code block that creates it? (And yes ... I know you didn't introduce this pattern ;-)

return switch (getCipService(response)) { case GetAttributeAllResponse gar when gar.getStatus() == CIPStatus.ServiceNotSupported.getValue() -> probeAttributesUsingSingleAttributeRequest(); case GetAttributeAllResponse gar -> { recordSupportedClasses(gar.getAttributes()); yield CompletableFuture.completedFuture(null); } case null, default -> CompletableFuture.completedFuture(null); };

I liked the suggestion! I will do it.

…ce the return on processing the CompletableFuture instead of setting the future as completed. Refactoring.

chrisdutz · 2026-08-09T16:07:55Z

    }

-    private CompletableFuture<Void> checkAttributesSingle() {
+    private CipService getCipService(EipPacket response) {


A simple getCipService would make me expect that it simply gets the CIP service ... here the method is actually extracting something if a very specific data-case is present or doesn't do anything if that's not the case.

Are we using this or could we use this in different places?

In the past we used a lot of "return null" methods and are more and more trying to use Optionals in Java ... I tink renaming it to something that indicates the fact that it's not just blindly accessing something, possibly something like:

Optional<CipService> extractCipService(EipPacket response)

Might not hide this detail?

Copilot

Pull request overview

This PR adds EtherNet/IP (CIP) “Get Attribute Single” support and uses it as a fallback capability probe when “Get Attribute All” is not supported by a target device (e.g., certain Rockwell ControlLogix models), so PLC4J can still detect and use the Message Router / Connection Manager paths.

Changes:

Implemented CIP GetAttributeSingleRequest / GetAttributeSingleResponse (plus AttributeID path segment type) in the EIP mspec and generated read-write classes.
Updated EipTcpConnection#probeAttributes() to fall back to Get_Attribute_Single when Get_Attribute_All is reported as unsupported.
Added a driver test case describing a GetAttributeSingle request/response exchange.

Reviewed changes

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

Show a summary per file

File	Description
protocols/eip/src/test/resources/protocols/eip/DriverTestsuite.xml	Adds a new driver test case for `GetAttributeSingle` request/response.
protocols/eip/src/main/resources/protocols/eip/eip.mspec	Implements `GetAttributeSingle*` messages and introduces `AttributeID` in logical segments.
plc4j/drivers/eip/src/main/java/org/apache/plc4x/java/eip/base/EipTcpConnection.java	Adds runtime fallback probing via `GetAttributeSingle` when `GetAttributeAll` is not supported.
plc4j/drivers/eip/src/main/generated/org/apache/plc4x/java/eip/readwrite/LogicalSegmentType.java	Extends logical segment type parsing to include `AttributeID`.
plc4j/drivers/eip/src/main/generated/org/apache/plc4x/java/eip/readwrite/GetAttributeSingleResponse.java	Generated implementation for parsing/serializing `GetAttributeSingleResponse`.
plc4j/drivers/eip/src/main/generated/org/apache/plc4x/java/eip/readwrite/GetAttributeSingleRequest.java	Generated implementation for parsing/serializing `GetAttributeSingleRequest`.
plc4j/drivers/eip/src/main/generated/org/apache/plc4x/java/eip/readwrite/AttributeID.java	New generated logical segment type for Attribute ID addressing.

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

+                      <GetAttributeSingleResponse>
+                        <reserved dataType="uint" bitLength="8">0</reserved>
+                        <status dataType="uint" bitLength="8">0</status>
+                        <extStatusSize dataType="uint" bitLength="8">0</extStatusSize>
+                        <data dataType="byte" bitLength="16">0x0100</data>
+                      </GetAttributeSingleResponse>


andvasp · 2026-08-10T22:02:15Z

+            new LogicalSegment(new ClassID((byte) 0, (short) classId.getValue())),
+            new LogicalSegment(new InstanceID((byte) 0, (short) 0)), // Class level discovery
+            new LogicalSegment(new AttributeID((byte) 0, (short) 1))) // Attribute ID 1: Revision


I was using instanceId 1 at previous commit. I changed it after search and find out that 0 is used to discover.
Looks like the PLC LOGIX5573 supports message router and connection manager. When I test it using instanceId = 0 I receive the information that it support but does not when using instance = 1. So I believe instanceId = 0 is correct.

andvasp · 2026-08-10T23:13:47Z

+    private CompletableFuture<Void> probeAttributesUsingSingleAttributeRequest() {
+        LOGGER.debug("Checking MessageRouter and ConnectionManager using GetAttributeSingle");
+
+        return checkAttributeSupport(CIPClassID.ConnectionManager).thenCompose(hasSupport -> {
+            useConnectionManager = hasSupport;
+            return checkAttributeSupport(CIPClassID.MessageRouter);
+        }).thenAccept(hasSupport -> {
+            useMessageRouter = hasSupport;
+        });
+    }


If we can check that the connection manager is available and an error happens while checking for message router support, I believe we should keep the connection manager as available and not fallback to unconnected code path. This way we would just update the comments on the exceptionally block.

What do you think?

        ['0x0E','true'  GetAttributeSingleResponse
-            // TODO: Implement
+            [reserved   uint    8           '0x00'                                                                     ]
+            [simple     uint    8           status                                                                     ]
+            [simple     uint    8           extStatusSize                                                              ]
+            [array      uint    8           extStatus count 'extStatusSize'                                            ]
+            [array      byte   servicesData count 'serviceLen - 4 - extStatusSize'                                     ]


feat(plc4j/eip): Implement "Get Attribute Single" on EIP protocol for…

ddd733f

… cases where "Get Attribute All" is not supported by the device.

andvasp mentioned this pull request Aug 5, 2026

[Bug]: Integration Issues with Rockwell ControlLogix PLC #2135

Open

19 tasks

chrisdutz requested changes Aug 5, 2026

View reviewed changes

andvasp commented Aug 7, 2026

View reviewed changes

andvasp marked this pull request as draft August 7, 2026 18:55

Addressing review comments.

fa7a737

andvasp force-pushed the feat/plc4j-eip-GetAttributeSingle branch from 7b6f222 to fa7a737 Compare August 7, 2026 19:21

Try to make the code on checkAttributeSupport more robust because for…

cc159f6

…ce the return on processing the CompletableFuture instead of setting the future as completed. Refactoring.

andvasp marked this pull request as ready for review August 9, 2026 12:22

chrisdutz requested changes Aug 9, 2026

View reviewed changes

sruehl requested a lite review from Copilot August 10, 2026 05:56

Copilot started reviewing on behalf of sruehl August 10, 2026 05:57 View session

Copilot AI reviewed Aug 10, 2026

View reviewed changes

andvasp added 2 commits August 10, 2026 18:35

Addressing Chris comments.

7bcbf37

Addressing Copilot comments.

203bc93

Uh oh!

Conversation

andvasp commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

andvasp commented Aug 5, 2026 •

edited

Loading