Incorrect units of variance in GPS topic

[mhkabir]

Couple of issues and concerns which I had while upstreaming the GPS-VIO interface which will also allow an offboard estimator to use the GPS accuracy information:

1 . This is from the vehicle_gps_position topic :

float32 s_variance_m_s # GPS speed accuracy estimate, (metres/sec)
float32 c_variance_rad # GPS course accuracy estimate, (radians)

For velocity, variance should be in (m/s)^2 and for course, in rad^2. The comment and field name are both incorrect, and it is not understandable whether we want variance or standard deviation here.

1. From the mavlink message GPS_INPUT :

speed_accuracy float GPS speed accuracy in m/s
horiz_accuracy float GPS horizontal accuracy in m
vert_accuracy float GPS vertical accuracy in m

The units suggest that these are standard deviations, however field spec is not clear from the word "accuracy". Judging from other messages with better docs e.g WIND_COV (horiz_accuracy float Horizontal speed 1-STD accuracy), it means sqrt variance.

1. Is there an objection to switching the vehicle_gps_position topic to covariances only (pos_cov and vel_cov), and derive the other uncertainty fields where needed? Seems kinda unnecessary to transport hdop, vdop, epv, eph, s_variance_m_s, c_variance_rad, etc. Also, modern estimators and any cascaded setup will benefit from the covariance availability. @LorenzMeier

I will send in a PR with the fixes and the new interface when as my concerns are confirmed by another pair of eyes.

[bkueng]

I agree it should be more precise & consistent. s_variance_m_s & c_variance_rad come directly from the ublox gps. The protocol spec just says it's the 'Speed accuracy estimate' in [m/s] and 'Heading accuracy estimate (both motion and vehicle)' in [rad]. So it should be standard deviation.

[mhkabir]

@bkueng What fix would you recommend here? Renaming the field and comment to be more precise?

Or as I think is probably much better - go directly to my third suggestion :

Is there an objection to switching the vehicle_gps_position topic to covariances only (pos_cov and vel_cov), and derive the other uncertainty fields where needed? Seems kinda unnecessary to transport hdop, vdop, epv, eph, s_variance_m_s, c_variance_rad, etc. Also, modern estimators and any cascaded setup will benefit from the covariance availability.

e.g the UAVCAN GPS driver already has all the covariance information, but it is not utilized fully: https://github.com/PX4/Firmware/blob/master/src/modules/uavcan/sensors/gnss.cpp#L119-L120

[bkueng]

Yes renaming and commenting is a first step. Don't know if covariances make sense as it would add values if you have a whole covariance matrix. Most of the code seems to use eph & epv (and so do the log analysis tools), and not vdop & hdop. One usage however is here: https://github.com/PX4/Firmware/blob/master/src/modules/mavlink/mavlink_messages.cpp#L1103. Very confusing, but it seems to be really hdop, as the mavlink field doc says 'GPS HDOP horizontal dilution of position (unitless)'.

[mhkabir]

@LorenzMeier Comments?

[julianoes]

@bkueng the GPS mavlink message just has the field name wrong but it is hdop according to the spec http://mavlink.org/messages/common#GPS_RAW_INT.

[mhkabir]

@julianoes Actually - I remember that the mavlink spec was changed to make it DOPs at some point, because we wanted QGC to display the HDOP/VDOP rather than the EPH/EPV.
The original message spec had EPH/EPV. We just kept using the fieldnames to maintain compatibility.

[mhkabir]

@bkueng @julianoes I have made the necessary changes here: https://github.com/mhkabir/Firmware/tree/gps_refactor

Most important changes in the GPS topic: 2971a69

Please review, and let me know if you want anything to be done differently.

[PX4BuildBot]

Hey, this issue has been closed because the label status/STALE is set and there were no updates for 30 days. Feel free to reopen this issue if you deem it appropriate.

(This is an automated comment from GitMate.io.)

[dagar]

@mhkabir did you want to open a PR for this?

[pavel-kirienko]

Kabir vs. the Bot -- who would win?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[julianoes]

I hope we don't lose against these bots!

[bresch]

@mhkabir can you continue that? I had the same confusion between variance and std dev of the speed accuracy today.

[priseborough]

The value currently being passed through for the UBlox receivers is a speed accuracy with units of m/s, not a variance with units of (m/s)**2 and the EKF and other consumers are using it as such. The use of 'variance' in the name is misleading, however the description is correct and the name contains the units. If we either change name or units, then we will break replay of logs gathered using the legacy topic definition. I think what we should do here is add a note above the s_variance_m_s and c_variance_rad variables to make it clear.

[mhkabir]

What if we create a new sensor_gps topic with correct everything and support the old one in parallel for one or two releases?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.