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

[v2.12] mavsdk_server: fix autopilot discovery #2388

Merged
merged 1 commit into from
Sep 2, 2024
Merged

Conversation

julianoes
Copy link
Collaborator

Instead of using the subscribe_on_new_systems callback, we just use a while loop here. That's because the subscribe_on_new_system callback won't tell us if an autopilot is discovered when another component of the same system is discovered first, thus triggering the callback early when no autopilot is around.

With a stupid old while loop we can just keep checking this until we have an autopilot and then exit.

Instead of using the subscribe_on_new_systems callback, we just use a
while loop here. That's because the subscribe_on_new_system callback
won't tell us if an autopilot is discovered when another component of
the same system is discovered first, thus triggering the callback early
when no autopilot is around.

With a stupid old while loop we can just keep checking this until we
have an autopilot and then exit.
Copy link

sonarcloud bot commented Sep 2, 2024

Copy link
Collaborator

@JonasVautherin JonasVautherin left a comment

Choose a reason for hiding this comment

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

But then what happens when MAVSDK does not connect to an Autopilot? E.g. when running as an autopilot itself?

This assumes that there is always an autopilot on the other side, doesn't it?

@julianoes
Copy link
Collaborator Author

@julianoes julianoes merged commit 71126ac into v2.12 Sep 2, 2024
32 checks passed
@julianoes julianoes deleted the pr-server-discovery branch September 2, 2024 19:54
@julianoes
Copy link
Collaborator Author

Thanks @JonasVautherin. I'm going to deploy and test this now 🤞.

@JonasVautherin
Copy link
Collaborator

Yes, but that was the assumption before as well:

Oh, right 🤦‍♂️

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