Up and Down Counter + Two Buttons + 7 Seg Display

Hello, I am new to uControllers and I haven't program for a long time.

I am having problems with a counter that counts Up when button one is pressed, counts down when button 2 is pressed and stops when both buttons are pressed. I assume that the display shows nothing when both buttons are not pressed. (english is my second language)

the count is shown in a 7 Segments Display and just counts from 0 to 3 or vice-verse, my problem is that I cant make it stop in any number. It always finish the for loop, I mean that it stops in 3 when the count is going up and the second button is pressed, and in 0 when the count is going down and the second button is pressed.

I think that I understand why is doing that: the for loop must end and then repeat the loop, I just can't find the way of doing it in a different way. Already tried everything I could think of :). And I still don't understand why the problem is to build a 2 bits Up/Down counter and not to use single leds in BCD to show the count. Why I don't ask my instructor is long story

thank you guys in advance

this is my code:

//---------------------------------------
const int buttonOne = 10;
const int buttonTwo = 12;

const int ledPinA =  4;
const int ledPinB =  5;
const int ledPinC =  7;
const int ledPinD =  8;
const int ledPinE =  9;
const int ledPinF =  3;
const int ledPinG =  2;
const int ledPinDP = 6; //The order of the pins is to make it easier to connect

int buttonOneState = 0; 
int buttonTwoState = 0; 
int i;

void setup() 
{ 
  pinMode(ledPinA, OUTPUT);
  pinMode(ledPinB, OUTPUT);
  pinMode(ledPinC, OUTPUT);
  pinMode(ledPinD, OUTPUT);
  pinMode(ledPinE, OUTPUT);
  pinMode(ledPinF, OUTPUT);
  pinMode(ledPinG, OUTPUT);
  pinMode(ledPinDP, OUTPUT);
  
  pinMode(buttonOne, INPUT); 
  pinMode(buttonTwo, INPUT); 
}

void loop()
{
   buttonOneState = digitalRead(buttonOne);
   buttonTwoState = digitalRead(buttonTwo);
  
   for( i=0; i< 4; i++)
  {
   if (buttonOneState == HIGH && buttonTwoState == LOW) {  
     delay(500); 
     led(i); }
     
   else if (buttonOneState == LOW && buttonTwoState == HIGH) {
     delay(500);   
     led(3-i); }
   
   else if (buttonOneState == HIGH && buttonTwoState == HIGH) {
     break;
     delay(500); } //here is where I don't know
     
   else {
     delay(500);
     led(4); }  
  }
  
 
}  


void led(int x)
{
  if (x == 0) {
  digitalWrite(ledPinA, LOW);
  digitalWrite(ledPinB, LOW);
  digitalWrite(ledPinC, LOW);
  digitalWrite(ledPinD, LOW);
  digitalWrite(ledPinE, LOW);
  digitalWrite(ledPinF, LOW);  
  digitalWrite(ledPinG, HIGH);  
  digitalWrite(ledPinDP, LOW);  }
  
else if (x == 1) {
  digitalWrite(ledPinA, HIGH);
  digitalWrite(ledPinB, LOW);
  digitalWrite(ledPinC, LOW);
  digitalWrite(ledPinD, HIGH);
  digitalWrite(ledPinE, HIGH);
  digitalWrite(ledPinF, HIGH);  
  digitalWrite(ledPinG, HIGH);  
  digitalWrite(ledPinDP, LOW);  }

else if (x == 2) {
  digitalWrite(ledPinA, LOW);
  digitalWrite(ledPinB, LOW);
  digitalWrite(ledPinC, HIGH);
  digitalWrite(ledPinD, LOW);
  digitalWrite(ledPinE, LOW);
  digitalWrite(ledPinF, HIGH);  
  digitalWrite(ledPinG, LOW);  
  digitalWrite(ledPinDP, LOW);  }
  
else if (x == 3) {
  digitalWrite(ledPinA, LOW);
  digitalWrite(ledPinB, LOW);
  digitalWrite(ledPinC, LOW);
  digitalWrite(ledPinD, LOW);
  digitalWrite(ledPinE, HIGH);
  digitalWrite(ledPinF, HIGH);  
  digitalWrite(ledPinG, LOW);  
  digitalWrite(ledPinDP, LOW);  }
  
else {
  digitalWrite(ledPinA, HIGH);
  digitalWrite(ledPinB, HIGH);
  digitalWrite(ledPinC, HIGH);
  digitalWrite(ledPinD, HIGH);
  digitalWrite(ledPinE, HIGH);
  digitalWrite(ledPinF, HIGH);  
  digitalWrite(ledPinG, HIGH);  
  digitalWrite(ledPinDP, HIGH);  }
  
}

// if there is a chance: how to post the code in a embedded window?

// if there is a chance: how to post the code in a embedded window?

Did you see the two rows of buttons at the top of the form you typed in? One of them has a # symbol on it. Select that button before pasting code. Or, paste the code, then select all of it, and then press the button.

You can even modify your post, and do that now. Make us all happy.

Each time that loop is called, it should look at whether the buttons are pressed, as it is doing now.

You have two buttons, and 4 states (and 4 actions that can occur):
Button 1 is pressed - increment a counter, and display a new value
Button 2 is pressed - decrement a counter, and display a new value
Button 1 and 2 are pressed - display the current valued
Neither button is pressed - display nothing

So, get rid of the for loops. They are not doing what you want (although they were a good idea, to verify that the digits can be displayed properly).

After reading the button states, decide what value to show, if any. Then, show that value (or nothing). Delay a little while, so the number can be read.

The fact that loop repeats endlessly means that you don't need to have any additional loops, for what you want to do.

Why I don't ask my instructor is long story

You really need to work on this. That person is (probably) not there because he/she couldn't get another job. That person is there because he/she wants to see you learn.

Give them a little respect by paying attention in class, and participating. Ask questions. Show that you are trying to learn.

Have you got pull-ups or pull-downs on the button pins?
You're not using the internal pull-ups.

Thanks for the idea about the counter variables Paul. The loop in uContollers in something I never worked with before.

About the long story, I might send you a message and you will understand me, this is not the place to explain.

Groove: I am using pull up resistors.

I will post an update later on

thank you

OK guys,

I used only one variable and it is stopping on the next digit but that is enough for my purpose. I am taking this class not for credit but to learn.
I think that there must be a debounce to make it work perfect but I will leave it this way

thanks again

here is the updated code for the loop:

void loop()
{
   buttonOneState = digitalRead(buttonOne);
   buttonTwoState = digitalRead(buttonTwo);
   delay(500);
    
   if (buttonOneState == HIGH && buttonTwoState == LOW) 
     { 
        
       led(up); 
       up = up++;
         if (up == 4) { up=0; }
         else {}
     }
     
   else if (buttonOneState == LOW && buttonTwoState == HIGH) 
    {
         
      led(up);
      up = up--;
        if (up == -1) { up=3; }
        else {}
    }
   
   else if (buttonOneState == HIGH && buttonTwoState == HIGH) 
   {
     delay(500);
   }
   
   else {  led(4); }  
}

A couple of comments. You have this code:

       up = up++;
         if (up == 4) { up=0; }
         else {}

If up is 0, up++ changes the value in up to 1, but after the current value is stored in up. Very confusing. Good this you weren't using the pre-increment version...

It is not necessary to have an else block for every if.

That code can be shortened to:

       up++;
         if (up == 4)
              up=0;

Or, to make it even shorter, you can use pre-increment and the modulo operator, %.

up = ++up % 4;

With the post-increment notation, up++, the value is used, and then incremented. With the pre-increment notation, the value is incremented, then used. When used by itself, either works. But,

char strArray[10];
int index = 3;
strArray[++index] = 'A';

and

strArray[index++] = 'A';

will result in 'A' being stored in different places in the array. The ++index form causes 'A' to be stored in strArray[4], and index to contain 4. The index++ form causes 'A' to be stored in strArray[3], and index to contain 4.