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

[JTC] Angle wraparound for first segment of trajectory #796

Merged

Conversation

christophfroehlich
Copy link
Contributor

@christophfroehlich christophfroehlich commented Oct 10, 2023

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 the trajectory 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 and position_error_angle_wraparound could also be improved, I opened #802

@christophfroehlich christophfroehlich added the backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble. label Oct 10, 2023
@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (9f7e9e9) 47.71% compared to head (d716819) 71.75%.
Report is 19 commits behind head on master.

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     
Flag Coverage Δ
unittests 71.75% <73.13%> (+24.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...de/diff_drive_controller/diff_drive_controller.hpp 100.00% <ø> (ø)
...iff_drive_controller/src/diff_drive_controller.cpp 68.63% <100.00%> (+20.57%) ⬆️
...jectory_controller/joint_trajectory_controller.hpp 100.00% <ø> (+20.00%) ⬆️
...include/joint_trajectory_controller/trajectory.hpp 73.33% <ø> (+23.33%) ⬆️
...nclude/tricycle_controller/tricycle_controller.hpp 100.00% <ø> (ø)
...roadcaster/src/force_torque_sensor_broadcaster.cpp 82.53% <75.00%> (+21.93%) ⬆️
...ory_controller/src/joint_trajectory_controller.cpp 80.45% <87.50%> (+33.33%) ⬆️
joint_trajectory_controller/src/trajectory.cpp 89.67% <77.77%> (+22.10%) ⬆️
tricycle_controller/src/tricycle_controller.cpp 60.94% <60.60%> (+28.29%) ⬆️

... and 28 files with indirect coverage changes

@bmagyar
Copy link
Member

bmagyar commented Oct 11, 2023

I think it's more important to keep Trajectory class consistent than avoiding ABI breakages.
Also... better to not load more things into update()

I'm in favour of this approach

@christophfroehlich christophfroehlich added backport-iron and removed backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble. labels Nov 11, 2023
@christophfroehlich
Copy link
Contributor Author

@bmagyar do you agree with the changes now and might want to approve it?

@christophfroehlich christophfroehlich force-pushed the jtc/angle_wraparound_trajectory branch from aef20c5 to 1bc1038 Compare November 14, 2023 08:09
Copy link
Contributor

mergify bot commented Nov 15, 2023

This pull request is in conflict. Could you fix it @christophfroehlich?

@christophfroehlich christophfroehlich force-pushed the jtc/angle_wraparound_trajectory branch from 1bc1038 to 43d03c1 Compare November 15, 2023 17:26
Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

Looking good! Thanks

@christophfroehlich christophfroehlich force-pushed the jtc/angle_wraparound_trajectory branch from 6201fd7 to 6756488 Compare November 19, 2023 21:26
@christophfroehlich christophfroehlich added the backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble. label Dec 7, 2023
Copy link
Member

@bmagyar bmagyar left a 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

@bmagyar bmagyar merged commit e0491ba into ros-controls:master Feb 10, 2024
12 of 16 checks passed
mergify bot pushed a commit that referenced this pull request Feb 10, 2024
mergify bot pushed a commit that referenced this pull request Feb 10, 2024
christophfroehlich added a commit that referenced this pull request Feb 12, 2024
christophfroehlich added a commit that referenced this pull request Feb 12, 2024
@christophfroehlich christophfroehlich deleted the jtc/angle_wraparound_trajectory branch March 7, 2024 12:45
christophfroehlich added a commit that referenced this pull request Mar 13, 2024
christophfroehlich added a commit that referenced this pull request Mar 27, 2024
bmagyar pushed a commit that referenced this pull request Mar 27, 2024
(cherry picked from commit e0491ba)

Co-authored-by: Christoph Fröhlich <[email protected]>
henrygerardmoore pushed a commit to henrygerardmoore/ros2_controllers that referenced this pull request Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble. bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants