Playback pause feature

[mabelzhang]

Usage

ros2 bag play <bagfile> --paused

Press space bar during playback to pause/unpause.

Sample output with numeric printout for readability:

$ ros2 bag play rosbag2_2020_04_02-18_23_54/ --paused
[INFO] [1592375522.558032913] [rosbag2_storage]: Opened database 'rosbag2_2020_04_02-18_23_54//rosbag2_2020_04_02-18_23_54_0.db3' for READ_ONLY.
[INFO] [1592375522.664716232] [rosbag2_transport]: Paused
 [INFO] [1592375523.266395695] [rosbag2_transport]: Resumed
publishing message 1
publishing message 2
publishing message 3
publishing message 4
publishing message 5
publishing message 6
 [INFO] [1592375524.766716700] [rosbag2_transport]: Paused
 [INFO] [1592375525.879586586] [rosbag2_transport]: Resumed
publishing message 7
publishing message 8
publishing message 9
publishing message 10
publishing message 11
publishing message 12
 [INFO] [1592375527.379393673] [rosbag2_transport]: Paused
 [INFO] [1592375528.161453555] [rosbag2_transport]: Resumed
publishing message 13
publishing message 14
publishing message 15
publishing message 16
publishing message 17
publishing message 18
 [INFO] [1592375529.661435598] [rosbag2_transport]: Paused
 [INFO] [1592375530.413582545] [rosbag2_transport]: Resumed
publishing message 19
publishing message 20

Implementation

Implementation is by changing Rosbag2Node to a LifecycleNode.
Tests run one thread for playing and another for activating/deactivating the lifecycle node.
Keyboard handling is in a separate thread from playback in play.cpp.

Feedback needed

• Keyboard interaction and terminal modification are copied directly from ROS 1 bag player.cpp. I don't have a Windows setup to test this, and the keyboard interaction has to be manually tested, right? Could someone help me test this in Windows, or is there a way to programmatically test the interaction? I looked in ROS 1 test_rosbag and did not find anything like that.
• Currently the sleep time to wait for key press is 30 ms. While the period of pause is taken into account, I don't know how rigorous the timestamp of the original bag file is followed, given that there is delay in activate/deactivate, pause/resume sleep time. The messages in test_play_timing are timed 1 second apart currently. I could try to add a more closely timed test if needed. Or I can decrease the sleep time that waits for a lifecycle state change.

I tried to use the pause feature to fix the recorded_messages_are_played_for_filtered_topics test, which publishes 3 messages and expects 2. We were hoping if we pause it in the beginning, we can expect 3. Unfortunately, it was not fixed. Locally, the Cyclone test passes always, but the FastRTPS test fails about 1 out of 4, even if I wait for 2 seconds for the subscriber to come up.

Future work

• Currently only "Paused" and "Resumed" are printed. In ROS 1, the timestamp is printed at all times during playback, and the timestamp freezes when paused. Once the timestamp is implemented for rosbag2, it would be useful to mirror ROS 1 behavior.

[mabelzhang]

Had to fix DCO 13 commits back X_X

[Karsten1987]

I think this could be rebased on top of master to limit the amount of commits.

[mabelzhang]

Okay, so sounds like we want to break this into 2 PRs (as opposed to 3), one for lifecycle pause/resume, and another for keyboard handling.

I did some refactoring, and I realized it doesn't make too much sense to move the command line --paused to a 3rd PR, because the lifecycle node is activated initially using that flag in PlayOptions. The tests also use that flag to create a few different cases to test pausing and resuming.

Currently, other than directly accessing the rosbag2_transport::Rosbag2Node lifecycle node in code, there is no way to pause/resume, since the nodes don't show up in ros2 lifecycle. So if we decouple the --paused command line flag from this PR, then the feature is not really useful to anyone. With the command line flag, at least the user can pause at the beginning (which was the reason for this PR), they just cannot resume during playback. The 2nd PR with keyboard handling will add the capability of pausing and resuming during playback.

I have removed keyboard handling from this PR. If people are okay with just 2 PRs, I will create the 2nd PR with the keyboard handling. If we want to break off the command line option into a 3rd PR, then the tests will need to be simplified, only to be added back later.

[danthony06]

This would be a really nice feature to have back, is there anything I can do to help move the progress along?

[mabelzhang]

@danthony06 Actually there is! Thank you for asking. I'm getting pulled to a different project with a tight timeline for the next 2-3 weeks at least, and most of my rosbag time has been spent on trying to fix a Windows debug mode failure (don't ask me how that's going). We would really like to get the LifeCycle node feature in, so any help on that end is appreciated!

Specifically, I have not had time to address Karsten's PR comments above. I think the most time-consuming is to learn about which states of a lifecycle node can transfer to which other state? Then some logic checking the states can be put in to make sure this comment #443 (comment) is addressed.

Other than that, my other lingering concern is that the lifecycle nodes don't show up in ros2 lifecycle nodes on the command line, which no one has replied yet. It would be good to look into why that is the case, and whether it is needed for lifecycle functions to be called. It could be that it's not needed? Then all is well.

Then I think all the comments above would be addressed more or less. We are planning on just getting the LifeCycle stuff in first, and move all the keyboard-related things to a separate PR later. That is a second area that we can use help on. Let me know if anything is unclear.

This PR currently comes from my fork, which is inconvenient for other contributors. As the PR already has some comments, hmm you can either open a PR to my fork (not ideal), or maybe you can base your stuff on my fork and when you're ready, we can close this PR? Anyway, we can figure that out.

[danthony06]

@mabelzhang Okay, I have some available time, and I'll start working on it and work through the issues you raised.

[mabelzhang]

@danthony06 Awesome thanks!! Let us know if you run into blockers.

[Karsten1987]

@danthony06 please apologize, your comment flew under my radar. Also, I feel Github renders the chronological order of inline comments weird, so let me just answer here:

With respect to your first comment, the ROS 1 bag implementation had a time_translator class that took care of manipulating the time and keeping track of time pauses and associated math. Do you think this is a better approach?

I think it makes sense to abstract the timing behavior out of the player class, which handles things like the duration we've been paused or the ratio in which messages are being replayed. I guess we can also leverage this later on when replaying with simulated time.
I am generally just concerned whether "sleeping" is the right way of seeking for the next message to be ready, but I guess we can roll this way as for now. How do you envision the time_translator class API to look like? Returning a delta for each loop until when we sleep?

[Kaju-Bubanja]

What is the status of this PR? Is it working on Ubuntu? I tried to checkout the PR and recompiled rosbag2, but I get TypeError: 'paused' is an invalid keyword argument for this function

[danthony06]

I think I have a fix and I'm testing it now.

…

On Wed, Jan 13, 2021, 9:54 AM Kaju-Bubanja ***@***.***> wrote: What is the status of this PR? Is it working on Ubuntu? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#443 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBZWHB6XDQSEG7IG7ZWPFDSZW65DANCNFSM4OAJTD6Q> .

[Kaju-Bubanja]

Sorry to bother you again, did you get around to test the fix you have? Can I assist with something? If yes, could you brief me shortly what tasks still need to be done?

[danthony06]

I'm planning on spending the rest of the week on it.

…

On Wed, Jan 20, 2021, 3:58 AM Kaju-Bubanja ***@***.***> wrote: Sorry to bother you again, did you get around to test the fix you have? Can I assist with something? If yes, could you brief me shortly what tasks still need to be done? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#443 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBZWHGIJVGE7UADHY75G3LS22SL5ANCNFSM4OAJTD6Q> .

[Kaju-Bubanja]

I realized that I forgot to compile rosbag2_transport. Now I can pause the bag at the beginning, but as mentioned above there is no way to resume or pause the bag during playback. I checked out the PRs/Issues of ros2cli but didn't find anything related to this PR. As I read the comments from @mabelzhang the code to pause was once here but got factored out into ros2cli? @danthony06 are you working on the keyboard handling? If yes in what repo/PR/issue? Also is there the ability to skip 1 frame ahead with a key? This is kinda a blocker for me now and I will be working on it in the coming weeks, so any guidance would be appreciated

[danthony06]

I've got a test branch set up on my fork of the repo. I'd appreciate help testing it out.

…

On Mon, Feb 1, 2021, 2:41 AM Kaju-Bubanja ***@***.***> wrote: I realized that I forgot to compile rosbag2_transport. Now I can pause the bag at the beginning, but as mentioned above there is no way to resume or pause the bag during playback. I checked out the PRs/Issues of ros2cli but didn't find anything related to this PR. As I read the comments from @mabelzhang <https://github.com/mabelzhang> the code to pause was once here but got factored out into ros2cli? @danthony06 <https://github.com/danthony06> are you working on the keyboard handling? If yes in what repo/PR/issue? Also is there the ability to skip 1 frame ahead with a key? This is kinda a blocker for me now and I will be working on it in the coming weeks, so any guidance would be appreciated — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#443 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBZWHH3XW2FZMUNVYWIETDS4ZSMVANCNFSM4OAJTD6Q> .

[Karsten1987]

@danthony06 Can you open a PR against @mabelzhang 's PR here? That way we have all the modifications and conversations in one PR rather than distributed across multiple contributors.

[mabelzhang]

This PR is from my fork (didn't have access to this repo then). Do you want me to move it to this repo?

[danthony06]

I can open a PR. I'd really appreciate help testing it as well.

…

On Thu, Feb 4, 2021, 10:10 PM Mabel Zhang ***@***.***> wrote: This PR is from my fork (didn't have access to this repo then). Do you want me to move it to this repo? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#443 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBZWHCS3TLF3O5AXMRO4MTS5NVR3ANCNFSM4OAJTD6Q> .

[emersonknapp]

This PR is from my fork (didn't have access to this repo then). Do you want me to move it to this repo?

I think that would make sense so that others could contribute to it - you have access to push a branch here?

[mabelzhang]

Looks like I do. Branch pushed to this repo: https://github.com/ros2/rosbag2/tree/start_paused_lifecycle

[danthony06]

Fixed my PR to the main repo here: #666

[emersonknapp]

Closing in favor of #666