-
Notifications
You must be signed in to change notification settings - Fork 21.4k
GCS_MAVLink: utilize VISION_SPEED_ESTIMATE covariance #32478
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4068,7 +4068,7 @@ void GCS_MAVLINK::handle_odometry(const mavlink_message_t &msg) | |
| // convert velocity vector from FRD to NED frame | ||
| Vector3f vel{m.vx, m.vy, m.vz}; | ||
| vel = q * vel; | ||
| visual_odom->handle_vision_speed_estimate(m.time_usec, timestamp_ms, vel, m.reset_counter, m.quality); | ||
| visual_odom->handle_vision_speed_estimate(m.time_usec, timestamp_ms, vel, 0, m.reset_counter, m.quality); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 0 seems like a poor choice as a default value to send in as an error. If you don't know then NaN would be a rather better choice.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not a fan of sending NaNs around internally to indicate that something is unknown or unused. Normally we do use zero. We should add a comment though to the handle_vision_speed_estimate function though similar towhat we have for quality, "// quality of -1 means failed, 0 means unknown, 1 is worst, 100 is best"
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @peterbarker and @rmackay9, thank you for the review and suggestions. I have added an explanation for the newly added parameter. |
||
| } | ||
|
|
||
| // there are several messages which all have identical fields in them. | ||
|
|
@@ -4137,7 +4137,11 @@ void GCS_MAVLINK::handle_vision_speed_estimate(const mavlink_message_t &msg) | |
| mavlink_msg_vision_speed_estimate_decode(&msg, &m); | ||
| const Vector3f vel = {m.x, m.y, m.z}; | ||
| uint32_t timestamp_ms = correct_offboard_timestamp_usec_to_ms(m.usec, PAYLOAD_SIZE(chan, VISION_SPEED_ESTIMATE)); | ||
| visual_odom->handle_vision_speed_estimate(m.usec, timestamp_ms, vel, m.reset_counter, 0); | ||
| float vel_err = 0; | ||
| if (!isnan(m.covariance[0])) { | ||
| vel_err = sqrtf(m.covariance[0]+m.covariance[4]+m.covariance[8]); | ||
| } | ||
| visual_odom->handle_vision_speed_estimate(m.usec, timestamp_ms, vel, vel_err, m.reset_counter, 0); | ||
| } | ||
| #endif // HAL_VISUALODOM_ENABLED | ||
|
|
||
|
|
||
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.
Hi @chobitsfan, thanks again for this. I see we've added a new upper constraint of "5" here. That is a lot of error so perhaps the EKF will just ignore the velocity (which would be good with that high an error) but what do you think? The VISO_VEL_M_NSE parameter also has a suggested upper limit of 5 but it's not strictly enforced so we do have a small change in behaviour here
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.
Hi @rmackay9 Thank you for reviewing. As you mentioned, the upper constraint of vel_err at 5 is based on the suggested upper limit of the VISO_VEL_M_NSE parameter. I constrain vel_err to 5 because visual odometry may report very large standard deviations when it becomes unreliable or drifts. Alternatively, should we use vel_err = fmaxf(_frontend.get_vel_noise(), vel_err), similar to how we handle GPS speed accuracy? Thank you.
Uh oh!
There was an error while loading. Please reload this page.
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.
Hi @chobitsfan,
Yes, I think we should use the MAX of the two and remove the upper limit of "5" unless we have a specific reason to change it.
So you mention that the external visual odometry system reports very large errors at times, does that cause problems? Do you find the EKF should be consuming the velocity but doesn't? Of course the external system could apply an upper limit but I'm not sure which is best.
Maybe @priseborough has an opinion
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.
Hi @rmackay9, Thank you for suggestions
No, not so far. When visual odometry reports very large errors, it indicates an unstable state, and the EKF should not consume its data.
Uh oh!
There was an error while loading. Please reload this page.
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.
Hi @chobitsfan,
OK, so if we want to protect against that then instead of constraining the error we should perhaps add a VISO_VEL_ERR_MAX parameter (a bit similar to the VISO_QUAL_MIN param) and then simply throw away the velocity if the error is over this value
This doesn't need to be done as part of this PR though
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.
Hi @rmackay9
Two test flight logs are included using the latest modifications to vel_err. In both flights, visual odometry became unstable, reported large errors, and eventually triggered the EKF failsafe.
2026-03-19 14-28-31.zip
Uh oh!
There was an error while loading. Please reload this page.
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.
Hi @chobitsfan, but with the previous code of clamping the vel error at 5 everything was OK?
I wonder if this means that the EKF loses confidence in its velocity estimate just because the incoming velocity value has come in with a high error value attached to it. I wonder if the EKF would actually be happier if it didn't get that velocity reading at all and was instead forced to estimate it from the position and acceleration values it's getting.
An expert like @priseborough might know the answer off the top of his head
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.
Hi @rmackay9 Sorry for the late reply. (the notification email was mixed in with PR merge notifications, so I missed it.)
I think this is already handled by the quality field. It is the responsibility of the visual odometry source to report the quality of its estimate. Unfortunately, the quality field is only supported in the Odometry message. I may open a PR in the MAVLink repository to add it to VISION_SPEED_ESTIMATE.
ardupilot/libraries/AP_VisualOdom/AP_VisualOdom_MAV.cpp
Lines 77 to 81 in 2d14165