Return code of `perform_command_mode_switch` is discarded

[domire8]

ros2_control/controller_manager/src/controller_manager.cpp

Line 2891 in da81eb1

RCLCPP_ERROR(get_logger(), "Error while performing mode switch.");

I have a design question for the controller manager. Why is it not doing something with the return value of perform_command_mode_switch? I think from a user perspective, it is already hard to differentiate between prepare_command_mode_switch and perform_command_mode_switch and the fact that one return value does something and not the other is a bit confusing.

Could you give an example of how different tasks should be distributed between prepare and perform? What if something goes wrong with the real hardware on perform_command_mode_switch, it seems there is no way to stop the switch anymore?

Thanks a lot for your expertise

[saikishor]

Hello @domire8

You are absolutely right, it should fail the command switching if this happens, similar to how the prepare_command_switch handles

ros2_control/controller_manager/src/controller_manager.cpp

Lines 1973 to 1981 in da81eb1

	if (!resource_manager_->prepare_command_mode_switch(
	switch_params_.activate_command_interface_request,
	switch_params_.deactivate_command_interface_request))
	{
	message = "Could not switch controllers since prepare command mode switch was rejected.";
	RCLCPP_ERROR(get_logger(), "%s", message.c_str());
	clear_requests();
	return controller_interface::return_type::ERROR;
	}

In this condition, it needs to be smart

ros2_control/controller_manager/src/controller_manager.cpp

Lines 2887 to 2892 in da81eb1

	if (!resource_manager_->perform_command_mode_switch(
	switch_params_.activate_command_interface_request,
	switch_params_.deactivate_command_interface_request))
	{
	RCLCPP_ERROR(get_logger(), "Error while performing mode switch.");
	}

Thanks for reporting the issue.

[domire8]

Hey, thanks for your fast reply! I imagine this is not going to be super straight forward as the perform_command_mode_switch is called in a void function (manage_switch). Any ideas on how to do that? I'd be happy to contribute if you wish so.

[domire8]

@christophfroehlich Could you elaborate how #2760 fixes this bug? The only thing that happened is that the manage_switch is returned early but how does the ControllerManager know that this was indeed a failure?

[christophfroehlich]

I haven't linked the PR to this issue, @saikishor ?

[saikishor]

@domire8 This part of the code evaluates if the switch is done successfully or not and then it fails or marks succeeded

ros2_control/controller_manager/src/controller_manager.cpp

Lines 2070 to 2072 in efa1dec

	auto switch_result = evaluate_switch_result(
	resource_manager_, switch_params_.activate_request, switch_params_.deactivate_request,
	strictness, get_logger(), to, message);

[domire8]

Oh, I didn't see that, makes sense. Thanks for fixing this so quickly!