refactor(jtc): improve jtc tolerances by thedevmystic · Pull Request #2133 · ros-controls/ros2_controllers

thedevmystic · 2026-01-29T15:55:27Z

Hello respected maintainers and reviewers!
This is Surya!

This PR addresses the following:

Seperate declaration from definition for better compilation speed and readability.
Refactor functions for better readability.
Add new function (create_error_trajectory_point).
Add extensive doxygen-style comments.
Add tests for new function (create_error_trajectory_point).

If you have question, why I moved (or seperated) it to tolerances.cpp it is because the file was getting way too long for no reason. It would both increase compile time and future maintainance.
If you have question, why I added create_error_trajectory_point it is because we can calculate the current error point and then compare it with tolerances with ease!

A note: This is a continuation of the previous PR #2048 (closed now). Instead of doing new features (new tolerances option) and refactoring in one PR. I split them into 2 PRs one doing refactoring (current) and another future PR adding feature.

This commit addresses the following: - Seperate declaration from definition for better compilation speed and readability. - Add new function (create_error_trajectory_point). - Add extensive doxygen-style comments. - Add tests for new function (create_error_trajectory_point).

codecov · 2026-01-29T16:26:02Z

Codecov Report

❌ Patch coverage is 92.70073% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.90%. Comparing base (272ac69) to head (fd82eab).
⚠️ Report is 75 commits behind head on master.

Files with missing lines	Patch %	Lines
...include/joint_trajectory_controller/tolerances.hpp	70.58%	6 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2133      +/-   ##
==========================================
+ Coverage   84.80%   84.90%   +0.10%     
==========================================
  Files         151      152       +1     
  Lines       14836    14890      +54     
  Branches     1286     1296      +10     
==========================================
+ Hits        12581    12642      +61     
+ Misses       1784     1779       -5     
+ Partials      471      469       -2

Flag	Coverage Δ
unittests	`84.90% <92.70%> (+0.10%)`	⬆️

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

Files with missing lines	Coverage Δ
joint_trajectory_controller/src/tolerances.cpp	`100.00% <100.00%> (ø)`
...int_trajectory_controller/test/test_tolerances.cpp	`100.00% <100.00%> (ø)`
...include/joint_trajectory_controller/tolerances.hpp	`74.00% <70.58%> (-5.80%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

thedevmystic · 2026-02-09T11:25:50Z

Just dropping a comment since it's been 2 weeks with no follow ups :)

thedevmystic · 2026-02-14T15:55:24Z

P.S.

Initially I didn't knew create_error_trajectory_point already has been implemented.

While we have compute_error_for_point in joint_trajectory_controller.hpp, that file is almost 2000 lines long.
So, we can move that logic from there to here. This function utilizes STL's std::transform and has hardened checks. And it also makes sense to put it here, as we create error point to check it with tolerances anyways.

But if we have to remove it then say it to me :)

thedevmystic · 2026-03-08T10:26:18Z

@christophfroehlich, It is ready for review! I would really appreciate a review as it has been more than a month. Thanks!

@saikishor, sorry to tag you but the previous tagging to Christoph didn't gave any attention. I think you would understand that it's been more than a month with one-sided comments by me :)

saikishor · 2026-03-08T10:39:50Z

@christophfroehlich, It is ready for review! I would really appreciate a review as it has been more than a month. Thanks!

@saikishor, sorry to tag you but the previous tagging to Christoph didn't gave any attention. I think you would understand that it's been more than a month with one-sided comments by me :)

@thedevmystic We understand, however, it is not only @christophfroehlich who has to review, there are other bnunch of reviewers too. Everyone of us are also busy with our day-to.day jobs, we will try to get back to this when we find some time. Lately, last few months the reviews are delayed for some reasons, we will do as much as we can to coverup

christophfroehlich · 2026-03-08T10:47:08Z

+1 . so we have to triage PRs and fix urgent stuff first. but you could help a lot in doing reviews in other PRs

saikishor

Move the respective codebase into the .cpp and check for duplications

saikishor · 2026-03-08T10:45:51Z

+bool check_trajectory_point_tolerance(
+  const trajectory_msgs::msg::JointTrajectoryPoint & state_error,
+  const std::vector<StateTolerances> & segment_tolerances, bool show_errors = false);


Why is this repeated? and why not move the code to .cpp?

In previous implementation it was inline, so I kept it inline. And we can't move a inline function to .cpp that's why.

saikishor · 2026-03-08T10:46:17Z

+inline bool check_trajectory_point_tolerance(
+  const trajectory_msgs::msg::JointTrajectoryPoint & state_error,


This is repeated right?

so we want to move all function impls to .cpp okay 👍

saikishor · 2026-03-08T10:46:48Z

+inline trajectory_msgs::msg::JointTrajectoryPoint create_error_trajectory_point(
+  const trajectory_msgs::msg::JointTrajectoryPoint & desired_state,
+  const trajectory_msgs::msg::JointTrajectoryPoint & current_state,
+  const std::vector<bool> & is_wraparounds, bool show_errors)
+{


Same here why is the codebase in .hpp?

Out of this reply but create_error_trajectory_point and compute_error_for_point are functional similar.

The first one calculates for entire trajectory, while the later one checks for a specific point.

I think that we can move it here and change joint_trajectory_controller.cpp to use it. As that file is already long. So, breaking it's functionality would be better.

Anyways, what you want to do:

Remove create_error_trajectory_point()

Keep it and remove compute_error_for_point() and change the implementation to use it.

Whatever is best, I can do!

Same here why is the codebase in .hpp?

It's because I marked it inline. If you want me to drop it then I can!

thedevmystic · 2026-03-09T07:38:16Z

@christophfroehlich, It is ready for review! I would really appreciate a review as it has been more than a month. Thanks!
@saikishor, sorry to tag you but the previous tagging to Christoph didn't gave any attention. I think you would understand that it's been more than a month with one-sided comments by me :)

@thedevmystic We understand, however, it is not only @christophfroehlich who has to review, there are other bnunch of reviewers too. Everyone of us are also busy with our day-to.day jobs, we will try to get back to this when we find some time. Lately, last few months the reviews are delayed for some reasons, we will do as much as we can to coverup

I really understand your problems too. But like it's been a month and a half. That's why. Anyways, thank you for giving your time.

And also I did that because the first tag was more than 2 weeks ago without any follow up.

Anyways, thanks again :)

thedevmystic · 2026-03-09T07:41:32Z

+1 . so we have to triage PRs and fix urgent stuff first. but you could help a lot in doing reviews in other PRs

I thought that only reviewers can do reviews, I do some kind of small reviews in other PRs. But thank you for considering me able to do reviews!

christophfroehlich · 2026-03-15T15:35:25Z

+1 . so we have to triage PRs and fix urgent stuff first. but you could help a lot in doing reviews in other PRs

I thought that only reviewers can do reviews, I do some kind of small reviews in other PRs. But thank you for considering me able to do reviews!

I am sure that you have carefully read the contributing guidelines. There we clearly encourage you to do so

Even if you are not a maintainer, you are still encouraged to review pull requests.
This helps us increase the review pace and increase code quality.
Also, you are very likely to find some issues/limitations nobody else is seeing.

Everyone can leave official reviews from github UI via the "Changes" tab.

thedevmystic · 2026-03-17T09:07:05Z

Sorry, I didnt read it.

github-actions · 2026-05-01T14:34:08Z

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.

github-actions · 2026-06-18T15:34:28Z

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.

thedevmystic · 2026-06-21T17:25:12Z

still working :)

…o jtc-tolerances

thedevmystic · 2026-06-29T16:34:36Z

hey, @christophfroehlich i did the PR #2127 because without it we get the same mult. def. errors. See PR #2478.

github-actions Bot requested review from MarqRazz, aprotyas, bijoua29, christophfroehlich and mhubii January 29, 2026 15:55

thedevmystic mentioned this pull request Jan 29, 2026

dynamically link JTC parameter validation instead of header only #2127

Merged

thedevmystic added 2 commits January 31, 2026 11:51

Merge branch 'master' into jtc-tolerances

2bf09f1

Merge branch 'master' into jtc-tolerances

9b2ad98

thedevmystic added 4 commits February 9, 2026 16:55

Merge branch 'master' into jtc-tolerances

0fb8e41

feat: add angular wraparound

d84ce1e

Merge branch 'master' into jtc-tolerances

11d49b1

fix: fix include order

fd82eab

saikishor requested changes Mar 8, 2026

View reviewed changes

github-actions Bot added stale and removed stale labels May 1, 2026

github-actions Bot added the stale label Jun 18, 2026

github-actions Bot removed the stale label Jun 22, 2026

thedevmystic added 2 commits June 29, 2026 18:55

Merge branch 'master' of github.com:ros-controls/ros2_controllers int…

11389e4

…o jtc-tolerances

add suggested fixes

06ad461

This was referenced Jul 12, 2026

Refactor: joint_trajectory_controllers library #2018

Open

Link JTC validation library instead of header-only #2478

Open

		inline bool check_trajectory_point_tolerance(
		const trajectory_msgs::msg::JointTrajectoryPoint & state_error,

Uh oh!

Conversation

thedevmystic commented Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jan 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thedevmystic commented Feb 9, 2026

Uh oh!

thedevmystic commented Feb 14, 2026

P.S.

Uh oh!

thedevmystic commented Mar 8, 2026

Uh oh!

saikishor commented Mar 8, 2026

Uh oh!

christophfroehlich commented Mar 8, 2026

Uh oh!

saikishor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

saikishor Mar 8, 2026

Choose a reason for hiding this comment

Uh oh!

thedevmystic Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

saikishor Mar 8, 2026

Choose a reason for hiding this comment

Uh oh!

thedevmystic Jun 29, 2026

Choose a reason for hiding this comment

Uh oh!

saikishor Mar 8, 2026

Choose a reason for hiding this comment

Uh oh!

thedevmystic Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thedevmystic Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

thedevmystic commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thedevmystic commented Mar 9, 2026

Uh oh!

christophfroehlich commented Mar 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thedevmystic commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented May 1, 2026

Uh oh!

github-actions Bot commented Jun 18, 2026

Uh oh!

thedevmystic commented Jun 21, 2026

Uh oh!

thedevmystic commented Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thedevmystic commented Jan 29, 2026 •

edited

Loading

codecov Bot commented Jan 29, 2026 •

edited

Loading

thedevmystic Mar 9, 2026 •

edited

Loading

thedevmystic commented Mar 9, 2026 •

edited

Loading

christophfroehlich commented Mar 15, 2026 •

edited

Loading

thedevmystic commented Mar 17, 2026 •

edited

Loading

thedevmystic commented Jun 29, 2026 •

edited

Loading