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

Pass along move_group_capabilities parameters #2587

Merged
merged 4 commits into from
Feb 19, 2024

Conversation

forrest-rm
Copy link
Contributor

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.

Copy link

codecov bot commented Dec 7, 2023

Codecov Report

Attention: 121 lines in your changes are missing coverage. Please review.

Comparison is base (d962501) 50.74% compared to head (2286851) 43.02%.
Report is 10 commits behind head on main.

Files Patch % Lines
...it_core/robot_state/test/robot_state_benchmark.cpp 0.00% 91 Missing ⚠️
...default_capabilities/get_group_urdf_capability.cpp 0.00% 23 Missing ⚠️
moveit_ros/tests/src/move_group_api_test.cpp 88.47% 3 Missing ⚠️
.../rdf_loader/include/moveit/rdf_loader/rdf_loader.h 0.00% 2 Missing ⚠️
...py/src/moveit/moveit_ros/moveit_cpp/moveit_cpp.cpp 0.00% 1 Missing ⚠️
...nning_scene_monitor/src/planning_scene_monitor.cpp 80.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@@ -228,6 +228,7 @@ def generate_move_group_launch(moveit_config):
move_group_params = [
moveit_config.to_dict(),
Copy link
Member

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?

Copy link
Contributor Author

@forrest-rm forrest-rm Dec 13, 2023

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.

Copy link
Contributor Author

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?

Copy link
Member

@henningkayser henningkayser Feb 9, 2024

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.

Copy link
Contributor Author

@forrest-rm forrest-rm Feb 15, 2024

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.

Copy link
Contributor

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?

@sjahr sjahr merged commit f47b4c0 into moveit:main Feb 19, 2024
11 of 12 checks passed
@forrest-rm
Copy link
Contributor Author

@sjahr, I created Issue #2694 to track a more integrated solution.

Is it possible to also back-port this to the humble and iron branches? I have tested this on humble.

@sjahr sjahr added backport-humble Mergify label that triggers a PR backport to Humble backport-iron Mergify label that triggers a PR backport to Iron labels Feb 20, 2024
mergify bot pushed a commit that referenced this pull request Feb 20, 2024
* Pass along move_group_capabilities parameters

* fix lint check

* Use move_group_capabilities as default launch argument

(cherry picked from commit f47b4c0)
mergify bot pushed a commit that referenced this pull request Feb 20, 2024
* Pass along move_group_capabilities parameters

* fix lint check

* Use move_group_capabilities as default launch argument

(cherry picked from commit f47b4c0)
Jakubivan added a commit to Jakubivan/moveit2 that referenced this pull request Mar 13, 2024
sjahr pushed a commit that referenced this pull request Apr 30, 2024
* 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 <39061824+forrest-rm@users.noreply.github.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 backport-iron Mergify label that triggers a PR backport to Iron
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants