refactor(jtc): improve jtc tolerances#2133
refactor(jtc): improve jtc tolerances#2133thedevmystic wants to merge 7 commits intoros-controls:masterfrom
Conversation
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).
github-actions
bot
requested review from
MarqRazz,
aprotyas,
bijoua29,
christophfroehlich and
mhubii
Codecov Report❌ Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Just dropping a comment since it's been 2 weeks with no follow ups :) |
P.S.Initially I didn't knew While we have But if we have to remove it then say it to me :) |
|
@christophfroehlich, sorry to tag you. But it's been almost 4 weeks without any follow-up. Thank you, and willing to make changes to suit the project! |
|
@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 |
|
+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
left a comment
saikishor
left a comment
There was a problem hiding this comment.
Move the respective codebase into the .cpp and check for duplications
| * \param[in] show_errors If true, logging messages about size mismatches will be shown. | ||
| * \return A JointTrajectoryPoint where positions, velocities, etc., are the difference. | ||
| */ | ||
| trajectory_msgs::msg::JointTrajectoryPoint create_error_trajectory_point( |
There was a problem hiding this comment.
| trajectory_msgs::msg::JointTrajectoryPoint create_error_trajectory_point( | |
| trajectory_msgs::msg::JointTrajectoryPoint compute_error_trajectory_point( |
| * \param[in] state_error The difference (error) between the actual and desired joint state. | ||
| * \param[in] joint_idx The index of the joint in the `state_error` message to be checked. | ||
| * \param[in] state_tolerance The tolerance structure defining the maximum allowed absolute errors. | ||
| * \param[in] show_errors If true, logging messages about the tolerance violation will be shown. |
There was a problem hiding this comment.
| * \param[in] show_errors If true, logging messages about the tolerance violation will be shown. | |
| * \param[in] show_errors If true, log messages about the tolerance violation will be printed |
| * \param[in] desired_state The commanded state point. | ||
| * \param[in] current_state The actual state point from the hardware. | ||
| * \param[in] is_wraparounds A vector indicating which joints are wraparound (e.g., continuous). | ||
| * \param[in] show_errors If true, logging messages about size mismatches will be shown. |
There was a problem hiding this comment.
| * \param[in] show_errors If true, logging messages about size mismatches will be shown. | |
| * \param[in] show_errors If true, log messages about size mismatches will be printed. |
| * | ||
| * \param[in] state_error Error point (difference between commanded and actual) for all joints. | ||
| * \param[in] segment_tolerances StateTolerances for all joints in the trajectory. | ||
| * \param[in] show_errors If the joint that violates its tolerance should be output to console |
There was a problem hiding this comment.
| * \param[in] show_errors If the joint that violates its tolerance should be output to console | |
| * \param[in] show_errors If true, the joint that violates its tolerance should be output to console. |
| bool check_trajectory_point_tolerance( | ||
| const trajectory_msgs::msg::JointTrajectoryPoint & state_error, | ||
| const std::vector<StateTolerances> & segment_tolerances, bool show_errors = false); |
There was a problem hiding this comment.
Why is this repeated? and why not move the code to .cpp?
There was a problem hiding this comment.
In previous implementation it was inline, so I kept it inline. And we can't move a inline function to .cpp that's why.
| inline bool check_trajectory_point_tolerance( | ||
| const trajectory_msgs::msg::JointTrajectoryPoint & state_error, |
There was a problem hiding this comment.
It is not repeated (as I written it). The above one is the declaration and the other one is the implementation.
I follow ROS style guide but when something is not mentioned in it I follow Google C++ Style Guide.
So, in it, when a function is inline but it is also long, we usually implement it in the bottom in Inline function implementation section.
It is not repeated but one is dec and other is impl.
There was a problem hiding this comment.
And again if you have question why it's inline, then it's again because previous implementation has it.
| 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) | ||
| { |
There was a problem hiding this comment.
Same here why is the codebase in .hpp?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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!
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 the reason I tagged him is that the Contributing guidelines said we can tag maintainers (I think I misunderstood it), and another PR that was not being reviewed for over a month the author tagged the maintainer reviewer and not others. And also I did that because the first tag was more than 2 weeks ago without any follow up. Anyways, thanks again :) |
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
Everyone can leave official reviews from github UI via the "Changes" tab. |
|
Yeah! I did re-read it. Because when I first read it I misread it and didn't read it that carefully as I said earlier that "I misunderstood". Silly me :) |
3 participants
Hello respected maintainers and reviewers!
This is Surya!
This PR addresses the following:
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_pointit 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.