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

[jazzy] Add PlayerClock::wakeup() to interrupt sleeping (backport #1869) #1875

Merged
merged 2 commits into from
Dec 3, 2024

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented Dec 3, 2024

This adds a new wakeup() method to PlayerClock. It wakes up the clock, which is (only) useful if it is currently sleeping, i.e., in a sleep_until() call. Just calling std::condition_variable::notify_all() on the clock's condition variable wakes it up from its sleep (i.e., std::condition_variable::wait_for() and std::condition_variable::wait_until()):

The thread will be unblocked when notify_all() or notify_one() is executed, or abs_time is reached. It may also be unblocked spuriously.

This can then replace the current workaround in Player::stop() to force-wakeup the clock, which is to resume the clock and then immediately pause it. Since we don't actually want to pause the clock, doing this may have unintended consequences, e.g., if the player is woken up and then thinks it's not in pause mode anymore.

I want to use this in #1848, but I thought a separate PR would be easier to review.


This is an automatic backport of pull request #1869 done by Mergify.

* Add PlayerClock::wakeup() to interrupt sleeping

Signed-off-by: Christophe Bedard <[email protected]>

* Use PlayerClock::wakeup() in new play_next() impl

Signed-off-by: Christophe Bedard <[email protected]>

---------

Signed-off-by: Christophe Bedard <[email protected]>
(cherry picked from commit c8feaea)
@mergify mergify bot requested a review from a team as a code owner December 3, 2024 04:43
@mergify mergify bot requested review from MichaelOrlov and hidmic and removed request for a team December 3, 2024 04:43
@MichaelOrlov MichaelOrlov changed the title Add PlayerClock::wakeup() to interrupt sleeping (backport #1869) [jazzy] Add PlayerClock::wakeup() to interrupt sleeping (backport #1869) Dec 3, 2024
- Make "is_sleeping()" and " wakeup()" functions as non-virtual and
defined only in the derived "TimeControllerClock" class.
- Use "TimeControllerClock" directly instead of the "PlayerClock" base
class in the player.cpp.

Signed-off-by: Michael Orlov <[email protected]>
Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

I made backporting PR ABI-compatible

  • Made "is_sleeping()" and " wakeup()" functions as non-virtual and
    defined only in the derived "TimeControllerClock" class.
  • Uses "TimeControllerClock" directly instead of the "PlayerClock" base
    class in the player.cpp.

@MichaelOrlov MichaelOrlov requested review from christophebedard and removed request for hidmic December 3, 2024 05:04
@MichaelOrlov
Copy link
Contributor

Pulls: #1875
Gist: https://gist.githubusercontent.com/MichaelOrlov/024215e861b5d29c57ff0307548bcbbb/raw/8513d6ff4baab7e22cabb56e455efa3d8d09854e/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_cpp rosbag2_transport
TEST args: --packages-above rosbag2_cpp rosbag2_transport
ROS Distro: jazzy
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14900

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@MichaelOrlov MichaelOrlov merged commit 6091bc7 into jazzy Dec 3, 2024
12 checks passed
@MichaelOrlov MichaelOrlov deleted the mergify/bp/jazzy/pr-1869 branch December 3, 2024 17:36
Copy link
Member

@christophebedard christophebedard left a comment

Choose a reason for hiding this comment

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

here's an after-the-fact approval ✔️

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.

2 participants