scrolling leds

hi,
i’m new to the arduino community, and also to the programming with arduino
so i have kind of a basic question

i want to scroll through 3 leds connected to pins 9,10 and 11
i want to do this using a potentio (10k)

the problem is he doesn’t scroll, it just lights up 1 led
no matter the value of the potentio

so this is what i’ve got

int potpin = 0;
int potval;
int led1 = 9;
int led2 = 10;
int led3 = 11;

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

void loop()
{
int potval = analogRead(potpin);

if(0 < potval < 342)
{ digitalWrite(led1, HIGH);
digitalWrite(led2,LOW);
digitalWrite(led3,LOW);
}
else if (342 < potval < 684)
{ digitalWrite(led1, LOW);
digitalWrite(led2,HIGH);
digitalWrite(led3,LOW);
}
else if (potval > 684)
{digitalWrite(led1,LOW);
digitalWrite(led2,LOW);
digitalWrite(led3,HIGH);
}
}

 if((0 < potval) && (potval < 342))

Etc

thank you, i knew it was gonna be a stupid mistake

Personally I would always put the variable on the left side of a comparison as it reads more naturally to me, especially when using and/or
if((potval > 0) && (potval < 342))as it makes it easier to see the range of values being checked, at least it does to me.

Both ways work, of course, but putting the value first seems unnatural somehow.

UKHeliBob: Both ways work, of course, but putting the value first seems unnatural somehow.

The advantage of putting the value first comes in when you make this mistake: if ('a' = incomingChar)

I see an easier and more flexible way to do this than a bunch of if statements. I’d declare the ledPins in an array and then use a for loop to display them all. A value calculated from reading the pot decides whether the led is on or off. The advantage being that it becomes trivial to add more leds to the deal.

something like:

int limitValue = analogRead(potPin) / ((1024 / ledCount) + 1); // redundant parentheses for clarity
for (byte n = 0; n < ledCount; n++)
{
    if (n <= limitValue) digitalWrite(ledPin[n], HIGH);
    else digitalWrite(ledPin[n], LOW);
}

Then I’d probably add a variable and a condition to only update the leds if the limitValue has changed.

Personally I'd use map() to map the pot value to a range of 0-2 and use that for an array index selector.

majenko: Personally I'd use map() to map the pot value to a range of 0-2 and use that for an array index selector.

One of these days I'll actually remember that function and use it. LOL

[quote author=James C4S link=topic=182047.msg1349204#msg1349204 date=1376151228]

UKHeliBob: Both ways work, of course, but putting the value first seems unnatural somehow.

The advantage of putting the value first comes in when you make this mistake: if ('a' = incomingChar) [/quote]I know what you mean but I still prefer the "variable first" construction and anyway I would never make such a mistake, well not often anyway. :)