Wire.onRequest causes slave arduino to freeze??

Hey guys,

I’m quite new to arduino and have slowly been trying build my programming knowledge. To do this I have undertaken the quite long (and tough) project of building a UAV. I have had several tough areas but have managed to google my way around the problems and learn a little bit a long the way. Until now!!

Now the whole idea behind the project is to split the the workload between two microcontrollers. The attitude sensing (IMU) and PID loops are controlled by a atmega328 which is fed setpoints from the flight management controlled (an atmega1280). The 1280 does all the heavy lifting of calculating distances and vectors based on waypoint data and GPS current position, which then calculates a desired heading and pitch angle (setpoints) which are sent to the 328.

The idea is to eventually have this all integrated into one board, so I planned to use I2C for the communications. The IMU (atmega328) is supposed to run as quickly as possible so that the PID loops can respond appropriately and such I set it up as a slave so that when the flight management controller (atmega1280) requests the current attitude data, it sends it, and is not held up waiting for the 1280 to respond.

So far I have managed to establish communications between the two controllers but I am unable to get the slave to respond to the onRequest function correctly. The master (atmega1280) requests the data and the slave (atmega328) initally sends some data that corresponds to the correct pitch, roll, and heading data but then stops after 6 lines. The 6 lines of data received do not appear to be updated as the position of the IMU changes as well. If I setup the I2C communications any other way than using Wire.onRequest the data gets sent flawlessly but slows down the cycle time considerably.

I have included my code for both the master and slave (please excuse my inefficient code, I’m still learning!) so that maybe that could be of some help to get to the bottom of my problem?

Slave (atmega 328 IMU):

#include <Wire.h>
#include <HMC5883L.h>
#include <EEPROM.h>
#include "EEPROMAnything.h"
#include <Servo.h>

Servo myservo;

HMC5883L compass;

int Xaccel = 0;
int Yaccel = 1;
int Zaccel = 2;

int y_accel = 0;
int z_accel = 0;
int x_accel = 0;

float ACC_X;
float ACC_Y;
float ACC_Z;

float GYR_X;
float GYR_Y;
float GYR_Z;

float ACC_PITCH;
float ACC_ROLL;

float ROLL;
float PITCH;
float HDG;

uint8_t data[6];
uint8_t sending[6];

char test;

int time;
int newtime;
int dt;

float rollRadians;
float pitchRadians;

float filt = 0.98;
float filt2 = 0.4;

int X_ACC_CAL;
int Y_ACC_CAL;
int X_ACC_CAL_VAL;
int Y_ACC_CAL_VAL;

int X_GYR_CAL;
int Y_GYR_CAL;
int Z_GYR_CAL;
float X_GYR_CAL_VAL;
float Y_GYR_CAL_VAL;
float Z_GYR_CAL_VAL;

byte testies;

boolean CALIBRATION = false;


void setup(){

  Wire.begin(0x02);
  analogReference(EXTERNAL);
  initGyro();
  compass = HMC5883L(); // Construct a new HMC5883 compass.

  delay(10);

  compass.SetScale(1.3); // Set the scale of the compass.
  compass.SetMeasurementMode(Measurement_Continuous); // Set the measurement mode to Continuous

  digitalWrite(13, HIGH);  
  delay(2000);

  if(digitalRead(8) == HIGH){
    calibrate(); 
    CALIBRATION == false;
    
  }
  digitalWrite(13, LOW);
  Wire.onRequest(requestEvent);
}

void loop(){
  
  EEPROM_readAnything(0, X_ACC_CAL_VAL);
  EEPROM_readAnything(2, Y_ACC_CAL_VAL);

  EEPROM_readAnything(4, X_GYR_CAL_VAL);
  EEPROM_readAnything(10, Y_GYR_CAL_VAL);
  EEPROM_readAnything(8, X_GYR_CAL_VAL);

  MagnetometerScaled MAG_RDG = compass.ReadScaledAxis();

  time = micros(); 

  ACC_X = get_X_Accel()-X_ACC_CAL_VAL;
  ACC_Y = get_Y_Accel()-Y_ACC_CAL_VAL;
  ACC_Z = get_Z_Accel()-493;

  GYR_X = (get_Gyro_Y_Data()/100)-(Y_GYR_CAL_VAL/100);
  GYR_Y = -(get_Gyro_X_Data()/100)-(X_GYR_CAL_VAL/100);
  GYR_Z = -(get_Gyro_Z_Data()/100)-(Z_GYR_CAL_VAL/100);


  ACC_ROLL = atan(ACC_X/ACC_Z)/PI*180;
  ACC_PITCH = atan(ACC_Y/ACC_Z)/PI*180;

  ROLL = (filt*(ROLL+(GYR_X*dt*0.00001)))+((1-filt)*ACC_ROLL);
  PITCH = (filt*(PITCH+(GYR_Y*dt*0.00001)))+((1-filt)*ACC_PITCH);
  HDG = (filt2*(HDG+(GYR_Z*dt*0.00001)))+((1-filt2)*TILTED_COMPASS(MAG_RDG));
  
 
  
 data[0] = lowByte(int(ROLL*10)+900);
 data[1] = highByte(int(ROLL*10)+900);
 data[2] = lowByte(int(PITCH*10)+900);
 data[3] = highByte(int(PITCH*10)+900);
 data[4] = lowByte(int(HDG));
 data[5] = highByte(int(HDG));
   

  newtime = micros();
  dt = newtime - time; 
  
}

void requestEvent(){
 
  //Wire.beginTransmission(0x01);
  Wire.send(data, 6);
 // Wire.endTransmission();
}

Master (atmega1280 flight management):

#include <Wire.h>

int c1, c2, c3, c4, c5, c6;

void setup()
{
  Wire.begin(1);        // join i2c bus (address optional for master)
  Serial.begin(9600);  // start serial for output
 // Wire.onReceive(receiveEvent);
}

void loop()
{
  Wire.requestFrom(2, 6);
  int cnt = 0;
  while(Wire.available())    // slave may send less than requested
  {
    cnt++;
    if (cnt == 1){
      c1 = 0;
      c1 = Wire.receive(); // receive a byte as character
     } 
   if (cnt == 2) {
      c2 = 0;
      c2 = (Wire.receive())*256;
    }
    
   if (cnt == 3) {
    c3 = 0;
    c3 = Wire.receive();
  }
  
   if (cnt == 4) {
      c4 = 0;
      c4 = (Wire.receive())*256;
    }
    
     if (cnt == 5) {
    c5 = 0;
    c5 = Wire.receive();
  }
  
   if (cnt == 6) {
      c6 = 0;
      c6 = (Wire.receive())*256;
    }
  Serial.print(((c1+c2)-900)*0.1, 1);
  Serial.print(", ");
  Serial.print(((c3+c4)-900)*0.1, 1);
  Serial.print(", ");
  Serial.print((c5+c6), DEC);
  Serial.println(" ");
  
}
}

I see at least one smiley icon embedded in your code posting, because you posted the code inside a text (quote) box. Code is best posted using the # icon as that will preserve proper code formatting.

Lefty

While you are doing that:

http://gammon.com.au/i2c

Thanks guys, I've edited my post to hopefully make things a bit easier.

Nick, thanks for the link. Very in depth. I'll mull that over for a while and see if I can find anything.

The master (atmega1280) requests the data and the slave (atmega328) initally sends some data that corresponds to the correct pitch, roll, and heading data but then stops after 6 lines. The 6 lines of data received do not appear to be updated as the position of the IMU changes as well.

It's only sending 6 bytes because that's all your requesting by using requestFrom(2,6). Another thing to keep in mind is that the slave code doesn't take into account sending fully updated results. If the request comes from the master in the middle of storing your data it will send a mix of new and old data. It may be overkill for what you're doing but I wrote a step by step guide on how to make a slave device. There's also a small section on configuring a digital pin to use as an external interrupt to signal the master when new data is available (similar to most I2C devices on the market). http://dsscircuits.com/articles/arduino-i2c-slave-guide.html

It's only sending 6 bytes because that's all your requesting by using

Does the request for 6 bytes not correspond to requesting the full array ( data[] ) of 6 bytes once? Or do I only need to request 1 byte for it to send the entire 6 byte array? And shouldn't this keep on cycling through each time it runs through loop(), so that each time it prints the current values in the array?

That's very interesting. Something I just realised looking back on the code in my first post is that there is actually a delay(1000) missing from the master (atmega1280) code to simulate the extra work it will be doing with GPS position calculations. I was thinking about doing something similar using a pin to signal when data is ready/requested but I thought there must be a way to get onRequest to work for my application?

Thanks very much for your help!

Does the request for 6 bytes not correspond to requesting the full array ( data ) of 6 bytes once? Or do I only need to request 1 byte for it to send the entire 6 byte array? And shouldn’t this keep on cycling through each time it runs through loop(), so that each time it prints the current values in the array?

It will send all six bytes. I pointed this out because you appeared to be wondering why it only sent 6 bytes. Your code looks OK to me and should work…sort of. If you read over Nick’s page he shows you how to have the Arduino work as both master and slave so they can communicate back and forth with each other. You’re “kind of” doing that with your setup with one difference your primary slave device (328) is also acting as a master communicating with your other sensors. I don’t know what would happen if your 328 is talking to the sensors and the request comes across from the master.

Huh?

 Wire.requestFrom(2, 6);
  int cnt = 0;
  while(Wire.available())    // slave may send less than requested
  {
    cnt++;
    if (cnt == 1){
      c1 = 0;
      c1 = Wire.receive(); // receive a byte as character
     } 
   if (cnt == 2) {
      c2 = 0;
      c2 = (Wire.receive())*256;
    }
    
   if (cnt == 3) {
    c3 = 0;
    c3 = Wire.receive();
  }
  
   if (cnt == 4) {
      c4 = 0;
      c4 = (Wire.receive())*256;
    }
    
     if (cnt == 5) {
    c5 = 0;
    c5 = Wire.receive();
  }
  
   if (cnt == 6) {
      c6 = 0;
      c6 = (Wire.receive())*256;
    }

Why clear a variable to zero each time when you overwrite it on the very next line?

Wouldn’t this be easier? …

#include <Wire.h>

void setup()
{
  Wire.begin ();        // join i2c bus (address optional for master)
  Serial.begin(9600);  // start serial for output
}

void loop()
{
  byte data [6];
  
  if (Wire.requestFrom (2, 6) != 6)
    return;  // didn't get 6 bytes
    
  for (byte i = 0; i < 6; i++)
    data [i] = Wire.receive ();
  
  int roll  = data[0] | (data [1] << 8);
  int pitch = data[2] | (data [3] << 8);
  int hdg   = data[4] | (data [5] << 8);

  Serial.print((roll-900)*0.1, 1);
  Serial.print(", ");
  Serial.print((pitch-900)*0.1, 1);
  Serial.print(", ");
  Serial.print(hdg, DEC);
  Serial.println(" ");
 
}

Wouldn't this be easier? ...

Far easier! I was clearing the variable to zero as I thought originally (and incorrectly) that was the reason the data was not being updated. I apologise for my inefficient code, I was just trying to see if I could get the data between the two arduino's.

If the master were to request data from the slave in middle of it writing the values to the array data[] could this possibly be the reason it would hang? Although at one point I had the array data[] being written to in the onRequest handler so it wouldn't be interrupted, but that didn't seem to make a difference?

I don't know what would happen if your 328 is talking to the sensors and the request comes across from the master.

This was something I had considered. Is it possible there is a conflict due to the 328 trying to act as a master and a slave? Would the i2c hardware in the chip not complete the first operation of receiving data before signalling the onRequest interrupt?

chrisbchips: If the master were to request data from the slave in middle of it writing the values to the array data[] could this possibly be the reason it would hang?

No, I don't think so. However you might protect the values like this:

noInterrupts ();
data[0] = lowByte(int(ROLL*10)+900);
data[1] = highByte(int(ROLL*10)+900);
data[2] = lowByte(int(PITCH*10)+900);
data[3] = highByte(int(PITCH*10)+900);
data[4] = lowByte(int(HDG));
data[5] = highByte(int(HDG));
interrupts ();

That way the 6 bytes are updated atomically.

Is it possible there is a conflict due to the 328 trying to act as a master and a slave?

There is a potential for conflict with multiple masters on the I2C bus. It is supposed to handle it, but conceivably the request for data will occur while you are reading the device at the slave end.

I don't know about it hanging though. Maybe post the updated code for both master and slave.

Also if they are all nearby, and speed is the essence, use SPI instead. That's somewhat faster and you can avoid contention, at least between the two Atmegas.

Ok, so here is what I have implemented (Thanks very much for your help Nick!) -

Slave (328):

#include <Wire.h>
#include <HMC5883L.h>
#include <EEPROM.h>
#include "EEPROMAnything.h"
#include <Servo.h>

Servo myservo;

HMC5883L compass;

int Xaccel = 0;
int Yaccel = 1;
int Zaccel = 2;

int y_accel = 0;
int z_accel = 0;
int x_accel = 0;

float ACC_X;
float ACC_Y;
float ACC_Z;

float GYR_X;
float GYR_Y;
float GYR_Z;

float ACC_PITCH;
float ACC_ROLL;

float ROLL;
float PITCH;
float HDG;

uint8_t data[6];
uint8_t sending[6];

char test;

int time;
int newtime;
int dt;

float rollRadians;
float pitchRadians;

float filt = 0.98;
float filt2 = 0.4;

int X_ACC_CAL;
int Y_ACC_CAL;
int X_ACC_CAL_VAL;
int Y_ACC_CAL_VAL;

int X_GYR_CAL;
int Y_GYR_CAL;
int Z_GYR_CAL;
float X_GYR_CAL_VAL;
float Y_GYR_CAL_VAL;
float Z_GYR_CAL_VAL;

byte testies;

boolean CALIBRATION = false;


void setup(){

  Wire.begin(0x02);
  analogReference(EXTERNAL);
  initGyro();
  compass = HMC5883L(); // Construct a new HMC5883 compass.

  delay(10);

  compass.SetScale(1.3); // Set the scale of the compass.
  compass.SetMeasurementMode(Measurement_Continuous); // Set the measurement mode to Continuous

  digitalWrite(13, HIGH);  
  delay(2000);

  if(digitalRead(8) == HIGH){
    calibrate(); 
    CALIBRATION == false;
    
  }
  digitalWrite(13, LOW);
  Wire.onRequest(requestEvent);
}

void loop(){
  
  
    

  EEPROM_readAnything(0, X_ACC_CAL_VAL);
  EEPROM_readAnything(2, Y_ACC_CAL_VAL);

  EEPROM_readAnything(4, X_GYR_CAL_VAL);
  EEPROM_readAnything(10, Y_GYR_CAL_VAL);
  EEPROM_readAnything(8, X_GYR_CAL_VAL);

  MagnetometerScaled MAG_RDG = compass.ReadScaledAxis();

  time = micros(); 

  ACC_X = get_X_Accel()-X_ACC_CAL_VAL;
  ACC_Y = get_Y_Accel()-Y_ACC_CAL_VAL;
  ACC_Z = get_Z_Accel()-493;

  GYR_X = (get_Gyro_Y_Data()/100)-(Y_GYR_CAL_VAL/100);
  GYR_Y = -(get_Gyro_X_Data()/100)-(X_GYR_CAL_VAL/100);
  GYR_Z = -(get_Gyro_Z_Data()/100)-(Z_GYR_CAL_VAL/100);


  ACC_ROLL = atan(ACC_X/ACC_Z)/PI*180;
  ACC_PITCH = atan(ACC_Y/ACC_Z)/PI*180;

  ROLL = (filt*(ROLL+(GYR_X*dt*0.00001)))+((1-filt)*ACC_ROLL);
  PITCH = (filt*(PITCH+(GYR_Y*dt*0.00001)))+((1-filt)*ACC_PITCH);
  HDG = (filt2*(HDG+(GYR_Z*dt*0.00001)))+((1-filt2)*TILTED_COMPASS(MAG_RDG));
  
 
 
 noInterrupts();
 data[0] = lowByte(int(ROLL*10)+900);
 data[1] = highByte(int(ROLL*10)+900);
 data[2] = lowByte(int(PITCH*10)+900);
 data[3] = highByte(int(PITCH*10)+900);
 data[4] = lowByte(int(HDG));
 data[5] = highByte(int(HDG));
 interrupts(); 
   




  newtime = micros();
  dt = newtime - time; 
  
}


void requestEvent(){
  digitalWrite(13, HIGH); 
  Wire.send(data, 6);
  digitalWrite(13, LOW);
  
}

Master (1280):

#include <Wire.h>

void setup()
{
  Wire.begin (1);        // join i2c bus (address optional for master)
  Serial.begin(9600);  // start serial for output
}

void loop()
{
  delay(300);
  byte data [6];
  
  if (Wire.requestFrom (2, 6) != 6)
    return;  // didn't get 6 bytes
    
  for (byte i = 0; i < 6; i++)
    data [i] = Wire.receive ();
  
  int roll  = data[0] | (data [1] << 8);
  int pitch = data[2] | (data [3] << 8);
  int hdg   = data[4] | (data [5] << 8);

  Serial.print((roll-900)*0.1, 1);
  Serial.print(", ");
  Serial.print((pitch-900)*0.1, 1);
  Serial.print(", ");
  Serial.print(hdg, DEC);
  Serial.println(" ");
 
}

Result is that there is no more hanging up, the code runs smoothly. But the data is not updated??! I’m completely perplexed!

Thanks for all your help already guys!

Is the slave toggling pin 13 to indicate it got the request?

Does the master print the message about the values?

Try making data volatile, the compiler may optimize changes away:

volatile uint8_t data[6];

The slave is toggling pin13 (the build in LED) so that I can see that the onRequest handler is being executed (and it is). The master prints the values, but they don't update as the IMU (slave) changes attitude.

I just tried changing data[] to a volatile uint8_t but it wouldn't compile. It seems the Wire library is only setup to handle uint8_t's?

I just tried changing data[] to a volatile uint8_t but it wouldn't compile.

On the master. That's the one that is apparently not getting the data.

Er, I withdraw that. You'll have to make it work on the slave.

Like this:

void requestEvent(){
  digitalWrite(13, HIGH); 
  Wire.send((byte *) data, 6);
  digitalWrite(13, LOW);  
}

Or:

void requestEvent(){
byte data [6];

  digitalWrite(13, HIGH); 
  data[0] = lowByte(int(ROLL*10)+900);
  data[1] = highByte(int(ROLL*10)+900);
  data[2] = lowByte(int(PITCH*10)+900);
  data[3] = highByte(int(PITCH*10)+900);
  data[4] = lowByte(int(HDG));
  data[5] = highByte(int(HDG));
  Wire.send(data, 6);
  digitalWrite(13, LOW);  
}

And make ROLL, PITCH and HDG volatile.

Oh, and lose the caps. All-caps are for constants, by convention. If they are changing, they aren't constants.

Thanks for your help Nick, very much appreciated!

Still no luck. Things that have been implemented:

  • data[] is being written to in the onRequest handler.
  • pitch, roll, hdg are all volatiles (and caps are gone).
  • data is changed to a volatile just for good measure.

Result is much the same. The position is updated each time the IMU (slave 328) is restarted, and it sends that updated position but then doesn't update again while it's running???