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

Moved 'imminent collision' state #1031

Merged
merged 1 commit into from
Sep 1, 2014

Conversation

m4gr3d
Copy link
Member

@m4gr3d m4gr3d commented Sep 1, 2014

Moved imminent collision state from the drone State to its Altitude.

m4gr3d added a commit that referenced this pull request Sep 1, 2014
@m4gr3d m4gr3d merged commit d8b2353 into DroidPlanner:master Sep 1, 2014
@m4gr3d m4gr3d deleted the cleanup_crash_alert branch September 1, 2014 23:04
@Thalek
Copy link

Thalek commented Sep 2, 2014

Observation: "imminent crash" would certainly belong in "Altitude", as far as nomenclature is concerned. But you've changed the name from "Imminent Crash" to "Imminent Collision" as a prelude to implementing collision detection and avoidance. Once that progresses further, will "Altitude" still be the appropriate place for those functions?

@arthurbenemann
Copy link
Member

@Thalek its more of a concern of software architecture than nomenclature (names are easy to change). The idea here is to move the method to the class the contain the most fields it needs to acess , thus reducing dependencies and coupling of multiple classes.

@Thalek
Copy link

Thalek commented Sep 2, 2014

Thank you for the clarification, Arthur.

I presume this class is anticipated to still be the best choice when more complicated collision/avoidance software is developed.

I really wish I knew enough programming to offer more than the occasional semi-informed comment. [wry smile]

@squilter
Copy link
Member

squilter commented Sep 2, 2014

@Thalek you're completely right. In a few years the collision detector will be implemented in flight code and it will be able to detect more than just altitude-related collisions. But that's still years away and we can just move it later.

@Thalek
Copy link

Thalek commented Sep 2, 2014

Thank you, Squilter.

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.

4 participants