Component Information Basics: add camera cap flag

[olliw42]

title says it

if there is a gimbal v2 protocol flag there should be also a camera protocol flag, shouldn't it?

[auturgy]

On the surface this makes sense to me. How is discovery currently carried out? (obviously it isn't by ArduPilot)

[olliw42]

How is discovery currently carried out?

this appears to be a highly troublesome topic, see the discussion #1724 (comment)

everything (most of) which is said there on gimbal protocol v2 applies 1to1 also camera (in fact, for camera it would be even more relevant)

I have a growing feeling that there are quite some massive under-the-hood changes attached with these flags which look worrysome

[hamishwillee]

I think this is a good idea, but I would prefer it was versioned so that in future we can update if needed.

I would actually consider adding two: COMPONENT_CAP_FLAGS1_CAMERA_V1 (the old trigger only messages) and COMPONENT_CAP_FLAGS1_CAMERA_V2 the thing we call "the camera protocol". That way a GCS can choose to cope with support for either or both, very clearly. You might argue we need the same approach for the old gimbal.

Why? Cameras are discovered because they emit a heartbeat with MAV_TYPE_CAMERA. But there is no way to work out whether the camera also supports new messages, old messages, both.

Note that there is a hole here for cameras attached to the autopilot. We don't know if such a camera exists or not. We simply assume it might and fire the "old" messages to it. Actually PX4 fires all messages at it and they are supposed to be forwarded if they aren't handled.

Thoughts?

[olliw42]

I think that's a good idea

I was actually not aware that a camera_v1 is formally existing, but it's clear what you refer too

concerning heartbeat and camera_v2, IMHO the camera_v2 concept is very clear, there is a camera component which is handling the camera_v2 protocol. This implies that the thing which handles the camera_v2 protocol emits heartbeats with the camera mav_type, and ideally (but not necessarily) a camera component id. I know - and understand what this means - that not everyone may agree with that and that it may not be adhered to in practice, but I would think that it should not be debated.

(for camera_v1 this comment does obviously not apply)

"external" cameras, like external peripherals in general, are outside of the scope of current mavlink. As much as I can see.

[hamishwillee]

I was actually not aware that a camera_v1 is formally existing,

It isn't separately documented as an API - just a set of separate messages that existed at the dawn of time. I think I should group and number them so that at least the story of these extra messages and how/when they should be used can be clear. I'll think about it.

[hamishwillee]

@julianoes And if we allow versioning, can I give this a version number?

[julianoes]

I'd say if there is no version it's implicitly 1, and then if we need another version we would add a 2?

[hamishwillee]

I find it less ambiguous to make it clear we mean "this protocol in this version" - i.e. we don't need to imply this, and if we do it now then people seeing this in future don't say "does this mean it supports some version of the protocol or just version one?"

[olliw42]

added camera_v2, as per suggestion of @hamishwillee
did not rename camera to camera_v1, as implied by suggestion of @julianoes

[auturgy]

There’s also the Ardupilotmega set.

…

On 3 Dec 2021, at 9:06 am, Hamish Willee ***@***.***> wrote:  @hamishwillee commented on this pull request. In message_definitions/v1.0/development.xml: > @@ -141,7 +141,10 @@ <description>Component supports the events interface protocol.</description> </entry> <entry value="64" name="COMPONENT_CAP_FLAGS1_CAMERA"> - <description>Component supports the camera protocol.</description> + <description>Component supports the camera v1 protocol.</description> + </entry> + <entry value="128" name="COMPONENT_CAP_FLAGS1_CAMERA_v2"> + <description>Component supports the camera v2 protocol.</description> @julianoes Before Gus invented the camera protocol there were already camera commands like DO_TRIGGER that were implemented in both autopilots and mavlink cameras. In fact most GCS still emit DO_TRIGGER in missions to capture images even if they also support the newer protocol. My assertion is that all these old things are a notional camera API v1 and that the gus API we call the camera protocol is actually camera protocol v2. I was going to confirm this is reasonable assertion in the dev call. You might further argue that these messages are also part of camera API v2 - the things that a camera would be expected to implement. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[olliw42]

this is why this whole flags concept is a bit dodgy.

For gimbal one also can argue that there could be a v1 flag, and also for v2 it is not clear what it means, if it means that there is a gimbal device, a gimbal manager, or both, or just one, or ArduPilots Solo sets of commands, or those with that 100x or sign bug or not ... and at the end someone will reasonably ask that there should be also a flag for whether this WS LED at the end of one of the vehicle legs is supported so that the GCS knows to drive it, or what buttons are supported ... ... ... so, potentially as many flags as there are messages and features ...

maybe, to be able to draw a line, one could argue that there should be only flags for things which are documented in the official mavlink docs ... which would suggest that there should be only a camera v2 but no camera v1 ... but there is a gimbal v1, and if it refers to gimbal manager v2 and/or gimbal device v2 isn't resolved ...

... it's dodgy concept, and I continue to feel that the discussion in that other thread is relevant :)

[dayjaby]

Can't we make this field a uint8 with the value being the highly preferred version of the gimbal/camera protocol. 0 = not supported, 1 = v1, 2 = v2, etc. Of course we lose the possibility to show that we are able to support BOTH v1 and v2. But I think that does not matter: For example a GCS will, presumably, support all these versions. So the GCS only needs to know which protocol is preferred by the drone, which is definitely never something like "v1 and v2".

And as things evolve naturally, this approach is capable of handling v3, v4, ... ;)

[hamishwillee]

A bit of a ramble but ...

@julianoes @auturgy It feels like it might be time to spend at least a few minutes seeing if we can resurrect the microservices versioning protocol proposal in the context of components. The issues and solutions coming up here are exactly the same as our earlier discussions. When I say resurrect, I mean look at again and see if we can take anything from it.

This matters for the same reason common flight modes do. Of course you can hard code for these kinds of differences in other ways, but you have to hard code. If we want to have an open system where unknown things can interact you need standardized definitions of what the protocols mean in each versions, and mechanisms for discovering them.

Specifically:

• Flags are one way of indicating versions and they are very efficient for setting and reading. However they are not infinitely scalable and they are not easy to extend for things outside the standard - for example an ArduPilot or "HamishFlight" specific camera protocol. But maybe there are workarounds for this.
• Either way we do need to clearly agree how and where we define the versions.
  • Currently the only hint at this is the mavlink devguide but that is not complete, and there are certainly aspects where ArduPilot does not match what is written there, but was in line with the state of play when the implementations were created.
  • The microservices proposal was to use XML in a services definition that outlined the messages, commands etc. This would have captured the differences as separate versions, and then ideally worked to a common implementation as one of the versions.

Can't we make this field a uint8 with the value being the highly preferred version of the gimbal/camera protocol. ...

@dayjaby This is a generic component information message indicating that this component supports a specific set of features. That means it might support a whole bunch of other things like parameter protocol so you'd need a field for each new thing you wanted to add support for.

I like the principle - it's conceptually similar to the proposed versioning protocol and worth thinking about the pros and cons. I guess the main one is that I'd like to be able to extend the set of services supported and the versions supported for those services in an effectively unbounded way. I'd also like to "think" about the possibility of a dialect being able to support its own services.

@olliw42 Re your comments above. Yes. Somewhere you have to draw a line in the sand about what the scope of your protocol version is. Currently the only definitions are in the docs. The microservices protocol proposed to do that as markup for the involved messages etc with link to docs for service definition in the library.

[hamishwillee]

PS It is OK for us to agree to use flags for this, but we should have a clear pattern in mind about versioning and how this will extend.

[olliw42]

concerning versioning in the flags, I think it is reasonable to assume/ require that the highest version which is indicated to be supported is also the preferred one. Always use the greatest and latest :).
That way one doesn't need any of the complications @dayjaby suggested.

[olliw42]

killed

[olliw42]

I know I'm a pain in the butt, but I really think that you guys want to give this #1724 (comment) a serious thought. I'm more and more convinced that this flags approach will eventually turn out to have been somewhat "short sighted".
(not that it matters at all to me, but I think it does to you guys :))

[hamishwillee]

@julianoes If you still want this in, feel free to merge.

I am not sure how I feel about Ollies thinking. I feel all this stuff we're doing with capabilities is because we don't have a generic approach and extensible pattern for this. I'm not sure it helps or hinders that we have separate autopilot, component, camera, gimbal etc capabilities.
I do kind of thing that many of these capabilities are actually microversion support, and it would be nice if we just agreed to use them as such.