Add the ability to change the node name of the recording node using the recorder API

[ricardo-manriquez]

Hello everyone, I'm new to this so I wanted to start asking a question

Description

I'm coding a system that will need to run multiple ROS bag recordings at the same time, and one limitation is that every node launched has the same name, I wanted to fix this problem, I wrote a small patch but it doesn't keep the original API which is a no-no.

My recording code is as follows

import rosbag2_py

bag_name = "test"

storage_options = rosbag2_py._storage.StorageOptions(
	uri='mytest2',
	storage_id='sqlite3'
)

record_options = rosbag2_py.RecordOptions()
recorder = rosbag2_py.Recorder()

recorder.record(storage_options, record_options, bag_name)

This code mimics the C++ API where this is legal

  Recorder(
    std::shared_ptr<rosbag2_cpp::Writer> writer,
    const rosbag2_storage::StorageOptions & storage_options,
    const rosbag2_transport::RecordOptions & record_options,
    const std::string & node_name = "rosbag2_recorder",
    const rclcpp::NodeOptions & node_options = rclcpp::NodeOptions());

So I wrote the following patch:

diff --git a/rosbag2_py/src/rosbag2_py/_transport.cpp b/rosbag2_py/src/rosbag2_py/_transport.cpp
index bbfa21b..12b5266 100644
--- a/rosbag2_py/src/rosbag2_py/_transport.cpp
+++ b/rosbag2_py/src/rosbag2_py/_transport.cpp
@@ -162,7 +162,7 @@ public:

   void record(
     const rosbag2_storage::StorageOptions & storage_options,
-    RecordOptions & record_options)
+    RecordOptions & record_options, std::string node_name)
   {
     if (record_options.rmw_serialization_format.empty()) {
       record_options.rmw_serialization_format = std::string(rmw_get_serialization_format());
@@ -170,7 +170,7 @@ public:

     auto writer = rosbag2_transport::ReaderWriterFactory::make_writer(record_options);
     auto recorder = std::make_shared<rosbag2_transport::Recorder>(
-      std::move(writer), storage_options, record_options);
+      std::move(writer), storage_options, record_options, node_name);
     recorder->record();

     exec_->add_node(recorder);

But this breaks the API.

My questions are as follows:

• What is the standard ros2/rosbag2 way to implement such behavior (function overloading in the C++ API)?
• If written correctly could this change be considered for merging?

I know one way to implement this would be using optional keyword arguments but I couldn't find any such example in the ros codebase, so maybe it's use is discouraged and there is a better way to implement this.

Thanks to everyone for their time and please tell me about any way to move this forward.

[ricardo-manriquez]

A respectful follow up to this ticket because I still don't know the proper way to solve this, I will rewrite my questions

I want to be able to choose the ROS topic name when launching a recording, the Python API does not have any option to do that but the C++ API does, in which way do rosbag developers implement method overloading? Do you use a second function with another name?

Is there a document describing the programming guidelines?

Thank you very much for your time!

[emersonknapp]

In Python you can do optional arguments with default values - that would make it a non-breaking change to the API. pybind11 allows you to specify default arguments in the function bindings.

I'd rather see your patch in a pull request than in an issue - easier to review, and the changes even get built and tested automatically :)

Note: this is either a duplicate or extension of #425. Perhaps we should combine the issues

[emersonknapp]

Is there a document describing the programming guidelines?

https://docs.ros.org/en/rolling/The-ROS2-Project/Contributing/Developer-Guide.html

https://docs.ros.org/en/rolling/The-ROS2-Project/Contributing/Code-Style-Language-Versions.html

What is the standard ros2/rosbag2 way to implement such behavior (function overloading in the C++ API)?

Do you mean in the rosbag2_py Python binding? If so then, I'd say look for any existing instances of it. Match existing style as best as you can, and if it doesn't exist yet then we probably haven't decided on how it should be handled.

If written correctly could this change be considered for merging?

Best evaluated in a pull request, especially given how simple the change is, it took longer to ask for permission than to put it up for review :)

[ricardo-manriquez]

Thank you very much for your comments, this is my first time around so I thought it was more polite to ask for advice rather than just go ahead, I will submit the PR today, I didn't see the previous issue so yeah it might be a good mechanism to implement that behavior.