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:
#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.
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.
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
#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
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.
// 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()
//********************************************^************************************************
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.
