keyboard controls for pause/resume toggle and play-next:#847
keyboard controls for pause/resume toggle and play-next:#847emersonknapp merged 3 commits intoros2:masterfrom
Conversation
fdbfddd to
07bf455
Compare
|
High level - I'm inclined to not have the keys be configurable via CLI options. Simplifies this code a bit - if we want to have some remapping later, maybe we do it via Node parameters for the Player Node. if anything is configurable, maybe it's enable/disable keyboard handler. Thoughts? |
c0c0f6b to
c73888f
Compare
Ok. I'll take that out and add disable to the cli |
95dd236 to
fee38a6
Compare
There was a problem hiding this comment.
I would prefer to hard code key codes for play_next and pause operations rather than providing them via player options. We have to many options and this key bindings pretty much trivial just need to agree upon on them.
Also have some concern about using time measurements in unit tests for validation.
It's used to lead to the flakiness on CI.
Also need to properly handle case when we registered callbacks and Player got destroyed but KeyboardHandler still active.
Pointer to this will become dangling pointer. Please refer to the KeybordHandler design document for reference.
fee38a6 to
918ffd3
Compare
918ffd3 to
d0c73e4
Compare
MichaelOrlov
left a comment
There was a problem hiding this comment.
@lihui815 thank you for taking actions and fixing previous comments/requests.
However I would like to request more changes.
Generic comment:
- Don't need to use base class
KeyboardHandlerBaseto refer to the key codes and basic functionality defined in it.
Please useKeyboardHandleralias directly instead.
e48fac3 to
fa67c87
Compare
emersonknapp
left a comment
There was a problem hiding this comment.
Only tiny nitpicks
rosbag2_transport/test/rosbag2_transport/mock_keyboard_handler.hpp
Outdated
Show resolved
Hide resolved
fa67c87 to
8a42ac5
Compare
MichaelOrlov
left a comment
There was a problem hiding this comment.
@lihui815 Implementation looks good to me.
However I would encourage you to make some "nitpick" changes in unit tests.
rosbag2_transport/test/rosbag2_transport/test_keyboard_controls.cpp
Outdated
Show resolved
Hide resolved
rosbag2_transport/test/rosbag2_transport/test_keyboard_controls.cpp
Outdated
Show resolved
Hide resolved
|
|
I think the
|
ac3c0c6 to
14f5ea5
Compare
Signed-off-by: Sonia Jin <diegothemuich@gmail.com>
14f5ea5 to
e1dbf82
Compare
MichaelOrlov
left a comment
There was a problem hiding this comment.
Looks good to me with green CI.
|
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
|
Cancelled above windows build - it was probably going to have the same warning as before. Pushed a fix commit
|
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
4ddc8de to
979cd20
Compare
|
Tests were failing because test environment is not a TTY. Mocked the constructor to have fake implementations for the OS stuff.
|
|
NOTE: still can't merge this until ros2/ros2#1184 is merged |
Closes #55
Depends on ros2/ros2#1184
adds keyboard controls for
ros2 bag playaction-ros-ci-repos-supplemental: https://gist.githubusercontent.com/lihui815/5edccb5d36176ccc6a779fd8a26122db/raw/5b9ce9db3628fddce2329118bbf2b6ab6a9ce2a3/kh.repos