TF prefix helper added

[kuralme]

Contributions via pull requests are much appreciated. Before sending us a pull request, please ensure that:

1. Limited scope. Your PR should do one thing or one set of things. Avoid adding “random fixes” to PRs. Put those on separate PRs.
2. Give your PR a descriptive title. Add a short summary, if required.
3. Make sure the pipeline is green.
4. Don’t be afraid to request reviews from maintainers.
5. New code = new tests. If you are adding new functionality, always make sure to add some tests exercising the code and serving as live documentation of your original intention.

To send us a pull request, please:

• Fork the repository.
• Modify the source; please focus on the specific change you are contributing. If you also reformat all the code, it will be hard for us to focus on your change.
• Ensure local tests pass. (colcon test and pre-commit run (requires you to install pre-commit by pip3 install pre-commit)
• Commit to your fork using clear commit messages.
• Send a pull request, answering any default questions in the pull request interface.
• Pay attention to any automated CI failures reported in the pull request, and stay involved in the conversation.

PR related to the issue

[christophfroehlich]

Thanks for your contribution. Looks fine in general, please consider my notes below.

Could you also open a PR in ros2_controllers using this method, and change temporarily the entry of control_toolbox in ros2_controllers.rolling.repos to your branch?

[kuralme]

I made the changes. If looks good, I will continue with the ros2_controllers.
Edit: updated the tests too

[codecov-commenter]

Codecov Report

❌ Patch coverage is 95.45455% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 83.15%. Comparing base (f5a3f2f) to head (1e39750).

Files with missing lines	Patch %	Lines
...ntrol_toolbox/include/control_toolbox/tf_utils.hpp	90.90%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #533      +/-   ##
==========================================
+ Coverage   83.02%   83.15%   +0.12%     
==========================================
  Files          29       31       +2     
  Lines        1968     1989      +21     
  Branches      110      115       +5     
==========================================
+ Hits         1634     1654      +20     
  Misses        268      268              
- Partials       66       67       +1     

Flag	Coverage Δ
unittests	83.15% <95.45%> (+0.12%)	⬆️

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

Files with missing lines	Coverage Δ
control_toolbox/test/tf_utils_tests.cpp	100.00% <100.00%> (ø)
...ntrol_toolbox/include/control_toolbox/tf_utils.hpp	90.90% <90.90%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

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

[christophfroehlich]

Suggested change

	bool tf_prefix_enabled, std::string prefix, const std::string & node_ns,
	bool tf_prefix_enabled, const std::string & prefix, const std::string & node_ns,

[christophfroehlich]

And I'm unsure if it is better removing tf_prefix_enabled as an argument, but just use a pattern like tf_prefix_enabled ? apply_tf_prefix(...) : frame; on the calling site? something like apply_tf_prefix(false) feels a bit strange for me. what do you think?

[kuralme]

Yes that makes better sense to remove that. I'll do so

[kuralme]

I have a question about the slash normalization design. With the latest version of helper utilized in the controllers(diff, omni, mecanum), their tests succeeds. Currently, it only removes the leading and add one trailing slash to tf prefix. But what about cases when user put multiple slashes or leading slash in the namespace?

I got the test cases for these, is this expected behaviour?

EXPECT_EQ(control_toolbox::apply_tf_prefix("robot2//", "/ns", "odom"), "robot2//odom");
EXPECT_EQ(control_toolbox::apply_tf_prefix("robot", "/ns", "/odom"), "robot//odom");

I can also make it ensure there is only one in between:

EXPECT_EQ(control_toolbox::apply_tf_prefix("robot2//", "/ns", "odom"), "robot2/odom");
EXPECT_EQ(control_toolbox::apply_tf_prefix("robot", "/ns", "/odom"), "robot/odom");
EXPECT_EQ(control_toolbox::apply_tf_prefix("robot/", "/ns", "/odom"), "robot/odom");

So, shall i change the design or leave as it is?