A switch and a LED problem

Go To Last Post
40 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I am using AVR AtMega 32, and developed a driver for the GPIO of the microcontroller.

On start up, the LED is OFF. The following code's purpose is to toggle a LED only once if a switch (dip switch or push button) is pressed, and if it is not pressed the LED's state (On/OFF) remains the same.

Meaning that on startup the LED is OFF, if the switch is pressed the LED should turn ON, if the switch is pressed again the LED should turn OFF again, and so on.

At any one of the two states of LED(ON/OFF), if the switch is released the LED remains on its state ON or OFF. But the code wrongly turns the LED ON if the switch is pressed and turns it OFF if the switch is released. 

int main(void)
 {
	Bool received;
	uint8 sw_count = 0;

	GPIO_init(PORTA_ID, GPIO_LOW, GPIO_INPUT);
	GPIO_init(PORTB_ID, GPIO_LOW, GPIO_INPUT);
	GPIO_init(PORTC_ID, GPIO_LOW, GPIO_INPUT);
	GPIO_init(PORTD_ID, GPIO_LOW, GPIO_INPUT);

	GPIO_writePinVal(PORTB_ID, P0, GPIO_HIGH);

	GPIO_setPinDir(PORTC_ID, P0, GPIO_OUTPUT);
	GPIO_writePinVal(PORTC_ID, P0, GPIO_HIGH);

	GPIO_setPinDir(PORTC_ID, P7, GPIO_OUTPUT);
	GPIO_writePinVal(PORTC_ID, P7, GPIO_HIGH);

	while(1)
	{
	 	GPIO_readPinVal(PORTB_ID, P0, &received);

		if(received == GPIO_LOW)
		{
			sw_count++;

			if(sw_count == 1)
			{
				GPIO_togglePinVal(PORTC_ID, P0);
			}
		}

		else
		{
			sw_count = 0;
		}
	}

 return 0;
 }

 

Please forget about the details of the functions like GPIO_init, GPIO_writePinVal, their code is long to place here. These functions work properly, I tested them. The switch is connected on Port B pin 0 and the LED is connected on Port C pin 0. The switch gives LOW to the AVR if it is pressed, and the AVR used its pull-up feature if it is not pressed. The LED is connected in a negative logic manner, a LOW from the AVR turns it ON and a HIGH turns it OFF. I used port C pin 7 to produce a HIGH signal to use it as the VCC for the LED.

The code is supposed run like that :

1. In the beginning of the program, 'received' variable has a value of "HIGH", when the switch is not pressed.

2. The loop continues until sometime the switch is pressed, the 'received variable' gets a value of 'LOW' and sw_count gets a value of 1(increments).

3. The if condition makes the LED to toggle,.

4. While the switch is still pressed, the loop continues increasing sw_count greater than 1, which makes the if(received == GPIO_LOW) condition be entered but the if(sw_count == 1) condition be skipped, so nothing should happen.

5. The loop continues incrementing sw_count and nothing happens, until the switch is released sometime.

6. Now the else clause in the outer if condition executes, assigning sw_count to 0.

7. The 0 value fo sw_count continues until the switch is pressed again, now sw_count increments to 1 and toggles the LED. And so on.

 

May anybody clarify the problem for me please ?

This topic has a solution.
Last Edited: Tue. Jul 23, 2019 - 09:24 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Well, you're not making it clear what you want the program to do, and what it is doing wrong.

 

But I'm sure it's a problem that you are treating a Bool as an integer (sw_count++).  See Therac-25 for how a bug like that killed people.

 

And unless you are explicitly debouncing your switch, you will detect multiple switch pushes every time the switch is closed or opened.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Your loop time is microseconds. You increment a boolean value. Mechanical switches bounce.
The switch bounce is most likely your root cause. Read up on debouncing.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I have a "typdef uint8 Bool" statement in my code.

I edited the 'sw_count' in the code above to be of type uint8, really the code requires that sw_count to increment from 0 to 1, 2, 3 ... . I used Bool but I meant uint8.

Last Edited: Sat. Jul 20, 2019 - 09:51 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

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

Last Edited: Sat. Jul 20, 2019 - 09:58 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Your code is unreliable - thats why we suggest debouncing. This involves reading the input a number of times, say, every 10ms and determining when you get a number of the same state in a row.

To do this you need a delay function. Avr-gcc have these as part of its standard library. My debounce code basically loads a variable with,say, a value of 5 if the input is’ ‘1’ else decrements the variable. Delay 10ms and repeat. When the variable gets to 0, we can assume the switch input is stable.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

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.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Yes. As I said, your loop takes microseconds to execute. The input is going high/low at a fast rate when you press/release the switch.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

AhmedH wrote:

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

You definitely need debouncing to make the code reliable, but you don't need a debouncing IC.  Debouncing in software is simple, and is something you should learn how to do.  The basic procedure is to detect a switch/button change (on->off or off->on), then delay long enough for any bouncing to end, maybe 5-20 ms.  So you will need to introduce the concept of time into your program.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

An example

// Button on Portb.0 (to GND)
// 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
         PORTC ^= (1<<PC0);              // toggle Led
         while(bit_is_clear(PORTB,PB0)); // wait until button released
      }
   }
}

 

Last Edited: Sun. Jul 21, 2019 - 08:57 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Visovian wrote:

An example

// Button on Portb.0 (to GND)
// 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
         PORTC ^= (1<<PC0);              // toggle Led
         while(bit_is_clear(PORTB,PB0)); // wait until button released
      }
   }
}

 

 

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.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

AhmedH wrote:

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.

This is a good catch.  That code could easily enter the loop again due to button-up bounce.

Quote:
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.

That's not very clear, but I think I know what you're suggesting.  As a little more clear solution, I would recommend another _delay_ms() after 

while(bit_is_clear(PORTB,PB0));
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

AhmedH wrote:
The following code's purpose is to toggle a LED only once if a switch (dip switch or push button) is pressed, and if it is not pressed the LED's state (On/OFF) remains the same. but it turns the LED ON if the switch is pressed and turns it OFF if the switch is released. 

So which is it, toggle on button press/release, or On when button pressed, Off when button released?   I'm confused???

 

Jim

 

Click Link: Get Free Stock: Retire early! PM for strategy

share.robinhood.com/jamesc3274
get $5 free gold/silver https://www.onegold.com/join/713...

 

 

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

 The following code's purpose is to toggle a LED only once if a switch (dip switch or push button) is pressed, and if it is not pressed the LED's state (On/OFF) remains the same. but it turns the LED ON if the switch is pressed and turns it OFF if the switch is released. 

What do you mean by toggle a led only once? That it turns on, then off...so you press the switch and see the led turn on and off one time?

if it is not pressed the LED's state (On/OFF) remains the same....unless it were blinking, then this has to be true in any case.

 

but it turns the LED ON if the switch is pressed and turns it OFF if the switch is released. 

Are you just saying the led is on when the switch is pressed & off when it is not being pressed without any other considerations or conditions?  

 

How many programmers does it take to change an led???

 

 

 

 

When in the dark remember-the future looks brighter than ever.   I look forward to being able to predict the future!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

avrcandies wrote:
How many programmers does it take to change an led???

How many programmers does it take to create a spec for an LED blinky program?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

AhmedH wrote:

	GPIO_init(PORTA_ID, GPIO_LOW, GPIO_INPUT);

I'm just intrigued to know what "library" it is that is providing these functions like GPIO_init() style functions? It seems to completely obfuscate the code while adding nothing. 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0


AhmedH wrote:
it turns the LED ON if the switch is pressed and turns it OFF if the switch is released. 

No micro is needed at all for this requirement!

Jim

Click Link: Get Free Stock: Retire early! PM for strategy

share.robinhood.com/jamesc3274
get $5 free gold/silver https://www.onegold.com/join/713...

 

 

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

purpose is to toggle a LED only once if a switch (dip switch or push button) is pressed

Get rid of the 330 ohm resistor & I bet it will "toggle" only once when a switch is pressed!! Be sure to look, since there will only be one opportunity.

When in the dark remember-the future looks brighter than ever.   I look forward to being able to predict the future!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

ki0bk wrote:

AhmedH wrote:
The following code's purpose is to toggle a LED only once if a switch (dip switch or push button) is pressed, and if it is not pressed the LED's state (On/OFF) remains the same. but it turns the LED ON if the switch is pressed and turns it OFF if the switch is released. 

So which is it, toggle on button press/release, or On when button pressed, Off when button released?   I'm confused???

 

Jim

 

 

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.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

avrcandies wrote:

 The following code's purpose is to toggle a LED only once if a switch (dip switch or push button) is pressed, and if it is not pressed the LED's state (On/OFF) remains the same. but it turns the LED ON if the switch is pressed and turns it OFF if the switch is released. 

What do you mean by toggle a led only once? That it turns on, then off...so you press the switch and see the led turn on and off one time?

if it is not pressed the LED's state (On/OFF) remains the same....unless it were blinking, then this has to be true in any case.

 

but it turns the LED ON if the switch is pressed and turns it OFF if the switch is released. 

Are you just saying the led is on when the switch is pressed & off when it is not being pressed without any other considerations or conditions?  

 

How many programmers does it take to change an led???

 

 

 

 

 

I clarified the desired behavior of the code on my post #1 that includes my source code.

I hope it's become clear.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:

AhmedH wrote:

	GPIO_init(PORTA_ID, GPIO_LOW, GPIO_INPUT);

I'm just intrigued to know what "library" it is that is providing these functions like GPIO_init() style functions? It seems to completely obfuscate the code while adding nothing. 

 

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. 

Last Edited: Mon. Jul 22, 2019 - 11:38 PM
This reply has been marked as the solution. 
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

Your code isn't working right because you're not handling the button-break bouncing.  This is one way to handle that bouncing:

 

   while(1)
   {
      if(bit_is_clear(PORTB,PB0))        // if button pressed
      {
         _delay_ms(30);                  // wait while button bouncing
         PORTC ^= (1<<PC0);              // toggle Led
         while(bit_is_clear(PORTB,PB0))  // wait until button released
            ;
         _delay_ms(30);                  // wait again while button bouncing       }
      }
   }
Last Edited: Tue. Jul 23, 2019 - 12:27 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

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

Well, that is good points for the effort.  However, it can cause headaches when you are starting out, since it gives more areas where things can go wrong.  So each level needs thoroughly checked, before the pieces are brought together.

You can spend so much time on debugging these "extras", that you have little time left for detailing the basic app you want to create (hence, be prepared to move slowly).   

 

 

When in the dark remember-the future looks brighter than ever.   I look forward to being able to predict the future!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

AhmedH wrote:

GPIO_readPinVal(PORTB_ID, P0, &received);

Visovian wrote:

if(bit_is_clear(PORTB,PB0))        // if button pressed

 

I don't know what happens inside these functions, I have never used them before.

But I am sure the PINx must be read as input pin. not the PORTx.

 

Are you sure these functions read the PINB value?

Majid

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

m.majid wrote:

AhmedH wrote:

GPIO_readPinVal(PORTB_ID, P0, &received);

Visovian wrote:

if(bit_is_clear(PORTB,PB0))        // if button pressed

 

I don't know what happens inside these functions, I have never used them before.

But I am sure the PINx must be read as input pin. not the PORTx.

 

Are you sure these functions read the PINB value?

 

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).

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

ki0bk wrote:

 

 

 

AhmedH wrote:

it turns the LED ON if the switch is pressed and turns it OFF if the switch is released. 

 

No micro is needed at all for this requirement!

Jim

 

 

Turning the LED ON if the switch is pressed and turning it OFF if the switch is released is a wrong behavior that happens when my code executes, not the desired behavior.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

kk6gm wrote:

Your code isn't working right because you're not handling the button-break bouncing.  This is one way to handle that bouncing:

 

   while(1)
   {
      if(bit_is_clear(PORTB,PB0))        // if button pressed
      {
         _delay_ms(30);                  // wait while button bouncing
         PORTC ^= (1<<PC0);              // toggle Led
         while(bit_is_clear(PORTB,PB0))  // wait until button released
            ;
         _delay_ms(30);                  // wait again while button bouncing       }
      }
   }

 

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.

Last Edited: Tue. Jul 23, 2019 - 09:22 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Suggest everyone take a bit more notice of post #24 !! ;-)

 

When you read inputs in an AVR it is the PINB not the PORTB register you need to read. So a line such as:

      if(bit_is_clear(PORTB,PB0))        // if button pressed

is supposed to be:

      if(bit_is_clear(PINB,PB0))        // if button pressed

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:

Suggest everyone take a bit more notice of post #24 !! ;-)

 

When you read inputs in an AVR it is the PINB not the PORTB register you need to read. So a line such as:

      if(bit_is_clear(PORTB,PB0))        // if button pressed

is supposed to be:

      if(bit_is_clear(PINB,PB0))        // if button pressed

 

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.

Last Edited: Tue. Jul 23, 2019 - 09:27 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Indeed, your function is:

 

	 	GPIO_readPinVal(PORTB_ID, P0, &received);

but no one here, but you, knows what is inside this function. So does it read the PORT or the PIN register?

 

In fact where does this GPIO_*() library come from?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:

Indeed, your function is:

 

	 	GPIO_readPinVal(PORTB_ID, P0, &received);

but no one here, but you, knows what is inside this function. So does it read the PORT or the PIN register?

 

In fact where does this GPIO_*() library come from?

 

@AhmedH posted the implementation of GPIO_readPinVal(... in #25 , it seems true...

except : 

if ( (pinNo >= GPIO_PINS_PER_PORT) || (pinNo < 0) )

whilst :

uint8 pinNo

 

anyway I think the key point is "release debouncing" as @kk6gm mentioned in #22

 

 

 

 

Majid

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The problem is all about switch bouncing, in my first post I asked everyone to forget about the details of my functions.

m.majid wrote:

 

 

@AhmedH posted the implementation of GPIO_readPinVal(... in #25 , it seems true...

except : 

if ( (pinNo >= GPIO_PINS_PER_PORT) || (pinNo < 0) )

whilst :

uint8 pinNo

 

anyway I think the key point is "release debouncing" as @kk6gm mentioned in #22

 

 

 

 

 

About this if ( (pinNo >= GPIO_PINS_PER_PORT) || (pinNo < 0) ), pinNo is uint 8 indeed, so it can take 0-255 inclusive. But for any port in the AVR, a pin number is between 0-7 inclusive. So if the user calls the function and passes a value for pinNo that is greater than 7 or less than 0, the function would return 0 to indicate that something is wrong and the function didn't operate as expected.

I know that I can make no restrictions about pinNo, but I intend to handle almost all cases and all inputs to the functions, to make my program more generic and work as intended even if the user does not know what he is doing.

 

Last Edited: Tue. Jul 23, 2019 - 12:07 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

AhmedH wrote:
I know that I can make no restrictions about pinNo, but I intend to handle almost all cases and all inputs to the functions, to make my program more generic and work as intended even if the user does not know what he is doing.

I personally disagree with your requirements. I think it is overcautiousness. However I didn't mean that.

 

I meant pinNo will NEVER be <0 since it is defined as uint8. it will be any value 0-255. as you yourself mentioned.

even if the function is called with any value <0 , then it will be casted to 0-255 inside the function.

so the code will never enter the if statement : [edited]

so the statement || (pinNo < 0) is useless: [edited]

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

[edited]

Majid

Last Edited: Tue. Jul 23, 2019 - 02:33 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

AhmedH wrote:

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.

By putting the 2nd delay in (after switch release is detected and the while() statement drops through), the release bouncing is "absorbed" by the 2nd delay, and the switch has stopped bouncing at the end of the 2nd delay, and so on the next time through the loop the if condition (switch LOW) is NOT met (the switch signal by now has stopped bouncing and is solidly high).  Without the 2nd delay then yes, the release bounce could easily result in toggling the LED on release.  

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

but I intend to handle almost all cases and all inputs to the functions, to make my program more generic and work as intended

 Sounds like you are barking up the wrong tree.  Concentrate on making one specific case work properly and well.  Then you will fully complete one working solution & can then build on that with your success.  Trying to do everything at once is a recipe for nothing.

When in the dark remember-the future looks brighter than ever.   I look forward to being able to predict the future!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

@ m.majid  Yes, you are right about '||pinNo < 0', my first version of the code didn't include it. But I included it in the newer version without noticing the uint8 data type.

Thank you.

Last Edited: Tue. Jul 23, 2019 - 03:10 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

avrcandies wrote:

but I intend to handle almost all cases and all inputs to the functions, to make my program more generic and work as intended

 Sounds like you are barking up the wrong tree.  Concentrate on making one specific case work properly and well.  Then you will fully complete one working solution & can then build on that with your success.  Trying to do everything at once is a recipe for nothing.

 

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.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

kk6gm wrote:

AhmedH wrote:

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.

By putting the 2nd delay in (after switch release is detected and the while() statement drops through), the release bouncing is "absorbed" by the 2nd delay, and the switch has stopped bouncing at the end of the 2nd delay, and so on the next time through the loop the if condition (switch LOW) is NOT met (the switch signal by now has stopped bouncing and is solidly high).  Without the 2nd delay then yes, the release bounce could easily result in toggling the LED on release.  

 

That made it clear, thanks.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

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.

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I'll just throw this in

bool button_state = GPIO_HIGH;
while(1)
{
    GPIO_readPinVal(POERTB_ID, P0, &received);
    if (received != button_state)
    {
        _delay_ms(30);
        button_state = received;
        if (button_state == GPIO_LOW)
        {
            GPIO_togglePinVal(PORTC_ID, P0);
        }
    }
}