Critical sections in Arduino, interrupt consuming data

Question

We all know the interrupts, but how do they really work?

The problem:

In my sketch an interrupt method (invoked when pin goes high) is consuming data from a buffer and the main loop() is filling the buffer at random speeds (from Internet - I'm using ESP8266, not the Arduino). Although I think there shouldn't be any threading problems with this buffer - I might be wrong - the program hangs up after random number of seconds (about half a minute average - and then the built-in watchdog resets it after few seconds). When I detached the interrupt method and merged it into the main loop() code, the problem disappeared (but I can't leave it like that). So it's probably either that I'm doing something illegal in the interrupt method (maybe it's just taking too long - how much time can an interrupt method take?) - or the second possibility - it is a threading problem.

The questions:

The main question is: how to correctly synchronize the interrupt method? How to create a critical section if the interrupt method cannot wait (for a semaphore) and if I leave it, it won't be invoked again because the pin is already high? And the detailed questions:

1. How to create a thread-safe variable (a bool flag), so that interrupt method can skip the critical section if the main loop has entered it? (I understand the interrupt method is "safe" - it won't be interrupted - but how to secure the regular code?)
2. If I detach an interrupt method that is triggered when a pin goes high when it is low (for just a few fast instructions), the pin gets high in that time, and then I'll re-attach the method to this pin again, will it be invoked? The same question for noInterrupts(); How to make sure it will be invoked, if I can't check the pin and invoke (or not) the method manually in an atomic transaction with re-attaching the method?
3. If I detach this method (in main loop), is it possible that it will be invoked before the control is back from the detachInterrupt() method? (because the pin got high from external device in this very moment) And the same question for noInterrupts();
4. I've read that noInterrupts(); causes some things, like reading incoming serial data, to be skipped. Can this method be even used if I don't want that? Or can I have guaranteed that it won't happen if the method is fast enough? (if so, please define the "fast enough"!)

So, is there a common template for creating a critical section between an interrupt method and the regular code? Or at least synchronizing a variable access? And if not, how to make sure the detached/blocked pin method will be invoked after re-attaching/re-enabling it?

The common buffer code:

(if you want to check whether this can cause any "threading" problems)

class Buffer
{
  protected:
    byte *buf;
    int len, readpos, writepos;
    bool wasFull, wasEmpty;
    long ratioSum, ratioCount;

  public:
    Buffer(int length)
    {
      len = length;
      buf = new byte[len];
      readpos = writepos = 0;
    }

    bool isFull()
    {
      bool full = writepos - readpos >= len;
      wasFull &= full;
      return full;
    }

    bool isEmpty()
    {
      bool empty = readpos >= writepos;
      wasEmpty &= empty;
      return empty;
    }

    void write(byte b)
    {
      if (isFull()) { if (!wasFull) { wasFull = true; Serial.println("Error: Buffer overflow!"); } return; }
      int pos = (writepos++) % len;
      buf[pos] = b;
      if (readpos >= len && writepos >= len) { readpos -= len; writepos -= len; }
    }

    byte read()
    {
      if (isEmpty()) { if (!wasEmpty) { wasEmpty = true; Serial.println("Error: Buffer underflow!"); } return 0; }
      int pos = (readpos++) % len;
      if (readpos >= len && writepos >= len) { readpos -= len; writepos -= len; }
      return buf[pos];
    }

    int available()
    {
      return writepos - readpos;
    }

    void measureRatio()
    {
      ratioSum += available();
      ratioCount++;
    }

    int getRatio()
    {
      int ratio;
      if (ratioCount > 0) 
        ratio = 100 * ((float)(ratioSum / ratioCount) / (float)len);
      else
        ratio = 100 * ((float)available() / (float)len);
      ratioSum = 0;
      ratioCount = 0;
      return ratio;
    }
};

PS:

I have discovered that printing to Serial in the interrupt method (even without a flush() is causing my program to crash after max half a second. Is it just a duration of printing or is it something else? Also if you're thinking that the warning printing in my Buffer code can cause the crashes, then I assure you that they're not because I always check isEmpty() and isFull() before each read and write operation, so they just can't occur.

Answer 1

Warning: I am not familiar with the ESP8266 hardware, and some of my answers below may be only valid on the AVR architecture.

The simplest way to synchronize your main code with your interrupt service routine is to define a critical section in your loop() that wraps access to the shared data, and to make sure this critical section cannot be interrupted:

noInterrupts();
buffer.write(data);
interrupts();

This should be fine as long as you make the critical section as short as possible. Majenko's recommendation, to inspect the write() method with “a fine toothed comb” in order to minimize the critical section, is definitively good advice.

To understand the relevance of the “as short as possible” requirement, think about what could happen if the interrupt triggering event (the pin going high) happens while interrupts are disabled. If this happens once, it's not a problem: the interrupt is put on hold (technically, an interrupt flag is raised), and then it's serviced as soon as interrupts are enabled again. However, if the pin goes high twice while interrupts are blocked, then, when you enable interrupts again, the interrupt handler will be called only once. Whether this is a problem or not I cannot say, it all depends on what job the handler is supposed to do.

Now, to the specific sub-questions:

How to create a thread-safe variable (a bool flag)

Chances are the boolean can be read and set atomically, so it's inherently thread-safe. On an 8-bit CPU, like the AVR, access to a multi-byte variable has to be protected inside a critical section. But since you have a 32-bit CPU, I bet you have atomic access to variables up to 32-bit wide.

If I detach an interrupt method that is triggered when a pin goes high when it is low (for just a few fast instructions), the pin gets high in that time, and then I'll re-attach the method to this pin again, will it be invoked?

Hard to say without reading both the datasheet of the hardware and the source code of attachInterrupt(). It may be safer to keep the handler attached and start its code with something like if (doNotRunInterruptHandler) return;.

The same question for noInterrupts();

Yes, it will be invoked, but at most once.

If I detach this method (in main loop), is it possible that it will be invoked before the control is back from the detachInterrupt() method?

Yes, the interrupt can fire while detachInterrupt() is checking to see whether you gave it valid parameters.

same question for noInterrupts();

Check the implementation. On the AVR, noInterrupts() is atomic.

I've read that noInterrupts(); causes some things, like reading incoming serial data, to be skipped.

Nothing like this will happen if your critical section is fast enough.

please define the "fast enough"!

It depends on how long you can afford to delay the interrupt service routine. For example, if you are reading serial data at 115200 bps and the UART has only a one-byte buffer, then a 86 µs critical section can make you loose one byte.

Then, there is the risk that several interrupts of the same kind fire during the same same critical section. As said before, the interrupt will only be serviced once. If this is a problem, then you should not block interrupts for longer than the minimum possible period of the triggering event.

Also, there is always the risk that several, unrelated interrupts fire during the same critical section. Then, the one with the lowest priority will have to wait for the critical section to finish and then for the higher priority interrupt to be serviced.

Answer 2

The trick here is not to make your interrupt routine, or your main routine "thread safe" - instead you need to make the single item that is shared by the two "thread safe". That means your Buffer class.

You need to go through that with a fine toothed comb and work out which bits could possibly be accessed by the two contexts at once and protect them.

The bits that jump out at me immediately are the calculations and manipulations of writepos and readpos. Accesses to those variables need to be made as atomic as possible. One of the best ways of doing that is to massively restrict the actual access to those variables and wrap those accesses in critical sections (disable / enable interrupts). That keeps the critical sections as short as possible.

For instance, instead of directly manipulating the variables, as you enter the function you first create "cached" copies of the variables you will modify that could be modified at the same time (such as writepos being written to in the ISR):

cli();
int cachedWritePos = writepos;
sei();

Then you do your manipulations and calculations on those cached variables. Finally, just before you exit the function, you write your now safely changed cached variable values back to the member variables:

cli();
writepos = cachedWritePos;
sei();

The critical sections are then as brief as possible, and any heavy calculations (I count the modulus operator % as a heavy calculation) don't block the interrupt from firing.

Of course that is not a foolproof method and doesn't protect against two contexts changing the same variable and it getting out of step. To counter that you will have to use a mutex. There's no getting around that, other than ensuring that your program never changes the same variables from two different contexts. The only mutex you really have access to is a manually implemented spinlock.

On that note, I am having some trouble following the logical flow of how your buffer works. There are bits that don't look quite right to me, such as this bit:

if (readpos >= len && writepos >= len) { readpos -= len; writepos -= len; }

According to my way of thinking both readpos and writepos have to have reached (or exceeded) the end of the buffer for them to wrap around to the start again. That can't really happen properly in a function that is only writing, or a function that is only reading. I think you want to only be checking writepos and wrapping that in the write function, and only checking readpos and wrapping that around in the read function, not both in both functions.

As far as using Serial in an interrupt goes, the general rule is: don't do it. I don't know about the ESP8266's serial code, but certainly on the Arduino it relies on interrupts being used to take each character from the TX buffer and transmitting it through the UART interface. If you are in an interrupt routine and your (64 byte) serial TX buffer fills up any attempts to write to that buffer (Serial.print) will block until the interrupt that sends the characters has made some room for you - and of course that interrupt will never happen while you're blocking because you are in an interrupt at the time.

On systems with multiple priority levels of interrupt (such as a PIC32) the same holds true if you are in an interrupt routine that is at a higher priority than the Serial interrupt. If you are in a lower priority one the serial interrupt can still interrupt your current interrupt routine and function correctly.

Answer 3

My problem with crashing is fixed! :) I will share here what was wrong - maybe someone will have similar problem in the future.

But first...

I have marked Edgar Bonet's answer because he answered most of the difficult questions that were bothering me and for pointing me out that reading a 16-bit variable on 8-bit processor is not atomic (it was the most significant clue) and for helping me to understand that noInterrupts() is the safe and only way to synchronize with the interrupt method in my case. Thanks Edgar! But credits also go to Majenko because I wouldn't have guessed it without him. He pushed me into the right direction in our chat discussion - that I have to secure all the methods accessing readpos and writepos. Thanks Majenko. I agree with him that my Buffer code is ugly and it's not a good way to implement a circular buffer, but it was still logically OK - it just needed one more thing...

The solution

You probably know it already. The key is the non-atomic reading of readpos and writepos. For example: the code from loop() starts to read readpos, reads the first byte of it, then the interrupt happens, readpos changes there and the loop() continues reading the other (changed) half, which is then glued with the old (out of date) half - so - a random value. Or another one: the loop writes first byte of writepos (as it fills the buffer) and then interrupt fires and reads the first (changed) byte with the old (out of date) second byte. So the solution is to just wrap all access to those variables with noInterrupts() - and the whole code works perfect! :) Thank you guys! Not only for helping me to fix it, but for helping me to understand it.

Here is how it looks like now (non-crashing version):

class Buffer
{
  protected:
    byte *buf;
    const int len;
    int readpos, writepos;
    bool wasFull, wasEmpty;
    long ratioSum, ratioCount;

  public:
    Buffer(const int length)
      : len(length)
    {
      buf = new byte[len];
      readpos = writepos = 0;
    }

    bool isFull()
    {
      noInterrupts();
      bool full = writepos - readpos >= len;
      wasFull &= full;
      interrupts();
      return full;
    }

    bool isEmpty()
    {
      noInterrupts();
      bool empty = readpos >= writepos;
      wasEmpty &= empty;
      interrupts();
      return empty;
    }

    void write(byte b)
    {
      if (isFull()) return;//{ if (!wasFull) { wasFull = true; Serial.println("Error: Buffer overflow!"); } return; }
      int pos = (writepos++) % len;
      buf[pos] = b;
      if (readpos >= len && writepos >= len) { readpos -= len; writepos -= len; }
    }

    byte read()
    {
      if (isEmpty()) return 0;// { if (!wasEmpty) { wasEmpty = true; Serial.println("Error: Buffer underflow!"); } return 0; }
      int pos = (readpos++) % len;
      //if (readpos >= len && writepos >= len) { readpos -= len; writepos -= len; }
      return buf[pos];
    }

    int available()
    {
      noInterrupts();
      int result = writepos - readpos;
      interrupts();
      return result;
    }

    int free()
    {
      noInterrupts();
      int result = len - available();
      interrupts();
      return result;
    }

    void MeasureRatio()
    {
      ratioSum += available();
      ratioCount++;
    }

    int getRatio()
    {
      int ratio;
      if (ratioCount > 0) 
        ratio = 100 * ((float)(ratioSum / ratioCount) / (float)len);
      else
        ratio = 100 * ((float)available() / (float)len);
      ratioSum = 0;
      ratioCount = 0;
      return ratio;
    }
};