Skip to content
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

Merged
merged 16 commits into from
Aug 28, 2018
Merged

Callbacks when time jumps #222

merged 16 commits into from
Aug 28, 2018

Conversation

sloretz
Copy link
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
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 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
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
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
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
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
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
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
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
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
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
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
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
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
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

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.

None yet

3 participants