search

mavlink / MAVSDK

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

Camera plugin uses deprecated messages #1605

Open dlech opened 2 years ago

dlech commented 2 years ago

The camera plugin currently uses deprecated messages like MAV_CMD_REQUEST_CAMERA_INFORMATION instead of MAV_CMD_REQUEST_MESSAGE.

This would make it unusable with a camera that only supports the "new" request message.

JonasVautherin commented 2 years ago

I see your point, but here is my personal opinion: MAVLink "servers" should implement both in the foreseeable future, because MAVLink does not provide a way to detect which command is supported and which is not (well, there is a MAV_CMD_ACK_ERR_NOT_SUPPORTED, but I don't think it is mandatory nor generally implemented, and it's not enough for MAV_CMD_REQUEST_MESSAGE which represents subcommands).

More specifically, if we move MAVSDK to using only MAV_CMD_REQUEST_MESSAGE, we will break all legacy systems that don't support the new way. "Deprecated" means that it may be removed in the future, not that it is not supported anymore. MAVLink still supports them, so MAVSDK should still support them (until MAVLink removes them entirely).

Now, we may be tempted to wish for a transition: after all, why not letting new servers implement only the new messages, and have MAVSDK detect which message it should use? The problem with that is that MAVLink cannot make a difference between an unsupported command and a command that timed out. It is very common for systems that don't support a command to just ignore it, and it is sometimes designed like this: only one component of the system is meant to know that command and answer to it, and the others just ignore it.

So how do you make MAVSDK detect if the new message is supported? If you send a "dummy" MAV_CMD_REQUEST_MESSAGE and get an ACK, then you know that the drone supports this message, but what happens if it times out? Should you assume that the drone does not support it, or should you assume it was an actual time out? How many times should you try again before you can be "reasonably sure" that it is actually not supported? And when you do that for one specific MAV_CMD_REQUEST_MESSAGE, it does not say anything about another one, does it?

Conclusion

In my personal opinion, the only way to move towards the new message is to remove the old one and break all the systems that still use it. This is not a MAVSDK decision but a MAVLink decision. Until then, MAVSDK should use the old way and drones should support both.

julianoes commented 2 years ago

I agree with @JonasVautherin and this was my stance about one year ago. I think by now we can start to try using the new MAV_CMD_REQUEST_MESSAGE and fallback to the previous one if it doesn't work. This might lead to suboptimal behavior for systems that don't support it yet but that might be ok in my opinion. The cameras that I'm aware off, except the ones with Yuneec H520, should support the new way.