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] Make goal_time_tolerance overwrite default value only if explicitly set #1192

Merged
merged 3 commits into from
Jul 9, 2024

Conversation

fmauch
Copy link
Contributor

@fmauch fmauch commented Jul 5, 2024

Since #716 we process the tolerances set from the action goal. This was done as described in https://github.com/ros-controls/control_msgs/blob/65814d985aa29bed0adfaed5f2ebd8cf26266056/control_msgs/action/FollowJointTrajectory.action#L6-L10 and https://github.com/ros-controls/control_msgs/blob/65814d985aa29bed0adfaed5f2ebd8cf26266056/control_msgs/msg/JointTolerance.msg#L7-L10.

However, the goal_time_tolerance was always overwritten with the value from the action goal. This actually changed the JTC's default behavior if

  • a goal_time_tolerance != 0 was set in the controller config and
  • no goal_time_tolerance or explicitly a 0.0 was given in the action goal.

In this, case, the controller would wait indefinitely until the robot reached the final target independent of the goal_time_tolerance configured inside the controller's default values.

This PR makes it such as

  • If a goal_time_tolerance of 0.0 is given in the action (which is the default if left empty), the value from the controller config will be used
  • If a goal_time_tolerance of -1.0 is given the controller will wait at the end of the trajectory until the error gets lower than the goal_tolerance for each joint.
  • If a goal_time_tolerance > 0.0 is given, the goal_time_tolerance from the action goal will be used.
  • Any other negative value for goal_time_tolerance will make the controller fall back to its configured tolerances completely.

The goal_time_tolerance value was always shadowing the one from the action server parameter.
This commit makes it behave much more like the path tolerances and the goal tolerances.
Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

Thanks for bringing this up, and fixing it immediately! But the test_tolerances test fails now, and we should test now the options for the goal_time_tolerances too.

: [ RUN      ] TestTolerancesFixture.test_deactivate_tolerances
2: /workspaces/ros2_humble_ws/src/ros2_controllers/joint_trajectory_controller/test/test_tolerances.cpp:190: Failure
2: Expected equality of these values:
2:   active_tolerances.goal_time_tolerance
2:     Which is: 0.10000000000000001
2:   0.0
2:     Which is: 0
2: [  FAILED  ] TestTolerancesFixture.test_deactivate_tolerances (0 ms)

@bmagyar
Copy link
Member

bmagyar commented Jul 6, 2024

Waiting for new test then?

Also updated expectDefaultTolerances to check for the goal_time_tolerance.
@fmauch
Copy link
Contributor Author

fmauch commented Jul 8, 2024

I've updated the test_tolerance tests accordingly. The jtc controller tests probably also need some updates, thoug.

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.

LGTM

@bmagyar
Copy link
Member

bmagyar commented Jul 9, 2024

I'll merge this so the release I'm pushing won't have behaviour-breaking changes.

@bmagyar bmagyar merged commit 776c432 into ros-controls:master Jul 9, 2024
5 of 18 checks passed
Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for fixing tests!

@fmauch fmauch deleted the jtc_goal_time_tolerance branch July 9, 2024 07:18
henrygerardmoore pushed a commit to henrygerardmoore/ros2_controllers that referenced this pull request Jul 10, 2024
henrygerardmoore pushed a commit to henrygerardmoore/ros2_controllers that referenced this pull request Jul 10, 2024
@christophfroehlich
Copy link
Contributor

@Mergifyio backport iron humble

Copy link
Contributor

mergify bot commented Jul 15, 2024

backport iron humble

✅ Backports have been created

destogl pushed a commit that referenced this pull request Jul 16, 2024
…itly set (backport #1192 + #1209) (#1208)

---------

Co-authored-by: Felix Exner (fexner) <[email protected]>
Co-authored-by: Christoph Froehlich <[email protected]>
destogl pushed a commit that referenced this pull request Jul 16, 2024
…itly set (backport #1192 + #1209) (#1207)

---------

Co-authored-by: Felix Exner (fexner) <[email protected]>
Co-authored-by: Christoph Froehlich <[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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants