-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Autotune enabled in GQC. #9904
Autotune enabled in GQC. #9904
Conversation
@bkueng and @MatejFranceskin could you have a look? Some things from the model are still missing and I'm working on them. The reason I drafted a PR is because the change is big and I'm guessing there will be comments to address. |
35b3c08
to
445e33d
Compare
On the UI side I see 2 things we need to make sure:
Not sure how to best reflect that, maybe only show the button for the rate controller tab and mention that it applies to both rate + attitude? |
3a54b64
to
6cc71ff
Compare
src/Vehicle/Autotune.cpp
Outdated
stopTimers(); | ||
_autotuneInProgress = false; | ||
_disarmMessageDisplayed = false; | ||
_autotuneStatus = tr("Autotune: Vehicle failed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_autotuneStatus = tr("Autotune: Vehicle failed"); | |
_autotuneStatus = tr("Autotune: Failed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already fixed.
Yes, same for fixedwings, only the rate and attitude controllers are tuned (not velocity and position), so the autotune button should only appear on the rate and attitude loop tabs. We could add the following message above the button: "By clicking on the "Autotune" button below, the drone will automatically tune its angular rate and attitude controllers". |
a30b412
to
d2439aa
Compare
@bkueng @bresch @bkueng and @bresch |
@batinkov Thanks. We might want to show something different in the plot at some point (e.g.: currently computed PID parameters, convergence of the algorithm, ...) but it requires more changes (e.g.: passing the data through mavlink) so it's good like that for now. |
src/QmlControls/CMakeLists.txt
Outdated
@@ -50,6 +50,7 @@ add_custom_target(QmlControlsQml | |||
EditPositionDialog.qml | |||
ExclusiveGroupItem.qml | |||
FactSliderPanel.qml | |||
AutotuneUI.qml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tab
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bkueng fixed :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for taking so long.
if (autotune->_autotuneInProgress) { | ||
if ((commandResult == MAV_RESULT_IN_PROGRESS) || (commandResult == MAV_RESULT_ACCEPTED)) { | ||
autotune->handleAckStatus(progress); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coding style of if/else
if () {
} else {
}
stopTimers(); | ||
_autotuneInProgress = false; | ||
_disarmMessageDisplayed = false; | ||
_autotuneStatus = tr("Autotune: Failed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there should be better handling of autotune being unsupported by the firmware. As far as I can tell it looks like all the user will get is a "failed" status which is not super clear. The ack will tell if the command is unsupported. In that case the user should get a clear message that Autotune is not supported by this vehicle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bkueng @DonLakeFlyer Can a frame know it supports the feature in advance?
In that case maybe you could send MAV_CMD_DO_AUTOTUNE_ENABLE.param1 with value 0 (no operation) as a test - the vehicle could ack with MAV_RESULT_UNKNOWN to indicate it doesn't understand the command for the current frame type.
It might of course be better to explicitly define that param1=0 is a no-op test.
If not, we could have a protocol bit, or component information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking to add it to a new comp info file, but your other options would work here too.
There's more small bits like this, for example if sensor calibration reset to factory is supported. Comp info would make this simple to extend and we can add as many as we want.
@@ -386,6 +387,8 @@ void Vehicle::_commonInit() | |||
|
|||
_objectAvoidance = new VehicleObjectAvoidance(this, this); | |||
|
|||
_autotune = _firmwarePlugin->createAutotune(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the autotune object really need to be in existence all of the time? Seems like this is only needed when autotune is running. Somewhere in the Tuning Qml hierarchy there is a controller isn't there. Couldn't the autotune C++ code be in there? That way the autoune code is only in existence when the Autotune UI is up.
files added in mavlink#9904 - AutotuneUI.qml was added twice - Autotune.cpp was not added to CMakeLists.txt
files added in #9904 - AutotuneUI.qml was added twice - Autotune.cpp was not added to CMakeLists.txt
@batinkov @bresch Did you guys already have a plan for the "autotuning guide" (referred to in the autotuning prompt)? Here are some questions/things that spring to mind
@bresch There are three tuning guides here: http://docs.px4.io/master/en/config_fw/ and a couple here http://docs.px4.io/master/en/config_mc/ Thoughts on how best to update these? It might be worth starting the relevant ones with a section explaining autotuning and pointing to the bits you still have to do manually. My concern here is that at the moment we support "autotuning" on rate and attitude controllers. However it isn't clear if and when you need to tune position and velocity controllers, and what will happen if you don't. Is autotuning enough that most users can go on holiday, job done? |
Before starting, let me mention that I'm planning to port the following guide to the PX4 doc:
Correct
VTOL transition: no, just tune MC and FW modes independently
I never tried
Yes, it should return "Autotuning: ack error"
Yes
That's strange, I would need to check
The drone can fly in alt/pos/auto modes during the auto-tuning, so the altitude shouldn't vary much. Also, if in pos/auto mode, the horizontal displacement should be small. In alt mode, it'll drift with wind and manual correction attempts will abort auto-tuning at the moment, this is still something we can improve if needed.
You can define that behavior in the
The time is 5s-20s (aborted if did not converge in 20s) per axis + 2s pause between each axis + 4s of testing if the new gains are applied in air. So, minimum of 21s (= 3x5+3x2) without in-air test, 25s with in-air test and a worst case of 70s (if each axis takes exactly 20s to converge). Given the flight we've made so far, it usually takes no more than 40s.
It isn't violent on a standard MC but some larger VTOL planes in MC mode might not like large kicks, this is why the amplitude of the injected control signal (required for system identification) can be adjusted via a parameter (MC_AT_SYSID_AMP and
Yes
AFAIK, the command is sent periodically and the status is returned in the progress field of COMMAND_ACK
The mathematical model used to estimate the dynamics of the drone assumes this it is a linear system with no coupling between the axes (SISO) and with a limited complexity (2 poles and 2 zeros). If the real drone is too far from those conditions, the model will not be able to represent the real dynamics of the drone. |
Thanks @bresch Just my two bits, but I find it a little odd that the autotune button is hidden inside the rate/attitude controller sections. I understand why, but were it me I would
Why? Well this is what most people will want to do and right now it isn't obvious that the autotune buttons on the tabs are linked. Maybe get a UX designer to comment.
That's one way of implementing a client side the long running protocol (LRP). Essentially the LRP says you send a command, it acks periodically with progress until it completes, then it acks with acknowledged. If you ping it with the same message while in progress you get the progress ACK resent - which is what happens here. But you're ignoring the final ACK and just using the last received progress being 100% or disarmed to indicate completion. Can you try out the gazebo? I tried fresh PX4 master and QGC daily today. The autotune button did not appear for multicopter. For plane the PID section did not appear. |
This won't work on ArduPilot, and that is a problem. They interpret this command as "enable autotune". That forces the fixed wing vehicle into an autotune mode. It ACKs with accepted immediately and completes - there is no progress update. While I prefer the PX4 approach, doing something different than the intent is non-standard. It means that a GCS has to do something different for every single flight stack. No blame attributes here - the current command does not document it's assumptions. Further, this implementation is not technically correct as per the long running protocol (though I have added mavlink/mavlink-devguide#405 to make it so if possible). Essentially the flight stack is supposed to emit the current progress - not the ground station poll. As pointed out, polling is potentially risky, particularly if you poll when the autotune has finished, because you will kick it off again. Upshot, if we're going to do this, we probably need to agree a new command and document the precise semantics. Before the next release. Thoughts - @julianoes @DonLakeFlyer ? |
It tried QGC daily in jMavSim and I also don't get the autotuning UI. @batinkov Could you please check what's happening? |
@bresch I don't know why you don't get the Autotune interface. I get it and it works as expected with PX4 (proprietary) and PX4 (open source) and gazebo_typhoon_h480, px4_sitl gazebo_standard_vtol, jmavsim, and px4_sitl gazebo. All possible combinations give me the interface and Autotune always works. Can someone else confirm they don't get the Autotune interface? |
@DonLakeFlyer I saw your comments up and I haven't forgotten about them. It's pretty crazy lately at work and that's why I'm delaying those fixes. |
Finally I'm not the right person to judge about MAVLink but it looks @hamishwillee is right about the Autotune implementation in PX4. Perhaps we need to change the polling mechanism in PX4 implementation and then QGC needs to change accordingly. |
Me, I don't get the autotune interface. Are you using ubuntu and current gazebo? |
@hamishwillee I'm using Ubuntu 18.04 LTS and gazebo 9.19.0. |
@batinkov How? QGC doesn't work for me on Ubuntu 18.04. |
Ramon fixed the 20.04 requirement a few days back. With that the docs are out of data and 18.04 is fine now. |
A new UI component with the corresponding model are added. They enables Autotune in GQC. The autotuning can be performed only when the vehicle is flying. The new Autotune UI and the old interface are put under a switch and only one of them is available to the pilot at at time.
Autotune.QGC.mp4