Why this LED blinking code doesn't work?

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

HI!
I wanna make a LED blinking than if I press the PB0 the blinking should accelerate any time I press the PB0.

here is my code what I wrote, but I can't realize the problem why my LED won't accelerate when I press the PB0 button.

 

#include <avr/io.h>
#define F_CPU 1000000UL 
#include <util/delay.h>

int PB0_bounce_counter = 0;	
int PB1_bounce_counter = 0;	
int Debounce = 50; 
int Delay = 500; 


void Wait( int ms_sec )
{
	int i;

	for( i = 0; i < ms_sec; ++i )

	_delay_ms( 1 );
}


int main(void){
	
    
	DDRB = (1<<PB1) | (1<<PB2) | (1<<PB3) | (1<<PB4);
	PORTB = (1<<PB1) | (1<<PB2) | (1<<PB3) | (1<<PB4);
	_delay_ms(2500);
	
	PORTB &=~ (1<<PB1) & (1<<PB2) & (1<<PB3) & (1<<PB4);
			
   while (1) 
  
    {	

	if(PINB & (1<<0))
	{
        	PB0_bounce_counter++; 
			
			if (PB0_bounce_counter==Debounce)
			{
	        		Delay = Delay - 50 ;
			}
			
	}
	
	else
	{
       		PB0_bounce_counter = 0;
	}
		
		
	PORTB = (1<<4);
	Wait(Delay);
	
	PORTB &=~(1<<4);	
	Wait(Delay);
		
    }
}

Actually, the LED is blinking but on a constant frequency.

 

Any suggestions pls.

Thank you.

Last Edited: Wed. Sep 12, 2018 - 09:34 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Show your connections.  Check the voltage right at the AVR pin for the button.

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

It looks like you need to hold the button down for 50 seconds before the delay gets smaller.

 

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

Show your connections.  Check the voltage right at the AVR pin for the button.

I use the original EasyAVR5 dev. board, not my wiring up etc...

 

 

 

It looks like you need to hold the button down for 50 seconds before the delay gets smaller.

 

 

I don't think it has anything to do with seconds of waiting.

I don't set up any seconds etc.

The 50 cycles should count the switch bouncings between the pressed and released state.

I assume after 50 bouncing the switch is released physically.

 

I played around again and I figured out actually maybe I missing the keypress reading, the keypress reading is inaccurate.

 

Here is a bit modded code for testing if my code is working, and yes, this code is working but the PB4 is not blinking.

My goal should be to have two switches and one blinking LED.

With the switches, I should manipulate the blinking time of the LED.

 

This is the modded code what is working but not as it should:

 

#include <avr/io.h>
#define F_CPU 1000000UL 
#include <util/delay.h>

int PB0_bounce_counter = 0;	
int PB1_bounce_counter = 0;	
int Debounce = 50; 
int Delay = 500; 


void Wait( int ms_sec )
{
	int i;

	for( i = 0; i < ms_sec; ++i )

	_delay_ms( 1 );
}


int main(void){
	
    
	DDRB = (1<<PB1) | (1<<PB2) | (1<<PB3) | (1<<PB4);
	PORTB = (1<<PB1) | (1<<PB2) | (1<<PB3) | (1<<PB4);
	_delay_ms(2500);
	
	PORTB &=~ (1<<PB1) & (1<<PB2) & (1<<PB3) & (1<<PB4);
			
   while (1) 
  
    {	

	if(PINB & (1<<0))
	{
        	PB0_bounce_counter++; 
			
			if (PB0_bounce_counter==Debounce)
			{
	        	//	Delay = Delay - 50 ;
	        	PORTB ^=(1<<1);
			}
			
	}
	
	else
	{
       		PB0_bounce_counter = 0;
	}
		
	/*	
	PORTB = (1<<4);
	Wait(Delay);
	
	PORTB &=~(1<<4);	
	Wait(Delay);
	*/	
    }
}

 

If I let the PB4 blinking part live in the code the keypress won't be recognized.

I mean this part at the end of the code:

 

    PORTB = (1<<4);
	Wait(Delay);
	
	PORTB &=~(1<<4);	
	Wait(Delay);

What did I do wrong?

 

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

In my experience, the best way to de-bounce a switch is to use a timer, as the number of bounces per switch closure can vary.   Can you put a scope in the input pin to verify the actual timing?

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

You have not enabled pull-up on PB0. Is the button connected to GND or to VCC?

 

If you want react to button immediately during function Wait() then try

 

void Wait( int ms_sec )
{
int i;

    if (! (PINB & (1<<0)))  {break;}  // if button pressed then get out of the function

	for( i = 0; i < ms_sec; ++i )
	_delay_ms( 1 );
}

 

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

Let's see how PB0_bounce_counter counts up after the switch opens in the # 1 program.

 

Loop 1st time: (time = 0 sec)
PB0_bounce_counter++ is executed. (PB0_bounce_counter = 1)
The LEDs are turned on and off. (It takes about 1 sec)

Loop 2nd: (time = 1 sec)
PB0_bounce_counter++ is executed. (PB0_bounce_counter = 2)
The LEDs are turned on and off. (It takes about 1 sec)

 

Loop 50 th time: (time = 49 sec)
PB0_bounce_counter++ is executed. (PB0_bounce_counter = 50)
Delay = Delay - 50 is executed.
The LEDs are turned on and off. (It takes about 1 sec)

Loop 51 th time: (time = 50 sec)
PB0_bounce_counter++ is executed. (PB0_bounce_counter = 51)
Delay = Delay - 50 is executed. (Delay = 450)
The LEDs are turned on and off. (It takes about 1 sec)

 

Loop 61 th time: (time = 60 sec)
PB0_bounce_counter++ is executed. (PB0_bounce_counter = 61)
Delay = Delay - 50 is executed. (Delay = -50)
The LEDs are turned on and off. (It takes about 1 sec)

Since it no longer has a flashing interval, it can not recognize blinking.

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

This is a remarkably simple piece of code for someone who's on this forum for 8 years and has 150 posts to his/her name.

I wonder what's the story behind that...

 

But more to the point.

The whole approach to the problem is sort of flawed.

It lacks structure and invites for an ad-hoc addition of random bits and pieces, which leads to an un maintainable mess.

So instead of letting myself being lured into that I'll give some general / extendable structure to build some projects around.



void main( void) {
    
    Timer_Initialize();     // This should generate a regular (for example 1ms) timer tick.
    
    for( ;;) {
        
        while( !Timer_Event( )) // Should be true every timer tick (1ms).
            ;
            
        Scan_Keys( );
        Handle_Leds( );
    }
}

 

The idea is to make your whole program a slave to the same regular timer tick.

In ARM hardware it is very common to have a hardware timer on the silicon which is especially designed for this task.

It is a very simple timer, and it can not do much more than generate periodic interrupts.

This saves silicon area compared to a lot of other uC families where a full blown complex timer is used for this simple task.

 

But the main part is that it divides and separates your program into separate blocks.

Because all those blocks are called at a regular interval, there is no more need for delay() functions, which waste cpu cycles and much worse, block operation of other tasks.

Each block should check some bits, (keyboard input) or output something (set a port pin for a led) and then return as fast as feasible.

 

This method works pretty well together with state machines.

A state machine for blinking a led could be something like:

#define LED_RED		( 1 << 4)
#define DELAY_TIME	500

void Handle_Leds( void) {
	enum {
		Init,
		On,
		Off,
		Delay,
	};

	static uint8_t State = Led_Init;	// Must be static to remain it's value after exit.
	static uint8_t State_Next;
	static uint16_t Time;
	
	switch( State) {
	case Init:
		DDRB |= LED_RED;
		State = On;
		break;
	case On:
		PORTB |= LED_RED;
		Time = Delay_Time;
		State_Next = Off;
		State = Delay;
		break;
	case Off:
		PORTB &= ~LED_RED;
		Time = Delay_Time;
		State_Next = On;
		State = Delay;
		break;
	case Delay:
		if( Time > 2) {
			// Minimum delay is 2. To get into, and get out of this delay state again.
			--Time;
		}
		else {
			State = State_Next;
		}
		break;
	}
}

This may seem like a lot of typing at first, but it makes your Led function timing independent from the "other stuff".

While you are focusing on the leds, you do not have to think about the keys.

While you are focusing on the keys, you do not have to think about the leds.

 

If you want to have some kind of communication between different "threads", the easiest way would be to do that with global variables.

There are other ways, but I think this is enough for a single post.

Paul van der Hoeven.
Bunch of old projects with AVR's:
http://www.hoevendesign.com

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

jodank wrote:

 

It looks like you need to hold the button down for 50 seconds before the delay gets smaller.

 

 

I don't think it has anything to do with seconds of waiting.

I don't set up any seconds etc.

The 50 cycles should count the switch bouncings between the pressed and released state.

I assume after 50 bouncing the switch is released physically.

 

 

   while (1) 

    {	

	if(PINB & (1<<0))
	{
        	PB0_bounce_counter++; 

			if (PB0_bounce_counter==Debounce)
			{
	        	//	Delay = Delay - 50 ;
	        	PORTB ^=(1<<1);
			}

	}

	else
	{
       		PB0_bounce_counter = 0;
	}

	/*
	PORTB = (1<<4);
	Wait(Delay);

	PORTB &=~(1<<4);
	Wait(Delay);
	*/
    }
}

 

 

What did I do wrong?

 

 

 

I think you don't truly understand how "_delay_ms()" works?

 

Like it was pointed out before, your debounce is flawed! You have to push the button for 50 seconds for anything to happen!
Try reducing your "Debounce" to for example a value of 1, then hold the button for more than one second and see what happens! wink

- Brian

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

jodank wrote:
I use the original EasyAVR5 dev. board, not my wiring up etc...

Show your connections, as I said.  Tell the voltage right at the pin, as I said -- when the button is pressed, and when the button is not pressed.  Is an active-high button just floating?  It would cause the symptoms.  I did not ask the questions just to up my post count.

 

So, that is the first step.  If indeed you sniff at those results -- "of COURSE the signal levels are proper; here are the values..." -- then analysis can move forward, with e.g  the delay discussions.

 

I'm out.

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

Note that writing programs works better if you take a little time up front to plan the exact sequence of what you hope is going to happen. Don't just sit in front of your C editor and type things in as they occur to you. This gets increasingly more important the more complex your designs get.
.
For buttons to be "responsive" to an end user you probably want to be checking their state something like every 50ms. (1,000 times more often than you currently do).

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

jodank wrote:

It looks like you need to hold the button down for 50 seconds before the delay gets smaller.

 

 

I don't think it has anything to do with seconds of waiting.

I don't set up any seconds etc.

The 50 cycles should count the switch bouncings between the pressed and released state.

I assume after 50 bouncing the switch is released physically.

....

What did I do wrong?

Think it through.  Every time you go through your main loop, it takes 1 second (2 500 ms delays as you blink the LED).  How many times do you have to go through the main loop to detect a button push and modify the delay time? 50, so 50 seconds.

 

Whether you use a timer or _delay_ms, you need to have a small timing 'tick' which you use for both timing operations, debouncing and blinking.  10 ms is a common value because it's a nice round number.  Call that timing tick 'T', so let's say T = 10ms.  Then you use 2 software counters - variables that your software increments or decrements each T ms - and associated software flags.  For debouncing you might want to count 2 to 5 ticks, while for your blink you would count 50 ticks for a 1-second blink (500 ms on, 500 ms off).

 

The key to understand is that your main loop now will be happening rather fast - every T ms.  And you get slower events by running software counters in that fast main loop.  Here is a rough outline, you can work out how this might be turned into actual code:

 

LOOP   // every T ms

   if (button_pushed)

      if (--button_timer_count == 0)

         reduce_blink_delay

         button_timer_value = auto_repeat_value

   else   // button not pushed

      button_timer_count = initial_value

 

   if (LED_state_flag == ON)

      turn_LED_on

   else

      turn_LED_off

   if (--blink_delay_count == 0)

      LED_state_flag = !LED_state_flag

      blink_delay_count = blink_delay

 

   _delay_ms(T)

END LOOP

 

Once you see how this works, you can easily add a second button to increase the blink delay in the same manner.  You can also then use a hardware timer to generate the ticks, and simply look for a timer tick flag in the main loop.  Lots of ways to do this, both with and without timer interrupts.

 

EDIT: things get rather interesting as you keep holding the button down after the debounce.  Do you want to force the user to release the button and then push again to decrement the delay again?  Or do you want to set up an auto-repeat the way most keys work?  Either one is easy.  I've modified the pseudo-code for auto-repeat.  Think about how you would change it for release-and-push-again.

 

Last Edited: Thu. Sep 13, 2018 - 03:24 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Ok, here is the schematic:

 

The voltage on the pin when the button is released is 0V.

The voltage when the pin is pressed is 4.63V.

Thanks for your time and helping to point me in the right direction.

 

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

Just out of interest why do it that way up? If you have the switch connect to Gnd not Vcc you can switch to using the resistor built into the AVR. No needfor the external 10K. It does mean the button reads 0 when closed and 1 when open but that can simply be inverted in the software.

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

clawson wrote:

Just out of interest why do it that way up? If you have the switch connect to Gnd not Vcc you can switch to using the resistor built into the AVR. No needfor the external 10K. It does mean the button reads 0 when closed and 1 when open but that can simply be inverted in the software.

 

The xtinies can even invert it in hardware.

 

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

clawson wrote:

Just out of interest why do it that way up? If you have the switch connect to Gnd not Vcc you can switch to using the resistor built into the AVR. No needfor the external 10K. It does mean the button reads 0 when closed and 1 when open but that can simply be inverted in the software.

Also, the idea of running Vcc off the board to switches and such just bothers me personally.  Much better IMO to run Gnd if possible.  

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

Ok guys, why the pins are set to gnd?
Because of the dev board manufacturer made it that way.
Yes I can make the pins setup to tristate, or set a pull-down or pull-up resistor,

but I never set them another way on my board.

 

You say if I understand correctly, I should setup the internal resistor on my input pin and trigger the button to GND and not to VCC?

 

I can do that also, no problem.

 

But, after reading your suggestions here about my code, I really realized does I made a BIG MASSIVE MESS of code. :-)

My debouncing code is not worth the time to "repair" I think so.

 

 

The biggest problem in such of code like mine posted here is the "_delay_ms" function, which will alter the whole cpu for a meaning of time...

 

I would appreciate if somebody can share a piece of debouncing code for beginners.

Before I used Bascom and there was the function for debouncing and I made some filters around the buttons on my hardware, 

but in C it is a bit hard for me to understand everything.

 

Maybe I'm the most stupid guy on the planet but I don't shame me to ask for help.

I'm also not a guru in this stuff and I really can't post good post even if I'm here in this community over 8 years.

I'm a hobbyist in avr and not a regular user of avr stuff, and I developed my projects in Bascom for years.

 As I know, this forum is a better place for C developers than for Bascom developers.

This is the reason behind my low post number and 8 years of been registered here.

I also read the rules of this forum and didn't found any point where is forbidden, to be

here or to must meet some post numbers to be here, or even I don't find any point

of disagreeing of posting maybe for you stupid but for me hard to understand questions. 

 

I'm trying to be honest and to follow the rules of this community.

I don't wanna post and hijack somebody's post just to be there, but I read really frequently 

the posted text here on this community, so I'm learning new stuff.

I also know does here are several persons, who are real gurus in AVR and C programming, I really appreciate that.

But they are not the gods from who we stupid peoples should have fear to ask stupid questions.

That type of behaviors, from my point of view, is intellectually humiliating and it is a question of the personality of that persons.

To be on top of a science is very cool, but don't forget what it's like to be a beginner.

 

I think I answered all the questions or not?

 

So, let us be friends and focus on AVR debouncing problem if you agree.

Last Edited: Sun. Sep 16, 2018 - 05:33 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

Here are some posts I wrote a few years ago that might be helpful to you.

 

https://www.embeddedrelated.com/...

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

That's what about I talk.

Thank you so much, man!

 

My best regards.

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

I think you should re-read my post #8 again.

I was just curious to your background, and I try to adopt to people with more experience in other subjects then programming.

 

I thought the way of adding structure into your program by making the key scan and led functions non-blocking was a valuable idea for you with which you can make yourself a better programmer. Using long software delays is almost always a bad solution, and it makes your program harder to understand if your uC has to do different things.

 

The only difficult part is to set up a hardware timer for "proper" timing.

But if approximate timing is acceptable you can easily use a software delay instead of a timer interrupt to add some cadence to your program.

Like so:

void main( void) {
    
    for( ;;) {
        _delay_ms(1);       // Wait a bit for a somewhat regular interval.
        do_keyboard( );
        do_leds( );
        do_relays( );
        // etc...
    }
}

The main idea is to separate funcionality from each other. This technique can be used in C, Basic, or any other programming language.

Paul van der Hoeven.
Bunch of old projects with AVR's:
http://www.hoevendesign.com

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

Paulvdh:
Thank's Paul for the direction and kk6gm gave me also a good tutorial in his #18 post,  what I'm now reading and trying to use it in my code.

 

I appreciate your time and also other peoples time too.

 

I will be out for some time and trying t rewrite my code to a less massive messy code... laugh

But I  will be back! with another messy code! cool

My best regards.

Thank you.