Hi, guys! I'm new around here and I hope I got the right section for my questions.
I'm working on a project in which I want to build a doll which dances to a random music which the user plays. I got my electret mic+amp working and first tried to use the fht library. All worked well till I've started wondering about the bins frequency range. So my idea is to get the rhytm of the song (30-500 hz) and by processing the period, to give a command to the motors. But sadly, I couldn't get good results with the FFT since the freq/bin is kinda large so i got to the Goertzel algorithm. My question to you guys is if you could take a look at my code below and tell me if I did something wrong since i get kinda weird results for the magnitudes at certain freqs like for 120 150 180 hz i get the same value and something like -20000.
Don't know if I got the reading right for the ADC
Don't know if my logic for the program is right
New to audio processing ^.^
Thanks in advance!
int v[320];
int N,k;
float coeff,cosine,sine,w,Q1,Q2,Q0,m[17];
int sample_rate=9600;
int freq[17];
byte mb[17];
void initADC()
{
ADMUX = 0x60; // set ADLAR to 1 so i get to read only the ADCH and use A0 for readings
ADCSRA = 0x07; //ADSC=0,ADEN=0, 125kHZ ADC clock freq => around 9600 samples/s
DIDR0 = 0x01;
ADCSRB = 0x00; //set free running mode for the ADC
}
void setup()
{
Serial.begin(115200);
initADC();
N=320;
/* I want 320 samples so i get a freq of 30 Hz/bin at the sample rate of 9600 (16 MHz/(128*13 clock ticks for a conversion))(hope it's the right logic)
*/
for(int i=0;i<17;i++)
freq[i]=(i+1)*30; //Sets the array of frequencies to be tested
}
void adcread()
{
ADCSRA=0xC7; // set ADSC, ADEN to 1 so the ADC starts converting
byte jH = ADCH; //get the first element converted by the ADC
v[0]=int(jH); //put the element into the sample array
for(int i=1;i<320;i++)
{
jH = ADCH; // gets the converted values of the ADC and puts them into the sample array
v[i] = int(jH); //
}
ADCSRA=0x07; //turn the ADC off
}
/*
the next function is the groetzel algorithm simplified as i found it
in a tutorial
*/
float groetzel(int target_freq)
{
int mag;
k=int(0.5+(N*target_freq/sample_rate));
Q1=0;Q2=0;
w=(2*3.14159/N)*k;
cosine=cos(w);
sine=sin(w);
coeff=2*cosine;
for(int i=0;i<320;i++)
{
Q0=coeff*Q1-Q2+v[i];
Q2=Q1;
Q1=Q0;
}
mag=sqrt(pow(Q1,2)+pow(Q2,2)-Q1*Q2*coeff);
return mag;
}
void loop()
{
adcread();
for(int i=0;i<17;i++)
{m[i]=groetzel(freq[i]); //for each frequency the samples collected are tested
}
}
First, I'll recommend that you Serial.print() your ADC data, and see if it looks like what you expect. I suspect that it doesn't. Here's why:
I don't see that the ADC is set to autotrigger. True, free-running mode is selected in ADCSRB, but ADATE - ADC Auto Trigger Enable - is never set in ADCSRA. That means that the ADC will run one conversion, and stop. I'd expect that you get the same data for every reading; I'd be surprised if you got more than a very few independent values in your array.
I don't see anything that waits for the ADC conversion to complete. There's no necessity to wait, of course, if you use the ADC interrupt to alert the processor that an ADC reading is ready. But, you don't use that interrupt. Best I can tell, the code initiates a single conversion, and then reads the ADC as fast as it can until it's done it 320 times. Somewhere in there, the conversion may finish, and the latter part of the data may all contain the result of the same conversion.
Look at your data. Try adding this code at the end of loop(), just before the final "}":
for (int k=0;k<320;k++) {
Serial.println(v[k]);
}
for (;;);
I'll recommend that you separate this effort into two parts: Getting the Goetzel algorithm to work, and getting good samples from the ADC. For the ADC, I'd recommend that you do it by writing an ADC ISR that fetches data, stores it in the array, bumps the counter, checks to see if it's acquired enough samples, turns off the ADC interrupt if it has, and exits. When it's done, Serial.print() the data, and make sure it's reasonable given the state of the ADC input. The ADC needs to be in free-running mode, with the ADC interrupt enabled, and with autotrigger enabled. You'll still have to start the first conversion. Given that you're directly manipulating the ADC control and status registers, it appears that you know where to find information about how the ADC works. You just need to read the rest of it.
I'll leave the Goetzel algorithm to you. I'd recommend testing it with simulated data, until you're confident that it works.
Thanks for the advice. I was thinking about getting an ISR sequence so I get more control on the conversion, but some lines in the manual got me confused (I'm using a Mega 2560): "Using the ADC Interrupt Flag as a trigger source makes the ADC start a new conversion as soon
as the ongoing conversion has finished. The ADC then operates in Free Running mode, constantly
sampling and updating the ADC Data Register. The first conversion must be started by
writing a logical one to the ADSC bit in ADCSRA. In this mode the ADC will perform successive conversions independently of whether the ADC Interrupt Flag, ADIF is cleared or not.
If Auto Triggering is enabled, single conversions can be started by writing ADSC in ADCSRA to
one. ADSC can also be used to determine if a conversion is in progress. The ADSC bit will be
read as one during a conversion, independently of how the conversion was started.", so I thought that if I'm using a free running mode i won't be needing the interrupt flags nor the auto trigger to be set. I will try converting using an ISR function like:
I'm not sure if the interrupt flag automatically clears when the ISR was executed or if I should clear it. I'm going to try the steps you advised me on. Hopefully it'll work. Thanks again!
Check 26.8.3 in the 10/2012 edition of the ATmega2560 datasheet, describing ADC control/status register A, ADCSRA. It states that the interrupt flag is cleared by hardware when executing the interrupt handling vector; there's no requirement for the code to manage it. The same section states that ADATE enables autotriggering when it's set; ADCSRB selects the trigger source, but doesn't enable autotriggering.
As for your code, I'd suggest:
Storing ADCH in an intermediate variable is unnecessary.
There's no need for conversion to int. ADCH is already unsigned char, or byte.
Clearing ADSC won't do anything. The datasheet says, of ADSC, "Writing zero to this bit has no effect." I'd suggest instead that you clear ADATE, stopping the autotrigger, and ADIE, shutting down the interrupt. If you clear ADATE only, you'll get one more ADC interrupt after the last value and maybe overwrite something outside the array, and that will be very hard to troubleshoot. If you just shut down the interrupt, you won't hear from it again, but you'll still be running conversions all the time, even though the data is unused.
I'd probably write it like this, making a local copy of the counter to avoid having to load the value of the volatile variable every time I look at it:
volatile unsigned int counter;
...
void ISR(ADC_vect)
{
unsigned int counterCopy;
if (counterCopy < 320) {
v[counterCopy++]=ADCH;
}
if (counterCopy >= 320) {
cbi(ADCSRA, ADIE);
}
counter = counterCopy;
}
I got it working, thanks again! I'll post the reading sequence of the ADC and the ISR routine if anyone will need help in the future. Seems like the variables that are modified inside the ISR routine have to be volatile.
volatile byte v[96];
volatile int counter=0;
volatile byte jL,jH;
void initADC()
{
//The program will get samples in free running mode from ADC0, with the ADC freq of 125kHz (prescaler set to 128)=>9600 samples/s
ADMUX = 0x40;
ADCSRA=0xAF;
DIDR0 = 0x01;
ADCSRB=0x00;
}
void ADCread()
{
counter=0;
ADCSRA=0xEF; //Start the first conversion
jL=ADCL; //get the data stored in ADCL and ADCH. If ADLAR is 0, you have to first read the ADCL and then ADCH
jH=ADCH; // If you don't need more than 8 bits, you can set the ADLAR to 1 in ADMUX register and read the ADCH only
v[counter++]=(jH << 8) | jL; //put the bytes in ADCH and ADCL together
}
ISR(ADC_vect)
{
//this routine fills up the rest of the buffer
if(counter<96)
{
jL=ADCL;
jH=ADCH;
v[counter++]=(jH << 8) | jL;
}
if(counter>=96)
ADCSRA=0xC7; //turn the interrupt flag and auto trigger off
}
jL=ADCL; //get the data stored in ADCL and ADCH. If ADLAR is 0, you have to first read the ADCL and then ADCH
isn't exactly correct. Whatever the state of ADLAR, if you're going to read ADCL at all, you have to read it first. Once you read ADCL, you have to go on and read ADCH, whether you need the data or not. ATmega2560 datasheet, 10/2012, 26.2.
Just for fun, try this: v[counter++] = ADC;and see it that works.
Seems like the variables that are modified inside the ISR routine have to be volatile.
I'll post the reading sequence of the ADC and the ISR routine if anyone will need help in the future. Seems like the variables that are modified inside the ISR routine have to be volatile.