Incorrect Timestamp Handling in GCS_MAVLINK::handle_vision_speed_estimate

[obicons]

Bug report

In GCS_MAVLINK::handle_vision_speed_estimate, correct_offboard_timestamp_usec_to_ms is called. I think that this is incorrect, because it does not properly handle the case when the timestamp is relative to time since system boot, which is allowed by the MAVLink specification. I think that we can fix this by looking at the magnitude of the timestamp to decide if we should convert to UNIX time.

Issue details

See above.

Version

Master branch on Github.

Platform [ * ] All [ ] AntennaTracker [ ] Copter [ ] Plane [ ] Rover [ ] Submarine

Airframe type Any airframe.

Hardware type Any hardware.

Logs N/A

[rmackay9]

@obicons,

I think the "correct_offboard_timestamp_usec_to_ms" will handle the difference in time between the companion computer and autopilot regardless of what base time the companion computer system is using. The companion computer time could be anything and I think AP will figure it out.

If you could provide more specific example of where the existing code is doing the wrong thing and this is causing problems that would help better understand what the fix should be.

[obicons]

@rmackay9 Thanks for your quick response! I think that are right re: correct_offboard_timestamp_usec_to_ms correcting regardless of the base time, if you assume the companion computer uses a fixed base time. I will validate this to confirm.

But is it correct to assume every message will use a timestamp in the same format? In the case of heterogenous timestamps, I think the behavior of correct_offboard_timestamp_usec_to_ms will be incorrect. I believe mixing timestamps is allowed by the spec, so a companion computer can cause issues while it is behaving correctly.

I'll work out an example.

[obicons]

@rmackay9 My apologies for the delay getting back to you. The behavior of correct_offboard_timestamp_usec_to_ms limits the maximum lag of a corrected timestamp. The time returned by correct_offboard_timestamp_usec_to_ms is always "close" to the autopilot's time, even when flip-flopping between timestamp formats occurs.

I noticed a bigger issue when I traced the path of the timestamp returned from correct_offboard_timestamp_usec_to_ms. The value of usec (the unprocessed remote time) in handle_common_vision_position_estimate_data is ignored (it's logged in one spot, but it doesn't change the vehicle's behavior.) After some experimentation, and given the implementation of correct_offboard_timestamp_usec_to_ms, ArduPilot behaves the same if we just throw away usec and use the autopilot's time. I confirmed this with two experiments: (1) I replaced usec with the current system time and (2) I supplied random values as the timestamp. When I traced the code even further, I found that the EKF implementations don't make much use of the corrected time.

To summarize:

• The timestamp provided in the message doesn't matter. We can supply random numbers as timestamps and the autopilot behaves the same.
• I think this is dangerous because (1) companion computers can send stale data with timestamps that indicate staleness, but ArduPilot consumes the data anyway; and (2) communication latency can exceed the limit of 500ms.

I have some ideas about fixes, but I'll stop here and listen if this is something concerning to you or not :)

Thanks for your time & efforts developing ArduPilot.