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

Add moveit_configs_utils package to simplify loading paramters #591

Merged
merged 46 commits into from
Jan 21, 2022

Conversation

JafarAbdi
Copy link
Member

@JafarAbdi JafarAbdi commented Aug 3, 2021

Description

Second attempt to simplify loading parameters

This's based on JafarAbdi/moveit_resources@44a82c4

Replace: #209

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

Copy link
Contributor

@AdamPettinger AdamPettinger left a comment

Choose a reason for hiding this comment

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

Overall I like this. I think there are pros/cons about this style of implementation but this (as written now) is already better than the current way of doing the same configs setup in every launch file

@tylerjw
Copy link
Member

tylerjw commented Aug 4, 2021

I like the pattern here. I wonder if there is a way to make it more generic so the sytax was more like:

parameters = ParameterBuilder('robot_config_package')
  .yaml('moveit_cpp.yaml')                                        # loads a yaml file into parameters
  .file_parameter('robot_description', 'xacro/panda.urdf.xacro')  # fills out full path based on package
  .parameter('planning_timeout', 0.1)                             # sets a parameter
  .to_dict()

Then it could be used for any sort of parameters, not just a moveit config. Then maybe there could be a moveit config builder that used the ParameterBuilder and was just a function or something like that.

Copy link
Contributor

@vatanaksoytezer vatanaksoytezer left a comment

Choose a reason for hiding this comment

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

Overall I like it that it is much more smaller, to be used around moveit. What I didn't like was this assumes default locations for some files such as semantic, kinematic, joint_limits, which can be hard to understand if you are looking at this for the first time and not familiar with moveit. I feel making all the parameters yaml is great and at least for example-wise specifying file names here would be more instructive and easier to understand imo.

@AdamPettinger
Copy link
Contributor

I like the pattern here. I wonder if there is a way to make it more generic so the sytax was more like:

parameters = ParameterBuilder('robot_config_package')
  .yaml('moveit_cpp.yaml')                                        # loads a yaml file into parameters
  .file_parameter('robot_description', 'xacro/panda.urdf.xacro')  # fills out full path based on package
  .parameter('planning_timeout', 0.1)                             # sets a parameter
  .to_dict()

Then it could be used for any sort of parameters, not just a moveit config. Then maybe there could be a moveit config builder that used the ParameterBuilder and was just a function or something like that.

I like this a lot. It would let you do multiple inside one launch file (e.g. when getting parameters from multiple packages), and would also let you use it to return a specific set of all parameters for a config package (e.g. moveit_configs, or if some other user wants to have their own bringup/config package they could provide a getPackageConfigs() call)

moveit_configs_utils/setup.py Outdated Show resolved Hide resolved
moveit_configs_utils/package.xml Outdated Show resolved Hide resolved
@JafarAbdi JafarAbdi changed the title [WIP]: Use moveit_configs_utils for run_move_group Add moveit_configs_utils package to simplify loading paramters Aug 13, 2021
Copy link
Member

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

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

This is awesome, all launch files look so much cleaner! I'm just not sure if we should remove the default values from the builder functions.

Some thought: Let's say all moveit config packages provide python scripts that export their custom moveit configs. Couldn't we then simply build config hierarchies by taking another included moveit config as default and just modifying specific parts, say the kinematics yaml or something else? Given this PR it seems like a really small step to get this done. The only thing we need to watch out for is circular dependencies.

self.__moveit_configs.robot_description_kinematics = {
self.__robot_description
+ "_kinematics": load_yaml(
self._package_path / (file_path or "config/kinematics.yaml")
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps these relative path strings should be stored in class variables

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean config/kinematics.yaml right .? I'll update the class to have them

moveit_config.robot_description_semantic,
moveit_config.robot_description_kinematics,
moveit_config.planning_pipelines,
moveit_config.joint_limits,
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume rviz needs these parameters for the moveit planning panel correct?

Copy link
Member

Choose a reason for hiding this comment

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

Probably out of the scope of this PR but it would be really nice if the rviz plugin had the option of being pointed at the ros node that had the moveit config loaded in it (maybe even as something that could be changed in the gui) and it would use the get_parameters service that ros starts up to read the moveit config from that other node.

@jrgnicho
Copy link
Contributor

I really like what's on here too. Coincidentally I had started working on something similar in order to port the ros-industrial pick and place demo and ended up creating a MoveItConfigHelper python class that looks a lot like what's on here although it isn't as polished. Eventually I was going to pull it out of the launch file and provide it in its own stand-alone package but since that's already being done here then I'll just wait until this PR gets merged. One thing I did that I did not see here is to parse the list of ros controllers from the yaml and provide them as another field in the MoveItConfig. I then use the list to spawn the controllers; I feel this could be done here too.

moveit2_rolling.repos Outdated Show resolved Hide resolved
@JafarAbdi JafarAbdi force-pushed the pr-moveit_configs_utils branch 2 times, most recently from 42272f7 to 79ccfb4 Compare September 9, 2021 19:38
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.

Overall I really like this PR. I just left a couple of comments.

moveit2_galactic.repos Outdated Show resolved Hide resolved
moveit2_galactic.repos Outdated Show resolved Hide resolved
moveit_config.robot_description_semantic,
moveit_config.robot_description_kinematics,
moveit_config.planning_pipelines,
moveit_config.joint_limits,
Copy link
Member

Choose a reason for hiding this comment

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

Probably out of the scope of this PR but it would be really nice if the rviz plugin had the option of being pointed at the ros node that had the moveit config loaded in it (maybe even as something that could be changed in the gui) and it would use the get_parameters service that ros starts up to read the moveit config from that other node.

@JafarAbdi JafarAbdi force-pushed the pr-moveit_configs_utils branch 2 times, most recently from a0dc18a to acfdc5a Compare September 9, 2021 23:52
@JafarAbdi JafarAbdi marked this pull request as draft September 10, 2021 00:18
@JafarAbdi JafarAbdi marked this pull request as ready for review September 10, 2021 01:49
@codecov
Copy link

codecov bot commented Sep 10, 2021

Codecov Report

Merging #591 (28b6849) into main (4c75dcf) will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #591      +/-   ##
==========================================
+ Coverage   57.96%   57.98%   +0.02%     
==========================================
  Files         309      309              
  Lines       26120    26120              
==========================================
+ Hits        15138    15143       +5     
+ Misses      10982    10977       -5     
Impacted Files Coverage Δ
...nning_scene_monitor/src/planning_scene_monitor.cpp 53.90% <0.00%> (+0.59%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c75dcf...28b6849. Read the comment docs.

@mergify
Copy link

mergify bot commented Jan 1, 2022

This pull request is in conflict. Could you fix it @JafarAbdi?

Copy link
Contributor

@stephanie-eng stephanie-eng left a comment

Choose a reason for hiding this comment

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

Small nit - some comments reference robot_name_moveit_configs as the default package name, which caused me a bit of confusion.

self.__planning_scene_monitor = value

@property
def move_group_capabilities(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Add move group capabilities loader function

return self

def robot_description_semantic(
self, file_path: Optional[str] = None, mappings: dict = None
Copy link
Contributor

Choose a reason for hiding this comment

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

Support passing LaunchConfiguration as a mapping

AndyZe and others added 2 commits January 14, 2022 11:04
Co-authored-by: Stephanie Eng <stephanie-eng@users.noreply.github.com>
@AndyZe
Copy link
Member

AndyZe commented Jan 14, 2022

I just spot-checked this on some of the tutorials. It still works well 👍

We still haven't tested it on a dual-arm system due to some unexpected issues, but I don't want to block a great PR forever. We can always fix up whatever issues emerge.

@tylerjw tylerjw added backport-foxy Mergify label that triggers a PR backport to Foxy backport-galactic Mergify label that triggers a PR backport to Galactic and removed backport-foxy Mergify label that triggers a PR backport to Foxy labels Jan 21, 2022
@tylerjw tylerjw merged commit 79e3fd7 into moveit:main Jan 21, 2022
mergify bot pushed a commit that referenced this pull request Jan 21, 2022
Co-authored-by: AndyZe <zelenak@picknik.ai>
Co-authored-by: Stephanie Eng <stephanie-eng@users.noreply.github.com>
Co-authored-by: Tyler Weaver <tyler@picknik.ai>
(cherry picked from commit 79e3fd7)
@JafarAbdi JafarAbdi deleted the pr-moveit_configs_utils branch January 21, 2022 18:48
tylerjw added a commit that referenced this pull request Jan 21, 2022
…ort #591) (#1019)

Co-authored-by: AndyZe <zelenak@picknik.ai>
Co-authored-by: Stephanie Eng <stephanie-eng@users.noreply.github.com>
Co-authored-by: Tyler Weaver <tyler@picknik.ai>
@tylerjw tylerjw mentioned this pull request Jan 24, 2022
6 tasks
tylerjw added a commit to tylerjw/moveit2 that referenced this pull request Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-galactic Mergify label that triggers a PR backport to Galactic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet