Make recorder node composable by inheritance

[bertaveira]

Recreate changes made in #892 for the Galactic branch on the Rolling branch.

This basically enables users to write their own recording nodes by inheriting the rosbag2_transport::Recorder node and keep access to the options and writer.

Very useful for writing a node that starts recording and stops recording on a service call or on a topic value change.

[bertaveira]

If this is approved I would also like to make a pull request to the Humble branch to have this change in there as well. Unfortunately, this had only been merged to the galactic branch before

[MichaelOrlov]

@bertaveira Nop. At least not so easy..
Will need to:

1. Create default objects for storage_options and record options
2. Create writer object and pass all these arguments to the next constructor the same way as it's made here
   

   rosbag2/rosbag2_py/src/rosbag2_py/_transport.cpp

   Lines 211 to 221 in 50e28cb

   	void record(
   	const rosbag2_storage::StorageOptions & storage_options,
   	RecordOptions & record_options)
   	{
   	if (record_options.rmw_serialization_format.empty()) {
   	record_options.rmw_serialization_format = std::string(rmw_get_serialization_format());
   	}
   	auto writer = rosbag2_transport::ReaderWriterFactory::make_writer(record_options);
   	auto recorder = std::make_shared<rosbag2_transport::Recorder>(
   	std::move(writer), storage_options, record_options);

3. Create setter methods for storage_options and record options
4. Add coverage in unit tests

[bertaveira]

Why was the same merge acceptable in the Galactic branch but not in Humble or Rolling? It is blocking us from going to Humble right now and this does not break anything for anyone. Simply enables classes that inherit this to have some extra access that allows it to setup and start recording internally.

1. As I can see record_options and storage_options already have default values of initialisation I think. The structures have default values so it should be initialised with those already right?
2. What do you mean by setter methods?

If I go back on the change of the throw error removal would it be acceptable? Basically would just change the writer and option structures to protected instead of private?

[MichaelOrlov]

@bertaveira setters means methods which provides API to assign record_options and storage_options after calling default constructor.
e.g.
set_record_options(const RecordOptions & record_options) and
set_storage_options(const rosbag2_storage::StorageOptions & storage_options)

Anyway just removing throw rclcpp::exceptions::UnimplementedError(); is not acceptable since need to create default writer and pass it to the another constructor.

Also since writer constructor accept record_options as parameter need to provide "setter" method for writer interface to be able to setup record_options after calling writer constructor.

[bertaveira]

I am not very familiar with this repo and the structure it has. I am totally ok with adding back the throw rclcpp::exceptions::UnimplementedError(); and just changing the variables to protected. I am not sure how to make unit tests and there doesn't seem to be documentation about how to do that. We are also in a bit of a hurry to get this to be usable by inheritance since we want to switch to Humble and this was already fixed in the galactic branch that we have been using.

How should we proceed?

[MichaelOrlov]

@bertaveira If we will not do changes in constructor, i.e. only move some variable from private to protected scope. Then, yes it's ok to proceed without unit test and extra effort for taking care about dependencies.

[MichaelOrlov]

LGTM

[MichaelOrlov]

Gist: https://gist.githubusercontent.com/MichaelOrlov/f63c462dbace9542864e5f7dbe09f94e/raw/6a5743834463552d23bfb7bb6ab0c4839fd6cadd/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_transport rosbag2_tests
TEST args: --packages-above rosbag2_transport rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/10856

• Linux
• Linux-aarch64
• Windows

[MichaelOrlov]

Failure on Linux CI job is flakiness in tests and unrelated to the changes in this PR

[berndpfrommer]

Thanks @bertaveira for pushing those changes, they will really come in handy!