Copilot commented on code in PR #2674:
URL: https://github.com/apache/plc4x/pull/2674#discussion_r3746998821
##########
protocols/eip/src/test/resources/protocols/eip/DriverTestsuite.xml:
##########
@@ -427,4 +427,143 @@
</steps>
</testcase>
+ <testcase>
+ <name>Get Attribute Single Request</name>
+ <steps>
+ <outgoing-plc-message name="Send GetAttributeSingleRequest">
+ <parser-arguments>
+ <response>false</response>
+ </parser-arguments>
+ <EipPacket>
+ <command dataType="uint" bitLength="16">111</command>
+ <packetLength dataType="uint" bitLength="16">22</packetLength>
+ <sessionHandle dataType="uint"
bitLength="32">1074224669</sessionHandle>
+ <status dataType="uint" bitLength="32">0</status>
+ <senderContext dataType="byte"
bitLength="64">0x504c433458202020</senderContext>
+ <options dataType="uint" bitLength="32">0</options>
+ <CipRRData>
+ <interfaceHandle dataType="uint" bitLength="32">0</interfaceHandle>
+ <timeout dataType="uint" bitLength="16">0</timeout>
+ <typeIdCount dataType="uint" bitLength="16">2</typeIdCount>
+ <typeIds isList="true">
+ <TypeId>
+ <id dataType="uint" bitLength="16">0</id>
+ <NullAddressItem>
+ <reserved dataType="uint" bitLength="16">0</reserved>
+ </NullAddressItem>
+ </TypeId>
+ <TypeId>
+ <id dataType="uint" bitLength="16">178</id>
+ <UnConnectedDataItem>
+ <packetSize dataType="uint" bitLength="16">8</packetSize>
+ <service>
+ <CipService>
+ <response dataType="bit" bitLength="1">false</response>
+ <service dataType="uint" bitLength="7">14</service>
+ <GetAttributeSingleRequest>
+ <requestPathSize dataType="uint"
bitLength="8">3</requestPathSize>
+ <classSegment>
+ <PathSegment>
+ <pathSegment dataType="uint"
bitLength="3">1</pathSegment>
+ <LogicalSegment>
+ <segmentType>
+ <LogicalSegmentType>
+ <logicalSegmentType dataType="uint"
bitLength="3">0</logicalSegmentType>
+ <ClassID>
+ <format dataType="uint"
bitLength="2">0</format>
+ <segmentClass dataType="uint"
bitLength="8">1</segmentClass>
+ </ClassID>
+ </LogicalSegmentType>
+ </segmentType>
+ </LogicalSegment>
+ </PathSegment>
+ </classSegment>
+ <instanceSegment>
+ <PathSegment>
+ <pathSegment dataType="uint"
bitLength="3">1</pathSegment>
+ <LogicalSegment>
+ <segmentType>
+ <LogicalSegmentType>
+ <logicalSegmentType dataType="uint"
bitLength="3">1</logicalSegmentType>
+ <InstanceID>
+ <format dataType="uint"
bitLength="2">0</format>
+ <instance dataType="uint"
bitLength="8">1</instance>
+ </InstanceID>
+ </LogicalSegmentType>
+ </segmentType>
+ </LogicalSegment>
+ </PathSegment>
+ </instanceSegment>
+ <attributeSegment>
+ <PathSegment>
+ <pathSegment dataType="uint"
bitLength="3">1</pathSegment>
+ <LogicalSegment>
+ <segmentType>
+ <LogicalSegmentType>
+ <logicalSegmentType dataType="uint"
bitLength="3">4</logicalSegmentType>
+ <AttributeID>
+ <format dataType="uint"
bitLength="2">0</format>
+ <attribute dataType="uint"
bitLength="8">1</attribute>
+ </AttributeID>
+ </LogicalSegmentType>
+ </segmentType>
+ </LogicalSegment>
+ </PathSegment>
+ </attributeSegment>
+ </GetAttributeSingleRequest>
+ </CipService>
+ </service>
+ </UnConnectedDataItem>
+ </TypeId>
+ </typeIds>
+ </CipRRData>
+ </EipPacket>
+ </outgoing-plc-message>
+ <incoming-plc-message name="Receive GetAttributeSingleResponse">
+ <parser-arguments>
+ <response>true</response>
+ </parser-arguments>
+ <EipPacket>
+ <command dataType="uint" bitLength="16">111</command>
+ <packetLength dataType="uint" bitLength="16">22</packetLength>
+ <sessionHandle dataType="uint"
bitLength="32">1074224669</sessionHandle>
+ <status dataType="uint" bitLength="32">0</status>
+ <senderContext dataType="byte"
bitLength="64">0x504c433458202020</senderContext>
+ <options dataType="uint" bitLength="32">0</options>
+ <CipRRData>
+ <interfaceHandle dataType="uint" bitLength="32">0</interfaceHandle>
+ <timeout dataType="uint" bitLength="16">0</timeout>
+ <typeIdCount dataType="uint" bitLength="16">2</typeIdCount>
+ <typeIds isList="true">
+ <TypeId>
+ <id dataType="uint" bitLength="16">0</id>
+ <NullAddressItem>
+ <reserved dataType="uint" bitLength="16">0</reserved>
+ </NullAddressItem>
+ </TypeId>
+ <TypeId>
+ <id dataType="uint" bitLength="16">178</id>
+ <UnConnectedDataItem>
+ <packetSize dataType="uint" bitLength="16">6</packetSize>
+ <service>
+ <CipService>
+ <response dataType="bit" bitLength="1">true</response>
+ <service dataType="uint" bitLength="7">14</service>
+ <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>
Review Comment:
In this test, `GetAttributeSingleResponse` uses a `<data>` field, but the
mspec/generated model defines the payload as `servicesData` (and also expects
an `extStatus` list when `extStatusSize` is present). As written, the driver
test XML likely won’t match the generated read-write model for
`GetAttributeSingleResponse`.
##########
plc4j/drivers/eip/src/main/java/org/apache/plc4x/java/eip/base/EipTcpConnection.java:
##########
@@ -280,6 +278,48 @@ private CompletableFuture<Void> probeAttributes() {
});
}
+ private CipService getCipService(EipPacket response) {
+ if (response instanceof CipRRData rr
+ && rr.getStatus() == CIPStatus.Success.getValue()
+ && rr.getTypeIds().size() > 1
+ && rr.getTypeIds().get(1) instanceof UnConnectedDataItem di) {
+ return di.getService();
+ }
+ return null;
+ }
+
+ 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;
+ });
+ }
Review Comment:
If the first `checkAttributeSupport` call succeeds (setting
`useConnectionManager=true`) but the second one fails exceptionally
(timeout/parse/etc), the exception is handled by `probeAttributes()` and the
connection may still proceed with `useConnectionManager=true`, which
contradicts the intended “any probe failure falls back to unconnected mode”
behavior. Reset both flags on any failure in this single-attribute probe to
avoid partially-applied capability detection.
##########
plc4j/drivers/eip/src/main/java/org/apache/plc4x/java/eip/base/EipTcpConnection.java:
##########
@@ -280,6 +278,48 @@ private CompletableFuture<Void> probeAttributes() {
});
}
+ private CipService getCipService(EipPacket response) {
+ if (response instanceof CipRRData rr
+ && rr.getStatus() == CIPStatus.Success.getValue()
+ && rr.getTypeIds().size() > 1
+ && rr.getTypeIds().get(1) instanceof UnConnectedDataItem di) {
+ return di.getService();
+ }
+ return null;
+ }
+
+ 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;
+ });
+ }
+
+ private CompletableFuture<Boolean> checkAttributeSupport(CIPClassID
classId) {
+
+ UnConnectedDataItem exchange = new UnConnectedDataItem(new
GetAttributeSingleRequest(
+ 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
Review Comment:
`checkAttributeSupport` probes `InstanceID` 0, but the rest of the
connection setup (routing path, GetAttributeAll probe, ConnectionManager open)
targets instance 1 for these objects. Probing instance 0 can yield false
negatives on devices that don’t expose class-level instance 0, causing the
driver to incorrectly disable MessageRouter/ConnectionManager support.
##########
protocols/eip/src/main/resources/protocols/eip/eip.mspec:
##########
@@ -157,10 +157,17 @@
[array byte servicesData count 'serviceLen - 6 - (2 *
serviceNb)' ]
]
['0x0E','false' GetAttributeSingleRequest
- // TODO: Implement
+ [implicit uint 8 requestPathSize
'(classSegment.lengthInBytes + instanceSegment.lengthInBytes +
attributeSegment.lengthInBytes)/2']
+ [simple PathSegment classSegment
]
+ [simple PathSegment instanceSegment
]
+ [simple PathSegment attributeSegment
]
]
['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' ]
Review Comment:
`GetAttributeSingleResponse` is modeled with `extStatusSize` + an
`extStatus` array, but other CIP service responses in the same mspec use a
single 8-bit `extStatus` field (e.g., `GetAttributeAllResponse`,
`CipReadResponse`, `CipWriteResponse`). Having `GetAttributeSingleResponse`
diverge here makes the CIP response header inconsistent across services and
will require special-casing in any shared handling/parsing code. Consider
aligning this response header representation with the existing pattern (or
refactoring all CIP responses together to the expanded format) so the model
stays consistent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
