Why this LED blinking code doesn't work?

Go To Last Post
79 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: 3

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: 2

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: 1

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.

The main reason for the CKDIV8 fuse is probably  in situations where the uC has to run on low voltages, where the clock has to be de-rated to be within speck ( Edit: See kk6gm's remark in #36). The combination of low voltage and low F_CPU can make for a power efficient design.

 

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: Sun. Sep 23, 2018 - 06:33 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
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The fuse just sets the initial value of the CLKPS (clock prescale) bits in CLKPR.

You are able to change this value in your code to achieve any clock speed, even

full speed, while leaving the fuse as-is.

 

--Mike

 

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

HuH!
Huge writings, you are really amazing how much info you are shared with me.

Thank's for such of helping.

 

By my side is the book from Kernighan &  Ritchie "The C programming language 2nd edition" and I digging non-stop in it.

 

I corrected my timing in the code from 173.3uS to 200uS.

Here is my example snippet from my code :

 

The old code:

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
}

 

New code:

static inline void Timer_On(void)
{
	TCCR0A |= (1 << WGM01);				// clear timer on compare match
	TCCR0B |= (1 << CS01);				// clock source and prescaler 8 setup ( Timer/Counter Clock Sources datasheet pg.58)
	OCR0A = 29;					// trigger interrupt every ([1 / (1.2E6 / 8)] * (29+1) = 200uS)
	TIMSK0 |= ( 1 << OCIE0A );			// enable output compare match interrupt
	sei();                                          // enable interrupts
}

This gives me a better calculation than the old code as you guy's told before, also on my scope it is really 200us...

 

The first point with the timer implementation was because of the switch-debouncing problem in my code,

but now I think  I wish to do something more also, but I have no clue how to achieve that.

Any suggestion please would be nice.

 

In my main code I don't really figured out how to wait 4ms until the PB0 is cleared:

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);
		        PORTB |= (1 << PB0);
		        
		          WAIT 4ms ???
		        
		        PORTB &= ~(1 << PB0);
				
				counter = 0;
			}
			
		}
		
				
	return 0;

 

Let's say, every 200ms PB0 goes from High to Low.

But the switching time on PB0 from High to Low should be delayed to 4ms and not another 200ms.

 

About the CKDIV8:

If I understand you, that should be programmed if I use the uC on a lover voltage.

On the spec. diag. from the datasheet, I can figure out does if my uC is powered with 5v then I 

have nothing to worry, because the safe operation area Fmax vs VCC is around 20MHz.

So, set or not set the CKDIV8 makes no root changes but in calculations...

From the datasheet, I have the information does the Attiny13 comes with the CKDIV8 fuse enabled by default when it is new.

 

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

Again, why are you choosing a 200us timer interrupt tick?  Why not something more human-brain friendly like 1ms (or 5ms or 10ms)?  If you have a reason, that's fine, but you're not communicating it with us.

 

As for your question, I think you should start thinking in terms of states.  You can define states with an enum statement, or straight defines.  Here is one skeletal structure that would work, using a variable Delay count and a fixed 4MS_DELAY count.  There are other ways that would also work, like the leap-frogging technique in the tutorial I linked to.

 

enum {PB0_OFF, PB0_ON};

uint8_t state = PB0_OFF;
...
while (1)
{
    if ((state == PB0_OFF) && (counter >= Delay))
    {
        PORTB |= (1 << PB0);
        state = PB0_ON;
        counter -= Delay;
    }
    else if ((state == PB0_ON) && (counter >= 4MS_DELAY))
    {
        PORTB &= ~(1 << PB0);
        state = PB0_OFF;
        counter -= 4MS_DELAY;
    }
}

Note that I didn't deal with atomically accessing the counter variable, which you need to do if it is being incremented in the ISR.

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

 Thanks for the quick replay kk6gm.

Again, why are you choosing a 200us timer interrupt tick?

 I'm working on it to migrate the code from the TIMER0 ISR and just let the counter++ in there but I have jet some problems with it, I won't give up.

 

So to say, when I make changes to the timer interrupt tick I have some problems with the debouncing again.
When I change my code to:
 

OCR0A = 149;		// trigger interrupt every ([1 / (1.2E6 / 8)] * (149+1) = 1ms)

The debouncing stuff is meesy and won't work again, it is to slow, I have to figure out how to separate the switch pressing 

code from the timer isr and use them in the main of my program and use the timer also for the debouncing....

 

 

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

jodank wrote:

So to say, when I make changes to the timer interrupt tick I have some problems with the debouncing again.
When I change my code to:
 

OCR0A = 149;		// trigger interrupt every ([1 / (1.2E6 / 8)] * (149+1) = 1ms)

The debouncing stuff is meesy and won't work again, it is to slow, I have to figure out how to separate the switch pressing 

code from the timer isr and use them in the main of my program and use the timer also for the debouncing....

The solution to this is to make all your delay values defines (or calculations) based on your tick interval, rather than hard-wired.  It might look something like this:

 

#define TICK_US 1000        // 1ms
#define DEBOUNCE_US 20000   // 20ms
#define DEBOUNCE_TICKS (DEBOUNCE_US/TICK_US)    // so 20 ticks
#define PB0_ON_US 200000
#define PB0_ON_TICKS (PB0_ON_US/TICK_US)
#define PB0_OFF_US 4000
#define PB0_OFF_TICKS (PB0_OFF_US/TICK_US)

You can even derive your OCR0A value this way

 

#define TICK_FREQ (1000000/TICK_US)     // 1000 Hz for 1000us tick
#define T0_PRESCALE 8
#define T0_TICK_VAL ((F_CPU/T0_PRESCALE/TICK_FREQ)-1)    // 149

Always define or calculate values that depend on other values in this manner, rather than hard-coded "magic numbers".

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

Hmmm...
I have somehow to separate the timer and the other stuff (PB0 on-off, switch bouncing etc...).

 

From now, I'm not so happy with this debouncing technique.

The button interacting is working in the ISR. I assume that is not a good implementation.

 

Ok, I need some time pls to understand all this what you are shared with me.

 

Thanks, guys.

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

jodank wrote:

Hmmm...
I have somehow to separate the timer and the other stuff (PB0 on-off, switch bouncing etc...).

 

From now, I'm not so happy with this debouncing technique.

The button interacting is working in the ISR. I assume that is not a good implementation.

 

Ok, I need some time pls to understand all this what you are shared with me.

 

Thanks, guys.

OK, think about this.  What if this was your entire timer ISR:

volatile bool timer_tick = false;

ISR(...)
{
    timer_tick = true;
}

And your main code:

// main while(1) loop here
...
if (timer_tick)
{
    timer_tick = false;
    // do here what you would do in ISR
}
...

 

Last Edited: Mon. Sep 24, 2018 - 06:38 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

@kk6gm #43

Setting a bool in your ISR also works, and it's also extensible, you could handle multiple bools and even at different rates.

Disadvantage is that if code gets more complex the ISR grows to handle those different flags.

 

With incrementing a counter in the ISR you have a very short ISR and you can still use as many derived timer vars as will fit in the uC.

A simple counter increment is also an easy way for making variable timing stuff.

If you want to do variable timing with bools in the ISR, more communication between the ISR and the main program is needed, and you have to maintain the code in 2 different places when anything changes.

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

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

Ok, I have two questions which are not clear for me in my "thinking" brain:

 

1. Timers have several types of operation mode.

I set up my timer in my program to work in CTC mode.

That mean's the compare register will be cleared when it reached the counter value in TCNTx.

 

In my program, I set that value in OCR0A to 29 ticks, so the timer will reset itself to 0 when the TCNTx value is 29.

That means in my case it would happen in every 200us.

 

Let's say, there is the code from above:

volatile bool timer_tick = false;

ISR(...)
{
    timer_tick = true;
}

In the ISR the timer_tick will every 200us set to true. ( I used the 200us because it is set in my code, no matter what time is set... )

 

the code in the main is doing something with the timer_tick.

That is also fine.

 

But, what if I need to do two or more job what have to be timed also?

Or what if some job should be done after let's say 600us? ( that is out of the 200us, should I implement a separate counter to count to 3? because 3cycle * 200us = 600us)...
Let's say:

 

If (timer_tick) then LED = ON  // every 200us the LED will go to on state.

 

But the led should go off in let's say 4us from the moment when it was lit up?

 

This should also be covered by the timer I assume.

Should I do something like this in my code?

 

If (timer_tick) then LED = ON

check timer_tick for 4 more ticks and then

LED = OFF

Actually, that different time sharing through one timer goes me over my head...

I don't know if I could really describe what I mean.

 

 

2. Should the ISR routine be written before the MAIN ?

Is there a regulation in C or can I write my ISR's after the MAIN also?

 

 

Thanks.

 

 

 

 

 

Last Edited: Tue. Sep 25, 2018 - 06:33 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

jodank wrote:
That mean's the compare register will be cleared when it reached the counter value in TCNTx.
Err no, wrong way round. The TCNT register will be cleared when it matches the compare register value.

jodank wrote:
Or what if some job should be done after let's say 600us?
So you use your 200us interrupt for multiple purposes:

volatile uint8_t tick_200;
volatile uint8_t tick_600;

ISR(TMR_vect) {
    static count = 0;
    tick_200 = 1;
    count++;
    if (count >= 3) {
        count = 0;
        tick_600 = 1;
    }
}

Now tick_200 becomes set every 200us but tick_600 only becomes set every 600us (because there's a count to 3 each time). Obviously you could extend this as long as the timer runs at your fastest required rate and your other processes are some integer multiple (like X3) of the core rate. So this one time interrupt could be running 10 or more different things. In fact think of your Windows/Linux machine - right now it is running 50..100 processes and they are all being triggered from a single 10ms timer.

2. Should the ISR routine be written before the MAIN ?

Is there a regulation in C or can I write my ISR's after the MAIN also?

Very much a question of personal taste - do you want the reader to have to look at the top or the bottom of the file to find main() so they can then follow on from that to all the other functions it calls. With ISR() (because they aren't actually called from main()) it really does not matter where you put it (many would perhaps put it in a separate interrupt.c or perhaps a timer.c or something). But consider the case of "normal" functions. In C code such as:

int main(void) {
    PORTB = add(123, 207);
}

uint16_t add(uint8_t a, uint8_t b) {
    return a + b;
}

then as the C compiler reaches the call to add() in main() it does not yet know that add is a function that takes two uint8_t and returns one uint16_t so you have two ways to tell it. One is to declare the function above main:

uint16_t add(uint8_t, uint8_t);

int main(void) {
    PORTB = add(123, 207);
}

uint16_t add(uint8_t a, uint8_t b) {
    return a + b;
}

Now the compiler has what it needs to know to prepare the inputs and accept the output when it comes to the use of add() in main. The other way to do this is:

uint16_t add(uint8_t a, uint8_t b) {
    return a + b;
}

int main(void) {
    PORTB = add(123, 207);
}

Because a function definition also acts as a declaration, by putting the function before main() where it is used there is no need for a separate declaration.

 

Now folks argue both ways as to which of these is "better". To be honest the formal, separate declaration of the function is probably the better approach (you will do this a lot when you switch to C++) but some argue that this requires two things (the declaration and the definition) to be maintained. Suppose I change it so that parameter 'a' is now uint16_t not uint8_t I might need to make this change in two places. 

 

The functions defined before invocation approach avoids this and is simply less typing - so some might prefer that.

 

I'm ambivalent myself - seen too many example of both - so will accept either. Having said that, when I open a 2000 line main.c I will often jump to the end hoping to find the main() function that invokes everything else at the bottom - sometimes I'm lucky and it is there!

 

As I said above ISR() are a bit different. They have a "void ISR(void)" interface anyway but nothing in your code calls them either so they don't need to be declared before use - so you can put them where it suits you.

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

Sorry because I was a few days out, actually I practiced and tried to understand the timer more closely to me etc.

 

I understand the timer in this meaning, am I right?

 

The timer is a completely self-standing device inside the uC.

Like a stopwatch in a man's hand.

A stopwatch has also different modes to make some measurements.

So the timer in the uC has also his modes, like CTC, OVf etc.

To set up the timer we need a prescaller to slow down the timing compared to the core timing of the uC,

to select a clock source, to set up the mode in which we wish to use the timer etc.

Like the man with the stopwatch who takes measurements of a runner average speeds.

The stopwatch is set in a mode to take several results and then calculates the average time...

This could be let's say the CTC mode of an avr timer. ( of course, CTC has nothing to do with the average speed takeing. )

 

But the timer inside the avr won't stop even if the program in the main is doing some other stuff.

Like the man with the stopwatch.

If the runner is stumbling after a period of time, the man with the stopwatch will count forward uninterrupted.

This is in nut shell the way I understand the timer in my imagination.

 

So, I set a task for myself to do :

To use a Timer in CTC mode and toggle the PB0 on the Attiny13 with a puls, which should 20ms be high and 100ms low.

Here is my code:
 

#define F_CPU 1200000UL			    // Define CPU frequency in Hz. - for Attiny13
#include <avr/io.h>
#include <avr/interrupt.h>

volatile uint8_t tick_20ms;                  // variable to set when 20ms achieved  
volatile uint8_t tick_100ms;                // variable to set when 100ms achieved

static inline void Timer_On(void)
{
	TCCR0A |= (1 << WGM01);		    // clear timer on compare match
	TCCR0B |= (1 << CS01);		    // clock source and prescaler 8 setup ( Timer/Counter Clock Sources data sheet pg.58)
	OCR0A = 149;			    // trigger interrupt every ([1 / (1.2E6 / 8)] * (149+1) = 1ms)
	TIMSK0 |= ( 1 << OCIE0A );	    // enable output compare match interrupt 
	sei();                              // enable interrupts
}

ISR(TIM0_COMPA_vect)			    // TIMER0 ISR
{
	static int counter = 0;             // defined the counter variable
	counter++;                          // incrementing the counter by 1 every 1ms
	
	if (counter >= 120)		    // 120 - 100 = 20cycle = 20ms
	{
		tick_20ms=1;	            // the counter achieved the 20ms
		counter = 0;                // reset the counter to zero
	} 
	
	if (counter >= 100)		    // 100cycle = 100ms
	{
		tick_100ms=1;               // the counter achieved the 100ms
	}
	
	
}

int main(void)
{
	
	DDRB |= (1 << PB0);                 // set PB0 as output
	PORTB &=~ ( 1 << PB0 );             // clear PB0
	Timer_On();	                    // start timer
	
while(1)
{
	if (tick_100ms)
	{
	    PORTB |=( 1 << PB0 );           // set PB0 to high   
	}

	if (tick_20ms)                       // 20ms high
        { 
            PORTB &=~( 1 << PB0 );           // clearing PB0 after 20ms 
	    tick_20ms = 0;
	    tick_100ms = 0;	
	}	
}

}

After checking the PB0 with my scope I got a nice signal with 20ms high and 100ms low state.

Is there something I should do in some other way?
Is this code good enough for a beginning in C?

 

Thank you for your time helping me.

My best regards.

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

Nothing jumps out at me that is problematic.  Given the approach you are taking, your code seems to be clean and efficient.  The one thing I would point out is that there is no need for a signed 16-bit integer for 'counter'.  Since it only goes between 0 and 120 (or at most one or two beyond, in some future worst case), you should be using a uint8_t for counter.

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

I would use

 

static uint8_t counter = 0;

 

for 2 reasons: the size of int varies with implementation and you need only 8 bits. In general it's better to use variables with a fixed size for portability.

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

A button is a deceptive thing: it seems impossibly simple and yet it is a devil to code for.   

 

Personally I don't understand why anyone does it anymore.  There are several working (but underdocumented and absurdly complex) libraries for Arduino that handle buttons: one object for each button.  Here is one of the best:  https://github.com/t3db0t/Button

 

People here especially beginners just don't get how much easier it is to work in Arduino instead of standard register-diddling C.   This is most true for the basic microcontroller operations like button reading, I2C, SPI, and, well, everything.   People smarter than you have spent many many hours writing and debugging these freely-downloadable Arduino libraries.  [Most] are excellent examples of good coding procedures because the code gets downloaded, tested, and studied by so many people.    Granted the IDE sucks, but it is possible to change the sizes and colors on just about everything in the Arduino IDE, it's just hard to find out how to actually do it.

 

Beginners think that they are learning how to do "real" coding when they "advance beyond" Arduino.  That is simply not true.   Your "professional" status as a programmer depends on a very simple equation:  (How much you got paid to solve a client's programming issue) / (How much time that you spent writing the code that solves your client's programming issue).  And nobody ever gets paid for studying the AVR's complex internal register and opcode structure.  So why do it if you don't have to and aren't getting paid to do it? 

 

Everything is simpler to do in Arduino.  The most complex algorythm is going to have the same code (more or less) in both AvR Studio C and Arduino because they are both using the same compiler (AVRGCC more or less).  It's in the added functionality of the supplemental keywords that Arduino gives you that makes code writing so much faster and debugging so much easier.   Look at all the time spent above on some silly relatively simple button implementation.  With Arduino libraries, I can write working and functional code for an additional button switch in less time than it takes me to physically add the pushbutton to the prototype circuit board.

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

jodank, if I were tutoring you I would suggest that, now that you have code working with flags communicating between ISR and main code, you try to achieve the same result using 'counter' alone, without flags.  The more ways you learn about how to communicate between an ISR and main code, the more tools you'll have in your toolbox.

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

Simonetta #50 :

I appreciate your explanation and I won't blame Arduino stuff but I watch thinks from some other angle of view.

I'm an old fashion programmer who learned 30 years ago programming language. At that time I made a huge 

mistake because I didn't enter into the world of C.

 

I learned Pascal, Fortran, Basic a bit C also at school but not a big deal.

I can remember of MS Visual Basic 1.0 when it was released early '90.

That was a big WOOOW!

- quick "drawing" of app's

- quick and simple writing of code

- the result a fancy app after several hours

etc.

That is really cool, but with the time, that type of app's grow and grow and grow,

so today, if you wish to compile in MS .NET an empty frame you will get an app about maybe 100Mb.

But who care for 100Mb app's on nowadays PC. Of course, the HDD's are in terabytes, RAM's are also in gigs...

 

But I lost the fundamental knowledge of programming through that visual app development system.

I can remember the old programmers from that days before 30 years and more, they told every time

"to be a good programmer means to be good in hardware".

 

Now I'm here, try to make a simple task with a uC and I stuck because I didn't realize timing, cycles, time constants, variables etc.

I don't care about that, because of the "modern"  developing tools like MS visual .NET stuff, who made from my brain a sponge.

And everything in that developing languages is so easy, cool, quick but at the end, I realize I don't have any control anymore over my app.

Because of some updated dll's what I never know they are used, because of some outstanding components what I also never know I used in my app etc...

 

I think every programmer should have a bit more knowledge of C language than the basics.

 

That's ok to make life easier and develop app's quicker etc. but why Arduino if that is also a derivate of C language?

Why should I not learn C but "Arduino C" ? 

That makes me no sense, C and that "Arduino C" is new for me, but C is the mother of "Arduino C" and the root of many languages on pc,

embedded systems, all uC's can be programmed with C but not with Arduino language, firmware for phones, car ecu etc. are made mostly in C and asm.

So, why to waste time to learn again an incompatible with the world language, just because there are many examples out there which

maybe or maybe not meet the requirement etc..

All that stuff looks for me so commercial.

I fall down into that net once a long time ago but now I have time and I will learn as that is supposed to work.

I wish to understand all the thinks what I can about the uC and use it on my hobby level.

I didn't mention, I wrote my avr project before in Bascom, that is a nice language too but, I never liked it.

I just used because it was easier to me to understand the language.

 

Who are familiar with C and use it on a professional level for let's say uC programming, I don't think they will

mess with some "Arduino C" and they will make the project in maybe same time or even a wors case scenario faster than I'm in "Arduino C".

 

I will learn the roots of the hardware and the major programming language for the hardware.

 

Don't think I hate Arduino, no no nono! 

that's a great product and well supported, it is just my angle of view.

I don't wana learn C for do income for mliving or so, just for my hobby stuff and for fun. :-)

 

kk6gm #51:
Can you pls show a small code snippet what you mean?

I'm not sure I understand what you mean in your suggestion.

Thank you.

 

 

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

jodank wrote:

kk6gm #51:
Can you pls show a small code snippet what you mean?

I'm not sure I understand what you mean in your suggestion.

 

volatile uint8_t counter = 0;

ISR(timer)
{
    counter++;
    // any other ISR code you need here
}
....
main()
{
....
    while (1)
    {
        if (counter something something)
        {
        }
        else if (counter something else)
        {
        }
        else
        {
        }
    }
}

The idea being that none of your program logic lives in the ISR.  All the ISR does is report advancing time, by incrementing the counter.  Everything else having to to with the counter is in your main loop (including resetting the counter when needed, don't forget that).  It's just a different way to approach the problem.

 

Note that as long as the counter is 8 bits, you can read it and set it safely in your main code, because those operations are atomic on 8-bit values.  If you needed to go to a 16 or 32-bit counter, or if you needed to do a RMW (read-modify-write) in the main code, like counter++, then you'd have to take extra precautions.

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

I understand your pain.

I also get lost when confronted with megabytes of software from a multitude of libraries when I want to do something apparently simple.

With uC's as simple as AVR's I can at least keep enough of the picture in my head to not go completely mad.

But even so, whenever programs get bigger than a few pages it gets increasingly more important to have a logical and clear structure in your code.

 

One of the pillars of writing good software is to compartmentalize it in seaparate blocks which have minimal interaction with each other and have a clear and well defined interface to interact with each other.

In your latest code from post #47 the code for blinking your led is divided between the ISR and the while loop in main.

When programs grow this gets increasingly more difficult to maintain and it gets ever easier to make mistakes, and bugs get harder to find.

 

kk6gm in #53 suggests to only increment Counter in the ISR, and manage the rest of the logic elsewhere. I also have a strong preference for this method, as I have written before in this tread.

I even go a step further with abstraction in writing a timer library as an intermediate step between the (low level) interrupt routine and the code of a program itself.

I already wrote about this in #33. I think you should study that post again.

 

About uint8_t versus uint16_t for the counter.

It is true that you can use a uint8_t here and that is faster and avoids some problems with 2 byte variable access. However:

The range of an uint8_t is very limited, and therefore not very suited for a "general purpose" timer or stopwatch function.

But whatever you choose. You should study and get familiar with the uses and limitations of both these options.

Only then can you make a good desicion for yourself.

This is also such an important item that you will encounter it many times when programming.

 

I noticed you have an oscilloscope, and you have used it to measure 2 outputs of the AVR.

I think it is a good exercise for you to extend your ISR function to a general timer library.

Again: Read carefully the section about working with the difference between 2 timestamps from post #33.

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

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

Paulvdh :

Thanks for the great support Paul, I appreciate that.

I read time by time back and forth all the post here and try to get the most out of it.

 

Yes I have a two channel DSO, not a big deal but a great tool for my level. It is the Siglent SDS1052DL+.

Works like a charm in most of my projects.

Now I use it to measuring the output controlled by the timer in my little Attiny13 ...

It is really fun for me now. :-)

 

I'm studying C several months and I made great steps but I can say, it is really a pain in the A... :-)

But it will worth the time, I can feel it...

 

At the time I rewrote the whole code again by the advice I got from you guys here on this forum.

I will post again the new code just to see do I make some good or bad progress. :-)

 

Thanks all of you here for the help.

My best regards.

 

 

Last Edited: Wed. Oct 3, 2018 - 11:26 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Paulvdh wrote:

About uint8_t versus uint16_t for the counter.

It is true that you can use a uint8_t here and that is faster and avoids some problems with 2 byte variable access. However:

The range of an uint8_t is very limited, and therefore not very suited for a "general purpose" timer or stopwatch function.

But whatever you choose. You should study and get familiar with the uses and limitations of both these options.

Only then can you make a good desicion for yourself.

This is also such an important item that you will encounter it many times when programming.

I agree that, while a uint8_t works for the given application, and is safe and painless, it may very well not be appropriate for bigger programs (though it could count 2500 ms worth of 10 ms ticks).  These are all things that the OP should be made aware of.  In fact, the next learning step after getting a uint8_t working could be to safely convert counter to a uint16_t.

Last Edited: Thu. Oct 4, 2018 - 12:00 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Ok, I have two questions:
1. what is OP?

2. Why you say "... to safely convert counter to a uint16_t" ?

 

Can I not define the counter variable just :

volatile uint16_t counter;

uint8_t is for 8bit length variable but uint16_t is for 16bit length variable, that tells me does 

uint8_t can be set to 0xFF or 255, or the counter can count max to 255 tick or counter can hold max number of  255.

uint16_t can hold 2 bytes long data, that is a much bigger number than uint8_t.

That's for me clear, but I can use a counter system to measure a bigger time constant than the 8bit variable can hold.

That is just one option how to do a larger time measuring.

Another option would be to define the counter variable like this:
 

volatile uint16_t counter;

Why is this not good? or why is this not safe?

I think I missing something.  frown

 

But, I will say something to you, I'm extra happy with my little attiny13 and my timer function and I wrote a new "debouncing" program

for my two switches and when I look on my scope that is perfect. 

I will show you my new creation, just to share with you my joy. laugh

I achieved that thank's to you guys.

 

Thank you.

 

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

OP = original poster (ie You) though sometimes it means original post (ie post #1 that started a thread)
.
Oh and you probably want to google "atomicity" and in the specific case of avr-gcc "ATOMIC_BLOCK"
.
uint8_t is naturally atomic but because the AVR doesn't do 16 bit loads uint16_t is not.

Last Edited: Thu. Oct 4, 2018 - 05:27 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thank you clawson, got it.

 

My best regards.

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

Here's a quick example of an atomicity failure:

 

16-bit counter = 0x00FF;

main code reads counter lsb (0xFF)

--interrupt occurs

--ISR increments counter to 0x0100

--ISR exits

main code resumes, reads counter usb (0x01)

main code works with (incorrect!) counter value of 0x01FF, neither the old counter value of 0x00FF, nor the new counter value of 0x0100.

Last Edited: Thu. Oct 4, 2018 - 08:31 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Somehow I got closer to the problem and meaning of atomic thinks.

But I have to read more to be more familiar...

 

That kind of problem above will happen only if I use the uint16_t on an 8bit uC I assume.

Somehow it is also logical for me that problem.

 

I think in that example from above, maybe let's say,  two or more separated tasks can access the 

counter from the main program too.

If they would make a write to that counter than the counter value would be again incorrect.

 

Is my understanding pointing in a good direction?

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

jodank wrote:

That kind of problem above will happen only if I use the uint16_t on an 8bit uC I assume.

Somehow it is also logical for me that problem.

More generally, if access to any data object can be interrupted and the interrupt code also accesses the data object.  So for an 8-bit uC that could be a single 16-bit integer, or it could be a triplet of 8-bit values like an XYZ coordinate, or it could be any set of data values that must be treated as a single entity.

 

Even an 8-bit object can have an atomicity fail.  Consider how this might fail (hint: read-modify-write)

 

volatile uint8_t counter = 0;

ISR(...)
{
    counter++;
}

main()
{
    while(1)
    {
        counter--;
    }
}

 

Quote:

I think in that example from above, maybe let's say,  two or more separated tasks can access the 

counter from the main program too.

If they would make a write to that counter than the counter value would be again incorrect.

If by tasks you mean preemptive tasks that can launch at any time via some OS or any other interrupt mechanism, then yes.  If you just mean modules that all execute in the main code, then no, because there is no danger of one interrupting the other.

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

Hmmm...
I think the code from above can be critical because of the ISR.

The ISR can increase the value of the counter variable until the 

same counter variable started to decrease in the main.

 

If the counter decrease in the main and the ISR is triggered at the same time the data maybe corrupted in the counter variable?

Could this be a case?

 

I have a question:
What is " preemptive tasks " actually I don't understand the word "preemptive". 

I can't find a translation for it. I also see that word is used in many documents and tutorials about atomicity...

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

Premptive means its not the code in the tasks doing the jobs that chooses when to hand control onto the next but a scheduler will simply stop any task at any time (pre-empt it) and hand control to some other task. A bit like "interrupts" in fact.
.
Preemption is usually done by a timer interrupt in fact.

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

jodank wrote:

Hmmm...
I think the code from above can be critical because of the ISR.

The ISR can increase the value of the counter variable until the 

same counter variable started to decrease in the main.

 

If the counter decrease in the main and the ISR is triggered at the same time the data maybe corrupted in the counter variable?

Could this be a case?

Yep.  Here is an example:

 

counter = 99;

 

MC (main code) loads counter (99)

INTERRUPT

--ISR loads counter (99)

--ISR increments local counter value (100)

--ISR saves back to counter (100)

MC decrements local counter value (98)

MC saves back to counter (98)

 

With counter starting at 99, one decrement and one increment should still leave counter at 99.  But it is at 98.  The ISR increment has been lost, because performing operations on even a byte value in memory is not atomic, and can be interrupted (and hence corrupted).

 

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

Ha ha ha ha ha!
I laugh because there are so many things what can happen accidentally and the code is buggy. smiley

I like this world of C and uC definitively.

 

So to say in general speaking, if a variable is declared as atomic then it is protected from interrupting the data

from an ISR or some other operation at the same time?

 

Like in the example above where the variable is changed by the MAIN and the ISR at the same time and the data is at the end corrupt.

 

Thanks mate.

Last Edited: Sat. Oct 6, 2018 - 07:27 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

timerlibrary.h wrote:

extern uint16_t counter_read( void);

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

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

And of course it gets worse when your counter variable is 16 bits while the AVR is an 8-bit uC architecture.

If the interrupt happens between reading or writing between the first and second byte of counter in the main routine...

The way around is to temporarily disable interrupts while acessing variables shared between interrupts and main code.
    
If you put all source code in a single file you can still accidentally read or write "counter" without properly disabling interrupts, or you can forget to enable interrupts after you have accessed "counter".

The way to do it properly is divided into some steps.
1). Make a "library" as a separate combination of source & header file for your timer functions.
2). Declare "counter" as: static volatile uint16_t counter;
3). Write an interface function to access "counter", as I did in post #33.
4). Declare the interface function as "extern" in the header file:

extern uint16_t counter_read( void);

With the steps above it is not possible anymore to directly access the "counter" variable directly (from another file it is declared in) .

It can only be accessed via the interface function, and this function always disables the interrupts and enables them again afterwards.

 

In post #33 I wrote as comment:

    // Simple atomic read of volatile counter.

The "simple" comment is because there is still an error in this code, but I did not want to make it too complex for you.

The bug in the "simple atomic read" is that interrupts are always enabled after counter_read() exits.

Even if the global interrupts were disabled before counter_read() was executed.

This can lead to very nasty and hard to find bugs.

For example if you later write a critical section:

cli()   // Critical section, with interrupts disabled.
counter_read();
do_something();
sei();

Then interrupts are still enabled while the function "do_something()" is executed.

There is a way to prevent this.

I consider it to be your homework to figure out how to prevent this.

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

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

kk6gm & Paulvdh:
Thanks for the nice explanation.

 

Yesterday night I fall into a trap with this atomic - nonatomic stuff with my program.

Actually, that messy think happened as discussed in post #62 - #65.

I measure the time in my ISR with a signal_counter++ then I reset the same counter in my main program to 0.

And bingo! I got the failure on my output screen on my scope.

The signal of the output pin was wrong - corrupt timing accure because the ISR and the main code changed the

value in the variable at the same time.

 

That's crap! angry

 

I'm working on my little nuclear weapon trigger, but I'm afraid about my little Tiny13, he will let the smoke out.

He is ticking and counting even if I cut the power too... laugh

Last Edited: Sat. Oct 6, 2018 - 02:45 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

jodank wrote:

Actually, that messy think happened as discussed in post #62 - #65.

I measure the time in my ISR with a signal_counter++ then I reset the same counter in my main program to 0.

And bingo! I got the failure on my output screen on my scope.

The signal of the output pin was wrong - corrupt timing accure because the ISR and the main code changed the

value in the variable at the same time.

Hmmm, I'm curious about this.  Just setting an 8-bit variable in main code can never have an atomicity problem, because it's not a RMW operation.  Setting a 16-bit variable CAN have an atomicity problem, but the last code you posted only had the counter counting to 120, meaning the MSB of the counter always remained 0.  In essence, your 16-bit counter was only an 8-bit counter.  This makes me wonder about what changed in your new code that revealed an atomicity problem.

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

Yea, I set the new signal_counter variable as a uint16_t to hold a higher value than 255.

The signal_counter variable should hold in my new code a value of 1004. So my old definition was not good

enough to do the stuff, but at midnight I realized the moon is shining and not the sun, so darkness falls on my eyes,

when I realized I have a problem in my underground lab.  smiley

My nuclear weapon switch will make time travel...

 

When I set the signal_counter to uint8_t than everything was fine again, no more bugs and insects. angel

 

Can we pls go a bit back in time. Just for my better understanding about variables.

I'm now a bit confused with variables and integer types in C.

I know how to configure variables in C but I'm confused when to use which integer type in C.

When I say "in C" I mean only C for AVR uC programming areal.

 

I read the book from Kernighan & Ritchie the past months and done almost all examples from it on my PC.

That was great and fine, and it was pretty close in mind as I'm thinking when I coding in VB .NET.

There I don't care about bit shifting, is my CPU 8,16,32,64 etc. bit etc...

But I realized here in C it is very important to know the architecture of the targeted uC, and must take care how to 

write/optimize the code in a way it is running the most efficient, and using the memory space inside the uC as best as possible.

 

In VB .NET I don't care how many RAM will eat up my variable, but here it is very important.

 

So, here is my problem:
Variables when and how to use them in the world of AVR.

 

I know there are several types of variables, I won't write them all now.

 

The uint8_t is a variable definition to hold 8 bit or 1 byte long data.

The uint16_t is a variable definition to hold  16 bit or 2 byte long data.

The int is a  is a variable definition can represent a number from -32767 to  32767

 

That's ok for now.

 

Here is a part of my code, just for example:

 


// configuring the variables

volatile uint16_t signal_counter = 0;
volatile uint8_t bounce_counter = 0;

ISR(TIM0_COMPA_vect)					// TIMER0 ISR
{

	signal_counter++;				// counter for signal creation
	bounce_counter++;				// counter for debouncing the switches

}

All the counters I defined in the example are just a number from 0 to some number, depend on how far they have to count.

Why should I not just define all the variables lets say as an INT and don't care about is the counter data 8bit or fit it in 16bit variable configuration,

is the uC 8bit or 16bit architecture etc.

 

If I would the code write like this what would/can go wrong:

 

// configuring the variables

volatile int signal_counter = 0;
volatile int bounce_counter = 0;

ISR(TIM0_COMPA_vect)				// TIMER0 ISR
{

	signal_counter++;			// counter for signal creation
	bounce_counter++;			// counter for debouncing the switches

}

Let's say I won't care about the usage of memory what the variables would use in the memory of the uC.

 

The atomicity problem will also appear even if the let say configure the signal_counter variable as an int.

 

Here is how I see and interpret the use of uint8_t and int ( this two is just listed as an example ).

 

If I would let's say get some 8bit data from a communication port like a serial port.

I would collect them in a variable configured in an uint8_t maybe.

 

Or if I would make some bit changing, bit shifting inside a variable then I would also use the uint8_t.

 

But let's say I have to do this task in the main:

int a = 5;
int b = 5;
int c = 0;

int main(void)
{
    c = a + b;
}

I could also configure the a,b,c variables as uint8_t I assume ( I didn't try), but why?

 

 

I'm really confused with this for now.

 

Thanks for any help.

 

Last Edited: Sat. Oct 6, 2018 - 04:39 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

'int' is a generic C signed integer, at least 16 bits but can be larger.  It will always cause atomicity problems on an 8-bit device like the AVR if shared between ISRs and main code.

 

IMO 'int' is almost always the wrong type to use in programming AVRs, because (a) it is signed, and very often it is used to only hold unsigned values, (b) it is often larger than needed, and (c) since it is not a defined size, it can cause problems when moving code to or from other platforms that have different sizes of int (there was a thread on this here very recently).  In almost all cases, use

 

uint8_t

int8_t

uint16_t

int16_t

 

or even the 32-bit types if needed.  I generally typedef these to u8, i8 (or s8), u16 and i16 (or s16).

 

And I'll repeat my personal mantra here:

 

don't use float if 32-bit ints will do

don't use 32-bit ints if 16-bit ints will do

don't use 16-bit ints if 8-bit ints will do

don't use signed if unsigned will do

 

This is not just about efficiency, but also a way to help document the code.  If I see an 'int' being used in code, I have to wonder if it can ever go negative.  If it can't, that's just a point of confusion.  Same for using 16-bit values to hold a count from 0 to 10, etc.

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

kk6gm:
 

I will jump back for short time to your post #62 to that example of atomicity problem between the ISR and the main code.

 

Let's say I will mod the code in main to this:

 

volatile uint8_t counter = 0;

ISR(...)
{
    counter++;
}

main()
{
    while(1)
    {
       if ( counter >= 255 )
         {
            counter = 0;
         }
    }
}

I assume this can also end up in an atomicity problem.

 

But what if I do this way:

 

volatile uint8_t counter = 0;

ISR(...)
{
    counter++;

      if ( counter >= 255 )
            {
               counter = 0;
            }
}

main()
{
    while(1)
    {
      // do something but don't change the value of counter variable, just read and use.

    }
}

I assume this kind of code can not be affected by atomicity problem, or?

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

jodank wrote:

kk6gm:
 

I will jump back for short time to your post #62 to that example of atomicity problem between the ISR and the main code.

 

Let's say I will mod the code in main to this:

 

volatile uint8_t counter = 0;

ISR(...)
{
    counter++;
}

main()
{
    while(1)
    {
       if ( counter >= 255 )
         {
            counter = 0;
         }
    }
}

I assume this can also end up in an atomicity problem.

First, a uint8_t will never go above 255, so >= 255 is no different than == 255.

 

There is no atomicity problem with the code you posted.  The counter >= 255 just reads 1 byte, so no multiple byte access and no RMW, thus no atomicity problem.  Same with counter = 0, no multiple byte access and no RMW.  But you would definitely have atomicity problems if counter was a 16-bit or 32-bit variable, and you would have to explicitly force atomic accesses in that case.

 

Quote:

But what if I do this way:

 

volatile uint8_t counter = 0;

ISR(...)
{
    counter++;

      if ( counter >= 255 )
            {
               counter = 0;
            }
}

main()
{
    while(1)
    {
      // do something but don't change the value of counter variable, just read and use.

    }
}

I assume this kind of code can not be affected by atomicity problem, or?

Again, since counter is a single byte, there can be no atomicity problem.  However, if counter was 16 bits, this code could fail, even though main code just reads the value.  Here is a failing sequence with 16-bit counter:

 

counter = 0xFF

MC reads LSB of counter (0xFF)

--INTERRUPT

--ISR increments counter to 0x100

--INTERRUPT RETURNS

MC reads MSB of counter (0x01)

 

Now MC is working with a broken value of counter, 0x1FF, rather than either the valid value of 0x0FF (before the interrupt) or 0x100 (after the interrupt).

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

Ok, but let's say this code can end up in atomicity problem?

I assume they can fail.

volatile uint8_t counter = 0;

ISR(...)
{
    counter++;
}

main()
{
    while(1)
    {
       if ( counter >= 10)
         {
            counter = 0;
         }
    }
}

 

But if I move the if statement from the main into the ISR, then it should not have an atomicity problem,

because the variable counter would be changed two times in the same cycle when the ISR was triggered.

 

1. ISR triggered

2. counter++

3. counter = 0 

 

If the counter is configured as a uint8_t and it is increasing as in the code above, in the ISR, which can increase the number to 255.

when the ISR has triggered the 256 cycles I assume the counter variable will reset to 0 even if I didn't reset it before through a code.

will that end up in an error or bug or that is fine to leave the counter to reset back to 0 "by it self"?

 

Last Edited: Sat. Oct 6, 2018 - 10:53 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

There is no atomicity problem in the code fragment you posted.  There is no way that either the ISR nor the main code will ever see an invalid (corrupted) value for counter, when counter is only 8 bits.

 

There is also no danger in letting an 8-bit counter roll over in the ISR, if that's what you want to do.

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

Finally here is my new code:

 

#define F_CPU 9600000UL		// Define CPU frequency in Hz.
#include <avr/io.h>
#include <avr/interrupt.h>

// configuring the variables
volatile uint8_t tick_ON_ms;
volatile uint8_t tick_OFF_ms;
volatile uint8_t signal_counter = 0;
volatile uint8_t bounce_counter = 0;
volatile uint8_t swp3 = 0;
volatile uint8_t swp4 = 0;
volatile uint8_t Signal_off_ms = 100;
volatile uint8_t signal_on_ms = 3;

static inline void Timer_On(void)
{
	TCCR0A |= (1 << WGM01);			// clear timer on compare match
	TCCR0B |= (1 << CS00) | (1 << CS01);	// clock source and prescaler 64 setup ( Timer/Counter Clock Sources data sheet pg.58)
	OCR0A = 149;				// trigger interrupt every ([1 / (9.6E6 / 64)] * (149+1) = 1ms)
	TIMSK0 |= ( 1 << OCIE0A );		// enable output compare match interrupt - for Attiny13
	sei();					// enable interrupts
}

ISR(TIM0_COMPA_vect)				// TIMER0 ISR
{

	signal_counter++;			// counter for signal creation
	bounce_counter++;			// counter for debouncing the switches

}

void Slow_down_key(void)
{

	if (PINB &(1 << PB3))			// check if PB3 is pressed
	{
		swp3 = 1;			// if PB3 is pressed remember it, set Switch_Was_Pressed = 1
		bounce_counter = 0;		// restart bounce_counter
	}

	if (bounce_counter >= 10)		// check if bounce_counter reached delay of 10ms to recheck the
	{					// state of PB3 ( check if PB3 is released )
		if (swp3)			// if Switch_Was_Pressed3 = true
		{
			if (!(PINB &( 1 << PB3 )))		// if PB3 is release after bounce_counter time
			{
				PORTB |= ( 1 << PB2 );		// set PB2 high
				swp3 = 0;			// reset the "Switch_Was_Pressed3" (swp3) variable

				Signal_off_ms +=10;		// the time of LOW will be every time longer by 10ms
				if (Signal_off_ms >= 240)	// Signal_off_ms should never go over 240ms
				{
					Signal_off_ms = 240;	// set the LOW to 240ms
				}
			}
		}
	}

}

void Speed_up_key(void)
{

if (PINB &(1 << PB4))			// check if PB4 is pressed

{
	swp4 = 1;			// if PB4 is pressed remember it, set Switch_Was_Pressed = 1
	bounce_counter = 0;		// restart bounce_counter
}

if (bounce_counter >= 10)		// check if bounce_counter reached delay of 10ms to recheck the
{					// state of PB3 ( check if PB4 is released )
	if (swp4)			// if Switch_Was_Pressed4 = true
	{
		if (!(PINB &( 1 << PB4 )))	   // if PB4 is release after bounce_counter time
		{
			PORTB |= ( 1 << PB1 );	   // set PB1 high
			swp4 = 0;		   // reset "Switch_Was_Pressed4" (swp4) variable
			Signal_off_ms -= 10;
			if (Signal_off_ms <= 10)   // Signal_off_ms should never fall under 10ms
			{
				Signal_off_ms = 10;
			}

		}
	}
}
}

int main(void)
{

	DDRB |= (1 << PB0) | ( 1 << PB1 ) | ( 1 << PB2 ) ;	// set PB0, PB1 and PB2 on PORTB as Output
	PORTB &=~ ( 1 << PB0 ) & ( 1 << PB1 ) & ( 1 << PB2 );	// clear PB0, PB1 and PB2
	Timer_On();	

while(1)
		{

//========== This part is the signal LO/HIGH width factory =================

			if (signal_counter >= Signal_off_ms)			// creates the signal-off time
			{
				tick_OFF_ms=1;					// signal-off width reached
			}

			if (signal_counter >= Signal_off_ms+signal_on_ms)	// creates the signal-on time
			{
				tick_ON_ms=1;					// signal-on width reached
				signal_counter = 0;
			}

//============================================================================

//============ Toggle the signal based on the width ==========================

			if (tick_OFF_ms)
				{
					PORTB |=( 1 << PB0 );			// max LOW time reached, go HIGH, set PB0 HIGH
				}

			if (tick_ON_ms)
				{
					PORTB &=~( 1 << PB0 );			// max HIGH time reached, go LOW, set PB0 LOW
					tick_ON_ms = 0;
					tick_OFF_ms = 0;
				}	

//===========================================================================	

Slow_down_key();		// increase the time of LOW by pressing the switch PB3
Speed_up_key();			// decrease the time of LOW by pressing the switch PB4

   }

}

 

My idea for the debouncing was:
When the switch is pressed the program wait for 10ms and checks if the switch is released.

If there is a low on the switch after 10ms I assume the switch is released and no bouncing anymore. ( maybe here I should let more time for debouncing, 30ms? ).

 

I'm a bit afraid about the atomicity problem in my code because I reset the signal_counter and debounce_counter in the main and the ISR.

But I configured that two variables as an uint8_t so it should not happen.

 

I tried to make it simple as I can/know.

I also breadboarded the whole stuff to have it out of the development board, and it is working like a charm.

I'm very happy now. laugh

 

The task was:
To toggle an output pin where the pulse_low width can be changed with the switch up and down (PB3 & PB4)

but the puls_high width will be the same width all the time, no matter what the pulse_low width is.

 

I assume it is not so bad for somebody who wrote his first program for an AVR in C.

What do you think about?

 

Thank's

Last Edited: Sun. Oct 7, 2018 - 01:08 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I have a question about some info on my output tab in Atmelstudio 7 when I build my file to write to the uC.

On the output tab is this text also to see:

...
Task "RunOutputFileVerifyTask"
Program Memory Usage     :    362 bytes   35.4 % Full
Data Memory Usage         :    8 bytes   12.5 % Full

...

 

What is "program memory" and what is "data memory" ?

 

I assume the program memory is the size of my program I wrote.

If yes, then here I can track down how big is my program i wrote and does it fit into my uC?

 

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

jodank wrote:

...
Task "RunOutputFileVerifyTask"
Program Memory Usage     :    362 bytes   35.4 % Full
Data Memory Usage         :    8 bytes   12.5 % Full

...

 

What is "program memory" and what is "data memory" ?

Program memory is all the data (program instructions and program data) stored in flash.  Data memory is all the SRAM memory directly used by the program (but not including memory used by the stack and other "hidden" uses).

 

Note that when you write something like

int x = 99;
char string[] = "Ninety Nine";

All that data (the integer and the string) will be stored in program memory, and when the program runs, it will be copied to data memory.  So it is living in two places in memory.  It is possible to store and access constant data in flash alone, thus saving that much data memory (at the cost of a bit more complexity and a bit slower access).  

https://www.nongnu.org/avr-libc/...