Implement converter plugin for CDR format and add converter plugins package #48

botteroa-si · 2018-10-18T10:48:41Z

This PR is based on #47.

This PR implements a converter plugin to convert ROS 2 messages to CDR format and back, using fastrtps as rmw implementation.
The plugin is contained in the new package rosbag2_converter_default_plugins.

This new package also contains helper methods to extract rmw functions needed by the converters. It might be a good idea to expose these helpers, to benefit plugins developers. In order to do that, though, we need visibility control to be added to the package.

…d link against rmw_fastrtps_cpp

Martin-Idel-SI · 2018-10-29T08:40:27Z

Rebased + CI:

Linux
Linux-aarch64
macOS
Windows

botteroa-si · 2018-10-29T13:44:01Z

@Karsten1987, we don't really understand why all the CI jobs failed. It seems that the problem is not related with our code, though.
Maybe you have a better understanding of what went wrong (we also tried to restart the jobs, but the result remained unchanged).

dirk-thomas · 2018-10-29T13:49:09Z

Based on the job parameters and error message are you maybe trying to run tests for a package which hasn't been built?

botteroa-si · 2018-10-29T13:57:25Z

@dirk-thomas, thank you very much. You're right, the parameters we gave are, for this branch, wrong. We didn't think about that.

botteroa-si · 2018-10-29T16:23:25Z

New CI:

Linux
Linux-aarch64
macOS
Windows

Karsten1987 · 2018-10-29T16:38:14Z

rosbag2_converter_default_plugins/plugin_description.xml

+>
+  <description>This is a converter plugin for cdr format.</description>
+</class>
+</library>


Suggested change

</library>

</library>

Not sure if this helps, but github complains that there is no new line for this file.

I guess it makes sense to fix that - I expect the xml linter to find this problem later on once it'll be enabled.

Karsten1987 · 2018-10-29T16:42:59Z

rosbag2_converter_default_plugins/src/rosbag2_converter_default_plugins/cdr/cdr_converter.cpp

+#include <string>
+
+#include "rcutils/strdup.h"
+#include "rmw/rmw.h"


why is this including "rmw/rmw.h"? In case of a different middleware, this is there is no CDR support available?
Shouldn't this directly link against fastcdr or similar?

See comment below.

Karsten1987 · 2018-10-29T16:45:34Z

rosbag2_converter_default_plugins/src/rosbag2_converter_default_plugins/cdr/cdr_converter.cpp

+namespace rosbag2_converter_default_plugins
+{
+
+void CdrConverter::deserialize(


I'd prefer a similar argument order than in the rmw_deserialize function:
https://github.com/ros2/rmw/blob/master/rmw/include/rmw/rmw.h#L247-L253

the ros_message is the last argument, so being consistent with the place of the input/output arguments.

Makes perfect sense, will do.

Karsten1987 · 2018-10-29T16:50:24Z

Just in case I missed something here. But this PR should convert ROS 2 messages to CDR and CDR messages to ROS 2 messages, right?
I don't think it's correct to use the rmw_serialize / rmw_deserialize functions here. If other, non-CDR midlewares are present, the rmw_serialize function will not convert it to CDR, but to the middleware dependent format.

Martin-Idel-SI · 2018-10-30T08:00:38Z

My understanding is that it will work - but since there is no other rmw with a serialization format, yet, it's hard to verify (I don't like to implement a test_rmw as that seems like a lot of work). The plugin is explicitly linked against rmw_fastrtps_cpp (lddtree or dependency walker confirm this) and this should result in the rmw_fastrtps_cpp-Version being called regardless of which rmw is used to run rosbag2_transport.

But I agree that one has to be careful. That's why we suggested using Poco to grab the real rmw_serialize and rmw_deserialize functions directly from the corresponding library before. Here, we tried to follow your suggestion to link against fastrtps directly.

The third way would be to reimplement rmw_serialize within this function. In other word, we'd have to reimplement this file here: https://github.com/ros2/rmw_fastrtps/blob/master/rmw_fastrtps_cpp/src/rmw_serialize.cpp . I'm of split opinion here: This would duplicate at least 70 lines of code (maybe more if we have to implement some of the functions, too - I haven't checked) and if those lines are touched ever again, they will diverge. On the other hand, avoiding Poco AND having a very clear linking process that definitely works might be worth it.
@Karsten1987 Since you implemented the logic in https://github.com/ros2/rmw_fastrtps/blob/master/rmw_fastrtps_cpp, maybe you could copy it here if that's what you'd rather like to see?

dirk-thomas · 2018-10-30T16:49:57Z

... since there is no other rmw with a serialization format ...

@Martin-Idel-SI That is not true. There are a couple of non-DDS RMW implementations using different serialization protocols: see https://discourse.ros.org/t/non-dds-based-rmw-implementation/5890

The plugin is explicitly linked against rmw_fastrtps_cpp

The problem is that you can't rely on FastRTPS even being present. It is absolutely viable to build ROS 2 without FastRTPS - either only with Connext or OpenSplice - or only with OPC UA or DPS.

Martin-Idel-SI · 2018-10-31T07:59:03Z

@dirk-thomas Thank you for the link to the other rmw implementation - that means we can at least to some manual testing 👍

Regarding the other point, would that mean that we'd have to reimplement our own serialization routines or is there some part that would be okay to link against (statically if we must)?

dirk-thomas · 2018-11-01T18:01:14Z

would that mean that we'd have to reimplement our own serialization routines or is there some part that would be okay to link against (statically if we must)?

I am not sure how you map between rmw implementations and serialization formats. My point was aiming to clarify that you can't rely on a specific rmw impl. to always be present.

It is possible build ROS 2 with a single rmw impl. and it should then be possible to record and playback without requiring any extra dependencies. That is probably already the case? I was just wondering about it when you mentioned to always link against FastRTPS.

botteroa-si · 2018-11-02T09:01:12Z

@dirk-thomas

It is possible build ROS 2 with a single rmw impl. and it should then be possible to record and playback without requiring any extra dependencies. That is probably already the case?

Yes, currently ros2 bag record stores the data in the rmw format of the incoming messages, and the resulting bag file can be played back on any system which uses the same rmw implementation.

With the changes introduced by PRs #56 and #57, it will be possible to specify the format to save the messages in, and to play back bags whose messages are in a different rmw format than the one in use. For this to be possible, it is necessary to be able to convert messages between different formats. In order to achieve this, the idea is to have converter plugins which can convert a ROS2 message into the format used by a specific rmw implementation and back.
The object of this PR is a converter plugin for the CDR format.

…t_plugins/cdr/cdr_converter.cpp Co-Authored-By: Karsten1987 <karsten@osrfoundation.org>

Load fastrtps dynamically

Karsten1987 · 2018-11-08T21:52:39Z

new CI:

* Deserialize McapWriterOptions from storage_config_uri YAML file Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

* Deserialize McapWriterOptions from storage_config_uri YAML file Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com> Signed-off-by: James Smith <james@foxglove.dev>

tfoote added the in review label Oct 18, 2018

botteroa-si added 5 commits October 29, 2018 09:16

ros2GH-111 Add package for converter plugins

0f7e51c

ros2GH-111 Add CDR converter plugin

00786fa

ros2GH-111 Add test for more primitives types

811f958

ros2GH-116 Fix cdr converter after rebase on new converters interface

b82923e

ros2GH-116 Use rmw_serialize/rmw_deserialize directly in converter an…

20a3a7d

…d link against rmw_fastrtps_cpp

Martin-Idel-SI force-pushed the feature/converters branch from 7d1c2b8 to 20a3a7d Compare October 29, 2018 08:32

Martin-Idel-SI and others added 2 commits October 29, 2018 15:27

Fix converter package.xml

2b8c8d8

Fix clang warnings

fb77c4b

Karsten1987 reviewed Oct 29, 2018

View reviewed changes

ros2GH-30 Change interface to the same convention as rmw_(de)serialize

1794ba3

Karsten1987 and others added 4 commits November 5, 2018 14:37

comply to new rcutils error handling API

3859622

use poco to load fastrtps

7b0a38f

Update rosbag2_converter_default_plugins/src/rosbag2_converter_defaul…

58a4cce

…t_plugins/cdr/cdr_converter.cpp Co-Authored-By: Karsten1987 <karsten@osrfoundation.org>

Merge pull request #2 from bsinno/load_fastrtps_dynamically

1d32171

Load fastrtps dynamically

botteroa-si mentioned this pull request Nov 8, 2018

rcutils_get_error_string_safe is not available in ROS2 master #59

Closed

Karsten1987 approved these changes Nov 8, 2018

View reviewed changes

Karsten1987 merged commit 1737d24 into ros2:master Nov 8, 2018

Karsten1987 removed the in review label Nov 8, 2018

anhosi deleted the feature/converters branch November 30, 2018 08:14

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

Support storage-config-file Writer configuration (ros2#48)

decd487

* Deserialize McapWriterOptions from storage_config_uri YAML file Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement converter plugin for CDR format and add converter plugins package #48

Implement converter plugin for CDR format and add converter plugins package #48

botteroa-si commented Oct 18, 2018 •

edited

Loading

Martin-Idel-SI commented Oct 29, 2018

botteroa-si commented Oct 29, 2018 •

edited

Loading

dirk-thomas commented Oct 29, 2018

botteroa-si commented Oct 29, 2018 •

edited

Loading

botteroa-si commented Oct 29, 2018

Karsten1987 Oct 29, 2018

Martin-Idel-SI Oct 30, 2018

Karsten1987 Oct 29, 2018

Martin-Idel-SI Oct 30, 2018

Karsten1987 Oct 29, 2018

Martin-Idel-SI Oct 30, 2018

Karsten1987 commented Oct 29, 2018

Martin-Idel-SI commented Oct 30, 2018

dirk-thomas commented Oct 30, 2018 •

edited

Loading

Martin-Idel-SI commented Oct 31, 2018

dirk-thomas commented Nov 1, 2018

botteroa-si commented Nov 2, 2018 •

edited

Loading

Karsten1987 commented Nov 8, 2018 •

edited

Loading

Implement converter plugin for CDR format and add converter plugins package #48

Implement converter plugin for CDR format and add converter plugins package #48

Conversation

botteroa-si commented Oct 18, 2018 • edited Loading

Martin-Idel-SI commented Oct 29, 2018

botteroa-si commented Oct 29, 2018 • edited Loading

dirk-thomas commented Oct 29, 2018

botteroa-si commented Oct 29, 2018 • edited Loading

botteroa-si commented Oct 29, 2018

Karsten1987 Oct 29, 2018

Choose a reason for hiding this comment

Martin-Idel-SI Oct 30, 2018

Choose a reason for hiding this comment

Karsten1987 Oct 29, 2018

Choose a reason for hiding this comment

Martin-Idel-SI Oct 30, 2018

Choose a reason for hiding this comment

Karsten1987 Oct 29, 2018

Choose a reason for hiding this comment

Martin-Idel-SI Oct 30, 2018

Choose a reason for hiding this comment

Karsten1987 commented Oct 29, 2018

Martin-Idel-SI commented Oct 30, 2018

dirk-thomas commented Oct 30, 2018 • edited Loading

Martin-Idel-SI commented Oct 31, 2018

dirk-thomas commented Nov 1, 2018

botteroa-si commented Nov 2, 2018 • edited Loading

Karsten1987 commented Nov 8, 2018 • edited Loading

botteroa-si commented Oct 18, 2018 •

edited

Loading

botteroa-si commented Oct 29, 2018 •

edited

Loading

botteroa-si commented Oct 29, 2018 •

edited

Loading

dirk-thomas commented Oct 30, 2018 •

edited

Loading

botteroa-si commented Nov 2, 2018 •

edited

Loading

Karsten1987 commented Nov 8, 2018 •

edited

Loading