apache / plc4x

PLC4X The Industrial IoT adapter
https://plc4x.apache.org/
Apache License 2.0
1.27k stars 403 forks source link

[Bug]: [PLC4J] [EtherNet/IP] All values read are null for Allen-Bradley PLC with EtherNet/IP or Logix #1646

Closed meichf closed 2 months ago

meichf commented 5 months ago

What happened?

I wrote two programs according to the documentation on the official website of PLC4X to read Allen Bradley PLC, model Logix5571, but all the values read were null.

Here is the code. The result is that all four tag values in asPlcValue are null.

code for EtherNet/IP protocol:

    String connectionString = "eip://192.168.0.2:44818?slot=0&bigEndian=false";

    try (PlcConnection plcConnection = PlcDriverManager.getDefault().getConnectionManager().getConnection(connectionString)) {
        final PlcReadRequest.Builder readrequest = plcConnection.readRequestBuilder();

    //(3.1)
        readrequest.addTagAddress("tag1", "%A:DINT");
        readrequest.addTagAddress("tag2", "%B:INT");
        readrequest.addTagAddress("tag3", "%C:SINT");
        readrequest.addTagAddress("tag4", "%D:REAL");

        //(3.2)
        final PlcReadRequest rr = readrequest.build();
        //(3.3)
        final PlcReadResponse szlresponse = rr.execute().get();

        PlcValue asPlcValue = szlresponse.getAsPlcValue();

    } catch (PlcConnectionException e) {
        throw new RuntimeException(e);
    } catch (Exception e) {
        throw new RuntimeException(e);
    }

code for Logix protocol:

   String connectionString = "logix://192.168.0.2:44818?slot=0&bigEndian=false";

    try (PlcConnection plcConnection = PlcDriverManager.getDefault().getConnectionManager().getConnection(connectionString)) {
        PlcConnectionMetadata metadata = plcConnection.getMetadata();
        final PlcReadRequest.Builder readrequest = plcConnection.readRequestBuilder();

        //(3.1)
        readrequest.addTagAddress("tag1", "A:DINT");
        readrequest.addTagAddress("tag2", "B:INT");
        readrequest.addTagAddress("tag3", "C:SINT");
        readrequest.addTagAddress("tag4", "D:REAL");

        //(3.2)
        final PlcReadRequest rr = readrequest.build();
        //(3.3)
        final PlcReadResponse szlresponse = rr.execute().get();
        //(3.4)
        PlcValue asPlcValue = szlresponse.getAsPlcValue();
        log.info("Tags: " + asPlcValue);
    } catch (PlcConnectionException e) {
        throw new RuntimeException(e);
    } catch (Exception e) {
        throw new RuntimeException(e);
    }`

here is the tags:

tags

Version

v0.12.0

Programming Languages

Protocols

chrisdutz commented 4 months ago

So I've just created a ManualTest for my Logix device. I created several Controller Tags (Including UDTs) They are all called "hurz_{datatypeName}" for simplicity. the Following program works when reading:

    public static void main(String[] args) throws Exception {
        ManualEipLogixDriverTest test = new ManualEipLogixDriverTest("logix://192.168.23.40");
        // This is the very limited number of types my controller supports.
        test.addTestCase("hurz_BOOL", new PlcBOOL(true));
        test.addTestCase("hurz_SINT", new PlcSINT(-42));
        test.addTestCase("hurz_INT", new PlcINT(-2424));
        test.addTestCase("hurz_DINT", new PlcDINT(-242442424));
        test.addTestCase("hurz_REAL", new PlcREAL(3.141593F));
        //test.addTestCase("hurz_UDT", new PlcStruct());

        long start = System.currentTimeMillis();
        test.run();
        long end = System.currentTimeMillis();
        System.out.printf("Finished in %d ms", end - start);
    }

However I can see the UDT being correctly sent on the wire using WireShark, just it seems that decoding it is a problem for the driver. Also can't I write. As soon as I try that I get errors.

chrisdutz commented 4 months ago

So if I look at your code, omit the "%" and the datatypes. Just "A", "B", "C", or "D" should be enough.

chrisdutz commented 4 months ago

Ok ... I learned something more ... when reading, you can omit the datatype ... however when writing it seems to be mandatory. I think that in general we will have to implement some sort of "load the symbol table when connecting" similar to what we do for ADS and use that information throughout the session ... especially for UDTs I otherwise have no way to process them correctly.

meichf commented 4 months ago

So if I look at your code, omit the "%" and the datatypes. Just "A", "B", "C", or "D" should be enough.

I‘ve tried this, but it doesn't work

chrisdutz commented 4 months ago

Are the tags you are trying to read controller tags or program tags? I tried with my logix device and above only worked for controller tags.

meichf commented 4 months ago

Are the tags you are trying to read controller tags or program tags? I tried with my logix device and above only worked for controller tags. It's controller tags. My device is the Allen Bradley 1756-L71, I don't know if the logix protocol is valid for this device.

hutcheb commented 4 months ago

The L71 is a little older than the one Chris has, it is probably running v20 and I think he should be using v35 or something. Can you confirm which version? There is probably going to be differences between v20 and the higher versions.

meichf commented 4 months ago

s Do you mean programming tools? I'm using the studio 5000, v33.

hutcheb commented 4 months ago

Then thats probably not the issue, Which version of PLC4X are you using?

meichf commented 4 months ago

I have tried both 0.12.0 and 0.13.0-SNAPSHOT

meichf commented 4 months ago

I have identified the problem, it works on version 0.11.0 .

chrisdutz commented 4 months ago

I think that is just plain EIP/CIP and no Logix extensions, right? I think we completely rewrote the driver for 0.12.0

hutcheb commented 4 months ago

@meichf Why are you setting the bigEndian parameter to false? Specifically for the logix driver in 0.13-SNAPSHOT.

meichf commented 4 months ago

I think that is just plain EIP/CIP and no Logix extensions, right? I think we completely rewrote the driver for 0.12.0

In version 0.11.0, both EtherNet/IP and Logix were successful, while in version 0.12.0, neither was successful.

meichf commented 4 months ago

@meichf Why are you setting the bigEndian parameter to false? Specifically for the logix driver in 0.13-SNAPSHOT.

It seems that without this parameter, I am unable to connect to the device.

meichf commented 4 months ago

I found that using version 0.11.0 to read data is slow, and the duration of returning data increases with the number of points. Then, I found the problem in issue #1373, so I rewrote the method. Now, reading values is relatively normal.


private CompletableFuture<PlcReadResponse> readWithoutMessageRouter(PlcReadRequest readRequest) {
    CompletableFuture<PlcReadResponse> future = new CompletableFuture<>();
    Map<String, ResponseItem<PlcValue>> values = new HashMap<>();
    List<CompletableFuture<Void>> internalFutures = new ArrayList<>();
    PathSegment classSegment = new LogicalSegment(new ClassID((byte) 0, (short) 6));
    PathSegment instanceSegment = new LogicalSegment(new InstanceID((byte) 0, (short) 1));

    DefaultPlcReadRequest request = (DefaultPlcReadRequest) readRequest;
    for (String tagName : request.getTagNames()) {
        CompletableFuture<Void> internalFuture = new CompletableFuture<>();
        RequestTransactionManager.RequestTransaction transaction = tm.startRequest();
        EipTag eipTag = (EipTag) request.getTag(tagName);
        String tag = eipTag.getTag();

        try {
            CipReadRequest req = new CipReadRequest(toAnsi(tag), 1);
            CipUnconnectedRequest requestItem = new CipUnconnectedRequest(classSegment, instanceSegment, req, (byte) this.configuration.getBackplane(), (byte) this.configuration.getSlot());
            List<TypeId> typeIds = new ArrayList<>(2);
            typeIds.add(nullAddressItem);
            typeIds.add(new UnConnectedDataItem(requestItem));
            CipRRData pkt = new CipRRData(sessionHandle, CIPStatus.Success.getValue(), DEFAULT_SENDER_CONTEXT, 0L, 0L, 0, typeIds);

            transaction.submit(() -> context.sendRequest(pkt)
                    .expectResponse(EipPacket.class, REQUEST_TIMEOUT)
                    .onTimeout(future::completeExceptionally)
                    .onError((p, e) -> future.completeExceptionally(e))
                    .check(p -> p instanceof CipRRData)
                    .unwrap(p -> (CipRRData) p)
                    .check(p -> p.getSessionHandle() == sessionHandle)
                    .handle(p -> {
                        List<TypeId> responseTypeIds = p.getTypeIds();
                        UnConnectedDataItem dataItem = (UnConnectedDataItem) responseTypeIds.get(1);
                        Map<String, ResponseItem<PlcValue>> readResponse = decodeSingleReadResponse(dataItem.getService(), tagName, eipTag);
                        values.putAll(readResponse);
                        internalFuture.complete(null);
                        transaction.endRequest();
                    }));
            internalFutures.add(internalFuture);
        } catch (SerializationException e) {
            future.completeExceptionally(e);
        }
    }

    CompletableFuture.allOf(internalFutures.toArray(new CompletableFuture[0])).thenRun(() -> {
        PlcReadResponse readResponse = new DefaultPlcReadResponse(readRequest, values);
        future.complete(readResponse);
    }).exceptionally(e -> {
        future.completeExceptionally(e);
        return null;
    });

    return future;
}
acfly commented 4 months ago

@chrisdutz The Rewritten method readWithoutMessageRouter realy works. Hope it will be merged into the next version.

chrisdutz commented 4 months ago

I think the new version of the driver sort of does an auto-detection of the PLC and decides which technique to use, based on that. Will see if I can have a look tomorrow afternoon.

kdxq commented 3 months ago

Hi,@chrisdutz, I encountered the same problem, only 0.11. can read and write, while 0.12 and 0.13-- SNAPSHOT both failed. For 0.11 reads, it supports int, dint,float,string , But it does not support bool; Writing only supports int and dint, not support bool and float types

chrisdutz commented 3 months ago

I'd really love to make the EIP drivers more rock-sollid, however I don't have any of the "big" AB devices, so it's difficult for me to work on this. It is possible, but it takes 3-4 times longer to do it without the hardware (and corresponding software). If anyone is willing to provide me with that (borrowing it ... not donating it ... however I would not object donations ;-) ... or by donating the funds to purchase a used model) that would help.

chrisdutz commented 3 months ago

I think the real solution to this problem is to switch back from the extended large forward messages to the older and smaller ones. Unconnected messages add a lot more traffic to the network as the packets are a lot bigger.

chrisdutz commented 3 months ago

Based on your changes I worked a bit on the EIP driver today and I think it should now be in a better state. I also added a new config option: "force-unconnected-operation" to the connection string to make the driver use the unconnected option. Also did I apply some other changes to that method that should make it more stable.

chrisdutz commented 3 months ago

Please test.

kdxq commented 3 months ago

Hi,@chrisdutz,I will test it latter. by the way, in the 0.13.0 version, I do not find the opcua-server,is 0.12.0 version the last version to support it?

chrisdutz commented 3 months ago

We recently split up the repo into two parts ... on part that contains the core of PLC4X (This repo) and one that contains additional tools, utilities, integration modules and examples.

This is a preparation step to coming legislative changes coming our way ... this way we can keep the main repo clear of most third party CVE issues.

kdxq commented 3 months ago

Hi,@chrisdutz, just I use 0.13.0-SNAPSHOT for test. get the same result, all tags get null values. what should I do for the next test can help you to improve the program ?

chrisdutz commented 3 months ago

Did you build PLC4X locally or use the latest SNAPSHOT from maven? I'M asking, because there seems to be an issue with the machine producing the SNAPSHOTS ... so if you're not building on your own, you've got 2 weeks old versions and they don't have my changes in them.

If you did build yourself, please create and attach a wireshark recording.

kdxq commented 3 months ago

Hi,@chrisdutz, you are right, I use the old SNAPSHOTS version(20240625). but what should I do to get the new version without build it by myself?

kdxq commented 3 months ago

could you give me an url which can download new SNAPSHOTS version manually?

chrisdutz commented 3 months ago

This should help: https://plc4x.apache.org/developers/building.html mvnw -P with-java -DskipTests install Should do the trick

chrisdutz commented 3 months ago

Currently there is no updated SNAPSHOT version as the system building them has taken a break. You need to checkout the latest sources from GitHub and then build it on your local machine.

kdxq commented 3 months ago

I will try it now

kdxq commented 3 months ago

wireshark-file.zip Hi,@chrisdutz, the attach file is the test result. now, the 0.13.0 version can read and write,just like 0.11.0 version. for reading, it supports int, dint,float,string, but it does not support bool; for Writing only supports int and dint, not support bool and float types. please take a look the attach file for details. waiting for your replay. thank you very much.

kdxq commented 3 months ago

if you need do any test. please tell me. I will do it as soon as possible.

chrisdutz commented 3 months ago

Yeah ... so as this Friday I'll be on a music-festival, the earliest time I will be able to have a look will be in 2 weeks. So please be patient.

kdxq commented 3 months ago

ok.

kdxq commented 3 months ago

Hi,@chrisdutz,Based on the perfect and stable performance of the plc4x-s7 driver, the original plan was to use the plc4x eip driver in our industrial production environment. However, currently, the eip driver lacks stability, so we have to abandon this idea. During the initial testing, we also encountered the following error, as shown in the figure below. Hoping it will be helpful for you to improve the driver. f0ae689dd62a6d1d105081a5b4bbab6

chrisdutz commented 3 months ago

Hi kdxq ... that's sad to hear, but nothing I guess we'll be able to change. The S7 was the first PLC4X driver that I implemented and I guess the most tested one. Strangely after that most feedback seemed to be around Modbus and just recently it seems people were starting to pick up EIP. And especially with the Allen Bradley devices, I only recently bought one. So expect things to mature, but not that quickly as in the past.

chrisdutz commented 3 months ago

And may I ask, what you are now using instead?

kdxq commented 3 months ago

I think the feedback from Modbus and EIP more indicates that S7 is already stable and perfect enough. We develop L2 software for the steel plant, which requires communication with different brands of PLCs(Siemens PLCs are still the most commonly used in steel plant of China). Thank you again for your excellent and perfect work for PLC4X @chrisdutz

kdxq commented 3 months ago

now, We are currently testing https://github.com/digitalpetri/ethernet-ip You are also one of the main contributors to this open-source software. If it doesn't work, we will ultimately choose the commercial kepserver

chrisdutz commented 3 months ago

Hehe ... in initial versions of PLC4X, before we had MSPEC I had hand implemented the S7 protocol, but we used the digital-petri drivers for Modbus and EIP and replaced them with our implementations as soon as we had mspec. So ... yeah ... I had some feedback while we were still using it and my PRs were all merged, I think ;-)

chrisdutz commented 3 months ago

Well ... regarding kepware ... the thing is, that this will be expensive (The last time I checked, if you want to use it on non-windows, the price was something around 5000€ per node) ... I did post on several occasions, if it would not be cheaper to simply finance someone of the PLC4X community to iron out the issues you were having?

You just had the bad luck to start using it the moment I decided to give up working on it in my free time as your steel plant is not the only one using it and I was spending a lot of time and money for providing the drivers to all for free without ever getting anything back in return.

kdxq commented 3 months ago

Yes, I have been following plc4x since plc4x0.8.0, and I know the contributions and excellent work you have done for plc4x. I remember when you announced give up, China's software WeChat reported that you gave up. our colleagues felt sorry for this. 1117b11d61d40595000783a12340199

kdxq commented 3 months ago

As developers in the enterprise, we know that paying for open source software is more cost-effective than commercial software, but there is no payment process for open source software. Many times it's not that we don't want to pay for it, it's just that there isn't a standardized company process in place.

kdxq commented 3 months ago

https://buymeacoffee.com/christoferu and this not support alipay

chrisdutz commented 3 months ago

Well I did have a company until 1,5 years ago. There I offered commercial consulting to the industry using the normal commercial channels, but unfortunately nobody used that so I had to give it up :-/ ... if you send me a personal email, I might have an option for you.

kdxq commented 3 months ago

@chrisdutz I just want to personally buy a few cups of coffee for you, to make an outstanding contribution to plc4x, not for solve problems. I don't want to try exploring company processes, it;s very complex. my email kdxq@foxmail.com

chrisdutz commented 3 months ago

Hmmm ... ok from having a look at the wireshark capture, I think I can explain why the reading of booleans doesn't work ... it seems your system is Little-Endian and I think the system would expect a boolean true to be tranmitted "0x0100" but instead it is "0x0001" ... Also do I notice, that you seem to be opening and closing the connection every few seconds. Have you thought of using the ConnectionCache? This should dramatically reduce the speed of the connection.

I'd really love to iron this out on Little-Endian devices, but as I mentioned before, it's super tricky and takes a lot longer to do so. I can see in the decoding that the code for parsing and serializing booleans probably doesn't work as expected for Little-Endian systems also do I think parsing a float BE and then rearranging the bytes is the wrong order when writing.

It would probably be super easy to test with a real device, but without I will have to give up as it would be unfair for the other folks who are also waiting in this wall of issues that has been growing over the last few weeks.

kdxq commented 3 months ago

Hi,@chrisdutz, Our project has been delayed for one month, and I think we have more time to do testing. I do hope that plc4x will become better and that those who put in the effort will be rewarded. If you are interested, I can mention supporting open source to our project manager. But I'm not sure if it can be approved, it depends on various reasons, and the price is just one of them (we have already done an inquiry for kepserver, 16000 RMB). So, I would like to know the specific price?