Skip to content
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

REQUEST_MESSAGE vs custom getters like REQUEST_CAMERA_INFORMATION #2013

Closed
JonasVautherin opened this issue Mar 31, 2023 · 10 comments · Fixed by #2381
Closed

REQUEST_MESSAGE vs custom getters like REQUEST_CAMERA_INFORMATION #2013

JonasVautherin opened this issue Mar 31, 2023 · 10 comments · Fixed by #2381

Comments

@JonasVautherin
Copy link
Collaborator

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 🙈.

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

  • Push the receivers to support both messages (i.e. the drone should support both REQUEST_MESSAGE(<camera_information>) and REQUEST_CAMERA_INFORMATION.
  • Keep the old way in MAVSDK until we feel like we can completely drop it.

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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

I don't think so 🤔. 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 😇.

@julianoes
Copy link
Collaborator

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
Copy link
Collaborator Author

JonasVautherin commented Apr 4, 2023

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

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
Copy link
Collaborator Author

JonasVautherin commented Apr 4, 2023

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

@JonasVautherin
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator Author

JonasVautherin commented Apr 10, 2023

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
Copy link
Collaborator

Yes, exactly!

@julianoes
Copy link
Collaborator

In v3 I want to remove the MAV_CMD_REQUEST_AUTOPILOT_CAPABILITIES.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants