Seven segment project using atmega16 and external interrupts

Question

I have been working on a simple project on AVR microcontroller atmega16 on proteus simulation.

The project is: When I push the push button it increments the seven segment to number 9 and after that if the we pushed the button again it overflows and return again to 0.
When I change the (flag=1;) position at while loop code it gives me different outputs and by this I mean when I push the push button it didn't response after the pushing.
It may require another pushing to the button to increment the seven segment proteus simulation

The only code that worked properly is when set flag=1; before exiting (the second if and else conditions)

So my question is what actually happened when I change the flag=1; statement position at the code.

#include<avr/io.h>
#include<avr/interrupt.h>
char flag=1;
char num=0;
void INT2_Init(void){
    DDRB&=~(1<<2);
    SREG|=(1<<7);
    GICR|=(1<<INT2);
    MCUCSR|=(1<<ISC2);
}
ISR(INT2_vect){
    flag=0;
    GIFR|=(1<<INTF2);
}

int main(void){
    DDRC=0x0f;
    PORTC=0;
    DDRB&=~(1<<2);
    INT2_Init();
    while(1){
        if(flag==0){
            if(num>8){
                num=0;
                PORTC=(PORTC&0xf0)|num;
            }
            else{
                num++;
                PORTC=(PORTC&0xf0)|num;
            }
        }
        flag=1;
    }

}

Answer 1

If some variable changes in main code and interrupt, you have to mark it volatile. Otherwise, the compiler may decide to cache variable value and omit its reading inside a loop.

volatile char flag=1;

Second, pay attention when and where your flag variables are changed. E.g.:

    while(1){
        // at this point flag is 0, comparison failed
        if(flag==0) {
          ...
        }
// Interrupt happens here, changing flag to 0
// Then flag is immediately reset, thus flag change is missed in the loop
        flag=1; 
    }

Instead consider to use such a pattern:

    while (1) {
         cli(); // disabling interrupts ensuring no interrupt happens inside 
         if (flag==0) {
           ...
         }
         flag = 1;
         sei(); // re-enable interrupts
    }

or, in this case, it can be simpler

    while (1) {
         if (flag==0) {
             flag = 1; // Reset flag only if it set, right after comparison
             ...
         }
    }

Answer 2

It is not entirely clear to me what you are asking. Do you just struggle to understand the code? Do my comments help?

#include<avr/io.h>
#include<avr/interrupt.h>
char flag=1;
char num=0;

void INT2_Init(void){
    DDRB&=~(1<<2);
    SREG|=(1<<7);
    GICR|=(1<<INT2);
    MCUCSR|=(1<<ISC2);
}

// sets flag to 0 on external interrupt (button press)
ISR(INT2_vect){
    flag=0;
    GIFR|=(1<<INTF2);
}

int main(void){
    DDRC=0x0f;
    PORTC=0;
    DDRB&=~(1<<2);
    INT2_Init();

    while(1){
        // if flag was set to 0 in external interrupt -> increment (or overflow) number on segment display
        if(flag==0){
            if(num>8){
                num=0;
                PORTC=(PORTC&0xf0)|num;
            }
            else{
                num++;
                PORTC=(PORTC&0xf0)|num;
            }
        }
        // reset the flag to 1 to prevent num from being incremented continuously
        flag=1;
    }
}

I would suggest to move flag=1; inside the if to make the intent clear that the statement should only execute once with every button press.

    while(1) {
        // if flag was set to 0 in external interrupt -> increment (or overflow) number on segment display
        if(flag==0){
          // reset the flag to 1 to prevent num from being incremented continuously
          flag=1;

            if(num>8){
                num=0;
                PORTC=(PORTC&0xf0)|num;
            }
            else{
                num++;
                PORTC=(PORTC&0xf0)|num;
            }
        }
    }

It is also advisable to use self descriptive variable names. Like buttonWasPressed instead of flag for example. That makes code like that much easier to read.