Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Bug]: PLC4X - 0.11.0 - Issue writing value to a Beckhoff PLC using ads driver #1156

Closed
2 of 16 tasks
patrickboisclair opened this issue Oct 17, 2023 · 33 comments
Closed
2 of 16 tasks

[Bug]: PLC4X - 0.11.0 - Issue writing value to a Beckhoff PLC using ads driver #1156

patrickboisclair opened this issue Oct 17, 2023 · 33 comments
Assignees
@chrisdutz
Labels
bug

Comments

@patrickboisclair
Copy link

What happened?

Hi all,

I upgraded from version 0.9.1 to 0.11.0 and I cannot write to my Beckhoff PLC anymore.

When I write 1 to a DINT, 16777216 is stored.

Is it a BIG-ENDIAN vs LITTLE-ENDIAN issue ?
Is it possible to specify the “endianness” of the PLC ?

According to the TWINCat doc, it seems they use LITTLE-ENDIAN.

The PLC type is a "CX8190 / Standard

Attached is 3 REALLY simple Java application.
Write "1" to a DINT and read it back.

First with version 0.9.1 -> WORKS
Second with version 0.10.0 -> DOESN'T WORK (same issue) Third with version 0.11.0 -> DOESN'T WORK (same issue)

I did upgrade our "big" project from 0.9.1 to 0.11.0 yesterday, so I did not notice the issue with 0.10.0... my bad on this.

Hope it can help, bring some light as what changed from 0.9.1 to 0.10.0 and 0.11.0

I will work on getting a Wireshark log during the day and will do more testing.
plc4xTests.zip

Version

v0.11.0

Programming Languages

  • plc4j
  • plc4go
  • plc4c
  • plc4net

Protocols

  • AB-Ethernet
  • ADS /AMS
  • BACnet/IP
  • CANopen
  • DeltaV
  • DF1
  • EtherNet/IP
  • Firmata
  • KNXnet/IP
  • Modbus
  • OPC-UA
  • S7
@splatch
Copy link
Contributor

splatch commented Oct 17, 2023 •
edited

@patrickboisclair can you check 0.11 with different syntax of tag - i.e. MAIN.Valeur:DINT/MAIN.ValeurDINT:DINT? Currently your tag does not specify type, I believe it should be resolved through symbol lookup, its just to ensure its not a typo. Sorry for asking about extra verification step.
While there were changes in how tags are handled between versions you tested, tag syntax remains similar.

@patrickboisclair
Copy link
Author

I'll try, think I get "invalid tag", but I will test it out right now

@patrickboisclair
Copy link
Author

I get the following exception:
xception in thread "main" org.apache.plc4x.java.api.exceptions.PlcInvalidTagException: Couldn't resolve symbolic address: MAIN.ValeurDINT:DINT invalid
at org.apache.plc4x.java.ads.protocol.AdsProtocolLogic.getDirectAdsTagForSymbolicName(AdsProtocolLogic.java:1689)
at org.apache.plc4x.java.ads.protocol.AdsProtocolLogic.lambda$110(AdsProtocolLogic.java:1467)
at java.base/java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:178)
at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1625)
at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:921)
at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:682)
at org.apache.plc4x.java.ads.protocol.AdsProtocolLogic.getDirectAddresses(AdsProtocolLogic.java:1468)
at org.apache.plc4x.java.ads.protocol.AdsProtocolLogic.write(AdsProtocolLogic.java:935)
at org.apache.plc4x.java.spi.connection.AbstractPlcConnection.write(AbstractPlcConnection.java:188)
at org.apache.plc4x.java.spi.messages.DefaultPlcWriteRequest.execute(DefaultPlcWriteRequest.java:58)
at com.noovelia.TestBeckhoff.test(TestBeckhoff.java:27)
at com.noovelia.Main.main(Main.java:10)

@patrickboisclair
Copy link
Author

The tag in the PLC is named ValeurDINT

@splatch
Copy link
Contributor

splatch commented Oct 17, 2023

Yeah, I see now why you got it without :DINT. There was a change in how symbolic tags are handled, they got smarter from 0.8 release where, beyond symbol name, its type had to be specified.

Please check and/or adjust following code:

try (PlcConnection plcConnection = new DefaultPlcDriverManager().getConnection(connectionString)) {
    PlcBrowseResponse response = plcConnection.browseRequestBuilder().addQuery("symbols", "")
        .build().execute().get();
    for (PlcBrowseItem symbol : response.getValues("symbols")) {
        System.out.println(symbol.getName() + " " + symbol.getTag() + " " + symbol.getOptions());
    }
}

It should print you all tags which got detected when connection was established. I don't think any more that discovery is a problem, its rather question of type mapping and/or eventual encoding.

@patrickboisclair
Copy link
Author

patrickboisclair commented Oct 17, 2023 •
edited by splatch

Yep, it works:

Constants.bMulticoreSupport SymbolicAdsTag{symbolicAddress='Constants.bMulticoreSupport'} {group-id=16448, offset=385430, size-in-bytes=1, comment=""}
MAIN.E_reset SymbolicAdsTag{symbolicAddress='MAIN.E_reset'} {group-id=61472, offset=385293, size-in-bytes=1, comment=""}
 Fix Patrick Boisclair  SymbolicAdsTag{symbolicAddress='MAIN.I_Lum_1'} {group-id=16448, offset=385291, size-in-bytes=1, comment=" Fix Patrick Boisclair "}
MAIN.Fb_Rand SymbolicAdsTag{symbolicAddress='MAIN.Fb_Rand'} {group-id=16448, offset=384728, size-in-bytes=544, comment=""}
 Does the target support multiple cores? SymbolicAdsTag{symbolicAddress='Constants.CompilerVersionNumeric'} {group-id=16448, offset=385436, size-in-bytes=4, comment=" Does the target support multiple cores?"}
Timer 3000ms SymbolicAdsTag{symbolicAddress='MAIN.T_Rand'} {group-id=16448, offset=384720, size-in-bytes=4, comment="Timer 3000ms"}
TwinCAT_SystemInfoVarList._TaskPouOid_PlcTask SymbolicAdsTag{symbolicAddress='TwinCAT_SystemInfoVarList._TaskPouOid_PlcTask'} {group-id=16448, offset=385824, size-in-bytes=4, comment=""}
MAIN.RAND SymbolicAdsTag{symbolicAddress='MAIN.RAND'} {group-id=16448, offset=385280, size-in-bytes=8, comment=""}
Global_Version.stLibVersion_Tc2_Utilities SymbolicAdsTag{symbolicAddress='Global_Version.stLibVersion_Tc2_Utilities'} {group-id=16448, offset=384036, size-in-bytes=36, comment=""}
TwinCAT_SystemInfoVarList._TaskOid_PlcTask SymbolicAdsTag{symbolicAddress='TwinCAT_SystemInfoVarList._TaskOid_PlcTask'} {group-id=16448, offset=385828, size-in-bytes=4, comment=""}
 Signal d'arr?t de la navette de transition si navette r?frig?r?e est pr?sente dans zone commune SymbolicAdsTag{symbolicAddress='Variables_Globales_GOX.bARRETZONECOMMNAVTRANSIT'} {group-id=16448, offset=385301, size-in-bytes=1, comment=" Signal d'arr?t de la navette de transition si navette r?frig?r?e est pr?sente dans zone commune"}
 Does the target support multiple cores? SymbolicAdsTag{symbolicAddress='Constants.bFPUSupport'} {group-id=16448, offset=385425, size-in-bytes=1, comment=" Does the target support multiple cores?"}
Global_Version.stLibVersion_Tc3_Module SymbolicAdsTag{symbolicAddress='Global_Version.stLibVersion_Tc3_Module'} {group-id=16448, offset=384108, size-in-bytes=36, comment=""}
Entr?e autoriant la g?n?ration d'un nombre al?atoire par Random3 SymbolicAdsTag{symbolicAddress='MAIN.E_Rand'} {group-id=61472, offset=385288, size-in-bytes=1, comment="Entr?e autoriant la g?n?ration d'un nombre al?atoire par Random3"}
 Does the target support multiple cores? SymbolicAdsTag{symbolicAddress='Constants.CompilerVersion'} {group-id=16448, offset=385416, size-in-bytes=8, comment=" Does the target support multiple cores?"}
MAIN.C_Lum SymbolicAdsTag{symbolicAddress='MAIN.C_Lum'} {group-id=16448, offset=385296, size-in-bytes=4, comment=""}
 Fix Patrick Boisclair  SymbolicAdsTag{symbolicAddress='MAIN.I_reset'} {group-id=16448, offset=385294, size-in-bytes=1, comment=" Fix Patrick Boisclair "}
MAIN.S_Lum_2 SymbolicAdsTag{symbolicAddress='MAIN.S_Lum_2'} {group-id=61488, offset=385292, size-in-bytes=1, comment=""}
 Does the target support multiple cores? SymbolicAdsTag{symbolicAddress='Constants.bLittleEndian'} {group-id=16448, offset=385303, size-in-bytes=1, comment=" Does the target support multiple cores?"}
 Does the target support multiple cores? SymbolicAdsTag{symbolicAddress='Constants.RuntimeVersion'} {group-id=16448, offset=385408, size-in-bytes=8, comment=" Does the target support multiple cores?"}
 Structure qui contient les donn?es de d?placement de la navette r?frig?r?e SymbolicAdsTag{symbolicAddress='Variables_Globales_GOX.stCOMMANDENAVETREFRIG'} {group-id=16448, offset=385360, size-in-bytes=48, comment=" Structure qui contient les donn?es de d?placement de la navette r?frig?r?e"}
Global_Version.stLibVersion_Tc2_Standard SymbolicAdsTag{symbolicAddress='Global_Version.stLibVersion_Tc2_Standard'} {group-id=16448, offset=384000, size-in-bytes=36, comment=""}
Global_Version.stLibVersion_Tc2_System SymbolicAdsTag{symbolicAddress='Global_Version.stLibVersion_Tc2_System'} {group-id=16448, offset=384072, size-in-bytes=36, comment=""}
MAIN.ValeurDINT SymbolicAdsTag{symbolicAddress='MAIN.ValeurDINT'} {group-id=16448, offset=394112, size-in-bytes=4, comment=""}
Nombre al?atoire entre 1-5000 ? toutes les 1s SymbolicAdsTag{symbolicAddress='MAIN.Rand_2'} {group-id=16448, offset=385272, size-in-bytes=4, comment="Nombre al?atoire entre 1-5000 ? toutes les 1s"}
Nombre al?atoire entre 1-5000 ? toutes les 10ms SymbolicAdsTag{symbolicAddress='MAIN.Rand_1'} {group-id=16448, offset=384724, size-in-bytes=4, comment="Nombre al?atoire entre 1-5000 ? toutes les 10ms"}
Nombre al?atoire entre 0-60 SymbolicAdsTag{symbolicAddress='MAIN.Rand_3'} {group-id=16448, offset=385276, size-in-bytes=4, comment="Nombre al?atoire entre 0-60"}
 Does the target support multiple cores? SymbolicAdsTag{symbolicAddress='Constants.nPackMode'} {group-id=16448, offset=385428, size-in-bytes=2, comment=" Does the target support multiple cores?"}
 Fix Patrick Boisclair  SymbolicAdsTag{symbolicAddress='MAIN.I_test'} {group-id=16448, offset=385300, size-in-bytes=1, comment=" Fix Patrick Boisclair "}
 Fix Patrick Boisclair  SymbolicAdsTag{symbolicAddress='MAIN.I_Rand'} {group-id=16448, offset=385289, size-in-bytes=1, comment=" Fix Patrick Boisclair "}
Timer 10ms SymbolicAdsTag{symbolicAddress='MAIN.T_2'} {group-id=16448, offset=384712, size-in-bytes=4, comment="Timer 10ms"}
Timer 1000ms SymbolicAdsTag{symbolicAddress='MAIN.T_3'} {group-id=16448, offset=384716, size-in-bytes=4, comment="Timer 1000ms"}
La SymbolicAdsTag{symbolicAddress='MAIN.T_1'} {group-id=16448, offset=384708, size-in-bytes=4, comment="La"}
TwinCAT_SystemInfoVarList._AppInfo SymbolicAdsTag{symbolicAddress='TwinCAT_SystemInfoVarList._AppInfo'} {group-id=16448, offset=385440, size-in-bytes=256, comment=""}
MAIN.E_Lum_1 SymbolicAdsTag{symbolicAddress='MAIN.E_Lum_1'} {group-id=61472, offset=385290, size-in-bytes=1, comment=""}
 Does the target support multiple cores? SymbolicAdsTag{symbolicAddress='Constants.bSimulationMode'} {group-id=16448, offset=385424, size-in-bytes=1, comment=" Does the target support multiple cores?"}
TwinCAT_SystemInfoVarList.__PlcTask SymbolicAdsTag{symbolicAddress='TwinCAT_SystemInfoVarList.__PlcTask'} {group-id=16448, offset=385832, size-in-bytes=88, comment=""}
MAIN.Patrick SymbolicAdsTag{symbolicAddress='MAIN.Patrick'} {group-id=16448, offset=462576, size-in-bytes=11, comment=""}
 Structure qui contient les donn?es de d?placement de la navette de transition SymbolicAdsTag{symbolicAddress='Variables_Globales_GOX.stCOMMANDENAVETTRANSIT'} {group-id=16448, offset=385312, size-in-bytes=48, comment=" Structure qui contient les donn?es de d?placement de la navette de transition"}
MAIN.E_test SymbolicAdsTag{symbolicAddress='MAIN.E_test'} {group-id=61472, offset=385295, size-in-bytes=1, comment=""}
TwinCAT_SystemInfoVarList._TaskInfo SymbolicAdsTag{symbolicAddress='TwinCAT_SystemInfoVarList._TaskInfo'} {group-id=16448, offset=385696, size-in-bytes=128, comment=""}
 Does the target support multiple cores? SymbolicAdsTag{symbolicAddress='Constants.nRegisterSize'} {group-id=16448, offset=385426, size-in-bytes=2, comment=" Does the target support multiple cores?"}
 Does the target support multiple cores? SymbolicAdsTag{symbolicAddress='Constants.RuntimeVersionNumeric'} {group-id=16448, offset=385432, size-in-bytes=4, comment=" Does the target support multiple cores?"}

@patrickboisclair
Copy link
Author

I did notice tho, that since 0.11.0, in order to write a value, we need to use:
writeBuilder.addTagAddress("ValueDINT", "MAIN.ValeurDINT", new PlcDINT(value));

Prior to 0.11.0:
writeBuilder.addTagAddress("ValueDINT", "MAIN.ValeurDINT", value);

If we dont use a PlcXXX type, it write Null

@patrickboisclair
Copy link
Author

Looks like when writing a tag, without specifying a PLCxxx datatype, in the class "PlcValueHandler.java":
public static PlcValue of(PlcTag tag, Object[] values) {
if (values.length == 1) {
Object value = values[0];
if(tag.getPlcValueType() == null) {
// TODO: This is a hacky shortcut ..
if(value instanceof PlcValue) {
return (PlcValue) value;
}
return new PlcNull();
}

the if (tag.getPlcValueType()... always return null

@chrisdutz
Copy link
Contributor

So if you do this it works:

writeBuilder.addTagAddress("ValueDINT", "MAIN.ValeurDINT", new PlcDINT(value));
And if you do this, it fails?

writeBuilder.addTagAddress("ValueDINT", "MAIN.ValeurDINT", value);

@patrickboisclair
Copy link
Author

patrickboisclair commented Oct 20, 2023 •
edited

Well works, let me explain:

writeBuilder.addTagAddress("ValueDINT", "MAIN.ValeurDINT", new PlcDINT(value));
Passing a "new PlcXXX(value)" write a value to the PLC, but not the good one as mentionned above.
Example: writing 1 writes 16777216 with this syntax.

writeBuilder.addTagAddress("ValueDINT", "MAIN.ValeurDINT", value);
Passing a value without a "PlcTYPE" parameter, always writes null to the Plc

Checking the code in PlcValueHandler.java:

public static PlcValue of(PlcTag tag, Object[] values) {
if (values.length == 1) {
    Object value = values[0];
    if(tag.getPlcValueType() == null) {
    // TODO: This is a hacky shortcut ..
   if(value instanceof PlcValue) {
         return (PlcValue) value;
   }
   return new PlcNull();
}

Passing the "new PlcDINT(value)" is "fixed" by the "hacky shortcut, mentionned in the comment of the code.
Its becausse:
the if (tag.getPlcValueType()... always return null

Hope it make senses. Let me know, if you need more infos / tests on this.

@chrisdutz
Copy link
Contributor

Well I guess a real fix would be to automatically create a PLCValue of a type that certainly can fit the value and to add an additional check on "isUInt()" before accessing it. Problem is that while assembling the request with "addTagAddress" the tag is not yet resolved and we therefore don't know the type yet.

@chrisdutz
Copy link
Contributor

I'll try to whip up a fix for this asap ...

@patrickboisclair
Copy link
Author

Hi Chris,
Thank you you very much. When the fix will be available Im willing to test it.
Sorry for the delay, had some busy days.

@chrisdutz
Copy link
Contributor

Well ... if the world would let me have a bit control over my free time I could say something about that ... but for now I'll just say: It's done when it's done :-/

@chrisdutz
Copy link
Contributor

Just as 2023 was a pretty turbulent year ... is this still an issue?

@patrickboisclair
Copy link
Author

patrickboisclair commented Feb 6, 2024 via email

@chrisdutz
Copy link
Contributor

chrisdutz commented Feb 6, 2024 •
edited

In my testsuite, that I'm running against my ADS device I'm reading each supported datatype, then write it back, then do 100 reads of all fields at the same time, but with random order ... so writing generally should work.

Have you tried with the latest SNAPSHOT version? A lot of work has been put into that.

@patrickboisclair
Copy link
Author

patrickboisclair commented Feb 6, 2024 via email

@chrisdutz
Copy link
Contributor

And ideally, if it doesn't work ... please create a Wireshark recording of the interaction and attach it to this issue.

@patrickboisclair
Copy link
Author

pc4x_11_WireShark.zip

Hi Chris,
Attached is the WireShark results of my test. I ran the test twice,

Here is the Java code:
`
final DefaultPlcDriverManager driver = new DefaultPlcDriverManager();
final PlcConnection plcConnection = driver.getConnection("ads:tcp://10.10.11.77?sourceAmsNetId=172.29.48.1.1.1&sourceAmsPort=34&targetAmsNetId=5.62.206.200.1.1&targetAmsPort=851");

    final Integer expectedValue = 1;

    // Write value to plc
    PlcWriteRequest.Builder writeBuilder = plcConnection.writeRequestBuilder();
    writeBuilder.addTagAddress("ValueDINT", "MAIN.ValeurDINT", new PlcDINT(expectedValue)); // With PLC4X 0.11.0, we need to pass a PlcXXX object, otherwise null is used ?

    PlcWriteRequest writeRequest = writeBuilder.build();
    writeRequest.execute().get();

    // Read the written value
    PlcReadRequest.Builder reaBuilder = plcConnection.readRequestBuilder();
    reaBuilder.addTagAddress("ValueDINT", "MAIN.ValeurDINT");

    PlcReadRequest readRequest = reaBuilder.build();
    PlcReadResponse response = readRequest.execute().get();

    Integer result = response.getInteger("ValueDINT");

    System.out.println("Read value: " + result.toString());

`

As you can see, I write 1 and read it back, and of course I expect 1 as the result, but I get :
Read value: 16777216

If you need anything else, pls let me know.

Best regards !

@patrickboisclair
Copy link
Author

patrickboisclair commented Feb 8, 2024 •
edited

PLC4X11_WrittenValue_TwinCAT

Oh and here is a screenshot of what is currently in the PLC using TwinCAT

NOTE: Using PLC4X 0.9.1 works as expected

@chrisdutz
Copy link
Contributor

Ok ... I can confirm that I am getting an odd value back ... but the funny thing is, the write request and the read response are absolutely correct (The screenshots are from your recording)
ads-write
ads-read
I'll look into this.

@chrisdutz
Copy link
Contributor

Ok ... so I think I have something here ;-) ... so if I enter 16777216 into my calculator and switch to hex, I get 0x01000000 ... so this is an endianess problem ... I really wonder why this has never been seen before cause I'm running an excessive testsuite on the ADS ... I would totally freak out, if the value I made up is accidentally an endianess-palindrome.

@hutcheb
Copy link
Contributor

hutcheb commented Feb 8, 2024

If you are write a value to the PLC and then reading that value back, I suspect you don't actually test endianess, just the fact that you can write the data and read the same data back.

@hutcheb
Copy link
Contributor

hutcheb commented Feb 8, 2024

What confuses me is that it would seem that the endianess of a Twincat system seems to be dependent on the architecture it is running on?

@chrisdutz
Copy link
Contributor

I'm just completely confused ... so in my tests I'm reading a value pre-defined in the PLC program and it's reading -242442424 = which is in hex: 0xF18C9F48 which I can see on the wire as 0x489F8CF1 ... I'm getting it in the exact same value and when I'm sending it back, I see the same values being sent to the PLC and the test then reads again and it's not changed.

Here however 1 is encoded as 0x00000001 which is not correct, it should be 0x01000000 ... I'll debug through this.

@hutcheb
Copy link
Contributor

hutcheb commented Feb 8, 2024

What value is it when you view it in Twincat though?

@chrisdutz
Copy link
Contributor

Well the thing is ... I was missing a "LittleEndian" switch in the parsePlcValue method. Now with this, the "writing of 1" works and still my big-ass test seems to work.

I've pushed a fix, so please pull and rebuild and try the SNAPSHOT. I'll continue trying to find out why it's not having any impact on my big test.

@chrisdutz
Copy link
Contributor

With my tests I have the expected values hard-coded in my PLC program and I read everything separately in a single element read and compare it with the expected value. If that matches, I write the same value back. If this has worked for all types, I then read all elements in a multi-element read and compare the results with the expected ones again. I am a bit frustrated, that this hasn't been revealed before ... I mean ... writing a single 1 and then reading it back should be a super-basic test.

@splatch
Copy link
Contributor

splatch commented Feb 8, 2024 •
edited

I've got today an report of broken writes within AMSADS binding I am doing for openHAB. The target device is an ancient BX9000 with TwinCAT2. Binding is already on plc4x 0.11, with all discovery stuff we had prior release. I am cleaning my desk to check this report, so your findings are definitely interesting. I'll be able to check if I get a proper error answers, cause TC2 seem to have its own quirks.

@chrisdutz
Copy link
Contributor

I can imagine that with TC2 a lot of things are a lot different ... so if you find out things are not compatible with the TC3 driver, that I am building, we should probably have multiple variants.

@patrickboisclair
Copy link
Author

Hi Chris,
I tried with the 0.12.0-SNAPSHOT and Im happy to say that it seems to work !

Thank you very much for your help, really appreciated !

@chrisdutz
Copy link
Contributor

Always happy to help ... and thanks for reporting and helping us find this issue.

And if you want to help any further ... the best thing you could do, would be to publicly talk about PLC4X ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@chrisdutz chrisdutz

Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@splatch @chrisdutz @hutcheb @patrickboisclair