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.

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

Add --start-paused option to ros2 bag play #904

Merged
merged 5 commits into from
Nov 8, 2021

Conversation

ivanpauno
Copy link
Member

I think this was proposed in #443.

That PR ended up being closed in favor of #696, but an option to start the player in a paused state hasn't been added AFAIS.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno added the enhancement New feature or request label Nov 4, 2021
@ivanpauno ivanpauno requested a review from a team as a code owner November 4, 2021 18:47
@ivanpauno ivanpauno self-assigned this Nov 4, 2021
@ivanpauno ivanpauno requested review from Karsten1987, jhdcs and jacobperron and removed request for a team November 4, 2021 18:47
Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with green CI, no concerns. Thanks for the upgrade!

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Member Author

This cppcheck error looks like a false positive:

Error: [rosbag2_cpp/src/rosbag2_cpp/clocks/time_controller_clock.cpp:62]: (error: selfInitialization) Member variable 'paused' is initialized by itself.

I've suppressed it in 32caf26, if another approach is prefered (e.g. renaming the argument), let me know and I will address that.

@emersonknapp
Copy link
Collaborator

Another option would be to name the ctor parameter bool start_paused, which should then not trigger the selfinitialization issue, right? Not going to block on that, though

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Copy link
Contributor

@jhdcs jhdcs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me code-wise. Only two (minor) comments on help text and variable names, but I'm not sure if we want to hold the PR back because of it.

Basically, I'm just trying to think about potential confusion points for future users and maintainers.

ros2bag/ros2bag/verb/play.py Outdated Show resolved Hide resolved
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Member Author

CI config:

  • Build: --packages-up-to rosbag2_tests ros2bag rosbag2_py rosbag2_transport
  • Test: --packages-select rosbag2_tests ros2bag rosbag2_py rosbag2_transport

Jobs:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno
Copy link
Member Author

CI is passing, @jhdcs @emersonknapp let me know if this can be merged or if something else is missing.

@emersonknapp emersonknapp merged commit 565039f into master Nov 8, 2021
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/rosbag2_start_paused_feature branch November 8, 2021 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants