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] Renaming variables, reordering trajectory checks #1032

Merged
merged 10 commits into from
Feb 16, 2025

Conversation

destogl
Copy link
Member

@destogl destogl commented Feb 9, 2024

This PR increseas readability of the JTC by renaming trajectory varialbes to be semantically correct and self explainable.

Also it reoders checks of the trajectories done in the callback. They are grouped based on the things they are checking, i.e., 1. data available, 2. time checks; 3. trajectory point checks.

Additionally, it adds "last_commanded_time_" for the open loop mode to be more correct when setting point before trajectory. #780

Copy link
Contributor

mergify bot commented Feb 10, 2024

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

@bmagyar
Copy link
Member

bmagyar commented Feb 11, 2024

Recently merged the wraparound feature which caused some conflicts.

Also, this may be interesting for @fmauch

@bmagyar bmagyar changed the title [JTC] Renaming varialbes, reodering trajectory checks and fixing open-loop mode by adding last commanded time. [JTC] Renaming variables, reordering trajectory checks and fixing open-loop mode by adding last commanded time. Feb 11, 2024
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.

LGTM and improves readability!

@christophfroehlich
Copy link
Contributor

👀 @destogl test_no_jump_when_state_tracking_error_not_updated fails now, I guess due to the last_commanded_time_ change.

saikishor
saikishor previously approved these changes Feb 12, 2024
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.

LGTM, apart from the following minor comment.

Now with the changes, it looks clear and good

bmagyar
bmagyar previously approved these changes Feb 19, 2024
@bmagyar
Copy link
Member

bmagyar commented Feb 19, 2024

Please adjust / extend the tests

henrygerardmoore pushed a commit to henrygerardmoore/ros2_controllers that referenced this pull request Jul 19, 2024
@christophfroehlich christophfroehlich added backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble. backport-iron labels Nov 2, 2024
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.

This includes #780, which has the same failing test: test_no_jump_when_state_tracking_error_not_updated

Copy link
Contributor

mergify bot commented Dec 16, 2024

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

@christophfroehlich christophfroehlich removed the backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble. label Feb 14, 2025
@christophfroehlich christophfroehlich changed the title [JTC] Renaming variables, reordering trajectory checks and fixing open-loop mode by adding last commanded time. [JTC] Renaming variables, reordering trajectory checks Feb 14, 2025
@christophfroehlich christophfroehlich marked this pull request as ready for review February 14, 2025 22:25
@christophfroehlich christophfroehlich requested review from bmagyar, saikishor and fmauch and removed request for VanshGehlot February 14, 2025 22:25
Copy link

codecov bot commented Feb 14, 2025

Codecov Report

Attention: Patch coverage is 87.17949% with 5 lines in your changes missing coverage. Please review.

Project coverage is 84.75%. Comparing base (a048212) to head (f5b82cf).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
...ory_controller/src/joint_trajectory_controller.cpp 86.84% 2 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1032      +/-   ##
==========================================
- Coverage   84.76%   84.75%   -0.01%     
==========================================
  Files         124      124              
  Lines       11444    11443       -1     
  Branches      967      967              
==========================================
- Hits         9700     9699       -1     
  Misses       1432     1432              
  Partials      312      312              
Flag Coverage Δ
unittests 84.75% <87.17%> (-0.01%) ⬇️

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

Files with missing lines Coverage Δ
...jectory_controller/joint_trajectory_controller.hpp 100.00% <ø> (ø)
...ntroller/test/test_trajectory_controller_utils.hpp 83.92% <100.00%> (ø)
...ory_controller/src/joint_trajectory_controller.cpp 85.11% <86.84%> (-0.03%) ⬇️

... and 2 files with indirect coverage changes

saikishor
saikishor previously approved these changes Feb 15, 2025
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.

in general, LGTM
2 minor comments, it is also not a problem not addressing them

Co-authored-by: Sai Kishor Kothakota <sai.kishor@pal-robotics.com>
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.

LGTM

@saikishor
Copy link
Member

@christophfroehlich it seems the downstream builds are failing from the UR drivers scaled JTC. Shall we inform Felix?

@christophfroehlich
Copy link
Contributor

@christophfroehlich it seems the downstream builds are failing from the UR drivers scaled JTC. Shall we inform Felix?

The changes are simple, I can open a PR for the ur_controllers

@christophfroehlich christophfroehlich merged commit fc56126 into master Feb 16, 2025
17 of 25 checks passed
@christophfroehlich christophfroehlich deleted the cleaning-jtc branch February 16, 2025 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants