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

Add _position_lock_gained bool and associated logic remaining to close PR #6523. #12769

Closed
wants to merge 1 commit into from

Conversation

mcsauder
Copy link
Contributor

@mcsauder mcsauder commented Aug 21, 2019

Describe problem solved by the proposed pull request
Nearly all of the changes proposed in PR #6523 have been incorporated in PX4:master at this point. This PR captures the only work remaining from PR #6523 so that it can be closed without losing the remaining work.

Test data / coverage
The changes in this PR affect the state of the local bools some_switch_changed and should_evaluate_rc_mode_switch in Commander::set_main_state_rc(). If position_lock_gained is false, the bool should_evaluate_rc_mode_switch will evaluate false and the method will return TRANSITION_NOT_CHANGED.

Anytime position_lock_gained is true, the results in Commander will be unchanged. The instances of position_lock_gained = false need careful examination. (I believe I have captured @LorenzMeier's original intent, but the logic in Commander.cpp has changed quite a lot from when the PR was originally proposed so I could use a hand ensuring this logic is correct.)

Flight tests on a 250 quad and pixhawk 4 mini performed identical to PX4:master, position lock is identified, the position tone plays and led color change is observable in the same way as upstream, and flight mode changes are obeyed from the RC transmitter:

Additional context
See PR #6523. @dagar and @julianoes , this PR is as we discussed in the dev call.

Hi @LorenzMeier , would you be willing to look this PR over and see if I've captured your intent?

All, please feel free to close this PR, (and #6523), if these changes are no longer required.

Thanks!

-Mark

Copy link
Contributor

@julianoes julianoes left a comment

Choose a reason for hiding this comment

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

I think the changes could work but I think the right person to review that would be @MaEtUgR.

@mcsauder mcsauder force-pushed the mode_switch_automodes branch from 8b95f22 to 1f1696b Compare September 3, 2019 22:10
@mcsauder
Copy link
Contributor Author

mcsauder commented Sep 3, 2019

Hi @MaEtUgR , would you be willing to have a look at this PR and let me know if these proposed changes should be held on to? (If not please feel free to close this PR or let me know to do so!) Thank you!

@mcsauder
Copy link
Contributor Author

Hi @MaEtUgR , what do you think, should we pursue these changes from PR #6523 or just let them go?

@mcsauder
Copy link
Contributor Author

Closing per devcall discussion. Thanks everyone!

@mcsauder mcsauder closed this Sep 18, 2019
@mcsauder mcsauder deleted the mode_switch_automodes branch September 18, 2019 16:07
@MaEtUgR
Copy link
Member

MaEtUgR commented Oct 21, 2019

I think that was added in #11915

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.

3 participants