start stop code

Hello,
I am new to arduino and trying to learn by starting with the basics. I am using 2 normally open pushbuttons and the onboard led. What I want to do is turn on the led and keep it on when button1 is pushed. Then when button2 is pushed it will turn the led off. I know it sounds simple but i can't figure it out. any help would be appreciated. Here is my code thus far:

const int buttonPin = 2;
const int buttonPin2 = 3;
const int ledPin = 13;

int buttonState = 0;
int buttonState2 = 0;
int ledState = 0;
void setup() {
pinMode(ledPin, OUTPUT);
pinMode(buttonPin, INPUT);
pinMode(buttonPin2, INPUT);
}

void loop() {
buttonState = digitalRead(buttonPin);

if (buttonState == HIGH) {
digitalWrite(ledPin, HIGH);

ledState = digitalRead(ledPin);
if (ledState == HIGH);
do digitalWrite(ledPin, HIGH);
while (buttonState2 == LOW);
buttonState2 = digitalRead(buttonPin2);
if (buttonState2 == HIGH);
do digitalWrite(ledPin, LOW);
while (buttonState == LOW);
digitalWrite(ledPin, LOW);

}
}

digitalWrite(ledPin, HIGH);
 
 ledState = digitalRead(ledPin);
 if (ledState == HIGH)[glow];[/glow]

You've just written the pin high - what is going to change its state?

OK i have removed it and now the led will not stay on after i have released buttonPin1. What now?

void loop() {
buttonState = digitalRead(buttonPin);
if (buttonState == HIGH) {
digitalWrite(ledPin, HIGH);}
buttonState2 = digitalRead(buttonPin2);
if (buttonState2 == HIGH);{
digitalWrite(ledPin, LOW);}

if (buttonState2 == HIGH)[glow];[/glow]{
 do  digitalWrite(ledPin, HIGH);
 while (buttonState2 == LOW);

The pin value needs to be set only once. Then, it will stay that way. It does not need constant reminding to stay HIGH.

In this loop, buttonState2 can not change, so the loop will never end.

now the led turns on without pushing button 1

const int buttonPin = 2;
const int buttonPin2 = 3;
const int ledPin = 13;

int buttonState = 0;
int buttonState2 = 0;
int ledState = 0;
void setup() {
pinMode(ledPin, OUTPUT);
pinMode(buttonPin, INPUT);
pinMode(buttonPin2, INPUT);
}

void loop() {
if (buttonState == HIGH);
do digitalWrite(ledPin, HIGH);
while (buttonState2 == LOW);

if (buttonState == HIGH)[glow];[/glow]

I'm not going to write this for you

Think it out.

Set the LED based on a VARIABLE you set, not based on a button action...

Meaning... When you check for button state... set a status variable... don't mess with pin state right away.

When all your button checking is done... only then check to see what you have....

  1. Check to see if last button press and current button press are the same...

  2. Based on item 1 for both buttons... No button presses... do nothing

  3. Button1 press mode=on

  4. Button2 press mode=off

SET LED based on MODE

What I'm really trying to point out is: Don't use this logic --> "ledState = digitalRead(ledPin);"

Your code seems over complicated and incomplete at the same time. Did you start at the beginning to learn, or do you have previous programming experience that you're working from?

I have about 12 years experience programming ladder logic. This code is new and very different. I am working on doing what you guys have suggested. Thanks for all the help.

Have you noticed how AWOL keeps repeating the same advice? You are writing your if-statements wrong.

turn on the led and keep it on when button1 is pushed. Then when button2 is pushed it will turn the led off.

Did you ever run across Pseudo-Code as a preliminary to implementation of any kind of software? Instead of ladder programming, what we are doing here is implementing the logic as a sequence of program steps. Instead of everything happening at once, things are done one step at at time. We can repeatedly test inputs and cause outputs to change accordingly by putting the whole logic sequence in a loop.

Is the following what you had in mind?

Turn on the LED when button 1 is pressed. If you release button 1 the LED stays on until you press button 2. If you release button 2, the LED stays off until you press button 1 again.

Here is a way to express that simplest of specifications in pseudo-code:

Loop Forever
BEGIN LOOP
    IF button1 is pressed THEN
       Turn on the LED.
    END IF

    IF button2 is pressed THEN
       Turn off the LED.
    END IF
END LOOP

Or what?

To me, the first step is a complete and accurate statement of the problem. Sometime pseudo-code helps organize things before implementation. It also serves to convert the program specification to some (maybe more succinct) architecture-independent sequence of program steps that can convey the intent to us mere humans (and serve as a reminder to yourself of what the heck it is that you intend to do) before trying to tell the compiler what we want the hardware to do.

Now, if my pseudo-code does not convey your intent, then maybe you can tell us a little more. For example, after pressing button 1 is it really necessary (or even desirable) to wait until the button is released before continuing? If it is, then add it to the pseudo code at that point. Do you need to handle a situation where both buttons are pressed at the same time? What are you supposed to do with that? Maybe the "IF" stuff needs to be a little more elaborate. Stuff like that.

My main point is that for people with no programming experience I recommend that they go through some of the examples and make sure they understand the syntax and semantics of simple sequences.

After the supplied "Blink" sketch, a program that uses two buttons to turn the LED on and off seems to be a great next step. Just start with the simplest possible operation (maybe like my pseudo-code) and make absolutely, positively certain that it works exactly the way that you expect. You will discover that putting extra semicolons after "if" clauses creates programs that don't work as expected. Attack that problem. Fix it. Then "enhance" its operation if you want to do something more elaborate.

Regards,

Dave

Footnote:
Programming and debugging are learned skills. Different people have different ways of learning. Helping people is a learned skill. Different people have different ways of trying to help.

You do your best and we will do our best. After all, no one was born knowing this stuff, right?

while(1){
if(digitalRead(buttonPin)){
digitalWrite(ledPin, HIGH);
}
if(digitalRead(buttonPin2)){
digitalWrite(ledPin, LOW);
}
}

Strange... I've seen this exact same problem in the forums today... :\ I'm just not sure where exactly.