Ligx/us/t100154 by StartAgainAgain · Pull Request #85 · enactic/openarm_ros2

StartAgainAgain · 2026-03-12T05:41:44Z

初步完成工程的完整配置包含can接口，kp，kd，关节限位等

…o oy_openarm_ros2 merge remote change for openarm_can url

Copilot

Pull request overview

This PR introduces an OY-motor-based ros2_control hardware interface for OpenArm and updates bringup/MoveIt2 configurations to default to the OY robot description and controller setups (including CAN interface parameters and tuned controller tolerances).

Changes:

Added a new OpenArm_oyHW hardware plugin (OY motors over CAN/CAN-FD) and registered it via pluginlib.
Updated launch + controller YAMLs to default to oy.urdf.xacro / OY controller configs, and adjusted controller spawning sequencing/flags.
Updated MoveIt2 bimanual config to reference the OY robot description and revised SRDF end-effector definitions; updated dependency repo URL/branch.

Reviewed changes

Copilot reviewed 18 out of 19 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
openarm_hardware/src/oy_hardware.cpp	New OY motor hardware interface implementation (state/command I/O, CAN init, gripper mapping).
openarm_hardware/include/openarm_hardware/oy_hardware.hpp	Header for the new OY hardware interface (defaults, gains, mappings).
openarm_hardware/openarm_hardware.xml	Registers `OpenArm_oyHW` plugin and comments out V10 plugin entry.
openarm_hardware/CMakeLists.txt	Builds OY hardware plugin source (V10 source currently commented out).
openarm_bringup/launch/openarm.launch.py	Switches defaults to OY description/config and changes how controller_manager gets robot_description.
openarm_bringup/launch/openarm.bimanual.launch.py	Switches defaults/paths to OY bimanual configs.
openarm_bringup/config/v10_controllers/*.yaml	Adds velocity interfaces + relaxes stopped velocity tolerance in V10 controller configs.
openarm_bringup/config/oy_controllers/openarm_oy_controllers.yaml	New unimanual OY controller configuration.
openarm_bringup/config/oy_controllers/openarm_oy_bimanual_controllers*.yaml	New bimanual OY controller configurations (incl. namespaced variant).
openarm_bimanual_moveit_config/launch/demo.launch.py	Defaults updated to OY and adds detailed docstring.
openarm_bimanual_moveit_config/config/ros2_controllers.yaml	Adds velocity command interface for both arms.
openarm_bimanual_moveit_config/config/openarm_bimanual.urdf.xacro	Switches included robot xacro from V10 to OY.
openarm_bimanual_moveit_config/config/openarm_bimanual.srdf	Updates end-effector parent links and adds `parent_group`.
openarm.repos	Points `openarm_can` to a different remote/branch (SSH URL).
.gitignore	Adds `.github/` and `openarm_description/` to ignored paths.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-12T05:47:11Z

+    std::string value = it->second;
+    std::transform(value.begin(), value.end(), value.begin(), ::tolower);
+    hand_ = (value == "true");
+  }


std::transform(..., ::tolower) is undefined behavior for negative char values. Cast to unsigned char (or use a lambda wrapping std::tolower(static_cast<unsigned char>(c))) when normalizing the parameter string.

Copilot · 2026-03-12T05:47:11Z

+    std::string value = it->second;
+    std::transform(value.begin(), value.end(), value.begin(), ::tolower);
+    can_fd_ = (value == "true");
+  }


std::transform(..., ::tolower) is undefined behavior for negative char values. Cast to unsigned char (or use a lambda wrapping std::tolower(static_cast<unsigned char>(c))) when normalizing the parameter string.

Copilot · 2026-03-12T05:47:12Z

+hardware_interface::return_type OpenArm_oyHW::write(
+    const rclcpp::Time& /*time*/, const rclcpp::Duration& /*period*/) {
+  // Control arm motors with MIT control
+  // OY motor MITParam order: {q, dq, kp, kd, tau}
+  std::vector<openarm::oy_motor::MITParam> arm_params;
+  for (size_t i = 0; i < ARM_DOF; ++i) {
+    double kp = (i < DEFAULT_KP.size()) ? DEFAULT_KP[i] : DEFAULT_KP.back();
+    double kd = (i < DEFAULT_KD.size()) ? DEFAULT_KD[i] : DEFAULT_KD.back();
+    //print kp kd pos vel tau for debugging
+    // if( i == 0 && pos_commands_[i] != 0.0) {  // Only print for the first joint if there's a command
+    //   RCLCPP_INFO(rclcpp::get_logger("OpenArm_oyHW"),
+    //               "Commanding joint %zu: pos_cmd=%.4f, vel_cmd=%.4f, kp=%.2f, kd=%.2f, tau_cmd=%.4f",
+    //               i, pos_commands_[i], vel_commands_[i], kp, kd, tau_commands_[i]);
+    // }
+    arm_params.push_back({pos_commands_[i], vel_commands_[i], 
+                          kp, kd, tau_commands_[i]});
+  }


write() always sends both pos_commands_ and vel_commands_ into the MIT command. If a velocity-only controller is active, pos_commands_ will typically remain at its default/stale value (often 0.0), which can cause the arm to fight toward that position while trying to follow the velocity command. Consider either (1) not exporting a velocity command interface unless a velocity-only mode is implemented, or (2) when only velocity is commanded, set q to the current position (and/or set kp=0) to avoid unintended position regulation.

Copilot · 2026-03-12T05:47:12Z

+         base_class_type="hardware_interface::SystemInterface">
+    <description>
+      ros2_control hardware interface for OpenArm with OY Motors.
+      Configurable arm DOF via hardware parameters.


The comment says the arm DOF is configurable via hardware parameters, but the implementation uses a compile-time constant ARM_DOF and does not parse any DOF parameter. Either implement parsing (and size all vectors / init motor lists accordingly) or update the description to avoid misleading plugin users.

Suggested change

Configurable arm DOF via hardware parameters.

The arm DOF is currently fixed in the implementation and is not configurable via hardware parameters.

Copilot · 2026-03-12T05:47:12Z

+ private:
+  // default configuration
+  static constexpr size_t ARM_DOF = 7;
+  static constexpr bool ENABLE_GRIPPER = true;


ENABLE_GRIPPER is declared but unused (the runtime behavior is controlled by hand_). This can be misleading for future maintenance; remove it or wire it into the default configuration logic (e.g., initialize hand_ from it when the parameter is absent).

Suggested change

static constexpr bool ENABLE_GRIPPER = true;

Copilot · 2026-03-12T05:47:13Z

+    openarm_->get_gripper().oy_mit_control_all(
+        {{motor_command, 0.0, GRIPPER_DEFAULT_KP, GRIPPER_DEFAULT_KD, 0.0}});
+  }
+  openarm_->recv_all(1000);


openarm_->recv_all(1000) inside write() can block up to 1s in the realtime-ish control loop, causing missed update deadlines and controller instability. Since read() already performs a recv_all(), consider moving reception exclusively to read() (or using a non-blocking / very short timeout here).

Suggested change

openarm_->recv_all(1000);

Copilot · 2026-03-12T05:47:13Z

        executable="ros2_control_node",
        output="both",
-        parameters=[robot_description_param, controllers_file_str],
+        parameters=[controllers_file_str],


ros2_control_node is no longer given the robot_description parameter, and this launch file also doesn't publish /robot_description as a topic. The remapping alone is unlikely to provide the URDF to controller_manager, which can prevent hardware/controller initialization. Pass robot_description_param into ros2_control_node (as done in the bimanual launch) or explicitly enable publishing robot_description on a topic and ensure the node subscribes to it.

Suggested change

parameters=[controllers_file_str],

parameters=[robot_description_param, controllers_file_str],

Copilot · 2026-03-12T05:47:13Z

    type: git
-    url: https://github.com/enactic/openarm_can.git
-    version: main
+    url: git@github.com:openvmi/openarm_can.git


Switching the .repos entry to an SSH URL (git@github.com:...) makes vcs import fail for users/CI without GitHub SSH keys configured. Prefer an HTTPS URL, and consider pinning to a tag/commit SHA instead of a moving branch to keep builds reproducible.

Suggested change

url: git@github.com:openvmi/openarm_can.git

url: https://github.com/openvmi/openarm_can.git

Copilot · 2026-03-12T05:47:13Z

+    left_gripper_controller:
+      type: forward_command_controller/ForwardCommandController
+
+    right_gripper_controller:
+      type: forward_command_controller/ForwardCommandController
+


In the namespaced bimanual controller config, left_gripper_controller / right_gripper_controller are declared as forward_command_controller/ForwardCommandController, while the non-namespaced bimanual config uses position_controllers/GripperActionController. If this file is meant to be a drop-in namespaced variant, this type change will break gripper action semantics (and the parameter schema differs: joints vs joint). Consider keeping the same controller types across both configs unless there’s a specific reason to change behavior when namespacing is enabled.

Copilot · 2026-03-12T05:47:14Z


 add_library(${PROJECT_NAME} SHARED
-  src/v10_simple_hardware.cpp
+  # src/v10_simple_hardware.cpp


This change stops building v10_simple_hardware.cpp, and the plugin description also comments out OpenArm_v10HW. If the repo is still expected to support the V10 (and the launch arguments still mention v10), consider keeping the V10 hardware plugin built/exported alongside the new OY plugin to avoid a breaking change.

Suggested change

# src/v10_simple_hardware.cpp

src/v10_simple_hardware.cpp

kou · 2026-03-12T08:17:48Z

Thanks for your contribution!

Could you use English for the pull request title and description?

Could you use one feature/fix in one pull request? If you have multiple improvements, could you split one large pull request to multiple small pull requests for easy to review?

StartAgainAgain and others added 17 commits January 13, 2026 11:10

update .gitignore

9332ce6

add oy motor hardware interface

9e28f50

add oy motor hardware interface

ae335da

complete oy_hardware

89fe3fe

change class name

5a001e3

更新 oy硬件接口，並配置插件“

7d4eb5b

add oy related files including controllers urdf and so on

2245d10

update srdf config and add some comments

61a674b

update calling function

c0b7ac6

modify kp kd value

8d61c61

update kp kd value

069941e

update kp value

e104e92

Update repository URL and version for openarm_can

efbd450

decrease kp to 2 kd to 0.2 for end motor

3ad5a5b

Merge branch 'oy_openarm_ros2' of github.com:openvmi/openarm_ros2 int…

6a8ec46

…o oy_openarm_ros2 merge remote change for openarm_can url

給joint_trajectory controller 添加愛速度控制參數，防止抖動

57e2629

update launch config

bb7b88e

Copilot AI review requested due to automatic review settings March 12, 2026 05:41

Copilot started reviewing on behalf of StartAgainAgain March 12, 2026 05:42 View session

Copilot AI reviewed Mar 12, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ligx/us/t100154#85

Ligx/us/t100154#85
StartAgainAgain wants to merge 17 commits intoenactic:mainfrom
openvmi:ligx/us/t100154

StartAgainAgain commented Mar 12, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

kou commented Mar 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	Configurable arm DOF via hardware parameters.
	The arm DOF is currently fixed in the implementation and is not configurable via hardware parameters.

	parameters=[controllers_file_str],
	parameters=[robot_description_param, controllers_file_str],

	url: git@github.com:openvmi/openarm_can.git
	url: https://github.com/openvmi/openarm_can.git

Conversation

StartAgainAgain commented Mar 12, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

kou commented Mar 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants