-
Notifications
You must be signed in to change notification settings - Fork 12
Simplify API and code by dropping (alleged) support for changing system structure during simulation #771
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
Comments
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). |
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 |
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.)
* Enable saving and restoring subsimulator state (#765) This is the first step towards closing #756. I've added functions corresponding to FMI 2.0's `fmi2{Get,Set,Free}FMUstate()` throughout the various layers of subsimulator interfaces and implementations: * `cosim::slave` and its implementation in `cosim::fmi::v2::slave_instance` * `cosim::simulator` and its implementation in `cosim::slave_simulator` This led me to also remove the `slave_state` and `state_guard` stuff that was in `slave_simulator.{hpp,cpp}`. The overloading of the "state" terminology became confusing, and it seemed like it was a lot of code for very little gain. (It was supposed to be a check of correct API usage, but I can't remember it ever actually catching a bug.) * Enable exporting and importing subsimulator state (#769) This is a follow-up to #765 and the second and final step to close #756. Here, I've implemented functionality to export the internal state of individual subsimulators in a generic, structured form, and to import them again later. This exported form is intended as an intermediate step before serialisation and disk storage. The idea was to create a type that can be inspected and serialised to almost any file format we'd like. The type is defined by `cosim::serialization::node` in `cosim/serialization.hpp`. It is a hierarchical, dynamic data type with support for a variety of primitive scalar types and a few aggregate types: strings, arrays of nodes, dictionaries of nodes, and binary blobs. (Think JSON, only with more types.) It is based on Boost.PropertyTree * Enable saving and restoring full simulation state (#777) 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.) * Feature/779 state serialization (#780) * Enable saving and restoring full simulation state Closes #768 * Added tinycbor based serialization method * Debug test * Replaced tinycbor -> libcbor (ver 0.1) * deserialization using cbor_reader * Separated ctx to own struct * Tag as std::optional in ctx * State serialization test * Removed comments * Addressed Linux build issue * Added BouncingBall reference model from modelica for adding byte vector serialization test * Added BouncingBall license in README * Added comments * Deleted state_serialization_test.cpp * Added libcbor as PRIVATE for `target_link_libraries` * Controlling fixed precision for logging float point values (#775) * Added an option to file_observer_config to set fixed precision value. * Fixed build failure * Reading canGetAndSetFMUstate and canSerializeFMUstate from model description file * Added transitive_libs for libcbor to avoid cmake error * linking libcbor statically. * Resetting time series observer upon state restore * State saving support with proxyfmu * Split proxyfmu from the save_state_test. * Uses upgraded thrift/boost * Fixed boost header issue * Fixed boost header issue * Fixed boost header issue * Update dependency * Updated action * Upgraded libzip * Upgraded libzip * libzip deprecated func fix * Observer fix * Addressing comments 1. Defining exported sourced explicitly 2. Try to use shared libcbor. * made libcbor PUBLIC in cmake (may require the consumer also declare libcbor for linking libcosim) * Addressed comments * Addressed comments * Addressed comments - 02 * Addressed comments - 03 * Updated cosim_capabilities -> simulator_capabilities. Using cosim::serialization::format::cbor to choose which serialization/deserialization to use * update * pretty_print format added * Updated error message * Updated error message * Updated error message * Using fmu::model_description::capabilities in copy_current_state() and export_state() * Updated error message * Don't require importing def_types.h * Using updated proxyfmu that hides thrift related libs * Updated field names for the model description * Updated proxyfmu dependency --------- Co-authored-by: Lars T. Kyllingstad <lars.kyllingstad@sintef.no> * Fixed an issue from the merge --------- Co-authored-by: Lars T. Kyllingstad <lars.kyllingstad@sintef.no>
Uh oh!
There was an error while loading. Please reload this page.
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. afterexecution::simulate_until()
orexecution::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()
afterfixed_step_algorithm::initialize()
has been called, then the subsimulator’s initialisation functions (simulator::setup()
andsimulator::do_iteration()
) will never be called. This, again, means that the underlying FMU will never have itsfmi2EnterInitializationMode()
andfmi2ExitInitializationMode()
functions called, and then it’s not allowed to call itsfmi2DoStep()
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:
execution
could be constructed directly from asystem_structure
, and we could remove several of its member functions:add_slave()
,add_function()
,connect_variables()
, andset_{real,integer,boolean,string}_initial_value()
. Thus, theexecution
API could be focused solely on actually running the simulation, not setting it up.algorithm
, we could removeadd_simulator
,remove_simulator()
,add_function()
,connect_variables()
, anddisconnect_variable()
. (Note how the removal functions aren't even exposed viaexecution
yet, and that there is noremove_function()
. It's all very half baked.)observer
, we could remove thesimulator_added()
,simulator_removed()
,variables_connected()
, andvariable_disconnected()
event functions. None of our observers use the latter two, so all could be replaced by a singlesetup()
function that gets a list ofobservable
pointers.manipulator
, the situation is much the same as forobserver
– no simulator added/removed events needed, just a setup function which gets a list ofmanipulable
pointers.Furthermore, it would allow us to simplify the implementation in several places:
inject_system_structure()
hack could be removed.algorithm
,observer
, andmanipulator
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.Proposed action plan:
observer
and its implementationsmanipulator
and its implementationsexecution
algorithm
and its implementationsThe text was updated successfully, but these errors were encountered: