arduino i2c doesn't work consistantly

I have been working on a car and it has four sensors for the wheels. The sensors are connected to an Arduino pro mini which uses i2c to talk to the master, the Arduino Uno. Every so often the sensors don’t send any data over even though the wheels are spinning. (the code may not be the most efficient but I am just testing at the moment.

this is the master code:

#include <Wire.h>
#include <Adafruit_MotorShield.h>
static byte rotationFR;
  static byte rotationFL;
  static byte rotationBR;
 static byte rotationBL;
 unsigned long StartTime = 0;
 
void parseNum(int howMany)
{
  byte rotation[4];
  byte index = 0;
while(Wire.available() > 0 && index < 4)
{
  rotation[index] = Wire.read();
  index++;
}
rotationFR =rotation[0];
  rotationFL =rotation[1];
  rotationBR =rotation[2];
  rotationBL = rotation[3];

 Serial.print("Motor Speed FR: "); 
  Serial.print(rotationFR);  
  Serial.print(" RPM - "); 
  Serial.print("Motor Speed FL: "); 
  
 Serial.print(rotationFL);  
  Serial.println(" RPM"); 
  Serial.print("Motor Speed BR: "); 
  
 Serial.print(rotationBR);  
 Serial.print(" RPM - "); 
  Serial.print("Motor Speed BL: "); 
  
  Serial.print(rotationBL);  
  Serial.println(" RPM"); 
}

Adafruit_MotorShield AFMS = Adafruit_MotorShield(); 
Adafruit_DCMotor *FRight = AFMS.getMotor(3);
Adafruit_DCMotor *FLeft = AFMS.getMotor(4);
Adafruit_DCMotor *BLeft = AFMS.getMotor(1);
Adafruit_DCMotor *BRight = AFMS.getMotor(2);

void setup() {
  Wire.begin();
  
Serial.begin(9600);
AFMS.begin();


FLeft->setSpeed(150);
FRight->setSpeed(150);
BLeft->setSpeed(150);
BRight->setSpeed(150);

}

void loop(){ 

FRight->run(FORWARD);
FLeft->run(FORWARD);
BRight->run(FORWARD);
BLeft->run(FORWARD);
if(millis() >= StartTime){ //requests the data every second
  StartTime = StartTime +1000;
   Wire.requestFrom(1,4);
 parseNum(4);
}


}

This is the slave code

#include <Wire.h>
#include "TimerOne.h"
#include "PinChangeInt.h"
const byte FRMotor =4;
const byte FLMotor =5;
const byte BRMotor =6;
const byte BLMotor =7;
unsigned int counterFR = 0;
unsigned int counterFL = 0;
unsigned int counterBR = 0;
unsigned int counterBL = 0;
static byte rotationFR = 0;
static byte rotationFL = 0;
static byte rotationBR = 0;
static byte rotationBL = 0;
float diskslots = 20;
void ISR_countFR()  
{
  counterFR++;
} 
void ISR_countFL()  
{
  counterFL++; 
} 
void ISR_countBR()  
{
  counterBR++;
} 
void ISR_countBL()  
{
  counterBL++; 
} 
// TimerOne ISR

void ISR_timerone()
{
  Timer1.detachInterrupt();  // Stop the timer
  rotationFR = (counterFR / diskslots) * 60.00/2; 
  counterFR = 0; 
  rotationFL = (counterFL / diskslots) * 60.00/2; 
  counterFL = 0;  
  rotationBR = (counterBR / diskslots) * 60.00/2;  
  counterBR = 0; 
  rotationBL = (counterBL / diskslots) * 60.00/2;  
  counterBL = 0;  
  Timer1.attachInterrupt( ISR_timerone );  // Enable the timer
}
void updateRot(){
  byte sendData[4]; 
  sendData[0]=rotationFR;
  sendData[1]=rotationFL;
 sendData[2]=rotationBR;
 sendData[3]=rotationBL;
  Wire.write(sendData,4);
  
}

void setup() {
Serial.begin(9600);
Wire.begin(1);
Timer1.initialize(1000000); // set timer for 1sec 
  attachPinChangeInterrupt(FRMotor, ISR_countFR,CHANGE);
  attachPinChangeInterrupt(FLMotor, ISR_countFL,CHANGE);
  attachPinChangeInterrupt(BRMotor, ISR_countBR,CHANGE);
  attachPinChangeInterrupt(BLMotor, ISR_countBL,CHANGE);
  Timer1.attachInterrupt( ISR_timerone ); // Enable the timer
  

}

void loop(){ 
Wire.onRequest(updateRot);
}

this is the output I get from running for a couple seconds(the wheels are turning at all times and never stop):

Motor Speed FR: 18 RPM - Motor Speed FL: 25 RPM
Motor Speed BR: 31 RPM - Motor Speed BL: 21 RPM
Motor Speed FR: 39 RPM - Motor Speed FL: 57 RPM
Motor Speed BR: 96 RPM - Motor Speed BL: 69 RPM
Motor Speed FR: 39 RPM - Motor Speed FL: 57 RPM
Motor Speed BR: 96 RPM - Motor Speed BL: 69 RPM
Motor Speed FR: 39 RPM - Motor Speed FL: 57 RPM
Motor Speed BR: 96 RPM - Motor Speed BL: 69 RPM
Motor Speed FR: 39 RPM - Motor Speed FL: 57 RPM
Motor Speed BR: 96 RPM - Motor Speed BL: 69 RPM
Motor Speed FR: 39 RPM - Motor Speed FL: 57 RPM
Motor Speed BR: 96 RPM - Motor Speed BL: 69 RPM
Motor Speed FR: 39 RPM - Motor Speed FL: 57 RPM
Motor Speed BR: 96 RPM - Motor Speed BL: 69 RPM
Motor Speed FR: 39 RPM - Motor Speed FL: 57 RPM
Motor Speed BR: 96 RPM - Motor Speed BL: 69 RPM
Motor Speed FR: 0 RPM - Motor Speed FL: 0 RPM
Motor Speed BR: 0 RPM - Motor Speed BL: 0 RPM
Motor Speed FR: 97 RPM - Motor Speed FL: 91 RPM
Motor Speed BR: 103 RPM - Motor Speed BL: 123 RPM
Motor Speed FR: 97 RPM - Motor Speed FL: 91 RPM
Motor Speed BR: 103 RPM - Motor Speed BL: 123 RPM
Motor Speed FR: 0 RPM - Motor Speed FL: 0 RPM
Motor Speed BR: 0 RPM - Motor Speed BL: 0 RPM
Motor Speed FR: 97 RPM - Motor Speed FL: 91 RPM
Motor Speed BR: 103 RPM - Motor Speed BL: 141 RPM
Motor Speed FR: 97 RPM - Motor Speed FL: 91 RPM
Motor Speed BR: 142 RPM - Motor Speed BL: 111 RPM
Motor Speed FR: 97 RPM - Motor Speed FL: 91 RPM
Motor Speed BR: 142 RPM - Motor Speed BL: 111 RPM
Motor Speed FR: 0 RPM - Motor Speed FL: 0 RPM
Motor Speed BR: 0 RPM - Motor Speed BL: 0 RPM
Motor Speed FR: 97 RPM - Motor Speed FL: 91 RPM
Motor Speed BR: 115 RPM - Motor Speed BL: 145 RPM
Motor Speed FR: 97 RPM - Motor Speed FL: 93 RPM
Motor Speed BR: 130 RPM - Motor Speed BL: 109 RPM
Motor Speed FR: 97 RPM - Motor Speed FL: 90 RPM
Motor Speed BR: 100 RPM - Motor Speed BL: 148 RPM
Motor Speed FR: 97 RPM - Motor Speed FL: 93 RPM
Motor Speed BR: 124 RPM - Motor Speed BL: 112 RPM
Motor Speed FR: 91 RPM - Motor Speed FL: 90 RPM
Motor Speed BR: 157 RPM - Motor Speed BL: 112 RPM
Motor Speed FR: 97 RPM - Motor Speed FL: 91 RPM
Motor Speed BR: 97 RPM - Motor Speed BL: 120 RPM
Motor Speed FR: 97 RPM - Motor Speed FL: 91 RPM
Motor Speed BR: 109 RPM - Motor Speed BL: 138 RPM
Motor Speed FR: 97 RPM - Motor Speed FL: 91 RPM
Motor Speed BR: 109 RPM - Motor Speed BL: 138 RPM
Motor Speed FR: 0 RPM - Motor Speed FL: 0 RPM
Motor Speed BR: 0 RPM - Motor Speed BL: 0 RPM
Motor Speed FR: 0 RPM - Motor Speed FL: 0 RPM
Motor Speed BR: 0 RPM - Motor Speed BL: 0 RPM
Motor Speed FR: 0 RPM - Motor Speed FL: 0 RPM
Motor Speed BR: 0 RPM - Motor Speed BL: 0 RPM
Motor Speed FR: 97 RPM - Motor Speed FL: 91 RPM
Motor Speed BR: 115 RPM - Motor Speed BL: 120 RPM
Motor Speed FR: 97 RPM - Motor Speed FL: 91 RPM
Motor Speed BR: 136 RPM - Motor Speed BL: 135 RPM
Motor Speed FR: 97 RPM - Motor Speed FL: 91 RPM
Motor Speed BR: 127 RPM - Motor Speed BL: 108 RPM
Motor Speed FR: 97 RPM - Motor Speed FL: 91 RPM
Motor Speed BR: 142 RPM - Motor Speed BL: 129 RPM
Motor Speed FR: 97 RPM - Motor Speed FL: 91 RPM
Motor Speed BR: 142 RPM - Motor Speed BL: 129 RPM
Motor Speed FR: 0 RPM - Motor Speed FL: 0 RPM
Motor Speed BR: 0 RPM - Motor Speed BL: 0 RPM
Motor Speed FR: 0 RPM - Motor Speed FL: 0 RPM
Motor Speed BR: 0 RPM - Motor Speed BL: 0 RPM
Motor Speed FR: 0 RPM - Motor Speed FL: 0 RPM
Motor Speed BR: 0 RPM - Motor Speed BL: 0 RPM
Motor Speed FR: 97 RPM - Motor Speed FL: 91 RPM
Motor Speed BR: 108 RPM - Motor Speed BL: 125 RPM
Motor Speed FR: 97 RPM - Motor Speed FL: 91 RPM
Motor Speed BR: 108 RPM - Motor Speed BL: 125 RPM
Motor Speed FR: 97 RPM - Motor Speed FL: 91 RPM
Motor Speed BR: 108 RPM - Motor Speed BL: 125 RPM
Motor Speed FR: 0 RPM - Motor Speed FL: 0 RPM
Motor Speed BR: 0 RPM - Motor Speed BL: 0 RPM

the motors aren’t the best so that’s why all the RPMs aren’t the same.

Can anyone please explain why the data sometimes cuts out.

Thank you.

how long are the cables between the devices, e.g I2C has linits on cable length and speed, etc
cars are very noisy electrical environment
have you considered canbus

The cables are around 25cm.

If I comment everything out and have it just wire one sensor’s data it works perfectly. it’s when I start adding more the cutting in and out starts happening.

25cm should be OK - have you tried reducing the clock rate
does the master poll the slaves at regular intervals?

The master polls them every 1 second I can try to reduce the clock rate

I changed the clock rate to low speed:

 Wire.setClock(10000);

It still gives me the same problem

If the I2C has a problem you usually get only high states, so the value would be 255 and not 0.

In the slave code remove the Timer1.detachInterrupt() and Timer1.attachInterrupt() calls in the ISR. The interrupt won't fire during the few calculations.

Simplify the calculations:

rotationFR = (counterFR / diskslots) * 60.00/2;

gets

rotationFR = counterFR * 3 / 2;

Are the RPMs plausible values?

On the master check the return value of Wire.requestFrom(). It must be 4, otherwise the value wasn't transfered.

All those interrupts (from the wheel sensors, the Serial library, the Timer1 library and the interrupt inside the Wire library) might be too much.

You can start by fixing all problems.

Master

This will work for 50 days: if(millis() >= StartTime)
You can better do things right from the start, see: https://www.arduino.cc/en/tutorial/BlinkWithoutDelay.

The Wire.requestFrom(1,4) and the Wire.read() belong to each other. There is no need to split them over two functions.

The Wire.setClock() function is just a draft, the source code for it is not finished yet. A speed of 10kHz does not work, start with 50kHz: Wire.setClock(50000) ;.
You may use the Wire.setClock() in the Slave as well, but the Master sets the clock speed.

The Master could detect if valid data is received as @pylon already wrote.

Wire.requestFrom(1,4);
if( Wire.available() != 4)
{
  Serial.println( "Error, Slave did not respond");
}
else
{
  rotationFR = Wire.read();
  rotationFL = Wire.read();
  rotationBR = Wire.read();
  rotationBL = Wire.read();

  Serial.print("Motor Speed FR: ");
  Serial.print(rotationFR);  
  Serial.print(" RPM - "); 
  ...
}

Slave

I prefer to start with I2C address 8. Below 8 is reserved.

The Wire.onRequest(updateRot) should be called just once in setup().

The ISR_timerone() takes a long time with those floating point calculations. Use the integer calculations that @pylon proposes.
Would it be possible to do those calculations in the loop() with a millis() timers ? I think it can be done in the loop(), but that requires extra code.

Detaching and attaching the interrupt inside its own ISR is very weird to do. I'm glad that @pylon agrees with me.

I implemented all the suggestions but now the problem seemed to get worse.
here is the new code for the master and the slave

Master:

#include <Wire.h>
#include <Adafruit_MotorShield.h>
#include <Servo.h>
static byte rotationFR ;
  static byte rotationFL;
  static byte rotationBR;
 static byte rotationBL;
 unsigned long previousMillis = 0; 
 const long interval = 1000;

 int FRSpeed;
 int FLSpeed;
 int BRSpeed;
 int BLSpeed;

Adafruit_MotorShield AFMS = Adafruit_MotorShield(); 
Adafruit_DCMotor *FRight = AFMS.getMotor(3);
Adafruit_DCMotor *FLeft = AFMS.getMotor(4);
Adafruit_DCMotor *BLeft = AFMS.getMotor(1);
Adafruit_DCMotor *BRight = AFMS.getMotor(2);

void setup() {
  Wire.begin();
  Wire.setClock(50000);
  servo1.attach(10);
  
Serial.begin(9600);
AFMS.begin();


FLeft->setSpeed(150);
FRight->setSpeed(150);
BLeft->setSpeed(150);
BRight->setSpeed(150);

}

void loop(){ 

FRight->run(FORWARD);
FLeft->run(FORWARD);
BRight->run(FORWARD);
BLeft->run(FORWARD);
unsigned long currentMillis = millis();

  if (currentMillis - previousMillis >= interval) {
    previousMillis = currentMillis;
       Wire.requestFrom(8,4);

       if( Wire.available() != 4)
{
  Serial.println( "Error, Slave did not respond");
}
else
{
  rotationFR = Wire.read();
  rotationFL = Wire.read();
  rotationBR = Wire.read();
  rotationBL = Wire.read();

  Serial.print("Motor Speed FR: "); 
  Serial.print(rotationFR);  
  Serial.print(" RPM - "); 
  Serial.print("Motor Speed FL: "); 
  
 Serial.print(rotationFL);  
  Serial.println(" RPM"); 
  Serial.print("Motor Speed BR: "); 
  
 Serial.print(rotationBR);  
 Serial.print(" RPM - "); 
  Serial.print("Motor Speed BL: "); 
  
  Serial.print(rotationBL);  
  Serial.println(" RPM"); 




  }
  }


}

Slave:

#include <Wire.h>
#include "TimerOne.h"
#include "PinChangeInt.h"
const byte FRMotor =4;
const byte FLMotor =5;
const byte BRMotor =6;
const byte BLMotor =7;
unsigned int counterFR = 0;
unsigned int counterFL = 0;
unsigned int counterBR = 0;
unsigned int counterBL = 0;
static byte rotationFR = 0;
static byte rotationFL = 0;
static byte rotationBR = 0;
static byte rotationBL = 0;
float diskslots = 20;
void ISR_countFR()  
{
  counterFR++;
} 
void ISR_countFL()  
{
  counterFL++; 
} 
void ISR_countBR()  
{
  counterBR++;
} 
void ISR_countBL()  
{
  counterBL++; 
} 
// TimerOne ISR

void ISR_timerone()
{
  if((counterFR *3 /2)>255){
    rotationFR =250;
  }
  else{
  rotationFR = (counterFR *3 /2); } // calculate RPM for Motor 1
  counterFR = 0; 
  
   if((counterFL *3 /2)>255){
    rotationFL =250;
  }
  else{ 
  rotationFL = (counterFL *3 /2);}  // calculate RPM for Motor 2
  counterFL = 0;  
  
   if((counterBR *3 /2)>255){
    rotationBR =250;
  }
  else{
  rotationBR = (counterBR *3 /2);}  // calculate RPM for Motor 1
  counterBR = 0; 
 
   if((counterBL *3 /2)>255){
    rotationBL =250;
  }
  else{
  rotationBL = (counterBL *3 /2);}  // calculate RPM for Motor 2 
  counterBL = 0;  
}
void updateRot(){
  byte sendData[4]; 
  sendData[0]=rotationFR;
  sendData[1]=rotationFL;
 sendData[2]=rotationBR;
 sendData[3]=rotationBL;
  Wire.write(sendData,4);
  
}

void setup() {
Serial.begin(9600);
Wire.begin(8);
Timer1.initialize(1000000); // set timer for 1sec 
  attachPinChangeInterrupt(FRMotor, ISR_countFR,CHANGE);
  attachPinChangeInterrupt(FLMotor, ISR_countFL,CHANGE);
  attachPinChangeInterrupt(BRMotor, ISR_countBR,CHANGE);
  attachPinChangeInterrupt(BLMotor, ISR_countBL,CHANGE);
  Timer1.attachInterrupt( ISR_timerone ); // Enable the timer
  Wire.onRequest(updateRot);

}

void loop(){ 

}

this is the new output:

Motor Speed BR: 6 RPM - Motor Speed BL: 0 RPM
Motor Speed FR: 0 RPM - Motor Speed FL: 0 RPM
Motor Speed BR: 3 RPM - Motor Speed BL: 0 RPM
Motor Speed FR: 0 RPM - Motor Speed FL: 0 RPM
Motor Speed BR: 3 RPM - Motor Speed BL: 0 RPM
Error, Slave did not respond
Motor Speed FR: 0 RPM - Motor Speed FL: 0 RPM
Motor Speed BR: 0 RPM - Motor Speed BL: 0 RPM
Motor Speed FR: 99 RPM - Motor Speed FL: 90 RPM
Motor Speed BR: 121 RPM - Motor Speed BL: 135 RPM
Motor Speed FR: 99 RPM - Motor Speed FL: 91 RPM
Motor Speed BR: 132 RPM - Motor Speed BL: 114 RPM
Error, Slave did not respond
Motor Speed FR: 0 RPM - Motor Speed FL: 0 RPM
Motor Speed BR: 0 RPM - Motor Speed BL: 0 RPM
Error, Slave did not respond
Motor Speed FR: 0 RPM - Motor Speed FL: 0 RPM
Motor Speed BR: 0 RPM - Motor Speed BL: 0 RPM
Error, Slave did not respond
Motor Speed FR: 0 RPM - Motor Speed FL: 0 RPM
Motor Speed BR: 0 RPM - Motor Speed BL: 0 RPM
Error, Slave did not respond
Motor Speed FR: 0 RPM - Motor Speed FL: 0 RPM
Motor Speed BR: 0 RPM - Motor Speed BL: 0 RPM
Motor Speed FR: 97 RPM - Motor Speed FL: 91 RPM
Motor Speed BR: 138 RPM - Motor Speed BL: 132 RPM
Motor Speed FR: 97 RPM - Motor Speed FL: 90 RPM
Motor Speed BR: 123 RPM - Motor Speed BL: 108 RPM
Error, Slave did not respond
Motor Speed FR: 0 RPM - Motor Speed FL: 0 RPM
Motor Speed BR: 0 RPM - Motor Speed BL: 0 RPM
Motor Speed FR: 97 RPM - Motor Speed FL: 90 RPM
Motor Speed BR: 124 RPM - Motor Speed BL: 105 RPM
Motor Speed FR: 97 RPM - Motor Speed FL: 90 RPM
Motor Speed BR: 114 RPM - Motor Speed BL: 142 RPM
Motor Speed FR: 94 RPM - Motor Speed FL: 88 RPM
Motor Speed BR: 141 RPM - Motor Speed BL: 93 RPM
Motor Speed FR: 12 RPM - Motor Speed FL: 1 RPM
Motor Speed BR: 3 RPM - Motor Speed BL: 1 RPM
Motor Speed FR: 93 RPM - Motor Speed FL: 0 RPM
Motor Speed BR: 0 RPM - Motor Speed BL: 0 RPM
Motor Speed FR: 19 RPM - Motor Speed FL: 0 RPM
Motor Speed BR: 0 RPM - Motor Speed BL: 0 RPM

I turned the wheels on and that is when I got the first error. After that the wheels were spinning and i got mixed data between 0s and errors. When the wheels are turned off i get constant data of zeroes. If i spin them manually the data is correct and seems to function properly.

The I2C bus has no checksum, and the Arduino Wire library does not detect all problems. So when it does detect a problem, there is a serious bus problem. The I2C bus with so many errors is totally unreliable.

Could you disconnect the motors, but let the sketch still use the Adafruit_Motorshield library. Then turn the wheels. Are those numbers okay ? If they are, the software with interrupts does not get in each others way too much, and it is most likely hardware noise.

What are your pullup resistors for the I2C bus ? Did you connect the GND pins of the Arduino boards ?

The pull-up resistors are 4.7k. I am using 4 1.5 volt batteries for the power for the Arduino UNO (master) and i have the 5v going into the raw pin of the pro mini. I have a separate power supply for the motors. I forgot to connect the two ground pins and when i did it cleared up all the errors. So not having common ground may have been the issue the whole time.

All errors ? You could increase an error counter when there is a problem on the I2C bus, and print that counter to the serial monitor. One error per day is still too much in my opinion.

So many things you have in your network; I have no complain for it. But, I would have complain and serious objection if you have connected them together without following the SSS (Small Start Strategy) approach. The SSS approach says –

1. Start with minimum software and hardware that works.

2. Add the next incremental quantity and make the system work. If not working, make it to work and write down the steps that have undertaken to make it work.

3. You proceed with SSS approach; the end result would be a working system; it does not matter how complex it is. Human efforts have always been rewarded with results — either with a working system or knowledge-base to design better system or both.

4. Based on @Koepel’s recommendation, I would suggest you to organize your codes as per following structure:

Master Codes:

#include<Wire.h>
#define slaveAddess 0x08

unsigned long presentmillis=0;

//add codes

void setup()
{
   Serial.begin(9600);
   Wire.begin();

   //add codes
}

void loop()
{
   if(millis() - presentmillis <1000)
   {
      presentmillis = millis();
      Wire.requestFrom(slaveAddress, 4);  //available will always show 4; the value is not coming from slave
      rotationFR = Wire.read();
      rotationFL = Wire.read();
      rotationBR = Wire.read();
      rotationBL = Wire.read();

      //add codes
    }
}

Slave Codes:

#include<Wire.h>
#define slaveAddess 0x08

void setup()
{
  Wire.begin(slaveAddress);
  Wire.onRequest(sendEvent);
    
  //add codes
}

void loop()
{
    //add codes
}

void sendEvent(int howmany) //howmany is always equal to the Wire.write() commands issued by Master
{
   //add codes
}

It fixed the present issues that I had and I don't know if that's all of them. I will put in an error counter to check for any future issues and organize my code based on @GolamMostafa's suggestion.