Should I use volatile with this library (and where)?

I'm using the Arduino PID library and I want to use it inside an ISR like:

ISR(TIMER1_COMPA_vect) {
  myPID.compute();
}

However, I can tell that this is a bad idea since variables used in ISRs need to use the volatile keyword. Okay, so I declare the PID like this:

PID myPID(&input, &output, &setpoint, Kp, Ki, Kd, DIRECT);

and I declare the input, output, and setpoint, with volatile. However, the library doesn't accept volatile variables.

My first thought to convert it was to add volatile everywhere. But should I add it:

Before internal variables
Before the class (when declaring it, like above) or in the header file?
In the variables in my library and the variable types for the arguments?

What should I do?

Solution

What should I do?

Use your code as shown without any of the volatile and be happy :)

The volatile keyword is only a hint to the compiler that a variable might change by unseen paths. If a simple variable was modified only by the ISR, then it would not hurt to mark volatile. Such a mark could prevent the compiler from doing the wrong thing with code like this:

volatile int n = 0;

ISR(TIMER1_COMPA_vect) {
    n = 1;
}


void loop() {

    n = 0;

    // At this point the tricky compiler might think 
    // "n must be zero, why do this comparison?"
    // it might also think,
    // "hey, I've got the value of n still sitting in a
    // register, I don't even have to read it from memory
    // The volatile keyword says "stop thinking"

    if(n == 1) {
      // do something when ISR() occurred just before if()
      // a very small window, but it would eventually happen
      n = 0;
    }

 }

For your object, it is only about whether the changes introduced by the ISR cause variable changes that other users of the object might not see. If the PID object is consuming the input and the only driver of the outputs via compute() there isn't an issue. In other words, the changes in compute() are not affecting the main loop()

The bigger concern should be consistency which is not fixed with volatile. If setpoint is a 32 bit integer then the value in memory cannot change atomically. The main loop() might have an innocent line

setpoint = setpoint + 20;

But this involves getting the value from memory into a register, performing the math and writing back out. At any clock tick in the process, the interrupt may occur. Exactly in the middle of storing the new value of setpoint to memory, the interrupt might occur (will occur eventually).

You protect this by disabling the interrupts in the main loop() while accessing the shared object.

nointerrupts()
setpoint = setpoint + 20;
interrupts()

Now you are ensured that the interrupt will not occur at the exact worst moment and execute compute() on a broken value of setpoint.