-
Notifications
You must be signed in to change notification settings - Fork 326
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
[JTC] Angle wraparound for first segment of trajectory #796
[JTC] Angle wraparound for first segment of trajectory #796
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #796 +/- ##
===========================================
+ Coverage 47.71% 71.75% +24.03%
===========================================
Files 41 41
Lines 3871 3639 -232
Branches 1833 1780 -53
===========================================
+ Hits 1847 2611 +764
+ Misses 751 709 -42
+ Partials 1273 319 -954
Flags with carried forward coverage won't be shown. Click here to find out more.
|
I think it's more important to keep Trajectory class consistent than avoiding ABI breakages. I'm in favour of this approach |
@bmagyar do you agree with the changes now and might want to approve it? |
aef20c5
to
1bc1038
Compare
This pull request is in conflict. Could you fix it @christophfroehlich? |
1bc1038
to
43d03c1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Thanks
6201fd7
to
6756488
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good! I only split the tests into the separate cases
(cherry picked from commit e0491ba)
(cherry picked from commit e0491ba)
(cherry picked from commit e0491ba)
(cherry picked from commit e0491ba)
(cherry picked from commit e0491ba)
(cherry picked from commit e0491ba)
(cherry picked from commit e0491ba) Co-authored-by: Christoph Fröhlich <[email protected]>
I experienced a problem with the angle_wraparound of continuous joints if the current position and the first point of the trajectory have an offset of 2*pi.
I forgot that case when I implemented angle_wraparound, and saw now that this was already part of ROS 1. So I ported the existing implementation and updated the tests (I think now that they were wrong before anyways).
This wraparound of
state_before_traj_msg_.positions
could also be done inside the update method of the controller instead of thetrajectory
class (avoiding ABI break). Comment if you think it should be changed.I added new tests for the trajectory class to check if
wraparound_joint
works.position_error_not_angle_wraparound
andposition_error_angle_wraparound
could also be improved, I opened #802