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

Fixes chomp config #4

Merged
merged 3 commits into from
Oct 13, 2021

Conversation

rickstaa
Copy link

@rickstaa rickstaa commented Oct 8, 2021

@rhaschke I added this pull request as a draft since I first wanted to double-check some things. In moveit#74 (comment) you mention:

If the existing files differ to the MSA-generated ones: Keep the generated ones, if generic files (e.g. planning pipeline settings) are concerned. But keep robot-specific settings like joint limits etc. For those, MSA can only use the defaults from the URDF.

  • Is it correct that based on this you want to drop commit 1615e0b. Or do we need to update the MSA?

  • The CHOMP planner only work with the robot arm and not the hand. As I'm not familiar enough with this planner's inner workings, do you know if it is supposed to also work with the hand? The same holds for the LERP planner. When I try to use the CHOMP planner with the hand I get the following error message:

[/move_group] [ INFO] [WallTime: 1633678064.212043032]: Combined planning and execution request received for MoveGroup action. Forwarding to planning and execution pipeline.
[/move_group] [ INFO] [WallTime: 1633678064.212578732]: Planning attempt 1 of at most 1
[/move_group] [ INFO] [WallTime: 1633678064.212724784]: Using planning pipeline 'chomp'
[/move_group] [ INFO] [WallTime: 1633678064.403040981]: CHOMP trajectory initialized using method: quintic-spline
[/move_group] [ INFO] [WallTime: 1633678064.403127845]: The following collision detectors are available in the planning scene.
[/move_group] [ INFO] [WallTime: 1633678064.403147780]: HYBRID
[/move_group] [ INFO] [WallTime: 1633678064.403163239]: Active collision detector is: HYBRID
[/move_group] [ INFO] [WallTime: 1633678064.486460539]: First coll check took 0.083258319
[move_group-5] process has died [pid 47290, exit code -11, cmd /opt/ros/noetic/lib/moveit_ros_move_group/move_group --debug __name:=move_group __log:=/home/ricks/.ros/log/2c9fa844-2809-11ec-94ed-5fa82dc7799c/move_group-5.log].
log file: /home/ricks/.ros/log/2c9fa844-2809-11ec-94ed-5fa82dc7799c/move_group-5*.log

When I look in the log directory, the move_group-5*.log log file does not seem to be written. If the CHOMP planner is supposed to work with the hand, I think we can merge this pull request and open an upstream issue.

@rickstaa rickstaa marked this pull request as draft October 8, 2021 07:06
@rickstaa rickstaa force-pushed the fixes_chomp_config branch from 662941c to 46ca8e7 Compare October 8, 2021 07:15
@rickstaa rickstaa force-pushed the fixes_chomp_config branch from 46ca8e7 to 70f6a55 Compare October 8, 2021 07:21
@rickstaa rickstaa mentioned this pull request Oct 8, 2021
@rickstaa rickstaa marked this pull request as ready for review October 8, 2021 08:02
@rhaschke rhaschke merged commit 4c390ac into rhaschke:noetic-devel-rickstaa Oct 13, 2021
@rickstaa
Copy link
Author

@rhaschke Thanks for merging this. Just to really be sure (i.e., for my migration guide). In this case we had to overwrite the config file from the MSA with the one used in the melodic branch?

Do we need to update the MSA or these values left out on purpose to keep the MSA general?

@rickstaa rickstaa deleted the fixes_chomp_config branch October 14, 2021 08:10
@rhaschke
Copy link
Owner

The previous config file obviously has some more config params. I'm not really sure, they are actually still used. I didn't check.
Can you check CHOMP code for them?

@rickstaa
Copy link
Author

rickstaa commented Oct 14, 2021

@rhaschke I looked at the codebase and below are my results:

Parameter Result
ridge_factor We should change this to 0.0 in the MSA since this is the CHOMP default. I am not sure why it is now set to 0.1.
add_randomness, hmc_discretization, hmc_annealing_factor, use_hamiltonian_monte_carlo Have been commented out since 6114fe5eb7773f81dfdb04f43f30ad35dd251739.
animate_path, animate_endeffector, animate_endeffector_segment, random_jump_amount Were removed in 6114fe5eb7773f81dfdb04f43f30ad35dd251739.
enable_failure_recovery We should change this to false in the MSA, since this is the CHOMP default. It is now set to true.

It looks like you were right. A lot of these parameters can be removed since they are not used any more.

Some of them however are commented out in the code base (see https://github.com/ros-planning/moveit/blob/d57721cf8503fe893347196cc1e1f9249ec57777/moveit_planners/chomp/chomp_motion_planner/include/chomp_motion_planner/chomp_parameters.h#L81-L88). I'm not sure why they were commented out in 6114fe5eb7773f81dfdb04f43f30ad35dd251739 instead of being removed. Maybe there were plans to add them back in when a bug was fixed? It looks like you and @raghavendersahdev worked on this in 2018.

For the other ones, we have to check whether we want to change the MSA value.

@rickstaa
Copy link
Author

I think #7 is good enough for the panda_moveit_config. I will make a note about the ridge_factor, enable_failure_recovery and the commented out values in rickstaa#26.

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