Add new FMU node-type - #1017
Conversation
stv0g
left a comment
stv0g
left a comment
There was a problem hiding this comment.
Nice work. Its great to see FMI support landing in VILLAS
| while (hasInputs && (writingTurn || currentTime >= stopTime)) { | ||
| pthread_cond_wait(&cv, &mutex); | ||
| } | ||
| double targetTime = currentTime + stepSize; |
There was a problem hiding this comment.
I think this not executing the FMU using a real-time scale?
Can you elaborate a bit how this node-type is indentend to be used?
E.g. does it expect periodic input data to proceed its internal time?
How does the internal time related to the real time?
Do we already have a PR for the node-type? I think this deserves some explanation.
There was a problem hiding this comment.
Right now, it needs periodic input data. My plan would be to include real-time later
pjungkamp
left a comment
pjungkamp
left a comment
There was a problem hiding this comment.
I really like the general coding style, it's clean and very readable. Could you please fix the commit such that you have one commit that introduces the FMU node type. You have two unrelated changes changing the capitalization of writing_turn in opendss.{hpp,cpp} which should not be part of that commit. You can either move those changes into their commit or drop them.
Signed-off-by: Ritesh.K <riteshkarki6@gmail.com>
|
As per the new Contribution guidelines, could you please briefly state if LLMs have been used for this contribution? |
|
The main logic was written by Jitpanu before. Edit: Sorry, I just saw the new Contribution guidelines regarding the use of LLMs,
|
|
Also, clang-format seems to format functions which have just a single line of body by rewriting it into a single line. I think that is why every time I ran clang-format, it would reformat the function body. I think that's why the code formatting looked different to what you expected. For the integration test, I did not know where to place the fmu file in the repository, otherwise the intergration test itself is working. But once I add the fmu file, I can remove the "Test not ready" comment from the integration test. |
pjungkamp
left a comment
pjungkamp
left a comment
There was a problem hiding this comment.
Just some more smaller nitpicks here.
Signed-off-by: Ritesh.K <riteshkarki6@gmail.com>
Signed-off-by: Ritesh.K <riteshkarki6@gmail.com>
Signed-off-by: Ritesh.K <riteshkarki6@gmail.com>
4 participants
@al3xa23
New node type implementation based on Functional Mock-up Interface (FMI 3.0). Currently supports Co-Simulation. Functionality can be further extended to include Model Exchange simulation too.