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

Reduced the minimum max velocity joint limit from 0.01 to 0.001 for totg #2937

Merged
merged 3 commits into from Nov 3, 2021

Conversation

grejj
Copy link

@grejj grejj commented Nov 1, 2021

Description

Currently the minimum value you can specify for the the max velocity outputted for a joint during time optimal trajectory parameterization is 0.01 rad/s or m/s. For joints that are unable to move that fast and whose velocity limits are less than 0.01, the planned path outputted is unachievable with trajectory velocities being higher than the maximum velocity of the joint.

Reduced the value to 0.001.

@grejj grejj requested a review from mlautman as a code owner November 1, 2021 21:04
@AndyZe
Copy link
Contributor

AndyZe commented Nov 1, 2021

@grejj I think this is a good change but do we even need a minimum. It looks to me like throwing an error might be better like this:

  if (max_acceleration_[j] <= 0)
  {
    ROS_ERROR_NAMED(LOGNAME, "Max acceleration must be greater than zero.");
    return false;
}

@codecov
Copy link

codecov bot commented Nov 1, 2021

Codecov Report

Merging #2937 (934894d) into master (d1465a7) will increase coverage by 0.05%.
The diff coverage is 0.00%.

❗ Current head 934894d differs from pull request most recent head 7642c16. Consider uploading reports for the commit 7642c16 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2937      +/-   ##
==========================================
+ Coverage   61.73%   61.78%   +0.05%     
==========================================
  Files         373      373              
  Lines       33425    33429       +4     
==========================================
+ Hits        20632    20651      +19     
+ Misses      12793    12778      -15     
Impacted Files Coverage Δ
...cessing/src/time_optimal_trajectory_generation.cpp 76.07% <0.00%> (-0.59%) ⬇️
...raint_samplers/src/default_constraint_samplers.cpp 79.60% <0.00%> (-0.28%) ⬇️
...nning_scene_monitor/src/planning_scene_monitor.cpp 68.93% <0.00%> (-0.12%) ⬇️
moveit_core/robot_model/src/joint_model_group.cpp 64.15% <0.00%> (+2.28%) ⬆️
...ipulation/pick_place/src/manipulation_pipeline.cpp 74.39% <0.00%> (+2.48%) ⬆️
...e/src/parameterization/model_based_state_space.cpp 72.68% <0.00%> (+3.11%) ⬆️
...eit_ros/manipulation/pick_place/src/pick_place.cpp 92.39% <0.00%> (+3.81%) ⬆️

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 d1465a7...7642c16. Read the comment docs.

@v4hn
Copy link
Contributor

v4hn commented Nov 2, 2021

Thanks for creating the PR after our brief discussion on Discord!
I agree with @AndyZe.
@grejj Could you modify your patch to make the parameterization fail when values less-equal 0 are encountered in velocity or acceleration bounds? This will also give users clear feedback when they use configurations with has_*_limits: true without specifying an actual limit.

…ion failure if limits less than/equal to zero
@AndyZe
Copy link
Contributor

AndyZe commented Nov 2, 2021

LGTM.

@rhaschke
Copy link
Contributor

rhaschke commented Nov 2, 2021

@grejj, please apply the suggested formatting changes. Ideally, install https://pre-commit.com to catch these at commit time.

@rhaschke rhaschke merged commit 0d0a6dd into moveit:master Nov 3, 2021
@welcome
Copy link

welcome bot commented Nov 3, 2021

Congrats on getting your first MoveIt pull request merged and improving open source robotics!

@grejj grejj deleted the devel branch November 3, 2021 16:08
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

4 participants