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

Consolidate components #2169

Merged
merged 3 commits into from
Dec 17, 2023
Merged

Consolidate components #2169

merged 3 commits into from
Dec 17, 2023

Conversation

julianoes
Copy link
Collaborator

@julianoes julianoes commented Oct 30, 2023

This should make dealing with the configuration and components a bit clearer.

This is the last step towards v2 🤞.

Closes #2125.

Base automatically changed from pr-split-mission-transfer to main November 4, 2023 05:06
@julianoes
Copy link
Collaborator Author

@JonasVautherin and @ThomasDebrunner I'm thinking of changing the API to require a default, so from:

Mavsdk mavsdk;

to:

Mavsdk mavsdk{Mavsdk::ComponentType::GroundStation};

What do you think? And should it be a breaking change or deprecation notice if you use it without constructor?

Related to #2125.

@JonasVautherin
Copy link
Collaborator

And should it be a breaking change or deprecation notice if you use it without constructor?

For me it is fine to completely remove the default and make the build fail if no value is passed. It's fine because it will be a major update, and it's very easy to fix ("oh, it now requires an argument"). Also it's good to make it explicit since apparently some people got confused by the fact that MAVSDK may behave as a ground station by default.

This is an attempt to avoid spurious segfaults happening in some of the
CI tests for some of the system tests. Presumably they happen on the
quick destruction of the objects right after construction.

Signed-off-by: Julian Oes <[email protected]>
This should make dealing with the configuration and components a bit
clearer.

Signed-off-by: Julian Oes <[email protected]>
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.

Nice! 🚀

@julianoes julianoes merged commit 6d50b92 into main Dec 17, 2023
27 checks passed
@julianoes julianoes deleted the pr-consolidate-components branch December 17, 2023 23:17
@julianoes julianoes restored the pr-consolidate-components branch December 17, 2023 23:20
@julianoes julianoes mentioned this pull request Dec 27, 2023
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants