Possible code error?

Hi all,

A few years back I wrote a basic program for an Arduino Uno which reads analog inputs and starts two different events based on these inputs. Besides delays, switches and indicator lights that's all there is to it. It worked for a little while, but there has always been periods where it wouldn't start any of the two events.

Due to the fact it's my first code and I haven't touched it since, i'm having difficulty fault finding. If anyone can take a look over it and tell me why void Ab() and void aB() won't run that would be great. I've established that it isn't a faulty relay through checking if it was being activated on the serial monitor.

I'm an electrician by trade and the program is for a tool I developed for use at work. For anyone familiar with a voltstick or non-contact voltage tester - it's basically two voltsticks attached to a box with a siren. The box is set up attached to cables in a roofspace, and when one of the voltsticks detects power loss (breaker switched off at the switchboard) , an event will start which will send a different tone to the siren via a relay depending on which stick has lost power. They also work individually. Here's a video showing the basic functions.

Some more info..
The two black buttons don't interact with the arduino, they're connected directly to the siren I ripped from a bike horn.
The red button is a reset button for the 10minute delay.

Code Below

// ONE MAN BAND
// dual voltstick siren notification system

int switchA = 13;                // the first switch
int switchB = 12;               // the second switch
int relay = 11;                 // the relay controlling siren
int ledA = 10;                  // the first LED
int ledB = 9;                   // the second LED
int resetButton = 8;            // reset siren button
int valueA;                     // value of switch A
int valueB;                     // value of switch B

unsigned long changeTime;              //time since reset button pressed
unsigned long delayStart_Ab_Init = 0;  // the time the delay started before Ab is triggered
bool delayRunning_Ab_Init = false;      // true if still waiting for Ab trigger delay to finish
unsigned long delayStart_Ab = 0;  // the time the delay started Ab
bool delayRunning_Ab = false;      // true if still waiting for Ab delay to finish
unsigned long delayStart_aB_Init = 0;  // the time the delay started before aB is triggered
bool delayRunning_aB_Init = false;      // true if still waiting for aB trigger delay to finish
unsigned long delayStart_aB = 0;  // the time the delay started aB
bool delayRunning_aB = false;      // true if still waiting for aB delay to finish



void setup(){  
  Serial.begin(9600);               // initialize serial communication with computer:
  pinMode (switchA,INPUT_PULLUP);           // setting switch 1 as input
  pinMode (switchB,INPUT_PULLUP);           // setting switch 2 as input
  pinMode (relay,OUTPUT);             // setting relay controlling siren as output
  pinMode (ledA,OUTPUT);           // setting led A as output
  pinMode (ledB,OUTPUT);           // setting led B as output
  pinMode (resetButton,INPUT);      // setting reset button as input
  digitalWrite(relay,LOW);           // set relay low
  digitalWrite(ledA,LOW);           // set led A low
  digitalWrite(ledB,LOW);           // set led B low
}


void loop(){

// establishing analog readings
  (void)analogRead(A0);
  int grounded1 = analogRead(A0);
  (void)analogRead(A1);
  int stickA = analogRead(A1);
  (void)analogRead(A2);
  int grounded2 = analogRead(A2);
  (void)analogRead(A3);
  int stickB = analogRead(A3);
  (void)analogRead(A4);
  int grounded3 = analogRead(A4);
  (void)analogRead(A5);
  int grounded4 = analogRead(A5);
  int relayValue = digitalRead(1);

// reset siren button
  int state = digitalRead(resetButton);
       if(state == HIGH && (millis() - changeTime)> 1000){
       delayRunning_Ab = false;    // stop delay
       delayRunning_aB = false;
       }

// led triggering
  if (stickA>800) {digitalWrite(ledA, HIGH);}
       else {digitalWrite(ledA,LOW);}
  if (stickB>800) {digitalWrite(ledB, HIGH);}
       else {digitalWrite(ledB,LOW);}

// reset all event delays after 10 minutes
  if (delayRunning_Ab && ((millis() - delayStart_Ab) >= 600000)) {
  delayRunning_Ab = false; // // prevent this code being run more then once Ab
  }
  if (delayRunning_aB && ((millis() - delayStart_aB) >= 600000)) {
  delayRunning_aB = false; // // prevent this code being run more then once aB
  }


// event triggering

  // if A is OFF and b is on, start 1 second timer to allow for verification, then start Ab event (RED)
  if (delayRunning_Ab_Init && ((millis() - delayStart_Ab_Init) >= 1000)) {
    Ab();
  }
      valueA = digitalRead(switchA);
      if ((delayRunning_Ab == false) && (delayRunning_Ab_Init == false) && (stickA<800) && (valueA == 1)) {
        delayStart_Ab_Init = millis();             // start timer for function delay Ab initialisation
        delayRunning_Ab_Init = true;    // start Ab initialisation delay
  }


  // if a is on and B is OFF, start 1 second timer to allow for verification, then start aB event (BLUE)
  if (delayRunning_aB_Init && ((millis() - delayStart_aB_Init) >= 1000)) {
    aB();
  }
      valueB = digitalRead(switchB);
      if ((delayRunning_aB == false) && (delayRunning_aB_Init == false) && (stickB<800) && (valueB == 1)) {
        delayStart_aB_Init = millis();             // start timer for function delay aB initialisation
        delayRunning_aB_Init = true;    // start aB initialisation delay
  }

// printing to serial monitor
  Serial.print(" a ");
  Serial.print(stickA);
  Serial.print(" b ");
  Serial.println(stickB);
  Serial.println(relayValue);

  delay(50);
}


void Ab() {
  delayRunning_Ab_Init = false; 
  digitalWrite(relay,HIGH);
  delay(2000);
  digitalWrite(relay,LOW);
  delay(1000);
     digitalWrite(relay,HIGH);
     delay(500);
  digitalWrite(relay,LOW);
  delay(500);
     digitalWrite(relay,HIGH);
     delay(500);
  digitalWrite(relay,LOW);
delayStart_Ab = millis();             // start timer for function delay Ab
delayRunning_Ab = true;    // start delay
changeTime = millis();
return;
}


void aB() {
  delayRunning_aB_Init = false; 
  digitalWrite(relay,HIGH);
  delay(2000);
  digitalWrite(relay,LOW);
  delay(1000);
     digitalWrite(relay,HIGH);
     delay(500);
  digitalWrite(relay,LOW);
  delay(500);
     digitalWrite(relay,HIGH);
     delay(500);
  digitalWrite(relay,LOW);
  delay(500);
     digitalWrite(relay,HIGH);
     delay(500);
  digitalWrite(relay,LOW);
delayStart_aB = millis();             // start timer for function delay aB
delayRunning_aB = true;    // start delay
changeTime = millis();
return;
}

Welcome to the forum

Please post your code here to make it easier to copy for examination rather than attaching it

Please follow the advice given in the link below when posting code, in particular the section entitled 'Posting code and common code problems'

Use code tags (the </> icon above the compose window) to make it easier to read and copy for examination

1 Like

int is only 2 bytes on uno

1 Like

How is everything wired up? Your reset button is declared as INPUT which requires it to have an external pull-up or pull-down resistor. Is one installed? A schematic, even hand drawn, would be helpful.

1 Like

Hello alexdyo
Mixing the delay() and millis() functions is not a good approach.
You can replace these delay() calls:

	Line 108:   delay(50);
	Line 115:   delay(2000);
	Line 117:   delay(1000);
	Line 119:      delay(500);
	Line 121:   delay(500);
	Line 123:      delay(500);
	Line 135:   delay(2000);
	Line 137:   delay(1000);
	Line 139:      delay(500);
	Line 141:   delay(500);
	Line 143:      delay(500);
	Line 145:   delay(500);
	Line 147:      delay(500);

Keep it simple and stupid.
To avoid using the delay() function, which blocks the expected real-time behaviour, design your own timer manager.
It is recommended to start with the mother of all Arduino timers, the IDE's BLINKWITHOUTDELAY example.
Using this example, you can see how millis() function works and extend this with some additional functions.

  1. startTimer() --> starts the timer
  2. stopTimer() --> stops the timer
    and
  3. actionTimer() --> performs an action when the timer is triggered.

Have a nice day and enjoy programming in C++ and learning.

1 Like

Thanks for the replies all,

I'll tackle the suggestions above, and failing that, I'll post again with a hand drawn schematic. Currently I only have very crude drawings that don't correlate with the "as-built" tool.

Would that be a problem as it's an unsigned long?

Yes it uses a resistor

@alexdyo

before answering you should mark the question.
This will make a clickable popup "qoute" visible
simply click on "quote" which will open a new posting with the marked text qouted automatically

I started analysing your code. What is not yet clear to me is what does

valueA == 1)

mean?
switch is in a position the channel shall be activated or de-activated?

I don't know what you mean with that?
To which variable are you referring to?

what does it mean if

stickA < 800

is true?
Does it mean no voltage detected?
best regards Stefan

@StefanL38

I started analysing your code. What is not yet clear to me is what does
valueA == 1)
mean?
switch is in a position the channel shall be activated or de-activated?

ValueA is a digitalRead(switchA);
1 or 0 depending on if the switch has been flicked to activate the ability for the function for voltstick A to start. If switchA is 0, then it won't start the siren function. This allows for me to use stick A or B individually without the other one triggering.

[quote="alexdyo, post:8, topic:1015559"]
Would that be a problem as it's an unsigned long?
[/quote]
I don't know what you mean with that?
To which variable are you referring to?

I'm referring to killzone_kid referencing the code under "// reset all event delays after 10 minutes"

what does it mean if
stickA < 800
is true?
Does it mean no voltage detected?

If either stick falls under 800, it means that voltage has been lost. The analog input is run from the voltstick and through an optocoupler. It is usually 900-1023 when voltage is detected, and 200-600 when no voltage is detected. This is the only way I could effectively get the fluke voltmeter to send a signal to the Arduino that was readable.

Thanks!

in the line

  if (delayRunning_Ab && ((millis() - delayStart_Ab) >= 600000)) {
  }

you use a simple number.
as an arduino uno is a 8bit microcontroller big numbers are represented as int which is 16bit
The largest number a 16-bit-vlaue can hold is 2^16 = 65536
with sign-bit just 2^15 = 32768

This means such a large number like 600000 is not represented correctly
use 600000UL instead

to me personal your variable-names are very confusing. Especially
delayRunning_Ab
delayRunning_Ab_Init
These names to not reflect what they are used for

@StefanL38

Great advice, didn't know that was the case. Already knocked it down to 2000000 but i'll throw a UL at the end for good measure.

Yeah the whole code is a disaster. The next iteration i'll start from scratch and try to make it more legible. For now hopefully the integer issue solves my problem.

Took all the fluff out of the code to see if I could even get the siren to trigger. Still doesn't work.

// ONE MAN BAND
// dual voltstick siren notification system

int switchA = 13;                // the first switch
int switchB = 12;               // the second switch
int relay = 11;                 // the relay controlling siren
int ledA = 10;                  // the first LED
int ledB = 9;                   // the second LED
int valueA;                     // value of switch A
int valueB;                     // value of switch B



void setup(){  
  Serial.begin(9600);               // initialize serial communication with computer:
  pinMode (switchA,INPUT_PULLUP);           // setting switch 1 as input
  pinMode (switchB,INPUT_PULLUP);           // setting switch 2 as input
  pinMode (relay,OUTPUT);             // setting relay controlling siren as output
  pinMode (ledA,OUTPUT);           // setting led A as output
  pinMode (ledB,OUTPUT);           // setting led B as output
  digitalWrite(relay,LOW);           // set relay low
  digitalWrite(ledA,LOW);           // set led A low
  digitalWrite(ledB,LOW);           // set led B low
}


void loop(){

// establishing analog readings
  (void)analogRead(A0);
  int grounded1 = analogRead(A0);
  (void)analogRead(A1);
  int stickA = analogRead(A1);
  (void)analogRead(A2);
  int grounded2 = analogRead(A2);
  (void)analogRead(A3);
  int stickB = analogRead(A3);
  (void)analogRead(A4);
  int grounded3 = analogRead(A4);
  (void)analogRead(A5);
  int grounded4 = analogRead(A5);


// led triggering
  if (stickA>800) {digitalWrite(ledA, HIGH);}
       else {digitalWrite(ledA,LOW);}
  if (stickB>800) {digitalWrite(ledB, HIGH);}
       else {digitalWrite(ledB,LOW);}


// event triggering

  // if A is OFF and b is on, start 1 second timer to allow for verification, then start Ab event (RED)
   valueA = digitalRead(switchA);
      if (stickA<800 && (valueA == 1)) {
        Ab();
  }


  // if a is on and B is OFF, start 1 second timer to allow for verification, then start aB event (BLUE)
   valueB = digitalRead(switchB);
      if (stickB<800 && (valueB == 1)) {
        aB();
  }

// printing to serial monitor
  Serial.print(" a ");
  Serial.print(stickA);
  Serial.print(" b ");
  Serial.println(stickB);
  delay(50);
}


void Ab() {
  digitalWrite(relay,HIGH);
  delay(2000);
  digitalWrite(relay,LOW);
  delay(1000);
     digitalWrite(relay,HIGH);
     delay(500);
  digitalWrite(relay,LOW);
  delay(500);
     digitalWrite(relay,HIGH);
     delay(500);
  digitalWrite(relay,LOW);
return;
}
if (delayRunning_Ab && ((millis() - delayStart_Ab) >= 600000)) {
  }

Actually, this should be okay. 600000 is a literal constant forced to type 'long', and the comparison expression is forced to unsigned long because the expression containing millis() is an unsigned long.

The common error would be something like

if (delayRunning_Ab && ((millis() - delayStart_Ab) >= 60 * 10000)) {
  }

where the expression 60 * 10000 is evaluated as an int by default, since the literal constants 60 and 10000 are evaluated as int type.

The Uno has only one ADC that is switched between analogue inputs as and when required. This could result in consecutive readings from 2 different analogue pins not to be as accurate as they could be.

One solution to this is to take a reading from a pin then discard it and take a second reading from the same pin and use that. The theory is that this allows the ADC to stabilise when switching between pins

The lines such as the one that you asked about are the dummy first readings

1 Like

Here is a code-version with variable names that reflect what the variables are used for and / or when the variables were set "active"

I also added two macros at the top that can be used for serial debugging.

// MACRO-START * MACRO-START * MACRO-START * MACRO-START * MACRO-START * MACRO-START * 

// a detailed explanation how these macros work is given in this tutorial
// https://forum.arduino.cc/t/comfortable-serial-debug-output-short-to-write-fixed-text-name-and-content-of-any-variable-code-example/888298

#define dbg(myFixedText, variableName) \
  Serial.print( F(#myFixedText " "  #variableName"=") ); \
  Serial.println(variableName);
// usage: dbg("1:my fixed text",myVariable);
// myVariable can be any variable or expression that is defined in scope

#define dbgi(myFixedText, variableName,timeInterval) \
  do { \
    static unsigned long intervalStartTime; \
    if ( millis() - intervalStartTime >= timeInterval ){ \
      intervalStartTime = millis(); \
      Serial.print( F(#myFixedText " "  #variableName"=") ); \
      Serial.println(variableName); \
    } \
  } while (false);
// usage: dbgi("2:my fixed text",myVariable,1000);
// myVariable can be any variable or expression that is defined in scope
// third parameter is the time in milliseconds that must pass by until the next time a 
// Serial.print is executed
// end of macros dbg and dbgi
// MACRO-END * MACRO-END * MACRO-END * MACRO-END * MACRO-END * MACRO-END * MACRO-END * 


// ONE MAN BAND
// dual voltstick siren notification system

#define activated   1
#define DEactivated 0

const int voltageThreshold = 800;

int switchA = 13;                // the first switch
int switchB = 12;               // the second switch
int relay = 11;                 // the relay controlling siren
int ledA = 10;                  // the first LED
int ledB = 9;                   // the second LED
int resetButton = 8;            // reset siren button
int switchA_State;                     // value of switch A
int switchB_State;                     // value of switch B

unsigned long changeTime;              //time since reset button pressed

unsigned long voltage_Ab_off_detected = 0;      // the time the delay started before Ab is triggered
bool          waitBeforeBeep_Ab_starts = false; // true if still waiting for Ab trigger delay to finish

bool          deactivate_Ab_beeping = false;   // true if still waiting for Ab delay to finish
unsigned long AbFinishedBeeping = 0;                // the time the delay started Ab


unsigned long voltage_aB_off_detected = 0;      // the time the delay started before aB is triggered
bool          waitBeforeBeep_aB_starts = false; // true if still waiting for aB trigger delay to finish

bool          deactivate_aB_beeping = false;   // true if still waiting for aB delay to finish
unsigned long aB_FinishedBeeping = 0;                // the time the delay started aB

unsigned long multiPurpTimer = 0;

void PrintFileNameDateTime() {
  Serial.println( F("Code running comes from file ") );
  Serial.println( F(__FILE__) );
  Serial.print( F("  compiled ") );
  Serial.print( F(__DATE__) );
  Serial.print( F(" ") );
  Serial.println( F(__TIME__) );  
}


void setup() {
  Serial.begin(9600);               // initialize serial communication with computer:
  Serial.println("Setup-Start");
  PrintFileNameDateTime();
  pinMode (switchA, INPUT_PULLUP);          // setting switch 1 as input
  pinMode (switchB, INPUT_PULLUP);          // setting switch 2 as input
  pinMode (relay, OUTPUT);            // setting relay controlling siren as output
  pinMode (ledA, OUTPUT);          // setting led A as output
  pinMode (ledB, OUTPUT);          // setting led B as output
  pinMode (resetButton, INPUT);     // setting reset button as input
  digitalWrite(relay, LOW);          // set relay low
  digitalWrite(ledA, LOW);          // set led A low
  digitalWrite(ledB, LOW);          // set led B low
}



void loop() {
  dbgi("0: top of loop",0,500);
  // establishing analog readings
  int grounded1 = analogRead(A0);
  int stickA = analogRead(A1);

  int grounded2 = analogRead(A2);
  int stickB = analogRead(A3);

  int grounded3 = analogRead(A4);
  int grounded4 = analogRead(A5);

  int relayValue = digitalRead(1);

  // reset siren button
  int state = digitalRead(resetButton);
  if (state == HIGH && (millis() - changeTime) > 1000) {
    deactivate_Ab_beeping = false;    // stop delay
    deactivate_aB_beeping = false;
  }

  // led triggering
  if (stickA > voltageThreshold) {
    digitalWrite(ledA, HIGH);
  }
  else {
    digitalWrite(ledA, LOW);
  }
  if (stickB > voltageThreshold) {
    digitalWrite(ledB, HIGH);
  }
  else {
    digitalWrite(ledB, LOW);
  }

  // reset all event delays after 10 minutes
  if (deactivate_Ab_beeping && ((millis() - AbFinishedBeeping ) >= 600000UL  )) {
    dbg("1 Ab:deactivate_Ab_beeping = false",1);
    deactivate_Ab_beeping = false; // // prevent this code being run more then once Ab
  }

  if (deactivate_aB_beeping && ((millis() - aB_FinishedBeeping ) >= 600000UL)) { //xxy
    dbg("2 aB:deactivate_aB_beeping = false",1);
    deactivate_aB_beeping = false; // // prevent this code being run more then once aB
  }


  // event triggering

  // if A is OFF and b is on, start 1 second timer to allow for verification, then start Ab event (RED)
  if (waitBeforeBeep_Ab_starts && ((millis() - voltage_Ab_off_detected ) >= 1000)) {
    Ab();
  }

  switchA_State = digitalRead(switchA);

  if (switchA_State == activated) {
    dbgi("4:switchA" ,switchA_State,500);
    
    if ((deactivate_Ab_beeping == false) && (waitBeforeBeep_Ab_starts == false) && (stickA < voltageThreshold) && (switchA_State == activated)) {
      waitBeforeBeep_Ab_starts = true;    // start Ab initialisation delay
      voltage_Ab_off_detected = millis(); // start timer that delays executing Ab 
      dbg("5: Ab:",voltage_aB_off_detected);
    }
  }

  // if a is on and B is OFF, start 1 second timer to allow for verification, then start aB event (BLUE)
  if (waitBeforeBeep_aB_starts && ((millis() - voltage_aB_off_detected) >= 1000)) {
    aB();
  }
  
  switchB_State = digitalRead(switchB);
  if (switchB_State == activated) {
    dbgi("6:switchB" ,switchB_State,500);
    
    if ((deactivate_aB_beeping == false) && (waitBeforeBeep_aB_starts == false) && (stickB < voltageThreshold)) {
      waitBeforeBeep_aB_starts = true;    // start aB initialisation delay
      voltage_aB_off_detected = millis(); // start timer to delay execution of aB 
      dbg("7 aB:",voltage_aB_off_detected);
    }
  }

  dbgi("100:",stickA,250);
  dbgi("101:",stickB,250);
/*  
  // printing to serial monitor
  Serial.print(" a ");
  Serial.print(stickA);
  Serial.print(" b ");
  Serial.println(stickB);
  Serial.println(relayValue);

  delay(50);
*/  
}


void Ab() {
  dbg("200 entering Ab",200);
  waitBeforeBeep_Ab_starts = false;
  digitalWrite(relay, HIGH);
  delay(2000);
  digitalWrite(relay, LOW);
  delay(1000);
  digitalWrite(relay, HIGH);
  delay(500);
  digitalWrite(relay, LOW);
  delay(500);
  digitalWrite(relay, HIGH);
  delay(500);
  digitalWrite(relay, LOW);
  AbFinishedBeeping = millis(); // start timer for function delay Ab
  dbg("201 ",AbFinishedBeeping);
  deactivate_Ab_beeping = true; // start delay
  changeTime = millis();
  dbg("202 leaving Ab",changeTime);
}


void aB() {
  dbg("300 entering aB",300);
  waitBeforeBeep_aB_starts = false;
  digitalWrite(relay, HIGH);
  delay(2000);
  digitalWrite(relay, LOW);
  delay(1000);
  digitalWrite(relay, HIGH);
  delay(500);
  digitalWrite(relay, LOW);
  delay(500);
  digitalWrite(relay, HIGH);
  delay(500);
  digitalWrite(relay, LOW);
  delay(500);
  digitalWrite(relay, HIGH);
  delay(500);
  digitalWrite(relay, LOW);
  aB_FinishedBeeping = millis(); // start timer for function delay aB
  dbg("301 ",aB_FinishedBeeping);
  deactivate_aB_beeping = true;  // start delay
  changeTime = millis();
  dbg("302 leaving aB",changeTime);
}

best regards Stefan

@UKHeliBob

Yes this is definitely noticeable in serial monitor, luckily the 800 cutoff works perfectly for eliminating the inaccuracies and fluctuation. If I need perfect accuracy for a future iteration i'll go down this path. Thanks for letting me know!

@StefanL38

Much appreciate you going to the effort to rewrite it. I'll load it up and report back if it works. Regardless, it's nice to have to code improved! After the last code I posted didn't work, i've got a feeling that there's something else going on. Perhaps the relay output doesn't work anymore.. Might try resoldering it to a different one.

@StefanL38

Loaded up the code and it's working flawlessly, nice to be able to see the "200 entering Ab" in serial to verify.

Looks like whatever is preventing my siren from working is a hardware issue. All cables are connected and the relay operates when fed voltage directly. Must be an issue with the Uno.

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