Hello guys, I'm trying to make a led blink at 3 different frequencies, upon successive keypresses. My problem right now it's that it's only blinking 2 times and I don't understand why. D6 is LED, D4 is the test output. Here it's my main function:

/*********************************************
Project : Test software
**********************************************
Chip type: ATmega164A
Clock frequency: 20 MHz
Compilers:  CVAVR 2.x
*********************************************/

#include <mega164a.h>

#include <stdio.h>
#include <delay.h>  
#include <string.h> 
#include <stdlib.h>
#include "defs.h"    

//*************************************************************************************************
//*********** BEGIN SERIAL STUFF (interrupt-driven, generated by Code Wizard) *********************
//*************************************************************************************************

#ifndef RXB8
#define RXB8 1
#endif

#ifndef TXB8
#define TXB8 0
#endif

#ifndef UPE
#define UPE 2
#endif

#ifndef DOR
#define DOR 3
#endif

#ifndef FE
#define FE 4
#endif

#ifndef UDRE
#define UDRE 5
#endif

#ifndef RXC
#define RXC 7
#endif

#define FRAMING_ERROR (1<<FE)
#define PARITY_ERROR (1<<UPE)
#define DATA_OVERRUN (1<<DOR)
#define DATA_REGISTER_EMPTY (1<<UDRE)
#define RX_COMPLETE (1<<RXC)


/*
 * Timer 1 Output Compare A interrupt is used to blink LED
 */
interrupt [TIM1_COMPA] void timer1_compa_isr(void)
{
LED1 = ~LED1; // invert LED    
}                                  

/*
 * main function of program
 */
void main (void)
{          


    Init_initController();  // this must be the first "init" action/call!
    #asm("sei")             // enable interrupts
    LED1 = 1;               // initial state, will be changed by timer 1
    while(TRUE)
    {
        wdogtrig();            // call often else processor will reset
                 

        if(SW1 == 0)        // pressed
        {
            delay_ms(30);   // debounce switch
            if(SW1 == 0)    
            {                // LED will blink slow or fast
                while(SW1==0){
                    wdogtrig();    // wait for release
                    }
                // alternate between values and values/4 for OCR1A register
                // 4C40H / 4 = 1310H
                // new frequency = old frequency * 4
                if(OCR1AH == 0x4C)  
                    {TCNT1H=0; TCNT1L=0; OCR1AH = 0x13; OCR1AL = 0x10;}
                else     
                    {TCNT1H=0; TCNT1L=0; OCR1AH = 0x4C; OCR1AL = 0x40;}            
            }                
        }                                       
        
        // measure time intervals on oscilloscope connected to pin TESTP
        for(i=0; i<3; i++) {
            TESTP = 1;
            delay_us(1);
            TESTP = 0;   // may check accuracy of 1us interval on oscilloscope     
        }
    } 

            
}// end main loop

And here it's the init.c function:

/* initialization file */



#include <mega164a.h>

#include "defs.h"

                                         



/*

* most intialization values are generated using Code Wizard and depend on clock value

*/

void Init_initController(void)

{

// Crystal Oscillator division factor: 1

#pragma optsize-

CLKPR=0x80;

CLKPR=0x00;

#ifdef _OPTIMIZE_SIZE_

#pragma optsize+

#endif



// Input/Output Ports initialization

// Port A initialization

// Func7=In Func6=In Func5=In Func4=In Func3=In Func2=In Func1=In Func0=In

// State7=T State6=T State5=T State4=T State3=T State2=T State1=T State0=T

PORTA=0x00;

DDRA=0x00;



// Port B initialization

PORTB=0x00;

DDRB=0x00;



// Port C initialization

PORTC=0x00;

DDRC=0x00;



// Port D initialization

PORTD=0b00100000; // D.5 needs pull-up resistor

DDRD= 0b01010000; // D.6 is LED, D.4 is test output



// Timer/Counter 0 initialization

// Clock source: System Clock

// Clock value: Timer 0 Stopped

// Mode: Normal top=FFh

// OC0 output: Disconnected

TCCR0A=0x00;

TCCR0B=0x00;

TCNT0=0x00;

OCR0A=0x00;

OCR0B=0x00;



// Timer/Counter 1 initialization

// Clock source: System Clock

// Clock value: 19.531 kHz = CLOCK/256

// Mode: CTC top=OCR1A

// OC1A output: Discon.

// OC1B output: Discon.

// Noise Canceler: Off

// Input Capture on Falling Edge

// Timer 1 Overflow Interrupt: Off

// Input Capture Interrupt: Off

// Compare A Match Interrupt: On

// Compare B Match Interrupt: Off



TCCR1A=0x00;

TCCR1B=0x0D;

TCNT1H=0x00;

TCNT1L=0x00;

ICR1H=0x00;

ICR1L=0x00;



// 1 sec = 19531 counts = 4C41H counts, from 0 to 4C40

// 4C40H = 4CH (MSB) and 40H (LSB)

OCR1AH=0x4C;

OCR1AL=0x40;



OCR1BH=0x00;

OCR1BL=0x00;



// Timer/Counter 2 initialization

// Clock source: System Clock

// Clock value: Timer2 Stopped

// Mode: Normal top=0xFF

// OC2A output: Disconnected

// OC2B output: Disconnected

ASSR=0x00;

TCCR2A=0x00;

TCCR2B=0x00;

TCNT2=0x00;

OCR2A=0x00;

OCR2B=0x00;



// External Interrupt(s) initialization

// INT0: Off

// INT1: Off

// INT2: Off

// Interrupt on any change on pins PCINT0-7: Off

// Interrupt on any change on pins PCINT8-15: Off

// Interrupt on any change on pins PCINT16-23: Off

// Interrupt on any change on pins PCINT24-31: Off

EICRA=0x00;

EIMSK=0x00;

PCICR=0x00;



// Timer/Counter 0,1,2 Interrupt(s) initialization

TIMSK0=0x00;

TIMSK1=0x02;

TIMSK2=0x00;



// USART0 initialization

// Communication Parameters: 8 Data, 1 Stop, No Parity

// USART0 Receiver: On

// USART0 Transmitter: On

// USART0 Mode: Asynchronous

// USART0 Baud rate: 9600

UCSR0A=0x00;

UCSR0B=0xD8;

UCSR0C=0x06;

UBRR0H=0x00;

UBRR0L=0x81;



// USART1 initialization

// USART1 disabled

UCSR1B=0x00;





// Analog Comparator initialization

// Analog Comparator: Off

// Analog Comparator Input Capture by Timer/Counter 1: Off

ACSR=0x80;

ADCSRB=0x00;

DIDR1=0x00;



// Watchdog Timer initialization

// Watchdog Timer Prescaler: OSC/2048

#pragma optsize-

#asm("wdr")

// Write 2 consecutive values to enable watchdog

// this is NOT a mistake !

WDTCSR=0x18;

WDTCSR=0x08;

#ifdef _OPTIMIZE_SIZE_

#pragma optsize+

#endif



}

And this is how my minimalistic setup looks like:

 

This is a bit of gone around your elbow to get to your nose.

In the first place- don't use interrupts to begin with. Just use a loop and prove to yourself that you can blink the LED at one given rate, using a delay() timer. Always start with small steps when learning and build on your successes. You will progress much faster.

Put a pushbutton on the breadboard and read that from a pin- don't use a keypress and serial comm attempt. You're making your initial tests too complex. once you an blink and LED, then see if you can simply read a keypress. Then combined the two. Then try interrupts.

Also, remember Ohm's law and put a resistor in series with the LED, so you're not drawing too much current- the ATMEGA has a limit for the whole package, and if you don't use a resistor, you could easily be driving 20mA or more, which is unnecessary. The LED only needs about 5-8mA at it's rated voltage.

This may help:

Title: Understanding Basic Electronics, 1st Ed.
Publisher: The American Radio Relay League
ISBN: 0-87259-398-3

 

This is a bit of gone around your elbow to get to your nose.

In the first place- don't use interrupts to begin with. Just use a loop and prove to yourself that you can blink the LED at one given rate, using a delay() timer. Always start with small steps when learning and build on your successes. You will progress much faster.

Put a pushbutton on the breadboard and read that from a pin- don't use a keypress and serial comm attempt. You're making your initial tests too complex. once you an blink and LED, then see if you can simply read a keypress. Then combined the two. Then try interrupts.

Also, remember Ohm's law and put a resistor in series with the LED, so you're not drawing too much current- the ATMEGA has a limit for the whole package, and if you don't use a resistor, you could easily be driving 20mA or more, which is unnecessary. The LED only needs about 5-8mA at it's rated voltage.

This may help:

Title: Understanding Basic Electronics, 1st Ed.
Publisher: The American Radio Relay League
ISBN: 0-87259-398-3

I'm only interested in doing this in simulation not in real life

 

I'm only interested in doing this in simulation not in real life

Then what is the point of that. Please do as the man says. You will learn much faster. Your goal should be about developing problem solving skills and not so much about programming.

 

Then what is the point of that. Please do as the man says. You will learn much faster. Your goal should be about developing problem solving skills and not so much about programming.

if(SW1 == 0)        // pressed
       {
           delay_ms(30);   // debounce switch
           if(SW1 == 0)   
           {                // LED will blink slow or fast
               while(SW1==0)
                   wdogtrig();    // wait for release
                  
               // alternate between values and values/4 for OCR1A register
               // 4C40H / 4 = 1310H
               // new frequency = old frequency * 4
               if(OCR1AH == 0x4C)
                   {TCNT1H=0; TCNT1L=0; OCR1AH = 0x13; OCR1AL = 0x10;}    //0x1310
               else if(OCR1AH == 0x13)
                   {TCNT1H=0; TCNT1L=0; OCR1AH = 0x2F; OCR1AL = 0xA9;}    //0x2fa9
               else
                   {TCNT1H=0; TCNT1L=0; OCR1AH = 0x4C; OCR1AL = 0x40;}    //0x4C40           
          
           }

This is what I have until now, also the declared ports and other stuffs. Is it correct? Im not familiar with atmega registers...

 

if(SW1 == 0)        // pressed
       {
           delay_ms(30);   // debounce switch
           if(SW1 == 0) 
           {                // LED will blink slow or fast
               while(SW1==0)
                   wdogtrig();    // wait for release
                
               // alternate between values and values/4 for OCR1A register
               // 4C40H / 4 = 1310H
               // new frequency = old frequency * 4
               if(OCR1AH == 0x4C)
                   {TCNT1H=0; TCNT1L=0; OCR1AH = 0x13; OCR1AL = 0x10;}    //0x1310
               else if(OCR1AH == 0x13)
                   {TCNT1H=0; TCNT1L=0; OCR1AH = 0x2F; OCR1AL = 0xA9;}    //0x2fa9
               else
                   {TCNT1H=0; TCNT1L=0; OCR1AH = 0x4C; OCR1AL = 0x40;}    //0x4C40         
        
           }

This is what I have until now, also the declared ports and other stuffs. Is it correct? Im not familiar with atmega registers...

Did you read the datasheet, and understand it for the ATMEGA? I recognize you want to work in simulation only, but the problem remains the same. You should do simulation just as if it's the real thing, otherwise you won't learn correctly, which will cost you in the end. You are trying to merge 2 disciplines (electronics, and embedded programming) in which you have little experience in either. Learn one, become proficient, and then learn the other and become proficient.

It may not seem like it, but I'm trying to help you. You have to get some experience under your belt in both to enough of a point you can tackle what you're trying to do. These are engineering sciences, and the first indicator as to whether or not either of these fields is for you is whether or not you have the discipline and tenacity to learn them correctly.

Title: Understanding Basic Electronics, 1st Ed.
Publisher: The American Radio Relay League
ISBN: 0-87259-398-3

https://ww1.microchip.com/downloads...4A_PA-644A_PA-1284_P_Data-Sheet-40002070B.pdf

Furthermore, you should flow-chart. Learn to flow-chart. Flow-charting is the best tool in the world to check your logic with, and programming is all about logic.

Here is a free, online flow-charting/diagram tool you can use, which works amazingly well.

https://app.diagrams.net/

 

Did you read the datasheet, and understand it for the ATMEGA? I recognize you want to work in simulation only, but the problem remains the same. You should do simulation just as if it's the real thing, otherwise you won't learn correctly, which will cost you in the end. You are trying to merge 2 disciplines (electronics, and embedded programming) in which you have little experience in either. Learn one, become proficient, and then learn the other and become proficient.

It may not seem like it, but I'm trying to help you. You have to get some experience under your belt in both to enough of a point you can tackle what you're trying to do. These are engineering sciences, and the first indicator as to whether or not either of these fields is for you is whether or not you have the discipline and tenacity to learn them correctly.

Title: Understanding Basic Electronics, 1st Ed.
Publisher: The American Radio Relay League
ISBN: 0-87259-398-3

https://ww1.microchip.com/downloads...4A_PA-644A_PA-1284_P_Data-Sheet-40002070B.pdf

Furthermore, you should flow-chart. Learn to flow-chart. Flow-charting is the best tool in the world to check your logic with, and programming is all about logic.

Here is a free, online flow-charting/diagram tool you can use, which works amazingly well.

https://app.diagrams.net/

This is my final code but it's blinking really weird, 3 times, then 4 times, 3 times, then 4 times, and so on...

#include <mega164a.h>

#include <stdio.h>
#include <delay.h> 
#include <string.h>
#include <stdlib.h>
#define LED1 PORTD.6        // PORTx is used for output
#define SW1 PIND.5          // PINx is used for input
// 1 sec = 19531 counts = 4C41H counts, from 0 to 4C40
// 4C40H = 4CH (MSB) and 40H (LSB)
//1 sec
OCR1AH=0x4C;
OCR1AL=0x4B;
// Port D initialization
PORTD=0b00100000; // D.5 needs pull-up resistor
DDRD= 0b01010000; // D.6 is LED, D.4 is test output
while(1){
if(SW1 == 0)        // pressed
        {
        for(i=0; i<2; i++){
                 if(OCR1AH == 0x4C) 
                    {TCNT1H=0; TCNT1L=0; OCR1AH = 0x13; OCR1AL = 0x10;}    //0x1310
                else if(OCR1AH == 0x13)
                    {TCNT1H=0; TCNT1L=0; OCR1AH = 0x2F; OCR1AL = 0xA9;}    //0x2fa9
                else
                    {TCNT1H=0; TCNT1L=0; OCR1AH = 0x4C; OCR1AL = 0x40;}    //0x4C40
                
                 } 
                 delay_ms(1000);
        }               
}

 

I've managed to do it in a much simpler way, thanks for your help!

int state = 0;
int delay[] = { 150, 250, 500 };

void handle_press() {
 
  /* blink three times at the rate specified for the current state */
  for (int i = 0; i < 3; ++i) {
    LED1 = 1;
    delay_ms(delay[state]);
    LED1 = 0;
    delay_ms(delay[state]);
  }
 
  /* then wait another second */
  delay_ms(1000);
 
  /* update state */
  state = (state + 1);
  if (state == 3) {
    state = 0;
  }
}

void loop() {
  while (true) {
    if (SW1 == 0) {
      handle_press();
    }
  }
}