2MHz SPI between two arduinos...help!

Please don't crosspost.

eriknyquist:
Surely impedance matching isn't necessary for such short distance/low freq?

Nope.

I suspect something like an interrupt coming along and using up more time than it takes two bytes to arrive over SPI, losing one of them.

Try doing "cli()". This will disable all interrupts. millis(), delay(), etc. will stop working but at least you'll know if that's the problem so you know where to start looking.

ok, thanks for the suggestion. Now, I have to ask, how do I implement that? is it as simple as adding

cli();

to my code in the setup?

eriknyquist:
ok, thanks for the suggestion. Now, I have to ask, how do I implement that? is it as simple as adding

cli();

to my code in the setup?

Yes

Surely impedance matching isn't necessary for such short distance/low freq?

I would say that anything over 20/24Mhz, you need to think about signal integrity (the digital equivalent of impedance matching).

At 16Mhz, it is questionable / fuzzle and likely implementation dependent - you should have no problem if it is done correctly via hardware spi.

eriknyquist:
ok, thanks for the suggestion. Now, I have to ask, how do I implement that? is it as simple as adding

cli();

to my code in the setup?

There are predefined macros that can be used:

void setup() {}

void loop()
{
  noInterrupts();
  // critical, time-sensitive code here
  interrupts();
  // other code here
}

Lefty

How are you printing the numbers out? SPI waits for no man, if you a dicking around with serial printing and converting binary to ASCII HEX you may well lose data.
How are you storing the numbers?
Are you using interrupts for the SPI?

I think you should post both halves of the code, there's little point commenting without all the facts.

Surely impedance matching isn't necessary for such short distance/low freq?

No.

As yes, don't cross post.


Rob

Sorry for cross-posting guys. Didn't realise it was a no no- won't happen again!
 Heres my master and slave code anyway.

[code]
MASTER:

#include <SPI.h>

void setup (void)
{
  unsigned int i;
  digitalWrite(SS, HIGH);  // ensure SS stays high for now

  // Put SCK, MOSI, SS pins into output mode
  // also put SCK, MOSI into LOW state, and SS into HIGH state.
  // Then put SPI hardware into Master mode and turn SPI on
  SPI.begin ();
  Serial.begin(9600);

  // Slow down the master a bit
  SPI.setClockDivider(SPI_CLOCK_DIV8);
    // enable Slave Select
    

}  // end of setup


void loop (void)
{
  unsigned int i;
  digitalWrite(SS, LOW);    // SS is pin 10

  // send test string
  //for (const char * p = "Hello, world!\n" ; c = *p; p++)
  //  SPI.transfer (c);

  for(i=0;i<250;i++){
    SPI.transfer (i);
  }
  //  SPI.transfer (c);

  // disable Slave Select
  digitalWrite(SS, HIGH);

  delay (2000);  // 1 seconds delay 
  byte a = SPDR;
  Serial.println(a);

}  // end of loop
SLAVE:

#include <SPI.h>
 
unsigned char buf [300];
volatile byte pos;
volatile boolean process_it;
char p;



 
void setup (void)
{
  Serial.begin (9600);   // debugging
 
  // have to send on master in, *slave out*
  pinMode(MISO, OUTPUT);
  
  // turn on SPI in slave mode
  SPCR |= _BV(SPE);
  
  // get ready for an interrupt 
  pos = 0;   // buffer empty
  process_it = false;
 
 // Slow down the master a bit
  SPI.setClockDivider(SPI_CLOCK_DIV8);

  
  // now turn on interrupts
  SPI.attachInterrupt();
  
  //SPI.setDataMode()
 
}  // end of setup
 
 
// SPI interrupt routine
ISR (SPI_STC_vect)
{
  byte c = SPDR;  // grab byte from SPI Data Register
  
  // add to buffer if room
  if (pos < sizeof buf)
    {
    buf [pos++] = c;
    
    // example: newline means time to process buffer
    // if this is set to 255 and master sends 255 then the whole things goes buck ape
    if (c == 249)
      process_it = true;
      
    }  // end of room available
}  // end of interrupt routine SPI_STC_vect
 
// main loop - wait for flag set in interrupt routine
void loop (void)
{
  int i; 
 /* SPI.begin();
    SPI.transfer (SPDR);*/
  if (process_it)
    {
    buf [pos] = 0;  
    Serial.print("\n\rstart...\n\r");
    for(i=0; i< pos; i++){
      Serial.print(buf[i], HEX);  // print as an ASCII-encoded hexadecimal
      Serial.print(" ");
      if(buf[i] != i){
        Serial.print("\n\rmismatch at byte no :");
        Serial.print(i, DEC);
        Serial.print("\n\r");
        Serial.print("value is: ");
        Serial.print(buf[i],DEC);
        Serial.print("\n\r");
        // do nothing as were now hosed
        while(1) { }
      }
    }
    Serial.print("\n\rend...\n\r");
    pos = 0;
    process_it = false;
    }  // end of flag set
    
}  // end of loop

[/code]

So yes, I am indeed dicking around with serial printing....however this works no problem at 1MHz and lower so I didn't think it was a problem. You think the serial printing might be slowing it down too much?

The serial printing needs almost a second in best case to print out the characters. During this time it interrupts the CPU after every character to put the next character in the buffer into the serial data register. The interrupts may block each other.

The other problem may be your interrupt routine. With 2MHz SPI speed you have 64 cycles between one SPI byte and the next one. A few cycles are needed to load the interrupt vector and save the processor state before entering your interrupt routine. Your routine is maybe 20-25 cycles so about half of the time slots available are used in the ISR leaving the other half to do the processing and printing to the serial port. At a speed of 9600 this may not be enough. Have you tried increasing your serial speed to let's say 57600? Does that change anything?

eriknyquist:
// SPI interrupt routine
ISR (SPI_STC_vect)
{
byte c = SPDR; // grab byte from SPI Data Register

// add to buffer if room
if (pos < sizeof buf)
{
buf [pos++] = c;

// example: newline means time to process buffer
// if this is set to 255 and master sends 255 then the whole things goes buck ape
if (c == 249)
process_it = true;

} // end of room available
} // end of int
[/QUOTE]Sometimes I can't resist an optimization opportunity when I see one. You are declaring a byte array of 300 bytes that will never overflow because pos is a byte. sizeof buf will never be reached. You'll never overwrite beyond the end of the buffer. You will have a limit of 256 bytes, but you will shave a few clock cycles. Since it wraparound to 256, do you need an overflow check? Overflow checks are important in many programs, but you never overflow because the byte becomes its own overflow preventer. Remove the overflow check for extra performance, if you've got a buffer bigger than the index variable, and if the buffer-full behavior isn't important.

This will perform slightly faster. It will have different behaviour when the buffer is full, it will start overwriting the beginning of the buffer again and suddenly discard the last 256 bytes of data (you'll have to test if this behavior is undesirable or not), but might work more reliably at a slightly higher speed. Since you're hosed anyway if the buffer is full, we probably don't care in what way it overflows? (depends on the application).

Here's a faster ISR which also is fixed-cycle (cycle count does not vary due to lack of "if" statement)

byte incomingByte; // Preallocate for performance
ISR (SPI_STC_vect)
{
incomingByte = SPDR;

// pos never overflows past 256, so we'll never overwrite beyond end of buffer, it wraps around
buf[pos++] = incomingByte;

// This becomes true the first time incomingByte equals 249
process_it |= (incomingByte == 249);
}

The only way I've managed to get really fast (can't remember how fast though) is to forget interrupts they are too slow, and use a tight polling loop. You ISR doesn't have a chance I think.

I'll see if I can find some old code that does that.


Rob

The cross-posting is strong in this one ...

Please do not cross-post. This wastes time and resources as people attempt to answer your question on multiple threads.

Threads merged.

  • Moderator

Regarding reply #12:

Please edit your post, select the code, and put it between [code] ... [/code] tags.

You can do that by hitting the # button above the posting area.

Thanks for all the suggestions guys, I really appreciate it. mrdrejohn, I tried your faster ISR and also with a serial speed of 115200. unfortunately same result.
tight polling loop- that's exactly what I need. I just don't know how to do it. I know I have to read the SPI status register (SPSR) which will tell me if data is available, I just don't know the correct way to implement it.
Anybody in the know?

I found my old code, it has a lot of error checking and preloading of the SPDR because it was a two-way transaction. It also detected the master and slave getting out of sync, this can be a real problem with SPI as there is no maximum period for the clock so once you get out of sync you can easily read say 100 bytes from end of one packet and the 156 from the start of the next one 2 seconds later. (This is not a problem if SS is used to sync the two).

I've removed all that code to leave a bare bones function.

#define MAX_BYTES	256;  // or 249 or whatever
byte	spi_data[MAX_BYTES];
volatile byte data_ready = false;

ISR	(SPI_STC_vect) {
  /////////////////////////////////////////////////////
  // This interrupt is caused by the first 8 clock pulses from the
  // master having shifted a byte into the SPDR
  //
  // Now we're here though we don't use interrupts but poll the 
  // SPIF flag. This gives much faster response to the server and 
  // reduces the delays the server has to apply between bytes.

  int count = 0;
  byte * ptr = data;
  *ptr++ = SPDR;		// get first byte ASAP

  for (int i = 0; i < MAX_BYTES - 1; i++) {
    while ((SPSR & (1 << SPIF)) == 0) ;  
    *ptr++ = SPDR;
  }

  data_ready = true;
}


void loop() {

  if (data_ready) {
    data_ready = false;
    // process the data
  }

}

Note that I do the whole job in an ISR, I'll get a caning for that but sometimes it's OK to break the rules I think, and after all what else are you going to do, you don't have time for any other code and you really don't want other interrupts during this either. You could however just set the data_ready flag and do the data acquisition elsewhere as we normally recommend, but this would add a large and non-deterministic latency and expose you to who-knows-what interrupts.

Note also this comment

This gives much faster response to the server and reduces the delays the server has to apply between bytes.

You may find that you have to add a small delay between bytes at the master end, if you do then it's probably better to just drop the bit rate, either way you've reached the end of the performance rope.

Adding a small delay after the first byte however may always be required to give the slave a bit more time to get ready.

It should go without saying that the data stream cannot be constant, you are doing a lot of printing so there has to be a large delay between bursts of data.

Totally untested but does compile.

EDIT: Small change made to the code.


Rob

Master:

for(i=0;i<250;i++){
    SPI.transfer (i);
  }

Slave:

ISR (SPI_STC_vect)
{
  byte c = SPDR;  // grab byte from SPI Data Register
  
  // add to buffer if room
  if (pos < sizeof buf)
    {
    buf [pos++] = c;
    
    // example: newline means time to process buffer
    // if this is set to 255 and master sends 255 then the whole things goes buck ape
    if (c == 249)
      process_it = true;
      
    }  // end of room available
}  // end of interrupt routine SPI_STC_vect

On this web page:

I mention that the master/slave sending (like you are doing) has a timing issue. You can see from your code above what it would be. You are hoping that the ISR with its code of putting stuff into a buffer, doing a compare, checking overflow, etc. plus the setup and leave time for the ISR (around 3 uS) has to keep up with the simple loop which is doing SPI.transfer as fast as it can.

You are just going to run out of clock cycles unless you slow the sending end. Graynomad's suggestion may work by sticking inside the ISR, although as he acknowledges, having lengthy ISRs will have its own issues (eg. millis() being out). At the highest clock rate (divide by 2) you need to be able to process the incoming data in 17 cycles (8 * 2 plus 1 extra seems necessary) which is 1.0625 uS.

Entering the ISR alone will take about 1.44 uS so even by the time you read the first byte it might be too late.

Hello,

I tried Nick Gammon's excellent example between 2 Arduinos and if I remove the delay(1000) I am having the same issue as the OP has.. It makes sense to me and reminds me of the Serial.AvailableforWrite() case - ie the master is sending too fast and it better check if it should send or not..

Is there a Serial.AvailableforWrite() alternative for SPI instead of using delays?

And....

Instead of the master broadcasting, would it be better if the Slave was polling when ready? Has anyone implemented that?

Thank you!!

PS:Nick, great page, you helped me a lot to understand SPI..

Nobody?