8000 [AVR] SPI atomicity fix avr (part 1/2 of #2376) by cmaglie · Pull Request #2381 · arduino/Arduino · GitHub
[go: up one dir, main page]
Skip to content

[AVR] SPI atomicity fix avr (part 1/2 of #2376) #2381

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

Closed
wants to merge 7 commits into from
Closed

[AVR] SPI atomicity fix avr (part 1/2 of #2376) #2381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
51dcd9c
[avr] Improved SPI speed on 16bit transfer.
cmaglie Oct 20, 2014
841e75d
[avr] Use a bitfield structure to save SPI state (Andrew Kroll)
cmaglie Oct 21, 2014
2ef946f
[avr] Small comments and headers fixes in SPI
cmaglie Oct 21, 2014
b6d0897
[avr] Made SPI.begin() and SPI.end() synchronized (Andrew Kroll)
cmaglie Oct 21, 2014
6f71582
[avr] Fix to atomic-access on SPI Transactions (Andrew Kroll)
cmaglie Oct 21, 2014
872da13
[avr] Added SPI.notUsingInterrupt() (Andrew Kroll)
cmaglie Oct 21, 2014
6df8847
[avr] SPI: removed redundant include
cmaglie Oct 21, 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
123 changes: 87 additions & 36 deletions hardware/arduino/avr/libraries/SPI/SPI.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
/*
* 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 @@ -10,45 +12,55 @@
*/

#include "SPI.h"
#include "pins_arduino.h"

SPIClass SPI;

uint8_t SPIClass::interruptMode = 0;
SPIflags_t SPIClass::modeFlags = {false, false, 0};
uint8_t SPIClass::interruptMask = 0;
uint8_t SPIClass::interruptSave = 0;
#ifdef SPI_TRANSACTION_MISMATCH_LED
uint8_t SPIClass::inTransactionFlag = 0;
#endif

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 (!modeFlags.initialized) {
modeFlags.initialized = true;
// 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);
}
SREG = sreg;
}

void SPIClass::end() {
SPCR &= ~_BV(SPE);
uint8_t sreg = SREG;
noInterrupts(); // Protect from a scheduler and prevent transactionBegin
if (!modeFlags.interruptMode && modeFlags.initialized) {
SPCR &= ~_BV(SPE);
modeFlags.initialized = false;
#ifdef SPI_TRANSACTION_MISMATCH_LED
modeFlags.inTransaction = false;
#endif
}
SREG = sreg;
}

// mapping of interrupt numbers to bits within SPI_AVR_EIMSK
Expand Down Expand Up @@ -90,11 +102,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 All @@ -121,12 +131,53 @@ void SPIClass::usingInterrupt(uint8_t interruptNumber)
case 7: mask = SPI_INT7_MASK; break;
#endif
default:
interruptMode = 2;
interrupts();
return;
modeFlags.interruptMode = 2;
break;
}
interruptMode = 1;
interruptMask |= mask;
interrupts();
if (!modeFlags.interruptMode)
modeFlags.interruptMode = 1;
SREG = sreg;
}

void SPIClass::notUsingInterrupt(uint8_t interruptNumber)
{
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:
if (interruptMask) {
modeFlags.interruptMode = 1;
} else {
modeFlags.interruptMode = 0;
}
break;
}
interruptMask &= ~mask;
SREG = sreg;
}
96 changes: 67 additions & 29 deletions hardware/arduino/avr/libraries/SPI/SPI.h
View file
Open in desktop
< 6D4E td class="blob-num blob-num-deletion empty-cell">
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,9 +20,19 @@
// usingInterrupt(), and SPISetting(clock, bitOrder, dataMode)
#define SPI_HAS_TRANSACTION 1

// SPI_HAS_NOTUSINGINTERRUPT means that SPI has notUsingInterrup() 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 a LED to this pin. The LED will turn
// each SPI.beginTransaction(). Connect an LED to this pin. The LED will turn
// on if any mismatch is ever detected.
//#define SPI_TRANSACTION_MISMATCH_LED 5

Expand Down Expand Up @@ -49,6 +60,14 @@
#define SPI_CLOCK_MASK 0x03 // SPR1 = bit 1, SPR0 = bit 0 on SPCR
#define SPI_2XCLOCK_MASK 0x01 // SPI2X = bit 0 on SPSR

// Flags for the state of SPI, used as needed.
// Normally inTransaction is not used.
typedef struct SPIflags {
bool initialized : 1; // tells us that begin() was called
bool inTransaction : 1;
uint8_t interruptMode : 6; // 0=none, 1=mask, 2=global (more can be added)
} __attribute__((packed)) SPIflags_t;

// define SPI_AVR_EIMSK for AVR boards with external interrupt pins
#if defined(EIMSK)
#define SPI_AVR_EIMSK EIMSK
Expand Down Expand Up @@ -147,36 +166,46 @@ class SPIClass {
// Initialize the SPI library
static void begin();

// If SPI is to used from within an interrupt, this function registers
// If SPI is used from within an interrupt, this function registers
// that interrupt with the SPI library, so beginTransaction() can
// prevent conflicts. The input interruptNumber is the number used
// with attachInterrupt. If SPI is used from a different interrupt
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// If SPI is to used from within an interrupt,
Whut?
// If SPI is used from within an 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);

// 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) {
#ifdef SPI_AVR_EIMSK
if (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 (inTransactionFlag) {
if (modeFlags.inTransaction) {
pinMode(SPI_TRANSACTION_MISMATCH_LED, OUTPUT);
digitalWrite(SPI_TRANSACTION_MISMATCH_LED, HIGH);
}
inTransactionFlag = 1;
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;
}
Expand All @@ -193,16 +222,20 @@ class SPIClass {
in.val = data;
if (!(SPCR & _BV(DORD))) {
SPDR = in.msb;
asm volatile("nop");
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 All @@ -225,23 +258,31 @@ class SPIClass {
// After performing a group of transfers and releasing the chip select
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add asm volatile("nop"); between lines 249 and 250 for speedup

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right right!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should add a NOP between 254 and 255 too (just after the while loop)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only before it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xxxajk

I've made some test with the following sketch:

#include <SPI.h>

char buff[1000];

void testByteTransfer() {
  unsigned long start = micros();
  for (uint16_t i=0; i<10000; i++) {
    SPI.transfer(0x55);
    SPI.transfer(0xAA);
    SPI.transfer(0x55);
    SPI.transfer(0xAA);
    SPI.transfer(0x55);
    SPI.transfer(0xAA);
    SPI.transfer(0x55);
    SPI.transfer(0xAA);
    SPI.transfer(0x55);
    SPI.transfer(0xAA);
  }
  unsigned long stop = micros();
  Serial.print("Byte transfer (us): ");
  Serial.println(stop-start);
  delay(500); // Wait for serial buffer to flush
}

void testWordTransfer() {
  unsigned long start = micros();
  for (uint16_t i=0; i<10000; i++) {
    SPI.transfer16(0xAA55);
    SPI.transfer16(0x55AA);
    SPI.transfer16(0xAA55);
    SPI.transfer16(0x55AA);
    SPI.transfer16(0xAA55);
    SPI.transfer16(0x55AA);
    SPI.transfer16(0xAA55);
    SPI.transfer16(0x55AA);
    SPI.transfer16(0xAA55);
    SPI.transfer16(0x55AA);
  }
  unsigned long stop = micros();
  Serial.print("Word transfer (us): ");
  Serial.println(stop-start);
  delay(500); // Wait for serial buffer to flush
}

void testSmallBlockTransfer() {
  unsigned long start = micros();
  for (uint16_t i=0; i<10000; i++)
    SPI.transfer(buff, 4);
  unsigned long stop = micros();
  Serial.print("Small block transfer (us): ");
  Serial.println(stop-start);
  delay(500); // Wait for serial buffer to flush
}

void testLargeBlockTransfer() {
  unsigned long start = micros();
  for (uint16_t i=0; i<100; i++)
    SPI.transfer(buff, 1000);
  unsigned long stop = micros();
  Serial.print("Large block transfer: ");
  Serial.println(stop-start);
  delay(500); // Wait for serial buffer to flush
}

void setup() {
  SPI.begin();
  // Set SPI to max speed
  SPI.beginTransaction(SPISettings(100000000, MSBFIRST, SPI_MODE0));
  Serial.begin(9600);

  testByteTransfer();
  testWordTransfer();
  testSmallBlockTransfer();
  testLargeBlockTransfer();
}

void loop() {
}

It basically measure the time to perform a couple of SPI transfers.
Tested on a Mega1280 I got the following results:

Byte transfer uS
with NOP 197304 (best)
without NOP 216260
Word transfer uS
with both NOPs 360804 (best)
with first NOP only 379732
with second NOP only 379732
without NOPs 398448
Block transfer uS (small) uS (large)
with both NOPs 72308 144760
with NOP inside send loop only 71676 144744
with NOP on the last byte only 70428 138472
without NOPs 69784 (best) 138460 (best)

so my conclusions are that the commit is right as is it now and the block transfer cannot be improved by adding NOPs. Do you agree or I'm missing something?

Copy link
Contributor
@xxxajk xxxajk Oct 22, 2014 •
edited by cmaglie
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The numbers don't lie. I'll see what I can come up with in about 3 to 4
hours.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested a few other alternatives, and it appears that there is enough latency in doing the other assignments so the nop is not needed.

// 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 (!inTransactionFlag) {
if (!modeFlags.inTransaction) {
pinMode(SPI_TRANSACTION_MISMATCH_LED, OUTPUT);
digitalWrite(SPI_TRANSACTION_MISMATCH_LED, HIGH);
}
inTransactionFlag = 0;
modeFlags.inTransaction = false;
#endif
if (interruptMode > 0) {
#ifdef SPI_AVR_EIMSK
if (interruptMode == 1) {
#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
Expand Down Expand Up @@ -271,12 +312,9 @@ class SPIClass {
inline static void detachInterrupt() { SPCR &= ~_BV(SPIE); }

private:
static uint8_t interruptMode; // 0=none, 1=mask, 2=global
static SPIflags_t modeFlags; // Flags for the state and mode of SPI
static uint8_t interruptMask; // which interrupts to mask
static uint8_t interruptSave; // temp storage, to restore state
#ifdef SPI_TRANSACTION_MISMATCH_LED
static uint8_t inTransactionFlag;
#endif
};

extern SPIClass SPI;
Expand Down
0