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.

Sign up for GitHub

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

Jump to bottom

[JTC] Process tolerances sent with action goal #716

Merged

bmagyar merged 32 commits into ros-controls:master from christophfroehlich:jtc/action_tolerances

Jul 3, 2024

Contributor

christophfroehlich commented Jul 24, 2023 •

edited

Loading

From here:

JointTrajectoryController ignores goal_time_tolerance set via FollowJointTrajectory.Goal.goal_time_tolerance together with path_tolerance and goal_tolerance .

This PR proposes a way how to process the tolerances: The tolerances from one action goal are not saved for the next one, but the default ones will be used if no tolerances are set with the following action goals.

(Temporary) deactivation is also implemented like documented in the msg definition.

From the node parameter we cannot set velocity or acceleration tolerances (except for a single stopped_velocity_tolerance for the goal tolerances of all joints). Should we add them as parameters as well, to have the same structure like the action message?

This new feature could break some existing projects, because the tolerances were just ignored and might now be breaking behaviors. Should we introduce a temporary parameter to opt-in?

Fixes #249


          Process tolerances sent with action goal

c3acc81

christophfroehlich requested review from bmagyar, aprotyas, destogl and livanov93

July 24, 2023 14:19


          Merge branch 'master' into jtc/action_tolerances

c168ab3

codecov-commenter commented Jul 24, 2023 •

edited by codecov bot

Loading

Codecov Report

Attention: Patch coverage is 35.29412% with 44 lines in your changes missing coverage. Please review.

Project coverage is 47.05%. Comparing base (0b43291) to head (534746c).
Report is 33 commits behind head on master.

❗ Current head 534746c differs from pull request most recent head 34ca300

Please upload reports for the commit 34ca300 to get more accurate results.

Additional details and impacted files

@@             Coverage Diff             @@
##           master     #716       +/-   ##
===========================================
- Coverage   71.86%   47.05%   -24.81%     
===========================================
  Files          41       41               
  Lines        3650     3925      +275     
  Branches     1794     1863       +69     
===========================================
- Hits         2623     1847      -776     
- Misses        707      780       +73     
- Partials      320     1298      +978

Flag	Coverage Δ
unittests	`47.05% <35.29%> (-24.81%)`	⬇️

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

Files	Coverage Δ
...jectory_controller/joint_trajectory_controller.hpp	`0.00% <ø> (-100.00%)`	⬇️
...ory_controller/src/joint_trajectory_controller.cpp	`48.15% <25.00%> (-31.38%)`	⬇️
...include/joint_trajectory_controller/tolerances.hpp	`45.26% <37.50%> (-30.42%)`	⬇️

... and 36 files with indirect coverage changes


          Merge branch 'master' into jtc/action_tolerances

0328c94

Contributor

mergify bot commented Aug 15, 2023

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

christophfroehlich added 2 commits

August 15, 2023 22:34


          Merge branch 'master' into jtc/action_tolerances

880b87b


          Remove whitespaces

57d6b32

christophfroehlich added the enhancement label

Contributor

mergify bot commented Aug 16, 2023

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


          Merge branch 'master' into jtc/action_tolerances

ac0e871

christophfroehlich mentioned this pull request

Joint trajectory controller detailed parameters don't exist on param server #149

Closed

Contributor

mergify bot commented Sep 12, 2023

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


          Merge branch 'master' into jtc/action_tolerances

2c717d0

Contributor

mergify bot commented Sep 14, 2023

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

christophfroehlich and others added 3 commits

September 14, 2023 18:48


          Merge branch 'master' into jtc/action_tolerances

31afb42


          Fix merge conflict

16a8828


          Merge branch 'master' into jtc/action_tolerances

6a95acb

christophfroehlich added the backport-iron label

Contributor

mergify bot commented Nov 15, 2023

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

christophfroehlich added 3 commits

November 15, 2023 20:10


          Merge branch 'master' into jtc/action_tolerances

4a3ba8b


          Merge branch 'master' into jtc/action_tolerances

bf6d034


          Merge branch 'master' into jtc/action_tolerances

22543a9

christophfroehlich added the backport-humble label

Contributor

mergify bot commented Dec 21, 2023

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

christophfroehlich and others added 2 commits

December 22, 2023 12:51


          Merge branch 'master' into jtc/action_tolerances

fcaa1b9


          Fix format

534746c

bmagyar approved these changes

View reviewed changes

christophfroehlich added 5 commits

June 20, 2024 12:28


          Don't accept negative tolerances

7cc1244


          Fix tests and improve error messages.

6d21d3a


          Add a test for wrong joints too

a5d548c


          Merge branch 'master' into jtc/action_tolerances

e48ca7b


          Move tolerances tests to own file

4f1b755

christophfroehlich requested a review from saikishor

June 20, 2024 14:47

saikishor approved these changes

View reviewed changes

Member

saikishor left a comment

Few minor comments. Rest looks good to me.
Thank you for the changes!

joint_trajectory_controller/include/joint_trajectory_controller/tolerances.hpp Outdated Show resolved Hide resolved

joint_trajectory_controller/include/joint_trajectory_controller/tolerances.hpp Outdated Show resolved Hide resolved

joint_trajectory_controller/include/joint_trajectory_controller/tolerances.hpp Outdated Show resolved Hide resolved

joint_trajectory_controller/include/joint_trajectory_controller/tolerances.hpp Outdated Show resolved Hide resolved

joint_trajectory_controller/include/joint_trajectory_controller/tolerances.hpp Outdated Show resolved Hide resolved

joint_trajectory_controller/include/joint_trajectory_controller/tolerances.hpp Outdated Show resolved Hide resolved

joint_trajectory_controller/include/joint_trajectory_controller/tolerances.hpp Outdated Show resolved Hide resolved

joint_trajectory_controller/include/joint_trajectory_controller/tolerances.hpp Outdated Show resolved Hide resolved

joint_trajectory_controller/src/joint_trajectory_controller.cpp Outdated Show resolved Hide resolved

joint_trajectory_controller/src/joint_trajectory_controller.cpp Outdated Show resolved Hide resolved

christophfroehlich and others added 2 commits

June 21, 2024 07:40


          Apply suggestions from code review

ec87dc0

Co-authored-by: Sai Kishor Kothakota <saisastra3@gmail.com>


          Remove duplicate check

54d279c

destogl approved these changes

View reviewed changes

Member

destogl left a comment

Very good PR! Thanks. I have a few minor comments, but we can also merge without resolving them. They are mostly cosmetic nature.

From the node parameter we cannot set velocity or acceleration tolerances (except for a single stopped_velocity_tolerance for the goal tolerances of all joints). Should we add them as parameters as well, to have the same structure like the action message?

This would be nice to have in the follow-up PR.

This new feature could break some existing projects, because the tolerances were just ignored and might now be breaking behaviors. Should we introduce a temporary parameter to opt-in?

No. Just to add a note to migration notes.

joint_trajectory_controller/include/joint_trajectory_controller/tolerances.hpp Outdated

                 SegmentTolerances tolerances;
                 tolerances.goal_time_tolerance = constraints.goal_time;
+                auto logger = rclcpp::get_logger("tolerance");

Member

destogl Jun 24, 2024

Maybe might be better to have a logger object and initialize child tolerances here of the JTC node.

Contributor Author

christophfroehlich Jun 24, 2024

I haven't had a better idea as passing the JTC logger as argument to the get_segment_tolerances methods. There is no persistent class with a configuration/constructor method to pass the logger only once.

joint_trajectory_controller/include/joint_trajectory_controller/tolerances.hpp Outdated

+               *
+               * \param default_tolerances The default tolerances to use if the action goal does not specify any.
+               * \param goal The new action goal
+               * \param params The ROS Parameters

Member

destogl Jun 24, 2024

I am not sure why do we need ROS parameters because the default tolerances should be added through parameters, isn't it?

Contributor Author

christophfroehlich Jun 24, 2024

The only field we need is the params.joints actually, I can change it to have a std::vector as argument instead?

Contributor Author

christophfroehlich Jun 24, 2024

done

joint_trajectory_controller/include/joint_trajectory_controller/tolerances.hpp Outdated

+                SegmentTolerances active_tolerances(default_tolerances);
+                active_tolerances.goal_time_tolerance = rclcpp::Duration(goal.goal_time_tolerance).seconds();
+                auto logger = rclcpp::get_logger("tolerance");

Member

destogl Jun 24, 2024

It would be great to have a static object for this, as mentioned above because we are creating objects all the time.

Contributor Author

christophfroehlich Jun 24, 2024

see above

joint_trajectory_controller/test/test_tolerances_utils.hpp Outdated

Member

destogl Jun 24, 2024

Not sure that we need a header file here. Maybe just add this on top of the .cpp file. Those are not too big.

Contributor Author

christophfroehlich Jun 24, 2024 •

edited

Loading

I use it in test_tolerances.cpp and test_trajectory_actions.cpp, that's why I created this header file.
I can add it to test_trajectory_controller_utils.hpp if you prefer

Contributor Author

christophfroehlich Jun 24, 2024 •

edited

Loading

I moved it into the other utils header file, but had to mark it as [[maybe_unused]].

christophfroehlich added 4 commits

June 24, 2024 19:25


          Pass std::vector instead of parameter struct

1a592ab


          Use a static child logger

2bdc0c2


          Remove test_tolerances_utils

36ecdda


          Update docs

34ca300

christophfroehlich mentioned this pull request

[JTC] Add more options for tolerance configuration #1180

Open

Contributor Author

christophfroehlich commented Jun 24, 2024

Very good PR! Thanks. I have a few minor comments, but we can also merge without resolving them. They are mostly cosmetic nature.

From the node parameter we cannot set velocity or acceleration tolerances (except for a single stopped_velocity_tolerance for the goal tolerances of all joints). Should we add them as parameters as well, to have the same structure like the action message?

This would be nice to have in the follow-up PR.

I opened a follow-up issue.

This new feature could break some existing projects, because the tolerances were just ignored and might now be breaking behaviors. Should we introduce a temporary parameter to opt-in?

No. Just to add a note to migration notes.

Done.

@destogl I addressed all your comments, please have a look once again.

Test still are fine.

$ colcon test --packages-select joint_trajectory_controller
Starting >>> joint_trajectory_controller
[Processing: joint_trajectory_controller]                   
[Processing: joint_trajectory_controller]                           
[Processing: joint_trajectory_controller]                             
[Processing: joint_trajectory_controller]                             
[Processing: joint_trajectory_controller]                             
[Processing: joint_trajectory_controller]                             
[Processing: joint_trajectory_controller]                             
[Processing: joint_trajectory_controller]                             
[Processing: joint_trajectory_controller]                             
Finished <<< joint_trajectory_controller [4min 49s]                   

Summary: 1 package finished [4min 50s]

bmagyar merged commit 07061f9 into ros-controls:master

5 of 20 checks passed

christophfroehlich deleted the jtc/action_tolerances branch

July 3, 2024 17:57

mergify bot pushed a commit that referenced this pull request


          [JTC] Process tolerances sent with action goal (#716)

d128129

(cherry picked from commit 07061f9)

# Conflicts:
#	doc/migration/Jazzy.rst
#	doc/release_notes/Jazzy.rst

mergify bot mentioned this pull request

[JTC] Process tolerances sent with action goal (backport #716) #1189

Merged

mergify bot pushed a commit that referenced this pull request


          [JTC] Process tolerances sent with action goal (#716)

c5506f7

(cherry picked from commit 07061f9)

# Conflicts:
#	doc/migration/Jazzy.rst
#	doc/release_notes/Jazzy.rst

mergify bot mentioned this pull request

[JTC] Process tolerances sent with action goal (backport #716) #1190

Merged

Contributor Author

christophfroehlich commented Jul 3, 2024

@bmagyar do we want to backport this to humble/iron? Then I'd update the release notes accordingly

fmauch mentioned this pull request

[JTC] Make goal_time_tolerance overwrite default value only if explicitly set #1192

Merged

bmagyar pushed a commit that referenced this pull request


          [JTC] Process tolerances sent with action goal (backport #716) (#1190)

f533cfd

bmagyar pushed a commit that referenced this pull request


          [JTC] Process tolerances sent with action goal (backport #716) (#1189)

112df35

This was referenced Jul 15, 2024

[JTC] Make goal_time_tolerance overwrite default value only if explicitly set (backport #1192 + #1209) #1207

Merged

[JTC] Make goal_time_tolerance overwrite default value only if explicitly set (backport #1192 + #1209) #1208

Merged

destogl mentioned this pull request

Adding use of robot description parameter in the Admittance Controller #1247

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

saikishor saikishor approved these changes

bmagyar bmagyar approved these changes

destogl destogl approved these changes

aprotyas Awaiting requested review from aprotyas

livanov93 Awaiting requested review from livanov93

Labels

backport-humble backport-iron enhancement