Skip to content

fix HIL-SERL keyboard teleop #2345

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+7 −4
Open

fix HIL-SERL keyboard teleop #2345

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions src/lerobot/teleoperators/keyboard/teleop_keyboard.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,11 +103,14 @@ def calibrate(self) -> None:

def _on_press(self, key):
if hasattr(key, "char"):
self.event_queue.put((key.char, True))
key = key.char
self.event_queue.put((key, True))
Comment on lines 104 to +107
Copy link

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

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

The change now allows special keys (like arrow keys, shift, ctrl) to be added to event_queue as keyboard.Key objects when they don't have a 'char' attribute. However, in get_action() (line 196), the code directly compares keys with keyboard.Key objects (e.g., key == keyboard.Key.up), which will work. But in the base KeyboardTeleop.get_action() (line 137), the code creates a set from current_pressed items without type awareness. This inconsistency could lead to mixed key types in current_pressed dictionary, which may cause unexpected behavior when comparing or processing keys.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@jpizarrom jpizarrom Oct 30, 2025

Choose a reason for hiding this comment

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

in linux spacial key do not have char, but don't know if this change affect other part of LeRobot


def _on_release(self, key):
if hasattr(key, "char"):
self.event_queue.put((key.char, False))
key = key.char
self.event_queue.put((key, False))

if key == keyboard.Key.esc:
Copy link

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

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

After the change at lines 110-111, the key variable is reassigned to key.char if it has a 'char' attribute. This will cause the ESC key comparison to fail because keyboard.Key.esc does not have a 'char' attribute, so key will still be keyboard.Key.esc at this check. However, this means the comparison will work correctly. But the logic is inconsistent with lines 104-107 in _on_press. Consider checking the original key value before reassignment, or use a separate variable to avoid confusion.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@jpizarrom jpizarrom Oct 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

in linux spacial key do not have char, but don't know if this change affect other part of LeRobot

logging.info("ESC pressed, disconnecting.")
self.disconnect()
Expand Down Expand Up @@ -214,8 +217,6 @@ def get_action(self) -> dict[str, Any]:
# this is useful for retrieving other events like interventions for RL, episode success, etc.
self.misc_keys_queue.put(key)

self.current_pressed.clear()

action_dict = {
"delta_x": delta_x,
"delta_y": delta_y,
Expand Down Expand Up @@ -266,6 +267,8 @@ def get_teleop_events(self) -> dict[str, Any]:
]
is_intervention = any(self.current_pressed.get(key, False) for key in movement_keys)

self.current_pressed.clear()

# Check for episode control commands from misc_keys_queue
terminate_episode = False
success = False
Expand Down