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

Revert "mavlink: GLOBAL_POSITION_INT send without lat/lon availability" #15396

Merged
merged 1 commit into from
Jul 22, 2020

Conversation

LorenzMeier
Copy link
Member

@LorenzMeier LorenzMeier commented Jul 22, 2020

Reverts #15308

85% of PX4 contributors are working with commercial vendors. So our contributors expect commercial quality out of this open source project, not some hobby entertainment. The fact that APM has been doing this for years doesn't make it a valid approach. Entering magic values into a valid range is against best practices. It is unlikely that a lat / lon of 0/0 ever hits a real system - yes. But if I were to review the PX4 codebase and found stuff like that I would loose trust into the implementers because very apparently they are too lazy to implement things correctly and have all sorts of magic easter eggs hidden.

And magic values are fine until they are not. And it is very simple to objectify this: If a unit test that does an input data sweep would fall over, then it is a bad change. In this case a sweep across all possible coordinates would generate an incorrect result here, so it is a bad change.

@julianoes julianoes requested a review from dagar July 22, 2020 09:52
@dagar dagar requested a review from DonLakeFlyer July 22, 2020 13:42
@dagar
Copy link
Member

dagar commented Jul 22, 2020

I don't disagree, but I view this as a Mavlink issue + workaround and certainly not tolerating magic values in general.

@DonLakeFlyer when you're available let's discuss the original issue you were trying to solve.

@dagar dagar merged commit 3b1be7d into master Jul 22, 2020
@dagar dagar deleted the revert-15308-pr-mavlink_global_int_continuous branch July 22, 2020 14:34
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