Mavsdk: Who should initiate TIMESYNC?

[JonasVautherin]

This is related to #1169. Looking at MAVSDK's timesync implementation, it seems like we send TIMESYNC messages in two situations:

1. In process_timesync, where we receive a TIMESYNC message and answer to it
2. In do_work, where we actually initiate the TIMESYNC exchange

Is it meant to be like this, or do different components have different needs? E.g. the "master" (I would guess the autopilot) could be the one initiating the sync, and he other components may or may not answer depending on their needs, right?

@irsdkv: your insights would be appreciated :slightly_smiling_face:!

[AlexisTM]

This is the same implementation as in MAVROS:

For the FCU to sync, the FCU sends a message and waits for the onboard computer to answer.
For the onboard computer to sync, it sends a message and waits for the FCU's answer.

This could be in tri-way protocol two-way, where the onboard computer sends a request, FCU replies and the onboard answers to the FCU. The onboard computer will have its sync from the 2 first messages, the FCU from the 3 last messages. This would remove one message but add one time to the TIMESYNC message. I do not know it would improve bandwidth.
Mavros implementation:

[Replies]
https://github.com/mavlink/mavros/blob/de9f39a719b091b8448214a17d27b3b1c415d0dc/mavros/src/plugins/sys_time.cpp#L308-L321

[Requests]
https://github.com/mavlink/mavros/blob/de9f39a719b091b8448214a17d27b3b1c415d0dc/mavros/src/plugins/sys_time.cpp#L334-L346

Precision Time Protocol 2: [NOT PX4's version]

[irsdkv]

Oh it was a long time ago and unfortunately I don't remember all important details now :(

But @AlexisTM explanation is correct.

Also I would to say that this code change disables ability to answer to TIMESYNC requests from other devices in common with disabling MAVSDK's own requests.

I think that it is a good idea to save the opportunity to sync time by default in case of other device want to do it.

What are you think about that, @AlexisTM ?

UPD:
Also I think that disabling by default MAVSDK's initiated timesync is not a bad idea, because it is needed only for positioning using external vision for now as I know.

[AlexisTM]

I am personally not using MAVSDK at the moment.

But indeed, it would make sense to have it disabled by default as only experienced users make use offboard control and need Timesync.
If the user uses advanced functions, I would either auto-enable timesync or give a feedback that timesync is not enabled and should for advanced usage. (advanced usage would be offboard control or external vision)

[irsdkv]

I checked out #1034 and found that it is only way to disable do_work and process_timesync both simultaneously.

And as @AlexisTM said this functionality really needed only for advanced usage.

So this PR seems like a good choice.

But @JonasVautherin please say why this is a problem to save this functionality enabled by default? Is it add payload or brings some other troubles?

[JonasVautherin]

please say why this is a problem to save this functionality enabled by default?

Because it's not clear to me how TIMESYNC supports more than 2 components doing the synchronization. Hence, if you have more than 2 components, I'm currently assuming that you will have to disable TIMESYNC in the extra ones, and you will have to know which one needs to enable it. In that case, it seems more legit to have it disabled by default IMO.

My reasoning is the following: the TIMESYNC message is a broadcast message (because it doesn't have target_system and target_component). Therefore, a component cannot know who the message was destined to. So if you have, say, the FCU synchronizing with a module M1 on the companion, and at the same time a module M2 on the companion synchronizing with the ground station, then I can't imagine how the FCU can make the difference between the response coming from M1 (which it is interested in) and the response coming from the ground station (which is destined to M2).

[AlexisTM]

@JonasVautherin I fully agree, this protocol was designed out of necessity for a time synchronization between two devices to allow better control.

I believe it is useless in most of the use-cases and instead will be a problem to the user if multiple endpoints are connected.

TIMESYNC only works if:

• There are only two devices
• The synchronization time is computed & filtered in each receiving end (the pixhawk for vision estimation input & the computer for position output)
• The link should be high speed with low jitter, probably only using hardware serial (USB-Serial has up to 20ms spikes)

[hamishwillee]

@AlexisTM Yes. There is a little discussion of this message here: https://github.com/mavlink/mavlink-devguide/issues/194#issuecomment-512037079

And one day I will find time to document it.

[julianoes]

I would vote that we make this feature opt-in for use cases where it is required, and supported.

[JonasVautherin]

Ok, that answers my question, and TIMESYNC is now disabled by default in MAVSDK. Closing this.