user70807: 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;   
   }
}


user80216: 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;
    }
}


70807: <p>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. <strong>Is there some obvious mistake I've made? Is millis just bad for keeping time?</strong> 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!</p>
<pre><code>    #include &lt;Servo.h&gt;

//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 &gt; threshold)
{
  
  if (sweep == 0)
  {
    
    for (int pos = 0; pos &lt;= 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 &gt; 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(&quot;SWEEP 1&quot;);
              sweep = 1;
            }
              
              sensorValue = analogRead (sensorPin);
              if (sensorValue &lt; threshold)
                {
                  break;
                }
     }
  }
  
  if (sweep == 1)
  {
    
    for (int pos = 180; pos &gt;= 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 &gt; 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(&quot;SWEEP 2&quot;);
            }
              
              sensorValue = analogRead (sensorPin);
              if (sensorValue &lt; threshold)
                {
                  break;
                }
    }
  }
}
   
   if (sensorValue &lt;= threshold) //if the light is low turn the servo back to starting point and stop doing anything
   {
    Serial.println(&quot;OFF&quot;);
    noTone(buzzerPin);
    myservo.write(0);
    counter = 0;   
   }
}
</code></pre>
 80216: <p>If you're still interested in refactored code, here is your code with <code>for</code> loops removed and restructured so state is tracked/updated appropriately in the <code>void loop()</code> as below:</p>
<pre><code>[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 &gt; 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) &gt; duration[counter]/tempo)
        {
            tone(buzzerPin, melody[counter], duration[counter]/tempo);
            counter += (counter &gt; 195) ? -196 : 1;
            previousMillis = currentMillis;
        }
    }
    else
    {
        Serial.println(&quot;OFF&quot;);
        noTone(buzzerPin);
        myservo.write(0);
        counter = 0; 
        pos = 0;
        dir = 1;
    }
}
</code></pre>
