tf_prefix param: add to IMU Broadcaster and fix slashes in DiffDrive #1104
Conversation
github-actions
bot
requested review from
bijoua29,
bmagyar,
christophfroehlich,
DasRoteSkelett,
destogl,
erickisos and
jaron-l
|
@saikishor FYI |
|
Hi @christophfroehlich @saikishor, |
rafal-gorecki
changed the title
| std::string tf_prefix = ""; | ||
| if (!params_.tf_frame_prefix.empty()) | ||
| { | ||
| tf_prefix = params_.tf_frame_prefix; | ||
| } | ||
| else | ||
| { | ||
| tf_prefix = std::string(get_node()->get_namespace()); | ||
| if (tf_prefix != "/") | ||
| { | ||
| tf_prefix += '/'; | ||
| } | ||
| } | ||
| if (tf_prefix.front() == '/') | ||
| { | ||
| tf_prefix.erase(0, 1); | ||
| } | ||
|
|
There was a problem hiding this comment.
Hello!
While reviewing this part again, I started to question myself, if we need this or not. I'm saying because in case of the diff_drive_controller, the odom message that is being published is a dynamic tf frame, but in case of the imu_state_broadcaster, as per the imu you always have a static transform link inside the URDF, I don't know if it make sense to have the tf_prefix in this case.
Right now, your changes will add namespace of the controller manager to the links, I'm not sure how right is it.
Does it make sense?. Should we check if the link exists from the loaded URDF?
There was a problem hiding this comment.
Hi @saikishor,
Thank you, for reply!
I'm not sure what difference it makes whether fame is dynamic or static in the case of tf_prefix. I treat it as an option that may be useful to someone. What I care about most is the ability to correctly assign a frame to a robot when many robots are launched.
I'm still wondering about the issue of sensor_name, because while in the case of a real robot there should be no problems, in the simulation you need to create sensor objects with different namespaces.
There was a problem hiding this comment.
Hello!
I think it is rather very important, because the sensor readings should correspond to a specific frame on the robot and this might be important. Regarding multi robot i agree that we would need this separation, then we can use the namespace, but why tf_prefix?. Moreover, i believe we have to make sure that it is a valid link.
Can you explain to me the instances that's a requirement for you?
There was a problem hiding this comment.
If you are asking specifically about tf_prefix, I don't need it, but I added it to unify arg in the packages.
As for checking whether a frame exists, it makes sense.
There was a problem hiding this comment.
What do you think about the problem with sensor_name? The simplest solution is to add a parameter that adds a namespace to sensor_name or add a sensor name prefix.
Generally, what I care about is being able to run many robots in simulation and on hardware while making a minimum number of changes to the code.
There was a problem hiding this comment.
@rafal-gorecki ok now I got the use-case. Yes, it is tricky to make it work with simulation with this namespace. What can be done is probably, instead of using the tf_prefix, just add it to the sensor_name already. I'm not sure how to automatically configure it based on the robot. The current changes however, doesn't fix it right?
Any ideas on how to solve it are welcome
There was a problem hiding this comment.
Thank you, so much for today. I will try to add this functionality soon. I would like to ask you to confirm whether this tf_prefix can be left for imu in order to standardize the packages, or should I get rid of it?
There was a problem hiding this comment.
Let's discuss your idea on how you are planning to tackle the idea of sensor name automatically, and then it's better to start changing
There was a problem hiding this comment.
- The best solution is probably to add some kind of standard that make
robot_state_publisherwith namespace add to alllinkreference_frameandsensor topicnamespaces. - Or add
namespaceargument to URDF and put it in all resonable field.
Then add the argument add_namespace_to_sensor_name (bool)
@saikishor what do you think?
There was a problem hiding this comment.
Second solution added
Related with #1102, rabase to master.