From 745ba7183c06908525a76958ef23fcdb7f861715 Mon Sep 17 00:00:00 2001 From: vedh1234 Date: Tue, 7 Apr 2026 19:43:03 +0530 Subject: [PATCH 1/5] fix: removed multi-hop from configure_controller --- controller_manager/src/controller_manager.cpp | 14 +------------- controller_manager/test/test_load_controller.cpp | 8 ++++---- 2 files changed, 5 insertions(+), 17 deletions(-) diff --git a/controller_manager/src/controller_manager.cpp b/controller_manager/src/controller_manager.cpp index 3510920e50..2e1a3f849e 100644 --- a/controller_manager/src/controller_manager.cpp +++ b/controller_manager/src/controller_manager.cpp @@ -1615,9 +1615,7 @@ controller_interface::return_type ControllerManager::configure_controller( auto controller = found_it->c; const auto & state = controller->get_lifecycle_state(); - if ( - state.id() == lifecycle_msgs::msg::State::PRIMARY_STATE_ACTIVE || - state.id() == lifecycle_msgs::msg::State::PRIMARY_STATE_FINALIZED) + if (state.id() != lifecycle_msgs::msg::State::PRIMARY_STATE_UNCONFIGURED) { RCLCPP_ERROR( get_logger(), "Controller '%s' can not be configured from '%s' state.", @@ -1625,16 +1623,6 @@ controller_interface::return_type ControllerManager::configure_controller( return controller_interface::return_type::ERROR; } - if (state.id() == lifecycle_msgs::msg::State::PRIMARY_STATE_INACTIVE) - { - RCLCPP_DEBUG( - get_logger(), "Controller '%s' is cleaned-up before configuring", controller_name.c_str()); - if (cleanup_controller(*found_it) != controller_interface::return_type::OK) - { - return controller_interface::return_type::ERROR; - } - } - // For cases, when the controller ends up in the unconfigured state from any other state cleanup_controller_exported_interfaces(*found_it); try diff --git a/controller_manager/test/test_load_controller.cpp b/controller_manager/test/test_load_controller.cpp index b30e2e0e46..bfc5e9d74e 100644 --- a/controller_manager/test/test_load_controller.cpp +++ b/controller_manager/test/test_load_controller.cpp @@ -238,7 +238,7 @@ TEST_P(TestLoadedControllerParametrized, inactive_controller_cannot_be_cleaned_u std::dynamic_pointer_cast(controller_if); size_t cleanup_calls = 0; test_controller->cleanup_calls = &cleanup_calls; - // Configure from inactive state: controller can no be cleaned-up + // Configure from inactive state: rejected because controller is not unconfigured test_controller->simulate_cleanup_failure = true; EXPECT_EQ(cm_->configure_controller(CONTROLLER_NAME_1), controller_interface::return_type::ERROR); ASSERT_EQ( @@ -262,15 +262,15 @@ TEST_P(TestLoadedControllerParametrized, inactive_controller_cannot_be_configure std::dynamic_pointer_cast(controller_if); size_t cleanup_calls = 0; test_controller->cleanup_calls = &cleanup_calls; - // Configure from inactive state + // Configure from inactive state: rejected because controller is not unconfigured test_controller->simulate_cleanup_failure = false; { ControllerManagerRunner cm_runner(this); - EXPECT_EQ(cm_->configure_controller(CONTROLLER_NAME_1), controller_interface::return_type::OK); + EXPECT_EQ(cm_->configure_controller(CONTROLLER_NAME_1), controller_interface::return_type::ERROR); } ASSERT_EQ( lifecycle_msgs::msg::State::PRIMARY_STATE_INACTIVE, controller_if->get_lifecycle_state().id()); - EXPECT_EQ(1u, cleanup_calls); + EXPECT_EQ(0u, cleanup_calls); } INSTANTIATE_TEST_SUITE_P( From 9ea1764890a3bd07587e6655f1908478a53a956b Mon Sep 17 00:00:00 2001 From: vedh1234 Date: Tue, 7 Apr 2026 20:06:49 +0530 Subject: [PATCH 2/5] chore: pre-commit run --- controller_manager/test/test_load_controller.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/controller_manager/test/test_load_controller.cpp b/controller_manager/test/test_load_controller.cpp index bfc5e9d74e..b0622fcb2f 100644 --- a/controller_manager/test/test_load_controller.cpp +++ b/controller_manager/test/test_load_controller.cpp @@ -266,7 +266,8 @@ TEST_P(TestLoadedControllerParametrized, inactive_controller_cannot_be_configure test_controller->simulate_cleanup_failure = false; { ControllerManagerRunner cm_runner(this); - EXPECT_EQ(cm_->configure_controller(CONTROLLER_NAME_1), controller_interface::return_type::ERROR); + EXPECT_EQ( + cm_->configure_controller(CONTROLLER_NAME_1), controller_interface::return_type::ERROR); } ASSERT_EQ( lifecycle_msgs::msg::State::PRIMARY_STATE_INACTIVE, controller_if->get_lifecycle_state().id()); From 0fc660dc50cf2668f92480a11ac01ca32f3abc96 Mon Sep 17 00:00:00 2001 From: Vedhas Talnikar Date: Thu, 9 Jul 2026 15:21:47 +0000 Subject: [PATCH 3/5] fix: address reviewer comments , merge tests and add comment --- controller_manager/src/controller_manager.cpp | 1 + .../test/test_load_controller.cpp | 40 +------------------ 2 files changed, 3 insertions(+), 38 deletions(-) diff --git a/controller_manager/src/controller_manager.cpp b/controller_manager/src/controller_manager.cpp index 2e1a3f849e..8fcf6b4938 100644 --- a/controller_manager/src/controller_manager.cpp +++ b/controller_manager/src/controller_manager.cpp @@ -1623,6 +1623,7 @@ controller_interface::return_type ControllerManager::configure_controller( return controller_interface::return_type::ERROR; } + // Remove any stale exported interfaces from a prior configure cycle before re-configuring. cleanup_controller_exported_interfaces(*found_it); try diff --git a/controller_manager/test/test_load_controller.cpp b/controller_manager/test/test_load_controller.cpp index b0622fcb2f..e7a4fe6b0e 100644 --- a/controller_manager/test/test_load_controller.cpp +++ b/controller_manager/test/test_load_controller.cpp @@ -221,57 +221,21 @@ TEST_P(TestLoadedControllerParametrized, can_not_start_finalized_controller) lifecycle_msgs::msg::State::PRIMARY_STATE_FINALIZED, controller_if->get_lifecycle_state().id()); } -TEST_P(TestLoadedControllerParametrized, inactive_controller_cannot_be_cleaned_up) +TEST_P(TestLoadedControllerParametrized, configure_controller_requires_unconfigured_state) { const auto test_param = GetParam(); EXPECT_EQ(cm_->configure_controller(CONTROLLER_NAME_1), controller_interface::return_type::OK); start_test_controller(test_param.strictness); - stop_test_controller(test_param.strictness); - ASSERT_EQ( lifecycle_msgs::msg::State::PRIMARY_STATE_INACTIVE, controller_if->get_lifecycle_state().id()); - std::shared_ptr test_controller = - std::dynamic_pointer_cast(controller_if); - size_t cleanup_calls = 0; - test_controller->cleanup_calls = &cleanup_calls; - // Configure from inactive state: rejected because controller is not unconfigured - test_controller->simulate_cleanup_failure = true; + // configure_controller must reject any state other than UNCONFIGURED EXPECT_EQ(cm_->configure_controller(CONTROLLER_NAME_1), controller_interface::return_type::ERROR); ASSERT_EQ( lifecycle_msgs::msg::State::PRIMARY_STATE_INACTIVE, controller_if->get_lifecycle_state().id()); - EXPECT_EQ(0u, cleanup_calls); -} - -TEST_P(TestLoadedControllerParametrized, inactive_controller_cannot_be_configured) -{ - const auto test_param = GetParam(); - - EXPECT_EQ(cm_->configure_controller(CONTROLLER_NAME_1), controller_interface::return_type::OK); - - start_test_controller(test_param.strictness); - - stop_test_controller(test_param.strictness); - ASSERT_EQ( - lifecycle_msgs::msg::State::PRIMARY_STATE_INACTIVE, controller_if->get_lifecycle_state().id()); - - std::shared_ptr test_controller = - std::dynamic_pointer_cast(controller_if); - size_t cleanup_calls = 0; - test_controller->cleanup_calls = &cleanup_calls; - // Configure from inactive state: rejected because controller is not unconfigured - test_controller->simulate_cleanup_failure = false; - { - ControllerManagerRunner cm_runner(this); - EXPECT_EQ( - cm_->configure_controller(CONTROLLER_NAME_1), controller_interface::return_type::ERROR); - } - ASSERT_EQ( - lifecycle_msgs::msg::State::PRIMARY_STATE_INACTIVE, controller_if->get_lifecycle_state().id()); - EXPECT_EQ(0u, cleanup_calls); } INSTANTIATE_TEST_SUITE_P( From 64405ef73f40971bcc3d594ab3edc63e35b4d0f3 Mon Sep 17 00:00:00 2001 From: Bence Magyar Date: Mon, 13 Jul 2026 15:06:00 +0100 Subject: [PATCH 4/5] docs: document strict configure_controller transition in migration and release notes --- doc/migration.rst | 7 +++++++ doc/release_notes.rst | 1 + 2 files changed, 8 insertions(+) diff --git a/doc/migration.rst b/doc/migration.rst index 19dd80b3d7..ba0eb7e7a1 100644 --- a/doc/migration.rst +++ b/doc/migration.rst @@ -127,6 +127,13 @@ ChainableControllerInterface controller_manager ****************** +* ``configure_controller`` now performs a single-step lifecycle transition and + only accepts controllers in the ``unconfigured`` state (`#3196 + `__). Previously an + ``inactive`` controller was implicitly cleaned up and then reconfigured. To + reconfigure an ``inactive`` controller, call ``cleanup_controller`` first and + then ``configure_controller``. + hardware_interface ****************** diff --git a/doc/release_notes.rst b/doc/release_notes.rst index a180ab9156..8244b32568 100644 --- a/doc/release_notes.rst +++ b/doc/release_notes.rst @@ -24,6 +24,7 @@ controller_manager * Added new ``cleanup_controller`` service to the controller manager to allow cleaning up controllers from external clients. (`#2414 `__) * Removed forwarding of the controller manager's ros arguments to the controllers via NodeOptions. (`#3016 `__) * The ``spawner`` now forwards all the parameter files parsed to the spawner node to the spawned controllers. This would support ``allow_substs`` approach. (`#3136 `__) +* ``configure_controller`` no longer implicitly cleans up an ``inactive`` controller before configuring; it now strictly requires the ``unconfigured`` state and returns an error otherwise. (`#3196 `__) hardware_interface ****************** From a5940d55bb1588d34b2bfc05516280cebfad4fcc Mon Sep 17 00:00:00 2001 From: Vedhas Talnikar Date: Thu, 30 Jul 2026 15:11:38 +0000 Subject: [PATCH 5/5] fix: failing test fix in services test --- .../test/test_controller_manager_srvs.cpp | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/controller_manager/test/test_controller_manager_srvs.cpp b/controller_manager/test/test_controller_manager_srvs.cpp index 7bd49797f9..6f1154a84e 100644 --- a/controller_manager/test/test_controller_manager_srvs.cpp +++ b/controller_manager/test/test_controller_manager_srvs.cpp @@ -629,6 +629,9 @@ TEST_F(TestControllerManagerSrvs, configure_controller_srv) rclcpp::Client::SharedPtr unload_client = srv_node->create_client( "test_controller_manager/unload_controller"); + rclcpp::Client::SharedPtr cleanup_client = + srv_node->create_client( + "test_controller_manager/cleanup_controller"); auto request = std::make_shared(); request->name = test_controller::TEST_CONTROLLER_NAME; @@ -651,9 +654,9 @@ TEST_F(TestControllerManagerSrvs, configure_controller_srv) lifecycle_msgs::msg::State::PRIMARY_STATE_INACTIVE, test_controller->get_lifecycle_state().id()); - // call configure again and check the state + it shouldn't throw any exception + // configure_controller must reject any state other than UNCONFIGURED result = call_service_and_wait(*client, request, srv_executor, true); - ASSERT_TRUE(result->ok); + ASSERT_FALSE(result->ok) << "Controller configured from inactive state: " << request->name; EXPECT_EQ(1u, cm_->get_loaded_controllers().size()); EXPECT_EQ( lifecycle_msgs::msg::State::PRIMARY_STATE_INACTIVE, @@ -701,9 +704,9 @@ TEST_F(TestControllerManagerSrvs, configure_controller_srv) lifecycle_msgs::msg::State::PRIMARY_STATE_INACTIVE, test_chainable_controller->get_lifecycle_state().id()); - // call configure again and check the state + it shouldn't throw any exception + // configure_controller must reject any state other than UNCONFIGURED result = call_service_and_wait(*client, request, srv_executor, true); - ASSERT_TRUE(result->ok); + ASSERT_FALSE(result->ok) << "Controller configured from inactive state: " << request->name; EXPECT_EQ(2u, cm_->get_loaded_controllers().size()); EXPECT_EQ( test_chainable_controller::TEST_CONTROLLER_NAME, @@ -715,6 +718,15 @@ TEST_F(TestControllerManagerSrvs, configure_controller_srv) lifecycle_msgs::msg::State::PRIMARY_STATE_INACTIVE, test_chainable_controller->get_lifecycle_state().id()); + // Cleanup to UNCONFIGURED so the duplicate-interface tests below can attempt configure + auto cleanup_request = + std::make_shared(); + cleanup_request->name = test_chainable_controller::TEST_CONTROLLER_NAME; + ASSERT_TRUE(call_service_and_wait(*cleanup_client, cleanup_request, srv_executor, true)->ok); + EXPECT_EQ( + lifecycle_msgs::msg::State::PRIMARY_STATE_UNCONFIGURED, + test_chainable_controller->get_lifecycle_state().id()); + // Now try to configure the chainable controller with duplicated command interfaces test_chainable_controller->set_command_interface_configuration(duplicated_chained_cmd_cfg); result = call_service_and_wait(*client, request, srv_executor, true);