user82812: I want two Arduino Nanos to communicate with each other using SPI, ideally at around 2MHz or faster.
I have two standard Nanos running at 16MHz on 5V.

The master uses SPI.transfer() to send an array across the wire.¹ For ease of troubleshooting, the array alternately contains a bunch of zeroes or a bunch of 255s; but never both.

The slave simply returns decimal 66 for every byte. This was chosen because it's easy to spot on a logic trace.

The master checks the responses, and if there are any values that aren't 66, it prints the array to the UART.


Even at 125Kbit, which is the slowest available speed, I'm regularly getting corrupted bytes. Interestingly, most of the time (but not always!) the corrupted byte is either 0 or 255 - in other words, exactly the same as the byte sent from the master! This strikes me as a very unlikely coincidence.
I have inspected the traffic with a logic analyser, and it seems that the slave just occasionally decides not to operate as instructed; and instead it just sends the input straight out again. As in, the byte going out on MISO is exactly the same as the byte coming in on MOSI. But only sometimes.
What is happening here? Why doesn't it work? And what can I do about it?

Master sketch:
#define MOSI    12
#define SCK     13

#include <SPI.h>
#include "pins_arduino.h"
#include <avr/wdt.h>

void setup(void) {
    Serial.begin(9600);

    digitalWrite(SS, HIGH);
    pinMode(SS, OUTPUT);
    digitalWrite(SCK, HIGH);
    pinMode(SCK, OUTPUT);
    pinMode(MOSI, OUTPUT);

    SPI.setClockDivider(SPI_CLOCK_DIV128);      // 125KHz
    SPI.begin();

    Serial.println("\n\nSTARTING...\n");
}

void do_transfer(byte attempt, byte payload) {

    // packet size of 200
    byte buffer[200];

    // initialise packet
    for (byte count = 0; count < sizeof(buffer); count++)
        buffer[count] = payload;

    // perform transfer
    digitalWrite(SS, LOW);
    SPI.transfer(buffer, sizeof(buffer));
    digitalWrite(SS, HIGH);    // SS is pin 10

    // tally results
    byte successes = 0, failures = 0;
    for (byte index = 0; index < sizeof(buffer); index++)
        if (buffer[index] == 66) successes++; else failures++;

    // print results
    Serial.print("Attempt ");
    Serial.print(attempt);
    Serial.print(" with payload ");
    Serial.print(payload);
    Serial.print(": ");
    if (failures == 0)
        Serial.print("SUCCESS");
    else {
        Serial.print(successes);
        Serial.print(" correct bytes, ");
        Serial.print(failures);
        Serial.print(" incorrect bytes");
        for (byte index = 0; index < sizeof(buffer); index++) {
            if (index % 10 == 0)
                Serial.print("\n\t");
            if (buffer[index] == 66)
                Serial.print("-");
            else
                Serial.print(buffer[index]);
            if (index < sizeof(buffer) - 1)
                Serial.print("\t");
        }
    }
    Serial.println();
    Serial.println();
}

void loop(void) {
    delay(800);                                                         // give the slave time to be ready

    for (byte attempt = 1; attempt <= 10; attempt+=2) {
        do_transfer(attempt, 0x00);
        do_transfer(attempt+1, 0xFF);
        wdt_reset();
    }
    while(true);                                                        // end of program
}

Slave sketch:
#include "pins_arduino.h"

void setup(void) {
    pinMode(MISO, OUTPUT);          // MISO needs to be an output
    SPCR |= _BV(SPE);               // enable SPI in slave mode
    SPCR |= _BV(SPIE);              // enable interrupts
    SPDR = 66;
}

ISR (SPI_STC_vect) { SPDR = 66; }   // just send 66 until the cows come home

void loop (void) { }

Connections: The following pins on both Arduinos are connected together:

SS (pin 10)
MISO (pin 11)
MOSI (pin 12)
SCK (pin 13)
GND


¹ The problem also happens if I send one byte at a time using SPI.transfer(), unless (maybe) if I insert a 1ms delay between each byte. Which of course is ridiculous.

user10794: Your problem here is timing. I tried your setup, and found that I did indeed get occasional errors at 125 kHz. Disabling Timer 0 (as suggested by Chrisl) fixed that:
  power_timer0_disable();


But while it now works up to 250KHz, I get many many errors at 500KHz.

OK, the time between clock pulses is now 2 µs, however as I document here the time taken to execute an ISR like SPI_STC_vect is 2.625 µs, therefore it can't keep up with having to put the data there every 2 s.
You actually have slightly more time than 2 µs because the critical time is the time between the end of the first byte (which is when the ISR is called) and the time before the start of the second byte, which is when the master will start sampling your response.
See screenshot below with the critical time boxed in yellow.

That time is 2.45 µs and the ISR takes 2.625 µs. Now some of that is the ISR epilogue which won't really matter that much, but you can see that things are tight. It's hard to know exactly at what point the processor starts calling the ISR after the last bit arrives, and the length of the instruction it is currently executing could be an influencing factor.
I got your sketches to work running at 250 kHz with these amendments to the slave:
#include "pins_arduino.h"
#include <avr/power.h>
#include <avr/sleep.h>

void setup(void) {
  pinMode(MISO, OUTPUT);          // MISO needs to be an output
  SPCR |= _BV(SPE);               // enable SPI in slave mode
  SPCR |= _BV(SPIE);              // enable interrupts
  SPDR = 66;
  power_timer0_disable(); 
  set_sleep_mode (SLEEP_MODE_IDLE);
}

ISR (SPI_STC_vect) { SPDR = 66; }   // just send 66 until the cows come home

void loop (void) 
 {
 sleep_mode ();
 }

The sleeping in idle mode makes sure that the processor is not half-way through an instruction when the interrupt is raised.
As expected (for the reasons above) it did not work at 500 kHz.
If you want to transfer data reliably you need to allow, in the master, for the time it takes the slave to do stuff. Every line of code in the slave means that the master has to wait for that to be executed.
I mention that in my page about SPI where I build in a delay in the master, if I want the slave to do calculations or something.


But the datasheet says that speeds of Fosc/4 are possible in slave mode, which would be 4MHz in this case. Would that be achievable if I removed the ISR and just used a busy loop?

Well, yes the datasheet has an example of doing a busy loop:
char SPI_SlaveReceive(void)
{
/* Wait for reception complete */
while(!(SPSR & (1<<SPIF)))
;
/* Return Data Register */
return SPDR;
}

Regardless of what they claim, what you intend has to be physically possible. So with Fosc/4, assuming a 16 MHz clock, you have 2 µs for a byte to arrive (250 ns * 8). So the hardware may well be able to receive one byte in that time, and you may just have enough clock cycles to notice it arriving and plonk it into memory somewhere (although things could be tight) but you won't have time to send a different response after each incoming byte, because you only have around 250 ns to do it in, and the loop that detects the interrupt will probably use up most or all of that.
After a bit of mucking around, I got it to work at 250 kHz where the objective was just for the slave to return some data (after all, I presume that is what the slave is supposed to do).
Master
#define MOSI    12
#define SCK     13

#include <SPI.h>
#include "pins_arduino.h"
#include <avr/wdt.h>

void setup(void) {
    Serial.begin(9600);

    digitalWrite(SS, HIGH);
    pinMode(SS, OUTPUT);
    digitalWrite(SS, HIGH);

    digitalWrite(SCK, HIGH);
    pinMode(SCK, OUTPUT);
    pinMode(MOSI, OUTPUT);

    SPI.begin();
    SPI.setClockDivider(SPI_CLOCK_DIV64);     

    Serial.println("\n\nSTARTING...\n");
    
}

void do_transfer(byte attempt, byte payload) {

    // packet size of 200
    byte buffer[200];

    // perform transfer
    digitalWrite(SS, LOW);
    delay (1);    // give slave time to react
    SPI.transfer(buffer, sizeof(buffer));
    digitalWrite(SS, HIGH);    // SS is pin 10

    // tally results
    byte successes = 0, failures = 0;
    for (byte index = 0; index < sizeof(buffer); index++)
        if (buffer[index] == index) successes++; else failures++;

    // print results
    Serial.print("Attempt ");
    Serial.print(attempt);
    Serial.print(" with payload ");
    Serial.print(payload);
    Serial.print(": ");
    if (failures == 0)
        Serial.print("SUCCESS");
    else {
        Serial.print(successes);
        Serial.print(" correct bytes, ");
        Serial.print(failures);
        Serial.print(" incorrect bytes");
        for (byte index = 0; index < sizeof(buffer); index++) {
            if (index % 10 == 0)
                Serial.print("\n\t");
            if (buffer[index] == index)
                Serial.print("-");
            else
                Serial.print(buffer[index]);
            if (index < sizeof(buffer) - 1)
                Serial.print("\t");
        }
    }
    Serial.println();
    Serial.println();
}

void loop(void) {
    delay(800);                                                         // give the slave time to be ready

    for (byte attempt = 1; attempt <= 10; attempt+=2) {
        do_transfer(attempt, 0x00);
        do_transfer(attempt+1, 0xFF);
        wdt_reset();
    }
    while(true);                                                        // end of program
}

Slave
#include "pins_arduino.h"
#include <avr/power.h>
#include <avr/sleep.h>

const int TEST_SIZE = 200;

volatile byte my_data [TEST_SIZE];

volatile byte dummy;

ISR (PCINT0_vect)
 {
 // handle pin change interrupt for D8 to D13 here

  // ignore transition to HIGH
  if (digitalRead (SS) == HIGH)
    return;
    
  SPDR = 0;
  
  for (byte i = 0; i < TEST_SIZE; i++)
    {
    SPDR = my_data [i];
    while(!(SPSR & (1<<SPIF)))
      {
      // if SS went high, give up
      if (PINB & (1 << 2))
        return;
      }
    } // end of for loop

    
 }  // end of PCINT0_vect

void setup(void) {
  pinMode(MISO, OUTPUT);          // MISO needs to be an output
  SPCR |= _BV(SPE);               // enable SPI in slave mode
  power_timer0_disable();
  for (int i = 0; i < TEST_SIZE; i++)
    my_data [i] = i;

  // pin change interrupt for D10
  PCMSK0 |= bit (PCINT2);  // want pin 10
  PCIFR  |= bit (PCIF0);   // clear any outstanding interrupts
  PCICR  |= bit (PCIE0);   // enable pin change interrupts for D8 to D13

  SPDR = 0;

  set_sleep_mode (SLEEP_MODE_IDLE);

}


void loop (void) 
  {
  sleep_mode ();
  }

At the next clock divisor (SPI_CLOCK_DIV32) I started getting occasional errors. After all, Fclk/32 is only 2 µs per clock pulse and 16 µs per byte, and the necessary stuff in the inner loop must be taking that long — I didn't calculate the individual instruction times, you are welcome to do that. :)
I was hoping to get better performance than that, but then I realised that even with a tight sending loop in the slave, you can't afford to send before the master has acknowledged the previous byte (ie. that it has read it) so you still have the tight interval between the end of one byte and the start of the next, which is the time in which you have to prepare the next byte for sending.
It's not as bad if you are the master, because you control when the byte starts, so you can take as long as you want between bytes.

82812: <p>I want two Arduino Nanos to communicate with each other using SPI, ideally at around 2MHz or faster.</p>
<p>I have two standard Nanos running at 16MHz on 5V.</p>
<ul>
<li><p>The master uses <code>SPI.transfer()</code> to send an array across the wire.¹ For ease of troubleshooting, the array alternately contains a bunch of zeroes <em>or</em> a bunch of 255s; but never both.</p>
</li>
<li><p>The slave simply returns decimal 66 for every byte. This was chosen because it's easy to spot on a logic trace.</p>
</li>
<li><p>The master checks the responses, and if there are any values that aren't 66, it prints the array to the UART.</p>
</li>
</ul>
<p>Even at 125Kbit, which is the slowest available speed, I'm regularly getting corrupted bytes. Interestingly, most of the time (but not always!) the corrupted byte is either 0 or 255 - in other words, exactly the same as the byte sent from the master! This strikes me as a very unlikely coincidence.</p>
<p>I have inspected the traffic with a logic analyser, and it seems that the slave just occasionally decides not to operate as instructed; and instead it just sends the input straight out again. As in, the byte going out on MISO is exactly the same as the byte coming in on MOSI. But only <em>sometimes.</em></p>
<p>What is happening here? Why doesn't it work? And what can I do about it?</p>
<hr />
<p>Master sketch:</p>
<pre><code>#define MOSI    12
#define SCK     13

#include &lt;SPI.h&gt;
#include &quot;pins_arduino.h&quot;
#include &lt;avr/wdt.h&gt;

void setup(void) {
    Serial.begin(9600);

    digitalWrite(SS, HIGH);
    pinMode(SS, OUTPUT);
    digitalWrite(SCK, HIGH);
    pinMode(SCK, OUTPUT);
    pinMode(MOSI, OUTPUT);

    SPI.setClockDivider(SPI_CLOCK_DIV128);      // 125KHz
    SPI.begin();

    Serial.println(&quot;\n\nSTARTING...\n&quot;);
}

void do_transfer(byte attempt, byte payload) {

    // packet size of 200
    byte buffer[200];

    // initialise packet
    for (byte count = 0; count &lt; sizeof(buffer); count++)
        buffer[count] = payload;

    // perform transfer
    digitalWrite(SS, LOW);
    SPI.transfer(buffer, sizeof(buffer));
    digitalWrite(SS, HIGH);    // SS is pin 10

    // tally results
    byte successes = 0, failures = 0;
    for (byte index = 0; index &lt; sizeof(buffer); index++)
        if (buffer[index] == 66) successes++; else failures++;

    // print results
    Serial.print(&quot;Attempt &quot;);
    Serial.print(attempt);
    Serial.print(&quot; with payload &quot;);
    Serial.print(payload);
    Serial.print(&quot;: &quot;);
    if (failures == 0)
        Serial.print(&quot;SUCCESS&quot;);
    else {
        Serial.print(successes);
        Serial.print(&quot; correct bytes, &quot;);
        Serial.print(failures);
        Serial.print(&quot; incorrect bytes&quot;);
        for (byte index = 0; index &lt; sizeof(buffer); index++) {
            if (index % 10 == 0)
                Serial.print(&quot;\n\t&quot;);
            if (buffer[index] == 66)
                Serial.print(&quot;-&quot;);
            else
                Serial.print(buffer[index]);
            if (index &lt; sizeof(buffer) - 1)
                Serial.print(&quot;\t&quot;);
        }
    }
    Serial.println();
    Serial.println();
}

void loop(void) {
    delay(800);                                                         // give the slave time to be ready

    for (byte attempt = 1; attempt &lt;= 10; attempt+=2) {
        do_transfer(attempt, 0x00);
        do_transfer(attempt+1, 0xFF);
        wdt_reset();
    }
    while(true);                                                        // end of program
}
</code></pre>
<p>Slave sketch:</p>
<pre><code>#include &quot;pins_arduino.h&quot;

void setup(void) {
    pinMode(MISO, OUTPUT);          // MISO needs to be an output
    SPCR |= _BV(SPE);               // enable SPI in slave mode
    SPCR |= _BV(SPIE);              // enable interrupts
    SPDR = 66;
}

ISR (SPI_STC_vect) { SPDR = 66; }   // just send 66 until the cows come home

void loop (void) { }
</code></pre>
<p>Connections: The following pins on both Arduinos are connected together:</p>
<ul>
<li>SS (pin 10)</li>
<li>MISO (pin 11)</li>
<li>MOSI (pin 12)</li>
<li>SCK (pin 13)</li>
<li>GND</li>
</ul>
<hr />
<p>¹ The problem also happens if I send one byte at a time using <code>SPI.transfer()</code>, unless (maybe) if I insert a 1ms delay between each byte. Which of course is ridiculous.</p>
 10794: <p>Your problem here is timing. I tried your setup, and found that I did indeed get occasional errors at 125 kHz. Disabling Timer 0 (as suggested by Chrisl) fixed that:</p>
<pre><code>  power_timer0_disable();
</code></pre>
<blockquote>
<p>But while it now works up to 250KHz, I get many many errors at 500KHz.</p>
</blockquote>
<p>OK, the time between clock pulses is now 2 µs, however as I document <a href="http://gammon.com.au/interrupts" rel="nofollow noreferrer">here</a> the time taken to execute an ISR like SPI_STC_vect is 2.625 µs, therefore it can't keep up with having to put the data there every 2 s.</p>
<p>You actually have slightly more time than 2 µs because the critical time is the time between the end of the first byte (which is when the ISR is called) and the time before the start of the second byte, which is when the master will start sampling your response.</p>
<p>See screenshot below with the critical time boxed in yellow.</p>
<p><a href="https://i.stack.imgur.com/UB5IT.png" rel="nofollow noreferrer"><img src="https://i.stack.imgur.com/UB5IT.png" alt="SPI timing screenshot" /></a></p>
<p>That time is 2.45 µs and the ISR takes 2.625 µs. Now some of that is the ISR epilogue which won't really matter that much, but you can see that things are tight. It's hard to know exactly at what point the processor starts calling the ISR after the last bit arrives, and the length of the instruction it is currently executing could be an influencing factor.</p>
<p>I got your sketches to work running at 250 kHz with these amendments to the slave:</p>
<pre class="lang-cpp prettyprint-override"><code>#include &quot;pins_arduino.h&quot;
#include &lt;avr/power.h&gt;
#include &lt;avr/sleep.h&gt;

void setup(void) {
  pinMode(MISO, OUTPUT);          // MISO needs to be an output
  SPCR |= _BV(SPE);               // enable SPI in slave mode
  SPCR |= _BV(SPIE);              // enable interrupts
  SPDR = 66;
  power_timer0_disable(); 
  set_sleep_mode (SLEEP_MODE_IDLE);
}

ISR (SPI_STC_vect) { SPDR = 66; }   // just send 66 until the cows come home

void loop (void) 
 {
 sleep_mode ();
 }
</code></pre>
<p>The sleeping in idle mode makes sure that the processor is not half-way through an instruction when the interrupt is raised.</p>
<p>As expected (for the reasons above) it did not work at 500 kHz.</p>
<p>If you want to transfer data reliably you need to allow, in the master, for the time it takes the slave to do stuff. Every line of code in the slave means that the master has to wait for that to be executed.</p>
<p>I mention that in <a href="http://www.gammon.com.au/spi" rel="nofollow noreferrer">my page about SPI</a> where I build in a delay in the master, if I want the slave to do calculations or something.</p>
<hr />
<blockquote>
<p>But the datasheet says that speeds of Fosc/4 are possible in slave mode, which would be 4MHz in this case. Would that be achievable if I removed the ISR and just used a busy loop?</p>
</blockquote>
<p>Well, yes the datasheet has an example of doing a busy loop:</p>
<pre class="lang-cpp prettyprint-override"><code>char SPI_SlaveReceive(void)
{
/* Wait for reception complete */
while(!(SPSR &amp; (1&lt;&lt;SPIF)))
;
/* Return Data Register */
return SPDR;
}
</code></pre>
<p>Regardless of what they claim, what you intend has to be physically possible. So with Fosc/4, assuming a 16 MHz clock, you have 2 µs for a byte to arrive (250 ns * 8). So the hardware may well be able to receive one byte in that time, and you may just have enough clock cycles to notice it arriving and plonk it into memory somewhere (although things could be tight) but you <em>won't</em> have time to send a different response after each incoming byte, because you only have around 250 ns to do it in, and the loop that detects the interrupt will probably use up most or all of that.</p>
<p>After a bit of mucking around, I got it to work at 250 kHz where the objective was just for the slave to return some data (after all, I presume that is what the slave is supposed to do).</p>
<h2>Master</h2>
<pre class="lang-cpp prettyprint-override"><code>#define MOSI    12
#define SCK     13

#include &lt;SPI.h&gt;
#include &quot;pins_arduino.h&quot;
#include &lt;avr/wdt.h&gt;

void setup(void) {
    Serial.begin(9600);

    digitalWrite(SS, HIGH);
    pinMode(SS, OUTPUT);
    digitalWrite(SS, HIGH);

    digitalWrite(SCK, HIGH);
    pinMode(SCK, OUTPUT);
    pinMode(MOSI, OUTPUT);

    SPI.begin();
    SPI.setClockDivider(SPI_CLOCK_DIV64);     

    Serial.println(&quot;\n\nSTARTING...\n&quot;);
    
}

void do_transfer(byte attempt, byte payload) {

    // packet size of 200
    byte buffer[200];

    // perform transfer
    digitalWrite(SS, LOW);
    delay (1);    // give slave time to react
    SPI.transfer(buffer, sizeof(buffer));
    digitalWrite(SS, HIGH);    // SS is pin 10

    // tally results
    byte successes = 0, failures = 0;
    for (byte index = 0; index &lt; sizeof(buffer); index++)
        if (buffer[index] == index) successes++; else failures++;

    // print results
    Serial.print(&quot;Attempt &quot;);
    Serial.print(attempt);
    Serial.print(&quot; with payload &quot;);
    Serial.print(payload);
    Serial.print(&quot;: &quot;);
    if (failures == 0)
        Serial.print(&quot;SUCCESS&quot;);
    else {
        Serial.print(successes);
        Serial.print(&quot; correct bytes, &quot;);
        Serial.print(failures);
        Serial.print(&quot; incorrect bytes&quot;);
        for (byte index = 0; index &lt; sizeof(buffer); index++) {
            if (index % 10 == 0)
                Serial.print(&quot;\n\t&quot;);
            if (buffer[index] == index)
                Serial.print(&quot;-&quot;);
            else
                Serial.print(buffer[index]);
            if (index &lt; sizeof(buffer) - 1)
                Serial.print(&quot;\t&quot;);
        }
    }
    Serial.println();
    Serial.println();
}

void loop(void) {
    delay(800);                                                         // give the slave time to be ready

    for (byte attempt = 1; attempt &lt;= 10; attempt+=2) {
        do_transfer(attempt, 0x00);
        do_transfer(attempt+1, 0xFF);
        wdt_reset();
    }
    while(true);                                                        // end of program
}
</code></pre>
<h2>Slave</h2>
<pre class="lang-cpp prettyprint-override"><code>#include &quot;pins_arduino.h&quot;
#include &lt;avr/power.h&gt;
#include &lt;avr/sleep.h&gt;

const int TEST_SIZE = 200;

volatile byte my_data [TEST_SIZE];

volatile byte dummy;

ISR (PCINT0_vect)
 {
 // handle pin change interrupt for D8 to D13 here

  // ignore transition to HIGH
  if (digitalRead (SS) == HIGH)
    return;
    
  SPDR = 0;
  
  for (byte i = 0; i &lt; TEST_SIZE; i++)
    {
    SPDR = my_data [i];
    while(!(SPSR &amp; (1&lt;&lt;SPIF)))
      {
      // if SS went high, give up
      if (PINB &amp; (1 &lt;&lt; 2))
        return;
      }
    } // end of for loop

    
 }  // end of PCINT0_vect

void setup(void) {
  pinMode(MISO, OUTPUT);          // MISO needs to be an output
  SPCR |= _BV(SPE);               // enable SPI in slave mode
  power_timer0_disable();
  for (int i = 0; i &lt; TEST_SIZE; i++)
    my_data [i] = i;

  // pin change interrupt for D10
  PCMSK0 |= bit (PCINT2);  // want pin 10
  PCIFR  |= bit (PCIF0);   // clear any outstanding interrupts
  PCICR  |= bit (PCIE0);   // enable pin change interrupts for D8 to D13

  SPDR = 0;

  set_sleep_mode (SLEEP_MODE_IDLE);

}


void loop (void) 
  {
  sleep_mode ();
  }
</code></pre>
<p>At the next clock divisor (SPI_CLOCK_DIV32) I started getting occasional errors. After all, Fclk/32 is only 2 µs per clock pulse and 16 µs per byte, and the necessary stuff in the inner loop must be taking that long — I didn't calculate the individual instruction times, you are welcome to do that. :)</p>
<p>I was hoping to get better performance than that, but then I realised that even with a tight sending loop in the slave, you can't afford to send before the master has acknowledged the previous byte (ie. that it has read it) so you still have the tight interval between the end of one byte and the start of the next, which is the time in which you have to prepare the next byte for sending.</p>
<p>It's not as bad if you are the master, because <em>you</em> control when the byte starts, so you can take as long as you want between bytes.</p>
