I2C between arduinos

I’m attempting to get a working example of sending commands from one Arduino to another, receiving information back via I2C and printing that to the serial monitor. I’ve tried to set this up so that the master takes Serial input from the keyboard and parse the input into the commands used, only nothing gets printed to the serial monitor when i issue what I think should be a correct set of instructions. Here’s the code I’ve come up with for master and slave. Any guidance?

Master:

//master
#include <Wire.h>
byte index;
short Variable1;
short Variable2;
short CMD;
int handler;
byte inI2Cbyte;
char inI2C[32];
byte I2Cindex;
char inChar;
char inCMD[32];
short I2CSystem;
short I2CStatus1;
short I2CStatus2;


void setup()
{
  Serial.begin(9600);
  Wire.begin();
  while(Serial.available() > 0)
  {
    inChar = Serial.read();
    if(inChar != ';' && index < 32) //":" is the terminator. Send ":" to indicate end of command?
      // while the inChar read from Serial.read does not contain terminator :, and the index is less than 15
    {
      inCMD[index] = inChar; //sets the inChar to the number in index, starting with 0
      index++; //increments the index by one
      inCMD[index] = '\0'; // this seems to add a null character to the char array. Is that necessary or desirable?
    }
    else if(inChar== ';')
    {
      Variable1 =(short)strtok(inCMD, ":");
      Variable2 =(short)strtok(inCMD, ":");
      CMD = (short)strtok(inCMD, ":");
      handler = 1;
      index = 0;
    }
  }

  inI2Cbyte = Wire.read(); 
  if(inI2Cbyte != ';') // ; terminates the read and causes a serialprint 
  {
    inI2C[index] = inI2Cbyte; //sets the inChar to the number in index, starting with 0
    index++; //increments the index by one
    inI2C[index] = '\0'; // this seems to add a null character to the char array. Is that necessary or desirable?
  }
  else if(inI2Cbyte == ';')  // Now, split the string to variables
  {
    I2CSystem = (short)strtok(inI2C, ":");
    I2CStatus1 = (short)strtok(NULL, ":");
    I2CStatus2 = (short)strtok(NULL, ":");
    I2Cindex = 0;
    Serial.print(I2CSystem);
    Serial.print(I2CStatus1);
    Serial.print(I2CStatus2);
  }
  else if(inI2Cbyte == '?')   	
  {
    Serial.print("Error getting" + CMD);
  }
}

void loop()
{
  while(handler = 1)
  {
    Wire.beginTransmission(2);
    Wire.write(Variable1);
    Wire.write(":");
    Wire.write(Variable2);
    Wire.write(":");
    Wire.write(CMD);
    Wire.write(";");
    Wire.endTransmission();
    Wire.requestFrom(2, 32);
    CMD = 0;
    handler = 0;
  }

}
//slave
#include <Wire.h>
byte index;
short Variable1;
short Variable2;
short CMD;
byte inI2Cbyte;
char inI2C[32];



void setup()
{
  Wire.begin(2);                // join i2c bus with address #2 (slave)
  Wire.onRequest(requestEvent); //declares that when I2C sends an onRequest, the function
  //requestEvent() gets called
  Wire.onReceive(recieveEvent); // declares that when I2C sends a command, the variable 
  //CMD gets updated by this function
}


void requestEvent()
{
  switch(CMD)
  {
  case 1: 
    sendVariable1(); 
    break;
  case 2: 
    sendVariable2(); 
    break;
  default: 
    break;
  }    
}

void recieveEvent(int howMany)
{
  inI2Cbyte = Wire.read(); 
  if(inI2Cbyte != ';') //; will indicate there was an error getting temperature. 
  {
    inI2C[index] = inI2Cbyte; //sets the inChar to the number in index, starting with 0
    index++; //increments the index by one
    inI2C[index] = '\0'; // this seems to add a null character to the char array. Is that necessary or desirable?
  }
  else if(inI2Cbyte == ';')  // Now, split the string to variables
  {
    Variable1 = (short)strtok(inI2C, ":");
    Variable2 = (short)strtok(NULL, ":");
    CMD = (short)strtok(NULL, ":");
    index = 0;
  }

}	


void sendVariable1()
{
  Wire.write(Variable1);
}

void sendVariable2()
{
  Wire.write(Variable2);
}

void loop()
{
}

Any guidance?

Not that you might consider helpful, but here goes, anyway.

Most of your global variables do not need to be global. Make them local where you can.

Your indenting is atrocious. Use Tools + Auto Format to make the code readable.

= != ==

	   	Variable1 =(short)strtok(inCMD, ":");
                Variable2 =(short)strtok(inCMD, ":");
                CMD = (short)strtok(inCMD, ":");
	   	I2CSystem = (short)strtok(inI2C, ":");
	   	I2CStatus1 = (short)strtok(NULL, ":");
                I2CStatus2 = (short)strtok(NULL, ":");

You can't cast a string to a byte. You could use atoi() to convert a string to an int. If the int contains a value that fits in a byte, it is safe to store it in a byte.

Paul, I've updated my post per your suggestion on the Auto Format front. I hope that makes it a bit easier to decipher.

As far as global variables, I intend to use the variables within other function calls (not listed in this code). If the variables are not global, they disappear when the function they are defined in ends, correct?

I'm not sure how I'm casting a string to a byte. I was under the impression that I was casting the tokenized string to a short. Should I replace

(short)strtok(inCMD, ":");

with
atoi(strtok(inCMD, ":")); ?

If the variables are not global, they disappear when the function they are defined in ends, correct?

Unless they are static. Values should be passed to functions, not stored in globals that the functions access, wherever possible.

I’m not sure how I’m casting a string to a byte.

strtok() returns a string. (short) is a cast. Since short is smaller than int, and there is only one size smaller than int, that size has one byte. Whether you call that byte a byte, a int8_t, a uint8_t, or an unsigned char doesn’t alter the fact that you can’t cast a string to a byte.

Should I replace

Exactly.

Paul, Thanks for the suggestion. I’ve played around with this a bit, and keep hitting my head against the wall. Much of what is out there on I2C is painfully unclear. I’ve gone with multiple iterations, debugging with a LCD attached to the slave to see what is coming in, and printing from the master to the serial monitor. So, I’m including a revised copy of my code that is stripped down without the particulars for my LCD and most of my debug Serial.println entries, but still verbose enough that I think it should be clear to see what I’m trying to do, and where I must be going wrong (I hope). I know there are some issues you mentioned with globals and passing variables, but I’m really more concerned with the I2C communication right now, and passing variables can come later. Here goes:

//Serial to I2C and back Master
#include<Wire.h>
char inSerial[20]; 
char serInChar; 
int index; 



char I2CHandler;     
char I2CCommandArray[9]; 


//constants for charArray constructor
char openBracket = '<';
char token =':';
char closeBracket ='>';
char terminator =';';
char nullCharacter ='\0';

// currently still using globals here - haven't quite worked out the passing of variables to other functions
// Rx/Tx over I2C is the priority for now

int intSlaveID;
int intFunctionToCall;
int intBytesToCall;
int intParameterToPass;
int caseToUse;
char slaveIDHighByte;
char slaveIDLowByte;
char functionToCallHigh;
char functionToCallLow;
char parameterToPassHigh;
char parameterToPassLow;

char responseArray[12];

boolean startOfSerEvent = false;
boolean endOfSerEvent = false;


void setup () 
{
  delay(750); // to prevent errors in displaying initial comments -- probably 
  //should not be commented out, even if the Serial.println's
  //called here are. Still trying to figure out why serial 
  //monitor sometimes prints these lines twice.
  Serial.begin(9600);
  Serial.println("Initialize library for I2C Master Test");
  Wire.begin(1);
  delay(750);
  Serial.println("Wire initialized");
  caseToUse = 1;


}


void loop()
{
  switch(caseToUse)
  {
  case 1: 
    SerialReceiveEvent(); 
    break;
  case 2: 
    createCommandArray(); 
    break;
  case 3: 
    sendCommand(); 
    break;
  case 4: 
    requestResponse(); 
    break;
  }
}


void SerialReceiveEvent()
{
  while (Serial.available()>0)
  {
    char serInChar =(char)Serial.read();     

    if (serInChar == '<')
    {
      startOfSerEvent = true; 
    }  
    else if (serInChar == '>')
    {
      endOfSerEvent = true;
    }
    if (serInChar == ';' && startOfSerEvent && endOfSerEvent)
    {
      inSerial[index] = '\0'; // Null terminate the string
      startOfSerEvent =false;
      endOfSerEvent =false;
      caseToUse = 2;
    }
    else if (startOfSerEvent && serInChar != '<' && serInChar != '>')
    {
      inSerial[index] = serInChar;
      index++;
    }
  }
}

void createCommandArray()
{

  intSlaveID = atoi(strtok(inSerial, ":"));
  intFunctionToCall = atoi(strtok(NULL, ":"));
  intBytesToCall = atoi(strtok(NULL, ":"));
  intParameterToPass = atoi(strtok(NULL, ":"));

  functionToCallLow = lowByte(intFunctionToCall);
  functionToCallHigh = highByte(intFunctionToCall);
  parameterToPassHigh = highByte(intParameterToPass);
  parameterToPassLow = lowByte(intParameterToPass);


  I2CCommandArray[0] = openBracket;
  I2CCommandArray[1] = functionToCallLow;
  I2CCommandArray[2] = functionToCallHigh;
  I2CCommandArray[3] = token;
  I2CCommandArray[4] = parameterToPassLow;
  I2CCommandArray[5] = parameterToPassHigh;
  I2CCommandArray[6] = token;
  I2CCommandArray[7] = closeBracket;
  I2CCommandArray[8] = terminator;
  I2CCommandArray[9] = nullCharacter;

  caseToUse = 3;

}


void sendCommand()
{
  Serial.println ("Send serial to i2c as command");
  Wire.beginTransmission(intSlaveID);
  Wire.write(I2CCommandArray);
  Wire.endTransmission();
  caseToUse = 4;
}



void requestResponse() //receive a byte array, and parse that array into appropriate variables.
{
  Serial.println("Requesting from I2C");
  int i;
  Wire.requestFrom(intSlaveID, intBytesToCall);
  while(Wire.available())
  {
    for(i=0; i<intBytesToCall; i++)
    {
      responseArray[i] = Wire.read();
      Serial.println(responseArray);
    }
    responseArray[i] ='\0';
  }
  // Serial.println(I2CRequestResponseArray);
  Serial.println("Read the response");
  Serial.println(responseArray);
  caseToUse = 1;
}
//I2C and back
#include <Wire.h>
int intSlaveID = 2;

//constants for charArray constructor -- just to make things easy
char openBracket = '<';
char token =':';
char closeBracket ='>';
char terminator =';';
char nullCharacter ='\0';

 // yes, this is all very verbose, but I've been all over the place trying to get this to work

char I2CinChar;
char inI2C[10]; 
                /*allocate space for the inI2C command array
                 this particular allocation is intended for the following:
                inI2C[0] = <                     |     start of incoming string 
                inI2C[1] = functionToCall        |     low byte
                inI2C[2] = functionToCall        |     high byte
                inI2C[3] =  ':'                  |     token seperator
                inI2C[4] = parameterToUse        |     low byte
                inI2C[5] = parameterToUse        |     high byte
                inI2C[6] =  ':'                  |     token seperator
                inI2C[7] =  '>'                  |     end of incoming string
                inI2C[8] = ';'                   |     terminator
                inI2C[9] = null terminator       |     
                inI2C[10]                        | to keep array even - necessary?
                */
int functionToCall;
int parameterToUse;                
int I2CInIndex; 


char outI2C[10]; 
                /*allocate space for the response array
                 this particular allocation is intended for the following:
                inI2C[0] = <               | start of out string 
                inI2C[1] = functionIDLow   | low byte
                inI2C[2] = functionIDHigh  | high byte
                inI2C[3] =  ':'            | token seperator
                inI2C[4] = variableLow     | low byte
                inI2C[5] = variableHigh    | high byte
                inI2C[6] =  '>'            | end of out string
                inI2C[7] =  ';'            | terminator
                inI2C[8] = '\0'            | null character                
                */

//for the sake of getting this set up, I'm assiging arbitary values to the functionID and the variable

int functionID=19234;
char functionIDLow;
char functionIDHigh;
int variable=16523;
char variableLow;
char variableHigh;                
int I2COutIndex;


boolean startOfI2CEvent = false;
boolean endOfI2CEvent = false;



void setup () 
{
  Wire.begin(intSlaveID);
  Wire.onRequest(TxHandler);
  Wire.onReceive(RxHandler);
}


void RxHandler(int howMany)
{
  while(Wire.available()>0)  
      {
    for (int i = 0; i < 12; i++)
      inI2C[i] = Wire.read();
    }
    processI2CBuffer();
}


void TxHandler()
{
  Wire.beginTransmission(1); 
  Wire.write(outI2C);
  Wire.endTransmission();
}


void processI2CBuffer()
{
  functionToCall = atoi(strtok(inI2C, ":"));
  parameterToUse = atoi(strtok(NULL, ":"));
  inI2C[10] = NULL;
  createCharOutI2C();  
}

void createCharOutI2C()
{
  outI2C[0] = openBracket;
  functionIDLow = lowByte(functionID);
  outI2C[1] = functionIDLow;
  functionIDHigh = highByte(functionID);
  outI2C[2] = functionIDHigh;
  outI2C[3] = token;
  variableLow = lowByte(variable);
  outI2C[4] = variableLow;
  variableHigh = highByte(variable);
  outI2C[5] = variableHigh;
  outI2C[6] = token;
  outI2C[7] = closeBracket;
  outI2C[8] = terminator;
  outI2C[9] = nullCharacter; 
}
//I know this is ugly, but I'm taking this stepwise.

void loop()
{

}
    if (serInChar == '<')

You created a variable, openBracket, containing this same value. Why not use it here? Why not give the variable a generic name, like startOfPacket, so you can change the value if you decide that you like ‘$’ better?\

    else if (startOfSerEvent && serInChar != '<' && serInChar != '>')
    {
      inSerial[index] = serInChar;
      index++;
    }

Doesn’t matter if there is room in the array?

Where do you ever reset index to 0?

    else if (serInChar == '>')
    {
      endOfSerEvent = true;
    }
    if (serInChar == ';' && startOfSerEvent && endOfSerEvent)
    {
      inSerial[index] = '\0'; // Null terminate the string
      startOfSerEvent =false;
      endOfSerEvent =false;
      caseToUse = 2;
    }

Is ‘>’ the end of packet marker? Or is ‘;’? If that if statement were an else if, the test for serInChar being ‘>’ or ‘<’ in the following statement would not be necessary.

  intSlaveID = atoi(strtok(inSerial, ":"));

NO! NEVER assume that strtok() will return something other than NULL.

  Wire.requestFrom(intSlaveID, intBytesToCall);
  while(Wire.available())
  {
    for(i=0; i<intBytesToCall; i++)
    {
      responseArray[i] = Wire.read();
      Serial.println(responseArray);
    }
    responseArray[i] ='\0';
  }

I don’t like this. You are assuming that the response will contain the requested number of bytes. Never a good idea. The Wire.available() function is telling you how many bytes are available(). You are then assuming that if one is available, all of them are. Does that seem like a good idea?

You are passing a non-NULL terminated array to a function that expects a NULL terminated array, and assuming that it will deal with it properly. Does that seem like a good idea?

I may be missing something, but it doesn’t appear that the second code has any way for you to know what it is doing.

Paul,
Thanks for taking the time to go through all this with me. If you’re ever in the Bay Area, I owe you copious amounts of great coffee.

One major point that has provided some better results is the fact that I was telling the chip to write an array, but not telling it how many bytes of that array to write. It’s in the notes here on arduino, but not emphasized. I really wish there were a way to enhance the documentation for arduino from users in something other than the forum. For all it’s flaws, a digg model seems like it should have happened long ago. Upvoting good solutions to common problems needs to happen.

Still, I’m not getting exactly what I want and there are a few points for clarification.

Doesn’t matter if there is room in the array?

Yes, it does. With a well-structured program (which resets the index to zero in one of its functions before writing to the array), this doesn’t seem completely necessary to check, though the code might prove more useful to others and applications outside what I’m intending. I’m thinking a while loop nested within that else if which checks that the index is less than the size of the array of inSerial might be a place to start.

2

NO! NEVER assume that strtok() will return something other than NULL

Yikes! Well, I was going on one of your previous posts here. I’m a little confused over the emphatic “NO!” – if strtok returns NULL, is that necessarily bad? At least for future strtok calls? For example, if I wanted to have an array which assigned values to variables, a NULL within an array would simply assign NULL to that variable, correct? That seems like it could be useful in some instances. Now, a check that what strtok returns isn not NULL might affect how one handles those bytes of the array – i.e. not trying to convert ASCII to an int if NULL

I don’t like this.

Upon further inspection, I don’t either. If a byte didn’t transmit, things could get wonky real quick. Would something like while(Wire.available()==intBytesToCall) be a good check? Perhaps some sort of if/else schema that if the wire buffer does not contain the expected number of bytes, a re-transmit of the last (maybe command) and request?

You are passing a non-NULL terminated array

Where?

On a couple of other notes/questions:

Variable names have been updated so if I want a $, that can be done easily.

The packet is contained within “<” and “>” I was using the “;” to signal the end of the packet so that if an application demanded it, “;” could be used within the array if the “>” had not yet been transmitted (not important to my application, but I thought this might be useful). Somehow, I thought it better to have both > and ; indicate the end of relevant data. Any downfalls to keeping this? I kind of only see an upside – at the expense of 1 byte transmitted, there would be a double check on the end of packet(">") and the terminator(";").

On missing something, you’re not. Well, not really. I condensed my code so that issues could be identified with how I’m doing things in the code, rather than checking every step and it elicited exactly the type of critiques I was looking for from you. Mirrored code with Serial.println on the master and mySerial.print on the slave (yes, regrettably, I’m using a Parallax LCD on the slave for debugging, and software serial is involved) is on my computer, and that’s what I’m applying the concepts to.

I'm not even sure I2C will work between arduinos. It is a master/slave scheme and the arduino code is set up to be the master.

Maybe you should try serial or SPI.