Skip to content

Conversation

pac48
Copy link
Collaborator

@pac48 pac48 commented Aug 7, 2022

No description provided.

@pac48 pac48 force-pushed the convert-interface-to-eigen branch 2 times, most recently from 96d7458 to c84de51 Compare August 8, 2022 14:54
Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

I propose to keep both interfaces for two most important methods, i.e., joints_delta and cartesian_deltas. The other two methods are also OK if returning only Eigen for now since there is no use-case where one would use other things.

@bmagyar
Copy link
Member

bmagyar commented Aug 8, 2022

Please rebase on master

@pac48 pac48 force-pushed the convert-interface-to-eigen branch from c84de51 to 7288044 Compare August 8, 2022 20:24
@pac48 pac48 force-pushed the convert-interface-to-eigen branch from 5904350 to fcebc37 Compare August 8, 2022 21:17
@pac48 pac48 force-pushed the convert-interface-to-eigen branch from fcebc37 to 42af298 Compare August 8, 2022 21:19
Copy link
Contributor

@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.

Sorry to keep nit-picking. It's almost there, I think.

pac48 and others added 7 commits August 9, 2022 09:06
…nterface into convert-interface-to-eigen

� Conflicts:
�	kinematics_interface/CMakeLists.txt
�	kinematics_interface/include/kinematics_interface/kinematics_interface_base.hpp
�	kinematics_interface_kdl/CMakeLists.txt
�	kinematics_interface_kdl/include/kinematics_interface_kdl/kinematics_interface_kdl.hpp
�	kinematics_interface_kdl/src/kinematics_interface_kdl.cpp
�	kinematics_interface_kdl/test/test_kinematics_interface_kdl.cpp
…ics_interface into convert-interface-to-eigen

� Conflicts:
�	kinematics_interface/CMakeLists.txt
�	kinematics_interface/include/kinematics_interface/kinematics_interface.hpp
�	kinematics_interface_kdl/CMakeLists.txt
�	kinematics_interface_kdl/include/kinematics_interface_kdl/kinematics_interface_kdl.hpp
�	kinematics_interface_kdl/test/test_kinematics_interface_kdl.cpp
@pac48 pac48 mentioned this pull request Aug 10, 2022
4 tasks
@codecov-commenter
Copy link

codecov-commenter commented Aug 10, 2022

Codecov Report

❗ No coverage uploaded for pull request base (master@d2d6746). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master       #6   +/-   ##
=========================================
  Coverage          ?   31.11%           
=========================================
  Files             ?        2           
  Lines             ?      180           
  Branches          ?      116           
=========================================
  Hits              ?       56           
  Misses            ?       20           
  Partials          ?      104           
Flag Coverage Δ
unittests 31.11% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

Few minor changes and why not use kinematic_interface as real library with .cpp file?

Comment on lines 99 to 110
auto joint_pos = Eigen::Map<const Eigen::VectorXd>(joint_pos_vec.data(), joint_pos_vec.size());
auto delta_x = Eigen::Map<const Eigen::VectorXd>(delta_x_vec.data(), delta_x_vec.size());
// TODO heap allocation should be removed for realtime use
Eigen::VectorXd delta_theta =
Eigen::Map<Eigen::VectorXd>(delta_theta_vec.data(), delta_theta_vec.size());

bool ret = convert_cartesian_deltas_to_joint_deltas(joint_pos, delta_x, link_name, delta_theta);
for (auto i = 0ul; i < delta_theta_vec.size(); i++)
{
delta_theta_vec[i] = delta_theta[i];
}
return ret;
Copy link
Member

Choose a reason for hiding this comment

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

Why not add this default implementation into a cpp file? We are anyway using this header as library. Isn't it?

Copy link
Member

Choose a reason for hiding this comment

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

This also enable those to be overloaded if this is useful from any reason.

@destogl destogl requested a review from bmagyar August 15, 2022 21:02
@destogl destogl merged commit 097aa64 into ros-controls:master Aug 17, 2022
christophfroehlich pushed a commit that referenced this pull request Oct 3, 2025
* bump version: changes to support rolling distro

* Add virtual function to avoid compilation error.

Signed-off-by: Marco A. Gutierrez <marcogg@marcogg.com>

* implement inv jacobian function and tests similar to pinocchio_interface_kdl

---------

Signed-off-by: Marco A. Gutierrez <marcogg@marcogg.com>
Co-authored-by: Marco A. Gutierrez <marcogg@marcogg.com>
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.

5 participants