How to set up a command-based communication using I2C

Instead of having both boards alternate between being Master/Slave, have one board as the master and the other as the slave. The master can then request data from the slave, which can either return the results of the processing, or a code indicating that the results are not yet ready. I would also not mess with variable-length payloads, instead use the first byte to indicate the type of payload, and the remaining bytes are the actual payload data, padded to fill the maximum payload length.

This would be expected when the M0 is not plugged in (unpowered). There are diodes on the input pins of almost all processors that prevent the pin from going more than about 0.7v above VCC, which in this case would be 0V since it is unpowered.

We are making progress, but I still have some issues.
But first of all, I would like to thank you all for taking your time and sharing your knowledge - much appreciated.

Let me explain the "protocol" I use. The payload from the master will (should) never exceed 16 bytes, and it could technically been padded such that it always uses 16 bytes. My I2C Stream will consist of 18 bytes:

1st byte: command (just an integer)
2nd byte: length of the payload (integer)
3th - 18th byte: payload

Here is the code of the slave. It does not do anything useful, it just returns some dummy data depending on the command and also prints out all information of interest.

#include "Wire.h"

#define M0_I2C_SLAVE_ADDRESS 13

volatile char commandBuffer [16];
volatile uint8_t command;
volatile uint8_t commandLength;
volatile uint8_t commandRequest = 0;
volatile uint8_t printRequest = 0;

enum {
    CMD_CTRL_RADIO = 169,
    CMD_INFO = 142,
    CMD_STATUS = 213
};

void setup() {
  Wire.begin(M0_I2C_SLAVE_ADDRESS);
  Wire.onReceive(receiveEvent);
  Wire.onRequest(requestEvent);
  SerialUSB.begin(115200); 
}

void receiveEvent(int nBytes){
  uint8_t i = 0;
  while(0 < Wire.available()){ // why is Wire.available() necessary here? Using "nBytes" won't work.
    if(i==0){
      command = Wire.read();
    }else if(i==1){
      commandLength = Wire.read();
    }else if(i>1 && i<15){
      commandBuffer[i] = Wire.read();
    }else{
      // do nothing, prevent overflow.
    }
    i++;
  }
  commandBuffer[15] = '\0';
  printRequest = 1;
}

void requestEvent (){
  commandRequest = 1;
}

void loop() {
  if(printRequest){
    SerialUSB.print("command: ");
    SerialUSB.print(command);
    SerialUSB.print('\n');
    SerialUSB.print("length: ");
    SerialUSB.print(commandLength, DEC);
    SerialUSB.print('\n');
    SerialUSB.print("buffer: ");
    for(int i=0;i<16;i++){
      SerialUSB.print(commandBuffer[i]);
    }
    SerialUSB.print('\n');
    printRequest = 0;
  }
  if(commandRequest){
    SerialUSB.println("Commando Request received!");
    if(command==CMD_CTRL_RADIO){
      Wire.write('y');
      Wire.write('o');
      Wire.write('!');
    }else if(command==CMD_INFO){
      Wire.write('f');
      Wire.write('o');
      Wire.write('o');
    }else if(command==CMD_STATUS){
      Wire.write('4');
      Wire.write('5');
      Wire.write('6');
    }else{
      SerialUSB.println("No valid command!");
    }
    commandRequest = 0;
  }
}

Now to the master. its code can either send any message from the serial monitor to the I2C interface, or it requests a response of 3 bytes when I type in '1', '2', or '3' in the terminal. Additionally, some random payload is transmitted.

#include <Wire.h>

#define M0_I2C_SLAVE_ADDRESS 13
#define MAX_MESSAGE_LENGTH 16
enum {
    CMD_CTRL_RADIO = 169,
    CMD_INFO = 142,
    CMD_STATUS = 213
};

void sendCommand(uint8_t cmd, uint8_t len, uint8_t responseSize){
  Wire.beginTransmission(M0_I2C_SLAVE_ADDRESS);
  Wire.write(cmd);
  Wire.write(len);
  if(cmd==CMD_CTRL_RADIO){
      Wire.write('x');
      Wire.write('y');
      Wire.write('z');
    }else if(cmd==CMD_INFO){
      // no payload
    }else if(cmd==CMD_STATUS){
      Wire.write('B');
    }else{
      Serial.println("No valid command!");
    }
  Wire.endTransmission ();
  if (Wire.requestFrom(M0_I2C_SLAVE_ADDRESS, responseSize) == 0){
    Serial.println("Error");
  }else{
    char buffer[3];
    buffer[0] = Wire.read();
    buffer[1] = Wire.read();
    buffer[2] = Wire.read();
    Serial.print("Response from M0 board: ");
    Serial.print(buffer[0]);
    Serial.print(buffer[1]);
    Serial.print(buffer[2]);
    Serial.print('\n');
  }
}
  
void setup(){
  Wire.begin();
  Serial.begin(115200);
}

void loop(){
  if (Serial.available()){
      uint8_t txData[MAX_MESSAGE_LENGTH];
      size_t bytesRead = Serial.readBytes(txData, MAX_MESSAGE_LENGTH);
      if(txData[0]=='1'){
        sendCommand(CMD_CTRL_RADIO,3,3);
      }else if(txData[0]=='2'){
        sendCommand(CMD_INFO,0,3);
      }else if(txData[0]=='3'){
        sendCommand(CMD_STATUS,1,3);
      }else{
        int i = 0;
        Wire.beginTransmission(M0_I2C_SLAVE_ADDRESS);
        while (i < bytesRead) {
          Wire.write(txData[i]);
          i++;
        }
        Wire.write('\r');
        Wire.endTransmission();
      }
  }
}

Here is the output of my serial monitors when I send all three commands one after another:

On the slave's side, everything looks good. But the response at the master is not readable.

Issue 1
Using Wire.write() by the slave outside of the request handler seems broken. I can get valid responses when I place the writing process inside this callback.

Issue 2
Why not place the response code insithe this handler then? Well, there is something odd with the order in which the callbacks are executed. Whenever I send a command from the master, the handlers are called in this order:

receiveEvent -> requestEvent -> receiveEvent

I am not quite shure why the receiveEvent is called twice. To code I use inside the master is the same you find in many example code snipplets. The problem now is that inside the requestEvent, I do not have received all bytes yet. How to prevent this?

This would be expected when the M0 is not plugged in (unpowered). There are diodes on the input pins of almost all processors that prevent the pin from going more than about 0.7v above VCC, which in this case would be 0V since it is unpowered.

Nice to know, thank you.

If you receive a misfire with zero bytes, then you still do something in the onReceive handler.

Can you transfer a package of binary data of a fixed size ? That is easier.

volatile an array or a struct    // for example 18 bytes in size
volatile bool newData;

void receiveEvent( int howMany)
{
  if( howMany == 18)       // expecting 18 bytes, ignore everything else
  {
    copy the data to the array or a struct
    newData = true;
  }
}

Your receiveEvent() has:

  1. while while(0 < Wire.available()) Why would you use a while-statement ? How many times do you want to read the package of data ?
  2. 0 < Wire.available() Who on earth uses that ? If I say it out loud: "while zero is lower than the amount of available data". Huh ?
  3. printRequest = 1; Outside the while-statement ? Why ?
  4. #define MAX_MESSAGE_LENGTH 16 Your data length is 18 for the I2C bus. The sketch is easier if you transfer 18 bytes over the I2C bus and use 18 bytes in the sketch.

We don't use readable ASCII for the I2C bus.
A fixed size of binary data is achieved this way:

float xyzData[3];

or with a struct:

struct myStruct
{
  byte command,
  int16_t temperature,
  float xyzData[3],
}

Some hints:

You are storing slave input in commandBuffer[2] ff., but output from [0] so that the first two bytes are garbage.

In "else" you should read() excess bytes, else Wire.available() will never return zero.

Finally add the null byte at buffer[i], just after the last input byte.

Calling Wire.write() outside requestEvent() is undefined behaviour. Please follow the examples.

With the master code you can determine the number of received bytes and output just these. Asking for more bytes might trigger another requestFrom.

That's how the Wire library examples receive I2C input. Please direct improvements to the library maintainers. Apart from that I cannot see any error in that code - so far.

I know ! and it is really bad. I want domi1 to write normal code that he needs instead of copying code that he does not need.

I think that this makes more sense (not that I'm totally happy with it):

void receiveEvent(int nBytes)
{
  int i;

  if (nBytes > 2)          // at least 2
  {
    command = Wire.read();
    commandLength = Wire.read();
    if( commandLength < BUFFER_SIZE)    // reserve one for zero-terminator
    {
      for (i=0; i<commandLength; i++)   
      {
        commandBuffer[i] = Wire.read();
      }
      commandBuffer[i] = '\0';
      printRequest = 1;
    }
  }
}

[EDIT] Changed the code, added BUFFER_SIZE.

We are almost there.

We don't use readable ASCII for the I2C bus.
A fixed size of binary data is achieved this way:
[...]
or with a struct:

I like the idea of having a struct of fixed size, I somehow forgot that there are structs in C.

if( howMany == 18)       // expecting 18 bytes, ignore everything else

That's a very good check. I will use that too.

Calling Wire.write() outside requestEvent() is undefined behaviour. Please follow the examples.

I have a hard time understanding how an API call can be undefined. A callback/handler is a function call based on an interrupt; a function call is a jump instruction; why shouldn't it be possible to execute certain instructions outside of that?

Why would you use a while-statement ? How many times do you want to read the package of data ? [...] That's how the Wire library examples receive I2C input. [...] I know ! and it is really bad.

I think we all agree that the provided examples and built-in libraries might be not optimal. I mean, if you follow the API calls, you will eventually find out that any read operation on the I2C bus is blocking..

Anyway, back to the task. Since the nRF is a 32-Bit CPU, I defined my struct to fit within 32 bits, otherwise another 32 bits would be allocated.

typedef struct Command{
  uint8_t cmd : 8;
  uint32_t payload : 24; 
}Command;

Adjusted master code:

#include "Wire.h"

#define M0_I2C_SLAVE_ADDRESS 13
#define SERIAL_BUFFER_LEN 64
#define COMMAND_LEN 4

enum {
    CMD_TUNE = 169,
    CMD_SERVICE = 172,
    CMD_INFO = 142,
    CMD_STATUS = 213
};

typedef struct Command{
  uint8_t cmd : 8;
  uint32_t payload : 24; 
}Command;

void sendCommand(Command commandStruct, uint8_t responseSize){
  Wire.beginTransmission(M0_I2C_SLAVE_ADDRESS);
  Wire.write((byte *)&commandStruct, COMMAND_LEN);
  Wire.endTransmission ();
  if (Wire.requestFrom(M0_I2C_SLAVE_ADDRESS, responseSize) == 0){
    Serial.println("Error");
  }else{
    char buffer[3];
    buffer[0] = Wire.read();
    buffer[1] = Wire.read();
    buffer[2] = Wire.read();
    Serial.print("Response from M0 board: ");
    Serial.print(buffer[0]);
    Serial.print(buffer[1]);
    Serial.print(buffer[2]);
    Serial.print('\n');
  }
}
  
void setup(){
  Wire.begin();
  Serial.begin(115200);
}

void loop(){
  if (Serial.available()){
      uint8_t txData[SERIAL_BUFFER_LEN];
      size_t bytesRead = Serial.readBytes(txData, SERIAL_BUFFER_LEN);
      if(txData[0]=='1'){
        Command commandTune = {
          .cmd = CMD_TUNE,
          .payload = 107500
        };
        sendCommand(commandTune,3);
      }else if(txData[0]=='2'){
        Command commandService = {
          .cmd = CMD_SERVICE,
          .payload = 14
        };
        sendCommand(commandService,3);
      }else if(txData[0]=='3'){
        Command commandInfo = {
          .cmd = CMD_INFO,
          .payload = 0
        };
        sendCommand(commandInfo,3);
      }else{
        // do nothing
      }
  }
}

Slave code:

#include "Wire.h"

#define M0_I2C_SLAVE_ADDRESS 13
#define COMMAND_LEN 4

enum {
    CMD_TUNE = 169,
    CMD_SERVICE = 172,
    CMD_INFO = 142,
    CMD_STATUS = 213
};

typedef struct Command{
  uint8_t cmd : 8;
  uint32_t payload : 24; 
}Command;

volatile uint8_t printRequest = 0;
volatile Command bufferedCommand = {.cmd = 0, .payload = 0};

void setup() {
  Wire.begin(M0_I2C_SLAVE_ADDRESS);
  Wire.onReceive(receiveEvent);
  Wire.onRequest(requestEvent);
  SerialUSB.begin(115200); 
}

void receiveEvent(int nBytes){
  //SerialUSB.println("Callback receiveEvent()!");  // still gets called twice with each transmission
  if(nBytes != COMMAND_LEN){
    SerialUSB.print("Length error: ");
    SerialUSB.println(nBytes,DEC);
  }else{
    byte cmdBuffer[COMMAND_LEN];
    for (uint8_t i=0; i<COMMAND_LEN; i++){
      cmdBuffer[i] = Wire.read();
    }
    bufferedCommand.cmd = cmdBuffer[0];
    bufferedCommand.payload = (uint32_t) cmdBuffer[3] << 16;
    bufferedCommand.payload |= (uint32_t) cmdBuffer[2] << 8;
    bufferedCommand.payload |= (uint32_t) cmdBuffer[1];
    printRequest = 1;
  }
}

void requestEvent (){
  if(bufferedCommand.cmd==CMD_TUNE){
      Wire.write('y');
      Wire.write('o');
      Wire.write('!');
  }else if(bufferedCommand.cmd==CMD_SERVICE){
      Wire.write('f');
      Wire.write('o');
      Wire.write('o');
  }else if(bufferedCommand.cmd==CMD_INFO){
      Wire.write('4');
      Wire.write('5');
      Wire.write('6');
  }else if(bufferedCommand.cmd==CMD_STATUS){
      Wire.write('x');
      Wire.write('y');
      Wire.write('z');
  }else{
      SerialUSB.println("No valid command!");
  }
}

void loop() {
  if(printRequest){
    SerialUSB.print("command: ");
    SerialUSB.print(bufferedCommand.cmd, DEC);
    SerialUSB.print('\n');
    SerialUSB.print("Payload: ");
    SerialUSB.println(bufferedCommand.payload, DEC);
    printRequest = 0;
  }
}

This works quite well expect for the fact that the callback receiveEvent() still gets called twice at each transmission for some reason. This makes the behaviour sometimes a bit buggy. In one off the calls, nBytes is equal to COMMAND_LEN and everything works as expected. In the other call, nBytes is equal to zero and the error message pops up.

Yes, I called that a "misfire". That is a bug. For safety you have to filter those out anyway.

You are very polite.

Please don't try to be smart and think ahead of a problem that does not exist. Always follow the KISS principle.

You do this:

typedef struct Command{
  uint8_t cmd : 8;
  uint32_t payload : 24; 
}Command;

Every Arduino board puts the struct elements at byte order to be compatible with all the other Arduino boards. Please don't use bit definitions, use normal uint8_t (or byte) or an array of bytes or other variables.
By the way, 24 bits is only 3 bytes. Your struct uses 5 bytes and only 4 are used. That is what happens if you forget about the KISS principle :wink:

There is a timing thing.
When a fast Master does a I2C write and a I2C read session, then a slower Slave might not be ready. Especially the slower boards such as a Arduino Uno needs "time to breathe".

Don't be surprised if things work better with a delay:

  Wire.beginTransmission(M0_I2C_SLAVE_ADDRESS);
  Wire.write((byte *)&commandStruct, COMMAND_LEN);
  int error = Wire.endTransmission ();

  delay(1);                            // give Slave "time to breathe"

  if (Wire.requestFrom(M0_I2C_SLAVE_ADDRESS, responseSize) == responseSize)  // all okay ?
  {
    Wire.readBytes( buffer, responseSize);       // <-- this is weird, but allowed
  }

Here is a puzzle for you:
Look at the loop() of the Slave. Suppose that some time ago the printRequest was set to 1 and the loop() detects that. Then suddenly new data arrives via the I2C bus while the loop() is still busy between the if(printrequest){ and the printRequest=0.
Then you have half the old data and half the new data.
Solution: make some kind of handshake; or make of copy of the struct and clear the flag with the interrupts turned off; or make an error counter in the onReceive handler.

...or some other event. After the callback function returns the caller can do everything, e.g. flush remaining bytes in the input buffer.

I don't think so. Wire.available() can work as documented only with an input buffer.

On a 32 bit machine all multi-byte variables should be aligned to their natural boundaries. It will cost some time to access a long variable not aligned to a 32 bit boundary. Also packing a 32 bit byte type into 24 bits is at least questionable. Did you check the struct size and compiler warnings?

Thanks for the hints, I will implement them.

Disagree. If I call sizeof(myStruct) then it returns 4. If I define the struct without bit field (with a uint8_t and uint32_t inside), then it returns 8 (because it is a 32-bit CPU I guess?).

But rethinking about it, It would have been better to define the payload as byte[3] array; that would be less confusing that bit fields I guess. And since it is binary data anyway, it does not matter.

Thanks, so the compiler packs the 24 bit into the struct. The structure should be byte aligned for an Arduino board. Perhaps that is a bug. I would use byte[4] for the whole package anyway.

Be careful with the volatile variables, you should not use them directly outside of the interrupt routines because an interrupt may occur between bytes of a multi-byte variable. Common practice is to temporarily disable interrupts while copying the volatile to another variable.

In the requestEvent() handler, only use a single Wire.write (not sure if this is still needed, at some point in time the Wire library supposedly had problems with multiple Wire.writes in requestEvent).

NEVER print to Serial in an interrupt service routine. I'm not familiar with SerialUSB, but Serial uses interrupts and interrupts are disabled inside an interrupt service routine, so potentially a full send buffer would block the code forever.

Try this code and see if it works on your hardware, I do not have a 32-bit processor to test it on:

master code:

#include "Wire.h"

#define M0_I2C_SLAVE_ADDRESS 13
#define SERIAL_BUFFER_LEN 64
#define COMMAND_LEN 4

enum {
  CMD_TUNE = 169,
  CMD_SERVICE = 172,
  CMD_INFO = 142,
  CMD_STATUS = 213
};

typedef struct __attribute__ ((packed))Command {
  uint8_t cmd;
  uint8_t payload[3];
} Command;

void sendCommand(Command commandStruct, uint8_t responseSize) {
  Wire.beginTransmission(M0_I2C_SLAVE_ADDRESS);
  Wire.write((byte *)&commandStruct, COMMAND_LEN);
  Wire.endTransmission ();
  if (Wire.requestFrom(M0_I2C_SLAVE_ADDRESS, responseSize) == 0) {
    Serial.println("Error");
  } else {
    char buffer[3];
    buffer[0] = Wire.read();
    buffer[1] = Wire.read();
    buffer[2] = Wire.read();
    Serial.print("Response from M0 board: ");
    Serial.print(buffer[0]);
    Serial.print(buffer[1]);
    Serial.print(buffer[2]);
    Serial.print('\n');
  }
}

void setup() {
  Wire.begin();
  Serial.begin(115200);
}

void loop() {
  if (Serial.available()) {
    uint8_t txData[SERIAL_BUFFER_LEN];
    size_t bytesRead = Serial.readBytes(txData, SERIAL_BUFFER_LEN);
    if (txData[0] == '1') {
      Command commandTune = {
        .cmd = CMD_TUNE,
        .payload = {107500 % 256, (107500 / 256) % 256, 107500 / (256l * 256)}
      };
      sendCommand(commandTune, 3);
    } else if (txData[0] == '2') {
      Command commandService = {
        .cmd = CMD_SERVICE,
        .payload = {14, 0, 0}
      };
      sendCommand(commandService, 3);
    } else if (txData[0] == '3') {
      Command commandInfo = {
        .cmd = CMD_INFO,
        .payload = {0, 0, 0}
      };
      sendCommand(commandInfo, 3);
    } else {
      // do nothing
    }
  }
}

slave code:

//for compiling on a board without SerialUSB
#ifndef SerialUSB
#define SerialUSB Serial
#endif

#include "Wire.h"

#define M0_I2C_SLAVE_ADDRESS 13
#define COMMAND_LEN 4

enum {
  CMD_TUNE = 169,
  CMD_SERVICE = 172,
  CMD_INFO = 142,
  CMD_STATUS = 213
};

typedef struct __attribute__ ((packed)) Command {
  uint8_t cmd;
  uint8_t payload[3];
} Command;

volatile bool printRequest = false;
volatile bool lengthError = false;
volatile int  lengthErrorBytes;
volatile bool noValidCommand = false;
volatile byte receiveBuffer[sizeof(Command)] = {0};
Command bufferedCommand = {.cmd = 0, .payload = {0, 0, 0}};

void setup() {
  Wire.begin(M0_I2C_SLAVE_ADDRESS);
  Wire.onReceive(receiveEvent);
  Wire.onRequest(requestEvent);
  SerialUSB.begin(115200);
}

void receiveEvent(int nBytes) {
  if (nBytes != COMMAND_LEN) {
    //flush buffer
    for (int i = 0; i < nBytes; i++) {
      Wire.read();
    }
    lengthError = true;
    lengthErrorBytes = nBytes;
  } else {
    for (uint8_t i = 0; i < COMMAND_LEN; i++) {
      receiveBuffer[i] = Wire.read();
    }
    printRequest = true;
  }
}

void requestEvent () {
  //only use a single Wire.write within requestEvent
  if (bufferedCommand.cmd == CMD_TUNE) {
    byte data[3] = {'y', 'o', '!'};
    Wire.write(data, 3);
  } else if (bufferedCommand.cmd == CMD_SERVICE) {
    byte data[3] = {'f', 'o', 'o'};
    Wire.write(data, 3);
  } else if (bufferedCommand.cmd == CMD_INFO) {
    byte data[3] = {'4', '5', '6'};
    Wire.write(data, 3);
  } else if (bufferedCommand.cmd == CMD_STATUS) {
    byte data[3] = {'x', 'y', 'z'};
    Wire.write(data, 3);
  } else {
    //never use print within an interrupt handler
    noValidCommand = true;
  }
}

void loop() {
  if (printRequest) {
    //copy receive buffer to bufferedCommand
    //note - memcpy does not like volatile
    byte* ptr = (byte*)(&bufferedCommand);
    noInterrupts();  //disable interrupts while accessing volatile variables
    for (size_t i = 0; i < sizeof(Command); i++) {
      *ptr++ = receiveBuffer[i];
    }
    printRequest = false;
    interrupts();
    SerialUSB.print("command: ");
    SerialUSB.print(bufferedCommand.cmd, DEC);
    SerialUSB.print('\n');
    SerialUSB.print("Payload: ");
    SerialUSB.println(bufferedCommand.payload[0] + bufferedCommand.payload[1] * 256l + bufferedCommand.payload[2] * 256l * 256, DEC);
  }

  if (lengthError){
    lengthError = false;
    SerialUSB.print("Length error: ");
    SerialUSB.println(lengthErrorBytes, DEC);
  }
  
  if (noValidCommand) {
    noValidCommand = false;
    SerialUSB.println("No valid command!");
  }
}