From 51dcd9c00ed8dfda91c91003f659f8ce2eb0432d Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Mon, 20 Oct 2014 19:46:08 +0200 Subject: [PATCH 1/7] [avr] Improved SPI speed on 16bit transfer. From https://github.com/arduino/Arduino/pull/2376#issuecomment-59671152 Quoting Andrew Kroll: [..this commit..] introduces a small delay that can prevent the wait loop form iterating when running at the maximum speed. This gives you a little more speed, even if it seems counter-intuitive. At lower speeds, it is unnoticed. Watch the output on an oscilloscope when running full SPI speed, and you should see closer back-to-back writes. Quoting Paul Stoffregen: I did quite a bit of experimenting with the NOP addition. The one that's in my copy gives about a 10% speedup on AVR. --- hardware/arduino/avr/libraries/SPI/SPI.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/hardware/arduino/avr/libraries/SPI/SPI.h b/hardware/arduino/avr/libraries/SPI/SPI.h index 962100bc016..b3d48f7a75e 100644 --- a/hardware/arduino/avr/libraries/SPI/SPI.h +++ b/hardware/arduino/avr/libraries/SPI/SPI.h @@ -193,16 +193,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; } From 841e75da7f3b1539affb88c6a15dabb86fed92b4 Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Tue, 21 Oct 2014 12:39:25 +0200 Subject: [PATCH 2/7] [avr] Use a bitfield structure to save SPI state (Andrew Kroll) --- hardware/arduino/avr/libraries/SPI/SPI.cpp | 12 +++------ hardware/arduino/avr/libraries/SPI/SPI.h | 29 +++++++++++++--------- 2 files changed, 21 insertions(+), 20 deletions(-) diff --git a/hardware/arduino/avr/libraries/SPI/SPI.cpp b/hardware/arduino/avr/libraries/SPI/SPI.cpp index 9a7a400e18f..5c9a76ad087 100644 --- a/hardware/arduino/avr/libraries/SPI/SPI.cpp +++ b/hardware/arduino/avr/libraries/SPI/SPI.cpp @@ -13,13 +13,9 @@ #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() { @@ -92,7 +88,7 @@ void SPIClass::usingInterrupt(uint8_t interruptNumber) { uint8_t mask; - if (interruptMode > 1) return; + if (modeFlags.interruptMode > 1) return; noInterrupts(); switch (interruptNumber) { @@ -121,11 +117,11 @@ void SPIClass::usingInterrupt(uint8_t interruptNumber) case 7: mask = SPI_INT7_MASK; break; #endif default: - interruptMode = 2; + modeFlags.interruptMode = 2; interrupts(); return; } - interruptMode = 1; + modeFlags.interruptMode = 1; interruptMask |= mask; interrupts(); } diff --git a/hardware/arduino/avr/libraries/SPI/SPI.h b/hardware/arduino/avr/libraries/SPI/SPI.h index b3d48f7a75e..bfb45efe3da 100644 --- a/hardware/arduino/avr/libraries/SPI/SPI.h +++ b/hardware/arduino/avr/libraries/SPI/SPI.h @@ -49,6 +49,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 padding : 1; + 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 @@ -158,9 +166,9 @@ 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 (interruptMode > 0) { + if (modeFlags.interruptMode > 0) { #ifdef SPI_AVR_EIMSK - if (interruptMode == 1) { + if (modeFlags.interruptMode == 1) { interruptSave = SPI_AVR_EIMSK; SPI_AVR_EIMSK &= ~interruptMask; } else @@ -171,11 +179,11 @@ class SPIClass { } } #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 SPCR = settings.spcr; SPSR = settings.spsr; @@ -230,15 +238,15 @@ class SPIClass { // signal, this function allows others to access the SPI bus inline static void endTransaction(void) { #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) { + if (modeFlags.interruptMode > 0) { #ifdef SPI_AVR_EIMSK - if (interruptMode == 1) { + if (modeFlags.interruptMode == 1) { SPI_AVR_EIMSK = interruptSave; } else #endif @@ -275,12 +283,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; From 2ef946f44224dd1481761073eae5f339bbcb7728 Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Tue, 21 Oct 2014 12:52:06 +0200 Subject: [PATCH 3/7] [avr] Small comments and headers fixes in SPI --- hardware/arduino/avr/libraries/SPI/SPI.cpp | 1 + hardware/arduino/avr/libraries/SPI/SPI.h | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/hardware/arduino/avr/libraries/SPI/SPI.cpp b/hardware/arduino/avr/libraries/SPI/SPI.cpp index 5c9a76ad087..5381033f571 100644 --- a/hardware/arduino/avr/libraries/SPI/SPI.cpp +++ b/hardware/arduino/avr/libraries/SPI/SPI.cpp @@ -1,6 +1,7 @@ /* * Copyright (c) 2010 by Cristian Maglie * Copyright (c) 2014 by Paul Stoffregen (Transaction API) + * Copyright (c) 2014 by Matthijs Kooijman (SPISettings AVR) * SPI Master library for arduino. * * This file is free software; you can redistribute it and/or modify diff --git a/hardware/arduino/avr/libraries/SPI/SPI.h b/hardware/arduino/avr/libraries/SPI/SPI.h index bfb45efe3da..22a3cc438aa 100644 --- a/hardware/arduino/avr/libraries/SPI/SPI.h +++ b/hardware/arduino/avr/libraries/SPI/SPI.h @@ -21,7 +21,7 @@ // 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 @@ -155,7 +155,7 @@ 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 From b6d0897f066eb86bb4eb3daa482e0ed67e696a92 Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Tue, 21 Oct 2014 12:57:41 +0200 Subject: [PATCH 4/7] [avr] Made SPI.begin() and SPI.end() synchronized (Andrew Kroll) --- hardware/arduino/avr/libraries/SPI/SPI.cpp | 56 ++++++++++++++-------- hardware/arduino/avr/libraries/SPI/SPI.h | 3 +- 2 files changed, 38 insertions(+), 21 deletions(-) diff --git a/hardware/arduino/avr/libraries/SPI/SPI.cpp b/hardware/arduino/avr/libraries/SPI/SPI.cpp index 5381033f571..2978f77cd73 100644 --- a/hardware/arduino/avr/libraries/SPI/SPI.cpp +++ b/hardware/arduino/avr/libraries/SPI/SPI.cpp @@ -2,6 +2,7 @@ * Copyright (c) 2010 by Cristian Maglie * Copyright (c) 2014 by Paul Stoffregen (Transaction API) * Copyright (c) 2014 by Matthijs Kooijman (SPISettings AVR) + * Copyright (c) 2014 by Andrew J. Kroll (atomicity fixes) * SPI Master library for arduino. * * This file is free software; you can redistribute it and/or modify @@ -20,32 +21,47 @@ uint8_t SPIClass::interruptSave = 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 (!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 diff --git a/hardware/arduino/avr/libraries/SPI/SPI.h b/hardware/arduino/avr/libraries/SPI/SPI.h index 22a3cc438aa..7a7d6131ed0 100644 --- a/hardware/arduino/avr/libraries/SPI/SPI.h +++ b/hardware/arduino/avr/libraries/SPI/SPI.h @@ -2,6 +2,7 @@ * Copyright (c) 2010 by Cristian Maglie * Copyright (c) 2014 by Paul Stoffregen (Transaction API) * Copyright (c) 2014 by Matthijs Kooijman (SPISettings AVR) + * Copyright (c) 2014 by Andrew J. Kroll (atomicity fixes) * SPI Master library for arduino. * * This file is free software; you can redistribute it and/or modify @@ -52,7 +53,7 @@ // Flags for the state of SPI, used as needed. // Normally inTransaction is not used. typedef struct SPIflags { - bool padding : 1; + 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; From 6f715822a041fe5b9b4b001b5b39b4a9e95626f2 Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Tue, 21 Oct 2014 13:03:32 +0200 Subject: [PATCH 5/7] [avr] Fix to atomic-access on SPI Transactions (Andrew Kroll) --- hardware/arduino/avr/libraries/SPI/SPI.cpp | 16 +++--- hardware/arduino/avr/libraries/SPI/SPI.h | 59 +++++++++++++++------- 2 files changed, 48 insertions(+), 27 deletions(-) diff --git a/hardware/arduino/avr/libraries/SPI/SPI.cpp b/hardware/arduino/avr/libraries/SPI/SPI.cpp index 2978f77cd73..7710b6e50a4 100644 --- a/hardware/arduino/avr/libraries/SPI/SPI.cpp +++ b/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; } diff --git a/hardware/arduino/avr/libraries/SPI/SPI.h b/hardware/arduino/avr/libraries/SPI/SPI.h index 7a7d6131ed0..cca8c0459f8 100644 --- a/hardware/arduino/avr/libraries/SPI/SPI.h +++ b/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,18 +174,8 @@ 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); @@ -186,6 +183,24 @@ class SPIClass { } 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,6 +253,8 @@ 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); @@ -245,16 +262,22 @@ class SPIClass { } 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 From 872da139f6ddeee7db61a0209724a6e1b63f7f9b Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Tue, 21 Oct 2014 13:12:25 +0200 Subject: [PATCH 6/7] [avr] Added SPI.notUsingInterrupt() (Andrew Kroll) --- hardware/arduino/avr/libraries/SPI/SPI.cpp | 41 ++++++++++++++++++++++ hardware/arduino/avr/libraries/SPI/SPI.h | 5 +++ 2 files changed, 46 insertions(+) diff --git a/hardware/arduino/avr/libraries/SPI/SPI.cpp b/hardware/arduino/avr/libraries/SPI/SPI.cpp index 7710b6e50a4..da61aaab23e 100644 --- a/hardware/arduino/avr/libraries/SPI/SPI.cpp +++ b/hardware/arduino/avr/libraries/SPI/SPI.cpp @@ -141,3 +141,44 @@ void SPIClass::usingInterrupt(uint8_t interruptNumber) 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; +} diff --git a/hardware/arduino/avr/libraries/SPI/SPI.h b/hardware/arduino/avr/libraries/SPI/SPI.h index cca8c0459f8..2e7eda6b900 100644 --- a/hardware/arduino/avr/libraries/SPI/SPI.h +++ b/hardware/arduino/avr/libraries/SPI/SPI.h @@ -20,6 +20,9 @@ // 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. @@ -169,6 +172,8 @@ 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); // Before using SPI.transfer() or asserting chip select pins, // this function is used to gain exclusive access to the SPI bus From 6df8847c5917d3969406f3a5e6714499e4109cc8 Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Tue, 21 Oct 2014 18:04:31 +0200 Subject: [PATCH 7/7] [avr] SPI: removed redundant include --- hardware/arduino/avr/libraries/SPI/SPI.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/hardware/arduino/avr/libraries/SPI/SPI.cpp b/hardware/arduino/avr/libraries/SPI/SPI.cpp index da61aaab23e..4466f7ff154 100644 --- a/hardware/arduino/avr/libraries/SPI/SPI.cpp +++ b/hardware/arduino/avr/libraries/SPI/SPI.cpp @@ -12,7 +12,6 @@ */ #include "SPI.h" -#include "pins_arduino.h" SPIClass SPI; SPIflags_t SPIClass::modeFlags = {false, false, 0};