First working rosbag2 implementation#6
Conversation
|
CI:
|
|
New CI:
|
|
New CI:
|
- 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 https://github.com/bsinno/aos-rosbag2/issues/5.
e2e232d to
e688690
Compare
Karsten1987
left a comment
There was a problem hiding this comment.
Given that this is demo code, I approve as-is. I have a few minor comments, but I believe these will be addressed in follow up PRs anyway.
| find_package(SQLite3 REQUIRED) # provided by sqlite3_vendor | ||
|
|
||
| add_library(librosbag | ||
| include/rosbag2/rosbag2.hpp |
There was a problem hiding this comment.
when only used within a source folder, you don't need to specify headers for your library, do you?
There was a problem hiding this comment.
True, but the IDE support is better if you do (at least with CLion)
There was a problem hiding this comment.
Also the fact that it is not required is only due to the way the colcon/ ament build works. Other tools e.g. CPack would also require all files to be listed.
| target_link_libraries(${PROJECT_NAME}_record librosbag) | ||
|
|
||
| add_executable(${PROJECT_NAME}_play src/rosbag2/demo_play.cpp) | ||
| target_link_libraries(${PROJECT_NAME}_play librosbag) |
There was a problem hiding this comment.
I believe there is an install routine missing.
The include folder as well as library/exe aren't installed.
| target_link_libraries(${PROJECT_NAME}_play librosbag) | ||
|
|
||
| if(BUILD_TESTING) | ||
| # TODO(Martin-Idel-SI): Add copyright linter or use ament_lint_auto() once available |
There was a problem hiding this comment.
what is missing to make this available?
There was a problem hiding this comment.
This PR should fix the issues we have: ament/ament_lint#82
| endif() | ||
| endif() | ||
|
|
||
| ament_package() |
There was a problem hiding this comment.
export include and deps?
| { | ||
| std::string filename("test.bag"); | ||
|
|
||
| std::remove(filename.c_str()); |
There was a problem hiding this comment.
It might be worth to modify the filename when there already exists one file with the same filename.
I feel that deleting a file without any warning is pretty dangerous.
There was a problem hiding this comment.
True, we should have annotated this line that it has only been added temporary to work with the hard coded file name.
The normal behavior would be to complain when a specified file already exists.
| } | ||
| }); | ||
|
|
||
| std::cout << "Waiting for messages..." << std::endl; |
There was a problem hiding this comment.
we could generally use rcutils logging here.
https://github.com/ros2/rcutils/blob/master/include/rcutils/logging.h
There was a problem hiding this comment.
Good idea, we will look into it!
| namespace rosbag2 | ||
| { | ||
|
|
||
| SqliteStorage::SqliteStorage(const std::string & database_name, bool shouldInitialize) |
There was a problem hiding this comment.
according to the ROS 2 style guide, this variable should be called should_initialize
| SqliteStorage::SqliteStorage(std::shared_ptr<SqliteWrapper> database) | ||
| : database_(std::move(database)) {} | ||
|
|
||
| bool SqliteStorage::write(const std::string & data) |
There was a problem hiding this comment.
I am not sure if data here should be a const char * because of the string length when putting binary code in it.
Also I believe the string length is computed differently between std::string and const char *:
https://akrzemi1.wordpress.com/2014/03/20/strings-length/
There was a problem hiding this comment.
Correct. At the moment everything is hard-coded to support only string messages. This will change with https://github.com/bsinno/aos-rosbag2/issues/22.
|
I added TODOs so that we won't forget your comments. |
* Adds the PlayFor.srv description. * Implements the play_for verb. * Allows the Subscription manager to spin based on a duration. * Adds to the CMakeLists.txt the message description. * Adds a basic test. * Switch to C++ 17 * Adds * for explicit optional deferentiation. * Adds test with playback for a certain duration. Duration is less than the further message in the queue. * Adds tests with filtered topics. * Improves testing code by removing duplicated code. * Adds PlayUntil service message. * Changes the implementation to support time until. * Adds the service. * Adds test coverage to Player::play_until(timestamp). - Allows to edit BagMetadata from MockReaderInterface. - Provides unit tests to play with different timing initial conditions. - Provides unit tests to play with filtered topics. - Alphasorts the tests for the CMakeList.txt. * Adds python bindings for play until verb * Adds a sample player node and provides instructions to use it. * Update rosbag2_samples/api_samples/bag_recorder_nodes/CMakeLists.txt Co-authored-by: Geoffrey Biggs <gbiggs@killbots.net> Co-authored-by: Geoffrey Biggs <gbiggs@killbots.net>
This PR provides a first cross platform implementation of a native ROS2 rosbag recorder and player.
In particular, the
rosbag2package contains now two executables, one to record messages of typestd_msgs/Stringfrom the topicstring_topicand the other to play to the same topic the same type of messages contained in a previously recorded bag file.The main focus of this PR was being able to read and write from/to an sqlite database.