PID code review

Using Arduino Programming Questions
1

I've been working on getting multiple PID controls working with my hardware. Now that I have the basics up and running, is there a more efficient way to code this? I'll be running 4 and possibly up to 6 PID's using roughly the same settings.

On side note: PID1 is rock steady,PID2 seems a bit jumpy, is there anything in the code that would cause that?

#include <PID_v1.h>

int actual1;
int demand1;
int actual2;
int demand2;

double Pk = 10; //speed it gets there
double Ik = 0;
double Dk = 0;
double Setpoint1, Input1, Output1;
double Setpoint2, Input2, Output2;


PID PID1(&Input1, &Output1, &Setpoint1, Pk, Ik, Dk, DIRECT);
PID PID2(&Input2, &Output2, &Setpoint2, Pk, Ik, Dk, DIRECT);


void setup() {
  pinMode (A0, INPUT);
  pinMode (A1, INPUT);
  pinMode (A2, INPUT);
  pinMode (A3, INPUT);
  pinMode (5, OUTPUT);
  pinMode (6, OUTPUT);
  pinMode (7, OUTPUT);
  pinMode (8, OUTPUT);
  
  
  Serial.begin(115200);
  
  PID1.SetMode(AUTOMATIC);
  PID1.SetOutputLimits(-255,255);
  PID1.SetSampleTime(10);
  PID2.SetMode(AUTOMATIC);
  PID2.SetOutputLimits(-255,255);
  PID2.SetSampleTime(10);

}

void loop() {
  // **  start PID 1 **
  actual1 = analogRead(A0);
  demand1 = analogRead(A1);

  actual1 = map(actual1,0,1023,341,682);

  actual1 = map(actual1,341,682,-255,255);
  demand1 = map(demand1,0,1023,-255,255);

  Input1 = actual1;
  Setpoint1 = demand1;

  PID1.Compute();

if (Output1 < 0) {
  Output1 = abs(Output1);
  analogWrite(5,Output1);
  analogWrite(6,0);
}
else if (Output1 >= 0) {
  Output1 = abs(Output1);
  analogWrite(6,Output1);
  analogWrite(5,0);
}
else {
  analogWrite(5,0);
  analogWrite(6,0);
}

  // **  start PID 2 **
  actual2 = analogRead(A2);
  demand2 = analogRead(A3);

  actual2 = map(actual2,0,1023,341,682);

  actual2 = map(actual2,341,682,-255,255);
  demand2 = map(demand2,0,1023,-255,255);

  Input2 = actual2;
  Setpoint2 = demand2;

  PID2.Compute();

if (Output2 < 0) {
  Output2 = abs(Output2);
  analogWrite(7,Output2);
  analogWrite(8,0);
}
else if (Output2 >= 0) {
  Output2 = abs(Output2);
  analogWrite(8,Output2);
  analogWrite(7,0);
}
else {
  analogWrite(7,0);
  analogWrite(7,0);
}

delay(10);
}
2 
  actual1 = analogRead(A0);
  demand1 = analogRead(A1)

When reading two different analog pins, the usual practice if to throw the first reading on the new pin away.

  actual1 = map(actual1,0,1023,341,682);

  actual1 = map(actual1,341,682,-255,255);

Why are two calls to map() needed?

if (Output1 < 0) {
  Output1 = abs(Output1);
  analogWrite(5,Output1);
  analogWrite(6,0);
}
else if (Output1 >= 0) {
  Output1 = abs(Output1);
  analogWrite(6,Output1);
  analogWrite(5,0);
}
else {
  analogWrite(5,0);
  analogWrite(6,0);
}

Less than 0, greater than or equal to 0, and everything else. Hmmm, just what is in that "everything else" bucket?

On side note: PID1 is rock steady,PID2 seems a bit jumpy, is there anything in the code that would cause that?

If you comment out the PID1 stuff, does that have an effect on PID2? Maybe the problem is the input devices for PID2.

3

Thanks for the reply.

When reading two different analog pins, the usual practice if to throw the first reading on the new pin away.

As far as throwing out the first reading on each new analog pin, I will look into that, and give it a try.

Why are two calls to map() needed?

The 2 mapping calls are 1, to map the full input reading down to the usable range of input motion, and 2 to map rhe usable range to the full output. This was part of someone else's code. I' may try to see how it works with a single map call.

Less than 0, greater than or equal to 0, and everything else. Hmmm, just what is in that "everything else" bucket?

For the else statement, as I understood the idea, was to make sure both pwm signals were 0 if no movement was required. Would removing the 'equal to' in the else if statement make more sense?

If you comment out the PID1 stuff, does that have an effect on PID2? Maybe the problem is the input devices for PID2.

I swapped components around, but got the same results. I will try commenting out pid1 and see.

Thanks for all the input!

4 
double Pk = 10; //speed it gets there
double Ik = 0;
double Dk = 0;

If you leave the I and D parameters zero you are just doing a P loop. You can probably gain a lot of efficiency by writing the P loop yourself:

error = setpoint - input;
output = constrain(error * Pk, -255, 255);

You can probably do all that in integers and save a lot of floating point overhead.

5

If you leave the I and D parameters zero you are just doing a P loop. You can probably gain a lot of efficiency by writing the P loop yourself:

This is just a base setup. I'll be working on tweaking settings once I get everything working properly. But I may try it to see how it works.

You can probably do all that in integers and save a lot of floating point overhead.

Could you explain this a bit more? I'm not sure what I should replace with integers.

Thank you!

6

I would recommend using the highest resolution possible from the analog pin to eliminate the

On side note: PID1 is rock steady,PID2 seems a bit jumpy, is there anything in the code that would cause that?

The concept of PID is to take an input and convert it to an output the input in most cases has no direct relation to the output example (temperature and signal to valve) we know what we want but have no way of predicting what signal we actually need to position the valve to reach that temperature. so if you degrade the input to only have 341 steps your valve position will have a max of 341 steps. now if you constrain the output to only use a portion of those steps (Proportional control only) your output could have much less!
Proof:

PID1.SetOutputLimits(-255,255);

your output is constrained to 511 steps( -255 to -1 = 255 steps and 0 to 255 = 256 steps)

your input error is multiplied by 10

double Pk = 10; //speed it gets there

and Proportional control routine:

error = setpoint - input;
Proportional = error * Pk;

You have mapped (map() returns int)the input to an integer (no floating point math) leaving you with 341 steps (682-341=341)

actual1 = map(actual1,0,1023,341,682);

Now you map it a second time to what could be a higher resolution but it still only has 341 steps

actual1 = map(actual1,341,682,-255,255);

with this in mind lets do the math to see how many steps you actually are using.

  • for sanity sake setpoint is zero
  • input is at its max per your map limit above

(255 - 0) * 10 = 2550
we now constrain it to 255 for our max output.
so if we do this on the negative side we will only be using +- 25 of the +- 255 input or 1/10 of the resolution of 341 so

In conclusion:
Your PID is only given 68 steps to control your output.

Recommendation:
use the analogRead() value directly into your PID and never use map.

float actual1;
// Kp = 5 because your range is exactly *2 of your intended code above
actual1 = (float) analogRead(A0);
Input1 = actual1 - 512; // subtract 512 to get the +- range you need

If you need to adjust it to a different range convert your analogRead() to a float and use multiplication to change the range

float actual1;
// note Kp = 10 Kp doesent change
actual1 = (float) analogRead(A0);
actual1 = (actual1 * 0.5) - 255; // (1024 / 2) = 512 and 512 -255 gives you a rante of +- 255 with a floating point resolution of 1024 steps
Input1 = actual1;

This should improve output quality of your PID because you will have more steps to work with (Kp at 10 you will end up with about 103)

Z

Also note that with Kp only (No Ki) when you are at setpoint your output will always be at zero (no error times anything = Zero)