What is the correct way of using C++ objects (and volatile) inside interrupt routines?

I am currently working with Atmel AVR microcontrollers (gcc), but would like the answer to apply to the microcontroller world in general, i.e. usually single-threaded but with interrupts.

I know how to use volatile in C code when accessing a variable that can be modified in an ISR. For example:

uint8_t g_pushIndex = 0;
volatile uint8_t g_popIndex = 0;
uint8_t g_values[QUEUE_SIZE];

void waitForEmptyQueue()
{
    bool isQueueEmpty = false;
    while (!isQueueEmpty)
    {
        // Disable interrupts to ensure atomic access.
        cli();
        isQueueEmpty = (g_pushIndex == g_popIndex);
        sei();
    }
}

ISR(USART_UDRE_vect) // some interrupt routine
{
    // Interrupts are disabled here.
    if (g_pushIndex == g_popIndex)
    {
        usart::stopTransfer();
    }
    else
    {
        uint8_t value = g_values[g_popIndex++];
        g_popIndex &= MASK;
        usart::transmit(value);
    }
}

Because g_popIndex is modified inside the ISR and accessed outside of the ISR, it must be declared volatile to instruct the compiler not to optimize memory accesses to that variable. Note that, unless I'm mistaken, g_pushIndex and g_values need not be declared volatile, since they are not modified by the ISR.

I want to encapsulate the code related to the queue inside a class, so that it can be reused:

class Queue
{
public:
    Queue()
    : m_pushIndex(0)
    , m_popIndex(0)
    {

    }

    inline bool isEmpty() const
    {
        return (m_pushIndex == m_popIndex);
    }

    inline uint8_t pop()
    {
        uint8_t value = m_values[m_popIndex++];
        m_popIndex &= MASK;
        return value;
    }

    // other useful functions here...

private:
    uint8_t m_pushIndex;
    uint8_t m_popIndex;
    uint8_t m_values[QUEUE_SIZE];
};

Queue g_queue;

void waitForEmptyQueue()
{
    bool isQueueEmpty = false;
    while (!isQueueEmpty)
    {
        // Disable interrupts to ensure atomic access.
        cli();
        isQueueEmpty = g_queue.isEmpty();
        sei();
    }
}

ISR(USART_UDRE_vect) // some interrupt routine
{
    // Interrupts are disabled here.
    if (g_queue.isEmpty())
    {
        usart::stopTransfer();
    }
    else
    {
        usart::transmit(g_queue.pop());
    }
}

The code above is arguably more readable. However, what should be done about volatile in this case?

1) Is it still needed? Does calling the method Queue::isEmpty() somehow ensures non-optimized access to g_queue.m_popIndex, even if the function is declared inline? I doubt that. I know that compilers use heuristics to determine if an access should not be optimized, but I dislike relying on such heuristics as a general solution.

2) I think a working (and efficient) solution is to declare the member Queue::m_popIndex volatile inside the class definition. However, I dislike this solution, because the designer of class Queue needs to know exactly how it will be used to know which member variable must be volatile. It will not scale well with future code changes. Also, all Queue instances will now have a volatile member, even if some are not used inside an ISR.

3) If one looks at the Queue class as if it were a built-in, I think the natural solution would be to declare the global instance g_queue itself as volatile, since it is modified in the ISR and accessed outside of the ISR. However, this doesn't work well, because only volatile functions can be called on volatile objects. Suddenly, all member functions of Queue must be declared volatile (not just the const ones or the ones used inside the ISR). Again, how can the designer of Queue know that in advance? Also, this penalize all Queue users. There is still the possibility of duplicating all member functions and having both volatile and non-volatile overloads in the class, so that non-volatile users are not penalized. Not pretty.

4) The Queue class could be templated on a policy class that can optionally add volatile to all its member variables only when needed. Again, the class designer need to know that in advance and the solution is more complicated to understand, but oh well.

I am curious to know if I am missing some easier solution to this. As a side note, I am compiling with no C++11/14 support (yet).

Yes, inline is definetely needed.
1) Compilers generally places a new copy of the inlined function in each place it is called. This optimization seems to not affect volatile variables. So this is OK.
2) I second this as the correct solution(with an extension). Because your only variable that needs to be volatile is really queue index.
3) Nope, no need to mark whole class instance volatile since it may prevent other potential optimizations.
4) You can use inheritance. An interface that declares which functions must a queue have, and two inherited classes for one using with ISR(has queue index volatile) and the other for not using ISR. Additionally, you can always define your class also templated:

template<typename T>
class IQueue
{
public:
        virtual bool isEmpty() const = 0;
        virtual T pop() = 0;
protected:
    uint8_t pushIndex;
    T values[QUEUE_SIZE];
};


template<typename T>
class ISRQueue : public IQueue<T>
{
    volatile uint8_t popIndex;
public:
    inline bool isEmpty()const
    {
        return (pushIndex == popIndex);
    }

    inline T pop()
    {
        T value = values[popIndex++];
        popIndex &= MASK;
        return value;
    }
};

template<typename T>
class Queue : public IQueue<T>
{
    uint8_t popIndex;
public:
    inline bool isEmpty()const
    {
        return (pushIndex == popIndex);
    }

    inline T pop()
    {
        T value = values[popIndex++];
        popIndex &= MASK;
        return value;
    }
};

typedef ISRQueue<uint8_t> ISRQueueUInt;
typedef ISRQueue<uint8_t> QueueUInt;

Recommended topics

Hot tags