Skip to content

Make configure_controller lifecycle transition strict (no multi-hop) #3196

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.

Sign up for GitHub

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

Open
vedh1234 wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Make configure_controller lifecycle transition strict (no multi-hop) #3196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
745ba71
fix: removed multi-hop from configure_controller
vedh1234 Apr 7, 2026
9ea1764
chore: pre-commit run
vedh1234 Apr 7, 2026
0fc660d
fix: address reviewer comments , merge tests and add comment
vedh1234 Jul 9, 2026
64405ef
docs: document strict configure_controller transition in migration an…
bmagyar Jul 13, 2026
ccb86f3
Merge branch 'master' into fix/strict-controller-transitions
bmagyar Jul 13, 2026
5201f7d
Merge branch 'ros-controls:master' into fix/strict-controller-transit…
vedh1234 Jul 30, 2026
a5940d5
fix: failing test fix in services test
vedh1234 Jul 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 2 additions & 13 deletions controller_manager/src/controller_manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1615,26 +1615,15 @@ 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.",
controller_name.c_str(), state.label().c_str());
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
// Remove any stale exported interfaces from a prior configure cycle before re-configuring.
cleanup_controller_exported_interfaces(*found_it);
Comment thread
bmagyar marked this conversation as resolved.

try
Expand Down
20 changes: 16 additions & 4 deletions controller_manager/test/test_controller_manager_srvs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -629,6 +629,9 @@ TEST_F(TestControllerManagerSrvs, configure_controller_srv)
rclcpp::Client<controller_manager_msgs::srv::UnloadController>::SharedPtr unload_client =
srv_node->create_client<controller_manager_msgs::srv::UnloadController>(
"test_controller_manager/unload_controller");
rclcpp::Client<controller_manager_msgs::srv::CleanupController>::SharedPtr cleanup_client =
srv_node->create_client<controller_manager_msgs::srv::CleanupController>(
"test_controller_manager/cleanup_controller");

auto request = std::make_shared<controller_manager_msgs::srv::ConfigureController::Request>();
request->name = test_controller::TEST_CONTROLLER_NAME;
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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<controller_manager_msgs::srv::CleanupController::Request>();
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);
Expand Down
39 changes: 2 additions & 37 deletions controller_manager/test/test_load_controller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,56 +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::TestController> test_controller =
std::dynamic_pointer_cast<test_controller::TestController>(controller_if);
size_t cleanup_calls = 0;
test_controller->cleanup_calls = &cleanup_calls;
// Configure from inactive state: controller can no be cleaned-up
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::TestController> test_controller =
std::dynamic_pointer_cast<test_controller::TestController>(controller_if);
size_t cleanup_calls = 0;
test_controller->cleanup_calls = &cleanup_calls;
// Configure from inactive state
test_controller->simulate_cleanup_failure = false;
{
ControllerManagerRunner cm_runner(this);
EXPECT_EQ(cm_->configure_controller(CONTROLLER_NAME_1), controller_interface::return_type::OK);
}
ASSERT_EQ(
lifecycle_msgs::msg::State::PRIMARY_STATE_INACTIVE, controller_if->get_lifecycle_state().id());
EXPECT_EQ(1u, cleanup_calls);
}

INSTANTIATE_TEST_SUITE_P(
Expand Down
7 changes: 7 additions & 0 deletions doc/migration.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
<https://github.com/ros-controls/ros2_control/pull/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
******************

Expand Down
1 change: 1 addition & 0 deletions doc/release_notes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ controller_manager
* Added new ``cleanup_controller`` service to the controller manager to allow cleaning up controllers from external clients. (`#2414 <https://github.com/ros-controls/ros2_control/pull/2414>`__)
* Removed forwarding of the controller manager's ros arguments to the controllers via NodeOptions. (`#3016 <https://github.com/ros-controls/ros2_control/pull/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 <https://github.com/ros-controls/ros2_control/pull/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 <https://github.com/ros-controls/ros2_control/pull/3196>`__)

hardware_interface
******************
Expand Down
Loading