-
Notifications
You must be signed in to change notification settings - Fork 248
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
Make Player and Recorder Composable (#902) #1419
Conversation
Topic 1 - types of the attributes to be exposed as paramsSo, let's start with the first issue: For now, I've done casts all around, since the numeric range of int64_t is extraordinarily wide to express a size in bytes or number of messages. Topic 2 - where to place the parsingFor now, I filled the missing constructor with param-parsing code. However, doing this means that we loose a lot of constructor chaining logic that is in place for the pre-existing ones. I ended up making this constructor independent on the others, and generating a new method Topic 3 - parameter validationI took inspiration from: |
I think it would be nice to move parameter declaration/parsing to static methods in StorageOptions, PlayOptions and RecordOptions (passing node handle). This way, no duplication of code happens between Player and Recorder. |
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 your contribution.
It looks like it is going to be a significant effort.
I like the idea of moving options initialization from node to the separate functions and the ability to reuse them from player and recorder.
However, I think that PR is more or less in the draft stage and some parts of it need some "love".
Here are some general comments for the review.
- Could you please rename
declare_record_options_rw_params(std::shared_ptr<rclcpp::Node> nh, RecordOptions & ro)
to theinit_record_options_from_node_params(std::shared_ptr<rclcpp::Node> node, RecordOptions & record_options);
anddeclare_play_options_rw_params(..)
to theinit_play_options_from_node_params
? - Could you please combine
declare_storage_options_r_params(..)
anddeclare_storage_options_rw_params()
in one function and name it to theinit_storage_options_from_node_params(std::shared_ptr<rclcpp::Node> node, StorageOptions & storage_options)
? It's Ok to call it from both the player and recorder - Please use full names for the parameters and variables. e.g.
(std::shared_ptrrclcpp::Node node, StorageOptions & storage_options) instead of the(std::shared_ptr<rclcpp::Node> nh, StorageOptions & so)
to adhere a common style in the rosbag2 and ROS2. - I think it makes sense to change structured node parameters definitions to the plain one-layer parameters list as we are doing it in CLI options.
For instance, currently, we haveIt would be better to have plain parameter structures without"record.all", "record.is_discovery_disabled", "storage.max_bagfile_size", "storage.max_bagfile_duration", // And so on parameters definitions for the node
recorder
and other
prefixes. It will simplify the definition of the parameters in launch files and command line for the composable recorder and player. And will match with parameters from CLI args for standalone rosbag2 recorder and player. - Will need to add a few unit tests. I envision the test approach as follows:
- Make test class for the player and recorder via inheritance to be able to have access to the protected
storage_options
,player_options
andrecord_options
for verifications. - Create node with expected parameters.
- Create relevant test player or recorder classes via passing previously instantiated node with parameters.
- Test if
storage_options
,player_options
andrecord_options
have the expected parameters.
- Make test class for the player and recorder via inheritance to be able to have access to the protected
Thank you! Will surely implement, starting next week :) |
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.
Left one pass of structural comments, I'd like to see this change look a little different top down.
However, please don't take the feedback as discouragement, it's a great start and I totally appreciate you taking on this issue!
No worries, and thank you both for the reviews! Will do a first pass of minor comments solving, so that I/we can focus then on more architectural issues you pointed out. |
@MichaelOrlov @emersonknapp did a first round of cleanup following your suggestions. Moreover, please check my replies about the constructor signature needed for componentization, feel free to mark as resolved those points if they are clearer to you now :) I will then implement tests |
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 moving forward with this PR and addressing my comments.
I confirm changes which you have marked as resolved among a few nitpicks, see inline.
As regards the @emersonknapp comments I agree that it would be better to move storage options init from node params to the rosbag2_transport
layer I have tried to prototype and made in on my branch. See my pull request roncapat#1 feel free to merge it on your branch.
As regards to the Player constructor and those Init()
method I agree that it would be better to have the cascade of the embaded constructors and get rid from the Init()
method but don;t envision how to do this without decomposing base node class from inheritance to the inner member variable. BUt not sure if it is a god idea and if it will be possible.
@emersonknapp may be you will have some ideas?
Please take a look on my latest changes in the roncapat#1
@MichaelOrlov very appreciated! Will surely merge, and also move Play and Record Options parsing in dedicated files in the meantime. |
If I do not include "<memory>"... cpplint says:
|
|
@emersonknapp @MichaelOrlov proposal: why don't we PIMPL Player like Recorder? If you notice, adding parameter parsing to Recorder was a way smoother process than Player, thants to its structure (based on PIMPL pattern). |
To implement test, starting from this point:
may I just expand MockPlayer and MockRecorder classes with getters for the PlayOptions, StorageOptions and RecordOptions internal structures? |
I don't see any specific reason why not, just not something we've done. Unless @MichaelOrlov has a different perspective.
No major concerns - just be wary of mocking so much that you're not testing any of the real code :) Sorry if my responses are a bit slow - I'm just starting a new job and figuring out the balance between that and my open source maintenance as I get ramped up. |
Don't worry! I'm implementing PIMPL in Player as a separate PR, so it can be reviewed individually. EDIT: see #1447. Give me some time to let it work properly and check if some changes in tests are required or not :) |
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.
One more quick thing: better executable names.
rosbag2_transport/CMakeLists.txt
Outdated
@@ -65,6 +69,12 @@ target_link_libraries(${PROJECT_NAME} | |||
yaml-cpp | |||
) | |||
|
|||
rclcpp_components_register_node( | |||
${PROJECT_NAME} PLUGIN "rosbag2_transport::Player" EXECUTABLE Rosbag2Player) |
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.
${PROJECT_NAME} PLUGIN "rosbag2_transport::Player" EXECUTABLE Rosbag2Player) | |
${PROJECT_NAME} PLUGIN "rosbag2_transport::Player" EXECUTABLE player) |
@MichaelOrlov one last thing... I believe it's better to rename the nodes, so that:
ros2 run rosbag2_transport player
The "Rosbag2" prefix is redundant IMHO.
And I believe that for node names lowercase convention is preferred.
(Same for Recorder here below)
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 Out of curiosity, is it possible to make it ros2 run rosbag2 player
without renaming rosbag2_transport
to the rosbag2
?
Maybe using an alias for the entry point?
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.
Or maybe if can get rid of rosbag2_transport
keyword in front. To be ros2 run Rosbag2Player
?
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.
I believe that the semantic of the ros2 run
command is 'package_name node_name' and can't be changed. We can only act on the node name.
Maybe we can then find a way to register the node under package rosbag2
nstead of 'roabag2_transport`. But it may be very tricky.
However, in the long term (I can open an Issue for later implementation) I just had a potentially nice idea: to upgrade ros2 bag play
CLI support with a parameter for an existing container name. This way, the CLI would load the component in an existing container, start playing (and eventually unloading it at the end of the playback). This would be a neat feature IMHO. Thoughts?
Summing up: simplest solution here is at least for now just to choose a simple and idiomatic node name. We may act on moving the nodes to rosbag2
package later on.
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.
(To register the nodes under "rosbag2" instead of "rosbag2_transport" we may need to hack the inner workings of this: https://github.com/ros2/rclcpp/blob/rolling/rclcpp_components%2Fcmake%2Frclcpp_components_register_node.cmake)
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 Ahh never mind. Feel free to go ahead and commit your suggestions with signoff.
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.
As regards:
This way, the CLI would load the component in an existing container, start playing (and eventually unloading it at the end of the playback). This would be a neat feature IMHO. Thoughts?
Curious why it would be useful at all if we already have entry point for ros2 bag play
and ros2 bag record
?
I only envision a case when we can run player or recorder and specify settings yaml file instead of the CLI parameters directly. Something like ros2 bag play --settings playback_settings.yaml
The same questions apply to how to use ros2 run rosbag2_transport player
in a real use case? How we will provide settings or node parameters in this case?
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.
As regards:
This way, the CLI would load the component in an existing container, start playing (and eventually unloading it at the end of the playback). This would be a neat feature IMHO. Thoughts?
Curious why it would be useful at all if we already have entry point for
ros2 bag play
andros2 bag record
?
Imagine people have already scripted their ros2 bag play/record
commands (for example, in some launch files). With a small modification (like the addition of one CLI parameter) they will be able to "upgrade their setup" and inject player/recorder in their component containers. Increased performance with almost zero effort (if you have specified all parameters in the CLI command, you won't have to migrate to a .yaml configuration file).
The same questions apply to how to use
ros2 run rosbag2_transport player
in a real use case? How we will provide settings or node parameters in this case?
Like normal node parameters via YAML file. Notice that this also enables people to use Node()
objects in LaunchConfiguration()
of launchfiles instead of the current only way to run player/recorder via launchfiles (to specify the full CLI command).
In the next days I'll write some docs to push as separate PR. It may be the right context to sum up all the past, present and potentially future ways to run players/recorders if it's ok for you :)
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 As regards:
magine people have already scripted their ros2 bag play/record commands (for example, in some launch files). With a small modification (like the addition of one CLI parameter) they will be able to "upgrade their setup" and inject player/recorder in their component containers. Increased performance with almost zero effort (if you have specified all parameters in the CLI command, you won't have to migrate to a .yaml configuration file).
I don't really understand about what increased performance and benefits you are talking about there. Maybe with documentation or some real-world examples will be clear to me.
As regards:
The same questions apply to how to use
ros2 run rosbag2_transport player
in a real use case? How we will provide settings or node parameters in this case?
Like normal node parameters via YAML file. Notice that this also enables people to use Node() objects in LaunchConfiguration() of launchfiles instead of the current only way to run player/recorder via launchfiles (to specify the full CLI command).
Still unclear to me how node parameters will be settled with ros2 run rosbag2_transport player
command. What you described seems more relevant for launch files launching composed nodes via the launcher. But it is irrelevant to the ros2 run rosbag2_transport player
command as far as understand.
In the next days I'll write some docs to push as separate PR. It may be the right context to sum up all the past, present and potentially future ways to run players/recorders if it's ok for you :)
Anyway, documentation would be very helpful and grateful to have.
Latest failures (linux):
|
rosbag2_transport/test/rosbag2_transport/test_composable_recorder.cpp
Outdated
Show resolved
Hide resolved
…der.cpp Signed-off-by: Patrick Roncagliolo <ronca.pat@gmail.com>
Signed-off-by: Patrick Roncagliolo <ronca.pat@gmail.com>
Signed-off-by: Patrick Roncagliolo <ronca.pat@gmail.com>
BUILD args: --packages-above-and-dependencies rosbag2_storage rosbag2_transport rosbag2_tests |
Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
About the warnings, what if we append the |
Ahh.. Windows!!! Need to debug it locally. |
Very sorry of not having a W10 machine to help you in this! |
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 See my suggestions for Windows warnings fixes.
Now the warnings should be 100% fixed. I tested the build locally and it is clean without warnings after the propoused fixes.
rosbag2_transport/src/rosbag2_transport/config_options_from_node_params.cpp
Outdated
Show resolved
Hide resolved
rosbag2_transport/test/rosbag2_transport/test_composable_player.cpp
Outdated
Show resolved
Hide resolved
…de_params.cpp Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Patrick Roncagliolo <ronca.pat@gmail.com>
…r.cpp Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Patrick Roncagliolo <ronca.pat@gmail.com>
@ros-pull-request-builder retest this please |
@emersonknapp Friendly ping here again. Could you please approve this PR additionally? |
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.
Ok, I haven't reviewed thoroughly but it looks like you've all put in the hard work on this so it's fine by me
Thank you very much! |
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-for-2023-12-14/35153/1 |
I finally got around to testing the composable recorder and it works GREAT! I can start recording by loading the recorder into a running camera driver node, and stop by unloading it. I can compress and record 10 camera streams at 40fps 1920x1200. CPU load is about 35% less than with separate recorder/driver setup and with the reduced memcpys the host feels much more responsive when running other tasks. |
Question though: right now I'm running on Rolling on Ubuntu 22.04, but upgrading my host to Ubuntu 24.04 is very difficult since we must be recording daily. Is there a chance that this feature will be backported to Iron? I tried backporting to humble but the code base has diverged so much in other places that I gave up. Haven't tried Iron yet. |
@berndpfrommer thank you very much for your feedback on this, it always feels great to see adoption and appreciation for this kind of contributions :) That said, about backporting, I think it's undoable. However, I can tell you that in my lab I run entire systems with source builds of Iron, and I deployed this updated package simply by checking out |
Completion Criteria
rosbag2_transport::Player
node configuration via parameters.rosbag2_transport::Player
node as a component.rosbag2_transport::Recorder
node configuration via parameters.rosbag2_transport::Recorder
node as a component.mock_player
andmock_recorder
.Closes #902
Please feel free to comment, review, share opinions...
Tagging @MichaelOrlov and @emersonknapp as per #902 comment from Emerson. FYI :)