Why this LED blinking code doesn't work?

Go To Last Post
36 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.

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

HI!
Here is my new version of the code what I wrote based on information from the internet and examples what I found and studied.

I had troubles with the timer setup but I hope I did it well.

Can somebody pls check my counter setup and maybe give some suggestion if I have done something wrong?

 

Actually, the code is working, the switches are debounced and the toggling speed on the PB0 is well done, but when I calculate the

timing on paper and when I compare it on my scope I get differences...

 

I use Attiny13 uC.

Here is my code:

 

#define F_CPU 1200000UL
#include <avr/io.h>
#include <avr/interrupt.h>

int counter = 0;                
int Delay = 1;
int Step = 100;

int SPS_PB0 = 1; 
int SWP_PB0 = 1; 

int SPS_PB3 = 1; 
int SWP_PB3 =1; 

static inline void Timer_On(void)
{
	TCCR0A |= (1 << WGM01);				// clear timer on compare match
	TCCR0B |= (1 << CS01);				// clock prescaler 8
	OCR0A = 25;					// trigger interrupt every ([1 / (1.2E6 / 8)] * 25 = 200uS) ??
	TIMSK0 |= ( 1 << OCIE0A );			// enable output compare match interrupt
	sei();                                          // enable interrupts
}


ISR(TIM0_COMPA_vect)					// TIMER0 ISR
{
	counter++;
	
	/****************************************** START rev up LED ***************************************/
	if ((PINB & (1 << PB4)) != SPS_PB0)   
	{
		if(!SWP_PB0)                     
		{
			Delay=Delay-Step; 
			
			if (Delay < 100)
			{
				Delay=1;
				PORTB = (1 << PB2);
			}
			else
			{
				PORTB &=~ (1 << PB2 );
			}
			
			counter = 0;           
			SWP_PB0 = 1;               
		}
		else
		SWP_PB0 = 0;  	      
	}
	SPS_PB0 = (PINB & (1 << PB4));        
	/****************************************** END rev up LED *****************************************/
	
	
	/****************************************** SLOW down LED ******************************************/
	if ((PINB & (1 << PB3)) != SPS_PB3)   
	{
		if(!SWP_PB3)                     
		{
			Delay = Delay + Step;
			
			if (Delay > 1000)
			{
				Delay=1000;
				PORTB = (1 << PB2);
			}
			else
			{
				PORTB &=~(1 << PB2);
			}
			
			counter = 0;
			SWP_PB3 = 1;              
		}
		else
		SWP_PB3 = 0;  	      
	}
	SPS_PB3 = (PINB & (1 << PB3));        
		
	/****************************************** END slow down LED **************************************/
	
	/* Toggle output based on the Delay setup */
	if (counter == Delay)
		{
			PORTB ^= (1 << PB0);
			counter = 0;
		}
}

int main(void)
{
	
	DDRB |= (1 << PB0) | (1 << PB2) ;	
	PORTB |= (1<<PB4) | (1<<PB3);	
	Timer_On();	
	
	while(1)
		{
			
		}
	return 0;
}

 

When I try this code with CKDIV8 fuse bit unchecked I get on the scope a signal which is 22uS off = 0V and 22uS on = +5V.

Should I get around 200uS?

 

When I try this code with CKDIV8 fuse bit checked I get on the scope a signal which is 176uS off = 0V and 1762uS on = +5V.

That's for me correct because the clock is divided by 8 but I still don't get a signal on and off state 200uS.

 

Should CKDIV8 fuse bit be checked or not?

 

Is this a better code than my previous direction/version?

What do I missing? 

 

Thank you.

 

 

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

[1 / (1.2E6 / 8)] * 25 = 167 us

 

--Mike

 

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

Is it really 1.2MHz anyway??

 

EDIT: Ah OK, I see it's one of these "odd" AVRs that has a 9.6MHz (rather than 8MHz) default clock. So I guess that without CKDIV F_CPU is 9,600,000 and with CKDIV8 it is 1,200,000

Last Edited: Thu. Sep 20, 2018 - 10:55 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

avr-mike wrote:

[1 / (1.2E6 / 8)] * 25 = 167 us

 

--Mike

 

Oh!
Thanks for this correction mate.

 

clawson:

Yes, this uC has an internal clock of 9.6MHz, so I made the calc: 9.6/8=1.2E6MHz.

Therefore I used this F and the CKDIV8 should be checked I assume.

 

So, is actually my signal on the scope with 176uS On and 176uS Off enough accurate compared to the math I made?

 

Is this Timer right configured?

 

Thanks.

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

There's a +1 missing too:

 

[1 / (1.2E6 / 8)] * (25 + 1) = 173.33 us

 

--Mike

 

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

avr-mike wrote:

There's a +1 missing too:

 

[1 / (1.2E6 / 8)] * (25 + 1) = 173.33 us

 

--Mike

 

 

Can you pls help me to understand the equation now?

I'm confused now.

 

Thanks.

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

jodank wrote:

avr-mike wrote:

There's a +1 missing too:

 

[1 / (1.2E6 / 8)] * (25 + 1) = 173.33 us

 

--Mike

 

 

Can you pls help me to understand the equation now?

I'm confused now.

 

Thanks.

To count N timer clock cycles you need to load a timer OCR register with N-1.  It's just how they work - I can explain more if you need to know why.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1
			if (Delay < 100)
			{
				Delay=1;
				PORTB = (1 << PB2);
			}

You mean Delay = 100, right?

 

Also, using global variables in your ISR is not a good approach.  If a variable is only used in the ISR it should be declared 'static' inside the ISR.  If the variable is to be shared between an ISR and main code, it does need to be global or static to the file, but then it must be declared 'volatile'.

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

kk6gm:

I would appreciate if you can a bit more explain the timer setup on a maybe of a more average way if it is actually possible.

I have done a lot of project with timer/counter in Bascom but even there I run into troubles until I finally figured out how it works and where I made my errors.

All that because I don't really understand that maybe simple function of an AVR...

 

Here is my I hope final code:

#define F_CPU 1200000UL
#include <avr/io.h>
#include <avr/interrupt.h>

volatile int counter = 0;				// Interrupt cycle counter
volatile int Delay = 500;				// PB0 toggle speed setup value	- changeable through PB3 & PB4
static int Delay_max = 1000;			        // ~100ms toggle ( slow toggle )
static int Delay_min = 100;				// ~10ms toggle	 ( fast toggle )
static int Step = 100;					// toggle step setup for PB3 & PB4

static int SPS_PB4 = 1;
static int SWP_PB4 = 1; 

static int SPS_PB3 = 1;
static int SWP_PB3 =1; 

static inline void Timer_On(void)
{
    TCCR0A |= (1 << WGM01);				// clear timer on compare match
    TCCR0B |= (1 << CS01);				// clock prescaler 8
    OCR0A = 25;					        // trigger interrupt every ([1 / (1.2E6 / 8)] * (25+1) = 173.3uS)
    TIMSK0 |= ( 1 << OCIE0A );			        // enable output compare match interrupt
    sei();                                              // enable interrupts
}

ISR(TIM0_COMPA_vect)					// TIMER0 ISR
{

    counter++;

    /****************************************** START rev up LED ***************************************/
    if ((PINB & (1 << PB4)) != SPS_PB4)
    {
        if(!SWP_PB4)
        {
            Delay = Delay-Step; 

            if (Delay <= Delay_min)
            {
                Delay = Delay_min;
                PORTB = (1 << PB2);
            }
            else
            {
                PORTB &=~ (1 << PB2 );
            }

            counter = 0;
            SWP_PB4 = 1; 

        }
        else
        SWP_PB4 = 0;
    }
    SPS_PB4 = (PINB & (1 << PB4));
    /****************************************** END rev up LED *****************************************/

    /****************************************** SLOW down LED ******************************************/
    if ((PINB & (1 << PB3)) != SPS_PB3)
    {
        if(!SWP_PB3)
        {
            Delay = Delay + Step;

            if (Delay >= Delay_max)
            {
                Delay = Delay_max;
                PORTB = (1 << PB2);
            }
            else
            {
                PORTB &=~(1 << PB2);
            }

            counter = 0;
            SWP_PB3 = 1;      

        }
        else
        SWP_PB3 = 0;
    }
    SPS_PB3 = (PINB & (1 << PB3));        

    /****************************************** END slow down LED **************************************/
}

int main(void)
{

    DDRB |= (1 << PB0) | (1 << PB2) ;
    PORTB |= (1<<PB4) | (1<<PB3);
    Timer_On();	

    while(1)
        {
            /* Toggle output based on the Delay setup */
            if (counter == Delay)

            {
                PORTB ^= (1 << PB0);
                counter = 0;
            }

        }

    return 0;

}

I assume I defined everything correct.

My final goal should be to click a relay on PB0.

 

Thank you all of you for helping.

My best regards/

Last Edited: Fri. Sep 21, 2018 - 08:05 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

Is there a question or questions about the last posted code #30 ?

 

Once you learn about "volatile", you also need to learn about "atomic access" to multi-byte variables.

 

A small style point:

jodank wrote:
if (counter == Delay)

I always do (counter >= Delay)  It costs you nothing to use the comparison, it operates identically, and helps to protect code from "running away".

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

A few random thoughts:

 

What is your counter counting?  From your comments it is counting ticks of 173.3 us.  Why?  That sure sounds like a strange value to me.  Why not set your timer up for 1ms or 5ms or 10ms?

 

If you do want to do all your calculations in units of milliseconds (which is a good idea IMO), and if your tick happens e.g. every 5ms, simply add 5 to your counter in the ISR, rather than incrementing it.

 

Agree with theusch that >= is a more bulletproof test than ==, especially when you start adding other time-consuming code to your main loop.  This raises the question, what if you do go over, so that e.g. counter equals 505 when you make the comparison?  One way to help keep long-term timing accuracy would be to do counter -= Delay rather than counter = 0.  (and BTW, you know you can write Delay -= Step, right?)

 

On the subject of atomic access, here's a quick explanation of one possible failure mode:

MAIN: fetch Delay LSB

Timer interrupts

ISR: modify Delay and exit

MAIN: fetch modified (!!) Delay MSB and perform calculation

 

Now you're working with a Delay value that is half old and half new, i.e. garbage.  The problem happened because the AVR cannot read or write a 16-bit int in a single operation, but must perform multiple operations, and an interrupt can occur in the middle of those multiple operations.

 

A common solution is to disable interrupts (or at least any interrupt that can affect the data you're after), access the data, then re-enable interrupts.  One method in your case could be:

 

cli();
bool time_to_blink = (counter >= Delay) ? true : false;  // allow safe access of both counter and Delay
sei();
if (time_to_blink)
{
}

Another method is to set single-byte flags in an ISR, then check them (which is a safe operation for the AVR) in main code.  For example, you could have this in your main code:

 

if (dec_Delay)
{
    Delay -= Step; // ignoring range checking for clarity
    dec_Delay = false;
}
else if (inc_Delay)
{
    Delay += Step;
    inc_Delay = false;
}

I'll let you figure out how dec_Delay and inc_Delay would be set in your ISR.

 

One last comment for now.  Why are you using signed integers for your count and delay?  Those values will never go below zero.  I have a little mantra that is helpful, especially for small (especially 8-bit) embedded processors:

 

Never use floats when longs will do

Never use longs when ints will do

Never use ints when bytes will do

Never use signed when unsigned will do

 

Last Edited: Fri. Sep 21, 2018 - 10:39 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Nice to see you've got an timer interrupt going.

About the CKDIV8 Fuse. My advise is to not program it. You want a (relatively) fast F_CPU, especially during software development and/or when current consumption is not of premium concern.

Higher clock rates gives more opportunity for your uC to do stuff.

Even when you want to run your AVR from a small battery it is usually better to use a high clock frequency and put the uC to sleep as much as possible.

I don't really understand why they bothered to implement that fuse.

 

So instead of using the CKDIV8 fuse, you should use another preload value for OCR0A. Start with 8*25 = 200 to get similar timing to what you had with the CKDIV8 fuse programmed.

Then later adjust the OCR0A value to get a timing that is easy to remember (such as 1ms or 10ms, just as kk6m suggests in #32).

If your program has to check different things between each timer tick, you do not want this interval to be too small, your uC needs time to do those things in the allotted interval, and this is another reason you want a relatively high F_CPU.

 

For most programs it is also recommended to keep the time spent in interrupts as low as possible.

The main reason is that it is hard to write reliable programs if you get into the area of interrupts which have to interrupt other interrupts.

Your code is much easier to understand and maintain if you do most of the processing in a flat structure in main().

So this is where we get into the area of interacting between interrupts and the main(), non interrupt code.

In your example from #22  -> #30 you added the volatile keyword to your counter variable, which is good.

(Have you read some background on what it does, and why it is neccasary?)

 

The next step will be to move the code from your ISR to main.

The minimum version of your ISR would be:

 

ISR(TIM0_COMPA_vect)					// TIMER0 ISR
{
    counter++;
}

 

In your main() function you can then use this counter variable for all kind of timing.

It is very common to write a few functions as an interface library for timing purposes.

For example, the first thing you should have learned about working with ISR's is the volatile keyword.

To interact with shared variables with ISR's you then also need to use cli() and sei() (or other methods) to ensure your program works correctly.

So the first (very simple) function of your timer library could be a simple function to read your counter value.

uint16_t counter_read( void) {
    // Simple atomic read of volatile counter.
    uint16_t temp;

    cli();
    temp = counter;
    sei();
    return temp;
}

 

The next thing you want to have is to be able to wait for your ISR to occur.

You could do that for example with:

void counter_wait( void) {
    // Wait untill the next ISR timer tick. (counter gets incremented).

    uint16_t tmp = counter_read();

    while( counter_read() == temp)
      ;
}

 

And then you can extend that with delay functions, a stopwatch or other timing related stuff you want.

A trivial next step is to use your counter functions for timing.

For example:


 uint16_t start, end;

 start = counter_read( );
 do_something( );
 end = counter_read( );

 printf( "do_domething took: %d ms \n", end - start);

 

A subtle but very important trick is to always calculate time intervals as the difference between the end  *** MINUS *** the start.

The reason is that your counter value can and will overflow, and this can easily lead to errors.

For example, if you use a 1ms timer ISR, a 16 bit counter will overflow about every minute. (an uint16_t can count to maximum of 65535 before it overflows to 0).

 

By calculating the difference, the calculated interval will wrap back to what it should be without errors.

I wrote a little example:

#include <stdio.h>
#include <stdint.h>

int main( void) {

	uint16_t start, end, interval;

	start = 65000;    // Just before a simulated overflow.
	end = 200;        // Just after a simulated overflow.
	interval = end - start;

	printf(" Interval = %d\n", interval);
}

 

You can compile and run this example on your pc (if you have a C compiler installed.

I saved the program as "asdf.c" and compiled to the output (with "-o") asdf.

On Linux you have to add the pathname to all "unknown" locations. The current working directory usually is not searched for programs.

So "./asdf.exe" means: Execute the program asdf.exe in the current working directory ( "./" is a reference to the current working directory).

paul@dualcore:~$ gcc asdf.c -o asdf.exe
paul@dualcore:~$ ./asdf.exe
 Interval = 736

With the information I gave above you should be able to verify why 736 is the right answer here.

 

Edit: Return temp, instead of counter in 2nd code snippet (Tnx avr-mike #34).

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

Last Edited: Sat. Sep 22, 2018 - 11:22 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Paulvdh wrote:

uint16_t counter_read( void) {
    // Simple atomic read of volatile counter.
    uint16_t temp;

    cli();
    temp = counter;
    sei();
    return counter;
}

 

I'm sure you meant to write return temp;

 

--Mike

 

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

Paulvdh wrote:
I don't really understand why they bothered to implement that fuse.

I'll leave the determination about a Good Thing or not to you experts.  But the datasheet, for [every?] AVR8 model, has an explanation on why there is [universal?] use of CKDIV8 on virgin chips.

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.

Last Edited: Sun. Sep 23, 2018 - 01:24 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Paulvdh wrote:
I don't really understand why they bothered to implement that fuse.

Because AVRs cannot run at high clock rates when powered by low (but in-spec) Vcc.  So if your system is designed for a low (but in-spec) Vcc, without CKDIV8 or equivalent, it would be running out of spec when powered up on the internal oscillator.  Having CKDIV8 or equivalent assures that the internal oscillator clock rate will be valid for any valid Vcc.  This chart is from a '328, but all AVRs have similar restrictions.

 

Last Edited: Sun. Sep 23, 2018 - 02:57 PM