Skip to content

Commit a9735bf

Browse files
committed
Fix atomicity issues in SPI::beginTransaction and SPI::endTransaction (Andrew Kroll)
Previously, it could happen that SPI::beginTransaction was interrupted by an ISR, while it is changing the SPI_AVR_EIMSK register or interruptSave variable (it seems that there is a small window after changing SPI_AVR_EIMSK where an interrupt might still occur). If this happens, interruptSave is overwritten with an invalid value, permanently disabling the pin interrupts. To prevent this, disable interrupts globally while changing these values.
1 parent 84b6cc2 commit a9735bf

File tree

1 file changed

+20
-2
lines changed
  • hardware/arduino/avr/libraries/SPI

1 file changed

+20
-2
lines changed

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

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,13 @@
2323
// SPI_HAS_NOTUSINGINTERRUPT means that SPI has notUsingInterrupt() method
2424
#define SPI_HAS_NOTUSINGINTERRUPT 1
2525

26+
// SPI_ATOMIC_VERSION means that SPI has atomicity fixes and what version.
27+
// This way when there is a bug fix you can check this define to alert users
28+
// of your code if it uses better version of this library.
29+
// This also implies everything that SPI_HAS_TRANSACTION as documented above is
30+
// available too.
31+
#define SPI_ATOMIC_VERSION 1
32+
2633
// Uncomment this line to add detection of mismatched begin/end transactions.
2734
// A mismatch occurs if other libraries fail to use SPI.endTransaction() for
2835
// each SPI.beginTransaction(). Connect an LED to this pin. The LED will turn
@@ -170,24 +177,29 @@ class SPIClass {
170177
// and configure the correct settings.
171178
inline static void beginTransaction(SPISettings settings) {
172179
if (interruptMode > 0) {
180+
uint8_t sreg = SREG;
181+
noInterrupts();
182+
173183
#ifdef SPI_AVR_EIMSK
174184
if (interruptMode == 1) {
175185
interruptSave = SPI_AVR_EIMSK;
176186
SPI_AVR_EIMSK &= ~interruptMask;
187+
SREG = sreg;
177188
} else
178189
#endif
179190
{
180-
interruptSave = SREG;
181-
cli();
191+
interruptSave = sreg;
182192
}
183193
}
194+
184195
#ifdef SPI_TRANSACTION_MISMATCH_LED
185196
if (inTransactionFlag) {
186197
pinMode(SPI_TRANSACTION_MISMATCH_LED, OUTPUT);
187198
digitalWrite(SPI_TRANSACTION_MISMATCH_LED, HIGH);
188199
}
189200
inTransactionFlag = 1;
190201
#endif
202+
191203
SPCR = settings.spcr;
192204
SPSR = settings.spsr;
193205
}
@@ -253,10 +265,16 @@ class SPIClass {
253265
}
254266
inTransactionFlag = 0;
255267
#endif
268+
256269
if (interruptMode > 0) {
270+
#ifdef SPI_AVR_EIMSK
271+
uint8_t sreg = SREG;
272+
#endif
273+
noInterrupts();
257274
#ifdef SPI_AVR_EIMSK
258275
if (interruptMode == 1) {
259276
SPI_AVR_EIMSK = interruptSave;
277+
SREG = sreg;
260278
} else
261279
#endif
262280
{

0 commit comments

Comments
 (0)