Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 12 commits into from Nov 8, 2018

Conversation

Projects
None yet
5 participants
@botteroa-si
Copy link
Contributor

commented Oct 18, 2018

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.

@tfoote tfoote added the in review label Oct 18, 2018

@Martin-Idel-SI Martin-Idel-SI force-pushed the bsinno:feature/converters branch from 7d1c2b8 to 20a3a7d Oct 29, 2018

@Martin-Idel-SI

This comment has been minimized.

Copy link
Contributor

commented Oct 29, 2018

Rebased + CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
@botteroa-si

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2018

@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

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2018

New CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
>
<description>This is a converter plugin for cdr format.</description>
</class>
</library>

This comment has been minimized.

Copy link
@Karsten1987

Karsten1987 Oct 29, 2018

Contributor
Suggested change
</library>
</library>

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

This comment has been minimized.

Copy link
@Martin-Idel-SI

Martin-Idel-SI Oct 30, 2018

Contributor

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

#include <string>

#include "rcutils/strdup.h"
#include "rmw/rmw.h"

This comment has been minimized.

Copy link
@Karsten1987

Karsten1987 Oct 29, 2018

Contributor

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?

This comment has been minimized.

Copy link
@Martin-Idel-SI

Martin-Idel-SI Oct 30, 2018

Contributor

See comment below.

namespace rosbag2_converter_default_plugins
{

void CdrConverter::deserialize(

This comment has been minimized.

Copy link
@Karsten1987

Karsten1987 Oct 29, 2018

Contributor

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.

This comment has been minimized.

Copy link
@Martin-Idel-SI

Martin-Idel-SI Oct 30, 2018

Contributor

Makes perfect sense, will do.

@Karsten1987

This comment has been minimized.

Copy link
Contributor

commented Oct 29, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Oct 30, 2018

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

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

... 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

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

@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

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2018

@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.

@Martin-Idel-SI

This comment has been minimized.

@Martin-Idel-SI

This comment has been minimized.

Martin-Idel-SI and others added some commits Nov 7, 2018

Update rosbag2_converter_default_plugins/src/rosbag2_converter_defaul…
…t_plugins/cdr/cdr_converter.cpp

Co-Authored-By: Karsten1987 <karsten@osrfoundation.org>
@Karsten1987

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2018

new CI:
Build Status
Build Status
Build Status
Build Status

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

@Karsten1987 Karsten1987 removed the in review label Nov 8, 2018

@anhosi anhosi deleted the bsinno:feature/converters branch Nov 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.