Commander inputs feel delayed/sluggish/strange on latest built firmware (complementary filter)

[theseankelly]

My CF2 isn't flying well on firmware built with the latest commits. Some symptoms include:

• Sluggishness
• Commander packets randomly feel delayed (anywhere from 0 to 3 second delays before CF reacts to some stick movements)
• Yaw is not right: Hammering the yaw stick full left or right proudces very slow rotation and occasional glitches where the CF rotates backwards (almost like a yaw wrap-around)

I'm flying with the complementary filter and a Devo7e and Devo10 radio (deviationTx).

[theseankelly]

I'm happy to look in to this bug sometime in the next few days; just wanted to log it in the system anyway.

[theseankelly]

Btw, flying great on a private branch which has some custom changes. Latest commit from master in that branch is from a long, long time ago: (Mar19, commit b63c552) -- so not really useful in narrowing down the problem. Just relevant because it's not a hardware/transmitter problem.

[tobbeanton]

Did a quick investigation and the problem seam to appear when the velocity controller was introduced. I think this commit can be the cause. Or a commit close to that.

[theseankelly]

It's commit d614849 which features quite a few changes to how the PID controller works. I'm reviewing that commit now.

Copter flies fine on the commit immediately prior to the one referenced above, so I'm quite confident it's the cause. The rework of how integral limits are used as well as capping under the new 'output limit' variable is suspect.

[theseankelly]

Yeah, changing lines 63 and 64 of attitude_pid_controller.c

from

static float rpRateLimit  = 150.0f;
static float yawRateLimit = 150.0f;

to

static float rpRateLimit  = 1000.0f;
static float yawRateLimit = 1000.0f;

Yields far better performance. I'm not entirely sure what this change was intended to fix (something with the position system?) so I'm hesitant to call this the right fix. Also, I picked 1000.0f completely randomly because it's probably sufficiently high to have no effect at all.

I disagree with constraining the PID output like this. The integral term is the only term that is subject to windup, and that was already being accounted for. With a limit like this, saturating the output ends up biasing towards the largest term, which will do all sorts of weird things. Would love to hear more about the problematic behavior this commit was trying to fix.

[tobbeanton]

Great find @theseankelly . I'm passing this on to @stephanbro as I'm pretty much lost as well.

[stephanbro]

@theseankelly this wasn't actually meant to fix any problematic behavior, but it sounds like in adding these changes it caused some other problems!

The idea with adding the output limit was it was a way to limit the maximum roll, pitch, and yaw. I extended that concept to the roll, pitch, and yaw rates for similar reasons, but that may not have been the best idea. Additionally, I didn't know the best values to set the rate limit at, so I'm not surprised that they would have to be changed. The units are in degrees/sec, so that should help with some understanding on what good values might be.

As for changing the yaw, the yaw setpoint should probably be limited to a maximum error to prevent the occasional glitches that you saw when holding down the stick.

[theseankelly]

Hmm, I don't entirely follow -- Limit the maximum roll, pitch and yaw how -- enforce max angles?

Limiting the PID output doesn't limit the actual roll/pitch/yaw angles, it limits the speed at which the controller can achieve the desired max angles (or the speed at which the desired rate of rotation can be achieved), which overrides the PID coefficients and produces unexpected behavior when the output limit is saturated. (The PID is trying to hit the setpoint, thinks it's doing so, but gets limited downstream, so the feedback loop doesn't really work properly.)

I was thinking about it some more and it explains the sluggishness I experienced (limiting the speed at which the setpoint is reached) as well as the weird yaw kick-back (the setpoint is what it is, and the PID is not responsive enough to hit it. Eventually the setpoint wraps around and goes negative, so the copter decides its faster to move in the opposite direction to meet the request.)

If you're trying to limit the max angles of attack or max rates of rotation it should be done at the commander level (limit the setpoint).

[theseankelly]

Can we pull this change out until the right change is reached? I'd still like to understand from @stephanbro what the change was aimed at fixing or preventing and -- if you're seeing some uncontrolled oscillations for example, the PID coefficients probably need to be tuned.

[stephanbro]

@theseankelly The limit on the rate controller doesn't make sense to be in the setpoint, as it is not something that will change with each setpoint update. The values should be parameters as they could change based on how the user would like the crazyflie to fly. The piece that is missing is a leash on the output of the upstream controller.

When flying in a position controlled mode, the output limit on the velocity controllers gives a limitation to the speed of crazyflie, as do the downstream controllers for the item they control. That limitation is very useful when the controller doesn't know if it's input has large step changes, like if a position setpoint jumps. The change was not intended to fix any tuning issues.

My thoughts on the changes should be to increase the rpRateLimit and yawRateLimit in the short term, as @theseankelly suggested, then in the longer term, add the leash on the output of the upstream controller.

[theseankelly]

I can't speak to the velocity controller since I don't know much about it, but it sounds like a perfectly valid thing to want to accomplish, and applying a limit to the velocity controller to accomplish this goal probably makes sense.

However, I still strongly, strongly advise against doing this on the rate/level PIDs. The goal of a PID loop is to get from state A to state B in the most optimal, controlled fashion possible. For huge setpoint changes, the PID will take care of getting to the desired state as fast as possible without overshoot (and if there's overshoot, it's a poorly tuned PID and the P,I,D coefficients need some work. Assuming overshoot is not desired: Some people would prefer a bit of overshoot for the sake of fast response times. But I digress...). Applying an artificial cap to the PID output will destabilize the system and make it less responsive, and worse: unpredictably unstable. Increasing rpRateLimit and yawRateLimit is just guesswork and isn't really an ideal solution.

[krichardsson]

Does manual and autonomous flight require different properties? Maybe we should parameterize this to allow different settings when needed? If so, I would suggest to make the default settings optimized for manual flight as this is the most common use case.

[theseankelly]

Sorry for the delay here. I still don't support increasing the rpRateLimit and yawRateLimit as a proper fix. Essentially, they'd just be increased high enough until they don't actually have any effect at all, at which point it's just a bunch of dead code checking for something that'll never happen. Also, there's no good way to tell how high is high enough -- I picked something totally random that seems better, but I can't tell whether it'll cover every single scenario. It's more likely that some aggressive-correction edge cases remain that might result in crashes.

I strongly suggest we remove this functionality entirely from the rate/angle PIDs. Can't really speak for the position controller though. If it's serving a function, then let's leave it -- will have to defer to you guys on that.

[theseankelly]

Here's a compromise that I hope keeps everyone happy: Wrap the limit test inside if(outputLimit != 0). In other words, let an outputLimit of '0' mean 'no limit'. Change the default outputLimit to zero and remove the code that sets them in the attitude controller. That way, the functionality to limit the total output exists in the PID (though I still don't really agree that it's a good thing to be doing in principal...) and the position controller will remain as-is today.

I hope to have a pull request out with this later today, once I can test it out.