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 rosbag2_interfaces package with playback service definitions #728

Merged
merged 2 commits into from
Apr 12, 2021

Conversation

emersonknapp
Copy link
Collaborator

@emersonknapp emersonknapp commented Apr 9, 2021

These .srv definitions will be used to control playback on the Player. Recording controls not yet defined, so none added.

Part of #696
Design at https://github.com/ros2/rosbag2/blob/master/docs/design/rosbag2_playback_time.md

Copy link
Collaborator

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

I'd vote to rename the project to rosbag2_srvs.
I don't mind too much about the actual services, but some of them can be replaced by std_srvs.

rosbag2_interfaces/CMakeLists.txt Show resolved Hide resolved
rosbag2_interfaces/srv/IsPaused.srv Show resolved Hide resolved
rosbag2_interfaces/srv/Pause.srv Show resolved Hide resolved
@Karsten1987
Copy link
Collaborator

it's all good. you can go ahead and merge as is. I just wanted to bring it up.

@emersonknapp
Copy link
Collaborator Author

it's all good. you can go ahead and merge as is. I just wanted to bring it up.

I'm just presenting where my thinking came from - I don't have strong opinions here, I am definitely open to making changes if there are good counterpoints.

For the package naming - I was thinking that potentially Play, Record, or Convert/Rewrite could be controlled by an Action with progress feedback - I don't have any Messages in mind right now though.

Copy link
Collaborator

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

I agree with your reasoning here. Thanks for the explanations.

Copy link

@david-prody david-prody 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

Emerson Knapp added 2 commits April 12, 2021 13:08
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@emersonknapp emersonknapp force-pushed the emersonknapp/rosbag2-interfaces branch from 7e9ce9b to 3fc07ba Compare April 12, 2021 20:08
@emersonknapp
Copy link
Collaborator Author

Gist: https://gist.githubusercontent.com/emersonknapp/20e6f6611bb120321b7294df2f6974bb/raw/9560335eef310dd4ebe284927edc903bb23f43a2/ros2.repos
BUILD args: --packages-up-to rosbag2 rosbag2_interfaces
TEST args: --packages-select rosbag2 rosbag2_interfaces
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/8207

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

@emersonknapp emersonknapp merged commit 14afcf6 into master Apr 12, 2021
@delete-merged-branch delete-merged-branch bot deleted the emersonknapp/rosbag2-interfaces branch April 12, 2021 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants