Design for rosbag2 handling of clock/simtime

[emersonknapp]

Designs #99
Designs #55

Adds a design doc (in a new design folder) which lays out how time should be handled in rosbag2 for all anticipated playback cases.

[Karsten1987]

Thanks for kicking this off. I have two major questions I'd like to have sorted out before starting to implement this:

• How does the /clock publisher frequency influence the overall system if the messages have a potential higher frequency.
• What does pause/resume mean in terms of recording and playing back (are we pausing time or pausing the player/recorder).

[emersonknapp]

@Karsten1987 @piraka9011 I've added a lot of clarification, please let me know if there's anything else I should add!

[emersonknapp]

@jaisontj @zmichaels11 @Karsten1987 @piraka9011 @mjeronimo friendly ping :) I need to get started implementing if we want to get any/all of this proposal into Galactic. I've just pushed a new revision that comes out of starting to implement and get a feel for the API.

I plan to start start opening PRs approximately in line with the "Implementation Staging" notes in the design - so if there are any high level concerns, those would be good to hear ASAP.

[Karsten1987]

This doc sounds pretty solid to me! Thanks for putting it together.

[emersonknapp]

Thanks @david-prody and @Karsten1987 for the extra comments! I've done another clarifying and pseudocode-ing pass. I also added in the concept of some "factory constructors" to differentiate the external-source vs time-control configuration modes of the clock.

The revisions have gotten small enough that I'll start opening PRs against this design, and will plan to merge this design in 48 hours if there are no major concerns.

[ros-discourse]

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/tooling-wg-breakout-session-rosbag2-playback-time-control/19711/1

[Karsten1987]

This came up during the migration of the GenericPublisher to rclcpp, but I believe starting from Foxy there's two timestamps exposed in the message info for each incoming message. It's a time-sent and a time-received stamp, which we should probably leverage within rosbag2. I don't know the actual state of this, saying if all the RMWs are actually really setting these two fields, but if so, we could think about using them.
That being said, we most likely might have to expose this as a flag to the player - whether to use sent or received stamps - but I wanted to bring this up because it might be related to your design.

c.f. ros2/rclcpp#1604

[emersonknapp]

Thanks! That's useful information - we will probably want to use the "sent" timestamp for playback, to be more faithful to the original. That could be an update to this design - but it could also be an update to recording, if we choose only to store one of the timestamps, then the "sent" time might make more sense than the "received" tmie.

[Karsten1987]

One more idea I wanted to vocalize here before I forget them. We can't certainly discuss this in more detail in the WG meeting.

What if I wanted to replay a bagfile in a sort of software-lock-step fashion, in which the system is synchronized and can be replayed deterministically. What this means to me is that we don't synchronize the system on a time source, but rather on a data driven source. Essentially what I mean here is that I'd like to have a way of replaying a message once at a time. Independent of time, simply sequentially. Imagine I have velocity commands recorded for a mobile robot and I want to replay each command given an external trigger - which is not time related but rather some other event from my system. Whenever that external signal comes, the next message will be replayed.

[emersonknapp]

I'd like to have a way of replaying a message once at a time

This sounds like the "play next message while paused" feature that @MichaelOrlov is working on implementing. We can extend this design to call it out specially. I think it can be implemented like follows:

return_code Player::play_next() {
  if (!clock->is_paused()) return failure;
  clock->jump(message_queue.front().time_stamp);
  return success;
}

If we keep the clock paused and jump ahead by message timestamps, the Player will be locked in with the trigger. Combined with topic filters, this would allow playing back message-by-message for subsets of topics, or just a single topic.

Extending the implementation started in #704, I would expect it to look like

PlayerClock::sleep_until(ROSTime ros_until) {
  // Called by "Player::play_messages_until_queue_empty" from the Player thread
  Lock lock(mutex);
  // Called before the "jump", knows what time we need to sleep until
  SteadyTime steady_until = ros_to_steady(ros_until);
  // if this wakes up before "jump" is called, then the loop will just immediately call "sleep_until" again
  // if "jump" is called during this sleep, it notifies the variable so we wake up
  condition_variable.wait_until(lock, steady_until);
  // since the jump set us to ros_until explicitly, this returns true, so "play_messages_until_queue_empty" plays the next message
  return now() >= ros_until;
}


PlayerClock::jump(ROSTime time) {
  // called by "Player::play_next" from separate callback thread via Service, keyboard, GUI, whatever
  // update references
  snapshot(time);
  // wake up any current sleepers
  condition_variable.notify_all();
}

Based on the above, I think that with the current development approach this feature can be explained with a small design note, and implemented with a small PR.

Do you envision more cases that we will need to account for?

[MichaelOrlov]

@emersonknapp I see one potential caveat. It looks like we are going to have "busy loop" when in pause.
I judged based on the logic described in comment.

// if this wakes up before "jump" is called, then the loop will just immediately call "sleep_until" again

It's going to bump processor core utilization close to 90-100% when in pause.

[emersonknapp]

No - it won't, because sleep_until will sleep for the amount of time between "now" and "until". It still sleeps this amount of time when paused, and then returns false, then is called again.

However, I do see that, using the above pseudocode, it would loop at a speed relative to the amount of time until the next message, which isn't the best behavior. This is an implementation detail though that can be easily solved, the this design discussion doesn't suggest that this should be a tight busy-loop - it should be a controlled wakeup loop at a much slower rate.

[MichaelOrlov]

I was worry about that when we are in pause mode

SteadyTime steady_until = ros_to_steady(ros_until);

Will return the same time stamp as a previous call since clock stopped.
And steady_until will have timestamp in the past relative to the current clock.
i.e.

condition_variable.wait_until(lock, steady_until);

will return immediately and function will return false since (now() < ros_until) and call for sleep_until(ROSTime ros_until) will be repeated again and again from the upper level. Effectively we will not have a sleep at all in pause mode.

Am I missed something?

[MichaelOrlov]

We can actually check that we are in pause mode and call

condition_variable.wait_for(lock, 5mSec)

in case if pause mode.

[emersonknapp]

We can actually check that we are in pause mode and call

Yes, that would be a good start for the implementation.

steady_until will have timestamp in the past relative to the current clock.

You might be right about there being a tight loop case in the current WIP code - I'll make sure to avoid it before marking them ready for review.