[avr] Fix to atomic-access on SPI Transactions (Andrew Kroll)

Author: cmaglie

hardware/arduino/avr/libraries/SPI/SPI.cpp

@@ -103,11 +103,9 @@ void SPIClass::end() {
 
 void SPIClass::usingInterrupt(uint8_t interruptNumber)
 {
-  uint8_t mask;
-
-  if (modeFlags.interruptMode > 1) return;
-
-  noInterrupts();
+  uint8_t mask = 0;
+  uint8_t sreg = SREG;
+  noInterrupts(); // Protect from a scheduler and prevent transactionBegin
   switch (interruptNumber) {
   #ifdef SPI_INT0_MASK
   case 0: mask = SPI_INT0_MASK; break;
@@ -135,11 +133,11 @@ void SPIClass::usingInterrupt(uint8_t interruptNumber)
   #endif
   default:
     modeFlags.interruptMode = 2;
-    interrupts();
-    return;
+    break;
   }
-  modeFlags.interruptMode = 1;
   interruptMask |= mask;
-  interrupts();
+  if (!modeFlags.interruptMode)
+    modeFlags.interruptMode = 1;
+  SREG = sreg;
 }
 

hardware/arduino/avr/libraries/SPI/SPI.h

@@ -20,6 +20,13 @@
 // usingInterrupt(), and SPISetting(clock, bitOrder, dataMode)
 #define SPI_HAS_TRANSACTION 1
 
+// SPI_ATOMIC_VERSION means that SPI has atomicity fixes and what version.
+// This way when there is a bug fix you can check this define to alert users
+// of your code if it uses better version of this library.
+// This also implies everything that SPI_HAS_TRANSACTION as documented above is
+// available too.
+#define SPI_ATOMIC_VERSION 1
+
 // Uncomment this line to add detection of mismatched begin/end transactions.
 // A mismatch occurs if other libraries fail to use SPI.endTransaction() for
 // each SPI.beginTransaction().  Connect an LED to this pin.  The LED will turn
@@ -167,25 +174,33 @@ class SPIClass {
   // this function is used to gain exclusive access to the SPI bus
   // and configure the correct settings.
   inline static void beginTransaction(SPISettings settings) {
-    if (modeFlags.interruptMode > 0) {
-      #ifdef SPI_AVR_EIMSK
-      if (modeFlags.interruptMode == 1) {
-        interruptSave = SPI_AVR_EIMSK;
-        SPI_AVR_EIMSK &= ~interruptMask;
-      } else
-      #endif
-      {
-        interruptSave = SREG;
-        cli();
-      }
-    }
+    uint8_t sreg = SREG;
+    noInterrupts();
     #ifdef SPI_TRANSACTION_MISMATCH_LED
     if (modeFlags.inTransaction) {
       pinMode(SPI_TRANSACTION_MISMATCH_LED, OUTPUT);
       digitalWrite(SPI_TRANSACTION_MISMATCH_LED, HIGH);
     }
     modeFlags.inTransaction = true;
     #endif
+
+    #ifndef SPI_AVR_EIMSK
+    if (modeFlags.interruptMode) {
+      interruptSave = sreg;
+    }
+    #else
+    if (modeFlags.interruptMode == 2) {
+      interruptSave = sreg;
+    } else if (modeFlags.interruptMode == 1) {
+      interruptSave = SPI_AVR_EIMSK;
+      SPI_AVR_EIMSK &= ~interruptMask;
+      SREG = sreg;
+    }
+    #endif
+    else {
+      SREG = sreg;
+    }
+
     SPCR = settings.spcr;
     SPSR = settings.spsr;
   }
@@ -238,23 +253,31 @@ class SPIClass {
   // After performing a group of transfers and releasing the chip select
   // signal, this function allows others to access the SPI bus
   inline static void endTransaction(void) {
+    uint8_t sreg = SREG;
+    noInterrupts();
     #ifdef SPI_TRANSACTION_MISMATCH_LED
     if (!modeFlags.inTransaction) {
       pinMode(SPI_TRANSACTION_MISMATCH_LED, OUTPUT);
       digitalWrite(SPI_TRANSACTION_MISMATCH_LED, HIGH);
     }
     modeFlags.inTransaction = false;
     #endif
-    if (modeFlags.interruptMode > 0) {
-      #ifdef SPI_AVR_EIMSK
+    #ifndef SPI_AVR_EIMSK
+    if (modeFlags.interruptMode) {
+      SREG = interruptSave;
+    } else {
+      SREG = sreg;
+    }
+    #else
+    if (modeFlags.interruptMode == 2) {
+      SREG = interruptSave;
+    } else {
       if (modeFlags.interruptMode == 1) {
         SPI_AVR_EIMSK = interruptSave;
-      } else
-      #endif
-      {
-        SREG = interruptSave;
       }
+      SREG = sreg;
     }
+    #endif
   }
 
   // Disable the SPI bus