[Humble Backport] tf_prefix param: fix slashes and add to IMU Broadcaster by rafal-gorecki · Pull Request #1102 · ros-controls/ros2_controllers

rafal-gorecki · 2024-04-20T11:30:37Z

Hello,
I encountered one error related to the tf_prefix parameter. Namely, the current code works in such a way that the use of namespace does not remove the first slash. It's not typical for a tf frame to start with / so I fixed it. Additionally, I don't think it's intuitive to add a slesh every time we use tf_prefix, if only because someone might want to give a prefix that doesn't end with a slesh, although I could be wrong about that.

Additionally, I encountered one error with the option

    validation: {
      not_empty<>: null
    }

it doesn't work well with namespaces and I don't know how to fix it yet. However, I see that it is not used in all packages, so I commented it temporarily.

mergify · 2024-04-20T11:31:13Z

@rafal-gorecki, all pull requests must be targeted towards the master development branch.
Once merged into master, it is possible to backport to humble, but it must be in master
to have these changes reflected into new distributions.

christophfroehlich · 2024-04-20T13:19:51Z

Is this a possible duplicate of #1060?

saikishor

It seems to be a duplication of #1060, but I like the fact that you have added some tests to the cases. However, I'm not entirely convinced in some places. I've left some comments here to discuss.

saikishor · 2024-04-20T17:46:42Z

diff_drive_controller/src/diff_drive_controller.cpp

-    else
-    {
-      tf_prefix = tf_prefix + "/";
+      tf_prefix.erase(0, 1);


Why do you always want to erase the first character? If the prefix is parsed without '/' then it removes the wrong character right?

I see that you did that when you are using node namespace, however, I think you need to correct the users tf_prefix as well, if he adds a '/' as well. Try to do same logic as done in #1060

Makes sense

diff_drive_controller/test/test_diff_drive_controller.cpp

saikishor · 2024-04-20T17:52:06Z

imu_sensor_broadcaster/src/imu_sensor_broadcaster_parameters.yaml

+    # FIXME: Currently does not work with namespace
+    # validation: {
+    #   not_empty<>: null
+    # }


Can you explain a bit more in what situation it doesn't work.

Yes sure:

I don't know exactly what's going on. However, when this is checked it then gets an error:

[ERROR] [1713772814.609746661] [test_namespace.test_imu_sensor_broadcaster]: Exception thrown during init stage with message: Invalid value set during initialization for parameter 'sensor_name': Parameter 'sensor_name' cannot be empty

The problem is probably that:

Adding a namespace causes the frame_id parameter to be searched for, which is no longer there (because it is ~/frame_id)

Something else with ControllerInterface

I think you have misinterpreted it. This information is need to get the proper state interfaces within the controller. Please look into the following code

ros2_controllers/imu_sensor_broadcaster/src/imu_sensor_broadcaster.cpp

Lines 48 to 49 in 9e97c00

imu_sensor_ = std::make_unique<semantic_components::IMUSensor>(

semantic_components::IMUSensor(params_.sensor_name));

imu_sensor_broadcaster/src/imu_sensor_broadcaster_parameters.yaml

saikishor · 2024-04-20T17:54:41Z

imu_sensor_broadcaster/src/imu_sensor_broadcaster_parameters.yaml

+  tf_frame_prefix_enable: {
+    type: bool,
+    default_value: true,
+    description:  "Enables or disables appending tf_prefix to tf frame id's.",


I don't understand the need for this new parameter. Can you explain me why it is needed ?

I followed the example of diff_drive. I think you can get rid of it.

saikishor · 2024-04-20T17:57:53Z

imu_sensor_broadcaster/test/test_imu_sensor_broadcaster.cpp

+  sensor_msgs::msg::Imu imu_msg;
+  subscribe_and_get_message(imu_msg, test_namespace);
+
+  EXPECT_EQ(imu_msg.header.frame_id, frame_prefix + frame_id_);


Shouldn't there be a slash in between?

A matter for discussion, but in my opinion the prefix can be arbitrary and the user does not necessarily have to want it to end slash

ok if you want it to be prefixed without a '/' at the end, why don't you parse it directly already appending it to the frame_id? This frame_id is only used for the published message. I don't see a point of adding a tf_prefix, if it is not namespaced.

ros2_controllers/imu_sensor_broadcaster/src/imu_sensor_broadcaster.cpp

Line 66 in 9e97c00

realtime_publisher_->msg_.header.frame_id = params_.frame_id;

What do you think about this?

Co-authored-by: Sai Kishor Kothakota <saisastra3@gmail.com>

rafal-gorecki · 2024-04-22T09:54:46Z

Dear @saikishor ,
I would like to ask for verification, I hope I added all the suggestions and managed to fix the error with the parameters that occurred with the namepatch. Should I rebase this PR and target it to master?

saikishor · 2024-04-22T09:56:47Z

Dear @saikishor , I would like to ask for verification, I hope I added all the suggestions and managed to fix the error with the parameters that occurred with the namepatch. Should I rebase this PR and target it to master?

Yes, please rebase and target it to the master :)

rafal-gorecki · 2024-04-22T10:15:28Z

The easiest way for me to do it was this way, a continuation #1104

…os-controls#1102)

rafal-gorecki added 2 commits April 20, 2024 13:20

tf_prefix: fix slashes and add to IMU

79ead8b

Formatting

38745aa

github-actions bot requested review from DasRoteSkelett, bmagyar, destogl, fmauch, mcbed, progtologist and saikishor April 20, 2024 11:30

saikishor requested changes Apr 20, 2024

View reviewed changes

rafal-gorecki changed the base branch from humble to master April 22, 2024 07:41

rafal-gorecki changed the base branch from master to humble April 22, 2024 07:41

rafal-gorecki and others added 6 commits April 22, 2024 09:57

Remove front slash suggestions

2c434b1

Update imu_sensor_broadcaster/src/imu_sensor_broadcaster_parameters.yaml

21eb1b5

Co-authored-by: Sai Kishor Kothakota <saisastra3@gmail.com>

Add matrix tests

765d8f6

Use matrix in tests

577dd30

Pre-commit

958976d

Solve param issue

18487bc

rafal-gorecki requested a review from saikishor April 22, 2024 09:54

rafal-gorecki mentioned this pull request Apr 22, 2024

tf_prefix param: add to IMU Broadcaster and fix slashes in DiffDrive #1104

Closed

rafal-gorecki closed this Apr 22, 2024

Quick fix namespace with sensor

3c9cf6b

rafal-gorecki reopened this Apr 26, 2024

github-actions bot requested review from aprotyas and erickisos April 26, 2024 09:29

github-actions bot requested a review from VX792 April 26, 2024 09:29

rafal-gorecki changed the title ~~tf_prefix param: fix slashes and add to IMU Broadcaster~~ Backport to Humble tf_prefix param: fix slashes and add to IMU Broadcaster Apr 26, 2024

rafal-gorecki changed the title ~~Backport to Humble tf_prefix param: fix slashes and add to IMU Broadcaster~~ [Humble Backport] tf_prefix param: fix slashes and add to IMU Broadcaster Apr 26, 2024

rafal-gorecki closed this Apr 26, 2024

henrygerardmoore pushed a commit to henrygerardmoore/ros2_controllers that referenced this pull request Jul 19, 2024

Enable services for setting the log-level in controller per default (r…

b7850f3

…os-controls#1102)

	imu_sensor_ = std::make_unique<semantic_components::IMUSensor>(
	semantic_components::IMUSensor(params_.sensor_name));

Comments

Conversation

rafal-gorecki commented Apr 20, 2024

Uh oh!

mergify bot commented Apr 20, 2024

Uh oh!

christophfroehlich commented Apr 20, 2024

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafal-gorecki Apr 22, 2024 • 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!

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!

rafal-gorecki commented Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saikishor commented Apr 22, 2024

Uh oh!

rafal-gorecki commented Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rafal-gorecki Apr 22, 2024 •

edited

Loading

rafal-gorecki commented Apr 22, 2024 •

edited

Loading

rafal-gorecki commented Apr 22, 2024 •

edited

Loading