SPI - Improved Gammon Synchronous Structure Contrib and Dodgy ISR discovery.....

Hi Team-

New user, just thought I'd add a bit of a contribution and also seek some advice.

In a nutshell, I'm using Arduino to do some control of Solar Charge Controllers using Serial ports.

I'm using a mega 2560 quite successfully.

Ran out of Serial Ports, my code is non-blocking so I don't like Software Serial - easiest way to add more serial IO is to use a second Arduino and have the two coordinate activities over SPI.

After many days I've come up with some working code for (near) synchronous transfer of data using SPI, with CRC check.

This code basically sends a structure from master to slave, and slave sends back another structure at the same time. It's a very basic framework. I've added Nick's CRC code into his SPI_Anything.h code, and amended the SPI Write Anything and Read Anything code to add pointers to two structures (incoming and outgoing) to make synchronous SPI communication easier... So here's my contribution, I hope it's useful to someone. I've commented extensively to hopefully make it self explanatory.

The Amended SPI Anything Library

#include <Arduino.h>

template <typename T> unsigned int SPI_writeAnything (T& outgoingStructure, T& incomingStructure)
  {
    //byteCRC=0;
 //byte returnByte;
    byte * outgoingByte = (byte*) &outgoingStructure;
 byte * incomingByte = (byte*) &incomingStructure;
 unsigned int i;
    
 for (i = 0; i < sizeof outgoingStructure; i++) {
      if (i==0) {
  SPI.transfer(*outgoingByte++);//we write the next byte but ignore the result as the result will be whatever was in the slave arduino's buffer.
  delayMicroseconds(20);// the Arduino implementation has an error that results in unpredictable behavior if this delay is not put here.
  }
  else
  {
  *incomingByte++=SPI.transfer(*outgoingByte++); //after the first byte is written every subsequent byte received will be the contents of the slave structure - so record in master structure.
  //delayMicroseconds(50);
  }
 
 }
 
 *incomingByte=SPI.transfer(0); //need to do one final dummy write to get the last byte of data from the slave structure.
 return i;
  }  // end of SPI_writeAnything
  
  
template <typename T> unsigned int SPI_readAnything_ISR(T& incomingStructure,T& outgoingStructure)
  {
    byte * incomingByte = (byte*) &incomingStructure;
 byte * outgoingByte = (byte*) &outgoingStructure;
    unsigned int i;
    *incomingByte++ = SPDR; //at the beginning of the interrupt, the very first byte will be sitting in the SPDR buffer.
 
    for (i = 1; i < sizeof incomingStructure; i++)

 {
 *incomingByte++ = SPI.transfer (*outgoingByte++); //with every subsequent byte we send the next byte of the local structure (*outgoingByte) 
 } //to the master and store to the remote structure locally (*incomingByte++).
  
    SPI.transfer(*outgoingByte);
 return i;
  }  // end of SPI_readAnything_ISR  


  
  template <typename T> unsigned int SPI_CalculateCRC (T& targetStructure)
  {

  byte * addr = (byte*) &targetStructure;
  byte crc = 0;
  long len=(sizeof targetStructure - sizeof (byte)); //the last byte of my structure is always the CRC.
 //so ignore the last byte of the structure or we'd
 //be calculating the CRC on our CRC.
  while (len--) 
    {
    byte inbyte = *addr++;
    for (byte i = 8; i; i--)
      {
      byte mix = (crc ^ inbyte) & 0x01;
      crc >>= 1;
      if (mix) 
        crc ^= 0x8C;
      inbyte >>= 1;
      }  // end of for
    }  // end of while
  return crc;
  }  // end of CRC Calculation

part 2 follows...

Master Arduino Code

// Written by M James using advice from Nick Gammon
// at http://www.gammon.com.au/spi
// April 2017


#include <SPI.h>
#include "SPI_anything.h"
#define SS 7
//#include <MemoryFree.h>
#define ALTERNATIVE_SS 7 //I'm using pin 7 as my master SS output.

// create a structure to store the different data values:
typedef struct SCCValues
{
  unsigned int SetPoint;
  unsigned int Yield;
  unsigned int MaxPow;
  unsigned int Temp;
  unsigned int BattCurrent;
  unsigned int BattVolts;
  unsigned int PanelVolts;
  unsigned int PanCurrent;
  unsigned int ChargerState;
  unsigned int ChargerMode;
  byte CRC;
};

SCCValues outgoingStructure;
SCCValues incomingStructure;


unsigned int t = 0;
long Checksum;

void setup (void)
{
  Serial.begin (115200);
  pinMode(ALTERNATIVE_SS, OUTPUT); //we don't want Slave Select to float - so assert as output.
  SPI.begin ();

  //initialise structure values.
  outgoingStructure.SetPoint = 147;
  outgoingStructure.Yield = 0;
  outgoingStructure.MaxPow = 0; 
  outgoingStructure.Temp = 0; 
  outgoingStructure.BattCurrent = 88; 
  outgoingStructure.BattVolts = 0; 
  outgoingStructure.PanelVolts = 0; 
  outgoingStructure.PanCurrent = 0;
  outgoingStructure.ChargerState = 0; 
  outgoingStructure.ChargerMode = 0; 
  outgoingStructure.CRC = 0;

  incomingStructure.SetPoint = 0;
  incomingStructure.Yield = 0;
  incomingStructure.MaxPow = 0;
  incomingStructure.Temp = 0;
  incomingStructure.BattCurrent = 0;
  incomingStructure.BattVolts = 0;
  incomingStructure.PanelVolts = 0;
  incomingStructure.PanCurrent = 0;
  incomingStructure.ChargerState = 0;
  incomingStructure.ChargerMode = 0;
  incomingStructure.CRC = 0;
}  // end of setup

void loop ()
{
  //calculate and add the CRC to the outgoing structure so that slave can compare.
  outgoingStructure.CRC = SPI_CalculateCRC(outgoingStructure);

//initialise SPI per Gammon http://www.gammon.com.au/forum/?id=10892&reply=9#reply9
#if SPI_HAS_TRANSACTION
  SPI.usingInterrupt (0);  // I am using external interrupt 0
  SPI.usingInterrupt (1);  // I am also using external interrupt 1
  SPI.beginTransaction (SPISettings (4000000, MSBFIRST, SPI_MODE0));  // 4 MHz clock (I'm using Atmega 2560 as M and Sl)
                                                                      // you might want to try 2000000 if probs.
#else
  SPI.setClockDivider(SPI_CLOCK_DIV4); // divide by 4 works for me - change to 8 if necessary
  SPI.setBitOrder(MSBFIRST);
  SPI.setDataMode(SPI_MODE0);
#endif // SPI_HAS_TRANSACTION

  digitalWrite(ALTERNATIVE_SS, LOW);    // I'm not using the default SS - change in #define if necessary.

  SPI_writeAnything(outgoingStructure, incomingStructure);
  digitalWrite(ALTERNATIVE_SS, HIGH);

#if SPI_HAS_TRANSACTION
  SPI.endTransaction ();   // allow external interrupts to fire now
#endif // SPI_HAS_TRANSACTION

  delay (20);  // for testing

  //We add some of the received values to the CRC as a quick and dirty way of being even more sure that the received values
  //are different to the last values - and thus being sure that the Incoming SPI Transfer has been successful.
  //The rest of the code immediately below is designed to show if the received data is unique - a "." gives you a strong
  //indication it is, an 'X' gives a strong indication it's the same as last time (ie prob the Slave didn't send a value)
  //an 'O' tends to indicate that the SPI isn't working at all and a 'C' indicates the CRC didn't match - eg corruption 
  //occurred during transfer.
  if (Checksum != long(incomingStructure.CRC) + long(incomingStructure.SetPoint) + long(incomingStructure.MaxPow)) {
    Serial.print(F(".")); //looks to be a unique value...
  }
  else if (incomingStructure.CRC == 0) //usually the CRC is 0 if no data at all is coming in from the slave.
  {
    Serial.print(F("O"));
  }
  else if (SPI_CalculateCRC(incomingStructure) != incomingStructure.CRC) { //is the CRC corrupted?
    Serial.print(F("C"));
  }
  else //if our 'unique checksum check' shows that these values are the same as the last transfer, assume tfr failed.
  {
    Serial.print(F("X"));
  }

  //Serial screen formatting code to make it pretty.
  t++;
  if (t >= 100) {
    Serial.println();
    //Serial.print(freeMemory()); //this was here as an earlier check to make sure probs weren't caused by mem leak.
    t = 0;
  }


  Checksum = long(incomingStructure.CRC) + long(incomingStructure.SetPoint) + long(incomingStructure.MaxPow);
  
  outgoingStructure.ChargerMode=rand()%100; //randomly change a component of the outgoing data to slave.
}  // end of loop

Slave Arduino Code

// SPI SLAVE ARDUINO CODE.
// Written by M James using advice from Nick Gammon
// at http://www.gammon.com.au/spi
// April 2017

#include <SPI.h>
#include "SPI_anything.h"

// create a structure to store the different data values:
typedef struct SCCValues
{
  unsigned int SetPoint;
  unsigned int Yield;
  unsigned int MaxPow;
  unsigned int Temp;
  unsigned int BattCurrent;
  unsigned int BattVolts;
  unsigned int PanelVolts;
  unsigned int PanCurrent;
  unsigned int ChargerState;
  unsigned int ChargerMode;
  byte CRC;
};

byte z = 0;
long SuccessReadCheckMark = 0; //used in a timer as a quick and dirty watchdog.

//initialise structure instances.
volatile SCCValues incomingStructure;
volatile SCCValues outgoingStructure;

volatile bool haveData = false;


void setup ()
{
  Serial.begin (115200);   // debugging

  // have to send on master in, *slave out*
  pinMode(MISO, OUTPUT);
  
  // turn on SPI in slave mode
  SPCR |= _BV(SPE);

  // turn on interrupts
  SPCR |= _BV(SPIE);

}  // end of setup

void loop ()
{

  
  //Print something each iteration of loop to keep the processor busy.
  if (z <= 100) { //format so that it doesn't make the serial output ugly.
    Serial.print(".");
    z++;
  }
  else
  {
    Serial.println();
    z = 0;
  }
  
  if (haveData)
  {
    //just put a bunch of random numbers into the structure elements
    //to send back to the master next time it asks.
    outgoingStructure.SetPoint = rand() % 32000;
    outgoingStructure.Yield = rand() % 32000;
    outgoingStructure.MaxPow = rand() % 32000;
    outgoingStructure.Temp = rand() % 32000;
    outgoingStructure.BattCurrent = rand() % 32000;
    outgoingStructure.BattVolts = rand() % 32000;
    outgoingStructure.PanelVolts = rand() % 32000;
    outgoingStructure.PanCurrent = rand() % 32000;
    outgoingStructure.ChargerState = rand() % 32000;
    outgoingStructure.ChargerMode = rand() % 32000;
    outgoingStructure.CRC = SPI_CalculateCRC(outgoingStructure);
    SuccessReadCheckMark = millis();
    
    if (SPI_CalculateCRC(incomingStructure) == incomingStructure.CRC) {

      Serial.println("-"); //dash means incoming CRC valid.
    }
    else
    {
      Serial.println("C"); //C's mean incoming CRC invalid - corrupt.
    }

  }

  //Quick and dirty way to check if Slave has stopped receiving valid updates from Master
  //and do something if it hasn't - you could add a while(1) loop here to trigger the
  //watchdog timer or something similar to get SPI working again.
  if ((millis() - SuccessReadCheckMark) >= 7000) {

    SuccessReadCheckMark = millis(); 

  }
  haveData = false;

}  // end of loop

// SPI interrupt routine
ISR (SPI_STC_vect)
{
  haveData = true;
  SPI_readAnything_ISR(incomingStructure, outgoingStructure); //send and rcv spi data.
}  // end of interrupt routine SPI_STC_vect

Note that I'm not using the standard Slave Select (SS) pin on the Master to signal the slave (as I intend to add an Ethernet shield to the master and thus want to keep the native SS pin free and available) - so in my case I'm using Pin 7 - another digital IO on the mega 2560 as the SS/CS that asserts low when it's ready to transmit to the slave - there's a sneaky define at the top of the Master code and you can modify accordingly.

Nick Please feel free to add this contribution to your page at Your SPI Examples Page if considered worthwhile.

OK... so as I mentioned, I hope this working code helps someone - shared in the spirit of integrating a lot of Nick's work into one place and one Library.

OK - now for my questions where I talk about some of the issues I have had with the code and ask for advice from people with more experience in these matters than I.

I struggled with an unusual problem for a several hours when I first devised this code - the problem was that the interrupt driven Slave code was giving erratic results. It was only returning data to the Master about 60% of the time. I tried all sorts of things, including:-

  • Slowing down the Clock Speed of the master (/8, /16 etc etc) - made no difference, sometimes made it worse.
  • Slowing down the frequency with which I sent data from the master (could be milliseconds to minutes - still made no difference
  • Playing around with the delayMicroseconds() component in the SPI_WriteAnything - found it was definitely needed, but increasing above about 20 microseconds made no difference and sometimes made things worse.
  • Lots and lots of other stuff....

I tracked the source of the problem to the Slave Side - my original problematic code was as follows:-

Original Problematic Slave Code - DO NOT USE

// SPI SLAVE ARDUINO CODE.
// Written by M James using advice from Nick Gammon
// at http://www.gammon.com.au/spi
// April 2017

#include <SPI.h>
#include "SPI_anything.h"

// create a structure to store the different data values:
typedef struct SCCValues
{
  unsigned int SetPoint;
  unsigned int Yield;
  unsigned int MaxPow;
  unsigned int Temp;
  unsigned int BattCurrent;
  unsigned int BattVolts;
  unsigned int PanelVolts;
  unsigned int PanCurrent;
  unsigned int ChargerState;
  unsigned int ChargerMode;
  byte CRC;
};

byte z = 0;
long SuccessReadCheckMark = 0; //used in a timer as a quick and dirty watchdog.

//initialise structure instances.
volatile SCCValues incomingStructure;
volatile SCCValues outgoingStructure;

volatile bool haveData = false;


void setup ()
{
  Serial.begin (115200);   // debugging

  // have to send on master in, *slave out*
  pinMode(MISO, OUTPUT);
  
  // turn on SPI in slave mode
  SPCR |= _BV(SPE);

  // turn on interrupts
  SPCR |= _BV(SPIE);

}  // end of setup

void loop ()
{

//  
//  //Print something each iteration of loop to keep the processor busy.
//  if (z <= 100) { //format so that it doesn't make the serial output ugly.
//    Serial.print(".");
//    z++;
//  }
//  else
//  {
//    Serial.println();
//    z = 0;
//  }
  
  if (haveData)
  {
    //just put a bunch of random numbers into the structure elements
    //to send back to the master next time it asks.
    outgoingStructure.SetPoint = rand() % 32000;
    outgoingStructure.Yield = rand() % 32000;
    outgoingStructure.MaxPow = rand() % 32000;
    outgoingStructure.Temp = rand() % 32000;
    outgoingStructure.BattCurrent = rand() % 32000;
    outgoingStructure.BattVolts = rand() % 32000;
    outgoingStructure.PanelVolts = rand() % 32000;
    outgoingStructure.PanCurrent = rand() % 32000;
    outgoingStructure.ChargerState = rand() % 32000;
    outgoingStructure.ChargerMode = rand() % 32000;
    outgoingStructure.CRC = SPI_CalculateCRC(outgoingStructure);
    SuccessReadCheckMark = millis();
    
    if (SPI_CalculateCRC(incomingStructure) == incomingStructure.CRC) {

      Serial.println("-"); //dash means incoming CRC valid.
    }
    else
    {
      Serial.println("C"); //C's mean incoming CRC invalid - corrupt.
    }

  }

  //Quick and dirty way to check if Slave has stopped receiving valid updates from Master
  //and do something if it hasn't - you could add a while(1) loop here to trigger the
  //watchdog timer or something similar to get SPI working again.
  if ((millis() - SuccessReadCheckMark) >= 7000) {

    SuccessReadCheckMark = millis(); 

  }
  haveData = false;

}  // end of loop

// SPI interrupt routine
ISR (SPI_STC_vect)
{
  haveData = true;
  SPI_readAnything_ISR(incomingStructure, outgoingStructure); //send and rcv spi data.
}  // end of interrupt routine SPI_STC_vect

You'll see that it's exactly the same as the working code above, except that the following lines at the beginning of the main loop are commented out:-

  //Print something each iteration of loop to keep the processor busy.
  if (z <= 100) { //format so that it doesn't make the serial output ugly.
    Serial.print(".");
    z++;
  }
  else
  {
    Serial.println();
    z = 0;
  }

What I found was that it was absolutely essential that the processor was made to do something meaty within the loop in between interrupt driven 'haveData' loops (in this case printing characters.. but a delayMicroseconds(2000) also worked fine). With this modification the Slave successfully returns valid data to the Master in about 99.999% of cases for me.

I suspect there's a race condition at work here... I suspect that with the code just in a tight loop doing nothing other than checking for a 'haveData' event, that sometimes the ISR Routine ran midway through the loop and caused unexpected results.

Having the processor 'do' something else means the chances of the race condition being encountered are smaller, as the processor is doing more 'stuff' between interrupts - so it's really just reducing the chance of my suspected 'race condition' occurring, thus hiding the problem.

What I'm hoping is that someone might be able to identify for me where the race is and help me make this code absolutely bullet proof..

Ta!

M

Hmm.. does nobody have any input at all?

Friendly suggestion: it might help if you could explain up front what your code does and why people might want to use it.

Very few people need to connect a Mega 2560 to another Arduino, in order to solve some rarely encountered issue.

You leave us with the impression that a code problem remains to be resolved, and the suggestion of a race condition or the need to insert a delay() is not reassuring.

jremington:
Friendly suggestion: it might help if you could explain up front what your code does and why people might want to use it.

Very few people need to connect a Mega 2560 to another Arduino, in order to solve some rarely encountered issue.

You leave us with the impression that a code problem remains to be resolved, and the suggestion of a race condition or the need to insert a delay() is not reassuring.

Ok then. I think I made it pretty clear what the code does, both in the text and in the commenting, and I have certainly seen plenty of examples on these forums of people trying to achieve what I have. To date I haven't seen anyone put this all together in a coherent way, so I thought I'd make a contribution that builds upon the work of others (Nick for one) and I've pointed out that that code works and works well other than a very strange issue that shouldn't occur but does.

I think you're saying you can't provide any guidance on the reason either? Nonetheless thanks for your response.

This problem occurs even with Nick Gammon's raw code so I think it's well and truly worth raising up the flagpole as it's quite possible that this problem might not just be restricted to arduino to arduino communication. It's about atomicity of code between IRQ routines and the main loop and I'd love to know a technical reason why its happening if others have insights.

Without any hubris intended, contrary to my Newbie status this aint my first rodeo with embedded systems so please do consider that I'm raising it with the community in mind. I spent a good hour or two making the code user friendly to help people to understand it. I hope I didn't fail...

Thank you.
This looks a very useful modification to the SPI_anything library.

I have been working with a setup that has one Master and 3 slaves.
This code has been helpful but I find some steps missing at times.

My primary concern is with the way the master starts communicating with the slaves.

Of these 2 options, which one is the correct one to go about it:

1st :

 //calculate and add the CRC to the outgoing structure so that slave can compare.
 outgoingStructure.CRC = SPI_CalculateCRC(outgoingStructure);

//initialise SPI per Gammon http://www.gammon.com.au/forum/?id=10892&reply=9#reply9
#if SPI_HAS_TRANSACTION
 SPI.usingInterrupt (0);  // I am using external interrupt 0
 SPI.usingInterrupt (1);  // I am also using external interrupt 1
 SPI.beginTransaction (SPISettings (4000000, MSBFIRST, SPI_MODE0));  // 4 MHz clock (I'm using Atmega 2560 as M and Sl)
                                                                     // you might want to try 2000000 if probs.
#else
 SPI.setClockDivider(SPI_CLOCK_DIV16); // I am using divide by 16
 SPI.setBitOrder(MSBFIRST);
 SPI.setDataMode(SPI_MODE0);
#endif // SPI_HAS_TRANSACTION

 digitalWrite(A1_SS, LOW);    // I'm not using the default SS - change in #define if necessary.

 SPI_writeAnything(outgoingStructure, incomingStructure);
 digitalWrite(A1_SS, HIGH);


 digitalWrite(A2_SS, LOW);    // I'm not using the default SS - change in #define if necessary.

 SPI_writeAnything(outgoingStructure, incomingStructure);
 digitalWrite(A2_SS, HIGH);


#if SPI_HAS_TRANSACTION
 SPI.endTransaction ();   // allow external interrupts to fire now
#endif // SPI_HAS_TRANSACTION

or 2nd :

 //calculate and add the CRC to the outgoing structure so that slave can compare.
 outgoingStructure.CRC = SPI_CalculateCRC(outgoingStructure);

//initialise SPI per Gammon http://www.gammon.com.au/forum/?id=10892&reply=9#reply9
#if SPI_HAS_TRANSACTION
 SPI.usingInterrupt (0);  // I am using external interrupt 0
 SPI.usingInterrupt (1);  // I am also using external interrupt 1
 SPI.beginTransaction (SPISettings (4000000, MSBFIRST, SPI_MODE0));  // 4 MHz clock (I'm using Atmega 2560 as M and Sl)
                                                                     // you might want to try 2000000 if probs.
#else
 SPI.setClockDivider(SPI_CLOCK_DIV16); // I am using divide by 16
 SPI.setBitOrder(MSBFIRST);
 SPI.setDataMode(SPI_MODE0);
#endif // SPI_HAS_TRANSACTION

 digitalWrite(A1_SS, LOW);    // I'm not using the default SS - change in #define if necessary.

 SPI_writeAnything(outgoingStructure, incomingStructure);
 digitalWrite(A1_SS, HIGH);


#if SPI_HAS_TRANSACTION
 SPI.endTransaction ();   // allow external interrupts to fire now
#endif // SPI_HAS_TRANSACTION


 //calculate and add the CRC to the outgoing structure so that slave can compare.
 outgoingStructure.CRC = SPI_CalculateCRC(outgoingStructure);

//initialise SPI per Gammon http://www.gammon.com.au/forum/?id=10892&reply=9#reply9
#if SPI_HAS_TRANSACTION
 SPI.usingInterrupt (0);  // I am using external interrupt 0
 SPI.usingInterrupt (1);  // I am also using external interrupt 1
 SPI.beginTransaction (SPISettings (4000000, MSBFIRST, SPI_MODE0));  // 4 MHz clock (I'm using Atmega 2560 as M and Sl)
                                                                     // you might want to try 2000000 if probs.
#else
 SPI.setClockDivider(SPI_CLOCK_DIV16); // I am using divide by 16
 SPI.setBitOrder(MSBFIRST);
 SPI.setDataMode(SPI_MODE0);
#endif // SPI_HAS_TRANSACTION


 digitalWrite(A2_SS, LOW);    // I'm not using the default SS - change in #define if necessary.

 SPI_writeAnything(outgoingStructure, incomingStructure);
 digitalWrite(A2_SS, HIGH);


#if SPI_HAS_TRANSACTION
 SPI.endTransaction ();   // allow external interrupts to fire now
#endif // SPI_HAS_TRANSACTION

Hi Acumensw - sorry it's so long for me to reply. I've been out in the Desert.

Could you please help me by highlighting exactly the part of the code that you have a question about? Or, if you've solved your issue, please let us know where I muffed up :slight_smile: