Adapt new interface #21

Martin-Idel-SI · 2018-08-20T15:42:02Z

This PR adapts the demo to the new storage API.

This should now build and test cleanly on all supported platforms. The adaptation of the new interface is as lightweight as possible - in particular, we don't save the serialized bag message for now but only its string part. The true saving will be added with a later pull request.

Note:
I had to change the storage factory: Using template methods, it is not really a good idea if the implementation is not contained in the header file. It is not visible from the outside what kind of template specializations exist and indeed, this results in missing symbols on Windows.
For now, I just exposed everything (i.e. we don't have a true pimpl here anymore), but we should reconsider the usage here.
In fact, exposing the implementation, librosbag2_storage could be made a header only library for now. I introduced a dummy function which we will (in some way or other) need later on to avoid rewriting the CMakeLists.txt.

Also: It might be beneficial to introduce a visibility header for librosbag2. Otherwise the library cannot be linked against on Windows.

Karsten1987 · 2018-08-20T16:09:24Z

rosbag2_storage/include/rosbag2_storage/impl/storage_factory_impl.hpp

@@ -66,7 +66,8 @@ get_interface_instance(

  std::shared_ptr<InterfaceT> instance = nullptr;
  try {
-    instance = class_loader->createSharedInstance(storage_id);
+    auto unmanaged_instance = class_loader->createUnmanagedInstance(storage_id);
+    instance = std::shared_ptr<InterfaceT>(unmanaged_instance);


what's the need for this change? Is this not resulting in the same?

Karsten1987 · 2018-08-20T16:14:27Z

rosbag2_storage/src/storage_factory.cpp

-template<>
-std::shared_ptr<ReadOnlyInterface> StorageFactory::open<ReadOnlyInterface>(
-  const std::string & uri, const std::string & storage_id)
+std::string get_storage_identifier(const std::string & uri)


I believe this function shall be part of the info plugin interface. In the end it's defined for each plugin.

Karsten1987 · 2018-08-21T18:53:22Z

rosbag2_storage/src/impl/storage_factory_impl.hpp

@@ -66,7 +66,8 @@ get_interface_instance(

  std::shared_ptr<InterfaceT> instance = nullptr;
  try {
-    instance = class_loader->createSharedInstance(storage_id);
+    auto unmanaged_instance = class_loader->createUnmanagedInstance(storage_id);


what's the difference here? Isn't this the same change in the end?

The python interface should accept all options that can be passed to rosbag2_transport

* rosbag2_transport package with python interface * use cpp for python extension * use rosbag2_transport cpp API * use rosbag2_transport API in cli * linters * GH-25 Rename target librosbag2 to rosbag2 CMake already prepends libraries with `lib`, so the old name resulted in `liblibrosbag2` * GH-21 Initial call of rosbag2.record() from rosbag2_transport * GH-21 Add missing copyright header * GH-21 Cleanup clang tidy issues * GH-21 Remove rclcpp dependency from rosbag2 * GH-21 Wire rosbag play into CLI * GH-21 Add missing test_depend in rosbag2_transport package.xml * GH-21 Unify name of python import * GH-21 Enable -a in CLI, show help on wrong args * GH-85 Introduce topic and type struct for readability * GH-85 Do not export sqlite3 as dependency from default plugins - not referenced in header, therefore unnecessary * GH-85 Move rosbag2 except typesupport to rosbag2_transport * GH-85 Add rosbag2 wrapper * GH-85 Change signature of create_topic to take TopicWithType * GH-85 Use rosbag2 in rosbag2_transport - Don't link against rosbag2_storage anymore * GH-84 Cleanup package.xmls and CMakeLists everywhere * GH-21 Add missing init() and shutdown() in record * GH-85 Fix Windows build * GH-85 Add visibility control to rosbag2 * GH-85 Cleanup and documentation * GH-87 Add test package rosbag2_tests * GH-87 [WIP] Add first working prototype of an end-to-end test * GH-87 Use test_msgs instead of std_msgs/String in end-to-end test * GH-87 Use SIGTERM instead of SIGKILL and refactor test * GH-87 Make end-to-end test work on Windows * GH-87 Fix uncrustify * GH-87 Refactor end-to-end test fixture * GH-21 Extend transport python module interface The python interface should accept all options that can be passed to rosbag2_transport * GH-87 Fix test fixture for Windows * GH-87 Refactor test fixture * GH-87 Separate record from play end-to-end test * GH-87 Make record end-to-end test work * GH-87 Publish before recording to create topic * GH-87 Fix record all on Windows * GH-87 Check for topics instead of all * GH-87 Wait until rosbag record opened database * GH-87 Delete directory recursively * GH-87 Delete directories recursively on Linux * GH-87 Reset ROS_DOMAIN_ID to protect against concurrent tests * GH-89 Make rosbag2 interfaces virtual and add explicit open() method This allows downstream packages (e.g. rosbag2_transport) to mock these interfaces in tests. * GH-87 Improve test and refactoring * GH-87 Minor refactoring to increase test readability * GH-87 Fix environmental variable behaviour on Mac * GH-87 Fix Windows build * GH-89 Use mock reader and writer in rosbag2_transport tests * GH-87 Add play end_to_end test * GH-87 Improvements of test * GH-87 Fix Windows build * GH-89 Cleanup: small documentation fixes. * GH-89 [WIP] Test if Writer and Reader work with class visibility * GH-87 Stabilize rosbag2_play test * GH-87 Minor refactoring of tests * GH-87 Rename end to end tests * add license agreement * GH-89 Simplification of writing to in-memory storage * GH-89 Stabilize transport tests * GH-87 Refactoring of tests - Extract temporary file handling - Extract subscription management * GH-87 Add pytest cache to gitignore * GH-87 Refactoring of play test - Extract Publisher manager * GH-87 Extract record test fixture for readability * GH-89 Refactor transport tests - Use subscription and publisher manager just as e2e tests - Use options in recording * GH-89 Use temporary directory fixture in sqlite tests * GH-89 Conform to naming standard for tests * GH-89 Prevent burst publishing of all messages - Improves test stability * GH-89 Improve play stability - Sometimes the first message is lost (discovery) * GH-25 Fix package.xmls * Consistently use project name in CMakeLists * Minor cleanup - make rosbag2_transport description more expressive - hide unnecessary methods in typesupport_helpers - fix incorrect logging in tests - minor cleanup * Change name of nodes in rosbag2_transport * Cleanup folder structure in rosbag2_storage and rosbag2_tests - use src/<package_name>/ and test/<package_name>/ folders everywhere - harmonises with all other packages - results in better header guards * Export sqlite3 dependency as package dependency * Create node in Rosbag2Transport always * Only hold one node in rosbag2_transport * Move all duplicate files to common package * Adapt namespacing in test commons package - use "using namespace" declaratives for tests - use package name as namespace * Replace "Waiting for messages..." message * GH-25 rename rosbag2_test_commons -> rosbag2_test_common * GH-25 Overwrite already existing test.bag when recording This is a temporary solution and will be handled properly once a file path can be passed via the cli. * GH-25 Cleanups - Log every subscription - move all dependencies onside BUILD_TESTING for rosbag2_test_common * fix cmake typo for test_common * Remove superfluous loop in rosbag2 transport * Delete superfluous test_msgs dependency * Add rclcpp to test dependencies - Apparently ament_export_dependencies does not work in rosbag2_test_common * Fix rosbag2 node test - Clock topic is no longer present on all nodes - Remove assumptions on foreign ros topics * Fix dependencies by exporting them explicitly

tfoote added the in review label Aug 20, 2018

Karsten1987 force-pushed the storage_interface branch from 15daad0 to 8f60c8c Compare August 20, 2018 21:48

Martin-Idel-SI added 14 commits August 21, 2018 09:45

Delete superfluous classes + polishing

372733c

Adapt SerializedBagMessage format

57422c8

Let sqlite3 storage use new interface

e2c6d22

Fix tests in rosbag2

fa4bf17

Write and read only data

daf149d

Replace creation of shared instance by unmanaged instance

ae2aa8b

Add pragma for windows

9b7d701

Add visibility control for Windows

0e55ca2

Expose template definitions

c212f09

Move const to better location

723a455

Replace strcpy

c8e3a42

Delete superfluous methods

9316f05

Use visibility control in rosbag2

be8599d

Minor cleanup

db5e4fc

Martin-Idel-SI force-pushed the feature/polish_interface branch from 5ab1aa2 to db5e4fc Compare August 21, 2018 15:09

Karsten1987 reviewed Aug 21, 2018

View reviewed changes

Karsten1987 merged commit 3dfc8f5 into ros2:storage_interface Aug 21, 2018

Karsten1987 removed the in review label Aug 21, 2018

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

anhosi pushed a commit to bosch-io/rosbag2 that referenced this pull request Sep 17, 2018

ros2GH-21 Initial call of rosbag2.record() from rosbag2_transport

feb2a14

anhosi pushed a commit to bosch-io/rosbag2 that referenced this pull request Sep 17, 2018

ros2GH-21 Add missing copyright header

e053489

anhosi pushed a commit to bosch-io/rosbag2 that referenced this pull request Sep 17, 2018

ros2GH-21 Cleanup clang tidy issues

abbb2ca

anhosi pushed a commit to bosch-io/rosbag2 that referenced this pull request Sep 17, 2018

ros2GH-21 Remove rclcpp dependency from rosbag2

4da3aab

anhosi pushed a commit to bosch-io/rosbag2 that referenced this pull request Sep 17, 2018

ros2GH-21 Wire rosbag play into CLI

1703b5b

anhosi pushed a commit to bosch-io/rosbag2 that referenced this pull request Sep 17, 2018

ros2GH-21 Add missing test_depend in rosbag2_transport package.xml

2ec7be0

anhosi pushed a commit to bosch-io/rosbag2 that referenced this pull request Sep 17, 2018

ros2GH-21 Unify name of python import

89c7e27

anhosi pushed a commit to bosch-io/rosbag2 that referenced this pull request Sep 17, 2018

ros2GH-21 Enable -a in CLI, show help on wrong args

e645229

anhosi pushed a commit to bosch-io/rosbag2 that referenced this pull request Sep 17, 2018

ros2GH-21 Add missing init() and shutdown() in record

40c9114

Martin-Idel-SI pushed a commit to bosch-io/rosbag2 that referenced this pull request Sep 19, 2018

ros2GH-21 Initial call of rosbag2.record() from rosbag2_transport

7dafa98

Martin-Idel-SI pushed a commit to bosch-io/rosbag2 that referenced this pull request Sep 19, 2018

ros2GH-21 Add missing copyright header

ab650b4

Martin-Idel-SI pushed a commit to bosch-io/rosbag2 that referenced this pull request Sep 19, 2018

ros2GH-21 Cleanup clang tidy issues

f85b223

Martin-Idel-SI pushed a commit to bosch-io/rosbag2 that referenced this pull request Sep 19, 2018

ros2GH-21 Remove rclcpp dependency from rosbag2

45ab77d

Martin-Idel-SI pushed a commit to bosch-io/rosbag2 that referenced this pull request Sep 19, 2018

ros2GH-21 Wire rosbag play into CLI

27afa21

Martin-Idel-SI pushed a commit to bosch-io/rosbag2 that referenced this pull request Sep 19, 2018

ros2GH-21 Add missing test_depend in rosbag2_transport package.xml

0e2e911

Martin-Idel-SI pushed a commit to bosch-io/rosbag2 that referenced this pull request Sep 19, 2018

ros2GH-21 Unify name of python import

fb1f072

Martin-Idel-SI pushed a commit to bosch-io/rosbag2 that referenced this pull request Sep 19, 2018

ros2GH-21 Enable -a in CLI, show help on wrong args

923f975

Martin-Idel-SI pushed a commit to bosch-io/rosbag2 that referenced this pull request Sep 19, 2018

ros2GH-21 Add missing init() and shutdown() in record

552983e

Martin-Idel-SI pushed a commit to bosch-io/rosbag2 that referenced this pull request Sep 19, 2018

ros2GH-21 Extend transport python module interface

d1a811c

The python interface should accept all options that can be passed to rosbag2_transport

Martin-Idel-SI pushed a commit to bosch-io/rosbag2 that referenced this pull request Sep 20, 2018

ros2GH-21 Initial call of rosbag2.record() from rosbag2_transport

55c23e2

Martin-Idel-SI pushed a commit to bosch-io/rosbag2 that referenced this pull request Sep 20, 2018

ros2GH-21 Add missing copyright header

200a537

Martin-Idel-SI pushed a commit to bosch-io/rosbag2 that referenced this pull request Sep 20, 2018

ros2GH-21 Cleanup clang tidy issues

ec4352c

Martin-Idel-SI pushed a commit to bosch-io/rosbag2 that referenced this pull request Sep 20, 2018

ros2GH-21 Remove rclcpp dependency from rosbag2

ca21e0a

Martin-Idel-SI pushed a commit to bosch-io/rosbag2 that referenced this pull request Sep 20, 2018

ros2GH-21 Wire rosbag play into CLI

b419300

Martin-Idel-SI pushed a commit to bosch-io/rosbag2 that referenced this pull request Sep 20, 2018

ros2GH-21 Add missing test_depend in rosbag2_transport package.xml

9937f97

Martin-Idel-SI pushed a commit to bosch-io/rosbag2 that referenced this pull request Sep 20, 2018

ros2GH-21 Unify name of python import

1d94085

Martin-Idel-SI pushed a commit to bosch-io/rosbag2 that referenced this pull request Sep 20, 2018

ros2GH-21 Enable -a in CLI, show help on wrong args

146d042

Martin-Idel-SI pushed a commit to bosch-io/rosbag2 that referenced this pull request Sep 20, 2018

ros2GH-21 Add missing init() and shutdown() in record

f831694

Martin-Idel-SI pushed a commit to bosch-io/rosbag2 that referenced this pull request Sep 20, 2018

ros2GH-21 Extend transport python module interface

52bf059

The python interface should accept all options that can be passed to rosbag2_transport

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adapt new interface #21

Adapt new interface #21

Martin-Idel-SI commented Aug 20, 2018

Karsten1987 Aug 20, 2018

Karsten1987 Aug 20, 2018

Karsten1987 Aug 21, 2018

Adapt new interface #21

Adapt new interface #21

Conversation

Martin-Idel-SI commented Aug 20, 2018

Karsten1987 Aug 20, 2018

Choose a reason for hiding this comment

Karsten1987 Aug 20, 2018

Choose a reason for hiding this comment

Karsten1987 Aug 21, 2018

Choose a reason for hiding this comment