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

Deprecate all non MISSION_*_INT usage #1389

Merged
merged 8 commits into from
Jun 25, 2020

Conversation

hamishwillee
Copy link
Collaborator

Fixes #1285

To progress this, have created PR. Docs will have to be updated too. Original description below


As it currently stands there is a hole in the mavlink spec with respect to _INT usage. There is a capability bit on the vehicle side. But there is no clear way to know whether the GCS side will support uploads of MISSION_ITEM_INT items. The only sideways sort of way to do that would be for the vehicle to remember if the GCS did a MISSION_REQUEST_INT. To both deal with that and remove complexity from the mavlink spec I propose a breaking change to only allow support for the _INT variants of the protocol.

  • This would mark MISSION_ITEM, MISSION_REQUEST and MAV_PROTOCOL_CAPABILITY_MISSION_INT as deprecated.
  • A gcs is required to support the _INT based protocol sequence.
  • If a gcs receives old non _INT requests it always respond with _INT responses.This provides an interim transition path to the future while these messages still exist. I've tested this in QGC and it works fine against PX4 and ArduPilot.

This reflects the reality of PX4/ArduPilot/QGC/MP of today which supports the _INT based protocol. I don't see any reason to maintain the non _INT protocol moving forward. So gets reduce complexity and get rid of it.

Related to #8122

@hamishwillee
Copy link
Collaborator Author

FYI @DonLakeFlyer @auturgy

It seems to me that this simplifies things, and breaks nothing. Any opposition to merging? I will then have to update docs as a follow up.

@auturgy
Copy link
Collaborator

auturgy commented Jun 2, 2020

@peterbarker can we test against MAVProxy?

@peterbarker
Copy link
Contributor

peterbarker commented Jun 2, 2020

@auturgy this only deprecates the messages, so unless someone acts on the deprecation warning it won't have any effect.

MAVProxy does have a setting which forces it to use _INT, so it won't break totally even if the messages were removed.

@hamishwillee
Copy link
Collaborator Author

@auturgy @DonLakeFlyer @julianoes That sounds like "OK to proceed"?

@julianoes
Copy link
Collaborator

Generally, I think that makes sense.

MAV_PROTOCOL_CAPABILITY_MISSION_INT as deprecated.

I'm not sure about this one. If it is deprecated, does that mean the autopilot should not set it anymore? If so, this would potentially break things because e.g. MAVSDK expects this to be set. If it is not set MAVSDK will refuse to upload/download missions as it (already) doesn't support non-int transfers.

Therefore, I would maybe describe it as "always has to be set" if missions are supported.

@DonLakeFlyer
Copy link
Contributor

How often is stable updates of MAVSDK? If not too long just fix it and push into next stable. It will take time for new firmwares to stop sending the bit anyway. By the time that happen there could be a new mavsdk can't there.

@DonLakeFlyer
Copy link
Contributor

DonLakeFlyer commented Jun 8, 2020

No that won't really work. So do this in stages. Mark the cap bit as continue to be sent, but ignored by updated consumers. With a deprecation date where it disappears. Then at that time all consumers will need to be up to date with latest spec.

@hamishwillee
Copy link
Collaborator Author

hamishwillee commented Jun 9, 2020

@julianoes I tidied up the description of MAV_PROTOCOL_CAPABILITY_MISSION_INT to reflect better what @DonLakeFlyer is saying.

I share your concern. We use this flag to say "we have been updated and we expect this type of item". If feels wrong to say "we trust that you'll always send us the right type irrespective of what we put in here". This is where I want the microservices versioning protocol in place to remove the ambiguity.

Don, I don't understand the harm on this being set, though I do understand that when all systems are updated this would no longer be necessary.

Julian, are you convinced by Don's suggestions?

@julianoes
Copy link
Collaborator

Mark the cap bit as continue to be sent, but ignored by updated consumers. With a deprecation date where it disappears.

Yes, that makes sense.

@hamishwillee I think you have it the wrong way round.

No longer required. Systems that support missions should ignore this flag and always return a MISSION_ITEM_INT rather than a MISSION_ITEM (deprecated).

The flag is set by the autopilot (and not the ground station) as I understand it.
Therefore, the flag is still required because it means that MISSION_INT can be chosen. By not setting the flag, older implementations of MAVSDK will break because they expect it to be set.

I would formulate like this:

This flag always needs to be set because MISSION_ITEM_INT is to be used rather than MISSION_ITEM which is deprecated.

@julianoes
Copy link
Collaborator

How often is stable updates of MAVSDK?

Irregular but generally every few weeks. However, if possible I try to keep things compatible as much as possible.

@hamishwillee
Copy link
Collaborator Author

@hamishwillee I think you have it the wrong way round.

I know what I mean, but my wording could still be bad. 2. This is supposed to be capturing the text "If a gcs receives old non _INT requests it always respond with _INT responses."

I'll make it the way you suggest.

@@ -3175,7 +3175,8 @@
<description>Autopilot supports the new param float message type.</description>
</entry>
<entry value="4" name="MAV_PROTOCOL_CAPABILITY_MISSION_INT">
<description>Autopilot supports MISSION_INT scaled integer message type.</description>
<deprecated since="2020-06" replaced_by="">This flag must always be set if missions are supported, because missions must always use MISSION_ITEM_INT (rather than MISSION_ITEM, which is deprecated).</deprecated>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DonLakeFlyer @julianoes You OK to merge this PR? Note Don that for now this should still be set (which I believe was the result of the discussion).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok!

@hamishwillee
Copy link
Collaborator Author

OK, I am merging this. We want to make it very clear that MISSION_ITEM should no longer be used and this does that. The arguments around the capability bit can be addressed later.

@hamishwillee hamishwillee merged commit 67a08ad into master Jun 25, 2020
@hamishwillee hamishwillee deleted the hamishwillee-del_mission_item branch June 25, 2020 01:49
amilcarlucas pushed a commit to ArduPilot/mavlink that referenced this pull request Sep 22, 2020
amilcarlucas pushed a commit to amilcarlucas/mavlink that referenced this pull request Sep 25, 2020
amilcarlucas pushed a commit to amilcarlucas/mavlink that referenced this pull request Sep 29, 2020
amilcarlucas pushed a commit to amilcarlucas/mavlink that referenced this pull request Oct 5, 2020
peterbarker pushed a commit to peterbarker/mavlink that referenced this pull request Oct 13, 2020
tridge pushed a commit to ArduPilot/mavlink that referenced this pull request Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Deprecate all non MISSION_*_INT usage with a breaking change
5 participants