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

esmini OSMP FMU #409

Closed
wants to merge 5 commits into from
Closed

esmini OSMP FMU #409

wants to merge 5 commits into from

Conversation

ClemensLinnhoff
Copy link
Contributor

@ClemensLinnhoff ClemensLinnhoff commented Mar 14, 2023

This example compiles an OSMP FMU with which esmini can be used in an FMU-based co-simulation.

It is based on the OSMPDummySource example.
Esmini is initialized via the API in the doInit function of the FMU.
In the doCalc function (a subsequent function of doStep), the OSI ground truth data is fetched via the API and copied to an OSI SensorView message.
The SensorView is the output of the FMU.

Completed steps:

  • Created an OSMP FMU which uses the esmini API to fetch OSI ground truth data and output as OSI SensorView
  • Enable setting the OpenScenario path via FMI
  • Include a minimal example ssd file (e.g. in combination with the OSMPDummySensor to be used with OpenMCx

This PR is related to #341

Signed-off-by: ClemensLinnhoff <clemens.linnhoff@persival.de>
@ClemensLinnhoff
Copy link
Contributor Author

Currently the OpenScenario file needs to be hardcoded here. In a next step I want to make it an FMI parameter, so it can be set by the co-simulation (e.g. in the system structure definition (ssd)).

@ClemensLinnhoff
Copy link
Contributor Author

In a first test I was able to run this esmini FMU together with the OSMPDummySensor using OpenMCx as a co-simulation platform. And it worked like a charm :)

@ClemensLinnhoff
Copy link
Contributor Author

I currently included FMI as a submodule. Since other 3rd party libraries are included in esmini otherwise, we might do this with FMI similarly. However, I quite like using submodules.

Signed-off-by: ClemensLinnhoff <clemens.linnhoff@persival.de>
Signed-off-by: ClemensLinnhoff <clemens.linnhoff@persival.de>
@ClemensLinnhoff
Copy link
Contributor Author

Currently the OpenScenario file needs to be hardcoded here. In a next step I want to make it an FMI parameter, so it can be set by the co-simulation (e.g. in the system structure definition (ssd)).

edit: This is already implemented now. The xosc file path is set via FMI parameter.

Signed-off-by: ClemensLinnhoff <clemens.linnhoff@persival.de>
@ClemensLinnhoff
Copy link
Contributor Author

I added an exemplary system structure defintion. This concludes this example from my point of view.

@ClemensLinnhoff ClemensLinnhoff marked this pull request as ready for review March 14, 2023 19:05
Signed-off-by: ClemensLinnhoff <clemens.linnhoff@persival.de>
@eknabevcc
Copy link
Collaborator

Excellent! I have not much clue about FMI. But it all looks good, and it passes the CI jobs 👍

So next step I think is to merge it and make it available for our additional stakeholder @jdsika to have a look as well.

There is basically only one technical concern before merging:
When I checked the branch locally I found out that some binary files (libs) were added by one commit. Although deleted in a later commit they are still present in history. Which is not desired, for example it will be downloaded for every clone of the repo. I fixed it (basically) by squashing your commits into one. It's still you as author.

Please check this branch: https://github.com/esmini/esmini/tree/feature/esmini_fmu

It also passes the CI. So if it looks OK to you I can merge it today. And then I'll close this PR with a reference to that single commit of yours.

@ClemensLinnhoff
Copy link
Contributor Author

Yes, I also always squash the commits to keep th repository clean. Uploading these binary files in the middle was probably just a mistake. So squashing it makes even more sense.
But as far as I know, you do not need the separate branch. You can just squash the commits during the merge.
Anyways, I am happy with it :)

After the merge I will then try to apply this esmini FMU in the test framework at OpenMSL. Then we can see it working together with a sensor model in a co-simulation on the GitHub server. Maybe we can reference the working example in the documentation then.
For now, I just tested this offline on my computer.

@jdsika
Copy link

jdsika commented Mar 23, 2023

How long are links of moved repositories on GitHub valid? I see that ALKS is still pointing to Andreas personal GitHub space. Soon we will move ALKS very likely again to OpenMSL.

This is really cool!

@eknabevcc
Copy link
Collaborator

Yes, I also always squash the commits to keep th repository clean. Uploading these binary files in the middle was probably just a mistake. So squashing it makes even more sense. But as far as I know, you do not need the separate branch. You can just squash the commits during the merge. Anyways, I am happy with it :)

After the merge I will then try to apply this esmini FMU in the test framework at OpenMSL. Then we can see it working together with a sensor model in a co-simulation on the GitHub server. Maybe we can reference the working example in the documentation then. For now, I just tested this offline on my computer.

All true! But I wrote "(basically)" because there was some minor hick-ups: 1. LFS handling missing for png image (ref: git-lfs/git-lfs#1939) and 2. Similar issue with two .so files during the rebase/squash. This happened when I did the rebase locally, and it could potentially also happen when squash-and-merge the PR here.

By the way, I don't think there is an option to rebase-and-squash for some reason, which I'm more familiar with. Maybe squash and merge would have the same effect, I'm not sure. Anyway, with a new branch I could do squash via git reset operation instead, which smoothly skips any added->deleted files.

In addition, we just introduced dev branch as the new top integration point for features and bug fixes. So I wanted to merge it on that one first, to not brake the rules :) (and this was done after you filed the PR, so you couldn't know)

@ClemensLinnhoff
Copy link
Contributor Author

Alright, that makes sense. Then I am fine with you merging your branch into dev!

eknabevcc pushed a commit that referenced this pull request Mar 23, 2023
Signed-off-by: ClemensLinnhoff <clemens.linnhoff@persival.de>
@eknabevcc
Copy link
Collaborator

Great! It's merged on dev now, commit 5b8f92f
It will be merged to master for next release.
Thanks a lot for this contribution!

How long are links of moved repositories on GitHub valid? I see that ALKS is still pointing to Andreas personal GitHub space. Soon we will move ALKS very likely again to OpenMSL.

Good finding! I'll look into the ALKS submodule ref tomorrow.

This is really cool!

Agree! :)

@ClemensLinnhoff
Copy link
Contributor Author

Perfect, thanks! Then we can close this PR?

@eknabevcc
Copy link
Collaborator

How long are links of moved repositories on GitHub valid? I see that ALKS is still pointing to Andreas personal GitHub space. Soon we will move ALKS very likely again to OpenMSL.

I updated the remote to https://github.com/asam-oss/OSC-ALKS-scenarios for now (commit 1029fb5). Staying on the latest release tag v0.4.1.

Note: This applies to dev branch so far. Will be merged to master by next release.

@eknabevcc
Copy link
Collaborator

Perfect, thanks! Then we can close this PR?

Yes, we can. I'll close it now. Just a few last words:

  1. Good work! I expected more need for my support. But you managed excellent without :)
  2. Regarding use of submodules: Yes, I agree fully. It's nice to use submodule instead of copies. We're doing it for ALKS. And I think we should do it in some more cases. However, in a few cases we did otherwise for basically one of three reasons: 1. Extreme compile time (e.g. OSG would take several hours to compile for relevant targets and variants - not feasible for CI) 2. Using just a subset (the case for e.g. OSG and SUMO) 3. Minor modifications that does not make sense to feed back to main project, I think this is the case for expr.
  3. I added fmi to the list of 3rd party project use (commit df43b23)

Thanks again for this contribution.

@eknabevcc eknabevcc closed this Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants