I2C problem

Hi ,
I'm trying to make a midi contoller with 4 arduinos,1 Master and 3 slaves(Buttons,Rotary potentiometers and Faders) using the wire library and not the midi library.It was imposed by my Teacher.I wrote and tested code on one master and one slave.It works find.My problem is when I connect all the arduinos together.What I want si that when the state of a button/potentiometer changes a value is send to the master.I think that to make it work on each slave I have to write a function in the loop which tells us whether one of the control state has changed.On the master in the loop I make requests to each slave.the first request to know if there was a change and as long as there are changes of state on that arduino the master should make request to the arduino to have the change value.But I don't know how to synchronise all this .I tried many things without success.Please help.

Here is the code for testing on the master and the slave rotary potentiometer

Master

#include <Wire.h>

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

void loop() {
 
     Wire.requestFrom(8,3);
      while(Wire.available()){
      byte c = Wire.read();
      Serial.println(c);  
}
       delay(1000);
}

slave

#include <Wire.h>

int address = 8;
#define NUM_AI 2
#define FILTER_AMOUNT 2

#define ANALOGUE_PIN_ORDER A0, A1
// Timeout is in microseconds
#define ANALOGUE_INPUT_CHANGE_TIMEOUT 250000

byte analogueInputs[NUM_AI];
// Variable to hold temporary analogue values, used for analogue filtering logic.
byte tempAnalogueInput;
// Variable to hold difference between current and new analogue input values.
int analogueDiff = 0;
// Array containing a mapping of analogue pins to channel index. This array size must match NUM_AI above.
byte analogueInputMapping[NUM_AI] = {ANALOGUE_PIN_ORDER};

// This is used as a flag to indicate that an analogue input is changing.
boolean analogueInputChanging[NUM_AI];
// Time the analogue input was last moved
unsigned long analogueInputTimer[NUM_AI];


void setup() {

Wire.begin(address);
Wire.onRequest(onRequest);
// Initialise each analogue input channel.
  for (int i = 0; i < NUM_AI; i++)
  {
    // Set the pin direction to input.
    pinMode(analogueInputMapping[i], INPUT);
    
    // Initialise the analogue value with a read to the input pin.
    analogueInputs[i] = analogRead(analogueInputMapping[i]);
    
    // Assume no analogue inputs are active
    analogueInputChanging[i] = false;
    analogueInputTimer[i] = 0;
  }

}

void loop() {
   delay(100); 
  }

  void onRequest(){

    for ( int i = 0; i < NUM_AI; i++)
  {
    // Read the analogue input pin, dividing it by 8 so the 10-bit ADC value (0-1023) is converted to a 7-bit MIDI value (0-127).
    tempAnalogueInput = analogRead(analogueInputMapping[i]) / 8;
    
    
    // Take the absolute value of the difference between the curent and new values 
    analogueDiff = abs(tempAnalogueInput - analogueInputs[i]);
    // Only continue if the threshold was exceeded, or the input was already changing
    if ((analogueDiff > 0 && analogueInputChanging[i] == true) || analogueDiff >= FILTER_AMOUNT)
    {
      // Only restart the timer if we're sure the input isn't 'between' a value
      // ie. It's moved more than FILTER_AMOUNT
      if (analogueInputChanging[i] == false || analogueDiff >= FILTER_AMOUNT)
      {
        // Reset the last time the input was moved
        analogueInputTimer[i] = micros();
        
        // The analogue input is moving
        analogueInputChanging[i] = true;
      }
      else if (micros() - analogueInputTimer[i] > ANALOGUE_INPUT_CHANGE_TIMEOUT)
      {
        
        analogueInputChanging[i] = false;
            
      }
      
      // Only send data if we know the analogue input is moving
      if (analogueInputChanging[i] == true)
      {
          
        // Record the new analogue value
        analogueInputs[i] = tempAnalogueInput;


         controlChange(16,22, analogueInputs[i]);
      }
    }
      
        
    if (analogueInputs[i] != tempAnalogueInput)
    {
        
      // Record the new analogue value
      analogueInputs[i] = tempAnalogueInput;
      
      
      // Send the analogue value 
      controlChange(16,22, analogueInputs[i]);
    }
     else{
         NoControlChange(16, 22);
     } 
  }
   }

  void controlChange(int channel, int control, int value){
     byte SendMidi[] =  {channel,control,value};                   
  Wire.write(SendMidi,3);  
 }
 void NoControlChange(int channel, int control){
     byte SendMidi[] =  {channel,control,200};                   
  Wire.write(SendMidi,3);  
 }

No one has commented yet, because it is easier for us when you show a sketch with a bug, so we can try to pinpoint the problem.

Common rule: Try to execute code in the loop(), not in a interrupt routine.
Special rule for Arduino I2C Slave: The code in the interrupt routine stretches the clock SCL pulse. The longer the interrupt routine takes, the less smooth the I2C bus is running.

I have only a vague idea:

Suppose the loop() checks the inputs, and compares it to the previous value and does the timing and so on.
If the code in loop() decides that it should be send to the master, then a flag is set in 'analogueInputChanging[]'.

In the interrupt routine onRequest(), those flags are checked and when a flag is set, then the new value is send to the Master. The flag is cleared by the onRequest() routine. All the newest values from the inputs have to be available as global variables of course.

When no flag was set, the three bytes should have a special value, indicating that there is no data.
It is hard to request sometimes one byte and sometimes three bytes. The Slave does not know how many bytes the Master wants. Always sending 3 bytes is easier.

The code to check for the flags is still done in the interrupt routine. That can be done in the loop(), but then buffering is needed. It is only a few flags, so I think it can be done in the interrupt routine.

I think that will work, because when there are more flags activated, then they will be processed one by one. Also, when a value is still changing, the code in onRequest() takes the newest value.

Is this for an Arduino Uno ? That is a 8-bit microcontroller. When more than a byte should stay together, then you have to turn off the interrupts temporary in the loop(). When data and a flag belong together, then you might have to turn off interrupts as well.

A variable that is used in the loop() and in a interrupt routine should be 'volatile'.

Koepel:
A variable that is used in the loop() and in a interrupt routine should be 'volatile'.

should? must?

It is a must :smiley_cat:

Thanks @Koepel for your reply.I also figured out that it will be much easier to always send 3 bytes.I also think that it will work because when the input is not changing 3 bytes are send and I just have to check those 3 bytes and if they are the same then I make a request to the other arduino.There was really no problem I'm the one who just tried complicating my code unnecessary.

Yeah. Although, it is possible when adding a onReceive() interrupt routine. Then the Master can send the command "I'm going to read one byte", and request one byte. That is a lot of overhead, so reading three bytes all the time is easier.

I prefer some kind of invalid data to tell the Master that there is no new data. For example all zeros or all 0xFF.

Part of the I2C protocol is that the Slave does not know how many bytes the Master is requesting during a request. The Slave can not even tell the Master that it has no more data. During a request, the Master controls the clock, the acknowledge after each databyte and the Master determines how many bytes are read.