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

Simplify API and code by dropping (alleged) support for changing system structure during simulation #771

Open
4 tasks
kyllingstad opened this issue Jul 18, 2024 · 2 comments
Labels
discussion needed Let's have a discussion about this enhancement New feature or request

Comments

@kyllingstad
Copy link
Member

kyllingstad commented Jul 18, 2024

In principle, the current libcosim API permits one to change the system structure after a simulation has started. More to the point, one is allowed to call execution::add_simulator(), execution::connect_variables(), etc. after execution::simulate_until() or execution::step() has been called. This was a deliberate design choice in the early OSP days, because we were told that the ability to dynamically add and remove entities was a desirable feature in certain human-in-the-loop simulation use cases.

Now, several years later, I am fairly certain that no one is using this for anything and it doesn't even work. Firstly, because we don't have tests for it, and secondly, for the following reason: If a subsimulator is added with fixed_step_algorithm::add_simulator() after fixed_step_algorithm::initialize() has been called, then the subsimulator’s initialisation functions (simulator::setup() and simulator::do_iteration()) will never be called. This, again, means that the underlying FMU will never have its fmi2EnterInitializationMode() and fmi2ExitInitializationMode() functions called, and then it’s not allowed to call its fmi2DoStep() function either.

This leaves us with two choices: fix this feature or drop it. I say drop it.

Dropping it would allow us to simplify the API in several places:

  • An execution could be constructed directly from a system_structure, and we could remove several of its member functions: add_slave(), add_function(), connect_variables(), and set_{real,integer,boolean,string}_initial_value(). Thus, the execution API could be focused solely on actually running the simulation, not setting it up.
  • In algorithm, we could remove add_simulator, remove_simulator(), add_function(), connect_variables(), and disconnect_variable(). (Note how the removal functions aren't even exposed via execution yet, and that there is no remove_function(). It's all very half baked.)
  • In observer, we could remove the simulator_added(), simulator_removed(), variables_connected(), and variable_disconnected() event functions. None of our observers use the latter two, so all could be replaced by a single setup() function that gets a list of observable pointers.
  • For manipulator, the situation is much the same as for observer – no simulator added/removed events needed, just a setup function which gets a list of manipulable pointers.

Furthermore, it would allow us to simplify the implementation in several places:

  • The whole inject_system_structure() hack could be removed.
  • algorithm, observer, and manipulator implementations would be less complex and potentially more efficient, because their variable mapping tables are set up just once, they don't have to take into consideration that the variable mappings can change dynamically.
  • It would make state saving and persistence simpler, because one wouldn't have to keep data related to system structure at various levels (algorithm, execution), only at the outer level or perhaps not at all.

Proposed action plan:

  • Simplify observer and its implementations
  • Simplify manipulator and its implementations
  • Simplify execution
  • Simplify algorithm and its implementations
@kyllingstad kyllingstad added enhancement New feature or request discussion needed Let's have a discussion about this labels Jul 18, 2024
@davidhjp01
Copy link
Contributor

After discussion, we agreed that we will drop a support on adding/removing simulators (models) and simplify libcosim code. But dynamic adding/removing connections dynamically can be useful (future work).

@kyllingstad
Copy link
Member Author

Pull request #777 dynamically prevents adding/removing subsimulators after initialisation, so it is a step in the right direction. But I think it would be even better to prevent it statically, i.e., by not having functions like add_simulator(), simulator_added() etc. at all. Instead, a complete list of subsimulators could be passed to the execution, algorithm, observers, etc. at some point, for example during construction.

kyllingstad added a commit that referenced this issue Nov 25, 2024
This closes #768. Some changes may warrant a bit of extra explanation:

**`execution` and `algorithm`:** I have taken one step towards prohibiting adding/removing sub-simulators after the co-simulation has begun, as decided in #771.  In particular, I've added the `execution::initialize()` function, which marks the point where such changes are no longer allowed.  (This is a backwards-compatible change, because it gets automatically called by `execution::step()` if it hasn't been called manually.)

**`slave_simulator`**: The changes in `slave_simulator.cpp` really ought to have been included in #769. Unfortunately, I didn't realise they were necessary before it was all put into the context of saving algorithm and execution state.  Basically, I thought I could get away with just saving each FMU's internal state, but it turns out that we also need to save the `slave_simulator` "get" and "set" caches.

(For those interested in the nitty-gritties, this is due to some subtleties regarding exactly when the cache values are set in the course of a co-simulation, relative to when the values are passed to the FMU. At the end of an "algorithm step", the "set cache" is out of sync with the FMUs input variables, and won't be synced before the next step. Simply performing an additional sync prior to saving the state is not sufficient, because that could have an effect on the FMUs output variables, thus invalidating the "get cache". That could in principle be updated too, but then the `slave_simulator` is in a whole different state from where it was when we started to save the state.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion needed Let's have a discussion about this enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants