Add pause/resume options to the bag recorder

[ivanpauno]

Implements half of #558: ability to pause/resume recording.

[MichaelOrlov]

@ivanpauno Thanks for your contribution.
I think that paused_ defined as atomic<int> is overkill and I don't see a real use case when it could be justified to use. I would propose to change it to the std::atomic<bool> or std::atomic_bool.

I would hide toggle_paused() API as private method unless you need it outside. e.g. in Rqtbag. It's sort of ambiguous and difficult to determine an outcome from it and would be preferable to use pause resume explicitly outside of the Rosbag2.

I am also worry about recording termination by CRL+C keyboard binding. As far as I understand, before start using KeyboardHandler in recording SIGINT = CTRL+C was handling somewhere inside Node class and then re-translating to the SIGTERM and we were catching and processing SIGTERM in _transport.cpp.
However KeyboardHandler override SIGINT handler on construction and it looks like now rclcpp::shutdown(); will not be called and we will abnormally exit from process without properly finishing recording.

[ivanpauno]

I am also worry about recording termination by CRL+C keyboard binding. As far as I understand, before start using KeyboardHandler in recording SIGINT = CTRL+C was handling somewhere inside Node class and then re-translating to the SIGTERM and we were catching and processing SIGTERM in _transport.cpp.
However KeyboardHandler override SIGINT handler on construction and it looks like now rclcpp::shutdown(); will not be called and we will abnormally exit from process without properly finishing recording.

Interesting, I didn't thought about signal handling.
I will take another look to that.

[ivanpauno]

I would hide toggle_paused() API as private method unless you need it outside. e.g. in Rqtbag. It's sort of ambiguous and difficult to determine an outcome from it and would be preferable to use pause resume explicitly outside of the Rosbag2.

I think it makes sense to offer the same API that the Player class uses.

[ivanpauno]

I am also worry about recording termination by CRL+C keyboard binding. As far as I understand, before start using KeyboardHandler in recording SIGINT = CTRL+C was handling somewhere inside Node class and then re-translating to the SIGTERM and we were catching and processing SIGTERM in _transport.cpp.

SIGINT isn't being retranslated to sigterm, the default rclcpp sigint signal handler is calling shutdown() asynchronously.

However KeyboardHandler override SIGINT handler on construction and it looks like now rclcpp::shutdown(); will not be called and we will abnormally exit from process without properly finishing recording.

Yes, this is an issue.
IMO KeyboardHandler should have an option to disable installing the signal handler, or it should be using signal chaining by default.
I don't like signal chaining though.

I will open a PR in the keyboard_handler repo and link it here.

[MichaelOrlov]

Yes, this is an issue. IMO KeyboardHandler should have an option to disable installing the signal handler, or it should be using signal chaining by default. I don't like signal chaining though.

I will open a PR in the keyboard_handler repo and link it here.

I think chaining signal handler would be a valid workaround. I can help with PR for KeyboardHandler

[MichaelOrlov]

@ivanpauno Wouldn't you mind to add at least one unit test to the https://github.com/ros2/rosbag2/blob/master/rosbag2_tests/test/rosbag2_tests/test_rosbag2_record_end_to_end.cpp
to cover new --start-paused command line option?
Ideally it would be nice to have one more test for pause/resume via simulating keyboard press. Similar to how it was done for the Player class. I hope it would be possible to create some mock writer to check in test if writer_->write(..) has been called.

[MichaelOrlov]

@ivanpauno If you wouldn't mind I would suggest that I will take over issue relating to the signal handler in KeyboardHandler ros-tooling/keyboard_handler#9 and you will focus on unit tests for the pause/resume options in bag recorder.
What do you think?

[ivanpauno]

@ivanpauno If you wouldn't mind I would suggest that I will take over issue relating to the signal handler in KeyboardHandler ros-tooling/keyboard_handler#9 and you will focus on unit tests for the pause/resume options in bag recorder.
What do you think?

That sounds fine to me!
I've left a comment in the issue.

[ivanpauno]

Ideally it would be nice to have one more test for pause/resume via simulating keyboard press. Similar to how it was done for the Player class. I hope it would be possible to create some mock writer to check in test if writer_->write(..) has been called.

Test added in b6c35a6.

[emersonknapp]

API looks good to me - I like a match with the Player. Sounds like you have the shutdown issue under control

[MichaelOrlov]

@emersonknapp @ivanpauno I think we shouldn't merge this PR untill ros-tooling/keyboard_handler#10 will be merged, otherwise recording will be broken.

[ivanpauno]

@emersonknapp @ivanpauno I think we shouldn't merge this PR untill ros-tooling/keyboard_handler#10 will be merged, otherwise recording will be broken.

Agreed, I will also have to update this PR to use the new signal handling options there.

[MichaelOrlov]

Agreed, I will also have to update this PR to use the new signal handling options there.

You can, but you don't have to.
The correct CTRL+C handling will be correctly processed by default via calling original signal handler from rclcpp context after calling KeyboardHandler signal handler.

[ivanpauno]

The correct CTRL+C handling will be correctly processed by default via calling original signal handler from rclcpp context after calling KeyboardHandler signal handler.

That's right, I forgot about that.
It seems that we only need to wait until that PR gets merged and then rerun CI here.

[MichaelOrlov]

@ivanpauno I see you have added unit test to cover keyboard press handling for pause/resume during recording.
However I don't see coverage for the new --start-paused command line option.
Could you please add one more test to the
https://github.com/ros2/rosbag2/blob/master/rosbag2_tests/test/rosbag2_tests/test_rosbag2_record_end_to_end.cpp

[ivanpauno]

Could you please add one more test to the
https://github.com/ros2/rosbag2/blob/master/rosbag2_tests/test/rosbag2_tests/test_rosbag2_record_end_to_end.cpp

I'm having issues when running those tests locally, they always timeout even when using the master branch.
Do you have any idea what could be happening?

Example output

[==========] Running 16 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 16 tests from RecordFixture
[ RUN ] RecordFixture.record_end_to_end_test_with_zstd_file_compression
/home/ivanpauno/ros2_ws/src/ros2/rosbag2/rosbag2_tests/test/rosbag2_tests/test_rosbag2_record_end_to_end.cpp:86: Failure
Value of: pub_manager.wait_for_matched(topic_name)
Actual: false
Expected: true
Expected find rosbag subscription

I have added a ros2 bag record --start-paused test in b11614f.
It's at least building locally, but IDK if it's passing.

[ivanpauno]

Rpr checker failure seems unrelated.

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[emersonknapp]

The end_to_end tests are marked xfail and therefore nothing is enforcing that they keep passing.

However - they are passing locally for me when built against master branch - I'm not sure what the issue is from the information you've provided.

Are you building all of ros2 up to rosbag2, or are you building against something like Rolling?

[ivanpauno]

Are you building all of ros2 up to rosbag2

Yes, I'm doing that.
I have pulled and rebuilt everything. I might have a random issue in my workspace, I will try a clean rebuild.

The end_to_end tests are marked xfail and therefore nothing is enforcing that they keep passing.

I have checked the output of the Test rosbag2 / build_and_test (pull_request) check in the Run xfail tests section, and the test I added is passing.

output

1: [ RUN      ] RecordFixture.record_end_to_end_test_start_paused
1: stdin is not a terminal device. Keyboard handling disabled.[INFO] [1637682700.124453840] [rosbag2_recorder]: Press SPACE for pausing/resuming
1: [INFO] [1637682700.127076721] [rosbag2_storage]: Opened database '/tmp/tmp_test_dir_ZUrO9M/record_end_to_end_test_start_paused/record_end_to_end_test_start_paused_0.db3' for READ_WRITE.
1: [INFO] [1637682700.127164920] [rosbag2_recorder]: Listening for topics...
1: [INFO] [1637682700.131385888] [rosbag2_recorder]: Subscribed to topic '/test_topic'
1: [INFO] [1637682700.133184375] [rosbag2_recorder]: All requested topics are subscribed. Stopping discovery...
1: [INFO] [1637682700.687889545] [rclcpp]: signal_handler(signum=2)
1: [INFO] [1637682700.913097269] [rosbag2_storage]: Opened database '/tmp/tmp_test_dir_ZUrO9M/record_end_to_end_test_start_paused/record_end_to_end_test_start_paused_0.db3' for READ_ONLY.
1: [       OK ] RecordFixture.record_end_to_end_test_start_paused (1262 ms)

[emersonknapp]

I'm inclined to move forward, given that everything is green.

[ivanpauno]

I'm inclined to move forward, given that everything is green.

Agreed, I think it's ready to go

requests addressed