-
-
Notifications
You must be signed in to change notification settings - Fork 898
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
Fixed PID tuning sliders disabling on certain values. #2053
Conversation
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage 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.
Good work @mikeller !!
Yes good work! Maybe the last 10.7.0 bugfix?? |
i have tested it,the behiavour with PD Balance slider seems that is solved, but trying to find behiavours in other slider i can see some break points with PD Gain slider. |
I think we have some rounding error with this... |
Yes, maybe we need to aply * 0.85 to this.PID_DEFAULT[2], now is aplying to P, it doesnt makes much sense |
@@ -49,7 +51,7 @@ TuningSliders.setDMinFeatureEnabled = function(dMinFeatureEnabled) { | |||
if (this.dMinFeatureEnabled) { | |||
this.defaultPDRatio = this.PID_DEFAULT[2] / this.PID_DEFAULT[0]; | |||
} else { | |||
this.defaultPDRatio = this.PID_DEFAULT[2] / (this.PID_DEFAULT[0] * 1.18); | |||
this.defaultPDRatio = this.PID_DEFAULT[2] / (this.PID_DEFAULT[0] * (1 / D_MIN_RATIO)); |
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.
Here, I think that we are wrong aplying D_MIN_RATIO to P and not D
This happens in |
I've had another look at this, and for what I can tell what we are seeing here is inherent to how this is implemented: It tries to determine if individual PID values have been adjusted by comparing the PID values to the calculated PID values based on the sliders. But because the two calculations are slightly different, with rounding in different places, the rounding will cause the results of these two calculations to be different in some cases. |
Yes, I remember the discussion before 10.6. I always prefered that this was implemented in firmware first, to reduce the complexity and maintainability of the Configurator, but a lot of users wanted this so we released it. Is clear that it has been a success, a lot of users like it, but the way it is implemented is a headache for us. Now I don't know what to do with this. Try to fix it? Release it as is? I'm ok with what you decide @mikeller . |
@McGiverGim: I think accepting that we have now is not perfect, and trying to get the firmware based implementation done is the best approach - we could probably play whack-a-mole with these irregularities until the cows come home. |
Fixes #2049. There is bugs, damn bugs, and rounding errors.