Add tf prefix helper and test #2803

kuralme · 2025-11-11T04:55:41Z

Added the tf prefix helper here instead of control_toolbox. Prefix enabler flag re-added and frame removed from input arguments.
Related PR

saikishor · 2025-11-11T04:59:35Z

controller_interface/include/controller_interface/helpers.hpp

+  if (!tf_prefix_enabled)
+  {
+    return std::string{};
+  }


Do we need to have this arg here? Can't we condition it outside whoever is using this code?

IMHO, the helper methods shouldn't condition things, they do some standard operation and the users adapt to their usecases

this refers to my comment here: ros-controls/ros2_controllers#1997 (comment)
The current state feels a bit unconvenient. If !tf_prefix_enabled then it is just the given odom frame without adding something. How would you design this?

How about something like this

If the tf_prefix is empty, then return empty
If the tf_prefix is ~, then then the node namespace
If the tr_pedix is anything else, then return that

Obviously, trimming the /'s

What do you think about it?

IMO, the previous version -helper without the prefix enable arg- was causing clutter when called with the ternary operator. It was still a one liner but looked more complicated then it is. I can remove it again regardless so it can comply the standard:

the helper methods shouldn't condition things

The decision is yours.

If the tf_prefix is empty, then return empty
If the tf_prefix is ~, then then the node namespace
If the tr_pedix is anything else, then return that

Not clear about this second one, do you mean i take node namespace if the user inputs the tilde character specifically or is it a result of some operation I am not aware?

Yes, if the user inputs tilde, you replace that part with node namespace. Wouldn't that work?. This way you wouldn't need the parameter of tf_prefix_enable or not right?

For the conditioning part, the part you say complicated. I need to check that part of the code and get back to you.

Previous one I tested here

Thanks for the reference. My question is do we really need tf_frame_prefix_enable parameter? If so, why?

I am simply using it because we have it. But it makes sense to ignore it regardless and apply this

If the tf_prefix is empty, then return empty
If the tf_prefix is ~, then then the node namespace
If the tr_pedix is anything else, then return that

If the user does input a prefix name, an extra enable flag seems redundant imo.
How shall i proceed?

I agree, the extra parameter flag seems to be superfluous.

christophfroehlich

Thanks. Could you please update the diff_drive PR, so that we see the final usage?

saikishor · 2025-11-13T09:32:31Z

controller_interface/include/controller_interface/helpers.hpp

+{
+  if (prefix.empty())
+  {
+    return std::string{};


Suggested change

return std::string{};

return "";

Won't this work?. Just a curious question

Its the same result but thats string literal and my return type is std::string so its an extra implicit conversion done by compiler. I can change anyway if you think looks simpler

please do, let the compiler to the job for us and we keep it more readable

saikishor · 2025-11-13T09:39:07Z

controller_interface/include/controller_interface/helpers.hpp

+    return std::string{};
+  }
+
+  std::string nprefix = prefix == "~" ? node_ns : prefix;


Suggested change

std::string nprefix = prefix == "~" ? node_ns : prefix;

std::string nprefix = prefix;

size_t pos = nprefix.find("~");

if (pos != std::string::npos) {

original.replace(pos, 1, node_ns);

}

Will something like this work?

Just thinking of cases, if users wants to add a prefix like ~/robot_base or something like that

We can also get back to this in future, if needed.

Just nitpicks.

Makes sense, I'll add this.
Edit: just realized you have suggested it for future reference but I went ahead and added it. And tested it with diff drive controller. I can revert back to previous one if you prefer

codecov · 2025-11-13T09:39:51Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.63%. Comparing base (23337e5) to head (09d868c).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2803      +/-   ##
==========================================
- Coverage   89.64%   89.63%   -0.02%     
==========================================
  Files         152      155       +3     
  Lines       17815    17834      +19     
  Branches     1455     1459       +4     
==========================================
+ Hits        15971    15985      +14     
- Misses       1260     1263       +3     
- Partials      584      586       +2

Flag	Coverage Δ
unittests	`89.63% <100.00%> (-0.02%)`	⬇️

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

Files with missing lines	Coverage Δ
...terface/include/controller_interface/tf_prefix.hpp	`100.00% <100.00%> (ø)`
...oller_interface/test/test_controller_tf_prefix.cpp	`100.00% <100.00%> (ø)`
...oller_interface/test/test_controller_tf_prefix.hpp	`100.00% <100.00%> (ø)`

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

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

saikishor

Approving as it serves the purpose.
Adding some nitpicks for future reference

bmagyar · 2025-11-15T07:26:06Z

controller_interface/include/controller_interface/helpers.hpp

         interface_type_list.end();
 }

+/**


I know it's extreme nitpick but could this be moved to a new file instead please?
I wanted to rename this file already to be something like hardware_interface_helpers or similar

why is this helper related to hardware interface? don't we have a different helper file there already?

see that's why I haven't renamed it yet, because I couldn't come up with a better name :D

the initial point stands: could this function please go to a new file called tf_prefix.hpp please?

helper and test added

9622039

github-actions bot requested review from bmagyar, destogl, erickisos, harderthan, mcbed, moriarty and olivier-stasse November 11, 2025 04:55

saikishor reviewed Nov 11, 2025

View reviewed changes

christophfroehlich changed the title ~~helper and test added~~ Add tf prefix helper and test Nov 11, 2025

christophfroehlich mentioned this pull request Nov 11, 2025

frame_id relative namespacing issue ros-controls/ros2_controllers#1999

Open

helper name changed, prefix enable flag removed

24f9e9b

christophfroehlich previously approved these changes Nov 13, 2025

View reviewed changes

christophfroehlich mentioned this pull request Nov 13, 2025

TF prefix helper added ros-controls/control_toolbox#533

Closed

6 tasks

saikishor reviewed Nov 13, 2025

View reviewed changes

saikishor previously approved these changes Nov 13, 2025

View reviewed changes

extend tilde (~) substitution with node namespace to full prefix

cbd3730

kuralme dismissed stale reviews from saikishor and christophfroehlich via cbd3730 November 13, 2025 15:07

bmagyar reviewed Nov 15, 2025

View reviewed changes

kuralme mentioned this pull request Nov 15, 2025

TF prefix helper for diff drive controller ros-controls/ros2_controllers#1997

Open

kuralme and others added 2 commits November 17, 2025 10:41

tf prefix helper moved to its own header

83ed3b9

Merge branch 'master' into tf_prefix_helper

09d868c

christophfroehlich requested review from bmagyar and saikishor November 18, 2025 09:32

christophfroehlich approved these changes Nov 18, 2025

View reviewed changes

-  std::string nprefix = prefix == "~" ? node_ns : prefix;
+  std::string nprefix = prefix;
+  size_t pos = nprefix.find("~");
+  if (pos != std::string::npos) {
+      original.replace(pos, 1, node_ns);
+    }

Add tf prefix helper and test #2803

Are you sure you want to change the base?

Add tf prefix helper and test #2803

Conversation

kuralme commented Nov 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kuralme Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saikishor Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kuralme Nov 11, 2025 • edited by saikishor Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kuralme Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christophfroehlich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kuralme Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

saikishor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kuralme Nov 11, 2025 •

edited

Loading

saikishor Nov 11, 2025 •

edited

Loading

kuralme Nov 11, 2025 •

edited by saikishor

Loading

kuralme Nov 12, 2025 •

edited

Loading

kuralme Nov 13, 2025 •

edited

Loading

codecov bot commented Nov 13, 2025 •

edited

Loading