Composable Player and Recorder nodes

[hidmic]

Description

To enable intra-process bag replaying and recording, sometimes necessary when dealing with high-frequency and/or high-bandwidth data streams, both rosbag2_transport::Player and rosbag2_transport::Recorder nodes have to be exposed as rclcpp components.

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.

Testing Notes / Suggestions

Updating test_record.cpp and test_play.cpp to use intra-process comms and inter-process comms for their nodes should do.

[doisyg]

Also interested in having composable and IPC player capability. What is the current status for Humble ? Any plans ?

[berndpfrommer]

Is introducing PR #892 an option? I don't think it would interfere with implementing a better solution later down the road and it would keep my stopgap solution alive.

[roncapat]

Is someone already tackling this? @Karsten1987? Otherwise, I may try to open a PR as soon as I find some spare time.

rosbag2/rosbag2_transport/src/rosbag2_transport/player.cpp

Lines 92 to 99 in 0b17e4d

	Player::Player(const std::string & node_name, const rclcpp::NodeOptions & node_options)
	: rclcpp::Node(node_name, node_options)
	{
	// TODO(karsten1987): Use this constructor later with parameter parsing.
	// The reader, storage_options as well as play_options can be loaded via parameter.
	// That way, the player can be used as a simple component in a component manager.
	throw rclcpp::exceptions::UnimplementedError();
	}

[emersonknapp]

@roncapat - @Karsten1987 has moved on to other projects and hasn't been working on rosbag2 for a while, none of the active maintainers currently have this on their plate, so contribution is very welcome. Perhaps you'd like to attend the rosbag2 working group Friday morning to discuss the design? https://calendar.google.com/calendar/u/0/embed?src=agf3kajirket8khktupm9go748@group.calendar.google.com (note default UTC time zone, it's 9am Pacific). If that's not feasible for you we can discuss over GitHub - feel free to open draft PRs early in development and ping @MichaelOrlov and myself to talk about overall structure.

[roncapat]

@emersonknapp thank you very much for feedback! I'll try (and let you know) if I can manage to align friday meeting in my agenda, otherwise I will surely draft something here on github and discuss with you here - happy to contribute if I can :)

[roncapat]

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

Do you have an opinion on this, or experience from other packages? Can we afford to loose half of the positive interval for those values?

My personal answer is: of course yes (see below) but looks a bit ugly to be forced by parameter system not to parse an unsigned integer. So I'd implement some ugly-yet-needed type casts there...

Example to prove we are not loosing anything concrete here:
StorageOptions.max_bagfile_size --> still around 8 Petabytes...
StorageOptions.max_bagfile_duration --> countless years...
StorageOptions.max_cache_size --> quintillions of messages...

PlayOptions.read_ahead_queue_size --> quintillions of messages...

RecordOptions.compression_queue_size --> quintillions of messages...
RecordOptions.compression_threads --> are all there enough CPU cores in the whole world?

[emersonknapp]

Personal opinion on that opinion - it ain't ugly if it's needed. We can just validate that the parameters are positive, and cast them to the necessary types. No big deal. Anybody who wants that extra range is doing something very special and can work around it by not using Parameters for configuration.

[roncapat]

Thanks! BTW, the linked PR builds fine, but I will probably have time to test it properly not earlier than August.

If you have time to review even part of the proposed code in the meantime, I will be very happy to incorporate feedbacks :)

[nachovizzo]

Any update on this one folks 👼 ? I'm happy to jump and help if that's neccesary

[roncapat]

Any update on this one folks 👼 ? I'm happy to jump and help if that's neccesary

Working hard right now on this prerequisite #1476! Feel free to follow or suggest improvements of course. #1476 started as a small tweak to better support configurability of QoS from parameter files, and became quite a huge patch due to all the inter-dependencies here-and-there in the codebase.

[roncapat]

@nachovizzo lots of updates on the associated PR #1419. Would you like testing / provide some feedback?

[nachovizzo]

@nachovizzo lots of updates on the associated PR #1419. Would you like testing / provide some feedback?

Sorry, I missed this notification. Silly me. Feedback: Extremely happy to see this got merged :D

Great job man!!!

[roncapat]

No worries, still valid :) If you ever happen to leverage this feature and discover issues, tag me and let me know!

[nachovizzo]

Sure thing ;)