LED it's not blinking at the expected rate

[robi10101298]

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:
[Mod edit: tidied up the code by removing blank line for better readability]

/*********************************************
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)
// USART0 Receiver buffer
#define RX_BUFFER_SIZE0 8
char rx_buffer0[RX_BUFFER_SIZE0];

#if RX_BUFFER_SIZE0 <= 256
unsigned char rx_wr_index0,rx_rd_index0,rx_counter0;
#else
unsigned int rx_wr_index0,rx_rd_index0,rx_counter0;
#endif
// This flag is set on USART0 Receiver buffer overflow
bit rx_buffer_overflow0;
// USART0 Receiver interrupt service routine
interrupt [USART0_RXC] void usart0_rx_isr(void)
{
char status,data;
status=UCSR0A;
data=UDR0;
if ((status & (FRAMING_ERROR | PARITY_ERROR | DATA_OVERRUN))==0)
   {
   rx_buffer0[rx_wr_index0++]=data;

#if RX_BUFFER_SIZE0 == 256
   // special case for receiver buffer size=256
   if (++rx_counter0 == 0) rx_buffer_overflow0=1;
#else
   if (rx_wr_index0 == RX_BUFFER_SIZE0) rx_wr_index0=0;
   if (++rx_counter0 == RX_BUFFER_SIZE0)
     {
     rx_counter0=0;
     rx_buffer_overflow0=1;
     }
#endif
   }
}

#ifndef _DEBUG_TERMINAL_IO_
// Get a character from the USART0 Receiver buffer
#define _ALTERNATE_GETCHAR_
#pragma used+
char getchar(void)
{
char data;
while (rx_counter0==0);
data=rx_buffer0[rx_rd_index0++];

#if RX_BUFFER_SIZE0 != 256
if (rx_rd_index0 == RX_BUFFER_SIZE0) rx_rd_index0=0;
#endif
#asm("cli")
--rx_counter0;
#asm("sei")
return data;
}
#pragma used-
#endif
// USART0 Transmitter buffer
#define TX_BUFFER_SIZE0 8
char tx_buffer0[TX_BUFFER_SIZE0];

#if TX_BUFFER_SIZE0 <= 256
unsigned char tx_wr_index0,tx_rd_index0,tx_counter0;
#else
unsigned int tx_wr_index0,tx_rd_index0,tx_counter0;
#endif
// USART0 Transmitter interrupt service routine
interrupt [USART0_TXC] void usart0_tx_isr(void)
{
if (tx_counter0)
   {
   --tx_counter0;
   UDR0=tx_buffer0[tx_rd_index0++];

#if TX_BUFFER_SIZE0 != 256
   if (tx_rd_index0 == TX_BUFFER_SIZE0) tx_rd_index0=0;
#endif
   }
}

#ifndef _DEBUG_TERMINAL_IO_
// Write a character to the USART0 Transmitter buffer
#define _ALTERNATE_PUTCHAR_
#pragma used+
void putchar(char c)
{
while (tx_counter0 == TX_BUFFER_SIZE0);
#asm("cli")
if (tx_counter0 || ((UCSR0A & DATA_REGISTER_EMPTY)==0))
   {
   tx_buffer0[tx_wr_index0++]=c;

#if TX_BUFFER_SIZE0 != 256
   if (tx_wr_index0 == TX_BUFFER_SIZE0) tx_wr_index0=0;
#endif
   ++tx_counter0;
   }
else
   UDR0=c;
#asm("sei")
}
#pragma used-
#endif
//*************************************************************************************************
//********************END SERIAL STUFF (USART0)  **************************************************
//*************************************************************************************************
//*******   if you need USART1, enable it in Code Wizard and copy coresponding code here  *********
//*************************************************************************************************
/*
 * 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
  
   }
        
}// 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:

What am I doing wrong?

 

[Bluejets]

You could start with a current limit resistor on series with the LED.
Most uC ports will handle only 20mA.
Anyones guess on how much excess current an LED without a resistor will try to pull from the port pin.
Possibly even damage the port or the complete chip.

 

[Harald Kapp]

You cannot invert a single pin for the LED with a statement like

LED1 = ~LED1; // invert LED

You'll have to do some bit arithmetic like this:

#define LED PD4 //will be D6 in your application

ISR (TIMER1_OVF_vect)    // Timer1 ISR
{
    PORTD ^= (1 << LED);    // bitwise EXOR
    TCNT1 = 63974;   // for 1 sec at 16 MHz // not required, I think
}

(taken from this website).

Your code possibly overrides the input on D5 as it inverts all bits on port D (not clear as I couldn't locate the definition of LED1 in the code) thus setting D5 to "0" which will lead to an endless while loop in main here:

               while(SW1==0)
                   wdogtrig();    // wait for release

Another possible issue is here:

// Port D initialization
PORTD=0b00100000; // D.5 needs pull-up resistor
DDRD= 0b01010000; // D.6 is LED, D.4 is test output

For one this should read:

// Port D initialization
PORTD=0b00100000; // D.5 needs pull-up resistor
DDRD= 0b001010000; // D.6 is LED, D.4 is test output

D7 was missing in DDRD= statement. This is not the issue, just for the sake of clarity.
Here's the catch: all other pins of port D are declared as inputs but left floating in the circuit. This leads to unpredictable behaviour. All unused inputs should be explicitly declared as Pull-Up. Or declare the unused pins as outputs.

 

[robi10101298]

You cannot invert a single pin for the LED with a statement like


You'll have to do some bit arithmetic like this:

#define LED PD4 //will be D6 in your application

ISR (TIMER1_OVF_vect)    // Timer1 ISR
{
    PORTD ^= (1 << LED);    // bitwise EXOR
    TCNT1 = 63974;   // for 1 sec at 16 MHz // not required, I think
}

(taken from this website).

Your code possibly overrides the input on D5 as it inverts all bits on port D (not clear as I couldn't locate the definition of LED1 in the code) thus setting D5 to "0" which will lead to an endless while loop in main here:

               while(SW1==0)
                   wdogtrig();    // wait for release

Another possible issue is here:

// Port D initialization
PORTD=0b00100000; // D.5 needs pull-up resistor
DDRD= 0b01010000; // D.6 is LED, D.4 is test output

For one this should read:

// Port D initialization
PORTD=0b00100000; // D.5 needs pull-up resistor
DDRD= 0b001010000; // D.6 is LED, D.4 is test output

D7 was missing in DDRD= statement. This is not the issue, just for the sake of clarity.
Here's the catch: all other pins of port D are declared as inputs but left floating in the circuit. This leads to unpredictable behaviour. All unused inputs should be explicitly declared as Pull-Up. Or declare the unused pins as outputs.

What I've found is that this code could possibly work

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           
          
           }

Also, LED's are declared in the defs.h file:

/* definitions / defines file */
#define DEFS_H

#define SW_VERSION        13   /* i.e. major.minor software version nbr. */

#ifndef NULL
#define NULL  0
#endif
        
// logix ...
#define TRUE    1
#define FALSE    0
#define DUMMY    0

#define wdogtrig()            #asm("wdr") // call often if Watchdog timer enabled

#define CR                0x0D
#define LF                0x0A 

#define LED1 PORTD.6        // PORTx is used for output
#define SW1 PIND.5          // PINx is used for input
#define TESTP PORTD.4       // test bit durations
#include "funct.h"

 

[Harald Kapp]

#define LED1 PORTD.6 // PORTx is used for output

This notation is unknown to me for AVR C programming. Where can I read more on it?

The other remarks about open inputs and missing resistors still are open.

 

[robi10101298]

This notation is unknown to me for AVR C programming. Where can I read more on it?

The other remarks about open inputs and missing resistors still are open.

This is Arduino like port declaration
Arduino - PortManipulation

 

[Harald Kapp]

Thanks for the link. But there is no mentioning of syntax like "PIND.5". On the contrary, the Arduino manual recommends using DigitalRead() and DigitalWrite().
However, obviously the CVAVR compiler supports this syntax, otherwise you'd have received an error.
It is o.k. as long as you stick to this compiler, but note that using such specialties limits portability of your code to other compilers (e.g. GCC).

 

[robi10101298]

You're welcome! I think right now I'm fine with this CVAVR compiler, I don't think I'll use other

 

[Harald Kapp]

Now back to your code: does it work or do you still have issues?

 

[robi10101298]

Now back to your code: does it work or do you still have issues?

Thanks for your help! It's blinking, but not at the expected rate, I mean it's having a chaotic behaviour... For example, I turn on the switch, it's blinking 2 times. I turn off the switch, it's still blinking a few times then it's not blinking anymore. I turn again on the switch it's blinking faster a lot of times

 

[Harald Kapp]

Lets try to sort this out.
First let's see whether the basic blinking routine works properly. To do this, change main() to a most basic code:

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       
     }
        
}// end main loop

This should now have the LED blinking with a rate determined by the parameters from the init file:

// 1 sec = 19531 counts = 4C41H counts, from 0 to 4C40
// 4C40H = 4CH (MSB) and 40H (LSB)
OCR1AH=0x4C;
OCR1AL=0x40;

Does this work as supposed? If so, does the alternate setting work too?

{TCNT1H=0; TCNT1L=0; OCR1AH = 0x2F; OCR1AL = 0xA9;}

Do the rates of blinking match what you expect? If so, the timer setup is o.k. If not, check your timer settings.

 

[robi10101298]

Lets try to sort this out.
First let's see whether the basic blinking routine works properly. To do this, change main() to a most basic code:

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      
     }
       
}// end main loop

This should now have the LED blinking with a rate determined by the parameters from the init file:

// 1 sec = 19531 counts = 4C41H counts, from 0 to 4C40
// 4C40H = 4CH (MSB) and 40H (LSB)
OCR1AH=0x4C;
OCR1AL=0x40;

Does this work as supposed? If so, does the alternate setting work too?

{TCNT1H=0; TCNT1L=0; OCR1AH = 0x2F; OCR1AL = 0xA9;}

Do the rates of blinking match what you expect? If so, the timer setup is o.k. If not, check your timer settings.

The problem is that when I'm using a switch, if it's on 0 position the circuit it's still blinking...So the switch it's obsolete...Right now it's blinking 3 times but I can't see so cleary if it's 3 times or 4 times.. How can I check this?

 

[Harald Kapp]

Start by eliminating the switch as I suggested.

I can't see so cleary if it's 3 times or 4 times.. How can I check this?

Lower the blink rate so you can easily count.

 

[robi10101298]

Start by eliminating the switch as I suggested.

Lower the blink rate so you can easily count.

Nope, still no effect... Can you please provide me a complete code from scratch?

 

[Harald Kapp]

Sorry, I don't have time to write the code myself. But you find examples on the internet, e.g. this one.

 

[robi10101298]

Sorry, I don't have time to write the code myself. But you find examples on the internet, e.g. this one.

This is my final code but it's having a really weird behavior... 3 times blinking, then 4 times, 3 times blinking, 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);
        }               
}

 

[Harald Kapp]

The for loop doesn't make sense.
When you first enter the loop PCR1AH == 0x4C as per declaration at the start of the program.
In the first iteration of the loop (i == 0) this will lead to OCR1AH = 0x13 (first if statement is true).
In the next iteration of the loop (i==1) this will lead to OCR1AH = 0x2F (else if statement is now true since OCR1AH == 0x13 from the first iteration).
Remove the for loop completely, it is not required here.
Also: no need to set TCNT1H=0; TCNT1L=0; every time. As these values don't change, it is sufficient to set them once at the beginning of the program.

 

[robi10101298]

The for loop doesn't make sense.
When you first enter the loop PCR1AH == 0x4C as per declaration at the start of the program.
In the first iteration of the loop (i == 0) this will lead to OCR1AH = 0x13 (first if statement is true).
In the next iteration of the loop (i==1) this will lead to OCR1AH = 0x2F (else if statement is now true since OCR1AH == 0x13 from the first iteration).
Remove the for loop completely, it is not required here.
Also: no need to set TCNT1H=0; TCNT1L=0; every time. As these values don't change, it is sufficient to set them once at the beginning of the program.

Thanks for your help but I've solved in a much easier way:

void handle_press() {
        int i;
        int state = 0;
int delay[] = { 150, 250, 500 };
  /* blink three times at the rate specified for the current state */
  for (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 main (void)
{         
unsigned char temp,i;
char a=1;
    int teamNr = 13;

    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) {
      handle_press();
        }                 

    }

            
}// end main loop