Message Cache Default Interpolation is RCL_SYSTEM_TIME

[zthorson]

I was experimenting with using message filter caches to lookup message data and hit an odd behavior where the interpolation time always needs to be RCL_SYSTEM_TIME. Otherwise you will get a:

terminate called after throwing an instance of 'std::runtime_error' what(): can't compare times with different time sources

It is my understanding that ROS2 design documents intend the preferred timestamp type to be RCL_ROS_TIME. Is this correct?

If so, I can put a pull request in for a one line change of:

message_filters/include/message_filters/message_traits.h

Line 89 in a18a34a

return rclcpp::Time(stamp.sec, stamp.nanosec);

to:

return rclcpp::Time(stamp);

This should use the message contructor instead of the sec/nanosec contructor, which defaults to RCL_ROS_TIME.

Alternately:

return rclcpp::Time(stamp.sec, stamp.nanosec, RCL_ROS_TIME);

[ijnek]

This issue has been raised a while back, but I just came across this issue and the suggested fix seems reasonable. What are your thoughts @gbiggs ?

[tfoote]

That does look like the wrong behavior it should be using the ROS time like the stamp. Either proposed approach would be reasonable.
One is more compact, and could remove the temporary variable, the other is more explicit and doesn't rely on the constructor defaulting behavior.

[gstrommer]

Hi! After updating from ROS2 Galactic to Humble, we get the same exception you mentioned at the beginning:
can't compare times with different time sources

We just subscribe to a nav_msgs::msg::Odometry with message_filters::Subscriber and delay these messages with a message_filters::TimeSequencer for a fixed time. It seems like this exception is thrown within the message_filters package and this change seems to be related to this. Why you don't check the time source of incoming messages and set it accordingly? For me, it seems like this change is not a fix, it just moves the problem to somewhere else.