Record and play multiple topics #27

anhosi · 2018-08-29T09:23:09Z

This PR is based on PR #26.

This extends both the recording and replaying to support multiple topics at the same time.

The required meta data (topic and type) is also stored in the storage

Current limitation:

Same as in Allow an arbitrary topic to be recorded #26 the topics to record must be present at start. Discovery for new topics will be added later.
Known issue
The after_write_action argument of Rosbag2::record is used only for the tests. We should be able to remove it soon, but will do so in a separate PR.

Karsten1987 · 2018-09-04T18:45:34Z

rosbag2/CMakeLists.txt

+    src/rosbag2/typesupport_helpers.cpp
+    WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})
+  if(TARGET rosbag2_typesupport_helpers_test)
+    ament_target_dependencies(rosbag2_typesupport_helpers_test rcl Poco ament_index_cpp


Karsten1987 · 2018-09-04T18:45:51Z

rosbag2/CMakeLists.txt

+    src/rosbag2/rosbag2_node.cpp
+    WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})
+  if(TARGET rosbag2_rosbag_node_test)
+    ament_target_dependencies(rosbag2_rosbag_node_test rclcpp Poco ament_index_cpp std_msgs)


Karsten1987 · 2018-09-04T18:50:46Z

rosbag2/src/rosbag2/generic_subscription.cpp

+{
+  return borrow_serialized_message(0);
+}
+


double whitespace?

Karsten1987 · 2018-09-04T18:54:34Z

rosbag2/src/rosbag2/rosbag2.cpp

      // without the sleep_for() many messages are lost.
-      std::this_thread::sleep_for(std::chrono::milliseconds(500));
-      publisher->publish(message->serialized_data.get());
+      std::this_thread::sleep_for(std::chrono::milliseconds(50));


shouldn't they be published according to their timestamp?

Yes and they will (in the future). We are currently working on that but it is not in scope for this PR.

Karsten1987 · 2018-09-04T18:55:35Z

rosbag2/src/rosbag2/rosbag2.cpp

 }

-void Rosbag2::play(const std::string & file_name, const std::string & topic_name)
+void Rosbag2::play(const std::string & file_name)
 {
  rosbag2_storage::StorageFactory factory;
  auto storage = factory.open_read_only(file_name, "sqlite3");

  if (storage) {


What happens in the else case?

Nothing 😁. The storage factory already complains if the storage cannot be loaded.

If we want to have more elaborate error handling I would postpone that until the rosbag2_transport has been introduced as we would have a CLI integration then. Then it would be easier to think about feedback to the user.

Karsten1987 · 2018-09-04T18:56:35Z

rosbag2/src/rosbag2/rosbag2_node.hpp

+#include <memory>
+#include <string>
+
+#include "rcutils/types.h"


Karsten1987 · 2018-09-04T19:04:49Z

rosbag2/test/rosbag2/rosbag2_read_integration_test.cpp

+    size_t expected_messages_number, std::string topic)
+  {
+    auto node = rclcpp::Node::make_shared("node_" + topic);
+    std::vector<typename T::_data_type> messages;


This maybe deserves a bit of documentation. The _data_type field is only available for primitives messages types whose single field is called data. Otherwise the variable is called like the field name:
https://github.com/ros2/rosidl/blob/master/rosidl_generator_cpp/resource/msg__struct.hpp.em#L199-L204

So this test can not easily be transformed into a complex msg type.

For this particular test it is sufficient. But I agree that we probably should add a test for a complex type. That way we can probably replace the std_msgs test dependency with test_msgs which looks less surprising.

We changed the read_integration_test to use also complex messages from test_msgs.

Karsten1987 · 2018-09-04T19:05:53Z

rosbag2/test/rosbag2/rosbag2_read_integration_test.cpp

+        });
+    subscriptions_.push_back(subscription);
+
+    while (messages_received < expected_messages_number) {


can this resolve in an endless loop in case there is no network connection etc? This should maybe timeout

In principle yes, but the test framework takes care of that so we do not need to "clutter" the test with timeout handling.

Karsten1987 · 2018-09-04T19:18:34Z

rosbag2/test/rosbag2/rosbag2_test_fixture.hpp

-  std::shared_ptr<rosbag2_storage::SerializedBagMessage> serialize_message(std::string message)
+  template<typename MessageT>
+  std::shared_ptr<rosbag2_storage::SerializedBagMessage> serialize_message(
+    std::string topic, typename MessageT::_data_type message)


some comment as above. I believe _data_type is not defined for every message type.

You are right. This is still used by the write_integration_test which is massively rewritten in #31. So I would prefer to address this in #31 to avoid an unecessary rebase.

Karsten1987 · 2018-09-04T19:21:35Z

rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_storage.cpp

+  auto statement = database_->prepare_statement("SELECT name, type FROM topics ORDER BY id;");
+  auto query_results = statement->execute_query<std::string, std::string>();
+
+  for (auto result : query_results) {


should there be a notification or similar in case no results for found for this query?

This could only happen if the bag is empty, i.e. contains no messages. Here the result will simply be empty which should not hurt.
I am not sure how we want to handle empty bags. Currently it would be opened, the play would initialize some things, loop over an empty list and shutdown => no harm there. So I propose to discuss this on a new issue.

…play multiple topics

- Uses a JOIN instead of mapping the topic_id to the name in code

This allows repeated dereferencing on same row without quering the database again.

…sses

Karsten1987 · 2018-09-05T16:04:05Z

rosbag2/include/rosbag2/rosbag2.hpp

 class Rosbag2
 {
 public:
  ROSBAG2_PUBLIC
  void record(
    const std::string & file_name,
-    const std::string & topic_name,
-    std::function<void(void)> after_write_action = nullptr);
+    std::vector<std::string> topic_names,


const std::vector<> &

Karsten1987 · 2018-09-05T16:10:58Z

rosbag2/src/rosbag2/rosbag2.cpp

 void Rosbag2::record(
  const std::string & file_name,
-  const std::string & topic_name,
-  std::function<void(void)> after_write_action)
+  std::vector<std::string> topic_names,


- this allows new topics to be discovered also after startup. - as a consquence, ros2 bag record won't throw any exception anymore if the specified topics are not available at startup - each time the recorder subscribes to a new topic, this will be logged to console

- make option available in RecordOptions - set to 100ms for now

- this allows new topics to be discovered also after startup. - as a consquence, ros2 bag record won't throw any exception anymore if the specified topics are not available at startup - each time the recorder subscribes to a new topic, this will be logged to console

- make option available in RecordOptions - set to 100ms for now

- use set for subscribed topics instead of vector - perform the asynchronous call only when discovery is needed

- this allows new topics to be discovered also after startup. - as a consquence, ros2 bag record won't throw any exception anymore if the specified topics are not available at startup - each time the recorder subscribes to a new topic, this will be logged to console

- make option available in RecordOptions - set to 100ms for now

- use set for subscribed topics instead of vector - perform the asynchronous call only when discovery is needed

- naming of methods - emphasize similarity between first subscription and discovery loop

* GH-27 Implement discovery for new topics after startup - this allows new topics to be discovered also after startup. - as a consquence, ros2 bag record won't throw any exception anymore if the specified topics are not available at startup - each time the recorder subscribes to a new topic, this will be logged to console * GH-27 Provide polling frequency as option - make option available in RecordOptions - set to 100ms for now * GH-27 Stop topic polling if subscription setup is complete * GH-27 Minor refactoring for readability * GH-27 Fix build after rebase * GH-145 add no-discovery option to ros2 bag record * GH-27 Refactor subscribing mechanism - use set for subscribed topics instead of vector - perform the asynchronous call only when discovery is needed * GH-27 Expose polling-interval as cli option * GH-27 Minor refactoring * GH-27 Refactor recorder - naming of methods - emphasize similarity between first subscription and discovery loop * small touch ups * remove launch from function name

Addressing feedback from ros/rosdistro#32534 (comment) Make mcap available as a shared library in the mcap_vendor package. It does not depend on lz4/zstd shared libraries but pulls them in directly using FetchContent. mcap itself is also fetched directly from git instead of using conan to pull in the dependency. Closes ros2#26, closes ros2#27 Signed-off-by: Jacob Bandes-Storch <jacob@foxglove.dev>

Addressing feedback from ros/rosdistro#32534 (comment) Make mcap available as a shared library in the mcap_vendor package. It does not depend on lz4/zstd shared libraries but pulls them in directly using FetchContent. mcap itself is also fetched directly from git instead of using conan to pull in the dependency. Closes ros2#26, closes ros2#27 Signed-off-by: Jacob Bandes-Storch <jacob@foxglove.dev> Signed-off-by: James Smith <james@foxglove.dev>

…ooling/rosbag_storage_mcap#28) Addressing feedback from ros/rosdistro#32534 (comment) Make mcap available as a shared library in the mcap_vendor package. It does not depend on lz4/zstd shared libraries but pulls them in directly using FetchContent. mcap itself is also fetched directly from git instead of using conan to pull in the dependency. Closes ros2#26, closes ros2#27 Signed-off-by: Jacob Bandes-Storch <jacob@foxglove.dev> Signed-off-by: James Smith <james@foxglove.dev>

…ooling/rosbag2_storage_mcap#28) Addressing feedback from ros/rosdistro#32534 (comment) Make mcap available as a shared library in the mcap_vendor package. It does not depend on lz4/zstd shared libraries but pulls them in directly using FetchContent. mcap itself is also fetched directly from git instead of using conan to pull in the dependency. Closes ros2#26, closes ros2#27 Signed-off-by: Jacob Bandes-Storch <jacob@foxglove.dev> Signed-off-by: James Smith <james@foxglove.dev>

tfoote added the in review label Aug 29, 2018

This was referenced Aug 30, 2018

Use serialized message directly #24

Merged

Record all topics #30

Merged

Martin-Idel-SI force-pushed the feature/record_and_play_multiple_topics branch from 3936546 to b9e6b22 Compare September 4, 2018 08:27

Karsten1987 reviewed Sep 4, 2018

View reviewed changes

botteroa-si and others added 24 commits September 5, 2018 09:05

ros2GH-61 Read topic directly from message when playing and allow to …

4516479

…play multiple topics

ros2GH-61 Add test for SqliteStorage and update old ones

85cba04

ros2GH-61 Obtain the topic name directly from the database

72a8c0b

- Uses a JOIN instead of mapping the topic_id to the name in code

ros2GH-62 Extend function to poll for any number of specified topics

2c0dd44

ros2GH-62 Allow subscription to several topics

ab3d5ec

ros2GH-61 Cache read row in result iterator

a671b58

This allows repeated dereferencing on same row without quering the database again.

ros2GH-62 Change demo-record to allow specifying multiple topics

2bc6abd

ros2GH-62 Add test to write non-string topic + refactoring

3444109

ros2GH-62 Add test for subscription to multiple topics

c35bcc3

ros2GH-62 Cleanup

cdf0784

ros2GH-62 Simplify test setup

8700122

ros2GH-61 Cleanup

746e2e8

ros2GH-61 consolidate storage integration test

146cdbf

ros2GH-61 enhance read integration test to check multiple topics

c246802

ros2GH-62 Consolidate write integration tests

58c4a91

ros2GH-62 Improve rosbag integration test

718bbdc

ros2GH-62: Polish rosbag2_rosbag_node_test

fc92936

ros2GH-62 Fix cpplint

ce00d44

ros2GH-62 Fix memory leak in rosbag helper

45625cc

ros2GH-62 Cleanup of subscriptions

366a588

ros2GH-62 do not use flaky timers in rosbag2_write_integration_test

dbc6a40

ros2GH-62 Use rmw_serialize_message_t consistently in test helper cla…

ace5619

…sses

ros2GH-73 Use test_msgs in read_integration_test

924c536

ros2GH-26 Cleanup: fix alphabetic orderung

3eaee5b

anhosi force-pushed the feature/record_and_play_multiple_topics branch from b9e6b22 to 3eaee5b Compare September 5, 2018 14:19

Karsten1987 approved these changes Sep 5, 2018

View reviewed changes

Karsten1987 merged commit c23d188 into ros2:master Sep 5, 2018

Karsten1987 removed the in review label Sep 5, 2018

anhosi deleted the feature/record_and_play_multiple_topics branch September 5, 2018 17:00

Martin-Idel-SI added a commit to bosch-io/rosbag2 that referenced this pull request Nov 26, 2018

ros2GH-27 Provide polling frequency as option

1751302

- make option available in RecordOptions - set to 100ms for now

Martin-Idel-SI added a commit to bosch-io/rosbag2 that referenced this pull request Nov 26, 2018

ros2GH-27 Stop topic polling if subscription setup is complete

525d0ac

Martin-Idel-SI added a commit to bosch-io/rosbag2 that referenced this pull request Nov 26, 2018

ros2GH-27 Minor refactoring for readability

224a8a2

anhosi pushed a commit to bosch-io/rosbag2 that referenced this pull request Nov 30, 2018

ros2GH-27 Provide polling frequency as option

eb06a3a

- make option available in RecordOptions - set to 100ms for now

anhosi pushed a commit to bosch-io/rosbag2 that referenced this pull request Nov 30, 2018

ros2GH-27 Stop topic polling if subscription setup is complete

81050be

anhosi pushed a commit to bosch-io/rosbag2 that referenced this pull request Nov 30, 2018

ros2GH-27 Minor refactoring for readability

7592b16

anhosi added a commit to bosch-io/rosbag2 that referenced this pull request Nov 30, 2018

ros2GH-27 Fix build after rebase

d3cd7c6

botteroa-si pushed a commit to bosch-io/rosbag2 that referenced this pull request Dec 6, 2018

ros2GH-27 Refactor subscribing mechanism

c36a5f1

- use set for subscribed topics instead of vector - perform the asynchronous call only when discovery is needed

botteroa-si pushed a commit to bosch-io/rosbag2 that referenced this pull request Dec 6, 2018

ros2GH-27 Expose polling-interval as cli option

8a7bfd7

botteroa-si added a commit to bosch-io/rosbag2 that referenced this pull request Dec 6, 2018

ros2GH-27 Minor refactoring

6c0d458

anhosi pushed a commit to bosch-io/rosbag2 that referenced this pull request Dec 6, 2018

ros2GH-27 Provide polling frequency as option

71b40d2

- make option available in RecordOptions - set to 100ms for now

anhosi pushed a commit to bosch-io/rosbag2 that referenced this pull request Dec 6, 2018

ros2GH-27 Stop topic polling if subscription setup is complete

05e02ea

anhosi pushed a commit to bosch-io/rosbag2 that referenced this pull request Dec 6, 2018

ros2GH-27 Minor refactoring for readability

4b9ef84

anhosi added a commit to bosch-io/rosbag2 that referenced this pull request Dec 6, 2018

ros2GH-27 Fix build after rebase

99121ee

anhosi added a commit to bosch-io/rosbag2 that referenced this pull request Dec 6, 2018

ros2GH-27 Refactor subscribing mechanism

235604d

- use set for subscribed topics instead of vector - perform the asynchronous call only when discovery is needed

anhosi added a commit to bosch-io/rosbag2 that referenced this pull request Dec 6, 2018

ros2GH-27 Expose polling-interval as cli option

6cc6171

anhosi pushed a commit to bosch-io/rosbag2 that referenced this pull request Dec 6, 2018

ros2GH-27 Minor refactoring

4def41e

anhosi added a commit to bosch-io/rosbag2 that referenced this pull request Dec 6, 2018

ros2GH-27 Refactor recorder

ee11257

- naming of methods - emphasize similarity between first subscription and discovery loop

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Record and play multiple topics #27

Record and play multiple topics #27

anhosi commented Aug 29, 2018

Karsten1987 Sep 4, 2018

anhosi Sep 5, 2018

Karsten1987 Sep 4, 2018

Karsten1987 Sep 4, 2018

anhosi Sep 5, 2018

Karsten1987 Sep 4, 2018

anhosi Sep 4, 2018 •

edited

Loading

Karsten1987 Sep 4, 2018

anhosi Sep 5, 2018

Karsten1987 Sep 4, 2018

anhosi Sep 5, 2018

Karsten1987 Sep 4, 2018

anhosi Sep 4, 2018

anhosi Sep 5, 2018

Karsten1987 Sep 4, 2018

anhosi Sep 4, 2018

Karsten1987 Sep 4, 2018

anhosi Sep 5, 2018

Karsten1987 Sep 4, 2018

anhosi Sep 4, 2018

Karsten1987 Sep 5, 2018

Karsten1987 Sep 5, 2018

Record and play multiple topics #27

Record and play multiple topics #27

Conversation

anhosi commented Aug 29, 2018

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

anhosi Sep 4, 2018 • edited Loading

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

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

anhosi Sep 4, 2018 •

edited

Loading