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

Run fmi2SetXXX function to initialize FMU start values #778

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

davidhjp01
Copy link
Contributor

@davidhjp01 davidhjp01 commented Nov 16, 2024

According to FMI 2.0.4 spec (Section 4.2.4), start values are read during instantiated state via fmi2SetXXX . But libcosim does not run these functions in instantiated state and this makes some of FMUs exported (e.g. Simulink) not initializing values accordingly.

For example see below cosim_demo_app that runs an integrator whose initial condition is set via parameter Parameter.Integrator_x0:

image
image

The initial condition is not properly set via the parameter (i.e. not starting from 10).
On the other hand, fmpy properly initialize this value when set in the simulation setting:
image

To fix this, this PR added an additional function initialize_start_values() that runs before simulator::setup() in fixed_step_algorithm::cosim::impl::initialize()

Check state_init_test.cpp to observe different behaviours with the previous libcosim version

@davidhjp01 davidhjp01 added the bug Something isn't working label Nov 16, 2024
@davidhjp01 davidhjp01 self-assigned this Nov 16, 2024
@davidhjp01 davidhjp01 marked this pull request as draft November 16, 2024 10:28
@davidhjp01 davidhjp01 marked this pull request as ready for review November 16, 2024 12:52
@davidhjp01
Copy link
Contributor Author

I guess this PR contains some conflicts with #777, will need to update based on the merge :)

Copy link
Member

@kyllingstad kyllingstad left a comment

Choose a reason for hiding this comment

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

The implementation looks good, and it's great to get this fixed. Correctness issues should always have high priority, IMO.

That said, I wonder if this really needs to be an entirely new interface function in simulator. Couldn't the things you do in the new simulator::initialize_start_values() function be done at the start of simulator::setup() instead, before it calls slave_->start_simulation()?

void initialize_start_values()
{
auto deltaT = duration::zero();
auto filter = [this](const variable_type& vt) {
Copy link
Member

Choose a reason for hiding this comment

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

variable_type and value_reference are just integers in disguise, and should therefore be passed by value rather than by reference.

Copy link
Member

Choose a reason for hiding this comment

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

Also, we have the scalar_value alias which you can use instead of that variant.

Copy link
Member

Choose a reason for hiding this comment

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

When adding an FMU, you should update tests/data/fmi2/README.md and possibly include a licence file, cf. #764.

@@ -518,6 +546,34 @@ class slave_simulator::impl
return result;
}

void initialize_start_values()
Copy link
Member

Choose a reason for hiding this comment

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

The functions are generally listed in the order they're supposed to be called, so this should come before setup().

@@ -119,6 +119,11 @@ class simulator : public manipulable
*/
virtual void start_simulation() = 0;

/**
* Runs fmiXSetINI (4.2.4) functions to initialize variables' start values.
Copy link
Member

Choose a reason for hiding this comment

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

We haven't made reference to the FMI spec anywhere else in this interface, nor in any other of our general interfaces, because they are supposed to be agnostic with respect to the underlying binary interface. All FMI-specific stuff is hidden away in the cosim::fmi module. (I admit that we're still leaking FMI details like a sieve, but we haven't been this explicit about it before.) I suggest writing a more general and extensive description of what the function is supposed to do instead.

Also, as commented elsewhere, the functions are generally in call order, so this should go before setup().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants