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

Refactor OSD flight mode logic #3790

Merged
merged 4 commits into from Aug 26, 2018

Conversation

Projects
None yet
3 participants
@teckel12
Copy link
Contributor

commented Aug 24, 2018

As of 2.0.0 POSHOLD always includes ALTHOLD, so remove that unused legacy logic. Also reformatted as per coding style.

teckel12 added some commits Aug 15, 2018

Refactor OSD flight mode logic
As of 2.0.0 POSHOLD always includes ALTHOLD, so remove that unused legacy logic.  Also reformatted as per coding style.
}

if (FLIGHT_MODE(FAILSAFE_MODE)) {
if (FLIGHT_MODE(FAILSAFE_MODE))

This comment has been minimized.

Copy link
@digitalentity

digitalentity Aug 25, 2018

Member

Please restore the { } coding style

This comment has been minimized.

Copy link
@teckel12

teckel12 Aug 26, 2018

Author Contributor

Really? The coding style standard document for INAV states the opposite. I only made this change to conform to the INAV coding style standard documentation because it was mixed style previously.

This comment has been minimized.

Copy link
@teckel12

teckel12 Aug 26, 2018

Author Contributor

See:
https://github.com/multiwii/baseflight/wiki/CodingStyle

Which is linked from: https://github.com/iNavFlight/inav/blob/master/docs/development/Development.md

And states:

if/else blocks which only contain a single line body must not use opening/closing braces:

This comment has been minimized.

Copy link
@digitalentity

digitalentity Aug 26, 2018

Member

Hm... I remember discussing this specific case. I'll create an issue to discuss coding style 😄

This comment has been minimized.

Copy link
@fiam

fiam Aug 26, 2018

Member

That document is outdated and has some questionable choices that no one is following anymore. The fix is updating the document, not following it blindly.

This comment has been minimized.

Copy link
@teckel12

teckel12 Aug 26, 2018

Author Contributor

Sorry, was only trying to follow the documented style guide, I have no horse in this race. I've just always followed the rule that if you touch the code, fix it as well.

This comment has been minimized.

Copy link
@fiam

fiam Aug 26, 2018

Member

Understood, no worries :-). I just saying that Devekopment.md needs some modernization. Among other things, we should always use braces.

Irrelevant

@digitalentity digitalentity merged commit bdb9ef9 into iNavFlight:development Aug 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@digitalentity digitalentity added this to the 2.1 milestone Aug 26, 2018

p = "CRS ";
} else if (FLIGHT_MODE(NAV_ALTHOLD_MODE) && navigationRequiresAngleMode()) {
else if (FLIGHT_MODE(NAV_ALTHOLD_MODE) && navigationRequiresAngleMode()) {

This comment has been minimized.

Copy link
@fiam

fiam Aug 26, 2018

Member

And here we have the reason why you should always add braces. Otherwise it leads to inconsistent and difficult to read code.

}

if (FLIGHT_MODE(FAILSAFE_MODE)) {
if (FLIGHT_MODE(FAILSAFE_MODE))

This comment has been minimized.

Copy link
@fiam

fiam Aug 26, 2018

Member

That document is outdated and has some questionable choices that no one is following anymore. The fix is updating the document, not following it blindly.

@digitalentity

This comment has been minimized.

Copy link
Member

commented Aug 26, 2018

@fiam @teckel12 let's revisit coding style in #3806 and fix code once we have a common ground,

iioel added a commit to iioel/inav that referenced this pull request Aug 29, 2018

Refactor OSD flight mode logic (iNavFlight#3790)
As of 2.0.0 POSHOLD always includes ALTHOLD, so remove that unused legacy logic.  Also reformatted as per coding style.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.