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.

Sign up for GitHub

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

Jump to bottom

Feature/779 state serialization #780

Draft
wants to merge 33 commits into
base: dev/state-persistence
Choose a base branch
from
Draft

Feature/779 state serialization #780

wants to merge 33 commits into from

Conversation

davidhjp01
Copy link
Contributor

@davidhjp01 davidhjp01 commented Dec 10, 2024 •
edited
Loading

closes #779

  • Uses libcbor for (de)serialization.
  • Overrides << and >> to (de)seserialize cosim::serialization::node object to std::o/istream
  • Added state_serilization_test.cpp which is based on save_state_test.cpp for testing.
  • Extended save_state_test.cpp for testing export and import of states to/from files
  • Added print_ptree for replacing the previous << operator for pretty-printing cosim::serialization::node.
  • Reads canGetAndSetFMUstate and canSerializeFMUstate cosim capabilities from modelDescrpition.xml and store them to cosim::model_description
  • Updated time_series_observer.cpp to clear internal buffers when state is restored (see the comment below).

TODOs

  • Add missing comments to the new APIs.
  • Support state saving with proxyfmu

@davidhjp01 davidhjp01 linked an issue Dec 10, 2024 that may be closed by this pull request
Implement CBOR based de-/serializer for state import/export #779
Open
3 tasks
@davidhjp01 davidhjp01 changed the base branch from master to dev/state-persistence December 10, 2024 04:18
@davidhjp01 davidhjp01 changed the base branch from dev/state-persistence to master December 10, 2024 04:20
@davidhjp01 davidhjp01 marked this pull request as draft December 10, 2024 04:51
@davidhjp01 davidhjp01 changed the base branch from master to dev/state-persistence December 10, 2024 08:16
@davidhjp01 davidhjp01 marked this pull request as ready for review January 15, 2025 08:21
@davidhjp01
Copy link
Contributor Author

@kyllingstad, @restenb the PR is ready for your reviews :)

davidhjp01 and others added 2 commits January 23, 2025 13:23
@davidhjp01
Added libcbor as PRIVATE for target_link_libraries
fbc9e0b
@davidhjp01
Controlling fixed precision for logging float point values (#775)
d3252ef 
* Added an option to file_observer_config to set
fixed precision value.

* Fixed build failure
@davidhjp01
Copy link
Contributor Author

I think we need to parse the following CoSimulation capabilities from each FMU's modelDescription file

  • canGetAndSetFMUstate
  • canSerializeFMUstate

so that users can check if they can export states from the FMUs they are running. I think FMUs not implementing these functions can throw exception or abruptly terminate?

@restenb
Copy link
Member

restenb commented Jan 24, 2025

I think we need to parse the following CoSimulation capabilities from each FMU's modelDescription file

  • canGetAndSetFMUstate
  • canSerializeFMUstate

so that users can check if they can export states from the FMUs they are running. I think FMUs not implementing these functions can throw exception or abruptly terminate?

Not sure what the behavior would be, but it makes sense to include this check in any case. An FMU that does not set these flags should not be possible to use with state saving.

@davidhjp01
Copy link
Contributor Author

davidhjp01 commented Jan 28, 2025 •
edited
Loading

I found the consumers of libcosim that tries to build libcbor during conan install process fails saying that it cannot find libcbor.so . I tried to add transitive_libs=True, transitive_headers=True, making PUBLIC/PRIVATE to cmake target_link_libraries but no luck.

I think this is bug in conan when upstream dependencies are added as shared library (related to conan-io/conan#13302) . Not sure if this is resolved in the newer version, but I am linking libcobor statically to avoid this issue. Any comments for better solutions are welcome @kyllingstad @restenb

davidhjp01
davidhjp01 commented Feb 3, 2025
View reviewed changes
src/cosim/observer/time_series_observer.cpp Outdated
Comment on lines 269 to 280
entry.second->observe(currentStep, currentTime);
entry.second->reset();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I experienced an issue with time-series-observer when the state was restored.

The observer was not able to fetch correct time-series values via get_step_numbers and get_real_samples when the state was restored back in simulation time. This was due to the earlier simulation data in buffer timeSamples_ was cleared (adjustIfFull). Because of this, the data needed in the restored state was not available anymore and the observer was no longer able to retrieve the correct time series data unless the simulation progresses until the time matches those in "max(timeSamples_)".

Since the data will be "re-generated" anyways after the restore, I suggest just to clear the buffers (both timeSamples_ and step_numbers) to avoid this issue.

@davidhjp01 davidhjp01 marked this pull request as draft February 6, 2025 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kyllingstad kyllingstad Awaiting requested review from kyllingstad

@restenb restenb Awaiting requested review from restenb

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement CBOR based de-/serializer for state import/export
3 participants
@davidhjp01 @restenb @kyllingstad