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

Plane: match cork() and push() #28825

Closed
wants to merge 1 commit into from

Conversation

andyp1per
Copy link
Collaborator

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.

@Hwurzburg
Copy link
Collaborator

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);
Copy link
Contributor

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

Copy link
Member

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.

@IamPete1
Copy link
Member

IamPete1 commented Dec 9, 2024

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 APM_BUILD_TYPE defines in other parts of the hal. Before we rush this fix in it might be worth double checking the other vehicle types, we may need a similar fix elsewhere at which point a define to error on copter only might be a better option.

@IamPete1
Copy link
Member

IamPete1 commented Dec 9, 2024

I wonder if we could add the same error hal SITL. That would make testing easier at least.

@andyp1per
Copy link
Collaborator Author

@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.

@andyp1per
Copy link
Collaborator Author

Here's the alternative if you prefer @IamPete1 - #28828

@IamPete1
Copy link
Member

IamPete1 commented Dec 9, 2024

We merged the other one.

@IamPete1 IamPete1 closed this Dec 9, 2024
@andyp1per andyp1per deleted the pr-plane-cork branch December 10, 2024 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New cork check fails on dev4.7 build on autopilots
4 participants