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

Add allowed_start_tolerance_joints parameter (Port moveit#3287) #3309

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

DaniGarciaLopez
Copy link
Contributor

@DaniGarciaLopez DaniGarciaLopez commented Feb 4, 2025

Description

Allow to specify a per joint allowed start tolerance for the TrajectoryExecutionManager. Port from moveit/moveit#3287 with some changes to be able to dynamically update its values at runtime.

Example of usage

In moveit_controllers.yaml:

trajectory_execution:
  allowed_execution_duration_scaling: 1.2
  allowed_goal_duration_margin: 0.5
  allowed_start_tolerance: 0.01
  allowed_start_tolerance_joints: {panda_joint3: 0.02, panda_joint5: 0.03, panda_finger_joint1: 0.005}

Joints that are not declared in allowed_start_tolerance_joints will use allowed_start_tolerance as the default.

Some remarks

  • I would personally have named this parameter "allowed_start_tolerance_joints" to be next to "allowed_start_tolerance" when sorted alphabetically. However, to maintain continuity with the original PR, I kept it as is.
  • I left the tests as they are because test_execution_manager.cpp is not used at the moment, right?
  • Why skip negative tolerances? I think, rather than defaulting to allowed_start_tolerance, it would be more intuitive to use the absolute value in that case.

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@DaniGarciaLopez DaniGarciaLopez changed the title Add joints_allowed_start_tolerance parameter (Port https://github.com/moveit/moveit/pull/3287) Add joints_allowed_start_tolerance parameter (Port [moveit#3287](https://github.com/moveit/moveit/pull/3287)) Feb 4, 2025
@DaniGarciaLopez DaniGarciaLopez changed the title Add joints_allowed_start_tolerance parameter (Port [moveit#3287](https://github.com/moveit/moveit/pull/3287)) Add joints_allowed_start_tolerance parameter (Port moveit#3287) Feb 4, 2025
@codecov-commenter
Copy link

codecov-commenter commented Feb 4, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 19.60784% with 41 lines in your changes missing coverage. Please review.

Project coverage is 45.55%. Comparing base (fbdd8c5) to head (7dc0087).

Files with missing lines Patch % Lines
...ution_manager/src/trajectory_execution_manager.cpp 19.61% 41 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3309      +/-   ##
==========================================
- Coverage   45.60%   45.55%   -0.04%     
==========================================
  Files         716      716              
  Lines       62388    62431      +43     
  Branches     7547     7553       +6     
==========================================
- Hits        28446    28435      -11     
- Misses      33776    33829      +53     
- Partials      166      167       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mikeferguson
Copy link
Contributor

  • I would personally have named this parameter allowed_start_tolerance_joints to be next to allowed_start_tolerance when sorted alphabetically. However, to maintain continuity with the original PR, I kept it as is.

There are so many parameter changes between ROS 1 and ROS 2 MoveIt that I wouldn't put very high value on "continuity with the original PR" - "allowed_start_tolerance_joints" does seem to be a much better name for this - I would further suggest that "allowed_start_tolerance" should actually be "allowed_start_tolerance_default" - to distinguish more clearly how it differs from the "_joints" parameter

Copy link
Contributor

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

This looks good to me as is!

Agree with @mikeferguson that you can pick whatever name you like for the new parameter, but would be nice to keep the name of the existing one to prevent people from having to update their configs.

That being said, comments in the parameter YAMLs and in the code go a long way in explaining these things!

Also, don't worry about the tutorial images failures. A bunch of packages got booted off Rolling because of a deprecated ros2 control header that needs to be updated, so nothing to do with these changes.

@sea-bass sea-bass added the backport-jazzy Mergify label that triggers a PR backport to Jazzy label Feb 5, 2025
@DaniGarciaLopez
Copy link
Contributor Author

Based on your feedback, I added a commit renaming the parameter to "allowed_start_tolerance_joints".

I think renaming "allowed_start_tolerance" to "allowed_start_tolerance_default" is a bit more controversial. On one hand, I agree that it better reflects its purpose; but on the other hand, as @sea-bass pointed out, this parameter is already used in most of the robot configs. I don’t think it's worth updating for such a minor improvement in clarity.

@DaniGarciaLopez DaniGarciaLopez changed the title Add joints_allowed_start_tolerance parameter (Port moveit#3287) Add allowed_start_tolerance_joints parameter (Port moveit#3287) Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-jazzy Mergify label that triggers a PR backport to Jazzy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants