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 CLI verb for burst mode of playback #980

Merged
merged 5 commits into from
Jun 3, 2022
Merged

Conversation

gbiggs
Copy link
Member

@gbiggs gbiggs commented Mar 29, 2022

This PR adds a new verb to the ros2bag CLI command, burst. The new verb runs the burst mode of playback that was recently added to rosbag2_transport's Player object.

A minor change is made to the burst(num_messages) API on the Player object: Specifying zero for the num_messages argument now means to burst the entire bag, rather than burst zero messages.

Something that is not working correctly and that I could use ideas for is how to make the player shut down when the burst is finished. Currently due to the way burst() is implemented, the player pauses after the burst and just sits there. Should we add a stop() method to the Player object?

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
@gbiggs gbiggs added the enhancement New feature or request label Mar 29, 2022
@gbiggs gbiggs requested a review from a team as a code owner March 29, 2022 02:29
@gbiggs gbiggs self-assigned this Mar 29, 2022
@gbiggs gbiggs requested review from emersonknapp and jhdcs and removed request for a team March 29, 2022 02:29
ros2bag/setup.py Outdated
@@ -42,6 +42,7 @@
'info = ros2bag.verb.info:InfoVerb',
'list = ros2bag.verb.list:ListVerb',
'play = ros2bag.verb.play:PlayVerb',
'burst = ros2bag.verb.burst:BurstVerb',
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe alphabetize?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in c1f02a6.

"""Burst data from a bag."""

def add_arguments(self, parser, cli_name): # noqa: D102
reader_choices = get_registered_readers()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Non-blocking - mildly wondering if this could be refactored out into common code, as a lot of it looks like copy-paste from play.py

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that it could, but I think I'd prefer to do that in a separate PR.

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
@gbiggs
Copy link
Member Author

gbiggs commented May 25, 2022

CI:

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

@MichaelOrlov
Copy link
Contributor

@gbiggs Build fails on code style linter.
Could you please fix it and we will merge it.

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
@gbiggs
Copy link
Member Author

gbiggs commented Jun 2, 2022

I've corrected the linter errors.

@MichaelOrlov
Copy link
Contributor

MichaelOrlov commented Jun 2, 2022

Running CI one more time, after fixing linter errors
CI launcher job: https://ci.ros2.org/job/ci_launcher/10357
CI_BUILD_ARGS: --packages-up-to ros2bag rosbag2_transport
CI_TEST_ARGS: --packages-select ros2bag rosbag2_transport

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

@MichaelOrlov
Copy link
Contributor

@gbiggs Sorry, now this branch has conflicts with master and can't be merged.
Could you please rebase on top of latest master?

@MichaelOrlov
Copy link
Contributor

MichaelOrlov commented Jun 3, 2022

Running CI after rebase and resolving merge conflicts:
CI launcher job: https://ci.ros2.org/job/ci_launcher/10362
CI_BUILD_ARGS: --packages-up-to ros2bag rosbag2_transport
CI_TEST_ARGS: --packages-select ros2bag rosbag2_transport

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

@MichaelOrlov
Copy link
Contributor

CI failure in test_play_services__rmw_fastrtps_cpp is well known issue and unrelated to the changes in this PR.

@MichaelOrlov MichaelOrlov merged commit 4122aea into master Jun 3, 2022
@delete-merged-branch delete-merged-branch bot deleted the gbiggs/burst_mode_cli branch June 3, 2022 05:20
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