current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Electrical Engineering Stack Exchange is a question and answer site for electronics and electrical engineering professionals, students, and enthusiasts. It's 100% free.
up vote 3 down vote favorite

Basically, I have two led lights connected to pin 2 (led 1) and pin 3 (led 2), and I want led 2 to light up every time led 1 lights up, and turn off every time led 1 turns off.

My code:

int led1=2;
int led2=3;

void setup()
{
    pinMode(led1, OUTPUT);
    pinMode(led2, OUTPUT);
}

void loop()
{
    if (digitalRead(led1 == HIGH))
    {
        digitalWrite(led2, HIGH);
    }
    else
    {
        digitalWrite(led2, LOW); //This line won't work.
    } 
    digitalWrite(led1, HIGH);
    delay(5000);
    digitalWrite(led1, LOW);
    delay(3000);
}

So after I upload this code, led 2 lights up whenever led 1 lights up, but led 2 wont turn off when led 1 turns off.

share|improve this question

3 Answers 3

up vote 2 down vote accepted

With the if() statement fixed as mentioned in another answer, it still won't work because you turn led1 on, then off at the end of the loop, so when the program gets back to the top of the loop, led1 is always off.

To get the effect you want, you need to do the if/else both after turning led1 on, and after turning led1 off.

share|improve this answer
up vote 3 down vote

your if statement is incorrect, what you want is

if (digitalRead(led1) == HIGH)
{
    digitalWrite(led2, HIGH);
}
else
{
    digitalWrite(led2, LOW); //This line won't work.
}

However, as Camil has already pointed out, there are other issues as well.

Your original "if" statement would get evaluated as follows: First, the "led1 == HIGH" statement gets evaluated. Since you defined led1 as 2, and "HIGH" is (presumably) defined as 1, this should result in a boolean FALSE, or the value 0. This result (the value 0) is then passed as input to the digitalRead function. Since this function expects an Arduino pin number as input, it effectively tries to read the value of pin 0 (which I don't think is defined). This nonetheless appears to results in a value greater than one, so that the body of the if clause is executed as your LED2 is evidently turned on.

As Camil already pointed out, with only the change in the if statement, your LED2 would never actually turn on, since at the end of the loop(), LED1 would always be off/low. So if all you want to do is turn on or off several pins, you could just use two digitalWrite() calls. If you do need to check if one pin is low or high (e.g. because other parts of the code can change its state), then you do need to make sure to do the comparison the against the output of digitalRead(), an not within its input.

share|improve this answer
    
That is true, but not what causes the problem. –  Camil Staps Jun 4 '13 at 18:32
    
No, it's not the only problem, but that was the reason it never went into the else part. Since our answers overlapped, I'm happy to withdraw mine. –  fm_andreas Jun 4 '13 at 18:35
    
Look at my answer. It is significantly different from yours. With your code, LED2 wouldn't get low either, because the if loop is only executed when LED1 is high. If our answers were essentially the same, I'd have withdrawn mine because you were earlier, however, your answer doesn't answer the question. –  Camil Staps Jun 4 '13 at 18:36
    
Thank you Camil and fm, but once i change my code to if (digitalRead(led1) == HIGH); led 2 won't even turn on. what's going on? –  Tony Yi Jun 4 '13 at 18:46
    
peter, thank you so much, Camil, thank you so much –  Tony Yi Jun 4 '13 at 19:12
up vote 6 down vote

This line:

if (digitalRead(led1 == HIGH))

Is incorrect. This should be:

if (digitalRead(led1) == HIGH)

Because you want to check the return value of digitalRead(). However, this doesn't cause the problem.

You can see the real problem when you try to think as the microcontroller. It does these steps (I start at digitalWrite(led1, HIGH)):

  1. Set LED1 high
  2. Wait
  3. Set LED1 low
  4. Wait
  5. Check:
    1. If LED1 is high, set LED2 high
    2. If LED1 is low, set LED2 low
  6. Go to 1.

When 5 is executed, the value of LED1 is always low. 5.2 will always be executed, 5.1 never. If you want to achieve this programmatically, you could use something like this:

void loop() {
    digitalWrite(led1, HIGH);
    digitalWrite(led2, HIGH);
    delay(5000);
    digitalWrite(led1, LOW);
    digitalWrite(led2, LOW);
    delay(3000);
}

If you want to do this with an if, you can either do something like this:

void calcLed2() {
    if (digitalRead(led1) == HIGH) {
        digitalWrite(led2, HIGH);
    } else {
        digitalWrite(led2, LOW);
    }
}

void loop() {
    digitalWrite(led1, HIGH);
    calcLed2();
    delay(5000);
    digitalWrite(led1, LOW);
    delay(3000);
}

Or put the if loop in the loop() (i.e. without a function call).

It would also be good practice if you set the initial state of the machine in your setup(). For example, set both LEDs high or both LEDs low. If you don't do this, it will work, but might give unexpected results in the first few seconds.

share|improve this answer
    
Are you sure LED1 is always high when 5 is executed? Surely LED1 is always low? –  Amos Jun 4 '13 at 18:57
    
@Amos yes, thanks for that. –  Camil Staps Jun 4 '13 at 18:57

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.