[galactic] allow for implementation of composable recorder via inheritance #892

berndpfrommer · 2021-10-26T13:12:34Z

This PR is meant to be a stop gap measure until a full implementation of a composable Node is complete.

It will allow developers to write their own recorder node under galactic by inheriting from rosbag2_transport::Recorder and const-casting the references returned by the accessors. To make this work the constructor was changed to no longer throw an exception, but instead log a warning.

The motivation for this PR is the need to record high bandwidth and frequency data that interprocess communication was not able to handle.

ros-discourse · 2021-10-26T21:04:48Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros2-speed/20162/28

emersonknapp · 2021-10-27T20:38:06Z

rosbag2_transport/include/rosbag2_transport/recorder.hpp

@@ -78,6 +78,18 @@ class Recorder : public rclcpp::Node
  ROSBAG2_TRANSPORT_PUBLIC
  const rosbag2_cpp::Writer & get_writer_handle();

+  ROSBAG2_TRANSPORT_PUBLIC


Can you explain more why you need the const-ref accessors for storage/record options? Would it be better instead to make these members protected so that your subclass could even modify them?

Of course protected is more convenient, but I thought that would be asking for too much :).
I figured you had plans to at some later point do a nice, clean, well documented implementation with proper naming conventions etc. If you make those members protected, people may derive from the class, start implementing their own recording nodes, and then bug you with questions about how your code works (and why their code doesn't). By providing just const accessors you are are allowing impatient people like myself to implement stuff via const cast, but essentially discouraging it. That was my sort of twisted rationale.

Yeah, I get that, but I think it would be even better to just expose subclassing as a supported use case. I think the flexibility is probably a good thing? I can't think of a reason why it would be very bad.

emersonknapp · 2021-10-27T20:43:35Z

rosbag2_transport/src/rosbag2_transport/recorder.cpp

 {
  // TODO(karsten1987): Use this constructor later with parameter parsing.
  // The reader, storage_options as well as record_options can be loaded via parameter.
  // That way, the recorder can be used as a simple component in a component manager.
-  throw rclcpp::exceptions::UnimplementedError();
+  RCLCPP_WARN(get_logger(), "rosbag2_transport::Recorder node is not implemented yet!");


This doesn't seem accurate. This constructor is "implemented" in this case - but the thing that's missing is: configuring StorageOptions/RecordOptions via Parameters. If this constructor is used as written here, then the options will all have default values - which I don't think would break anything.

If your subclass looked like the following (with options being protected rather than private), then I believe you could have full control over the configuration:

class MyRecorder : rosbag2_transport::Recorder { MyRecorder(const std::string & name, const rclcpp::NodeOptions & node_options) : Recorder(name, node_options) { storage_options_.whatever = something; /// etc. record_options_.something = whatever; /// etc. } };

At that point, I'd remove the bit about "not implemented" and instead just leave the TODO note about being able to configure via Parameters.

Agreed that the message is not accurate. I couldn't think of a better one, and like in my comment above, wanted to remind the user with a warning that they are venturing into sort of unsupported territory.
If you are generously allowing protected member access already, how about including stop_discovery_ so it can be set to false as well?
BTW I haven't figured out how to actually stop a recording (other than ctrl-c). Here's a link to my recorder class. It will be tossed out the moment you provide a standard implementation.

I think it would not be too crazy to expose these things as protected - I don't see any real reason why Recorder should not be extensible, these were initially just made private as there wasn't a use case for subclassing yet.

BTW I haven't figured out how to actually stop a recording

Yes, currently the only way to stop recording is to deconstruct the Recorder object. This doesn't necessarily have to require a SIGINT (ctrl-c). A Python interface for recording cancel was added in #854, and you can see how it uses scope exit to deconstruct the Recorder. But it would make sense to me to shift this functionality down a level to the C++ layer instead.

I wouldn't know how to deconstruct the Recorder if it's loaded as a composable node, so yes, some way to stop the recording would be nice.
Before going with inheritance I tried writing a composable node that then instantiates a Recorder object, but strange things happened. I got complaints (I believe from the logger) about duplicate nodes, then segfaults.

I wouldn't know how to deconstruct the Recorder if it's loaded as a composable node

I think you unload the node from the component container? That would be a way to deconstruct it.

I got complaints (I believe from the logger) about duplicate nodes,

The Recorder is a Node - so I think the errors are coming from having a extra-Node-in-a-composed-component, which is not allowed (from my understanding). At some point, we had some of it separated, but at the end of the day a Recorder needs to be able to explore the ROS graph and create Subscriptions, which is a functionality that needs a Node - so it makes sense for it to have control over it's Node lifecycle.

The alternative that I can think of would be to pass a Node to the recorded to use, but ownership would be fuzzy there.

berndpfrommer · 2021-10-28T14:34:04Z

Changed the PR according to review: don't throw exception, change private -> protected.
Here is an example package that implements a derived class.

emersonknapp · 2021-10-28T17:36:47Z

rosbag2_transport/include/rosbag2_transport/recorder.hpp

@@ -78,7 +78,7 @@ class Recorder : public rclcpp::Node
  ROSBAG2_TRANSPORT_PUBLIC
  const rosbag2_cpp::Writer & get_writer_handle();

-private:
+protected:


i'd like to think carefully about whether all of this should be protected. Can we just expose what you need, for now? That would allow for reviewing best compartmentalizing on a piece-by-piece basis.

emersonknapp · 2021-10-28T17:40:28Z

rosbag2_transport/src/rosbag2_transport/recorder.cpp

@@ -50,12 +50,13 @@ namespace rosbag2_transport
 Recorder::Recorder(
  const std::string & node_name,
  const rclcpp::NodeOptions & node_options)
-: rclcpp::Node(node_name, node_options)
+: rclcpp::Node(node_name, rclcpp::NodeOptions(node_options).start_parameter_event_publisher(false)),


Does this parameter publisher option need to be a default? If you are subclassing, your class constructor can call this constructor with whatever node options you want. It seems like we shouldn't force this option on all possible users?

Same with stop_discovery_.

E.g.

class MyRecorder : Recorder { MyRecorder(node_name, node_options) : Recorder(node_name, NodeOptions(node_options).start_parameter_event_publisher(false)) { stop_discovery_ = false; } };

Signed-off-by: Bernd Pfrommer <bernd.pfrommer@gmail.com>

berndpfrommer · 2021-10-28T20:28:48Z

Another round of changes based on the last review.
If you want a minimal constructor may as well not create the Writer in the constructor, but leave that to the derived class as well. That however requires the writer_ member variable to be protected. It's a matter of taste which of the two is the lesser evil but by choosing the constructor to be minimal at least it's a consistent choice.

emersonknapp

I think I like this approach best - it allows you to do whatever you need while also requiring a very minimal diff here

emersonknapp · 2021-10-28T22:15:31Z

Gist: https://gist.githubusercontent.com/emersonknapp/13ac6cff0a0d3a8941710d9fd88c6141/raw/af1051d958157af305c6bbd330b47f1604fc745a/ros2.repos
BUILD args: --packages-up-to rosbag2_transport rosbag2_tests rosbag2
TEST args: --packages-select rosbag2_transport rosbag2_tests rosbag2
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/9264

Linux
Linux-aarch64
macOS
Windows

emersonknapp · 2021-10-29T00:15:04Z

Sorry - I forgot that this was targeting galactic - rerunning CI against that distro

emersonknapp · 2021-10-29T00:16:00Z

Gist: https://gist.githubusercontent.com/emersonknapp/1c30619c474d1a3b55cad90dce0c60e6/raw/22fa16f0e09fc0f446a3f032910f858bd2c60336/ros2.repos
BUILD args: --packages-up-to rosbag2_transport rosbag2_tests rosbag2
TEST args: --packages-select rosbag2_transport rosbag2_tests rosbag2
ROS Distro: galactic
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/9269

Linux
Linux-aarch64
macOS
Windows

berndpfrommer · 2021-10-31T21:11:04Z

If I want this commit also to find its way into new releases, should I submit a separate PR against the master branch or is there a way by which you can cherry pick the commit into master?

emersonknapp · 2021-11-01T18:33:10Z

Generally speaking we go the other direction: develop on master and cherry-pick backports to released distros. I am considering this one a kind of "end of the line" patch to make this usable in Galactic, where we should probably develop the more complete feature moving forward.

…#892) Signed-off-by: Bernd Pfrommer <bernd.pfrommer@gmail.com>

hidmic · 2021-11-02T13:31:05Z

@emersonknapp shall we open a ticket for this? Intra-process comms are an important feature for many.

emersonknapp · 2021-11-02T15:17:01Z

Yeah, an issue in the tracker is the best way to go

hidmic · 2021-11-02T17:55:24Z

FYI #902.

berndpfrommer · 2022-02-22T13:51:08Z

I just checked and as of today 2/22/2022 the modified header files have not made it into the official ubuntu galactic packages. Is there a schedule for when that will happen?

berndpfrommer · 2022-08-13T12:26:54Z

@emersonknapp sorry to bother you again with this but it's 9 months now and the currently distributed Ubuntu galactic packages don't seem to have this commit. I don't understand how the release mechanism works or where to ask such questions. Could you please let me know how to get this moving?
Thanks!

roncapat · 2023-05-23T08:29:42Z

Is there an example implementation of such composable recorder?
FYI @Fixit-Davide

berndpfrommer · 2023-05-23T09:27:15Z

Here is an example implementation: https://github.com/berndpfrommer/rosbag2_composable_recorder

roncapat · 2023-05-23T09:55:23Z

Thank you so much!

berndpfrommer requested a review from a team as a code owner October 26, 2021 13:12

berndpfrommer requested review from emersonknapp and mjeronimo and removed request for a team October 26, 2021 13:12

berndpfrommer mentioned this pull request Oct 26, 2021

can we run rosbag2 record / play as a ROS2 node or component? #685

Closed

emersonknapp reviewed Oct 27, 2021

View reviewed changes

berndpfrommer force-pushed the accessors_for_recorder branch 2 times, most recently from c18f947 to e5d36df Compare October 28, 2021 14:25

berndpfrommer changed the title ~~provide more accessors to Recorder, allow Node constructor~~ allow for implementation of composable node via inheritance Oct 28, 2021

berndpfrommer changed the title ~~allow for implementation of composable node via inheritance~~ allow for implementation of composable recorder via inheritance Oct 28, 2021

emersonknapp reviewed Oct 28, 2021

View reviewed changes

allow for implementation of composable recorder via inheritance

4249120

Signed-off-by: Bernd Pfrommer <bernd.pfrommer@gmail.com>

berndpfrommer force-pushed the accessors_for_recorder branch from e5d36df to 4249120 Compare October 28, 2021 20:25

emersonknapp approved these changes Oct 28, 2021

View reviewed changes

emersonknapp changed the title ~~allow for implementation of composable recorder via inheritance~~ [galactic] allow for implementation of composable recorder via inheritance Oct 29, 2021

emersonknapp merged commit ecf8313 into ros2:galactic Oct 29, 2021

eborghi10 pushed a commit to eborghi10/rosbag2 that referenced this pull request Nov 2, 2021

allow for implementation of composable recorder via inheritance (ros2…

74f86f6

…#892) Signed-off-by: Bernd Pfrommer <bernd.pfrommer@gmail.com>

berndpfrommer deleted the accessors_for_recorder branch February 22, 2022 13:52

This was referenced Sep 16, 2022

Make recorder node composable by inheritance #1093

Merged

[Humble] make recorder node composable by inheritance (backport #1093) #1097

Merged

berndpfrommer mentioned this pull request Nov 13, 2022

Composable Player and Recorder nodes #902

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[galactic] allow for implementation of composable recorder via inheritance #892

[galactic] allow for implementation of composable recorder via inheritance #892

berndpfrommer commented Oct 26, 2021

ros-discourse commented Oct 26, 2021

emersonknapp Oct 27, 2021

berndpfrommer Oct 27, 2021

emersonknapp Oct 28, 2021

emersonknapp Oct 27, 2021

berndpfrommer Oct 27, 2021 •

edited

Loading

emersonknapp Oct 28, 2021

berndpfrommer Oct 28, 2021

emersonknapp Oct 28, 2021 •

edited

Loading

berndpfrommer commented Oct 28, 2021

emersonknapp Oct 28, 2021

emersonknapp Oct 28, 2021

berndpfrommer commented Oct 28, 2021

emersonknapp left a comment •

edited

Loading

emersonknapp commented Oct 28, 2021

emersonknapp commented Oct 29, 2021

emersonknapp commented Oct 29, 2021

berndpfrommer commented Oct 31, 2021

emersonknapp commented Nov 1, 2021

hidmic commented Nov 2, 2021

emersonknapp commented Nov 2, 2021

hidmic commented Nov 2, 2021

berndpfrommer commented Feb 22, 2022

berndpfrommer commented Aug 13, 2022

roncapat commented May 23, 2023

berndpfrommer commented May 23, 2023 •

edited

Loading

roncapat commented May 23, 2023

[galactic] allow for implementation of composable recorder via inheritance #892

[galactic] allow for implementation of composable recorder via inheritance #892

Conversation

berndpfrommer commented Oct 26, 2021

ros-discourse commented Oct 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berndpfrommer Oct 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emersonknapp Oct 28, 2021 • edited Loading

Choose a reason for hiding this comment

berndpfrommer commented Oct 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berndpfrommer commented Oct 28, 2021

emersonknapp left a comment • edited Loading

Choose a reason for hiding this comment

emersonknapp commented Oct 28, 2021

emersonknapp commented Oct 29, 2021

emersonknapp commented Oct 29, 2021

berndpfrommer commented Oct 31, 2021

emersonknapp commented Nov 1, 2021

hidmic commented Nov 2, 2021

emersonknapp commented Nov 2, 2021

hidmic commented Nov 2, 2021

berndpfrommer commented Feb 22, 2022

berndpfrommer commented Aug 13, 2022

roncapat commented May 23, 2023

berndpfrommer commented May 23, 2023 • edited Loading

roncapat commented May 23, 2023

berndpfrommer Oct 27, 2021 •

edited

Loading

emersonknapp Oct 28, 2021 •

edited

Loading

emersonknapp left a comment •

edited

Loading

berndpfrommer commented May 23, 2023 •

edited

Loading