-
Notifications
You must be signed in to change notification settings - Fork 224
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Callbacks when time jumps #222
Conversation
Taking out of review, changes coming |
callbacks on ROS time activation or deactivation callbacks on forward time jumps callbacks on backward time jumps
956c663
to
8c50120
Compare
rclpy/rclpy/clock.py
Outdated
original_callback(TimeJump(clock_change, duration)) | ||
|
||
if post_callback is not None and callable(post_callback): | ||
post_callback = callback_shim |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the callback_shim
function only be defined when it is necessary within the if
block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rclpy/rclpy/clock.py
Outdated
handler = JumpHandle( | ||
clock=self._clock_handle, threshold=threshold, pre_callback=pre_callback, | ||
post_callback=post_callback) | ||
return handler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: return the `JumpHandle(...) directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rclpy/src/rclpy/_rclpy.c
Outdated
clock_change = "RCL_SYSTEM_TIME_NO_CHANGE"; | ||
break; | ||
default: | ||
PyErr_Format(PyExc_RuntimeError, "Unknown time jump type"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: including the unexpected value in the message helps if the problem occurs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rclpy/test/test_clock.py
Outdated
pre_callback2.assert_called() | ||
post_callback2.assert_called() | ||
pre_callback2.assert_called() | ||
post_callback2.assert_called() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these two lines supposed to check 3
instead of 2
?
Same below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 1859f95
rclpy/test/test_time_source.py
Outdated
post_cb = Mock() | ||
threshold = JumpThreshold( | ||
min_forward=Duration(seconds=0.5), min_backward=None, on_clock_change=False) | ||
handler = clock.create_jump_callback( # noqa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: only suppress the expected code instead of everything.
Same below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed suppression in 4a0b4de
I noticed that the tests added in this PR seem to fail on xenial release nightlies: ros2/build_farmer#147 |
This adds callbacks when ROSTime jumps. The implementation closely mimics the implementation in
rclcpp
.Time jump callbacks use the
rcl_clock_t
time jump callbacks. Python code is called in the callbacks usingPyObject_CallObject()
.Requires ros2/rcl#284