ATtiny ADC Problems

Go To Last Post
13 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hello all,

I come from a background of ST/Microchip programming, and in order to teach myself using Atmel micros, I'm writing myself a small software library.

At the moment I'm having trouble with getting ADC reads with interrupts to work.

The code I have so far is here: https://github.com/jamesfowkes/Code-Library/blob/master/AVR/lib_adc.c.

I'm using it in the following manner with an ATTINY84A

In main():

ADC_Enable(true); // Should set ADEN in ADCSRA
ADC_EnableInterrupts(true); // Should set ADIE in ADCSRA
ADC_SelectPrescaler(LIB_ADC_PRESCALER_DIV64); // Sets /64 prescaler (125kHz ADC clock with 8MHz fCPU)
ADC_SelectReference(LIB_ADC_REF_VCC);

And then in a timer interrupt callback (which I know works correctly):

uint16_t result;
result = ADC_GetReading(LIB_ADC_CH_0);
/* post result to serial port... */

The relevant functions of code in the library is:

uint16_t ADC_GetReading(LIB_ADC_CHANNEL_ENUM eChannel)
{
	validReading = false;
	SetChannel(eChannel);
	ADCSRA |= (1 << ADSC); // Start conversion
	while (!validReading){ asm("nop"); }
	return lastReading;
}

ISR(ADC_vect)
{
	uint16_t result = 0;
	result = ADCL;
	result |= ADCH << 8;
	lastReading = result;
	validReading = true;
}

validReading is declared as static volatile bool.
lastReading is declared as static volatile uint16_t.

As far as I'm aware, the ADC_GetReading function should run until the ADC completes, the validReading flag is set, and the result can be returned. However, as far as I can tell the ISR is never called (I tried to turn an LED on inside it and it stayed off).

If I disable the ADC interrupt and do the following instead:

uint16_t ADC_GetReading(LIB_ADC_CHANNEL_ENUM eChannel)
{
	validReading = false;
	SetChannel(eChannel);
	ADCSRA |= (1 << ADSC); // Start conversion
	while (ADCSRA & (1 << ADSC)){ asm("nop"); }
	uint16_t result = ADCL;
	result |= ADCH << 8;
	return result;
}

This works correctly.

However, my intention is to use an interrupt+callback architecture to remove the while loop entirely. But I want to get the interrupt working first.

Can anyone suggest where I might be going wrong? I've scoured a lot of websites/tutorials and I'm stumped.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Never block in an ISR. In fact if you are using interrupts I don't see why you would ever want to block anyway?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Sorry, maybe I didn't make the structure of my code clear enough.

Quote:
Never block in an ISR.

I'm not blocking within the ISR itself. The ADC_GetReading function starts a conversion, and blocks waiting for the ISR to set the validReading boolean.

Quote:
In fact if you are using interrupts I don't see why you would ever want to block anyway?

My intent is to remove the blocking while loop entirely and provide the ADC module with a function pointer that the ISR will call on conversion complete.

Right now, I am just using this code to test that I can successfully trigger the ISR. That doesn't appear to be happening at the moment.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:
provide the ADC module with a function pointer that the ISR will call on conversion complete.

If it is called only once, will it be automagically inlined?

If it is not inlined, the burden of "saving context in the stack; do some cute treatment; restore the context fromthestact;" will be exactly the same than if it were in the main() -anyway, it might be- ?

edited : can you ... light/shut a LED from your ISR to be sure it is called?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:
can you ... light/shut a LED from your ISR to be sure it is called?

I've tried that. I turn an LED on before starting the conversion and turn it off inside the ISR.

The LED stays on, suggesting the ISR is never called.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:
If it is called only once, will it be automagically inlined?

If it is not inlined, the burden of "saving context in the stack; do some cute treatment; restore the context fromthestact;" will be exactly the same than if it were in the main() -anyway, it might be- ?

I don't think GCC will inline code across translation units?

My ADC module is designed to be generic so I can use it across multiple projects. A particular public function might be called once, or many time.

This got me thinking though, my next step should be to check the optimization settings.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:
And then in a timer interrupt callback (which I know works correctly):

uint16_t result;
result = ADC_GetReading(LIB_ADC_CH_0);
/* post result to serial port... */

Interrupts are disabled when ISR's are executing. You block in the timer ISR and wait for a flag to be set in another ISR. That will not happend unless you do something uggly.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

Sorry, maybe I didn't make the structure of my code clear enough.

You said:
Quote:
And then in a timer interrupt callback (which I know works correctly):

Surely such a callback is called IN THE CONTEXT OF AN ISR? If so there are two issues: one is that you should keep whatever is being done very very short and the other is that it's almost called with the I-bit in SREG disabled. As such you cannot then call a function that blocks on an interrupt flag being set as it will never occur (which is most likely the problem here).

This whole thing smacks of organic growth. Don't just keep adding bits to a program to grow its complexity. Take a step back, design a good (modular!) solution then implement it. If the timer callback is asynchronous and the ADC reading is asynchronouos I would not mix the two. Let them each operate in their own threads of execution.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
Surely such a callback is called IN THE CONTEXT OF AN ISR?

Ah yes, that will be the problem. Although, no need to shout. I'm sorry, I thought you were referring to the ADC ISR being blocked, hence my reply. I didn't realise you were referring the timer.

My main application (for any project) is normally driven by an asynchronous state machine, but in order to test the ADC module I took a shortcut and just used a timer.

Guess that'll teach me not to take shortcuts...

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Given the AVRs basic interrupt system where it's vital that ISRs complete in a timely fashion even the idea of calling a timer callback is not a great idea. Apart from anything else the call from the ISR is going to involve it PUSH/POPing about 12 registers you don't actually need to be saved because the calling code cannot "see" what the called function might actually be using/corrupting.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

None of my applications (so far) are particularly time/memory critical, so I don't think that's an immediate concern.

All my callbacks do (apart from the broken one above) is set a flag for the state machine handler to pick up.

I suppose I could just pass in a pointer to that flag and have the ISR set it directly.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

so I don't think that's an immediate concern.

That depends on whether you care that other interrupts in the system may be delayed or even lost. As soon as you start tying up the interrupt system for long periods and using MULTIPLE interrupts you will start to face such issues.
Quote:

I suppose I could just pass in a pointer to that flag and have the ISR set it directly.

A much better idea.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Just to expand upon the solution presented above. It is possible ( though in almost all cases ) a bad idea to re-enable interrupts within an ISR. To do so you would only need to reset the global interrupt enable flag with a call to sei(), at the top of the timer interrupt handler. By doing so, the code as it is should work. It leads to potential problems, however.

Nested interrupts on an AVR are, in general, a bad idea. The complete lack of programmable interrupt priority makes it difficult to ensure that there will not be a stack explosion if an interrupt takes a bit too long. A program with nested interrupts is also much more likely to fail in a way that is difficult to diagnose because of race conditions and the like. By far the simplest ( and most reliable ) way to deal with ISRs is to make them specific ( as opposed to generic ) and simple get in, do some minimal work, and get out. Anything more is asking for trouble and should only be done in such cases where there is a very clear reason that any other method will not work.

Martin Jay McKee

As with most things in engineering, the answer is an unabashed, "It depends."