ardupilot
ardupilot copied to clipboard
ArduPilot
Quicktune C++ conversion (WIP)
Add Quicktune to C++ code (conversion from the VTOL-quicktune lua script).
Just doing Copter for starters.
Tested in SITL, and one flight on a 5" copter.
Still a work in progress.
Edit: Tridge has added support for quadplanes and an autotest for it and made it not run for helis. I plan to add an autotest for the copter version shortly. I have also updated the PR to limit quicktune to only alt_hold and loiter in copter, and Tridge did something similar for quadplanes.
Edit 2: Tridge updated it to make it always allocate, to be more in line with the pattern in copter, and moved it to its own thread. He also added a max attitude error, so it auto-aborts if the vehicle starts oscillating badly during the tune and made it abort the tune if the pilot changes to a mode that doesn't support quicktune. Note that it does not abort if the pilot changes between modes that do support quicktune, allowing people to, for example, do the tuning in loiter mode, then switch to alt_hold to test the tune before saving the gains.
Now tested on a quadplane as well.
As for the flash concerns, yes, it uses around 5k of flash, but autotune uses around 15k of flash, so for a tuner, quicktune is actually rather light.
Very nice! Will obviously require some include guards given the flash cost. Not done a review, but I think it would be better to try and combine the autotune parameter limits here. Ultimately I think this should be a function of the autotune module rather than a separate thing so that eventually we can have a "fast" autotune.
@tridge thanks for including me on this PR. I will have to learn more about how the quick tune works. Copters and heli’s have a very different way of tuning. For instance, heli use the feedforward gain where copters do not. Also they tune their yaw axis in a very different way. so at this point, I’m not saying no, but I will have to have somebody show me more about how Quik tune works. I’ve been toying with an idea of how to do a similar quick tune using the frequency domain tuning.
This looks like it is something that can be turned on at any time, in any mode. Is that correct?
I think this needs to be done as a separate flight mode. I am not comfortable with making this something that just runs over any flight mode.
I am also not comfortable with this being stuck in the mode.ccp file like it is as fundamental as the attitude controller.
This looks like it is something that can be turned on at any time, in any mode. Is that correct?
@lthall Yes, that is the intent. IMO it makes for a much more user-friendly interface than a separate mode where you have to get used to the different behaviour along with watching the tuning process, which is already a risky & stressful time... I have been taken by surprise by autotune's different behavioura few times and had near-crashes because of that. One of the big advantages of quicktune is that while tuning, it just hovers there, with almost no difference to hovering normally in e.g. loiter mode, so it's much less risky. It also reduces code duplication - to make this a mode, I would have to copy all of e.g. mode Loiter into a new file and then add the quicktune stuff to it and change the name...
We could limit it to only work in certain modes if that'd be better? Also, the current function in mode.cpp is more complex - I am working on a simpler method, using the aux functions as per Tridge's feedback, although there will still be ~3 lines in mode.cpp. I could add it only to the modes we want it to work in, rather than to mode.cpp if that's better?
We can't have this turn on in any mode. At very least we would need to restrict this to manual modes and not during failsafe events.
There is a wider requirement for a code pocket that is constantly running where we make adjustments to the gains. That would be a good place for the ground resonance handling to reside for example. We also have a future problem if we run the rate controllers at a higher update rate that may also be able to make use of the same pocket.
@MichelleRos in my opinion it should be a feature that can be activated only in Position Hold and Loiter. Thank you for this, useful here but it would also be useful for VTOLs without using LUA.
For what it is worth, I am +1 for not having it turn on in any mode. I think that was acceptable when it was a script because no script, no problem. As this is going into cpp I think this needs to have the same constraints on it as the current autotune.
As this is essentially an alternative approach to the current autotune I think it would be best if this inherited from AC_Autotune. Fundamentally, we are doing a test, measuring the response, fiddling the gains, and repeating. There must be enough commonality with the current autotune that we can make this an "Autotune type" or "Autotune option". For example, heli has a different autotune that it inherits from the base class. The main difference here is that we would need some kind of auto tune _MODE or _OPTION param to select which version of autotune you wanted.
From a user perspective I think it is easier if the interface with autotune is kept as similar as possible to the existing method (hence my suggestion to inherit from the existing). It is when we have things that appear, externally, to be very different things that users get confused. I know I certainly do.... 😄
100% agree with @MattKear - I think the bar is higher for this to go into C++ and it needs to be done in a sympathetic way to the existing code. It also needs to be done in a way that doesn't confuse users. Quiktune and autotune both have pros and cons and I think the way we have been advising users to get an initial tune with quiktune before progressing to autotune is a not unreasonable one - but I think ideally this would be built into autotune itself so that you could still use the same parameters for selecting (e.g.) axis. I think currently the parameter options for quiktune are too complex and need to be slimmed down a bit in the same way that the parameter set for autotune is very simple. I can imagine a future autotune first doing a quiktune and then an autotune in the same pass and would not want the configuration to create a user expectation of either/or.
I agree with @lthall that it should be limited to manual modes. in addition, I'm not sure why we would let the user choose a mode other than alt-hold or stabilize. This is being used to tune an aircraft that may not even be able to successfully fly in loiter or guided modes. So why tune in those modes?
It might be helpful to put the use case slightly differently @bnsgeyer - sure that might be true, but in my experience (mostly quadplanes, a couple of multi-rotors) the the use case for QuickTune is that the vehicle can fly in Q-Loiter, but not well, and specifically not well enough for autotune so you run quick tune until QLoiter is "ok". I have usually used QuickTune like this:
- manual tune in QStabilize/QHover until it can QLoiter (Loiter in multi rotor)
- quicktune in QLoiter/Loiter to get it "good enough" that autotune will run
- autotune
Which is also what how the wiki says to use it. Which to me means - the mode I want quicktune to work in most of all is: QLoiter/Loiter
Quicktune is there to help people get from a very poor tune to a basic tune. It is promoted as safer, easier and more likely to be successful than AutoTune.
Quicktune is supposed to replace a basic manual tune. In my recommended tuning process manual tuning happens before AutoTune and before any position controlled mode is used.
If we let it be run in guided we will have people loading ArduCopter on a brand new aircraft and expecting to press take-off then switch on Quicktune.
Now we have 12 parameters (one an options bitmask) to make it easier for people who struggle to do a manual tune safely and we are talking about letting it run in automated modes. This just seems a little crazy to me.
As a big proponent of scripting I concur that I would like to see this remain a script instead of the overhead of flash and maintenance of C++ code.
I have been working on improvements to make scripts more feasible on autopilots with less free RAM, which should enable the quicktune script to be more widely useful. I don't know if it's in the cards for boards with <=1M flash or no SD slots though.
Assuming we elevate this to C++ and don't just leave it as a LUA script the key things I would like to see are:
- Limited to Alt_Hold and Loiter modes. (Pos_Hold could be included and I need to hear why Randy thinks Guided would be a good idea)
- Automatically stop running if the pilot is asking for a non zero climb rate.
- Reduce the footprint in Copter::update_flight_mode() to a single line and put code in a separate function.
- Change percentage and percentage reduction to simple multiplication factors, consistent with Copter.
I wonder if this would be better as a new type of autotune. The existing mode autotune already has all the functionality it needs to hook into a mode and does the pilot override thing and can get at all the params it needs directly.
My suggestion would be a new "AUTOTUNE_TYPE" param you could select the existing one or quicktune. I think you would have to pull out a common parent class, but you only need a run, save, stop and reset methods and no changes to copter except it would need to decide which one to use when you enter the mode for the first time.
I wonder if this would be better as a new type of autotune. The existing mode autotune already has all the functionality it needs to hook into a mode and does the pilot override thing and can get at all the params it needs directly.
My suggestion would be a new "AUTOTUNE_TYPE" param you could select the existing one or quicktune. I think you would have to pull out a common parent class, but you only need a
run,save,stopandresetmethods and no changes to copter except it would need to decide which one to use when you enter the mode for the first time.
100% agree, I think this would be a much better structure and save substantially on flash costs
Having reviewed, I definitely think this should be done as a subclass of autotune - autotune was refactored exactly for this reason for Heli and a lot of the flash cost is the surrounding infristructure that autotune already handles. This would also be less confusing for the user and sets the stage for iterative future improvements.
Pulled in @tridge's code to add quadplane support, and autotest, and turning off quicktune on helis. Thanks Tridge!
Also fixed compilation on certain boards, simplified the code in mode.cpp and made quicktune only able to be run in Alt_hold, Loiter or Guided. Note also that if a person never uses quicktune's aux function, quicktune won't even get allocated (only has a nullpointer), which helps with memory use and ensures it won't run unless people want it. It also has an enable parameter that people need to set to enable it.
From a user perspective I think it is easier if the interface with autotune is kept as similar as possible to the existing method
I agree with the sentiment, but not the conclusion. The "existing method" is actually the VTOL quicktune script. We have for some time been recommending that as the initial tuning approach. Many users never run autotune. So keeping the workflow similar to the existing VTOL quicktune script I think is a good thing. I plan on removing QAUTOTUNE from quadplanes after this PR goes in, with it being available only with an option on the custom build server or in SITL.
I strongly disagree on making this a subclass of autotune. I think that will entangle the code too much. The autotune structure is already very complex and adding this in will make it even more so.
I also think the workflow in autotune is really hard for many first time users. The workflow in quicktune is much easier for new users.
@MichelleRos PR to fix the param meta-data issues: https://github.com/MichelleRos/ardupilot/pull/2
I just realised that this doesn't tune Angle P for roll, pitch and yaw. Have I got this right? This would mean that the best case outcome is a poor but stable tune. Good if you are able to do autotune afterwards or are then going to manually tune Angle P.
It also appears to tune yaw as if it is roll and pitch, always setting D to be non-zero. (this is dangerous)
I have opened a feature request here to discuss alternative approaches to this PR. #27624
a poor but stable tune
It is certainly not a poor tune. It's not gonna be absolutely perfectly optimal for the vehicle, sure, but that's what autotune is for. The point of quicktune is to quickly get a stable and safely flyable tune.
a poor but stable tune
It is certainly not a poor tune. It's not gonna be absolutely perfectly optimal for the vehicle, sure, but that's what autotune is for. The point of quicktune is to quickly get a stable and safely flyable tune.
Not setting or even evaluating Angle P is not even a basic tune. It is most certainly a poor tune. Lets call a spade a spade here. Quicktune is potentially a way of getting an aircraft to a minimally stable tune. And that assumes you have reset Angle P to a conservatively low value so I am being generous even with that statement.
stable tune
That was exactly the point I was making... And repeatedly calling it a "poor" tune is very disparaging of quicktune (and kinda rude, in my opinion) given that it has actually helped many people get from a tune that might crash their vehicle at any moment to a tune that won't.
I'm pretty sure even my 5" quad that gets angle_p gains over 20 for roll & pitch still flies fine on 4.5 - in fact, other than flying Acro mode with FPV goggles, I kinda doubt I'd be able to tell the difference without squinting at logs.
Not setting or even evaluating Angle P is not even a basic tune. It is most certainly a poor tune. Lets call a spade a spade here.
numerous people who have actually used quicktune find it good. It works extremely well. Yes, autotune when it works can produce a better tune, but quicktune quickly gets someone something flyable.
Quicktune or at least a quicktune like tuning approach has obvious value. And the LUA script and both your advocacy for it has clearly demonstrated a justification for it. That is why I started the feature request to give people an opportunity to provide feedback on how it should be integrated into the code and what features are required. To do this we also need to have a clear understanding of both it's strengths and weaknesses and where it fits into the tuning process.
The LUA script may result in a good initial tune and a dramatic improvement over the defaults on some aircraft but also be a poor tune even dangerous when evaluated as a final tune. Quicktune has been billed as being able to replace Autotune and Manual tuning making it fair to evaluate it as a final tune.
I would encourage you interoperate my description as simply an accurate engineering evaluation in the context of multirotor performance expectations and put the personal stuff aside.