From 591617e06510eeddd489b545509b68aeb13d5edb Mon Sep 17 00:00:00 2001 From: Joakim Haugen Date: Thu, 9 Jul 2026 14:31:29 +0200 Subject: [PATCH] Fix three ECCO power-bond bugs and add regression tests Bug 1 (ecco_algorithm.cpp): add_power_bond() stored all four bond variable IDs but never called expose_for_getting() on them. adjust_step_size() then called get_real() on all four, which throws 'Variable with reference N not found in exposed variables' for any FMU-input variable not previously exposed. The default file_observer accidentally masked this by exposing all variables; a LogConfig-restricted observer omitting bond input variables (force, in_vel) triggered the crash. Fix: call expose_for_getting() for all four variables at the top of add_power_bond(). Bug 2 (osp_config_parser.cpp): the uniqueCount != numPowerBonds validation in add_power_bonds() was placed inside the per-bond loop. With two bonds configured it fired after the first iteration with numPowerBonds==1 and uniqueCount==2, throwing a spurious error. Fix: move the check outside the loop so it runs after all bonds have been registered. Bug 3 (ecco_algorithm_multi_bond_test.cpp): the test called add_power_bond() manually after inject_system_structure() had already registered the bond from the XML (double registration), and used wrong hardcoded value references (19/22/26/24 are ECCO step-size parameters, not bond variables). Fix: remove the duplicate add_power_bond() call and correct the variable refs to chassis.velocity=23, chassis.force=4, wheel.out_spring_damper_f=15, wheel.in_vel=7. New tests: - ecco_algorithm_powerbond_logconfig_test: loads quarter-truck via inject_system_structure with a file_observer_config that deliberately omits the FMU-input bond variables; verifies no crash (regresses Bug 1). - ecco_algorithm_two_bond_test: loads OspSystemStructure_MultiBond.xml (two independent chassis+wheel pairs, two bonds) and simulates 0.1 s; verifies no spurious throw (regresses Bug 2). - OspSystemStructure_MultiBond.xml: new test data for the two-bond scenario. --- src/cosim/algorithm/ecco_algorithm.cpp | 4 + src/cosim/osp_config_parser.cpp | 21 ++-- tests/CMakeLists.txt | 4 +- .../OspSystemStructure_MultiBond.xml | 97 +++++++++++++++++++ tests/ecco_algorithm_multi_bond_test.cpp | 34 +++---- ...cco_algorithm_powerbond_logconfig_test.cpp | 83 ++++++++++++++++ tests/ecco_algorithm_two_bond_test.cpp | 71 ++++++++++++++ 7 files changed, 287 insertions(+), 27 deletions(-) create mode 100644 tests/data/fmi2/quarter_truck/OspSystemStructure_MultiBond.xml create mode 100644 tests/ecco_algorithm_powerbond_logconfig_test.cpp create mode 100644 tests/ecco_algorithm_two_bond_test.cpp diff --git a/src/cosim/algorithm/ecco_algorithm.cpp b/src/cosim/algorithm/ecco_algorithm.cpp index ab3503d0..93f9b83d 100644 --- a/src/cosim/algorithm/ecco_algorithm.cpp +++ b/src/cosim/algorithm/ecco_algorithm.cpp @@ -309,6 +309,10 @@ class ecco_algorithm::impl void add_power_bond(cosim::variable_id input_a, cosim::variable_id output_a, cosim::variable_id input_b, cosim::variable_id output_b) { + simulators_.at(input_a.simulator).sim->expose_for_getting(input_a.type, input_a.reference); + simulators_.at(output_a.simulator).sim->expose_for_getting(output_a.type, output_a.reference); + simulators_.at(input_b.simulator).sim->expose_for_getting(input_b.type, input_b.reference); + simulators_.at(output_b.simulator).sim->expose_for_getting(output_b.type, output_b.reference); energies_.emplace_back(); energies_.emplace_back(); inputVariables_.push_back(input_a); diff --git a/src/cosim/osp_config_parser.cpp b/src/cosim/osp_config_parser.cpp index 038ebc40..a0d671bc 100644 --- a/src/cosim/osp_config_parser.cpp +++ b/src/cosim/osp_config_parser.cpp @@ -891,17 +891,20 @@ void add_power_bonds(const std::vector& } systemStructure.add_power_bond(pbName, powerbond); + } - // Check that the number of unique power bond names is equal to the number of power bonds. Otherwise, it is not possible to correctly connect the bonds. - std::sort(powerBondNames.begin(), powerBondNames.end()); - auto uniqueCount = static_cast(std::unique(powerBondNames.begin(), powerBondNames.end()) - powerBondNames.begin()); - auto numPowerBonds = static_cast(systemStructure.get_power_bonds().size()); + // Check that the number of unique power bond names is equal to the number of + // power bonds. This must be verified after all bonds have been added; checking + // inside the loop would compare a partial count against the full unique-name + // count and spuriously throw when more than one bond is configured. + std::sort(powerBondNames.begin(), powerBondNames.end()); + auto uniqueCount = static_cast(std::unique(powerBondNames.begin(), powerBondNames.end()) - powerBondNames.begin()); + auto numPowerBonds = static_cast(systemStructure.get_power_bonds().size()); - if (uniqueCount != numPowerBonds) { - std::ostringstream oss; - oss << "The number of powerbonds (" << numPowerBonds << ") is not equal to the number of unique power bond names (" << uniqueCount << ") found in the configured system. Power bond names must be unique pr. bond, that is found on only and exactly the two VariableConnections that form the bond."; - throw std::runtime_error(oss.str()); - } + if (uniqueCount != numPowerBonds) { + std::ostringstream oss; + oss << "The number of powerbonds (" << numPowerBonds << ") is not equal to the number of unique power bond names (" << uniqueCount << ") found in the configured system. Power bond names must be unique pr. bond, that is found on only and exactly the two VariableConnections that form the bond."; + throw std::runtime_error(oss.str()); } } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 37ec3d67..0b131b80 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -1,7 +1,9 @@ set(tests "ecco_algorithm_from_system_structure_test" - "ecco_algorithm_multi_bond_test" + "ecco_algorithm_multi_bond_test" + "ecco_algorithm_powerbond_logconfig_test" "ecco_algorithm_test" + "ecco_algorithm_two_bond_test" "file_observer_dynamic_logging_test" "file_observer_logging_test" "file_observer_logging_from_config_test" diff --git a/tests/data/fmi2/quarter_truck/OspSystemStructure_MultiBond.xml b/tests/data/fmi2/quarter_truck/OspSystemStructure_MultiBond.xml new file mode 100644 index 00000000..555bb3b1 --- /dev/null +++ b/tests/data/fmi2/quarter_truck/OspSystemStructure_MultiBond.xml @@ -0,0 +1,97 @@ + + + + 0.0001 + ecco + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + 0.99 + 0.0001 + 0.00001 + 0.01 + 0.2 + 1.5 + 0.2 + 0.15 + 1e-6 + 1e-6 + + diff --git a/tests/ecco_algorithm_multi_bond_test.cpp b/tests/ecco_algorithm_multi_bond_test.cpp index 8a6052dc..7bdd4e82 100644 --- a/tests/ecco_algorithm_multi_bond_test.cpp +++ b/tests/ecco_algorithm_multi_bond_test.cpp @@ -58,11 +58,15 @@ int main() auto chassisIndex = entityMaps.simulators.at("chassis"); auto wheelIndex = entityMaps.simulators.at("wheel"); - auto chassisForce = cosim::variable_id{chassisIndex, cosim::variable_type::real, 19}; - auto chassisVel = cosim::variable_id{chassisIndex, cosim::variable_type::real, 22}; - auto wheelCForce = cosim::variable_id{wheelIndex, cosim::variable_type::real, 26}; - auto wheelCVel = cosim::variable_id{wheelIndex, cosim::variable_type::real, 24}; - ecco_algo->add_power_bond(chassisVel, chassisForce, wheelCForce, wheelCVel); // chassis -> wheel (chassis port) + // Correct refs from FMU model descriptions: + // Chassis: velocity (FMU output) = ref 23, force (FMU input) = ref 4 + // Wheel: out_spring_damper_f (FMU output) = ref 15, in_vel (FMU input) = ref 7 + // inject_system_structure already registered the power bond from OspSystemStructure.xml; + // do NOT call add_power_bond again here. + auto chassisVel = cosim::variable_id{chassisIndex, cosim::variable_type::real, 23}; + auto chassisForce = cosim::variable_id{chassisIndex, cosim::variable_type::real, 4}; + auto wheelOutForce = cosim::variable_id{wheelIndex, cosim::variable_type::real, 15}; + auto wheelInVel = cosim::variable_id{wheelIndex, cosim::variable_type::real, 7}; auto file_obs = std::make_unique("./logDir", logXmlPath); execution.add_observer(std::move(file_obs)); @@ -71,9 +75,9 @@ int main() auto t_observer = std::make_shared(50000); execution.add_observer(t_observer); t_observer->start_observing(chassisVel); - t_observer->start_observing(wheelCVel); + t_observer->start_observing(wheelInVel); t_observer->start_observing(chassisForce); - t_observer->start_observing(wheelCForce); + t_observer->start_observing(wheelOutForce); auto csv_observer = std::make_shared("."); execution.add_observer(csv_observer); @@ -86,9 +90,7 @@ int main() t_observer->get_step_numbers(chassisVel.simulator, startTime, midTime, gsl::make_span(stepNums, 2)); const auto numSamples = stepNums[1] - stepNums[0]; std::vector chassisVels(numSamples); - std::vector wheelCVels(numSamples); - std::vector wheelGVels(numSamples); - std::vector groundVels(numSamples); + std::vector wheelInVels(numSamples); std::vector steps(numSamples); std::vector timeValues(numSamples); @@ -100,22 +102,20 @@ int main() gsl::make_span(steps), gsl::make_span(timeValues)); t_observer->get_real_samples( - wheelCVel.simulator, - wheelCVel.reference, + wheelInVel.simulator, + wheelInVel.reference, 0, - gsl::make_span(wheelCVels), + gsl::make_span(wheelInVels), gsl::make_span(steps), gsl::make_span(timeValues)); - std::cout << "time,step #,stepsize,chassisVel,wheelCVel,wheelGVel,groundVel" << std::endl; + std::cout << "time,step #,stepsize,chassisVel,wheelInVel" << std::endl; for (int i = 1; i < numSamples; ++i) { std::cout << cosim::to_double_time_point(timeValues[i]) << "," << steps[i] << "," << cosim::to_double_duration(timeValues[i] - timeValues[i - 1], timeValues[i - 1]) << "," << chassisVels[i] - << "," << wheelCVels[i] - << "," << wheelGVels[i] - << "," << groundVels[i] + << "," << wheelInVels[i] << std::endl; } } catch (const std::exception& e) { diff --git a/tests/ecco_algorithm_powerbond_logconfig_test.cpp b/tests/ecco_algorithm_powerbond_logconfig_test.cpp new file mode 100644 index 00000000..120ca687 --- /dev/null +++ b/tests/ecco_algorithm_powerbond_logconfig_test.cpp @@ -0,0 +1,83 @@ +/** + * Regression test for: crash "Variable with reference N not found in exposed variables" + * when using the ECCO algorithm with a file_observer config that omits power bond input + * variables (force/in_vel) from the logged variable list. + * + * Root cause: ecco_algorithm::impl::add_power_bond stored all four bond variable IDs but + * never called expose_for_getting on them. adjust_step_size then called get_real() on all + * four variables, which throws for any variable not previously exposed for getting. The + * default file_observer (no config) accidentally hid the bug by exposing ALL variables; + * a config-restricted observer only exposes the listed variables, triggering the crash for + * FMU-input variables (force, in_vel) not present in the log config. + */ + +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include + + +#define REQUIRE(test) \ + if (!(test)) throw std::runtime_error("Requirement not satisfied: " #test) + +int main() +{ + try { + const auto testDataDir = std::getenv("TEST_DATA_DIR"); + REQUIRE(testDataDir); + cosim::log::setup_simple_console_logging(); + cosim::log::set_global_output_level(cosim::log::info); + + constexpr cosim::time_point endTime = cosim::to_time_point(0.1); + + auto resolver = cosim::default_model_uri_resolver(); + const auto configPath = cosim::filesystem::path(testDataDir) / "fmi2" / "quarter_truck" / "OspSystemStructure.xml"; + const auto config = cosim::load_osp_config(configPath, *resolver); + + auto ecco_params = std::get(config.algorithm_configuration); + auto ecco_algo = std::make_shared(ecco_params); + + auto execution = cosim::execution(config.start_time, ecco_algo); + + // inject_system_structure registers the power bond (chassis <-> wheel) by calling + // ecco_algorithm::add_power_bond internally. + const auto entityMaps = cosim::inject_system_structure(execution, config.system_structure, config.initial_values); + REQUIRE(entityMaps.simulators.size() == 2); + + // Build a log config that intentionally omits the FMU-input bond variables: + // chassis.force (ref=4, FMU input / bond "output_a") + // wheel.in_vel (ref=7, FMU input / bond "output_b") + // Before the fix, adjust_step_size would call get_real() on these refs and throw + // "Variable with reference N not found in exposed variables" because no observer + // had called expose_for_getting on them. + cosim::file_observer_config log_config{}; + log_config.set_timestamped_filenames(false); + // Log only output variables for chassis — force (ref=4) is deliberately absent. + log_config.log_simulator_variables("chassis", {"position", "velocity"}); + // Log only output variables for wheel — in_vel (ref=7) is deliberately absent. + log_config.log_simulator_variables("wheel", {"position", "out_spring_damper_f", "velocity"}); + + auto file_obs = std::make_unique( + cosim::filesystem::current_path() / "powerbond_logconfig_logs", + log_config); + execution.add_observer(std::move(file_obs)); + + // This must complete without throwing. Before the fix it would crash with: + // "Variable with reference N not found in exposed variables" + auto simResult = execution.simulate_until(endTime); + REQUIRE(simResult); + + } catch (const std::exception& e) { + std::cerr << "Error: " << e.what() << std::endl; + return 1; + } + + return 0; +} diff --git a/tests/ecco_algorithm_two_bond_test.cpp b/tests/ecco_algorithm_two_bond_test.cpp new file mode 100644 index 00000000..8cee98d3 --- /dev/null +++ b/tests/ecco_algorithm_two_bond_test.cpp @@ -0,0 +1,71 @@ +/** + * Regression test for: add_power_bonds() spuriously throws + * "The number of powerbonds (1) is not equal to the number of unique power bond + * names (2)" when two bonds are configured. + * + * Root cause: the uniqueCount != numPowerBonds validation was placed inside the + * per-bond loop in add_power_bonds() (osp_config_parser.cpp). After the first + * bond is added, numPowerBonds == 1 but uniqueCount == 2 (computed up front from + * all connections), causing a spurious throw. The check must run after all bonds + * have been registered, i.e. outside the loop. + * + * This test uses two independent chassis+wheel pairs (OspSystemStructure_MultiBond.xml), + * each connected by a separate power bond. Before the fix, load_osp_config() throws + * on that file. After the fix it must load, simulate, and complete without error. + */ + +#include +#include +#include +#include +#include + +#include +#include +#include +#include + + +#define REQUIRE(test) \ + if (!(test)) throw std::runtime_error("Requirement not satisfied: " #test) + +int main() +{ + try { + const auto testDataDir = std::getenv("TEST_DATA_DIR"); + REQUIRE(testDataDir); + cosim::log::setup_simple_console_logging(); + cosim::log::set_global_output_level(cosim::log::info); + + constexpr cosim::time_point endTime = cosim::to_time_point(0.1); + + auto resolver = cosim::default_model_uri_resolver(); + const auto configPath = cosim::filesystem::path(testDataDir) / "fmi2" / "quarter_truck" / "OspSystemStructure_MultiBond.xml"; + + // Before the fix this throws: + // "The number of powerbonds (1) is not equal to the number of unique + // power bond names (2) ..." + // because the uniqueCount check fired inside the per-bond loop after + // only the first of the two bonds had been added. + const auto config = cosim::load_osp_config(configPath, *resolver); + + auto ecco_params = std::get(config.algorithm_configuration); + auto ecco_algo = std::make_shared(ecco_params); + + auto execution = cosim::execution(config.start_time, ecco_algo); + + const auto entityMaps = cosim::inject_system_structure( + execution, config.system_structure, config.initial_values); + + REQUIRE(entityMaps.simulators.size() == 4); // chassis1, wheel1, chassis2, wheel2 + + auto simResult = execution.simulate_until(endTime); + REQUIRE(simResult); + + } catch (const std::exception& e) { + std::cerr << "Error: " << e.what() << std::endl; + return 1; + } + + return 0; +}