Add playback rate command line arg

[mabelzhang]

Addresses #290

Both -r and -s were taken, so I wasn't sure what is the next best letter to use for rate / speed. I just put in -x for this first implementation.

Examples of usage:

$ ros2 bag play -x 2.5 rosbag2_2020_01_15-21_25_18
$ ros2 bag play -x 0.5 rosbag2_2020_01_15-21_25_18
$ ros2 bag play -x 1 rosbag2_2020_01_15-21_25_18

with updated usage message:

$ ros2 bag play -h
usage: ros2 bag play [-h] [-s STORAGE] [-r READ_AHEAD_QUEUE_SIZE] [-x RATE]
                     bag_file

ros2 bag play

positional arguments:
  bag_file              bag file to replay

optional arguments:
  -h, --help            show this help message and exit
  -s STORAGE, --storage STORAGE
                        storage identifier to be used, defaults to "sqlite3"
  -r READ_AHEAD_QUEUE_SIZE, --read-ahead-queue-size READ_AHEAD_QUEUE_SIZE
                        size of message queue rosbag tries to hold in memory
                        to help deterministic playback. Larger size will
                        result in larger memory needs but might prevent delay
                        of message playback.
  -x RATE, --rate RATE  rate at which to play back messages

Signed-off-by: Mabel Zhang mabel@openrobotics.org

[oceanusxiv]

So this is speculation on my part, but what would happen if the user enters a negative rate, nothing in the options forbid this and it would I think result in the player playing all messages as fast as possible with the current implementation. Maybe add a check on the sign of the rate?

[emersonknapp]

Like @eric1221bday mentioned, the input isn't being validated. I think the requirement we want is rate > 0 because 0 seems unecessary and negative seems like a potentially desirable feature but is not implemented in this PR

Also, could you include a unit test for this feature? Some simple smoke test at least

[Karsten1987]

just throwing in an idea here: We could also remove the -r from the read ahead queue. This would clearly break stuff, but I am also convinced that -r will be used more frequently for rate than for the read ahead queue.

[emersonknapp]

-r will be used more frequently for rate than for the read ahead queue.

Agreed, i think it should be fine to have the queue be long-form only

[mabelzhang]

bbb4215 should address all the comments above -

• removed -r from --read-ahead-queue-size and use for --rate.
• added range > 0.0 check
• added tests

[Karsten1987]

@mabelzhang Can you run CI on this? I want to make sure we don't have regression on OSX and Windows with this.

[mabelzhang]

• Linux
• Linux-aarch64
• macOS
• Windows

[Karsten1987]

new round of CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[mabelzhang]

Oh oops didn't see that. I just started another round with a better .repos file heh

• Linux
• Linux-aarch64
• macOS
• Windows

[Karsten1987]

@mabelzhang you might have to rebase your work on top of master.

[mabelzhang]

New set of CIs. The previous one was building on eloquent branch for many repos, I must have copied from the wrong ros2.repos file.

• Linux
• Linux-aarch64
• macOS
• Windows

[mabelzhang]

Finally

• Linux
• Linux-aarch64
• macOS
• Windows

The Windows cmake just has this mysterious complaint that some other builds also have:

No project() command is present. The top-level CMakeLists.txt file must

[Karsten1987]

Can you point to that other project which also has this warning? Are you aware of any ticket tracking this issue already?

I just want to avoid any regression introduced by this patch.

[mabelzhang]

So the warning is this one:
https://ci.ros2.org/job/ci_windows/9516/cmake/new/
and the history graph looks like up and down from 0 to 1 alternately.

This earlier build with the same warning is just building plain ros2.repos with no additional flags:
https://ci.ros2.org/job/ci_windows/9512/cmake/new/

This one has the same warning too:
https://ci.ros2.org/job/ci_windows/9509/cmake/new/
This is the earliest one I see. There are other builds that say 1 warning in the graph, but when I check their page, they say 0, like 9506, 9500, 9499. I'm not sure why it's like that.

I don't know if a ticket is tracking this.

[Karsten1987]

@mabelzhang with #323 being merged, do you mind rebasing your branch and re-run CI? I am confident that CI should return all green then and this PR finally being ready to get merged ;)

[mabelzhang]

Hurray

• Linux
• Linux-aarch64
• macOS
• Windows

[Karsten1987]

lgtm. @mabelzhang feel free to merge.