-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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
Plane: match cork() and push() #28825
Conversation
this allowed me to get thru prearm.....removing the check is what I was going to fly with today if this hadn't been created... |
@@ -1163,7 +1163,7 @@ class Plane : public AP_Vehicle { | |||
void dspoiler_update(void); | |||
void airbrake_update(void); | |||
void landing_neutral_control_surface_servos(void); | |||
void servos_output(void); | |||
void servos_output(bool cork = true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this. It's not my area but it might be safer to remove the default from the argument and force the caller to make a choice.. maybe add a comment to help them make the choice. Just an idea though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, the default hides the other users (here on github anyway) which are important to understand the scope of the change.
I'm not sure its really fair to call this a bug. I agree its a bit odd, but plane had been working fine like this for years. I would like to tidy up the oddness at some point. I'm slightly hesitant to change plane to fix a check that was added for a feature that plane does not use, seems backwards. We do have |
I wonder if we could add the same error hal SITL. That would make testing easier at least. |
@IamPete1 I am happy to instead remove the internal error if @tridge and @peterbarker agree - it was added because I was assured it should be safe 😄 - I don't really have strong opinions either way. |
We merged the other one. |
Hopefully closes #28823
I'm kind of in two minds about this internal error now. We could just remove it, but lets see how far the rabbit hole goes.