# Help! - Beginner Servo Issue

**URL:** <https://forum.arduino.cc/t/help-beginner-servo-issue/1021739>\
**Category:** Programming\
**Created:** [August 13, 2022, 1:51am UTC](https://forum.arduino.cc/t/help-beginner-servo-issue/1021739 "2022-08-13T01:51:21Z")\
**Posts on this page:** 10\
**Page:** 1

<div class="post-metadata">

**Author:** ![warporpoise](https://avatars.discourse-cdn.com/v4/letter/w/34f0e0/32.png) [@warporpoise](https://forum.arduino.cc/u/warporpoise)\
**Post date:** [August 13, 2022, 1:51am UTC](https://forum.arduino.cc/t/help-beginner-servo-issue/1021739/1 "2022-08-13T01:51:21Z")

</div>

I have a beginner question about controlling servos (well, RC warning lights that use PWM).

I will include my code below, but the issue is that when the switch for the servo portion is pressed the servo I am using for testing follows the code the first time the button is pressed, but will not respond to further presses. The LED (simple +5v) circuit is functioning correctly.

I am very new to this, so I'm thinking I'm missing something simple. The code also works flawlessly in an online Arduino simulator that I was initially testing with.

## My Code:

```arduino
#include <Servo.h>

 const int LED = 12;
 const int buttonPin2 = 7;
 const int buttonPin1 = 8;
 const int servo1Pin = 9;
 const int servo2Pin = 10;
 Servo servo1;
 Servo servo2;
 int state = LOW;               

void setup()
{
  servo1.attach(servo1Pin);
  servo2.attach(servo2Pin);
  
  pinMode(buttonPin1, INPUT);
  pinMode(buttonPin2, INPUT);
  pinMode(LED, OUTPUT);
}
void loop()
{
  delay(150);
 if(digitalRead(buttonPin1)==LOW)  
  {
      servo1.write(0);
      servo2.write(0);
      delay(1000);
      servo1.write(180);
      servo2.write(180);
      delay(1000);
      servo1.write(90);
      servo2.write(90);
      delay(1000);
  }

 delay(150);
 if(digitalRead(buttonPin2)==LOW)          
     {   
     state = !state;                     
     digitalWrite(LED,state);         
     }
 }

```

* * *

I don't have a good circuit diagram, but I don't think the circuit is the issue since the servo moves the first time the button is pressed.

Thanks in advance for any help!

---

<div class="post-metadata">

**Author:** ![LarryD](https://dub1.discourse-cdn.com/arduino/user_avatar/forum.arduino.cc/larryd/32/215550_2.png) [@LarryD](https://forum.arduino.cc/u/LarryD)\
**Post date:** [August 13, 2022, 1:57am UTC](https://forum.arduino.cc/t/help-beginner-servo-issue/1021739/2 "2022-08-13T01:57:29Z")

</div>

How are your switches wired ?

Using **delay( )** freezes your sketch for that period of time, do not use it.

---

<div class="post-metadata">

**Author:** ![red\_car](https://avatars.discourse-cdn.com/v4/letter/r/b77776/32.png) [@red\_car](https://forum.arduino.cc/u/red_car)\
**Post date:** [August 13, 2022, 1:58am UTC](https://forum.arduino.cc/t/help-beginner-servo-issue/1021739/3 "2022-08-13T01:58:17Z")

</div>

A hand written drawing will do.

How are your buttons wired? If wired to GND (active LOW) then they would typically be initialised as INPUT\_PULLUP.

How is everything being powered? Particularly the servos.

The delay() statements block the code, so you won't be able to check the buttons are being pressed during those times... you need to use millis() for timing.

---

<div class="post-metadata">

**Author:** ![LarryD](https://dub1.discourse-cdn.com/arduino/user_avatar/forum.arduino.cc/larryd/32/215550_2.png) [@LarryD](https://forum.arduino.cc/u/LarryD)\
**Post date:** [August 13, 2022, 2:42am UTC](https://forum.arduino.cc/t/help-beginner-servo-issue/1021739/4 "2022-08-13T02:42:07Z")

</div>

How are your servos powered ?

---

<div class="post-metadata">

**Author:** ![StefanL38](https://dub1.discourse-cdn.com/arduino/user_avatar/forum.arduino.cc/stefanl38/32/713548_2.png) [@StefanL38](https://forum.arduino.cc/u/StefanL38)\
**Post date:** [August 13, 2022, 8:19am UTC](https://forum.arduino.cc/t/help-beginner-servo-issue/1021739/5 "2022-08-13T08:19:59Z")

</div>

IMHO a short "use millis()" is wayy too lesss instruction to make this understandable

I will explain in comments inside the code what your code is doing

```arduino
#include <Servo.h>

const int LED = 12;
const int buttonPin2 = 7;
const int buttonPin1 = 8;
const int servo1Pin = 9;
const int servo2Pin = 10;
Servo servo1;
Servo servo2;
int state = LOW;

void setup() {
  servo1.attach(servo1Pin);
  servo2.attach(servo2Pin);

  pinMode(buttonPin1, INPUT);
  pinMode(buttonPin2, INPUT);
  pinMode(LED, OUTPUT);
}

void loop() {
  delay(150); // processor is absorbed for 150 milliseconds in twiddling his thumbs unable to do anything else

  if (digitalRead(buttonPin1) == LOW) {
    servo1.write(0);
    servo2.write(0);
    delay(1000); // processor is absorbed for 1 second in twiddling his thumbs unable to do anything else
    servo1.write(180);
    servo2.write(180);
    delay(1000); // processor is absorbed for 1 second in twiddling his thumbs unable to do anything else
    servo1.write(90);
    servo2.write(90);
    delay(1000); // processor is absorbed for 1 second in twiddling his thumbs unable to do anything else
  }

  delay(150); // processor is absorbed for 150 milliseconds in twiddling his thumbs unable to do anything else
  // you have to hold down your button until all the delaying has been finished
  // only them your code will react on the button press
  if (digitalRead(buttonPin2) == LOW) {
    state = !state;
    digitalWrite(LED, state);
  }
}
```

now the question is how to solve this?

delay() is blocking. Your code has to be **re** -written in a **non** -blocking way

This is based on the following basic rule

## let do function loop() ALL looping and use ZERO delay()'s

all other functions work in a quickly jump in / quickly jump out manner

If really **all** functions can rely on

"the code will call **me** in about 100 microseconds again and again and again"

These functions do **not** need any **inner** while-loop or for-loop  
**zero** delay(), **zero** while-loops and **zero** -for-loops are allowed

This is a **totally different** approach than

- switch LED on
- wait for 2 seconds // processor is absorbed for 2 second in twiddling his thumbs unable to do anything else
- switch LED off

Still you need to "delay" executing parts of your code.  
This is done in a different way by a very often repeated checking how much time has passed by

This needs thinking **new**. And this will need some time to learn  
I have written a little tutorial that eplains how to use non-blocking timing

> [@Example-code for timing based on millis() easier to understand through the use of example-numbers / avoiding delay()](https://forum.arduino.cc/t/example-code-for-timing-based-on-millis-easier-to-understand-through-the-use-of-example-numbers-avoiding-delay/974017):
>
> non-blocking timing. Execute code only from time to time. There are a lot of different ways to learn programming. This thread wants to add another approach that is different to the yet existing ones. UPDATE 06.01.2023 if you are mainly interested in applying non-blocking timing you can do a quick read of this short tutorial / demonstration If you are intersted in understanding the details how it works go on reading here. I use an everyday analogon in this post to explain the basic principle …

best regards Stefan

---

<div class="post-metadata">

**Author:** ![MicroBahner](https://dub1.discourse-cdn.com/arduino/user_avatar/forum.arduino.cc/microbahner/32/238923_2.png) [@MicroBahner](https://forum.arduino.cc/u/MicroBahner)\
**Post date:** [August 13, 2022, 8:40am UTC](https://forum.arduino.cc/t/help-beginner-servo-issue/1021739/6 "2022-08-13T08:40:08Z")

</div>

> [@warporpoise](#):
>
> ```arduino
> if(digitalRead(buttonPin1)==LOW)  
> 
> ```

This let me assume, that your buttons are wired to ground. In this case, you must initialise the pin with

```arduino
  pinMode(buttonPin1, INPUT_PULLUP);

```

as @red_car already suggested.

The delay() are ok in the first run. But be aware, that your buttons are checked again only 1 second after the last servo move. May be better to omit at least the last delay() in the if-block.

---

<div class="post-metadata">

**Author:** ![LarryD](https://dub1.discourse-cdn.com/arduino/user_avatar/forum.arduino.cc/larryd/32/215550_2.png) [@LarryD](https://forum.arduino.cc/u/LarryD)\
**Post date:** [August 13, 2022, 5:59pm UTC](https://forum.arduino.cc/t/help-beginner-servo-issue/1021739/7 "2022-08-13T17:59:24Z")

</div>

Another way:

```cpp
// https://forum.arduino.cc/t/help-beginner-servo-issue/1021739

// ********************************************^************************************************
// ServosMovingStateMachine.ino
//
//
// Version YY/MM/DD Comments
// ======= ======== ========================================================
// 1.00 22/08/13 Running code
// 1.01 22/08/13 Added some comments
// 1.02 22/08/13 Added a few more comments
//
// ********************************************^************************************************

#include <Servo.h>

Servo servo1;
Servo servo2;

#define PUSHED LOW
#define RELEASED HIGH

#define SERVOmoving true
#define SERVOstopped false

const byte heartbeatLED = 13;
const byte LED = 12;

const byte buttonPin2 = 7;
const byte buttonPin1 = 8;

const byte servo1Pin = 9;
const byte servo2Pin = 10;

boolean servoMovingFlag = SERVOstopped;

byte LEDstate = LOW;

byte lastButtonPin1;
byte lastButtonPin2;

//timing stuff
unsigned long heartbeatMillis;
unsigned long switchMillis;
unsigned long commonMillis;

unsigned long delayInterval;

//use names that mean something to you
enum Machine {StartState, StateOne, StateTwo, StateThree};
Machine mState = StartState;

// s e t u p ( )
// ********************************************^************************************************
void setup()
{
  servo1.attach(servo1Pin);
  servo2.attach(servo2Pin);

  pinMode(buttonPin1, INPUT_PULLUP);
  pinMode(buttonPin2, INPUT_PULLUP);

  pinMode(heartbeatLED, OUTPUT);
  pinMode(LED, OUTPUT);

} //END of setup()

// l o o p ( )
// ********************************************^************************************************
void loop()
{
  // ********************************* heartbeat T I M E R
  //is it time to toggle the heartbeatLED (every 500ms) ?
  if (millis() - heartbeatMillis >= 500ul)
  {
    //restart this TIMER
    heartbeatMillis = millis();

    //toggle the heartbeatLED
    digitalWrite(heartbeatLED, !digitalRead(heartbeatLED));
  }

  // ********************************* checkSwitches T I M E R
  //is it time to check the switches (every 50ms) ?
  if (millis() - switchMillis > 50)
  {
    //restart this TIMER
    switchMillis = millis();

    checkSwitches();
  }

  // *********************************
  //check our State Machine
  checkStateMachine();

  // *********************************
  //other non blocking code goes here
  // *********************************

} //END of loop()

// c h e c k S t a t e M a c h i n e ( )
// ********************************************^************************************************
//servicing the "current state" in our State Machine and doing what is required.
void checkStateMachine()
{
  switch (mState)
  {
    // *****************
    case StartState:
      {
        //do nothing
      }
      break;

    // *****************
    case StateOne:
      {
        //has the common TIMER expired ?
        if (millis() - commonMillis >= delayInterval)
        {
          servo1.write(180);
          servo2.write(180);

          //new interval for the common TIMER
          delayInterval = 1000ul;

          //restart the common TIMER
          commonMillis = millis();

          //next state in our machine
          mState = StateTwo;
        }
      }
      break;

    // *****************
    case StateTwo:
      {
        //has the common TIMER expired ?
        if (millis() - commonMillis >= delayInterval)
        {
          servo1.write(90);
          servo2.write(90);

          //new interval for the common TIMER
          delayInterval = 1000ul;

          //restart the common TIMER
          commonMillis = millis();

          //next state in our machine
          mState = StateThree;
        }
      }
      break;

    // *****************
    case StateThree:
      {
        //has the common TIMER expired ?
        if (millis() - commonMillis >= delayInterval)
        {
          servoMovingFlag = SERVOstopped;

          //next state in our machine
          mState = StartState;
        }
      }
      break;

  } //END of switch/case

} //END of checkStateMachine()

// c h e c k S w i t c h e s ( )
// ********************************************^************************************************
//this function is called every 50ms; this effectively de-bounces inuts.
void checkSwitches()
{
  byte currentState;

  // ********************************* b u t t o n P i n 1
  currentState = digitalRead(buttonPin1);

  //has the switch changed position
  if (lastButtonPin1 != currentState)
  {
    //update to the new state
    lastButtonPin1 = currentState;

    // ****************
    //if the servos are not moving, is the switch pushed ?
    if (servoMovingFlag == SERVOstopped && currentState == PUSHED)
    {
      servoMovingFlag = SERVOmoving;

      servo1.write(0);
      servo2.write(0);

      //new interval for the common TIMER
      delayInterval = 1000ul;

      //restart the common TIMER
      commonMillis = millis();

      //next state in our machine
      mState = StateOne;
    }

  } //END of buttonPin1

  // ********************************* b u t t o n P i n 2
  currentState = digitalRead(buttonPin2);

  //has the switch changed position
  if (lastButtonPin2 != currentState)
  {
    //update to the new state
    lastButtonPin2 = currentState;

    // ****************
    //is the switch pushed ?
    if (digitalRead(buttonPin2) == PUSHED)
    {
      LEDstate = !LEDstate;
      digitalWrite(LED, LEDstate);
    }

  } //END of buttonPin2

} //END of checkSwitches()

// ********************************************^************************************************

```

---

<div class="post-metadata">

**Author:** ![StefanL38](https://dub1.discourse-cdn.com/arduino/user_avatar/forum.arduino.cc/stefanl38/32/713548_2.png) [@StefanL38](https://forum.arduino.cc/u/StefanL38)\
**Post date:** [August 13, 2022, 6:15pm UTC](https://forum.arduino.cc/t/help-beginner-servo-issue/1021739/8 "2022-08-13T18:15:23Z")

</div>

@LarryD

What do you think about giving your states name that explain spot-on what is happening in the state?

Do you really think that the name **commonMillis** makes it easy to understand what the variable is used for?

best regards Stefan

---

<div class="post-metadata">

**Author:** ![LarryD](https://dub1.discourse-cdn.com/arduino/user_avatar/forum.arduino.cc/larryd/32/215550_2.png) [@LarryD](https://forum.arduino.cc/u/LarryD)\
**Post date:** [August 13, 2022, 6:27pm UTC](https://forum.arduino.cc/t/help-beginner-servo-issue/1021739/9 "2022-08-13T18:27:48Z")

</div>

_What do you think about giving your states name that explain spot-on what is happening in the state?_

**Yes** that's what should be done.

The comment says use names that mean something to you.

The generic names seen were left there **intentionally** to spur the OP to get involved.  
🙂

_Do you really think that the name **commonMillis** makes it easy to understand what the variable is used for?_

Point taken.

However, that name **suggests** the TIMER is generic, used in different places in the code.

The sketch is **purposefully** written so, when the OP reads through it, they can see that they may want to rewrite the code in a way that documents things to **their liking**.

Suppling a sketch that works is the goal, however, I try to generate a discussion with the OP to **test their commitment** and to find out **if they understand** why and what is being done.

---

<div class="post-metadata">

**Author:** ![system](https://dub1.discourse-cdn.com/arduino/user_avatar/forum.arduino.cc/system/32/1140315_2.png) [@system](https://forum.arduino.cc/u/system)\
**Post date:** [February 9, 2023, 6:27pm UTC](https://forum.arduino.cc/t/help-beginner-servo-issue/1021739/10 "2023-02-09T18:27:52Z")

</div>

This topic was automatically closed 180 days after the last reply. New replies are no longer allowed.