High TF Broadcast Rate

[outrider-jkoch]

I noticed in this commit (600c55c) that the transform was changed to support working with ROS time. Maybe I am misunderstanding, but I'm confused why a static transform (from metadata) needs to send an TF2 message for every received IMU and Lidar packet. I notice a large quantity of TF2 messages are being sent but never with dynamically changing data. Could this be throttled in anyway or is it necessary to send the TF2 message at this rate?

We have observed this with all of our sensors:
OS-0 and OS-1
Commit: 3c2b6a1

[Samahu]

Data isn’t update but timestamp is. Subscribers like RVIZ would complain about the TF data being too old if we simply use static broadcaster. This is the reason we send the TF messages. Currently, we are sending the TF messages at the same frequency as the IMU and PointCloud messages (which are 100Hz ane 10Hz respectively) but this is perhaps doesn’t need to be the case. I would need to check if reducing the rate of TF messages to 1Hz would be sufficient and wouldn’t cause a warning about data too old.

[Aposhian]

rviz on ROS 2 doesn't warn when static transforms are being published. Even so, publishing messages at high rates is a significant computational expense, and shouldn't be done just to silence warnings. A static transform broadcaster is correct for unchanging transforms

[Samahu]

The reason I update the TF messages even though the TF data don't change was due to a concern raised by one of our users. The message states:

[ WARN] [1668158414.957718441]: odometry: Could not get transform from base_link to os_sensor (stamp=1668158375.848681) after 0.500000 seconds ("wait_for_transform_duration"=0.500000)! Error="Lookup would require extrapolation 38.615832763s into the past. Requested time 1668158375.848681211 but the earliest data is at time 1668158414.464514017, when looking up transform from frame [os_sensor] to frame [base_link]. canTransform returned after 0.504406 timeout was 0.5."

It is a warning level log message and does give the impression that there is a problem (NOTE: the message keeps getting emitted). It is one of those cases where ROS behavior is inconsistent. I am not sure if the problem still exist in ROS2, I haven't tried. But if it doesn't we can drop the dynamic rebroadcast of TF messages.

[Samahu]

Otherwise, I am more inclined towards keeping current code as is but reduce the frequency of TF messages published by the driver to 2-10 Hz.

[Aposhian]

Sending an unchanging value on TF instead of TF static, at least on ROS 2, is incorrect. If that is required to eliminate warnings on ROS, then it should be on ROS, but not ROS 2. ROS 2 should definitely use tf static for this. This is how it is done in the unofficial driver, and this plays nice with rviz and other tools on ROS 2. https://github.com/ros-drivers/ros2_ouster_drivers/blob/5425ed023d3c1c5293a9af90d2ae81c5df889334/ros2_ouster/src/ouster_driver.cpp#L138

[Samahu]

@outrider-jkoch we have addressed the issue for the ROS2 driver, do you mainly use ROS1?

[outrider-jkoch]

We are using ROS1 at the moment but we have some plans to use ROS2 in the future. Until then though having the rate for the TF broadcast reduced would be desirable.

[Samahu]

We are using ROS1 at the moment but we have some plans to use ROS2 in the future. Until then though having the rate for the TF broadcast reduced would be desirable.

Thanks for the feedback, please take a look at #134 and let me know if this seems like a reasonable compromise to you.

[Samahu]

@outrider-jkoch I haven't got any feedback from you about the proposed solution #134 . Since I don't see any harm in it and that it does bring down the TF publish rate from 120+ Hz to 20-40Hz I went ahead and merged the solution. Let us know if the updated TF publish rate still poses a problem on your end.

[outrider-jkoch]

I think this is an improvement from what it was. However, I'm still skeptical that switching from static_transform_broadcaster to transform_broadcaster was correct. As @Aposhian said with regards to ROS2, sending an unchanged transform on /tf versus /tf_static is incorrect. I didn't look much into the problem that was experienced in #9 but there seems to be a problem with latching the transform in that context that would need more investigation.

[Samahu]

I think this is an improvement from what it was. However, I'm still skeptical that switching from static_transform_broadcaster to transform_broadcaster was correct. As @Aposhian said with regards to ROS2, sending an unchanged transform on /tf versus /tf_static is incorrect. I didn't look much into the problem that was experienced in #9 but there seems to be a problem with latching the transform in that context that would need more investigation.

Yeah, I am not arguing that but at the same time because of the issue reported in #9 - which I have experienced it myself when working with RVIZ - I am not inclined atm to change it back to static tf broadcaster for the ROS1 driver unless there is a compelling reason. We did switch back, however, for the ROS2 version of the driver.