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

Revert "Add Clock::sleep_until method (#1748)" #1793

Merged
merged 1 commit into from
Oct 5, 2021
Merged

Conversation

Blast545
Copy link
Contributor

@Blast545 Blast545 commented Oct 1, 2021

This reverts commit 81df584.

Attempts to address nighly_osx_debug test regressions.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
This reverts commit 81df584.

Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
@Blast545 Blast545 force-pushed the blast545_revert1748 branch from 0cf60cd to fb643a0 Compare October 1, 2021 18:42
@clalancette
Copy link
Contributor

@emersonknapp FYI.

@Blast545
Copy link
Contributor Author

Blast545 commented Oct 1, 2021

I'll wait until I verify that the PR also introduced the osx regressions before checking this PR ready for review.

  • ci_osx_debug @ master Build Status

  • ci_osx_debug + revert PR 1748 Build Status

@Blast545
Copy link
Contributor Author

Blast545 commented Oct 4, 2021

CI looks OK, but it's been some days, rebuilding JIC:

  • ci_osx_debug @ master Build Status

  • ci_osx_debug + revert PR 1748 Build Status

@Blast545 Blast545 marked this pull request as ready for review October 4, 2021 18:10
@Blast545
Copy link
Contributor Author

Blast545 commented Oct 4, 2021

Standard CI up to rclcpp:

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

@Blast545 Blast545 requested a review from clalancette October 4, 2021 18:12
@clalancette
Copy link
Contributor

@ros-pull-request-builder retest this please

@Blast545 Blast545 merged commit d04319a into master Oct 5, 2021
@delete-merged-branch delete-merged-branch bot deleted the blast545_revert1748 branch October 5, 2021 15:14
@emersonknapp
Copy link
Collaborator

Noted - i think reverting was the right move, better to get back to green.

sloretz added a commit that referenced this pull request Nov 3, 2021
sloretz added a commit that referenced this pull request Nov 3, 2021
This reverts commit d04319a.

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
sloretz added a commit that referenced this pull request Nov 5, 2021
* Revert "Revert "Add Clock::sleep_until method (#1748)" (#1793)"

This reverts commit d04319a.

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Context, Shutdown Callback, Condition Var per call

The `Clock` doesn't have enough information to know which Context should
wake it on shutdown, so this adds a Context as an argument to
sleep_until().

Since the context is per call, the shutdown callback is also registered
per call and cannot be stored on impl_.

The condition_variable is also unique per call to reduce spurious
wakeups when multiple threads sleep on the same clock.

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Throw if until has wrong clock type

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* unnecessary newline

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Shorten line

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Fix time jump thresholds and add ROS time test

Use -1 and 1 thresholds because 0 and 0 is supposed to disable the
callbacks

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Shorten line

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* rclcpp::ok() -> context->is_valid()

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* No pre-jump handler instead of noop handler

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* If ros_time_is_active errors, let it throw

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Get time source change from callback to avoid race if ROS time toggled quickly

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Fix threshold and no  pre-jump callback

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Use RCUTILS_MS_TO_NS macro

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Explicit cast for duration to system time

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Throws at the end of docblock

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Add tests for invalid and non-global contexts

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Bi0T1N pushed a commit to Bi0T1N/rclcpp that referenced this pull request Dec 15, 2021
This reverts commit 81df584.

Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
Bi0T1N pushed a commit to Bi0T1N/rclcpp that referenced this pull request Dec 15, 2021
* Revert "Revert "Add Clock::sleep_until method (ros2#1748)" (ros2#1793)"

This reverts commit d04319a.

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Context, Shutdown Callback, Condition Var per call

The `Clock` doesn't have enough information to know which Context should
wake it on shutdown, so this adds a Context as an argument to
sleep_until().

Since the context is per call, the shutdown callback is also registered
per call and cannot be stored on impl_.

The condition_variable is also unique per call to reduce spurious
wakeups when multiple threads sleep on the same clock.

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Throw if until has wrong clock type

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* unnecessary newline

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Shorten line

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Fix time jump thresholds and add ROS time test

Use -1 and 1 thresholds because 0 and 0 is supposed to disable the
callbacks

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Shorten line

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* rclcpp::ok() -> context->is_valid()

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* No pre-jump handler instead of noop handler

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* If ros_time_is_active errors, let it throw

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Get time source change from callback to avoid race if ROS time toggled quickly

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Fix threshold and no  pre-jump callback

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Use RCUTILS_MS_TO_NS macro

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Explicit cast for duration to system time

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Throws at the end of docblock

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Add tests for invalid and non-global contexts

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants