Disable interrupts when doing critical things?

Question

I've got a few functions like this that log, or do more critical things like get/save configuration variables.

/**
   Write weather data to csv
   commit: datetime,minHumidity,maxHumidity,minTemperature,maxTemperature,rainfall,irrigationRan
*/
void writeWeatherData() {
  noInterrupts();
  DBG_OUTPUT.println(F("[WEATHER DATA] Attempting to save weather data..."));
  bool writeHeader = SPIFFS.exists("/weather_data.csv") ? false : true;
  File file = SPIFFS.open("/weather_data.csv", "a");
  if (!file) {
    addPrintError("[WEATHER DATA] There was an error opening weather_data.csv for writing.");
    interrupts();
    return;
  }
  String data = "";
  if (writeHeader) {
    data += "datetime,minHumidity,maxHumidity,minTemperature,maxTemperature,rainfall,irrigationRan\r\n";
  }
  DateTime now = rtc.now();
  char buf[] = "MM/DD/YY hh:mm:ss";
  String t = now.toString(buf);
  float rainfall = getRainfall();
  data += t + "," + minHumidity + "," + maxHumidity + "," + minTemperature + "," + maxTemperature + "," + rainfall + "," + irrigationRan + "\r\n";
  if (file.print(data)) {
    DBG_OUTPUT.println(F("[WEATHER DATA] File was written."));
  } else {
    addPrintError("[WEATHER DATA] File write failed.");
  }
  file.close();
  DBG_OUTPUT.println(F("[WEATHER DATA] Task completed."));
  interrupts();
}

I surround code with noInterrupts and interrupts however I am not sure if it is required. Assume that an interrupt is triggered at the exact moment when the above function is saving something, what exactly happens if I didn't have noInterrupts .etc.?

Answer 1

Interrupts happen all the time and code works just fine without locking them out. It doesn't matter if it's at the exact moment that a function is saving something - a properly written interrupt handler will save any necessary state so that whatever was interrupted can continue without problems. If it doesn't, the software will crash hard.

You should lock out interrupts as little as possible. Interrupts are used for critical timing functions and for network and other communications. Locking them out will cause these functions to be unreliable and may disrupt network connections.

The only time you should disable interrupts is when a data structure that an interrupt handler needs to use is in an inconsistent state. If an interrupt handler needs to use a shared data structure, disable interrupts before beginning to modify it outside of the interrupt handler, and re-enable them when you're done. Make sure it's as brief as possible.

Interrupt handlers should do as little as possible. They definitely should not call SPIFFS, should not call the real time clock, should not allocate or instantiate objects and should not do i/o operations. And you should not be disabling interrupts around calls to SPIFFS in hopes of making it safe for your interrupt handler to also call SPIFFS.

Take this as an example: suppose you want to use SPIFFS in both an interrupt handler and outside the interrupt handler. So you think you'll be clever and disable interrupts before you call SPIFFS outside the interrupt handler. Do you know exactly how SPIFFS works? Do you know whether SPIFFS uses any other functions that you might need to disable interrupts for in other parts of your code so that the interrupt handler won't disrupt them? Do you know for certain that SPIFFS doesn't allocate memory temporarily? Imagine that it does - now you also need to find every place in your code that might allocate memory and lock out interrupts around it, too. And maybe SPIFFS doesn't allocate memory - but do you know that SPIFFS in the future will never allocate memory?

You can't make assumptions about exactly how modules that you call are implemented, or how they'll be implemented in the future.

Unless you seriously know what you're doing, on microcontrollers like Arduinos, the ESP8266 and the ESP32, the best thing an interrupt handler can do is just set a flag to indicate that the interrupt occurred, and let code in the loop() check for the flag. The flag should most likely just be a simple Boolean variable declared as volatile so that the C compiler is aware that its value may suddenly change:

volatile boolean interrupt_flag = false;

It's safe to set that interrupt_flag to true inside the interrupt handler and it's safe to check the value in loop(), reset it to false and do whatever work was needed. Don't do more than this unless you know what you're doing.

Answer 2

John Romkey provided you an excellent answer. I am adding this just to provide another perspective, hopefully complementary.

Interrupts are meant to handle the most time-critical tasks. Those tasks that cannot wait for the next loop() iteration, such as counting a pulse from an encoder, or getting a byte out of the UART receive buffer. If you delay them too much, they may miss their deadline, resulting in unreliable operation of the whole program (counter missing pulses, UART missing bytes...).

Sometimes you have a piece of code that cannot afford being interrupted. Maybe it is extremely time-critical, like the generation in software of a pulse that is exactly 0.5 µs wide. More commonly it will be reading data from memory that is being modified by an interrupt handler, and you don't want that data to be modified in the middle of the read operation. Such pieces of code are called critical sections, and you keep them safe in between noInterrupts() and interrupts(). If an interrupt request fires while the program is running a critical section, the request is put on hold and serviced only when the critical section is done. This adds some latency to the interrupt which, if excessive, can lead to the interrupt missing its deadline. This is the reason critical sections should be kept as short as possible.

You have been told that interrupt handlers should be kept as short as possible, but I am not sure you know why it is so. The fundamental reason is that interrupt handlers are themselves critical sections. While a handler is running, other interrupts are blocked. On some architectures (notably not AVR, I don't know about the ESP), only interrupts of lower priority are blocked, but it is still an issue. The rationale for keeping those handlers short applies equally to any critical section.

Now, just to provide an example of the sort of bad things that can happen if you block interrupts unnecessarily, consider this piece of code:

noInterrupts();
Serial.println("Inside critical section.");
interrupts();

What this does is put the string "Inside critical section." in a software buffer. Not a huge task you may say. Actually pushing the bytes out to the UART is the job of an interrupt handler, triggered by the UART when it is ready to accept a new byte. But then what happens if there is not enough free space in the buffer for the string? In this case, Serial.println() waits in a busy loop. It waits for the interrupt to move bytes out of the buffer and make space for the new string. But wait, we have just disabled interrupts... See the problem? This is the reason it is generally advised to never Serial.print() inside an interrupt handler. The same applies to any critical section.

Edit: As Juraj points out in a comment, the HardwareSerial code now implements a workaround against this problem: if it detects it has to wait with interrupts disabled, it takes care of calling the interrupt handler itself. This is a (relatively) late addition to the HardwareSerial code. Their authors presumably witnessed too many beginners doing debug prints from interrupt handlers and being bitten by this issue. Note that it is a workaround, not a solution: it will often make the critical section last for an unreasonably long time.

I wouldn't rely on similar APIs, or even on other cores, systematically implementing this kind of safeguards though: some library authors just trust the user to not do silly things.