-
Notifications
You must be signed in to change notification settings - Fork 993
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
WIP: Add odometry message to local position plugin & coordinate frame review #473
Conversation
To address the first point of this I have created the following document. @vooon, @LorenzMeier, @mhkabir. Input would be appreciated |
https://github.com/mavlink/mavros/blob/master/mavros/src/lib/uas_frame_conversions.cpp#L24 Would it not be sufficient to just change static const Eigen::Quaterniond FRAME_ROTATE_Q = UAS::quaternion_from_rpy(M_PI, 0.0, 0.0); to static const Eigen::Quaterniond FRAME_ROTATE_Q = UAS::quaternion_from_rpy(M_PI, 0.0, M_PI_2); ? |
@andre-nguyen this change would be sufficient for correcting the change from the ENU to NED frame. However, the reluctance to change this is because some messages (e.g. ones for IMU data) are actually expressed in the body frame not the NED frame as @mhkabir pointed out in #438. If nothing else we need to have a body frame -> ENU frame and NED -> ENU frame conversion. But by making the FRAME_ROTATE_Q variable an enum we will be able to define an arbitrary number of useful rotations such as NED -> body frame or ENU -> custom frame. Also by making the transform depend on an enumeration if a specific transform was found to have a bug in it, then the entire API wouldn't need to be re-worked, just that specific rotations. |
Good point |
One limitation I'm running into is that FRAME_ROTATE_Q by nature has to be a static rotation. This makes the NED -> body frame transformation not fit into the scope of the frame transformation lib as the quaternion from NED -> body, and for that matter ENU -> body, will always be changing. I don't know how best to approach that piece, I think that the lib should remain a library of strict frame to frame definitions and that each plugin can convert things into the body frame as necessary. |
|
||
//! Transform for vector3 | ||
static const Eigen::Transform<double, 3, Eigen::Affine> FRAME_TRANSFORM_VECTOR3(FRAME_ROTATE_Q); | ||
//static const Eigen::Transform<double, 3, Eigen::Affine> FRAME_TRANSFORM_VECTOR3(FRAME_ROTATE_Q); |
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.
It is ok for research, but later i think better to make two consts: FRAME_ROTATE_LOCAL_Q
, FRAME_ROTATE_PLATFORM_Q
.
Also platform
? Not base
?
In light of the need of dynamic rotations (see discussion about @voon's 4th comment) I've implemented new functions for body->enu and body-> ned frames. |
Check REP103, at least IMU should be reported in ros-body frame. And for that static rotation around Roll (X) is good enough. And here we may use TF, define several transforms: Static:
Dynamic:
Then later anyone may check applied conversions in TF tree. But that approach cost in CPU usage than internal Eigen calculations. |
@scott-eddy Looks to me that @vooon suggestion is good :) |
Sounds good to add the rotation from fcu_body to ros_body. Today I'll walk through and clean up all of the naming conventions and try to document it somewhere for clarity. I'm still a little hazy on the desired implementation. Are we going to:
2 would be more in line with how things are done now, and since CPU is generally limited on companion computer's I would vote we do it that way. |
Perhaps 2-option is more effecient (since we do not need to resolve TF on each iteration), but if you can please add switch for static/tf selection. |
With your latest push, what's the "aircraft" frame and the "base_link" frame referring to? |
@mhkabir base_link is the body-fixed frame expressed in ROS coordinates adhering to REP 105 and REP 103. The "aircraft" frame is the PX4 body-fixed frame. I wanted to avoid calling one frame body and one base_link as both are rigidly attached to the vehicle. "aircraft" was chosen because the body-fixed frame for aircraft is usually defined with Z Down. Open to other naming conventions but it seems to make sense. Obviously when all is said and done documentation needs to be developed, we should decide where to house it and how to inform other programs (e.g. PX4, Arducopter) |
Testing with 4a591db looks good. I ran some tests with the aircraft on the bench with the following:
This yields the following acceleration profile: While not intuitive, this is the correct rotation of the reported accelerations. I would have expected that when on wheels, one g would show up in the base_link frame as -9.8[m/s/s] as the g is aligned with the down direction. However, as explained to me in PX4/Firmware #3551 this is not the correct way to think about accelerometer measurements. The bench test also provided the following angular rate profile: These rates are correct for the base_link frame:
|
@scott-eddy Could you check the following accelerations quickly in the IMU data output (
|
@mhkabir those are the accelerations measured with this. See above plot. |
Tested today that the orientation transformation from NED->aircraft to ENU->base_link is correct. After that I confirmed that rotating the reported NED velocities to the base_link frame results in accurate body-frame velocities. I shook the vehicle in the x axis while rotating it about the Z, then shook in the Y while rotating back the other way about the Z. I repeated this as follows: |
* (e.g. transfrom attitude from representing from base_link -> NED | ||
to representing base_link -> ENU) | ||
|
||
* General function. Please use specialized enu-ned and ned-enu variants. |
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.
Break of *
"wall" looks ugly, but i may fix that later.
Also check please |
That would be imu_pub for me, I think. Or maybe vision_pose. Only those two are active for me.
|
@mhkabir do you get the error after running catkin clean --all and rebuilding. imu_pub doesn't produce errors on my end so if you can get it repeatably then it must be in vision_pose |
@scott-eddy Yep, vision_pose. I'm listening to a TF from my VIO system, note, not the normal PoseStamped topic. You might want to check that code path. |
@mhkabir The only place that I can see an Eigen::Quaternion within vision pose is here. However, the function is transform_orientation which should be okay. @tonybaltovski doesn't Eigen::Quaternion<double, 0> define a quaternion with dimension = 0? Perhaps that contributes to the problem. |
CI failed. |
Fixed ROS_BREAK, did not have to |
@scott-eddy @mhkabir seems most problems are fixed, so what we need is testing? |
TODO: update README. |
@mhkabir I tested this with vision_pose running, sending a geometry_msgs/PoseStamped from rqt at a rate of 10Hz with no problem. I can't seem to duplicate the bug you have seen |
@mhkabir can you send me a gist of your config and pluginlist files |
WIP: Add odometry message to local position plugin & coordinate frame review
On mavros 0.18.4 and PX4 1.5.1. I am sending a "almost null yaw" (as I am at 0° rotation) quaternion : |
This PR will add a nav_msgs/Odometry output to the local position plugin, solving #470 directly. Additionally #438 will be addressed by this PR with an in-depth analysis of the current coordinate frames used, transformations between frames, and making these frames as REP 105 and REP 103 compliant with minimal impact on the current user base (e.g. with changing as few frame names as possible).
Coordinate Frame Examination
Odometry Message Publication
Documentation
Here is a discussion of the proposed changes and why they are being made.
To help facilitate ensuring frames are properly rotated I've begun tabulating each plugin's data and what frame it is represented in on the FCU. Not being and Arducopter developer I will not be diving into frame representations on that firmware stack but have included it anyway if someone wants to take the lead on it.