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 new msa planners #2955

Merged
merged 11 commits into from
Jan 7, 2022
Merged

Add new msa planners #2955

merged 11 commits into from
Jan 7, 2022

Conversation

rickstaa
Copy link
Contributor

@rickstaa rickstaa commented Nov 11, 2021

This pull request updates the MSA such that it:

  • Creates the config and launch files for the STOMP planner 97a8c5d.
  • Creates the config and launch files for the CHOMP-STOMP planning pipeline b28da2d.
  • Only uses required parameters in the CHOMP configuration 504d9fe.
  • Uses the right values for the CHOMP parameters 40f5b49.

It further also removes deprecated code from the CHOMP planner.

The output of the MSA can be found in this repository (Please NOTE that this config uses all the pull requests that are listed in moveit/panda_moveit_config#96).

@rickstaa rickstaa marked this pull request as draft November 11, 2021 08:56
@rickstaa rickstaa force-pushed the add_new_msa_planners branch 4 times, most recently from 3f34344 to b28da2d Compare November 11, 2021 09:19
@rickstaa rickstaa marked this pull request as ready for review November 11, 2021 09:57
@codecov
Copy link

codecov bot commented Nov 11, 2021

Codecov Report

Merging #2955 (3612369) into master (fd36674) will increase coverage by 0.12%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2955      +/-   ##
==========================================
+ Coverage   61.55%   61.67%   +0.12%     
==========================================
  Files         370      374       +4     
  Lines       33145    33261     +116     
==========================================
+ Hits        20399    20509     +110     
- Misses      12746    12752       +6     
Impacted Files Coverage Δ
...ners/chomp/chomp_interface/src/chomp_interface.cpp 93.34% <ø> (ø)
...er/include/chomp_motion_planner/chomp_parameters.h 100.00% <ø> (ø)
...homp/chomp_motion_planner/src/chomp_parameters.cpp 84.85% <ø> (ø)
.../moveit/setup_assistant/tools/moveit_config_data.h 44.45% <ø> (ø)
...t_setup_assistant/src/tools/moveit_config_data.cpp 13.93% <0.00%> (-1.46%) ⬇️
...moveit/move_group_interface/move_group_interface.h 36.85% <0.00%> (-9.58%) ⬇️
...t_ros/robot_interaction/src/locked_robot_state.cpp 96.88% <0.00%> (-3.12%) ⬇️
moveit_core/robot_state/src/robot_state.cpp 52.36% <0.00%> (-0.19%) ⬇️
.../move_group_interface/src/move_group_interface.cpp 47.66% <0.00%> (-0.04%) ⬇️
...it/collision_detection/test_collision_common_pr2.h 98.12% <0.00%> (-0.04%) ⬇️
... and 20 more

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 fd36674...3612369. Read the comment docs.

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. Please use templates instead of source code to generate files (see below).

@rickstaa
Copy link
Contributor Author

@rhaschke I will update the stomp config generation so that it includes the right planning groups tomorrow. I think after that this commit is ready to be merged.

rickstaa and others added 7 commits November 30, 2021 23:17
This commit removes several redundant parameters that were left after
6114fe5.
…unch/stomp_planning_pipeline.launch.xml

Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com>
…unch/ompl-chomp_planning_pipeline.launch.xml

Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com>
This commit replaces the `chomp_config.yaml` injection method by a
simple template. This was done since the `chomp_config.yaml` file does
not contain any variables that need to be dynamically generated.
This commit makes sure that the STOMP planning config that is generated
using the MSA is general. The old version created the Panda Emika Franka
stomp config found in the
[panda_moveit_config](https://github.com/ros-planning/panda_moveit_config/blob/noetic-devel/config/stomp_planning.yaml)
package.
@rickstaa rickstaa marked this pull request as ready for review December 7, 2021 11:56
@rickstaa
Copy link
Contributor Author

rickstaa commented Dec 7, 2021

@rhaschke This pull request is finished. I now made sure the STOMP config works with all robots instead of creating the Panda Stomp config (i.e. 87e0b87). I further cleaned up the CHOMP configuration file creation (70a24a0). I, however, found some problems when trying out the STOMP algorithm. Would you mind checking my comments made on this pull request before merging (i.e. comments regarding default values)?

Stomp problems

  • The STOMP algorithm does not work with the hand. I am not familiar with the stomp algorithm, so I am not sure if it supports the parallel yaw gripper; when I try controlling this gripper, MoveIt crashes.
  • The STOMP algorithm seems to reach the planning timeout but still can plan. The following console logs are shown:

image

@@ -52,14 +52,9 @@ void CHOMPInterface::loadParams()
nh_.param("obstacle_cost_weight", params_.obstacle_cost_weight_, 1.0);
nh_.param("learning_rate", params_.learning_rate_, 0.01);

// nh_.param("add_randomness", params_.add_randomness_, false);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please double-check if these commented parameters can really be removed or that they will be enabled again in the future. The code that uses them was removed in 6114fe5 by
@raghavendersahdev. If they are needed we can drop 504d9fe.

moveit_setup_assistant/src/tools/moveit_config_data.cpp Outdated Show resolved Hide resolved
moveit_setup_assistant/src/tools/moveit_config_data.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

I generally approve. Just a small nitpick:

…unch/stomp_planning_pipeline.launch.xml

Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com>
@rickstaa
Copy link
Contributor Author

rickstaa commented Jan 7, 2022

@rhaschke Thanks for reviewing this pull request. Did you or @raghavendersahdev verify that the code removal I did in 6c930d7 is justified?

@rhaschke
Copy link
Contributor

rhaschke commented Jan 7, 2022

Did you or @raghavendersahdev verify that the code removal I did in 6c930d7 is justified?

As you said, the corresponding code was commented out a long time ago. I think it's good to have this cleanup.

@rhaschke rhaschke merged commit ea86018 into moveit:master Jan 7, 2022
@rickstaa rickstaa deleted the add_new_msa_planners branch January 7, 2022 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants