Skip to content
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

[Servo] Fix Twist transformation #2311

Merged
merged 4 commits into from Aug 23, 2023

Conversation

ibrahiminfinite
Copy link
Contributor

@ibrahiminfinite ibrahiminfinite commented Aug 21, 2023

Description

Fixes #2150
Previous discussions on #2301

The problem had boiled down to:

How do we reconcile that some users (like myself) want the rotation-only part of the twist, whereas others (like those who reported #2150) want the actual translation+rotation portion of twisting the EE frame w.r.t. the base frame?

This PR addresses the above by adding the option for proper twist conversion while preserving the existing method of twist conversion (which is wrong).
The selection is made using the parameter apply_twist_commands_about_ee_frame.

@sea-bass
@AndyZe

@codecov
Copy link

codecov bot commented Aug 21, 2023

Codecov Report

Patch coverage: 69.05% and project coverage change: +0.04% 🎉

Comparison is base (05f3501) 50.70% compared to head (11fdaf0) 50.73%.

❗ Current head 11fdaf0 differs from pull request most recent head be8860c. Consider uploading reports for the commit be8860c to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2311      +/-   ##
==========================================
+ Coverage   50.70%   50.73%   +0.04%     
==========================================
  Files         386      386              
  Lines       31922    31958      +36     
==========================================
+ Hits        16183    16212      +29     
- Misses      15739    15746       +7     
Files Changed Coverage Δ
moveit_ros/moveit_servo/src/servo.cpp 63.88% <45.84%> (-2.24%) ⬇️
moveit_ros/moveit_servo/tests/test_integration.cpp 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sea-bass sea-bass self-requested a review August 22, 2023 07:17
Copy link
Contributor

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable.

Got an unused variable failing CI here, but other small comments on naming and ... comments.

moveit_ros/moveit_servo/config/servo_parameters.yaml Outdated Show resolved Hide resolved
moveit_ros/moveit_servo/include/moveit_servo/servo.hpp Outdated Show resolved Hide resolved
Copy link
Member

@AndyZe AndyZe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks great and I agree with @sea-bass's comments. Will test it now.

@AndyZe
Copy link
Member

AndyZe commented Aug 22, 2023

Here's a video with a twist command in panda_link8 frame, planning frame is panda_link0. There is a combo of rotation and translation, so that's good. I have to say that the translation is in the opposite direction of what I expected!?

I made this test by modifying a few lines in servo_keyboard_input.cpp

rotate

@ibrahiminfinite
Copy link
Contributor Author

I have to say that the translation is in the opposite direction of what I expected!?

The transformation pulled from TF2 was the wrong one ,T_command_planning instead of T_planning_command.

Also restored servo_keyboard_input demo with the option to switch between the planning and end effector frames.

@AndyZe
Copy link
Member

AndyZe commented Aug 23, 2023

It looks exactly correct now. Nice work @ibrahiminfinite !

@tylerjw tylerjw merged commit 5af8db7 into moveit:main Aug 23, 2023
8 checks passed
@ibrahiminfinite ibrahiminfinite deleted the twist_transform_fix branch February 24, 2024 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong equations used in Servo twist transformations
4 participants