Problem with an ISR

Hello,
I wrote this sketch ( autocorelation code comes from an instructable) because I need to calculate audio input signals frequencies "non-stop". So I use the ISR(ADC_vect) (known as conversion complete interrupt) to fill-up th array "rawData". When it's full, the arduino launchs the autocorelation to calculate the signal period and then I it determines its frequency and during that time I disable all interrupts. Next, I re-enable interrupts and the array is re-filled with news incoming datas (the array is refreshed).

BUT it doesn't work ! The arduino calculate the frequency once and it never refresh the array or I don't known why but if I change the signal's frequency nothing happens. My LCD is freezed...

I tested my sketch on Proteus only...do you believe that the "error" comes from me or from Proteus ?

If I comment the lines where I disabled and then re-enable interrupts it works but it gives unwanted interrupts which...interrupts the program every 26µs (the sample rate period)...

//Algo d'autocorrelation tiré d'un instructable : http://www.instructables.com/id/Reliable-Frequency-Detection-Using-DSP-Techniques/
#include <LiquidCrystal.h>;
LiquidCrystal lcd(12, 11, 5, 4, 3, 2);

/************Variables*****************/

////Variables relatives à la copie du signal
const int nbData = 800; // nombres d'échantillons du signal
char rawData[nbData] = {}; //tableau de stockage de la copie du signal
volatile unsigned int index = 0;

float frequency = 0;//variable pour stockage de la frequence mesurée

/*******Pour l'autocorrelation *************/
float sample_freq = 38461.54;//fréquence d'échantillonnage en Hz, 1 conversion toutes les 52µs

 
void setup() {
  lcd.begin(16,2);

  cli();
  
  ADCSRA = 0;//initialisation des registres ADCSRA et ADCSRB pour le CAN
  ADCSRB = 0;
  //mettre le voltage de référence
  //Alignement à gauche des bits -> resultat de la conversion dans registre ADCH
  ADMUX |= 0x60;
  //parametrage des prédiviseurs d'horloge pour le CAN à un prediviseur de 64 - 16mHz/64=250kHz = f_horlogeCAN
  //active le déclenchement automatique en gros dès que la CAN précédente est finie, on en recommnce direct une autre
  //active l'interruption quand toutes les mesures sont faites
  //autorise la conversion
  ADCSRA |= (1<<ADEN) | (1<<ADATE) | (1<<ADIE) | (1<<ADPS0) | (1<<ADPS2); 
  ADCSRA |= (1<<ADSC);//démarre la conversion (starts the ADC measurment)
  sei();
}

ISR(ADC_vect){//éxecute le code à chaque fin de conversion A/N
  if(index < nbData){//Si on a pas encore rempli un taleau de 800 échantillons
    rawData[index] = ADCH;//mettre la tension mesurée dans le tableau
    ++ index;
  }
}
void loop(){
  
  if(index == nbData){// si j'ai mes 400 valeurs
    
    //uint8_t saveSREG = SREG; //copie du registre pour pouvoir le restaurer après avoir traité l'admf
    cli(); //désactiver toute interruption
    
    frequency = runAutocorrelation();
    lcd.setCursor(0,0);
    lcd.print(frequency);
    index = 0;
    
    sei();
    //SREG = saveSREG; //restaurer le SREG et re autoriser les interruptions  
  } 
}
////////////////////////////////////////////////////////////////////////////////////////////////////////
////////////////////////////////////// Autocorrelation /////////////////////////////////////////////////
////////////////////////////////////////////////////////////////////////////////////////////////////////

float runAutocorrelation(){
  
  int i,k;//compteurs
  long sum, sum_old;//résultats de correlation
  float thresh = 0;
  int period = 0;
  byte pd_state = 0;
  float freq_per = 0;
  sum = 0;
  pd_state = 0;
  
  // Autocorrelation
  for(i=0; i < nbData; i++)
  {
    sum_old = sum;
    sum = 0;
    for(k=0; k < nbData-i; k++) sum += (rawData[k])*(rawData[k+i])/256;
    
    // Peak Detect State Machine
    if (pd_state == 2 && (sum-sum_old) <=0) 
    {
      period = i-1;//à l'origine c'etait juste period = i mais c'est plus precis comme ça...
      pd_state = 3;
    }
    if (pd_state == 1 && (sum > thresh) && (sum-sum_old) > 0) pd_state = 2;
    if (!i) {
      thresh = sum * 0.5;
      pd_state = 1;
    }
  }
  // Frequence mesurée en Hz
  freq_per = sample_freq/period;
  return freq_per;
}

I'm sorry for my bad english...I'm a french student

Hugo

What is Proteus? Is that an Arduino Simulator? If so, don't trust it - microcontroller simulators are often really bad.

Don't leave interrupts globally disabled - instead enable and disable the ADC interrupt only.
Lots of things stop working if you leave interrupts globally disabled.

You can see the Proteus window in the attachement :wink: It's a global electronics simulator and I found the arduino Uno modelisation on the net...
I though that it would be better to disable all interrupts because this sketch is a part of a bagpipes tuner project in which are externals interrupts because of 3 push buttons. And these musn't disrupt the frequency processing.

Can I do it without using the I-bit in SREG ? Indeed if I had the ADC interrupt only, I would have set the ADEN bit in ADCSRA

Over in the CCS PIC processor forums, they state right up at the top "PIC 101 - NO more Proteus questions". That software is notorious for "working" without things like power, ground etc. It is also often guilty of doing things significantly different than the real hardware. The general consensus over there is "get some real hardware and work with that". Especially these days when you can get a little Uno board for a very reasonable price and it is easy to work with, try code on etc. (I do miss the days of wire wrapped boards full of logic chips and having to run the 2716 EPROM through the "amnesia machine" (UV EPROM eraser). Still have a pad of conductive foam with a bunch of 2716, 2732 and even some 2764's on it !!

Ok, I have already a UNO board but I haven't the necessary aditionnal electronics to try my code in good conditions....However, thank's for your explaination :slight_smile:
So is there an error in my code ? Could it work properly in reality or my program will be freeze up as proteus does ?

I do miss the days of wire wrapped boards full of logic chips

You need a psychiatrist! 8)

during that time I disable all interrupts.

Why? Lots of things rely on interrupts functioning ALL THE TIME.

If I comment the lines where I disabled and then re-enable interrupts it works but it gives unwanted interrupts which...interrupts the program every 26µs

Well, there's a clue.

Stop disabling interrupts. Copy the data that the ISR manipulates and work with the copy.

All variables used in an ISR and in other places need to be volatile.

What do you mean ? the array rawData should be volatile too ??? :o

And to answer your question, I disabled ALL interrupts because I have other interrupts related to push buttons and I want to process my frequency detection in one time (to spend time...if it does :-X )

What do you mean ? the array rawData should be volatile too

Yes.

I have other interrupts related to push buttons

Properly written code does not need to use interrupts to deal with switches.

I want to process my frequency detection in one time

That doesn't make sense. Something is happening at some frequency that you care about. Twiddling your thumbs waiting for some time to pass, while an interrupt handler gets called over and over to count the number of events doesn't make sense.