-
Notifications
You must be signed in to change notification settings - Fork 24
Keyboard joy #652
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
base: main
Are you sure you want to change the base?
Keyboard joy #652
Conversation
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.
Pull request overview
This PR introduces keyboard_joy, a new ROS 2 Python package that enables keyboard-based control by publishing sensor_msgs/Joy messages. The package serves as a joystick replacement for development and testing purposes, with configurable key mappings and support for both hold and sticky axis modes. Note that the current implementation is limited to Xorg environments.
Key changes:
- New keyboard_joy package with configurable YAML-based key mappings
- Support for both axis control (hold/sticky modes) and button inputs via keyboard
- Comprehensive test suite using pytest with mocked keyboard listener
Reviewed changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| mission/keyboard_joy/keyboard_joy/keyboard_joy_node.py | Main node implementation with keyboard event handling, Joy message publishing, and axis/button state management |
| mission/keyboard_joy/config/key_mappings.yaml | Default key mapping configuration defining axis and button bindings |
| mission/keyboard_joy/launch/keyboard_joy_node.launch.py | Launch file for starting the keyboard_joy node with configurable parameters |
| mission/keyboard_joy/test/test_keyboard_joy_node.py | Unit tests covering key mapping, axis modes, and button state transitions |
| mission/keyboard_joy/setup.py | Python package setup configuration with dependencies and entry points |
| mission/keyboard_joy/setup.cfg | Installation script configuration for ROS 2 |
| mission/keyboard_joy/package.xml | ROS 2 package manifest with dependencies |
| mission/keyboard_joy/README.md | Package documentation describing functionality and known limitations |
| mission/keyboard_joy/keyboard_joy/init.py | Empty package initialization file |
| mission/keyboard_joy/resource/keyboard_joy | Empty resource marker file for ament |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
0588283 to
8893e9c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #652 +/- ##
==========================================
+ Coverage 35.62% 36.90% +1.28%
==========================================
Files 38 36 -2
Lines 2254 2173 -81
Branches 686 702 +16
==========================================
- Hits 803 802 -1
+ Misses 1272 1188 -84
- Partials 179 183 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
bro why bootleg chatgpt talking |
Q3rkses
left a comment
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.
you are looking good cooking
| start_message = r""" | ||
| ██ ▄█▀▓█████▓██ ██▓ ▄▄▄▄ ▒█████ ▄▄▄ ██▀███ ▓█████▄ ▄▄▄██▀▀▀▒█████ ▓██ ██▓ | ||
| ██▄█▒ ▓█ ▀ ▒██ ██▒▓█████▄ ▒██▒ ██▒▒████▄ ▓██ ▒ ██▒▒██▀ ██▌ ▒██ ▒██▒ ██▒▒██ ██▒ | ||
| ▓███▄░ ▒███ ▒██ ██░▒██▒ ▄██▒██░ ██▒▒██ ▀█▄ ▓██ ░▄█ ▒░██ █▌ ░██ ▒██░ ██▒ ▒██ ██░ | ||
| ▓██ █▄ ▒▓█ ▄ ░ ▐██▓░▒██░█▀ ▒██ ██░░██▄▄▄▄██ ▒██▀▀█▄ ░▓█▄ ▌▓██▄██▓ ▒██ ██░ ░ ▐██▓░ | ||
| ▒██▒ █▄░▒████▒ ░ ██▒▓░░▓█ ▀█▓░ ████▓▒░ ▓█ ▓██▒░██▓ ▒██▒░▒████▓ ▓███▒ ░ ████▓▒░ ░ ██▒▓░ | ||
| ▒ ▒▒ ▓▒░░ ▒░ ░ ██▒▒▒ ░▒▓███▀▒░ ▒░▒░▒░ ▒▒ ▓▒█░░ ▒▓ ░▒▓░ ▒▒▓ ▒ ▒▓▒▒░ ░ ▒░▒░▒░ ██▒▒▒ | ||
| ░ ░▒ ▒░ ░ ░ ░▓██ ░▒░ ▒░▒ ░ ░ ▒ ▒░ ▒ ▒▒ ░ ░▒ ░ ▒░ ░ ▒ ▒ ▒ ░▒░ ░ ▒ ▒░ ▓██ ░▒░ | ||
| ░ ░░ ░ ░ ▒ ▒ ░░ ░ ░ ░ ░ ░ ▒ ░ ▒ ░░ ░ ░ ░ ░ ░ ░ ░ ░ ░ ░ ▒ ▒ ▒ ░░ | ||
| ░ ░ ░ ░░ ░ ░ ░ ░ ░ ░ ░ ░ ░ ░ ░ ░ ░ ░ | ||
| ░ ░ ░ ░ ░ ░ | ||
| """ |
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.
yes
Andeshog
left a comment
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.
Consider separating the ros layer (publishing) and the logic layer. This would also make the code easier to test, as you would not need to go through ros. Personally I find nested dataclasses better than dictionaries here (easier to read, and safer to use). It would also pretty much mirror the yaml file. Also consider Enum class for "mode" instead of string
| parameters: | ||
| axis_increment_rate: 0.02 # update interval (higher = slower response) |
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.
The name says rate but the value is a period, as the bot mentioned. Choose one (50 Hz or 0.02 s). Also publish rate/period should probably be a param too
| axes: | ||
| # Left stick (surge/sway) | ||
| w: [1, 1.0, 'hold'] | ||
| s: [1, -1.0, 'hold'] | ||
| a: [0, 1.0, 'hold'] | ||
| d: [0, -1.0, 'hold'] | ||
|
|
||
| # Heave | ||
| Key.space: [2, 1.0, 'hold'] # Up (RT) | ||
| Key.shift: [2, -1.0, 'hold'] # Down (LT) | ||
|
|
||
| # Rotation (pitch/yaw) | ||
| Key.up: [4, 1.0, 'hold'] |
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.
Would be nice to have a comment or something explaining what the three values in the list represent.
| self.joy_msg = Joy() | ||
| self.joy_msg.axes = [0.0] * (max_axis_index + 1) | ||
| self.joy_msg.buttons = [0] * (max_button_index + 1) |
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.
Just an idea, but would it make sense to utilize the frame id of the joy message, e.g. "Joystick" and "Keyboard"? Wouldnt make a difference unless handled by the subscriber, but maybe nice for logs/bags?
|
|
||
| self.declare_parameter('config', '') |
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.
prefer
self.declare_parameter('config', Parameter.Type.STRING)|
|
||
| self.joy_pub = self.create_publisher(Joy, 'joy', 10) |
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.
topic name parameter in orca.yaml?
| config_file = self.get_parameter( | ||
| 'config' | ||
| ).get_parameter_value().string_value or os.path.join( | ||
| get_package_share_directory('keyboard_joy'), 'config', 'key_mappings.yaml' | ||
| ) |
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.
The default case here seems redundant if the node is launched anyway?
| max_axis_index = max((v[0] for v in self.axis_mappings.values()), default=-1) | ||
| max_button_index = max(self.button_mappings.values(), default=-1) | ||
|
|
||
| self.joy_msg = Joy() | ||
| self.joy_msg.axes = [0.0] * (max_axis_index + 1) | ||
| self.joy_msg.buttons = [0] * (max_button_index + 1) |
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.
Not super clear what is going on here (especially the +1), would be nice with a comment or abstraction. Ties back to the comment in config file perhaps
This PR introduces
keyboard_joy, a python package that publishessensor_msgs/Joymessages based on keyboard input.TODO: the current implementation uses
pynputand only works on xorg (not Wayland).