-
Notifications
You must be signed in to change notification settings - Fork 287
[WIP] Make Player a rosbag2 component (instead of rosbag2_transport)
#1603
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
base: rolling
Are you sure you want to change the base?
Conversation
rosbag2 componentrosbag2 component (instead of rosbag2_transport
rosbag2 component (instead of rosbag2_transportrosbag2 component (instead of rosbag2_transport
Signed-off-by: Patrick Roncagliolo <[email protected]>
14a3681 to
7a80a0e
Compare
|
@MichaelOrlov may I ask if you are still interested in this, and if the proposed method is ok for you, so I can apply the same strategy for Recorder?? |
- Cleanups in CMakeLists.txt - Expose only the necessary constructors in rosbag2::player - Add visibility control - Add missing dependencies in package.xml - Add component_player.hpp file Signed-off-by: Michael Orlov <[email protected]>
Signed-off-by: Michael Orlov <[email protected]>
MichaelOrlov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @roncapat, Yes, I am still interested in this PR.
However, it is too long to describe what needs to be changed.
Instead, I did prototyping and made a couple of direct commits to your branch. Please see c00bf42 and 4f6414e
Please review the changes and feel free to add a composable_recorder.{hpp}cpp file in a similar way.
I've tested loading a new composable rosbag2::Plyer in my local setup and can confirm that it is compiles and runs as expected. However, it would be good to add some unit tests.
To test it I ran in one terminal
ros2 run rclcpp_components component_containerin another terminal
ros2 component load /ComponentManager rosbag2 rosbag2::Player -p storage.uri:=/recordings/talker/talker.mcap -p play.loop:=trueAfter that, you can try to run ros2 component list and ros2 topic echo /topic to make sure that rosbag2::Player composable node is up and running.
I took talker.mcap from the https://github.com/ros2/rosbag2/blob/rolling/rosbag2_py/test/resources/mcap/talker/talker.mcap
Signed-off-by: Michael Orlov <[email protected]>
|
@MichaelOrlov looks nice to me! As soon as I have a bit of time, I will do the same for Recorder class. |
|
cc: @emersonknapp |
Signed-off-by: Patrick Roncagliolo <[email protected]>
Signed-off-by: Patrick Roncagliolo <[email protected]>
|
@MichaelOrlov done, sorry for the delay. |
Signed-off-by: Michael Orlov <[email protected]>
MichaelOrlov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roncapat Thank you for the follow-up.
The latest changes look mostly good to me. I pushed one commit with fixes for the CPPLint warnings.
What we are missing now is a basic integration unit tests to make sure that invoking composable player can be constructed and play messages and composable recorder can be constructed from CLI command and record messages.
In Rosbag2 we have a special package rosbag2_tests for such type of integration tests where almost all required instrumentation already defined.
Could you please write a few integration tests?
You can use test_rosbag2_record_end_to_end.cpp and test_rosbag2_play_end_to_end.cpp as a good reference and example.
Also feel free to use cdr_test bag files from resource folder for playback test.
For recording tests feel free to use RecordFixture class. There are already present instrumentation to handle recorded files from Rosbag2 recorder in the system temp folder.
Also could you please sign your 51d303f commit to pass DCO check on CI?
rosbag2 component (instead of rosbag2_transportrosbag2 component (instead of rosbag2_transport)
Closes #1736
This is a proposal to address https://github.com/ros2/rosbag2/pull/1510/files#r1484600093.
CC @MichaelOrlov @fujitatomoya
NOTE: I compiled it and the component gets correclty exposed via
ros2 component typesbut I haven't tested it yet.If it works, I extend it also to
Recorderand then it's ready for merge.