feat(plc4j/eip): Detect Connection Manager and Message using "Get Attribute Single" - #2674
feat(plc4j/eip): Detect Connection Manager and Message using "Get Attribute Single" #2674andvasp wants to merge 5 commits into
Conversation
… cases where "Get Attribute All" is not supported by the device.
| 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(); |
There was a problem hiding this 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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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);
}
There was a problem hiding this comment.
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 ;-)
There was a problem hiding this comment.
Hi Chris!
I push a new version. Please check if it is better now.
| } | ||
|
|
||
| private CompletableFuture<Void> checkAttributesSingle() { | ||
| private CipService getCipService(EipPacket response) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)There was a problem hiding this comment.
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);
};
There was a problem hiding this comment.
I liked the suggestion! I will do it.
7b6f222 to
fa7a737
Compare
…ce the return on processing the CompletableFuture instead of setting the future as completed. Refactoring.
| } | ||
|
|
||
| private CompletableFuture<Void> checkAttributesSingle() { | ||
| private CipService getCipService(EipPacket response) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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(plusAttributeIDpath segment type) in the EIP mspec and generated read-write classes. - Updated
EipTcpConnection#probeAttributes()to fall back toGet_Attribute_SinglewhenGet_Attribute_Allis reported as unsupported. - Added a driver test case describing a
GetAttributeSinglerequest/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> |
| 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 |
There was a problem hiding this comment.
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.
| 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; | ||
| }); | ||
| } |
There was a problem hiding this comment.
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 @chrisdutz ?
| ['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' ] |
There was a problem hiding this comment.
Yes. Now I'm curious.
The "header" message specification layout is the same in both messages (Reply Service, Reserved, Status, Additional Status Size, and Additional Status).
I don't know much about mspec, but it looks to me like the extStatusSize field is missing from GetAttributeAllResponse. Am I doing something wrong with GetAttributeSingleResponse? Why is there this difference?
@chrisdutz, could you clarify this?
There was a problem hiding this comment.
Hehe ... well ... I could immagine, that we simply got the others wrong ;-)
I would need to check if possibly all others we only had extStatus=0 ... in that case our other cases would have worked. I'll let Claude do some research ... however my gut-feeling tells me that I would expect your version to be right and ours to be wrong.
There was a problem hiding this comment.
Yup ... you were right ... guess we need to update our mspec ;-)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (3)
protocols/eip/src/main/resources/protocols/eip/eip.mspec:170
extStatusSizeis the number of 16-bit additional-status words, but this models each entry as 8 bits and subtracts only one byte per entry. Any response with a nonzero additional status will therefore split the status word and expose its second byte asservicesData; serialization also emits an invalid packet. DefineextStatusasuint 16, subtract2 * extStatusSize, regenerate the generated models, and cover a nonzero additional-status response.
[array uint 8 extStatus count 'extStatusSize' ]
[array byte servicesData count 'serviceLen - 4 - extStatusSize' ]
protocols/eip/src/test/resources/protocols/eip/DriverTestsuite.xml:430
- This fixture is not currently exercised: the only runner referencing
/protocols/eip/DriverTestsuite.xml,EIPDriverIT, is annotated@Disabled. Consequently the new GetAttributeSingle codec—and especially the ServiceNotSupported fallback—can regress without CI detecting it. Add coverage to an active parser/serializer suite and a connection-level test that scripts status0x08followed by the two single-attribute probes, or re-enable the driver-suite runner.
<testcase>
plc4j/drivers/eip/src/main/java/org/apache/plc4x/java/eip/base/EipTcpConnection.java:256
- Because the single-attribute future is composed here, failures from either fallback probe reach the outer handler. If ConnectionManager was confirmed before the MessageRouter probe fails, its flag is intentionally preserved, but the handler currently says every failure means both capabilities are absent and logs it as a GetAttributeAll failure. Update that comment and log message to describe continuing with capabilities already confirmed.
case GetAttributeAllResponse gar when gar.getStatus() == CIPStatus.ServiceNotSupported.getValue()
-> probeAttributesUsingSingleAttributeRequest();
Would be just necessary change uint 8 to 16 like below? |
Hi @chrisdutz ,
Implement "Get Attribute Single" on EIP protocol for cases where "Get Attribute All" is not supported by the device to address #2135.