Pls help to find a mistake in my code (solved!)

Hi,
i am new here! :slight_smile:
I have some sort of problem finding mistake in this code. It should readADC on 2 channels @ timer interrupt, summ them together, and, when redings count up to defined value, it should divide result with adjustValue to get oversampled and scaled result on LCD. BUT i get only 0.00V and 0.00A. Interrupt works, sampleCounter counts, if assign a constant number to ‚ÄúADCvolts‚ÄĚ or ‚ÄúADCamps‚ÄĚ - i get reading.
Can You see - where is the problem? Is it something with variables(local/global)? Please advise!
Ihave wrote different code with ADCreadingBufferV[100] witch is updatet from ISR and where final value is calculated before showing (in another isr but out of timer interrupt for ADC) ant there everything works as expected.

#define samplesCount 200    
#define ADCampsAdjust (samplesCount*0.4374)
#define ADCvoltsAdjust (samplesCount*0.25)
#define rangeMaxVolts 3100
#define rangeMaxAmps 3500
#include <avr/pgmspace.h>
#include <LiquidCrystal.h>
#include <util/delay.h> 
#include <EEPROM.h>
#include <avr/wdt.h>

// LiquidCrystal display setup:
LiquidCrystal lcd(3, 4, 11, 10, 9, 8);

//======= ADC input pins ==============//
const int analogInVolts = A0;
const int analogInAmps = A1;
int ADCreadingPointer = 0;
unsigned long ADCreadingsA, ADCreadingsV;
unsigned long ADCvolts, ADCamps;
boolean newData = 0;


void showFloat(unsigned int _floatValue, char _measure, int _maxRange){  // prints to LCD @ current cursor float from int - showFloat(1234,A); = 12.34A
	unsigned int whole, frac;
          frac = _floatValue % 100;
          whole = _floatValue / 100;
          if(whole<10){lcd.print(' ');}lcd.print(whole,DEC); lcd.print('.'); if(frac<10){lcd.print(0,DEC);} lcd.print(frac,DEC); lcd.print(_measure);
}

void updateReadings(){
      lcd.setCursor(0,0);
      lcd.print((1023-analogRead(analogInAmps)),DEC); 
      clearRemainingLCD(10);
      
      lcd.setCursor(0,1);
      showFloat(ADCvolts,'V',rangeMaxVolts);
      clearRemainingLCD(4);
      showFloat(ADCamps,'A',rangeMaxAmps);
}

ISR(TIMER2_OVF_vect) { // adc reading interrupt
  cli(); 
  if(ADCreadingPointer != 0){
       ADCreadingsV = ADCreadingsV + analogRead(analogInVolts);
       ADCreadingsA = ADCreadingsA + (1023-analogRead(analogInAmps));
       ADCreadingPointer--;
  }else{
       ADCvolts = ADCreadingsV/ADCvoltsAdjust;
       ADCamps = ADCreadingsA/ADCampsAdjust;
       newData = 1;
       ADCreadingsV = 0; ADCreadingsA = 0;
  }
  sei();
}

void clearRemainingLCD(byte _clrCount){ // prints xx spaces on lcd, starting @ current cursor
	for(byte clrcnt=0; clrcnt<_clrCount; clrcnt++){
		lcd.print(' '); // print space on lcd
	}
}

void loop(){  // main loop
if(newData==1){
  newData = 0;
  updateReadings(); 
}
}

void setup(){
  pinMode(analogInVolts, INPUT); // input from voltage divider
  pinMode(analogInAmps, INPUT); // input from current sense ic

// Timer2 setup
   //TCCR2 = (_BV(CS22)|_BV(CS21)|_BV(CS20));// 1/1024 45hz @ 12MHz 
   //TCCR2 = (_BV(CS22)|_BV(CS21));// 1/256 182hz @ 12MHz / 244hz @ 16MHz
   TCCR2 = (_BV(CS22)|_BV(CS20));// 1/128 365hz @ 12MHz / 488hz @ 16MHz
  // TIMSK |= _BV(TOIE2);   
   TIMSK |= _BV(TOIE2);

//  analogReference(DEFAULT);
  analogReference(INTERNAL); // using internal voltage refference 2,56V
  cli();
  lcd.begin(16, 2);
  lcd.print("ADC Test 3"); // Welcome text
   _delay_ms(1000);
   sei();
}

An analogRead takes about 100us - are you sure you want to be doing one in a timer interrupt?

Variable names like void showFloat(unsigned int _floatValue, that are really an int are misleading .... Not the cause of the problem but at least obfuscating ..

You must declare any variable that you write in the interrupt routine and read in the main loop as 'volatile'. E.g.:

volatile unsigned long ADCreadingsA, ADCreadingsV;
volatile unsigned long ADCvolts, ADCamps;
volatile boolean newData = 0;

Otherwise, the compiler is free to optimise away the reads and your code may never see the updated values.

Anachrocomputer: You must declare any variable that you write in the interrupt routine and read in the main loop as 'volatile'. E.g.:

volatile unsigned long ADCreadingsA, ADCreadingsV;
volatile unsigned long ADCvolts, ADCamps;
volatile boolean newData = 0;

Otherwise, the compiler is free to optimise away the reads and your code may never see the updated values.

Thanks, i'll try it out.

robtillaart: Variable names like void showFloat(unsigned int _floatValue, that are really an int are misleading .... Not the cause of the problem but at least obfuscating ..

Not at all - the point is that although its declared int it is to be treated as a float, so the name documents this. Would you call it intValue then?

robtillaart:
Variable names like void showFloat(unsigned int _floatValue, that are really an int are misleading … Not the cause of the problem but at least obfuscating …

=] maybe. :slight_smile: the showFloat() just prints 1234 as 12.34V or else. I could better name it _inputData or something…

although its declared int it is to be treated as a float

I would disagree - it is treated as a fixed-point, not floating point.

The interrupt routine appears to sum ADC values until ADCReadingPointer falls to zero and then it divides the sums, and then it sets them to zero - the wanted result will only be visible for one timer period.

Also ADCReadingPointer seems to always be zero so nothing will happen?

You want to move as much logic as possible out of the interrupt routine, in general, and have a specific way to synchronize with it from the rest of the code.

Try checking out how to use the AD converters with interrupts. In this case, every time the timer kicked in, you'd only need to set a bit and exit the timed interruption (of course, you should check first if the AD is free). You would then wait for the AD interrupt to occur and then put the data you've just read in a circular buffer. Change the channel you want, repeat procedure.

This way, you can set a flag bit that is read in the main loop to start readings.

Also, isn't there a way for a timer to automatically start an AD conversion?

MarkT: The interrupt routine appears to sum ADC values until ADCReadingPointer falls to zero and then it divides the sums, and then it sets them to zero - the wanted result will only be visible for one timer period.

Also ADCReadingPointer seems to always be zero so nothing will happen?

You want to move as much logic as possible out of the interrupt routine, in general, and have a specific way to synchronize with it from the rest of the code.

You are right! Just find out that also and wanted to post - ans saw Your post =] i was missing a "ADCReadingPointer = samplesCount;" Thanks!

bubulindo: Try checking out how to use the AD converters with interrupts. In this case, every time the timer kicked in, you'd only need to set a bit and exit the timed interruption (of course, you should check first if the AD is free). You would then wait for the AD interrupt to occur and then put the data you've just read in a circular buffer. Change the channel you want, repeat procedure.

This way, you can set a flag bit that is read in the main loop to start readings.

Also, isn't there a way for a timer to automatically start an AD conversion?

Yes, i know =] there is posibility to set AD converter in "free-running" mode, check for its interrupt, store data and switch channel. It will be "ADC_test _4.pde" But about circular buffers - if i define unsigned int ADCbuuferV[200]; it ocupies 400bytes of RAM, right? And i need 2x - then its 800b. I am using Atmega8 witch has 1024b RAM, with my code, i can afford only 2x [150]. Is there another way to get more counts per oversampled reading?

Starting ADC conversion in a timer, gathering result in ADC ISR:

const uint8_t startPin = 3;    // pin 3 = x, 4 = y, 5 = z

void Sample( int8_t index )    // 0 - 2 => x - z; -1 => stop ADC
{
  if ( index >= 0 )
  {
#if defined( ADCSRB ) && defined( MUX5 )
    // the MUX5 bit of ADCSRB selects whether we're reading from channels
    // 0 to 7 (MUX5 low) or 8 to 15 (MUX5 high).
    ADCSRB = ( ADCSRB & ~( 1 << MUX5 ) )   | 
             ( ( ( ( startPin + index ) >> 3 ) & 0x01 ) << MUX5 );
#endif
  
    // set the analog reference (high two bits of ADMUX) and select the
    // channel (low 4 bits).  this also sets ADLAR (left-adjust result)
    // to 0 (the default).
    ADMUX = ( analog_reference << 6 )  |  ( ( startPin + index ) & 0x07 );

    ADCSRA |= _BV( ADIE );
    sbi( ADCSRA, ADSC );    // start conversion
  }
  else
  {  // don't actually turn off ADC; there's no real point. Just...
    ADCSRA &= ~_BV( ADIE );  // turn off ADC interrupt
  }
}

void StartSampling()
{
  Sample( 0 );
}

int samples[ 3 ];
ISR( ADC_vect )
{
  uint8_t sampleIndex;
//digitalWrite( 13, HIGH );
  // save this sample, start next; if we've just collected a set of three, pass them to the orientation object and stop ADC (Sample( -1 ))
  samples[ sampleIndex = ( ADMUX & 0x07 ) - startPin ] = ADCW;  // get sampled pin number from the MUX
  Sample( ++sampleIndex < 3 ? sampleIndex : ( orientation.AcceptSample( samples ), -1 ) );
//digitalWrite( 13, LOW );
}


void StartTimer()
{
  // timer setup for interrupt
  TCCR1A    = 0;                // clear control register A 
  TCCR1B    = _BV( WGM13 );     // WaveformGenerationMode: phase & freq correct PWM; timer counts up to ICR1, then down; interrupt when count == 0
  ICR1      = 16000;             
  TCCR1B   |= _BV( CS10 );      // ClockSelect bits: no prescaling
  TIMSK1    = _BV( TOIE1 );     // Timer Overflow Interrupt Enable
}

void setup()
{
  pinMode( 13, OUTPUT );
  Serial.begin( 9600 );
  
  StartTimer();
}


ISR( TIMER1_OVF_vect )
{
  StartSampling();
}

With the DigitalWrites uncommented, this produces the expected three pulses every ms or so very clearly on a scope trace.

Also, I apologize for the somewhat cryptic ternary/comma operator statement :wink: The key part is (a) the reading of ADCW to obtain the just completed sample, and (b) the calling of Sample( n ) with the appropriate index or -1 after the last.

Arrange for the interrupt routine to just do the conversion and summing (and keep a count of how many times it has run) - all the other logic should be outside the ISR.

Thus to get the data you disable interrupts, read the sums, read the count of how many samples were taken, then zero them all again and re-enable interrupts. The ISR is simpler, which is good as code in an ISR is harder to debug.