-
Notifications
You must be signed in to change notification settings - Fork 574
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
Pass along move_group_capabilities parameters #2587
Pass along move_group_capabilities parameters #2587
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2587 +/- ##
==========================================
- Coverage 50.74% 43.02% -7.72%
==========================================
Files 392 692 +300
Lines 32553 56297 +23744
Branches 0 7272 +7272
==========================================
+ Hits 16517 24218 +7701
- Misses 16036 31917 +15881
- Partials 0 162 +162 ☔ View full report in Codecov by Sentry. |
@@ -228,6 +228,7 @@ def generate_move_group_launch(moveit_config): | |||
move_group_params = [ | |||
moveit_config.to_dict(), |
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.
This line should already include the capabilities. Could you verify this?
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.
The capabilities parameter is not currently included in the to_dict()
function:
https://github.com/ros-planning/moveit2/blob/8d2eb900d222f6609ee2588f6f5ebb97986635bd/moveit_configs_utils/moveit_configs_utils/moveit_configs_builder.py#L119C5-L119C23
I tried adding the capabilities parameter to the to_dict()
function but that didn't seem to work. It seems that the additional parameter needs to added it to move_group_params
after move_group_configuration
, but I'm not sure why.
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.
@henningkayser, did you have additional thoughts on this PR or are you able to approve it?
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.
sorry for late response... Params override each other, and move_group_configuration provides empty capabilities by default (lines 200 , 214). So I think the best solution would be to set the default to moveit_config.move_group_capabilities in line 200. That way, the launch file would still allow using the launch argument if necessary.
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.
@henningkayser, with a slight modification because moveit_config.move_group_capabilities is a Dict, your suggestion works. It still seems odd that the capabilities parameter is handled separately instead as part of moveit_config.to_dict() like the other moveit_config parameters.
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.
@forrest-rm You're right. Do you mind opening a brief issue so we can track fixing this in a more appropriate way in the future?
* Pass along move_group_capabilities parameters * fix lint check * Use move_group_capabilities as default launch argument (cherry picked from commit f47b4c0)
* Pass along move_group_capabilities parameters * fix lint check * Use move_group_capabilities as default launch argument (cherry picked from commit f47b4c0)
…oveit#2696)" This reverts commit 65970f0.
* Pass along move_group_capabilities parameters * fix lint check * Use move_group_capabilities as default launch argument (cherry picked from commit f47b4c0) Co-authored-by: Forrest Rogers-Marcovitz <[email protected]>
The move_group_capabilities is currently unused in launches.py even though it is listed as a parameter of MoveItConfigs.
This fix is necessary to support MoveIt Task Constructor (MTC). In the move_group.lauch.py generated by the MoveIt Setup Assistant, the following line should be added when needed by MTC:
moveit_config.move_group_capabilities = {"capabilities": "move_group/ExecuteTaskSolutionCapability"}
I am open to suggestions if there's a better way of handling the move_group_capabilities parameter or to add the previous note as a comment in the auto generated launch scripts.
This was tested on the main and humble branches.