Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[wip] Add options to disable signal handling in Player and Recorder Python API #1685

Open
wants to merge 7 commits into
base: rolling
Choose a base branch
from
Open

[wip] Add options to disable signal handling in Player and Recorder Python API #1685

wants to merge 7 commits into from

Conversation

MichaelOrlov
Copy link
Contributor

@MichaelOrlov MichaelOrlov commented Jun 1, 2024 •
edited
Loading

  • This PR adds option enable_signal_handling to the rosbag2_py.Recorder.record(..), rosbag2_py.Player.play(..) and rosbag2_py.Player.Burst(..) Python exposed API.

  • The previous versions of API were also preserved for backward compatibility.

  • Note: When using a new API with enable_signal_handling = False the caller shall handle signals on the Python level and call Player.cancel() or Recorder.cancel() after processing signals to correctly stop player or recorder.

  • Closes [rosbag2_py.Recorder] Should add option to disable signal handling changes #1678

@MichaelOrlov MichaelOrlov mentioned this pull request Jun 1, 2024
Open
@MichaelOrlov MichaelOrlov changed the title Add options to disable signal handling in Player and Recorder Python API [wip] Add options to disable signal handling in Player and Recorder Python API Jun 1, 2024
@MichaelOrlov MichaelOrlov force-pushed the morlov/add_option_to_disable_signal_handlers branch 2 times, most recently from b47eecb to db057e4 Compare June 2, 2024 02:34
@MichaelOrlov MichaelOrlov mentioned this pull request Jun 2, 2024
Merged
@MichaelOrlov MichaelOrlov force-pushed the morlov/add_option_to_disable_signal_handlers branch from 605a3c6 to 4ec3d89 Compare June 12, 2024 07:11
@MichaelOrlov MichaelOrlov changed the title [wip] Add options to disable signal handling in Player and Recorder Python API Add options to disable signal handling in Player and Recorder Python API Jun 12, 2024
@MichaelOrlov MichaelOrlov marked this pull request as ready for review June 12, 2024 23:24
@MichaelOrlov MichaelOrlov requested a review from a team as a code owner June 12, 2024 23:24
@MichaelOrlov MichaelOrlov requested review from emersonknapp and james-rms and removed request for a team June 12, 2024 23:24
@MichaelOrlov
Copy link
Contributor Author

Moving back to the draft. I had forgotten that I wanted to try to move common signal handling functionality to a separate class to avoid code duplication.

@MichaelOrlov MichaelOrlov changed the title Add options to disable signal handling in Player and Recorder Python API [wip] Add options to disable signal handling in Player and Recorder Python API Jun 13, 2024
@MichaelOrlov
Add options to disable signal handling in Player and Recorder Python API
8b43291 
- Also added test coverage for signals handling in Python API

Signed-off-by: Michael Orlov <[email protected]>
@MichaelOrlov
Address old uncrustify style warnings
b43820e 
Signed-off-by: Michael Orlov <[email protected]>
@MichaelOrlov MichaelOrlov force-pushed the morlov/add_option_to_disable_signal_handlers branch from 4ec3d89 to cf2988a Compare August 17, 2024 18:44
@MichaelOrlov
Move common signal handling functionality to a separate class
f626764 
- Add DeferredSignalHandler class
- Note: Windows support will be added in a follow-up PR

Signed-off-by: Michael Orlov <[email protected]>
@MichaelOrlov MichaelOrlov force-pushed the morlov/add_option_to_disable_signal_handlers branch from cf2988a to bd9cb78 Compare August 17, 2024 18:51
@MichaelOrlov MichaelOrlov force-pushed the morlov/add_option_to_disable_signal_handlers branch from bd9cb78 to 6d94fda Compare August 17, 2024 18:54
@MichaelOrlov
Copy link
Contributor Author

@ros-pull-request-builder retest this please

@MichaelOrlov
Copy link
Contributor Author

Pulls: #1685
Gist: https://gist.githubusercontent.com/MichaelOrlov/fe66fb6be231886ff50808671ff10a0d/raw/24c185caa08533e6bf8bb154bc771fd3089e876d/ros2.repos
BUILD args: --packages-above-and-dependencies ros2bag rosbag2_py rosbag2_tests
TEST args: --packages-above ros2bag rosbag2_py rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14409

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@MichaelOrlov
Address Windows build failure
01d5d65 
- Add missing macros to work with signal set

Signed-off-by: Michael Orlov <[email protected]>
@MichaelOrlov
Disable tests that check signal handling on Windows
12bdf3d 
- Rationale: The signal handling on Windows hasn't been implemented yet.

Signed-off-by: Michael Orlov <[email protected]>
@MichaelOrlov
Copy link
Contributor Author

Re-run CI after attempt to fix Windows CI failures
Gist: https://gist.githubusercontent.com/MichaelOrlov/fe66fb6be231886ff50808671ff10a0d/raw/24c185caa08533e6bf8bb154bc771fd3089e876d/ros2.repos
BUILD args: --packages-above-and-dependencies ros2bag rosbag2_py rosbag2_tests
TEST args: --packages-above ros2bag rosbag2_py rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14516

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emersonknapp emersonknapp Awaiting requested review from emersonknapp emersonknapp was automatically assigned from ros2/tooling-reviewers

@james-rms james-rms Awaiting requested review from james-rms james-rms was automatically assigned from ros2/tooling-reviewers

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[rosbag2_py.Recorder] Should add option to disable signal handling changes
1 participant
@MichaelOrlov