-
Notifications
You must be signed in to change notification settings - Fork 248
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add play-for functionality #960
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
@MichaelOrlov: @agalbachicar has addressed your comments. Can you please re-review? |
@gbiggs I am a bit confused. As far as I can see you add new command line parameter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gbiggs Thank you for your effort. I think It's get very close to be good to go.
Please see my comments.
rosbag2_test_common/include/rosbag2_test_common/subscription_manager.hpp
Outdated
Show resolved
Hide resolved
* 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
@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? |
@agalbachicar I don't quite understand what build configuration you are talking about. |
After a more detailed look in the code I think that your changes in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
@MichaelOrlov Thank you for making those improvements. I have incorporated them into the PR's branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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>
@clalancette We have CI failure on Windows which is unrelated to the changes and kind of relates to the infrastructure. |
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. |
@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: |
The error in the CI is not in the tests, which are all passing. It's an error finding a particular source file, 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 |
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. |
CI looks good. We can merge this when the freeze ends. |
@clalancette @gbiggs Can we already rebase and merge this PR? |
* 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 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>
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>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
@gbiggs We forgot to handle the Will need to add the same if (play_until_time >= starting_time_ && message_ptr->time_stamp > play_until_time) {
break;
} Inside 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 Thanks for promptly addressing findings. |
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 |
This PR adds the ability to play for a specified number of seconds.
This work was co-authored by @agalbachicar.