Skip to content
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.

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

Support chained controllers #1482

Merged
merged 42 commits into from
Sep 6, 2022
Merged

Conversation

pac48
Copy link
Contributor

@pac48 pac48 commented Aug 7, 2022

This PR adds support for ros2_control's chained controllers. Only simple chains are supported currently. That is, controller can only chain to at most one other controller. I also added logic to start and stop the chained controller in the correct order.

I created a demo repository under ros2_control with that uses the chnages made in this PR. Here is a link to the README

@pac48 pac48 changed the title Pr support chained controllers Support chained controllers Aug 7, 2022
@codecov
Copy link

codecov bot commented Aug 7, 2022

Codecov Report

Merging #1482 (f4278cb) into main (337416d) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1482      +/-   ##
==========================================
+ Coverage   51.10%   51.11%   +0.01%     
==========================================
  Files         380      380              
  Lines       31802    31802              
==========================================
+ Hits        16249    16252       +3     
+ Misses      15553    15550       -3     
Impacted Files Coverage Δ
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp 76.43% <0.00%> (+1.08%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@v4hn
Copy link
Contributor

v4hn commented Aug 9, 2022

Only simple chains are supported currently. That is, controller can only chain to at most one other controller

Why? It does not sound like it would be too difficult to extend it?
That being said I do wonder whether it isn't ros_control's responsibility to tell you which hardware it is responsible to on top of which other controller it feeds into...

@pac48
Copy link
Contributor Author

pac48 commented Aug 9, 2022

Only simple chains are supported currently. That is, controller can only chain to at most one other controller

Why? It does not sound like it would be too difficult to extend it? That being said I do wonder whether it isn't ros_control's responsibility to tell you which hardware it is responsible to on top of which other controller it feeds into...

The response from the ros2_control list controllers service does indicate which controllers are chained to which. The logic in this PR is really about taking that information and converting it to a format that MoveIt understands.

Regarding chaining multiple controllers, MoveIt's controller manager plugin assumes there exists a direct mapping from controllers to controllable interfaces. If the controllers are chained with only one connection, then it is straight forward to propagate the controllable interfaces to the beginning of the chain, which ultimately controls those interfaces. If branching occurs in the chain, then propagating the interfaces is ambiguous.

Lastly, I believe the ros2_control maintainers are working towards a simplified service interface to handle chained controllers. When that happens, handling the general case will be more straightforward.

@pac48 pac48 force-pushed the pr-support-chained-controllers branch from 4ef460a to 7ed2e6a Compare August 24, 2022 21:04
@pac48 pac48 force-pushed the pr-support-chained-controllers branch from 7ed2e6a to ce1bc0c Compare August 24, 2022 21:31
@pac48 pac48 force-pushed the pr-support-chained-controllers branch from 3b6a2cd to fa594a2 Compare August 24, 2022 23:29
Copy link
Member

@tylerjw tylerjw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One tiny nit. Otherwise, this looks good.

Copy link
Member

@AndyZe AndyZe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested it now, and also tested that it doesn't break the regular Joint Trajectory Controller. Once you rebase, we should be clear to merge it. :)

@AndyZe AndyZe merged commit 3db960a into moveit:main Sep 6, 2022
sea-bass pushed a commit to sea-bass/moveit2 that referenced this pull request Oct 18, 2022
* fix controller list if chained controllers exist

* add comments and clean code

* added additional comments

* fix formatting

* fix white space

* add const reference and chhnage variable name

* simplify logic to only  work with one layer chain

* Don't return false when not finding optional parameter

* Update moveit_ros/perception/pointcloud_octomap_updater/src/pointcloud_octomap_updater.cpp

Co-authored-by: AndyZe <andyz@utexas.edu>

* add debug information

* print controller names

* print controllers with not known type

* load controller dependencies

* start chained controllers in switch

* reverse order of activate controllers

* prevent stoppping controller twice

* revert all debug changes

* add ROS error if a controller chains to more than one

* use loop to index chained connections

* update ros_control

* add empty controller allocator for admittance controller

* fix plugin xml

* Update moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp

Co-authored-by: AndyZe <andyz@utexas.edu>

* Update moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp

Co-authored-by: AndyZe <andyz@utexas.edu>

* Update moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp

Co-authored-by: AndyZe <andyz@utexas.edu>

* Update moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp

Co-authored-by: AndyZe <andyz@utexas.edu>

* fix map indexing

* add comment

* Update moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp

Co-authored-by: Tyler Weaver <squirrel428@protonmail.com>

* Typos

Co-authored-by: JafarAbdi <cafer.abdi@gmail.com>
Co-authored-by: Jafar <jafar.uruc@gmail.com>
Co-authored-by: AndyZe <andyz@utexas.edu>
Co-authored-by: Vatan Aksoy Tezer <vatan@picknik.ai>
Co-authored-by: Tyler Weaver <squirrel428@protonmail.com>
Co-authored-by: AndyZe <zelenak@picknik.ai>
@tylerjw tylerjw added the backport-humble Mergify label that triggers a PR backport to Humble label Oct 18, 2022
mergify bot pushed a commit that referenced this pull request Oct 18, 2022
* fix controller list if chained controllers exist

* add comments and clean code

* added additional comments

* fix formatting

* fix white space

* add const reference and chhnage variable name

* simplify logic to only  work with one layer chain

* Don't return false when not finding optional parameter

* Update moveit_ros/perception/pointcloud_octomap_updater/src/pointcloud_octomap_updater.cpp

Co-authored-by: AndyZe <andyz@utexas.edu>

* add debug information

* print controller names

* print controllers with not known type

* load controller dependencies

* start chained controllers in switch

* reverse order of activate controllers

* prevent stoppping controller twice

* revert all debug changes

* add ROS error if a controller chains to more than one

* use loop to index chained connections

* update ros_control

* add empty controller allocator for admittance controller

* fix plugin xml

* Update moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp

Co-authored-by: AndyZe <andyz@utexas.edu>

* Update moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp

Co-authored-by: AndyZe <andyz@utexas.edu>

* Update moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp

Co-authored-by: AndyZe <andyz@utexas.edu>

* Update moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp

Co-authored-by: AndyZe <andyz@utexas.edu>

* fix map indexing

* add comment

* Update moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp

Co-authored-by: Tyler Weaver <squirrel428@protonmail.com>

* Typos

Co-authored-by: JafarAbdi <cafer.abdi@gmail.com>
Co-authored-by: Jafar <jafar.uruc@gmail.com>
Co-authored-by: AndyZe <andyz@utexas.edu>
Co-authored-by: Vatan Aksoy Tezer <vatan@picknik.ai>
Co-authored-by: Tyler Weaver <squirrel428@protonmail.com>
Co-authored-by: AndyZe <zelenak@picknik.ai>
(cherry picked from commit 3db960a)
henningkayser pushed a commit that referenced this pull request Oct 28, 2022
(cherry picked from commit 3db960a)

Co-authored-by: Paul Gesel <paulgesel@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-humble Mergify label that triggers a PR backport to Humble
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants