UDP connection can disconnect when heartbeats are not received

[jperez-droneup]

Creating a UDP connection to a vehicle/simulator via setup_udp_remote or add_any_connection will set the always_connected flag to true in all cases. This means that even though MAVSDK could be sending heartbeats (configured in the Mavsdk::Configuration::_always_send_heartbeats), the lack of response on those heartbeats does not lead to realizing/announcing the disconnect. With this change, if the Mavsdk will properly use heartbeats to detect connection and disconnection if it is configured to send those heartbeats. This means that the system will not be considered connected (and fire the subscribe_on_new_system callback) until the first heartbeat response is received, and will be considered disconnected (and fire the subscribe_is_connected callback) when the configured amount of time passes without a heartbeat response.

[JonasVautherin]

Hmm does that work? It seems weird to set always_connected to !always_send_heartbeats, doesn't it?

And when always_connected is set to false, is the system still sending heartbeats even if nobody is answering? I thought it was the point of that flag 🤔

[jperez-droneup]

Hmm does that work? It seems weird to set always_connected to !always_send_heartbeats, doesn't it?

The thought is that if MAVSDK is sending heartbeats, then we should not be considered as always connected. The connection status should depend upon the status of the heartbeats. If MAVSDK is not sending heartbeats, then we have to assume the client either doesn't care about the connection or are implementing their own connection logic; in either case, MAVSDK should act as if it is always connected.

And when always_connected is set to false, is the system still sending heartbeats even if nobody is answering? I thought it was the point of that flag 🤔

The point of the always_connected flag is to determine whether or not to register a timeout for the heartbeats. Here are the possibilities:
always_connected=true and always_send_heartbeats=true: heartbeats are sent, but connection status is not updated if heartbeats are missed, because it is considered always connected. So here the connection status callback is not called.
always_connected=true and always_send_heartbeats=false: heartbeats are not sent, connection is considered always connected. in this scenario we have no inherent connection status checking and so no connection callback.
always_connected=false and always_send_heartbeats=true: heartbeats are sent, and once heartbeats are missed, a disconnect is recognized and the connection status callback is fired. This is the desired case I am looking for.
always_connected=false and always_send_heartbeats=false: heartbeats are not sent, but an external heartbeat is required to consider the system connected and the external heartbeat will need to continue to poll or MAVSDK will recognize it as a disconnect.

[jperez-droneup]

Another solution is to pass down the always_connected value the user wants, all the way down from add_any_connection, and default it to true for backward compatibility. Though this would also require making sure it works for serial and TCP, which I have not investigated. To make that even simpler, only add the argument to add_udp_connection.

I am of the opinion that if MAVSDK is sending heartbeats, then it should also monitor and broadcast (via callbacks) the status of the connection based on those heartbeats, and so it should be automatic as I have written it. Though I would accept either solution.

[julianoes]

Thanks @jperez-droneup!

@JonasVautherin I wonder why we need that _always_connected flag at all? It looks like it was added in #774 presumably because of the problems trying to create an FTP client and server which wasn't really properly supported. It is now with System vs Component, and the FTP plugin is split in #2060.

I'm gonna try to make a PR where I just remove that flag alltogether and see where it leads.

[julianoes]

This is my alternative fix: #2077

@jperez-droneup it would be great if you could test this one to check whether it works for you.

[jperez-droneup]

This is my alternative fix: #2077

@jperez-droneup it would be great if you could test this one to check whether it works for you.

Your fix does indeed fix the issue, so the connection status change callback is properly firing. However, I am still seeing the issue from case1 of #2075, just FYI.

[JonasVautherin]

However, I am still seeing the issue from case1 of #2075, just FYI.

Just to be sure I understand correctly. Case one of #2075 is not fixed by this PR (#2074), is it? In other words, does that mean that this (#2074) could be closed in favor of #2077, but issue #2075 is still open because of its case 1?

[jperez-droneup]

However, I am still seeing the issue from case1 of #2075, just FYI.

Just to be sure I understand correctly. Case one of #2075 is not fixed by this PR (#2074), is it? In other words, does that mean that this (#2074) could be closed in favor of #2077, but issue #2075 is still open because of its case 1?

That is correct. I would rather you just remove the always_connected flag altogether than my approach to work around it. And yes #2075 is still present in case1.

[julianoes]

#2075 is "user error" as I understand it, or at least I wouldn't know how to avoid it. So that's where I suggest #2078.

[julianoes]

Thanks for the PR but I'm closing it because I prefer removing the _always_connected flag fully, which was done in #2077.

[JonasVautherin]

Yeah thanks a lot @jperez-droneup! The whole discussion and your insights were very useful!