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

Allow setting continuous states in Event Mode if reinit = false #1955

Merged
merged 5 commits into from
Sep 10, 2024

Conversation

t-sommer
Copy link
Collaborator

@t-sommer t-sommer commented Jun 6, 2024

fixes #1950

small changes to make it less likely that someone misinterprate it, maybe need to ad some symbols to continuous-time state to make it linkable?
Copy link
Collaborator

@KarlWernersson KarlWernersson left a comment

Choose a reason for hiding this comment

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

did some changes

Fixed tag for <<state,continuous-time states>>
Co-authored-by: Christian Bertsch <Christian.Bertsch@de.bosch.com>
@t-sommer
Copy link
Collaborator Author

Should setting (all) variables with <<causality>> = <<parameter>> really be allowed? I suggest to change

This function can be called for variables with <<causality>> = <<input>>, <<causality>> = <<parameter>>, <<variability>> = <<tunable>>, and <<state,continuous-time states>> with <<reinit>> = `false`.

to

This function can be called for variables with <<causality>> = <<input>>, <<variability>> = <<tunable>>, and <<state,continuous-time states>> with <<reinit>> = `false`.

@chrbertsch
Copy link
Collaborator

Torsten S: I want to change the second comma do "and" .
Masoud: I will look into this regarding clocks.

Copy link
Collaborator

@masoud-najafi masoud-najafi left a comment

Choose a reason for hiding this comment

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

May be I'm missing something here. Acccording to the definition of "reinit", if reinit=false, you are not allowed to reinitialize the state variable. So you cannot call fmi3SetXX. Is it a typo or I'm missing something?

@t-sommer
Copy link
Collaborator Author

t-sommer commented Aug 5, 2024

reinit:

If true, state may be reinitialized by the FMU in Event Mode.
If false, state will not be reinitialized.

I think this is correct, since an importer may only set a continuous state in Event Mode, if the FMU does not recalculate it.

@masoud-najafi
Copy link
Collaborator

OK, I got your point. Then let's first clarify the reinit attribute. Is it just for information purposes? What's useful for?
An FMU may go to the Event mode for several reasons. For a particular event, the FMU may reinitialize the state with reinit==true). For other events, the "importer" may reinitialize the same state. Let's take the BouncingBall example. The FMU is supposed to change the height and the velocity at zero-crossing event instants, so reinit for height and velocity is true. The importer may also be able to change these state variables at other events (other that zero-crossing events), for example if t>10, the ball change the velocity due to a sudden external wind.
By saying "reinit=true", should the importer be forbidden from updating the state? I'm not really in favor of this.

changed
<<causality>> = <<parameter>>, <<variability>> = <<tunable>>
to
<<causality>> = <<parameter>> and <<variability>> = <<tunable>>
@KarlWernersson
Copy link
Collaborator

@masoud-najafi I see your point, i don't have a strong requirement that you are forbidden to set continuous time state with reinit = true, its a larger chance that you screw things up though, but this is still an advanced feature so i am ok with letting all states be set. from the master.

@chrbertsch
Copy link
Collaborator

FMI Design Webmeeting:

Torsten: are clocks covered by this, or is this unrelated?
Karl: yes, it is unrelated.
Klaus: it is fine, but an explicit bullet list would be more clear.
Torsten: this would be geeneral issue at several places.
Torsten: this PR is about making things consistent. Masoud's suggestion goes beyond this. @masoud-najafi : if you want to proceed with this, please open a new PR
Klaus, Karl: this would not be a bugfix.

@chrbertsch chrbertsch merged commit 8ddf60b into main Sep 10, 2024
6 checks passed
@chrbertsch chrbertsch deleted the 1950-set-continuous-states-in-event-mode branch September 10, 2024 13:21
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.

Allow setFloat64() for continuous states in Event Mode if reinit = false
4 participants