Help with millis to get accurate timing

Question

I'm essentially trying to create a music box which is light activated using an LDR. When it's open a song plays, while a servo rotates a figurine back and forth, and an LED dims and illuminates in sync with the servo. The code sort of works when I don't include the analogWrite for the LED, but even then the music timing is not great. When I do include analogWrite for the LED the music is completely distorted and doesn't work whatsoever. Is there some obvious mistake I've made? Is millis just bad for keeping time? I'm sure there is a better way to approach this overall, but I'm still an amateur at arduino. My code works by using an array for the notes and the duration of each. I refer to the duration array and use it as dynamic intervals for my millis to keep time. Hopefully that makes sense, my coding jargon is pretty basic!

    #include <Servo.h>

//defining the value of the notes
#define NOTE_C4  262
#define NOTE_D4  294
#define NOTE_E4  330
#define NOTE_F4  349
#define NOTE_G4  392
#define NOTE_A4  440
#define NOTE_B4  494
#define NOTE_C5  523
#define NOTE_D5  587
#define NOTE_E5  659
#define NOTE_F5  698
#define NOTE_G5  784
#define NOTE_A5  880
#define NOTE_B5  988

//order of the notes
int melody[] = {
   NOTE_E4, NOTE_G4, NOTE_A4, NOTE_A4, 0, 
   NOTE_A4, NOTE_B4, NOTE_C5, NOTE_C5, 0, 
   NOTE_C5, NOTE_D5, NOTE_B4, NOTE_B4, 0,
   NOTE_A4, NOTE_G4, NOTE_A4, 0,
   
   NOTE_E4, NOTE_G4, NOTE_A4, NOTE_A4, 0, 
   NOTE_A4, NOTE_B4, NOTE_C5, NOTE_C5, 0, 
   NOTE_C5, NOTE_D5, NOTE_B4, NOTE_B4, 0,
   NOTE_A4, NOTE_G4, NOTE_A4, 0,
   
   NOTE_E4, NOTE_G4, NOTE_A4, NOTE_A4, 0, 
   NOTE_A4, NOTE_C5, NOTE_D5, NOTE_D5, 0, 
   NOTE_D5, NOTE_E5, NOTE_F5, NOTE_F5, 0,
   NOTE_E5, NOTE_D5, NOTE_E5, NOTE_A4, 0,
   
   NOTE_A4, NOTE_B4, NOTE_C5, NOTE_C5, 0, 
   NOTE_D5, NOTE_E5, NOTE_A4, 0, 
   NOTE_A4, NOTE_C5, NOTE_B4, NOTE_B4, 0,
   NOTE_C5, NOTE_A4, NOTE_B4, 0,

   NOTE_A4, NOTE_A4, 

   NOTE_A4, NOTE_B4, NOTE_C5, NOTE_C5, 0, 
   NOTE_C5, NOTE_D5, NOTE_B4, NOTE_B4, 0,
   NOTE_A4, NOTE_G4, NOTE_A4, 0,

   NOTE_E4, NOTE_G4, NOTE_A4, NOTE_A4, 0, 
   NOTE_A4, NOTE_B4, NOTE_C5, NOTE_C5, 0, 
   NOTE_C5, NOTE_D5, NOTE_B4, NOTE_B4, 0,
   NOTE_A4, NOTE_G4, NOTE_A4, 0,
   
   NOTE_E4, NOTE_G4, NOTE_A4, NOTE_A4, 0, 
   NOTE_A4, NOTE_C5, NOTE_D5, NOTE_D5, 0, 
   NOTE_D5, NOTE_E5, NOTE_F5, NOTE_F5, 0,
   NOTE_E5, NOTE_D5, NOTE_E5, NOTE_A4, 0,
   
   NOTE_A4, NOTE_B4, NOTE_C5, NOTE_C5, 0, 
   NOTE_D5, NOTE_E5, NOTE_A4, 0, 
   NOTE_A4, NOTE_C5, NOTE_B4, NOTE_B4, 0,
   NOTE_C5, NOTE_A4, NOTE_B4, 0,

   NOTE_E5, 0, 0, NOTE_F5, 0, 0,
   NOTE_E5, NOTE_E5, 0, NOTE_G5, 0, NOTE_E5, NOTE_D5, 0, 0,
   NOTE_D5, 0, 0, NOTE_C5, 0, 0,
   NOTE_B4, NOTE_C5, 0, NOTE_B4, 0, NOTE_A4,

   NOTE_E5, 0, 0, NOTE_F5, 0, 0,
   NOTE_E5, NOTE_E5, 0, NOTE_G5, 0, NOTE_E5, NOTE_D5, 0, 0,
   NOTE_D5, 0, 0, NOTE_C5, 0, 0,
   NOTE_B4, NOTE_C5, 0, NOTE_B4, 0, NOTE_A4
};

// duration of each note
int duration[] = {
  125, 125, 250, 125, 125, 
  125, 125, 250, 125, 125,
  125, 125, 250, 125, 125,
  125, 125, 375, 125, 
  
  125, 125, 250, 125, 125, 
  125, 125, 250, 125, 125,
  125, 125, 250, 125, 125,
  125, 125, 375, 125, 
  
  125, 125, 250, 125, 125, 
  125, 125, 250, 125, 125,
  125, 125, 250, 125, 125,
  125, 125, 125, 250, 125,

  125, 125, 250, 125, 125, 
  250, 125, 250, 125, 
  125, 125, 250, 125, 125,
  125, 125, 375, 375,

  250, 125,

  125, 125, 250, 125, 125,
  125, 125, 250, 125, 125,
  125, 125, 375, 125, 
  
  125, 125, 250, 125, 125, 
  125, 125, 250, 125, 125,
  125, 125, 250, 125, 125,
  125, 125, 375, 125, 
  
  125, 125, 250, 125, 125, 
  125, 125, 250, 125, 125,
  125, 125, 250, 125, 125,
  125, 125, 125, 250, 125,

  125, 125, 250, 125, 125, 
  250, 125, 250, 125, 
  125, 125, 250, 125, 125,
  125, 125, 375, 375,
  
  250, 125, 375, 250, 125, 375,
  125, 125, 125, 125, 125, 125, 125, 125, 375,
  250, 125, 375, 250, 125, 375,
  125, 125, 125, 125, 125, 500,

  250, 125, 375, 250, 125, 375,
  125, 125, 125, 125, 125, 125, 125, 125, 375,
  250, 125, 375, 250, 125, 375,
  125, 125, 125, 125, 125, 3000
};

Servo myservo;  // create servo object to control a servo

int servoPin = 9;
int ledPin = 11;
int buzzerPin = 3;
int sensorPin = A4;
int sweep = 0; // variable to control which direction the servo is moving
int sensorValue = 0;
int counter = 0; //a counter to keep track of which note the song is up to
float tempo = 1; // a variable for the tempo - lower number = faster temp (2 =  double speed)
unsigned long currentMillis = 0;
unsigned long previousMillis = 0;
int threshold = 500;

void setup()
{
  myservo.attach (servoPin);
  pinMode (ledPin, OUTPUT); 
  pinMode (buzzerPin, OUTPUT);
  pinMode (sensorPin, INPUT);
  Serial.begin(9600); 
}


void loop(){
  
sensorValue = analogRead (sensorPin);
if (sensorValue > threshold)
{
  
  if (sweep == 0)
  {
    
    for (int pos = 0; pos <= 180; pos ++) // goes from 0 degrees to 180 degrees in steps of 1 degree
    { 
      myservo.write(pos);              // tell servo to go to position in variable 'pos'
      analogWrite(ledPin, map(pos, 0, 180, 0, 255)); //mapping the value from the for loop to get the full range of PWM for led fading
        
        currentMillis = millis();
        if (currentMillis - previousMillis > duration[counter]/tempo)
        {
          tone(buzzerPin, melody[counter], duration[counter]/tempo);
          counter++;
          previousMillis = currentMillis;
        }
          
          if (counter == 196)
          {
            counter = 0; //reset the counter so the song starts again       
          }
            
            if (pos == 179)
            {
              Serial.println("SWEEP 1");
              sweep = 1;
            }
              
              sensorValue = analogRead (sensorPin);
              if (sensorValue < threshold)
                {
                  break;
                }
     }
  }
  
  if (sweep == 1)
  {
    
    for (int pos = 180; pos >= 0; pos --)  // goes from 180 degrees to 0 degrees
    {
      myservo.write(pos);              // tell servo to go to position in variable 'pos'
      analogWrite(ledPin, map(pos, 0, 180, 0, 255));
       
       currentMillis = millis();
       if (currentMillis - previousMillis > duration[counter]/tempo)
        {
          tone(buzzerPin, melody[counter], duration[counter]/tempo);
          counter++;
          previousMillis = currentMillis;
        }
          
          if (counter == 196)
          {
            counter = 0;       
          }
            
            if (pos == 1)
            {
              sweep = 0;
              Serial.println("SWEEP 2");
            }
              
              sensorValue = analogRead (sensorPin);
              if (sensorValue < threshold)
                {
                  break;
                }
    }
  }
}
   
   if (sensorValue <= threshold) //if the light is low turn the servo back to starting point and stop doing anything
   {
    Serial.println("OFF");
    noTone(buzzerPin);
    myservo.write(0);
    counter = 0;   
   }
}

What board are you using? Try moving the LED to pin 5, 6 or 10 (on Uno), tone and analogWrite would be using the same timer on pins 3 and 11 on a Uno, which sounds problematic. — Mat, Commented Dec 11, 2020 at 5:50
What Mat said. Also, the music timing is inside the for loop that drives the servo; you could also decouple those and set up a millis timer for the music and a millis timer for the servo and let the loop do its work without the for loop. That won't solve the timer problem, but the code would be much shorter and easier to read, and the music and servo timings would be independent (but tuneable). I'll post code that shows how to do that in an answer if you want. — ocrdu, Commented Dec 11, 2020 at 12:20
@Mat Arduino pin 10 uses the same timer for PWM as the servo library does (timer 1). So I'd stick with Arduino pin 5 or 6 for the LED. — Gerben, Commented Dec 11, 2020 at 13:46
@Gerben: ah good point. Or maybe just put the LED on the same pin as the servo if that's it is just supposed to be on when the servo moves. Frees up a timer :-) — Mat, Commented Dec 11, 2020 at 14:10

Joshua Voskamp · Accepted Answer · 2021-10-26 20:01:09Z

If you're still interested in refactored code, here is your code with for loops removed and restructured so state is tracked/updated appropriately in the void loop() as below:

[your variable declaration code as given]
...
unsigned long currentMillis = 0;
unsigned long previousMillis = 0;
int threshold = 500;

int pos = 0;
int dir = 1;


void setup()
{
  myservo.attach (servoPin);
  pinMode (ledPin, OUTPUT); 
  pinMode (buzzerPin, OUTPUT);
  pinMode (sensorPin, INPUT);
  Serial.begin(9600);
  pos = 0;
  dir = 1;
  previousMillis = millis();
}

void loop(){

    sensorValue = analogRead (sensorPin);
    if (sensorValue > threshold)
    {
        pos += dir;
        if (pos == 180) || (pos == 0)
        {
            dir *= -1;
        }
        
        myservo.write(pos);
        analogWrite(ledPin, map(pos, 0, 180, 0, 255));
        
        currentMillis = millis();
        
        if ((currentMillis - previousMillis) > duration[counter]/tempo)
        {
            tone(buzzerPin, melody[counter], duration[counter]/tempo);
            counter += (counter > 195) ? -196 : 1;
            previousMillis = currentMillis;
        }
    }
    else
    {
        Serial.println("OFF");
        noTone(buzzerPin);
        myservo.write(0);
        counter = 0; 
        pos = 0;
        dir = 1;
    }
}

Stack Exchange Network

Help with millis to get accurate timing

1 Answer 1

Your Answer

Not the answer you're looking for? Browse other questions tagged
time
millis
analogwrite
timing
or ask your own question.

Hot Network Questions

Help with millis to get accurate timing

1 Answer 1

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged timemillisanalogwritetiming or ask your own question.

Related

Hot Network Questions

Not the answer you're looking for? Browse other questions tagged
time
millis
analogwrite
timing
or ask your own question.