Skip to content

[AVR] SPI atomicity fix avr (part 1/2 of #2376), second try #2449

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Dec 2, 2014
Merged

[AVR] SPI atomicity fix avr (part 1/2 of #2376), second try #2449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
53e25d8
[avr] Improved SPI speed on 16bit transfer.
cmaglie Oct 20, 2014
8344812
[avr] Made SPI.begin() and SPI.end() synchronized (Andrew Kroll)
cmaglie Oct 21, 2014
d9537cb
[avr] Added SPI.notUsingInterrupt() (Andrew Kroll)
cmaglie Oct 21, 2014
84b6cc2
[avr] Made SPI.usingInterrupt() synchronized (Andrew Kroll)
cmaglie Nov 18, 2014
a9735bf
Fix atomicity issues in SPI::beginTransaction and SPI::endTransaction…
cmaglie Nov 18, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
119 changes: 90 additions & 29 deletions hardware/arduino/avr/libraries/SPI/SPI.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* Copyright (c) 2010 by Cristian Maglie <c.maglie@bug.st>
* Copyright (c) 2014 by Paul Stoffregen <paul@pjrc.com> (Transaction API)
* Copyright (c) 2014 by Matthijs Kooijman <matthijs@stdin.nl> (SPISettings AVR)
* Copyright (c) 2014 by Andrew J. Kroll <xxxajk@gmail.com> (atomicity fixes)
* SPI Master library for arduino.
*
* This file is free software; you can redistribute it and/or modify
Expand All @@ -14,6 +15,7 @@

SPIClass SPI;

uint8_t SPIClass::initialized = 0;
uint8_t SPIClass::interruptMode = 0;
uint8_t SPIClass::interruptMask = 0;
uint8_t SPIClass::interruptSave = 0;
Expand All @@ -23,32 +25,51 @@ uint8_t SPIClass::inTransactionFlag = 0;

void SPIClass::begin()
{
// Set SS to high so a connected chip will be "deselected" by default
digitalWrite(SS, HIGH);
uint8_t sreg = SREG;
noInterrupts(); // Protect from a scheduler and prevent transactionBegin
if (!initialized) {
// Set SS to high so a connected chip will be "deselected" by default
digitalWrite(SS, HIGH);

// When the SS pin is set as OUTPUT, it can be used as
// a general purpose output port (it doesn't influence
// SPI operations).
pinMode(SS, OUTPUT);
// When the SS pin is set as OUTPUT, it can be used as
// a general purpose output port (it doesn't influence
// SPI operations).
pinMode(SS, OUTPUT);

// Warning: if the SS pin ever becomes a LOW INPUT then SPI
// automatically switches to Slave, so the data direction of
// the SS pin MUST be kept as OUTPUT.
SPCR |= _BV(MSTR);
SPCR |= _BV(SPE);
// Warning: if the SS pin ever becomes a LOW INPUT then SPI
// automatically switches to Slave, so the data direction of
// the SS pin MUST be kept as OUTPUT.
SPCR |= _BV(MSTR);
SPCR |= _BV(SPE);

// Set direction register for SCK and MOSI pin.
// MISO pin automatically overrides to INPUT.
// By doing this AFTER enabling SPI, we avoid accidentally
// clocking in a single bit since the lines go directly
// from "input" to SPI control.
// http://code.google.com/p/arduino/issues/detail?id=888
pinMode(SCK, OUTPUT);
pinMode(MOSI, OUTPUT);
// Set direction register for SCK and MOSI pin.
// MISO pin automatically overrides to INPUT.
// By doing this AFTER enabling SPI, we avoid accidentally
// clocking in a single bit since the lines go directly
// from "input" to SPI control.
// http://code.google.com/p/arduino/issues/detail?id=888
pinMode(SCK, OUTPUT);
pinMode(MOSI, OUTPUT);
}
initialized++; // reference count
SREG = sreg;
}

void SPIClass::end() {
SPCR &= ~_BV(SPE);
uint8_t sreg = SREG;
noInterrupts(); // Protect from a scheduler and prevent transactionBegin
// Decrease the reference counter
if (initialized)
initialized--;
// If there are no more references disable SPI
if (!initialized) {
SPCR &= ~_BV(SPE);
interruptMode = 0;
#ifdef SPI_TRANSACTION_MISMATCH_LED
inTransactionFlag = 0;
#endif
}
SREG = sreg;
}

// mapping of interrupt numbers to bits within SPI_AVR_EIMSK
Expand Down Expand Up @@ -90,11 +111,9 @@ void SPIClass::end() {

void SPIClass::usingInterrupt(uint8_t interruptNumber)
{
uint8_t mask;

if (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;
Expand Down Expand Up @@ -122,11 +141,53 @@ void SPIClass::usingInterrupt(uint8_t interruptNumber)
#endif
default:
interruptMode = 2;
interrupts();
return;
break;
}
interruptMode = 1;
interruptMask |= mask;
interrupts();
if (!interruptMode)
interruptMode = 1;
SREG = sreg;
}

void SPIClass::notUsingInterrupt(uint8_t interruptNumber)
{
// Once in mode 2 we can't go back to 0 without a proper reference count
if (interruptMode == 2)
return;
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;
#endif
#ifdef SPI_INT1_MASK
case 1: mask = SPI_INT1_MASK; break;
#endif
#ifdef SPI_INT2_MASK
case 2: mask = SPI_INT2_MASK; break;
#endif
#ifdef SPI_INT3_MASK
case 3: mask = SPI_INT3_MASK; break;
#endif
#ifdef SPI_INT4_MASK
case 4: mask = SPI_INT4_MASK; break;
#endif
#ifdef SPI_INT5_MASK
case 5: mask = SPI_INT5_MASK; break;
#endif
#ifdef SPI_INT6_MASK
case 6: mask = SPI_INT6_MASK; break;
#endif
#ifdef SPI_INT7_MASK
case 7: mask = SPI_INT7_MASK; break;
#endif
default:
break;
// this case can't be reached
}
interruptMask &= ~mask;
if (!interruptMask)
interruptMode = 0;
SREG = sreg;
}
44 changes: 42 additions & 2 deletions hardware/arduino/avr/libraries/SPI/SPI.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* Copyright (c) 2010 by Cristian Maglie <c.maglie@bug.st>
* Copyright (c) 2014 by Paul Stoffregen <paul@pjrc.com> (Transaction API)
* Copyright (c) 2014 by Matthijs Kooijman <matthijs@stdin.nl> (SPISettings AVR)
* Copyright (c) 2014 by Andrew J. Kroll <xxxajk@gmail.com> (atomicity fixes)
* SPI Master library for arduino.
*
* This file is free software; you can redistribute it and/or modify
Expand All @@ -19,6 +20,16 @@
// usingInterrupt(), and SPISetting(clock, bitOrder, dataMode)
#define SPI_HAS_TRANSACTION 1

// SPI_HAS_NOTUSINGINTERRUPT means that SPI has notUsingInterrupt() method
#define SPI_HAS_NOTUSINGINTERRUPT 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
Expand Down Expand Up @@ -153,37 +164,55 @@ class SPIClass {
// with attachInterrupt. If SPI is used from a different interrupt
// (eg, a timer), interruptNumber should be 255.
static void usingInterrupt(uint8_t interruptNumber);
// And this does the opposite.
static void notUsingInterrupt(uint8_t interruptNumber);
// Note: the usingInterrupt and notUsingInterrupt functions should
// not to be called from ISR context or inside a transaction.
// For details see:
// https://github.com/arduino/Arduino/pull/2381
// https://github.com/arduino/Arduino/pull/2449

// Before using SPI.transfer() or asserting chip select pins,
// this function is used to gain exclusive access to the SPI bus
// and configure the correct settings.
inline static void beginTransaction(SPISettings settings) {
if (interruptMode > 0) {
uint8_t sreg = SREG;
noInterrupts();

#ifdef SPI_AVR_EIMSK
if (interruptMode == 1) {
interruptSave = SPI_AVR_EIMSK;
SPI_AVR_EIMSK &= ~interruptMask;
SREG = sreg;
} else
#endif
{
interruptSave = SREG;
cli();
interruptSave = sreg;
}
}

#ifdef SPI_TRANSACTION_MISMATCH_LED
if (inTransactionFlag) {
pinMode(SPI_TRANSACTION_MISMATCH_LED, OUTPUT);
digitalWrite(SPI_TRANSACTION_MISMATCH_LED, HIGH);
}
inTransactionFlag = 1;
#endif

SPCR = settings.spcr;
SPSR = settings.spsr;
}

// Write to the SPI bus (MOSI pin) and also receive (MISO pin)
inline static uint8_t transfer(uint8_t data) {
SPDR = data;
/*
* The following NOP introduces a small delay that can prevent the wait
* loop form iterating when running at the maximum speed. This gives
* about 10% more speed, even if it seems counter-intuitive. At lower
* speeds it is unnoticed.
*/
asm volatile("nop");
while (!(SPSR & _BV(SPIF))) ; // wait
return SPDR;
Expand All @@ -193,16 +222,20 @@ class SPIClass {
in.val = data;
if (!(SPCR & _BV(DORD))) {
SPDR = in.msb;
asm volatile("nop"); // See transfer(uint8_t) function
while (!(SPSR & _BV(SPIF))) ;
out.msb = SPDR;
SPDR = in.lsb;
asm volatile("nop");
while (!(SPSR & _BV(SPIF))) ;
out.lsb = SPDR;
} else {
SPDR = in.lsb;
asm volatile("nop");
while (!(SPSR & _BV(SPIF))) ;
out.lsb = SPDR;
SPDR = in.msb;
asm volatile("nop");
while (!(SPSR & _BV(SPIF))) ;
out.msb = SPDR;
}
Expand Down Expand Up @@ -232,10 +265,16 @@ class SPIClass {
}
inTransactionFlag = 0;
#endif

if (interruptMode > 0) {
#ifdef SPI_AVR_EIMSK
uint8_t sreg = SREG;
#endif
noInterrupts();
#ifdef SPI_AVR_EIMSK
if (interruptMode == 1) {
SPI_AVR_EIMSK = interruptSave;
SREG = sreg;
} else
#endif
{
Expand Down Expand Up @@ -271,6 +310,7 @@ class SPIClass {
inline static void detachInterrupt() { SPCR &= ~_BV(SPIE); }

private:
static uint8_t initialized;
static uint8_t interruptMode; // 0=none, 1=mask, 2=global
static uint8_t interruptMask; // which interrupts to mask
static uint8_t interruptSave; // temp storage, to restore state
Expand Down