Refactor interpolation_method class

[thedevmystic]

Hello, there ros2_controllers maintainers!
Again this is Surya! Here this is first batch of joint_trajectory_controller library refactor.

As in the issue #2018, I have discussed with @christophfroehlich, to do refactor in batches.

This batch focus on interpolation_method.hpp

I have refactored the following,

1. Add extensive doxygen-style comments, so future maintainance can go smoothly.
2. Refactored Maps and added reverse map.
3. Refactored from_string function.
4. Changed reference of InterpolationMethodMap in joint_trajectory_controller.cpp to ReverseInterpolationMethodMap as it makes much more sense.

If it is good, I can move forward to refactor trajectory.hpp and trajectory.cpp to handle edge cases, enhance readability, add comments. As when I was using it, I got segmentation fault (Trying to access memory [nil]).

[codecov]

Codecov Report

❌ Patch coverage is 14.28571% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.91%. Comparing base (89fd9e1) to head (bbe3bb7).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...nt_trajectory_controller/interpolation_methods.hpp	14.28%	11 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2019      +/-   ##
==========================================
- Coverage   84.97%   84.91%   -0.06%     
==========================================
  Files         148      148              
  Lines       14359    14367       +8     
  Branches     1230     1230              
==========================================
- Hits        12201    12200       -1     
- Misses       1731     1740       +9     
  Partials      427      427              

Flag	Coverage Δ
unittests	84.91% <14.28%> (-0.06%)	⬇️

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

Files with missing lines	Coverage Δ
...ory_controller/src/joint_trajectory_controller.cpp	83.88% <ø> (ø)
...nt_trajectory_controller/interpolation_methods.hpp	23.52% <14.28%> (-32.03%)	⬇️

🚀 New features to boost your workflow:

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

[christophfroehlich]

First of all, please ensure that tests pass locally, including pre-commit. (run pre-commit install for the future, and pre-commit run --all to fix the issues now).

I'd appreciate a review from @fmauch because he recently mentioned also that JTC needs some refactoring.

[thedevmystic]

Of course, @christophfroehlich. The main issue is that, pre-commit on my computer, don't work as intended, even if I install it via pip3 install pre-commit. It passed every test in my computer, but failed here. I'm extremely sorry for that.

From next time onwards I will run the pre-commit workflow before PR.

[thedevmystic]

I have fixed all stylistic conflicts proposed by pre-commit. Now the PR is full ready to be reviewed. Now, I will wholeheartedly appreciate a review from @fmauch.

[thedevmystic]

While I didn't changed it. One suggestion is that to change RCLCPP_INFO to RCLCPP_WARN or equivalent, as user's configuration failed knowingly. And it can lead to unnoticed inconsistency.

[fmauch]

I hope, you don't mind me nitpicking on docstrings, but since most of this PR is adding docstrings, that seems justified :-)

[fmauch]

This looks good to me, thanks for iterating :-)

[thedevmystic]

This looks good to me, thanks for iterating :-)

No need to thank me. It's my pleasure to do something for my beloved open source community 😄.

And be a part of ros-controls. If I can make something I use better, then why not.

[thedevmystic]

This series of commits fixes the descriptions and comments, as suggested by @christophfroehlich.

This commit addresses,

1. Fixes descriptions.
2. Fixes deprecated message.

NOTE: About the CI/CD failure, it doesn't seem like it is due to this PR, or commit. As it indicates failures that are not even touched by the previous commit.

Thanks!

[christophfroehlich]

Minor comments from my side.

[christophfroehlich]

NOTE: About the CI/CD failure, it doesn't seem like it is due to this PR, or commit. As it indicates failures that are not even touched by the previous commit.

No this is a known flaky test.

[christophfroehlich]

Thanks for the iteration!

[thedevmystic]

Thanks for the iteration!

Again, by thanking me, you made me a outsider. The only motive I have, behind this PR, is that I use ROS and I can make it better then why not.

I just love the code and the community! That's why I did it. No need to thank me!

So, hope you a nice day forward @christophfroehlich.

[christophfroehlich]

I haven't looked into the logs before but the deprecation warning is now called always because this is a global variable in the file.

In file included from /workspaces/ros2_rolling_ws/src/ros2_controllers/joint_trajectory_controller/include/joint_trajectory_controller/joint_trajectory_controller.hpp:32,
                 from /workspaces/ros2_rolling_ws/src/ros2_controllers/joint_trajectory_controller/test/test_trajectory_controller.cpp:35:
/workspaces/ros2_rolling_ws/src/ros2_controllers/joint_trajectory_controller/include/joint_trajectory_controller/interpolation_methods.hpp: In function ‘void __static_initialization_and_destruction_0()’:
/workspaces/ros2_rolling_ws/src/ros2_controllers/joint_trajectory_controller/include/joint_trajectory_controller/interpolation_methods.hpp:79:60: warning: ‘joint_trajectory_controller::interpolation_methods::InterpolationMethodMap’ is deprecated: InterpolationMethodMap will be removed in future releases. Instead, use the direct lookup methods. [-Wdeprecated-declarations]
   79 | const std::unordered_map<std::string, InterpolationMethod> InterpolationMethodMap(
      |                                                            ^~~~~~~~~~~~~~~~~~~~~~
/workspaces/ros2_rolling_ws/src/ros2_controllers/joint_trajectory_controller/include/joint_trajectory_controller/interpolation_methods.hpp:79:60: note: declared here

I think there is nothing we can do about that. We could silence the warning in joint_trajectory_controller.hpp, but if one uses it from this include then there would not be any warning at all.

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
#include "joint_trajectory_controller/interpolation_methods.hpp"
#pragma GCC diagnostic pop

We should remove it after the next release sync, can you please open a draft PR doing that cleanup?

[thedevmystic]

Done! This PR is opened as PR #2041.