search

tcunit / TcUnit

An unit testing framework for Beckhoff's TwinCAT 3
Other
258 stars 72 forks source link

implicit typecasting throws errors + solution #25

Closed Aliazzzz closed 5 years ago

Aliazzzz commented 5 years ago

Hi,

I found that certain runtimes (not all) produce the following code exceptions on two FB__Assert methods. Especially "SetTestFailed" and "FINDTestSuiteInstancePath" are affected.

The problem lies in DWORD_TO_ULINT( ADR(This^)).

The used typecaste conversion is not very clean. Mixing adresses and integer value's is always a bad idea. The compiler gives a warning (a hint! ) when the library is compiled: possibly cannot convert FB_Assert to a DWORD. An address is simply no DWORD as it depends on the compiler of the target platform if the solution is tolerant enough. TwinCAT runtimes are also affected by this phenomenon as Beckhoff uses mixed CPU architectures with underlying compilers by Codesys (ARM/x86/x64/PowerPC etc).

// Original code METHOD PRIVATE SetTestFailed VAR Counter : UINT; END_VAR FOR Counter := 1 TO GVL.AmountOfInitializedTestSuites BY 1 DO IF GVL.TestSuiteAssertAddresses[Counter] = DWORD_TO_ULINT( ADR(This^)) THEN GVL.TestSuiteAddresses[Counter].SetTestFailed(TestName := GVL.CurrentTestNameBeingCalled); END_IF END_FOR

EDIT; Another solution was suggested by a projectmember of mine; 

   IF GVL.TestSuiteAssertAddresses[Counter] = ANY_TO_ULINT( ADR(this^) ) THEN
       ...
   END_IF
sagatowski commented 5 years ago

Good catch! Maybe it would make even more sense to change the datatype of the TestSuiteAssertAddresses to PVOID?

Aliazzzz commented 5 years ago

Hi,

The first solution was that but this solution has a better compiler tolerance.

sagatowski commented 5 years ago

Hi,

The first solution was that but this solution has a better compiler tolerance.

Why? Is it because PVOID is not IEC61131-3 standard but something coming out of codesys?

Aliazzzz commented 5 years ago

Both are supported by CODESYS. But as far as I understand, the ANY_TO_ULINT Function conversion has a better compiler tolerance (wider compiler acceptance) then a POINTER TO PVOID oriented solution. We have seen this effect in the following scenario:

We had a working (no errors, no warnings) library for Raspberry Pi SL v3.5.14.20 but the same Library source would not compile (several typecast errors) for Control WIN x64 runtime.

So in Beckhoff language: A CX9xxx ARM device yields no errors, while the same lib used in PC Runtime (x64) yields several errors. The compiler for ARM devices reacts differently then the x64 compiler.

I bet that these effects will occur in Beckhoff based hardware, allthough they have not been reported yet I presume. This because TwinCAT uses the same Compilers

sagatowski commented 5 years ago

I just checked the code, and I could not find this issue in the code. There is no DWORD_TO_ULINT in the FB_Assert FB. I also checked to compile/check all objects in the code for all TwinCAT-architectures, in other words:

And no errors/warnings were raised.

Aliazzzz commented 5 years ago

Hi,

This is very very interesting, as it seems that this issue is caused by "translation" of TwinCAT to CODESYS...... Now I have to doublecheck the original TcUnit code also, like you already did to see what the methods looks like in TwinCAT.

Could you maybe c/p them and send them to me? This will save me a lot of time.

To be continued

Sent from my Smartphone

From: Jakob Sagatowski notifications@github.com Sent: Tuesday, June 4, 2019 9:10:49 AM To: tcunit/TcUnit Cc: Aliazzzz; Author Subject: Re: [tcunit/TcUnit] implicit typecasting throws errors + solution (#25)

I just checked the code, and I could not find this issue in the code. There is no DWORD_TO_ULINT in the FB_Assert FB. I also checked to compile/check all objects in the code for all TwinCAT-architectures, in other words:

And no errors/warnings were raised.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/tcunit/TcUnit/issues/25?email_source=notifications&email_token=AH4C24CLGNDDHM4QPBQ32BLPYYIPTA5CNFSM4HSHXXK2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODW3UMJA#issuecomment-498550308, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AH4C24AST5BMZVZ4VN6H4FTPYYIPTANCNFSM4HSHXXKQ.

sagatowski commented 5 years ago

How is the translation done? Does it exist a function in the vanilla codesys to translate a TwinCAT library into a codesys library? I thought they were the same?

I think it makes more sense that you install TwinCAT on your engineering machine and do a GIT-clone of the software, as it's likely you will have similar issues with other methods.

sagatowski commented 5 years ago

SetTestfailed:

FOR Counter := 1 TO GVL.AmountOfInitializedTestSuites BY 1 DO
    IF GVL.TestSuiteAssertAddresses[Counter] = ADR(THIS^) THEN
        GVL.TestSuiteAddresses[Counter].SetTestFailed(TestName := GVL.CurrentTestNameBeingCalled);
    END_IF
END_FOR

FindTestSuiteInstancePath

FOR Counter := 1 TO GVL.AmountOfInitializedTestSuites BY 1 DO
    IF GVL.TestSuiteAssertAddresses[Counter] = ADR(THIS^) THEN
        FindTestSuiteInstancePath := GVL.TestSuiteAddresses[Counter].GetInstancePath();
    END_IF
END_FOR
Aliazzzz commented 5 years ago

Hi,

I had installed TwinCAT and exported the biggest part of the code via PLCOpen xml. Sadlly I uninstalled it several days ago because of disk space. That's why I asked you for a c/p of the TwinCAT methods. I will set up a more permanent virtual machine for TwinCAT shortly.

Regards

Sent from my Smartphone

From: Jakob Sagatowski notifications@github.com Sent: Tuesday, June 4, 2019 2:15:33 PM To: tcunit/TcUnit Cc: Aliazzzz; Author Subject: Re: [tcunit/TcUnit] implicit typecasting throws errors + solution (#25)

How is the translation done? Does it exist a function in the vanilla codesys to translate a TwinCAT library into a codesys library? I thought they were the same?

I think it makes more sense that you install TwinCAT on your engineering machine and do a GIT-clone of the software, as it's likely you will have similar issues with other methods.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/tcunit/TcUnit/issues/25?email_source=notifications&email_token=AH4C24AXNBFY2U5DYGZLOP3PYZMGLA5CNFSM4HSHXXK2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODW4L2FA#issuecomment-498646292, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AH4C24DU3ASLXW2MPNDUE5DPYZMGLANCNFSM4HSHXXKQ.

sagatowski commented 5 years ago

I see. I've never tried the export to plcopen-functionality in TwinCAT. According to the Beckhoff documentation: https://infosys.beckhoff.com/english.php?content=../content/1033/tcplclibsystem/html/tcplclibsys_pvoid.htm&id=7908544662706759177

It doesn't specify that this is an extension of the IEC61131-3 standard, though the documentation might not be fully correct. I assume the reason the conversion to ULINT for PLCOpen is to be fully IEC-compliant.

I've included the code in the previous comment. I'll close this ticket.

Aliazzzz commented 5 years ago

Hi Jakob,

Just for sake of argument and a recap:

Observe the following code;

GVL.TestSuiteAssertAddresses[Counter] = ADR(THIS^)

In TwinCAT you made an implicit typecast in this assignment (did you do this on purpose?). In Codesys I had make this typecast explicit, because of compiler errors caused by ADR(THIS^) which is compared, if I remember correctly, an Interface (an array of interfaces). We, the CfUnit team, came up with a solution which is simple and multi target (x86, x64, ARM) tolerant;

GVL.TestSuiteAssertAddresses[Counter] = ANY_TO_ULINT(ADR(THIS^));

I hope this remark is resolved in a suffiencient manner

PS. I am sorry to have had you confused over this, as I assumed the explicit typecasting was from your basecode, but I had isneserted it myself some weeks ago and forget allready.

sagatowski commented 5 years ago

Hi Aliazz! Thank you for looking deeper into this. It surely seems that TwinCAT handles this differently than the codesys vanilla compiler, which I feel that I have to get now. I'm storing the address in a PVOID, and then the compiler makes sure to convert the PVOID to a 32 or 64-bit type for me. It's the same with the ADR-operator, which will return the result in a PVOID (which in turn is an UXINT, which is converted to ULINT or UDINT in compile-time). This is the reason the TwinCAT-compiler doesn't complain (no errors or warnings), as it simply will make sure to use the correct type depending on which target architecture that is selected. I would thus say that for the sake for only using this framework for TwinCAT, this is fine.

Now, given the fact that this code is used also for CfUnit (which obviously was not foreseen when I created the framework), this does not work anymore why I need to change this.

What I'll do in the future is to make sure to use as few TwinCAT-specifics as possible so that it is as easy as possible to use this for codesys. I'll also install codesys, and use CfUnit to understand the differences better.

I'll try the ANY_TO_ULINT variant, verify that it works with TwinCAT (for different architectures), and then commit the new version.

Thanks for putting your time/effort into this, it's highly appreciated. I'll re-open the issue until we have something that both of us are happy with.

Aliazzzz commented 5 years ago

Thank you for making sure to use as few TwinCAT-specifics as possible so that it is as easy as possible to use this for codesys!

Thumbs up!

sagatowski commented 5 years ago

Solved by removing requirement to have an instance of FB_Assert in each test-suite and instead have that functionality it in FB_TestSuite. Implemented in commit 4339ef54507668830299c85d60ac7c23f18520a1.