My code doesn't work.

I am trying to get a green led blink when a button is pressed and when it's not the red LED should blink.
But my code doesn't.

Code:

const int LED1 = 2;
const int LED2 = 4; 
const int KNOP = 7;
unsigned long previousMillis = 0;
long OnTime = 500;           
long OffTime = 750;          
int ledState = LOW;
int Status ; 

void setup () {
 pinMode (LED1, OUTPUT);
 pinMode (LED2, OUTPUT);
 pinMode (KNOP, INPUT);
 
 }

void loop(){
Status = digitalRead(KNOP); 



if(Status == HIGH){ 
 LEDaan (); 

}
else  {
 LEDuit ();
}


if(Status == LOW)
LEDaan2 (); 

else {
 LEDuit2();
 }

}


 void LEDaan()  {

loop;{
 
 unsigned long currentMillis = millis();
if((LED2 == HIGH) && (currentMillis - previousMillis >= OnTime))
 {
   ledState = LOW;  
   previousMillis = currentMillis;  
   digitalWrite(LED2, LOW);  
 }
 else if ((LED2 == LOW) && (currentMillis - previousMillis >= OffTime))
 {
   ledState = HIGH;  
   previousMillis = currentMillis;   
   digitalWrite(LED2, HIGH);   

 }

}
}

void LEDuit(){
digitalWrite(LED2, LOW);

}
void LEDaan2(){
loop;{                
 
 unsigned long currentMillis = millis();
if ((LED1 == HIGH) && (currentMillis - previousMillis >= OnTime))
 {
   ledState = LOW;  
   previousMillis = currentMillis;  
   digitalWrite(LED1, LOW);  
 } 
 else if ((LED1 == LOW) && (currentMillis - previousMillis >= OffTime))
 {
   ledState = HIGH;  
   previousMillis = currentMillis;   
   digitalWrite(LED1, HIGH);   

 }

}
}

void LEDuit2(){
 digitalWrite (LED1, LOW);
}
const int LED2 = 4;
    if ((LED2 == HIGH)

So, LED2 equals 4
Will it ever equal HIGH ?

void LEDaan2(){
 loop;{

What's that?

Why not combine these

 if(Status == HIGH){
  LEDaan ();
 
 }
 else  {
  LEDuit ();
 }
 

 if(Status == LOW)
 LEDaan2 ();

else {
  LEDuit2();
  }

like this

 if(Status == HIGH){
  LEDaan ();
  LEDuit2();
 
 }
 else  {
  LEDuit ();
  LEDaan2 ();
 }

Please use the code button </> when posting code.

…R

UKHeliBob:

const int LED2 = 4;
    if ((LED2 == HIGH)

So, LED2 equals 4
Will it ever equal HIGH ?

That's to point out to which pin the LED is connected to. This shouldn't interfere with the rest of the code.


TolpuddleSartre:

void LEDaan2(){

loop;{



What's that?

LEDaan2 is to activate one of the LED's. i used loop to make the go on and of continuously if the button is pressed.

If you want to know if a pin is high or low you have to READ the pin. It's no good just comparing the NUMBER of the pin with HIGH. You'll find digitalRead() is useful for this purpose.

Steve

i used loop to make the go on and of continuously

Really?

Hi,
Welcome to the forum.

Can you please post a copy of your circuit, in CAD or a picture of a hand drawn circuit in jpg, png?

How have you got your switch wired?
Your code looks as though you have a button from the Arduino input to 5V, do you have a 10k pull down resistor between the Arduino input and gnd to make sure the input goes to gnd when the button is not pressed?

What model Arduino are you using?

Thanks.. Tom... :slight_smile:

TolpuddleSartre:

void LEDaan2(){

loop;{



What's that?

Bad_At_Coding:
That's to point out to which pin the LED is connected to. This shouldn't interfere with the rest of the code.


LEDaan2 is to activate one of the LED's. i used loop to make the go on and of continuously if the button is pressed.

I'll go out on a limb and state that I believe what @TolpuddleSartre means is that it is syntactically misshapen.

dougp:
I'll go out on a limb and state that I believe what @TolpuddleSartre means is that it is syntactically misshapen.

Alas, correcting the syntax will not solve anything.

...R

OP,
I’ll go out on a limb, and you may hate me for this - but you have missed the basic language tutorials entirely.
Go back to the Forum Home Page, and study HOW to use the Arduino language (C/C++) - and it’s specifics to the Arduino platform.

To read a pin’s state - as noted above, you use digitalRead()
To set a pin’s state - as noted above, you use digitalWrite()

To loop - ignore the loop() function - that’t the main ‘container’ function of Arduino-land.
It does loop - but outside the scope of anything you are likely to be doing at this stage.

Looping within your functions may achieved be with for(), do-while() or other constructs.
While you’re learning, see if you can abandon delay() once you have the basics working.

TomGeorge:
Hi,
Welcome to the forum.

Can you please post a copy of your circuit, in CAD or a picture of a hand drawn circuit in jpg, png?

How have you got your switch wired?
Your code looks as though you have a button from the Arduino input to 5V, do you have a 10k pull down resistor between the Arduino input and gnd to make sure the input goes to gnd when the button is not pressed?

What model Arduino are you using?

Thanks… Tom… :slight_smile:

I use an Arduino Nano
And btw none of the lights go on now.

lastchancename:
OP,
I’ll go out on a limb, and you may hate me for this - but you have missed the basic language tutorials entirely.
Go back to the Forum Home Page, and study HOW to use the Arduino language (C/C++) - and it’s specifics to the Arduino platform.

To read a pin’s state - as noted above, you use digitalRead()
To set a pin’s state - as noted above, you use digitalWrite()

To loop - ignore the loop() function - that’t the main ‘container’ function of Arduino-land.
It does loop - but outside the scope of anything you are likely to be doing at this stage.

Looping within your functions may achieved be with for(), do-while() or other constructs.
While you’re learning, see if you can abandon delay() once you have the basics working.

Is this a better code then not using the loop within functions?

const int LED1 = 2;
const int LED2 = 4; 
const int KNOP = 7;
unsigned long previousMillis = 0;
long OnTime = 500;           
long OffTime = 750;          
int ledState = LOW;
int Status ; 

void setup () {
  pinMode (LED1, OUTPUT);
  pinMode (LED2, OUTPUT);
  pinMode (KNOP, INPUT);
  
  }

void loop(){
  
 Status = digitalRead(KNOP); 
 
 
 if(Status == HIGH){
  LEDaan ();
  LEDuit2();
 
 }
 else  {
  LEDuit ();
  LEDaan2 ();
 }
  }
 
  void LEDaan()  {
 var2 = 0;
while(var2 < 400){
  var2++;       
  
  digitalRead(LED2)
  
  unsigned long currentMillis = millis();
 if((LED2 == HIGH) && (currentMillis - previousMillis >= OnTime))
  {
    ledState = LOW;  
    previousMillis = currentMillis;  
    digitalWrite(LED2, LOW);  
  }
  else if ((LED2 == LOW) && (currentMillis - previousMillis >= OffTime))
  {
    ledState = HIGH;  
    previousMillis = currentMillis;   
    digitalWrite(LED2, HIGH);   

  }
    }  
      }

 void LEDuit(){
  
 digitalWrite(LED2, LOW);
 
 }

 void LEDaan2(){
var = 0;
while(var < 200){
  var++;       
  
  digitalRead(LED1)
  
  unsigned long currentMillis = millis();
 if ((LED1 == HIGH) && (currentMillis - previousMillis >= OnTime))
  {
    ledState = LOW;  
    previousMillis = currentMillis;  
    digitalWrite(LED1, LOW);  
  } 
  else if ((LED1 == LOW) && (currentMillis - previousMillis >= OffTime))
  {
    ledState = HIGH;  
    previousMillis = currentMillis;   
    digitalWrite(LED1, HIGH);   

  }
    }   
      }

 void LEDuit2(){
  digitalWrite (LED1, LOW);
 }

Hi,
Thanks for the diagram.
9ff5ceb2b28fee16d40c43f2ad41a198867151dd.jpg

Tom… :slight_smile:

    if ((LED1 == HIGH) && (currentMillis - previousMillis >= OnTime))

2 lines previously you read the state of LED1 (or at least try to) and throw away what is returned. Then in the line above you test the value of the LED1 pin not the state of the pin.

Have you tried compiling the program ?
Several errors are reported

UKHeliBob:

    if ((LED1 == HIGH) && (currentMillis - previousMillis >= OnTime))

2 lines previously you read the state of LED1 (or at least try to) and throw away what is returned. Then in the line above you test the value of the LED1 pin not the state of the pin.

Have you tried compiling the program ?
Several errors are reported

This one is not with compiling issues

const int LED1 = 2;
const int LED2 = 4; 
const int KNOP = 7;
unsigned long previousMillis = 0;
long OnTime = 500;           
long OffTime = 750;          
int ledState = LOW;
int Status ; 
unsigned long vard = 0;
unsigned long var = 0; 

void setup () {
  pinMode (LED1, OUTPUT);
  pinMode (LED2, OUTPUT);
  pinMode (KNOP, INPUT);
  
  }

void loop(){
  
 Status = digitalRead(KNOP); 
 
 
 if(Status == HIGH){
  LEDaan ();
  LEDuit2();
 
 }
 else  {
  LEDuit ();
  LEDaan2 ();
 }
  }
 
  void LEDaan()  {

while(vard < 400){
  vard++;       
  
  digitalRead(LED2);
  
  unsigned long currentMillis = millis();
 if((LED2 == HIGH) && (currentMillis - previousMillis >= OnTime))
  {
    ledState = LOW;  
    previousMillis = currentMillis;  
    digitalWrite(LED2, LOW);  
  }
  else if ((LED2 == LOW) && (currentMillis - previousMillis >= OffTime))
  {
    ledState = HIGH;  
    previousMillis = currentMillis;   
    digitalWrite(LED2, HIGH);   

  }
    }  
      }

 void LEDuit(){
  
 digitalWrite(LED2, LOW);
 
 }

 void LEDaan2(){
while(var < 200){
  var++;       
  
  digitalRead(LED1);
  
  unsigned long currentMillis = millis();
 if ((LED1 == HIGH) && (currentMillis - previousMillis >= OnTime))
  {
    ledState = LOW;  
    previousMillis = currentMillis;  
    digitalWrite(LED1, LOW);  
  } 
  else if ((LED1 == LOW) && (currentMillis - previousMillis >= OffTime))
  {
    ledState = HIGH;  
    previousMillis = currentMillis;   
    digitalWrite(LED1, HIGH);   

  }
    }   
      }

 void LEDuit2(){
  digitalWrite (LED1, LOW);
 }

That’s a huge step forward from your first code!
Congratulations in paying attention to the tutorials.

The flow of your code will become more obvious if you press Ctrl+T in the IDE, which will realign and balance up your indenting.

Also, the extra empty/blank lines aren’t necessary unless they emphasise or assist with the readability of the code blocks.

I reckon you’re only a few hours from a working sketch - and you’ve included millis() as well! That will make future functionality much easier to add.

One hint, put your switch ‘upside down’ between the pin and ground, then you can eliminate the external resistor, and use INPUT_PULLUP. Less parts & wiring, but you need to ‘invert’your sensing of the input states.

    digitalRead(LED2);
    unsigned long currentMillis = millis();
    if ((LED2 == HIGH) && (currentMillis - previousMillis >= OnTime))

You are still reading the state of the pin named LED2, throwing away the result then testing the value of the pin.

Same with LED1

    digitalRead(LED1);
    unsigned long currentMillis = millis();
    if ((LED1 == HIGH) && (currentMillis - previousMillis >= OnTime))

Either save the value when you read it and use the saved value or read the value when you want to use it e.g.

LED1state = digitalRead(LED1);
if (LED1state == HIGH....
 or

if ((digitalRead(LED1) == HIGH)....

Either of those will work but doing the read and then ignoring the result doesn't work.

Steve

Hi,
I think this will work, without all the nesting in the if statements.

const int GreenLEDPin = 2;
const int RedLEDPin = 4;
const int KNOPPin = 7;
unsigned long currentMillis;
unsigned long previousMillis ;
long interval;
long OnTime = 500;
long OffTime = 750;
bool GreenledState = LOW;
bool RedledState = LOW;
bool Status ;

void setup ()
{
  pinMode (GreenLEDPin, OUTPUT);
  pinMode (RedLEDPin, OUTPUT);
  pinMode (KNOPPin, INPUT);
}

void loop()
{
  currentMillis = millis();
  Status = digitalRead(KNOPPin);
  if (Status == HIGH)
  {
    digitalWrite(RedLEDPin, LOW);
    BlinkGreen();
  }
  else
  {
    digitalWrite(GreenLEDPin, LOW);
    BlinkRed();
  }
}

void BlinkGreen()
{
  if (GreenledState == HIGH)  // Check LED state to load relevant delay time
  {
    interval = OnTime;
  }
  else
  {
    interval = OffTime;
  }
  if (currentMillis - previousMillis >= interval)
  {
    // save the last time you blinked the LED
    previousMillis = currentMillis;
    // if the LED is off turn it on and vice-versa:
    if (GreenledState == LOW)
    {
      GreenledState = HIGH;
    }
    else
    {
      GreenledState = LOW;
    }
    // set the LED with the ledState of the variable:
    digitalWrite(GreenLEDPin, GreenledState);
  }
}

void BlinkRed()
{
  if (RedledState == HIGH)   // Check LED state to load relevant delay time
  {
    interval = OnTime;
  }
  else
  {
    interval = OffTime;
  }
  if (currentMillis - previousMillis >= interval) {
    // save the last time you blinked the LED
    previousMillis = currentMillis;
    // if the LED is off turn it on and vice-versa:
    if (RedledState == LOW)
    {
      RedledState = HIGH;
    }
    else
    {
      RedledState = LOW;
    }

    // set the LED with the ledState of the variable:
    digitalWrite(RedLEDPin, RedledState);
  }
}

I have like you, used blink without delay, but I used the ledstate value to determine the delay interval.
I have also renamed your pin and variables to describe what they represent.

Tom... :slight_smile: