[SOLVED] ADC Conversion Complete Interrupt Not Firing

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

Hi all -

After weeks of searching and tearing my hair out and my fiftieth read through of the ADC and Interrupt portions of the datasheet, I finally resorted to opening a thread. It is as the title says - the ADC refuses to trigger conversion complete interrupts or, for that matter, even convert once! Old versions of this code and a (working!) C version are at https://github.com/npiscitello/adc_led.

 

What I Want It To Do:

There are LEDs on PB0-PB5 - I'm trying to implement a variable speed chase sequence. I'm doing this by using a potentiometer and an analog input to adjust the length of the delay between steps in the chase. The chase is definitely working - the full code has a timer implemented that drives the chase at a fixed speed using a compare interrupt (see the github link above). Basically, I was planning on writing the left adjusted ADC value in ADCH to the OCR0A register every time a conversion finished (aka, in the ISR). This would allow the timer to just do it's thing independently. I'm sure there are easier ways to do this, but I am using this exercise to learn specifically how to make multiple interrupts play nicely together.

 

What It's Doing:

Instead of chasing, the first LED turns on (as expected) and stays on (not as expected), exhibiting a complete failure to chooch. I can only assume that this is because the ADC conversion ISR is not being executed. In fact, due to other tests on similar versions of the code (all in the commit history), I don't think the ADC even converts once. I know that you need to kick off the first conversion for the ADC to start free-running by setting `ADSC` in `ADCSRA`. I have checked the prescalers to ensure the ADC clock is faster than the requisite 50 kHz. I've compared my ADC setup to the working C version and it seems to be identical (except for enabling the interrupt in the asm code below) - I've also checked the register settings against the datasheet more times than I can count. As mentioned above, I've verified my chase sequence using a known-working timer setup. I've verified that the ISR is being loaded into the correct location to the best of my ability - the internals of WinAVR are still mysterious to me, but both external interrupts and all the timer interrupts I've tried work and the ADC interrupt has the correct index (vector number 21, if the reset vector is at index 0). If there's any other checks I should do, I'd very much appreciate to hear about it! I've obviously missed a pretty crucial one...

 

Technical Details:

Processor: Bare ATMEGA328P-PU on a breadboard

Fuses: default (lfuse: 0x62, hfuse: 0xD9, efuse: 0x07)

Clock: internal 8 MHz

Simulation: none (I tried to load a couple simulation packages but none of them seemed to work on my laptop)

Compiler: avr-gcc 4.3.3, provided by WinAVR 20100110

My Experience: I'm fairly experienced with C and C++ in the Arduino IDE and with embedded code (c++11) for other platforms, but this is my first time with assembler for any platform and my first time using a bare AVR chip with an ISP. I haven't quite read the 328P datasheet cover to cover, but I'm familiar with most sections and intimate with a few.

Available Equipment: I have an Adafruit TinyISP, Windows 10 laptop with an ArchLinux VM, and a DMM on my bench and I have access to a digital oscilloscope.

 

Thank you guys in advance for the help. This is my first actual post asking for code help - ususally I can figure it out on my own (aka Google can figure it out for me) but this one has me completely stumped. Please help me get my pixies dancing again!

~Nick P

; Nick P
; January 2017
; Atmel ATMEGA328P-PU
; avr-gcc 4.3.3 (WinAVR 20100110)

#include <avr/io.h>
#include <avr/interrupt.h>

;=== REGISTER MAP ===;
; r16: temporary working reg
#define TEMP r16
; r17: minimum LED for chase sequence
#define MIN r17
; r18: maximimum LED for chase sequence
#define MAX r18
; r19: current LED lit
#define LOC r19
; r20: LED direction (1 for ascending, 0 for descending)
#define DIR r20
#define ASC 0x01
#define DSC 0x00
; r21: ADC value to be written to output compare
#define ADC r21

;=== UTILITY MACROS ===;
#define low(x) ((x) & 0xFF)
#define high(x) (((x) >> 8) & 0xFF)

; run when the chip resets
.global main
main:
  ; divide system clock by 64 (I would prescale it down by 256 but the ADC needs at least 100kHz)
  ; this gives a system clock speed of 8 MHz / 64 = 125 kHz
  ldi TEMP,0x00 | _BV(CLKPCE)
  sts CLKPR,TEMP
  ldi TEMP, 0x00 | _BV(CLKPS2) | _BV(CLKPS1)
  sts CLKPR,TEMP

  ; enable output pins
  ldi TEMP,0x00 | _BV(DDB5) | _BV(DDB4) | _BV(DDB3) | _BV(DDB2) | _BV(DDB1) | _BV(DDB0)
  out _SFR_IO_ADDR(DDRB),TEMP

  ; set min and max for LED chase and initialize location to min LED
  clr MIN
  clr MAX
  clr LOC
  clr DIR
  ldi MIN,_BV(PORTB0)
  ldi MAX,_BV(PORTB5)
  mov LOC,MIN
  ldi DIR,ASC

  ; power on the features we are using
  lds TEMP,PRR
  cbr TEMP,PRADC
  sts PRR,TEMP

  ; set stack pointer to the end of RAM (required for returning from interrupts)
  ldi TEMP,low(RAMEND)
  sts SPL,TEMP
  ldi TEMP,high(RAMEND)
  sts SPH,TEMP

  ; set up ADC voltage reference and input pin
  ; ADMUX [ REFS1 | REFS0 | ADLAR | - | MUX3 | MUX2 | MUX1 | MUX0 ]
  lds TEMP,ADMUX  ; load ADMUX into TEMP
  cbr TEMP,REFS1  ; enable internal voltage ref
  sbr TEMP,REFS0  ; enable internal voltage ref
  cbr TEMP,MUX3   ; set ADC0 as the ADC pin
  cbr TEMP,MUX2   ; set ADC0 as the ADC pin
  cbr TEMP,MUX1   ; set ADC0 as the ADC pin
  cbr TEMP,MUX0   ; set ADC0 as the ADC pin
  sbr TEMP,ADLAR  ; left adjust result for 8-bit precision
  sts ADMUX,TEMP  ; set ADMUX to TEMP

  ; set up ADC trigger source
  ; ADCSRB [ - | ACME | - | - | - | ADTS2 | ADTS1 | ADTS0 ]
  lds TEMP,ADCSRB ; load ADCSRB into TEMP
  cbr TEMP,ADTS2  ; select ADC interrupt as trigger source
  cbr TEMP,ADTS1  ; select ADC interrupt as trigger source
  cbr TEMP,ADTS0  ; select ADC interrupt as trigger source
  sts ADCSRB,TEMP ; set ADCSRB to TEMP

  ; set up ADC clocking and triggering, enable ADC
  ; ADCSRA [ ADEN | ADSC | ADATE | ADIF | ADIE | ADPS2 | ADPS1 | ADPS0 ]
  lds TEMP,ADCSRA ; load ADCSRA into TEMP
  sbr TEMP,ADEN   ; enable ADC
  cbr TEMP,ADPS2  ; divide system clock by 2 (62.5 kHz)
  cbr TEMP,ADPS1  ; divide system clock by 2 (62.5 kHz)
  cbr TEMP,ADPS0  ; divide system clock by 2 (62.5 kHz)
  sbr TEMP,ADATE  ; enable auto trigger
  sbr TEMP,ADIE   ; enable ADC interrupts
  sbr TEMP,ADIF   ; clear pending ADC interrupts
  sbr TEMP,ADSC   ; trigger first read
  sts ADCSRA,TEMP ; set ADCSRA to TEMP

  sei             ; globally enable interrupts
  out _SFR_IO_ADDR(PORTB),LOC   ; start at minimum LED
  rjmp loop       ; jump into infinite loop

; infinite loop. I guess this could be a sleep maybe?
loop:
  lds ADC,ADCH    ; read the ADC value
  rjmp loop       ; loop infinitely

; run when the ADC finishes a conversion
.global ADC_vect
ADC_vect:
  cpi DIR,ASC     ; compare the direction reg with the ascend direction constant...
  breq ascend     ; ...and skip to the ascend code if they're equal...
  rjmp descend    ; ...or to the descend code if they're not.

ascend:
  lsl LOC         ; increment LED location (originally used rol, but it screwed up the compare
  out _SFR_IO_ADDR(PORTB),LOC   ; write LED location out
  cpse LOC,MAX    ; compare the location to the max location...
  reti            ; ...and skip this instruction if they're the same.
  ldi DIR,DSC     ; reverse direction
  reti            ; return from ISR

descend:
  lsr LOC         ; decrement LED location (originally used ror, but it screwed up the compare)
  out _SFR_IO_ADDR(PORTB),LOC   ; write LED location out
  cpse LOC,MIN    ; compare the current location with the min location...
  reti            ; ...and skip this instruction if they're the same.
  ldi DIR,ASC     ; reverse direction
  reti            ; return from ISR

 

This topic has a solution.

"Try assembler", they said!

"It'll be fun!", they said...

Last Edited: Wed. Feb 1, 2017 - 08:03 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Why are you using the adc interrupt when the isr code does not touch the adc?

This reply has been marked as the solution. 
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I'm at a bit of a loss to understand how:

  ; set up ADC clocking and triggering, enable ADC
  ; ADCSRA [ ADEN | ADSC | ADATE | ADIF | ADIE | ADPS2 | ADPS1 | ADPS0 ]
  lds TEMP,ADCSRA ; load ADCSRA into TEMP
  sbr TEMP,ADEN   ; enable ADC
  cbr TEMP,ADPS2  ; divide system clock by 2 (62.5 kHz)
  cbr TEMP,ADPS1  ; divide system clock by 2 (62.5 kHz)
  cbr TEMP,ADPS0  ; divide system clock by 2 (62.5 kHz)
  sbr TEMP,ADATE  ; enable auto trigger
  sbr TEMP,ADIE   ; enable ADC interrupts
  sbr TEMP,ADIF   ; clear pending ADC interrupts
  sbr TEMP,ADSC   ; trigger first read
  sts ADCSRA,TEMP ; set ADCSRA to TEMP

is any more efficient or readable (it isn't!) than:

ldi r24, (1 << ADEN) | (1 << ADATE) | (1 << ADIE) | (1 << ADIF) | (1 << ADSC)
STS ADCSRA, r24

or if you really want to so an RMW:

ldi r24, (1 << ADEN) | (1 << ADATE) | (1 << ADIE) | (1 << ADIF) | (1 << ADSC)
LDS r25, ADCSRA
OR r25, r24
STS ADCSRA, r25

(though in an init you probably just want to use assignment, not update).

 

Anyway, as Russell says, this:

; infinite loop. I guess this could be a sleep maybe?
loop:
  lds ADC,ADCH    ; read the ADC value
  rjmp loop       ; loop infinitely

doesn't actually make any use of "ADC" anyway. Also, if you are getting interrupts why not:

.global ADC_vect
ADC_vect:
  lds ADC,ADCH    ; read the ADC value

Also that is a very bad choice for a local register name:

; r21: ADC value to be written to output compare
#define ADC r21

because iom328p.h has:

C:\SysGCC\avr\avr\include\avr>grep ADC iom328p.h | grep SFR
#define ADC     _SFR_MEM16(0x78)
#define ADCW    _SFR_MEM16(0x78)
#define ADCL _SFR_MEM8(0x78)
#define ADCH _SFR_MEM8(0x79)
#define ADCSRA _SFR_MEM8(0x7A)
#define ADCSRB _SFR_MEM8(0x7B)

(very surprised the preprocessor did not warn you about this redefinition!)

 

PS If you know C, C++ and Arduino then it's often a good idea to sketch out in C the algo you want to implement then feed that into the C compiler with -save-temps and the .i file that is created will be a template for what you want to achieve in the asm.

(or you could just write in C - I fail to see the speed/size requirement here that would force one to need to use asm?)

Last Edited: Wed. Feb 1, 2017 - 09:20 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Oof, asked for my code to be torn apart and you delivered! yes

 

First, there is no size/speed requirement - I'm solely using this as an exercise to expose myself to assembler. I will check out the -save-temps option though and see how the compiler sets up the ADC.

 

Regarding the RMWs: those were some of the first lines I ever wrote in assembler so I broke them out intentionally to be extra verbose and never changed them over once I got more comfortable with the instructions themselves. I'll change them all over to assignments, if only to improve readability (which I suppose is more important in assembler than most other languages, now that I think about it...).

 

clawson wrote:
Also that is a very bad choice for a local register name:

This is one thing I actually did see in my traversal of headers - however, on my platform, `ADC` is just not defined when dealing in assembler:

C:\WinAVR-20100110\avr\include\avr\iom328p.h
...
#ifndef __ASSEMBLER__
#define ADC     _SFR_MEM16(0x78)
#endif
#define ADCW    _SFR_MEM16(0x78)
...

Thanks for the tip - I'll change is just to be safe!

 

clawson wrote:
doesn't actually make any use of "ADC" anyway. Also, if you are getting interrupts why not:

The value of the ADC isn't used in this test program because I just wanted to see if I could get the interrupt to fire at all. As stated above, eventually ADCH will be copied into OCR0A every time the ADC interrupt fires. From an older version:

; run when the ADC finishes a conversion
.global ADC_vect
ADC_vect:
  lds ADC,ADCH    ; load the converted ADC value into a temp register
  sts OCR0A,ADC   ; set the output compare for timer 0 to the ADC value
  reti            ; return from ISR

And that's the problem - in all the code I've written so far, the ADC never fires an interrupt (or, more specifically, the ADC ISR is never run). Is the 

lds ADC,ADCH

not enough to count as 'reading' the ADCH register so it can be re-filled? Does that matter? I did copy that line over into the ISR real quick just to see if it helped but the output on the LEDs didn't change. FWIW, other interrupts do work - in the full version before I started stripping the code to isolate this error, timer0 very happily spews compare interrupts to drive the LEDs while the OCR0A register doesn't seem to ever not be zero - the above ADC_vect snippet shows the code from that version. When that code is run, the first LED lights and stays on for a while (the timer starts counting before the interrupt is enabled, so it has to overflow back to 0 before the first interrupt is called) and then a very fast chase starts ( (I think because, essentially, the timer compare interrupt is being fired every time the timer ticks - OCR0A always seems to be 0).

 

TL;DR: This is in assembler as an exercise, not for any technical reason. Most of the things mentioned are newbie errors that will get changed. My problem is specifically that the ADC conversion complete ISR never runs despite other ISRs working.

 

I'll implement the suggestions after class tonight and keep you guys posted. Thanks!

"Try assembler", they said!

"It'll be fun!", they said...

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

thatvolvonut wrote:
the ADC never fires an interrupt
How are you observing this? Simulator or real silicon with a debugger? Or just by the final LED behaviour?

 

My reading of the 328 data is that by simply setting ADEN, ADATE and ADSC (and leaving ADTS bits at the default 000 selection of "free running") is that ADIF will repeatedly trigger conversions. With ADIE and an ISR vector provided I would therefore expect the ISR code to be called repeatedly.

 

But at 62.5kHz for the ADC clock and 13 ADC clock ticks for a conversion your sample rate will be 4,808Hz which is an interrupt ever 208us. So your shifting of the LED between PORTB.0 anbd PORTB.5 is going to be very rapid indeed. In fact so rapid that I rather image (because of human persistence of vision) that it'll look like all the LEDs are on all the time.

 

BTW how does ADPS[2:0]=000 give 62.5kHz anyway? At one point you say:

Fuses: default (lfuse: 0x62, hfuse: 0xD9, efuse: 0x07)

Clock: internal 8 MHz

So that means F_CPU=1MHz. In my world 1MHz/2 = 500kHz and if that's the case after 13 ADC clocks the sample rate is 38.462kHz so that the interrupt period would be 26us in fact (VERY fast LEDs!!).

 

Even if you whack the ADPS up to the max (111 = /128) then you are still looking at a 601Hz sample rate and interrupts at 1.66ms

 

To actually "see" something I think you do need ADPS set to the maximum but also slow the CPU down well below 1MHz - use CLKPR and use some serious division.

 

(or rethink the design - maybe have a counter in the ISR and only do the LED shifting on every 256th interrupt or something?)

 

Remember that even "slow" 8 bit micros are going blisteringly fast as far as human eyes/brains are concerned!

Last Edited: Wed. Feb 1, 2017 - 12:43 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

 With ADIE and an ISR vector provided I would therefore expect the ISR code to be called repeatedly.

Perhaps that is the problem !

 

Make sure that the vector (it's not really a vector but a restart addr) are placed in 0x002A and there is a jmp to your ADC label. 

(show the table, perhaps your compiler don't "see" that ADC ISR are used and place a bad vector jump instead) 

 

 

edit typo

Last Edited: Wed. Feb 1, 2017 - 03:36 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I come to you with my head hung in shame - 

 

On clawson's advice, I went through my code to get rid of all the `sbr` and `cbr` instructions in favor of bitwise assignments for setup. Turns out, my PRR setup was wrong and the ADC wasn't even getting power! Now that the bits are flipped correctly, my LEDs are chooching nicely. (On further reading, I realized that I didn't even need to be touching PRR. I've always been good at messing with things I shouldn't be...)

 

Many thanks to clawson for also reminding me about the CKDIV8 fuse!

clawson wrote:
BTW how does ADPS[2:0]=000 give 62.5kHz anyway?

 

For posterity - the solution to this problem is on github: https://github.com/npiscitello/adc_led/commit/3b14c9c67e8ee2e1d764aae7b5de2d498cf7d0c4

(in case the link goes dead: the solution was essentially replacing the PRR RMW in the original code with

ldi TEMP, 0xFF & !_BV(PRTIM0) & !_BV(PRADC)
sts PRR,TEMP

 

My next project will be to look at the `.i` file generated with -save-temps and correlate that with my assembler code, just so I can get a good understanding of the process. After that, I'll be back in C land for a while - assembler is fun, but I'm not up for this kind of headache every time I want to figure out a new feature xD

 

Thanks again for all the help!

~Nick P

"Try assembler", they said!

"It'll be fun!", they said...

Last Edited: Thu. Feb 2, 2017 - 03:53 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Just to say but you could just as easily make the wrong write to PRR in C as in Asm. That alone is not the reason to give up on using Asm.