-
Notifications
You must be signed in to change notification settings - Fork 102
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add gravity compensation filter #153
Add gravity compensation filter #153
Conversation
Co-authored-by: Denis Štogl <[email protected]>
fixed gravity compensation calculated values fixed guard condition issues (rclcpp init must be called before) fixed configure issues (low-level must be configured to avoid undefined logger)
528d4ea
to
5715e8f
Compare
ab3838d
to
b18ffbd
Compare
The new changes to this PR stem from using the filter in admittance control and realizing mistakes. One major mistake was the switch to wrench transforms (as suggested in a TODO in previous version of this code), because it (correctly) applies an additional torque, which should not happen due to gravity being a field and not a force exerted at a certain point than generates a torque when moved. So one should not transform the wrench to base or what ever other computation frame, just rotate it, and remove the gravity force on the correct axes. Additionally, since there are I refactored variable names and comments to better explain what happens, and simplified the frames (no need to compute in world frame, I would still like to add a test using a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The calculations look too complex to me – at least to the older version as I can remember. Is everything necessary?
Also, tests should not declare parameters if possible, but generate_parameter_library should take care of it
transform_data_out_sensor_ = p_tf_Buffer_->lookupTransform( | ||
data_out.header.frame_id, parameters_.sensor_frame, rclcpp::Time()); | ||
// rotation only (because gravity is a field) from world (gravity frame) to sensor frame | ||
rot_sensor_world_ = p_tf_Buffer_->lookupTransform( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this is correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the rotation, well, if I use the full transform, the -mg force along z world gets transformed from the world to sensor frame, generating a force matching -mg in the sensor frame, but also the cross product of the relative distance between sensor and world with the -mg, creating a torque. Gravity is not creating such a torque at the CoG. The only cross product is the distance of CoG to center of measurement with the simple gravity force at CoG (but oriented correctly).
The same calculation is done there https://github.com/ros-controls/ros2_controllers/blob/master/admittance_controller/include/admittance_controller/admittance_rule_impl.hpp#L318
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a short documentation here please? Some (pseudo-) formulas to describe the mathematics would be great, otherwise it is very hard to review.
Maybe in a separate readme.[md,rst] together with some high-level description of what the filter is good for.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## ros2-master #153 +/- ##
===============================================
+ Coverage 75.55% 77.31% +1.76%
===============================================
Files 24 29 +5
Lines 1174 1283 +109
Branches 92 97 +5
===============================================
+ Hits 887 992 +105
- Misses 238 240 +2
- Partials 49 51 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I haven't gone through the math
control_toolbox/include/control_filters/gravity_compensation.hpp
Outdated
Show resolved
Hide resolved
control_toolbox/include/control_filters/gravity_compensation.hpp
Outdated
Show resolved
Hide resolved
control_toolbox/src/control_filters/gravity_compensation_filter_parameters.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: Sai Kishor Kothakota <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good to me.
I'm just not sure about using lookup transform directly in the RT loop. We should take a look at it and improve it.
There are thoughts and some explanation on the reason of using lookup transform in some out-dated/hidden remarks in the PR, but the main issue was that the computation requires to know something the kinematics/controllers did not know in the use case we had, especially if the real life sensor had a thickness, and the 'sensor_frame' was not at the flange (end-effector frame). I think it was decided to let it as is, but I agree it is not nice to have lookup transforms in an RT loop. I think it requires passing more information to the update loop if the lookup must be removed, so that the controller itself, having access to the kinematics can compute it, but it might also require the sensor position and CoG position of the mass to pre-compute those transforms for the filter. Not so trivial. |
@guihomework yes, I understood that it was used due to the end effector transformation. Maybe, it would be nice to have an update method with this extra transformation arg, so if a controller can compute it, then it can use this method to avoid it. This can be a future work |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the initial work and the discussion, I think we can merge this now!
ac87f66
into
ros-controls:ros2-master
--------- Co-authored-by: Daniel Zumkeller <[email protected]> Co-authored-by: Denis Štogl <[email protected]> Co-authored-by: Christoph Froehlich <[email protected]> Co-authored-by: Sai Kishor Kothakota <[email protected]> (cherry picked from commit ac87f66)
--------- Co-authored-by: Daniel Zumkeller <[email protected]> Co-authored-by: Denis Štogl <[email protected]> Co-authored-by: Christoph Froehlich <[email protected]> Co-authored-by: Sai Kishor Kothakota <[email protected]> Co-authored-by: GuiHome <[email protected]>
This is a rework of PR #115 for the gravity-compensation filter part, now using generate_parameter_library.