-
Notifications
You must be signed in to change notification settings - Fork 523
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 in Moveit #1395
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1395 +/- ##
==========================================
- Coverage 61.55% 61.11% -0.43%
==========================================
Files 274 275 +1
Lines 24977 25239 +262
==========================================
+ Hits 15371 15422 +51
- Misses 9606 9817 +211
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the handling of the service response is so complicated, It makes me wonder if the message is designed the right way. If anything, I'd assume that other consumers (if there are any) would have to deal with the same kind of filtering. How about adding a flag to the request include_chained_controllers
and move the logic into ros2_control?
I wanted to propose something very similar. IMO we should not have logic of chained controllers here in MoveIt2, ros2_control should provide this in the most suitable format. I was thinking earlier about extending the topic, but was too focused on functionality getting working. @pac48 What are actual data you would like to get from ros2_control? What should be the proper format of information? Would you like to get a list of controllers in the chain, or something else? You basically need mapping between controller and interfaces, I am right? (I am just doing something similar for another feature) |
@destogl MoveIt2 creates a mapping from controller name to its claimed interface and uses it to validate whether a trajectory can be executed with the given controller. This assumption does not hold when there are chained controllers. Two possible solutions are:
|
I think we need a larger discussion on how to rework the MoveIt controller managers. I'll admit, I don't know anything about chained controllers at all. People who should be involved: @destogl @pac48 @mikeferguson @AndyZe @henningkayser @JafarAbdi @schornakj If we do rework the controller managers, it will break some functionality in MTC and therefore MoveIt Studio. But I think it needs to be done at some point, anyway. I think there are very few people (maybe none) who understand the difference between a SimpleControllerManager, MoveItMultiControllerManager, and MoveItControllerManager -- as an example of how the current system is difficult to use. @mikeferguson commented on the discussion here:
|
@AndyZe I think this particular part is separate from what I was commenting on - there are two different controller plugins in MoveIt2:
The simple controller manager is something I added almost a decade ago - originally as an alternative to the ros_control based thing. Somewhere along the way it seems that the architecture changed a bit maybe and now we actually use the simple one in parallel with ros2_control? (at least, that's what the MSA export seems to imply, but I haven't dug too far into it). And yes, it seems there is definitely confusion over what plugin to run - an example: #1394 (comment) |
Well, here are some issues I know about:
I guess the benefit of a controller mgr is the ability to control arm + gripper at the same time |
The mapping isn't really at the joint level (conceptually, although I believe it is doing a joint-level matching in our current implementations) - the mapping is really from planning groups -> controllers, a common robot might have something like:
In this case, each of those 7-dof arm joints is actually mapped to two different action interfaces, depending on which planning group was used. In theory, the MoveitSimpleControllerManager is the "minimal" implementation, and then the other ones implement ros2_control interfaces as well (I think the Multi basically adds support for mobile bases, right?) |
OK - thanks. Well, I think we should still have a discussion. To me, it sounds like we need to:
|
I think your perspective is a lot different from mine because I've never used humanoids. 👍 |
@mikeferguson are you aware of any reason why we couldn't handle all cases with the MoveItSimpleControllerManager? Assuming the necessary controllers are running |
I think this is not a realistic option. The MoveItSimpleControllerManager is hard-coded to handle just a few types of controllers, so it doesn't support situations where users have developed their own controllers. We shouldn't put ourselves into a situation where MoveIt has to explicitly enumerate each possible type of supported controller. |
One particular situation where the MoveItControllerManager is required is when multiple controllers can actuate the same joint group but cannot be running at the same time. This will come up more often as we use the admittance controller, since a user might only want to use that controller for a subset of the motion segments in a larger trajectory. |
I think you could end up with two controller managers:
At the moment, the simple controller does NOT support a mobile base AFAIK |
I made an issue on the ros2_control |
@mikeferguson @DLu what do you guys use for a controller manager for mobile robots? Grepping through the Hello Stretch codebase, it looks like the MoveItSimpleControllerManager is used. So the only mobile manipulator I'm aware of in ROS2 doesn't even use the MoveItMultiControllerManager. |
Ah, the MoveItMultiControllerManager is for multiple ros2_control nodes, not for mobile robots. OK, please disregard the previous question. https://github.com/ros-planning/moveit2/tree/main/moveit_plugins/moveit_ros_control_interface |
Currently, Moveit2 can not handle the new chained controllers format in ros2_control. The reason why is because Moveit2 creates a map from controllers to joint interfaces, but does not account for chained controllers. For example, see the diagram below
A mapping would be created from JTC to Chained Controller 1 and joint_4/pos, which would cause a failure if trying to plan with joints 1-3. My changes will convert the above structure to the one shown below.
Checklist