Analogread causing servo to misbehave

Since I went from my test situation of a switch controlling an IF routine to an analogread, things aren't working as before.

Before, if the switch was pressed, then the servo would whip back and forth.

Now, the servo sweeps only in one direction and then stays there...'stuck'.

I guess the obvious answer is because the analogpin is still reading a HIGH, but my debounce routine should preclude this, shouldn it?

THANKS

#include <Bounce.h>
#include <Wire.h>
#include "RTClib.h"
#include <Servo.h>
#include <SPI.h>         
#include <Ethernet.h>
#include <EthernetUdp.h>	

byte mac[] = {0x00, 0x11, 0x22, 0x33, 0xFB, 0x11};

EthernetClient client;

	

boolean EthernetRunning = false;			//ATTN! Not sure how this will affect things...

IPAddress server(207,7,108,203); // Joyent 

////////////SERVO////////////
Servo servo1;
////////////PIN LOCATIONS////////////
const int DoorMonitorPin = 5;			// the number of the magnetic reed switch pin 5
const int DoorBellPin = 0;				// white doorbell wire to ANALOG pin 0
const int LEDPin1 = 8;      			// the number of the LED pin 8
const int PiezoPin = 7;					// the number of the piezo buzzer pin 7
////////////DOORBELL 1 INPUT////////////
long debounce = 10000;   				// how long to leave before polling analog read again
int DoorBellPinReadValue = 0;			// Variable to store wireless doorbell read value  
long DoorBellValTriggeredMillis = 0;	// When was the analog read triggered
int Button1State = 0;					// used to set the button state
////////////LED 1 OUTPUT////////////
int LED1State  = 0;						// used to turn the LED 'on' & make it flash.
long LED1OnDuration  = 30000;			// time to keep the LED on for (30s)
long LED1TimeStamp1 = 0;				// will store last time LED was updated for state 1
long LED1TimeStamp2 = 0;       			// will store last time LED was updated for state 2
long FlashRate = 400;					// How fast should the LED flash?
////////////REED SWITCH////////////
long DoorMonitorDoorbellPressed = 0;	// Stores the millis value for the last time doorbell was rung for the reed switch routine
long DoorMonitorWaitTime = 60000;		// ms to wait to see if door is opened (1m)
int DoorMonitorWaitState = 0;			// A state where the sketch is monitoring the reed switch
int DoorMonitorPinReadValue = 0;		//Variable to store the magnetic reed switch read value  


RTC_DS1307 RTC;

enum {
  OFF, ON, FLASH
};

void setup() {
  pinMode(LEDPin1,OUTPUT);      
  pinMode(PiezoPin, OUTPUT);
  pinMode(DoorMonitorPin, INPUT);
  digitalWrite(DoorMonitorPin, HIGH);		// turn on reed input pin's pull-up resistor
  digitalWrite(14 + DoorBellPin, HIGH);		// set pullup on the analog pin
  digitalWrite(LEDPin1, OFF);
  servo1.attach(6);
  LED1State  = OFF;
  Serial.begin(57600);
  Wire.begin();
  RTC.begin();

  Serial.println("Annunciator Panel v0.9");
  Ethernet.begin(mac);	   
  if (Ethernet.begin(mac) == 0)
  {
    Serial.println("Error: No DHCP!");
    EthernetRunning = false;
  }
  if(EthernetRunning)
  {
    Udp.begin(localPort);
    Serial.println("DHCP is working so enabling UDP.");
  }
  Serial.println();
  if (! RTC.isrunning())
  {
    Serial.println("Error: The RTC is NOT available!");
  }
}

void loop() {
  DateTime now = RTC.now();
  DoorBellPinReadValue = analogRead(DoorBellPin);
  unsigned long currentMillis = millis();
  
  
  if (DoorBellPinReadValue < 100) { 
  // If the doorbell pin voltage is less    than 100 units (i.e. triggered)  
    if (currentMillis-DoorBellValTriggeredMillis > debounce)  	// and if we're not currently debouncing same
    {
      DoorBellValTriggeredMillis = currentMillis;

          // SPECIFIC TO FRONT DOOR CODE BELOW
          //====================================
      DoorMonitorDoorbellPressed = currentMillis;		//Time snapshot of when doorbell pressed. Doesn't matter if overwritten by another press.
      DoorMonitorWaitState = 1;
      //Waiting  for the door to be opened.
      //SPECIFIC TO FRONT DOOR CODE ABOVE!!

      if (Button1State == OFF)
      {
        // button pressed
        //===============
        Button1State = ON;
        if (LED1State  == OFF)
        {
          // the button's pressed and LED is OFF
          // turn it ON
          //====================================
          Serial.println("Button One pressed.");
          digitalWrite(LEDPin1, ON);
          LED1State  = ON;
          servo1.write(180);
          Serial.println("Servo: Move!");
          LED1TimeStamp1 = currentMillis;
          Serial.println();
        }
        else if (LED1State  == ON)
        { 		
          // if the button's pressed and LED is ON
          // start flashing
          //======================================
          digitalWrite(LEDPin1, OFF);
          LED1State  = FLASH;
          servo1.write(180);
          Serial.println("Servo: Move!");
          Serial.print("Button One pressed again.");
          Serial.println();
          LED1TimeStamp1 = currentMillis;	
        }
      }
    }
    else
    {
      Button1State = OFF;
      servo1.write(0);
    }
  }
  if (DoorMonitorWaitState == 1) {
    DoorMonitorPinReadValue = digitalRead(DoorMonitorPin);  
    if (DoorMonitorDoorbellPressed + DoorMonitorWaitTime > currentMillis
      ) 
    {
      if (DoorMonitorPinReadValue == HIGH ) 
      {
        DoorMonitorWaitState = 0;
        //        Serial.println("Stand easy - the door got opened!");
      }
    }
    if (DoorMonitorDoorbellPressed + DoorMonitorWaitTime < currentMillis
      ) 
    {         
      DoorMonitorWaitState = 0;
      //Serial.println("The door just went unanswered!");
    }
  }
  if (LED1TimeStamp1 + LED1OnDuration  < currentMillis) {		
    // If the predefined interval has elapsed for a button press
    // turn led OFF!
    //==================================================================
    digitalWrite(LEDPin1, OFF);
    LED1State  = OFF;
  }
  if (LED1State  == FLASH && LED1TimeStamp2 + FlashRate < currentMillis) {        	
    // If the predefined interval has elapsed for the second button    press
    // make the LED 'flash' by toggling it
    //===================================================================
    digitalWrite(LEDPin1, !digitalRead(LEDPin1));
    LED1TimeStamp2 = currentMillis;
  }
 
}

void playTone(long duration, int freq) {
  duration *= 1000;
  int period = (1.0 / freq) * 1000000;
  long elapsed_time = 0;
  while (elapsed_time < duration)
  {
    digitalWrite(PiezoPin,HIGH);
    delayMicroseconds(period / 2);
    digitalWrite(PiezoPin, LOW);
    delayMicroseconds(period / 2);
    elapsed_time += (period);
  }
}

void PrintDateTime(DateTime t) {
  char datestr[24];
  sprintf(datestr, "%04d-%02d-%02d  %02d:%02d:%02d  ", t.year(), t.month(), t.day(), t.hour(), t.minute(), t.second());
  Serial.print(datestr);
}

What kind of device is connected to the door bell pin? Switches are almost always digital devices. They are pressed or not.

PaulS:
What kind of device is connected to the door bell pin? Switches are almost always digital devices. They are pressed or not.

It's a wireless doorbell. I press doorbell button on the transmitter, signal sent wirelessly to the receiver. Receiver was connected to a speaker, it's now connected to the arduino's analog pin.

Do I need to introduce yet another variable to be 'set' by a positive analog read and then unset once the code it triggers is executed?

Receiver was connected to a speaker

And, you've used an oscilloscope to look at the signal, and have determined that you can read it effectively using an analogRead()?

PaulS:
And, you've used an oscilloscope to look at the signal, and have determined that you can read it effectively using an analogRead()?

You know me, Paul, "oscilloscope" is my middle name!

To be honest (and this isn't a dig at you, you're the man) I'm a bit fed up if I have to do that.

a) I'm following a write-up on the web of a successful implementation of this way of sending a signal, of which there are many derivatives and variants
i) I'm even using the EXACT same doorbell as used on the write-up

b) The arduino recognises processes the received signal just fine - it's just that the servo doesn't return to normal. I'm nearly there!!

c) The Arduino is widely touted as a means for ordinary folk to interface hardware and software with relative ease. On this forum I'm often advised to take precautions (zener diodes, oscilloscopes, negative voltages etc) that feel out of my reach. It feels a bit like false advertising - but then I know it isn't and you guys just know so much more than the people on the web and on make.com pushing out these tempting write-ups.

I hope you don't think less of me.

c) The Arduino is widely touted as a means for ordinary folk to interface hardware and software with relative ease. On this forum I'm often advised to take precautions (zener diodes, oscilloscopes, negative voltages etc) that feel out of my reach. It feels a bit like false advertising - but then I know it isn't and you guys just know so much more than the people on the web and on make.com pushing out these tempting write-ups.

I hope you don't think less of me.

The thing is that the Arduino is in my opinion the easiest microcontroller platform for beginners to do all kinds of fun and/or useful things. So many software library and sketches avalible, lots of projects documented and posted, etc. However there is nothing fool proof about the basic electrical properties of the AVR controller chip, applying reverse polarity signals or voltages above +5.5vdc can instantly destroy the chip. So much is dependent on a beginners knowledge of basic electronics, and if little or no experience is apparent many of us will try and stress all the bad evil things that can happen and what one should do to try and avoid those problems.

So in your example of wiring the speaker alarm output directly to an arduino analog input pin one would ask how do you know that you will wire it with the proper polarity so as to not damage the input pin. How would one know if the voltage level of the signal is more then 5.5vdc or not? That doesn't even address what kind of signal it is for proper software recognition of the signal.

So yes we sometime may come across as overly picky about wanting details before trying to give beginners possible solutions, but it's based on hearing lots of new comers with damaged boards asking for help and advice after it's a little to late to help, as the damage has already happened. :wink:

Lefty

retrolefty:
So yes we sometime may come across as overly picky about wanting details before trying to give beginners possible solutions, but it's based on hearing lots of new comers with damaged boards asking for help and advice after it's a little to late to help, as the damage has already happened. :wink:
Lefty

Thanks, Lefty. Totally understand your concern here. I have used a multimeter myself on the speaker wire, but other than that, relied on the expertise of those who designed and successfully implemented this on the web.

I've modified my code to this. I hope it will be more successful. Going to test it later on tonight.

Would be interested, though, in any thoughts on why my original doesn't work. The signal is received and can be processed as many times as the button is pushed (the debouncing works, too) it's just the servo that stopped returning to base when I implemented the analogread. Perhaps because my else brackets got dislodged. Who knows. I don't. :slight_smile:

#include <Bounce.h>
#include <Wire.h>
#include "RTClib.h"
#include <Servo.h>
#include <SPI.h>
#include <Ethernet.h>
#include <EthernetUdp.h>
byte mac[] = {0x00, 0x11, 0x22, 0x33, 0xFB, 0x11};
EthernetClient client;
////////////NTP UPDATE////////////
unsigned int localPort = 8888;			// NTP Port
byte timeServer[] = {193, 79, 237, 14};	// ntp1.nl.net NTP server
const int NTP_PACKET_SIZE= 48;			// NTP time stamp is in the first 48 bytes of the message
byte pb[NTP_PACKET_SIZE];				// Buffer to hold incoming and outgoing packets
#if ARDUINO >= 100
EthernetUDP Udp;
#endif
unsigned long currentMillis = 0;
////////////CONNECTIVITY////////////
boolean EthernetRunning = false;		//ATTN! Not sure how this will affect things...
////////////SERVER////////////
IPAddress server(207,7,108,203);		// Joyent
////////////SERVO////////////
Servo servo1;
//////////// I/O PINS////////////
const int DoorMonitorPin = 5;			// the number of the magnetic reed switch pin 5
const int DoorBellPin = 0;				// white doorbell wire to ANALOG pin 0
const int LEDPin1 = 8;      			// the number of the LED pin 8
const int PiezoPin = 7;					// the number of the piezo buzzer pin 7
////////////DOORBELL 1 INPUT////////////
long debounce = 10000;   				// how long to leave before polling analog read again
int DoorBellPinVal = 0;			// Variable to store wireless doorbell read value
long DoorBellValTriggeredMillis = 0;	// When was the analog read triggered
int Button1State = 0;					// Used to set the button state
int DoorBellTriggered = 0;				// Used to set the triggered state
////////////LED 1 OUTPUT////////////
int LED1State  = 0;						// Used to turn the LED 'on' & make it flash.
long LED1OnDuration  = 30000;			// Time to keep the LED on for (30s)
long LED1TimeStamp1 = 0;				// Will store last time LED was updated for state 1
long LED1TimeStamp2 = 0;       			// Will store last time LED was updated for state 2
long FlashRate = 400;					// How fast should the LED flash?
////////////REED SWITCH////////////
long DoorMonitorDoorbellPressed = 0;	// Stores the millis value for the last time doorbell was rung for the reed switch routine
long DoorMonitorWaitTime = 60000;		// How many ms to wait to see if door is opened (1m)
int DoorMonitorWaitState = 0;			// The sketch is monitoring the reed switch if this is '1'
int DoorMonitorPinReadValue = 0;		// Stores the magnetic reed switch read value
////////////RTC////////////
RTC_DS1307 RTC;
enum {
  OFF, ON, FLASH
};
void setup() {
  pinMode(LEDPin1,OUTPUT);
  pinMode(PiezoPin, OUTPUT);
  pinMode(DoorMonitorPin, INPUT);
  digitalWrite(DoorMonitorPin, HIGH);		// turn on reed input pin's pull-up resistor
  digitalWrite(14 + DoorBellPin, HIGH);		// set pullup on the analog pin
  digitalWrite(LEDPin1, OFF);
  servo1.attach(6);
  LED1State  = OFF;
  Serial.begin(57600);
  Wire.begin();
  RTC.begin();

  Serial.println("Annunciator Panel v0.9");
  Ethernet.begin(mac);
  if (Ethernet.begin(mac) == 0)
  {
    Serial.println("Error: No DHCP!");
    EthernetRunning = false;
  }
  if(EthernetRunning)
  {
    Udp.begin(localPort);
    Serial.println("DHCP is working so enabling UDP.");
  }
  Serial.println();
  if (! RTC.isrunning())
  {
    Serial.println("Error: The RTC is NOT available!");
  }
}
void loop() {
  DateTime now = RTC.now();
  currentMillis = millis();
  DoorBellPinVal = analogRead(DoorBellPin);
  
  if (DoorBellPinVal < 100) {
    if (currentMillis-DoorBellValTriggeredMillis > debounce)  	// And if we're not within debounce time
    {
      DoorBellTriggered = 1;
	  DoorBellValTriggeredMillis = currentMillis;
	}
  }
  if (DoorbellTriggered == 1) {
	  DoorBellTriggered = 0;
      DoorMonitorDoorbellPressed = currentMillis;				//Time snapshot of when doorbell pressed. Doesn't matter if overwritten by another press.
      DoorMonitorWaitState = 1;      							//Waiting  for the door to be opened.
      if (Button1State == OFF)
      {
        // button pressed
        //===============
        Button1State = ON;
        if (LED1State  == OFF)
        {
          // the button's pressed and LED is OFF
          // turn it ON
          //====================================
          Serial.println("Button One pressed.");
          digitalWrite(LEDPin1, ON);
          LED1State  = ON;
          servo1.write(180);
          Serial.println("Servo: Move!");
          LED1TimeStamp1 = currentMillis;
          Serial.println();
        }
        else if (LED1State  == ON)
        {
          // if the button's pressed and LED is ON
          // start flashing
          //======================================
          digitalWrite(LEDPin1, OFF);
          LED1State  = FLASH;
          servo1.write(180);
          Serial.println("Servo: Move!");
          Serial.print("Button One pressed again.");
          Serial.println();
          LED1TimeStamp1 = currentMillis;
        }
      }
    else
    {
      Button1State = OFF;
      servo1.write(0);
    } 
}	
  if (DoorMonitorWaitState == 1) {
    DoorMonitorPinReadValue = digitalRead(DoorMonitorPin);
    if (DoorMonitorDoorbellPressed + DoorMonitorWaitTime > currentMillis
      )
    {
      if (DoorMonitorPinReadValue == HIGH )
      {
        DoorMonitorWaitState = 0;
        //        Serial.println("Stand easy - the door got opened!");
      }
    }
    if (DoorMonitorDoorbellPressed + DoorMonitorWaitTime < currentMillis
      )
    {
      DoorMonitorWaitState = 0;
      //Serial.println("The door just went unanswered!");
    }
  }
  if (LED1TimeStamp1 + LED1OnDuration  < currentMillis) {
    // If the predefined interval has elapsed for a button press
    // turn led OFF!
    //==================================================================
    digitalWrite(LEDPin1, OFF);
    LED1State  = OFF;
  }
  if (LED1State == FLASH && LED1TimeStamp2 + FlashRate < currentMillis) {
    // If the predefined interval has elapsed for the second button    press
    // make the LED 'flash' by toggling it
    //===================================================================
    digitalWrite(LEDPin1, !digitalRead(LEDPin1));
    LED1TimeStamp2 = currentMillis;
  }
}
  // set all bytes in the buffer to 0
  memset(pb, 0, NTP_PACKET_SIZE);
  // Initialize values needed to form NTP request
  // (see URL above for details on the packets)
  pb[0] = 0b11100011;   // LI, Version, Mode
  pb[1] = 0;     // Stratum, or type of clock
  pb[2] = 6;     // Polling Interval
  pb[3] = 0xEC;  // Peer Clock Precision
  // 8 bytes of zero for Root Delay & Root Dispersion
  pb[12]  = 49;
  pb[13]  = 0x4E;
  pb[14]  = 49;
  pb[15]  = 52;

  // all NTP fields have been given values, now
  // you can send a packet requesting a timestamp:
  Udp.beginPacket(address, 123); //NTP requests are to port 123
  Udp.write(pb,NTP_PACKET_SIZE);
  Udp.endPacket();
}
Servo servo1;

This does not even begin to hint at what the servo is doing. A more meaningful name would not be a waste.

  servo1.attach(6);

Which is a PWM pin. Does attaching the servo to another, non-PWM, pin make any difference?

  digitalWrite(14 + DoorBellPin, HIGH);		// set pullup on the analog pin

But, you are not using the analog pin as a digital pin, so why are you doing this?

  if (DoorBellPinVal < 100) {
    if (currentMillis-DoorBellValTriggeredMillis > debounce)  	// And if we're not within debounce time
    {
      DoorBellTriggered = 1;
	  DoorBellValTriggeredMillis = currentMillis;
	}
  }

And, if DoorBellPinVal is > 100, what happens? If the interval is less than debounce, what happens?

When I'm writing code, if(something) is immediately followed by {, }, else, {, and } follow immediately. Then, I determine what code belongs in each block. Having the empty else block stare at me ensures that I don't forget to deal with the not true condition.

Sometimes that simply means deleting the else block, but its presence serves to remind me that I need to consider the case.

The else block to move the servo back is attached to the if(Button1State) statement. Again, I'd like to encourage you to use more meaningful names. I don't see where Button1State is assigned a value before it is used, nor do I know what it means. So, I can't tell if the else statement is in the right place, or not.

The mix of styles, with some { on new lines and some on the line with the if statement, and the random indentation do not help with seeing the structure of the program. Pick one style (I prefer the new line style myself) and stick with it. Use Tools + Auto format again (and again and again, if necessary as you revise the code.

PaulS:

  digitalWrite(14 + DoorBellPin, HIGH);		// set pullup on the analog pin

But, you are not using the analog pin as a digital pin, so why are you doing this?

Good question. I am blindly following Roo Reynold's code. It's what he did, and it has worked well up to now. I'll take this bit out and see what happens.

PaulS:

  if (DoorBellPinVal < 100) {

if (currentMillis-DoorBellValTriggeredMillis > debounce)  // And if we're not within debounce time
    {
      DoorBellTriggered = 1;
  DoorBellValTriggeredMillis = currentMillis;
}
  }



And, if DoorBellPinVal is > 100, what happens? If the interval is less than debounce, what happens?

Nothing happens and nothing is supposed to happen. Do I need to cater for these situations if I don't want anything to change/happen?

PaulS:
The else block to move the servo back is attached to the if(Button1State) statement. Again, I'd like to encourage you to use more meaningful names. I don't see where Button1State is assigned a value before it is used, nor do I know what it means. So, I can't tell if the else statement is in the right place, or not.

I'll look at Button1State to see if I can name it better and if it's necessary at all.

PaulS:
The mix of styles, with some { on new lines and some on the line with the if statement, and the random indentation do not help with seeing the structure of the program. Pick one style (I prefer the new line style myself) and stick with it. Use Tools + Auto format again (and again and again, if necessary as you revise the code.

BIT sad about this, because I've been really working at my indentation since your last chiding. I'll try to do better.

Nothing happens and nothing is supposed to happen. Do I need to cater for these situations if I don't want anything to change/happen?

No, you don't. But, are you sure that nothing should happen?

If the reading is below some value, you set DoorBellTriggered to 1. If the reading is above that value, it seems to me that the door bell was not triggered, so DoorBellTriggered should be set to 0, shouldn't it?

PaulS:

Nothing happens and nothing is supposed to happen. Do I need to cater for these situations if I don't want anything to change/happen?

No, you don't. But, are you sure that nothing should happen?

If the reading is below some value, you set DoorBellTriggered to 1. If the reading is above that value, it seems to me that the door bell was not triggered, so DoorBellTriggered should be set to 0, shouldn't it?

Thanks, Paul - I wonder if that's the clue to this whole servo episode...

What I've done is a) improved my naming and b) compared my code from before when I was using a push button and my code now that I've integrated my analog reading.

To recap, before, the servo swung to 180deg and then swung straight back. Ideal for ringing a bell on a button push. This is my desired result.

I've noticed two things:-

Previous code

if (nurseryvalue == HIGH)
  {
    if (FrontDoorRoutine == OFF)
    {
      // button pressed
      //===============
      FrontDoorRoutine = ON;
      if (FrontDoorLED == OFF)
      {
        // the button's pressed and LED is OFF
        // turn it ON as this is a "first press".
        //====================================
        digitalWrite(ledPin1, ON);
        FrontDoorLED = ON;
        RingBellServo.write(180);
        FrontDoorLEDTimeStamp1 = currentMillis;
      }
      else if (FrontDoorLED == ON)
      { 		
        // the button's pressed and LED is ON
        // start flashing as this is a "second press".
        //======================================
        digitalWrite(ledPin1, OFF);
        FrontDoorLED = FLASH;
        RingBellServo.write(180);
        FrontDoorLEDTimeStamp1 = currentMillis;	
      }
    }
  }
  else
  {
    FrontDoorRoutine = OFF;
    RingBellServo.write(0);
  }

Current code

if (DoorBellTriggered == 1)
  {
    if (FrontDoorRoutine == OFF)
    {
      // button pressed
      //===============
      FrontDoorRoutine = ON;
	  if (FrontDoorLED == OFF)
      {
        // the button's pressed and LED is OFF
        // turn it ON as this is a "first press".
        //====================================
        digitalWrite(LEDPin1, ON);
        FrontDoorLED = ON;
        RingBellServo.write(180);
		FrontDoorLEDTimeStamp1 = currentMillis;
        DoorBellTriggered = 0;
      }
      else if (FrontDoorLED == ON)
      {
        // the button's pressed and LED is ON
        // start flashing as this is a "second press".
        //======================================
        digitalWrite(LEDPin1, OFF);
        FrontDoorLED = FLASH;
        RingBellServo.write(180);
        FrontDoorLEDTimeStamp1 = currentMillis;
        DoorBellTriggered = 0;
      }
    }
    else
    {
      FrontDoorRoutine = OFF;
	  RingBellServo.write(0);
    } 
  }
  1. I seem to have lost a closing bracket in my newer code after the else if routine. Surely this is adversely affecting how/when/if the else routine is operating.
  2. I think I'm resetting DoorBellTriggered in the wrong place. Perhaps it would be better to reset it straight after the if routine that checks for DoorBellTriggered being true is found to be true.

How should a servo operate?

  1. Should it:
    a) move to a position and stay there
    or
    b) should it move back when it's reached that position?
  2. If it's told to move to position 180 and then in the very next line told to move to position 0, should it complete the first move before doing the next, or does having two instructions so close together effectively cancel the first move out?

THANKS

	  if (FrontDoorLED == OFF)
      {
     }
      else if (FrontDoorLED == ON)
      {

If FrontDoorLED is not OFF, what else could it be besides ON?

Some of your names still leave a bit to be desired. DoorBellTriggered is a great name. It implies that something might have happened to get us to the point that we need to do something. Then, we encounter FrontDoorRoutine. No clue what this is supposed to signify, but if it is OFF (whatever off means), we do something (that results in the servo being moved one way). If not, we move the servo the other way.

DoorBellTriggered is compared to 1 and set to 0. FrontDoorRoutine is compared to ON and set to OFF. The inconsistencies make the code hard to follow. The snippet makes it impossible to tell what type the twp variables are, or what type and value ON and OFF are and have.

There is, in the if(FrontDoorLED == OFF) and else if(FrontDoorLED == ON) blocks, the same code. That code should be moved outside the if/else if blocks.

I seem to have lost a closing bracket in my newer code after the else if routine. Surely this is adversely affecting how/when/if the else routine is operating.

The else if should not be an else if. It should be simply an else. Yes, it does look like you are doing things differently. The random indents make it impossible to determine what that is, though.

How should a servo operate?

  1. Should it:
    a) move to a position and stay there
    or
    b) should it move back when it's reached that position?
  2. If it's told to move to position 180 and then in the very next line told to move to position 0, should it complete the first move before doing the next, or does having two instructions so close together effectively cancel the first move out?

The Servo.write() methods starts the servo moving, and returns. If you issue another one before the servo has had time to move to the position defined on the first call, it will stop trying to get there, and go to the position defined in the second call. You need to add a delay() (or the millis() equivalent) between the two calls.

I would like to say that this segment of my code is based on reply 23 from this thread. I've made some changes but the else/else ifs took their initial form there.

PaulS:
If FrontDoorLED is not OFF, what else could it be besides ON?

It could be flashing (FLASH). Are you saying I need to cater for this state? It seems to work well without this, but perhaps I haven't thoroughly tested if a third button press is ignored as it should be.

PaulS:
Then, we encounter FrontDoorRoutine. No clue what this is supposed to signify

I' m having real trouble thinking of a different name, as this is a variable used to see whether the LED is currently off, on or flashing. I've changed it to FrontDoorLEDStateManager but I doubt you'll like this state any better!

PaulS:
if it is OFF (whatever off means)

This confused me as well. I don't see how it can work to set an int to ON, OFF or FLASH - but this appears to be done using 'enum'.

PaulS:
There is, in the if(FrontDoorLED == OFF) and else if(FrontDoorLED == ON) blocks, the same code. That code should be moved outside the if/else if blocks.

With respect, it's not exactly the same, and I don't see how things could continue to work as they do if I do as you suggest. However, I am likely wrong.

PaulS:
The else if should not be an else if. It should be simply an else.

Again, with respect, not sure about this. There are three conditions I want to cater for: routine not on, routine on and routine flashing. Surely if I make it an 'else', I reduce my ability to check to two conditions, off and on. I wouldn't be able to deal with a situation whereby the button is pressed and the routine is in 'flash' mode.

PaulS:
The Servo.write() methods starts the servo moving, and returns.

Aha, so telling the servo to move to position 180 will in fact cause the servo to do that, and then return to its dormant position once it reaches 180? In that case I don't need to 'tell' the servo to return...

THANK YOU PAUL!

The Servo.write() methods starts the servo moving, and returns.

Aha, so telling the servo to move to position 180 will in fact cause the servo to do that, and then return to its dormant position once it reaches 180?

No, that's not what Paul meant. He meant that the Servo.write() call returns immediately. It doesn't wait for the servo to finish moving. When a servo is set to a position, it stays there until it is told to move to a different one. If you set a different position before the servo arrives at the first one, it starts to move toward the new position straight away.

General purpose servos don't have a 'dormant' position.

Many thanks for your informed clarification!

It could be flashing (FLASH).

OK, so then ignore everything I said about the else if. Since there clearly is another possible value, the code you have is correct (at least as far as the else if statement is concerned).

I' m having real trouble thinking of a different name

FrontDoorState would work for me.

With respect, it's not exactly the same

if (FrontDoorLED == OFF)
{
// the button's pressed and LED is OFF
// turn it ON as this is a "first press".
//====================================
digitalWrite(ledPin1, ON);
FrontDoorLED = ON;
RingBellServo.write(180);
FrontDoorLEDTimeStamp1 = currentMillis;
}
else if (FrontDoorLED == ON)
{
// the button's pressed and LED is ON
// start flashing as this is a "second press".
//======================================
digitalWrite(ledPin1, OFF);
FrontDoorLED = FLASH;
RingBellServo.write(180);
FrontDoorLEDTimeStamp1 = currentMillis;
}
Looks the same to me. Not all of the code is the same, but the duplicate code doesn't need to be there. On the other hand, maybe what's needed is an else block, to handle the case where FrontDoorLED == FLASH. If you add that, then you can decide whether there is anything in all three blocks that is the same, and move that stuff outside of the if/else if/else structure.

Again, with respect, not sure about this.

Well, you should be. It's your code, and you are right.

If I add a 1 second delay to my sketch after telling the servo where to go it works as expected.

Clearly the new analog reach version was for some reason working quicker (too quickly) and not giving the servo time to react.