[Bug]: ADS driver option load-symbol-and-data-type-tables=false doesn't resolve the variable #1626

mrwhy-orig · 2024-05-30T10:01:05Z

chrisdutz · 2024-05-30T13:39:32Z

Is there a particular reason for setting the option to false?
I mean ... yes: The resolution of symbolic addresses should in that case still work asynchronously, but the browsing would be quite a bit of work and be quite stressfull on the PLC. I would be leaning toward not allowing to browse in case of loading of the tables being disabled.

mrwhy-orig · 2024-06-03T07:44:46Z

I had some connection issues with the option being true. Like one out of ten I ran into an exception. So I thought maybe the Symboltable is to big... I really only need a very small subset, I'll put this option to false.

chrisdutz · 2024-06-14T16:05:16Z

The size should not matter ... might be something else going wrong. A wireshark capture could help, but if you do, I should make you aware that it would contain the structure of all variables in your PLC ... if that's something you don't want to share with everyone, you might consider sending me a link to cdutz@apache.org ... then I could have a look and possibly find out why this 1 of 10 doesn't work.

Other than that ... yes ... the driver should lookup the symbolic address, if you disable the loading on connection.

fdupont-epsilia · 2024-09-27T19:44:24Z

I when through the same though process as mrwhy-orig. Maybe symbolic too big, trying « load-symbol-and-data-type-tables » to « false », which end up « INVALID_ADDRESS ».

In my case, 10 out of 10 the connection doesn't work.

In the connection, I set the usual : target-ams-net-id, arget-ams-port, source-ams-net-id and source-ams-port
Had to increase the timeout. Made it bigger than necessary : timeout-request=20000

Program hang on the connection.
Wireshark attached.
1626-fdupont.pcapng.gz

Last packet is smaller than the others.

Test program :

PlcDriverManager plcDriver = new DefaultPlcDriverManager();
  try (PlcConnection plcConnection = plcDriver.getConnectionManager()
    .getConnection(/*ads...  */) {
    //...
  } catch (Exception ex) {
    //...        
  }

Two logs of « maybe » interest :

{
    "level": "TRACE",
    "threadName": "nioEventLoopGroup-2-1",
    "loggerName": "org.apache.plc4x.java.spi.Plc4xNettyWrapper",
    "message": "Failure while processing payload {} with handler {}",
    "throwable": {
        "className": "java.lang.NullPointerException",
        "message": "Cannot invoke \"org.apache.plc4x.java.ads.readwrite.AdsDataTypeTableEntry.getDataType()\" because \"adsDataTypeTableEntry\" is null",
        "stepArray": [{
                "className": "org.apache.plc4x.java.ads.protocol.AdsProtocolLogic",
                "methodName": "resolveDirectAdsTagForSymbolicNameFromDataType",
                "fileName": "AdsProtocolLogic.java",
                "lineNumber": 1779
            }
        ]
    }
}, {
    "level": "WARN",
    "threadName": "nioEventLoopGroup-2-1",
    "loggerName": "io.netty.channel.DefaultChannelPipeline",
    "message": "An exceptionCaught() event was fired, and it reached at the tail of the pipeline. It usually means the last handler in the pipeline did not handle the exception.",
    "throwable": {
        "className": "io.netty.handler.codec.DecoderException",
        "message": "java.lang.ClassCastException: class org.apache.plc4x.java.ads.readwrite.AdsReadResponse cannot be cast to class org.apache.plc4x.java.ads.readwrite.AmsTCPPacket (org.apache.plc4x.java.ads.readwrite.AdsReadResponse and org.apache.plc4x.java.ads.readwrite.AmsTCPPacket are in unnamed module of loader 'app')",
        "stepArray": [{
                "className": "io.netty.handler.codec.MessageToMessageDecoder",
                "methodName": "channelRead",
                "fileName": "MessageToMessageDecoder.java",
                "lineNumber": 98
            }
        ],
        "cause": {
            "className": "java.lang.ClassCastException",
            "message": "class org.apache.plc4x.java.ads.readwrite.AdsReadResponse cannot be cast to class org.apache.plc4x.java.ads.readwrite.AmsTCPPacket (org.apache.plc4x.java.ads.readwrite.AdsReadResponse and org.apache.plc4x.java.ads.readwrite.AmsTCPPacket are in unnamed module of loader 'app')",
            "stepArray": [{
                    "className": "org.apache.plc4x.java.spi.Plc4xNettyWrapper",
                    "methodName": "decode",
                    "fileName": "Plc4xNettyWrapper.java",
                    "lineNumber": 191
                }, {
                    "className": "io.netty.handler.codec.MessageToMessageCodec$2",
                    "methodName": "decode",
                    "fileName": "MessageToMessageCodec.java",
                    "lineNumber": 81
                }, {
                    "className": "io.netty.handler.codec.MessageToMessageDecoder",
                    "methodName": "channelRead",
                    "fileName": "MessageToMessageDecoder.java",
                    "lineNumber": 88
                }
            ]
        }
    }
}

Nothing of level « error ».

chrisdutz · 2024-09-27T21:55:10Z

The problem is, that I've found out how to load a symbol, not however hot to load one datatype definition of only one symbol.

So currently I would not disable the symbol table loading when relying on symbolic addresses.

I am working on implementing that... However too much to do, too little time to do so and no funding to make overtime work appealing.

fdupont-epsilia · 2024-09-30T11:31:57Z

No problem. We appreciate the work you do.

Should I transfer the connection problem I have (when load-symbol-and-data-type-tables is true) into another issue?
It does this into the production environment only. In my development environment, I reproduced the few variables I access, and it works fine.

chrisdutz · 2024-09-30T11:41:16Z

In your pcap I can see the PLC is sending data in high frequency to the client. I assume this is the symbol table (As that's read first) it seems that it doesn't even come to the data-type-table. I don't think that a new issue is required, as indeed we need to be able to dynamically resolve tags.

However the problem, is that the ADS driver has already become quite complex and my attempt to refactor it over the last week, was super complex. Here I tried to make the AdsTagHandler correctly resolve Ads Tags using the data in the symbol- and data-type-table. These changes already resulted in very complex refactoring.

I therefore started implementing a new ADS driver, that will use a different approach ... one that would fix my problem and automatically also this issue ... however, as stated in my recent blog post (https://github.com/chrisdutz/blog/blob/main/plc4x/throwing-the-towel.adoc) ... I'm no longer doing big stuff like that for free ... so I will be implementing this, but I'll only donate it back to PLC4X if I get enough donations for this.

chrisdutz · 2024-09-30T21:20:56Z

I do think today I had an idea, how I could probably work around your problem without rewriting the entire driver.
I could probably read the tables in chunks and always wait for them to be loaded ... I guess if I load all in 1MB chunks, this should probably work and it would not actually fix the issue you reported, but it would make the problem go away ;-)

fdupont-epsilia · 2024-10-03T15:02:59Z

Is it something you are planning to try in the following weeks? Any quick fix is welcome.
In our project, we are at a point of evaluating ours options with ADS, in the allowed timeframe.
It was a bummer when we discovered we got stuck at the connection process on the production environment.

chrisdutz · 2024-10-21T19:38:46Z

Sorry for the late response ... I was stuck in pre-conference work and just noticed that you had asked.

So I got some additional information and it seems as if the maximum size of a request read from an ADS device is considered to be 16384 bytes. This is not defined in the spec itself, but seems to be used throughout multiple implementations.
So my guess might have been correct, that we need to chunk large symbol- and data-type-tables.

I am actually currently working on a new implementation of the ADS driver, which is greatly reworked, supports this chunking and which should also support lazy resolution of types (so no need to load the entire tables on startup)

However, will this driver most probably not become part of the open-source project too soon. If you are still considering using PLC4X you should probably ping me directly and I could tell you how we could reach an agreement.

chrisdutz · 2024-11-02T13:49:33Z

Also turns out that even if in general it's possible to load blocks in chunks ... when reading the symbol table at least my PLC doesn't like this ... Only if I request the exact number of bytes the symbol table has, does the PLC not respond with an empty response.

fdupont-epsilia · 2024-11-12T15:24:25Z

Follow up on the connection problem.

I did some digging. Turns out the Beckhoff in production doesn't returns « basic types » (UDINT, USINT, REAL, BOOL, and more) in the dataTypeTable (ReservedIndexGroups.ADSIGRP_DATA_TYPE_TABLE_UPLOAD).
« java.lang.NullPointerException » is the result of not finding « UDINT » when doing SymbolicAdsTag("TwinCAT_SystemInfoVarList._AppInfo.OnlineChangeCnt", org.apache.plc4x.java.api.types.PlcValueType.UDINT

Doesn't seems to hit a size limit. Received everything declared in adsTableSizes.
symbolCount: 1640, symbolLength: 226832, dataTypeCount: 213, dataTypeLength: 175744, extraCount: 2000, extraLength: 11

I would appreciate if anyone has idea why those types are missing.

In preliminary test, adding them manually works. Could it break something?
Will continue testing and adding other types during the day.

Example of one, adding them at line 387 :

plc4x/plc4j/drivers/ads/src/main/java/org/apache/plc4x/java/ads/protocol/AdsProtocolLogic.java

Line 387 in 729860e

String dataTypeName = "UDINT";
if (!dataTypeTable.containsKey(dataTypeName))
{
  dataTypeTable.put(dataTypeName, new AdsDataTypeTableEntry(
    120,// entryLength,
    1,// version,
    0,// hashValue,
    0,// typeHashValue,
    4,// size,
    0,// offset,
    19,// dataType,
    4225,// flags,
    0,// arrayDimensions,
    0,// numChildren,
    dataTypeName,// dataTypeName,
    "",// simpleTypeName,
    "",// comment,
    Collections.emptyList(),// arrayInfo,
    Collections.emptyList(),// children,
    new byte[]{-107, 25, 7, 24, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 8, 2, 0, 15, 1, 68, 105, 115, 112, 108, 97, 121, 77, 105, 110, 86, 97, 108, 117, 101, 0, 48, 0, 15, 10, 68, 105, 115, 112, 108, 97, 121, 77, 97, 120, 86, 97, 108, 117, 101, 0, 35, 120, 102, 102, 102, 102, 102, 102, 102, 102, 0, 0, 0, 0})// rest)
  );
}

chrisdutz · 2024-11-12T15:28:15Z

Well... I guess using our enum first in order to parse the values and going to the table next... Our checking the table and using our enum if the key doesn't exist, could be an option.

fdupont-epsilia · 2024-11-12T15:58:07Z

Maybe would have to modify multiple parts. « executeRead » has problems also with missing « basic types ».

During SymbolicAdsTag("TwinCAT_SystemInfoVarList._AppInfo.OnlineChangeCnt", org.apache.plc4x.java.api.types.PlcValueType.UDINT
Trying to find « UDINT » happen at dataTypeTable.get(child.getDataTypeName())

plc4x/plc4j/drivers/ads/src/main/java/org/apache/plc4x/java/ads/protocol/AdsProtocolLogic.java

Line 1849 in 729860e

    
           AdsDataTypeTableEntry childAdsDataTypeTableEntry = dataTypeTable.get(child.getDataTypeName());

When reading a Tag of a type not in dataTypeTable, « REAL » or another, resolvedTags.get(adsTag) returns null.
The Tag is in resolvedTags, but something must be different in the object.

plc4x/plc4j/drivers/ads/src/main/java/org/apache/plc4x/java/ads/protocol/AdsProtocolLogic.java

Line 680 in 729860e

DirectAdsTag directAdsTag = resolvedTags.get(adsTag);

IsmoLeszczynski · 2024-11-14T12:01:18Z

I actually fixed some issues relating to this a while back when figuring out why TwinCat2 wouldn't work when loading the symbols. There were a couple of issues, one being that the driver requested some newer fields that only TwinCat3 supports when doing the initial connection, and another being that TwinCat2 devices don't return the base data types. I solved these by adding a version check during the initial connection to skip the newer parameters if connecting to a TwinCat2 device, and side loading the base data types in to the data type table based on type definitions I copied from a TwinCat3 result table.

I've been meaning to put these changes up for a PR, but ended up being swamped with other work recently. I'll see if I can get it up here by the end of the week.

fdupont-epsilia · 2024-11-14T13:36:04Z

It is a work in progress. What I have done is to load types from a json file. I don't know if this approach would make the cut into plc4x.
Beside base data type, string and array of all size are missing. Only solution is to add them manually.

if (new java.io.File(configuration.getdataTypeTableJSONFile()).exists())
{
	try {
		java.io.Reader reader = new java.io.FileReader(configuration.getdataTypeTableJSONFile());
		com.google.gson.Gson gson = new com.google.gson.Gson();
		java.lang.reflect.Type dataTypeTableType = new com.google.gson.reflect.TypeToken<Map<String, AdsDataTypeTableEntry>>() {}.getType();
		Map<String, AdsDataTypeTableEntry> dataTypeFromJson = gson.fromJson(reader, dataTypeTableType);

		for (Map.Entry<String, AdsDataTypeTableEntry> entry : dataTypeFromJson.entrySet()) {
			String key = entry.getKey();
			AdsDataTypeTableEntry value = entry.getValue();
			if (!dataTypeTable.containsKey(key)) {
				dataTypeTable.put(key, value);
			}
		}
	}
	catch (java.io.FileNotFoundException e) {
		LOGGER.debug("Error reading json file of data types: {}", configuration.getdataTypeTableJSONFile(), e);
	}
}

IsmoLeszczynski · 2024-11-14T13:56:53Z

Out of curiosity, what device are you using and TwinCAT version is it running? The fix I worked on only guarantees that the base types are included, my TC2 and TC3 devices provide any custom ones in the table otherwise.

fdupont-epsilia · 2024-11-14T18:52:29Z

The one not returning all type, the « Add Route Dialog » into « TwinCAT Config Mode » showing : TwinCat 3.1.4020
I don't have the hardware model.

chrisdutz · 2024-11-15T12:05:26Z

We've already got an enum with all the simple types: org.apache.plc4x.java.ads.readwrite.AdsDataType ... so I guess it would make sense to have some sort of "getOrDefault" for accessing the data-type table and to generate summy table entries in case the type is not found in the table.

But also we still need null-checks wherever the dataType is accessed.

fdupont-epsilia · 2024-11-15T13:19:55Z

So far, importing them from a file is good enough for me.

Yeah, depending on how many fields of AdsDataTypeTableEntry are truly used/important, should be not too hard to generate them.

Would require some work to parse missing arrays, such as « ARRAY [1..13] OF WORD ».

STRING, size is +1. But for some reason, « rest » has one less byte at STRING(10) compare to 7.

STRING(7)
"size": 8,
"rest": [-107, 25, 7, 24, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 7, 0, 0]

STRING(10)
"size": 11,
"rest": [-107, 25, 7, 24, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 10, 0]

I attached the types I needed to add so far: Plc4xAdsDataTypeTable.json

chrisdutz · 2024-11-15T13:41:30Z

Well as I said ... it's already there ... and I would love to avoid doing some file operation to load something ... I'm currently updating the driver to make use of this function:

    protected Optional<AdsDataTypeTableEntry> getDataTypeTableEntry(String name) {
        if(dataTypeTable.containsKey(name)) {
            return Optional.of(dataTypeTable.get(name));
        }
        try {
            AdsDataType adsDataType = AdsDataType.valueOf(name);
            // !It seems that the dataType value differs from system to system,
            // !However, we never really seem to use that value, so I would say it doesn't matter.
            return Optional.of(new AdsDataTypeTableEntry(
                128, 1, 0, 0, adsDataType.getNumBytes(), 0,
                adsDataType.getValue(), 0, 0, 0,
                adsDataType.name(), "", "",
                Collections.emptyList(), Collections.emptyList(), new byte[0]));
        } catch (IllegalArgumentException e) {
            return Optional.empty();
        }
    }

chrisdutz · 2024-11-15T13:42:17Z

So I had a look at what the primitive types contain on my system and saw that the structure was generally all the same.

chrisdutz · 2024-11-15T14:30:40Z

So just pushed a change, that should unify access to the dataTypeTable using the above method ... please give it a try. I tested it with my devices and it didn't cause any problems.

fdupont-epsilia · 2024-11-15T16:30:47Z

ARRAY [1..13] OF WORD, UDINT, USINT, REAL worked. Will test other base type later.
STRING(7), STRING(10) didn't work.

"throwable": {
    "className": "org.apache.plc4x.java.api.exceptions.PlcRuntimeException",
    "message": "Could not resolve data type STRING(7)",
    "stepArray": [{
        "className": "org.apache.plc4x.java.ads.protocol.AdsProtocolLogic",
        "methodName": "resolveDirectAdsTagForSymbolicNameFromDataType",
        "fileName": "AdsProtocolLogic.java",
        "lineNumber": 1886
      }, {
        "className": "org.apache.plc4x.java.ads.protocol.AdsProtocolLogic",
        "methodName": "getDirectAdsTagForSymbolicName",
        "fileName": "AdsProtocolLogic.java",
        "lineNumber": 1861
      }, {
        "className": "org.apache.plc4x.java.ads.protocol.AdsProtocolLogic",
        "methodName": "lambda$113",
        "fileName": "AdsProtocolLogic.java",
        "lineNumber": 1625
      }
....
{
        "className": "org.apache.plc4x.java.ads.protocol.AdsProtocolLogic",
        "methodName": "getDirectAddresses",
        "fileName": "AdsProtocolLogic.java",
        "lineNumber": 1626
      }, {
        "className": "org.apache.plc4x.java.ads.protocol.AdsProtocolLogic",
        "methodName": "read",
        "fileName": "AdsProtocolLogic.java",
        "lineNumber": 640
      }

chrisdutz · 2024-11-15T16:33:08Z

Hmmm ... yeah ... so the STRINGs were always a bit special ... guess next week will be another friday afternoon to spend 4h ... if you don't beat me to it.

fdupont-epsilia · 2024-11-15T21:37:44Z

Code below is working, I'm sure you will rework it in a much better form.

Took a while to figure out to get AdsDataType "CHAR" 0x1E (30) instead of "STRING" 0x20 (32).

    protected Optional<AdsDataTypeTableEntry> getDataTypeTableEntry(String name) {
        if(dataTypeTable.containsKey(name)) {
            return Optional.of(dataTypeTable.get(name));
        }
        try {
            AdsDataType adsDataType;
            int numBytes;
            if (name.startsWith("STRING(")) {
                adsDataType = AdsDataType.valueOf("CHAR");
                java.util.regex.Pattern pattern = java.util.regex.Pattern.compile("\\((\\d+)\\)");
                java.util.regex.Matcher matcher = pattern.matcher(name);
                if (matcher.find()) {
                    numBytes = Integer.parseInt(matcher.group(1)) + 1;
               }
                else {
                    return Optional.empty();
                }
            }
            else {
                adsDataType = AdsDataType.valueOf(name);
                numBytes = adsDataType.getNumBytes();
            }

            // !It seems that the dataType value differs from system to system,
            // !However, we never really seem to use that value, so I would say it doesn't matter.
            return Optional.of(new AdsDataTypeTableEntry(
                128, 1, 0, 0, numBytes, 0,
                adsDataType.getValue(), 0, 0, 0,
                name, "", "",
                Collections.emptyList(), Collections.emptyList(), new byte[0]));
        } catch (IllegalArgumentException e) {
            return Optional.empty();
        }
    }

After all, « ARRAY [1..13] OF WORD » is returned by the plc.
Will continue testing other basic type I am using. And validate if writing works.

chrisdutz · 2024-11-15T22:03:19Z

If you'd whip up a PR, I'd be happy to merge it .... this way you get credited for it? Possibly also worth adding the WSTRING thing that way too while you're at it.

fdupont-epsilia · 2024-11-19T19:41:44Z

I did a PR. Tested read and write, so far so good.

#1902

mrwhy-orig added the bug label May 30, 2024

splatch added driver-ads java Pull requests that update Java code ADS https://plc4x.apache.org/users/protocols/ads.html and removed driver-ads labels Jul 4, 2024

sruehl assigned chrisdutz Nov 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug]: ADS driver option load-symbol-and-data-type-tables=false doesn't resolve the variable #1626

[Bug]: ADS driver option load-symbol-and-data-type-tables=false doesn't resolve the variable #1626

mrwhy-orig commented May 30, 2024

chrisdutz commented May 30, 2024

mrwhy-orig commented Jun 3, 2024

chrisdutz commented Jun 14, 2024

fdupont-epsilia commented Sep 27, 2024

chrisdutz commented Sep 27, 2024

fdupont-epsilia commented Sep 30, 2024

chrisdutz commented Sep 30, 2024

chrisdutz commented Sep 30, 2024

fdupont-epsilia commented Oct 3, 2024

chrisdutz commented Oct 21, 2024

chrisdutz commented Nov 2, 2024

fdupont-epsilia commented Nov 12, 2024 •

edited

Loading

chrisdutz commented Nov 12, 2024

fdupont-epsilia commented Nov 12, 2024

IsmoLeszczynski commented Nov 14, 2024

fdupont-epsilia commented Nov 14, 2024 •

edited

Loading

IsmoLeszczynski commented Nov 14, 2024

fdupont-epsilia commented Nov 14, 2024

chrisdutz commented Nov 15, 2024

fdupont-epsilia commented Nov 15, 2024

chrisdutz commented Nov 15, 2024

chrisdutz commented Nov 15, 2024

chrisdutz commented Nov 15, 2024

fdupont-epsilia commented Nov 15, 2024

chrisdutz commented Nov 15, 2024

fdupont-epsilia commented Nov 15, 2024

chrisdutz commented Nov 15, 2024

fdupont-epsilia commented Nov 19, 2024

[Bug]: ADS driver option load-symbol-and-data-type-tables=false doesn't resolve the variable #1626

[Bug]: ADS driver option load-symbol-and-data-type-tables=false doesn't resolve the variable #1626

Comments

mrwhy-orig commented May 30, 2024

What happened?

Version

Programming Languages

Protocols

chrisdutz commented May 30, 2024

mrwhy-orig commented Jun 3, 2024

chrisdutz commented Jun 14, 2024

fdupont-epsilia commented Sep 27, 2024

chrisdutz commented Sep 27, 2024

fdupont-epsilia commented Sep 30, 2024

chrisdutz commented Sep 30, 2024

chrisdutz commented Sep 30, 2024

fdupont-epsilia commented Oct 3, 2024

chrisdutz commented Oct 21, 2024

chrisdutz commented Nov 2, 2024

fdupont-epsilia commented Nov 12, 2024 • edited Loading

chrisdutz commented Nov 12, 2024

fdupont-epsilia commented Nov 12, 2024

IsmoLeszczynski commented Nov 14, 2024

fdupont-epsilia commented Nov 14, 2024 • edited Loading

IsmoLeszczynski commented Nov 14, 2024

fdupont-epsilia commented Nov 14, 2024

chrisdutz commented Nov 15, 2024

fdupont-epsilia commented Nov 15, 2024

chrisdutz commented Nov 15, 2024

chrisdutz commented Nov 15, 2024

chrisdutz commented Nov 15, 2024

fdupont-epsilia commented Nov 15, 2024

chrisdutz commented Nov 15, 2024

fdupont-epsilia commented Nov 15, 2024

chrisdutz commented Nov 15, 2024

fdupont-epsilia commented Nov 19, 2024

fdupont-epsilia commented Nov 12, 2024 •

edited

Loading

fdupont-epsilia commented Nov 14, 2024 •

edited

Loading