I'm designing a guitar tuner through ATmega16p and CodeVisionAVR and i just can't get my code to run

Question

I'm designing a guitar tuner through an atmel mega16 processor and CodeVisionAVR for my university's second project. I have connected a mono jack to the processor's PINA.7 (ADC converter) and GND. I have 7 LEDs (PORTB.0..6) that should turn on through a series of if/elseif based on the frequency of the fundamental of the signal.

I'm taking the fundamental of the signal through a DFT (i know there are faster FTs but our university told us we should use a DFT, they know why) of 800 samples. Out of the 800 samples selected, it calculates the frequency spectrum. Then the next for is used to calculate the absolute value of each frequency, and picks the largest, so it can be a good refrence point for a guitar tuner.

Momentairly, i have included in the main function just a large frequency condition to see if the LED lights up, but it doesn't.

I have tried switching on LEDs from 0 to 6 throughout the code and it seems to stop at F = computeDft();, so i removed the variable, and just let the computeDft(); run, but the next leds did not light up. Is the function never getting called? I have tried the function in Visual Studio with a generated cosine function and it works perfectly. It always detects the fundamental. Why doesn't it work in CVAVR?

#define M_PI 3.1415926f
#define N 800

unsigned char read_adc(void)
{
ADCSRA |= 0x40;  //start conversion;
while (ADCSRA&(0x40)); //wait conversion end
return (float)ADCH;
}

typedef struct 
{
    float re;
    float im;
} Complex;

float computeDft()
{      
    unsigned char x[N] = {0};
    float max = 0;   
    float maxi = 0;
    float magnitude = 0; 
    Complex X1[N] = {0};
    int n = N;
    int k;       
    for (n = 0; n < N; ++n)
    {
        for (k = 0; k < n; k++)
        {       
            x[k] = read_adc();            
            X1[n].re += x[k] * cos(n * k * M_PI / N);
            X1[n].im -= x[k] * sin(n * k * M_PI / N);
        }
    }                     
    for (k = 0; k < n; k++)  
    {
        magnitude = sqrt(X1[k].re * X1[k].re +  X1[k].im * X1[k].im);
        if (magnitude > maxi) 
        {
        maxi = magnitude;
        max = k;   
        }
    }                                             
    return max;   
}


/*
 * main function of program
 */
void main (void)
{          
    float F = 0;
    Init_initController();  // this must be the first "init" action/call!
    #asm("sei")             // enable interrupts
    LED1 = 1;               // initial state, will be changed by timer 1 
    L0 = 0;
    L1 = 0;
    L2 = 0;
    L3 = 0;
    L4 = 0;
    L5 = 0;
    L6 = 0;
    ADMUX = 0b10100111; // set ADC0
    ADCSRA = 0b10000111; //set ADEN, precale by 128

    while(TRUE)
    {
        wdogtrig();         // call often else processor will reset ;        
        F = computeDft();  
        if (F > 50 && F < 200)
        {
            L3 = 1;
        }
    } 


}// end main loop 

The result i'm trying to achieve is a signal from a phone or a computer (probably a YouTube video of a guy tuning his guitar) is sent through the jack to the processor in the AD converter (PINA.7). The main function calls the computeDft; function, which will ask the read_adc(); to add to x[k] the value of the voltage that is being sent through the cable, then compute it's Dft. The same function then selects the frequency of the fundamental (the one with the highest absolute value), then returns it. Inside the main function, a variable will be assigned the value of the fundamental, and through a series of ifs, it will compare it's value to the standard guitar strings frequencies of 82.6, 110, etc...

Answer 1

1. First of all: just picking the bigger harmonic in DFT, is not good as a tuner, since, depending on the instrument played, overtones may have a larger amplitude. The decent tuner may be done by using e.g. auto-correlation algorithm.

2. I see this line in your project:

 wdogtrig();         // call often else processor will reset ; 

Why you need the watchdog in the first place? Where it is configured? What timeout it is set for? How you think, how long would it take to perform both nested loops in computeDft()? With a lot of floating point operations inside including calculation of sine and cosine at each step? On a 16MHz 8-bit MCU? I think that will take several seconds at least, so do not use the watchdog at all, or reset it more often.

3. Look at

cos(n * k * M_PI / N);

(by the way, are you sure it is cos(n * k * M_PI / N); not cos(n * k * 2 * M_PI / N);?)

since cos(x) = cos(x + 2 * M_PI), you can see this formula can be expressed as cos((n * k * 2) % (2 * N) * M_PI / N). I.e. you can precalculate all 2*N possible values and put them as a constant table into the flash memory.

4. Look at nested loops in computeDft()

Inside the inner loop, you are calling read_adc() each time!

You want to pick the signal into the buffer once, and then perform DFT over the saved signal. I.e. first you read ADC values into x[k] array:

for (k = 0; k < N; k++)
{       
    x[k] = read_adc();            
}

and only then you perform DFT calculations over it:

for (n = 0; n < N; ++n)
{
    for (k = 0; k < n; k++)
    {       
        X1[n].re += x[k] * cos(n * k * M_PI / N);
        X1[n].im -= x[k] * sin(n * k * M_PI / N);
    }
}   

5. Look carefully at two cycles:

for (n = 0; n < N; ++n)
     ..
        X1[n].re += x[k] * cos(n * k * M_PI / N);
        X1[n].im -= x[k] * sin(n * k * M_PI / N);
}

Here at each step, you are calculating the value of X1[n], none of the previous X1 values are used.

And another loop below:

for (k = 0; k < n; k++)  
{
    magnitude = sqrt(X1[k].re * X1[k].re +  X1[k].im * X1[k].im);
    ...
}

here you are calculating the magnitude of X1[k] and no previous of next values of X1 are used. So, you can simply combine them together:

for (n = 0; n < N; ++n)
{
    for (k = 0; k < n; k++)
    {       
        X1[n].re += x[k] * cos(n * k * M_PI / N);
        X1[n].im -= x[k] * sin(n * k * M_PI / N);
    }
    magnitude = sqrt(X1[n].re * X1[n].re +  X1[n].im * X1[n].im);
    if (magnitude > maxi) 
    {
    maxi = magnitude;
    max = k;   
    }
}

Here you can clearly see, you need no reason to store X1[n].re and X1[n].im in any array. Just get rid of them!

for (n = 0; n < N; ++n)
{
    float re = 0;
    float im = 0;
    for (k = 0; k < n; k++)
    {       
        re += x[k] * cos(n * k * M_PI / N);
        im -= x[k] * sin(n * k * M_PI / N);
    }
    magnitude = sqrt(re * re +  im * im);
    if (magnitude > maxi) 
    {
        maxi = magnitude;
        max = k;   
    }
}

That's all! You have saved 6 KB by removing pointless Complex X1[N] array

6. There is a error in your initialization code:

ADMUX = 0b10100111; // set ADC0

I don't know what is "ATmega16P", I assume it works the same as "ATmega16". So most significant bits of this register, called REFS1 and REFS0 are used to select the reference voltage. Possible values are:

• 00 - external voltage from AREF pin;
• 01 - AVCC voltage taken as reference
• 11 - internal regulator (2.56V for ATmega16, 1.1V for ATmega168PA)

10 is an incorrect value.

7. the guitar output is a small signal, maybe several dozens of millivolts. Also, it is an AC signal, which can be as positive, so negative as well. So, before putting the signal onto MCU's input you have to shift it (otherwise you'll see only the positive half wave) and amplify it.

I.e. it is not enough just to connect jack plug to GND and ADC input, you need some schematics which will make the signal of the appropriate level.

You can google for it. For example this: (from This project)