Allow an arbitrary topic to be recorded #26

Martin-Idel-SI · 2018-08-28T12:10:46Z

This is based on #24

This PR adds the ability to record an arbitrary topic of an arbitrary ROS 2 type. To do this, the following changes are made:

Extend the SQLite default storage plugin to contain a table of topics with type info
Subscribe to and publish raw messages of arbitrary known topics, where the type info is retrieved from the corresponding type-info package (using the ideas presented in loading ts from ament index #20)
To subscribe to messages without type info at compile time, we added a "GenericPublisher". Once this is possible with rclcpp this part can be deleted
The demo_record takes the name of the topic to be recorded as a parameter. If the topic does not exist, rosbag record exits early.
For now, the topic must be available at start up of rosbag_record. We have a small timeout to allow discovery of nodes and topics.

We make the following assumptions regarding types:

When calling node->get_topic_names_and_types() all ROS 2 topics will be of the form <package-name>/<message-type> (e.g. std_msgs/String), while neither the package name nor the message type contain any further slashes.
When a topic has multiple types, it is not a ROS 2 topic and won't be processed by rosbag

Known issues:

The flaky test rosbag_write_integration_test is flaky #25 is not resolved yet

Karsten1987

I will reiterate once you had a chance to rebase this PR, but I requested changes for the rosbag2Node class.

Karsten1987 · 2018-09-03T23:50:28Z

rosbag2/src/rosbag2/rosbag2.cpp

+  const std::string & topic_name, const std::shared_ptr<rclcpp::Node> & node)
+{
+  // TODO(Martin-Idel-SI): This is a short sleep to allow the node some time to discover the topic
+  // This should be replaced by an auto-discovery system in the future


I believe getting the topic type can eventually be excluded and done via the command line interface.

Here are my problems with this approach:

How do you plan to support subscribing to new topics which are not available at startup time? If I say rosbag2 record -all I would eventually expect also new topics that are created after startup to be recorded. In that case, this will have to be done in our C++ loops, so I don't think it's a good idea to do this in the python code.

I thought that the python code would really only be a python wrapper. But if you get topics and types in python, then you'd have further functionality there. I think it would be cleaner to have all functionality in C++

I see your point. I believe we could have an overload for record (for a later time):

record(const std::string & topic_name, ...)

record(const std::string & topic_name, const std::string & topic_type)

The reason I am bringing up the python interface is, because it's easier to load the type information from python directly.

The thing with recording new topics is tricky though. This has to be somehow specified how often we look for new topics then. I believe in the current ROS 1 implementation, only the topics available at startup time are recorded. Otherwise I would expect asking for all available topics after every spin.

Karsten1987 · 2018-09-03T23:52:31Z

rosbag2/src/rosbag2/rosbag2.cpp

+
+    if (type.empty()) {
+      RCUTILS_LOG_INFO_NAMED(ROS_PACKAGE_NAME, "Topic could not be found. Abort.");
+      return;


wouldn't it make sense to return an boolean here or throw? Somehow the calling user would not necessarily notice that the recording fails.

+1 for the throw

rosbag2/src/rosbag2/rosbag2_node.cpp

Karsten1987 · 2018-09-03T23:58:21Z

rosbag2/src/rosbag2/rosbag2.cpp

+      return;
+    }
+
+    auto subscription = node->create_generic_subscription(


This generic subscription should be created directly through its constructor and a regular node can be given as a parameter. I believe this makes things easier later on when the generic sub/pub are in ros2 and thus keeps changes to a minimum.

If it's just for keeping changes to a minimum, I'm not really sure this is true: Now, we can just delete the node class (including its test), adapt the node type in rosbag2 and maybe slightly adapt the method calls in rosbag2. In the other case, we have to rewrite all publishing and subscribing code in rosbag2 so I think it's more a matter of taste.

Karsten1987 · 2018-09-04T00:00:07Z

rosbag2/test/rosbag2/test_helpers.hpp

+namespace test_helpers
+{
+
+std::shared_ptr<rmw_serialized_message_t> serialize_string_message(std::string message)


why not const std::string & message?

Karsten1987 · 2018-09-04T00:02:04Z

rosbag2_storage/src/impl/storage_factory_impl.hpp

-    RCUTILS_LOG_ERROR_NAMED(ROS_PACKAGE_NAME,
-      "Requested storage id %s does not exist", storage_id.c_str());
+    // This prevents unnecessary error messages when loading a ReadWrite storage as ReadOnly storage
+    if (last_attempt) {


what's the exact purpose of this? When is decided that this is a last attempt? Can you elaborate on this?

Sure: Before this change, whenever you used rosbag_play, you'd get an error message "Requested storage id sqlite3 does not exist" because there is no ReadOnlyStorage sqlite3 from pluginlib's perspective. Obviously, since there is a ReadWriteStorage sqlite3, we still open the storage and everything works correctly, but the error message is highly irritating.

I deleted the "last_attempt" workaround and just print a debug message at that point, printing an overall failure message when no loading succeeded.

Karsten1987 · 2018-09-04T00:03:16Z

rosbag2_storage/test/test_plugin.hpp

@@ -32,13 +33,15 @@ class TestPlugin : public rosbag2_storage::storage_interfaces::ReadWriteInterfac

  rosbag2_storage::BagInfo info() override;

-  bool has_next() const override;
+  void create_topic(const std::string & name, const std::string & type_id) override;


topic_id here is the topic type? Or which id is meant here? If so, I would recommend having it renamed to type_type in order to keep the vocabulary in sync with upstream

Changed to name and type

Karsten1987 · 2018-09-04T00:05:18Z

rosbag2_storage_default_plugins/src/rosbag2_storage_default_plugins/sqlite/sqlite_wrapper.cpp

@@ -40,39 +44,14 @@ SqliteWrapper::~SqliteWrapper()
  sqlite3_close(db_ptr);
 }

-void SqliteWrapper::execute_query(const std::string & query)
+SqliteStatement SqliteWrapper::prepare_statement(std::string query)


why the change from const std::string & ?

No change, this is just git messing up. But changed to const ref.

Karsten1987 · 2018-09-04T00:06:27Z

...ault_plugins/test/rosbag2_storage_default_plugins/sqlite/sqlite_wrapper_integration_test.cpp

+
+#include "rcutils/types.h"
+
+#include "../../../src/rosbag2_storage_default_plugins/sqlite/sqlite_wrapper.hpp"


I know it's a test, but given that long include path, is there any use to having this wrapper file public?

It will be made public in #31, because at that point we need it in other tests as well. But until then, I think it's cleaner to do it like this.

- Two table db layout (messages and topics) - Messages table references topics table but without foreign key for improved write performance - Create_topic must be called for every topic prior to storing a message of this topic. - Sqlite_storage caches all known topics - At least for now the type information is stored as a simple string.

…pass topic name by terminal

…n Rosbag2::play

…efined before playing back messages

…Y flag

- pass a few strings as const reference - throw error when no topics could be found

Karsten1987 · 2018-09-04T16:46:53Z

rosbag2/CMakeLists.txt

@@ -20,13 +20,22 @@ find_package(rclcpp REQUIRED)
 find_package(rcutils REQUIRED)
 find_package(std_msgs REQUIRED)
 find_package(rosbag2_storage REQUIRED)


nitpick: Alphabetical order

Karsten1987 · 2018-09-04T18:08:36Z

rosbag2/test/rosbag2/test_helpers.hpp

+  auto initial_capacity = 8u + static_cast<size_t>(test_message->data.size());
+  auto msg = new rmw_serialized_message_t;
+  *msg = rcutils_get_zero_initialized_char_array();
+  auto ret = rcutils_char_array_init(msg, initial_capacity, &rcutils_allocator);


the allocator pointer will dangle here when the rcutils_allocator goes out of scope.

* GH-61 Read topic directly from message when playing and allow to play multiple topics * GH-61 Add test for SqliteStorage and update old ones * GH-62 Extend function to poll for any number of specified topics * GH-62 Allow subscription to several topics * GH-61 Obtain the topic name directly from the database - Uses a JOIN instead of mapping the topic_id to the name in code * GH-61 Cache read row in result iterator This allows repeated dereferencing on same row without quering the database again. * GH-62 Change demo-record to allow specifying multiple topics * GH-62 Add test to write non-string topic + refactoring * GH-62 Add test for subscription to multiple topics * GH-62 Cleanup * GH-62 Simplify test setup * GH-61 Cleanup * GH-61 consolidate storage integration test * GH-62 Consolidate write integration tests * GH-61 enhance read integration test to check multiple topics * GH-62 Improve rosbag integration test * GH-62: Polish rosbag2_rosbag_node_test * GH-62 Fix cpplint * GH-62 Fix memory leak in rosbag helper * GH-62 Cleanup of subscriptions * GH-62 do not use flaky timers in rosbag2_write_integration_test * GH-62 Use rmw_serialize_message_t consistently in test helper classes * GH-73 Use test_msgs in read_integration_test * GH-26 Cleanup: fix alphabetic orderung

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>

…topic across splits in the bag recorder (#26) Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

tfoote added the in review label Aug 28, 2018

anhosi force-pushed the feature/extend_db_schema branch from 8a6789e to 4198bfc Compare August 28, 2018 14:36

anhosi mentioned this pull request Aug 29, 2018

Record and play multiple topics #27

Merged

Karsten1987 requested changes Sep 4, 2018

View reviewed changes

anhosi and others added 26 commits September 4, 2018 09:27

ros2GH-54 Make first rcl subscription prototype work

247825c

ros2GH-54 find type name from topic

bc22b63

ros2GH-54 Publish messages from database knowing only topic name and …

eb3b13a

…pass topic name by terminal

ros2GH-54 Refactoring of typesupport helpers

eb39a72

ros2GH-54 Use c++ typesupport

56ca8f4

ros2GH-54 Use cpp typesupport and rclcpp::Node for publisher

a2760b2

ros2GH-54 Add raw subscription and use in rosbag_record

3dad432

ros2GH-54 Add Rosbag2Node and Rosbag2Publisher classes and use them i…

899d004

…n Rosbag2::play

ros2GH-54 Rename Rosbag2Publisher to RawPublisher

2fc5d4b

ros2GH-54 Minor refactoring of Rosbag2Node

f235764

ros2GH-54 Fix read integration tests and linters

5b4b54f

ros2GH-54 Extract and test waiting for topic into its own method

9fcfcdb

ros2GH-55 Refactor Rosbag2Node::create_raw_publisher()

df7a8b7

ros2GH-54 Add subscription method to rosbag node

3e23135

ros2GH-54 Keep subscription alive

1c4ac02

ros2GH-54: Extract subscription to correct class

d2697dc

ros2GH-55 Change interface of raw_publisher to match subscriber

2bd61e5

ros2GH-54 Add test for rosbag node

8012636

ros2GH-54 Unfriend rclcpp class

c6f75bf

ros2GH-54 Make test more robust

013ae5d

ros2GH-54 Fix build

b67f15b

ros2GH-54 Minor cleanup and documentation

a58d8f5

ros2GH-55 Minor refactoring + TODO comment

0cfbbe3

ros2GH-54 Change dynamic library folder on Windows

05143de

ros2GH-54 Fix build

faa5fa8

botteroa-si and others added 11 commits September 4, 2018 09:27

ros2GH-55 Check that topic and associated type in bag file are well d…

4326b34

…efined before playing back messages

ros2GH-54 Prevent unnecessary error message loading storage

7d81087

ros2GH-54 Fix memory leak

770c7ea

ros2GH-54 stabilize node test

2cf37f5

ros2GH-55 Check if database exists when opening storage with READ_ONL…

3d0b394

…Y flag

ros2GH-54 Minor cleanup of subscriber

989fa84

ros2GH-54 Wait a small amount of time to let node discover other nodes

8ffee15

Add logging to false case

7f68b72

ros2GH-54 Catch exceptions and exit cleanly

a85a6aa

Use rmw_serialized_message_t and rcutils_char_array_t consistently

0fa614b

GH-4 Refactoring for correctness

e838700

- pass a few strings as const reference - throw error when no topics could be found

Martin-Idel-SI force-pushed the feature/extend_db_schema branch from 0c919db to e838700 Compare September 4, 2018 08:26

Improve error messages when loading plugins

44219d4

Karsten1987 reviewed Sep 4, 2018

View reviewed changes

Karsten1987 force-pushed the feature/extend_db_schema branch from 0e9e7a3 to 83be1ad Compare September 4, 2018 18:01

Karsten1987 reviewed Sep 4, 2018

View reviewed changes

alphabetical order

1b78982

Karsten1987 force-pushed the feature/extend_db_schema branch from 83be1ad to 1b78982 Compare September 4, 2018 18:27

type_id -> type

7e73f7d

Karsten1987 approved these changes Sep 4, 2018

View reviewed changes

Karsten1987 merged commit bdd7fd1 into ros2:master Sep 4, 2018

Karsten1987 removed the in review label Sep 4, 2018

anhosi deleted the feature/extend_db_schema branch September 5, 2018 06:57

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

ros2GH-26 Cleanup: fix alphabetic orderung

3eaee5b

emersonknapp added a commit that referenced this pull request Jun 10, 2024

Add support for caching a single latched message per transient_local …

49447a0

…topic across splits in the bag recorder (#26) Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow an arbitrary topic to be recorded #26

Allow an arbitrary topic to be recorded #26

Martin-Idel-SI commented Aug 28, 2018 •

edited

Loading

Karsten1987 left a comment

Karsten1987 Sep 3, 2018

Martin-Idel-SI Sep 4, 2018

Karsten1987 Sep 4, 2018

Karsten1987 Sep 3, 2018

anhosi Sep 4, 2018

Martin-Idel-SI Sep 4, 2018

Karsten1987 Sep 3, 2018

Martin-Idel-SI Sep 4, 2018

Karsten1987 Sep 4, 2018

Martin-Idel-SI Sep 4, 2018

Karsten1987 Sep 4, 2018

Martin-Idel-SI Sep 4, 2018 •

edited

Loading

Karsten1987 Sep 4, 2018

Martin-Idel-SI Sep 4, 2018 •

edited

Loading

Karsten1987 Sep 4, 2018

Martin-Idel-SI Sep 4, 2018

Karsten1987 Sep 4, 2018

Martin-Idel-SI Sep 4, 2018

Karsten1987 Sep 4, 2018

Karsten1987 Sep 4, 2018 •

edited

Loading


		#include "rcutils/types.h"

		#include "../../../src/rosbag2_storage_default_plugins/sqlite/sqlite_wrapper.hpp"

Allow an arbitrary topic to be recorded #26

Allow an arbitrary topic to be recorded #26

Conversation

Martin-Idel-SI commented Aug 28, 2018 • edited Loading

Karsten1987 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

Choose a reason for hiding this comment

Martin-Idel-SI Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Martin-Idel-SI 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

Karsten1987 Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

Martin-Idel-SI commented Aug 28, 2018 •

edited

Loading

Martin-Idel-SI Sep 4, 2018 •

edited

Loading

Martin-Idel-SI Sep 4, 2018 •

edited

Loading

Karsten1987 Sep 4, 2018 •

edited

Loading