Add support for firmware side sliders #1821
Conversation
|
Still a lot of work required, this is just for testing currently with lots of random code duplicated and nowhere near finished 😄 |
|
@IvoFPV , let me know where you need any help. I was looking at #294 and would love to make some progress toward this regard. We need to somehow adjust the sliders based on quad class. We are overkilling the higher PID values with them being RED now (#1839) and it is not the case across the board. For example, a whoop requires higher PID (60+). A 5" on 6s with a really light frame requires PIDs in the 20s. |
|
This issue / pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs within a week. |
| updateTabList(FEATURE_CONFIG.features); | ||
| } | ||
|
|
||
| TuningSliders.setExpertMode(checked); |
|
Some thoughts:
|
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs within a week. |
mikeller
force-pushed
the
firmware-sliders
branch
from
fa559f0 to
3d80a62
Compare
|
Rebased, done some basic testing, seems to be working. Will need more testing. |
|
Thanks @mikeller i have start testing now. I can see that after move some sliders,PITCH value for p,i,d and f goes to wrong values, also after this if you want to put sliders on 1, never goes to default values.
In the other hand, seems that disabling-enabling Dmin doesnt have efect on D values yet. |
mikeller
force-pushed
the
firmware-sliders
branch
4 times, most recently
from
8824c75 to
18b9095
Compare
|
@asizon: Is this when using 4.3 firmware with the firmware based simplified settings, or older firmware without them? The configurator sliders will stop working with pre-simplified-settings 4.3 firmware after this. |
|
@mikeller tested with latest 4.3 firmware, firmware based sliders, aniway I cant continue testing since there is an error and port is always loading: |
|
|
mikeller
force-pushed
the
firmware-sliders
branch
from
18b9095 to
da2b1de
Compare
|
@asizon: Ok, should be fixed now. |
|
Thankyou @mikeller , this slider code is so complex, maybe merge it and continue the work in other PRs?. I have made another test, but seems that this feature need some extra work to make it work as expected. P values doesnt go correctly. Also if you move slider fast, values doesnt change, is super weird: |
|
In my opinion, we must throw away the old pid sliders code, it never worked reliable, and use only the new firmware sliders. But this will produce a lot of complains of users that want to use it with old versions of Betaflight. |
mikeller
force-pushed
the
firmware-sliders
branch
from
da2b1de to
afb8c05
Compare
|
@McGiverGim: I can see your point. But I do not fully agree with 'it never worked reliable'. I agree that they are not perfect, and there are a number of edge cases where they fail, but based on the amount of feedback we are getting for them, with very little of it negative / bug reports, I think that the value that users are getting out of them outweighs the rough edges. |
mikeller
force-pushed
the
firmware-sliders
branch
from
afb8c05 to
301a676
Compare
mikeller
force-pushed
the
firmware-sliders
branch
from
301a676 to
41fb373
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
@mikeller , agreed. The user base - in general - loves the sliders and the option to still type in what you want is key. As they are transferred from Configurator based to Firmware based we just need to keep an eye on CONSISTANCY of how they work! Something to consider is hiding the new sliders unless "Expert Mode" is toggled in the configurator. I would not worry about that type of thing in the OSD sliders, just the configurator. But that is something that can be looked at in the new PR. Removing the sliders or altering how the current sliders work in form would cause an uproar IMHO and it would be hard to defend the "BF is too complicated" complains that would follow. Mspatz First, as the slider goes higher it makes PD ratio lower, not higher. Also it does not align with the fact that you tune in this order ... Filters -> D -> P-> I-> FF. For optimal wash performace, you want D as high as you can - just below the oscillation point - and then balance P off of it. And then on to i-Term, etc.... So in the current code, every time you change the PD Balance slider you have to typically tweak the P & D Gain slider too. In Configurator 10.6, that was not the case (by orignal deaign). Alas, I argured against the change back when it was made from 10.6 to 10.7 and failed; I'm not sure the majority agrees (yet) but hope a couple vidoes since has changed some minds (if anyone even watches); and reverting back to the better behavior does go agains my conaistancy argument. But rest assured, if it would be UAVflight, it would be the other way to be LESS complicated.] |
There was a problem hiding this comment.
Approved. The code seems ok to me. Is big so it will surely appear some bug but nothing that I can see looking at the code.
I commented in the firmware PR that I think is better to use a more "dynamic" and generic approach, without a list of defined sliders. It was rejected because it was not expected to modify the sliders in a future. Now we can see a lot of conditionals and elements hardcoded and defined in the Configurator, more because we maintain backwards compatibility with versions without firmware support. I only hope this will not end in a nightmare of maintenance.
| }, | ||
| "pidTuningRollPitchRatioSliderHelp": { | ||
| "message": "Roll Pitch Ratio", | ||
| "description": "This needs a meaningful help text" |
There was a problem hiding this comment.
This description is missing?
There was a problem hiding this comment.
@McGiverGim , this should be "Pitch Roll Ratio" (flipped) as most raise Pitch gains. So as the slider goes up, the Pitch should go up, not Roll. That said, i reckon this is controlled in BF now (firmware based)?
There was a problem hiding this comment.
Yes - these were missing originally in the pull request. I have added placeholders for now, so we can decide for each one if we need a help text, or if we should remove the tooltip.
@spatzengr: The respective simplified setting in the firmware is currently called simplified_roll_pitch_ratio - so if we decide to rename this then we should do it now, before any of this is released, and change it in the firmware too.
There was a problem hiding this comment.
@mikeller , I created the other slider tool tips and can give the new ones a shot. I think we should change Roll/Pitch slider to Pitch/Roll for the reasons outlined above. I also think D_min slider maybe should change, but that would not revise the name and is 50/50 to me on that one (could go either way).
I actually just created a branch in my repo to take a look at doing this change, but i'm sure it would take me hours and you minutes. So if you are willing, it would not hurt my feelings. This was the only FW thing and the rest should be configurator items to polish up.
There was a problem hiding this comment.
@spatzengr: Hehe, one could argue that these hours would be well spent on honing your JS / git / GitHub skills. 😛
But I don't mind doing the change - we can probably combine it with the tooltip texts, so let me know when you have these, and I can do the change.
| }, | ||
| "pidTuningIGainSliderHelp": { | ||
| "message": "I Gain", | ||
| "description": "This needs a meaningful help text" |
| }, | ||
| "pidTuningDMinRatioSliderHelp": { | ||
| "message": "D Min Ratio", | ||
| "description": "This needs a meaningful help text" |
5 participants
To go with betaflight/betaflight#9119. For now just for testing the calculation.
Todo: