millis(), timer problem not sure ???

Hi All,

I am trying to get a pin array to flash 3 LED'S randomly while at the same time create a siren effect using the tone function. Each part if my code works as expected separately but when i combine them only the siren part of the code works. I am guessing it's probably a timer problem ?? but i just cannot resolve my problem. Any help as always would be very much appreciated.

Thank You

[code]int i;
 int ledarray[] = {0,1,2};
 #define wait 1
 #define time 10
 int freq = 0;
 int ledState = LOW;
 unsigned long previousMillis = 0;
 const long interval = 40;

 void setup(){
   
 for (i = 0; i < 3; i++){
 pinMode (ledarray[i], OUTPUT);
  }
   }
 void loop() {
  i = random(3);
 unsigned long currentMillis = millis();
 if(currentMillis - previousMillis >= interval) {
    previousMillis = currentMillis;   
    if (ledState == LOW)
      ledState = HIGH;
    else
      ledState = LOW;
    digitalWrite(ledarray[i], ledState);
   }
  //CODE ABOVE AND BELOW THIS LINE WORKS AS EXPECTED SEPERATELY BUT WILL NOT WORK WHEN COMBINED
  for(int j=0; j<3; j++){
  test();
  }
  for(int j=0; j<10; j++){
  test_2();
  }
   }
    void test(){
  for (freq = 450; freq < 1800; freq += 1) {
    tone(10, freq, time);     // Beep pin, freq, time
    delay(wait);
  }
  for (freq = 1800; freq > 450; freq -= 1) {
    tone(10, freq, time);     // Beep pin, freq, time
    delay(wait);
  }
   }
 void test_2(){
  for (freq = 500; freq < 800; freq += 1) {
    tone(10, freq, time);     // Beep pin, freq, time
    delay(wait);
  }
   }

[/code]

"Each part if my code works as expected separately but when i combine them only the siren part of the code works. I am guessing it's probably a timer problem ??"

No.
The problem is that you're using delay() in the 'siren parts'.

Your for loops are blocking the execution of any other code. For example the code inside the function test() takes 13 and a half seconds to execute, and nothing else happens while it's doing that. And of course you do it 4 times in a row, then do test2() eleven times in a row, and I didn't add up how long that takes to run. So even though you use millis() in your code, you are not applying that timing to all of the code.

Have a look at how millis() is used to manage timing without blocking in Several Things at a Time

...R

OK thanks to all for your reply. It was my understanding that the millis() function would look after the LED'S regardless of what else was running in the rest of the loop, although the delay in the siren part of the code been a problem did cross my mind but i just wasn't sure. So its back to the drawing board.

cnc55:
OK thanks to all for your reply. It was my understanding that the millis() function would look after the LED'S regardless of what else was running in the rest of the loop, although the delay in the siren part of the code been a problem did cross my mind but i just wasn't sure. So its back to the drawing board.

Right. There is no magic involved with millis(). It's just a function that returns the number of milliseconds since the Arduino it's running on restarted.

To use millis() as a timer, you save a particular time by assigning the current value of millis() to it (millis() returns an unsigned long number.) Then, every time through the loop() function, check to see if the current millis() is at least a particular number of milliseconds greater than a previously saved number of milliseconds.

This timing loop needs to encompass all the code you want to run at a particular time.

Check out the IDE example 'Blink Without Delay', for the basics, but then import, load and understand Demonstration code for several things at the same time for a much cleaner (and debugged) version that is written in easy to understand code.

This concept may be the most important thing in Arduino programming.

Thanks Chris i appreciate you time, i have already been playing around with the "several things" code and am struggling to implement it into the test function of my posted code below to get rid of the delay.

void test(){
for (freq = 450; freq < 1800; freq += 1) {
tone(10, freq, time);
delay(wait);
}

bool test()
{
  // current time
  unsigned long currentTime = millis();
  // start tine for tone duration
  static unsigned long startTime = 0;
  // frequency to play
  static unsigned int freq = 450;

  // if not started playing a tone
  if(startTime == 0)
  {
    // save the start time
    startTime = millis();
    // play tone
    tone(10, freq);
  }
  else
  {
    // if tone played for given wait time
    if(mills() - startTime >= wait)
    {
      // next frequency
      freq++;
      // reset startTime so next call to test() will start timing and play new tone
      startTime = 0;
      // if at end of frequency range
      if(freq >= 1800)
      {
        // reset to start frequency
        freq = 450;
        // indicate that we have played one sequence
        return true;
      }
    }
  }
  // indicate that a sequence is in progress
  return false;
}

I'm not familiar with the tone function, you might need notone as well (no idea). Each call to the above function will do a little step (basially checking time). If you simply call it from loop, it will play the tone range permanently.

The function returns a bool; if one full cycle is done (450 to 1800) is done, it returns true (else it returns false) and you can check for that.

void loop()
{
  static bool fPlayCompleted = false;

  // if not played completely
  if(fPlayCompleted == false)
  {
    fPlayCompleted = test();
  }

  // check button to restart
  if(digitalRead(yourButton) == LOW)
  {
    // indicate that we want to play again
    fPlayCompleted = false;
  }
}

Note that the button needs to be wired between pin and ground and an pullup resistor is required (you can use the internal pullup for that).

Thank You sterretje, i was thinking that i needed a "++" in there somewhere as all of the "blink without delay" example stuff is concerned with turning a pin ON/OFF but wasn't sure where to implement it. I appreciate your code provided and am looking forward to making it work.Thank You for your time.

sterretje thank you so much you example code "bool test()" works great. One more question if you wouldn't mind. When i reach this line "if(freq >= 1800)" how would i modify the code to increment back down to freq = 450

The simplest is probably to use another static variable (initialized to 1). This variable will either contain 1 or -1. Instead of using freq++, you add the value of this variable to freq. If the value is positive, the code will go from 450 to 1800, when it's negative it goes the other way.

You also need to change the 'if' where you check the 1800 limit and test for < 450 or >= 1800.

Lastly when you reach the boundaries, instead of reseting to 450, multiply your new variable by -1.

Try to implement it. If you don't come right, let us know.

Note: if you had mentioned this before I would probably have used a totally different approach :wink:

Bed time here, so a next reply might take a while.

Thank You again i appreciate your time and hope i can get it to work.

Hi again sterretje,

Success i managed to implement your suggestion and "test()" function works great. So Thank You again another little bit learned. However i now have another problem and i just can't get my head round it. In my code posted below i want to implement "test_2()" and when i do all i get from the buzzer is distortion despite the fact that each function works as expected when i use them individually.

 int i;
 int ledarray[] = {0,1,2};
 #define wait 1
 int freq = 0;
 int ledState = LOW;
 unsigned long previousMillis = 0;
 const long interval = 40;

 void setup(){
 
 for (i = 0; i < 3; i++){
 pinMode (ledarray[i], OUTPUT);
  }
   }
   
 void loop() {
   
  i = random(3);
 unsigned long currentMillis = millis();
 if(currentMillis - previousMillis >= interval) {
   previousMillis = currentMillis;   

    if (ledState == LOW)
      ledState = HIGH;
    else
      ledState = LOW;
    digitalWrite(ledarray[i], ledState);
   
  }
  for(int j=0; j<3; j++){
  test();
  }
  for(int y=0; y<10; y++){
  test_2();
  }
    }
    
  bool test(){
  // current time
  unsigned long currentTime = millis();
  // start tine for tone duration
  static unsigned long startTime = 0;
  // frequency to play
  static unsigned int freq = 450;
  static unsigned int inc = 1;
  // if not started playing a tone
  if(startTime == 0)
  {
    // save the start time
    startTime = millis();
    // play tone
    tone(10, freq);
  }
  else
  {
    // if tone played for given wait time
    if(millis() - startTime >= wait){
    freq += inc;
     startTime = 0;
      if((freq <450)||(freq >= 1800))
         inc =- inc;
    }
   }
  }

  bool test_2(){
  // current time
  unsigned long currentTime = millis();
  // start tine for tone duration
   static unsigned long startTime = 0;
  // frequency to play
   static unsigned int freq = 500;

  // if not started playing a tone
  if(startTime == 0)
  {
    // save the start time
    startTime = millis();
    // play tone
    tone(10, freq);
  }
  else
  {
    // if tone played for given wait time
    if(millis() - startTime >= wait)
    {
      // next frequency
      freq++;
      // reset startTime so next call to test() will start timing and play new tone
      startTime = 0;
      // if at end of frequency range
       if(freq >= 800){
        freq = 500;
       }   
    }
  }
 }

I guess you don't understand the principle :wink: The way you have implemented it is that both test and test_2 are basically running in parallel and test and test_2 are interfering with each other.

Simple code

void loop()
{
  test();
  test_2();
}

I hope that the below 'picture' makes clear what happens

loop() -> test   +--------------+----------> test_2  +--------------+-----------------+
  ^         |    |              ^               |    |              ^                 |
  |         |    |              |               |    |              |                 |
  |         |    (n)            |               |    (n)            |                 |
  |         |    |              |               |    |              |                 |
  |         +-> time -(y)-> change freq         +-> time -(y)-> change freq           |
  |                                                                                   |
  +-----------------------------------------------------------------------------------+

To get the above working, you will have to re-implement the return values that I implemented in the original so you know when one sequence was completed.

bool test() {
  // current time
  unsigned long currentTime = millis();
  // start tine for tone duration
  static unsigned long startTime = 0;
  // frequency to play
  static unsigned int freq = 450;
  static unsigned int inc = 1;
  // if not started playing a tone
  if (startTime == 0)
  {
    // save the start time
    startTime = millis();
    // play tone
    tone(10, freq);
  }
  else
  {
    // if tone played for given wait time
    if (millis() - startTime >= wait) {
      freq += inc;
      startTime = 0;
      if ((freq < 450) || (freq >= 1800))
        inc = - inc;

// ADD THIS
      // if freq <450, this indicates that we have done one sequence from 450 to 1800 and back
      if(freq < 450)
      {
        indicate to calling function (loop()) that sequence was complete
        return true;
      }


    }
  }
// ADD THIS
  // indicate that sequence not completed yet
  return false;
}

You need to do something similar for test_2.

Now the calling program can check if a sequence was completed and take action based on that (I gave a simple example before with a button before).

You now need to think differently; loop() already performs the loop and you can not use for or while loops because it fully defeats the prupose of the implementation without delays.

The below keeps track of the number of cycles; and implements a simple statemachine. In state 1, the code will play the sequence 450 to 1800 and back 3 times, next reset the cycle counter and change the currentState to 2. In state 2, the code will play the sequence 500 to 800 10 times and after that reset the cycle counter and set the currentState to 1 so the next time.

Hi again

Thank You so much for your patience and time. I did not receive the code example for the statemachine part of the code i need to implement in the loop(). You refer to it in the last paragraph of your previous reply.

P.S sorry for been a nuisance but i am getting there and learning along the way.

OOPS, will have to recreate it. Give me some time.

void loop()
{
  // remember number of cycles done
  static byte numCycles = 0;
  // current state to execute
  static byte currentState = 1;

  switch (currentState)
  {
    // loop 450 to 1800 and back three times
    case 1:
      // if a cycle is completed
      if (test() == true)
      {
        // increment cycle counter
        numCycles++;
      }
      // if we reached required number of cycles
      if (numCycles == 3)
      {
        // reset number of cycles so it can be used in next state
        numCycles = 0;
        // goto state 2
        currentState = 2;
      }
      break;
    // loop 500 to 800 ten time
    case 2:
      // if a cycle is completed
      if (test_2() == true)
      {
        // increment cycle counter
        numCycles++;
      }
      // if we reached required number of cycles
      if (numCycles == 10)
      {
        // reset number of cycles so it can be used in next state
        numCycles = 0;
        // goto state 1
        currentState = 1;
      }
      break;
    // in case of a programming error, we reset numCycles and go to the first state
    default:
      // reset number of cycles so it can be used in next (previous) state
      numCycles = 0;
      // goto state 1
      currentState = 1;
      break;
  }
}

Thank You so much all is working like a charm, i had removed the return values while playing around with a copy of the code and forgot to uncomment them.Your help is very much appreciated and i have learned quiet a bit from your efforts. I have posted the code below for anyone who might find it useful.

int i;
 int ledarray[] = {0,1,2};
 #define wait 1
 int freq = 0;
 int ledState = LOW;
 unsigned long previousMillis = 0;
 const long interval = 40;

 void setup(){
 
 for (i = 0; i < 3; i++){
 pinMode (ledarray[i], OUTPUT);
  }
   }
   
 void loop() {
   
  i = random(3);
 unsigned long currentMillis = millis();
 if(currentMillis - previousMillis >= interval) {
   previousMillis = currentMillis;   

    if (ledState == LOW)
      ledState = HIGH;
    else
      ledState = LOW;
    digitalWrite(ledarray[i], ledState);
   }
  
 static byte numCycles = 0;
  // current state to execute
  static byte currentState = 1;

  switch (currentState)
  {
    // loop 450 to 1800 and back three times
    case 1:
      // if a cycle is completed
      if (test() == true)
      {
        // increment cycle counter
        numCycles++;
      }
      // if we reached required number of cycles
      if (numCycles == 3)
      {
        // reset number of cycles so it can be used in next state
        numCycles = 0;
        // goto state 2
        currentState = 2;
      }
      break;
    // loop 500 to 800 ten time
    case 2:
      // if a cycle is completed
      if (test_2() == true)
      {
        // increment cycle counter
        numCycles++;
      }
      // if we reached required number of cycles
      if (numCycles == 10)
      {
        // reset number of cycles so it can be used in next state
        numCycles = 0;
        // goto state 1
        currentState = 1;
      }
      break;
    // in case of a programming error, we reset numCycles and go to the first state
    default:
      // reset number of cycles so it can be used in next (previous) state
      numCycles = 0;
      // goto state 1
      currentState = 1;
      break;
  }
   }
    
  bool test(){
  // current time
  unsigned long currentTime = millis();
  // start tine for tone duration
  static unsigned long startTime = 0;
  // frequency to play
  static unsigned int freq = 450;
  static unsigned int inc = 1;
  // if not started playing a tone
  if(startTime == 0)
  {
    // save the start time
    startTime = millis();
    // play tone
    tone(10, freq);
  }
  else
  {
    // if tone played for given wait time
    if(millis() - startTime >= wait){
    freq += inc;
     startTime = 0;
      if((freq <450)||(freq >= 1800))
         inc =- inc;
    if(freq < 450)
      {
        //indicate to calling function (loop()) that sequence was complete
        return true;
      }
      }
  }

  // indicate that sequence not completed yet
  return false;
}

  bool test_2(){
  // current time
  unsigned long currentTime = millis();
  // start tine for tone duration
   static unsigned long startTime = 0;
  // frequency to play
   static unsigned int freq = 500;

  // if not started playing a tone
  if(startTime == 0)
  {
    // save the start time
    startTime = millis();
    // play tone
    tone(10, freq);
  }
  else
  {
    // if tone played for given wait time
    if(millis() - startTime >= wait)
    {
      // next frequency
      freq++;
      // reset startTime so next call to test() will start timing and play new tone
      startTime = 0;
      // if at end of frequency range
       if(freq >= 800){
        freq = 500;
        return true;
      }
    }
  }
  // indicate that a sequence is in progress
  return false;
}