-
Notifications
You must be signed in to change notification settings - Fork 988
Adding publishing of an REP-105 compliant ECEF transform #948
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
base: rolling-devel
Are you sure you want to change the base?
Adding publishing of an REP-105 compliant ECEF transform #948
Conversation
|
I should disclosed that AI was used in the creation of this patch... although, frankly, I don't know how much credit I want to give it; it did a pretty bad job at figuring out the transforms and I had to help it a lot. |
|
Also, as far as testing, I actually did the work on Jazzy and tested there. If/when this review is accepted, I will also put up for review the push to the jazzy and kilted branches. |
532108f to
86c9ecc
Compare
ayrton04
left a comment
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.
This looks really great, thank you! I haven't had the cycles to validate the math, so I'm going to have to trust that it's functional. Assuming this works, I'd be a big fan of deprecating anything related to UTM.
src/navsat_transform.cpp
Outdated
| tf2::Transform E2C(R_ecef_to_enu, translated_origin); | ||
|
|
||
| // Store the transform (earth -> local_enu) | ||
| earth_cartesian_transform_ = E2C; |
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.
Instead of making this method void, can we make it return a transform? You could even then make the method static, which should simplify testing.
earth_cartesian_transform_ = computeEarthToCartesian(origin_latitude_, origin_longitude_, origin_altitude_);
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.
made this change, and added a few trivial unit tests to exercise it
The navsat_transform_node can now publish an ecef earth frame. This is controlled using the parameters 'broadcast_earth_transform' and 'earth_frame_id'. Publishing the earth transform is only valid if use_local_cartesian is set to true.
- Remove superfluous blank lines in navsat_transform.hpp - Refactor computeEarthToCartesian to free function returning tf2::Transform - Add unit tests for computeEarthToCartesian: - Origin maps to zero in local frame - Rotation matrix is orthonormal - Rotation matrix determinant is 1 - Correct axis directions at equator/prime meridian - Correct behavior at north pole - Axis directions at equator with 90° longitude
d3632da to
859c490
Compare
|
This is certainly an interesting addition. I still think ECEF is a bit underused in ROS and I'd like to see it used more! However, it rather seems to me the overall approach to TF "outputs" of navsat_transform node could be changed. Currently (without this PR), there are options Adding another parameter that even limits the possible values of some of these seems to be increasing confusion. I suggest restructuring this to something like this list of parameters:
Of course, from each pair of inverse parameters, only one could be set. Many of these conversions are implemented in https://github.com/gazebosim/gz-math/blob/gz-math9/include/gz/math/SphericalCoordinates.hh . With the addition of gz_math_vendor to Jazzy+, this could simplify some of these computations. UTM is not covered, however. As a side note, when digging into this, I always had this idea of having a many-frames UTM representation. So the UTM frame published by this node could actually be named |
|
That looks well thought through! Ideally we can choose to force a UTM frame instead of reparenting. For UTM it's ok to transform if you're a few kilometers over a UTM border. A lot of robotic applications run the risk of operating across a border. But most will be within some kilometer range. In that case it's easier to stick to one UTM frame instead of flipping everytime you cross the border. |
|
I definitely don't suggest disabling sticky UTM zone. It's very practical and should still be configurable. |
The navsat_transform_node can now publish an ecef earth frame.
This functionality can be enabled by setting 'broadcast_earth_transform' to true. The frame ID used for the earth transform can be customized using the
earth_frame_idparameter.Note, that publishing the earth transform is only valid if use_local_cartesian is set to true.