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

API for receiving status notifications on outgoing mission requests #357

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

jonathanreeves
Copy link

This is a companion PR for the main implementation in MAVSDK:
mavlink/MAVSDK#2463

Details can be found in the MAVSDK PR.

@julianoes
Copy link
Collaborator

This is ok, but my question is: do you need this notification? All you care about is if the mission is uploaded, so you are given a new mission, right? You don't really need to know when someone downloads it, I would think, it's more of an internal thing.

@JonasVautherin
Copy link
Collaborator

do you need this notification?

Isn't this the API to download a mission? I.e. the proto for mavlink/MAVSDK#2463?

@jonathanreeves
Copy link
Author

This is ok, but my question is: do you need this notification? All you care about is if the mission is uploaded, so you are given a new mission, right? You don't really need to know when someone downloads it, I would think, it's more of an internal thing.

Yeah it's a great question, and one that I've definitely gone back and forth on. I think you could make a weak argument that app code would want the option to know if something went wrong during an air -> ground mission download, but if it succeeds you probably don't care. Even still it could be viewed as an internal thing that's happening behind the scenes, in some ways it's cleaner for users of the SDK to not have to worry about it. Honestly I'm very comfortable throwing these changes away if you think that's best for the API. I was sort of just erring on the side of being explicit and make sure error information wasn't hidden. Just let me know your thoughts, I will not at all be offended if we think the best thing is just close this and keep the error status hidden behind the scenes.

@jonathanreeves
Copy link
Author

do you need this notification?

Isn't this the API to download a mission? I.e. the proto for mavlink/MAVSDK#2463?

It is, but my guess is this is error information that most users of the SDK (myself including honestly) are not super likely to use. There's no application interaction required for a ground station to request and stream a mission plan from the air vehicle.

@julianoes
Copy link
Collaborator

I would say that if you are using it or intending on using it, then that means there is a use case and you should leave it. If not, I'd probably pass on it.

@jonathanreeves
Copy link
Author

I would say that if you are using it or intending on using it, then that means there is a use case and you should leave it. If not, I'd probably pass on it.

I doubt we'll be using it anytime soon so I'll go ahead and close this. I'm still planning to open a proto PR with the changes to make these methods async (as discussed in the mission download change), but we can deal with that separately.

@julianoes julianoes reopened this Dec 13, 2024
@julianoes julianoes merged commit 7d1776a into mavlink:main Dec 13, 2024
3 checks passed
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.

3 participants