Add time jump callback #608
Conversation
When use_sim_time is set the python tf2 buffer should be cleared. However, this wasn't happening. To handle this a time jump callback was added to clear the buffer when a time jump is detected.
erichlf
force-pushed
the
erichlf/python_time_jump
branch
from
a4c5a3f
to
d4b4d21
Compare
|
This build errors make no sense to me |
tf2_ros_py/tf2_ros/buffer.py
Outdated
| self.jump_handle = rclpy.clock.Clock().create_jump_callback | ||
| ( | ||
| threshold, | ||
| post_callback=self.time_jump_callback | ||
| ) |
There was a problem hiding this comment.
I think this needs to be done in a slightly different way.
In particular, I'm pretty sure that the clock object has to be kept around, otherwise this whole thing will be destroyed immediately.
There are then two paths we have to deal with; when the node is passed in, and when it is not. When it is passed in, we can just use the clock object that is stored in the Node. When it is not, then we have to create and hold onto a clock object ourselves. So something like:
if node is not None:
self.jump_handle = node.get_clock().create_jump_callback(threshold, post_callback=self.time_jump_callback)
else:
self.clock = rclpy.clock.Clock()
self.jump_handle = self.clock.create_jump_callback(threshold, post_callback=self.time_jump_callback)
(that else case is actually problematic since it isn't respecting use_sim_time for simulation; the only way to deal with that would be to make node non-optional here. But we can do that later.)
erichlf
force-pushed
the
erichlf/python_time_jump
branch
from
94da029
to
f709031
Compare
|
These failing tests are baffling. This is definitely valid syntax. |
erichlf
force-pushed
the
erichlf/python_time_jump
branch
2 times, most recently
from
795086c
to
c7f073c
Compare
erichlf
force-pushed
the
erichlf/python_time_jump
branch
from
c7f073c
to
d289c07
Compare
|
@ros-pull-request-builder retest this please |
|
CI:
|
|
thank you @erichlf , I am looking forward to using this! 🙌 |
| @@ -94,6 +104,10 @@ def __get_frames( | |||
| ) -> FrameGraphSrvResponse: | |||
| return FrameGraph.Response(frame_yaml=self.all_frames_as_yaml()) | |||
|
|
|||
| def time_jump_callback(time_jump: TimeJump): | |||
| rclpy.logging.get_logger("tf2_buffer").warning("Detected jump back in time. Clearing tf buffer.") | |||
| self.clear() | |||
There was a problem hiding this comment.
@erichlf @clalancette I'd like to use this functionality, so I gave this a monkey patch test drive on humble but didn't manage to get the callback to trigger, alas. (when my rosbag looped).
Quick debugging: it doesn't seem to get unregistered, which is good, but the callback_shim doesn't seem to get called.
This method doesn't look like it captures self so even if it were triggered I'm not sure this behaves how expected anymore (presumably it was working at some point but I saw there was refactoring during the PR).
Might be in need of some test coverage before the feature gets released 🙂
There was a problem hiding this comment.
OH and I'm willing to admit that there's a slew of more complex reasons why I can't get it to trigger on my side (like, for example, monkey patching onto humble 😆 ). But I'm hoping that the comment is helpful for rolling development nonetheless! 🙇
When use_sim_time is set the python tf2 buffer should be cleared. However, this wasn't happening. To handle this a time jump callback was added to clear the buffer when a time jump is detected.