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

Check for a Ruckig jerk limit parameter #3375

Merged
merged 4 commits into from Mar 30, 2023

Conversation

AndyZe
Copy link
Contributor

@AndyZe AndyZe commented Mar 16, 2023

Description

We want the ability to set custom joint jerk limits for the Ruckig smoother, but in MoveIt1, the RobotModel does not contain that info. My feeling is, there probably isn't a desire to create a large API-breaking PR in MoveIt1 for adding a jerk limit. Here's what adding jerk to the RobotModel would look like, from MoveIt2: moveit/moveit2#683

So, I made this small, simple PR to add a ROS jerk limit parameter.

Test instructions

I've been testing like this:

  1. Add Ruckig jerk params to panda_moveit_config/launch/demo.launch
  • <param name="/ruckig/panda_joint1/jerk_limit" type="double" value="300.0" /> (and so on)
  1. Add the Ruckig smoothing adapter to the top of the list at ompl_planning_pipeline.launch.xml
  • default="default_planner_request_adapters/AddRuckigTrajectorySmoothing
  1. Launch with: roslaunch moveit_resources_panda_moveit_config demo.launch rviz_tutorial:=true

@simonschmeisser
Copy link
Contributor

Is that original PR actually API-breaking? or just simply ABI-breaking? We don't guarantee either one actually and I would vote for the real deal

@moveit moveit deleted a comment from codecov bot Mar 17, 2023
@AndyZe
Copy link
Contributor Author

AndyZe commented Mar 17, 2023

@simonschmeisser would you mind reviewing this PR then? It's the first step

moveit/moveit_msgs#155

// For now, we check for a jerk parameter
ros::NodeHandle nh;
double jerk_limit = DEFAULT_MAX_JERK;
if (nh.getParam("ruckig/" + vars.at(i) + "/jerk_limit", jerk_limit))
Copy link
Member

Choose a reason for hiding this comment

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

where in the moveit config are these being set?

Copy link
Contributor Author

@AndyZe AndyZe Mar 23, 2023

Choose a reason for hiding this comment

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

I would think it goes in a launch file. Wherever the user wants to put it. This is the only parameter.

Copy link
Member

Choose a reason for hiding this comment

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

this clearly needs to be documented. The warning below hints at the joint_limits.yaml btw, which doesn't contain this parameter at all. Please either leave a note about fixing this, or do so by providing necessary updates for the resources package, possibly tutorials

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix the message below (thanks!) and update the tutorials and (maybe) moveit_resources.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here's the documentation PR: moveit/moveit_tutorials#764

I'm going to go ahead and test this one last time then merge it

// For now, we check for a jerk parameter
ros::NodeHandle nh;
double jerk_limit = DEFAULT_MAX_JERK;
if (nh.getParam("ruckig/" + vars.at(i) + "/jerk_limit", jerk_limit))
Copy link
Member

Choose a reason for hiding this comment

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

this clearly needs to be documented. The warning below hints at the joint_limits.yaml btw, which doesn't contain this parameter at all. Please either leave a note about fixing this, or do so by providing necessary updates for the resources package, possibly tutorials

@codecov
Copy link

codecov bot commented Mar 29, 2023

Codecov Report

Patch coverage: 83.34% and project coverage change: -0.01 ⚠️

Comparison is base (f6f8de9) 61.85% compared to head (dc48295) 61.84%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3375      +/-   ##
==========================================
- Coverage   61.85%   61.84%   -0.01%     
==========================================
  Files         380      380              
  Lines       33601    33605       +4     
==========================================
- Hits        20780    20778       -2     
- Misses      12821    12827       +6     
Impacted Files Coverage Δ
...rajectory_processing/src/ruckig_traj_smoothing.cpp 60.92% <83.34%> (+0.30%) ⬆️

... and 6 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@AndyZe AndyZe merged commit c7d8a19 into moveit:master Mar 30, 2023
6 checks passed
@AndyZe AndyZe deleted the andyz/ruckig_jerk_param branch March 30, 2023 20:13
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.

None yet

3 participants