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.

Sign up for GitHub

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

Jump to bottom

Add PlayerClock::wakeup() to interrupt sleeping #1869

Merged
merged 2 commits into from
Dec 3, 2024
Merged

Add PlayerClock::wakeup() to interrupt sleeping #1869

merged 2 commits into from
Dec 3, 2024

Conversation

christophebedard
Copy link
Member

@christophebedard christophebedard commented Nov 25, 2024 •
edited
Loading

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.

@christophebedard christophebedard self-assigned this Nov 25, 2024
@christophebedard christophebedard requested a review from a team as a code owner November 25, 2024 22:54
@christophebedard christophebedard requested review from emersonknapp, james-rms and MichaelOrlov and removed request for a team, emersonknapp and james-rms November 25, 2024 22:54
@christophebedard christophebedard mentioned this pull request Nov 25, 2024
Merged
MichaelOrlov
MichaelOrlov reviewed Nov 27, 2024
View reviewed changes
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.

@christophebedard Thanks for addressing my previous comments.
Now, overall looks good. However, it seems I found one missed ROSBAG2_CPP_PUBLIC qulifier.

rosbag2_cpp/include/rosbag2_cpp/clocks/player_clock.hpp Show resolved Hide resolved
@christophebedard christophebedard force-pushed the christophebedard/clock-add-wakeup-method branch from 190649f to 49a0e93 Compare November 27, 2024 16:38
@MichaelOrlov
Copy link
Contributor

Pulls: #1869
Gist: https://gist.githubusercontent.com/MichaelOrlov/2c001ee0885d084435cdba39959dbf63/raw/87904b154664acc97ea6b9ca957eeab0ee8fe9b3/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_cpp rosbag2_transport
TEST args: --packages-above rosbag2_cpp rosbag2_transport
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14875

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

@christophebedard
Copy link
Member Author

christophebedard commented Nov 29, 2024 •
edited
Loading

As mentioned in #1848 (comment), we'll merge this after merging #1848.

@christophebedard christophebedard force-pushed the christophebedard/clock-add-wakeup-method branch from 49a0e93 to 347585d Compare December 2, 2024 17:29
@christophebedard
Copy link
Member Author

As mentioned in #1848 (comment), we'll merge this after merging #1848.

It has been merged, so I rebased this PR on rolling and adapted this PR for changes from #1848, see 347585d.

MichaelOrlov
MichaelOrlov approved these changes Dec 2, 2024
View reviewed changes
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.

LGTM.

BTW. we likely will be able to backport it to Jazzy if we will remove virtual keyword for newly added functions during backporting.

@MichaelOrlov
Copy link
Contributor

MichaelOrlov commented Dec 2, 2024 •
edited by christophebedard
Loading

Pulls: #1869
Gist: https://gist.githubusercontent.com/MichaelOrlov/9e91a8d8d83d3812cff842102f69b308/raw/87904b154664acc97ea6b9ca957eeab0ee8fe9b3/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_cpp rosbag2_transport
TEST args: --packages-above rosbag2_cpp rosbag2_transport
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14890

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

@MichaelOrlov
Copy link
Contributor

Two new warnings on Windows are in the StaticSingleThreadedExecutor and unrelated to the changes from this PR

'rclcpp::executors::StaticSingleThreadedExecutor::~StaticSingleThreadedExecutor': Use rclcpp::executors::SingleThreadedExecutor (compiling source file C:\ci\ws\src\ros2\rclcpp\rclcpp\test\rclcpp\executors\test_executors.cpp)

@MichaelOrlov MichaelOrlov merged commit c8feaea into rolling Dec 3, 2024
12 checks passed
@MichaelOrlov MichaelOrlov deleted the christophebedard/clock-add-wakeup-method branch December 3, 2024 00:29
@MichaelOrlov
Copy link
Contributor

https://github.com/Mergifyio backport jazzy

@mergify Mergify
Copy link

mergify bot commented Dec 3, 2024 •
edited
Loading

backport jazzy

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Dec 3, 2024
@christophebedard @mergify
Add PlayerClock::wakeup() to interrupt sleeping (#1869)
5e0385e 
* 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 mentioned this pull request Dec 3, 2024
Merged
MichaelOrlov added a commit that referenced this pull request Dec 3, 2024
@mergify @christophebedard @MichaelOrlov
[jazzy] Add PlayerClock::wakeup() to interrupt sleeping (backport #1869…
6091bc7 
…) (#1875)

* Add PlayerClock::wakeup() to interrupt sleeping (#1869)

* 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)

* Make backporting PR ABI compatible

- 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]>

---------

Signed-off-by: Michael Orlov <[email protected]>
Co-authored-by: Christophe Bedard <[email protected]>
Co-authored-by: Michael Orlov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichaelOrlov MichaelOrlov MichaelOrlov approved these changes

Assignees

@christophebedard christophebedard

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@christophebedard @MichaelOrlov