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

GCS_MAVLINK: Fix Deprecation of MISSION_REQUEST #28854

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

menschel
Copy link
Contributor

Minor Protocol Issue; Finding during another investigation mavlink/qgroundcontrol#12184

MISSION_REQUEST is deprecated since 2020, replaced with MISSION_REQUEST_INT, but ardupilot still uses it to request mission items from GCS.

https://mavlink.io/en/services/mission.html#uploading_mission
upload

wireshark_mavlink_upload

@menschel menschel force-pushed the pr-fix-deprecated-mavlink-mission-request branch from b86d074 to dd04ec6 Compare December 13, 2024 14:51
@menschel
Copy link
Contributor Author

menschel commented Dec 13, 2024

After applying the patch.

Mavlink_Mission_Upload_with_Fix

Mission Request Message is deprecated since 2020.
Replaced by Mission Request Int.
@menschel menschel force-pushed the pr-fix-deprecated-mavlink-mission-request branch from dd04ec6 to 1c43fae Compare December 13, 2024 15:39
@peterbarker
Copy link
Contributor

We continue to request MISSION_ITEM so as not to break older ground control stations.

I deal with one which this PR would adversely affect (I'm nudging the author to update....).

Which GCSs have you tested with these changes?

@menschel
Copy link
Contributor Author

menschel commented Dec 14, 2024

I deal with one which this PR would adversely affect (I'm nudging the author to update....).

Please add a link there that marks this PR as duplicate, so it is to be closed along with it.

Which GCSs have you tested with these changes?

I'm hunting some Mavlink issues with QGroundControl and ELRS 900M, very frustrating topic because ELRS 2G4 works perfectly and mLRS 900M also works perfectly.

EDIT:
These also work with the change.

@menschel
Copy link
Contributor Author

menschel commented Dec 14, 2024

Tested again after a git hard reset onto the change, and the internal error is gone. It was likely the check in line 371 being reverted on stashed changes.

@peterbarker
Copy link
Contributor

Requires ArduPilot/MAVProxy#1492

@peterbarker
Copy link
Contributor

I deal with one which this PR would adversely affect (I'm nudging the author to update....).

Please add a link there that marks this PR as duplicate, so it is to be closed along with it.

That particular GCS is not open-source.

Which GCSs have you tested with these changes?

I'm hunting some Mavlink issues with QGroundControl and ELRS 900M, very frustrating topic because ELRS 2G4 works perfectly and mLRS 900M also works perfectly.

EDIT: These also work with the change.

* [APM-Planner_2-2.0.30-rc3](https://github.com/ArduPilot/apm_planner/releases/tag/2.0.30-rc3)

* [MissionPlanner1.3.82](https://github.com/ArduPilot/MissionPlanner/releases/tag/MissionPlanner1.3.82)

Nice! I've fixed MAVProxy to work with it.

We'll need to follow a process here so as to give people plenty of warning that things are going to break. It is well past time that we did follow some process to get this done.

There are many GCSs and tools out there that would be adversely affected by this patch if we were to simply merge it. MAVProxy users are likely to update relatively frequently - but uGCS users perhaps not.

We may need to carry an options bit for a while to allow a user to get the old behaviour back.

I am curious to know what real-world problem you are trying to solve with this patch. I assume there's a client which only handles MISSION_REQUEST_INT, and can't be modified to also handle MISSION_REQUESTs from the autopilot?

@menschel
Copy link
Contributor Author

We'll need to follow a process here so as to give people plenty of warning that things are going to break. It is well past time that we did follow some process to get this done.

It would be the professional way to introduce breaking changes with a new major version.
A deprecation warning in the release notes from 4.6 onward is a fair warning of an error that will hit with 5.0 .

I am curious to know what real-world problem you are trying to solve with this patch. I assume there's a client which only handles MISSION_REQUEST_INT, and can't be modified to also handle MISSION_REQUESTs from the autopilot?

None, just a side finding. A decade or two on the job, does teach to address technical debt immediately.

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.

2 participants