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

Fix: TOTG can return vels/accels greater than the limits #3394

Merged
merged 6 commits into from Apr 6, 2023

Conversation

AndyZe
Copy link
Contributor

@AndyZe AndyZe commented Apr 3, 2023

This is a 2-line bug fix for this test failure:

/home/andy/ws_moveit/src/moveit/moveit_core/trajectory_processing/test/test_time_optimal_trajectory_generation.cpp:395: Failure
The difference between trajectory.getAcceleration(time)[0] and max_accelerations[0] is 4, which exceeds 1e-3, where
trajectory.getAcceleration(time)[0] evaluates to 32,
max_accelerations[0] evaluates to 28, and
1e-3 evaluates to 0.001.
Time: 0.01
/home/andy/ws_moveit/src/moveit/moveit_core/trajectory_processing/test/test_time_optimal_trajectory_generation.cpp:395: Failure
The difference between trajectory.getAcceleration(time)[0] and max_accelerations[0] is 28, which exceeds 1e-3, where
trajectory.getAcceleration(time)[0] evaluates to 0,
max_accelerations[0] evaluates to 28, and
1e-3 evaluates to 0.001.
Time: 0.02

@AndyZe AndyZe requested a review from mlautman as a code owner April 3, 2023 21:38
@AndyZe AndyZe marked this pull request as draft April 3, 2023 21:38
@codecov
Copy link

codecov bot commented Apr 3, 2023

Codecov Report

Patch coverage: 85.72% and project coverage change: -0.72 ⚠️

Comparison is base (c7d8a19) 61.79% compared to head (b25f9b8) 61.06%.

❗ Current head b25f9b8 differs from pull request most recent head 19fa271. Consider uploading reports for the commit 19fa271 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3394      +/-   ##
==========================================
- Coverage   61.79%   61.06%   -0.72%     
==========================================
  Files         380      380              
  Lines       33605    33616      +11     
==========================================
- Hits        20762    20524     -238     
- Misses      12843    13092     +249     
Impacted Files Coverage Δ
...ry_processing/time_optimal_trajectory_generation.h 50.00% <ø> (-50.00%) ⬇️
...cessing/src/time_optimal_trajectory_generation.cpp 42.46% <85.72%> (-47.48%) ⬇️

... and 4 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 marked this pull request as ready for review April 5, 2023 14:37
@@ -844,7 +843,6 @@ Eigen::VectorXd Trajectory::getAcceleration(double time) const
const double acceleration =
2.0 * (it->path_pos_ - previous->path_pos_ - time_step * previous->path_vel_) / (time_step * time_step);

time_step = time - previous->time_;
Copy link
Contributor Author

@AndyZe AndyZe Apr 5, 2023

Choose a reason for hiding this comment

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

Note that time_step is already calculated a few lines above.

I think this deleted line was intended to improve accuracy but it caused the bug where getAcceleration() returned values over the limit.

The accuracy should still be good if TOTG is run with a small timestep. The default timestep is 0.001. https://github.com/ros-planning/moveit/blob/9fd645d76d0a4fbdbf55f8aecb09fd5a31a63e1a/moveit_core/trajectory_processing/src/time_optimal_trajectory_generation.cpp#L994

@AndyZe AndyZe changed the title Debugging a TOTG failure Fix: TOTG can return vels/accels greater than the limits Apr 5, 2023
@AndyZe AndyZe merged commit 1eb3252 into moveit:master Apr 6, 2023
6 checks passed
@AndyZe AndyZe deleted the andyz/totg_jump branch April 6, 2023 15:01
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

2 participants