Skip to content

Callbacks when time jumps#222

Merged
sloretz merged 16 commits intomasterfrom
time_jump_callbacks
Aug 28, 2018
Merged

Callbacks when time jumps#222
sloretz merged 16 commits intomasterfrom
time_jump_callbacks

Conversation

@sloretz
Copy link
Copy Markdown
Contributor

@sloretz sloretz commented Aug 10, 2018

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 using PyObject_CallObject().

Requires ros2/rcl#284

@sloretz sloretz added enhancement New feature or request in review Waiting for review (Kanban column) labels Aug 10, 2018
@sloretz sloretz self-assigned this Aug 10, 2018
@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Aug 15, 2018

Taking out of review, changes coming

@sloretz sloretz added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Aug 15, 2018
callbacks on ROS time activation or deactivation
callbacks on forward time jumps
callbacks on backward time jumps
@sloretz sloretz force-pushed the time_jump_callbacks branch from 956c663 to 8c50120 Compare August 28, 2018 00:06
@sloretz sloretz added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Aug 28, 2018
@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Aug 28, 2018

CI just rclpy because I don't expect changes to affect anything downstream.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

original_callback(TimeJump(clock_change, duration))

if post_callback is not None and callable(post_callback):
post_callback = callback_shim
Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handler = JumpHandle(
clock=self._clock_handle, threshold=threshold, pre_callback=pre_callback,
post_callback=post_callback)
return handler
Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clock_change = "RCL_SYSTEM_TIME_NO_CHANGE";
break;
default:
PyErr_Format(PyExc_RuntimeError, "Unknown time jump type");
Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pre_callback2.assert_called()
post_callback2.assert_called()
pre_callback2.assert_called()
post_callback2.assert_called()
Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 1859f95

post_cb = Mock()
threshold = JumpThreshold(
min_forward=Duration(seconds=0.5), min_backward=None, on_clock_change=False)
handler = clock.create_jump_callback( # noqa
Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed suppression in 4a0b4de

@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Aug 28, 2018

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@sloretz sloretz merged commit dce1750 into master Aug 28, 2018
@sloretz sloretz removed the in review Waiting for review (Kanban column) label Aug 28, 2018
@sloretz sloretz deleted the time_jump_callbacks branch August 28, 2018 22:00
@dhood
Copy link
Copy Markdown
Member

dhood commented Sep 13, 2018

I noticed that the tests added in this PR seem to fail on xenial release nightlies: ros2/build_farmer#147

@tfoote tfoote mentioned this pull request Sep 20, 2018
8 tasks
ryantqiu pushed a commit to snorkel-marlin-repos/ros2_rclpy_pr_222_5b5d8f40-2caf-46ba-8ab3-51323a0e57e9 that referenced this pull request Oct 1, 2025
Original PR #222 by sloretz
Original: ros2/rclpy#222
ryantqiu added a commit to snorkel-marlin-repos/ros2_rclpy_pr_222_5b5d8f40-2caf-46ba-8ab3-51323a0e57e9 that referenced this pull request Oct 1, 2025
Merged from original PR #222
Original: ros2/rclpy#222
ryantqiu pushed a commit to snorkel-marlin-repos/ros2_rclpy_pr_222_c0c34138-671f-4cb5-a12e-1cf2e8dc5b10 that referenced this pull request Oct 2, 2025
Original PR #222 by sloretz
Original: ros2/rclpy#222
ryantqiu added a commit to snorkel-marlin-repos/ros2_rclpy_pr_222_c0c34138-671f-4cb5-a12e-1cf2e8dc5b10 that referenced this pull request Oct 2, 2025
Merged from original PR #222
Original: ros2/rclpy#222
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants