Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the Gello codebase to introduce a device abstraction layer for teleop controllers. The refactor extracts robot control logic from the main OGRobotServer class into modular device classes, enabling support for multiple teleop interfaces.
- Introduces a device abstraction pattern with BaseDevice and specific implementations (JoyLo and R1ProT)
- Moves robot control logic from OGRobotServer into device-specific classes
- Updates function signatures to use cleaner parameter names (base_cmd instead of joint_cmd)
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| joylo/gello/robots/sim_robot/og_teleop_utils.py | Updates parameter names from joint_cmd to base_cmd for improved clarity |
| joylo/gello/robots/sim_robot/og_sim.py | Major refactor to use device abstraction and move control logic to device classes |
| joylo/gello/devices/r1prot.py | New R1ProT device implementation with ROS2 integration for robot control |
| joylo/gello/devices/joylo.py | New JoyLo device implementation maintaining original ZMQ-based control logic |
| joylo/gello/devices/device_base.py | Base device class providing common functionality for all teleop devices |
| joylo/gello/devices/init.py | Device library registration for available teleop controllers |
| joylo/experiments/run_r1prot.py | New experiment script for running R1ProT teleop configurations |
| joylo/experiments/launch_nodes.py | Updates to support teleop parameter in node launching |
| joylo/configs/joycon_calibration_c8-48-05-86-17-0c.yaml | New JoyCon calibration configuration |
| joylo/configs/joycon_calibration_48-31-77-90-f4-55.yaml | Updated JoyCon calibration values |
| joylo/configs/joint_config_jellyfish_pro.yaml | New joint configuration for jellyfish pro variant |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| action[self.robot.trunk_action_idx] = interpolated_trunk_pos | ||
|
|
||
| def get_base_cmd(self): |
There was a problem hiding this comment.
The get_base_cmd method returns self.base_cmd[:3] but self.base_cmd is a list that includes a timestamp as the 4th element. When self.base_cmd is None, this will raise an AttributeError. The method should handle the None case.
| def get_base_cmd(self): | |
| def get_base_cmd(self): | |
| if self.base_cmd is None: | |
| return [0.0, 0.0, 0.0] |
| theta3 = (theta1 + theta2 - self._current_trunk_tilt) | ||
| theta4 = 0.0 | ||
|
|
||
| action[self.robot.trunk_action_idx] = th.tensor([theta1, theta2, theta3, theta4], dtype=th.float) |
There was a problem hiding this comment.
Missing return statement in the get_action method. The method should return the action tensor after all modifications are complete.
| # self._current_trunk_translate = np.clip(self._current_trunk_translate + self._joint_cmd["trunk"][0].item() * og.sim.get_sim_step_dt(), 0.25, 0.65) | ||
| # self._current_trunk_tilt = np.clip(self._current_trunk_tilt + self._joint_cmd["trunk"][1].item() * og.sim.get_sim_step_dt(), -np.pi / 2, np.pi / 2) | ||
| self._current_trunk_translate = np.clip(self._current_trunk_translate + torso_control_cmd[2] * og.sim.get_sim_step_dt(), 0.25, 0.65) | ||
| self._current_trunk_tilt = np.clip(self._current_trunk_tilt + torso_control_cmd[4] * og.sim.get_sim_step_dt(), -np.pi / 2, np.pi / 2) | ||
| # print(f" _current_trunk_translate { self._current_trunk_translate}") | ||
| # print(f" _current_trunk_tilt { self._current_trunk_tilt}") | ||
| # print(f" control trunk { torso_control_cmd}") |
There was a problem hiding this comment.
Multiple commented-out code lines should be removed to improve code cleanliness and maintainability.
joylo/gello/devices/r1prot.py
Outdated
| if(self.current_left_arm_pos is None or self.current_left_arm_pos.dim()!= 1 or self.current_left_arm_pos.numel() != 7 | ||
| or self.current_right_arm_pos is None or self.current_right_arm_pos.dim()!= 1 or self.current_right_arm_pos.numel() != 7): |
There was a problem hiding this comment.
The condition check has inconsistent spacing around operators. Should be 'dim() != 1' instead of 'dim()!= 1' for consistency with Python style guidelines.
| if(self.current_left_arm_pos is None or self.current_left_arm_pos.dim()!= 1 or self.current_left_arm_pos.numel() != 7 | |
| or self.current_right_arm_pos is None or self.current_right_arm_pos.dim()!= 1 or self.current_right_arm_pos.numel() != 7): | |
| if(self.current_left_arm_pos is None or self.current_left_arm_pos.dim() != 1 or self.current_left_arm_pos.numel() != 7 | |
| or self.current_right_arm_pos is None or self.current_right_arm_pos.dim() != 1 or self.current_right_arm_pos.numel() != 7): |
joylo/gello/devices/r1prot.py
Outdated
| print(f"cmd_time: {cmd_time:.9f} s") | ||
| print(f" ✅ base cmd deltaT: {(current_time - cmd_time):.9f}") |
There was a problem hiding this comment.
Debug print statements should be removed or converted to proper logging to avoid cluttering production output.
wensi-ai
changed the title
1 participant
No description provided.