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.

Sign up for GitHub

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

Jump to bottom

keyboard controls for pause/resume toggle and play-next: #847

Merged
merged 3 commits into from
Sep 27, 2021
Merged

keyboard controls for pause/resume toggle and play-next: #847

merged 3 commits into from
Sep 27, 2021

Conversation

lihui815
Copy link
Contributor

@lihui815 lihui815 commented Aug 13, 2021 •
edited by emersonknapp
Loading

@lihui815 lihui815 requested a review from emersonknapp August 13, 2021 14:27
@emersonknapp
Copy link
Collaborator

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?

@lihui815 lihui815 force-pushed the sonia-pauseres branch 3 times, most recently from c0c0f6b to c73888f Compare August 14, 2021 04:41
@lihui815
Copy link
Contributor Author

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?

Ok. I'll take that out and add disable to the cli

@lihui815 lihui815 force-pushed the sonia-pauseres branch 2 times, most recently from 95dd236 to fee38a6 Compare August 14, 2021 05:53
MichaelOrlov
MichaelOrlov reviewed Aug 16, 2021
View reviewed changes
Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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.

rosbag2_transport/include/rosbag2_transport/play_options.hpp Outdated Show resolved Hide resolved
@emersonknapp emersonknapp mentioned this pull request Aug 24, 2021
Closed
emersonknapp
emersonknapp requested changes Sep 2, 2021
View reviewed changes
ros2bag/ros2bag/verb/play.py Outdated Show resolved Hide resolved
rosbag2_transport/CMakeLists.txt Outdated Show resolved Hide resolved
rosbag2_transport/include/rosbag2_transport/play_options.hpp Outdated Show resolved Hide resolved
rosbag2_transport/include/rosbag2_transport/player.hpp Outdated Show resolved Hide resolved
rosbag2_transport/include/rosbag2_transport/player.hpp Outdated Show resolved Hide resolved
rosbag2_transport/include/rosbag2_transport/player.hpp Outdated Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/player.cpp Outdated Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/player.cpp Outdated Show resolved Hide resolved
emersonknapp
emersonknapp approved these changes Sep 3, 2021
View reviewed changes
Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

LGTM

rosbag2_transport/include/rosbag2_transport/player.hpp Outdated Show resolved Hide resolved
MichaelOrlov
MichaelOrlov requested changes Sep 8, 2021
View reviewed changes
Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

@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 KeyboardHandlerBase to refer to the key codes and basic functionality defined in it.
    Please use KeyboardHandler alias directly instead.

rosbag2_transport/include/rosbag2_transport/play_options.hpp Outdated Show resolved Hide resolved
rosbag2_transport/include/rosbag2_transport/player.hpp Outdated Show resolved Hide resolved
rosbag2_transport/include/rosbag2_transport/player.hpp Show resolved Hide resolved
rosbag2_transport/include/rosbag2_transport/player.hpp Outdated Show resolved Hide resolved
rosbag2_transport/include/rosbag2_transport/player.hpp Outdated Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/player.cpp Outdated Show resolved Hide resolved
rosbag2_transport/include/rosbag2_transport/player.hpp Outdated Show resolved Hide resolved
rosbag2_transport/include/rosbag2_transport/player.hpp Outdated Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/player.cpp Outdated Show resolved Hide resolved
rosbag2_transport/src/rosbag2_transport/player.cpp Show resolved Hide resolved
@emersonknapp emersonknapp mentioned this pull request Sep 13, 2021
Closed
@lihui815 lihui815 force-pushed the sonia-pauseres branch 7 times, most recently from e48fac3 to fa67c87 Compare September 16, 2021 16:19
@lihui815 lihui815 changed the title [WIP] keyboard controls for pause/resume toggle and play-next: keyboard controls for pause/resume toggle and play-next: Sep 16, 2021
@lihui815 lihui815 marked this pull request as ready for review September 16, 2021 17:22
@lihui815 lihui815 requested a review from a team as a code owner September 16, 2021 17:22
@lihui815 lihui815 requested review from zmichaels11 and removed request for a team September 16, 2021 17:22
emersonknapp
emersonknapp approved these changes Sep 16, 2021
View reviewed changes
Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

Only tiny nitpicks

rosbag2_transport/src/rosbag2_transport/player.cpp Outdated Show resolved Hide resolved
rosbag2_transport/test/rosbag2_transport/mock_keyboard_handler.hpp Outdated Show resolved Hide resolved
@emersonknapp emersonknapp mentioned this pull request Sep 16, 2021
Merged
MichaelOrlov
MichaelOrlov requested changes Sep 21, 2021
View reviewed changes
Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

@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
@emersonknapp
Copy link
Collaborator

emersonknapp commented Sep 23, 2021 •
edited
Loading

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

@emersonknapp
Copy link
Collaborator

I think the KEYBOARD_HANDLER_PUBLIC delcarations caused it to be a dllimport on the Mock class. They're not needed within the test library, so removal should fix the windows build. Also addressed last review comments

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

MichaelOrlov
MichaelOrlov approved these changes Sep 24, 2021
View reviewed changes
Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

Looks good to me with green CI.

@MichaelOrlov
Copy link
Contributor

  • Windows Build Status

Simplify MockKeyboardHandler
cd1ec54 
Signed-off-by: Emerson Knapp <[email protected]>
@emersonknapp
Copy link
Collaborator

Cancelled above windows build - it was probably going to have the same warning as before. Pushed a fix commit

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

Mock keyboardhandler OS functions
979cd20 
Signed-off-by: Emerson Knapp <[email protected]>
@emersonknapp
Copy link
Collaborator

Tests were failing because test environment is not a TTY. Mocked the constructor to have fake implementations for the OS stuff.

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

@emersonknapp
Copy link
Collaborator

NOTE: still can't merge this until ros2/ros2#1184 is merged

@emersonknapp emersonknapp merged commit 6173fe1 into ros2:master Sep 27, 2021
@lihui815 lihui815 deleted the sonia-pauseres branch September 27, 2021 22:54
@lihui815 lihui815 restored the sonia-pauseres branch September 27, 2021 22:54
@lihui815 lihui815 deleted the sonia-pauseres branch September 29, 2021 20:39
@emersonknapp emersonknapp mentioned this pull request Mar 28, 2022
Closed
@clalancette clalancette mentioned this pull request Apr 29, 2022
Closed
87 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichaelOrlov MichaelOrlov MichaelOrlov approved these changes

@emersonknapp emersonknapp emersonknapp approved these changes

@zmichaels11 zmichaels11 Awaiting requested review from zmichaels11

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Pause/resume ros2 bag play with keyboard controls
3 participants
@lihui815 @emersonknapp @MichaelOrlov