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.

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

Add new integral function block for determining approximate integral over time. #648

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

franz-hoepfinger-4diac
Copy link

No description provided.

Copy link

Test Results

   110 files  ±0     110 suites  ±0   48s ⏱️ ±0s
27 531 tests  - 2  27 531 ✅  - 2  0 💤 ±0  0 ❌ ±0 
27 532 runs   - 2  27 532 ✅  - 2  0 💤 ±0  0 ❌ ±0 

Results for commit 054e1c9. ± Comparison against base commit b2b0d82.

This pull request removes 2 tests.
org.eclipse.fordiac.ide.structuredtextfunctioneditor.tests.STFunctionValidatorPartialAccessTest ‑ testValidPartialAccessOnArraySubscript
org.eclipse.fordiac.ide.structuredtextfunctioneditor.ui.tests.STFunctionMultiQuickfixTest ‑ fixUnnecessaryConversionNested

</EventOutputs>
<InputVars>
<VarDeclaration Name="IN" Type="REAL" Comment="Input value" InitialValue="0.0"/>
<VarDeclaration Name="TM" Type="DINT" Comment="Time since last call in msec" InitialValue="0"/>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer TM to have type TIME to better indicate the meaning of this value. But I fully understand the implications this has on the integration as soon as people start to use values in nanoseconds...

IF (1.0E38 - IN * DINT_TO_REAL(TM) < OUT OR -1.0E38 - IN * DINT_TO_REAL(TM) > OUT) THEN
OVERFLOW := TRUE;
ELSIF (TM > 0) THEN // Time Difference since last call must be positive.
OUT := OUT + IN * DINT_TO_REAL(TM) / 1000.0;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When testing this in the Interpreter I got the following case:

  • send RESET
  • set IN to 9.0E9, TM to 1 send REQ; OUT goes to 8999999.0
  • set the input to 1.0, send REQ; OUT does not change
  • set the input to -1.0, send REQ; OUT also does not change

this is caused by the variable resolution of the REAL datatype - a warning for the case where the integration does not work (i.e. OUT does not change with IN <> 0) could be nice-to-have

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are totally right:
our colleges at CODESYS have same issue with same Function.
this comes from the internal representation of REAL.

maybe we should also make a LREAL Version, as well a Fixed-comma Version in DINT.
i will think about it, and in the MEantime make the warning as written above.

@MartinMelikMerkumians
Copy link
Member

MartinMelikMerkumians commented Oct 23, 2024

Seeing the comments and the implications for the correct operation of this Function Block, I don't think this should be written as a SFB in ST at all, but as a SiFB, which can do all the relevant checking and error signalling on the system level.
TM can then be a TIME, which a check if TM's value is lower than current clock resolution, and IN and OUT could be ANY_NUMs, eliminating the need to define multiple, functional identical versions of this FB

@diplfranzhoepfinger
Copy link

@MartinMelikMerkumians i totally agree to you.
in my opinion we should make a First Version with severe Warnings about REAL.

and then step a big Moment back, go deep, take time, and make the improved Version with ANY_NUM,
wich will imply even more Documentation and more testing.

agree ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants