Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new ROS 2 servo controller backend that drives up to 4 servos over I2C via a “PDB” device, and wires it into the servo_pkg package so it can be run as a console script.
Changes:
- Reformats distortion coefficient serialization in the mrcal→ROS/OpenCV YAML conversion script.
- Registers a new
pdb_controllerconsole entry point inservo_pkg. - Introduces
pdb_controller.pyimplementing an I2C-based servo controller (SMBus) and addssmbus2to Python requirements.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Nav/aruco_localizer/scripts/convert_mrcal_to_opencv_ros.py | Small formatting/parenthesization change for YAML “data” field construction. |
| src/HW-Devices/servo_pkg/setup.py | Adds pdb_controller executable entry point for ROS 2 ros2 run. |
| src/HW-Devices/servo_pkg/servo_pkg/pdb_controller.py | New I2C/SMBus-based servo controller node subscribing to per-servo Float32 topics and writing PWM values to hardware registers. |
| requirements.txt | Adds smbus2 dependency (and fixes formatting of odrive line). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "console_scripts": [ | ||
| "USB_Servo = servo_pkg.USB_Servo:main", | ||
| "servo_client = servo_pkg.servo_client:main", | ||
| "i2c_Servo = servo_pkg.i2c_Servo:main", | ||
| "pi_Servo = servo_pkg.pi_Servo:main", | ||
| "pdb_controller = servo_pkg.pdb_controller:main", | ||
| ], |
There was a problem hiding this comment.
A new pdb_controller executable is being added, but there is no corresponding launch file under launch/ (unlike the existing usb_servo_launch, i2c_servo_launch, and pi_servo_launch). Adding a launch file (and any needed YAML config) will make this controller discoverable and runnable in the same way as the other servo backends.
| def convert_from_radians(angle, servo_info): | ||
| total_range = servo_info.max - servo_info.min | ||
| return int(servo_info.min + (total_range * angle / servo_info.rom)) | ||
|
|
||
|
|
||
| def convert_to_radians(value, servo_info): | ||
| total_range = servo_info.max - servo_info.min | ||
| return servo_info.rom * (value - servo_info.min) / total_range | ||
|
|
There was a problem hiding this comment.
The radians<->PWM conversion helpers are duplicated from servo_pkg/USB_Servo.py (same formulas). Consider moving these conversions into a shared utility module (e.g., servo_pkg/conversions.py) and importing from both controllers to avoid future drift if the mapping ever changes.
|
|
||
| if not (servo_info.min <= target_value_us <= servo_info.max): | ||
| self.get_logger().warning( | ||
| f"Servo {port} input out of range. " f"Last angle: {current_position}" |
There was a problem hiding this comment.
The out-of-range warning logs Last angle: {current_position} rather than the requested angle (msg.data) that was actually out of range. This makes the warning misleading when debugging; include the requested angle (and optionally the valid range) in the message.
| f"Servo {port} input out of range. " f"Last angle: {current_position}" | |
| f"Servo {port} input out of range. " | |
| f"Requested angle: {msg.data} rad, " | |
| f"valid PWM range: [{servo_info.min}, {servo_info.max}] us, " | |
| f"last angle: {current_position}" |
| rclpy.spin(node) | ||
| node.destroy_node() | ||
| rclpy.shutdown() |
There was a problem hiding this comment.
main() spins the node without a try/finally, so a KeyboardInterrupt or exception can skip destroy_node() and leave the I2C bus handle open. Wrap rclpy.spin(node) in try: ... finally: to always close the SMBus and call rclpy.shutdown().
| rclpy.spin(node) | |
| node.destroy_node() | |
| rclpy.shutdown() | |
| try: | |
| rclpy.spin(node) | |
| finally: | |
| node.destroy_node() | |
| rclpy.shutdown() |
1 participant
YOLO merging due to SAR (All nighter number 6 of 2026 go RARR). (I may have lost my mind)