-
-
Notifications
You must be signed in to change notification settings - Fork 7k
[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.
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
Changes from all commits
51dcd9c
841e75d
2ef946f
b6d0897
6f71582
872da13
6df8847
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | |||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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 | |||||||||||||||||||||||||||||||||
|
@@ -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 | |||||||||||||||||||||||||||||||||
|
|||||||||||||||||||||||||||||||||
|
@@ -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 | |||||||||||||||||||||||||||||||||
|
@@ -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 | |||||||||||||||||||||||||||||||||
// (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; | |||||||||||||||||||||||||||||||||
} | |||||||||||||||||||||||||||||||||
|
@@ -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;< A93C /td> | |||||||||||||||||||||||||||||||||
} 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; | |||||||||||||||||||||||||||||||||
} | |||||||||||||||||||||||||||||||||
|
@@ -225,23 +258,31 @@ class SPIClass { | ||||||||||||||||||||||||||||||||
// After performing a group of transfers and releasing the chip select | |||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add asm volatile("nop"); between lines 249 and 250 for speedup There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, right right! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. only before it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | |||||||||||||||||||||||||||||||||
|
@@ -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; | |||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
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,
:-)