delay inside a function not working?

Hi everybody,

so i have this function

void alarm (){
  int ledState=1;
  int melodyFail[]={NOTE_AS5,NOTE_A5,NOTE_GS5,NOTE_G5};
  for (int thisFail = 0; thisFail < 4; thisFail) {
       digitalWrite(ledPin3, ledState);
       digitalWrite(ledPin4, ledState);  
       digitalWrite(ledPin5, ledState);  
       tone(8, melodyFail[thisFail], 300);
       delay(400);
       ledState=!ledState;
       Serial.println(thisFail);
       thisFail++;
  }
  thisNote=0;
  thisLed=0;

which works when my slave arduino receives a certain message from the i2c bus.

The whole for loop plays a melody in case the alarm function is called (I used the same structure in the void loop and it works perfectly)

The thing is the melody isn’t being played when i use this structure inside the alarm void, and it just plays the last note of the loop (although the loop it starts and finishes correctly as i have printed the variable that makes the loop keep going).

The variable “thisFail” it goes from 0 to 3 really fast avoiding the delay there’s inside.

I think the problem is the delay but i don’t know why it’s not working inside the function but it does work in the main loop.

Thank you for your answers

anyone?

 for (int thisFail = 0; thisFail < 4; thisFail) {


      thisFail++;
 }

Normally that would be written as:

 for (int thisFail = 0; thisFail < 4; thisFail++) {
       ...
  }

However, I don’t believe that is the source of your problem.

We need to see the rest of your code. Maybe you have something else which is using the same timer as tone()?

Which Arduino?

Note: bumping your own post after less than 10 minutes is rude. It takes more than 10 minutes to write a reasoned response.

Give it 24 hours until you bump it again.

MorganS:
Normally that would be written as:

 for (int thisFail = 0; thisFail < 4; thisFail++) {


  }



However, I don't believe that is the source of your problem.

We need to see the rest of your code. Maybe you have something else which is using the same timer as tone()?

Which Arduino?

Sorry about the bump.

#include <Wire.h>
#include "Pitches.h"
int sensor=0;
int thisNote;
int thisLed;

int ledPin3 = 3;
int ledPin4 = 4;
int ledPin5 = 5;
 int leds[] = {ledPin3, ledPin4, ledPin5, ledPin3, ledPin4, ledPin5, ledPin3, ledPin4, ledPin5, ledPin3, ledPin4, ledPin5, ledPin3,
                ledPin4, ledPin5, ledPin3, ledPin4, ledPin5, ledPin3, ledPin4, ledPin5, ledPin3, ledPin4, ledPin5, ledPin3};
  
//melody array, it shows the order the notes are gonna be played when you touch the corresponding array position in the sensors array
int melody[] = {NOTE_E4, NOTE_F4, NOTE_G4, NOTE_E4, NOTE_C4, NOTE_F4, NOTE_E4, NOTE_D4, NOTE_C4, NOTE_D4, NOTE_D4, NOTE_E4, NOTE_C4, 
                NOTE_E4, NOTE_F4, NOTE_G4, NOTE_E4, NOTE_C4, NOTE_F4, NOTE_E4, NOTE_D4, NOTE_C4, NOTE_D4, NOTE_E4, NOTE_C4};

// note durations: 4 = quarter note, 8 = eighth note, etc.:
int noteDurations[] = {4, 4, 2, 2, 2, 4, 4, 4, 4, 4, 4, 4, 4, 
                       4, 4, 2, 2, 2, 4, 4, 4, 4, 4, 4, 4};

void setup() {
   pinMode(ledPin3, OUTPUT); // configura el pin 13 como salida
   pinMode(ledPin4, OUTPUT); // configura el pin 13 como salida
   pinMode(ledPin5, OUTPUT); // configura el pin 13 como salida
   Wire.begin(8);                // join i2c bus with address #8
   Wire.onReceive(receiveEvent); // what to do when receiving data
// Wire.onRequest(requestEvent); //function to run when asking for data
   Serial.begin(9600);           // start serial for output
  
}

void loop() {

 int opcion=1;
 
  
 if (opcion==1){

if (sensor==11){
  tone(8, NOTE_C4,100);
}
if (sensor==12){
  tone(8, NOTE_E4,100);
}  
 }

 if (opcion==0){
 for (thisNote=0; thisNote<26; thisNote){
 //   while (Wire.available()) { // loop through all but the last
  
  int start = 0; 
  thisLed=0;
  while (start==0){
  digitalWrite(leds[thisLed], HIGH); 
  int noteDuration = 1000 / noteDurations[thisNote];  
  delay(107);
   
  if (sensor==1){
     digitalWrite(leds[thisLed], LOW);
     tone(8, melody[thisNote], noteDuration);
     thisNote++;
     thisLed=thisLed+1;
    }  

    if (thisNote==25) {
     delay (500);
     thisLed=0;
      for (int thisNote = 0; thisNote < 25; thisNote++) {
       digitalWrite(leds[thisLed], HIGH);    
       int noteDuration = 500 / noteDurations[thisNote];
       tone(8, melody[thisNote], noteDuration);
       int pauseBetweenNotes = noteDuration * 1.30;  
       delay(pauseBetweenNotes);
       digitalWrite(leds[thisLed], LOW); 
       thisLed=thisLed+1;
       }
      thisNote=0;
      thisLed=0;
      }
  Serial.print("SENSORON:");
  Serial.print(thisNote);                    // presenta sensor output 1 
  Serial.println("\t"); 
  delay(200);  
}
} 
//}                          
}
}

// function that executes whenever data is received from master
// this function is registered as an event, see setup()
void receiveEvent (int HowMany) {

    sensor = Wire.read(); // receive byte as a character
     Serial.println(sensor); 
     if (sensor==123) alarm();
}

void alarm (){
  int ledState=1;
  int melodyFail[]={NOTE_AS5,NOTE_A5,NOTE_GS5,NOTE_G5};
  for (int thisFail = 0; thisFail < 4; thisFail) {
       digitalWrite(ledPin3, ledState);
       digitalWrite(ledPin4, ledState);  
       digitalWrite(ledPin5, ledState);  
       tone(8, melodyFail[thisFail], 300);
       delay(400);
       ledState=!ledState;
       Serial.println(thisFail);
       thisFail++;
  }
  thisNote=0;
  thisLed=0;

the int opcion is because of a switch that is still not placed in the hardware.

// function that executes whenever data is received from master
// this function is registered as an event, see setup()
void receiveEvent (int HowMany) {

For "event", read "Interrupt Service Routine (ISR)". During an ISR, interrupts are disabled, so you can't call functions that need interrupts enabled, like delay().

Also, you shouldn’t. Keep interrupts as short as possible. Just register they happened and act on it in loop().

Some tips:
Press ctrl+T to automagical make your code wayyyyy more readable.

opcion, you mean option?

int ledPin3 = 3;
int ledPin4 = 4;
int ledPin5 = 5;

Those are useless names. Why place the pin name after it? If you would have called them led1, led2 and led3 I might understand. But even then, arrays!

const byte LedPins[] = {3, 4, 5};

I also use byte because just using int for everything is bad habbit / memory wast. Just pick the smallest possible variable size that fits your needs. And const because it doesn’t change on runtime.

int leds[] = {ledPin3, ledPin4, ledPin5, ledPin3, ledPin4, ledPin5, ledPin3, ledPin4, ledPin5, ledPin3, ledPin4, ledPin5, ledPin3,
              ledPin4, ledPin5, ledPin3, ledPin4, ledPin5, ledPin3, ledPin4, ledPin5, ledPin3, ledPin4, ledPin5, ledPin3
             };

That order is completely sequential, you can easily determine the right led on runtime.

digitalWrite(leds[thisLed % 3], HIGH);
        if (thisNote == 25) {
          delay (500);
          thisLed = 0;
          for (int thisNote = 0; thisNote < 25; thisNote++) {

You do know by doing that you have TWO versions of thisNote? A global (which you checked to be 25) and a local which you increment in a for loop.

septillion, you forgot in

const byte LedPins = {3, 4, 5};

PaulS:

// function that executes whenever data is received from master

// this function is registered as an event, see setup()
void receiveEvent (int HowMany) {



For "event", read "Interrupt Service Routine (ISR)". During an ISR, interrupts are disabled, so you can't call functions that need interrupts enabled, like delay().

what do you suggest then? what could i do?

because i need a delay inbetween the notes... if not i can't play a melody

septillion:
Also, you shouldn’t. Keep interrupts as short as possible. Just register they happened and act on it in loop().

Some tips:
Press ctrl+T to automagical make your code wayyyyy more readable.

opcion, you mean option?

int ledPin3 = 3;

int ledPin4 = 4;
int ledPin5 = 5;



Those are useless names. Why place the pin name after it? If you would have called them led1, led2 and led3 I might understand. But even then, arrays!


const byte LedPins = {3, 4, 5};



I also use byte because just using int for everything is bad habbit / memory wast. Just pick the smallest possible variable size that fits your needs. And const because it doesn't change on runtime.



int leds = {ledPin3, ledPin4, ledPin5, ledPin3, ledPin4, ledPin5, ledPin3, ledPin4, ledPin5, ledPin3, ledPin4, ledPin5, ledPin3,
             ledPin4, ledPin5, ledPin3, ledPin4, ledPin5, ledPin3, ledPin4, ledPin5, ledPin3, ledPin4, ledPin5, ledPin3
            };



That order is completely sequential, you can easily determine the right led on runtime.


digitalWrite(leds[thisLed % 3], HIGH);






if (thisNote == 25) {
         delay (500);
         thisLed = 0;
         for (int thisNote = 0; thisNote < 25; thisNote++) {



You do know by doing that you have TWO versions of thisNote? A global (which you checked to be 25) and a local which you increment in a for loop.

Thank you for all the advices…

The sequentiality of the leds was just a temporary thing to check if it was working properly, as it’s going to change in the final project.

Could you please explain a bit more about the:
Also, you shouldn’t. Keep interrupts as short as possible. Just register they happened and act on it in loop().

By interruptions, you mean voids and functions?

No he means interrupts. There is no such thing as a “void”. The void in front of a function definition just means the function doesn’t return anything.

It looks like there are some terms you’re not sure of. Maybe some time with professor Google would help to clear those up.

Delta_G:
No he means interrupts.

...and the routines that service them.

Could you please explain a bit more about the:
Also, you shouldn't. Keep interrupts as short as possible. Just register they happened and act on it in loop().

Suppose that you have a have a house, and that there is a push-button switch by the door, to ring the doorbell.

Suppose that you are in the middle of cooking dinner, and the doorbell rings. You can:
Stop what you are doing, go answer the door, spend 20 minutes digging the lawn mower out that the neighbor wants to borrow, return to the kitchen to find that dinner has burnt to a crisp, and order pizza.

Or, you can answer the door, promise to dig the mower out after dinner, and return to complete a home cooked meal. After dinner, you can go dig the mower out.

Doing time-consuming things (digging the mower out) in an interrupt handler is not correct. Noting that something needs doing (dig the mower out after dinner) is. Of course, you still need to do the time-consuming thing, but not in the interrupt handler.

@PaulS

I love the analogy; karma added.