DO_REPOSITION ignores frame, always assumes AMSL

[julianoes]

It looks like the frame of a DO_REPOSITION command is ignored. The altitude is always assumed to be AMSL:

https://github.com/PX4/Firmware/blob/96995852cb6a2e2e23894c7ea05dd8385c595172/src/modules/navigator/navigator_main.cpp#L357

This was discovered by @mzahana in mavlink/MAVSDK#485.

[julianoes]

It looks like we ignore the frame for ALL command_ints:
https://github.com/PX4/Firmware/blob/5acb9dfed0d6b097ce4e9d239e8d39a88cb7088c/src/modules/mavlink/mavlink_receiver.cpp#L497-L524

@dagar did you know?

[dagar]

Yes I'm aware. I wanted to make all of these things respect frame (both commands and missions), but what is it supposed to be if sent with COMMAND_LONG (no frame)?
https://mavlink.io/en/messages/common.html#COMMAND_LONG

[mzahana]

I guess it would be more consistent to have frame field in COMMAND_LONG, no?

[julianoes]

I vote to start ignoring warning when receiving a COMMAND_LONG and assuming a frame in that case and after a while not accepting it anymore.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[hamishwillee]

@julianoes Can we just stop accepting COMMAND_LONG now. Probably safer. If we do accept it then should we should emit a deprecation STATUS_TEXT with frame used info included? That way there is a cost to sending us those messages.

[julianoes]

@hamishwillee agreed.

[hamishwillee]

@julianoes So this went stale last time, how do we get work done so it gets completed?

[julianoes]

Ok, so I started looking into it and found that we don't even store the frame from any incoming COMMAND_INT, so clearly we're just ignoring this and not adhering to the spec. I don't even know where to start 😕.

https://github.com/PX4/Firmware/blob/6178ead6ea48568972fa71ef95d87683783c482d/src/modules/mavlink/mavlink_receiver.cpp#L518-L545

[hamishwillee]

That has implications for any test suite :-(.

Proposal.

1. We store the frame.
2. We reject any message that does not use the frames we support
   I can try do this if you want.

[julianoes]

Sure, I think that would make sense.

[julianoes]

It's still wrong you bot.

[hamishwillee]

One day I will find time

[ericjohnson97]

I think the current version still doesn't support any other frames besides MAV_FRAME_GLOBAL

[jperez-droneup]

I am having issue with this bug as well. We would prefer to run MAV_CMD_DO_REPOSITION with MAV_FRAME_GLOBAL_RELATIVE_ALT_INT. Is this bug planned to be fixed in the near future?

[hamishwillee]

@julianoes @dagar Recent discussion on this area is that if it is sent with COMMAND_LONG (which has no frame) it should be rejected with a result of MAV_RESULT_COMMAND_INT_ONLY.

I am also proposing MAV_RESULT_COMMAND_UNSUPPORTED_FRAME so that we can relatively easily do the right thing and indicate that we don't support "wrong" frames. Currently we don't even return unsupported which is horribly buggy.

I am having issue with this bug as well. We would prefer to run MAV_CMD_DO_REPOSITION with MAV_FRAME_GLOBAL_RELATIVE_ALT_INT. Is this bug planned to be fixed in the near future?

@jperez-droneup No.

I have unassigned me as I am never going to find time, and would not be the most efficient person to do so. Given @dagar hasn't done it in years probably he won't either.

You are most welcome to create a PR that implements another frame and/or errors if unsupported frames are used.