Use serialized message directly #24

Martin-Idel-SI · 2018-08-24T15:40:54Z

This PR makes rosbag subscribe and publish directly to the underlying middleware implementation.

It contains:

Use rosbag2_storage::SerializedBagMessage and publish/subscribe directly to rcutils_char_array_t
The SQLite3 storage plugin now has a rather generic interface to deal with serialized messages (this is also a preparation to deal with other topics than string topics)
Subscribing and Publishing to std_msgs/String topic of predefined name
Builds on all supported platforms

It does not contain:

Subscribing to an arbitrary topic (will be done in a follow-up PR)

Note that the rosbag2_write_integration_test still sometimes fails for an unknown reason (see #25).

Karsten1987 · 2018-08-27T16:51:55Z

rosbag2/src/rosbag2/rosbag2.cpp

  storage->create_topic();

  if (storage) {
    auto node = std::make_shared<rclcpp::Node>("rosbag_node");
    auto subscription = node->create_subscription<std_msgs::msg::String>(
      topic_name,
-      [&storage, after_write_action](std::shared_ptr<rmw_serialized_message_t> msg) {
+      [&storage, after_write_action](std::shared_ptr<rcutils_char_array_t> msg) {


I think we should continue to subscribe to rmw_serialized_message rather than rcutils_char_array_t. The two type are currently typedef'd, but that might change in the future.

Martin-Idel-SI · 2018-08-28T14:02:26Z

Done.

Karsten1987 · 2018-08-29T20:48:41Z

rosbag2/src/rosbag2/rosbag2.cpp

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


you can publish shared_ptr<rmw_serialized_message_t> now.

I know. We are currently rewriting the test and this will be improved as part of that effort if that's okay with you.

Karsten1987 · 2018-08-29T20:49:40Z

rosbag2/test/rosbag2/rosbag2_read_integration_test.cpp

@@ -45,6 +45,7 @@ class RosBag2IntegrationTestFixture : public Rosbag2TestFixture
    auto node = std::make_shared<rclcpp::Node>("subscriber_node");
    auto subscription = node->create_subscription<std_msgs::msg::String>("string_topic",
        [this, &messages](const std_msgs::msg::String::ConstSharedPtr message) {
+          std::cout << "\ndata = " << message->data << "\n";


nit: we normally use printf to be threadsafe.

In tests we prefer cout for readability. But we will remove this line as it is a debugging left-over.

Karsten1987 · 2018-08-29T20:52:46Z

rosbag2_storage/src/rosbag2_storage/ros_helper.cpp

+namespace rosbag2_storage
+{
+
+std::shared_ptr<rcutils_char_array_t>


shouldn't this be a SerializedBagMessage essentially?

Yes and no. In the end, we want a SerializedBagMessage, but since this function only really fills the rcutils_char_array_t part, I think it's cleaner to just provide this.

Karsten1987 · 2018-08-29T20:55:57Z

rosbag2_storage/src/rosbag2_storage/ros_helper.cpp

+std::shared_ptr<rcutils_char_array_t>
+make_serialized_message(const void * data, size_t size)
+{
+  auto rcutils_allocator = new rcutils_allocator_t;


I think this allocator is leaked. Nobody is cleaning this up.
The destructor should call delete on the allocator (rcutils_char_array_fini doesn't do it).

We saw this, too. This is fixed in #27, to simplify rebasing I'd just leave it here.

Karsten1987 · 2018-08-29T20:57:27Z

rosbag2_storage/test/test_plugin.hpp

@@ -32,7 +32,7 @@ class TestPlugin : public rosbag2_storage::storage_interfaces::ReadWriteInterfac

  rosbag2_storage::BagInfo info() override;

-  bool has_next() const override;
+  bool has_next() override;


why can't this function be const?

Karsten1987 · 2018-08-29T20:59:31Z

rosbag2_storage/include/rosbag2_storage/storage_interfaces/base_read_interface.hpp

@@ -30,7 +30,7 @@ class ROSBAG2_STORAGE_PUBLIC BaseReadInterface
 public:
  virtual ~BaseReadInterface() = default;

-  virtual bool has_next() const = 0;
+  virtual bool has_next() = 0;


why is this change necessary?

We need to actually change the internal state of the storage to correctly ask whether there is a next entry. In the followup PR, we'll do this the following way (here, it's done as simple as possible):

We have a read_statement_ member which needs to be initialized somewhere. This is either done in read_next() or in has_next() if it is not already initialized. This read statement then gets reused for every read to allow fast reading. If we don't do this, has_next() would fail if read_next() has never been called, which is awkward.

In order to allow the function to be const this would imply we'd have to make the read_statement (and by transitivity a lot of the other state of the storage) mutable, which is also not very nice. So we thought changing to non-const here is simpler.

If you think the const is worth it to signify that it shouldn't matter how often this function is called, all other function calls will result in the same results (i.e. the state as it is visible to users of this class is unchanged), we can change it back and make the necessary changes to mutable state.
If it's okay, I'd want to do this in a follow-up PR to avoid rebase hell for the other PRs because there are a lot of changes to the implementation in the other open PRs.

Regarding the constness: I agree that having const might improve the interface from a users point of view (for the sqlite implementation it probably would not be too hard to do so). However, this imposes a big limitation on the internal implementation of all future storage plugins (basically a lot of member need to be mutable).
One could also argue that the information whether more data is available might be expensive to obtain and thus should be considered an important part of the internal state.

Also if has_next() is const what about read_next? The arguments for/ against constness are pretty much the same.

To distinguish between read and write access we have the ReadOnlyInterface and ReadWriteInterface.

…ive tests

…eWrapper

…e TODOs

- Remove virtual on methods as this was added only for unit tests. We decided to use only integration tests for the sqlite plugins. - Changes semantics of SqliteStatement: represents always a prepared statement if not null. - Ensures that a SqliteStatementWrapper cannot be copied and does not publicly expose its sqlite_stmt as this would cause memory corruption.

- Uses a std::tuple for row data - Exposes an iterator interface for the query result

This is not storage plugin specific but will be needed by most (if not all) plugins.

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

sorry for being late on this. Hope the rebasing is not becoming too crazy :)

Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>

Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp> Signed-off-by: James Smith <james@foxglove.dev>

tfoote added the in review label Aug 24, 2018

Martin-Idel-SI mentioned this pull request Aug 28, 2018

Allow an arbitrary topic to be recorded #26

Merged

Karsten1987 reviewed Aug 28, 2018

View reviewed changes

Karsten1987 reviewed Aug 29, 2018

View reviewed changes

Martin-Idel-SI and others added 21 commits August 30, 2018 09:29

Adapt new interface

22e771d

Try to write and read rcutils_char_array_t BLOBs in sqlite

ab73851

Add simple test for arbitrary char ptr

b155060

Refactor SqliteWrapper and add tests

79c1a3d

Write and read actual timestamp from serialized message and add relat…

3afe8bd

…ive tests

Add SqliteStatementWrapper class and refactor SqliteStorage and Sqlit…

b7831bd

…eWrapper

Refactor test fixture

18f8c70

ros2GH-50 Assert message content in write_integration_test, and remov…

94bf386

…e TODOs

ros2GH-50 Remove sqlite_storage_plugin unit tests

226740e

ros2GH-50 Refactor SqliteStatements and SqliteStorage

e2237ac

ros2GH-50 Make has_next() method no more const

114bdd9

ros2GH-50 Fix build after rebase

0a23b29

ros2GH-50 Refactor after rebase

e4ca9e9

ros2GH-52 Extend statement wrapper with a generic bind

f618390

ros2GH-59 Introduce general read interface for sqlite statements

96281bf

- Uses a std::tuple for row data - Exposes an iterator interface for the query result

ros2GH-59 Cleanup: remove unused files

d3061aa

ros2GH-59 make sqlite interface fluent

3ae2da8

ros2GH-59 move creation of serialized message to rosbag2_storage

179a1d9

This is not storage plugin specific but will be needed by most (if not all) plugins.

Change rcutil_char_array_t to rmw_serialized_message_t in subscriber

725bf95

Remove debugging output in test

45f1508

Martin-Idel-SI force-pushed the feature/use_rmw_directly branch from 98976b7 to 45f1508 Compare August 30, 2018 08:33

Karsten1987 approved these changes Sep 3, 2018

View reviewed changes

Karsten1987 merged commit 3f77b33 into ros2:master Sep 3, 2018

Karsten1987 removed the in review label Sep 3, 2018

Martin-Idel-SI deleted the feature/use_rmw_directly branch September 4, 2018 08:37

james-rms pushed a commit to james-rms/rosbag2 that referenced this pull request Nov 17, 2022

fix: always exec pip install (ros2#24)

2f2b4fc

Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>

james-rms pushed a commit to james-rms/rosbag2 that referenced this pull request Nov 17, 2022

fix: always exec pip install (ros2#24)

fca498a

Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp> Signed-off-by: James Smith <james@foxglove.dev>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use serialized message directly #24

Use serialized message directly #24

Martin-Idel-SI commented Aug 24, 2018 •

edited

Loading

Karsten1987 Aug 27, 2018

Martin-Idel-SI commented Aug 28, 2018

Karsten1987 Aug 29, 2018

Martin-Idel-SI Aug 30, 2018

Karsten1987 Aug 29, 2018

anhosi Aug 30, 2018 •

edited

Loading

Karsten1987 Aug 29, 2018

Martin-Idel-SI Aug 30, 2018

Karsten1987 Aug 29, 2018

Martin-Idel-SI Aug 30, 2018 •

edited

Loading

Karsten1987 Aug 29, 2018

Karsten1987 Aug 29, 2018

Martin-Idel-SI Aug 30, 2018

anhosi Aug 30, 2018

Karsten1987 commented Sep 3, 2018

Use serialized message directly #24

Use serialized message directly #24

Conversation

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

Choose a reason for hiding this comment

Martin-Idel-SI commented Aug 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anhosi Aug 30, 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

Martin-Idel-SI Aug 30, 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

Karsten1987 commented Sep 3, 2018

Martin-Idel-SI commented Aug 24, 2018 •

edited

Loading

anhosi Aug 30, 2018 •

edited

Loading

Martin-Idel-SI Aug 30, 2018 •

edited

Loading