Add cleanup_controller lifecycle transition

[soham2560]

Brief

This PR completes #1236 by @bailaC which was started to fix #759.

TODOs

• Address previous discussions on original PR
  • Port over suggestions from original PR refer 3b5f695
  • Change only service name to unconfigure_controller as discussed with @saikishor and @bmagyar refer 0e86448
  • Add documentation refer bf7418c

[codecov]

Codecov Report

❌ Patch coverage is 91.35802% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.40%. Comparing base (c993e5b) to head (ec00352).
⚠️ Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
controller_manager/src/controller_manager.cpp	84.00%	1 Missing and 3 partials ⚠️
.../controller_manager/controller_manager_services.py	25.00%	3 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2414   +/-   ##
=======================================
  Coverage   89.40%   89.40%           
=======================================
  Files         151      152    +1     
  Lines       17024    17104   +80     
  Branches     1420     1424    +4     
=======================================
+ Hits        15220    15292   +72     
- Misses       1250     1257    +7     
- Partials      554      555    +1     

Flag	Coverage Δ
unittests	89.40% <91.35%> (+<0.01%)	⬆️

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

Files with missing lines	Coverage Δ
controller_manager/controller_manager/__init__.py	100.00% <ø> (ø)
.../include/controller_manager/controller_manager.hpp	97.36% <ø> (ø)
...ontroller_manager/test/test_cleanup_controller.cpp	100.00% <100.00%> (ø)
...ller_manager/test/test_controller_manager_srvs.cpp	99.33% <100.00%> (+0.01%)	⬆️
.../controller_manager/controller_manager_services.py	81.17% <25.00%> (-1.36%)	⬇️
controller_manager/src/controller_manager.cpp	76.67% <84.00%> (+0.13%)	⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[soham2560]

For the suggestions from #1236 refer 3b5f695

[christophfroehlich]

Why naming the transition unconfigure? Usually we try to stick to the ROS 2 lifecycle nomenclature, there is no unconfigure?
https://design.ros2.org/articles/node_lifecycle.html

[soham2560]

Why naming the transition unconfigure? Usually we try to stick to the ROS 2 lifecycle nomenclature, there is no unconfigure?
https://design.ros2.org/articles/node_lifecycle.html

This had come out of a conversation I had with @saikishor and @bmagyar some weeks back. @saikishor 's opinion was that unconfigure is a much more intuitive transition name and preferred for human facing interfaces which makes sense to me too(hence change is only for verbs and services) and @bmagyar also agreed that maybe we should make the change across the board (like in rqt controller manager, as you might remember we had a similar discussion at #2151) and maybe not remain tied to the node lifecycle naming scheme.

They might be able to much better explain their own views, but that was the reasoning at the time that convinced me to go for it

[saikishor]

Why naming the transition unconfigure? Usually we try to stick to the ROS 2 lifecycle nomenclature, there is no unconfigure?
https://design.ros2.org/articles/node_lifecycle.html

This had come out of a conversation I had with @saikishor and @bmagyar some weeks back. @saikishor 's opinion was that unconfigure is a much more intuitive transition name and preferred for human facing interfaces which makes sense to me too(hence change is only for verbs and services) and @bmagyar also agreed that maybe we should make the change across the board (like in rqt controller manager, as you might remember we had a similar discussion at #2151) and maybe not remain tied to the node lifecycle naming scheme.

They might be able to much better explain their own views, but that was the reasoning at the time that convinced me to go for it

@christophfroehlich if needed, we can discuss this at PMC meeting

[destogl]

Why naming the transition unconfigure? Usually we try to stick to the ROS 2 lifecycle nomenclature, there is no unconfigure? https://design.ros2.org/articles/node_lifecycle.html

In the node lifecycle naming, there is even no such transition. So I think this is fine as it doesn't collide with anything.

[christophfroehlich]

In the node lifecycle naming, there is even no such transition. So I think this is fine as it doesn't collide with anything.

I'm not sure if I understand: The proposal here is to transit from inactive state to unconfigured, this transition exists and is called cleanup?

[soham2560]

we decided in todays PMC meet to go back to cleanup, have made relevant changes at c5c3a13 and
40cf540

[christophfroehlich]

+1

[saikishor]

Just some minor nitpicks

[saikishor]

Suggested change

	if (cleanup_controller(controller_name) != controller_interface::return_type::OK)
	{
	return controller_interface::return_type::ERROR;
	}
	if (is_controller_inactive(*controller.c) && (cleanup_controller(controller_name) != controller_interface::return_type::OK))
	{
	return controller_interface::return_type::ERROR;
	}

Should we check if it is inactive before doing the cleanup? I mean it would also be in unconfigured state right?

[saikishor]

I see that you are also doing similar check in the cleanup_controller, if this is the case, I'm okay to remove here