From 53e25d8b5534c842a89eb62da1ab2558463c077b Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Mon, 20 Oct 2014 19:46:08 +0200 Subject: [PATCH 1/5] [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 | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/hardware/arduino/avr/libraries/SPI/SPI.h b/hardware/arduino/avr/libraries/SPI/SPI.h index b54e2dfd538..24ebc125ea5 100644 --- a/hardware/arduino/avr/libraries/SPI/SPI.h +++ b/hardware/arduino/avr/libraries/SPI/SPI.h @@ -184,6 +184,12 @@ class SPIClass { // 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; @@ -193,16 +199,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; } From 8344812ce89dad2dcdbea4d2aca2f70fae5730c5 Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Tue, 21 Oct 2014 12:57:41 +0200 Subject: [PATCH 2/5] [avr] Made SPI.begin() and SPI.end() synchronized (Andrew Kroll) --- hardware/arduino/avr/libraries/SPI/SPI.cpp | 61 +++++++++++++++------- hardware/arduino/avr/libraries/SPI/SPI.h | 2 + 2 files changed, 43 insertions(+), 20 deletions(-) diff --git a/hardware/arduino/avr/libraries/SPI/SPI.cpp b/hardware/arduino/avr/libraries/SPI/SPI.cpp index ef13e2ae992..4bd72dfdb44 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 @@ -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; @@ -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 diff --git a/hardware/arduino/avr/libraries/SPI/SPI.h b/hardware/arduino/avr/libraries/SPI/SPI.h index 24ebc125ea5..c8d4ce3b6a3 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 @@ -281,6 +282,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 From d9537cb7daba28688efb1d1a60d727cfeef69981 Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Tue, 21 Oct 2014 13:12:25 +0200 Subject: [PATCH 3/5] [avr] Added SPI.notUsingInterrupt() (Andrew Kroll) --- hardware/arduino/avr/libraries/SPI/SPI.cpp | 42 ++++++++++++++++++++++ hardware/arduino/avr/libraries/SPI/SPI.h | 10 ++++++ 2 files changed, 52 insertions(+) diff --git a/hardware/arduino/avr/libraries/SPI/SPI.cpp b/hardware/arduino/avr/libraries/SPI/SPI.cpp index 4bd72dfdb44..40d4278b717 100644 --- a/hardware/arduino/avr/libraries/SPI/SPI.cpp +++ b/hardware/arduino/avr/libraries/SPI/SPI.cpp @@ -151,3 +151,45 @@ void SPIClass::usingInterrupt(uint8_t interruptNumber) interrupts(); } +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; +} diff --git a/hardware/arduino/avr/libraries/SPI/SPI.h b/hardware/arduino/avr/libraries/SPI/SPI.h index c8d4ce3b6a3..2f9b56541a5 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 notUsingInterrupt() method +#define SPI_HAS_NOTUSINGINTERRUPT 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 @@ -154,6 +157,13 @@ 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 From 84b6cc27a5b49b57c12accff40a004b2655b557d Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Tue, 18 Nov 2014 20:02:27 +0100 Subject: [PATCH 4/5] [avr] Made SPI.usingInterrupt() synchronized (Andrew Kroll) --- hardware/arduino/avr/libraries/SPI/SPI.cpp | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/hardware/arduino/avr/libraries/SPI/SPI.cpp b/hardware/arduino/avr/libraries/SPI/SPI.cpp index 40d4278b717..077b6a38db8 100644 --- a/hardware/arduino/avr/libraries/SPI/SPI.cpp +++ b/hardware/arduino/avr/libraries/SPI/SPI.cpp @@ -111,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; @@ -143,12 +141,12 @@ 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) From a9735bf91fda36eb9cd97274f01b86f1fb234155 Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Tue, 18 Nov 2014 20:03:23 +0100 Subject: [PATCH 5/5] 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. --- hardware/arduino/avr/libraries/SPI/SPI.h | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/hardware/arduino/avr/libraries/SPI/SPI.h b/hardware/arduino/avr/libraries/SPI/SPI.h index 2f9b56541a5..cee618c80db 100644 --- a/hardware/arduino/avr/libraries/SPI/SPI.h +++ b/hardware/arduino/avr/libraries/SPI/SPI.h @@ -23,6 +23,13 @@ // 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 @@ -170,17 +177,21 @@ class SPIClass { // 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); @@ -188,6 +199,7 @@ class SPIClass { } inTransactionFlag = 1; #endif + SPCR = settings.spcr; SPSR = settings.spsr; } @@ -253,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 {