Naive clock jump implementation - allows for clock reuse and simplified Player setup#754
Conversation
|
@MichaelOrlov FYI |
53ac074 to
5224a91
Compare
5224a91 to
5183692
Compare
|
Gist: https://gist.githubusercontent.com/emersonknapp/c71f7d18aab7ff09541be9b2d0f22c1b/raw/8401041718d2e42dd2704c81592b62925181b0e2/ros2.repos
|
Karsten1987
left a comment
There was a problem hiding this comment.
I am generally okay with it, just a few comments inline.
Before really approving it, I'd like to @emersonknapp's opinion of tests for this. Are we having tests covering all the new things in this PR?
rosbag2_cpp/include/rosbag2_cpp/clocks/time_controller_clock.hpp
Outdated
Show resolved
Hide resolved
…ed setup Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
… Add tests for jump Signed-off-by: Emerson Knapp <eknapp@amazon.com>
5183692 to
89fe16a
Compare
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
|
@Karsten1987 @MichaelOrlov let me know how you feel about the latest revision - it's maybe a little overloaded (doing a couple different things), but since this is a "reconfigure some things for ease of use" PR I am thinking that's probably OK |
MichaelOrlov
left a comment
There was a problem hiding this comment.
@emersonknapp New version looks good to me among a few minor comments in tests and doxygen comments.
rosbag2_cpp/include/rosbag2_cpp/clocks/time_controller_clock.hpp
Outdated
Show resolved
Hide resolved
Karsten1987
left a comment
There was a problem hiding this comment.
This looks good to me. A few nitpicks inline and I also agree with some comments of @MichaelOrlov to extend a few test cases.
I do request changes though because I don't understand why you deleted a constructor in the Player. Did I miss something here?
rosbag2_cpp/include/rosbag2_cpp/clocks/time_controller_clock.hpp
Outdated
Show resolved
Hide resolved
…n comments, jump backward test Signed-off-by: Emerson Knapp <eknapp@amazon.com>
|
Clock can be created in the constructor so it is never null, and have all its state simply maintained while looping or repeated calls to
play.This is an intentionally simple implementation that accomplishes this single goal, and is definitely not safe for use during playback. There is significantly more complexity involved in handling jumping during playback - but that is out of the scope of this change.