[feat] Admittance Controller#47
Conversation
|
First of all, thanks for this contribution! This is amazing :) Have you tested this with the FR3 or with the UR robots? |
There was a problem hiding this comment.
Pull request overview
Adds a new ROS2 ros2_control plugin implementing a cascaded Cartesian admittance (inner virtual mass-spring-damper) with a Cartesian impedance/OSC outer loop, including optional runtime stiffness updates via topics.
Changes:
- Introduces
CartesianAdmittanceControllercontroller implementation + header. - Adds a full parameter schema (
cartesian_admittance_controller.yaml) and generates a new parameter library target. - Registers the controller as a plugin and links it into the main library build.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/cartesian_admittance_controller.cpp |
Implements the admittance inner loop, impedance outer loop, subscriptions, and safety checks. |
include/crisp_controllers/cartesian_admittance_controller.hpp |
Declares controller class, state, buffers, and parameter plumbing. |
src/cartesian_admittance_controller.yaml |
Defines controller parameters for outer-loop impedance + inner-loop admittance + F/T sensor + variable stiffness. |
CMakeLists.txt |
Adds parameter generation for the new controller and links the generated library. |
crisp_controllers.xml |
Registers the new controller plugin for pluginlib discovery. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (params_.nullspace.damping) { | ||
| nullspace_damping.diagonal() = params_.nullspace.damping * weights; | ||
| } else { | ||
| nullspace_damping.diagonal() = 2.0 * nullspace_stiffness.diagonal().cwiseSqrt(); | ||
| } |
There was a problem hiding this comment.
setStiffnessAndDamping() checks if (params_.nullspace.damping) but nullspace.damping is a double with default -1.0 (meaning “auto”). Since -1.0 is truthy, this branch will run and assign a negative damping, which is unstable. Use an explicit numeric check (e.g., > 0.0) to decide whether to override the computed damping.
| // Orientation error: quaternion-based with antipodal check | ||
| Eigen::Quaterniond q_inner(inner_SE3_.rotation()); | ||
| Eigen::Quaterniond q_desired(desired_SE3.rotation()); | ||
| if (q_desired.coeffs().dot(q_inner.coeffs()) < 0.0) { | ||
| q_inner.coeffs() = -q_inner.coeffs(); | ||
| } | ||
| Eigen::Quaterniond q_error(q_inner.inverse() * q_desired); | ||
| Eigen::Vector3d adm_rot_error; | ||
| adm_rot_error << q_error.x(), q_error.y(), q_error.z(); | ||
| adm_rot_error = inner_SE3_.rotation() * adm_rot_error; // Rotate to world frame | ||
|
|
||
| Eigen::Vector<double, 6> adm_error; | ||
| adm_error << adm_pos_error, adm_rot_error; | ||
|
|
There was a problem hiding this comment.
The admittance orientation error is computed by taking the vector part of the quaternion error ([x,y,z]) and rotating it into world frame. This is not the SO(3) logarithm and produces incorrect magnitudes for non-small rotations (e.g., near 180°), which can destabilize the admittance spring term. Consider computing rotational error via pinocchio::log3(R_inner.transpose() * R_desired) (and then mapping to the desired reference frame) to match the impedance loop’s error definition.
There was a problem hiding this comment.
+1 if I am not mistaken this is how we do it in the cartesian controller
| ft_sensor_frame_id = model_.getFrameId(params_.ft_sensor.frame); | ||
| RCLCPP_INFO(get_node()->get_logger(), | ||
| "Using F/T sensor frame: %s (id=%d)", params_.ft_sensor.frame.c_str(), ft_sensor_frame_id); |
There was a problem hiding this comment.
ft_sensor_frame_id is set via model_.getFrameId(params_.ft_sensor.frame) without validating that the frame name exists. If the parameter is misspelled or missing from the URDF, this can silently select an unintended frame (or an invalid id) and corrupt the wrench transform. Add an explicit model_.existFrame(params_.ft_sensor.frame) (or equivalent) check and fail configuration with a clear error if the frame does not exist.
| ft_sensor_frame_id = model_.getFrameId(params_.ft_sensor.frame); | |
| RCLCPP_INFO(get_node()->get_logger(), | |
| "Using F/T sensor frame: %s (id=%d)", params_.ft_sensor.frame.c_str(), ft_sensor_frame_id); | |
| if (!model_.existFrame(params_.ft_sensor.frame)) { | |
| RCLCPP_ERROR( | |
| get_node()->get_logger(), | |
| "F/T sensor frame '%s' does not exist in the robot model. Please check the " | |
| "ft_sensor.frame parameter and the URDF frames.", | |
| params_.ft_sensor.frame.c_str()); | |
| return CallbackReturn::ERROR; | |
| } | |
| ft_sensor_frame_id = model_.getFrameId(params_.ft_sensor.frame); | |
| RCLCPP_INFO( | |
| get_node()->get_logger(), | |
| "Using F/T sensor frame: %s (id=%d)", | |
| params_.ft_sensor.frame.c_str(), | |
| ft_sensor_frame_id); |
| // For each axis, use explicit damping if > 0, otherwise compute from stiffness | ||
| damping.diagonal() | ||
| << (params_.task.d_pos_x > 0 ? params_.task.d_pos_x : 2.0 * std::sqrt(params_.task.k_pos_x)), | ||
| (params_.task.d_pos_y > 0 ? params_.task.d_pos_y : 2.0 * std::sqrt(params_.task.k_pos_y)), | ||
| (params_.task.d_pos_z > 0 ? params_.task.d_pos_z : 2.0 * std::sqrt(params_.task.k_pos_z)), | ||
| (params_.task.d_rot_x > 0 ? params_.task.d_rot_x : 2.0 * std::sqrt(params_.task.k_rot_x)), | ||
| (params_.task.d_rot_y > 0 ? params_.task.d_rot_y : 2.0 * std::sqrt(params_.task.k_rot_y)), | ||
| (params_.task.d_rot_z > 0 ? params_.task.d_rot_z : 2.0 * std::sqrt(params_.task.k_rot_z)); |
There was a problem hiding this comment.
setStiffnessAndDamping() computes task damping from the parameter stiffness values (params_.task.k_*) even when topic stiffness overrides are active. If the intent is “critical damping when d_* < 0”, this should be based on the effective stiffness actually used (topic or params), otherwise the controller can become under/over-damped when stiffness changes at runtime.
| // For each axis, use explicit damping if > 0, otherwise compute from stiffness | |
| damping.diagonal() | |
| << (params_.task.d_pos_x > 0 ? params_.task.d_pos_x : 2.0 * std::sqrt(params_.task.k_pos_x)), | |
| (params_.task.d_pos_y > 0 ? params_.task.d_pos_y : 2.0 * std::sqrt(params_.task.k_pos_y)), | |
| (params_.task.d_pos_z > 0 ? params_.task.d_pos_z : 2.0 * std::sqrt(params_.task.k_pos_z)), | |
| (params_.task.d_rot_x > 0 ? params_.task.d_rot_x : 2.0 * std::sqrt(params_.task.k_rot_x)), | |
| (params_.task.d_rot_y > 0 ? params_.task.d_rot_y : 2.0 * std::sqrt(params_.task.k_rot_y)), | |
| (params_.task.d_rot_z > 0 ? params_.task.d_rot_z : 2.0 * std::sqrt(params_.task.k_rot_z)); | |
| // For each axis, use explicit damping if > 0, otherwise compute critical damping | |
| // from the effective stiffness currently in use (stiffness matrix diagonal). | |
| damping.diagonal() | |
| << (params_.task.d_pos_x > 0 ? params_.task.d_pos_x : 2.0 * std::sqrt(stiffness(0, 0))), | |
| (params_.task.d_pos_y > 0 ? params_.task.d_pos_y : 2.0 * std::sqrt(stiffness(1, 1))), | |
| (params_.task.d_pos_z > 0 ? params_.task.d_pos_z : 2.0 * std::sqrt(stiffness(2, 2))), | |
| (params_.task.d_rot_x > 0 ? params_.task.d_rot_x : 2.0 * std::sqrt(stiffness(3, 3))), | |
| (params_.task.d_rot_y > 0 ? params_.task.d_rot_y : 2.0 * std::sqrt(stiffness(4, 4))), | |
| (params_.task.d_rot_z > 0 ? params_.task.d_rot_z : 2.0 * std::sqrt(stiffness(5, 5))); |
There was a problem hiding this comment.
And this is actually intended. The question is: when we update the controller's stiffness, should we update the damping accordingly?
After spending some time thinking about it, I decided that it would be better not to change it and always keep the same. Hence, damping is always derived from the original parameters of the controller, if they are not specified
| void parse_target_stiffness_(); | ||
|
|
||
| /** | ||
| * @brief Reads the F/T sensor data from the realtime buffer and applies filtering |
There was a problem hiding this comment.
The docstring for parse_ft_sensor_() says it “applies filtering”, but the implementation currently only copies the wrench into ft_wrench_ without any filtering. Either update the comment to match behavior or add the intended filtering (e.g., EMA / deadband) to avoid misleading maintainers.
| * @brief Reads the F/T sensor data from the realtime buffer and applies filtering | |
| * @brief Reads the F/T sensor data from the realtime buffer and updates the internal wrench |
| base_frame: | ||
| type: string | ||
| default_value: "" | ||
| description: "Name of the end-effector frame" |
There was a problem hiding this comment.
base_frame.description currently reads "Name of the end-effector frame" (copy/paste from end_effector_frame). This is misleading for users configuring the controller; update it to describe the base/root frame used for kinematics (or whatever base_frame actually means here).
| description: "Name of the end-effector frame" | |
| description: "Name of the base/root frame used as reference for kinematics computations" |
There was a problem hiding this comment.
Note that this typo is present in cartesian controller as well: https://github.com/utiasDSL/crisp_controllers/blob/3b234cb1a9c1f8554458b0303bfe36c65552e438/src/cartesian_controller.yaml#L13
danielsanjosepro
left a comment
There was a problem hiding this comment.
Thanks again for this 🙏 I have a just few discussion points :)
|
|
||
| // 10. Semi-implicit Euler integration | ||
| double dt = period.seconds(); | ||
| if (dt <= 0.0) dt = 0.001; // safety |
There was a problem hiding this comment.
Is this even possible?
There was a problem hiding this comment.
In general, I really like the way it is structured (a few redundant comments in the update function) but the code looks great :)
There just a few things we could discuss before we proceed:
- this controller shares a lot of logic with the cartesian controller and it would make sense to abstract this logic in some way. The pro of doing this is that we dont have so much code duplication. The con is that we need to refactor the other controller, and make sure that we stay backwards compatible (in the case of a refactoring I would rather make a major version bump for the whole package)
- similar to the cartesian_controller, this controller does a few mem allocations inside the update loop which could potentially be pre allocated. But I guess this is still not too bad and we can live with this :)
One final comment: add yourself (and Lev as well) to the docs with your contribution and also feel free to add an extra description of the admittance controller somewhere. Ideally in the controller details section!
Let us discuss the first point if you want, if you feel like the refactor is just much unnecessary work (including testing the change) we can also consider leave it as it is
There was a problem hiding this comment.
Thank you for your feedback! Regarding shared logic with the Cartesian controller, I also noticed that it would actually require a bit more refactoring than just changing some fields from private to protected (as I originally suggested).
With that in mind, let's first modify, debug & merge this PR as a standalone controller, then proceed with refactoring separately. Let me know what you think!
Thank you for taking the time to review them! We've tested it only on Franka Panda robot yet. I think it would be better to test this on both of them to make sure we're not missing anything. I will try to run it on UR and let you know when it would work properly! |
Implement a new admittance controller with an inner virtual mass-spring-damper loop driven by F/T sensor readings and an outer impedance control loop. The inner MSD state evolves via semi-implicit Euler integration and its output pose serves as the target for the existing Cartesian impedance logic. Includes support for variable admittance stiffness via topic, F/T sensor EMA filtering, static offset subtraction, and all compensation torques from CartesianController.
F/T sensor processing (EMA filtering, static offset subtraction) is not the controller's responsibility. The wrench is now passed through directly from the topic. Removed filter_alpha, use_static_offset, and static_offset parameters from the YAML.
get_optional() on LoanedStateInterface is only available in Kilted+, not Jazzy. The guards were incorrectly using ROS2_VERSION_ABOVE_HUMBLE. set_value() returning bool is an Iron+ change so those guards correctly remain ROS2_VERSION_ABOVE_HUMBLE.
Validate and clamp both impedance and admittance stiffness in setStiffnessAndDamping(), setAdmittanceParameters(), parse_target_stiffness_(), and parse_target_adm_stiffness_(). Defaults: translational=900, rotational=60.
This reverts commit 790a0e1.
…riable_max_stiffness
da0627f to
b60eadf
Compare
47335c6 to
7d7ac97
Compare
|
Hello, @danielsanjosepro! I believe I have addressed most of your suggestions, and commented the rest. Also I've tested admittance controller using pixi_ur_ros2, see lvjonok/pixi_ur_ros2#5. Finally, I've modified documentation to include myself in contributors & briefly describe admittance controller implementation. Before we proceed, I have several concerns:
With that in mind, I believe we could proceed with merging this PR. Once again, thank you for your detailed feedback & opinion! |
|
Thanks for testing this out! It is always nice to have a few examples using the controller :)
Thank you for this great addition! |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Cartesian Admittance Controller with Cartesian Impedance in outer loop
Summary
CartesianAdmittanceControllerros2_control plugin that combines an admittance inner loop (virtual mass-spring-damper) with a Cartesian impedance outer loopHow the Admittance Controller Works
The controller has two cascaded loops:
Inner Loop: Admittance (Virtual MSD)
The admittance layer maintains an internal pose state$x_{inner}$ that evolves as a virtual mass-spring-damper driven by external forces:
where:
changeReferenceFrametarget_posetopic)Note that this required an external F/T sensor. This could also be replaced by the robot's external wrench estimation (for example, as provided by a Franka manipulator). This also requires proper URDF, where there is a separate frame for F/T wrench measurement: the controller transforms measured force from local frame to world-aligned pinocchio frame
Integration uses semi-implicit Euler on SE3 manifold at each control cycle:
Outer Loop: Impedance
The resulting position$x_{inner}$ is used as a target for the outer loop Cartesian controller. The outer controller is identical to the one at https://github.com/utiasDSL/crisp_controllers/blob/main/src/cartesian_controller.cpp. Unfortunately, I was unable to properly inherit from the Cartesian Controller, since most of its fields are private.
Adaptive stiffness parameters
Similar to #46, the admittance controller introduces variable stiffness for both inner admittance & outer impedance controllers, which could be configured separately.
Parameters
Admittance-specific parameters
All
CartesianControllerparameters are also available for the impedance outer loop (task.*,nullspace.*,filter.*,friction.*, etc.).Changes
cartesian_admittance_controller.hppcartesian_admittance_controller.cppcartesian_admittance_controller.yamlCMakeLists.txtcrisp_controllers.xmlNotes
crisp_pyare required. PR with the corresponding change will be opened shortlyprotectedinstead ofprivate. I believe this would simplify implementation and keep the outer Cartesian controller implementation always up-to-date between the admittance outer and regular Cartesian controllers.