-
Notifications
You must be signed in to change notification settings - Fork 15
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
Smooth ramping of ATR and Torque Tilt #2
base: main
Are you sure you want to change the base?
Conversation
d224d82
to
7005dda
Compare
Following the example of inputtilt, ATR setpoints are now ramped slower when near the current setpoint. This allows for much higher ramping speeds without causing a jerking effect. Instead of using 5deg/sec and 3deg/sec one can now easily run 20 and 12 or even more without any perceived downsides. Feature: Smoothened/faster setpoint ramping for ATR Signed-off-by: Dado Mista <[email protected]>
Following the example of inputtilt, TT setpoints are now ramped slower when near the current setpoint. This allows for much higher ramping speeds without causing a jerking effect. Instead of using 5deg/sec and 3deg/sec one can now easily run 20 and 12 or even more without any perceived downsides. Feature: Smoothened/faster setpoint ramping for Torque Tilt Signed-off-by: Dado Mista <[email protected]>
src/torque_tilt.c
Outdated
} | ||
} else { | ||
rate_limitf(&tt->offset, target_offset, step_size); | ||
} |
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.
Just having a very quick look now, I'll want this extracted into a function, not copy-pasted. I think there are also multiple ways to do the smoothing, there might be a better way that'd allow simpler (needing less context) implementation in a function. At least those were the thoughts I had when looking at the original PR...
Signed-off-by: Dado Mista <[email protected]>
Factored it out into a function now (without changing the logic). Haven't done any test rides with it yet to see if functionality got affected... |
The way I did something similar was by having targets already smooth, multiplying target_diff by a "Responsiveness" parameter (let's say 15) to produce deg/s, which gets limited by "Max Speed". Then multiply by dt to get the step. So just a rate limited P control. I like how simple yet user friendly it is (for example changing Max Speed doesn't affect near-target behavior). It's been working great for me, but tbf I haven't tried the proposed changes, so maybe I'm missing out on something. |
Where are we on this? People are asking for this feature and I'm tired of sending them custom Refloat packages... |
I'm sorry for the delay, I wanted to test this, but I don't have that much experience with ATR. I'll get to it this weekend. |
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 tested this and it does feel smoother (I only really set up some decent ATR right before doing the test, I don't have a feel for it), but I'd like to get the math right.
((1 - smoothing_factor) * *ramped_step); | ||
// Linearly ramped down step size is provided as minimum to prevent overshoot | ||
float centering_step = fminf(fabsf(*ramped_step), fabsf(tiltback_target_diff / 2) * step) * | ||
sign(tiltback_target_diff); |
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'm not really great at math, so I may totally be missing the point, but I find this computation overly complicated and ultimately flawed.
The step
here is no longer the step in itself, it's a multiplier for the tiltback_target_diff
and the step is thus a linear function of the diff. The way I understand it you are filtering towards the real step (ramped_step
) if outside the window and you're filtering towards half of the real step when within the window.
The line above is what I think is not working really well, consider the case when you've just entered the smooth_center_window
after you've filtered almost towards step * tiltback_target_diff
, so that will be the value of *ramped_step
. centering_step
, though, after going through the fminf()
, will be fabsf(tiltback_target_diff / 2) * step
, which is almost half of that, so that transition won't be smooth at all. Or am I wrong on this?
I don't dispute it's working in a way (not sure why the above non-continuity doesn't noticeably manifest), but I want the math in the package to be solid. I think this is supposed to do in essence what @aeraglyx described in #2 (comment), except his solution I think is (to the extent I can make that claim, again, I'm not really too good at math) mathematically correct. There are likely more ways of doing this too, e.g. I was thinking of maybe an exponential function (while this one is linear), etc.
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 have to admit that I simply copy pasted this code without questioning it from the Input-tilt implementation. Now that you forced me to look into it I have to say that something does look fishy about that code, good job for catching that.
Before further investigating I double-checked that I didn't introduce the problem during refactoring. Thus the problem should also affect the original input tilt implementation.
I think the problem starts when ramped_step_size is updated for the smooth center window. If the smooth center window is made large enough (>> 2), it appears as if ramped_step_size could actually temporarily increase instead of decreasing, since "tiltback_target_diff / 2" can be larger than 1.
I haven't had time yet to look into a fix but I figured I owed you a response acknowledging the problem you found.
Very excited to see what you have in mind after reading through Discord, @lukash! I've been toying with this too, might as well share some findings:
Here's a plot comparing a few options on some generated noisy data.
I'm probably risking being super annoying considering that you teased about posting plots, that's not my intention 😅 And not sure how useful it is, but I found out that this form is pretty much identical to a 2nd order IIR + rate limiting. Essentially what my previous comment suggested but with additional speed smoothing (2nd order requires 4x less filtering per "speed EMA", 3rd order 6x, not sure what's up with that). I don't have this in code, sorry. |
This feature has been tested by a handful of riders for the past few months.
Following the example of inputtilt where ramping rates of over 50deg/sec are used,
we're doing the same for ATR and Torque Tilt now.
Setpoints are ramped slower when near the current setpoint. This allows for much higher
ramping speeds without causing a jerking effect.
Instead of using 5deg/sec and 3deg/sec one can now easily run 20 and 12
or even more without any perceived downsides.