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
RPM Limiter #12054
RPM Limiter #12054
Conversation
This comment has been minimized.
This comment has been minimized.
AUTOMERGE: (FAIL)
|
src/main/flight/mixer.c
Outdated
if (mixerConfig()->rpm_limiter_rpm_linearization) { | ||
throttle = constrainf(-pidOutput, 0.01f, 1.0f); | ||
} else { | ||
throttle = constrainf(throttle-pidOutput, 0.01f, 1.0f); |
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.
really, throttle should be allowed to go all the way down to 0.0, otherwise it is just like adding more idle.
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.
This was supposed to fix an issue headsup was having with two of his motors stopping completely when arming and rolling while still on the launch blocks. I can't reproduce it now though so I'll assume his dshot_idle_rpm is set wrong.
This is something that dynamic idle does on line 648 as well
throttle = MAX(throttle, 0.01f);
src/main/flight/mixer.c
Outdated
motorsSaturated = true; | ||
} | ||
} | ||
averageRPM = 100 * averageRPM / (getMotorCount() * motorConfig()->motorPoleCount / 2.0f); |
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.
This line of code is a good reminder that we should just have one place in the code where RPM is calculated, and then just pass it around to all the other parts of code that need it.
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.
Would this also be the opportunity to read that value is as a UINT16 so we can end-run around the identified overflow at 32767, and sanity check that behavior?
src/main/flight/mixer.c
Outdated
float acceleration = scaledRPMLimit - mixerRuntime.rpmLimiterPreviousRPMLimit; | ||
if (acceleration > 0) { | ||
acceleration = MIN(acceleration, mixerRuntime.rpmLimiterAccelerationLimit); | ||
scaledRPMLimit = mixerRuntime.rpmLimiterPreviousRPMLimit + acceleration; |
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.
This line of code look just like the sort of FF that BF uses for the rate controller. You may consider adding a FF gain to this acceleration.
This bit of code will also act different if you are running a different pidloop rate. IE faster pidloops will have less of the acceleration effect, while slower pidloops will have more of an acceleration effect.
Also why don't you allow negative acceleration? wouldn't we want to slow the motors down faster?
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.
Could you elaborate what you mean with the feedforward? This is just the code which limits the acceleration of the rpm setpoint.
Looptime is taken into account in mixer_init.c on line 340
mixerRuntime.rpmLimiterAccelerationLimit = mixerConfig()->rpm_limiter_acceleration_limit * 1000.0f * pidGetDT();
Acceleration limit is only applied when positive because otherwise this would apply a deceleration limit limiting how fast the motors could spin down. I tested this, and in my opinion it feels weird and unsafe especially with low limits.
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.
oh, i was reading this bit of code wrong, I thought that this was adding an acceleration to the new scaledRPMLimit, not adding a slew filter on the acceleration limit. Ahh so this is just s slew limit filter, i see now.
src/main/flight/mixer_init.c
Outdated
@@ -322,6 +331,19 @@ void mixerInitProfile(void) | |||
} | |||
} | |||
#endif | |||
|
|||
#ifdef USE_RPM_LIMITER | |||
mixerRuntime.rpmLimiterExpectedThrottleLimit = 1.0f; |
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.
You should try and figure out how to store the value of this once you disarm. That way the learned limit won't have to be relearned every time you plug in a new battery.
src/main/flight/mixer.c
Outdated
} | ||
float smoothedRPMError = averageRPMSmoothed - scaledRPMLimit; | ||
float rpm_limiterP = smoothedRPMError * mixerRuntime.rpmLimiterPGain; //+ when overspped | ||
float rpmLimiterD = (smoothedRPMError - mixerRuntime.rpmLimiterPreviousSmoothedRPMError) * mixerRuntime.rpmLimiterDGain; // + when quickly going overspeed |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
Line 338 and 339 in mixer_init.c account for the loop frequency
mixerRuntime.rpmLimiterIGain = mixerConfig()->rpm_limiter_i * 0.0001f * pidGetDT();
mixerRuntime.rpmLimiterDGain = mixerConfig()->rpm_limiter_d * 0.00000003f * pidGetPidFrequency();
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 saw, good job.
src/main/flight/mixer.c
Outdated
} | ||
else { | ||
throttle = throttle * mixerRuntime.rpmLimiterExpectedThrottleLimit; | ||
scaledRPMLimit = ((mixerConfig()->rpm_limiter_rpm_limit)) * 100.0f; |
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 do not understand this line of code, it seems wrong as it will always use the same number here for the scaledRPMLimit, which will create a very weird error when you compare it to the average motor output. Say you are at idle, this would always assume that there is a very large error...
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.
Line 376 is applying the dynamic throttle limit
Line 377:
You are correct that there would be a large error, but I limit the pid output to only be positive on line 406. This is because we only ever want the pid controller to reduce the throttle to bring rpms to the setpoint. The regulation is admittedly not the best, which is why I tested pre-activating the pid controller once the rpms got close to the limit, but this felt really weird. I think the dynamic throttle limit helps hide poor regulation, but this was also why I started experimenting with linearization which doesn't have this issue.
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 guess I would need to see some logs of that method to see what its doing a bit better.
@Tdogb when looking at the linearization version of the code I am surprised to see that there is just a PID controller. Say for instance that you are at hover throttle, and have nearly no error between the average RPM and what you say the desired average RPM should be, in this case you would only have iterm holding your hover throttle, which would mean that this controller is highly dependent on the iterm to control it. Without the iterm you would never reach the throttle that you want to reach. It would be nice to have some amount of FeedForward taking the rcCommand[Throttle] and multiplying it by some sort of dynamic_throttle_limit to try and be able to mainly rely on the estimated throttle needed (IE the rcCommand[Throttle] * dynamic_throttle_limit) and then use the pids to compensate for the rest. That would likely be the most responsive and the least likely to overshoot one way or another. If I'm wrong here please correct me, but it seems that in a hover its pretty much just the iterm holding the throttle. Do you have any logs of the rpm pids during flight?
|
I have always liked the idea of regulating the rpm on each motor independently to get it to more accurately achieve the requested motor output. |
@Quick-Flash I really like the idea of a feedforward. Maybe worth looking into. Here are some blackbox logs to look at https://drive.google.com/drive/folders/1TmjaugqXaCLQKI3WepCKa-czvUOtbgJF?usp=share_link |
Are these logs all taken on the same drone? if so it seems that the linearized version is adding noise to your motors :( as the nonlinearized log has cleaner motor traces. |
to help with logging making sure that the setpoint throttle is logged after you change it would be useful. But yes the non linearized version looks like it doesn't depend much on the pid controller, Also logging the averaged RPM would be useful to help see whats happening. Thanks for the logs BTW |
Logs are from the same drone, it has a crazy tune on it, so the motors look noisy even with the rpm limiter off, they don't get hot though. I'm wondering, since rpm limiting does not require critical accuracy or response times, might be worth it to move the PT1 cutoff frequency even lower and just ensure we are always getting clean traces. Another thing to note is that the rpm linearization uses much lower pid terms than the non-linearized does. Maybe relying more on FF than the other gains may help with the noise? Definitely want to try it out. Btw, which log are those screenshots from? Is that linearized or non-linearized? |
First is the hover log, and second is the linearized log. |
@Tdogb |
I'll get some blackbox logs with less noisy motors/tune and check. Hard to see through all that noise. I'm sure a FF will help though |
If you do log the pre throttle and the post throttle. Thatd be super helpful. |
This comment has been minimized.
This comment has been minimized.
I just found a major issue, on smaller quads with faster motors, there is an overflow at ~32767 rpm, which causes average rpm to go negative. This can be seen at 00:18 in this video: https://drive.google.com/file/d/1PJtOv1aXg0CtlhtxdG7aZf0vEKtOJpfl/view?usp=sharing |
Thats a 16 bit signed overflow. for RPM values it makes more sense to read it as a u16 rather than an i16 as that would double the rpm. Only place I can see that could cause it in your code is the RPM telemetry giving you a negative number... but that doesn't make much sense. Unsure where in the code that would break. |
What's weird is that the individual rpms reported on the osd still look good...something to note is that this is my only blujay quad, so maybe that has issues during a crash? I've noticed that on all esc firmwares, the reported rpm during a crash goes crazy |
I'm still trying to work out from a much more naive approach why a simpler scheme wouldn't work - practically from a spec series compliance standpoint, the intention is to ensure that no corner of the quadcopter is producing more thrust than the nominal spec combination (effective battery voltage * KV * propeller advance ratio .:. in whatever air column is present). The more elegant answer is just mirroring the existing RPM Dynamic Idle code as a top end limiter, and again structuring that as a hard-cap type limitation where that max RPM is not intended to be exceeded... but my above lazier approach still feels more robust overall (and would tend to fail stupidly, but in a non-flyaway state that would basically result in limping into a perch mode) |
you can try to use the motor output limit to control how much you want during the turns... But that will require re-turning, or at least adjusting the master multiplier. |
This is precisely what I'm looking to do with testing - basically use the RPM Spec with the LlamaSpec to provide the dynamic tune plus motor output limiting, so that if it's a spec bird, literally zero additional thought is required, it should fly basically the same no matter what battery voltage gets thrown at it. 4S 1800mAh and 5S 1500mAh packs have both worked really well for me. |
I have a branch that does motor output limiting. I’ve tested it and it works. You need to use max rpm instead of average rpm with motor limiting https://github.com/Tdogb/betaflight/tree/rpm-limiter-max-rpm-pr |
Right, at this point I'm also trying to see if I can build a TinyTrainer preset with the same overall layout, but that carries the motor output limiting with 10% margin for that as well (since those are much more backyard setups), and seeing if that can take the 'battery needs to be high end and fresh' away from that spec class as well. |
I think it’s actually good that the quad had access to all of the motors since it makes it recover from crashes better. Also we’ve done tons of testing with the current rpm limiter and it has seemed to be great |
i think we can use a dynamic motor limit to cap the RPM. But that should come with an automatic PID multiplier. And that requires more testing... If we ever come up with a limiter like that, we can just add another type. Instead of active yes/no we can have avg/max/off in future. |
Do you want to test this code? Here you have an automated build: |
@KarateBrot @Tdogb
|
@Tdogb |
Need to address or dismiss @KarateBrot reviews |
yeah, they were addressed already. Just PR author didn't click close conversation. |
Oh, I see. I'll review again. |
RPM Limiter
Hey all, this pull request contains rpm limiter code which I have written in order to make spec racing more fair. The RPM limiter attempts to limit the average RPM using a pid controller.
Motivation: In spec racing, limiting RPMs through software instead of through hardware saves money, keeps components competitive for longer, and allows quads with different hardware setups to race competitively.
Standard RPM Limiter:
set rpm_limit = 130
means 13000 rpm. Average was chosen for better feel in the turnsStandard RPM Limiter Problems:
Standard RPM Limiter Throttle Limit Learning:
RPM Linearization
While experimenting with the rpm limit, I decided to try setting the rpm limit proportional to the current throttle. I then kept the PID controller turned on the entire rpm range. This would attempt to control the RPM such that it is proportional to the throttle. This essentially turns rpm linearization into a sort of closed loop vbat sag compensation. Making the throttle response feel more consistent and independent of the battery voltage. It also made the throttle more consistent on a high throttle launch since the pid controller was proactively controlling the rpms to the setpoint.
RPM Linearized Limiter
rpm_limiter_idle_throttle
at 0 throttle and going to ‘rpm_limit’ at full throttleBenefits of RPM Linearization
Downsides of RPM Linearization
The acceleration limit
—————————————————————————————
What I think still needs to be improved
What needs to be discussed
RPM Limiter is being beta tested for spec racing in Street League, and community feedback is going to be gathered. I believe this has great potential outside of just the spec racing community, so I am making this pull request.
Tested on:
Not tested on:
Media:
DVR Dump: https://drive.google.com/drive/folders/1PEOfvd8uQwPa_J3lpeEmDEtxwCNh0rn6?usp=share_link
Street League Announcement Video (Includes blackbox of measured average RPM overplayed on video): https://www.youtube.com/watch?v=wF7rOV6DJGA