Problem with ADC ISR when using multiple ADCx channels

Hello,

First of all, I am new to this forum, so sorry for possible vagueness and possible misleading AVR terms in my post, but I will try to do my best.

I am currently creating a device that would be able to detect the direction of incoming sound (sound source). This is just a part of final project, but I am stuck with this problem FOR DAYS…

I am using: Arduino Uno (ATMEGA328P), simple electret mics (~1, 5 cm), mic amps: LM358.

Since I am quite new at AVR, it took me awhile to figure out ways, how to read sound at fixed, and equal intervals using ISR (instead of using simple analogRead). I managed to setup registers and write code that was able to read data using only one mic. Below, I present my code (my description is below the code), which was modified a bit (code that is not important for this problem was removed).

#define SAMPLES_QTY	            64  // Qty of samples to be picked from ADC.
#define TIMER_INT			    64  // Timer 1 OCR. 64 cycles at 16Mhz -> 4us.	
#define CHANNELS				 3  // Mic number.

volatile uint8_t adcMSB = 0;
volatile uint8_t adcLSB = 0;	
volatile uint16_t acqSamples[CHANNELS];
volatile bool readCompleted;

double realData[CHANNELS][SAMPLES_QTY];
double imagData[CHANNELS][SAMPLES_QTY];

void setup()
{
	Serial.begin(115200);

	doSetup();
}

void loop()
{
	readData();
	
	//dummy loop
	for (int j = 0; j < SAMPLES_QTY; j++) {
	Serial.print((int)realData[0][j]);
	Serial.println(" ");
	}
}

void readData() { 
	for (int i = 0; i < CHANNELS; i++) {
		acqSamples[i] = 0;
		for (int j = 0; j < SAMPLES_QTY; j++) {
			realData[i][j] = 0;
			imagData[i][j] = 0;
		}
	}
	readCompleted = false;
	uint8_t originalTIMSK0 = TIMSK0;			// Recording Timer0 Mask;
	TIMSK0 = 0x00;								// Disable Timer0.

	TCNT1 = 0;
	//now_time = micros();
	TIMSK1 |= (1 << OCIE1B);	
	while (!readCompleted) {};
	TIMSK1 &= ~(1 << OCIE1B);
	//dur = micros() - now_time;
	TIMSK0 = originalTIMSK0;					// Enable back Timer0.
}

void doSetup() {
	// Setup Timer 1
	cli();
	TCCR1A = 0x00;
	TCCR1B = 0x00;
	TCCR1C = 0x00;
	TIFR1 = 0x00;           				   // Timer/Counter Int. Flag Register. to enable Out. Comp. A Match Flag: TIFR1 |= (1 << OCF1B);

	TCCR1A = ((0 << COM1A1) | (0 << COM1A0) |  // OFF. Compare Match Output, Timer 1, A1:A0 (control of OC1A pin).
			  (0 << COM1B0) | (0 << COM1B1) |  // OFF. Compare Match Output, Timer 1, B1:B0 (control of OC1B pin).
			  (0 << WGM11) | (0 << WGM10));    // Waveform Generation Mode, Timer 1, 0:1. Mode: CTC; TOP: OCRA. 
	
	TCCR1B = ((0 << ICNC1) |				   // Input Capture Noise Canceler, Timer 1, (used by ICP pin).
			  (0 << ICES1) |                   // Input Capture Edge Select, Timer 1, (used by ICP pin).
			  (0 << WGM13) |                   // Waveform Generation Mode, Timer 1, Mode: CTC; TOP: OCRA.
			  (1 << WGM12) |				   // Waveform Generation Mode, Timer 1, Mode: CTC; TOP: OCRA.	
			  (0 << CS12) | (0 << CS11) | (1 << CS10)); // Clock Select. Arduino UNO 16Mhz / 1 = 16 Mhz.
	
	TIMSK1 = 0x00;			    			   // Out. Comp. Match Int. to enable Out. Comp. Match A Int.: TIMSK1 |= (1 << OCIE1B);	
	TCNT1 = 0x00;							   // Timer/Counter.
	OCR1A = TIMER_INT;					       // Output Compare Value. 
	OCR1B = TIMER_INT - 2;
	
	// Setup ADC
	ADMUX = 0x00;
	ADCSRA = 0x00;
	ADCSRB = 0x00;
	
	ADMUX = ((1 << REFS0) |								  // Setting reference voltage (Vcc).
			 (0 << ADLAR) |								              // ADC right adjust (for ADCL and ADCH).
			 (0 << MUX3) | (0 << MUX2) | (0 << MUX1) | (0 << MUX0));  // Analog channel select: ADC0.
	
	ADCSRA = ((1 << ADEN)  |						        // Enable ADC. 
			  (1 << ADATE) |					            // Auto Trigger enable.
			  (1 << ADIE)  |							    // ADC Interrupt enable.
			  (1 << ADPS2) | (0 << ADPS1) | (1 << ADPS0));  // Setting prescaler 32 -> 500Khz ADC clock -> Fs = 38461Hz -> T = 26us.

	ADCSRB |= ((1 << ADTS2) | (0 << ADTS1) | (1 << ADTS0)); // ADC auto trigger source selection -> Timer 1 Compare Match B selected.

	DIDR0 = 0x00;											// Turn Off Digital Input Buffer.
	sei();
}

ISR(TIMER1_COMPB_vect) {
	// Invoked on OCR (CTC).
}

ISR(ADC_vect) {
		// If the result is left adjusted (ADLAR) and no more than 8-bit precision is required, it is sufficient to read ADCH, 
		// otherwise, ADCL mus be read first, then ADCH. 
	if (!((acqSamples[0] == SAMPLES_QTY) && (acqSamples[1] == SAMPLES_QTY) &&  (acqSamples[2] == SAMPLES_QTY))) { 
		adcLSB = ADCL;
		adcMSB = ADCH;
		switch (ADMUX) {
			case 0x40: // ADC0  
				realData[0][acqSamples[0]] = ((adcMSB << 8) | (adcLSB & 0xff)) - 327;
				acqSamples[0] += 1;
				ADMUX = 0x41;
				break;
			case 0x41: // ADC1 
				realData[1][acqSamples[1]] = ((adcMSB << 8) | (adcLSB & 0xff)) - 327;
				acqSamples[1] += 1;
				ADMUX = 0x42;
				break;
			case 0x42: // ADC2 
				realData[2][acqSamples[2]] = ((adcMSB << 8) | (adcLSB & 0xff)) - 327;
				acqSamples[2] += 1;
				ADMUX = 0x40;
				break;	
		}  
	} else readCompleted = true;
}

IMPORTANT: I know that assigning zero bit values is useless, but I did that on purpose, because that is clearer to me. Moreover, some code comments might be incorrect, because I might have made some adjustments and forgot to update comments…

I have chosen to use ADC Auto Trigger (ADATE = 1) mode that will be attached to Timer 1 compare match B event. I have chosen CTC mode for my Timer 1, I have also selected a pre-scaler of 1, so that means I am keeping clock speed at 16Mhz. Since CTC mode compares only OCR1A value and Auto Trigger mode is attached to B event, I set OCR1B to a less value (i.e. -2). I am not quite sure whether that is mandatory, because sometimes I could not feel any difference… Then I setup ADC. I setup reference voltage (Vcc), then I select initial ADC0 port (which is not, obviously mandatory since I cleared all values of this register just above). Then I enable ADC, switch Auto-Trigger mode on, enable ADC interrupt, setting pre-scaler to 500Khz, which should provide me with ~38461 sampling rage (500k/13) [when there is only one mic of course]. Finally I switch off digital filters (I found that as a good habit in such cases) and turn on global interrupts again.

So that’s how I understand it “should” work from my POV: every time Timer1 compare match B event occurs, ADC conversion starts, while it is happening, Timer1 compare match B event will occur (my TIMER_INT is equal to 64 so that leads to a period of 4us while my sampling frequency is just 26us) and ADC will just ignore this event and carry on its conversion. After ADC finishes current conversion, ADC ISR is executed. ADC ISR is supposed to be handing multiple channels, so there, I read ADCL and ADCH values, compare, check WHAT channel is currently active, assign data to array and switch to another ADC channel. I do that for three channels and ADC conversion is stopped, when 64 samples are taken for each array/channel.

HOWEVER, that does not work properly. Although each mic is assigned to a corresponding ADCx channel, it seems, that the order and data is totally ruined. For example, when I switch my first mic into ADC0 channel (others are pulled out completely), read ADC0 data (I play 2khz sound very close to the mic), loop it out realData[0], and I see that it looks like being good. When I switch the same mic into ADC1 channel (other are still out), read data, and loop the data of realData[1], it looks like good, BUT… BUT, when I decide to loop out the data of ADC0 channel (realData[0]) (my mic is still attached to ADC1), IT DOES SHOW SIMILAR DATA TO realData[1], which is total non-sense… and therefore I cannot figure out the order of my mics and my thing does not work. Looks like data is totally ruined and incorrect… I do not know what is wrong here: Does my code have silly bugs? Is this ADC conversion approach for multiple channels correct? Do I read it properly? Is there a simpler way of doing this?

Guys, it just drives me crazy… :slight_smile: just a week ago, I knew nothing about AVR registers, internal functionality etc. and I really pushed myself forward, but these things are new to me, so I migh have understood quite a few things not in a way as I was supposed to.

THANKS for your help and patience.

Your code is too complex to figure out quickly (which is not meant as a criticism) - just that I'm lazy.

Have you read the section of the Atmega328 datasheet that explains the ADC?

There is only one ADC and when you switch channels you need to leave time for the new channel to settle. Might thatReply be part of your problem?

I think the Arduino can do about 10,000 samples per second - I presume that's on one pin. If you switch between pins the total will be far lower and, of course, will be shared among the pins.

If that suggestion is irrelevant, perhaps you could describe in English what your code is trying to do.

...R

Hi, thanks for your answer.

Yes, I have read 328 datasheet, I spent a bunch of time studying it. Although there are some unclear areas to me.

Anyway, I have described my code a little bit below the code text in my previous post, but I will try to do my best and provide more detailed description then :slight_smile: I will try to write a comment for each code line.

#define SAMPLES_QTY	            64  // Qty of samples to be picked for each microphone from each ADC channel (FFT will be done later)
#define TIMER_INT			    64  // This defines TImer 1 compare match B event rate: 1/16000000 * 64 = 4us. I just though that would be enough. I do not have substantiation for this - it was based on trial and error. As mentioned, CTC of Timer 1 should trigger ADC conversion.
#define CHANNELS				 3  // Mic number. Self explaining: that is numbers of mics - channels. 

volatile uint8_t adcMSB = 0;   // This variable is used to store high bit of converted ADC data (ADCH)
volatile uint8_t adcLSB = 0;	// This one is for ADCL (low bit).
volatile uint16_t acqSamples[CHANNELS]; // This array stores the number of acquired samples in each channel. i.e. acqSamples[0] stores how many samples are in zero channel, acqSamples[1] in second etc. All in all it is just a simple counter of elements in each dimension of data array.
volatile bool readCompleted;   // It is a flag that should be switched on if data for all channels has been fully acquired.

void loop()
{
	readData();  // Function that runs data read.
	
	//dummy loop. This loop is for testing purposes - to bring out data.
	for (int j = 0; j < SAMPLES_QTY; j++) {
	Serial.print((int)realData[0][j]);
	Serial.println(" ");
	 }
}

/* This function is executed before main loop. It is for setting register values, configuring ADC and Timer 1. I HAVE DELETED ALL CODE WHERE ZEROS WERE ASSIGNED, SO IT SHOULD BE CLEARER NOW :) */
void doSetup() {
	// Setup Timer 1
	cli();     // Disabling global interrupts.
	TCCR1A = 0x00;   // clearing desired registers.
	TCCR1B = 0x00;
	TCCR1C = 0x00;
	TIFR1 = 0x00;           				  

	TCCR1B = ((1 << WGM12) |	// Mode: CTC; TOP: OCRA.	
			  (0 << CS12) | (0 << CS11) | (1 << CS10)); // Clock Select. Arduino UNO 16Mhz / 1 = 16 Mhz.
	
	TIMSK1 = 0x00;			    			   	
	TCNT1 = 0x00;						     
	OCR1A = TIMER_INT;	// Output Compare Value.  This value resets TCNT, but compare match will be based on OCR1B, so I set  latter one a bit lower value (again -  based on trial and error - not know if its OK).
	OCR1B = TIMER_INT - 2;
	
	// Setup ADC
	ADMUX = 0x00;   //clearing registers.
	ADCSRA = 0x00;
	ADCSRB = 0x00;
	
	ADMUX = (1 << REFS0); // Setting reference voltage (Vcc).
				
	ADCSRA = ((1 << ADEN)  | // Enable ADC. 
			  (1 << ADATE) | // Auto Trigger enable.
			  (1 << ADIE)  |	 // ADC Interrupt enable.
			  (1 << ADPS2) | (0 << ADPS1) | (1 << ADPS0));  // Setting prescaler 32 -> 500Khz ADC clock -> Fs = ~38461Hz -> T = 26us.

	ADCSRB |= ((1 << ADTS2) | (0 << ADTS1) | (1 << ADTS0)); // ADC auto trigger source selection -> Timer 1 Compare Match B selected.

	DIDR0 = 0x00; // Turn Off Digital Input Buffer.
	sei();  //enable global interrupts again.
}

/* this function is for launching data acquisition process */
void readData() { 
        readCompleted = false;  //This flag is modified in ADC ISR event - to stop ADC conversion when all arrays from all channels are filled.
	// Here I disable system timer 0, just to improve perfomance. I did not notice any malfunctions, because of this.
        uint8_t originalTIMSK0 = TIMSK0;			// Recording Timer0 Mask;
	TIMSK0 = 0x00;								// Disable Timer0.

	TCNT1 = 0;  // clearing timer counter.
	TIMSK1 |= (1 << OCIE1B);	// enabling interrupts - starting timer 1.
	while (!readCompleted) {};  // loop which is executed until ADC ISR marks readCompleted flag - until all arrays ar filled.
	TIMSK1 &= ~(1 << OCIE1B); //stopping timer 1.
	TIMSK0 = originalTIMSK0;	// Enable back Timer0.
}

/* ADC ISR event. It should be executed each time Timer1 compare match B event is triggered and ADC is in "idle" state (there are no conversions in progress) */
ISR(ADC_vect) { 
	  // First line checks acqSamples array, which holds the number of elements acquired from each channel. If all required number of samples is collected, readCompleted flag is marked and process is stopped in readData() function above.
           if (!((acqSamples[0] == SAMPLES_QTY) && (acqSamples[1] == SAMPLES_QTY) &&  (acqSamples[2] == SAMPLES_QTY))) { 
		adcLSB = ADCL;  // Reading appropriate bits from ADC
		adcMSB = ADCH;
		switch (ADMUX) {  // Checking, which ADC channel is currently active. Since I have set 6th bit of ADMUX (Vcc), so initially I have 0100000 - that is 0x40 in hex, if I change channel to 1, I have 01000001 -  that is 0x41 etc. So in this way I check which channel is active,  and so on. 
			case 0x40: // ADC0  
				//Insert acquired sample into appropriate array (i.e. realData[0] corresponds to zero channel, realData[1] to first ADC channel etc.
                                realData[0][acqSamples[0]] = ((adcMSB << 8) | (adcLSB & 0xff)) - 327; // I combine Low and High bits into 16bit number.
				acqSamples[0] += 1; //increase the total number of samples acquired for this channel (in this case zero channel).
				ADMUX = 0x41;  // SWITCH to NEXT channel.
				break;  
			case 0x41: // Do all of the work that was described above for ADC1. 
				realData[1][acqSamples[1]] = ((adcMSB << 8) | (adcLSB & 0xff)) - 327;
				acqSamples[1] += 1;
				ADMUX = 0x42; // Settting next channel ADC2
				break;
			case 0x42: // Do all of the work for ADC2.
				realData[2][acqSamples[2]] = ((adcMSB << 8) | (adcLSB & 0xff)) - 327;
				acqSamples[2] += 1;
				ADMUX = 0x40; //GOING BACK TO ADC0!!!!!!!!!!!!! and then loop starts again -> Adc0, adc1, adc2 -> adc0 etc.
				break;	
		}  
	} else readCompleted = true;  // If all arrays have required quantity of sampled data - process is stopped.
}

ISR(TIMER1_COMPB_vect) {
	// Invoked on OCR (CTC).
}

This code might look complicated, but it is not so difficult actually. It is also difficult to believe that arduino is capable of grabbing just 10000 samples/s. What is the reason for that? Why is it so low compared to the rates specified in datasheet then? I have read A LOT on the internet about ISR and timers and most of them provides comments based on 328 datasheet and do not specify some sort of arduino limitations. What do you mean by “settling channel”?

Well, I was thinking about reading the data in succession (read all of the data for adc0, then for adc1 etc.) and that might work, but I am not sure whether my phase difference calculations will do OK, when there will be such delay between sampling from each mic - so I though sampling each sample from all mics “at the same time” should provide me better results. HOWEVER… if you could suggest me simpler way of doing this, I can rewrite code completely and do everything from scratch - that is not an issue. I just want to get correct data :slight_smile:

Thank you very much. If you have any questions - do not hesitate and ask :slight_smile: I really appreciate your help.

t is also difficult to believe that arduino is capable of grabbing just 10000 samples/s

It's goverened by the ADC clock speed. You can push it higher, at the risk of reduced resolution or increased noise.

Yes, so that is why, I selected 500Khz ADC clock rate, which should provide me ~38460 sampling freq. In such way I would be able to detect frequencies up to 38460/2 = ~19230 Hz (with a single mic). But as my problem states, I have issues reading multiple channels.

In such way I would be able to detect frequencies up to 38460/2 = ~19230 Hz (with a single mic)

...on a single channel. You have only one ADC

The data sheet says it takes 13 cycles of the ADC clock to do one conversion (that's where you got 38460 from 500kHz). I think, if you stick to a single channel it can do a new conversion every 13 cycles, but I'm not sure if you need to allow time to access the data. (I don't remember how many bits of precision you will get at that rate).

I may have been wrong to use the word settling - that may only apply if you change the reference voltage. But you can't switch channels for free either.

If you are running at full speed then (as far as I understand the datasheet) when you change channel the first reading on "new" channel will actually be the residual data from the previous channel - which means you must ignore the first reading after a channel change - which effectively halves the total sampling rate. So, if you are getting 38kSps on one channel the most you will get is 19k on two channels - i.e. 9.5k per channel.

There may be some useful stuff in this link http://www.instructables.com/id/Girino-Fast-Arduino-Oscilloscope/

You may need one or more faster external ADCs for what you want to do. ...R