Add logic to Ros2ControlManager to match ros2_control #3332

pac48 · 2025-02-10T18:54:38Z

Description

The logic for the chained controllers still does not match the expectation of ROS2 control. This PR makes a few changes:

Activation/deactivation is expected to be disjoint. For example, if controller B is a dependency of A (A chains to B) but controller B is also a dependency of C (B chains to B), then the switch from A->B to C->B would cause B to be in both the activation and deactivate list. This causes ROS2 control to through an error and reject the switch. The simplifyControllerActivationDeactivation function adds the logic needed to avoid this from happening.
The valid transitions for chained controllers depend asymmetrically on whether the controller is being started or stopped. If A chained to B, then when A is started, B should be automatically started. B is allowed to be started and stopped on its own if A is not running. However, if both A and B are running, B cannot be shut down alone, both must be shut down together or A alone.
Lastly, ROS 2 control should not be told to shut down controllers that are not running or start controllers that are already running. Doing so can cause an error. A check is added to only start controllers that are not already running.

codecov-commenter · 2025-02-10T19:53:14Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 98.01980% with 2 lines in your changes missing coverage. Please review.

Project coverage is 45.83%. Comparing base (7db0bd4) to head (3a4eae6).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...ontrol_interface/src/controller_manager_plugin.cpp	94.60%	2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3332      +/-   ##
==========================================
- Coverage   45.97%   45.83%   -0.13%     
==========================================
  Files         716      717       +1     
  Lines       62477    62574      +97     
  Branches     7564     7564              
==========================================
- Hits        28715    28675      -40     
- Misses      33594    33731     +137     
  Partials      168      168

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Paul Gesel <[email protected]>

sjahr

How can this be tested / can we add very basic unittesting to make this code more robust (not blocking this PR)?

The test failures are cause by flacky unittests.

moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp

Signed-off-by: Paul Gesel <[email protected]>

moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp

…ygen Signed-off-by: Paul Gesel <[email protected]>

…oveit2 into pr-more-controller-manager-logic

moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp

griswaldbrooks

Ok, done with comments about C++. I'm not qualified to review the functional aspects of this code.

Signed-off-by: Paul Gesel <[email protected]>

moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp

…ager_plugin.cpp Co-authored-by: Sebastian Castro <[email protected]>

Signed-off-by: Paul Gesel <[email protected]>

…oveit2 into pr-more-controller-manager-logic

sea-bass

LGTM -- I'll wait until this one is merged before releasing Jazzy/Rolling (sadly I already released Humble like a day or 2 ago)

sea-bass · 2025-02-12T03:58:14Z

Huh, somehow there are clang-tidy fixes being suggested

Signed-off-by: Paul Gesel <[email protected]>

sea-bass · 2025-02-12T17:00:34Z

@griswaldbrooks you may resolve your own comments now 🤣

* Add logic to Ros2ControlManager to match ros2_control Signed-off-by: Paul Gesel <[email protected]> * Add Ros2ControlManager test Signed-off-by: Paul Gesel <[email protected]> * move simplifyControllerActivationDeactivation to function and add doxygen Signed-off-by: Paul Gesel <[email protected]> * move queue.pop_back up Signed-off-by: Paul Gesel <[email protected]> * Update moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp Co-authored-by: Sebastian Castro <[email protected]> * Update moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp Co-authored-by: Sebastian Castro <[email protected]> * Update moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp Co-authored-by: Sebastian Castro <[email protected]> * pr feedback Signed-off-by: Paul Gesel <[email protected]> * clang fixes Signed-off-by: Paul Gesel <[email protected]> --------- Signed-off-by: Paul Gesel <[email protected]> Co-authored-by: Sebastian Castro <[email protected]> Co-authored-by: Sebastian Jahr <[email protected]> (cherry picked from commit dbf07b1) # Conflicts: # moveit_plugins/moveit_ros_control_interface/CMakeLists.txt # moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp

* Add logic to Ros2ControlManager to match ros2_control Signed-off-by: Paul Gesel <[email protected]> * Add Ros2ControlManager test Signed-off-by: Paul Gesel <[email protected]> * move simplifyControllerActivationDeactivation to function and add doxygen Signed-off-by: Paul Gesel <[email protected]> * move queue.pop_back up Signed-off-by: Paul Gesel <[email protected]> * Update moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp Co-authored-by: Sebastian Castro <[email protected]> * Update moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp Co-authored-by: Sebastian Castro <[email protected]> * Update moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp Co-authored-by: Sebastian Castro <[email protected]> * pr feedback Signed-off-by: Paul Gesel <[email protected]> * clang fixes Signed-off-by: Paul Gesel <[email protected]> --------- Signed-off-by: Paul Gesel <[email protected]> Co-authored-by: Sebastian Castro <[email protected]> Co-authored-by: Sebastian Jahr <[email protected]> (cherry picked from commit dbf07b1)

griswaldbrooks · 2025-02-12T22:33:15Z

How so?

sea-bass · 2025-02-12T22:52:54Z

How so?

Damn, you got double sniped. I'm sorry :(

(cherry picked from commit dbf07b1) Co-authored-by: Paul Gesel <[email protected]>

pac48 force-pushed the pr-more-controller-manager-logic branch from 42463c1 to 3dc2995 Compare February 10, 2025 20:26

pac48 marked this pull request as ready for review February 10, 2025 20:26

pac48 force-pushed the pr-more-controller-manager-logic branch from 3dc2995 to db1a7d8 Compare February 10, 2025 20:31

pac48 requested a review from dyackzan February 10, 2025 20:31

pac48 force-pushed the pr-more-controller-manager-logic branch 2 times, most recently from d67c296 to 8e7dd31 Compare February 10, 2025 20:49

pac48 mentioned this pull request Feb 10, 2025

Add hangar_sim PickNikRobotics/moveit_pro_example_ws#113

Open

pac48 requested a review from sjahr February 10, 2025 21:47

pac48 force-pushed the pr-more-controller-manager-logic branch 2 times, most recently from 618ec3a to 0164264 Compare February 10, 2025 23:17

Add logic to Ros2ControlManager to match ros2_control

33a9708

Signed-off-by: Paul Gesel <[email protected]>

pac48 force-pushed the pr-more-controller-manager-logic branch from 0164264 to 33a9708 Compare February 10, 2025 23:21

sjahr reviewed Feb 11, 2025

View reviewed changes

moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp Outdated Show resolved Hide resolved

pac48 requested a review from sjahr February 11, 2025 16:58

pac48 force-pushed the pr-more-controller-manager-logic branch from 1452fa3 to 9db62e5 Compare February 11, 2025 17:09

Add Ros2ControlManager test

704dd4e

Signed-off-by: Paul Gesel <[email protected]>

pac48 force-pushed the pr-more-controller-manager-logic branch from f060e31 to 704dd4e Compare February 11, 2025 17:22

Merge branch 'main' into pr-more-controller-manager-logic

49ed964

griswaldbrooks reviewed Feb 11, 2025

View reviewed changes

moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp Outdated Show resolved Hide resolved

griswaldbrooks reviewed Feb 11, 2025

View reviewed changes

moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp Outdated Show resolved Hide resolved

griswaldbrooks reviewed Feb 11, 2025

View reviewed changes

moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp Show resolved Hide resolved

pac48 added 2 commits February 11, 2025 15:52

move simplifyControllerActivationDeactivation to function and add dox…

6180eb6

…ygen Signed-off-by: Paul Gesel <[email protected]>

Merge branch 'pr-more-controller-manager-logic' of github.com:pac48/m…

31c6bd9

…oveit2 into pr-more-controller-manager-logic

pac48 requested a review from griswaldbrooks February 11, 2025 22:53

griswaldbrooks reviewed Feb 11, 2025

View reviewed changes

moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp Show resolved Hide resolved

griswaldbrooks suggested changes Feb 11, 2025

View reviewed changes

move queue.pop_back up

77da98f

Signed-off-by: Paul Gesel <[email protected]>

sea-bass reviewed Feb 12, 2025

View reviewed changes

sea-bass added the backport-humble Mergify label that triggers a PR backport to Humble label Feb 12, 2025

sea-bass added the backport-jazzy Mergify label that triggers a PR backport to Jazzy label Feb 12, 2025

pac48 and others added 5 commits February 11, 2025 18:31

Update moveit_plugins/moveit_ros_control_interface/src/controller_man…

2210564

…ager_plugin.cpp Co-authored-by: Sebastian Castro <[email protected]>

Update moveit_plugins/moveit_ros_control_interface/src/controller_man…

1fb2585

…ager_plugin.cpp Co-authored-by: Sebastian Castro <[email protected]>

Update moveit_plugins/moveit_ros_control_interface/src/controller_man…

90c315c

…ager_plugin.cpp Co-authored-by: Sebastian Castro <[email protected]>

pr feedback

a2570ad

Signed-off-by: Paul Gesel <[email protected]>

Merge branch 'pr-more-controller-manager-logic' of github.com:pac48/m…

4a568d1

…oveit2 into pr-more-controller-manager-logic

pac48 requested review from sea-bass and griswaldbrooks February 12, 2025 01:37

sea-bass approved these changes Feb 12, 2025

View reviewed changes

pac48 requested a review from sea-bass February 12, 2025 14:59

clang fixes

2d91d45

Signed-off-by: Paul Gesel <[email protected]>

pac48 force-pushed the pr-more-controller-manager-logic branch from 11dded3 to 2d91d45 Compare February 12, 2025 15:06

sjahr approved these changes Feb 12, 2025

View reviewed changes

Merge branch 'main' into pr-more-controller-manager-logic

3a4eae6

sjahr enabled auto-merge February 12, 2025 15:49

sea-bass approved these changes Feb 12, 2025

View reviewed changes

sjahr added this pull request to the merge queue Feb 12, 2025

Merged via the queue into moveit:main with commit dbf07b1 Feb 12, 2025
9 checks passed

mergify bot mentioned this pull request Feb 12, 2025

Add logic to Ros2ControlManager to match ros2_control (backport #3332) #3342

Open

mergify bot mentioned this pull request Feb 12, 2025

Add logic to Ros2ControlManager to match ros2_control (backport #3332) #3343

Merged

sea-bass pushed a commit that referenced this pull request Feb 12, 2025

Add logic to Ros2ControlManager to match ros2_control (#3332) (#3343)

7e1fc53

(cherry picked from commit dbf07b1) Co-authored-by: Paul Gesel <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add logic to Ros2ControlManager to match ros2_control #3332

Add logic to Ros2ControlManager to match ros2_control #3332

pac48 commented Feb 10, 2025 •

edited

Loading

codecov-commenter commented Feb 10, 2025 •

edited

Loading

sjahr left a comment

griswaldbrooks left a comment

sea-bass left a comment

sea-bass commented Feb 12, 2025

sea-bass commented Feb 12, 2025

griswaldbrooks commented Feb 12, 2025

sea-bass commented Feb 12, 2025

Add logic to Ros2ControlManager to match ros2_control #3332

Add logic to Ros2ControlManager to match ros2_control #3332

Conversation

pac48 commented Feb 10, 2025 • edited Loading

Description

codecov-commenter commented Feb 10, 2025 • edited Loading

Codecov Report

sjahr left a comment

Choose a reason for hiding this comment

griswaldbrooks left a comment

Choose a reason for hiding this comment

sea-bass left a comment

Choose a reason for hiding this comment

sea-bass commented Feb 12, 2025

sea-bass commented Feb 12, 2025

griswaldbrooks commented Feb 12, 2025

sea-bass commented Feb 12, 2025

pac48 commented Feb 10, 2025 •

edited

Loading

codecov-commenter commented Feb 10, 2025 •

edited

Loading