patterncsharpMinor
Implementation of a PID Controller
Viewed 0 times
pidimplementationcontroller
Problem
I've implemented a PID controller, but seeing as I'm not an expert in control theory, have I missed any edge cases?
```
public class PIDController
{
public enum PIDMode
{
Manual,
Auto,
}
public enum PIDAction
{
Indirect,
Direct,
}
public PIDMode Mode { get; set; }
public PIDAction Action { get; set; }
public double Proportional { get; set; }
public double Integral { get; set; }
public double Derivative { get; set; }
public double Minimum { get; set; }
public double Maximum { get; set; }
public double DeltaMinimum { get; set; }
public double DeltaMaximum { get; set; }
private double _ProportionalTerm;
private double _Integrator;
private double _Derivator;
public double Setpoint { get; set; }
private double _Feedback;
public double Feedback
{
get
{
return _Feedback;
}
}
public void Calculate(double Input, long Time)
{
double output;
// Compute the error value
double Error = Setpoint - Input;
if (Mode == PIDMode.Auto)
{
if (Action == PIDAction.Direct)
Error = 0 - Error;
// Compute the proportional component
_ProportionalTerm = 1000.0f * (Error - _Derivator) / (double)Time;
// Compute the integrator component, clamped to min/max delta movement
_Integrator += (float)Error * (float)Time / 1000.0f;
if (_Integrator DeltaMaximum)
_Integrator = DeltaMaximum;
// Add the proportional component
output = (Proportional * Error);
// Add the integral component
output += Integral * _Integrator;
// Add the derivative component
output += Derivative * _ProportionalTerm;
// Clamp output to min/max
if (output Maximum)
output = Maximum;
}
```
public class PIDController
{
public enum PIDMode
{
Manual,
Auto,
}
public enum PIDAction
{
Indirect,
Direct,
}
public PIDMode Mode { get; set; }
public PIDAction Action { get; set; }
public double Proportional { get; set; }
public double Integral { get; set; }
public double Derivative { get; set; }
public double Minimum { get; set; }
public double Maximum { get; set; }
public double DeltaMinimum { get; set; }
public double DeltaMaximum { get; set; }
private double _ProportionalTerm;
private double _Integrator;
private double _Derivator;
public double Setpoint { get; set; }
private double _Feedback;
public double Feedback
{
get
{
return _Feedback;
}
}
public void Calculate(double Input, long Time)
{
double output;
// Compute the error value
double Error = Setpoint - Input;
if (Mode == PIDMode.Auto)
{
if (Action == PIDAction.Direct)
Error = 0 - Error;
// Compute the proportional component
_ProportionalTerm = 1000.0f * (Error - _Derivator) / (double)Time;
// Compute the integrator component, clamped to min/max delta movement
_Integrator += (float)Error * (float)Time / 1000.0f;
if (_Integrator DeltaMaximum)
_Integrator = DeltaMaximum;
// Add the proportional component
output = (Proportional * Error);
// Add the integral component
output += Integral * _Integrator;
// Add the derivative component
output += Derivative * _ProportionalTerm;
// Clamp output to min/max
if (output Maximum)
output = Maximum;
}
Solution
It looks reasonable, but I don't see why you need to make the Proportional term a member of the class, it doesn't need to be saved, at best it might make sense to be able to query it with a get, but a set seems misleading.
I'd be reluctant to allow public setting of the Integral and Derivatives too, querying them might be useful, but letting users of the class set it seems odd.
Forcing the scaling by 1000.0 seems arbitrary and inflexible too, this would make sense to expose as a settable constant in my view.
I'd be reluctant to allow public setting of the Integral and Derivatives too, querying them might be useful, but letting users of the class set it seems odd.
Forcing the scaling by 1000.0 seems arbitrary and inflexible too, this would make sense to expose as a settable constant in my view.
Context
StackExchange Code Review Q#72, answer score: 9
Revisions (0)
No revisions yet.