common.xml: deprecate the _INT frames

[peterbarker]

the frame of an altitude is not dependent on how the altitude is being transported (how other fields in the same message are being transported).

ArduPilot has treated these frames to be the same as the non-_INT equivalents in most places for a very long time.

We can save some confusion and perhaps a small amount of flash space if we remove the _INT frames

---

HamishW Edit

This marks MAV_FRAME_GLOBAL_INT, MAV_FRAME_GLOBAL_RELATIVE_ALT_INT, MAV_FRAME_GLOBAL_TERRAIN_ALT_INT as deprecated in favour of using their non-_INT alternatives, and simplifies the descriptions of scaling in the various messages.

The _INT variants were clearly created to allow specification of frames where the values of lat/lon are scaled, to go with the corresponding ones that don't make any statements about scaling. However these are used mainly in COMMAND_INT which explicitly specifies the scaling anyway - so the two values are synonyms (also true elsewhere these are used).

ArduPilot and MAVSDK, and presumably PX4 tend to implement code such that the INT versions are just synonyms. But sometimes one option or the other is forgotten, and this results in bugs. The idea here is to deprecate the pointless options, and eventually stop them being sent by GCS.

• On merge, update dev guide - see common.xml: deprecate the _INT frames #2092 (comment)

[peterbarker]

I've created a PR on the ArduPilot repository for discussion at our next DevCall.

[peterbarker]

We discussed at our DevCall and we think this is a good change. Just waiting for some discussion upstream.

[hamishwillee]

The _INT cases explicitly specify that lat/lon are scaled 1E7, while the others imply they are not. PX4 handles the case to make the decision about the way to decode the values.

I'll raise in the meeting this evening.

[nexton-winjeel]

I wasn't in the ArduPilot dev call, but my assumption is the problem is:

• the frame is independent from the types used to transport the position fields in a message.
• the latitude/longitude scaling should be determined from the type of the field:
  • no scaling if the field is a float;
  • 1E7 scaling if the field is int32_t.

[peterbarker]

On Tue, 12 Mar 2024, Hamish Willee wrote: The _INT cases explicitly specify that lat/lon are scaled 1E7, while the others imply they are not. PX4 handles the case to make the decision about the way to decode the values.

I'd be a little worried if PX4 could accept a 1e0 in an _INT message - but that's what you're implying :-P

[hamishwillee]

OK, so I think I get this.

You're basically always scaling when sending in a COMMAND_INT irrespective of the _INT type, possibly because that command says what the scaling of lat/lon should be and otherwise the frames are the same. You're also saying that in the other places MAV_FRAME is used, the differences specified in _INT are irrelevant. Right?

This made sense to @julianoes who has implemented this and treated those frames as synonymous.

1. I'm wondering if we should remove the field mappings in the frame definitions, as it is "by comparison" that they are confusing.
   So for example we currently have for the non-INT type:

   Global (WGS84) coordinate frame + altitude relative to the home position. First value / x: latitude, second value / y: longitude, third value / z: positive altitude with 0 being at the altitude of the home position.

   I read that as "the values are unscaled" if you're using this frame when I compare to the _INT version.

   Could we reasonably have:

   Global (WGS84) coordinate frame + altitude relative to the home position (altitudes are positive and 0 is the altitude of the home position).

   In any command that uses this we'd need to make sure the scaling was specified or implied.

2. If we're going to do this, there are numerous places where we recommend the _INT variants. We should also:

   • Check that in these cases the non-INT version is indeed synonymous.
   • Replaces with the non-INT version - I'm thinking of things like https://mavlink.io/en/messages/common.html#POSITION_TARGET_GLOBAL_INT

Sorry if I'm being dim. I'm a bit worried about touching frames - it always ends badly.

[peterbarker]

You're basically always scaling when sending in a COMMAND_INT irrespective of the _INT type, possibly because that command says what the scaling of lat/lon should be and otherwise the frames are the same.

Correct. COMMAND_INT specifying the values as being 1e7 and the frame specifying a multiplier leads to a natural conundrum of which to trust. So rely on the message (in this case COMMAND_INT) to specify the multiplier as it knows what the storage for the data looks like.

You're also saying that in the other places MAV_FRAME is used, the differences specified in _INT are irrelevant. Right?

Yes. Many years ago we found many bugs in the way we treat these frames - leading to a lack of correct multiplication or incorrect application thereof.

commit b0ea662c9b72d1db4ab74ba83eb8b80611c87881
Author: Peter Barker <pbarker@barker.dropbear.id.au>
Date:   Tue Dec 3 17:11:22 2019 +1100

    AP_Mission: consider _INT frames equivalent to non _INT variants
    
    AP_Mission: consider TERRAIN_ALT_INT equivalent to TERRAIN_ALT
    
    AP_Mission: consider RELATIVE_ALT_INT equivalent to RELATIVE_ALT
    
    AP_Mission: consider GLOBAL_INT equivalent to GLOBAL

This made sense to @julianoes who has implemented this and treated those frames as synonymous.

Nice!

1. I'm wondering if we should remove the field mappings in the frame definitions, as it is "by comparison" that they are confusing.
   So for example we currently have for the non-INT type:
   > Global (WGS84) coordinate frame + altitude relative to the home position. First value / x: latitude, second value / y: longitude, third value / z: positive altitude with 0 being at the altitude of the home position.
   
   
   I read that as "the values are unscaled" if you're using this frame when I compare to the `_INT` version.
   Could we reasonably have:
   > Global (WGS84) coordinate frame + altitude relative to the home position (altitudes are positive and 0 is the altitude of the home position).

I agree.

   In any command that uses this we'd need to make sure the scaling was specified or implied.

Yes.

Sorry if I'm being dim. I'm a bit worried about touching frames - it always ends badly.

:-)

[Maarrk]

We can save some confusion

I guess I am the confused person 😄

If I can chip in as mostly a user, and a very rare and novice contributor to either project, this would make my life easier. Recently we were trying to use a new feature of ArduPilot and ran exactly into the issue this change would prevent.

This is my current understanding, relying mostly on documentation:

I assume that MAV_FRAME sent in COMMAND_INT should behave the same way for MAV_FRAME_GLOBAL and MAV_FRAME_GLOBAL_INT (a search of the repo shows them together), but only the first one was accepted in the check.

quoted from ArduPilot/ardupilot#26387

[hamishwillee]

@Maarrk Yes. The theory is that these two frames should always be synonymous when used in a COMMAND_INT. However because they both exist and this is not always obvious, not every usage correctly allows either frame to be used.

Peter wants to clarify this. I'm supportive. I just know that frame definitions have historically caused much pain to modify.

[Maarrk]

Do you think there could be a mention of that in the command microservice documentation? Especially since it explicitly forbids using an unexpected frame:

Note that it is an error to ignore the specified frame and use some other arbitrary frame.

Proposed addition (I can make the PR to devguide if you think it's OK):

The message type COMMAND_INT implies encoding of global location as integers, deprecating separate frames ending with _INT. For backwards compatibility, flight stacks are advised to support the deprecated _INT frames and treat them the same as the MAV_FRAME_GLOBAL and MAV_FRAME_GLOBAL_RELATIVE_ALT counterpart.

I hope it's not too verbose, but past me would appreciate it.

Thank you both for taking this on and for your commitment to stability!

[hamishwillee]

Thanks @Maarrk
Yes, if this goes in we will need to update the devguide - thanks for pointing that out. However I want this PR to get merged before we make any changes in the guide. When that happens I will make some changes, using your words as inspriration.

[peterbarker]

On Tue, 19 Mar 2024, Hamish Willee wrote: We really want to discourage people calling this, so that in future we can rename and remove it. Any thoughts on that?

Sounds good. We need a very long time-frame for removing this stuff, of course. We need a couple of years before we can suggest to GCS operators to not send in the _INT frames to allow firmware implementors to make their adjustments. And then we need a couple more years before we can remove the framesfrom the firmwares...

[hamishwillee]

Note, I have removed mention of waypoint because the location depends on context - it doesn't have to be a waypoint.

I removed the mention of terrain map because again, this depends on context - it might come from a LIDAR or similar.

[hamishwillee]

@julianoes Can you sanity check this one too? As part of that, can you check whether PX4 does indeed use these as synonyms and support either in the cases where the command is sent?

I'll ping in Auterion too.

[julianoes]

I doubt PX4 does it correct yet, we'll have to fix that.

[hamishwillee]

@auturgy @peterbarker Any final words on this one? If not, I'll merge (probably next week). Feedback from Silvan and Alessandro in Auterion positive.

[peterbarker]

@auturgy @peterbarker Any final words on this one? If not, I'll merge (probably next week). Feedback from Silvan and Alessandro in Auterion positive.

LGTM, thanks!

Perhaps fewer commits would be nice, but that's it.

[auturgy]

Commits will get squashed on merge. Probably no need to wait until next week.

[hamishwillee]

OK. I'm feeling brave.,