Update controller_manager_plugin to fix MoveIt-managed controller switching #785
Conversation
schornakj
force-pushed
the
pr-update-for-controller-manager-msgs
branch
from
9cfd185
to
ca7fc77
Compare
moveit_plugins/moveit_ros_control_interface/src/controller_manager_plugin.cpp
Show resolved
Hide resolved
|
@AndyZe since we were talking about potential deadlocks with mutexes in this code, I took a look through it and it was originally designed in a way where deadlocks won't happen. The functions that lock the mutex don't call each other, and if (for example) |
| for (const std::string& it : activate) | ||
| { | ||
| ControllersMap::iterator c = managed_controllers_.find(it); | ||
| if (c != managed_controllers_.end()) | ||
| { // controller belongs to this manager | ||
| request->start_controllers.push_back(c->second.name); | ||
| for (const auto& claimed_resource : c->second.claimed_interfaces) | ||
| for (const auto& required_resource : c->second.required_command_interfaces) |
There was a problem hiding this comment.
What if Controller A requires the position command interface for Joint 1 and Controller B requires the velocity command interface for Joint1. Will that be seen as a conflict here? I think it should be.
(Refer to the example here)
I think you will want to mark it as a conflict if both controllers have any command_interface for the same joint
There was a problem hiding this comment.
I investigated this, and when this function collects interface names they are modified here to remove the command type suffixes (/position, /velocity, etc.). This identifies conflicts between controllers that want to use different command types for the same joint.
I updated the PR to add a comment to be more detailed about what happens there. The implementation of the code is different than what I would have written (using std::transform to modify the names in-place through the return value from inserting into a map is a little unclear in my opinion) but in the interest of keeping this PR simple I won't change it here.
There was a problem hiding this comment.
Ok, that's fine with me. Thanks for the comment.
Codecov Report
@@ Coverage Diff @@
## main #785 +/- ##
==========================================
+ Coverage 54.88% 55.43% +0.56%
==========================================
Files 196 196
Lines 21359 21395 +36
==========================================
+ Hits 11720 11858 +138
+ Misses 9639 9537 -102
Continue to review full report at Codecov.
|
Description
This is a partial replacement for #731 that only includes the changes required to allow MoveIt-managed controller switching to work again while excluding changes to the behavior of the controller manager plugin.
controller_manager_msgsin ros2_control v1.1.0. The most significant change is that resources that are required but not claimed by the controller (for example, if the controller is not active) are now listed in a separate message field than resources that are actively claimed by the controller.scoped_lockin place ofunique_lock.Checklist