Search code examples
cembeddedavr

ATmega8 up/down counter not counting correctly

I'm wanting to build a 5 digit up/down counter. When I can get the simulation to work I'll be much happier buying the bits and pieces to build it. What I have so far uses the ATmega8, but quite frankly any solution would work for me as long as the components are reasonably cheap.

I found a simple up counter on the web and have changed it to include a down function also. I removed some hideous _delay_ms(1000) statements from the code which were probably there to prevent multiple counts while the input is being held? It made the counter very slow and unresponsive. Here's what I have so far: Circuit diagram

The problem I have is that it doesn't count up and when it counts down it doesn't display zero.

The code: #include #include #include #include 

#define F_CPU 4000000

volatile uint16_t digits[5]={0,0,0,0,0}; //INITIALISE VARIABLE TO STORE INDIVIDUAL DIGITS OF THE NUMBER

void breakup(uint16_t num) {  //FUNCTION TO FIND THE INDIVIDUAL DIGITS OF THE NUMBER
                              // AND STORE THEM IN A GLOBAL VARIABLE
    DDRD=0xFF; // INITIALISE PORTD AS ALL OUTPUT
    PORTD=0x00;
    unsigned int i=0;
    while (num!=0) {
        digits[i]=num%10;
        num=num/10;
        i++;
    }
    for(i=0;i<5;i++) {
        PORTD=(1<<i); // 'i'th PORTD GIVEN HIGH
        display(digits[i]);
        _delay_us(600);
        PORTD=(0<<i); //'i'th PORTD GIVEN LOW
    }
}

void display (uint16_t num) { // Bit patterns changed from the original to correct for my hardware layout.
    DDRB=0b11111111;
    PORTB=0xFF;
    switch(num) {
        case 0:
        PORTB=0b00111111;
        break;
        case 1:
        PORTB=0b00000110;
        break;
        case 2:
        PORTB=0b01011011;
        break;
        case 3:
        PORTB=0b01001111;
        break;
        case 4:
        PORTB=0b01100110;
        break;
        case 5:
        PORTB=0b01101101;
        break;
        case 6:
        PORTB=0b01111101;
        break;
        case 7:
        PORTB=0b00000111;
        break;
        case 8:
        PORTB=0b01111111;
        break;
        case 9:
        PORTB=0b01101111;
        break;
        default:
        PORTB=0xFF;
    }
}

int main(void) {
    DDRB=0xFF; //Initialise PORTB as all outputs.
    DDRC=0x00;   //Initialise PORTC as all inputs.
    PORTC=0b01000000;  //Enable internal pullup for the reset pin.
    char x;
    char down_button_press = 0;
    char up_button_press = 0;
    uint16_t i, c=5;
    while(1) {
        x=PINC&0b00000001; //Check condition of PC0 DOWN button.
        if (x==0 && down_button_press==0) {
            c--;
            breakup(c); //Pass c to the breakup routine and display.
            down_button_press++;
        }
        if (x==1) {
        down_button_press = 0;
        }
        x=PINC&0b00000010; //Check condition of PC1 UP button.
        if (x==0 && up_button_press==0) {
            c++;
            breakup(c); //Pass c to the breakup routine and display.
            up_button_press++;
        }
        if (x==1) {
        up_button_press = 0;
        }
        else
        breakup(c);
    }
}

The counting up and counting down bits of the code at the end are essentially identical, so I don't know why it counts down, but not up. In an attempt to stop the counter making multiple counts while the button is pressed I have included a sort of latch && up_button_press==0. It's not supposed to be debouncing code, it's just so I get single increments and single decrements with each button press. What I have managed to work out is that is doesn't count up because the up-latch isn't clearing when the button goes back high - so the && up_button_press==0 condition is never true:

if (x==1) {
    up_button_press = 0;
    }

If someone can help me to get the thing to work I'd appreciate it. I can add the Proteus simulation file if that helps?

Solution

  • there are, as you know a couple problems with the code.

    1) regarding the code block, in breakup() that starts with:

    while (num!=0) {

    This loop should always iterate 5 times, regardless of the value of num

    This will result in all possible digits being displayed, including 0

    The following code is how I would implement the desired functionality.

    Notice the conversion of most magic numbers to have a meaningful names.

    Notice the cleanup of the logic in the main() function.

    Notice the extension for the possible displayed values from 16k to 99999.

    Notice the removal of the clutter and unneeded variables.

    Notice the use of meaningful (and unique) variable names.

    I'm wondering why setting the 7 segment digits using the internal pullup resistors rather than the PINxy outputs. But I left it the same as the OPs posted code.

    #include <stdint.h>

#define F_CPU 4000000

#define NUM_DIGITS (5)
#define MAX_DISPLAYED_VALUE (99999)

#define UP_BUTTON (0b00000010)
#define DN_BUTTON (0b00000001)

#define DIGIT_0   (0b00111111)
#define DIGIT_1   (0b00000110)
#define DIGIT_2   (0b01011011)
#define DIGIT_3   (0b01001111)
#define DIGIT_4   (0b01100110)
#define DIGIT_5   (0b01101101)
#define DIGIT_6   (0b01111101)
#define DIGIT_7   (0b00000111)
#define DIGIT_8   (0b01111111)
#define DIGIT_9   (0b01101111)

// prototypes
void initialize( void );
void display   ( uint8_t digitValue );
void breakup   ( uint32_t num );

uint8_t digits[]={0,0,0,0,0}; //INITIALISE VARIABLE TO STORE INDIVIDUAL DIGITS OF THE NUMBER



int main(void)
{

    uint32_t  num = 0;  // initialize value to display

    initialize();       // initialize hardware

    while(1)
    {
        if (PINC & DN_BUTTON)
        { // then down button pressed
            if( num > 0 )
            {
                num--;
            }
        }

        else if ( PINC & UP_BUTTON)
        { // then up button pressed
            if( num < MAX_DISPLAYED_VALUE )
            {
                num++;
            }
        } // end if which button
        breakup( num );
    } // end while forever
} // end function: main



void breakup(uint32_t num)
{  //FUNCTION TO FIND THE INDIVIDUAL DIGITS OF THE NUMBER
                              // AND STORE THEM IN A GLOBAL VARIABLE
    for( size_t i=0; i< NUM_DIGITS; i++)
    {
        digits[i]= (uint8_t)(num%10);
        num=num/10;
    }

    for( size_t i=0; i<NUM_DIGITS; i++)
    {
        display(digits[i]); // preset the value to display
        PORTD=(1<<i);  // select which 7 segment digit
        _delay_us(600);
        PORTD = 0;    // unselect all the seven segment displays
    }
}



void display ( uint8_t digitValue)
{ // Bit patterns changed from the original to correct for my hardware layout.

    switch(digitValue)
    {
        case 0:
            PORTB=DIGIT_0;
            break;

        case 1:
            PORTB=DIGIT_1;
            break;

        case 2:
            PORTB=DIGIT_2;
            break;

        case 3:
            PORTB=DIGIT_3;
            break;

        case 4:
            PORTB=DIGIT_4;
            break;

        case 5:
            PORTB=DIGIT_5;
            break;

        case 6:
            PORTB=DIGIT_6;
            break;

        case 7:
            PORTB=DIGIT_7;
            break;

        case 8:
            PORTB=DIGIT_8;
            break;

        case 9:
            PORTB=DIGIT_9;
            break;

        default:
            break;
    } // end switch
} // end function: display



void initialize()
{
    DDRB=0xFF;          // all outputs
    //PORTB=0xFF;       // enable all internal pullups

    DDRC=0x00;         // all inputs.
    PORTC=0b01000000;  //Enable internal pullup for the reset pin.

    DDRD=0xFF;         // all outputs
    PORTD=0x00;        // disable all internal pullups
} // end function: initialize