Add play-until functionality #962

gbiggs · 2022-02-16T06:31:11Z

This PR adds the ability to play until a given timestamp.

This work was co-authored by @agalbachicar.

This PR requires #960 to be merged first (at which point it will probably need to be rebased).

Signed-off-by: Geoffrey Biggs <[email protected]>

MichaelOrlov · 2022-02-17T00:08:56Z

@gbiggs Let's finish review for #960 first.

hidmic

First pass!

hidmic · 2022-02-25T14:02:13Z

rosbag2_py/src/rosbag2_py/_transport.cpp

+    "play", &rosbag2_py::Player::play, py::arg("storage_options"), py::arg(
+      "play_options"), py::arg("duration") = std::nullopt)
+  .def(
+    "play_until", &rosbag2_py::Player::play, py::arg("storage_options"), py::arg(


@gbiggs should be rosbag2_py::Player::play_until instead?

hidmic · 2022-02-25T14:04:33Z

rosbag2_py/src/rosbag2_py/_transport.cpp

+      "play_options"), py::arg("duration") = std::nullopt)
+  .def(
+    "play_until", &rosbag2_py::Player::play, py::arg("storage_options"), py::arg(
+      "play_options"), py::arg("timestamp") = std::nullopt)


@gbiggs hmm, shouldn't timestamp be mandatory for play_until()?

hidmic · 2022-02-25T14:13:39Z

rosbag2_transport/src/rosbag2_transport/player.cpp

+      const rcutils_duration_value_t duration =
+      static_cast<rcutils_duration_value_t>(request->duration.sec) *
+      static_cast<rcutils_duration_value_t>(1000000000) +
+      static_cast<rcutils_duration_value_t>(request->duration.nanosec);


@gbiggs nit: consider

Suggested change

const rcutils_duration_value_t duration =

static_cast<rcutils_duration_value_t>(request->duration.sec) *

static_cast<rcutils_duration_value_t>(1000000000) +

static_cast<rcutils_duration_value_t>(request->duration.nanosec);

const rcutils_duration_value_t duration =

rclcpp::Duration(request->duration).nanoseconds();

hidmic · 2022-02-25T14:14:48Z

rosbag2_transport/src/rosbag2_transport/player.cpp

+      const rcutils_time_point_value_t timestamp =
+      static_cast<rcutils_time_point_value_t>(request->time.sec) *
+      static_cast<rcutils_time_point_value_t>(1000000000) +
+      static_cast<rcutils_time_point_value_t>(request->time.nanosec);


@gbiggs nit: consider

Suggested change

const rcutils_time_point_value_t timestamp =

static_cast<rcutils_time_point_value_t>(request->time.sec) *

static_cast<rcutils_time_point_value_t>(1000000000) +

static_cast<rcutils_time_point_value_t>(request->time.nanosec);

const rcutils_time_point_value_t timestamp =

rclcpp::Time(request->time).nanoseconds();

hidmic · 2022-02-25T14:16:35Z

rosbag2_transport/src/rosbag2_transport/player.cpp

    // Do not move on until sleep_until returns true
    // It will always sleep, so this is not a tight busy loop on pause


@gbiggs this should probably be relocated after the next if clause and before the while loop.

hidmic · 2022-02-25T14:28:55Z

rosbag2_transport/test/rosbag2_transport/test_play_for.cpp

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


@gbiggs nit: can we collapse this two lines into one?

hidmic · 2022-02-25T14:31:24Z

rosbag2_transport/test/rosbag2_transport/test_play_for.cpp

+{
+  auto await_received_messages = sub_->spin_subscriptions();
+
+  player_->play(std::chrono::nanoseconds(std::chrono::milliseconds(1000)).count());


@gbiggs fyi: you could also do

Suggested change

player_->play(std::chrono::nanoseconds(std::chrono::milliseconds(1000)).count());

player_->play(RCUTILS_MS_TO_NS(1000));

or equivalently

Suggested change

player_->play(std::chrono::nanoseconds(std::chrono::milliseconds(1000)).count());

player_->play(RCUTILS_S_TO_NS(1));

Same everywhere else.

hidmic · 2022-02-25T14:32:56Z

rosbag2_transport/test/rosbag2_transport/test_play_for.cpp

+  auto replayed_test_primitives = sub_->get_received_messages<test_msgs::msg::BasicTypes>(
+    kTopic1);


@gbiggs nit:

Suggested change

auto replayed_test_primitives = sub_->get_received_messages<test_msgs::msg::BasicTypes>(

kTopic1);

auto replayed_test_primitives =

sub_->get_received_messages<test_msgs::msg::BasicTypes>(kTopic1);

Same elsewhere.

hidmic · 2022-02-25T14:46:46Z

rosbag2_transport/test/rosbag2_transport/test_play_for.cpp

+  std::future<void> await_received_messages_;
+};
+
+TEST_F(RosBag2PlayForTestFixture, play_for_all_are_played_due_to_duration)


@gbiggs nit^N: test case names read a bit awkward. For instance, how about play_for_a_long_duration_publishes_some here?

hidmic · 2022-02-25T14:50:01Z

rosbag2_transport/test/rosbag2_transport/test_play_until.cpp

+  EXPECT_THAT(replayed_test_primitives, SizeIs(0u));
+
+  auto replayed_test_arrays = sub_->get_received_messages<test_msgs::msg::Arrays>("/topic2");
+  // All messages should have arrived.


@gbiggs leftover?

agalbachicar · 2022-03-23T20:10:59Z

Before proceeding to fix this PR, I'd like to know your preference on the implementation @MichaelOrlov and @hidmic . We proposed and discussed this in the meeting group on 2021-05-21. The feature request changed a bit to be: "play until timepoint" rather than "play until next message of topic T" since then.

Also, we needed to have this functionality in another rosbag2 fork first that should be compliant with foxy. As a consequence, this patch looks so odd at first (likewise to #960 at first). We aimed for API compatibility or minimal changes across ros2 releases. That said, we are eager to push to get these changes in before the code freeze (as everyone else contributing :)) and hoping you could make some time to guide us to get these changes in.

Looking forward to hearing your thoughts.

MichaelOrlov · 2022-04-01T01:20:20Z

@agalbachicar @gbiggs I think you will need to rebase first and rework this PR since we already have play_messages_from_queue(play_until_time); API implemented in #960.
Implementation for this PR is going to be sort of a light-way.
I assume will need to add new parameter for command line and in playback options data structure. Further in Player::Play() use two parameters play_for and play_until to determine which time is latter, to use it as parameter for existing play_messages_from_queue(play_until_time); call.
Please take in to consideration the way how we workout unit tests in #960 and use similar approach.

gbiggs · 2022-05-10T22:30:37Z

Closing in favour of #1005

* Implements play until on top of play for. Signed-off-by: Agustin Alba Chicar <[email protected]> * Adds python bindings Signed-off-by: Agustin Alba Chicar <[email protected]> * Adds test for play until. Signed-off-by: Agustin Alba Chicar <[email protected]> * Fixes variable name. Signed-off-by: Agustin Alba Chicar <[email protected]> * Adds the same docstring for play_until and play_duration options than in PlayOptions to clarify usage. Signed-off-by: Agustin Alba Chicar <[email protected]> * Updates the CLI to combine the seconds and nanoseconds part of the playback_until stamp Signed-off-by: Agustin Alba Chicar <[email protected]> * Changes to play verb parameters: - Unifies the use of '-' instead of '_' to split words in arguments. - Uses mutually exclusive arguments: --playback-until-sec and --playback-until-nsec to pass the timestamp. Signed-off-by: Agustin Alba Chicar <[email protected]> * Solves review comments. - Adds test cases to the player. - Renames play_until and similar occurrences to contain timestamp. - Renames play_for and similar occurrences to not contain for and include duration. Signed-off-by: Agustin Alba Chicar <[email protected]> * Solves review comments to tests. Signed-off-by: Agustin Alba Chicar <[email protected]> * Modifies play_until_is_equal_to_the_total_duration The test body now evaluates a bag whose duration is exactly the same as given timestamp in PlayOptions. Signed-off-by: Agustin Alba Chicar <[email protected]>

gbiggs added 3 commits February 15, 2022 21:37

Add play-for functionality

f6bdeb5

Signed-off-by: Geoffrey Biggs <[email protected]>

Add play-for to the CLI

b2f3f96

Signed-off-by: Geoffrey Biggs <[email protected]>

Add play-until functionality

841608a

Signed-off-by: Geoffrey Biggs <[email protected]>

gbiggs added the enhancement New feature or request label Feb 16, 2022

gbiggs requested a review from a team as a code owner February 16, 2022 06:31

gbiggs self-assigned this Feb 16, 2022

gbiggs requested review from MichaelOrlov and hidmic and removed request for a team February 16, 2022 06:31

hidmic reviewed Feb 25, 2022

View reviewed changes

agalbachicar mentioned this pull request May 10, 2022

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

Merged

gbiggs closed this May 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add play-until functionality #962

Add play-until functionality #962

gbiggs commented Feb 16, 2022 •

edited

Loading

MichaelOrlov commented Feb 17, 2022

hidmic left a comment

hidmic Feb 25, 2022

hidmic Feb 25, 2022

hidmic Feb 25, 2022

hidmic Feb 25, 2022

hidmic Feb 25, 2022

hidmic Feb 25, 2022

hidmic Feb 25, 2022

hidmic Feb 25, 2022

hidmic Feb 25, 2022

hidmic Feb 25, 2022

agalbachicar commented Mar 23, 2022

MichaelOrlov commented Apr 1, 2022

gbiggs commented May 10, 2022

		// Do not move on until sleep_until returns true
		// It will always sleep, so this is not a tight busy loop on pause

	player_->play(std::chrono::nanoseconds(std::chrono::milliseconds(1000)).count());
	player_->play(RCUTILS_MS_TO_NS(1000));

	player_->play(std::chrono::nanoseconds(std::chrono::milliseconds(1000)).count());
	player_->play(RCUTILS_S_TO_NS(1));

		auto replayed_test_primitives = sub_->get_received_messages<test_msgs::msg::BasicTypes>(
		kTopic1);

Add play-until functionality #962

Add play-until functionality #962

Conversation

gbiggs commented Feb 16, 2022 • edited Loading

MichaelOrlov commented Feb 17, 2022

hidmic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agalbachicar commented Mar 23, 2022

MichaelOrlov commented Apr 1, 2022

gbiggs commented May 10, 2022

gbiggs commented Feb 16, 2022 •

edited

Loading