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

PARAM_EXT_TRIMMED: supported or not, how to know and what to do #1449

Closed
olliw42 opened this issue Aug 10, 2020 · 11 comments
Closed

PARAM_EXT_TRIMMED: supported or not, how to know and what to do #1449

olliw42 opened this issue Aug 10, 2020 · 11 comments
Labels
Dev Call Issues to be discussed during the Dev Call

Comments

@olliw42
Copy link
Contributor

olliw42 commented Aug 10, 2020

The recent PARAM_EXT_TRIMMED improvements to the PARAM_EXT protocol are very worthwhile, but some points are unclear to me.

What should a component do in case it gets a PARAM_EXT_REQUEST_READ or PARAM_EXT_REQUEST_LIST with trimmed = 1, but doesn't support the trimmed messages?

Should it respond with the untrimmed PARAM_EXT messages, or should it just not respond at all?

SOLVED! Answer (see below): It should respond with untrimmed PARAM_EXT messages,

I think one could add respective flags to the capabilities field in the AUTOPILOT_VERSION message.

I believe that both MP and QGC expect this message from a component, and uses it e.g. to determine if MAVFTP is supported. It also has a MAV_PROTOCOL_CAPABILITY_MAVLINK2 bit (but I'm not sure if that is actually used). That is, even though the name is "AUTOPILOT_VERSION" and not COMPONENT_VERSION it is a message useful for all components already.

Therefore, adding flags

MAV_PROTOCOL_CAPABILITY_PARAM_EXT
MAV_PROTOCOL_CAPABILITY_PARAM_EXT_TRIMMED

would allow quick determination if the component supports the PARAM_EXT and/or the PARAM_EXT_TRIMMED messages.
(I think the assumption is that the old PARAM messages MUST always be supported)

@hamishwillee
Copy link
Collaborator

@olliw42

What should a component do in case it gets a PARAM_EXT_REQUEST_READ or PARAM_EXT_REQUEST_LIST with trimmed = 1, but doesn't support the trimmed messages?

The recipient must return the trimmed messages if it gets the 1 and supports trimmed messages. Otherwise it should return the untrimmed/original messages.
A sender that sets 1 must always be able to handle either response, and should not set params using the untrimmed messages until it has received a trimmed message in response to either read or request list.

This is robust, and means that we don't need a capability.

See https://mavlink.io/en/messages/common.html#PARAM_EXT_SET_TRIMMED

If there is no response to this message, and it is unknown whether the _TRIMMED messages are supported (because no PARAM_EXT_REQUEST_READ or PARAM_EXT_REQUEST_LIST has been performed yet), then fall back to PARAM_EXT_SET.

See https://mavlink.io/en/messages/common.html#PARAM_EXT_REQUEST_READ and https://mavlink.io/en/messages/common.html#PARAM_EXT_REQUEST_LIST

Request TRIMMED variants of PARAM_EXT messages. Set to 1 if _TRIMMED message variants are supported, and 0 otherwise. This signals the recipient that _TRIMMED messages are supported by the sender (and should be used if supported by the recipient).

Probably "(and should be used if supported by the recipient)." should be "(and must be used if supported by the recipient)."

This will be clearer when I get around to showing guide docs.

@olliw42
Copy link
Contributor Author

olliw42 commented Aug 10, 2020

many thx for the quick response

The recipient must return the trimmed messages if it gets the 1 and supports trimmed messages. Otherwise it should return the untrimmed/original messages.

ok

A sender that sets 1 must always be able to handle either response, and should not set params using the untrimmed messages until it has received a trimmed message in response to either read or request list.

you mean "should not set params using untrimmed or trimmed messages until it has received a untrimmed or trimmed message in response to either read or request list", right?

This is robust, and means that we don't need a capability.

The majority of the capability flags are not really needed, since for the majority of them procedures can be devised to robustly infer them otherwise. Ultimately by just timing out. So, it seems to me that the capability flags are NOT about robustness, they are as much as I can see more about convenience, e.g. speed up of inferring capabilities at first connection.

So, I don't buy that argument that they are not needed because it is already robust :)

@hamishwillee
Copy link
Collaborator

Erky, yes I mean:

A sender that sets 1 must always be able to handle either response, and should not set params using the trimmed messages until it has received a trimmed message in response to either read or request list.

@hamishwillee
Copy link
Collaborator

And what I mean is that in this case we have a clear fallback. I don't propose to use up one of our valuable protocol bits for this. But if you can convince Julian, go for it :-)

@olliw42
Copy link
Contributor Author

olliw42 commented Aug 10, 2020

it are 64 bits, and so far only 16 are used ...

note that the bits not only handle EXT vs EXT_TRIMMED, but also NORMAL vs EXT ... as much as I know there is no clear and robust fallback for that. This would open using EXT/EXT_TRIMMED also for normal parameter handling and not only for camera.

@hamishwillee
Copy link
Collaborator

I repeat, if @julianoes thinks it is a good idea then sure. It would certainly be good to move towards just one parameter system.

@olliw42
Copy link
Contributor Author

olliw42 commented Aug 10, 2020

It would certainly be good to move towards just one parameter system.

one hardly can disagree with this :)

(it would then had been better to introduce also TRIMMED versions of REQUEST_LIST and REQUEST_READ, since like now there will be this trimmed field floating around for ever)

I surely can't know, but I suspect that flags like MAV_PROTOCOL_CAPABILITY_PARAM_FLOAT and MAV_PROTOCOL_CAPABILITY_PARAM_UNION had been introduced to handle a similar transition (some time ago I asked what they mean and it appeared that no one knew what they are for, so could have been for helping in a transitioning thing).

anyway, as you say, let's hear julian :)

@hamishwillee hamishwillee added the Dev Call Issues to be discussed during the Dev Call label Aug 12, 2020
@hamishwillee
Copy link
Collaborator

FWIW I think the argument for protocol bit makes sense - we do want to encourage only having one microservice "version" active and for that you need a bit.

I argued originally for trimmed versions of all messages - ie a complete new API "PARAM_V3_XXXX". @julianoes and Don were opposed. Apparently this design is much easier for them to manage.

Anyway, I have added to the Devcall next week, so it will hopefully be dealt with then, if not before.

@olliw42
Copy link
Contributor Author

olliw42 commented Aug 12, 2020

FWIW I think the argument for protocol bit makes sense - we do want to encourage only having one microservice "version" active and for that you need a bit.

:)

I argued originally for trimmed versions of all messages - ie a complete new API "PARAM_V3_XXXX".

well ... given that it still has limitations which one would have be resolved in a next real version I wouldn't have called it V3 either ... how many protocol version do you want to have at the end

@julianoes and Don were opposed. Apparently this design is much easier for them to manage.

it is much easier since it's also backwards compatible and works with older code

my comment was just meant to say that the trimmed version doesn't really help with progressing towards one final all good parameter system. So, when seen as a correction to V2 and not a step towards V3, the trimmed field is a smart solution.

I wonder if the bits shouldn't be better be called
MAV_PROTOCOL_CAPABILITY_PARAM_V2
MAV_PROTOCOL_CAPABILITY_PARAM_V2_TRIMMED

@julianoes
Copy link
Collaborator

julianoes commented Aug 24, 2020

I don't propose to use up one of our valuable protocol bits for this.

I agree that the protocol bits would be the easiest, most straight-forward mechanism to signal PARAM_EXT or PARAM_EXT_TRIMMED. However, the last time we had the discussion (a cache checksum for missions) to add a protocol bit with @LorenzMeier he pushed back, and the reasoning I remember is:

  1. The protocol bits are scarce! (I was under the impression that they are almost exhausted but we have only used 16 of 64 as @olliw42 rightfully points out!)
  2. We should use microservice versioning for this! (Pull requests for an initial implementation were ready to be merged but no one cared enough for it to be merged.)

So, this being said, I opted to not use the capability bit because I did not want to have the discussion about whether it would warrant a bit with and I did not want to push ahead microservice versioning because that would be much more work. Instead, I thought that the upgrade path can be done within the messages using the flag 😏.

@olliw42
Copy link
Contributor Author

olliw42 commented Aug 29, 2020

that's a very understandable line of reasoning, and I happily accept it for the moment.

I however wish to leave the comment that you can't continue to not solve pressing issues for infinity.

It leads to awkward situations. For instance, I do have a gimbal and a camera, and I strongly wonder how could anyone such as a GCS figure out if the gimbal does support param v2, or just param v1? For instance, for a camera QGC does it the most ugly way: Upon connection it calls all camera parameters with param v1, just to moments later call them again with param v2, and thereby also using such a strict timing that many need to be recalled. And remind: All this is going through a typically low-bandwidth high-loss link! For instance, for FTP such reservations did not seem to exist and the simple an practical way to use the bits. And so on and forth. Awkward. Inconsistent. Painful.

I think I once have seen the microservice versioning proposal ... and found it quite complicated. I'm not surprised it's rotting.

One can quite overdo the versioning thing. UAVCAN v1 is a nice example of overdoing. Ideally you should just have very few microservice versions. That there is talk about a param v3 is ONLY because of all the mistakes which have been done before of not thinking about what one is doing. I still fail to see that after a decade of sending parameters it's still flawed in it's basics. It's ok that the first shot might not be perfect, but the second should have solved the issues which could have been seen at that time.

I thought that the upgrade path can be done within the messages using the flag

well, you could. But it should then have gone into PARAM_LIST and deprecate PARAM_EXT_LIST so that one could have handled param v1, v2, v2trimmed along the same lines ...

Anyway, as said, I understand the reasoning and certainly accept it :). And since the questions in this issue were all answered I close the issue :)

@olliw42 olliw42 closed this as completed Aug 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dev Call Issues to be discussed during the Dev Call
Projects
None yet
Development

No branches or pull requests

3 participants