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

Hold: Only use MISSION_ITEM_INT #8122

Closed

Conversation

DonLakeFlyer
Copy link
Contributor

@DonLakeFlyer DonLakeFlyer commented Dec 13, 2019

This pull request relates to a breaking mavlink mission protocol spec change I want to make which would need to be coordinate/testing across al major GCS makers. The change is basically to always use MISSION_ITEM_INT no matter what. I have tested this pull against PX4 and it works fine. Given discussion in #8086 I assume it will work fine for PX4 as well. But I won't be able to test that till next week.

Here it is:

  • Mark MISSION_ITEM, MISSION_REQUEST and MAV_PROTOCOL_CAPABILITY_MISSION_INT as deprecated
  • Change spec for MISSION_REQUEST to always respond with MISSION_ITEM_INT item NOT MISSION_ITEM. This is a breaking change.

Related to #8086

@DonLakeFlyer
Copy link
Contributor Author

DonLakeFlyer commented Dec 13, 2019

@hamishwillee, @peterbarker, @rmackay9, @WickedShell, @meee1 FYI

If someone wants to push this through mavlink before I can do it. That's fine. I won't be able to get to it till mid-next week. If at all possible I'd like to release the QGC Stable which is going out with this change.

I don't think there is any way to do with with a breaking spec change. But that said I think the simplification is worth it.

@DonLakeFlyer
Copy link
Contributor Author

MY other proposal would be to allow MISSION_ITEM_INT.x/y to be something like max int to represent a NaN like optional params work for MISSION_ITEM.

@hamishwillee
Copy link
Contributor

If someone wants to push this through mavlink before I can do it. That's fine. I won't be able to get to it till mid-next week. If at all possible I'd like to release the QGC Stable which is going out with this change.
I don't think there is any way to do with with a breaking spec change. But that said I think the simplification is worth it.

The "right" MAVLink way to do this would be to deprecate MISSION_REQUEST, MISSION_ITEM and leave MAV_PROTOCOL_CAPABILITY_MISSION_INT alone. Then add a new capability or version MAV_PROTOCOL_CAPABILITY_MISSION_VER2 so there is an easy way to detect the break behaviour (things that no longer support the MISSION_REQUEST).

Whether or not we make this break, does it not make sense to make the change easy to detect?
Further, if we do this, we should also try pull in this change to the protocol: mavlink/mavlink#1171 (comment)

NOTE: I haven't been tracking the discussion properly - why can't stacks stop using MISSION_REQUEST and use MISSION_REQUEST_INT as per the spec?

@auturgy FWIW this is the kind of thing formalised microservices versions helps. This is now messy.

MY other proposal would be to allow MISSION_ITEM_INT.x/y to be something like max int to represent a NaN like optional params work for MISSION_ITEM.

I don't understand what you are suggesting here. Something like MISSION_ITEM_INT.x -- PARAM5 / local: x position in meters * 1e4, global: latitude in degrees * 10^7. UINT32_MAX indicates not supported. What's the benefit?

@auturgy
Copy link

auturgy commented Dec 16, 2019

@hamishwillee sigh. I’m past arguing about that. Simply put, juice not worth the squeeze. Let’s not spread that debate to here though.

@DonLakeFlyer DonLakeFlyer reopened this Dec 24, 2019
@DonLakeFlyer DonLakeFlyer changed the title Only use MISSION_ITEM_INT Hold: Only use MISSION_ITEM_INT Dec 24, 2019
@DonLakeFlyer
Copy link
Contributor Author

Waiting till after stable and I put up the mavlink discussion before this gone in.

@DonLakeFlyer DonLakeFlyer deleted the MissionItemIntAlways branch March 2, 2020 17:01
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.

None yet

3 participants