General Coding question- Setting int values

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

So I am having a little problem with a interrupt based timer I am trying to implement. My interrupt vector is:

ISR(TIMER0_COMPA_vect)
{
	milsec++;
	if(milsec == 1000)
	{
		buzz_stim++;
		milsec = 0;
	}
}

with the code block where this is used:

void buzzer_short()
{ 
        millis_timer(1);
	buzz_interval = buzz_stim;
        sbi(PORTB,7);
	interval_diff = buzz_interval - buzz_stim;
	if (interval_diff > 2)
	{
		cbi(PORTB,7);
	}
}

buzzer_short() is called out through out my program and I am trying to used it for a piezo buzzer, so when it's time for it to be called the buzzer sounds for a second or two and the turns off. I know the issue with my code is that buzz_interval is basically always equal to buzz_stim so the interval_diff is always zero. Being that I am still honing my coding skills, I'm just not sure how to write where every time this block is called, buzz_interval resets and doesn't reset until it's called next. Thanks!

 

P.S. This is based off of a millisecond timer 

void millis_timer(uint8_t millis)
{
    TCCR0A |=  (1<<WGM01);
    TCCR0B |= (1<<CS02)|(1<<CS00);
    OCR0A = millis*7.815 - 1;
    TIMSK0 |= (1<<OCIE0A);
    sei();
}

 

This topic has a solution.

Cody W Phipps

Last Edited: Sun. Oct 11, 2020 - 01:48 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

VOLATILE

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

So instead of:

int buzz_interval;

It should be:

volatile int buzz_interval;

in my global variables?

Cody W Phipps

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

clawson wrote:
VOLATILE

and he wrote a whole Tutorial about it: https://www.avrfreaks.net/forum/tutcoptimization-and-importance-volatile-gcc

Top Tips:

  1. How to properly post source code - see: https://www.avrfreaks.net/comment... - also how to properly include images/pictures
  2. "Garbage" characters on a serial terminal are (almost?) invariably due to wrong baud rate - see: https://learn.sparkfun.com/tutorials/serial-communication
  3. Wrong baud rate is usually due to not running at the speed you thought; check by blinking a LED to see if you get the speed you expected
  4. Difference between a crystal, and a crystal oscillatorhttps://www.avrfreaks.net/comment...
  5. When your question is resolved, mark the solution: https://www.avrfreaks.net/comment...
  6. Beginner's "Getting Started" tips: https://www.avrfreaks.net/comment...
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

At the very least

 

volatile uint16_t   milsec;
volatile uint16_t   buzz_stim;

Jim

 

Until Black Lives Matter, we do not have "All Lives Matter"!

 

 

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

rather than re-invent the wheel, here's how Arduino does it:

 

https://github.com/arduino/Ardui...

 

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

OUCH!  Don't use floating point here! (*7.815).  Small/medium micros do much better with integers.

 

The 'volatile' issue has been mentioned.  Any variable shared between ISR and main code must be 'volatile'.

 

But I think this is what you are asking about.  You keep reloading buzz_interval with buzz_stim, then checking if they are different.  They will not be (at most, an interrupt may come along and increment buzz_stim afterwards, giving a difference of 1, but never >2).

 

You need something like this:

 

void buzzer_short()
{ 
    millis_timer(1);
    buzz_interval = buzz_stim;
    sbi(PORTB,7);
    do
    {
        interval_diff = buzz_stim - buzz_interval;
    }
    while (interval_diff <= 2);
    cbi(PORTB,7);
}

Also, if buzz_stim is more than 8 bits (on an 8-bit machine), you have to make all main code accesses to buzz_stim atomic.

This reply has been marked as the solution. 
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The OP code won't suffer from lack of volatile modifier because it contains a bug:

 

volatile uint8_t buzz_stim; // DON'T FORGET THIS

void buzzer_short()
{
    millis_timer(1);
    buzz_interval = buzz_stim;
    sbi(PORTB, 7);

    /* There should be a loop here to continually check interval_diff */
    interval_diff = buzz_interval - buzz_stim;
    if (interval_diff > 2)
    {
        cbi(PORTB, 7);
    }

    /* This whole code is far from optimal but this addition will suffice */
    do  {
        /* I reversed the subtraction here - you had another bug */
        interval_diff = buzz_stim - buzz_interval;
    } while (interval_diff < 2);

    cbi(PORTB, 7);
}

 

Last Edited: Tue. Oct 6, 2020 - 08:00 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thanks N.Winterbottom and kk6gm for the do while suggestion and the rest for the volatile tip. One more thing to add to thing for it to work for me, the sbi(PORTB,7) needs to be pulled into the do loop:

 

void buzzer_short()
{ 
	millis_timer(1);
	buzz_interval = buzz_stim;
	do 
	{
		sbi(PORTB,7);
		interval_diff = buzz_stim - buzz_interval;
	} 
	while (interval_diff < 1);
	cbi(PORTB,7);
}

 

 

Kartman wrote:

rather than re-invent the wheel, here's how Arduino does it:

 

https://github.com/arduino/Ardui...

 

This looks like something you would put into Arduino sketch. 

Cody W Phipps

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

phippstech wrote:
One more thing to add to thing for it to work for me, the sbi(PORTB,7) needs to be pulled into the do loop

That doesn't make sense, and suggests that something else is not right.

 

Also, I see that you are now testing for < 1 (so == 0).  That test can appear to fail because buzz_stim could be incremented in the ISR right after it is loaded into buzz_interval.  This is true no matter what interval_diff you are looking for (looking for an interval of X will always result in an interval between X-1+tiny_delta and X), but the "wrong answer" will be most obvious when comparing to 0.

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

Thanks for that. I marked the correct solution 

Cody W Phipps

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

phippstech wrote:
This looks like something you would put into Arduino sketch. 

 

Err no. It what implements the Arduino millis() function - which is what you seem to be doing. Note the atomic access of the tick counter. If your tick counter is anything larger than a uint8_t, then you need to ensure this.