Go Down

Topic: Problem with multi-channel ADC interrupts (Read 581 times) previous topic - next topic

pilant

I've got some code using a single channel ADC interrupt handler working fine.  However, when I attempt to add support in the ISR for another ADC channel, it appears no interrupts are generated.  Here is the code for the ADC interrupt setup and ISR:

Code: [Select]

#include <Arduino.h>

#include "sna_adc.h"
#include "sna_keypad.h"

volatile int analogValue1;
volatile int analogValue2;
volatile int adcChan;

// Function:    snaAdcIntSetup()
// Description: This funtion sets up the ADC for generating interrupts.
// Arguments:   None
// Returns:     None
void snaAdcIntSetup() {
  // Set for the first ADC channel.
  ADMUX = ADCH1;

  // Set ADEN, ADATE, prescale (128), and enable interrupts.
  ADCSRA = (1 << ADEN) | (1 << ADATE) | (1 << ADPS2) | (1 << ADPS1) | (1 << ADPS0) | (1 << ADIE);

  // Set free running.
  ADCSRB = 0;

  // Enable global interrupts.
  sei();

  // Kick off the first ADC.
  analogValue1 = 0;
  analogValue2 = 0;

  // Set ADSC to start ADC conversion.
  ADCSRA |= (1 << ADSC);
}

// The ADC interrupt function.
ISR(ADC_vect) {
//  int adcChan;
  int tempVal;

  // Get the ADC channel causing the interrupt.
  adcChan = ADMUX & ((1 << MUX3) | (1 << MUX2) | (1 << MUX1) | (1 << MUX0));

  // Read low first
  tempVal = ADCL | (ADCH << 8);

  switch (adcChan) {
    case ADC1:
      // If quiescent, ready to read
      if (tempVal == 0) {
        // Because of the continuous use of the ADC, only keep "real" values.
        if (analogValue1 > ADC_NOISE_VALUE) {
          snaKeypadPutValue(analogValue1);
          // Don't keep anthing until another key pressed.
          analogValue1 = 0;
        }
      }

      // If the analog value just read is greater than the previously read, save it.
      // Otherwise ignore it; assuming the key is just being pressed and low pressure
      // might cause the value to be low.
      if (tempVal > analogValue1) {
        analogValue1 = tempVal;
      }

      // Set up for ADC Channel 2.
//      ADMUX = ADCH2;
      break;
    case ADC2:
      analogValue2 = tempVal;

      // Set up for ADC channel 1.
      ADMUX = ADCH1;
      break;
  }
  // Dismiss the interrupt.
}


Here is the header file, sna_adc.h:

Code: [Select]

#ifndef __SNA_ADC_H__
#define __SNA_ADC_H__

#define ADC_NOISE_VALUE 10

#define ADC0 0
#define ADC1 1
#define ADC2 2
#define ADC3 3
#define ADC4 4
#define ADC5 5
#define ADC6 6
#define ADC7 7

// Define the ADC channels used.  ADLAR will be zero.
#define ADCH1 ((1 << REFS0) | ADC1)
#define ADCH2 ((1 << REFS0) | ADC2)

extern volatile int analogValue1;
extern volatile int analogValue2;
extern volatile int adcChan;

void snaAdcIntSetup();

#endif // __SNA_ADC_H__


With the line "ADMUX = ADCH2;" commented out, the first channel works as expected.  It is connected to a "one-wire" 4x3 keypad.  The snaKeypadPutValue() call is actually a macro which places the value received into a ring buffer for later processing.  This all works great.

However, when I uncomment this line to enable the second ADC channel, running the code with the second channel floating results in no interrupts.  When I ground, connect to 3.3V or 5.0V lines, I start getting interrupts on the first channel with fairly low values (< 20).  I don't yet know if I'm getting any interrupts on the second channel.  Curiously, the interrupts continue even when the second channel is unconnected (to be floating again).

It almost seems a though I've got some sort of timing / order / sequencing issue, but from the examples I was able to find for using multiple ADC channels, what I have *should* work.  (I know, famous last words  :P )

Any help appreciated.

- Mark

Peter_n

#1
Feb 23, 2015, 12:49 am Last Edit: Feb 23, 2015, 12:50 am by Peter_n
Do you redefine ADC0 ... ADC7 ?
The MUX0...MUX3 for the ADMUX and be used as integer for the channel.
Instead of (1<<REFS0) you can use _BV(REFS0) or bit(REFS0).
Code: [Select]

ADMUX = bit(REFS0) | 2;      // channel 2


I remember that a certain sequence of code was needed to start the ADC. Or was that for a timer, or perhaps only for the ATmega8. I'm not sure anymore...

Can you make a test sketch for the Arduino Uno ?

pilant

I defined ADC0 - 7 because I couldn't find them in a header file.  If they exist, I have no problem using them.  I had seen the _BV, but had not investigated if it was available for use.  Although, as I understand it, _BV is little more than a macro to do the shifting I did inline.

The last thing snaAdcIntSetup() does is set ADSC to start the conversion.  As the ADC is set up to be free running, it should not need to be set again.  At least, that is how I interpret the Atmel spec sheet.

The source code I included is part of a larger sketch; probably 1500 - 2000 lines altogether.  Here is a stripped down sketch which should demonstrate the issue:

Code: [Select]

#include <Arduino.h>

#define ADC_NOISE_VALUE 10

#define ADC0 0
#define ADC1 1
#define ADC2 2
#define ADC3 3
#define ADC4 4
#define ADC5 5
#define ADC6 6
#define ADC7 7

// Define the ADC channels used.  ADLAR will be zero.
#define ADCH1 ((1 << REFS0) | ADC1)
#define ADCH2 ((1 << REFS0) | ADC2)

volatile int analogValue1;
volatile int analogValue2;
volatile int adcChan;

void setup() {
  Serial.begin(9600);
 
  snaAdcIntSetup();
}

void loop() {
  int oldAnalogValue1;
  int oldAnalogValue2;
 
  oldAnalogValue1 = 0;
  oldAnalogValue1 = 0;

  while (1) {
    // If the first channel value has changed, print it out.
    if (analogValue1 != oldAnalogValue1) {
      Serial.print("Value1: ");
      Serial.println(analogValue1);
      oldAnalogValue1 = analogValue1;
    }

    // If the second channel value has changed, print it out.
    if (analogValue1 != oldAnalogValue1) {
      Serial.print("Value1: ");
      Serial.println(analogValue1);
      oldAnalogValue1 = analogValue1;
    }
  }
}

// Function:    snaAdcIntSetup()
// Description: This funtion sets up the ADC for generating interrupts.
// Arguments:   None
// Returns:     None
void snaAdcIntSetup() {
  // Set for the first ADC channel.
  ADMUX = ADCH1;

  // Set ADEN, ADATE, prescale (128), and enable interrupts.
  ADCSRA = (1 << ADEN) | (1 << ADATE) | (1 << ADPS2) | (1 << ADPS1) | (1 << ADPS0) | (1 << ADIE);

  // Set free running.
  ADCSRB = 0;

  // Enable global interrupts.
  sei();

  // Kick off the first ADC.
  analogValue1 = 0;
  analogValue2 = 0;

  // Set ADSC to start ADC conversion.
  ADCSRA |= (1 << ADSC);
}

// The ADC interrupt function.
ISR(ADC_vect) {
//  int adcChan;
  int tempVal;

  // Get the ADC channel causing the interrupt.
  adcChan = ADMUX & ((1 << MUX3) | (1 << MUX2) | (1 << MUX1) | (1 << MUX0));

  // Read low first
  tempVal = ADCL | (ADCH << 8);

  switch (adcChan) {
    case ADC1:
      // If quiescent, ready to read
      if (tempVal == 0) {
        // Because of the continuous use of the ADC, only keep "real" values.
        if (analogValue1 > ADC_NOISE_VALUE) {
//          snaKeypadPutValue(analogValue1);
          // Don't keep anthing until another key pressed.
          analogValue1 = 0;
        }
      }

      // If the analog value just read is greater than the previously read, save it.
      // Otherwise ignore it; assuming the key is just being pressed and low pressure
      // might cause the value to be low.
      if (tempVal > analogValue1) {
        analogValue1 = tempVal;
      }

      // Set up for ADC Channel 2.
      // Uncomment the following to enable the second channel.
//      ADMUX = ADCH2;
      break;
    case ADC2:
      analogValue2 = tempVal;

      // Set up for ADC channel 1.
      ADMUX = ADCH1;
      break;
  }
  // Dismiss the interrupt.
}


Instead of the "one wire" keypad, a simple push button switch set up as a simple voltage divider would work for the input to the first channel.  A pot, with one leg connected to Vcc and the other leg grounded, would have the wiper connected to the second channel.

Note, I haven't actually tested this, but it should work. (Again, famous last words.)

- Mark

Peter_n

#3
Feb 23, 2015, 10:56 am Last Edit: Feb 23, 2015, 11:03 am by Peter_n
Can you tell why you use the trigger for ADCSRA ?
Do you only want an ADC interrupt when a button is pressed ? With an interrupt pin ?  or using an analog comparator ?

According to the datasheet : "Switching to Free Running mode (ADTS[2:0]=0) will not cause a trigger event, even if the ADC Interrupt Flag is set".

This is a similar topic (without the trigger) : http://forum.arduino.cc/index.php?topic=245847.0
The ADCSRB is set to 0x40, But I don't understand why. When I fix your code with 0x40 for ADCSRB and without the trigger, the ISR runs. I didn't check if the analog values are okay.




pilant

I'm setting ADATE because the according to the description, it must be set for ADTS0-2 to be interpreted.  ADTS[2:0] need to be set to zero for Free Running mode.  So it is unclear if clearing ADATE will allow free running mode, or it will use some default (free running?)

I'm looking for continuous ADC use.  This is to allow the keypad ring buffer to be filled regardless of what else is happening in the sketch.  Right now, I don't do anything on a buffer overrun, but simply overwrite the "older" buffer entries.  I haven't decided if I really need to test for this condition.

Since I'm not using the comparator, I believe ACME (0x40) in ADCSRB should be clear.  Unfortunately, ACME is not described with the other bit in ADCSRB, but elsewhere in the spec, it is described.

Still confused.

- Mark

Peter_n

For a keypad it is strange to fill a buffer with a few thousand samples a second. I say, it is even wrong.

The trigger is used to sample at a frequency from a TIMER that triggers the ADC. That way a steady pace of for example 100 samples per second is possible.
Perhaps 10 to 50 samples a second is okay for a keypad. That can be done in a sketch with millis().

I'm sure that a trigger should not be used in free running mode.
Without the ACME, the free running mode is not working and the ISR is only called once or twice. I don't understand the ACME, but others use it in free running mode to "enable the mux"... ?

pilant

> For a keypad it is strange to fill a buffer with a few thousand samples a second. I say, it is even wrong.

True, but then the keypad isn't generating these values.  The ISR essentially dismisses the interrupt if the keypad analog value isn't in range (by pressing a key).  It is only when the value is valid that it gets placed in the rung buffer.  If I could trigger on the leading edge of the pin, that would be better.  However figure 24-2 isn't entirely clear if this is possible.

> The trigger is used to sample at a frequency from a TIMER that triggers the ADC...

True, but then I'm looking at handling the keypad values essentially asynchronously.  So a timer wouldn't be appropriate.  The comparator wouldn't work because of the limited inputs (pins).

This is the reason I'm running the ADC in free running mode.

> Without the ACME

ACME only seems to be used with the comparator (figure 23-1), the ADC section (24) defines ACME in ADCSRB but doesn't indicate it is used at all for ADC conversions.  I think I need to spend more time digging into sections 23 and 24 of the spec.  This doesn't appear to be as obvious as I had originally hoped.

- Mark

Peter_n

Okay, the buffer is only filled with valid values. But I would like to scan the keys at a normal rate  :P  or use millis() in the sketch.

Without the ACME the ISR is not running :o and others set it, but I don't know why. The ACME seems to be used for normal analog inputs, not the compare mode. I used ADC with registers in the past with different configurations, I must have forgotten it  :-[

pilant

> But I would like to scan the keys...

I could certainly do this.  I guess I've just been spoiled by real asynchronous handling and interrupts.  This is the first time I've tried doing some of this on a micro-controller  :P

> Without the ACME the ISR is not running...

I wonder why the single channel version works as expected?  The Atmel spec is certainly a bit lacking in this area.  I'll probably wander over to the Atmel site and see what I can find in their forums.

It is curious that the ACME (Analog Comparator Multiplexer Enable) bit is needed for non-comparator operations. *Maybe* it is really a multiplexer enable when the comparator is not used; which would make sense.

- Mark

pilant

Well, I finally got it working; although not quite like I had it originally.  The fundamental change was to switch from Free Running to Single Conversion.  This was accomplished by clearing ADATE in ADCSRA, switching the ADMUX channels in the ISR, and then setting ADSC in ADCSRA as the last thing in the ISR to start the next conversion.

This all worked as I would have expected... and even gave me the correct results  :P

From doing a lot of digging on the AVR Freaks forums, I came to the conclusion trying to use Free Running mode with interrupts was, shall we say, problematic at best.  It was then I thought about it a while and decided the Single Conversion approach would do exactly what I need.  From a logic standpoint, the only change is having to set ADSC explicitly with the Single Conversion.

If I ever need more than two ADC channels, I can simply add the minimal code in the ISR to handle the additional channels.

I just wanted to close the loop, with a resolution.  BTW, here is the working code:

Code: [Select]

#include <Arduino.h>

#define ADC_NOISE_VALUE 10

#define ADC0 0
#define ADC1 1
#define ADC2 2
#define ADC3 3
#define ADC4 4
#define ADC5 5
#define ADC6 6
#define ADC7 7

// Define the ADC channels used.  ADLAR will be zero.
#define ADCH1 ((1 << REFS0) | ADC1)
#define ADCH2 ((1 << REFS0) | ADC2)

volatile int analogValue1;
volatile int analogValue2;
volatile int adcChan;

void setup() {
  Serial.begin(9600);
 
  snaAdcIntSetup();
}

void loop() {
  int oldAnalogValue1;
  int oldAnalogValue2;
 
  oldAnalogValue1 = 0;
  oldAnalogValue1 = 0;

  while (1) {
    // If the first channel value has changed, print it out.
    if (analogValue1 != oldAnalogValue1) {
      Serial.print("Value1: ");
      Serial.println(analogValue1);
      oldAnalogValue1 = analogValue1;
    }

    // If the second channel value has changed, print it out.
    if (analogValue1 != oldAnalogValue1) {
      Serial.print("Value1: ");
      Serial.println(analogValue1);
      oldAnalogValue1 = analogValue1;
    }
  }
}

// Function:    snaAdcIntSetup()
// Description: This funtion sets up the ADC for generating interrupts.
// Arguments:   None
// Returns:     None
void snaAdcIntSetup() {
  // Set for the first ADC channel.
  ADMUX = ADCH1;

  // Set ADEN, prescale (128), and enable interrupts.
  ADCSRA = (1 << ADEN) | (1 << ADPS2) | (1 << ADPS1) | (1 << ADPS0) | (1 << ADIE);

  // Clear everything.  No free running because ADATE is clear in ADSRA.
  ADCSRB = 0;

  // Enable global interrupts.
  sei();

  // Kick off the first ADC.
  analogValue1 = 0;
  analogValue2 = 0;

  // Set ADSC to start ADC conversion.
  ADCSRA |= (1 << ADSC);
}

// The ADC interrupt function.
ISR(ADC_vect) {
//  int adcChan;
  int tempVal;

  // Get the ADC channel causing the interrupt.
  adcChan = ADMUX & ((1 << MUX3) | (1 << MUX2) | (1 << MUX1) | (1 << MUX0));

  // Read low first
  tempVal = ADCL | (ADCH << 8);

  switch (adcChan) {
    case ADC1:
      // If quiescent, ready to read
      if (tempVal == 0) {
        // Because of the continuous use of the ADC, only keep "real" values.
        if (analogValue1 > ADC_NOISE_VALUE) {
//          snaKeypadPutValue(analogValue1);
          // Don't keep anthing until another key pressed.
          analogValue1 = 0;
        }
      }

      // If the analog value just read is greater than the previously read, save it.
      // Otherwise ignore it; assuming the key is just being pressed and low pressure
      // might cause the value to be low.
      if (tempVal > analogValue1) {
        analogValue1 = tempVal;
      }

      // Set up for ADC Channel 2.
      // Uncomment the following to enable the second channel.
      ADMUX = ADCH2;
      break;
    case ADC2:
      analogValue2 = tempVal;

      // Set up for ADC channel 1.
      ADMUX = ADCH1;
      break;
  }

  // Set ADSC to start the next ADC conversion.
  ADCSRA |= (1 << ADSC);
}


- Mark

Peter_n

That is a good solution, at least no use of the not-very-understandable ACME bit.
When you change the mux, and run start the ADC immediately after that, the first few ADC values might not be accurate.

Go Up