feat(plc4j/eip): Detect Connection Manager and Message using "Get Attribute Single"

[andvasp]

Hi @chrisdutz ,

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

[chrisdutz]

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.

[chrisdutz]

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.

[andvasp]

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);
}

[chrisdutz]

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 ;-)

[andvasp]

Hi Chris!

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

[andvasp]

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.

[chrisdutz]

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?

[chrisdutz]

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?