Quicktune C++ conversion (WIP) #27578
Conversation
|
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. |
MichelleRos
force-pushed
the
pr/quicktunecpp
branch
from
8bd7e18
to
e583032
Compare
|
very nice to see this. Great to see quiktune graduate to C++! |
|
need to be in build_options.py and extract_features.py |
|
@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. |
@lthall We could limit it to only work in certain modes if that'd be 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. |
|
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. |
|
We should add Guided to the list of acceptable modes I think. |
|
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:
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:
|
MichelleRos
force-pushed
the
pr/quicktunecpp
branch
from
e583032
to
dc6d15b
Compare
|
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 |
100% agree, I think this would be a much better structure and save substantially on flash costs |
tridge
force-pushed
the
pr/quicktunecpp
branch
from
ea02881
to
b2f51a4
Compare
|
@MichelleRos I've pushed some updates and rebased |
tridge
force-pushed
the
pr/quicktunecpp
branch
6 times, most recently
from
6e76e07
to
be1d97f
Compare
tridge
force-pushed
the
pr/quicktunecpp
branch
3 times, most recently
from
8c00081
to
1be3282
Compare
| if (auto_filter <= 0) { | ||
| BIT_SET(filters_done, uint8_t(axis)); | ||
| } |
| if (slew_parm != Param::END) { | ||
| float P = get_param_value(slew_parm); | ||
| AxisName axis = get_axis(slew_parm); | ||
| // local ax_stage = string.sub(slew_parm, -1) |
C++ version of lua script, with some enhancements
disabled by default except in SITL
replaces QAUTOTUNE in default build
for quadplane and copter
now disabled by default
this is a separate commit in case it isn't to the taste of the reviewer It saves around 500 bytes
tridge
force-pushed
the
pr/quicktunecpp
branch
from
1be3282
to
bfe2c33
Compare
tridge
force-pushed
the
pr/quicktunecpp
branch
from
bfe2c33
to
ea4c935
Compare
Add Quicktune to C++ code (conversion from the VTOL-quicktune lua script).
Tested in SITL, multiple multcopters and quadplanes
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.