Interrupt Based Seven Segment

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

So I have common anode, multiplexed seven segment display that I'm trying to make count down when a button is pushed. I had this working (except for the "flashing" issue which is mentioned below) that display part of the code was this:

    int  time_Set,value_time;
	time_Set = new_time*100;
	value_time = time_Set / 1000;
	//ten-thousandths digit
	value_time = time_Set / 1000;
	time_Set = time_Set % 1000;
	if (value_time < 1) //If 1st digit is zero, don't display anything
	{	
		sbi(PORTC, 0);
		sbi(PORTC, 1);
		sbi(PORTC, 6);
		sbi(PORTC, 7);
		PORTD = seg_code[value_time];
	}
		else
		{	
			cbi(PORTC, 0);
	    	sbi(PORTC, 1);
			sbi(PORTC, 6);
			sbi(PORTC, 7);
			PORTD = seg_code[value_time];
		}
		_delay_ms(1);
		
    //thousandths digit
	value_time = time_Set / 100;
	time_Set = time_Set % 100;
	sbi(PORTC, 0);
        cbi(PORTC, 1);
	sbi(PORTC, 6);
	sbi(PORTC, 7);
	PORTD = seg_code[value_time];
    _delay_ms(1);

    //hundredths digit
	value_time = time_Set / 10;
	sbi(PORTC, 0);
	sbi(PORTC, 1);
	cbi(PORTC, 6);
	sbi(PORTC, 7);
	PORTD = seg_code[value_time];
	_delay_ms(1);
	
    //Tenths digit
	value_time = time_Set % 10;
	sbi(PORTC, 0);
	sbi(PORTC, 1);
	sbi(PORTC, 6);
	cbi(PORTC, 7);
	PORTD = seg_code[value_time];
	_delay_ms(1);

Those 1 ms delays are additional timing issues within the code so I wanted to trim the number of delay functions in my code. So I'm attempting to make everything interrupt based. 

#define F_CPU 8000000UL
#include <avr/io.h>
#include <util/delay.h>
#include <avr/interrupt.h>
#include <stdlib.h>
//Segment Code 
#define ZERO     0b11000000
#define ONE      0b11111001
#define TWO      0b10100100
#define THREE    0b10110000
#define FOUR     0b10011001
#define FIVE     0b10010010
#define SIX      0b10000010
#define SEVEN    0b11111000
#define EIGHT    0b10000000
#define NINE     0b10011000
//Define the Inputs 
#define Timer_Button !(PINA & (1<<PINA1))
#define sbi(a, b) (a) |= (1 << (b))  // Set bit high
#define cbi(a, b) (a) &= ~(1 << (b)) // Set bit low

// Globals	
int  timeselect, retimeselect,buzz_interval,interval_diff,new_time;
char seg_code[]={ZERO, ONE, TWO, THREE, FOUR, FIVE, SIX, SEVEN, EIGHT, NINE};
volatile uint8_t buzz_stim;
volatile uint8_t milsec;
volatile uint8_t dmilsec;
volatile uint8_t hunsec;
volatile uint8_t tensec;

void Timer_init(void)
{
	PCMSK0 |= (1<<PCINT1);
	PCICR  |= (1<<PCIE0);
	sei();
}
void millis_timer(uint8_t millis)
{
	TCCR0A |=  (1<<WGM01);
	TCCR0B |= (1<<CS02)|(1<<CS00);
	//=(F_CPU*2/frequency*2*N)-1
	OCR0A = millis*8 - 1;
	TIMSK0 |= (1<<OCIE0A);
	sei();
}
ISR(TIMER0_COMPA_vect)
{
	milsec++;
	dmilsec++;
	if(dmilsec > 3)
	{
		dmilsec = 0;
	}
	if(milsec == 10)
	{
		tensec++;
		milsec = 0;
	}

	if(tensec == 10)
	{
		hunsec++;
		buzz_stim++;
		tensec =0;
	}
	if(hunsec == 10)
	{
		hunsec = 0;
	}
}
void buzzer_short()
{ 
	millis_timer(1);
	sbi(PORTB,7);
	buzz_interval = buzz_stim;

	do 
	{
		interval_diff = buzz_stim - buzz_interval;
	} 
	while (interval_diff < 2);
	cbi(PORTB,7);
}
unsigned char timer_button_state()
{
	if (Timer_Button)
	{
		_delay_ms(10);
		if (Timer_Button) 
        buzzer_short();
		return 1;
	}
	return 0;
}
ISR(PCINT0_vect)
{
	new_time = timeselect;
	if (timeselect <= 1)
		{
			timeselect = retimeselect;
		}
	if (timer_button_state())
	{	
		new_time--;
		timeselect = new_time;
	}
}
void set_timer()
{	
	millis_timer(1);
	Timer_init();
    int  time_Set,value_time;
	time_Set = new_time*100;
	value_time = time_Set / 1000;

	switch (dmilsec)
	{	
		case 0://ten-thousandths digit
			value_time = time_Set / 1000;
			time_Set = time_Set % 1000;
			if (value_time < 1) //If 1st digit is zero, don't display anything
			{	
				sbi(PORTC, 0);
				sbi(PORTC, 1);
				sbi(PORTC, 6);
				sbi(PORTC, 7);
				PORTD = seg_code[value_time];
			}
			else
			{	
				cbi(PORTC, 0);
				sbi(PORTC, 1);
				sbi(PORTC, 6);
				sbi(PORTC, 7);
				PORTD = seg_code[value_time];
			}
			break;

		case 1://thousandths digit
			value_time = time_Set / 100;
			time_Set = time_Set % 100;
			sbi(PORTC, 0);
			cbi(PORTC, 1);
			sbi(PORTC, 6);
			sbi(PORTC, 7);
			PORTD = seg_code[value_time];
			break;

		case 2://hundredths digit
			value_time = time_Set / 10;
			sbi(PORTC, 0);
			sbi(PORTC, 1);
			cbi(PORTC, 6);
			sbi(PORTC, 7);
			PORTD = seg_code[value_time];
			break;

		case 3://Tenths digit
			value_time = time_Set % 10;
			sbi(PORTC, 0);
			sbi(PORTC, 1);
			sbi(PORTC, 6);
			cbi(PORTC, 7);
			PORTD = seg_code[value_time];
			break;
	}
}
int main()
{
	DDRA = 0x00; // All of port A are inputs
	DDRB = 0xff; // Declaring all bits of Port B as outputs
	DDRC = 0xff; // Declaring all bits of Port C as outputs
	DDRD = 0xff; // Declaring all bits of port D as outputs
	timeselect = 10; 
	retimeselect =11;

	while(1)
	{	
	      set_timer();
	}
}

The following is happening:

  1. If the displayed number is supposed to be 1000, both the thousandths and hundredths digit are completely on (displaying 8.)  If that number is supposed to be 9 and below, the hundredths digit is completly illuminated.
  2. The digits seem to "flash" when button is pushing, meaning all digits turn off at once for less than a second when button is pushed

 

This topic has a solution.

Cody W Phipps

Last Edited: Thu. Oct 22, 2020 - 07:40 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I might hazard a guess that the interrupt priority of PCINT is greater than that of TIMER_COMP - so if you sit in PCINT with a button held down, TIMER_COMP will never fire - nor will your display.  All those modulo and division instructions will take rather a long time - consider a separate variable for each digit.  Furthermore, your PCINT_vect variables are not volatile.  Hope that helps.  S.

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

I'll add a couple of things.  Check your button inside the timer interrupt (after you set your segments and digits, not before).  It's free and it will avoid the timer ISR getting delayed (you've even got a _delay_ms and a buzzer delay in the button interrupt - BAD!).

 

Second, only turn on your digit after you have set the segments.  The sequence is LAST_DIGIT_OFF...SET_CURRENT_SEGMENTS...CURRENT_DIGIT_ON.  You'd be surprised at how much ghosting you can get in low light if you turn on a new digit with the old segments on.

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

You keep on calling millis_timer(1). Just set timer 0 up in CTC mode for 1ms. Set and forget.

In the isr, output one digit at a time - compute the value and seven seg lookup in the main line code.

In the isr, have a down count for your buzzer. If the down count is not zero, turn the buzzer on, if it is zero, turn the buzzer off. Your main line code just loads the down counter with the on time when you want to buzz.

In the isr, read the switch input and debounce.

 

Your code will then be much simpler and work better. 

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

kk6gm wrote:

I'll add a couple of things.  Check your button inside the timer interrupt (after you set your segments and digits, not before).  It's free and it will avoid the timer ISR getting delayed (you've even got a _delay_ms and a buzzer delay in the button interrupt - BAD!).

 

 

So put the block of code inside of the compare vector that is ticking my ms timer instead of the PCINT0 vector?

Cody W Phipps

Last Edited: Mon. Oct 12, 2020 - 08:33 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Another thing or two: 

 

Why are all your '_init' routines inside set_timer()?  Do you really need to re-initialize them every trip through your while{} loop?  For that matter, you should put sei() after ALL the intialization is done (again in main()).  What happens if you get an interrupt while initializing?

 

What counter does _delay_ms() use?   S.

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

phippstech wrote:
So put the block of code inside of the compare vector that is ticking my ms timer instead of the PCINT0 vector?

Eliminate the PCINT0 interrupt and ISR entirely.  Do any time-critical work inside the timer ISR.  Do less time-critical work in your main loop.  As a rule of thumb, do the least amount of work possible in the ISR, and do everything else outside of the ISR.  Communicate between ISR and main loop with single byte flags or variables.

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

kk6gm wrote:

Eliminate the PCINT0 interrupt and ISR entirely.  

 

So I cut and pasted that block of code into three different places and now the buzzer constantly sounds once the timer button is pushed in all three spots I put it in. The buzzer goes off in the third position if the button is held down. 

ISR(TIMER0_COMPA_vect)
{
	new_time = timeselect;
	if (timeselect <= 1)
	{
		timeselect = retimeselect;
	}
	if (timer_button_state())
	{
		new_time--;
		timeselect = new_time;
	}
	milsec++;
	dmilsec++;
	if(dmilsec > 3)
	{
		dmilsec = 0;
	}
	if(milsec == 10)
	{
		tensec++;
		milsec = 0;
	}

	if(tensec == 10)
	{
		hunsec++;
		buzz_stim++;
		tensec =0;
	}
	if(hunsec == 10)
	{
		hunsec = 0;
	}
}
int main()
{
	DDRA = 0x00; // All of port A are inputs
	DDRB = 0xff; // Declaring all bits of Port B as outputs
	DDRC = 0xff; // Declaring all bits of Port C as outputs
	DDRD = 0xff; // Declaring all bits of port D as outputs
	timeselect = 10; 
	retimeselect =11;

	while(1)
	{	
	new_time = timeselect;
	if (timeselect <= 1)
	{
		timeselect = retimeselect;
	}
	if (timer_button_state())
	{
		new_time--;
		timeselect = new_time;
	}
		set_timer();
	}
}
int main()
{
	DDRA = 0x00; // All of port A are inputs
	DDRB = 0xff; // Declaring all bits of Port B as outputs
	DDRC = 0xff; // Declaring all bits of Port C as outputs
	DDRD = 0xff; // Declaring all bits of port D as outputs
	timeselect = 10; 
	retimeselect =11;
	new_time = timeselect;
	if (timeselect <= 1)
	{
		timeselect = retimeselect;
	}
	if (timer_button_state())
	{
		new_time--;
		timeselect = new_time;
	}
	while(1)
	{	

		set_timer();
	}
}

Thank you for your patience on this, I am more of a design engineer who is trying to get better at coding. I have dedicated time during the week for general learning. Hoping between that and you guys, I'm striving to be proficient enough to contribute like you are doing. 

Cody W Phipps

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

phippstech wrote:
Thank you for your patience on this, I am more of a design engineer who is trying to get better at coding. I have dedicated time during the week for general learning. Hoping between that and you guys, I'm striving to be proficient enough to contribute like you are doing. 

Fair enough, we were all there once. :)

 

Let's start with the basics.  How often does your timer interrupt run?  You generally want to refresh each multiplexed digit at least 100 times per second, so if you have 4 digits that is 400 times per second, or a 2.5 ms timer interrupt, but I don't know how many digits you have.

 

Now, if you want your buzzer to buzz for e.g. 1/2 second, that is 200 timer interrupts (assuming 4 digits).  You will start your buzzer, then count 200 timer interrupts and on the 200th interrupt, turn the buzzer off.

 

So, tell us your timing requirements and we can proceed from there.

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

1/2 second would be fine with me and that refresh sounds like it will work as well. I will a be using four digits when the time value is ten (it can also be fifteen). This is only about a fourth of the code (which I have the other parts working and I think the lessons learned from here will improve the other parts dramatically as well), for that time value will eventually countdown by seconds ( time value I select is in minutes).

Cody W Phipps

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1
ISR()
{
    do_critical_work();
    do_less_critical_work();
}

For some reason, the text I put before this code block disappeared, and I cannot insert text before the block now.  So I will put the text here.

 

You can use the same 2.5ms timer interrupt for most or all of your timing needs, including reading and debouncing buttons, counting seconds, etc.

 

You also have to decide how much work to do inside the ISR vs. outside the ISR.  The two code blocks (above and below) show the basic idea.

The above code makes your ISR longer, and can make other interrupts less responsive.  The alternative is:

volatile unsigned char timer_tick = FALSE;

ISR()
{
    do_critical_work();
    timer_tick = TRUE;
}

// in main code

while (1)
{
    if (tier_tick)
    {
        do_less_critical_work();
        timer_tick = FASWE;
    }
}

Most of the time you will want to use the 2nd method, but it's always a judgement call.

Last Edited: Wed. Oct 14, 2020 - 07:38 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

kk6gm wrote:

 

You can use the same 2.5ms timer interrupt for most or all of your timing needs, including reading and debouncing buttons, counting seconds, etc.

 

 

 

To me, the second method would be probably the best. So if I'm reading this correctly, I should do away with dmilsec variable ( the msec timer for the display), the milsec, tensec, and hudsec variables inside of my TIMER0_COMPA vector? I guess millis_timer(1); will turn into millis_timer(2.5); as well.

Cody W Phipps

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

Your timer ISR could be as simple as this

 

ISR()
{
    turn_off_all_digits();
    set_new_segments();// calculated in main loop or below in ISR
    turn_on_new_digit();
    timer_tick = TRUE;// signal main code that a tick has happened
}

(all the function calls above would actually be simple port writes)

 

This is taking the principle of a short, fast ISR to its maximum.  Only the direct timing-critical code (updating the display) is executed in the ISR.

 

As long as your main code cannot possibly miss a timer_tick notification (by running too long without testing timer_tick), this approach is workable.  But it is also possible to add more work inside the ISR, and if you do not have other time-critical interrupts in your system, there is no drawback to this approach.  Thus you see the basic tradeoff - doing so much work in main code that you might miss a timer_tick notification on the one hand, and doing so much work inside the ISR so that you cause excessive interrupt response delays on the other hand.  It is a juggling act that you will need to solve for each application.

Last Edited: Wed. Oct 14, 2020 - 10:16 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

Then you could work towards something like this:

 

https://www.avrfreaks.net/forum/...

 

 

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

Alright, I think I'm beginning to get somewhere. Seems like I got the digits displaying but I can't seem to get "timeselect" to decrement. It is staying at ten. I'll go after that _delay function in timer_button_state next. 

// Globals	
int  timeselect,digit,retimeselect,buzz_interval,interval_diff,new_time,thou_digit,mod_digit_tt, ten_thou_digit;
char seg_code[]={ZERO, ONE, TWO, THREE, FOUR, FIVE, SIX, SEVEN, EIGHT, NINE};
volatile uint8_t buzz_stim;
volatile unsigned char timer_tick = false;

void millis_timer(uint8_t millis)
{
	TCCR0A |=  (1<<WGM01); //Enable CTC
	TCCR0B |= (1<<CS02)|(1<<CS00); //1024 prescaler
	//=(F_CPU*2/frequency*2*N)-1
	OCR0A = millis*8 - 1;
	TIMSK0 |= (1<<OCIE0A);
	sei();
}
unsigned char timer_button_state()
{
	if (Timer_Button)
	{
		_delay_ms(10);
		if (Timer_Button) 
      //  buzzer_short();
		return 1;
	}
	return 0;
}
void Timer_init(void)
{
	PCMSK0 |= (1<<PCINT1);
	PCICR  |= (1<<PCIE0);
	sei();
}
void turn_off_all_digits()
{
	sbi(PORTC, 0);
	sbi(PORTC, 1);
	sbi(PORTC, 6);
	sbi(PORTC, 7);
}
void set_timer()
{	
    int  time_Set,value_time;
	time_Set = timeselect*100;
	ten_thou_digit = time_Set / 1000;
	mod_digit_tt = ten_thou_digit % 1000;
	thou_digit = mod_digit_tt / 100;
	digit++;
	if (digit > 4)
		{
			digit = 0;
		}
}
void turn_on_new_digit()
{
	if (digit == 1)
	{
		//ten-thousandths digit
		if (ten_thou_digit < 1) //If 1st digit is zero, don't display anything
			{	
				sbi(PORTC, 0);
				sbi(PORTC, 1);
				sbi(PORTC, 6);
				sbi(PORTC, 7);
				PORTD = seg_code[ten_thou_digit];
			}
			else
				{	
					cbi(PORTC, 0);
					sbi(PORTC, 1);
					sbi(PORTC, 6);
					sbi(PORTC, 7);
					PORTD = seg_code[ten_thou_digit];
				}
	}
	if (digit == 2)
	{
		//thousandths digit
		sbi(PORTC, 0);
		cbi(PORTC, 1);
		sbi(PORTC, 6);
		sbi(PORTC, 7);
		PORTD = seg_code[thou_digit];
	}
	if (digit == 3)
	{
		//hundredths digit
		sbi(PORTC, 0);
		sbi(PORTC, 1);
		cbi(PORTC, 6);
		sbi(PORTC, 7);
		PORTD = seg_code[0];	
	}

	if (digit == 4)
	{
		//Tenths digit
		sbi(PORTC, 0);
		sbi(PORTC, 1);
		sbi(PORTC, 6);
		cbi(PORTC, 7);
		PORTD = seg_code[0];
	}
}
ISR(TIMER0_COMPA_vect)
{
	turn_off_all_digits();
	set_timer();
	turn_on_new_digit();
	timer_tick = true;
}

int main()
{
	DDRA = 0x00; // All of port A are inputs
	DDRB = 0xff; // Declaring all bits of Port B as outputs
	DDRC = 0xff; // Declaring all bits of Port C as outputs
	DDRD = 0xff; // Declaring all bits of port D as outputs
	timeselect = 10; 
	retimeselect = 10;
	new_time = timeselect;
	while(1)
	{	
		millis_timer(2.5);
		Timer_init();
		if (timer_button_state())
		{
			new_time--;
			timeselect = new_time;
		}
		if (timeselect <= 1)
		{
			timeselect = retimeselect;
		}
		if (timer_tick)
			{
				timer_tick = false;
			}
	}
}

 

Cody W Phipps

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

init_timer() turns on the pcints - but there's no ISR for it. That is not going to have a good outcome.

millis_timer(2.5);

The millis_timer() function accepts an integer, but you are providing a floating point value. It will be either 2 or 3. Make your choice.

 

As has been mentioned before, set your timer in CTC mode to give a consistent timebase. Your current method is making it difficult as you are making the problem way more complex that it needs to be.

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

Get rid of _delay_ms() altogether.  Count timer ticks for any delays.  _delay_ms() just wastes clock cycles, cycles that could be used to perform useful work.

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

instead of  PORTC, 6  don't use 6, give it a name so it's more clear than "6"

 

 You keep repeating the same things in the listing

 

A way that might be used any time,  note how much shorter this is. 

myport= PORTC | (1 <<0 ) | (1 << 1) | (0 << 6) |  (0 << 7);  Get current port bits and prep final result for all digits off

if digit==1 {myport &= ~(1<< 0)    ...any other digit 1 stuff };  \\make bit low, will turn on digit 1

if digit==2 {myport &= ~(1<< 1)    ...any other digit 2 stuff };  \\make bit low, will turn on digit 2

if digit==3 {myport &= ~(1<< 6)    ...any other digit 3 stuff };  \\make bit low, will turn on digit 3

if digit==4 {myport &= ~(1<< 7)    ...any other digit 4 stuff};   \\make bit low, will turn on digit 4

..

PORTC=myport   ;  /sets PORTC digits as needed

 

 

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

Last Edited: Sat. Oct 17, 2020 - 05:31 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

kk6gm wrote:

  Count timer ticks for any delays.  _delay_ms() just wastes clock cycles, cycles that could be used to perform useful work.

 

How would you write that. I kept trying things and the buzzer works sometimes but other times it just stays on. Also, the timer value is not decrementing. 

Cody W Phipps

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

phippstech wrote:

kk6gm wrote:

  Count timer ticks for any delays.  _delay_ms() just wastes clock cycles, cycles that could be used to perform useful work.

 

How would you write that. I kept trying things and the buzzer works sometimes but other times it just stays on. Also, the timer value is not decrementing. 

There are a few different ways to do this.  Here is one example to help explain the process.

 

volatile uint8_t timer_tick = FALSE;

ISR()
{
    // do any ISR work
    timer_tick = TRUE;      // tell main code the timer has ticked
}

// this is in the main code loop

if (timer_tick)             // run this code every timer tick
{
    timer_tick = FALSE;     // don't leave flag set
    // do any timer tick work
    manage_buzzer();
}

// buzzer management

uint8_t buzzer_count = 0;   // or uint16_t if needed

void manage_buzzer(void)
{
    if (buzzer_count != 0)
    {
        buzzer_count--;
        PORT |= BUZZER_BIT; // turn buzzer on
    }
    else
        PORT &= ~BUZZER_BIT; // turn buzzer off
    }
}

// in main code, how to turn on buzzer for 200 ticks
    buzzer_count = 200;

An alternate approach to managing the buzzer:

 

uint8_t buzzer_count = 0;           // or uint16_t if needed

void manage_buzzer(void)
{
    if (buzzer_count != 0)
    {
        if (--buzzer_count == 0)    // end of buzzer period
            PORT &= ~BUZZER_BIT;     // turn buzzer off
    }
}

// in main code, how to turn on buzzer for 200 ticks
    buzzer_count = 200;
    PORT != BUZZER_BIT;             // turn buzzer on here

 

 

Last Edited: Tue. Oct 20, 2020 - 05:13 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thank you kk6gm for your help. You definitely made some light bulbs go off and I was able to correct the rest of the mistakes, namely the decrementing issue with your last reply. Also thank you everyone else for you input and tips. I have also incorporated those. Below is what I have now and if I there isn't any more objections, I will mark it as the solution. I'm sure I will be back soon. Till next time.

 

#define F_CPU 8000000UL
#include <avr/io.h>
#include <util/delay.h>
#include <avr/interrupt.h>
#include <stdlib.h>
//Segment Code 
#define ZERO     0b11000000
#define ONE      0b11111001
#define TWO      0b10100100
#define THREE    0b10110000
#define FOUR     0b10011001
#define FIVE     0b10010010
#define SIX      0b10000010
#define SEVEN    0b11111000
#define EIGHT    0b10000000
#define NINE     0b10011000

//Define the Inputs 
#define Timer_Button !(PINA & (1<<PINA1))
#define sbi(a, b) (a) |= (1 << (b))  // Set bit high
#define cbi(a, b) (a) &= ~(1 << (b)) // Set bit low

// Globals	
int  timeselect,digit,retimeselect,buzz_interval,interval_diff,new_time,thou_digit,mod_digit_tenthou, ten_thou_digit, mod_digit_thou, hund_digit, ten_digit, sec;
uint8_t previousReading  = 1; // holds last 8 pin reads from pushbutton
uint8_t buttonWasPressed = 1; // keeps track of debounced button state 
uint8_t buzzer_count = 0;
uint8_t debounce_count = 0;
uint8_t digit_on = 0;
char seg_code[]={ZERO, ONE, TWO, THREE, FOUR, FIVE, SIX, SEVEN, EIGHT, NINE};
volatile unsigned char timer_tick = false;
unsigned char select_time = false;

void millis_timer(uint8_t millis)
{
	TCCR0A |=  (1<<WGM01); //Enable CTC
	TCCR0B |= (1<<CS02)|(1<<CS00); //1024 prescaler
	//=(F_CPU*2/frequency*2*N)-1
	OCR0A = millis*8 - 1;
	TIMSK0 |= (1<<OCIE0A);
	sei();
}
void buzzer(void)
{ 
	if (buzzer_count != 0)
		{
		    buzzer_count--;
			sbi(PORTB,7); // turn buzzer on
	    }
	    else
			{
	    		cbi(PORTB,7); // turn buzzer off
			}
}

void timer_button_state()
{
	if ((Timer_Button) != previousReading) 
		{  
			if(!buttonWasPressed) 
				{                                                             
					if(debounce_count < 15) 
							debounce_count++; 
							else 
								debounce_count = 0;
								buttonWasPressed = 1; 
//Below are the tasks to do if timer button is pushed
								buzzer_count = 75;
								if(select_time) //Select time mode 			
									{	
										new_time--;
										timeselect = new_time;
									}
//End of timer button tasks							
				}
					else  
						buttonWasPressed = 0; 	
			}
	previousReading = (Timer_Button);        
}
void turn_off_all_digits()
{
	digit_on = PORTC | (1<<0) | (1<<1) | (1<<6) | (1<<7);
	digit_on &= (1<<0) | (1<<1) | (1<<6) | (1<<7);
	PORTC = digit_on;
	
}
void display()
{	
    int time_Set;
	time_Set = timeselect*100;
	ten_thou_digit = time_Set / 1000;
	mod_digit_tenthou = time_Set % 1000;
	thou_digit = mod_digit_tenthou / 100;
	hund_digit = sec / 10;
	ten_digit = sec % 10;
	digit++;
	if (digit > 4)
		{
			digit = 0;
		}
}
void turn_on_new_digit()
{
	digit_on = PORTC | (1<<0) | (1<<1) | (1<<6) | (1<<7);
	if (digit == 1)
	{
		//ten-thousandths digit
		if (ten_thou_digit < 1) //If 1st digit is zero, don't display anything
			{	
				digit_on &= (1<<0) | (1<<1) | (1<<6) | (1<<7);
				PORTD = seg_code[ten_thou_digit];
			}
			else
				{	
					digit_on &= ~(1<<0) | (1<<1) | (1<<6) | (1<<7);
					PORTD = seg_code[ten_thou_digit];
				}
	}
	if (digit == 2)
	{
		//thousandths digit
		digit_on &=  (1<<0) | ~(1<<1) | (1<<6) | (1<<7);
		PORTD = seg_code[thou_digit];
	}
	if (digit == 3)
	{
		//hundredths digit
		digit_on &=  (1<<0) | (1<<1) | ~(1<<6) | (1<<7);
		if (timeselect)
		{
			PORTD = seg_code[0];
		}
			else
			{
				PORTD = seg_code[hund_digit];	
			}
	}
	if (digit == 4)
	{
		//Tenths digit
		digit_on &=  (1<<0) | (1<<1) | (1<<6) | ~(1<<7);
		//PORTD = seg_code[ten_digit];
		if (timeselect)
		{
			PORTD = seg_code[0];
		}
			else
			{
				PORTD = seg_code[ten_digit];
			}
	}
	PORTC = digit_on;
}
ISR(TIMER0_COMPA_vect)
{
	turn_off_all_digits();
	display();
	turn_on_new_digit();
	timer_button_state();
	buzzer();
	timer_tick = true;
}
int main()
{
	DDRA = 0x00; // All of port A are inputs
	DDRB = 0xff; // Declaring all bits of Port B as outputs
	DDRC = 0xff; // Declaring all bits of Port C as outputs
	DDRD = 0xff; // Declaring all bits of port D as outputs
	timeselect = 10; 
	retimeselect = 10;
	new_time = timeselect;
	while(1)
	{	
		millis_timer(2);
		if (timer_tick)
			{
				select_time = true;
				if (timeselect < 1)
					{
						timeselect = retimeselect;
						new_time = timeselect;
					}
				timer_tick = false;
			}
	}
}

 

Cody W Phipps

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

Why do you keep on calling millis_timer()? Set it once for CTC mode and leave it alone.

 

Move your digit to 7segment translation out of the ISR code - the translation only needs to be done when you change the digit values not every tick.

 

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

Kartman wrote:

Why do you keep on calling millis_timer()? Set it once for CTC mode and leave it alone.

 

 

Where do I put it then? Putting it outside of the while loop causes it not to work. I have moved the buzzer() and display() into the main loop.  

Cody W Phipps

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

Again, set the timer up for CTC mode. I gave you a link to an example. Thus, you only need to set the timer up once, and it keeps on ticking.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
PORTC | (1<<0) | (1<<1) | (1<<6) | (1<<7);

don't use 0,1,6,7 use names for each, so you can 

a) tell what is going on

b) catch mistakes quickly

It will make a big difference in your life.

 

	digit_on = PORTC | (1<<0) | (1<<1) | (1<<6) | (1<<7);
	digit_on &= (1<<0) | (1<<1) | (1<<6) | (1<<7);
	PORTC = digit_on;

This should be one line of code--- you are just setting PORTC to some value

Do you need digit_on at all?  Where does it ever differ from portc?  It seems to be a weak shadow implementation.

 

buzzer does not need to be in irq...if not needed to be there, don't put it there.

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

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

Kartman wrote:

Again, set the timer up for CTC mode. I gave you a link to an example. Thus, you only need to set the timer up once, and it keeps on ticking.

 

I'm very familiar with your link for I studied it before I even posted. Thank you for your input though. 

I prefer to keep it this way because this is the way I learn directly from Microchip/Atmel. If you notice I am using CTC mode as well, just seems the initialization methods are different. If this causes an issue in the future, I will follow your method and will come back to this post and comment on said problem. 

Cody W Phipps

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

avrcandies wrote:

instead of  PORTC, 6  don't use 6, give it a name so it's more clear than "6"

 

 You keep repeating the same things in the listing

 

A way that might be used any time,  note how much shorter this is. 

myport= PORTC | (1 <<0 ) | (1 << 1) | (0 << 6) |  (0 << 7);  Get current port bits and prep final result for all digits off

if digit==1 {myport &= ~(1<< 0)    ...any other digit 1 stuff };  \\make bit low, will turn on digit 1

if digit==2 {myport &= ~(1<< 1)    ...any other digit 2 stuff };  \\make bit low, will turn on digit 2

if digit==3 {myport &= ~(1<< 6)    ...any other digit 3 stuff };  \\make bit low, will turn on digit 3

if digit==4 {myport &= ~(1<< 7)    ...any other digit 4 stuff};   \\make bit low, will turn on digit 4

..

PORTC=myport   ;  /sets PORTC digits as needed

 

I followed your previous advice and now you are saying that I'm wrong. Pretty confusing. 

Cody W Phipps

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

But ultimately, I do agree. It should be one line. Here is the updated code with above changes (sorry Kartman calling millis_timer(2); in the loop still). I do have one last question, should I be declaring all of my ints with the uint8_t instead of just int?

 

//Define the Inputs
#define Timer_Button !(PINA & (1<<PINA1))
#define sbi(a, b) (a) |= (1 << (b))  // Set bit high
#define cbi(a, b) (a) &= ~(1 << (b)) // Set bit low
#define digit_1 (PORTC, 0) //Ten-thousandth digit
#define digit_2 (PORTC, 1) //Thousandth digit
#define digit_3 (PORTC, 6) //Hundredth digit
#define digit_4 (PORTC, 7) //Tenth digit

// Globals
int  timeselect,digit,retimeselect,buzz_interval,interval_diff,new_time,thou_digit,mod_digit_tenthou, ten_thou_digit, mod_digit_thou, hund_digit, ten_digit, sec;
uint8_t previousReading  = 1; // holds last 8 pin reads from pushbutton
uint8_t buttonWasPressed = 1; // keeps track of debounced button state
uint8_t buzzer_count = 0;
uint8_t debounce_count = 0;
uint8_t digit_on = 0;
char seg_code[]={ZERO, ONE, TWO, THREE, FOUR, FIVE, SIX, SEVEN, EIGHT, NINE};
volatile unsigned char timer_tick = false;
unsigned char select_time = false;

void millis_timer(uint8_t millis)
{
	TCCR0A |=  (1<<WGM01); //Enable CTC
	TCCR0B |= (1<<CS02)|(1<<CS00); //1024 prescaler
	//=(F_CPU*2/frequency*2*N)-1
	OCR0A = millis*8 - 1;
	TIMSK0 |= (1<<OCIE0A);
	sei();
}
void buzzer(void)
{
	if (buzzer_count != 0)
	{
		buzzer_count--;
		sbi(PORTB,7); // turn buzzer on
	}
	    else
		{
	    	       cbi(PORTB,7); // turn buzzer off
		}
}
void timer_button_state()
{
	if ((Timer_Button) != previousReading)
	{
		if(!buttonWasPressed)
		{
			if(debounce_count < 15)
				debounce_count++;
					else
						debounce_count = 0;
						buttonWasPressed = 1;
						//Below are the tasks to do if timer button is pushed
						buzzer_count = 75;
						if(select_time) //Select time mode
						{
							new_time--;
							timeselect = new_time;
						}
						//End of timer button tasks
		}
			else
				buttonWasPressed = 0;
	}
	previousReading = (Timer_Button);
}
void turn_off_all_digits()
{
	PORTC = (1<<digit_1) | (1<<digit_2) | (1<<digit_3) | (1<<digit_4);
}
void display()
{
    int time_Set;
	time_Set = timeselect*100;
	ten_thou_digit = time_Set / 1000;
	mod_digit_tenthou = time_Set % 1000;
	thou_digit = mod_digit_tenthou / 100;
	hund_digit = sec / 10;
	ten_digit = sec % 10;
	digit++;
	if (digit > 4)
	{
		digit = 0;
	}
}
void turn_on_new_digit()
{
	if (digit == 1)
	{
		//Ten-thousandths digit
		if (ten_thou_digit < 1) //If 1st digit is zero, leave blank
		{
			PORTC = (1<<digit_1) | (1<<digit_2) | (1<<digit_3) | (1<<digit_4);
			PORTD = seg_code[ten_thou_digit];
		}
			else
			{
				PORTC = ~(1<<digit_1) | (1<<digit_2) | (1<<digit_3) | (1<<digit_4);
				PORTD = seg_code[ten_thou_digit];
			}
	}
	if (digit == 2) //Thousandths digit
	{
		PORTC  = (1<<digit_1) | ~(1<<digit_2) | (1<<digit_3) | (1<<digit_4);
		PORTD = seg_code[thou_digit];
	}
	if (digit == 3) //Hundredths digit
	{
		PORTC = (1<<digit_1) | (1<<digit_2) | ~(1<<digit_3) | (1<<digit_4);
		if (timeselect)
		{
			PORTD = seg_code[0];
		}
			else
			{
				PORTD = seg_code[hund_digit];
			}
	}
	if (digit == 4) //Tenths digit
	{
		PORTC = (1<<digit_1) | (1<<digit_2) | (1<<digit_3) | ~(1<<digit_4);
		if (timeselect)
		{
			PORTD = seg_code[0];
		}
			else
			{
				PORTD = seg_code[ten_digit];
			}
	}
}
ISR(TIMER0_COMPA_vect)
{
	turn_off_all_digits();
	turn_on_new_digit();
	timer_button_state();
	timer_tick = true;
}
int main()
{
	DDRA = 0x00; // All of port A are inputs
	DDRB = 0xff; // Declaring all bits of Port B as outputs
	DDRC = 0xff; // Declaring all bits of Port C as outputs
	DDRD = 0xff; // Declaring all bits of port D as outputs
	timeselect = 10;
	retimeselect = 10;
	new_time = timeselect;
	while(1)
	{
		millis_timer(2);
		if (timer_tick)
		{
			display();
			buzzer();
			select_time = true;
			if (timeselect < 1)
			{
				timeselect = retimeselect;
				new_time = timeselect;
			}
			timer_tick = false;
		}
	}
}

 

Cody W Phipps

Last Edited: Thu. Oct 22, 2020 - 03:58 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

Purely on grounds of style, and avoidance of confusion, might I suggest that instead of this:

 

phippstech wrote:

 

...
void timer_button_state()
{
	if ((Timer_Button) != previousReading)
	{
		if(!buttonWasPressed)
		{
			if(debounce_count < 15)
				debounce_count++;
					else
						debounce_count = 0;
						buttonWasPressed = 1;
						//Below are the tasks to do if timer button is pushed
						buzzer_count = 75;
						if(select_time) //Select time mode
						{
							new_time--;
							timeselect = new_time;
						}
						//End of timer button tasks
		}
			else
				buttonWasPressed = 0;
	}
	previousReading = (Timer_Button);
}
...

 

you might consider changing your indentations and use of braces to make it clear that what is happening is what you intended to happen? (Makes it easier for other readers of your code, too!). Try:

...
void timer_button_state()
{
    if ((Timer_Button) != previousReading)
    {
	if(!buttonWasPressed)
	{
	    if(debounce_count < 15)
	    {
		debounce_count++;
	    }
	    else
	    {
		debounce_count = 0;
	    }
	    buttonWasPressed = 1;
	    //Below are the tasks to do if timer button is pushed
  	    buzzer_count = 75;
	    if(select_time) //Select time mode
	    {
		new_time--;
		timeselect = new_time;
	    }
	    //End of timer button tasks
	}
	else
	{
	    buttonWasPressed = 0;
	}
    }
    previousReading = (Timer_Button);
}
...

This makes it obvious that only the statement following the 'if' or the 'else' is to be executed, and not the whole block; and clarifies the semantics that the two legs of the if/else statement are mutually exclusive.

 

This is a matter of personal style, of course, and your original code is semantically equal to mine (unless I copied something incorrectly) but I do think that single line statements after control statements offer a huge risk of accidental misinterpretation, particularly if the indentation is unaligned. (MISRA insists on each { or } having a line to itself, and that they are used even for single line statements - something with which I wholeheartedly agree!)

 

As an aside - where logical expressions are being used, I prefer to see them expressed using booleans - true or false - rather than the equivalent 1 or 0. That way it's clear that they can only hold those two values (even though they are almost certainly implemented as ints internally. They're defined in <stdbool.h>.

 

Neil

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

As another aside (bit OT):

 

barnacle wrote:

As an aside - where logical expressions are being used, I prefer to see them expressed using booleans ... (even though they are almost certainly implemented as ints internally.

Neil

 

Oh lord I hope not.  In a memory-limited device like an AVR you're going to use two bytes for a boolean?  Really?

I guess it could encode, "True, False, File Not Found" and about 65,533 other possible "boolean" values... cheeky

 

I'm thinking about some systems I built in assembler using a dozen or more bits in GPIORs as flag collections (they're handy for that) and I'm now wondering what 'C' code would do if each flag bit had to be loaded as an int16_t into the 32 general purpose registers to be useful...  The pushing and the popping and the smashing of the stack would be quite impressive - for a kaboom.

 

Can we at least have at most a char or uint8_t?  Oh dear me.  S.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
//Define the Inputs
#define Timer_Button !(PINA & (1 << PINA1))
#define sbi(a, b) (a) |= (1 << (b))	 // Set bit high
#define cbi(a, b) (a) &= ~(1 << (b)) // Set bit low

#define DIGIT_1 (PC0) //Ten-thousandth digit
#define DIGIT_2 (PC1) //Thousandth digit
#define DIGIT_3 (PC6) //Hundredth digit
#define DIGIT_4 (PC7) //Tenth digit

#define NUM_DIGITS (4)

// Globals
int timeselect, retimeselect, buzz_interval, interval_diff, new_time, thou_digit, mod_digit_tenthou, ten_thou_digit, mod_digit_thou, hund_digit, ten_digit, sec;
uint8_t previousReading = 1;  // holds last 8 pin reads from pushbutton
uint8_t buttonWasPressed = 1; // keeps track of debounced button state
uint8_t buzzer_count = 0;
uint8_t debounce_count = 0;
uint8_t digit_on = 0;

const uint8_t seg_code[] = {ZERO, ONE, TWO, THREE, FOUR, FIVE, SIX, SEVEN, EIGHT, NINE};

unsigned char select_time = false;

volatile uint8_t timer_tick = false;
volatile uint8_t digits[NUM_DIGITS];
volatile uint8_t debounceCount = 30; //if this value is 0, button is pressed and debounced.

void millis_timer(uint8_t millis)
{
	TCCR0A = (1 << WGM01);	//Enable CTC
	OCR0A = millis * 8 - 1; //=(F_CPU*2/frequency*2*N)-1
	TIMSK0 |= (1 << OCIE0A);
	TCCR0B = (1 << CS02) | (1 << CS00); //1024 prescaler
	sei();
}

void buzzer(void)
{
	if (buzzer_count != 0)
	{
		buzzer_count--;
		sbi(PORTB, 7); // turn buzzer on
	}
	else
	{
		cbi(PORTB, 7); // turn buzzer off
	}
}
void timer_button_state()
{
	if ((Timer_Button) != previousReading)
	{
		if (!buttonWasPressed)
		{
			if (debounce_count < 15)
				debounce_count++;
			else
				debounce_count = 0;
			buttonWasPressed = 1;
			//Below are the tasks to do if timer button is pushed
			buzzer_count = 75;
			if (select_time) //Select time mode
			{
				new_time--;
				timeselect = new_time;
			}
			//End of timer button tasks
		}
		else
			buttonWasPressed = 0;
	}
	previousReading = (Timer_Button);
}

void display()
{
	int time_Set;
	time_Set = timeselect * 100;
	digits[0] = seg_code[time_Set / 1000];
	mod_digit_tenthou = time_Set % 1000;
	digits[1] = seg_code[mod_digit_tenthou / 100];
	digits[2] = seg_code [sec / 10];
	digits[3]  = seg_code[sec % 10];
}

ISR(TIMER0_COMPA_vect)
{
	static uint8_t digitNum = 0;

	switch (digitNum)
	{
	case 0:
				PORTC &= ~(1 << DIGIT_4);
				PORTD = digits[digitNum];
				PORTC |= ( 1 << DIGIT_1);
				break;

			case 1:
				PORTC &= ~(1 << DIGIT_1);
				PORTD = digits[digitNum];
				PORTC |= ( 1 << DIGIT_2);
			break;

			case 2:
				PORTC &= ~(1 << DIGIT_2);
				PORTD = digits[digitNum];
				PORTC |= ( 1 << DIGIT_3);
			break;

			case 3:
				PORTC &= ~(1 << DIGIT_3);
				PORTD = digits[digitNum];
				PORTC |= ( 1 << DIGIT_4);
			break;
	}
	digitNum++;
	digitNum &= 3;

	if (Timer_Button)
	{
		if (debounceCount)
		{
			debounceCount--;
		}
	}
	else
	{
		debounceCount = 15; //allow 30ms of debounce
	}
	timer_tick = true;
}

int main()
{
	DDRA = 0x00; // All of port A are inputs
	DDRB = 0xff; // Declaring all bits of Port B as outputs
	DDRC = 0xff; // Declaring all bits of Port C as outputs
	DDRD = 0xff; // Declaring all bits of port D as outputs
	timeselect = 10;
	retimeselect = 10;

	new_time = timeselect;

	//something to display on startup
	digits[0] = seg_code[1];
	digits[1] = seg_code[2];
	digits[2] = seg_code[3];
	digits[3] = seg_code[4];

	millis_timer(2); //setup timer once

	while (1)
	{
		if (timer_tick)
		{
			display();		//how often do you need to update the display??
			buzzer();
			select_time = true;
			if (timeselect < 1)
			{
				timeselect = retimeselect;
				new_time = timeselect;
			}
			timer_tick = false;
		}
	}
}

I hacked the code around a bit to simplify it. No guarantees it will work though. Hopefully it should give a few hints - much less pissing about with the display and a simple debounce.

 

 

Note: The website munged the formatting!

Last Edited: Fri. Oct 23, 2020 - 12:06 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

re: bytes or bools

 

Scroungre wrote:
Oh lord I hope not.  In a memory-limited device like an AVR you're going to use two bytes for a boolean?  Really?

 

If you use a construct of the type
 

if (xxx)
{
    ...
}

isn't xxx going to be promoted to a bool before the comparison anyway? Which is the size of an int? Though the size of the int is of course implementation dependent...

 

Or does the standard say not true/false but non-zero/zero?

 

My point was not to do with the storage of the bit pattern in an AVR particularly (for example, one could use individual bits at the cost of increased processing to extract a bit before the test) but for clarity in the code. If you want to detect on a logical value, use true/false; if you want to use non-zero/zero then make it explicit with a comparison (which a compiler ought to optimise away anyway.)

 

Neil

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

instead of  PORTC, 6  don't use 6, give it a name so it's more clear than "6"

 

I followed your previous advice and now you are saying that I'm wrong. Pretty confusing. 

 

How are you confused?...don't use numbers, use names so you can tell what is being done.  Are you mixing together  two different responses?

 

The idea behind the digits is you only turn on the digit you need (pref with turning all off) & simplify things so there will be no else. ...your simplification  ended up not being simplified.

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

Last Edited: Thu. Oct 22, 2020 - 04:31 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

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

Thanks for the help. 

Cody W Phipps