Add delay option by kosuke55 · Pull Request #789 · ros2/rosbag2

kosuke55 · 2021-06-23T14:19:50Z

Hi, thank you for your work.
In this PR I added --delay option related to #786.
If we run for the following, the rosbag will play after a 5 second sleep.

ros2 bag play <rosbag2 file> -d 5

It also works with --loop like

ros2 bag play <rosbag2 file> -d 5 -l

zmichaels11

Thanks for your PR!

I left a couple comments/suggestions.

rosbag2_transport/include/rosbag2_transport/play_options.hpp

zmichaels11 · 2021-06-23T16:52:16Z

rosbag2_transport/src/rosbag2_transport/player.cpp

  is_in_play_ = true;
  try {
    do {
+      float delay =


Can you move delay out of the loop? I don't think it can change between iterations.

I'd also suggest raising a warning or exception when delay is an invalid value. That way the user knows when they enter an invalid value.

I moved it out of the loop. And raising warning and set to 0 if dealy value is invalid. 0b4687f

zmichaels11 · 2021-06-23T16:53:48Z

rosbag2_transport/test/rosbag2_transport/test_play_loop.cpp

  const float rate = 1.0;
  const bool loop_playback = false;
+  double clock_publish_frequency = 0.0;
+  const float delay = 1.0;


Can you also test for an invalid delay value?

I added tests for delay in dfee76a.

danthony06 · 2021-06-24T03:45:08Z

I know at one point there was a plan to implement node lifecycles for rosbag. Is this still the case, because this seems like a case where it would fit in really well.

kosuke55 · 2021-06-24T05:30:08Z

@zmichaels11
Thaks for your review. I modified so please check them.

emersonknapp · 2021-06-24T19:52:10Z

ros2bag/ros2bag/verb/play.py

        play_options.loop = args.loop
        play_options.topic_remapping_options = topic_remapping
        play_options.clock_publish_frequency = args.clock
+        play_options.delay=args.delay


linter is failing on this line - it will help you to run the tests locally before putting up for review!

Thaks for your review and advice. I modified it and checked that test_flake8.py paased. 1c0cbff

emersonknapp · 2021-06-24T20:03:59Z

I know at one point there was a plan to implement node lifecycles for rosbag. Is this still the case, because this seems like a case where it would fit in really well.

There isn't an active effort going on - I don't personally have a concrete idea what this would look like. We could take that discussion to another issue if we want to pick it up. With some quick diagrams or something showing what we would want.

emersonknapp · 2021-06-28T18:29:31Z

Looking very close - you will need to sign your commits for the DCO check to pass. You can do that with git rebase master --signoff

kosuke55 · 2021-06-30T04:37:18Z

@emersonknapp
Oh, sorry and thaks. I did that.

emersonknapp · 2021-06-30T21:17:35Z

Gist: https://gist.githubusercontent.com/emersonknapp/df22ce71e23066faa49f1ad4b777faee/raw/e66eb02dae79a8dbe4184fb2c6bf28be493f1741/ros2.repos
BUILD args: --packages-up-to ros2bag rosbag2_py rosbag2_transport rosbag2 rosbag2_tests
TEST args: --packages-select ros2bag rosbag2_py rosbag2_transport rosbag2 rosbag2_tests
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/8646

Linux
Linux-aarch64
macOS
Windows

emersonknapp · 2021-06-30T23:37:15Z

The Windows runners are very slow - and so timing-based tests don't always work the way you would expect there. Is there any deterministic way to check on this test, without depending on actual timing? (worst case, we could just expand the error margin to accommodate these slow runners)

kosuke55 · 2021-07-01T01:56:59Z

@emersonknapp
hmm, I can't think of any other way than using the actual time.
This test was made with reference to other parts. Other tests have a fairly large margin .

rosbag2/rosbag2_transport/test/rosbag2_transport/test_play_timing.cpp

Lines 122 to 123 in c8b1032

    
           ASSERT_THAT(replay_time, Gt(0.5 * message_time_difference)); 
        
           ASSERT_THAT(replay_time, Lt(message_time_difference));

I don't think there are many cases where we actually want to delay by 0.x seconds, so how about simply increasing the margin (1.0, etc.)?

kosuke55 · 2021-07-01T01:57:49Z

And this test is included in the rate test, but it's better to separate them.

Signed-off-by: kosuke55 <kosuke.tnp@gmail.com>

rosbag2_transport/include/rosbag2_transport/play_options.hpp

Signed-off-by: kosuke55 <kosuke.tnp@gmail.com>

MichaelOrlov · 2021-07-02T07:08:27Z

rosbag2_transport/test/rosbag2_transport/test_play_timing.cpp

+    auto reader = std::make_unique<rosbag2_cpp::Reader>(std::move(prepared_mock_reader));
+    auto player = std::make_shared<rosbag2_transport::Player>(
+      std::move(
+        reader), storage_options_, play_options_);


Nitpick. It would be better if

std::move( reader), storage_options_, play_options_);

would be on one line

std::move(reader), storage_options_, play_options_);

OK, I modified all same part in 75eca9f.

Signed-off-by: kosuke55 <kosuke.tnp@gmail.com>

emersonknapp · 2021-07-02T21:43:24Z

Linux
Linux-aarch64
macOS
Windows

kosuke55 · 2021-07-05T15:04:51Z

@emersonknapp
Is there anything else I need to fix in this code? If not, I would like to send a same PR for the galactic branch as well.

emersonknapp · 2021-07-06T21:38:36Z

No - nothing left. I just didn't look at it over the holiday weekend. Please feel free to open a Galactic backport PR

* Add delay option Signed-off-by: kosuke55 <kosuke.tnp@gmail.com>

Backport #789 to galactic * Add delay option Signed-off-by: kosuke55 <kosuke.tnp@gmail.com>

kosuke55 requested a review from a team as a code owner June 23, 2021 14:19

kosuke55 requested review from Karsten1987 and zmichaels11 and removed request for a team June 23, 2021 14:19

zmichaels11 reviewed Jun 23, 2021

View reviewed changes

kosuke55 force-pushed the feature/delay branch from 820a09a to 688fc5a Compare June 24, 2021 03:42

zmichaels11 approved these changes Jun 24, 2021

View reviewed changes

emersonknapp reviewed Jun 24, 2021

View reviewed changes

kosuke55 force-pushed the feature/delay branch from 1c0cbff to c8b1032 Compare June 30, 2021 04:36

emersonknapp approved these changes Jun 30, 2021

View reviewed changes

kosuke55 force-pushed the feature/delay branch from fcb1514 to 598463f Compare July 1, 2021 03:10

kosuke55 added 9 commits July 1, 2021 12:22

Add delay option

4e0375d

Signed-off-by: kosuke55 <kosuke.tnp@gmail.com>

Add statement of delay

e3ce267

Signed-off-by: kosuke55 <kosuke.tnp@gmail.com>

Add invalid delay warning and move it out of loop

dc96bae

Signed-off-by: kosuke55 <kosuke.tnp@gmail.com>

Fix format using Uncrustify

b18b1ca

Signed-off-by: kosuke55 <kosuke.tnp@gmail.com>

Not raising warn if delay is 0

0c5fde3

Signed-off-by: kosuke55 <kosuke.tnp@gmail.com>

Add delay tests

c544824

Signed-off-by: kosuke55 <kosuke.tnp@gmail.com>

Fix format

39b399e

Signed-off-by: kosuke55 <kosuke.tnp@gmail.com>

Separate delay test from rate test

a2e7aae

Signed-off-by: kosuke55 <kosuke.tnp@gmail.com>

Use large delay margin

5383a5e

Signed-off-by: kosuke55 <kosuke.tnp@gmail.com>

kosuke55 force-pushed the feature/delay branch from 598463f to 5383a5e Compare July 1, 2021 03:23

MichaelOrlov reviewed Jul 2, 2021

View reviewed changes

rosbag2_transport/include/rosbag2_transport/play_options.hpp Outdated Show resolved Hide resolved

Modfiy description of delay

f7d5d60

Signed-off-by: kosuke55 <kosuke.tnp@gmail.com>

MichaelOrlov reviewed Jul 2, 2021

View reviewed changes

Fix format of std::move

75eca9f

Signed-off-by: kosuke55 <kosuke.tnp@gmail.com>

emersonknapp merged commit bd665cb into ros2:master Jul 6, 2021

kosuke55 added a commit to kosuke55/rosbag2 that referenced this pull request Jul 7, 2021

Add delay option (ros2#789)

fde004b

* Add delay option Signed-off-by: kosuke55 <kosuke.tnp@gmail.com>

kosuke55 mentioned this pull request Jul 7, 2021

[backport galactic] Add delay option (#789) #812

Merged

wep21 deleted the feature/delay branch July 7, 2021 17:33

emersonknapp pushed a commit that referenced this pull request Jul 8, 2021

[backport galactic] Add delay option (#789) (#812)

e6f4d76

Backport #789 to galactic * Add delay option Signed-off-by: kosuke55 <kosuke.tnp@gmail.com>

clalancette mentioned this pull request Apr 29, 2022

ROS 2 Humble needed release notes ros2/ros2_documentation#2446

Closed

87 tasks

Conversation

kosuke55 commented Jun 23, 2021

Uh oh!

zmichaels11 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danthony06 commented Jun 24, 2021

Uh oh!

kosuke55 commented Jun 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emersonknapp commented Jun 24, 2021

Uh oh!

emersonknapp commented Jun 28, 2021

Uh oh!

kosuke55 commented Jun 30, 2021

Uh oh!

emersonknapp commented Jun 30, 2021

Uh oh!

emersonknapp commented Jun 30, 2021

Uh oh!

kosuke55 commented Jul 1, 2021

Uh oh!

kosuke55 commented Jul 1, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emersonknapp commented Jul 2, 2021

Uh oh!

kosuke55 commented Jul 5, 2021

Uh oh!

emersonknapp commented Jul 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants