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

Introduce rosbag2_transport layer and CLI #38

Merged
merged 90 commits into from Sep 28, 2018

Conversation

Projects
None yet
8 participants
@Martin-Idel-SI
Copy link
Contributor

commented Sep 20, 2018

This PR does a number of things:

  • Introduce rosbag2_transport to make rosbag2 independent of any subscription/publishing code and therefore rclcpp or rcl
  • Make rosbag2 a thin wrapper around rosbag2_storage (for now). Later on, rosbag2 can be used by other tooling
  • Introduce first CLI versions: ros2 bag play <filename> and ros2 bag record <topic names> as well as ros2 bag record --all. The two verbs and the given options are fully functional.

This PR improves testing:

  • Added a new package rosbag2_tests containing e2e-testing: We start ros2 bag record/play commands in separate processes
  • Make tests in rosbag2_transport not rely on particular storage, which decreases dependencies
  • Conform to naming conventions: All tests start with test, all other files don't.

This PR should also improve test stability to basically as much as we can

  • We always start any publishers or subscribers before the real test starts to make discovery easier
  • We start publishers and subscribers single threaded, we only ever use another thread for spinning
  • Any test failures which we have now seen (very rare as in <1% of all runs) must be related to discovery being too slow. There is nothing much we can do about it instead of wait longer.

To discuss:
There are a few duplicate files in testing (they are named the same to make it easy to spot). How should we go about this problem?

  • Should we create additional testing libraries in each package which provides headers/objects which are potentially interesting in other tests?
  • Or should we create another package rosbag2_test_commons which all packages might depend on as a pure testing dependency?

Karsten1987 and others added some commits Aug 25, 2018

GH-25 Rename target librosbag2 to rosbag2
CMake already prepends libraries with `lib`, so the old name resulted
in `liblibrosbag2`
GH-85 Do not export sqlite3 as dependency from default plugins
- not referenced in header, therefore unnecessary
GH-85 Use rosbag2 in rosbag2_transport
- Don't link against rosbag2_storage anymore
@Karsten1987

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2018

There is one problem I'd like to have fixed on this PR:

✔  09:58:54  ~/workspace/osrf/rosbag2_ws
 ➭ ros2 bag record -a
DISCLAIMER
ros2 bag is currently under development and not ready to use yet
[ERROR] [rosbag2_storage]: Could not open 'test.bag' with 'sqlite3'. Error: SQL error when preparing statement 'CREATE TABLE topics(id INTEGER PRIMARY KEY,name TEXT NOT NULL,type TEXT NOT NULL);' with return code: 1
[ERROR] [rosbag2_storage]: Could not load/open plugin with storage id 'sqlite3'.
libc++abi.dylib: terminating with uncaught exception of type std::runtime_error: No storage could be initialized. Abort
Abort trap: 6

This error occurs when trying to record the bag file twice in a row.

@greimela-si

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2018

This error occurs when trying to record the bag file twice in a row.

Yes, that is the error message when trying to overwrite a bagfile.

For this PR we could add the behavior that bagfiles are overwritten by default, since the bagname is fixed.
In the future, we should probably offer a better error message if the user tries to overwrite a bagfile.

@Karsten1987

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2018

I am in favor of either overwriting it or gracefully failing if the bag file already exists. But somehow it shouldn't be terminating and the user should be informed about what's happening.

another comment is to rename rosbag2_test_commons to rosbag2_test_common. See here:
ros2/rmw_connext#240

Looks like it's a German thing ;-)

@Karsten1987
Copy link
Contributor

left a comment

a few more comments, sorry :)

endif()

find_package(ament_cmake REQUIRED)
find_package(ament_index_cpp REQUIRED)

This comment has been minimized.

Copy link
@Karsten1987

Karsten1987 Sep 27, 2018

Contributor

can that line be inside the if(BUILD_TESTING) ?

Show resolved Hide resolved rosbag2_transport/src/rosbag2_transport/rosbag2_transport_python.cpp
Show resolved Hide resolved rosbag2_transport/src/rosbag2_transport/rosbag2_transport_python.cpp
Show resolved Hide resolved rosbag2_transport/src/rosbag2_transport/rosbag2_transport.cpp
subscriptions_.clear();
}

std::shared_ptr<Rosbag2Node> Rosbag2Transport::setup_node()

This comment has been minimized.

Copy link
@Karsten1987

Karsten1987 Sep 27, 2018

Contributor

why is the node not setup directly in the constructor? If so, then we don't need that check whether it's already set up or not.

This comment has been minimized.

Copy link
@Karsten1987

Karsten1987 Sep 27, 2018

Contributor

the same for init and shutdown btw

This comment has been minimized.

Copy link
@anhosi

anhosi Sep 27, 2018

Contributor

Same reason as above for init. The setup_node may only be called after rclcpp::init has happended.

This comment has been minimized.

Copy link
@Karsten1987

Karsten1987 Sep 28, 2018

Contributor

so it looks like you can't really call rclpy.init() and then spin your nodes in c++.
I guess with that we can leave the code as it is.

Show resolved Hide resolved rosbag2_transport/src/rosbag2_transport/rosbag2_transport.cpp
Show resolved Hide resolved rosbag2_transport/src/rosbag2_transport/player.cpp
Show resolved Hide resolved rosbag2_transport/src/rosbag2_transport/player.hpp

anhosi and others added some commits Sep 27, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2018

new CI:

Build Status
Build Status
Build Status
Build Status

@Martin-Idel-SI

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2018

new CI (my fault, test_msgs were still being find_package'd):

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

Martin-Idel-SI added some commits Sep 27, 2018

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

@Martin-Idel-SI Martin-Idel-SI force-pushed the bsinno:rosbag2_transport branch from 5bad5b8 to 490697f Sep 27, 2018

@Martin-Idel-SI

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2018

Let's try a new CI then:

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

@Karsten1987 Karsten1987 merged commit 1370c7f into ros2:master Sep 28, 2018

@Karsten1987 Karsten1987 removed the in review label Sep 28, 2018

@greimela-si greimela-si deleted the bsinno:rosbag2_transport branch Sep 28, 2018

Martin-Idel-SI added a commit to bsinno/rosbag2 that referenced this pull request Nov 30, 2018

Martin-Idel-SI added a commit to bsinno/rosbag2 that referenced this pull request Dec 5, 2018

Martin-Idel-SI added a commit to bsinno/rosbag2 that referenced this pull request Dec 6, 2018

Martin-Idel-SI added a commit to bsinno/rosbag2 that referenced this pull request Dec 6, 2018

Martin-Idel-SI pushed a commit to bsinno/rosbag2 that referenced this pull request Dec 11, 2018

Martin-Idel-SI added a commit to bsinno/rosbag2 that referenced this pull request Dec 13, 2018

sriramster pushed a commit to sriramster/rosbag2 that referenced this pull request Feb 28, 2019

(demo, sqlite3) First working rosbag2 implementation (ros2#6)
* First implementation of writer

* Extract storage interface and sqlite3 implementation

* Add test for sqlite storage

* Split main() and rosbag2::record()

* Add close() method to Storage

* Add getMessage() method and refactor test

* Refactor SqliteStorage constructor and open()

* Add linters

* Fix uncrustify

* Fix cpplint

* Specify test working directory

* Better error handling

* Use gmock matchers for assertions

* Add test fixture for SqliteStorage tests

* Extract message retrieval in tests into separate method

* Add integration test for rosbag2::record()

* Add ignore files for empty packages

* Introduce create() method and refactor open()

* Use shared pointer of Storage instead of SqliteStorage

* Remove getDatabaseHandle() method

* Fix uncrustify

* Improve storage interface and add storage factory

* Remove need of sleep() from integration test by usage of std::future

* Move deletion of test database from fixture constructor to destructor

* Use sqlite3 directly in intergration test instead of own sqlite wrapper

* Move rosbag2::record() into Rosbag2 class

* Use the test name as database file name

* Add build instructions to README

* ros2GH-37 Rename camelCase methods to snake_case

* Use common test fixture

* Add RAII wrapper for sqlite API

* Mock away sqlite from sqlite_storage test

* Use more reasonable assert

* Add test

* Add virtual destructor to WritableStorage

* Use file_name instead of database_name in StorageFactory

* Implement saving of test files in a tmp directory for linux/Mac

* Try to implement saving of test files in a tmp directory for Windows

* Write and use proper gmock SqliteWrappe mock

* Refactor integration test and get rid of promise/future where possible

* Throw exception in resource aquisition constructors

* Make SqliteWrapper destructor virtual

* Refactor test fixture and update SqliteWrapper mock

* Fix warning when moving a temporary object

* ros2GH-38 Refactor integration test

* ros2GH-38 Get rid of superfluous string constructor in emplace_back()

* ros2GH-38 Assert also execute_query() argument in sqlite_storage_test

* ros2GH-38 add StorageFactory test

* ros2GH-38 Refactor rosbag2 Test Fixture

* ros2GH-40 Add first implementation of a rosbag reader and publisher

* ros2GH-40 Add StorageFactory test when reading non-existing file

* ros2GH-40 Fix uncrustify

* ros2GH-40 Minor cleanup of CMakeLists

* ros2GH-40 Wrap sqlite statements

* ros2GH-40 Remove superfluous import

* ros2GH-40 Use better include

* ros2GH-40 Add play integration test

* ros2GH-40 Fix Warning when moving a temporary object in reading

* ros2GH-40 Initialize database pointer to nullpointer

* ros2GH-40 Fix reader integration test

* ros2GH-40 Polish storage wrapper

* Revert "ros2GH-40: Wrap sqlite statements"

* ros2GH-38 Fix Test Fixture after rebase

* ros2GH-38 Refactor read_integration_test and refix Windows conversion warning

* ros2GH-38 Add StorageFactory test

* Simplify storage factory test

* ros2GH-38 Try to fix flaky test

* ros2GH-38 Move rclcpp::shutdown() at the end

* ros2GH-41 Fix windows warning due to virtual explicit operator bool

* ros2GH-41 Use sqlite3 vendor package in rosbag2

* ros2GH-41 Stop linking tests to sqlite

* ros2GH-41 Fix test fixture on Windows

* ros2GH-41 Cleanup test fixture includes

* ros2GH-41 Print test database name

* ros2GH-41 Correctly determine temp dir on Windows

* ros2GH-41 Show error message on sqlite_open failure

* ros2GH-41 Actually create temp dir on Windows

* ros2GH-41 Fix bool conversion warning in VS2015 build

* Fix CMakeLists.txt after rebase

* ros2GH-40 Implement workouround to fix flaky test

* Update package.xml

* Add gtest test dependencies to package.xml

* ros2GH-40 Move to sqlite3_storage_plugin folder

- The separation into the intended structure and plugin apis is not
  there yet. However, most code will stay in the storage plugin for
  sqlite3 file.
- Proper separation of this code into storage plugin and rosbag layer
  will be done in bsinno/aos-rosbag2#5.

* ros2GH-40 Add TODO comments and small cleanup
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.