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

Fixed wing altitude control fixes #10541

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

breadoven
Copy link
Collaborator

@breadoven breadoven commented Dec 16, 2024

Should reduce the possibility for pitch instability issues such as #10536. Also increases value of nav_fw_alt_control_response which testing shows can be set higher for improved response.

Tested on HITL as far as it can be but also via flight testing which tends to indicate a lower PID P setting reduces the potential for pitch instability providing a safer starting point for further tuning.

@breadoven breadoven added this to the 8.0 milestone Dec 16, 2024
@mmosca
Copy link
Collaborator

mmosca commented Dec 18, 2024

@breadoven how much of it is a reflect of bad mechanical setup and how much of it is inav being too aggressive on the defaults?

@breadoven
Copy link
Collaborator Author

@breadoven how much of it is a reflect of bad mechanical setup and how much of it is inav being too aggressive on the defaults?

The altitude control method changed from position to velocity so it's not unexpected that the PIDs might need tweaking from the current values. Others testing 8.0 have had similar pitch instability issues. The basic default for P is 40 but the Configurator defaults are 25 for a wing with a tail and 35 for a wing without a tail so both lower than 40 and in fact 30 is between these values. I guess the Configurator default settings should also be adjusted, certainly for flying wing at least, I'm sure there shouldn't be a problem for planes with a tail using the current value.

@mmosca
Copy link
Collaborator

mmosca commented Dec 19, 2024

@breadoven The configurator defaults are probably what most users will encounter first. Unfortunatelly, this would also be brough over in a diff.

So we would need to both change the values in the configurator for those and mention in the release notes that users may want to lower vertical P and increase nav_fw_alt_control_response. Do you have suggested values for wing without a tail?

Another option is to apply this pr and mention it in configurator, but change the profile values after 8.0 is out and we get more feedback on the effect of these changes.

@b14ckyy
Copy link
Collaborator

b14ckyy commented Dec 19, 2024

@mmosca if we control velocity now and not position, I think between Elevator and Delta mix setups there should be no relevant difference anymore. IMHO we can just remove these entries from the presets.

I am just not sure about the diff transfer of the old values in this case. Should we rename to prevent it or leave it in release notes that 80% don't read?

@mmosca
Copy link
Collaborator

mmosca commented Dec 19, 2024

If the default profile values work, I don't think we need to rename it. The values are close to the defaults for wing with tail anyway.Mentioning it in the release notes and announcements should be enough.

@breadoven
Copy link
Collaborator Author

I think we should just apply this PR and mention the change in altitude control in the release notes, specifically that the PID settings for nav_fw_pos_z may need rechecking and adjusting or resetting to defaults then retuning. Shouldn't need to mention changing nav_fw_alt_control_response in the release notes given it's a new setting, would be useful though to highlight it as something that will affect altitude control behaviour.

And probably best to remove the Configurator profile settings and only reapply them if need be when more feedback is available. I'd have thought it makes sense they should be different simply because flying wings seem to have a noticeably more sluggish pitch response than planes with a tail. Having said that the conventional motor glider I have uses a nav_fw_pos_z_p of 30 rather than 25 and flies fine so coming up with a universal setting is never going to work in all cases anyway.

@mmosca mmosca added the Release Notes Add this when a PR needs to be mentioned in the release notes label Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release Notes Add this when a PR needs to be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants