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

Make Player and Recorder Composable (#902) #1419

Merged
merged 62 commits into from
Dec 11, 2023
Merged

Conversation

roncapat
Copy link
Contributor

@roncapat roncapat commented Jul 13, 2023

Completion Criteria

  • Allow rosbag2_transport::Player node configuration via parameters.
  • Register rosbag2_transport::Player node as a component.
  • Allow rosbag2_transport::Recorder node configuration via parameters.
  • Register rosbag2_transport::Recorder node as a component.
  • Write tests
    • Be able to access to the protected storage_options, player_options and record_options for verifications via mock_player and mock_recorder.
    • Create node with expected parameters and check all parametwrs w.r.t. what is provided in the YAML files.
    • Check if composition works.

Closes #902

Please feel free to comment, review, share opinions...

Tagging @MichaelOrlov and @emersonknapp as per #902 comment from Emerson. FYI :)

@roncapat roncapat requested a review from a team as a code owner July 13, 2023 16:51
@roncapat roncapat requested review from gbiggs and emersonknapp and removed request for a team July 13, 2023 16:51
@roncapat roncapat marked this pull request as draft July 13, 2023 16:52
@roncapat
Copy link
Contributor Author

roncapat commented Jul 13, 2023

Topic 1 - types of the attributes to be exposed as params

So, let's start with the first issue: RecordOptions, PlayOptions and StorageOptions feature many uint64_t and even some size_t attributes. However, rclcpp parameter API only supports signed integers, being int64_t the widest. We would loose the ability to set the upper half of the uint64_t value range. About size_t, it is both unsigned and platform dependent (even if I think on 90% of current ROS 2 setups with 64 bit x86 or ARM, it should correspond again to an uint64_t).

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 parsing

For 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 init() (to be renamed of course with something appropriate) to avoid duplicating lots of code.
I think it is not possible to embed instead everything in the initializer list (before constructor body), but suggestions are welcome.

Topic 3 - parameter validation

I took inspiration from:
https://github.com/ros2/rclcpp/blob/6e97990a32afa17ab98ef4c800783c5cce801786/rclcpp_components/src/component_manager.cpp#L51-L61
however, do we actually want to put descriptive text of each parameters?

@MichaelOrlov MichaelOrlov requested review from MichaelOrlov and removed request for gbiggs July 13, 2023 22:24
@roncapat
Copy link
Contributor Author

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.

@roncapat roncapat marked this pull request as ready for review July 20, 2023 11:56
Copy link
Contributor

@MichaelOrlov MichaelOrlov left a 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 the init_record_options_from_node_params(std::shared_ptr<rclcpp::Node> node, RecordOptions & record_options); and declare_play_options_rw_params(..) to the init_play_options_from_node_params?
  • Could you please combine declare_storage_options_r_params(..) and declare_storage_options_rw_params() in one function and name it to the init_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 have
      "record.all",
      "record.is_discovery_disabled",
      "storage.max_bagfile_size",
     "storage.max_bagfile_duration",
      // And so on parameters definitions for the node
    
    It would be better to have plain parameter structures without 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 and record_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 and record_options have the expected parameters.

rosbag2_storage/src/rosbag2_storage/storage_options.cpp Outdated Show resolved Hide resolved
rosbag2_storage/src/rosbag2_storage/storage_options.cpp Outdated Show resolved Hide resolved
rosbag2_storage/src/rosbag2_storage/storage_options.cpp Outdated Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/record_options.cpp Outdated Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/record_options.cpp Outdated Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/record_options.cpp Outdated Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/record_options.cpp Outdated Show resolved Hide resolved
@roncapat
Copy link
Contributor Author

Thank you! Will surely implement, starting next week :)

Copy link
Collaborator

@emersonknapp emersonknapp left a 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!

rosbag2_transport/src/rosbag2_transport/player.cpp Outdated Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/play_options.cpp Outdated Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/play_options.cpp Outdated Show resolved Hide resolved
rosbag2_transport/include/rosbag2_transport/player.hpp Outdated Show resolved Hide resolved
@roncapat
Copy link
Contributor Author

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.

@roncapat
Copy link
Contributor Author

roncapat commented Aug 1, 2023

@MichaelOrlov @emersonknapp did a first round of cleanup following your suggestions.
My proposal here is to address first the location of the parsing functions (waiting on your feedback here under) and afterwards the style of the Player constructor(s) (the one with that init() fun).

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

Copy link
Contributor

@MichaelOrlov MichaelOrlov left a 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

@roncapat
Copy link
Contributor Author

roncapat commented Aug 8, 2023

@MichaelOrlov very appreciated! Will surely merge, and also move Play and Record Options parsing in dedicated files in the meantime.

@roncapat
Copy link
Contributor Author

roncapat commented Aug 8, 2023

@MichaelOrlov

If I do not include "<memory>"... cpplint says:

Error: /__w/rosbag2/rosbag2/rosbag2_transport/include/rosbag2_transport/play_options_from_node_params.hpp:24:  Add #include <memory> for shared_ptr<>  [build/include_what_you_use] [4]
Error: /__w/rosbag2/rosbag2/rosbag2_transport/include/rosbag2_transport/record_options_from_node_params.hpp:24:  Add #include <memory> for shared_ptr<>  [build/include_what_you_use] [4]
Error: /__w/rosbag2/rosbag2/rosbag2_transport/include/rosbag2_transport/storage_options_from_node_params.hpp:24:  Add #include <memory> for shared_ptr<>  [build/include_what_you_use] [4]
Error: Category 'build/include_what_you_use' errors found: 3
Error: Total errors found: 3

@MichaelOrlov
Copy link
Contributor

@MichaelOrlov

If I do not include ""... cpplint says:
@roncapat Ok, you are right. We need it for the shared pointer.

@roncapat
Copy link
Contributor Author

roncapat commented Aug 10, 2023

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?

@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).

@roncapat
Copy link
Contributor Author

To implement test, starting from this point:

Make test class for the player and recorder via inheritance to be able to have access to the protected storage_options, player_options and record_options for verifications.

may I just expand MockPlayer and MockRecorder classes with getters for the PlayOptions, StorageOptions and RecordOptions internal structures?

@emersonknapp
Copy link
Collaborator

why don't we PIMPL Player like Recorder?

I don't see any specific reason why not, just not something we've done. Unless @MichaelOrlov has a different perspective.

may I just expand MockPlayer and MockRecorder classes with getters for the PlayOptions, StorageOptions and RecordOptions internal structures?

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.

@roncapat
Copy link
Contributor Author

roncapat commented Aug 21, 2023

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 :)

Copy link
Contributor Author

@roncapat roncapat left a 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.

@@ -65,6 +69,12 @@ target_link_libraries(${PROJECT_NAME}
yaml-cpp
)

rclcpp_components_register_node(
${PROJECT_NAME} PLUGIN "rosbag2_transport::Player" EXECUTABLE Rosbag2Player)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
${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)

Copy link
Contributor

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?

Copy link
Contributor

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 ?

Copy link
Contributor Author

@roncapat roncapat Dec 9, 2023

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.

Copy link
Contributor Author

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)

Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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?

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 :)

Copy link
Contributor

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.

rosbag2_transport/CMakeLists.txt Outdated Show resolved Hide resolved
@roncapat
Copy link
Contributor Author

roncapat commented Dec 9, 2023

Latest failures (linux):

/tmp/ws/src/rosbag2/rosbag2_transport/test/rosbag2_transport/test_composable_recorder.cpp:238
Expected equality of these values:
  storage_options.max_bagfile_size
    Which is: 2147483646
  8601600678

/tmp/ws/src/rosbag2/rosbag2_transport/test/rosbag2_transport/test_composable_recorder.cpp:239
Expected equality of these values:
  storage_options.max_bagfile_duration
    Which is: 2147483646
  54321689657

…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>
@MichaelOrlov
Copy link
Contributor

  • Reruning CI after addressing Windows CI failures

BUILD args: --packages-above-and-dependencies rosbag2_storage rosbag2_transport rosbag2_tests
TEST args: --packages-above rosbag2_storage rosbag2_transport rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/13037

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

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
@MichaelOrlov
Copy link
Contributor

  • re-run Windows CI after fixing warnings
  • Windows Build Status

@roncapat
Copy link
Contributor Author

About the warnings, what if we append the f suffix to the literals in those lines? Should it work or do you think it is caused by something different?

@MichaelOrlov
Copy link
Contributor

About the warnings, what if we append the f suffix to the literals in those lines? Should it work or do you think it is caused by something different?

Ahh.. Windows!!! Need to debug it locally.

@roncapat
Copy link
Contributor Author

Very sorry of not having a W10 machine to help you in this!

Copy link
Contributor

@MichaelOrlov MichaelOrlov left a 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.

roncapat and others added 2 commits December 11, 2023 10:09
…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>
@MichaelOrlov
Copy link
Contributor

@ros-pull-request-builder retest this please

@MichaelOrlov
Copy link
Contributor

  • re-run Windows CI after fixing warnings
  • Windows Build Status

@MichaelOrlov
Copy link
Contributor

@emersonknapp Friendly ping here again. Could you please approve this PR additionally?
We will not be able to merge this PR since you made a request for changes on one of your reviews.
All your concerns and suggestions have been taken into account and addressed.
The CI is green.

Copy link
Collaborator

@emersonknapp emersonknapp left a 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

@MichaelOrlov MichaelOrlov merged commit 0fcc839 into ros2:rolling Dec 11, 2023
13 of 14 checks passed
@roncapat
Copy link
Contributor Author

Thank you very much!

@ros-discourse
Copy link

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

@berndpfrommer
Copy link
Contributor

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.
THANK YOU VERY MUCH for all the hard work you have put in!

@berndpfrommer
Copy link
Contributor

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.

@roncapat
Copy link
Contributor Author

roncapat commented Mar 8, 2024

@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 rolling versions of rcpputils and rosbag2 and rebuilding everything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Composable Player and Recorder nodes
6 participants