Adapt to fmi4c instance handles - #1638
Conversation
fmi4c renamed fmiHandle to fmuHandle and moved all runtime calls to fmi2InstanceHandle/fmi3InstanceHandle returned by the instantiate functions. Store that instance per component and pass it to every get/set/step/terminate call, and reset it after freeInstance to avoid a double free in the destructor. SignalDerivative is split into FMI 2 and FMI 3 variants, since the handle types no longer alias; FMI 3 has no setRealInputDerivatives. Drop the "Loading FMI version 3..." line from the expected test results, which fmi4c no longer prints.
AnHeuermann
added
the
CI/Update Submodules
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1638 +/- ##
=======================================
Coverage 29.24% 29.24%
=======================================
Files 68 68
Lines 12960 12985 +25
Branches 8389 8405 +16
=======================================
+ Hits 3790 3798 +8
- Misses 7979 7996 +17
Partials 1191 1191
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThe change separates FMU descriptor handles from instantiated FMI 2 and FMI 3 component handles. FMI 3 derivative retrieval is implemented. FMI 3 expected outputs no longer include loading messages. The ChangesFMI instance handle separation
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The update changes FMI runtime handle usage but currently treats FMI 3 derivative failures as successful calls, which can allow invalid values into simulations. This bounded correctness issue should be fixed before merging. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description includes all required sections: Related Issues, Purpose, and Approach. It explains the macOS and warning issues, the fmi4c v1.2 update, the implementation changes, and the test-output updates. Full details: Docstring CoverageExplanation Docstring coverage is 1.46% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 137 functions across 15 files. (1 skipped: 1 unsupported.)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b5b77758-3e86-4f99-a4f5-f526989ccf90
📒 Files selected for processing (35)
3rdPartysrc/OMSimulatorLib/Component.hsrc/OMSimulatorLib/ComponentFMU3CS.cppsrc/OMSimulatorLib/ComponentFMU3CS.hsrc/OMSimulatorLib/ComponentFMU3ME.cppsrc/OMSimulatorLib/ComponentFMU3ME.hsrc/OMSimulatorLib/ComponentFMUCS.cppsrc/OMSimulatorLib/ComponentFMUCS.hsrc/OMSimulatorLib/ComponentFMUME.cppsrc/OMSimulatorLib/ComponentFMUME.hsrc/OMSimulatorLib/FMUInfo.cppsrc/OMSimulatorLib/FMUInfo.hsrc/OMSimulatorLib/SignalDerivative.cppsrc/OMSimulatorLib/SignalDerivative.hsrc/OMSimulatorLib/Variable.cppsrc/OMSimulatorLib/Variable.htestsuite/tests/CompositeModels/FeedthroughConnections.pytestsuite/tests/CompositeModels/FeedthroughSetValue1.pytestsuite/tests/CompositeModels/FeedthroughSetValue2.pytestsuite/tests/CompositeModels/connectFmi2AndFmi3.pytestsuite/tests/reference-fmus/3.0/cs/BouncingBall-cs.pytestsuite/tests/reference-fmus/3.0/cs/Dahlquist-cs.pytestsuite/tests/reference-fmus/3.0/cs/Feedthrough-cs.pytestsuite/tests/reference-fmus/3.0/cs/Resource-cs.pytestsuite/tests/reference-fmus/3.0/cs/Stair-cs.pytestsuite/tests/reference-fmus/3.0/cs/VanDerPol-cs.pytestsuite/tests/reference-fmus/3.0/me/BouncingBall-me.pytestsuite/tests/reference-fmus/3.0/me/Dahlquist-me.pytestsuite/tests/reference-fmus/3.0/me/Feedthrough-me.pytestsuite/tests/reference-fmus/3.0/me/Resource-me.pytestsuite/tests/reference-fmus/3.0/me/Stair-me.pytestsuite/tests/reference-fmus/3.0/me/VanDerPol-me.pytestsuite/tests/simulation/FMI3_SimpleSimulation1.pytestsuite/tests/simulation/FMI3_SimpleSimulation2.pytestsuite/tests/simulation/FMI3_SimpleSimulation3.py
💤 Files with no reviewable changes (19)
- testsuite/tests/CompositeModels/connectFmi2AndFmi3.py
- testsuite/tests/CompositeModels/FeedthroughSetValue2.py
- testsuite/tests/simulation/FMI3_SimpleSimulation1.py
- testsuite/tests/reference-fmus/3.0/me/Dahlquist-me.py
- testsuite/tests/reference-fmus/3.0/cs/Stair-cs.py
- testsuite/tests/reference-fmus/3.0/cs/Resource-cs.py
- testsuite/tests/CompositeModels/FeedthroughSetValue1.py
- testsuite/tests/reference-fmus/3.0/me/Stair-me.py
- testsuite/tests/simulation/FMI3_SimpleSimulation2.py
- testsuite/tests/reference-fmus/3.0/cs/VanDerPol-cs.py
- testsuite/tests/simulation/FMI3_SimpleSimulation3.py
- testsuite/tests/reference-fmus/3.0/cs/BouncingBall-cs.py
- testsuite/tests/reference-fmus/3.0/me/Feedthrough-me.py
- testsuite/tests/reference-fmus/3.0/me/BouncingBall-me.py
- testsuite/tests/reference-fmus/3.0/me/Resource-me.py
- testsuite/tests/reference-fmus/3.0/cs/Dahlquist-cs.py
- testsuite/tests/reference-fmus/3.0/cs/Feedthrough-cs.py
- testsuite/tests/reference-fmus/3.0/me/VanDerPol-me.py
- testsuite/tests/CompositeModels/FeedthroughConnections.py
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
| if (fmi3OK != fmi3_getOutputDerivatives(instance, vrs.data(), order, orders.data(), values, order)) | ||
| logError("fmi3_getOutputDerivatives failed"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- target file ---'
sed -n '1,180p' src/OMSimulatorLib/SignalDerivative.cpp
printf '%s\n' '--- bound callers and definitions ---'
rg -n -C 5 'getRealOutputDerivative|SignalDerivative|fmi3_getOutputDerivatives|oms_status_error|oms_status_ok' src/OMSimulatorLibRepository: OpenModelica/OMSimulator
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- SignalDerivative.cpp: FMI 3 constructor ---'
sed -n '85,121p' src/OMSimulatorLib/SignalDerivative.cpp
printf '%s\n' '--- ComponentFMU3CS derivative path ---'
rg -n -C 12 'getRealOutputDerivative' src/OMSimulatorLib/ComponentFMU3CS.cpp src/OMSimulatorLib/ComponentFMU3CS.h
printf '%s\n' '--- SignalDerivative contract ---'
sed -n '1,130p' src/OMSimulatorLib/SignalDerivative.hRepository: OpenModelica/OMSimulator
Length of output: 7731
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- complete ComponentFMU3CS::getRealOutputDerivative ---'
sed -n '1510,1575p' src/OMSimulatorLib/ComponentFMU3CS.cpp
printf '%s\n' '--- direct callers ---'
rg -n -C 8 'getRealOutputDerivative\s*\(' src/OMSimulatorLib --glob '*.cpp' --glob '*.h'
printf '%s\n' '--- fmi4c declarations and version references ---'
rg -n -C 6 'fmi3_getOutputDerivatives|fmi4c|FMI4C_VERSION|FMI4C' . --glob '!*.json' --glob '!*.xml' --glob '!*.md'Repository: OpenModelica/OMSimulator
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- fmi4c source/version files tracked by the repository ---'
git ls-files | rg -i '(^|/)(fmi4c|thirdparty|3rd|external)(/|$)|fmi4c' | head -200
printf '%s\n' '--- dependency declarations ---'
rg -n -C 5 'fmi4c' CMakeLists.txt cmake src tests .github --glob '!*.cpp' --glob '!*.h' --glob '!*.md' 2>/dev/null | head -240Repository: OpenModelica/OMSimulator
Length of output: 2095
🌐 Web query:
fmi4c 1.2 fmi3_getOutputDerivatives return status output values contract
💡 Result:
In the FMI 3.0 standard, the function fmi3GetOutputDerivatives is used to retrieve the time derivatives of continuous output variables [1]. While fmi4c is a C library for importing FMUs that supports FMI 3.0 [2], the specific behavior regarding the return status and output values contract is governed by the FMI 3.0 specification rather than the fmi4c wrapper itself. According to the FMI 3.0 standard: 1. Function Signature: The function is defined as fmi3Status fmi3GetOutputDerivatives(fmi3Instance instance, const fmi3ValueReference valueReferences[], size_t nValueReferences, const fmi3Int32 orders[], fmi3Float64 values[], size_t nValues) [3][4]. 2. Return Status (fmi3Status): The function returns a status code indicating the success or failure of the operation (e.g., fmi3OK, fmi3Warning, fmi3Discard, fmi3Error, or fmi3Fatal) [5][6]. 3. Output Values Contract: - The values array is populated with the requested derivatives [3][4]. - The derivatives are calculated with respect to time (j-th derivatives) for variables specified by the valueReferences and their corresponding orders (orders array) [1][5]. - This function is only permitted to be called in specific states, typically including StepMode, StepDiscarded, or Terminated [3][4]. - The contract requires the FMU to provide the requested derivatives if it supports this capability [5][7]. If an FMU does not implement the calculation of output derivatives, it may return an error status or not implement the function [3][4]. When using fmi4c, ensure the FMU being imported explicitly supports the required derivative calculations, as the library acts as an interface to the underlying FMI API defined by the FMU's modelDescription.xml [2][5].
Citations:
- 1: https://github.com/modelica/fmi-standard/blob/main/docs/4_2_co-simulation_api.adoc
- 2: https://github.com/robbr48/fmi4c
- 3: https://github.com/modelica/Reference-FMUs/blob/c8d4e6d9/src/fmi3Functions.c
- 4: https://github.com/OpenModelica/OMSimulator/blob/e65251357c2f42bcda474428f122247864b141b8/testsuite/tests/resources/Dahlquist3/sources/fmi3Functions.c
- 5: https://fmi-standard.org/docs/3.0/
- 6: https://fmi-standard.org/docs/3.0.2/
- 7: https://github.com/modelica/fmi-standard/blob/main/docs/4___co-simulation.adoc
🌐 Web query:
site:github.com/robbr42/fmi4c fmi3_getOutputDerivatives
💡 Result:
The repository appears to be robbr48/fmi4c, not robbr42/fmi4c. It is a C library supporting FMI 1.0, 2.0, and 3.0. (github.com)
I found no indexed match for fmi3_getOutputDerivatives. Note that the FMI 3 API function is conventionally named fmi3GetOutputDerivatives (camelCase), and the FMI specification documents it for retrieving continuous-output derivatives. (github.com)
Citations:
- 1: https://github.com/fmi-tools?utm_source=openai
- 2: https://github.com/modelica/fmi-standard/blob/main/docs/4_2_co-simulation_api.adoc?utm_source=openai
🌐 Web query:
github fmi4c fmi3_getOutputDerivatives source
💡 Result:
The function fmi3GetOutputDerivatives is part of the FMI 3.0 standard API [1][2]. Within the fmi4c library, which serves as a C wrapper for importing FMI units, this function is typically handled via function pointers assigned to a structure (e.g., fmu->fmi3.getOutputDerivatives) [3][4]. The library implements placeholder functions for API calls, including fmi3GetOutputDerivatives, to prevent crashes when a specific FMU does not export that function [3]. In the context of actual FMU implementations or reference models (such as those in the Modelica Reference-FMUs repository), the source code for fmi3GetOutputDerivatives generally follows this structure: #define MASK_fmi3GetOutputDerivatives (StepMode | StepDiscarded | Terminated) fmi3Status fmi3GetOutputDerivatives(fmi3Instance instance, const fmi3ValueReference valueReferences[], size_t nValueReferences, const fmi3Int32 orders[], fmi3Float64 values[], size_t nValues) { UNUSED(nValues); BEGIN_FUNCTION(GetOutputDerivatives); #ifdef GET_OUTPUT_DERIVATIVE for (size_t i = 0; i < nValueReferences; i++) { CALL(getOutputDerivative(S, (ValueReference)valueReferences[i], orders[i], &values[i])); } #else UNUSED(valueReferences); UNUSED(nValueReferences); UNUSED(orders); UNUSED(values); NOT_IMPLEMENTED; #endif END_FUNCTION; } This implementation verifies the current FMI state using a mask and provides a mechanism to either execute the derivative calculation or return a "not implemented" status if the specific FMU does not support it [5][6].
Citations:
- 1: https://github.com/modelica/fmi-standard/blob/main/docs/4_2_co-simulation_api.adoc
- 2: https://fmi-standard.org/docs/3.0.2/
- 3: https://github.com/robbr48/fmi4c
- 4: robbr48/fmi4c@ae60a13
- 5: https://github.com/modelica/Reference-FMUs/blob/c8d4e6d9/src/fmi3Functions.c
- 6: https://github.com/OpenModelica/OMSimulator/blob/e65251357c2f42bcda474428f122247864b141b8/testsuite/tests/resources/Dahlquist3/sources/fmi3Functions.c
Propagate non-OK FMI 3 derivative statuses.
SignalDerivative only logs a non-fmi3OK result from fmi3_getOutputDerivatives. ComponentFMU3CS::getRealOutputDerivative then returns oms_status_ok, so invalid derivative values can reach SystemWC. Record the failure and return oms_status_error.
1 participant
Related Issues
Some users want to compile OMSimulator on macOS. But there are some issues like OpenModelica/OMSimulator-3rdParty#94.
We should update fmi4c to the latest version. Another upside: We get rid of many compiler warnings.
Purpose
Approach
fmi4c renamed
fmiHandletofmuHandleand moved all runtime calls tofmi2InstanceHandle/fmi3InstanceHandlereturned by the instantiate functions. Store that instance per component and pass it to everyget/set/step/terminatecall, and reset it afterfreeInstanceto avoid a double free in the destructor.SignalDerivativeis split into FMI 2 and FMI 3 variants, since the handle types no longer alias; FMI 3 has nosetRealInputDerivatives.Drop the
"Loading FMI version 3..."line from the expected test results, which fmi4c no longer prints.