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

Frame conversions #317

Merged
merged 5 commits into from
Jun 28, 2015
Merged

Frame conversions #317

merged 5 commits into from
Jun 28, 2015

Conversation

TSC21
Copy link
Member

@TSC21 TSC21 commented Jun 26, 2015

Finally, after some tinkering with @BlackCoder, we were able to get to the proper conversions to position and orientation between ROS ENU and PX4 NED frames.
So, as this is a common code for many plugins, I decided to create a specific lib file where all the functions for frame conversions are installed. This functions are called by the plugins to do the conversions online.
ADD: This PR also updates some comments and doxygen documentation on plugins.

Right now, I think I'm just missing:

  • Add covariance frame conversion to uas_frame_conversions.cpp (partially)

Update: Relative to covariance conversions, I'm still missing:

  • Conversion to 7x7 full pose covariance matrix (attitude in quaternions instead of RPY)
  • Conversion to quaternion covariance only (based on the previous)
  • Note that transform_frame_covariance_pose6x6() can also be used to other covariances matrices, (p.e., lin_vel + RPY, ang_vel+RPY, and why not position+lin_vel since the conversion is the same - not very used)

Update2: I will leave the above covariances conversions to another PR.

This PR Fix #194, #148 (please continue list).

return tf::Vector3(x, y, z);
};

static tf::Vector3 convert_velocity(float _vx, float _vy, float _vz){
Copy link
Contributor

Choose a reason for hiding this comment

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

Why have different functions that perform the same conversion?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just naming types

@TSC21 TSC21 force-pushed the frame_conversions branch 7 times, most recently from 6354923 to 8cd9fa7 Compare June 26, 2015 23:00
@mhkabir
Copy link
Member

mhkabir commented Jun 27, 2015

I suggest that the naming should be changed to "transform_frame" and no need to make 5x copies of the same function, since we have the exact same conversion for all data from NED to ENU and back.
@TSC21 @vooon @tonybaltovski

@TSC21
Copy link
Member Author

TSC21 commented Jun 27, 2015

I suggest that the naming should be changed to "transform_frame"

Ok why not

5x copies of the same function

This was just a naming convention, as we also have functions to convert attitude (rpy and q). Like this we can name different functions for different physical variables (position, velocity, acceleration, ...). If not, how to call it? I created a general_xyz which is being used for any case that doesn't fit into the previous one.

@vooon
Copy link
Member

vooon commented Jun 27, 2015

I don't see why you really need different function for each type. And if you use same conversion do one general func and call it in inlined pos, att etc. funcs.

Also as i see, there no more difference between body and local?
Conversion is same, but specialized ned-enu, enu-ned suffixes more readable, and describe what we want to do.

@TSC21
Copy link
Member Author

TSC21 commented Jun 27, 2015

Ok i'll launch a general conversion function then. But as you say that there's no need to multiple functions to different types, i don't see why we have to say the direction since the conversion is exactly the same. The direction is marked on comments as you can see.

From flight tests done by @BlackCoder, it seems that there's no need to differentiate local from body frames. Mocap and vision plugins already flight tested. Just have to be sure that imu plugin data corresponds to what is expect (which it should since the rotation matrix values correspond to the previous transformations applied to pitch and yaw values). As to ENU to NED conversions used on plugins, it's all good to go, since it was tested and confirmed using a mocap system.

@TSC21 TSC21 force-pushed the frame_conversions branch 5 times, most recently from 37d325e to 41f22ee Compare June 27, 2015 20:14
@vooon
Copy link
Member

vooon commented Jun 27, 2015

Best documentation is the code itself. Commenting each entry worse, than two inlined ned_enu, enu_ned variants.
Anyway compiller optimize to direct call underlying func.

@vooon
Copy link
Member

vooon commented Jun 27, 2015

And i see, that there difference in general and rpy. Why?

@vooon
Copy link
Member

vooon commented Jun 27, 2015

Also a note, that those functions are best target for unit tests. Now test exist only for libmavconn, because most plugins require real FCU (at least SITL), write test for them little complex.

@TSC21
Copy link
Member Author

TSC21 commented Jun 27, 2015

Best documentation is the code itself. Commenting each entry worse, than two inlined ned_enu, enu_ned variants.

I still can't find a use to tell the direction on the function name, as the conversion is exactly the same.

@vooon
Copy link
Member

vooon commented Jun 28, 2015

Where did you find 7x7 covariance? geometry_msgs/PoseWithCovariance uses 6x6.

@TSC21
Copy link
Member Author

TSC21 commented Jun 28, 2015

Where did you find 7x7 covariance? geometry_msgs/PoseWithCovariance uses 6x6.

Well 7x7 pose matrices exists and can be used. Check MRPT: http://www.mrpt.org/tutorials/programming/maths-and-geometry/2d_3d_geometry/#4_Conversions_between_classes.

Quaternion covariance also exists. Check page 7 of this paper.

So I assume a 7x7 covariance matrix can be built, even though ROS doesn't support it yet, but many applications may depend on a covariance matrix like this. The problem is that the propagation of covariance in quaternion representation is not so trivial.

@@ -207,13 +206,12 @@ class IMUPubPlugin : public MavRosPlugin {

auto imu_msg = boost::make_shared<sensor_msgs::Imu>();

// PX4 NED (w x y z) -> ROS ENU (x -y -z w) (body-fixed)
tf::Quaternion orientation(att_q.q2, -att_q.q3, -att_q.q4, att_q.q1);
tf::Quaternion orientation(att_q.q1, att_q.q2, att_q.q3, att_q.q4);
Copy link
Member

Choose a reason for hiding this comment

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

NO! You missed that FCU and ROS use different field convention.
It still should be orientation(q2, q3, q4, q1).

Copy link
Member Author

Choose a reason for hiding this comment

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

FCU: w x y z
ROS: x y z w
right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

@TSC21 TSC21 mentioned this pull request Jun 28, 2015
magn_msg->magnetic_field.x = imu_hr.ymag * GAUSS_TO_TESLA;
magn_msg->magnetic_field.y = imu_hr.xmag * GAUSS_TO_TESLA;
magn_msg->magnetic_field.z = -imu_hr.zmag * GAUSS_TO_TESLA;
auto mag_field = UAS::transform_frame_ned_enu_xyz(imu_hr.xmag, imu_hr.ymag, imu_hr.zmag);
Copy link
Member

Choose a reason for hiding this comment

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

There bee GAUSS units, not milliTESLA.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it

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.

4 participants