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

Split converters #70

Merged
merged 11 commits into from
Dec 6, 2018
Merged

Conversation

Martin-Idel-SI
Copy link
Contributor

@Martin-Idel-SI Martin-Idel-SI commented Dec 3, 2018

This PR splits the converter interfaces into two interfaces: A serializer and a deserializer. This allows to have plugins which only support converting to OR from a custom serialization format (such as the plugin for old rosbags).

The changes also mimic the storage_interface architecture:

  • As a plugin author, I can choose to write a plugin for serializers, deserializers or both at the same time (implementing only one interface)
  • As a user in rosbag2, I only have to interact with the serializers and deserializers (if I can't cirectly use the converter class), so not much changes

The main change is the serialization_format_converter_factory and its impl file as well as the usage in converter.cpp.

In addition, the error message if a converter is missing was changed (see end to end test for an example).

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

- Allow plugins which can only read or write
- Most important example: plugin for old rosbags
- When loading a serializer, try to load both serializer and converter
- Similar for deserializers
- adapt namespaces to folder structure
- folder structure similar to rosbag2_storage
- We want to use template functions that require the pluginlib import
- The pluginlib import should not be exported (this creates issues with
  downstream packages)
- Similar to the storage factory, use a pimpl
@Martin-Idel-SI Martin-Idel-SI mentioned this pull request Dec 5, 2018
2 tasks
Copy link
Collaborator

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. We could maybe iterate over the name for the serialization format converter interfaces, now that it has its own namespace.

docs/converter_plugin_development.md Outdated Show resolved Hide resolved
docs/converter_plugin_development.md Outdated Show resolved Hide resolved
namespace converter_interfaces
{

class SerializationFormatConverterInterface
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given that we have interfaces in the namespace, we could maybe rename SerializationFormatConverterInterface to only SerializationFormatConverter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me. Done.

Karsten1987 and others added 2 commits December 6, 2018 09:41
Co-Authored-By: Martin-Idel-SI <external.Martin.Idel@bosch-si.com>
@Karsten1987 Karsten1987 merged commit ffbca3e into ros2:master Dec 6, 2018
@Karsten1987 Karsten1987 deleted the feature/split_converters branch December 6, 2018 16:20
@Karsten1987 Karsten1987 restored the feature/split_converters branch December 6, 2018 16:21
@anhosi anhosi deleted the feature/split_converters branch December 10, 2018 09:15
sriramster pushed a commit to sriramster/rosbag2 that referenced this pull request Feb 28, 2019
* ros2GH-69 Read storage content in a separate thread

For now the publishing starts only after the reading is completly
done. This should change aufter ros2GH-68 is done and a thread-safe
queue can be used instead of std::queue.

* ros2GH-71 Add integration test for timing behavior

* ros2GH-68 Introduce vendor package for shared queue

- Download and install headers from moodycamel readerwriterqueue
- Download and install headers from moodycamel concurrentqueue
- Use readerwriterqueue in code to load and publish concurrently

* ros2GH-71 Retain time difference of messages when playing a bag file

- The main (play) thread sleeps until the time for publishing the
  message is reached.
- Using std::chrono time_point and duration for type-safe time
  arithmetic instead of rcutils time types.

* ros2GH-71 Improve stability of read test

- Subscribers need to maintain a longer history if the messages are
  not consumed fast enough.

* ros2GH-71 Fix Classloader instance lifetime

The Classloader instance needs to outlive all objects created by it.

* ros2GH-71 Extract playing code into a class of its own

Reason: record and play have almost no common code but do the exact
opposite with the storage and rclcpp.

* ros2GH-70 Do not link explicitly against std_msgs

- only required in tests
- this decreases the amount of packages needed for a clean build without tests

* ros2GH-70 Fix error message of storage

* ros2GH-70 Fix pluginlib/storage issue for recording

* ros2GH-71 Cleanup: variable naming

* ros2GH-70 Load storage continuously instead of as fast as possible

- Only load if queue contains less than 1000 messages
- Wait a millisecond before loading again once the queue is long enough

* ros2GH-70 Add options struct to allow specification of queue size

* ros2GH-72 Wait for messages to fill up

* ros2GH-74 Rename integration tests to play/record tests

* ros2GH-74 Use test_msgs in integration tests

- gets rid of string_msgs dependency

* ros2GH-70 Rename is_not_ready to is_pending, use bulk reading to queue

* ros2GH-70 Harmonize storage_loading_future variable

* ros2GH-88 Read messages in order of their timestamps

- Currently, we write sequentially in order of arrival time so
  reading in id order is fine
- This may change at a later time and should not change the reading
  behaviour, i.e. we need to read in order of timestamps

* Fix compiler error on Mac

* ros2GH-8 Fix: use correct ros message type in test

* ros2GH-8 Cleanup: minor code style fixes

* ros2GH-8 Refactor future usage in player

Make the future a class member of player to avoid having to hand it
into several functions which is difficult with a move-only type.

* ros2GH-8 Cleanup: remove verbose logging for every stored message

* ros2GH-8 Refactor rosbag2 interface

Add an explicit overload for record without a topic_names argument to
record all topics.

* fix: call vector.reserve instead of default initalization

* fix record demo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants