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

[cmaglie]

This pull request broke into 6 simpler commits the big patch in #2376.

/cc @xxxajk @PaulStoffregen

[xxxajk]

By the way, just a little note here, not sure if you are aware of it or not, but NOT_AN_INTERRUPT -- isn't defined ;-)

[cmaglie]

The most complex part of this patch is indeed this one:
cmaglie@10b3c3c

@xxxajk may I ask you if you can provide a test case that shows the problem solved by this patch?

For the rest everything looks ok to me, but I'll wait for some other comments too.

[cmaglie]

NOT_AN_INTERRUPT -- isn't defined ;-)

Already reported here #2379, and already working on that!

[xxxajk]

'k, just making sure it isn't missed again, thanks, I'm reviewing now

[xxxajk]

Any reason this isn't in SPI.h?

Also Should be <pins_arduino.h> in order to speed up compiles.

Explanation:
Directories named by -I are searched before the standard system include directories. If the directory dir is a standard system include directory, the option is ignored to ensure that the default search order for system directories and the special treatment of system headers are not defeated.

Headers whose names are enclosed in double-quotes ( "" ) shall be searched for first in the directory of the file with the #include line, then in directories named in -I options, and last in the standard system include directories.
For headers whose names are enclosed in angle brackets ( "<>" ), the header shall be searched for only in directories named in -I options and then in the standard system include directories.

[cmaglie]

IMHO in SPI.cpp the include is not needed at all because:

• SPI.cpp includes "SPI.h"
• SPI.h includes <Arduino.h>
• Arduino.h includes "pins_arduino.h"

BTW I agree with you that this line:
https://github.com/arduino/Arduino/blob/master/hardware/arduino/cores/arduino/Arduino.h#L237
should be:

#include <pins_arduino.h>

[xxxajk]

If you look throughout the code, you will find lots of this mistake.

[xxxajk]

Looks good, see line notes for small final improvements.

[xxxajk]

Yes, probably after 24hrs from now I can write a smaller test than what I
used, which contains yet to be released code... Should be quite easy to do,
will require two arduinos to do the test.

[ArduinoBot]

Build successful. Please test this code using one of the following:
http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-2381-BUILD-72-linux32.tgz
http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-2381-BUILD-72-linux64.tgz
http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-2381-BUILD-72-windows.zip
http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-2381-BUILD-72-macosx.zip

[xxxajk]

Linux-32bit build is broken
libastylej.so: wrong ELF class: ELFCLASS64 (Possible cause: architecture word width mismatch)
I'll copy files from my other install in order to fix this :-)

[xxxajk]

AVR_SPI_SLAVE_EMULATOR

/* 
   Emulate an SPI device that has IRQ servicing.

   Approximately every 1 ms, interrupt the master for service.
   The interrupt only clears after 5 0x11 seen in a row.
   Any other value restarts the count.

   Other data also comes in as well,

   Watch the I_REQUEST_PIN with an LED or 'scope.
   If it gets stuck LOW then there is a bug in SPI.

   Contact: <[email protected]>
*/

#include <Arduino.h>
#include <SPI.h>

#define I_REQUEST_PIN 2

volatile int32_t nextirq;
volatile uint8_t saw;

static int my_putc(char c, FILE *t) {
        Serial.write(c);
        return 0;
}

void setup() {
        SPI.begin();
        SPI.setDataMode(SPI_MODE0);
        digitalWrite(SS, HIGH);
        digitalWrite(13, HIGH);
        pinMode(SS, INPUT);
        pinMode(13, INPUT);
        pinMode(MISO, OUTPUT);
        pinMode(MOSI, INPUT);
        digitalWrite(I_REQUEST_PIN, HIGH);
        pinMode(I_REQUEST_PIN, OUTPUT);
        SPCR |= _BV(SPE);
        saw = 0;
        nextirq = millis();
        SPI.attachInterrupt();
}

void loop() {
        if(millis() != nextirq) {
                nextirq = millis();
                noInterrupts();
                digitalWrite(I_REQUEST_PIN, LOW);
                interrupts();
        }
}

ISR (SPI_STC_vect) {
        byte c = SPDR;
        if(c == 0x11) {
                saw++;
        } else {
                saw = 0;
        }
        if(digitalRead(I_REQUEST_PIN)) {
                saw = 0;
        } else {
                if(saw == 5) {
                        saw = 0;
                        digitalWrite(I_REQUEST_PIN, HIGH);
                }
        }
}

AVR_SPI_MASTER_FEEDER

/* 
   Talk to a pretend SPI device that has IRQ servicing.

   Feeds an SPI slave with 0xee data from loop as pretend interaction.
   Every so often an interrupt occurs to service the slave.
   The interrupt only clears after 5 0x11 seen in a row.

   Watch the I_REQUEST_PIN with an LED or 'scope.
   If it gets stuck LOW then there is a bug in SPI.

   Contact: <[email protected]>
*/

#include <Arduino.h>
#include <SPI.h>


#define I_REQUEST_PIN 2

SPISettings SPI_Settings;

void ISR_Call(void) {
        digitalWrite(SS, LOW);
        SPI.beginTransaction(SPI_Settings);
        SPI.transfer(0x11);
        SPI.transfer(0x11);
        SPI.transfer(0x11);
        SPI.transfer(0x11);
        SPI.transfer(0x11);
        SPI.endTransaction();
        digitalWrite(SS, HIGH);
}

void setup() {
        SPI.begin();
        digitalWrite(I_REQUEST_PIN, HIGH); // Enables Pullup
        digitalWrite(SS, HIGH);
        pinMode(SS, OUTPUT);
        pinMode(I_REQUEST_PIN, INPUT);
        SPI_Settings = SPISettings(1000000, MSBFIRST, SPI_MODE0);        
        SPI.usingInterrupt(digitalPinToInterrupt(I_REQUEST_PIN));
        attachInterrupt(digitalPinToInterrupt(I_REQUEST_PIN), ISR_Call, LOW);
}

void loop() {
        digitalWrite(SS, LOW);        
        SPI.beginTransaction(SPI_Settings);
        SPI.transfer(0xee);
        SPI.transfer(0xee);
        SPI.transfer(0xee);
        SPI.transfer(0xee);
        SPI.transfer(0xee);
        SPI.transfer(0xee);
        SPI.transfer(0xee);
        SPI.transfer(0xee);
        SPI.transfer(0xee);
        SPI.endTransaction();
        digitalWrite(SS, HIGH);
        delay(2);
}

[xxxajk]

Power from only 1 of the UNO boards.

Connections for above:

MASTER SLAVE
 +5     +5
 GND    GND
 2      2
 10     10
 11     11
 13     13

[cmaglie]

@xxxajk
thanks for the clear test case, it indeed shows that the problem exists and that your fix is effective.

I've slightly modified the master test example (so the transaction contains also the SS pin toggle, but the issue is triggered anyway):

/* 
   Talk to a pretend SPI device that has IRQ servicing.

   Feeds an SPI slave with 0xee data from loop as pretend interaction.
   Every so often an interrupt occurs to service the slave.
   The interrupt only clears after 5 0x11 seen in a row.

   Watch the I_REQUEST_PIN with an LED or 'scope.
   If it gets stuck LOW then there is a bug in SPI.

   Contact: <[email protected]>
*/

#include <Arduino.h>
#include <SPI.h>


#define I_REQUEST_PIN 2

SPISettings SPI_Settings;

void ISR_Call(void) {
        SPI.beginTransaction(SPI_Settings);
        digitalWrite(SS, LOW);
        SPI.transfer(0x11);
        SPI.transfer(0x11);
        SPI.transfer(0x11);
        SPI.transfer(0x11);
        SPI.transfer(0x11);
        digitalWrite(SS, HIGH);
        SPI.endTransaction();
}

void setup() {
        SPI.begin();
        digitalWrite(I_REQUEST_PIN, HIGH); // Enables Pullup
        digitalWrite(SS, HIGH);
        pinMode(SS, OUTPUT);
        pinMode(I_REQUEST_PIN, INPUT);
        SPI_Settings = SPISettings(1000000, MSBFIRST, SPI_MODE0);        
        SPI.usingInterrupt(digitalPinToInterrupt(I_REQUEST_PIN));
        attachInterrupt(digitalPinToInterrupt(I_REQUEST_PIN), ISR_Call, LOW);
}

void loop() {
        SPI.beginTransaction(SPI_Settings);
        digitalWrite(SS, LOW);        
        SPI.transfer(0xee);
        SPI.transfer(0xee);
        SPI.transfer(0xee);
        SPI.transfer(0xee);
        SPI.transfer(0xee);
        SPI.transfer(0xee);
        SPI.transfer(0xee);
        SPI.transfer(0xee);
        SPI.transfer(0xee);
        digitalWrite(SS, HIGH);
        SPI.endTransaction();
        delay(2);
}

The following trace shows the interrupt serviced in a normal way. It is serviced twice because the slave is slow to put the IRQ line back HIGH when the master complete the first ISR, but this doesn't influence the test.

The following trace shows how the transaction comes into play when the master is doing his periodic transfer and the slave sends an interrupt signal in the middle of it. In this case the transaction does its job successfully and the interrupt is serviced correctly when the first transaction complete.

And this one is the trace that shows the problem: in this case the interrupt happens at the beginning of the master's periodic transfer (probably in the middle of the beginTransaction() method). We can see that the interrupt is not serviced any more.

In conclusion, even if I'm having hard times to fully understand what's happening and why the ISR gets stuck, I'm for merging this pull request.

Any other review? Paul?

[xxxajk]

I made the ISR that way to fully stop, so that it was easier to verify the bug, however, yes, it will fail because the slave never sees the full sequence. It gets a corrupted one. That is what is involved when doing things on a parallel level, and ISRs are a form of parallel programming. I have over 20 years experience doing things like this in mixed C/ASM, and at least 30 in pure ASM on 6502. Sorting out these sorts of things come natural to me.

An example, to help you understand what is going on, and learn from me:
The same techniques can be done with malloc/free etc. I already provide an ISR safe one so that an ISR can allocate memory on the heap without causing a corrupted heap. Corruption can occur when you are in one of the heap modification routines, and an interrupt occurs that wants to use it too. The fix is to simply make the functions atomic.The nice thing is that after you do it that way, many valuable libc calls open up for use that depend upon it. According to standards, malloc its self is not considered ISR safe-- HOWEVER it must be thread safe! Personally I consider an ISR on a microcontroller a thread, and technically it can be considered a thread of execution in another context.

The reason I am going into depth like this is that being a lead dev, you really need to be aware of these sorts of issues. Please, feel free to look at my github code in your spare time, so that you can see how to deal with things like this. In particular, this repo: https://github.com/xxxajk/xmem2 You'll find all sorts of very advanced ISR handling, on both AVR and ARM. You are welcome to integrate the heap stuff into Arduino as well. Also, if you need any help in the future, don't hesitate to ask for clues.

[xxxajk]

So, I did misunderstand the question, partially :-P

SPI.beginTransaction() needs to be ISR safe, I use it all the time to service devices on SPI when an IRQ happens. I don't need to take a hit in the middle of user land and get corruption.

SPI.usingInterrupt()... that's a complicated question. In my threaded xmem2 stuff, I do sometimes start SPI from different contexts. So it may be a good idea to protect it from threads and schedulers. I'm certain the various RTOS people would like this too.

Now for an answer that is direct, I don't currently use usingInterrupt() in an ISR in a pure IRQ context. I could think of a situation where you would want to do this though. For a simple example, you push a button that locks into an 'on' or 'off' position, and attach/detach accordingly. Same effect could be part of a shut-down IRQ, where you want to detach everything you attached, then when woken up by an IRQ, you attach everything back.

[matthijskooijman]

For a simple example, you push a button that locks into an 'on' or 'off' position, and attach/detach accordingly. Same effect could be part of a shut-down IRQ, where you want to detach everything you attached, then when woken up by an IRQ, you attach everything back.

I can see that. However, it wouldn't be terrible in that case if the interrupt would remain registered with the SPI library even though it's actually detached, right? It'll mean that SPI transactions in that case are slightly slower because they disable the interrupt while it's not really needed, but that seems a better trade-off than making all SPI transactions slower and disabling interrupts even when they don't really need it (which is a lot more common).

Hence the proposal to disallow usingInterrupt from ISR context, to simplify logic and prevent needing to disable interrupts when interruptMode is 0. @xxxajk, agreed?

[xxxajk]

When 0... sounds okay.

[matthijskooijman]

Great, so we agree on that :-)

Other important decision would be whether to include a notUsingInterrupt function? Naïvely I'd say this is not needed - you know what interrupts might do SPI transfers at the start of the program so you register those and be done with it. Allowing to unregister interrupts could be useful from a performance perspective, but it is not needed for atomicity.

Furthermore, the proposed implementation only works properly for unregistering pin interrupts - when other interrupt types are registered (e.g. interruptMode is 2) the SPI library does not currently know how many of these there are. The current implementation assumes 1, breaking atomicity if there were 2 and 1 is unregistered. This can be fixed by leaving interruptMode at 2 (even when all interrupts are unregistered), but this means that the biggest performance win of notUsingInterrupt (namely preventing globally disabling interrupts during every SPI transaction) is negated. The alternative would be to do "reference counting" by counting the number of non-pin interrups disabled (or increasing interruptMode beyond 2, perhaps). This would require some additional bits of memory, but I don't suppose this would be a problem :-)

I'm inclined to not offer a usingInterrupt function right now. If we do think we need it, I'd offer the reference counting function so it is at least correct and useful. @PaulStoffregen, what do you think?

[xxxajk]

So long as the first demo has no issues, I'm good with it.

[matthijskooijman]

@xxxajk, with "the first demo", you mean the test sketches posted near the start of this PR that illustrate the atomicity problem?

[xxxajk]

Correct,

[xxxajk]

Idea... extra set of methods that don't care so speed up for simple sketches

[cmaglie]

I've the first example of @xxxajk already set up and I can quickly do the test again if needed.

[xxxajk]

Consider it something to test against.

[PaulStoffregen]

I have a usage case on Teensy that needs SPI.notUsingInterrupt(). Of course, it's not a big problem for me if Arduino's SPI library doesn't implement this, but maybe explaining this usage case would help.

My audio library can play files from the SD card. Actual reading of data from the card, via the SD library, is done from a low-priority interrupt. When the user begins playing a file, the library registers that interrupt with SPI.usingInterrupt(). When the file finishes playing, the interrupt is unregistered with SPI.notUsingInterrupt().

Some users wish to also access the card, when not playing a file, but perhaps also generating sounds with synthesis features. If the library can't unregister its interrupt, then while the user does SD card access, the audio library interrupt is needlessly masked during potentially long SPI operations. Disabling the audio processing interrupt for too long can cause any generated output sounds to glitch. If the library is processing input audio (like a sound reactive project that occasionally plays sounds itself), disabling the interrupt for too long can cause lost input data, which can look like all sorts of wrong results to FFT and other types of real-time analysis.

I would agree with the idea that once interruptMode == 2, it can not be lowered back to 1 or 0. In fact, lowering from 1 back to 0 isn't needed either. But when it's 1, the individual bit in interruptMask should be able to be cleared.

Also, from a documentation point of view, I believe it would be wise to only promote SPI.usingInterrupt() to be called with pin interrupt numbers. My intention behind interruptMode 2 was forward compatibility for some future where Arduino defines more interrupt numbers for things like timers. When/if such numbers are ever defined and published, so attachInterrupt(someTimerNum, function, TimerEventType) allows Arduino-style access to timers, those future numbers could be backwards compatible, because interruptMode 2 is a catch-all for unknown stuff.

[PaulStoffregen]

Probably the reason notUsingInterrupt() is in this pull request is because Andrew used my copy (for Teensy) of the SPI library as the starting point for this work, then submitted it here. It's great to consider all these extra little things, but it does make focusing only on the atomic access issue hardware. Usually when I contribute changes to Arduino, I open up Arduino's version of the code and selectively copy just one feature and create one pull request, which keeps the decision making much simpler and easier.

[matthijskooijman]

Thanks for the usecase, it seems notUsingInterrupt would be good to have after all. You're right that it's not needed for this atomicity fix per se, as well.

[matthijskooijman]

Also, from a documentation point of view, I believe it would be wise to only promote SPI.usingInterrupt() to be called with pin interrupt numbers.

Right, you mean that it now suggests that you can pass -1 for "other interrupts", but that we should not recommend this and instead have the code only for when the interrupt APIs get extended in the future. Makes sense to me.

[xxxajk]

Again, yes your code is indeed the primary start. I thought it may be handy
to add mode 2 as well, so that I can use it in a timed task, although
currently I do not.

[cmaglie]

To recap we have:

• add a check in beginTransaction to skip transaction stuff entirely when interruptMode is 0
• once interruptMode becomes 2 it should never go back to 0 with notUsingInterrupt

Is anyone going to try an implementation of what we said here?
In case, I count to work on that between today and tomorrow.

[matthijskooijman]

Yes, that's pretty much it. However, that check for interruptMode being 0 is already present, but the code should be restructured so that it doesn't disable interrupts when interruptMode is 0.

[matthijskooijman]

Oh, and I won't have time for this soon.

[PaulStoffregen]

Yes, pretty much this. Also, I wanted to look at the runtime cost of using the bitfield.

Sadly, I also don't have time to work on this at the moment. Today I'm chasing a bug in Adafruit's nRF80001 bluetooth library. Later today I have an urgent hardware project... just the moment PCBs arrive from OSH Park.

Here's my copy of SPI, with the alternate approach (assuming interruptMode isn't volatile) we've been discussion. It lacks notUsingInterrupt() on the AVR side.

https://github.com/PaulStoffregen/SPI/blob/master/SPI.h#L170

If I had more hours in the day, I'd merge this into a 1.5.x clone and submit a new pull request as 4 or 5 nice, clean commits. But hopefully this code at least helps?

[cmaglie]

That helps a lot, thanks Paul!

I've just checked the impact of using bitfields, basically before and after this commit: cmaglie@841e75d

The test sketch is very simple:

#include <SPI.h>
void testTransactionSpeed() {
  char buff[10] = {1,2,3,4,5,6,7,9,10};
  unsigned long start = micros();
  for (uint16_t i = 0; i < 25000; i++) {
    SPI.beginTransaction(SPISettings(100000000, MSBFIRST, SPI_MODE0));
    SPI.transfer(buff, 4);
    SPI.endTransaction();
  }
  unsigned long stop = micros();
  Serial.print("25000x transactions of 4 bytes completed in (us) : ");
  Serial.println(stop - start);
  Serial.print("Throughput (mbit/s) : ");
  Serial.print(800000.0/(stop - start));
  Serial.println(" out of 8.00");
}
void setup() {
  Serial.begin(9600);
  SPI.begin();
  testTransactionSpeed();
}
void loop() {
}

The result are:

What	FLASH used	RAM used	Time to complete test	Speed
No BITFIELD	4730	289	176064	4.54 Mbit/s
w/ BITFIELD	4744	287	183908	4.35 Mbit/s

The only very small gain is with RAM usage, in front of a runtime penalty of 4%.
Unless I'm missing something really important, I'm going to remove the bit-field patch too.

[PaulStoffregen]

Wow, I had a feeling the bitfield would impact performance, but I didn't know it would be 4%.

[cmaglie]

Please follow up on #2449

[nickgammon]

Would you please document these functions on the reference page: http://arduino.cc/en/Reference/SPI

• beginTransaction
• usingInterrupt
• notUsingInterrupt
• transfer16
• endTransaction

I have code that is now failing under IDE 1.6.0 due to SPI behaving differently to previous versions. I had to find a reference to this page in the source, but no official documentation on the web site.

Specifically, SPI.transfer (0) is hanging indefinitely. I am presuming that an interrupt is (unexpectedly) being generated due to legacy code (ie. code that used to work in all previous versions of the IDE) not setting up beginTransaction / endTransaction or an interrupt handler.

[nickgammon]

It turns out that I was inadvertently calling SPI.begin() twice and SPI.end() once. This meant that after calling SPI.end() and then calling SPI.begin() again the second SPI.begin() was effectively a NO-OP because of the reference counter. It might be worth pointing out in the documentation that SPI.begin() and SPI.end() must be balanced in the number of calls, or you get unexpected behaviour.

[nickgammon]

See: #2654