Add pause and resume service calls for rosbag2 recorder

[rshanor]

Add service calls to pause recording and resume recording.

• Closes Pause and resume recording via service call. #1130

Signed-off-by: Rick Shanor [email protected]

[rshanor]

cc @MichaelOrlov @ivanpauno -- The command line interface you recommend already existed as of #905

[MichaelOrlov]

@rshanor Could you please add one more service for is_paused?

 srv_is_paused_ = create_service<rosbag2_interfaces::srv::IsPaused>( 
   "~/is_paused", 
   [this]( 
     rosbag2_interfaces::srv::IsPaused::Request::ConstSharedPtr, 
     rosbag2_interfaces::srv::IsPaused::Response::SharedPtr response) 
   { 
     response->paused = is_paused(); 
   }); 

Also please don't create a new class for class RecordSrvsPauseResumeTest : public RecordSrvsTest we can use RecordSrvsTest with default snapshot parameter equal to true false.
See my rework in https://github.com/ros2/rosbag2/pull/1122/files#diff-8fc22b4f6714cc8937f931b0b8afb31ca75bb9a15189816cc36cc9058b22f6dcR42

[rshanor]

@MichaelOrlov are you close to merging that PR? I saw your changes, I can hold off and pull your update in.

I will add an is_paused API. I didn't have an immediate use for it but happy to add it.

[MichaelOrlov]

@rshanor I would be happy to merge it if someone would approve it.

[rshanor]

@rshanor I would be happy to merge it if someone would approve it.

Sorry I cant help there.

[MichaelOrlov]

@rshanor I am actually thinking about adding call for bag split on handling service call for resume.
When we are doing pause resume from remote and recording everything in one bag it will be not very convenient (terrible) on replay.
It will looks like you have some data then you will have a long pause you will have to wait until next data will appear.
It will make much more sense to start new bag after resuming recording to avoid big pauses on replay.

[rshanor]

@MichaelOrlov I agree wanting to split is a common use case, I was just going to make those two calls back to back :)

Are there any cases where people really want to be using the same bagfile? Should we add a flag or just always call split?

[MichaelOrlov]

@rshanor I can't imagine a real use case when pausing with recording to the same file would be useful or preferable rather than starting a new file each time on pause/resume.

We store all files in one folder during split and they will be reflected in final metadata.yaml file.
After recording with multiple splits it will be possible to replay all files in sequence the way as it would be a one file without delays between each pause/resume by specifying folder for player.

Although I would keep C++ API pause/resume without split inside for more granular control when using them directly.

Only one problem I would envision is as corner case when we are starting recording in pause and resuming recording after sometime - it's going to be an empty first bag.

[rshanor]

@MichaelOrlov if thats OK with you, thats OK with me, I also dont see a compelling use case.

[MichaelOrlov]

@rshanor #1122 has been merged yesterday. Feel free to rebase and proceed with this PR.

[emersonknapp]

I can't imagine a real use case when pausing with recording to the same file would be useful or preferable rather than starting a new file each time on pause/resume.

Imagine you pause/resume very frequently - leading to lots and lots of tiny bagfiles. Maybe you generally just load this data into analysis tools instead of using for live playback. I think there are plenty of reasons why time gaps in a single bagfile would be totally acceptable.

I don't think it's reasonable to make splitting the only or default behavior for pause/resume. At the very least, separate that conversation / design decision out of this initial implementation PR, keep it granular.

[emersonknapp]

lgtm as is

[rshanor]

@MichaelOrlov sorry for the delay, just addressed your comments. Been a busy week. I did not implement the split call as @emersonknapp suggested. While I also agree that this is what a user wants 95% of the time, it makes sense to keep that discussion separate and I dont want to block this PR further.

[MichaelOrlov]

@rshanor Thank you! Looks good to me.

As regards to starting new bag on resume. Let's move it out of scope of this PR - I agree.

[MichaelOrlov]

Gist: https://gist.githubusercontent.com/MichaelOrlov/59ea3ed67c34c667e42a0143a4d22a1e/raw/f5b5a0aa21030aa182990f0da92bccf423f4f590/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_tests rosbag2_transport
TEST args: --packages-above rosbag2_tests rosbag2_transport
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11081

• Linux
• Linux-aarch64
• Windows

[ros-discourse]

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-2022-11-17/28457/1