Program Flow advice - Functions and Button inputs

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

Hello All,

I'm seeking some advice on how to control program flow. I've just made a small breakout board that uses a shift register to drive a 2-digit seven segment display. Currently I have it doing several tricks, the outer segments "chase" around in a circle, it counts up in hex, blinks the decimal points, and counts down in decimal.

When I designed the board I also included a button which I would now like to implement. I know how to handle the debounce, etc, but my current implementation calls a function from main that I have to wait to get back from... for example:

void Chase(u8 direction, volatile u8 *buffer_address)
{
  if (direction == 1) {
    u8 display = 0x80;
    while (display > 2) {
      *buffer_address = display;
      Delay_ms(50);
      display >>= 1;
    }
  }
  else {
    u8 display = 0x02;
    while (1) {
      display <<= 1;
      *buffer_address = display;
      Delay_ms(50);
      if (display == 0x80) break;
    }
  }
}

As you can see there is a 50ms delay before the next segment is lit up. This means that as currently implemented I have to wait 400ms before this function returns to main in order to make any changes due to a button press.

Is there a better way to implement this type of behavior so that button presses will have an immediate effect on program flow?

Thanks!

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

Yes. You need to check whether you need to do some shifting stuff or not every 50ms and then do it.

To get the hang of it, you could move the 50ms delay to main loop for starters, and then the Chase() routine could be split into two parts: part 1 initializes the background stuff, i.e. sets direction and buffer address. You could still call it Chase() or Chase_init(). You call this when you need to.

Then, Part 2 of the thingy, would look at the direction and display variables if it needs to display something, move forware (do the shifts) and decide when to stop displaying and moving forward. For instance, if the variable called display is 0, then there is no display process running.

I think this is called a state machine, albeit a very simple one.

Edit: So the beef is that part 2 is called Chase_run() and it is run every 50ms in the mainloop. Now that mainloop runs every 50ms, also your buttons are checked every 50ms.

If you want more responsiveness, then, you could run your mainloop every 1ms, so then your Chase_run() code (or mainloop, how you like to think of it) performs stuff only after counting to 50, so the actual led chasing rate is still 50ms.

So in any case, it is better to think you need 50 times 1ms delay than 1 times 50ms delay.

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

Ok, I see where you're are going with this. So now I make the main loop one big state machine... since I want to check on the button about every 10ms I can do this with the state machine instead of with a separate timer.

Here's my crack at some psuedo-code, comments appreciated.

int main(void) {
  InitIO();
  InitTimers();

  unsigned char current_state = 0
  while(1) {  
    //Button Debounce
      //Check for a button press
        //Handle debounce
        //Change state if button is pressed

    //State Machine Follows
    
    //State 0 - Do nothing

    //State 1 - Chase outer ring of display
      //Check a variable that tracks (counts)
      //the chase progress

      //Start/Continue/or end progress based on
      //tracking variable


    //State 2 - Count up
      //Check to see how many state loops have passed
        //Increment counter if it is time to do so

    //State etc...

    //State machine done... pause before next loop
    Delay_ms(10);
  }
}

Does this end up bloating the code because I'm now counting loops for everything and not just using a delay routine for the actual amount of time I need to waste?

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

Inline delays are always bad, there are other things the micro could be doing. Code bloat isn't a problem.
I think the computer science types call the state machine above a "paced loop".
Matt

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

While it is certainly possible that the move to a state machine construction will increase code size slightly, look at what you have gained. By keeping track of position off of a smaller count you are able to reduce the latency between operations as they can be interlaced. This model also moves you closer to an interrupt driven model where there is even less time taken up with useless delays.

For instance, if the counters are made global it is possible to do something like this...

volatile uint8_t button_time = 0;
volatile uint8_t display_time = 0;

ISR(/*Timer with 1ms tick*/){
	++button_time;
	++display_time;
}

int main(void){
	// Initialization Stuff

	while(1){
		if( button_time >= 10 ){
			// Do button updating
			button_time = 0;
		}

		if( display_time >= 50 ){
			// Do display stuff
			display_time = 0;
		}

		// Time to do other stuff…
	}

	return 0;
}

Here, only the increment is actually required and that leaves a lot of time unalloted which would have been taken up in the delay. In that time the program can do more work or the uC put to sleep to reduce power consumption.

Martin Jay McKee

As with most things in engineering, the answer is an unabashed, "It depends."

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

I've got a working program using a state machine in the main loop. Right now its dependent on a 10ms delay but I plan to move the timing to an interrupt as mckeemj suggested.

Thanks for the help. Here's the code for any interested:

#define F_CPU 1000000UL

#include 
#include 
#include 
#include 

#define ShiftPort 	PORTD
#define ShiftDDR	DDRD
#define LatchPin	(1 << 5)
#define ClkPin		(1 << 6)
#define DataPin		(1 << 4)
#define ccPort		PORTD
#define ccDDR		DDRD
#define ccPin0		(1 << 1)
#define ccPin1		(1 << 0)
//Debounce definitions
#define KEY_DDR 	DDRD
#define KEY_PIN 	PIND
#define KEY_PORT 	PORTD
#define BTN1 		3
#define KEY_MSK 	(1 << BTN1)

typedef unsigned char u8; //now we can type "u8" instead of "unsigned char" (saves time and headaches)

const u8 numerals[] PROGMEM = {
    0xFC, //0b11111100 = 0
    0x60, //0b01100000 = 1
    0xDA, //0b11011010 = 2
    0xF2, //0b11110010 = 3
    0x66, //0b01100110 = 4
    0xB6, //0b10110110 = 5
    0xBE, //0b10111110 = 6
    0xE0, //0b11100000 = 7
    0xFE, //0b11111110 = 8
    0xF6, //0b11110110 = 9
    0xEE, //0b11101110 = A
    0x3E, //0b00111110 = b
    0x9C, //0b10011100 = C
    0x7A, //0b01111010 = d
    0x9E, //0b10011110 = E
    0x8E, //0b10001110 = F
};

PGM_P numerals_p PROGMEM = numerals;

volatile u8 digit_ones = 0x00;
volatile u8 digit_tens = 0x00;

//Debouce variables
unsigned char debounce_cnt = 0;
volatile unsigned char key_press;
unsigned char key_state;

/*void Delay_ms(int cnt) {
	while (cnt-->0) {
		_delay_ms(1);
	}
}*/

//### InitPorts function ###
//Initializes the ports needed to operate the shift register and button
//##########################
void InitPorts(void) 
{
  ShiftDDR |= (LatchPin | ClkPin | DataPin);
  ShiftPort &= (~LatchPin & ~ClkPin & ~DataPin);  //Set LatchPin, ClkPin and DataPin low

  ccDDR |= (ccPin0 | ccPin1);
  ccPort &= (~ccPin1); //Set ccPin1 low
  ccPort |= (ccPin0);  //Set ccPin0 high

  KEY_DDR &= ~KEY_MSK; //Set Key pins as inputs
  KEY_PORT |= KEY_MSK; //Enable internal pull-up resistor for Key pins
}


void InitTimers(void)
{
  TCCR0B |= (1<<CS01);
  TIMSK |= (1<<TOIE0);
  sei();
}

//### ShiftOut function ###
//Shifts the data passed to it out to the shift register
//#########################
void ShiftOut(u8 shiftData)
{
  ShiftPort &= (~LatchPin & ~ClkPin & ~DataPin);  	//All pins low: LatchPin low signals a write operation
  for (char i=0; i<8; i++)				//Loop through all 8 bits
  {
    ShiftPort &= ~ClkPin;				//Set ClkPin low
    if (shiftData & (1<<i)) ShiftPort |= DataPin;	//Set DataPin high if current bit is 1
    else ShiftPort &= ~DataPin;				//Set DataPin low if current bit is 0
    ShiftPort |= ClkPin;				//Set ClkPin high to increment shift register
  }
  ShiftPort |= LatchPin;  				//Set LatchPin high to signal end of write operation
}

void display_hex(u8 n) {
  digit_tens = pgm_read_byte(numerals_p + (n/16));
  digit_ones = pgm_read_byte(numerals_p + (n%16));
}

unsigned char get_key_press(unsigned char key_mask)
{
  cli();               // read and clear atomic !
  key_mask &= key_press;                        // read key(s)
  key_press ^= key_mask;                        // clear key(s)
  sei();
  return key_mask;
}

int main(void) {
  InitPorts();
  InitTimers();

  //State machine variables
  u8 loop_count = 0;
  u8 current_state = 3;
  u8 event_count = 0;
  u8 this_state_status = 0;

  while(1) {  
    //Button Debounce
    //This is the famous "danni debounce" code used with get_key_press function above.
    //It was written by Peter Dannegger: danni@specs.de
    static unsigned char ct0, ct1;
    unsigned char i;

    debounce_cnt = 0;

    i = key_state ^ ~KEY_PIN;    // key changed ?
    ct0 = ~( ct0 & i );          // reset or count ct0
    ct1 = ct0 ^ (ct1 & i);       // reset or count ct1
    i &= ct0 & ct1;              // count until roll over ?
    key_state ^= i;              // then toggle debounced state
    key_press |= key_state & i;  // 0->1: key press detect
    //end of danni debounce code

    //Get button read
    if( get_key_press( 1<<BTN1 ))
    {
      //Do stuff
      loop_count = 0;
      event_count = 0;
      if (++current_state > 3) current_state = 1;
    }

    //State Machine Follows
    //TODO:  Add a reset state function
    
    //State 0 - Do nothing

    //State 1 - Chase outer ring of display
    if (current_state == 1)
    {
      //Is this the first time through?
      if ((loop_count==0) && (event_count==0)) {
        //Setup the first frame and reset loop_count
        digit_tens = 0x80;
        digit_ones = 0x80;
      } else if (loop_count >= 5) {
        //Shift the digit buffers
        digit_ones >>= 1;
        digit_tens <<= 1;
        //reset loop_count
        loop_count = 0;
      
      
        //Have the digit buffers overflowed our pattern?
        if (digit_ones < 4) {
          //increment the event_count, we've complete 1 cycle
          ++event_count;
          //Setup the first frame and 
          digit_ones = 0x80;
        }
        if (digit_tens == 0) digit_tens = 0x04;

        //Is event_count larger than it should be
        if (event_count > 4) {
          //TODO Reset state machine
        }
      }
      //Increment loop_count
      ++loop_count;
    }

    //State 2 - Count up
    if (current_state == 2) {
      ++current_state; //Advance to next state becuase this one is empty right now
      //TODO: Make this state do something
      //Check to see how many state loops have passed
        //Increment counter if it is time to do so
    }

    //State 3 - Count up in hex
    if (current_state == 3) {
      //Initial setup
      if (event_count == 0) {
        display_hex(event_count++);
      }  
      else if (loop_count >= 20) {
        loop_count = 0;

        //if we counted too far
        if (event_count == 0xFF) {
          current_state = 1;
          event_count = 0;
          loop_count = -1;
        }
        else {
          display_hex(event_count++);
        }
      }
      ++loop_count;  
    }
    //State etc...

    
    _delay_ms(10);  //TODO: Transition to using an interrupt driven counter instead of this delay.

  }
}

//This ISR handles the scanning of the multiplexed display
ISR(TIMER0_OVF_vect)
{
  u8 temp_cc = (ccPort & (ccPin0 | ccPin1)); //Store common cathode setting
  ccPort |= (ccPin0 | ccPin1);  //Set both cathodes high (off)
  //If the 'ones' cathode was just on
  if (temp_cc & ccPin1) {
    ShiftOut(digit_tens);	//Shift out 'tens' data
    ccPort &= ~(ccPin1);	//Set 'tens' cathode low
  }
  //Otherwise 'tens' cathode was just low
  else 
  {
    ShiftOut(digit_ones);	//Shift out 'ones' data
    ccPort &= ~(ccPin0);	//Set 'ones' cathode low
  }
}
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

People have you headed down a direction that is an improvement on your original, and it's certainly an OK direction. But... just to confuse things :) OK, not really, but just to offer an alternative...

Eventually having all those delays sprinkled around gets hard to manage. My approach would be:

1. Set up the timer for 1 mSec ticks.
2. Keep a variable that counts up to 50 mSec (or what ever you need) and then rolls over to zero.
3. Debounce switches as I describe in this thread: https://www.avrfreaks.net/index.php?name=PNphpBB2&file=viewtopic&t=68838&highlight=debounce

4. Then outside the interrupt, do periodic work with something like:

while (true)
{
    if (current_tick_count == TIME_TO_SHIFT_DISPLAY)
    {
       // clever code here
    }
    if (current_tick_count == MORE_FREQENT_THING_T1
    ||  current_tick_count == MORE_FREQENT_THING_T2)
    {
        // something that happens twice per roll-over
    }
}

Note that everything happens according to it's own personal metronome. Note that there are no, zip, zero, nada calls to delay_ms().

The 'if' check can be simplified by choosing "convenient" numbers so that instead of a large boolean expression you can mask off high-order bits and just compare against the low order bits of the current_tick_count.

Just wanted to mention an alternative way to deal with your scheduling issue.

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

Thanks dbc,

I think you are on the same page as the others. I intend to set up a 10ms tick via a timer interrupt, then have the state machine and debounce check for how many interrupts have gone by for when they will trigger.

As for your debounce code that you linked too. I only have one button on this board and I want to be able to detect short or long click events. I know how to implement this with the danni debounce code (and already have), but I don't see how it would be possible with the snippet you suggested.

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

BTW, don't forget to make your interrupt-driven counter volatile. See Cliff's FAQ #1 or the AVR-libc Manual: Frequently Asked Questions #1 for the reason why.

Stu

Engineering seems to boil down to: Cheap. Fast. Good. Choose two. Sometimes choose only one.

Newbie? Be sure to read the thread Newbie? Start here!

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

Quote:
As for your debounce code that you linked too. I only have one button on this board and I want to be able to detect short or long click events.

Changing my code for one button is easy :) Changing it to handle long v. short pushes would require some thought, I agree. I'd probably do that as another obsever state machine looking at the debounced value.

BTW - stu_san speaks wise words about volatile -- variables changed inside an interrupt and observed outside an interrupt must be declared volatile.

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

stu_san wrote:
BTW, don't forget to make your interrupt-driven counter volatile. See Cliff's FAQ #1 or the AVR-libc Manual: Frequently Asked Questions #1 for the reason why.

Stu

Been there, done that. I must say, the first time I ran into the "volatile" gotcha it took me way too long to figure out why things weren't working. I must have spent two days thinking I had hardware problems!

Now it's old hat and in fact I'm already using the volatile keyword with my buffer bytes. Since I'm using a multiplexed 7-segment display (two digits share A-G pins) I have an ISR that reads the two bytes that serve as digit-buffers and handles scanning the display.

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

dbc wrote:
Quote:
As for your debounce code that you linked too. I only have one button on this board and I want to be able to detect short or long click events.

Changing my code for one button is easy :) Changing it to handle long v. short pushes would require some thought, I agree. declared volatile.

This is what I am using for a button on my motorcycle. It makes the start button respond to single, double, or a hold down type button press.

//
//=========================================================================
//
//		uint8_t getpin(volatile uint8_t *pinx, uint8_t pin_number)
//
//=========================================================================
//
//		This function reads the value of the pin pin_number of port pinx
//		and returns a value representing either a single click, a double
//		click, or a held down button. The function assumes a spst, normally
//		open, push button. The port must be configured so that the pin
//		sees a logic one when the button is not being pushed.
// 
//		The returned result always shows a single button press just before
//		the resolution of a double click, or hold down. For exanmple, if
//		the button is pushed and held long enough to represent a held down
//		push, the first call to the function will yield SWn=0. Subsequent
//		calls will return SWn=3 for as long as the button is held down.
//
//		The function takes approximately 4us/call with an 8MHz clock.
//
//		Author: GDK
//		Date:	24 Nov 2008
//
//
//=========================================================================

#include 

#define MAX1 25
#define MAX2 25
#define IDLE 1

static uint8_t 	SWn, 
				Count,
				Pin_val,
				Pushed,
				Inhibit,
				Released;


uint8_t getpin(uint8_t *pinx, uint8_t pin_number)
{


	Pin_val = *pinx & (1 << pin_number);  	// Read pin
	if(Pin_val)
		Pin_val = 1;
	else
		Pin_val = 0;

	if(Pin_val == 1) { 
		if(Pushed) {
			Released = 1;
			if(++Count > MAX2) {
				Pushed = 0; Released = 0; Count = 0, Inhibit = 0;
				SWn = IDLE;
			} else {
				SWn = 0;
			}

		} else {							// Not Pushed
			Pushed = 0; Released = 0; Count = 0, Inhibit = 0;
			SWn = IDLE;
		}
	} else { 								// Pin_val == 0
							
		if(!Inhibit) {
			if(Pushed) {
				if(Released) {
					Pushed = 0; Released = 0; Count = 0, Inhibit = 1;
					if(++Count > MAX2) {
						SWn = 0;			// Single Click
					} else {
						SWn = 2;			// Double Click
					}
				} else {					// Not Released
					if(++Count > MAX1) {
						Pushed = 0; Released = 0; Count = 0, Inhibit = 1;
						SWn = 3;			// Held Down
					} else {
						SWn = 0;			// Single Click
					}
				}
			} else {						// Not Pushed
				Pushed = 1;
				SWn = 0;					// Single Click
			}
		}
	}
	return SWn;
}