Publish clock after delay is over and disable delay on next loops

[nicolaloi]

• Fixes Rosbag2 play --clock option ignores delay #1858.

clock_publish_timer is now initialized with autostart = false, so it doesn't immediately start publishing the /clock topic. It is "activated" later with clock_publish_timer->reset() at the beginning of the playback thread, ensuring that the (optional) user-defined delay period has passed.

Additionally, the clock_ now starts in a paused state if a delay time is specified by the user. It resumes after the delay period has ended.

[fujitatomoya]

lgtm with green CI.

@MichaelOrlov could you have 2nd review?

[MichaelOrlov]

@nicolaloi Thanks for the PR. Overall looks good among one undefined behavior / race introducing with the

            if (!play_options_.start_paused) {
              clock_->resume();
            }

in Play method.

[nicolaloi]

Ah yes, the clock will resume automatically at the next loop regardless of user requests.

Then to not overcomplicate things, I would suggest removing the "pause clock until delay is over" feature from this PR, and keeping the "publish clock after the delay is over" feature only, which is the actual fix for issue #1858. Pausing the clock is not required to fix this specific issue since anyway the clock_publish_timer_ was starting to publish after the clock was resumed.

I was pausing the clock as a comprehensive "inner" change for the Player class when in delayed status, but it is not required for the "outer" behavior of the timed published clock. If this "pause clock until delay is over" feature is desired, it can be a separate PR.

[nicolaloi]

I was pausing the clock as a comprehensive "inner" change for the Player class when in delayed status, but it is not required for the "outer" behavior of the timed published clock. If this "pause clock until delay is over" feature is desired, it can be a separate PR.

Removing

if (!play_options_.start_paused) {
       clock_->resume();
}

to keep the "pause clock until delay is over" feature only, but if it is not okay I can reintegrate and try to fix this "pause clock until delay is over" feature in this PR too.

Extra change: now before sleeping for the delay period, the clock_publish_timer_->cancel() is called. This does not affect the first standard loop (since clock_publish_timer_ has autostart=false), however for the subsequent (optional) loops this prevents the clock from being published until the player exits again the subsequent delayed periods (to be consistent with the first delay period).

[fujitatomoya]

keeping the "publish clock after the delay is over" feature only, which is the actual fix for issue #1858

i prefer this approach, one thing at a time. and easier to backport to downstream distros.

and the question here is that clock needs to be paused in delay duration? i am not quite sure about what state delay is...
in the implementation, it is playing state in PlayerImpl::play during duration period, but we cannot pause the sleep duration at all.

[MichaelOrlov]

@nicolaloi Thanks for addressing the comments from the review.
It looks good to me in the current form. However, I would move the delay out of the playback loop. IMO, there is no value in making a delay in the loop after we have finished the playback round.
Delay needs for the purporse to be able to wait for other nodes (subscribers and other dependend nodes in the pipeline) to start and be discoverable. When we have run the first playback round all other nodes still up un running. Thereofre, there is no need to make a delay on a consequent rounds of replay.

[nicolaloi]

Okay, I will incorporate the change to the delay loop behavior in this PR.

Need to invoke clock_->jump(starting_time_) before the loop, before starting the clock_publish_timer, but I am not sure if locking the mutex during the call is needed also in this pre-loop case.

[MichaelOrlov]

@nicolaloi Thanks for the updates.
I think now it would be safe to init clock in pause mode in constructor and resume it after the delay. i.e.

 clock_ = std::make_unique<rosbag2_cpp::TimeControllerClock>(
      starting_time_, std::chrono::steady_clock::now,
      std::chrono::milliseconds{100},
      play_options_.start_paused || play_options_.delay > rclcpp::Duration(0, 0));

Also may I ask you in future do a separate commit after a round of review instead of push force the branch with the the same changed commit?
We are squashing all commits before merging to the target branch after review anyway automatically. However, the separate commits allow to keep history of changes during review.

[MichaelOrlov]

@nicolaloi Could you please also rebase your branch on top of the latest rolling branch?
We made some changes. Now we support multiple readers.

[MichaelOrlov]

@nicolaloi Thanks for the PR and for addressing the review comments. LGTM now.

[MichaelOrlov]

Pulls: #1861
Gist: https://gist.githubusercontent.com/MichaelOrlov/b0a96eb6a82f324d25e2f267ed68985d/raw/a84f0c1d7aca1838d0241fbdfc44c78d16a91c18/ros2.repos
BUILD args: --packages-above-and-dependencies ros2bag rosbag2_transport
TEST args: --packages-above ros2bag rosbag2_transport
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14914

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[MichaelOrlov]

Re-run Windows CI job since previous failure is unrelated

ERROR: Failed building wheel for PyQt5-sip

• Windows

[MichaelOrlov]

https://github.com/Mergifyio backport jazzy

[mergify]

backport jazzy

✅ Backports have been created

• #1878 Publish clock after delay is over and disable delay on next loops (backport #1861) has been created for branch jazzy