Release 3 by vamsikalagaturu · Pull Request #33 · Robots4Sustainability/eddie-ros

vamsikalagaturu · 2025-12-17T17:17:10Z

No description provided.

…odes and waypoints for arm control

Clarified instructions for cloning repositories and added a note about dependent packages for 'eddie_description'. Removed redundant cloning step.

…ver-interface Refactor/action server interface

This was added since if we want to change the gains dynamically, we do not want the torques to spike up. However, at the moment, every single torque command is passed through the filter before being sent to the motors (smoothing does not only happen on gain change).

Also modified the file to not use the gripper, since we do not have access to the gripper at the moment. Removed the rotation tolerance as well for now.

Added instructions for installing Zenoh router.

Create a dedicated pid.cpp file

Updated section on visualizing Cartesian error to reference the Cartesian Error Visualizer and provided installation instructions.

…rget flag, clean up commented out code

Changed evaluate bilateral constraint function Made torque smoothing only active during dynamic parameter change

Fixes #21 return to idle

…hing logic

…or PID components

Could be used for filtering sensor data, but might cause some delays, so decided not to use it right now. Also removed torque publishers since we replaced them with logs.

Feature/pid gains

Copilot

Pull request overview

This PR represents a significant "Release 3" update to the Eddie ROS interface, introducing major enhancements to the robot control system including dynamic PID tuning, improved action server APIs, torque smoothing, and extensive refactoring of the control architecture.

Key Changes:

Refactored PID controller into a separate module with structured output (P, I, D components) and added dynamic parameter tuning at runtime
Updated action server APIs to use result codes instead of boolean success flags, providing more granular feedback
Implemented torque interpolation/smoothing when PID gains change and added comprehensive logging for debugging
Added new FSM state transition (EXECUTE → IDLE) to properly handle completion of arm control goals

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 27 comments.

Show a summary per file

File	Description
src/pid.cpp	New file implementing PID controller with structured output for component visibility
src/interface.cpp	Major refactoring: added dynamic PID parameters, torque smoothing, bilateral deadband constraints, and FSM improvements
src/interface-parameters.cpp	Added PID parameter declaration, reloading, and callback handling for runtime tuning
src/interface-test.cpp	Updated test file to match new action API (result_code/result_message) and PIDOutput structure
src/sim-interface.cpp	Updated simulator to use new action result API with result codes and messages
include/eddie_ros/interface.hpp	Added PIDOutput struct, TorqueInterpolator, new member variables, and renamed helper methods
include/eddie_ros/eddie_ros.fsm.hpp	Added E_EXECUTE_EXIT_IDLE event and T_EXECUTE_IDLE transition
include/eddie_ros/eddie_ros.fsm	Updated FSM definition with new EXECUTE→IDLE transition
action/ArmControl.action	Replaced success/message with result_code/result_message and added result code constants
action/GripperControl.action	Replaced success/message with result_code/result_message, updated feedback fields, and changed position scale
CMakeLists.txt	Added pid.cpp to build targets
package.xml	Added eddie_description dependency
r4s.repos	Updated branch references from main/feature branches to dev
README.md	Added documentation for PID tuning, visualization tools, and corrected parameter naming examples
robot_setup.md	Reorganized setup steps and added Zenoh router installation requirement

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

Copilot · 2025-12-17T18:05:30Z

src/interface.cpp

        RCLCPP_INFO(get_logger(), "Left arm chain constructed successfully");
    }
-    if (!tree.getChain("base_link", "kinova_right_grasp_link", rightarm_chain)) {
+    if (!tree.getChain("base_link", "kinova_left_grasp_link", rightarm_chain)) {


Incorrect chain endpoint specified. The right arm chain is being constructed with "kinova_left_grasp_link" as the end effector instead of "kinova_right_grasp_link". This will cause the right arm kinematic chain to be incorrectly configured, leading to wrong pose calculations and control commands.

Suggested change

if (!tree.getChain("base_link", "kinova_left_grasp_link", rightarm_chain)) {

if (!tree.getChain("base_link", "kinova_right_grasp_link", rightarm_chain)) {

Copilot · 2025-12-17T18:05:31Z

src/interface.cpp

+            target_pose_relative(arm_side) = KDL::Frame::Identity();
+            has_new_target(arm_side) = true;


Setting has_new_target to true when clearing the target is counterintuitive and likely incorrect. This flag typically indicates a new target has been set, but here it's being set when clearing/resetting the target. This could cause the main control loop to incorrectly interpret the identity transform as a new target pose.

Copilot · 2025-12-17T18:05:31Z

src/interface-parameters.cpp

+    const double error_sum_tol = 0.9;
+    const double decay_rate = 0.5;
+


Magic numbers for error_sum_tol (0.9) and decay_rate (0.5) are hardcoded. Consider making these configurable parameters so they can be tuned without recompiling, or at minimum define them as named constants with explanatory comments.

Suggested change

const double error_sum_tol = 0.9;

const double decay_rate = 0.5;

// Tolerance for the accumulated PID error sum used when applying torque smoothing

// after gain changes. Consider exposing this as a configurable ROS parameter if

// runtime tuning is required.

static constexpr double kErrorSumTolerance = 0.9;

// Exponential decay rate applied during torque smoothing. Lower values result in

// slower, smoother transitions; higher values respond faster but may be less smooth.

// Consider exposing this as a configurable ROS parameter if runtime tuning is required.

static constexpr double kDecayRate = 0.5;

const double error_sum_tol = kErrorSumTolerance;

const double decay_rate = kDecayRate;

Copilot · 2025-12-17T18:05:32Z

src/interface.cpp

        RCLCPP_INFO(get_logger(), "Left arm chain constructed successfully");
    }
-    if (!tree.getChain("eddie_base_link", "eddie_right_arm_robotiq_85_grasp_link", rightarm_chain)) {
+    if (!tree.getChain("eddie_base_link", "eddie_right_arm_end_effector_link", rightarm_chain)) { //TODO: revert to eddie_right_arm_robotiq_85_grasp_link


TODO comment indicates temporary link name that should be reverted. This suggests the code is in an incomplete state and may not work correctly with the actual hardware configuration. Consider either completing this change or removing the TODO if the current configuration is intentional.

Copilot · 2025-12-17T18:05:32Z

src/interface.cpp

+        // robif2b_kg3_robotiq_gripper_update(&kinova_leftgripper);
+        // robif2b_kinova_gen3_update(&kinova_leftarm);


Commented-out robif2b update calls suggest this functionality has been disabled. If this is temporary for testing, add a TODO comment explaining why and when to re-enable. If permanent, remove the commented code to improve readability.

Copilot · 2025-12-17T18:05:36Z

src/interface.cpp

    if (should_control_right_arm()) {
-        robif2b_kg3_robotiq_gripper_update(&kinova_rightgripper);
-        robif2b_kinova_gen3_update(&kinova_rightarm);
+        // robif2b_kg3_robotiq_gripper_update(&kinova_rightgripper);
+        // robif2b_kinova_gen3_update(&kinova_rightarm);


Commented-out robif2b update calls suggest this functionality has been disabled. If this is temporary for testing, add a TODO comment explaining why and when to re-enable. If permanent, remove the commented code to improve readability.

Copilot · 2025-12-17T18:05:37Z

src/interface.cpp

+        /* for (int i = 0; i < num_jnts_rightarm; i++) {
            saturate(&tau_ctrl_rightarm(i), -KINOVA_TAU_CMD_LIMIT, KINOVA_TAU_CMD_LIMIT);
            eddie_state->kinova_rightarm_state.eff_cmd[i] = tau_ctrl_rightarm(i);
-        }
+        } */


Commented-out code for direct torque command assignment. If this alternative approach is no longer needed, remove it. If kept for reference, add a comment explaining why this approach was replaced with the interpolation logic.

Copilot · 2025-12-17T18:05:37Z

src/interface.cpp

+        std::chrono::milliseconds(1),
+        std::bind(&EddieRosInterface::publish_pid_components, this)
    );
+    RCLCPP_INFO(this->get_logger(), "PID component publisher timer started at 50 Hz.");


Log message incorrectly states "50 Hz" when the timer is set to 1 millisecond (1000 Hz). The comment should reflect the actual frequency.

Suggested change

RCLCPP_INFO(this->get_logger(), "PID component publisher timer started at 50 Hz.");

RCLCPP_INFO(this->get_logger(), "PID component publisher timer started at 1000 Hz.");

Copilot · 2025-12-17T18:05:37Z

src/pid.cpp

+}
+
+// clamp integral term to prevent windup - max and min value it can have (based on these plots below)
+// plot P term, I term, D term separately for debugging
+// p + i + d
+// feedback for pos, velocity separately


Comments at the end of the file appear to be development notes rather than proper documentation. These should either be removed or moved to proper documentation if they're intended as implementation guidance.

Suggested change

}

// clamp integral term to prevent windup - max and min value it can have (based on these plots below)

// plot P term, I term, D term separately for debugging

// p + i + d

// feedback for pos, velocity separately

}

Copilot · 2025-12-17T18:05:38Z

action/GripperControl.action

@@ -1,13 +1,21 @@
 # Request: Command to control gripper
-float64 target_position     # 0.0 = fully closed, 1.0 = fully open
+float64 target_position     # 100.0 = fully closed, 0.0 = fully open


Gripper position range documentation updated from "0.0 = fully closed, 1.0 = fully open" to "100.0 = fully closed, 0.0 = fully open". This is a breaking API change that reverses the polarity and changes the scale. Ensure all calling code has been updated accordingly and consider documenting this as a breaking change in release notes.

bhunecke and others added 30 commits October 30, 2025 15:43

update pid values and cartesian error publisher

ac8cf7c

refactor action definitions for arm and gripper control, add status c…

15e0a2b

…odes and waypoints for arm control

Add deadband parameter to PID controller and update initialization

2647538

Add dynamic parameter callback for adjustments

0cf0957

Add section for adjusting PID gains at runtime in README

48d0f53

fix missing eddie description dependency

fc9d2fb

refactor arm control action interface to use updated status codes

9c5ef6e

refactor test and sim interfaces to use updated status codes

0e6e96d

Update robot_setup.md for clarity and redundancy

1387cb7

Clarified instructions for cloning repositories and added a note about dependent packages for 'eddie_description'. Removed redundant cloning step.

Merge pull request #28 from Robots4Sustainability/refactor/action-ser…

cfb6b40

…ver-interface Refactor/action server interface

refactor: update idle state to run gravity compensation

1af9be2

Undo changes in PID control

185dc2d

Revert the PID class changes

7aa0d18

Implement the deadband in compute_cartesian_ctrl

11aa2de

Add publishers to visualize the raw vs. smoothed torques

f32cfa4

add idle and execute state transitions based on execution flags

85a4442

Fix evaluate bilateral constraint return

ebc44ff

Also modified the file to not use the gripper, since we do not have access to the gripper at the moment. Removed the rotation tolerance as well for now.

add execute to idle transition and update event handling

dc3b0cf

Adjust the P-gain for the controller based on deadband

f3498f7

Include Zenoh router installation instructions

11d3099

Added instructions for installing Zenoh router.

Activate smoothing based on dynamic gain change

fa89bc6

Create a dedicated pid.cpp file

Change version to 'dev' for eddie-ros and robif2b

3838bb0

Revise Cartesian error visualization instructions

4c3f6ed

Updated section on visualizing Cartesian error to reference the Cartesian Error Visualizer and provided installation instructions.

fix goal cancel callback by clearing relative target and reset new ta…

aee8c3d

…rget flag, clean up commented out code

refactor: rename helper methods for clarity and remove unused parameters

8bc4de5

update logging

3a1f4f9

update signal handler

49332dc

Reverted adjusting p-gain changes, since controller already handles that

95ecfba

Changed evaluate bilateral constraint function Made torque smoothing only active during dynamic parameter change

Refactor torque smoothing parameters and publishers for consistency

cf4c8b4

rinaalo and others added 9 commits December 12, 2025 15:38

Merge pull request #30 from Robots4Sustainability/fix/return-to-idle

57a3959

Fixes #21 return to idle

Add TorqueInterpolator for smooth torque transitions and update smoot…

10e321c

…hing logic

Refactor PID control to return structured output and add publishers f…

14a73da

…or PID components

Add a low pass filter function (not being used right now)

95a5c64

Could be used for filtering sensor data, but might cause some delays, so decided not to use it right now. Also removed torque publishers since we replaced them with logs.

Change some pid values, clean up ccode

40fee23

Publishing pid components with a timer

742e199

Add eddie_description link to robot_setup.md

fa244fd

Merge branch 'release-3' into feature/pid-gains

bf5fa40

Merge pull request #32 from Robots4Sustainability/feature/pid-gains

75f6385

Feature/pid gains

vamsikalagaturu requested a review from Copilot December 17, 2025 17:50

Copilot started reviewing on behalf of vamsikalagaturu December 17, 2025 17:50 View session

Copilot AI reviewed Dec 17, 2025

View reviewed changes

Robots4Sustainability deleted a comment from Copilot AI Dec 17, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Release 3#33

Release 3#33
vamsikalagaturu wants to merge 39 commits intorelease-2from
release-3

vamsikalagaturu commented Dec 17, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	if (!tree.getChain("base_link", "kinova_left_grasp_link", rightarm_chain)) {
	if (!tree.getChain("base_link", "kinova_right_grasp_link", rightarm_chain)) {

		target_pose_relative(arm_side) = KDL::Frame::Identity();
		has_new_target(arm_side) = true;

		const double error_sum_tol = 0.9;
		const double decay_rate = 0.5;

-    const double error_sum_tol = 0.9;
-    const double decay_rate = 0.5;
+    // Tolerance for the accumulated PID error sum used when applying torque smoothing
+    // after gain changes. Consider exposing this as a configurable ROS parameter if
+    // runtime tuning is required.
+    static constexpr double kErrorSumTolerance = 0.9;
+    // Exponential decay rate applied during torque smoothing. Lower values result in
+    // slower, smoother transitions; higher values respond faster but may be less smooth.
+    // Consider exposing this as a configurable ROS parameter if runtime tuning is required.
+    static constexpr double kDecayRate = 0.5;
+    const double error_sum_tol = kErrorSumTolerance;
+    const double decay_rate    = kDecayRate;

		// robif2b_kg3_robotiq_gripper_update(&kinova_leftgripper);
		// robif2b_kinova_gen3_update(&kinova_leftarm);

	RCLCPP_INFO(this->get_logger(), "PID component publisher timer started at 50 Hz.");
	RCLCPP_INFO(this->get_logger(), "PID component publisher timer started at 1000 Hz.");

Conversation

vamsikalagaturu commented Dec 17, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants