Issue 759: This will add the cleanup service #1236

bailaC · 2023-12-18T13:42:57Z

bailaC · 2023-12-18T13:47:04Z

@destogl Please check the draft PR. I think the main logic of the transition is missing. I will check again at my end.

codecov · 2023-12-18T13:55:19Z

Codecov Report

Attention: Patch coverage is 33.33333% with 34 lines in your changes are missing coverage. Please review.

Project coverage is 47.45%. Comparing base (ff3177b) to head (4d4ed45).

❗ Current head 4d4ed45 differs from pull request most recent head 418e47a. Consider uploading reports for the commit 418e47a to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #1236       +/-   ##
===========================================
- Coverage   75.89%   47.45%   -28.45%     
===========================================
  Files          41       41               
  Lines        3352     3595      +243     
  Branches     1926     1962       +36     
===========================================
- Hits         2544     1706      -838     
- Misses        417      459       +42     
- Partials      391     1430     +1039

Flag	Coverage Δ
unittests	`47.45% <33.33%> (-28.45%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
.../include/controller_manager/controller_manager.hpp	`18.18% <ø> (-13.97%)`	⬇️
controller_manager/src/controller_manager.cpp	`38.91% <33.33%> (-30.46%)`	⬇️

... and 30 files with indirect coverage changes

saikishor

Hello @bailaC,

After looking roughly at the cleanup_controller service, it seems to me similar to the unload_controller service that exist in the controller manager.

ros2_control/controller_manager/src/controller_manager.cpp

Lines 601 to 661 in 89c8658

    
           controller_interface::return_type ControllerManager::unload_controller( 
        
             const std::string & controller_name) 
        
           { 
        
             std::lock_guard<std::recursive_mutex> guard(rt_controllers_wrapper_.controllers_lock_); 
        
             std::vector<ControllerSpec> & to = rt_controllers_wrapper_.get_unused_list(guard); 
        
             const std::vector<ControllerSpec> & from = rt_controllers_wrapper_.get_updated_list(guard); 
        
             // Transfers the active controllers over, skipping the one to be removed and the active ones. 
        
             to = from; 
        
             auto found_it = std::find_if( 
        
               to.begin(), to.end(), 
        
               std::bind(controller_name_compare, std::placeholders::_1, controller_name)); 
        
             if (found_it == to.end()) 
        
             { 
        
               // Fails if we could not remove the controllers 
        
               to.clear(); 
        
               RCLCPP_ERROR( 
        
                 get_logger(), 
        
                 "Could not unload controller with name '%s' because no controller with this name exists", 
        
                 controller_name.c_str()); 
        
               return controller_interface::return_type::ERROR; 
        
             } 
        
             auto & controller = *found_it; 
        
             if (is_controller_active(*controller.c)) 
        
             { 
        
               to.clear(); 
        
               RCLCPP_ERROR( 
        
                 get_logger(), "Could not unload controller with name '%s' because it is still active", 
        
                 controller_name.c_str()); 
        
               return controller_interface::return_type::ERROR; 
        
             } 
        
             if (controller.c->is_async()) 
        
             { 
        
               RCLCPP_DEBUG( 
        
                 get_logger(), "Removing controller '%s' from the list of async controllers", 
        
                 controller_name.c_str()); 
        
               async_controller_threads_.erase(controller_name); 
        
             } 
        
             RCLCPP_DEBUG(get_logger(), "Cleanup controller"); 
        
             // TODO(destogl): remove reference interface if chainable; i.e., add a separate method for 
        
             // cleaning-up controllers? 
        
             controller.c->get_node()->cleanup(); 
        
             executor_->remove_node(controller.c->get_node()->get_node_base_interface()); 
        
             to.erase(found_it); 
        
             // Destroys the old controllers list when the realtime thread is finished with it. 
        
             RCLCPP_DEBUG(get_logger(), "Realtime switches over to new controller list"); 
        
             rt_controllers_wrapper_.switch_updated_list(guard); 
        
             std::vector<ControllerSpec> & new_unused_list = rt_controllers_wrapper_.get_unused_list(guard); 
        
             RCLCPP_DEBUG(get_logger(), "Destruct controller"); 
        
             new_unused_list.clear(); 
        
             RCLCPP_DEBUG(get_logger(), "Destruct controller finished"); 
        
             RCLCPP_DEBUG(get_logger(), "Successfully unloaded controller '%s'", controller_name.c_str()); 
        
             return controller_interface::return_type::OK; 
        
           }

What do you think?

This reverts commit 9b8d702.

destogl

@bailaC can you please write tests for the newly added code.

saikishor

Apart from the following changes, I want to know your opinion @destogl @bailaC on the following

Does it make sense to call the service alone unconfigure_controller instead of cleanup_controller (We can leave the rest of the methods without renaming as internally it's fine)?. Because it might give more or less the basic idea of the service in the context of controllers.

saikishor · 2024-02-02T17:43:18Z

controller_manager/src/controller_manager.cpp

+      "Could not unload controller with name '%s' because no controller with this name exists",
+      controller_name.c_str());
+    return controller_interface::return_type::ERROR;
+  }


I believe the above cleanup controller call should come after this, if not you get the error message of controller not found from cleanup rather than the unload controller

saikishor · 2024-02-02T17:45:59Z

controller_manager/src/controller_manager.cpp

+
+  response->ok = cleanup_controller(request->name) == controller_interface::return_type::OK;
+
+  RCLCPP_DEBUG(


Print the cleanup finished message if the response of the clean_controller method is OK. If not, better to print an error or leave the earlier error from within the method

saikishor · 2024-02-02T17:49:33Z

controller_manager/test/test_controller_manager_srvs.cpp

+    cm_->get_loaded_controllers()[0].c->get_state().id());
+  result = call_service_and_wait(*client, request, srv_executor, true);
+  ASSERT_TRUE(result->ok) << "Controller cleaned in inactive state: " << request->name;
+  EXPECT_EQ(1u, cm_->get_loaded_controllers().size());


Suggested change

EXPECT_EQ(1u, cm_->get_loaded_controllers().size());

EXPECT_EQ(

lifecycle_msgs::msg::State::PRIMARY_STATE_UNCONFIGURED,

cm_->get_loaded_controllers()[0].c->get_state().id());

EXPECT_EQ(1u, cm_->get_loaded_controllers().size());

it's good to check that the transition is successful

If not, in future we might face a situation that it will be returned ok, but never transitioned

saikishor · 2024-02-02T17:50:17Z

controller_manager/test/test_controller_manager_srvs.cpp

+    cm_->get_loaded_controllers()[0].c->get_state().id());
+  result = call_service_and_wait(*client, request, srv_executor, true);
+  ASSERT_FALSE(result->ok) << "Controller can not be cleaned in active state: " << request->name;
+  EXPECT_EQ(1u, cm_->get_loaded_controllers().size());


Suggested change

EXPECT_EQ(1u, cm_->get_loaded_controllers().size());

EXPECT_EQ(

lifecycle_msgs::msg::State::PRIMARY_STATE_ACTIVE,

cm_->get_loaded_controllers()[0].c->get_state().id());

EXPECT_EQ(1u, cm_->get_loaded_controllers().size());

saikishor · 2024-02-02T17:51:44Z

controller_manager/test/test_controller_manager_srvs.cpp

+  // scenario: call the cleanup service when no controllers are loaded
+  // expected: it should return ERROR as no controllers will be found to cleanup
+  auto result = call_service_and_wait(*client, request, srv_executor);
+  ASSERT_FALSE(result->ok) << "Controller not loaded: " << request->name;


Suggested change

ASSERT_FALSE(result->ok) << "Controller not loaded: " << request->name;

EXPECT_EQ(

lifecycle_msgs::msg::State::PRIMARY_STATE_UNCONFIGURED,

cm_->get_loaded_controllers()[0].c->get_state().id());

ASSERT_FALSE(result->ok) << "Controller not loaded: " << request->name;

mergify · 2024-02-09T10:26:54Z

This pull request is in conflict. Could you fix it @bailaC?

mergify · 2024-04-29T12:56:25Z

This pull request is in conflict. Could you fix it @bailaC?

bailaC closed this Dec 18, 2023

bailaC reopened this Dec 18, 2023

saikishor reviewed Dec 18, 2023

View reviewed changes

bailaC mentioned this pull request Dec 21, 2023

Add cleanup_controller service and method to controller manager and also add its CLI script #759

Open

8 tasks

bailaC force-pushed the issue-759 branch from 1bfe623 to e48546c Compare January 14, 2024 09:30

bailaC added 3 commits January 22, 2024 14:33

Issue-759: Adding cleanup service

ae8b359

Revert "Issue-759: Adding cleanup service"

b204120

This reverts commit 9b8d702.

Issue-759: Adding cleanup service

85a8fe9

destogl force-pushed the issue-759 branch from e48546c to 85a8fe9 Compare January 22, 2024 13:33

Optimize cleanup controllers method.

bde0339

destogl reviewed Jan 22, 2024

View reviewed changes

bailaC added 3 commits January 25, 2024 09:51

Fixing format

2607b38

fixing pre-commit changes

fd1449b

Adding tests for cleanup service.

bcb013d

destogl marked this pull request as ready for review February 2, 2024 15:10

Merge branch 'master' into issue-759

4d4ed45

github-actions bot requested review from bmagyar, christophfroehlich, DasRoteSkelett, destogl, livanov93, moriarty and saikishor February 2, 2024 15:10

destogl mentioned this pull request Feb 2, 2024

check for state of the controller node before cleanup #1363

Merged

saikishor requested changes Feb 2, 2024

View reviewed changes

saikishor mentioned this pull request Feb 20, 2024

How to reconfigure controller #1033

Closed

Adding controller_manager tests for cleanup controller

6a6fe91

bailaC added 3 commits March 5, 2024 20:03

Removed unwanted file.

8dfefc9

Merge branch 'master' into issue-759

872c385

Correcting clang format and test case

418e47a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 759: This will add the cleanup service #1236

Issue 759: This will add the cleanup service #1236

bailaC commented Dec 18, 2023 •

edited

Loading

bailaC commented Dec 18, 2023

codecov bot commented Dec 18, 2023 •

edited

Loading

saikishor left a comment

destogl left a comment

saikishor left a comment •

edited

Loading

saikishor Feb 2, 2024

saikishor Feb 2, 2024

saikishor Feb 2, 2024

saikishor Feb 2, 2024

saikishor Feb 2, 2024

mergify bot commented Feb 9, 2024

mergify bot commented Apr 29, 2024

	controller_interface::return_type ControllerManager::unload_controller(
	const std::string & controller_name)
	{
	std::lock_guard<std::recursive_mutex> guard(rt_controllers_wrapper_.controllers_lock_);
	std::vector<ControllerSpec> & to = rt_controllers_wrapper_.get_unused_list(guard);
	const std::vector<ControllerSpec> & from = rt_controllers_wrapper_.get_updated_list(guard);

	// Transfers the active controllers over, skipping the one to be removed and the active ones.
	to = from;

	auto found_it = std::find_if(
	to.begin(), to.end(),
	std::bind(controller_name_compare, std::placeholders::_1, controller_name));
	if (found_it == to.end())
	{
	// Fails if we could not remove the controllers
	to.clear();
	RCLCPP_ERROR(
	get_logger(),
	"Could not unload controller with name '%s' because no controller with this name exists",
	controller_name.c_str());
	return controller_interface::return_type::ERROR;
	}

	auto & controller = *found_it;

	if (is_controller_active(*controller.c))
	{
	to.clear();
	RCLCPP_ERROR(
	get_logger(), "Could not unload controller with name '%s' because it is still active",
	controller_name.c_str());
	return controller_interface::return_type::ERROR;
	}
	if (controller.c->is_async())
	{
	RCLCPP_DEBUG(
	get_logger(), "Removing controller '%s' from the list of async controllers",
	controller_name.c_str());
	async_controller_threads_.erase(controller_name);
	}

	RCLCPP_DEBUG(get_logger(), "Cleanup controller");
	// TODO(destogl): remove reference interface if chainable; i.e., add a separate method for
	// cleaning-up controllers?
	controller.c->get_node()->cleanup();
	executor_->remove_node(controller.c->get_node()->get_node_base_interface());
	to.erase(found_it);

	// Destroys the old controllers list when the realtime thread is finished with it.
	RCLCPP_DEBUG(get_logger(), "Realtime switches over to new controller list");
	rt_controllers_wrapper_.switch_updated_list(guard);
	std::vector<ControllerSpec> & new_unused_list = rt_controllers_wrapper_.get_unused_list(guard);
	RCLCPP_DEBUG(get_logger(), "Destruct controller");
	new_unused_list.clear();
	RCLCPP_DEBUG(get_logger(), "Destruct controller finished");

	RCLCPP_DEBUG(get_logger(), "Successfully unloaded controller '%s'", controller_name.c_str());

	return controller_interface::return_type::OK;
	}


		response->ok = cleanup_controller(request->name) == controller_interface::return_type::OK;

		RCLCPP_DEBUG(

Issue 759: This will add the cleanup service #1236

Are you sure you want to change the base?

Issue 759: This will add the cleanup service #1236

Conversation

bailaC commented Dec 18, 2023 • edited Loading

bailaC commented Dec 18, 2023

codecov bot commented Dec 18, 2023 • edited Loading

Codecov Report

saikishor left a comment

Choose a reason for hiding this comment

destogl left a comment

Choose a reason for hiding this comment

saikishor left a comment • edited Loading

Choose a reason for hiding this comment

saikishor Feb 2, 2024

Choose a reason for hiding this comment

saikishor Feb 2, 2024

Choose a reason for hiding this comment

saikishor Feb 2, 2024

Choose a reason for hiding this comment

saikishor Feb 2, 2024

Choose a reason for hiding this comment

saikishor Feb 2, 2024

Choose a reason for hiding this comment

mergify bot commented Feb 9, 2024

mergify bot commented Apr 29, 2024

bailaC commented Dec 18, 2023 •

edited

Loading

codecov bot commented Dec 18, 2023 •

edited

Loading

saikishor left a comment •

edited

Loading