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

ENU<->ECEF transforms fix. #847

Merged
merged 4 commits into from
Nov 1, 2017
Merged

ENU<->ECEF transforms fix. #847

merged 4 commits into from
Nov 1, 2017

Conversation

pavloblindnology
Copy link

ENU<->ECEF transformations are totally screwed up resulting in crazy positions in global_position\local topic. fake_gps plugin is also affected.
P.S. This is my 1st contributing to mavros - tried to follow the conventions as much as possible.

Copy link
Member

@TSC21 TSC21 left a comment

Choose a reason for hiding this comment

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

Thanks for checking this out @pavloblindnology. I did not particularly checked this on a real thing, so I applied the rotations with a fixed frame.
Can you give us a comparison from before/after this changes? After you make the requested changes, I will merge it.

*/
template<class T>
inline T transform_frame_ecef_enu(const T &in) {
return detail::transform_static_frame(in, StaticTF::ECEF_TO_ENU);
inline T transform_frame_ecef_enu(const T &map_origin, const T &in) {
Copy link
Member

Choose a reason for hiding this comment

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

Keep it as is - pass the origin like:

inline T transform_frame_ecef_enu(const T &in,const T &map_origin) {
 	return detail::transform_static_frame(in, map_origin, StaticTF::ECEF_TO_ENU);

*/
template<class T>
inline T transform_frame_enu_ecef(const T &in) {
Copy link
Member

Choose a reason for hiding this comment

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

same as above

@@ -64,6 +64,48 @@ using Matrix6d = Eigen::Matrix<double, 6, 6>;
using Matrix9d = Eigen::Matrix<double, 9, 9>;


Eigen::Matrix3d ecef_enu_rotation(const Eigen::Vector3d &map_origin)
Copy link
Member

Choose a reason for hiding this comment

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

You don't have to move this out: Just create the same method transform_static_frame but with a new parameter, keeping the previous:
Eigen::Vector3d transform_static_frame(const Eigen::Vector3d &vec, const Eigen::Vector3d &map_origin, const StaticTF transform)

@@ -213,6 +217,11 @@ Covariance9d transform_static_frame(const Covariance9d &cov, const StaticTF tran
}
}

Eigen::Vector3d transform_frame(const Eigen::Vector3d &vec, const Eigen::Matrix3d &R)
Copy link
Member

Choose a reason for hiding this comment

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

no need

Eigen::Vector3d map_origin {}; //!< origin of map frame
Eigen::Vector3d local_ecef {}; //!< local ECEF coordinates on map frame
Eigen::Vector3d map_origin {}; //!< geodetic origin of map frame [lla]
Eigen::Vector3d ecef_origin; //!< geocentric origin of map frame [m]
Copy link
Member

Choose a reason for hiding this comment

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

Eigen::Vector3d ecef_origin {};

/**
* @brief Conversion from geodetic coordinates (LLA) to ECEF (Earth-Centered, Earth-Fixed)
*/
GeographicLib::Geocentric map(GeographicLib::Constants::WGS84_a(),
Copy link
Member

Choose a reason for hiding this comment

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

@TSC21 TSC21 closed this Oct 31, 2017
@TSC21 TSC21 reopened this Oct 31, 2017
@TSC21
Copy link
Member

TSC21 commented Oct 31, 2017

@vooon why is Travis not launching at all?

@vooon
Copy link
Member

vooon commented Nov 1, 2017

Do not know, Travis do not list that PR at all. But we also have ROS CI, lets rely in it.

@vooon
Copy link
Member

vooon commented Nov 1, 2017

@pavloblindnology common for ftf - rotation arg after input vec. Can you please add two tests to test_frame_conversions.cpp?

@pavloblindnology
Copy link
Author

Updated according to review. Added unit tests.
Comparison was done by moving the vehicle twice (before and after changes) and comparing the paths in apm planner and rviz (global_position\local).

APM Planner path before changes
apmplanner-before

Rviz global_position\local path before changes
rviz-before

APM Planner path after changes
apmplanner-after

Rviz global_position\local path after changes
rviz-after

@pavloblindnology
Copy link
Author

Additionally fixed this copy paste bug 0c4b0f7, which is already fixed.

@@ -214,19 +222,25 @@ inline T transform_frame_baselink_aircraft(const T &in) {
/**
* @brief Transform data expressed in ECEF frame to ENU frame.
*
* in - local ECEF coordinates [m],
Copy link
Member

Choose a reason for hiding this comment

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

For Doxygen doumentation:

@param in - local ECEF coordinates [m]
@param map_origin - geodetic origin [lla]
@return local ENU coordinates [m]

}

/**
* @brief Transform data expressed in ENU frame to ECEF frame.
*
* in - local ENU coordinates [m].
Copy link
Member

Choose a reason for hiding this comment

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

For Doxygen doumentation:

@param in - local ECEF coordinates [m]
@param map_origin - geodetic origin [lla]
@return local ENU coordinates [m]

@TSC21
Copy link
Member

TSC21 commented Nov 1, 2017

@pavloblindnology almost there. Please rebase (solve conflicts) and make the changes for docs. Then, all will be good. Thanks for the fix!

Copy link
Member

@vooon vooon left a comment

Choose a reason for hiding this comment

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

LGTM.

@vooon vooon added this to the Version 0.22 milestone Nov 1, 2017
@vooon vooon merged commit 4fbf58a into mavlink:master Nov 1, 2017
@vooon vooon added the bug label Nov 1, 2017
@vooon
Copy link
Member

vooon commented Nov 1, 2017

@pavloblindnology thank you! Small fixes i done myself. And after CI done i will release, as it is significant bugfix.

@pavloblindnology
Copy link
Author

@vooon You're welcome) What is the schedule of CI for ROS?

@pavloblindnology pavloblindnology deleted the ecef2enu branch November 1, 2017 13:21
@TSC21
Copy link
Member

TSC21 commented Nov 1, 2017

@pavloblindnology it is already solved. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants