-
Notifications
You must be signed in to change notification settings - Fork 386
Add tf prefix helper and test #2803
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
9622039
24f9e9b
cbd3730
83ed3b9
09d868c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -95,6 +95,39 @@ inline bool interface_list_contains_interface_type( | |||||
| interface_type_list.end(); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * @brief Apply tf prefix | ||||||
| * @param tf_prefix_enabled Whether tf prefixing is enabled | ||||||
| * @param prefix The tf prefix to apply | ||||||
| * @param node_ns Node namespace to use as prefix if prefix is empty | ||||||
| * @return Slash normalized prefix | ||||||
| */ | ||||||
| inline std::string apply_tf_prefix( | ||||||
| const bool tf_prefix_enabled, const std::string & prefix, const std::string & node_ns) | ||||||
| { | ||||||
| if (!tf_prefix_enabled) | ||||||
| { | ||||||
| return std::string{}; | ||||||
|
||||||
| return std::string{}; | |
| return ""; |
Won't this work?. Just a curious question
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please do, let the compiler to the job for us and we keep it more readable
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to have this arg here? Can't we condition it outside whoever is using this code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, the helper methods shouldn't condition things, they do some standard operation and the users adapt to their usecases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previous one I tested here
Thanks for the reference. My question is do we really need tf_frame_prefix_enable parameter? If so, why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, the extra parameter flag seems to be superfluous.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| // Copyright (c) 2025, ros2_control developers | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| #include <gmock/gmock.h> | ||
|
|
||
| #include "controller_interface/helpers.hpp" | ||
| #include "test_controller_tf_prefix.hpp" | ||
|
|
||
| TEST_F(TestControllerTFPrefix, DisabledPrefixReturnsEmpty) | ||
| { | ||
| EXPECT_EQ(controller_interface::apply_tf_prefix(false, "robot", "/ns"), ""); | ||
| EXPECT_EQ(controller_interface::apply_tf_prefix(false, "", "/ns"), ""); | ||
| } | ||
|
|
||
| TEST_F(TestControllerTFPrefix, EmptyPrefixUsesNamespace) | ||
| { | ||
| EXPECT_EQ(controller_interface::apply_tf_prefix(true, "", "/ns"), "ns/"); | ||
| } | ||
|
|
||
| TEST_F(TestControllerTFPrefix, ExplicitPrefixUsed) | ||
| { | ||
| EXPECT_EQ(controller_interface::apply_tf_prefix(true, "robot", "/ns"), "robot/"); | ||
| } | ||
|
|
||
| TEST_F(TestControllerTFPrefix, NormalizesPrefixSlashes) | ||
| { | ||
| EXPECT_EQ(controller_interface::apply_tf_prefix(true, "/robot1", "/ns"), "robot1/"); | ||
| EXPECT_EQ(controller_interface::apply_tf_prefix(true, "robot2//", "/ns"), "robot2//"); | ||
| EXPECT_EQ(controller_interface::apply_tf_prefix(true, "/robot3/", "/ns"), "robot3/"); | ||
| EXPECT_EQ(controller_interface::apply_tf_prefix(true, "/", "/ns"), ""); | ||
| } | ||
|
|
||
| TEST_F(TestControllerTFPrefix, EmptyPrefixAndNamespace) | ||
| { | ||
| EXPECT_EQ(controller_interface::apply_tf_prefix(true, "", ""), ""); | ||
| } | ||
|
|
||
| TEST_F(TestControllerTFPrefix, ComplexNamespace) | ||
| { | ||
| EXPECT_EQ(controller_interface::apply_tf_prefix(true, "", "/ns/"), "ns/"); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| // Copyright (c) 2025, ros2_control developers | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| #ifndef TEST_CONTROLLER_TF_PREFIX_HPP_ | ||
| #define TEST_CONTROLLER_TF_PREFIX_HPP_ | ||
|
|
||
| #include <gmock/gmock.h> | ||
| #include <gtest/gtest.h> | ||
|
|
||
| class TestControllerTFPrefix : public ::testing::Test | ||
| { | ||
| public: | ||
| void SetUp() override | ||
| { | ||
| // placeholder | ||
| } | ||
| void TearDown() override | ||
| { | ||
| // placeholder | ||
| } | ||
| }; | ||
|
|
||
| #endif // TEST_CONTROLLER_TF_PREFIX_HPP_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_helpersor similarThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this helper related to hardware interface? don't we have a different helper file there already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.hppplease?