-
Notifications
You must be signed in to change notification settings - Fork 240
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
Expose pause/resume related services on the Player #729
Conversation
a1d1292
to
07d8f25
Compare
I'm opening this up for review because this is the implementation - but we will definitely not merge without tests. I am manually testing with this simple Python app for keyboard control and it's working great! (need to
|
6a55111
to
0764314
Compare
@Karsten1987 this is ready to go, could you give it a look when you have a chance? |
There was a problem hiding this 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.
@@ -157,6 +194,39 @@ void Player::play() | |||
} | |||
} | |||
|
|||
void Player::pause() | |||
{ | |||
if (clock_) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can clock_
ever be nullptr
? On that note, where is clock_
currently initialized?
same below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clock is currently intitialized in prepare_clock
, which call from play
. I would like to initialize it in the constructor (and therefore avoid having it be on the heap) - but we don't have access to the "initial time" until the reader is opened, which only happens when we call play
.
It may make sense, to me, to open the reader in the constructor, now that we have all arguments present at that point. What do you think about that approach? We could start the clock paused, then play
could be the same function as resume
, rather than doing the other setup.
I wouldn't want to make that change here, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am okay if you don't address it here, but I'd like to have it noted in an GH issue or an TODO to keep track of it. I think we should - now with its public header - review the overall API and offload some of the initialization work from play
to the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with all of that
rosbag2_transport/test/rosbag2_transport/rosbag2_play_test_fixture.hpp
Outdated
Show resolved
Hide resolved
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
0764314
to
db3da04
Compare
Gist: https://gist.githubusercontent.com/emersonknapp/bd7f6cd9db15a825e6522b7e5f9efad4/raw/2cdbc90e9cf69cd9c850cfca808846fce74cc5bf/ros2.repos |
and add TODO for clock setup in the constructor Signed-off-by: Emerson Knapp <eknapp@amazon.com>
3df3c96
to
31d6718
Compare
…ixed Signed-off-by: Emerson Knapp <eknapp@amazon.com>
380f1b3
to
be39616
Compare
…on variable Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Depends on #728
Depends on #704
Exposes "Pause", "Resume", "TogglePaused", and "IsPaused" services for the Player.