Open kyllingstad opened 4 months ago
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 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.
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 implementations