button/LED status code styling

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

I am looking for some code to emulate.

 

I have a single button that cycles through 6 options (timer1 configurations). The device has 6 status LEDS - each one indicates which of the 6 options is currently selected.

 

I am using Danni's key debounce code the button. I want my code to "de-couple" the LED from the timer1 one options. I typically rely heavily on global variables. I'd think I need my code to pass parameters/arguments.  Or should I look into state machines?

 

I write spaghetti code and I'm trying to get better.

 

Any help at all is greatly appreciated!

 

Thanks!

This topic has a solution.
Last Edited: Sun. Oct 1, 2017 - 10:45 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Welcome to AVR-Freaks!

 

We really can't comment without some code to reference! When you add your code listing, use the "<>" button in the tool bar of the forum edit window. Then, just copy and paste into the code window and it will show up as nicely formatted and indented green code (if you wrote it indented). 

 

Jim

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

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

I write spaghetti code

Tagliatelle code is much better not to mention pappardelle code.

John Samperi

Ampertronics Pty. Ltd.

www.ampertronics.com.au

* Electronic Design * Custom Products * Contract Assembly

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

Great! I have pasted it below.

 

I'm using timer1 and timer2 as a pair of output frequency generators. Selecting between six predefined timer states is the point of this project. Five continuous wave outputs and one toggling output state (toggles a 39215 Hz signal on and off at 1 Hz). I also want to have six LEDs that visually indicate the selected state.

 

The code below has 4 buttons: advance, reverse, on/off and toggle. In the next version I'm going to just use three buttons: advance, reverse, and output enable/disable (while still being able to advance so I can select the desired state, then enable the output)  The toggle will be an additional state that will be called by the advance_ouput() or reverse_output() function rather than by a separate pushbutton.

 

Button descriptions:

advance and reverse (KEY0 and KEY4) move between timer1/timer2 states.

Toggle (KEY5) enables on/off toggling at 1 Hz of a 39215 Hz signal from timer1 while leaving a 40 kHz signal on timer2

On/Off (KEY2) turns the output on and off and resets the output. I want this to be more of an enable/disable that doesn't reset the timer states, but allows me to disable the output, move through the timer states, with LED output, then enable the output - so it's a more user-friendly experience.

 

I know there are many ways to do it and I know what I've posted below isn't super easy to understand. I kept having to add flags to get things to work properly.

 

Just in case anyone tries to use this, it still isn't perfect because when I press the key to call reverse_output() and go from toggle_output() to reverse_output(), it selects the wrong state.

 

My main concern is, how do I make this simple to understand and make it so it reads like a book, nice and logical. Also being modular is another goal, partly as in the idea of programming the interface, not the implementation.

 

Initially, I was going to pose my problem and ask for ideas about a general program layout/structure but I wasn't sure how to ask that.

 

My main ideas are:

De-couple LED output from the timer states. Then have other functions call them together or independently, as necessary.

Is there another option for cycling through states with each press of the advance or reverse button? I'm using switch/case code.

I was curious to know if someone had an example of well-written, similar code that I can learn from.

 

If I can clarify anything please let me know. I really appreciate your time.

 

/************************************************************************/
/*                      Woosh123 frequency generator v1.1    
/*
/*                                            */
/*                      Debouncing 8 Keys    */
/*   Sampling 4 Times    */
/*                                                                      */
/*              Author: Peter Dannegger                                 */
/*                                                                      */
/************************************************************************/

#include <util\atomic.h>  // need "--std=c99"
#include <avr/io.h>

/*

Hooking up the wires:

LEDs:
Arduino digital pins 0-6 are the outputs of the LEDs
Arduino digital pin 7 has 3 modes - off - on - and toggle --it is an output indicator

Switches:
Arduino digital 12 is the "reverse LED" switch
Arduino digital 10 disables all LEDs
Arduino digital 8 is the "advance LED" switch
Arduino digital 13 is the "toggle output on digital pin 9" switch It doesn't affect digital pin 11


Outputs:
Arduino digital 9 (portb.1) is the output of timer1
Arduino digital 11 (portb.3) is the output of timer2


Configuring Buttons:
The buttons use a basic state machine type configuration to advance and reverse LEDs. 
Functions can be called for each state, configured independently, though probably ideally symmetrically
in advance_output() and reverse_output()
in rewrite the state machine needs to be fixed to ignore multiple on/off presses if an advance/reverse hasn't 
occurred. Right now I'm doing that with a flag.
Also in rewrite to decouple the led output from the frequency output
Also in rewrite resuming after turning off the output should output the same frequency. It does that here
but it's ugly
this is major speghetti code


Feature: 
Can go forward, back, and disable LEDs. pressing one switch advances the lit LED. Pressing the other 
switch results in the previous LED being lit. Another switch sets up a toggled output
from timer1. Pressing the final switch disables all LEDs. 
Goal is to be able to use three switches and a bank of up to 8 LEDs to control (via functions) and indicate (via LEDs) the 
output/output-state


*/




typedef unsigned char u8;
typedef signed short s16;

#define XTAL  16e6  // 16MHz

#define KEY_PIN  PINB
#define KEY_PORT PORTB
#define KEY_DDR  DDRB
#define KEY0  0
#define KEY1  1
#define KEY2  2
#define KEY3  3
#define KEY4  4
#define KEY5  5
#define KEY6  6
#define KEY7  7

#define LED_DDR  DDRD
#define LED_PORT PORTD
#define LED0  0
#define LED1  1
#define LED2  2
#define LED3  3
#define LED4  4
#define LED5  5
#define LED6  6
#define LED7  7


u8 key_state;    // debounced and inverted key state:
     // bit = 1: key pressed
u8 key_press;    // key press detect

u8 frequency_led_state = 0b00000000; //these leds are turned on sequentially on portd0 1 2 3 4 5 7 8
u8 output_enable = 0; //0 means off - 1 means continuous wave enabled - 2 means pulse wave enabled


volatile long toggle_interval =0;



u8 get_key_press( u8 key_mask )
{
  ATOMIC_BLOCK(ATOMIC_FORCEON){  // read and clear atomic !
    key_mask &= key_press;  // read key(s)
    key_press ^= key_mask;  // clear key(s)
  }
  return key_mask;
}



void clear_timer1(){
 LED_PORT = 0B00000000;
 frequency_led_state = LED_PORT;
 TCCR1B = 0x00;
 TCCR1A = 0x00;
 
}

void clear_timer1_and_timer2(){
    //clear all the registers
 TCCR1B = 0b00000000;
 TCCR1A = 0b00000000;
 TCCR2B = 0b00000000;
 TCCR2A = 0b00000000;
 
}



void set_timer1_2300(){
 TCCR1B |= (1 << CS10); // set prescaler to 0
 TCCR1B |= (1 << WGM12); //put timer 1 in ctc mode a mode where the top is defined in register OCR1A
 TCCR1A |= (1 <<COM1A0); // turn on bits in compare match to toggle.
 OCR1A = 3478; // 2300 Hz
 
 
}

void set_timer1_40000(){
 TCCR1B |= (1 << CS10); // set prescaler to 0
 TCCR1B |= (1 << WGM12); //put timer 1 in ctc mode a mode where the top is defined in register OCR1A
 TCCR1A |= (1 <<COM1A0); // turn on bits in compare match to toggle.
 OCR1A = 200; // 40000 Hz
}

void set_timer1_39215(){
 TCCR1B |= (1 << CS10); // set prescaler to 0
 TCCR1B |= (1 << WGM12); //put timer 1 in ctc mode a mode where the top is defined in register OCR1A
 TCCR1A |= (1 <<COM1A0); // turn on bits in compare match to toggle.
 OCR1A = 204; //39215.5
}

void set_timer1_1000000(){
 TCCR1B |= (1 << CS10); // set prescaler to 0
 TCCR1B |= (1 << WGM12); //put timer 1 in ctc mode a mode where the top is defined in register OCR1A
 TCCR1A |= (1 <<COM1A0); // turn on bits in compare match to toggle.
 OCR1A = 8; //1000000
 
}

void set_timer1_8000000(){
 TCCR1B |= (1 << CS10); // set prescaler to 0
 TCCR1B |= (1 << WGM12); //put timer 1 in ctc mode a mode where the top is defined in register OCR1A
 TCCR1A |= (1 <<COM1A0); // turn on bits in compare match to toggle.
 OCR1A = 0; //8000000
 
}


 
void set_timer2_3012(){
 TCCR2B |= (1 << CS21) |(1 << CS20); // set prescaler to 32
 TCCR2A |= (1 << WGM21); //put timer 2 in ctc mode a mode where the top is defined in register OCR2A
 TCCR2A |= (1 <<COM2A0); // turn on bits in compare match to toggle.
 OCR2A = 83; //set value to trigger compare match. this determines the frequency
}

void set_timer2_40000(){
 TCCR2B |= (1 << CS21); // set prescaler to 8
 TCCR2A |= (1 << WGM21); //put timer 2 in ctc mode a mode where the top is defined in register OCR2A
 TCCR2A |= (1 <<COM2A0); // turn on bits in compare match to toggle.
 OCR2A = 20; //set value to trigger compare match. this determines the frequency
}

void set_timer2_1000000(){
 TCCR2B |= (1 << CS20); // set prescaler to 0
 TCCR2A |= (1 << WGM21); //put timer 2 in ctc mode a mode where the top is defined in register OCR2A
 TCCR2A |= (1 <<COM2A0); // turn on bits in compare match to toggle.
 OCR2A = 8; //set value to trigger compare match. this determines the frequency
 
}

void set_timer2_8000000(){
 TCCR2B |= (1 << CS20); // set prescaler to 0
 TCCR2A |= (1 << WGM21); //put timer 2 in ctc mode a mode where the top is defined in register OCR2A
 TCCR2A |= (1 <<COM2A0); // turn on bits in compare match to toggle.
 OCR2A = 0; //set value to trigger compare match. this determines the frequency
 
}



void advance_output(){

clear_timer1_and_timer2();
if(output_enable ==1){
  
 
 switch(frequency_led_state){
  case 0b00000000:
   	LED_PORT = 0B00000001;
   	frequency_led_state = LED_PORT;
   	set_timer1_40000();
   	set_timer2_40000();
   	break;
  
  
   case 0b00000001: //40kHz + -700 offset
   	LED_PORT = 0B00000010;
   	frequency_led_state = LED_PORT;
   	set_timer1_39215();
   	set_timer2_40000();
   	break;

   case 0b00000010: //3kHz + -700 offset
   	LED_PORT = 0B00000100;
   	frequency_led_state = LED_PORT;
   	set_timer1_2300();
   	set_timer2_3012();
   	break;
  

   case 0b00000100: //both at 1 MHz
   	LED_PORT = 0B00001000;
   	frequency_led_state = LED_PORT;
   	set_timer1_1000000();
   	set_timer2_1000000();
   	break;
  

   case 0b00001000:
   	LED_PORT = 0B00010000;
   	frequency_led_state = LED_PORT;
     	set_timer1_8000000();
     	set_timer2_8000000();
     	break;
  
   case 0b00010000:
   	LED_PORT = 0B00100000;
   	frequency_led_state = LED_PORT;
   	break;
  
   case 0b00100000:
   	LED_PORT = 0B01000000;
   	frequency_led_state = LED_PORT;
   	break;

  
   case 0b01000000: //this is is the overflow value > change the frequency_led_state  value here to overflow at a different value
   	LED_PORT = 0B00000000;
   	frequency_led_state = LED_PORT;
	break;
}
}
}

void reverse_output(){
  
  if(output_enable ==1){

 clear_timer1_and_timer2();
 switch(frequency_led_state){
 
   case 0b00000000:
   	LED_PORT = 0B01000000;
   	frequency_led_state = LED_PORT;
   	break;
  
   case 0b01000000:
   	LED_PORT = 0B00100000;
   	frequency_led_state = LED_PORT;
   	break;

   case 0b00100000:
   	LED_PORT = 0B00010000;
   	frequency_led_state = LED_PORT;
   	set_timer1_8000000();
   	set_timer2_8000000();
   	break;

   case 0b00010000: //both at 1 MHz
   	LED_PORT = 0B00001000;
   	frequency_led_state = LED_PORT;
   	set_timer1_1000000();
   	set_timer2_1000000();
   	break;
  
   case 0b00001000: //3kHz + -700 offset
   	LED_PORT = 0B00000100;
   	frequency_led_state = LED_PORT;
   	set_timer1_2300();
   	set_timer2_3012();  
   	break;
  
   case 0b00000100: //40kHz + -700 offset
	LED_PORT = 0B00000010;
   	frequency_led_state = LED_PORT;
   	set_timer1_39215();
   	set_timer2_40000();  
   	break;  
 
   case 0b00000010: //both at 40kHz
   	LED_PORT = 0B00000001;
   	frequency_led_state = LED_PORT;
   	set_timer1_40000();
   	set_timer2_40000();
   	break;

   case 0b00000001: //this is is the overflow value > change the frequency_led_state  value here to overflow at a different value
   	LED_PORT = 0B00000000;
   	frequency_led_state = LED_PORT;
   	break;
}
}
}

void toggle_output(){
 
 if (toggle_interval >=500){
    set_timer1_39215(); //this is the output frequency
    LED_PORT = 0B10000000; //this is specific to the frequency set above - referenced in advance
   
 }

 if (toggle_interval >=1000){
    clear_timer1();
    toggle_interval = 0;
    LED_PORT = 0B00000000;
   
 }
}


ISR( TIMER0_COMPA_vect )  // every 10ms
{
  static u8 ct0 = 0xFF, ct1 = 0xFF; // 8 * 2bit counters
  u8 i;

  i = ~KEY_PIN;    // read keys (low active)
  i ^= key_state;   // 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
  
  
 //this is patched in here badly maybe
 //anyway - when enable_output is 2 this will call a function that toggles the output using the toggle_output function
 if (output_enable ==2){
  toggle_interval += 10;
  toggle_output();
  }
  }


int main( void )
{
  TCCR0A = 1<<WGM01;   // T0 Mode 2: CTC
  TCCR0B = 1<<CS02^1<<CS00;  // divide by 1024
  OCR0A = XTAL / 1024.0 * 10e-3 -1; // 10ms
  TIMSK0 = 1<<OCIE0A;   // enable T0 interrupt

  //KEY_DDR = 0;    // input
  //SET INPUTS
 // KEY_DDR |= (1<<PB0) | (1<<PB1); 
   KEY_DDR |= (0<<PB0) | (0<<PB2) | (0<<PB4) |(0<<PB5);

  KEY_PORT = 0xFF;   // pullups on
  LED_PORT = 0x00;   // LEDs off (low active)
  
 //LED_DDR = 0xFF;   // LED output
    //SET OUTPUTS
  LED_DDR |= (1<<PD0) | (1<<PD1) | (1<<PD2) | (1<<PD3) | (1<<PD4) | (1 <<PD5) | (1<<PD6) | (1<<PD7);
  KEY_DDR |= (1<<PB1) | (1<<PB3); //set pb1 and pb3 outputs for oc1a and oc2a

   
    
key_state = ~KEY_PIN;   // no action on keypress during reset
  sei();




  for(;;){     // main loop
 
 //OR and AND arduino pin 7 with the LED output - this keeps led pin 7 lit-OFF-or toggling depending on output_enable's state
 
 if(output_enable ==1){
 	LED_PORT |= 0B10000000;
 }
 if(output_enable == 0){
 	LED_PORT &= 0B00000000;
 }
 
 
  if( get_key_press( 1<<KEY0 ))

 
  {
 
   advance_output();

  }


    if( get_key_press( 1<<KEY1 ))
  
  {
   
  }

    if( get_key_press( 1<<KEY2 ))

  {
   switch (output_enable){
    case 0:
      output_enable =1;
     
     //LED_PORT = 0B10000000;
          // frequency_led_state = LED_PORT;
    
     break;
    case 1:
     clear_timer1_and_timer2();
     output_enable=0; 
     
     //resets output to zero
     LED_PORT = 0B00000000;
     frequency_led_state = LED_PORT;
     break;
     
    case 2:
     clear_timer1_and_timer2();
     output_enable =0;
     break;
     
  }
  }


/*
    if( get_key_press( 1<<KEY3 ))

  {
   LED_PORT ^= 1<<LED3;

  }
*/

    if( get_key_press( 1<<KEY4 ))

    {

     reverse_output();
 
  }

   if( get_key_press( 1<<KEY5 )){
      output_enable = 2;
  
  
   
   }
/*
    if( get_key_press( 1<<KEY6 ))
      LED_PORT ^= 1<<LED6;

    if( get_key_press( 1<<KEY7 ))
      LED_PORT ^= 1<<LED7;
*/
  }
}

 

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

Tagliatelle code is much better not to mention pappardelle code.

With a nice aglio e olio.  Mmmm...

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"Read a lot.  Write a lot."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

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

woosh123 wrote:
I write spaghetti code and I'm trying to get better.
You mean you want to try and complicate the spaghetti above and beyond what you already have.

 

I started to read through your code but gave up almost instantly when I found the indentation was not consistent. Most programmers use this to understand the flow of execution and nesting. Without it the code is already "all over the place". So anyway I posted your code into:

 

https://www.tutorialspoint.com/o...

 

(the left pane) then clicked [beautify] top left and it resulted in:

#include <util\atomic.h>  // need "--std=c99"
#include <avr/io.h>

/*

Hooking up the wires:

LEDs:
Arduino digital pins 0-6 are the outputs of the LEDs
Arduino digital pin 7 has 3 modes - off - on - and toggle --it is an output indicator

Switches:
Arduino digital 12 is the "reverse LED" switch
Arduino digital 10 disables all LEDs
Arduino digital 8 is the "advance LED" switch
Arduino digital 13 is the "toggle output on digital pin 9" switch It doesn't affect digital pin 11


Outputs:
Arduino digital 9 (portb.1) is the output of timer1
Arduino digital 11 (portb.3) is the output of timer2


Configuring Buttons:
The buttons use a basic state machine type configuration to advance and reverse LEDs.
Functions can be called for each state, configured independently, though probably ideally symmetrically
in advance_output() and reverse_output()
in rewrite the state machine needs to be fixed to ignore multiple on/off presses if an advance/reverse hasn't
occurred. Right now I'm doing that with a flag.
Also in rewrite to decouple the led output from the frequency output
Also in rewrite resuming after turning off the output should output the same frequency. It does that here
but it's ugly
this is major speghetti code


Feature:
Can go forward, back, and disable LEDs. pressing one switch advances the lit LED. Pressing the other
switch results in the previous LED being lit. Another switch sets up a toggled output
from timer1. Pressing the final switch disables all LEDs.
Goal is to be able to use three switches and a bank of up to 8 LEDs to control (via functions) and indicate (via LEDs) the
output/output-state


*/




typedef unsigned char u8;
typedef signed short s16;

#define XTAL  16e6  // 16MHz

#define KEY_PIN  PINB
#define KEY_PORT PORTB
#define KEY_DDR  DDRB
#define KEY0  0
#define KEY1  1
#define KEY2  2
#define KEY3  3
#define KEY4  4
#define KEY5  5
#define KEY6  6
#define KEY7  7

#define LED_DDR  DDRD
#define LED_PORT PORTD
#define LED0  0
#define LED1  1
#define LED2  2
#define LED3  3
#define LED4  4
#define LED5  5
#define LED6  6
#define LED7  7


u8 key_state;    // debounced and inverted key state:
// bit = 1: key pressed
u8 key_press;    // key press detect

u8 frequency_led_state = 0b00000000; //these leds are turned on sequentially on portd0 1 2 3 4 5 7 8
u8 output_enable = 0; //0 means off - 1 means continuous wave enabled - 2 means pulse wave enabled


volatile long toggle_interval =0;



u8 get_key_press( u8 key_mask )
{
    ATOMIC_BLOCK(ATOMIC_FORCEON) { // read and clear atomic !
        key_mask &= key_press;  // read key(s)
        key_press ^= key_mask;  // clear key(s)
    }
    return key_mask;
}



void clear_timer1() {
    LED_PORT = 0B00000000;
    frequency_led_state = LED_PORT;
    TCCR1B = 0x00;
    TCCR1A = 0x00;

}

void clear_timer1_and_timer2() {
    //clear all the registers
    TCCR1B = 0b00000000;
    TCCR1A = 0b00000000;
    TCCR2B = 0b00000000;
    TCCR2A = 0b00000000;

}



void set_timer1_2300() {
    TCCR1B |= (1 << CS10); // set prescaler to 0
    TCCR1B |= (1 << WGM12); //put timer 1 in ctc mode a mode where the top is defined in register OCR1A
    TCCR1A |= (1 <<COM1A0); // turn on bits in compare match to toggle.
    OCR1A = 3478; // 2300 Hz


}

void set_timer1_40000() {
    TCCR1B |= (1 << CS10); // set prescaler to 0
    TCCR1B |= (1 << WGM12); //put timer 1 in ctc mode a mode where the top is defined in register OCR1A
    TCCR1A |= (1 <<COM1A0); // turn on bits in compare match to toggle.
    OCR1A = 200; // 40000 Hz
}

void set_timer1_39215() {
    TCCR1B |= (1 << CS10); // set prescaler to 0
    TCCR1B |= (1 << WGM12); //put timer 1 in ctc mode a mode where the top is defined in register OCR1A
    TCCR1A |= (1 <<COM1A0); // turn on bits in compare match to toggle.
    OCR1A = 204; //39215.5
}

void set_timer1_1000000() {
    TCCR1B |= (1 << CS10); // set prescaler to 0
    TCCR1B |= (1 << WGM12); //put timer 1 in ctc mode a mode where the top is defined in register OCR1A
    TCCR1A |= (1 <<COM1A0); // turn on bits in compare match to toggle.
    OCR1A = 8; //1000000

}

void set_timer1_8000000() {
    TCCR1B |= (1 << CS10); // set prescaler to 0
    TCCR1B |= (1 << WGM12); //put timer 1 in ctc mode a mode where the top is defined in register OCR1A
    TCCR1A |= (1 <<COM1A0); // turn on bits in compare match to toggle.
    OCR1A = 0; //8000000

}



void set_timer2_3012() {
    TCCR2B |= (1 << CS21) |(1 << CS20); // set prescaler to 32
    TCCR2A |= (1 << WGM21); //put timer 2 in ctc mode a mode where the top is defined in register OCR2A
    TCCR2A |= (1 <<COM2A0); // turn on bits in compare match to toggle.
    OCR2A = 83; //set value to trigger compare match. this determines the frequency
}

void set_timer2_40000() {
    TCCR2B |= (1 << CS21); // set prescaler to 8
    TCCR2A |= (1 << WGM21); //put timer 2 in ctc mode a mode where the top is defined in register OCR2A
    TCCR2A |= (1 <<COM2A0); // turn on bits in compare match to toggle.
    OCR2A = 20; //set value to trigger compare match. this determines the frequency
}

void set_timer2_1000000() {
    TCCR2B |= (1 << CS20); // set prescaler to 0
    TCCR2A |= (1 << WGM21); //put timer 2 in ctc mode a mode where the top is defined in register OCR2A
    TCCR2A |= (1 <<COM2A0); // turn on bits in compare match to toggle.
    OCR2A = 8; //set value to trigger compare match. this determines the frequency

}

void set_timer2_8000000() {
    TCCR2B |= (1 << CS20); // set prescaler to 0
    TCCR2A |= (1 << WGM21); //put timer 2 in ctc mode a mode where the top is defined in register OCR2A
    TCCR2A |= (1 <<COM2A0); // turn on bits in compare match to toggle.
    OCR2A = 0; //set value to trigger compare match. this determines the frequency

}



void advance_output() {

    clear_timer1_and_timer2();
    if(output_enable ==1) {


        switch(frequency_led_state) {
        case 0b00000000:
            LED_PORT = 0B00000001;
            frequency_led_state = LED_PORT;
            set_timer1_40000();
            set_timer2_40000();
            break;


        case 0b00000001: //40kHz + -700 offset
            LED_PORT = 0B00000010;
            frequency_led_state = LED_PORT;
            set_timer1_39215();
            set_timer2_40000();
            break;

        case 0b00000010: //3kHz + -700 offset
            LED_PORT = 0B00000100;
            frequency_led_state = LED_PORT;
            set_timer1_2300();
            set_timer2_3012();
            break;


        case 0b00000100: //both at 1 MHz
            LED_PORT = 0B00001000;
            frequency_led_state = LED_PORT;
            set_timer1_1000000();
            set_timer2_1000000();
            break;


        case 0b00001000:
            LED_PORT = 0B00010000;
            frequency_led_state = LED_PORT;
            set_timer1_8000000();
            set_timer2_8000000();
            break;

        case 0b00010000:
            LED_PORT = 0B00100000;
            frequency_led_state = LED_PORT;
            break;

        case 0b00100000:
            LED_PORT = 0B01000000;
            frequency_led_state = LED_PORT;
            break;


        case 0b01000000: //this is is the overflow value > change the frequency_led_state  value here to overflow at a different value
            LED_PORT = 0B00000000;
            frequency_led_state = LED_PORT;
            break;
        }
    }
}

void reverse_output() {

    if(output_enable ==1) {

        clear_timer1_and_timer2();
        switch(frequency_led_state) {

        case 0b00000000:
            LED_PORT = 0B01000000;
            frequency_led_state = LED_PORT;
            break;

        case 0b01000000:
            LED_PORT = 0B00100000;
            frequency_led_state = LED_PORT;
            break;

        case 0b00100000:
            LED_PORT = 0B00010000;
            frequency_led_state = LED_PORT;
            set_timer1_8000000();
            set_timer2_8000000();
            break;

        case 0b00010000: //both at 1 MHz
            LED_PORT = 0B00001000;
            frequency_led_state = LED_PORT;
            set_timer1_1000000();
            set_timer2_1000000();
            break;

        case 0b00001000: //3kHz + -700 offset
            LED_PORT = 0B00000100;
            frequency_led_state = LED_PORT;
            set_timer1_2300();
            set_timer2_3012();
            break;

        case 0b00000100: //40kHz + -700 offset
            LED_PORT = 0B00000010;
            frequency_led_state = LED_PORT;
            set_timer1_39215();
            set_timer2_40000();
            break;

        case 0b00000010: //both at 40kHz
            LED_PORT = 0B00000001;
            frequency_led_state = LED_PORT;
            set_timer1_40000();
            set_timer2_40000();
            break;

        case 0b00000001: //this is is the overflow value > change the frequency_led_state  value here to overflow at a different value
            LED_PORT = 0B00000000;
            frequency_led_state = LED_PORT;
            break;
        }
    }
}

void toggle_output() {

    if (toggle_interval >=500) {
        set_timer1_39215(); //this is the output frequency
        LED_PORT = 0B10000000; //this is specific to the frequency set above - referenced in advance

    }

    if (toggle_interval >=1000) {
        clear_timer1();
        toggle_interval = 0;
        LED_PORT = 0B00000000;

    }
}


ISR( TIMER0_COMPA_vect )  // every 10ms
{
    static u8 ct0 = 0xFF, ct1 = 0xFF; // 8 * 2bit counters
    u8 i;

    i = ~KEY_PIN;    // read keys (low active)
    i ^= key_state;   // 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


//this is patched in here badly maybe
//anyway - when enable_output is 2 this will call a function that toggles the output using the toggle_output function
    if (output_enable ==2) {
        toggle_interval += 10;
        toggle_output();
    }
}


int main( void )
{
    TCCR0A = 1<<WGM01;   // T0 Mode 2: CTC
    TCCR0B = 1<<CS02^1<<CS00;  // divide by 1024
    OCR0A = XTAL / 1024.0 * 10e-3 -1; // 10ms
    TIMSK0 = 1<<OCIE0A;   // enable T0 interrupt

    //KEY_DDR = 0;    // input
    //SET INPUTS
// KEY_DDR |= (1<<PB0) | (1<<PB1);
    KEY_DDR |= (0<<PB0) | (0<<PB2) | (0<<PB4) |(0<<PB5);

    KEY_PORT = 0xFF;   // pullups on
    LED_PORT = 0x00;   // LEDs off (low active)

//LED_DDR = 0xFF;   // LED output
    //SET OUTPUTS
    LED_DDR |= (1<<PD0) | (1<<PD1) | (1<<PD2) | (1<<PD3) | (1<<PD4) | (1 <<PD5) | (1<<PD6) | (1<<PD7);
    KEY_DDR |= (1<<PB1) | (1<<PB3); //set pb1 and pb3 outputs for oc1a and oc2a



    key_state = ~KEY_PIN;   // no action on keypress during reset
    sei();




    for(;;) {    // main loop

//OR and AND arduino pin 7 with the LED output - this keeps led pin 7 lit-OFF-or toggling depending on output_enable's state

        if(output_enable ==1) {
            LED_PORT |= 0B10000000;
        }
        if(output_enable == 0) {
            LED_PORT &= 0B00000000;
        }


        if( get_key_press( 1<<KEY0 ))


        {

            advance_output();

        }


        if( get_key_press( 1<<KEY1 ))

        {

        }

        if( get_key_press( 1<<KEY2 ))

        {
            switch (output_enable) {
            case 0:
                output_enable =1;

                //LED_PORT = 0B10000000;
                // frequency_led_state = LED_PORT;

                break;
            case 1:
                clear_timer1_and_timer2();
                output_enable=0;

                //resets output to zero
                LED_PORT = 0B00000000;
                frequency_led_state = LED_PORT;
                break;

            case 2:
                clear_timer1_and_timer2();
                output_enable =0;
                break;

            }
        }


        /*
            if( get_key_press( 1<<KEY3 ))

          {
           LED_PORT ^= 1<<LED3;

          }
        */

        if( get_key_press( 1<<KEY4 ))

        {

            reverse_output();

        }

        if( get_key_press( 1<<KEY5 )) {
            output_enable = 2;



        }
        /*
            if( get_key_press( 1<<KEY6 ))
              LED_PORT ^= 1<<LED6;

            if( get_key_press( 1<<KEY7 ))
              LED_PORT ^= 1<<LED7;
        */
    }
}

I hope it's clear to you already that this is a major step forward in being able to follow the structure of this program?

 

You then have things like:

#include <util\atomic.h>

Do not use '\' in C programs for path separator. It should always be '/' whether you program in Windows or Linux (because only one of these is compatible with both systems).

 

Next you have:

typedef unsigned char u8;
typedef signed short s16;

Just don't do this! There has been a standard in C since 1999 (before in fact) that suggests:

#include <stdint.h>

and now your types are uint8_t and int16_t and every C compiler in the world (since 99) should know about this. It does not help if every Tom, Dick and Harry just make up their own (quite often wrong) versions of these things. Just get used to using uin8_t etc.

 

Next you have five different functions that all look like:

void set_timer1_NNNNN() 

When you see duplicated code in C it usually means there's a way for the recurring thing to be done by one (and therefore more easily maintainable) routine that can handle the 5 different variants. Even if this meant:

void set_timer(int variant) {
    const int dividers[] = {3478, 200, 204, 8, 0 , 83} 
    TCCR1B |= (1 << CS10); // set prescaler to 0
    TCCR1B |= (1 << WGM12); //put timer 1 in ctc mode a mode where the top is defined in register OCR1A
    TCCR1A |= (1 <<COM1A0); // turn on bits in compare match to toggle.
    OCR1A = dividers[variant]; // correct divider to use
}

You might then do something like:

#define FREQ_2300 1
#define FREQ_40000 2
#define FREQ_39125 3
#define FREQ_1000000 4
#define FREQ_8000000 5

and later:

set_timer1(FREQ_39125);

and now they all use the same routine.

 

However I suspect there's actually a formula that relates the frequency to the OCR value (in fact the thing you use to calculate 2478, 204, etc in the first place). So you probably don't need the lookup table but can:

void set_timer(uint32_ freq) {
    TCCR1B |= (1 << CS10); // set prescaler to 0
    TCCR1B |= (1 << WGM12); //put timer 1 in ctc mode a mode where the top is defined in register OCR1A
    TCCR1A |= (1 <<COM1A0); // turn on bits in compare match to toggle.
    OCR1A = formula(freq); // correct divider to use
}

and then just

set_timer(39125);

There are other points in the code that seem to have very similar repeating blocks that could probably be done programmatically too.

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

I'll work on implementing your suggestions this weekend and I'll report back with the updated code. 

 

It seems like a state machine will be the best way to handle the button presses to advance, reverse and disable the output. I've been reading "Embedded C Programming and the Atmel AVR, 2nd Edition" and it has a detailed example of a state machine using stop lights.  Do you think that would a state machine would be the best way to approach this?

 

I like that code beautifier.

 

Thanks!

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

I've updated the code and it's working as I hoped and is more readable and easy to modify. Your advice was very helpful.

 

/************************************************************************/
/*
arduino digital pins 0-4 - output LEDs
arduino digital pin 6 - toggle/interrupt signal enable LED - this can be thought of as interrupting the output signal
arduino digital pin 7 - output and continuous wave enabled. LED
pin 8 - output and continuous wave enabled
pin 9 - output1
pin 10 - toggle/interrupt signal enable switch - this can be thought of as interrupting the output signal.
pin 11 - output2
pin 12 - advance output to next state switch
pin 13 - reverse output to previous state switch

outputs are square waves at the following frequencies:
selection timer1 timer2
0         39215 40000
1         40000 40000
2   	  2300  3012
3   	  1MHz  1MHz
4     	  8MHz  8MHz



future improvements
don't use a toggle function for the on/off button because then other functions that want to turn off the
on/off button end up toggling the button if called more than once. better compose the toggle from separate on/off functions that
can then be called independently. 

                              */
/*                                                                      */
/************************************************************************/

#include <util/atomic.h>  // need "--std=c99"




#define XTAL  16e6  // 8MHz

#define KEY_PIN  PINB
#define KEY_PORT PORTB
#define KEY_DDR  DDRB
#define KEY0  0
#define KEY1  1
#define KEY2  2
#define KEY3  3
#define KEY4  4
#define KEY5  5
#define KEY6  6
#define KEY7  7

#define LED_DDR  DDRD
#define LED_PORT PORTD
#define LED0  0
#define LED1  1
#define LED2  2
#define LED3  3
#define LED4  4
#define LED5  5
#define LED6  6
#define LED7  7


//for checking state of output


//define
#define FREQ_2300 0
#define FREQ_3012 1
#define FREQ_40000 2
#define FREQ_39125 3
#define FREQ_1000000 4
#define FREQ_8000000 5

#define FREQ_TIMER1_39215_TIMER2_40000 0
#define FREQ_TIMER1_40000_TIMER2_40000 1
#define FREQ_TIMER1_2300_TIMER2_3012 2
#define FREQ_TIMER1_1000000_TIMER2_1000000 3
#define FREQ_TIMER1_8000000_TIMER2_8000000 4


#define FREQ_TIMER1_39215_TIMER2_40000_LED 0
#define FREQ_TIMER1_40000_TIMER2_40000_LED 1
#define FREQ_TIMER1_2300_TIMER2_3012_LED 2
#define FREQ_TIMER1_1000000_TIMER2_1000000_LED 3
#define FREQ_TIMER1_8000000_TIMER2_8000000_LED 4


#define MIN_LED_THAT_IS_FREQUENCY_INDICATOR  0
#define MAX_LED_THAT_IS_FREQUENCY_INDICATOR  4

#define MIN_OUTPUT_FREQUENCY_SELECTION 0
#define MAX_OUTPUT_FREQUENCY_SELECTION 4

#define DISABLE 0
#define ENABLE 1

#define DISABLED 0
#define ENABLED 1




//globals:
uint8_t output_status = 0; //this will be between 0 and 4
uint8_t output_status_LED = 0; //this variable will hold the current output state of the LED between 0 and 4
int8_t CURRENT_OUTPUT_FREQUENCY_SELECTION = -1; //set as -1 so no frequency is selected when the device is turned on
int8_t CURRENT_OUTPUT_FREQUENCY_SELECTION_LED = -1;
uint8_t KEY3_OR_KEY4_WAS_PRESSED = 0;
uint8_t toggle_mode = DISABLED;
uint8_t toggle_interrupt_passes_counter = 0;
uint8_t toggle_is_in_high_state = 0;

uint8_t TOGGLE_FREQUENCY = 10; //Multiply this value by 20 (10*2) to get the period of the toggle functionality



uint8_t key_state;    // debounced and inverted key state:
// bit = 1: key pressed
uint8_t key_press;    // key press detect


ISR(TIMER0_COMPA_vect)  // every 10ms
{
    static uint8_t ct0 = 0xFF, ct1 = 0xFF; // 8 * 2bit counters
    uint8_t i;

    i = ~KEY_PIN;    // read keys (low active)
    i ^= key_state;   // 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

//this is used by the toggle function that's called by pressing KEY2
    if((toggle_interrupt_passes_counter > TOGGLE_FREQUENCY) && (toggle_mode == ENABLED) && (output_status == ENABLED)) {

        //this switch just toggles the  output on/off each time the interrupt timer counter reaches 50
        switch(toggle_is_in_high_state) {
        case 0:
            toggle_is_in_high_state = 1;
            turn_on_timers();
            toggle_interrupt_passes_counter = 0;
            break;

        case 1:
            toggle_is_in_high_state = 0;
            turn_off_continous_wave();
            toggle_interrupt_passes_counter = 0;
            break;
        }

    }

    toggle_interrupt_passes_counter++;
}


uint8_t get_key_press( uint8_t key_mask )
{
    ATOMIC_BLOCK(ATOMIC_FORCEON) {  // read and clear atomic !
        key_mask &= key_press;  // read key(s)
        key_press ^= key_mask;  // clear key(s)
    }
    return key_mask;
}

void initialize_timer0() {
    TCCR0A = 1<<WGM01;   // T0 Mode 2: CTC
    TCCR0B = 1<<CS02^1<<CS00;  // divide by 1024
    OCR0A = XTAL / 1024.0 * 10e-3 -1; // 10ms
    TIMSK0 = 1<<OCIE0A;   // enable T0 interrupt

}

void initilize_ports_and_leds() {
    KEY_DDR = 0;    // input
    KEY_PORT = 0xFF;   // pullups on
    LED_PORT &= 0B00000000;   // LEDs off (low active)
    LED_DDR = 0xFF;   // LED output
    key_state = ~KEY_PIN;   // no action on keypress during reset
    KEY_DDR |= (1<<PORTB1) | (1<<PORTB3); //set pb1 and pb3 outputs for oc1a and oc2a
}


void clear_timer1_and_timer2() {
    //clear all the registers
    TCCR1B = 0b00000000;
    TCCR1A = 0b00000000;
    TCCR2B = 0b00000000;
    TCCR2A = 0b00000000;
}

void set_power_indicator_led() {
    switch(output_status_LED) {


    case DISABLED:
        PORTD |= (1 << PORTD7); // PD7 goes high
        output_status_LED = ENABLED; //this sets the output to status to enabled
        break;

    case ENABLED:
        PORTD &= ~(1 << PORTD7); // PD7 goes low
        output_status_LED = DISABLED; //this sets the output status to disabled
        break;
    }
}

void set_output_status() {

    switch(output_status) {
    case DISABLED:
        output_status = ENABLED; //this sets the output to status to enabled
        break;
    case ENABLED:

        output_status = DISABLED; //this sets the output status to disabled
        break;
    }

}

void advance_LED() {
    if(CURRENT_OUTPUT_FREQUENCY_SELECTION_LED < MAX_LED_THAT_IS_FREQUENCY_INDICATOR) {
        CURRENT_OUTPUT_FREQUENCY_SELECTION_LED +=1;
    }
    else {
        CURRENT_OUTPUT_FREQUENCY_SELECTION_LED = CURRENT_OUTPUT_FREQUENCY_SELECTION_LED;
    }
}

void reverse_LED() {
    if(CURRENT_OUTPUT_FREQUENCY_SELECTION_LED > MIN_LED_THAT_IS_FREQUENCY_INDICATOR) {
        CURRENT_OUTPUT_FREQUENCY_SELECTION_LED -=1;
    }
    else {
        CURRENT_OUTPUT_FREQUENCY_SELECTION_LED = CURRENT_OUTPUT_FREQUENCY_SELECTION_LED;
    }
}

void set_timer1() {


    const int dividers[] = {204, 200, 3478, 8, 0}; // f = 39215, 40000, 2300, 1000000, 8000000
    TCCR1B |= (1 << CS10); // set prescaler to 0
    TCCR1B |= (1 << WGM12); //put timer 1 in ctc mode a mode where the top is defined in register OCR1A
    TCCR1A |= (1 <<COM1A0); // turn on bits in compare match to toggle.
    OCR1A = dividers[CURRENT_OUTPUT_FREQUENCY_SELECTION]; // correct divider to use
}

void set_timer2() {

    const int dividers[] = {20, 20, 83, 8, 0}; //f = 40000, 40000, 3012, 1000000, 8000000


    if (dividers[CURRENT_OUTPUT_FREQUENCY_SELECTION] == 83) {

        TCCR2B |= (1 << CS21) |(1 << CS20); // set prescaler to 32
    }

    if (dividers[CURRENT_OUTPUT_FREQUENCY_SELECTION] == 20) {

        TCCR2B |= (1 << CS21); // set prescaler to 8
    }
    else {

        TCCR2B |= (1 << CS20); // set prescaler to 0
    }



    TCCR2A |= (1 << WGM21); //put timer 2 in ctc mode a mode where the top is defined in register OCR2A
    TCCR2A |= (1 <<COM2A0); // turn on bits in compare match to toggle.
    OCR2A = dividers[CURRENT_OUTPUT_FREQUENCY_SELECTION]; // correct divider to use
}

void advance_output() {

    if(CURRENT_OUTPUT_FREQUENCY_SELECTION < MAX_OUTPUT_FREQUENCY_SELECTION) {
        CURRENT_OUTPUT_FREQUENCY_SELECTION +=1;

    }

    if(CURRENT_OUTPUT_FREQUENCY_SELECTION == MAX_OUTPUT_FREQUENCY_SELECTION)
        CURRENT_OUTPUT_FREQUENCY_SELECTION = MAX_OUTPUT_FREQUENCY_SELECTION;
}

void reverse_output() {



    if(CURRENT_OUTPUT_FREQUENCY_SELECTION > MIN_OUTPUT_FREQUENCY_SELECTION) {

        CURRENT_OUTPUT_FREQUENCY_SELECTION -=1;
    }
    else {
        CURRENT_OUTPUT_FREQUENCY_SELECTION = CURRENT_OUTPUT_FREQUENCY_SELECTION;
    }
}



void set_frequency_indicator_LED() {

    switch(CURRENT_OUTPUT_FREQUENCY_SELECTION_LED) {
    case FREQ_TIMER1_39215_TIMER2_40000_LED:
        PORTD &= 0B11000000; //turn off all pins but leave PORTD7 (THE POWER INDICATOR) alone
        PORTD |= (1 << PORTD0); //enable PORTD0
        break;

    case FREQ_TIMER1_40000_TIMER2_40000_LED:
        PORTD &= 0B11000000; //turn off all pins but leave PORTD7 (THE POWER INDICATOR) alone
        PORTD |= (1 << PORTD1); //enable PORTD1
        break;

    case FREQ_TIMER1_2300_TIMER2_3012_LED:
        PORTD &= 0B11000000; //turn off all pins but leave PORTD7 (THE POWER INDICATOR) alone
        PORTD |= (1 << PORTD2); //enable PORTD2
        break;

    case FREQ_TIMER1_1000000_TIMER2_1000000_LED:
        PORTD &= 0B11000000; //turn off all pins but leave PORTD7 (THE POWER INDICATOR) alone
        PORTD |= (1 << PORTD3); //enable PORTD3
        break;

    case FREQ_TIMER1_8000000_TIMER2_8000000_LED:
        PORTD &= 0B11000000; //turn off all pins but leave PORTD7 (THE POWER INDICATOR) alone
        PORTD |= (1 << PORTD4); //enable PORTD4
        break;


    }

}

void advance_continous_wave() {

    clear_timer1_and_timer2();
    advance_LED();
    set_frequency_indicator_LED();
    advance_output();
    if(output_status) {
        turn_on_timers();
    }
}

void reverse_continous_wave() {

    clear_timer1_and_timer2();
    reverse_LED();
    set_frequency_indicator_LED();
    reverse_output();

    if(output_status && CURRENT_OUTPUT_FREQUENCY_SELECTION != -1) {
        turn_on_timers();
    }

}

void turn_on_timers() {

    set_timer1();
    set_timer2();
}

void turn_on_continous_wave_without_advancing_or_reversing_it() {
    //this sets timer1 and timer2 without advancing them if KEY3 or KEY4 haven't been pressed


    turn_on_timers();


}

void turn_off_continous_wave() {
    clear_timer1_and_timer2();
}






int main( void)
{

    initialize_timer0();
    initilize_ports_and_leds();
    sei();

    for(;;) {     // main loop







        if(get_key_press( 1<<KEY0)) {
            KEY3_OR_KEY4_WAS_PRESSED = 0;

            set_power_indicator_led();
            set_output_status();

            //this clears the timer1 timer2 if output is disabled to turn off oscillators
            if(output_status == DISABLED && CURRENT_OUTPUT_FREQUENCY_SELECTION != -1) {
                toggle_mode = DISABLED; //turn off toggle mode if it's running


                //turn of wave
                turn_off_continous_wave();
            }

            if(output_status == ENABLED && CURRENT_OUTPUT_FREQUENCY_SELECTION != -1 && KEY3_OR_KEY4_WAS_PRESSED ==0) {

                //turn on wave if output frequency hasn't been changed via key3 or key4

                turn_on_timers();

            }
        }



        /*if( get_key_press( 1<<KEY1 ))
        LED_PORT ^= 1<<LED1;
        */
        if( get_key_press( 1<<KEY2 ))
        {

            if(output_status == ENABLED && (CURRENT_OUTPUT_FREQUENCY_SELECTION !=-1)) {

                switch(toggle_mode) {

                //turn on toggle mode
                case DISABLED:

                    toggle_mode = ENABLED;
                    PORTD |= (1 << PORTD6);
                    break;

                //restore continuous wave function and disable toggle_mode
                case ENABLED:
                    if(output_status != 0) {
                        turn_on_continous_wave_without_advancing_or_reversing_it();
                    }
                    toggle_mode = DISABLED;
                    PORTD &= ~(1 << PORTD6);
                    break;
                }

            }
        }



        if( get_key_press( 1<<KEY3 ))
        {}

        if( get_key_press( 1<<KEY4 )) {
            KEY3_OR_KEY4_WAS_PRESSED =1; //this flag is referenced by key0 when enabled/disables the output to make sure frequency hasn't changed
            advance_continous_wave();

        }



        if( get_key_press( 1<<KEY5 )) {
            KEY3_OR_KEY4_WAS_PRESSED =1;
            reverse_continous_wave();


        }

        if( get_key_press( 1<<KEY6 ))
            LED_PORT ^= 1<<LED6;

        if( get_key_press( 1<<KEY7 ))
            LED_PORT ^= 1<<LED7;
    }
}

 

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

Why is your code so long, since it is doing something fairly simple...pages and pages and pages of code to set some timers & leds?  This could probably be done in 40 lines of assembly,  let alone 4 pages of C-code.  Of course, making it very compact can make it more difficult to maintain or understand.  Likewise, making it excessively long can have the same effect, since what is being done gets blurred by the length.

 

void set_frequency_indicator_LED()  ....why use 21 lines of C-code to turn on an led, selected by a parameter? (and leave another led untocuched)...this could be done in 3 or 4 lines at most.  Make the parameter 1 2 4 8 ... and just do an and/or statement..no case statement needed at all.  Something like:  PORTD= ledparam | (1<<power_led_position), which would always turn on the power led, if that is sufficient, plus others as set by the parameter.

 

 

When in the dark remember-the future looks brighter than ever.

Last Edited: Sun. Oct 1, 2017 - 09:25 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hello,

 

  Since you appear to be using an Arduino as a hardware platform then I suggest using Arduino libraries for buttons.

This is the one that I use:

http://log.liminastudio.com/itp/...

 

Like many downloadable Arduino libraries, it is underdocumented and therefore difficult to understand.  It is also missing about a dozen detailed documented example programs that would demonstrate its features. 

 

Basically the library compares the state of the button to the state that the button had the last time that it was checked.  The loop() calls  buttonXXX.process() at scheduled intervals: I use 10 intervals per second.  This negates the need for debouncing.  You can set up callback functions to handle button quick or long presses.  If a button is released or held for a predefined interval then the code will execute this function when process() is called. 

 

   You can have many push-button switches.  Each switch will set up a unique object in SRAM with its own process() function.  Then you have separate functions for each button's action, such as quick press or long press.  I've been able to implement routines for handling quick and long presses easily enough.  I'm at a loss for understanding how to implement double-clicks and redo-activity-while-button-is-held (like setting the time on a watch) actions, because the library has no demo programs for these actions.

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

Awesome,  great improvement.  Thank you so much. I agree that it's length actually blurs the ease of understanding and usage - opposite of my intention :(

 

Fixed the function you mentioned:

void set_frequency_indicator_LED() {

//this bit shift raises 2 to the current output_frequency_selection_led value 
uint8_t freq = 1 << CURRENT_OUTPUT_FREQUENCY_SELECTION_LED; 

//binary 11000000
uint8_t power_and_toggle_led_mask = 192; 

//if these LEDs are lit leave them on, turn off the rest
PORTD &= power_and_toggle_led_mask; 

//turn on frequency indicator LEDs
PORTD |= (freq); 

}

 

Last Edited: Sun. Oct 1, 2017 - 11:06 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

#defines and constants in uppercase-good. Variable names in upper case - bad.

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

Thanks Kartman!