PARAM_EXT_TRIMMED: move fields to even more optimize for zero byte truncation

[olliw42]

it is a very good improvement to optimize the PARAM_EXT messages to better benefit from zero byte truncation. However, I think this could have been pushed a bit more.

This PR reorders the fields such that param_id is right before param_value. That way also param_id is truncated if the value happen to be zero.

This for sure will not happen extremely often, but a zero value might not be too uncommon (might the default) as well as param_id less than 16 chars, so it may happen often enough to be worthwhile given how simple the change is.

[hamishwillee]

@olliw42 Unless I'm a bit confused this will only happen for a parameter with id 0, which there can only be one of (at most, if any). That is very rare. While I appreciate any data saving is a saving, and that mavlink is for machine-machine transfer, as a reader, I also find it useful to see those ids first when interpreting/understanding messages.

@julianoes Leaving this one for you to decide on.

[olliw42]

@hamishwillee
param_id is the name string, not the index integer. So, savings happen whenever the parameter name is shorter than 16 chars. For instance, for a parameter name like "MNT_TYPE" there would be 8 additional bytes being saved (if the value of that parameter is 0, which in ArduPilot it is per default)

[hamishwillee]

Duh, thank you @olliw42 . @julianoes, changed my tune. "Approved" by me if you're OK with it.

[olliw42]

@hamishwillee could you pl discuss and possibly settle this at the dev call tomorrow? would be most kind

[hamishwillee]

Yes, already on the list.

[olliw42]

thx sir

[hamishwillee]

Dev call didn't have right people in it to discuss. Making an executive decision to merge on grounds of "still WIP" and this is why we have WIP.

@DonLakeFlyer @dogmaphobic @DanielePettenuzzo @julianoes FYI, this is a minor optimistisation - a field reordering on the PARAM_EXT_TRIMMED messages that will save a few bytes off the param id (name) if it is less than 16 bytes and the param value is zero.

[dogmaphobic]

I was just talking to @DonLakeFlyer about this yesterday. From Don's and my perspective, this whole PARAM_EXT_V3 should be discarded. It doesn't make a whole lot of sense to support two semi-identical set of messages. If we switch to the new one, every single camera implementation in existence would stop working.

[hamishwillee]

@dogmaphobic Incorrect ^^^^, or "late to the party".

We already dumped PARAM_EXT_V3. The messages are named _TRIMMED and there is a clear mechanism that allows a GCS to work reliably with any existing camera. Essentially the GCS asks for the new messages in the read requests. If the camera knows about the new messages it returns those. If it doesn't it returns the old ones. The GCS has to handle the response and not send out the trimmed messages unless it has had a vehicle message that shows the vehicle knows the new type.

[olliw42]

many thx for having considered this in the dev call, and merging it.

I'm a bit confused however about what I should think of it.

I mean, merging with argument it's WIP anyhow is fine, but maybe not so much if it will be taken back in few weeks. So, is this going to stay now for a bit longer than just to the next argument?
(in short, is it worth for me to implement it or not LOL)

also, related (and the latest conversation I think would have been better placed in the other issue/pr #1449 / #1450) is the question of the flags. I understand the reasoning for why to say only EXT flag but no EXT_TRIMMED flag, but I don't find it convincing. As said before, ultimately one could also sort out PARAM V1 or PARAM EXT or PARAM EXT TRIMMED by some timeout and/or auxiliary protocol procedures, so therefore no flag at all is fundamentally needed. And this argument applies equally to both EXT and EXT_TRIMMED flags. It's strange then to argue that a gcs should be informed about whether V1 or EXT/EXT_TRIMMED is supported but not also about EXT and EXT_TRIMMED, so it can do the right thing right away. In fact, the auxiliary protocol procedures now put in place on top of the messages could be lifted/eased with such a flag. As said, I believe to understand the reasoning, but I don't find it convincing.
(here too, in short, without the flags settled I wonder: is it worth for me to implement anything or not LOL)

[hamishwillee]

@olliw42 This PR is the no brainer part of your proposal. The only reason I mention WIP is because that is the bar I use for changes I will make without broad consultation.

With respect to flags, I guess we both find each other's arguments unconvincing. When Julian comes back I will be led by him.

[olliw42]

With respect to flags, I guess we both find each other's arguments unconvincing.

hahaha ... put a smile on my face ... good start of a day

[DonLakeFlyer]

Magic trimming based on field ordering seems stupid to me and will always leads to this, oops shit, we need a V2 of this message. Why not implement better trimming support in mavlink such that it can be specified on a field by field basis?

[hamishwillee]

Why not implement better trimming support in mavlink such that it can be specified on a field by field basis?

Probably because to trim a field you need to add information to indicate the used length, and because doing that would be a mavlink3-level incompatibility. IMO definitely a worthy evolution but I would do it as an automatic thing and only for the "big costs". I.e. just auto trim all non-empty arrays since these tend to be big. This might be done by specifying that if an array field starts with a non-zero byte this is the length.

And of course someone else will argue that we need to be obvious about the length, since otherwise people will over-allocate.