Use converters when playing back files #56

Martin-Idel-SI · 2018-10-31T13:24:40Z

This PR lays the foundations for using converters to play (and record) bagfiles with different middleware serialization formats.

Content of this PR:

The "deleted lines" are nearly all related to mocking rosbag2_storage::StorageFactory for testing the new functionality. Nothing really happens here.
This PR adds a converter, which loads converter plugins and then, given a SerializedBagMessage converts it to a SerializedBagMessage of (potentially) another format.
To do this, we need to allocate a ROS2-Message of unknown type at compile time.

Pointer to how the ROS message allocation/deallocation works:

We use the rosidl_introspection_typesupport_t.
Given the message, we allocate a chunk of memory (and zero it - this is necessary) according to the size of the message from the introspection type support.
For all members, we must now recursively do the following:

For primitive data types or arrays of primitives, we are done as they live inside the allocated message
For std::string and std::vector<bool> the zero-allocated memory does not work correctly, so we have to overwrite the memory with a true empty string or empty vector. All other vectors seem to work correctly.

This message now works - we can cast it to the correct type and assign values, and pass it somewhere, etc.
When it's time to deallocate, we need to recursively free all memory. This implies

For strings, deleting the content of the string
For vectors, deleting the content of the vector
For arrays, deleting the content of the array if it's not an array of primitives
Finally deallocating the content of the message itself.

As of now, the methods have a drawback in that any further dynamic memory allocation uses the free store (new). The allocator of the message is only used to allocate the bare message struct. I think that it might be possible to pass the allocator around except for nested message types, as I don't know the type of the vector to allocate.

Note: The way the converter plugins are used in the SequentialReader is not yet final and will be changed in a followup PR, which will additionally store serialization format information on a per topic basis.

rosbag2/src/rosbag2/types/ros2_message.cpp

…ests

- Added mocks for storage and converters (and factories)

- Use convert only if necessary (different input and output formats), converters are only loaded if really necessary. - Allocate_ros2_message is public to enable extensive tests for this function. - Helper function to get any typesupport by name - Helper function for empty ros2_message

- Treats most messages already. - Some combinations of nested messages with arrays are still missing - Cleanup of DynamicArrayNested messages is failing - Main difficulty is the cleanup of the allocated ros2_message which needs to be done manually - The test_ros2_message is intended to be run with valgrind and there should be no leaks or problems with free!

Swapping with empty container seems more stable than deleting the data pointer of the container.

rosbag2/test/rosbag2/types/test_ros2_message.cpp

rosbag2/include/rosbag2/types/ros2_message.hpp

rosbag2/src/rosbag2/types/ros2_message.cpp

- The TODO comments have been removed because they're no longer relevant: they have been discussed in the PR review

botteroa-si · 2018-11-13T17:02:22Z

CI:

Linux
Linux-aarch64
macOS
Windows

botteroa-si · 2018-11-14T11:12:54Z

New CI:

Linux
Linux-aarch64
macOS
Windows

botteroa-si · 2018-11-14T16:46:38Z

New CI:

Linux
Linux-aarch64
macOS
Windows

- Big strings are not treated with small string optimization and need to be checked, too.

anhosi · 2018-11-15T16:06:43Z

@Karsten1987 As discussed I added allocation tests for bigger strings (no small string optimization) and nested arrays. The cleanup actually already covered these cases.
These new tests require two new messages types in test_msgs and will not compile until those are available. -> so no CI for now.

rosbag2/src/rosbag2/types/ros2_message.cpp

anhosi · 2018-11-21T09:14:39Z

rosbag2/src/rosbag2/types/ros2_message.cpp

-      msg->allocator.deallocate(msg->message, msg->allocator.state);
-      delete msg;
-    };
+  auto deleter = std::bind(&deallocate_ros2_message, std::placeholders::_1, intro_ts_members);


Why not keeping the lambda? This change does not compile on Windows VS2017.

anhosi · 2018-11-21T10:08:47Z

New round of CI

Linux
Linux-aarch64
macOS
Windows

anhosi

@Karsten1987 Why did you want all these functions to be exported? They are not intended for direct use (except maybe deallocate_ros2_message). Should we put them in a nested namespace (e.g. rosbag2::ros2_message) instead?

tfoote added the in review label Oct 31, 2018

This was referenced Oct 31, 2018

Use converters when recording a bag file #57

Merged

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

Merged

Martin-Idel-SI commented Nov 2, 2018

View reviewed changes

rosbag2/src/rosbag2/types/ros2_message.cpp Show resolved Hide resolved

Martin-Idel-SI and others added 17 commits November 9, 2018 10:04

ros2GH-112 Open storage for reading handing in rmw_identifier

e30b5ce

ros2GH-113 Cleanup: better naming

2e9f6c9

ros2GH-113 Introduce interface for StorageFactory to allow mocks in t…

d017279

…ests

ros2GH-113 Add test for SequentialReader for using converters

d7c7572

- Added mocks for storage and converters (and factories)

ros2GH-113 Fix DynamicArrayNested deallocation

b2b97e5

Swapping with empty container seems more stable than deleting the data pointer of the container.

ros2GH-113 Add test for BoundedArrayNested deallocation

43833e6

ros2GH-113 Refactoring of deallocation code

565beb2

ros2GH-113 Fix string initialization in all types

da808af

ros2GH-113 Fix vector<bool> initialization

faac84a

ros2GH-113 Add test for deallocation of topic name + Refactoring

87f94dc

ros2GH-113 Minor refactoring of converter

9565c6c

ros2GH-113 Make sure to throw an error if converters do not exist

92c69e8

ros2GH-113 Delete superfluous imports

0b5ab69

ros2GH-113 Small fix for deleting vectors

8ad6af7

ros2GH-113 Fix build after rebase

eb1d1d2

botteroa-si force-pushed the feature/use_converters branch from ace2d84 to eb1d1d2 Compare November 9, 2018 11:34

Karsten1987 reviewed Nov 13, 2018

View reviewed changes

rosbag2/src/rosbag2/types/ros2_message.cpp Outdated Show resolved Hide resolved

botteroa-si added 2 commits November 13, 2018 16:27

ros2GH-30 Minor refactoring

13f2e70

- The TODO comments have been removed because they're no longer relevant: they have been discussed in the PR review

ros2GH-30 Give an allocator as parameter to allocate_ros2_message()

74602cd

ros2GH-111 Add missing test dependencies for CDR converter test

647d6f0

Karsten1987 mentioned this pull request Nov 14, 2018

rosidl_typesupport_introspection_cpp potentially unsafe ros2/rosidl#320

Closed

ros2GH-128 Extend message allocation test to also cover big strings

9071440

- Big strings are not treated with small string optimization and need to be checked, too.

Karsten1987 mentioned this pull request Nov 15, 2018

add new test messages for arrays of arrays ros2/rcl_interfaces#52

Merged

anhosi added 2 commits November 16, 2018 16:48

ros2GH-128 Add tests for nested arrays

9e1cb19

ros2GH-128 always initialize vectors with a placement new

e5d1460

anhosi force-pushed the feature/use_converters branch from 7e29614 to e5d1460 Compare November 16, 2018 15:49

Karsten1987 reviewed Nov 19, 2018

View reviewed changes

rosbag2/src/rosbag2/types/ros2_message.cpp Outdated Show resolved Hide resolved

Karsten1987 mentioned this pull request Nov 20, 2018

enable access functions for primitive types ros2/rosidl#328

Merged

Karsten1987 added 4 commits November 20, 2018 17:40

pass by ref

0253dfc

use new getter functions

8317066

consistent function naming

740e820

uncrustify

bdfe850

anhosi reviewed Nov 21, 2018

View reviewed changes

ros2GH-30 Fix windows build

aec25aa

Karsten1987 mentioned this pull request Nov 23, 2018

Use uint8 #61

Merged

Karsten1987 approved these changes Nov 23, 2018

View reviewed changes

use visibility macros on all functions

6b58674

Karsten1987 merged commit cb28689 into ros2:master Nov 23, 2018

Karsten1987 removed the in review label Nov 23, 2018

anhosi deleted the feature/use_converters branch November 23, 2018 08:24

anhosi reviewed Nov 23, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use converters when playing back files #56

Use converters when playing back files #56

Martin-Idel-SI commented Oct 31, 2018 •

edited

Loading

botteroa-si commented Nov 13, 2018

botteroa-si commented Nov 14, 2018

botteroa-si commented Nov 14, 2018

anhosi commented Nov 15, 2018

anhosi Nov 21, 2018

anhosi commented Nov 21, 2018 •

edited

Loading

anhosi left a comment

Use converters when playing back files #56

Use converters when playing back files #56

Conversation

Martin-Idel-SI commented Oct 31, 2018 • edited Loading

botteroa-si commented Nov 13, 2018

botteroa-si commented Nov 14, 2018

botteroa-si commented Nov 14, 2018

anhosi commented Nov 15, 2018

anhosi Nov 21, 2018

Choose a reason for hiding this comment

anhosi commented Nov 21, 2018 • edited Loading

anhosi left a comment

Choose a reason for hiding this comment

Martin-Idel-SI commented Oct 31, 2018 •

edited

Loading

anhosi commented Nov 21, 2018 •

edited

Loading