[Humble Backport] tf_prefix param: fix slashes and add to IMU Broadcaster #1102
Conversation
github-actions
bot
requested review from
bmagyar,
DasRoteSkelett,
destogl,
fmauch,
mcbed,
progtologist and
saikishor
|
@rafal-gorecki, all pull requests must be targeted towards the |
|
Is this a possible duplicate of #1060? |
| else | ||
| { | ||
| tf_prefix = tf_prefix + "/"; | ||
| tf_prefix.erase(0, 1); |
There was a problem hiding this comment.
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
| # FIXME: Currently does not work with namespace | ||
| # validation: { | ||
| # not_empty<>: null | ||
| # } |
There was a problem hiding this comment.
Can you explain a bit more in what situation it doesn't work.
There was a problem hiding this comment.
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 emptyThe 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
There was a problem hiding this comment.
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
imu_sensor_broadcaster/src/imu_sensor_broadcaster_parameters.yaml
Outdated
Show resolved
Hide resolved
| tf_frame_prefix_enable: { | ||
| type: bool, | ||
| default_value: true, | ||
| description: "Enables or disables appending tf_prefix to tf frame id's.", |
There was a problem hiding this comment.
I don't understand the need for this new parameter. Can you explain me why it is needed ?
There was a problem hiding this comment.
I followed the example of diff_drive. I think you can get rid of it.
| 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_); |
There was a problem hiding this comment.
Shouldn't there be a slash in between?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
What do you think about this?
|
Dear @saikishor , |
Yes, please rebase and target it to the master :) |
|
The easiest way for me to do it was this way, a continuation #1104 |
rafal-gorecki
changed the title
rafal-gorecki
changed the title
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.