AVR Freaks

Is this code unreliable and I should use a switch debouncing IC?

Can switch bouncing result in a behavior that is similar to just checking whether the switch is pressed or released and hence turn the LED on or OFF?

Like the case in my code.

This code is really helpful, but I think it will toggle the LED if the switch is released too.

When the switch is released, it will produce a HIGH-LOW bouncing and the "if(bit_is_clear(PORTB,PB0))" condition executes, then a 30 ms delay happens and it toggles according to the LOW level produced during bouncing.

I think inserting an if(bit_is_clear(PORTB, PB0)) between  "_delay_ms(30)" and "PORTC ^= (1<<PC0)" solves this problem.

Thank you for your help.

The desired behavior of the code is to toggle the LED on button press, the LED remains the same on button release.

The wrong behavior that happens when the code executes is ON when button pressed, OFF when button released.

I don't use libraries or ready-to-use header files like "io.h" and "interrupt.h."

These functions belong to a program I wrote myself and is called a "driver". In writing a driver, you dig into the details of the SFRs and their addresses in any microcontroller and write the definitions of functions to access and use a certain peripheral. Then a user of your driver can call these functions to do anything he wants with this certain peripheral even without knowing anything about the SFRs and their addresses. As you can see, I gave the four ports initial data (GPIO_LOW) and initial direction (GPIO_INPUT), determined the direction of pins, and wrote on them without need to mention DDRx, PORTx, and PINx registers.

This gives you a level of abstraction to hide all the details from the user, and let him concentrate on writing his application on the microcontroller.

And if you use another microcontroller, you can just change the definitions of the functions and keep the calls to the functions and the application the same.

About using  initialization functions like 

GPIO_init(PORTA_ID, GPIO_LOW, GPIO_INPUT);

On startup, the AVR initializes all its ports to be input with LOW on them by hardware, so apparently no need to use this function in my code. But what if you use another microcontroller that needs you to initialize its ports, or even you use an AVR and you want to initializes a port(s) to be an output or to have a given value on it?

I hope that it is clear why I write my own functions to access the AVR peripherals. 

These functions read the PINB value, using the word PORTB is to tell the program that we want to read from PINB not PINA, PINC or anything else.

In my own function 

GPIO_readPinVal(PORTB_ID, P0, &received);

PORTB_ID is not the register PORTB but it is just an identificaton number for PORTB, look at those that I wrote in a header file to use in this code :

#define PORTA_ID   0
#define PORTB_ID   1
#define PORTC_ID   2
#define PORTD_ID   3

Look at my definition for the function 

Bool GPIO_readPinVal (uint8 portID, uint8 pinNo, Bool *pinRead)
 {
 /* This function is used to read a value from a pin of a port,
    If the HAL layer provides the function with a port ID for either
    port of ports A, B, C, or D (defined as symbolic constants in 
    GPIO_registers.h). And the pin number is a number between 0-7
    (defined as symbolic constants in GPIO_registers.h) inclusive,
    the function returns 1. The function returns 0 otherwise. */
 /* The HAL layer should pass the address of a Bool variable as 
    an argument to read the value in */


    Bool local_errorState = 1;

    if ( (pinNo >= GPIO_PINS_PER_PORT) || (pinNo < 0) )
    {
        local_errorState = 0;
    }

    else
    {
        switch(portID)
        {
            case PORTA_ID :
            {
                GET_BIT(GPIO_PINA, pinNo, *pinRead);
                break;
            }

            case PORTB_ID :
            {
                GET_BIT(GPIO_PINB, pinNo, *pinRead);
                break;
            }

            case PORTC_ID :
            {
                GET_BIT(GPIO_PINC, pinNo, *pinRead);
                break;
            }

            case PORTD_ID :
            {
                GET_BIT(GPIO_PIND, pinNo, *pinRead);
                break;
            }

            default :
            {
                local_errorState = 0;
                break;
            }
        }
    }

     return local_errorState;
 }

GPIO_PINS_PER_PORT equals 8, to make sure that the user enters a pin number between 0 and 7 inclusive.

 GET_BIT(GPIO_PINA, pinNo, *pinRead) is a macro that takes the pin number "pinNo" in the register "PINA" and assigns this bit to the value addresses by the pointer pinRead, that the user passes in the function call (received in my application program).

I didn't think this code would work but it worked in hardware and simulation, even your old original code that has used only one _delay_ms(30) worked on hardware and simulation. Both codes work hardware, and the following code that I suggested works on hardware and simulation

// Led on Portc.0
// Led toggles when button pressed

#include <avr/io.h>
#include <util/delay.h>         

int main(void)
{
   DDRC  |=  (1<<PC0);              // PC0 output
   DDRB  &= ~(1<<PB0);              // PB0 input
   PORTB |=  (1<<PB0);              // pull-up on PB0  

   while(1)
   {
      if(bit_is_clear(PORTB,PB0))        // if button pressed
      {
         _delay_ms(30);                  // wait while button bouncing

         if(bit_is_clear(PORTB, PB0))
         {
            PORTC ^= (1<<PC0);              // toggle Led
         }

         while(bit_is_clear(PORTB,PB0)); // wait until button released
      }
   }
}

Now, by tracing your final code I see that in the switch transition from pressed to released, bouncing occurs and a LOW signal is produced.

So the code enters the if condition and delays 30 ms, it will toggle the LED even though it was just a switch release. It will skip the while loop because the signal is HIGH. Then it will delay 30ms .

I find that it will toggle in switch release too, but in real hardware connection and even in simulation your final code works well.

Any explanation why your final code works while I trace the code and find that it will not.

This is not my function, my function is clear after post #25, I think it is not deliberate.

It will confuse a complete beginner, I understood what is meant by this statement.

That is something to remember, in simple codes like that trying to handle everything is possible.

But trying to handle everything in more complicated codes is tough.

In data structures, when I wrote a data structure I made some assumptions and gave the user some limitations on his inputs that ignoring them will be considered his own mistake.

Thank you, I liked what you said and felt it can be extended to be a life style.

Thanks for everybody who tried to help.

I used @ kk6gm  approach in the solution post, and used my own approach and both work.

My approach is :

while(1)
	{		
	 	GPIO_readPinVal(PORTB_ID, P0, &received);
		
		if(received == GPIO_LOW)
		{
			_delay_ms(30);
			
			GPIO_readPinVal(PORTB_ID, P0, &received);

			if(received == GPIO_LOW)
			{
				GPIO_togglePinVal(PORTC_ID, P0);
			}
			
			GPIO_readPinVal(PORTB_ID, P0, &received);

			while(received == GPIO_LOW)
			{
				GPIO_readPinVal(PORTB_ID, P0, &received);
			}
			
		}
	}

It is as follows : 

1. Read the value of the switch pin.

2. If the value is LOW(switch is pressed), wait for 30 milliseconds to skip all the bouncing.

3. Read the value of the switch pin again, if it is LOW then the level is stable. Now toggle the LED

4.  Read the LOW level of the switch pin again, and wait indefinitely until switch is released (HIGH value).

5. During switch release , bouncing will produce a LOW signal that makes the if(received == GPIO_LOW) TRUE.

6. It will wait for 30 miliseconds, then it will read the switch pin value again.

7. Now the switch is stable on release, so it will read HIGH.

8. This HIGH level makes the inner if(received == GPIO_LOW) False, so no toggling occurs.

9. It will read the switch pin value again, to find that it is HIGH, to discard the waiting while loop and start eveything again.