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

FW Position control: switch from L1 to NPFG guidance by default #19603

Merged
merged 1 commit into from
Dec 6, 2022

Conversation

sfuhrer
Copy link
Contributor

@sfuhrer sfuhrer commented May 6, 2022

Now with the next release branched off I think it would be a good moment to start transitioning from L1 to the NPFG fixed-wing guidance. .
It will give us more robustness in wind, deterministic flight paths and cleaner interfaces.
As a first step I would propose to make it default but keep the L1 option, and get more testing feedback in prior to removing the legacy L1 controller.

Signed-off-by: Silvan Fuhrer <silvan@auterion.com>
@sfuhrer sfuhrer requested review from dagar, RomanBapst and tstastny May 6, 2022 11:27
@tstastny
Copy link

I see no downside to this (and keeping both is becoming a bit of a headache in the code) - but probably we should hear from others who weren't the one who wrote the new library in question @dagar @RomanBapst @Jaeyoung-Lim

Copy link
Member

@Jaeyoung-Lim Jaeyoung-Lim left a comment

Choose a reason for hiding this comment

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

This will change the behavior of all previous fixedwing platforms if the existing planes have been tuned for L1.

While I agree that there is no harm on making it default for SITL vehicles, shouldn't we at least keep the existing air frames(real airframes) to keep using L1 for real vehicles unless it has been flight tested with NPFG?

If they are not maintained, we might as well delete them

@tstastny
Copy link

This will change the behavior of all previous fixedwing platforms if the existing planes have been tuned for L1.

this is not actually true. the period and damping params are one-to-one reusable. only in very high winds does the behavior change.

@dagar
Copy link
Member

dagar commented May 10, 2022

I see no downside to this (and keeping both is becoming a bit of a headache in the code) - but probably we should hear from others who weren't the one who wrote the new library in question @dagar @RomanBapst @Jaeyoung-Lim

Unless we want to look at thoroughly testing both on an ongoing basis I think we should figure out a full (safe) transition plan and fully drop L1 once we're confident.

@tstastny
Copy link

a full (safe) transition plan

@dagar should I start an issue to populate some objectives? e.g. (off the top of my head):

  • how to transfer tuning of existing systems
  • SITL (functional) tests for various waypoint switching behaviors (there have been a couple hiccups in the last months, as typically expected. Just had to add a condition to navigator.. but these are things that need to be flushed out)
  • flight testing bare minimums before transition (hours? platforms? modes?)

or would here and/or another place be better to organize this?

@Jaeyoung-Lim
Copy link
Member

this is not actually true. the period and damping params are one-to-one reusable. only in very high winds does the behavior change.

@tstastny Right, but isnt the parameter name different and if you have a custom period, it will roll back to the default with this change?

@sfuhrer
Copy link
Contributor Author

sfuhrer commented May 11, 2022

@tstastny Right, but isnt the parameter name different and if you have a custom period, it will roll back to the default with this change?

We could add them to the param translation. Though I wonder if that doesn't let to worse behavior than setting them to the new default (10), given that the old default was 20. @tstastny why again is there so much room between the old and new default? Did the old one have to be that big bc the controller itself was less robust?

@tstastny
Copy link

Npfg has the optional functionality to automatically increase the period when the condition and current tuning would result in unstable behavior (assuming one has properly identified the roll time constant). So a lower period can be set without worrying. -- as to why 10s specifically, that was my preferred tuning for an easyglider. But has also been used on larger aircraft. 20s for l1 was probably set before as you mention to be a safe default for most aircraft.

We could bump to 20 for default if folks felt safer.

@dagar dagar merged commit 2b8295e into main Dec 6, 2022
@dagar dagar deleted the pr-enable-npfg-by-default-master branch December 6, 2022 17:54
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.

4 participants