Add play-for functionality #960

gbiggs · 2022-02-15T12:40:00Z

This PR adds the ability to play for a specified number of seconds.

This work was co-authored by @agalbachicar.

MichaelOrlov

@gbiggs Thank you for your contribution.
I like the idea with adding possibility to play recorded bag file until some specified timestamp.
Although it would be nice to add one more parameter to be able to start playback after some timestamp.

I have some comments to the design.
In particular the way how duration for playback passing to the player. It's passing as separate parameter to the constructor and other method of the class. Which is kind of going against our generic design which is follow the rule that all parameters should be passed via PlayOptions data structure.
Also duration parameter uses std::optional<rcutils_duration_value_t> type with optional wrapper from C++17. While we are currently formally supporting compilation C++17 code we are trying to avoid new C++17 primitives in Rosbag2 at least.
Especially in this particular case it will be better to use rclcpp::Duration data type, and use some constant with negative value for indicating None value.
We have done recently similar feature for delaying playback for some amount of time. Please refer to the #789 and #843. Where we are using rclcpp::Duration for delay value.
Also I think it will be reasonable to define command line parameter to be as floating point value to be able more precisely specify timestamp until which need to playback. Please refer to the example of how it was done for delay parameter in https://github.com/ros2/rosbag2/pull/843/files

Another comment for the API in PlayFor.srv service it would be nice to define it more generic as "Play.srv" and provide two parameters start_time and duration

I also have concern about the way how unit tests written with usage of the newly added spin_subscriptions_for(duration);.
In particular tests TEST_F(RosBag2PlayForTestFixture, play_for_none_are_played_due_to_duration) and TEST_F(RosBag2PlayForTestFixture, play_for_less_than_the_total_duration) are going to be very flaky.
Since relying on the real time delay in tests is non deterministic approach which is always leading to the flakiness on the heavy loaded machines in CI.
Also please use subscription_manager, PublicationManager and wait_for_matched methods to avoid lost messages and flakiness in unit tests.

ros2bag/ros2bag/verb/play.py

rosbag2_transport/CMakeLists.txt

rosbag2_transport/src/rosbag2_transport/player.cpp

rosbag2_transport/test/rosbag2_transport/test_play_for.cpp

gbiggs · 2022-03-16T00:49:41Z

@MichaelOrlov: @agalbachicar has addressed your comments. Can you please re-review?

MichaelOrlov · 2022-03-16T01:43:15Z

@gbiggs I am a bit confused. As far as I can see you add new command line parameter --playback_duration which is hosted inside playback_options and has type rclcpp::Duration which is very good 👍
However the old parameter -duration still there.
Did you forget to remove it?

MichaelOrlov

@gbiggs Thank you for your effort. I think It's get very close to be good to go.
Please see my comments.

rosbag2_interfaces/srv/Play.srv

rosbag2_interfaces/srv/PlayFor.srv

rosbag2_py/src/rosbag2_py/_transport.cpp

rosbag2_test_common/include/rosbag2_test_common/subscription_manager.hpp

rosbag2_transport/src/rosbag2_transport/player.cpp

rosbag2_transport/test/rosbag2_transport/test_play_for.cpp

* Removes duration parameter. A leftover after switching to playback_duration. * Fixes comment. * Solves format in rosbag2_py -> _transport.cpp * Applies style suggestions. * Changes play() to return a boolean indicating whether the request could be fulfilled. * Removes extra unnecessary code.

MichaelOrlov

@gbiggs I have only one nitpick for the implementation, in checking atomic variable.

Although I have a concern in regards to the tests execution time.
I've tried to run newly added tests locally on my machine and it turns out that running time for newly added test_play_for test suite taking around 1 minute. Most of the newly added tests running in 10 sec.
This is too much. Need to redesign tests to run them faster.

I already reworked one last test play_should_return_false_when_interrupted to be deterministic and run faster. See my comments in line.
Please take a look on the idea and rework other tests.

rosbag2_transport/src/rosbag2_transport/player.cpp

rosbag2_transport/test/rosbag2_transport/test_play_for.cpp

* Adresses reviewer's comments. * Improve test time by adding an optional argument to SubscriptionManager::spin_subscriptions() - Reduces test_play_for execution time from 50s to 6s approximately.

agalbachicar · 2022-03-23T19:55:28Z

@MichaelOrlov I see an error in the last build here. Where can I see the build configuration to try to reproduce it and determine whether it is because of this PR or a preexisting problem?

MichaelOrlov · 2022-03-26T07:25:29Z

@agalbachicar I don't quite understand what build configuration you are talking about.
Form the first glance I have a suspicious that tests failures relates to your changes in spin_subscriptions()

MichaelOrlov · 2022-03-26T08:56:13Z

After a more detailed look in the code I think that your changes in spin_subscription doesn't relate to the tests failures.
It looks like failures on the CI baseline.

MichaelOrlov

@agalbachicar @gbiggs It was to tedious making a lot of comments and propose changes on code review.
I prepared all proposed changes on separate branch gbiggs#17

I had a concern that after last changes tests still rely on the assumption that subscription waiting for N messages however in tests valid expectation that it will be published less then N` messages or nothing. And waiting time reduced to the 1 second.
Such assumption tend to make tests false negative or flaky due to the possible delays on CI.

Please consider to merge my changes on your branch as result of the code review.

gbiggs · 2022-03-28T07:43:10Z

@MichaelOrlov Thank you for making those improvements. I have incorporated them into the PR's branch.

MichaelOrlov

@gbiggs Approving now.
However need to fix DCO and run CI pipeline.

* Removes duration parameter. A leftover after switching to playback_duration. * Fixes comment. * Solves format in rosbag2_py -> _transport.cpp * Applies style suggestions. * Changes play() to return a boolean indicating whether the request could be fulfilled. * Removes extra unnecessary code. Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Adresses reviewer's comments. * Improve test time by adding an optional argument to SubscriptionManager::spin_subscriptions() - Reduces test_play_for execution time from 50s to 6s approximately. Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

gbiggs · 2022-03-30T00:17:11Z

CI (build --packages-up-to rosbag2, test --packages-select ros2bag rosbag2_py rosbag2_transport):

Linux
Linux-aarch64
Windows

gbiggs · 2022-03-30T02:45:12Z

The Windows CI errors appear to be infrastructure failures, so I'm going to try again:

MichaelOrlov · 2022-03-30T04:14:39Z

@clalancette We have CI failure on Windows which is unrelated to the changes and kind of relates to the infrastructure.
It seems that it's common for all builds on CI for windows target.
Do you have any clue what it is and when or how it could be fixed?

clalancette · 2022-03-30T12:30:07Z

Hm, I don't know. It is failing to install the RTI connext binaries, but as far as I can tell that happened successfully in the nightlies: https://ci.ros2.org/view/nightly/job/nightly_win_rel/2256 . I'll try it one more time, and if that doesn't work I'll loop in some other people.

gbiggs · 2022-04-12T07:57:13Z

@clalancette Any updates on the CI situation?

clalancette · 2022-04-12T14:35:02Z

@clalancette Any updates on the CI situation?

Oh, that latest build I ran looks like it did the right thing. From an infrastructure issue, it looks like things are working. Any remaining issues will have something to do with this PR.

Some of the warnings in that latest build have already been fixed, but I'm not sure all of them have been. I'd say it's worthwhile doing another run to see how things look now, I'll kick it off:

gbiggs · 2022-04-12T23:44:42Z

The error in the CI is not in the tests, which are all passing. It's an error finding a particular source file, C:\ci\ws\src\ros2\rosbag2\rosbag2_transport\src\rosbag2_transport\qos.cpp.

The CI failure doesn't appear to be related to this PR, although I'm not sure why it's happening. That source file exists in the repository, both in master and in the PR's branch.

clalancette · 2022-04-13T14:26:16Z

The error in the CI is not in the tests, which are all passing. It's an error finding a particular source file, C:\ci\ws\src\ros2\rosbag2\rosbag2_transport\src\rosbag2_transport\qos.cpp.

Geoff and I talked, and I think the problem is that that qos.cpp is being compiled as part of one of the tests again: https://github.com/ros2/rosbag2/pull/960/files#diff-81a326226779dbc5abab3808861f8664841bdafaed9ff22ebf59ae30b6233219R108-R113 . But that shouldn't be necessary, since that file should be getting pulled in as part of the rosbag2_transport library. So my suggestion is to remove that and try CI again.

gbiggs · 2022-04-14T00:33:29Z

I've pushed the change and fired off a CI run.

Linux
Linux-aarch64
Windows

gbiggs · 2022-04-17T23:01:21Z

CI looks good. We can merge this when the freeze ends.

MichaelOrlov · 2022-05-10T23:48:59Z

@clalancette @gbiggs Can we already rebase and merge this PR?
When new feature freeze will end up?

* Removes duration parameter. A leftover after switching to playback_duration. * Fixes comment. * Solves format in rosbag2_py -> _transport.cpp * Applies style suggestions. * Changes play() to return a boolean indicating whether the request could be fulfilled. * Removes extra unnecessary code. Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Adresses reviewer's comments. * Improve test time by adding an optional argument to SubscriptionManager::spin_subscriptions() - Reduces test_play_for execution time from 50s to 6s approximately. Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

gbiggs · 2022-05-11T03:49:24Z

I've rebased the PR to master. Here's another round of CI for that.

Linux
Linux-aarch64
Windows

MichaelOrlov · 2022-06-01T07:09:32Z

@gbiggs Build fail with error:

--- stderr: rosbag2_transport
20:57:20 In file included from �[K/home/jenkins-agent/workspace/ci_linux/ws/src/ros2/rosbag2/rosbag2_transport/src/rosbag2_transport/player.cpp�[K:
20:57:20 �[K/home/jenkins-agent/workspace/ci_linux/ws/src/ros2/rosbag2/rosbag2_transport/include/rosbag2_transport/player.hpp:245:��[Kerro��[KPlay�[K’ is not a member o�[Krosbag2_interfaces::�[K’; did you mea�[KP�[K’?
20:57:20   245 |   rclcpp::Service<rosbag2_interfaces::srv::�[KPlay�[K>::SharedPtr srv_play_for_;
20:57:20       |                                            �[K^~~~�[K
20:57:20       |                                            �[KP�[K
20:57:20 �[K/home/jenkins-agent/workspace/ci_linux/ws/src/ros2/rosbag2/rosbag2_transport/include/rosbag2_transport/player.hpp:245:��[Kerro�[Ktemplate argument 1 is invalid
20:57:20   245 |   rclcpp::Service<rosbag2_interfaces::srv::PlayFor��[K::SharedPtr srv_play_for_;

It seems something wrong with code after rebase.

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

…/960/files (#14) * Add play-for functionality Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Add play-for to the CLI Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net> * Adds playback_duration to PlayOptions. * Changes from PlayFor to Play srv message and changes start_offset and playback_duration. * Restores play_for tests. * Removes extra SubscriptionManager methods. * Solves comment about extra sent message. * Reorders code and comment. * Removes the SKIP_TEST flag. Co-authored-by: Geoffrey Biggs <gbiggs@killbots.net> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Removes duration parameter. A leftover after switching to playback_duration. * Fixes comment. * Solves format in rosbag2_py -> _transport.cpp * Applies style suggestions. * Changes play() to return a boolean indicating whether the request could be fulfilled. * Removes extra unnecessary code. Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Adresses reviewer's comments. * Improve test time by adding an optional argument to SubscriptionManager::spin_subscriptions() - Reduces test_play_for execution time from 50s to 6s approximately. Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

* Redesigned tests to be more deterministic and running faster * Fixed bug in `play_for()` flow when replaying in loop or multiple times from the same player instance. Signed-off-by: Michael Orlov <michael.orlov@apex.ai> Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

gbiggs · 2022-06-01T09:00:23Z

It looks like the rebase resurrected some code that had been deleted, which is what led to the errors. I've corrected them. Here's some CI.

Linux
Linux-aarch64
Windows

MichaelOrlov · 2022-06-01T19:00:04Z

@gbiggs We forgot to handle the play_until_time scenario for play_next API.

Will need to add the same

 if (play_until_time >= starting_time_ && message_ptr->time_stamp > play_until_time) {
      break;
    }

Inside play_next.
Also it will need to move this part https://github.com/ros2/rosbag2/pull/960/files#diff-547fe4e718fdbdb2aa48c572ef5fa0706144c8e90e47890c50199f7d3834ae80R210-R213 to the constructor and make play_until_time as member variable.

Do you prefer to fix it in current PR or create a follow up PR after merging this one?

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

gbiggs · 2022-06-02T01:56:07Z

I think it's easier and faster to just correct the behaviour now, so I've done so.

Linux
Linux-aarch64
Windows

MichaelOrlov · 2022-06-02T05:13:20Z

@gbiggs Thanks for promptly addressing findings.
CI is green, merging then.

ros-discourse · 2022-08-23T20:44:23Z

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

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-8-18-2022/27050/1

gbiggs requested review from emersonknapp and MichaelOrlov February 15, 2022 12:40

gbiggs marked this pull request as ready for review February 15, 2022 12:46

gbiggs requested a review from a team as a code owner February 15, 2022 12:46

gbiggs mentioned this pull request Feb 16, 2022

Add play-until functionality #962

Closed

MichaelOrlov requested changes Feb 17, 2022

View reviewed changes

gbiggs force-pushed the gbiggs/play_for branch from b2f3f96 to aa698ae Compare March 15, 2022 05:31

gbiggs requested a review from MichaelOrlov March 16, 2022 00:49

MichaelOrlov requested changes Mar 16, 2022

View reviewed changes

MichaelOrlov requested changes Mar 18, 2022

View reviewed changes

rosbag2_transport/src/rosbag2_transport/player.cpp Outdated Show resolved Hide resolved

rosbag2_transport/test/rosbag2_transport/test_play_for.cpp Outdated Show resolved Hide resolved

rosbag2_transport/test/rosbag2_transport/test_play_for.cpp Outdated Show resolved Hide resolved

agalbachicar mentioned this pull request Mar 23, 2022

Add play-next-n functionality #963

Closed

MichaelOrlov requested changes Mar 26, 2022

View reviewed changes

MichaelOrlov approved these changes Mar 29, 2022

View reviewed changes

gbiggs force-pushed the gbiggs/play_for branch from 718ac03 to 46edcdd Compare March 30, 2022 00:13

agalbachicar mentioned this pull request May 10, 2022

Adds play until functionality -- follow up to #962 #1005

Merged

gbiggs force-pushed the gbiggs/play_for branch from 59c52ef to 33d1151 Compare May 11, 2022 03:45

gbiggs and others added 8 commits June 1, 2022 07:58

Add play-for functionality

2be261f

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

Add play-for to the CLI

8cefe4c

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

Remove unnecessary source file from test binary

97dd138

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

Correct errors introduced by rebase

5a8a0a7

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

gbiggs force-pushed the gbiggs/play_for branch from 33d1151 to 5a8a0a7 Compare June 1, 2022 08:56

Correct play_next behaviour

38ed671

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

MichaelOrlov merged commit ccafe85 into ros2:master Jun 2, 2022

gbiggs deleted the gbiggs/play_for branch June 2, 2022 22:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add play-for functionality #960

Add play-for functionality #960

gbiggs commented Feb 15, 2022

MichaelOrlov left a comment

gbiggs commented Mar 16, 2022

MichaelOrlov commented Mar 16, 2022

MichaelOrlov left a comment

MichaelOrlov left a comment •

edited

Loading

agalbachicar commented Mar 23, 2022

MichaelOrlov commented Mar 26, 2022

MichaelOrlov commented Mar 26, 2022

MichaelOrlov left a comment

gbiggs commented Mar 28, 2022

MichaelOrlov left a comment

gbiggs commented Mar 30, 2022

gbiggs commented Mar 30, 2022

MichaelOrlov commented Mar 30, 2022

clalancette commented Mar 30, 2022

gbiggs commented Apr 12, 2022

clalancette commented Apr 12, 2022

gbiggs commented Apr 12, 2022

clalancette commented Apr 13, 2022

gbiggs commented Apr 14, 2022

gbiggs commented Apr 17, 2022

MichaelOrlov commented May 10, 2022

gbiggs commented May 11, 2022

MichaelOrlov commented Jun 1, 2022

gbiggs commented Jun 1, 2022

MichaelOrlov commented Jun 1, 2022

gbiggs commented Jun 2, 2022

MichaelOrlov commented Jun 2, 2022

ros-discourse commented Aug 23, 2022

Add play-for functionality #960

Add play-for functionality #960

Conversation

gbiggs commented Feb 15, 2022

MichaelOrlov left a comment

Choose a reason for hiding this comment

gbiggs commented Mar 16, 2022

MichaelOrlov commented Mar 16, 2022

MichaelOrlov left a comment

Choose a reason for hiding this comment

MichaelOrlov left a comment • edited Loading

Choose a reason for hiding this comment

agalbachicar commented Mar 23, 2022

MichaelOrlov commented Mar 26, 2022

MichaelOrlov commented Mar 26, 2022

MichaelOrlov left a comment

Choose a reason for hiding this comment

gbiggs commented Mar 28, 2022

MichaelOrlov left a comment

Choose a reason for hiding this comment

gbiggs commented Mar 30, 2022

gbiggs commented Mar 30, 2022

MichaelOrlov commented Mar 30, 2022

clalancette commented Mar 30, 2022

gbiggs commented Apr 12, 2022

clalancette commented Apr 12, 2022

gbiggs commented Apr 12, 2022

clalancette commented Apr 13, 2022

gbiggs commented Apr 14, 2022

gbiggs commented Apr 17, 2022

MichaelOrlov commented May 10, 2022

gbiggs commented May 11, 2022

MichaelOrlov commented Jun 1, 2022

gbiggs commented Jun 1, 2022

MichaelOrlov commented Jun 1, 2022

gbiggs commented Jun 2, 2022

MichaelOrlov commented Jun 2, 2022

ros-discourse commented Aug 23, 2022

MichaelOrlov left a comment •

edited

Loading