Help with ADC complete conversion interrupt using Timer1 Interrupt

Hi everyone, I'm looking for some help for a project I'm working on.
I'm working with an Arduino Nano with an ATmega328P. I have basic understanding of operators, pointers, GPIO's, ADC converts etc. I've been learning about the Arduino IDE for 3 months now.

I am trying to use Timer1 to generate an interrupt every 100 msec. Then inside this timer, measure the uC temp using starting an ADC conversion. Then using an ADC ISR to update global memory to the last 10 measurements at 10Hz.

What I can't figure out is how to call/flag the ADC ISR after the timer1 ADC takes place to then gather 10 measurements. Below is my ADC ISR temperature program:

#include <stdint.h>
#include <Arduino.h>
#include "avr/interrupt.h"

// global variables
volatile uint8_t * ioPORTB;
volatile uint8_t * ioDDRB;
volatile uint8_t * ioPINB;

volatile uint8_t * pADMUX;
volatile uint8_t * pADCSRA;
volatile uint8_t * pADCL;
volatile uint8_t * pADCH;

volatile uint8_t * pSREG;

uint8_t msg[80];

uint16_t g_adcResult;

ISR(ADC_vect, ISR_BLOCK)
{
  //ADC conversion complete
  g_adcResult = *pADCL; // lower 8 bits of result 
  g_adcResult = g_adcResult | (((uint16_t)*pADCH) << 8); // 0x ADCH byte | ADCL byte
  g_adcResult = g_adcResult & 0x03FF; // clear bits 15 - 10 (ADC result is only 10 bits)
 
}

int main(void)
{
  init(); 

  float voltage;
  
  ioPORTB = (uint8_t *)0x25;
  ioDDRB = (uint8_t *)0x24;
  ioPINB = (uint8_t *)0x23; // PINB register [pb7 pb6 pb5 .. etc] 

  // setup points to MMR for ADC
  pADMUX = (uint8_t *)0x7C;
  pADCSRA = (uint8_t *)0x7A;
  pADCL = (uint8_t *)0x78;
  pADCH = (uint8_t *)0x79;

  pSREG = (uint8_t *)0x5F;

  uint16_t result;
  
  // make PB1 an output
  *ioDDRB = 0x02; // DDRB[7:0] = 0000 0010

  //configure ADC periperial
  *pADMUX = 0xC8; // 1100 1000 REFS = 1.1v, MUX =  1000
  *pADCSRA = 0x8F; // 1000 1111 ADEN = 1, ADIE = 1 (interupt enabled), ADPS = 111 (divide by 128 i.e. 16MHz / 128 = 125 kHz)
  //Once a converison completes, ADIF/ADC Flag is set

  
  // init serial port
  Serial.begin(9600);

  // sent out start of program message
  sprintf((char *)msg, "start of program\n");
  Serial.write((char *)msg);
  
  while(1)
  {
    // set PB1 high
    *ioPORTB = 0x02; // 0000 0010    

    // Read ADC channel ADC0
    *pADCSRA = (*pADCSRA) | 0x40; // start a conversion

    //write result to serial port
    *pSREG = (*pSREG) & (~0x80); // clear bit 7 disable interrupts globally
    voltage = (float)g_adcResult * (1100.0/1024.0);
    sprintf((char *)msg, "result = %d\n", g_adcResult);
    *pSREG = (*pSREG) | (0x80); // set bit7: enable interrupts globally
    Serial.write((char *)msg);

    sprintf((char *)msg, "voltagemV = %d\n", (uint16_t)voltage);
    Serial.write((char *)msg);
    
    // set PB1 low
    *ioPORTB = 0x00; // 0000 0000
  }
  
  return 0;
}

Below is my proof of concept of a timer1 interrupt that toggles the onboard LED every 100msec.

//timer1 will interrupt at 10Hz
#include <stdint.h>
#include <Arduino.h>
#include "avr/interrupt.h"

//storage variables
boolean toggle1 = 0;
volatile uint8_t * ioPORTB;
volatile uint8_t * ioDDRB;
volatile uint8_t * ioPINB;

uint8_t x;


void setup(){
  
  //set pins as outputs
  pinMode(13, OUTPUT);

  x = 0;

cli();//stop interrupts


//set timer1 interrupt at 10Hz
  TCCR1A = 0;// set entire TCCR1A register to 0
  TCCR1B = 0;// same for TCCR1B
  TCNT1  = 0;//initialize counter value to 0
  // set compare match register for 10hz increments
  OCR1A = 1562;// = (16*10^6) / (1*1024) - 1 (must be <65536)
  // turn on CTC mode
  TCCR1B |= (1 << WGM12);
  // Set CS12 and CS10 bits for 1024 prescaler
  TCCR1B |= (1 << CS12) | (1 << CS10);  
  // enable timer compare interrupt
  TIMSK1 |= (1 << OCIE1A);


sei();//allow interrupts

}//end setup



ISR(TIMER1_COMPA_vect){//timer1 interrupt 10Hz toggles pin 13 (LED)
//generates pulse wave of frequency 10Hz/2 = 5kHz 
  if (toggle1){
    digitalWrite(13,HIGH);
    toggle1 = 0;
  }
  else{
    digitalWrite(13,LOW);
    toggle1 = 1;
  }
}

I just can't figure out how to combine the two programs above. Create 100msec interrupt using Timer1, begin ADC, then use ISR(ADC_Vect) or ISR(TIMER1_COMPB_vect). I would appreciate any help as I've spent the last 10 hours trying to logically figure this out.

An ATMega328p cannot have an interrupt that gets interrupted. If an Interrupt is being processed, any other interrupts get queued for processing when the current interrupt processing ends.

You need to get your ADC value without an ADC interrupt, or your ADC interrupt needs to save the value where it can be picked up later by the timer ISR. Be careful that the two byte ADC value does not change while you are getting it one byte at a time.

the ADC interrupt is a lower priority than the timer interrupt therefore cannot interrupt it
what you can try is reenabling interrupts on entry to your timer interrupt routine
e.g. here I have a timer interrupting every 20mSec, reading temperature from a BMP280 using I2C and storing results in to riind buffer - the main loop is then transmitting the results over Bluetooth

//timer1 interrupt called every 20mSec - read BMP280 temperature copy to ring buffer
ISR(TIMER1_COMPA_vect){
      sei();                                            //allow interrupts
      testRecord.seq++;                                 // increment sequence number
      bmp280.getTemperature(testRecord.temperature);    // read current temperature
      bmp280.triggerMeasurement();                      // start next measurement
      //  add structure to ring buffer
      memcpy((byte*) &data[inData], (byte*)&testRecord, sizeof(testRecord));;
      // increment index to ring buffer and if at end reset to start
      if((inData+=sizeof(testRecord))>=sizeof(testRecord)*100) inData=0;
      // increment count of records in buffer
      inDataCount++;
 }

keep the code in the interrupt routine as short as possible - you have to be careful - if you take too long the timer my interrupt itself and there may be other interrupts occuring
you can check timing by looking at signals using an oscilloscope

there are processors which allow one to change the interrupt priorities to enable interrupts to interrupt other interrupt service routines so one can set up priority levels as required by the application

What is wrong with the timer ISR setting a volatile flag that the loop function can check so that the loop function can do work such as analogRead() ?

Are interrupts needed at all ?

vaj4088:
What is wrong with the timer ISR setting a volatile flag that the loop function can check so that the loop function can do work such as analogRead() ?

Are interrupts needed at all ?

Interrupts are needed for the project I'm dealing with sadly.
Essentially its:
1)Timer1 generates interrupt every 100msec. Inside this, start ADC conversion
2)Use ADC conversion complete interrupt ISR(ADC_Vect, BLOCK) and save the 10 last readings within the ADC ISR
3)Serial.print last 10 readings from ADC ISR inside the main loop.

I think a flag may be needed inside timer1 to call ISR(ADC_Vect) every 100msec. I know how to do parts 1 and 2, but I'm banging my head against the wall trying to understand how to use timer1 alongside ADC along with ADC ISR.

Something like this, perhaps?

/*
 * Timer1 triggers a first ADC conversion. During this conversion, the 
 * Timer1 ISR changes the ADC trigger to the conversion ready flag so
 * the next conversion is triggered immediately. The ADC ISR stores the
 * measurement, and after a given number of samples, it disables the ADC
 * ISR and resets the ADC trigger source to Timer1 again, to start the 
 * next cycle. It also sets the "done" flag so the main loop knows that
 * the measurements are ready.
 *
 * Pin PB5 (D13 on an UNO) is pulsed to verify the sampling rate using an
 * oscilloscope.
 */


// Helpers

// Convert the prescaler factor to the correct bit pattern to write to the
// TCCR1B register.
// ATmega328P datasheet § 15.11 (TCCR1A/B)
constexpr uint8_t prescaler_to_CS_bits(uint16_t prescaler) {
  return prescaler == 1 ? 0b001 :
         prescaler == 8 ? 0b010 :
         prescaler == 64 ? 0b011 :
         prescaler == 256 ? 0b100 :
         prescaler == 1024 ? 0b101 :
         0;
}

// Set the prescaler of Timer1 to the given factor.
// Possible values: 1, 8, 64, 256, 1024
// ATmega328P datasheet § 15.11 (TCCR1A/B)
void set_prescaler(uint16_t prescaler) {
  uint8_t bits = prescaler_to_CS_bits(prescaler);
  TCCR1B &= ~(0b111 << CS10);
  TCCR1B |= bits << CS10;
}

// Configure Timer1 in Clear Timer on Compare Match (CTC) Mode.
// ATmega328P datasheet § 15.11 (TCCR1A/B)
void set_CTC_mode() {
  uint8_t modebits = 0b0100; // CTC
  TCCR1A &= ~(0b11 << WGM10);
  TCCR1B &= ~(0b11 << WGM12);
  TCCR1A |= ((modebits >> 0) & 0b11) << WGM10;
  TCCR1B |= ((modebits >> 2) & 0b11) << WGM12;
}

// -------------------------------------------------------------------------- //

// Configuration
constexpr uint16_t buffersize = 10; // Number of ADC samples to read

constexpr uint16_t prescaler = 256;
constexpr uint16_t period = 0x1869; // 10 Hz

// Toggle pin PB5 (D13 on an UNO) to check the sampling rate on a scope:
// PB5 is toggled when the a conversion is done.
constexpr bool PULSE_OUTPUT = true;

// Sample frequency
constexpr float freq = 1. * F_CPU / prescaler / (1ul + period);

// Checks
static_assert(prescaler_to_CS_bits(prescaler) != 0, "Invalid prescaler");

// Buffer for samples
volatile uint16_t buffer[buffersize];
volatile uint16_t *const buffer_end = buffer + buffersize;
volatile uint16_t *write_ptr = buffer;
volatile bool done = false;

// -------------------------------------------------------------------------- //

void setup() {
  Serial.begin(115200);
  Serial.print("Frequency (Hz): ");
  Serial.println(freq, 6);
  Serial.print("Period    (µs): ");
  Serial.println(1e6 / freq, 6);

  // Initialization:
  // --------------

  noInterrupts();

  set_CTC_mode();            // Timer1 CTC mode
  set_prescaler(prescaler);  // Timer1 Prescaler

  OCR1B = period;            // Compare B: Triggers ADC conversion
  OCR1A = period;            // Compare A: Resets Timer1

  ADMUX  |= 0b11 << REFS0;   // ADC Bandgap Reference
  ADMUX  |= 8 << MUX0;       // ADC channel 8 (temperature sensor)
  ADCSRB |= 0b101 << ADTS0;  // ADC Trigger: Timer1 Compare Match B
  ADCSRA &= ~(1 << ADIE);    // ADC Interrupt Disable
  ADCSRA &= ~(1 << ADATE);   // ADC Auto Trigger Disable
  ADCSRA |= 1 << ADEN;       // ADC Enable

  // Do a first ADC conversion (first conversion is always slower)
  ADCSRA |= 1 << ADSC;       // ADC Start Conversion
  while (!(ADCSRA & (1 << ADIF))); // Wait for conversion to finish
  ADCSRA |= 1 << ADIF;       // Clear conversion ready flag

  if (PULSE_OUTPUT)
    DDRB |= 1 << PB5;        // Built-in LED Output mode

  interrupts();

  delay(2000);

  // Start:
  // -----

  noInterrupts();
  
  TCNT1 = 0;             // Timer1 Reset
  TIFR1  |= 1 << OCF1B;  // Clear trigger source to start conversion
  ADCSRA |= 1 << ADATE;  // ADC Auto Trigger Enable
  TIMSK1 |= 1 << OCIE1B; // Timer1 Compare B Match Interrupt Enable
  
  interrupts();
}

void loop() {
  if (done) {
    uint16_t sum = 0;
    for (uint16_t value : buffer)
      sum += value;
    Serial.println(sum / buffersize);
    done = false;
  }
}

// -------------------------------------------------------------------------- //

ISR(TIMER1_COMPB_vect) {
  done = false;
  ADCSRB &= ~(0b111 << ADTS0);  // ADC Trigger: Free Running Mode
  ADCSRA |= 1 << ADIE;          // ADC Interrupt Enable
}

ISR (ADC_vect) {
  *write_ptr++ = ADC; // Store the ADC sample in the buffer

  if (write_ptr >= buffer_end) { // If the buffer is full
    ADCSRB |= 0b101 << ADTS0;    // ADC Trigger: Timer1 Compare Match B
    ADCSRA &= ~(1 << ADIE);      // ADC Interrupt Disable
    write_ptr = buffer;          // Reset write index
    done = true;                 // Signal to loop() that buffer is full
  }

  if (PULSE_OUTPUT)
    PINB |= 1 << PB5; // Toggle built-in LED
}

Note: taking 10 ADC samples in free running mode takes 130 clock cycles at 125 kHz = 1.04 ms, so make sure that the Timer1 interrupt rate is well below that.

Pieter

freebird88:
The way I've been taught to set prescaler's is the hard way, by using points such as *pTCCR1A = 0x40; and *pTCCR1B = 0x01;
Could you briefly explain to me how this works?

// Convert the prescaler factor to the correct bit pattern to write to the

// TCCR1B register.
// ATmega328P datasheet § 15.11 (TCCR1A/B)
constexpr uint8_t prescaler_to_CS_bits(uint16_t prescaler) {
  return prescaler == 1 ? 0b001 :
        prescaler == 8 ? 0b010 :
        prescaler == 64 ? 0b011 :
        prescaler == 256 ? 0b100 :
        prescaler == 1024 ? 0b101 :
        0;
}

prescaler_to_CS_bits is simply a function that returns the bit pattern corresponding to the given prescaler setting. E.g. prescaler_to_CS_bits(64) returns 011[sub]2[/sub]. The definition of these bit patterns is in the datasheet. If the prescaler is not an allowed value, the function returns zero.
The question marks and colons are part of the ternary conditional operator.

There is no need to define your own pointers to peripheral registers, these are all defined in the AVR headers included by the Arduino framework. The bit positions within a register are available as constants, see my example code above.
When dealing with bits in a register, binary is more meaningful than hexadecimal, especially since the fields are small.

freebird88:
Lastly, I'm not sure free running mode for Timer1 is best. I need timer 1 to interrupt every 100 msec or 10 Hz, but then the main loop to serial print the average at 1 Hz or 1 second rate. Below is how I know how to set the interrupt rate but not inside the ISR.

The timer is not in free running mode, the ADC is (for a brief amount of time).
The timer interrupt fires every 100 ms in the code I posted. As soon as it fires it triggers an ADC conversion (this happens in hardware, because the ADC trigger source is set to Timer1 Compare B). After the conversion is started, the Timer1 compare interrupt fires, which changes the trigger source of the ADC to the ADC conversion ready flag. This means that as soon as the conversion is ready, the next one is started (referred to as "free running mode"). After each conversion is complete, the ADC ISR stores the value. When that ISR has stored 10 values, it changes the ADC trigger source again, so it won't keep on starting new conversions (exit free running mode). Timer1 is selected as the new ADC trigger, so it'll start a conversion next time Timer1 fires (approx. 99 ms later).

So the timing is as follows: Timer1 fires, a conversion is started. 104 μs later, the conversion is done, the second conversion is started and the first value is stored. 104 μs later, the third conversion is started and the second value is stored ... When the 10 th conversion is done, an 11 th conversion is started, the 10 th value is stored, and the ADC ISR sees that 10 values have been captured, so it stops the ADC from starting any new conversions, and it disables itself, so the 11 th conversion result is never used or stored. (You could change the code so it never starts that 11 th conversion in the first place, but it requires an extra if statement.) Now the code does nothing until Timer1 fires again, 100 ms after the previous time it fired, or ≈99 ms after the last conversion was finished. (In the mean time, it runs the main loop, of course.)

The interrupt rate and other settings of Timer1 are set once in the setup, and they aren't changed afterwards.

If you want the main loop to run at 1 Hz and the measurements at 10 Hz (which is not the same as your initial question), you need at least 10 buffers of 10 values (or 1 buffer of 10 values and 9 or 10 averages if you compute the average immediately), and cycle through the buffers, switching to the next one each time Timer1 fires.

freebird88:
You elude to making sure my timer1 interrupt rate is well below 130 clock cycles at 125 kHz but could you logically explain that to me?

The number of ADC clock cycles required for a single conversion are listed in the datasheet. For free running mode, this is 13 cycles/conversion. You wanted 10 conversions, so that's 130 cycles. The ADC clock is set to 125 kHz by default (by the Arduino Core), so 130 / (125,000 Hz) = 1.04×10-3 s.

If you want to take 10 samples every 100 ms, you have to make sure that taking 10 samples takes less than 100 ms, otherwise your timing won't be correct. You also need to take into account the overhead of computing the average and sending the data, as well as ISR overhead.