Open barzanisar opened 6 years ago
If you are getting the new ui for sliders on 3.3 branch then something is wrong with your build. I'd delete build directory and start again. I just tried building stable 3.3 and it was fine using the old slider ui. And complaining about missing roll/pitch parameters.
Not quite sure what you mean by wierd? On master the sliders use a new ui control for changing values. Also master has roll/pitch removed since the parameters controlling those are gone.
@DonLakeFlyer I did what you said and now the stable 3.3 shows the correct ui. Thanks!
Now I checked out a branch from master to solve https://github.com/PX4/Firmware/issues/9379
These are the changes I made.
This is what the tuning sliders look like:
I don't know why some of the sliders show negative values, while some (for e.g. MC_PITCHRATE_FF) show zero.
Also, is this the new UI for tuning sliders or something is wrong?
Also, is this the new UI for tuning sliders or something is wrong?
This is the new UI for tuning sliders.
I don't know why some of the sliders show negative values, while some (for e.g. MC_PITCHRATE_FF) show zero.
Most likely missing parameter meta data problems. Can you double check the meta data for those params and make sure it is fully specified. Min/Max/Increment.
@DonLakeFlyer If this is the new UI for tuning setup, then I don't see any purpose of having it if we already have a much better UI in the "Advanced" Tuning. What do you say? Shouldn't we just have the Advanced page pop up by default when the user clicks Tuning setup?
By the way, why did we have tuning sliders in the first place?
@LorenzMeier @DonLakeFlyer I discussed this with @RomanBapst and @bkueng.
Multicopter: If the tuning sliders are to include all the necessary tuning parameters, then we think the "Advanced" page of the tuning setup (in QGC master) already does that with a much better UI. Hence, there's no need to have a separate page that does the same thing with a much less user friendly UI (as you can see in the images above). However, "Advanced" page is only offered for multicopters. What is your take on this? Should we discard the tuning sliders for multicopters and only keep the Advanced page? If yes, then we can have Advanced page pop up by default.
Plane: There isn't any advanced tuning page so I guess we should keep their tuning sliders for now. I will add more parameters/sliders important for FW tuning to this page. However, I don't like the new UI for tuning sliders. It is confusing to select parameter values not visible on the sliders. If the purpose of changing the UI was to make sure the values are also visible to the users as they slide, then I suggest we can just have a separate text box for each slider displaying the current value of the slider (which should update in real time as the user slides). That text box can also be used to take in user input for setting the parameter value and update the slider position accordingly.
VTOL: How should the tuning page look like for VTOL? Should we make it switch between multicopter and plane's tuning setup page depending on the its transition mode?
Let's have a discussion this afternoon to settle this.
@LorenzMeier @bkueng @RomanBapst @thomasgubler This is what we discussed. Feel free to add anything I missed.
This is the view for normal users which really only adjusts some parameters that are related to the vehicle weight and is not tuning anything.
This is the view for normal users which really only adjusts some parameters that are related to the vehicle weight and is not tuning anything.
What was the locking issue we discussed?
What was the locking issue we discussed?
VTOL: Lock tuning view for MC when in FW mode and vice versa
If the tuning sliders are to include all the necessary tuning parameters, then we think the "Advanced" page of the tuning setup (in QGC master) already does that with a much better UI.
That doesn't make any sense to me. There is no reason to need live charts or all the advanced tuning controls to tune things like THrottle Hover. That will scare the hell out of the majority of the users who never need to do any sort of advanced PID tuning.
@DonLakeFlyer I've reworded Barza's comment to make it more clear. She didn't propose that.
Everyone needs to keep in mind that the current Advanced tuning page is a WIP in progress which is missing 75% of the needed functionality. This is a known thing which Lorenz and I have discussed. I will get to a better working version soon.
@DonLakeFlyer Updated comments are in. Gist of it: Split tuning into "Setup" and "Tuning" such that setup is something that you have to do and tuning is the thing that you do with live graphs (better than doing it blindly) and only if you know what you are doing.
Ah, get it now. I think that is fine.
@DonLakeFlyer How do we add another "Setup" tab in Vehicle Setup? i.e. here:
Also, How can I change the UI for current tuning sliders to "Desktop view"?
Once I know these things, I can fix the "Tuning" and "Setup" sliders.
Now that Lorenz explained this to me I really don't think adding yet another setup button is the thing to do there. There are already a lot of buttons. The concept of having Tuning page (Maybe renamed to something else) which has says tabs or something like that in it is a better way to go.
@barzanisar Let me put together a visual proposal as to how to reorg this and then you can put all the things in the right places.
See the images from Pull Request. I think this gives us space to categorize the different types of Tuning parameters. But the Basic tuning is still really basic so it doesn't scare normal users away. I think also in some cases there may not be any "Advanced" page if there is nothing intersting to put there.
Bringing the details from pull request here so things are in one place:
The idea is to have 3 categories of tuning available for each vehicle type.
Basic:
This is the existing Tuning page. It should contains the simple controls to adjust flight characteristics of a standard airframe. Meant for use by non-power users who already have a decent flying vehicle already.
Advanced:
This is the place for a designed page of parameter values which relate to power user tweakable tuning params. Used when you are creating a new vehicle from scratch and a standard airframe didn't get you a decent flying vehicle. Or for tweaking a vehicle beyond "decent" flying characteristics.
PID Tuning:
This is the existing Advanced PID Tuning.
The reason this doesn't work from a user model perspective is that hover throttle is NOT tuning - it is as needed as calibrating the RC. It is a mechanic/user activity. There is also no such thing as "basic tuning" because you can only touch control gains in a reasonable way by watching the performance impact. So you always need the graphs and it is always a developer activity. I would not want to have this in the same pane because you essentially lure users into fiddling with sliders. We need to get to a point where a reasonably built frame does not require tuning and when you start to tune, you need to really know what you are doing.
For throttle hover I would say it's "sort of" not tuning even though it's based on vehicle dynamics. But I get what you are saying. So here's the page for a fixed wing:
Where do those sensitivy sliders go? I don't think you need graphs to tweak those. Or do you? PX4 multi-rotor use to have these sorts of sensitivy/feel sliders as well: MPC_ROLL_TC and MC_PITCH_TC which disappeared somewhere along the way. These feel like basic tuning to me. Where does this sort of stuff go if not under Tuning?
I would not want to have this in the same pane because you essentially lure users into fiddling with sliders.
Which was why my original implementation has the PID Tuning page behind an Advanced button. Given your explanation I would say that the way QGC currently does currently this is better. But still somehow wrong? To me things which affect the flight dynamics/characteristics of the vehicle fall under Tuning. In my world of not a firmware or drone power user, throttle hover and min throttle are flight dynamics hence "Tuning". I'm concerned that you world is biased to knowing too much about controllers and what not, so "Tuning" means something different to you than the average user.
Also, keep in mind that the "Tuning" button can be changed to some other name if something comes up with something better.
But something to keep in mind: The "Tuning" page which has including things like throttle hover and whatnot has been like that for many years now. So renaming needs to be taken as a major change. Also moving those sliders to someplace else has a similar problem. If it's wrong it's wrong. But the fact that it is suddenly horribly wrong after so long a time seems strange to me.
In my world of not a firmware or drone power user, throttle hover and min throttle are flight dynamics hence "Tuning".
@DonLakeFlyer As a non power user, I agree that throttle hover and min throttle are something user has to "tune" depending on the "first flight behaviour". However, as I understand @LorenzMeier point of view, we have to ensure two things:
Hence, in my opinion it would be nice to have a separate pane for the "mandatory tuning" meant for every user (which will include hover throttle etc) and a separate "Tuning" pane which will contain the common tuning params (including PID gains) used by power users.
For very "Advanced Tuning" params, power users can then look up these params in the "Parameters" pane.
Every user is forced to set these params right after their first couple of flights, even if they are working with standard frames.
That doesn't have much to do with these being in separate panes. That's just a feature. Adding yet another top level button to keep Tuning and PID tuning separate is worse in two ways:
Non-power users are kept isolated from tuning the PID gains.
In current builds PID Tuning is behind an advanced check box. That would seem to accomplish this goal pretty well. We can add more warnings whenm a user gets there if you want.
Every user is forced to set these params right after their first couple of flights, even if they are working with standard frames.
The real question is how to do this. The way QGC does it for things like calibration is that it checks for parameters to be equal to 0. This indicates they are not set and a calibration is needed. Take MPC_THR_HOVER as an example. How does QGC know that the user needs to adjust this? The possible test is MPC_THR_HOVER != default value. But that fails if the default value matches what the correct throttle hover setting is.
Remove the “Save Values” button and rename the “Reset to Saved Values” button to “Reset to Previous Values”.
Why? Since you are chaning parameter values live, the idea is that you can save a good set of values for use by a later Reset. Then if you get yourself stuck with something which isn't better you can Reset to the values you saved. But if you do improve things, you can save again and do more tweaking. You need both a Save and Reset step to do that.
For fixed wing the FW_PSP_OFF seems kind of a power user thing. Is it to counteract the torque of the motor? I don't quite understand it. If you want this on the Basic page someone is going to need to come up with the end user understanable wording.
@LorenzMeier When we talked last time about wanting to get a vehicle flying well just using standard airframe setup you talked about vibration. Saying that was the real killer to most vehicles. In Tuning should there be a vibration page which can be used to see vibration somehow? This would be a normal user feature.
Why? Since you are chaning parameter values live, the idea is that you can save a good set of values for use by a later Reset. Then if you get yourself stuck with something which isn't better you can Reset to the values you saved. But if you do improve things, you can save again and do more tweaking. You need both a Save and Reset step to do that.
I am confused. Where do the parameters get saved when we press Save Values? on vehicle or on QGC? Don't the parameters get saved/set on the vehicle automatically whenever we change them live?
Where do the parameters get saved when we press Save Values
Internally in QGC. Since all parameter changes are live to the vehicle as you are tuning you could get yourself into an unflyable state with bad params. This feature essentially allows you to get back to a "Last Known Good" state of flyable params. I think we need help from @hamishwillee to make some sense as to how to explain this to the user.
@DonLakeFlyer What happens to the saved values when we restart QGC with a different vehicle type? Will QGC make note of the values saved for each vehicle so that when we relaunch QGC, it detects the vehicle config and show the values we last saved for that vehicle?
What happens to the saved values when we restart QGC with a different vehicle type?
They are lost. They are just meant to be used while you are actively tuning. It's sort of like making a backup so you can revert.
@DonLakeFlyer I think all this confusion can be cleared if we rename "Save Values" to "Save Values in QGC" and add a warning note below, for e.g: "WARNING: The parameter values saved in QGC will be lost once you disconnect the vehicle from QGC."
Sorry, I missed the request for feedback.
What if we were to name this so that it is obvious that the values are ephemeral/temporary? How about clipboard? So "Tuning Clipboard", "Save Clipboard", "Restore Clipboard".
Notes
Does that work?
I knew Hamish would come up with something. :) Clipboard is exactly what it does and people will understand that 100 times better.
Can we continue the discussion about the tuning page? @dagar recently informed me about it and I think like he also suggested we only need small improvements to the available PID tuning page currently available under the "Advanced" checkbox to make it a super useful tool.
EDIT: I made some suggestions in a new issue to not congest this one here: https://github.com/mavlink/qgroundcontrol/issues/8405
When I compile master or stable v3.3 branch, I get these weird versions of tuning sliders. However, when I open the binary version, the sliders look normal. Can anyone figure out why this is so? Is there any special way of compiling to see the tuning sliders? @DonLakeFlyer @dogmaphobic