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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

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

Comment on lines 156 to 161
if (fabs(last_commanded_time_.seconds()) < std::numeric_limits<float>::epsilon())
{
last_commanded_time_ = time;
}
current_trajectory_->set_point_before_trajectory_msg(
last_commanded_time_, last_commanded_state_, joints_angle_wraparound_);
Copy link
Member

Choose a reason for hiding this comment

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

In this case, the last_command_time_ would be same for the first 2 iterations. Is that okay?. If its fine, then all 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.72%. Comparing base (fc20a27) to head (a2c6189).
Report is 1 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.73%   84.72%   -0.01%     
==========================================
  Files         124      124              
  Lines       11408    11408              
  Branches      955      956       +1     
==========================================
- Hits         9666     9665       -1     
  Misses       1436     1436              
- Partials      306      307       +1     
Flag Coverage Δ
unittests 84.72% <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 3 files with indirect coverage changes

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