mavlink / MAVSDK

API and library for MAVLink compatible systems written in C++17
https://mavsdk.mavlink.io
BSD 3-Clause "New" or "Revised" License
606 stars 498 forks source link

REQUEST_MESSAGE vs custom getters like REQUEST_CAMERA_INFORMATION #2013

Open JonasVautherin opened 1 year ago

JonasVautherin commented 1 year ago

It's currently part of the list for MAVSDK v2, and though I don't think I can contribute an implementation, I think I can contribute a (relatively informed) opinion.

TL;DR: I don't think this can go in MAVSDK v2 :see_no_evil:.

We cannot detect what the receiver supports

There is no way to detect if the receiver supports REQUEST_MESSAGE, because the requested message could have been requested by someone else, or it could be transmitted spontaneously by the receiver. So making statistics to try to guess stuff like "was this CAMERA_INFORMATION message an answer to my request, or is it a coincidence?" is probably not worth it by far.

Also each message is independent. So even if we managed to detect that REQUEST_MESSAGE is supported for CAMERA_INFORMATION, it would not say anything about e.g. REQUEST_PROTOCOL_VERSION.

What we can do

In other words, the implementation work is not on the MAVSDK side, but on the receiver side (e.g. make sure that PX4/Ardupilot support the newer messages, make sure that cameras do, push proprietary players to do it).

Some day, maybe we will feel comfortable in dropping the old way and telling people "well if your drone does not support the new way, that's your problem".

julianoes commented 1 year ago

Thanks for bringing this up. Let's talk through it and make a decision.

My question is: Wouldn't a receiver that doesn't support REQUEST_MESSAGE nack it and therefore tell you that it doesn't support it?

JonasVautherin commented 1 year ago

Wouldn't a receiver that doesn't support REQUEST_MESSAGE nack it

I don't think so :thinking:. One problem with MAVLink is that the specs do not require to nack unsupported messages. I am pretty sure I have seen implementations that just ignore unsupported messages (resulting in a timeout, indistinguishable from an actual timeout in a sane way IMO).

So same thing there: one would have to make it a requirement to nack unsupported messages, ask implementations to change to that, and in a few years when we are fairly sure that all the implementations we care about do it, we could rely on nacks :innocent:.

julianoes commented 1 year ago

So actually UNSUPPORTED is for a command that is not supported but not necessarily for a command param that is not supported. So, in that case, it should probably be failed. I would argue a decent camera implementation would do that, rather than timing out.

So what about switching to the new REQUEST_MESSAGE command and if it gets UNSUPPORTED, FAILED or a timeout, then we fall back to the old one? This means that old implementations would still work, just slower, while new ones will work nicely, and quickly. What am I missing?

JonasVautherin commented 1 year ago

I wish it worked, and maybe I'm being too pessimistic. But I see many problems there :see_no_evil:.

I would argue a decent camera implementation would do that

My understanding of the specs is that it is legal to timeout. For instance, it is legal to broadcast the command and expect multiple systems/components to broadcast their message. I suppose it would be rational to not have all the components return UNSUPPORTED in that case. The specs clearly mention the broadcast possibility : "Request the target system(s) emit a single instance of a specified message".

What am I missing?

Say the drone only supports the new message, and does not time out for unsupported commands. Say the request happens to timeout. What do we do? If we fallback to the old message, it will always time out. Should we then fallback again to the new message? Should we somehow keep a state in MAVSDK that says "I am pretty sure that this drone supports the new message because I got an answer once"? Then we need that logic for every message we request (because the drone could use some of the new messages, and some of the old ones), and we should consider resetting that state if the drone is disconnected/reconnected (because we don't really know if it is the same drone or a different one).

On top of that, what do we do if the detection logic fails? Say MAVSDK requests a message using the new way, and receives an answer. It could totally be that it is a coincidence (the drone decided to send that message at this time/some other GCS requested it at the same time). So MAVSDK will think "oh great, it supports the new message", and maybe it does not. What do we do then?

JonasVautherin commented 1 year ago

Actually, maybe one thing we could do is always use both from MAVSDK. I think that could work :thinking:.

JonasVautherin commented 1 year ago

Actually, maybe one thing we could do is always use both from MAVSDK. I think that could work thinking.

Well thinking more about that, it is actually quite useless. Because it does not change the fact that drones should support both (on the contrary, it duplicates messages for nothing).

Again: once we decide that the supported drones all moved to the new message, it makes sense to drop the old messages in MAVSDK and use the new ones. But before that, I really don't see a solution to sanely detect what is supported.

julianoes commented 1 year ago

But I see many problems there

Yes, there are many problems. But at some point switching and saying we don't support the old way also doesn't work and also has problems. So my idea is to try hard and do a best effort solution that - while having corner cases - generally works.

For instance, it is legal to broadcast the command and expect multiple systems/components to broadcast their message.

Yes, it gets indeed tricky with multiple systems and components, so let's only use these commands to specific sysid/compid pairs and not use broadcast.

Should we somehow keep a state in MAVSDK that says "I am pretty sure that this drone supports the new message because I got an answer once"?

I think you're missing something here. A success is not if we receive the message that we asked for but we receive the ack AND the message. That's a definite confirmation that it worked.

JonasVautherin commented 1 year ago

A success is not if we receive the message that we asked for but we receive the ack

Oh, that's a good point! It is a command, so it will be acked in case of success, and therefore you can detect a success (but not all failures: a timeout is indistinguishable from an unsupported command).

So I think you're right: you can probably try both until one (or both) is acked, and when it is, keep it (or keep the new one if both are supported). Or of course if it is UNSUPPORTED then fallback to the other.

So what about switching to the new REQUEST_MESSAGE command and if it gets UNSUPPORTED, FAILED or a timeout, then we fall back to the old one?

I don't think that is entirely correct. If you have a timeout, you cannot conclude anything, so you should not fall back. I think it's better to try both messages until an answer (SUCCESS/UNSUPPORTED/FAILED) is received, rather than trying the new one for an arbitrary number of seconds and blocking everything during that time in case only the old one is supported.

A success is [when] we receive the ack AND the message

Hmm I think the ack is enough to detect that the message is supported, no need to deal with the message (which again will be a mess because the message is unreliable and can be sent for many other reasons).

IMO the code in MAVSDK should always handle those messages as "unreliable, spontaneous messages", but MAVSDK can request them separately if it wants to encourage the drone to send it. I.e. don't write logic that says "send the REQUEST, do something with the response, and in the same logic do something with the message and wrap that whole thing to look like a reliable request/response". If that makes sense?

julianoes commented 1 year ago

Yes, exactly!

julianoes commented 2 months ago

In v3 I want to remove the MAV_CMD_REQUEST_AUTOPILOT_CAPABILITIES.