Problem mit ADC Auto-Trigger interrupt

[Markus (Gast)]

Hallo Leute,

ich habe ein Problem mit dem ADC im Auto-Trigger Mode auf meinem 
ATmega168. Der Auto-Trigger ist auf Compare Match A mit Timer 1 gelegt. 
Dadurch wird eine Wandlung gestartet und bei deren Ende wird ein 
Interrupt ausgeführt.

Hier mal die Init des ADCs:

void Init_adc(void)
{
  // PORT C als Eingang
  DDRC = 0x00;

  // Interne 1,1V als Referenz
  ADMUX |= (1<<REFS0) | (1<<REFS1);         

  // Frequenzvorteiler auf 64 setzen (288 kHz)
  ADCSRA |= (1<<ADPS2) | (1<<ADPS1) | (1<<ADATE) | (1<<ADIE) | (1<<ADEN);

  // Auto-Trigger on Timer/Counter0 Compare Match A
  ADCSRB |= (1<<ADTS0) | (1<<ADTS1);
}

In der ISR lese ich 4 ADC Kanäle aus und addiere diese Werte in einem 
Array zur Mittelwertbildung auf. Jedesmal wenn ich einen Kanal 
ausgelesen habe, inkrementiere ich die Laufvariable "channel" damit ich 
so den nächsten ADC-Kanal auswählen kann um im nächsten Interrupt diesen 
Kanal auszulesen und an entsprechender Position im Array abzulegen. Wenn 
ich meine 4 Messungen habe, so wir mit einer weiteren Laufvariable "i" 
ein Flag gesetzt und in der main ausgewertet.

Mein Problem ist nun, dass meine Laufvariable "channel" nur ein einziges 
mal inkrementiert wird. Sie wird mit 0 initialisiert und ist global als 
volatile unsigned char deklariert. Sie bleibt also auf dem Wert 1 
stehen. Zum debuggen habe ich mal eine kleine LED benutzt und diese an 
verschiedenen Positionen der IF-Abfragen eingeschaltet, dadurch konnte 
ich herausfinden dass "channel" nur den Wert 1 erreicht.

Das Array ist übrigens auch global als volatile unsigned int deklariert.

Könnt ihr vielleicht ein Fehler im Code sehen? Ich blick schon gar nicht 
mehr durch :)

LG Markus

Hier noch der Code des Interrupts:

ISR(ADC_vect)
{
  ADC_channel[channel] += ADCW;    // ADC-Kanäle auslesen (Wichtig: "channel" muss mit 0 initialisiert werden!)

  channel++;

  if(channel > 3)
  {
    ADMUX |= (0<<0);    // ADC0-Kanal wählen
    i++;        // Laufvariable zur Zählung der Messungen
    channel = 0;
  }
  else
  {
    ADMUX |= (channel<<0);    // Setzen des nächsten ADC-Kanals
  }

  if(i == 4)
  {
    strom_irq = 1;      // Flag setzen zur Auswertung von mehreren Messungen
  }
}

[Läubi .. (laeubi)]

Erstmal würde ich die Zählvariable static in der ISR definieren. Sonst 
könnte der compiler denken das diese nur lokal in der ISR genutzt wird. 
Da die ISR aber nie (direkt) aufgerufen wird ist der Wert dort immer 1!

Was macht das i dort? und wie ist das definiert? Und das schieben um 0 
an verschiedenen Stellen ist auch unsinnig.

[Karl H. (kbuchegg)]

Mal abgesehen von dem Murks den du da mit ADMUX aufführst:
Hast du schon abgeklärt, ob
  der Timer regelmässig seine ISR auslöst
  als Folge davon der ADC gestartet wird und der wiederrum
  seinerseits die zugehörige ISR auslöst.

[X- R. (x-rocka)]

Ich hätte eher getippt, dass der ADMUX immer auf Kanal 3 stehen bleibt, 
oder wo werden die MUX bits vorm odern genullt?

ADMUX |= (0<<0);
ADMUX |= (channel<<0);

oh ja, "i"s sind ja schon gefährlich, globale gehen gar nicht!
und das i muss auch zurückgesetzt werden!

[Karl H. (kbuchegg)]

Also sind wir wieder mal beim üblichen:

Poste den kompletten Code!

(Ich schwör: irgendwann bau ich ein Expertensystem, das eine Anfrage 
analysiert und wenn es um Programmieren geht, den Code durch den 
Compiler jagt und sobald da irgendwelche undefined References auftauchen 
die automatische Meldung "Poste kompletten Code" als Antwort einstellt 
und den Thread sperrt.)

[Markus (Gast)]

Danke für die Antworten.

Hab die Laufvariablen als static deklariert, hat leider auch nicht 
geholfen.

"i" ist ebenfalls nur eine Laufvariable und, jetzt, auch als static 
volatile unsigned char deklariert.

Ja, den Timer und den ADC habe ich schon getestet und sie funktionieren. 
Ebenso wird der Interrupt regelmäßig aufgerufen. Das einzige Problem das 
ich feststellen konnte, war das nicht vorhandene inkrementieren meiner 
Laufvariable.

Hehe, ja ich weiß, dass das teilweise recht unsinnig ist :) jedoch 
sollte es so eigentlich auch funktionieren und keine Fehler verursachen. 
Zumindest nehme ich das mal an. Sobald der Code funktioniert wird er 
bereinigt und ordentlich geschrieben.

Ich verstehe aber nach wie vor nicht warum die Laufvariable nicht 
inkrementiert wird.

[Karl H. (kbuchegg)]

Markus schrieb:

> Ich verstehe aber nach wie vor nicht warum die Laufvariable nicht
> inkrementiert wird.

Wir auch nicht.
Bitte!
Beschreib nicht deinen Code! Poste ihn einfach. Copy&Paste gibt es jetzt 
seit fast 15 Jahren. Das ist für dich einfacher und weniger Tipparbeit 
und für uns ist es auch einfacher.

> Sobald der Code funktioniert wird er bereinigt und ordentlich
> geschrieben

Falsche Vorgehensweise.
Schreib ihn gleich ordentlich und bereinigt.
Eine nicht unbeträchtliche Anzahl an Fehlern entsteht nur dadurch, dass 
der Code eben nicht ordentlich und bereinigt ist.

[spess53 (Gast)]

Hi

>(Ich schwör: irgendwann bau ich ein Expertensystem, das eine Anfrage
>analysiert und wenn es um Programmieren geht, den Code durch den

Gute Idee. Hoffentlich nicht nur für C.

MfG Spess

[Markus (Gast)]

Immer mit der Ruhe, ein alter Mann ist kein D-Zug :)

Hier der Rest des Codes:

Die globalen Variablen:

static volatile unsigned char strom_irq = 0, channel = 0, i = 0;
static volatile unsigned int ADC_channel[4];

Die Init des Timer 0 zur PWM-Generierung:

void PWM_init(void)
{
  // Form generation mode using WGM bits and Compare output mode using COM bits. Phase correct Mode 1
  TCCR0A |= (1<<WGM00) | (1<<COM0A1) | (1<<COM0B1);

  // Set the Clock select bits to determine Clock frequency. Prescaler = 1
  TCCR0B |= (1<<CS00);

  // Enable OC0A and OC0A_Reverse as outputs = 1
  DDRD |= (1<<PD5) | (1<<PD0);
}

Die Funktion die den Motor antreibt:

void motor_pwm(unsigned char SPD_M, char DIR_M)
{
  if(DIR_M)              // BACKWARD
  {
    PORTD |= (1<<PD0);
    OCR0A = ~SPD_M;
  }
  else                  // FORWARD
  {
    PORTD &= ~(1<<PD0);
    OCR0A = SPD_M;
  }
}

Und die Main:

int main (void)
{
  unsigned int strom = 0;

  PWM_init();
  Init_adc();            // Initialisierung des ADC

  motor_pwm(127,FORWARD);          // FORWARD = 0, BACKWARD = 1

  for(;;)              // Endless loop
  {
    if(strom_irq == 1)
    {
      ADC_channel[0] /= 4;      // Mittelwert aus vier Messungen bilden für ADC0-Kanal
      strom = (ADC_channel[0] * 1100) / 1024;  // Stromwert berechnen
      strom_irq = 0;
    }
  }

  return 0;
}

Mehr als das gibt es bisher noch nicht. Möchte noch eine Übertragung 
meiner gemessenen Werte per UART realisieren, aber das hat noch Zeit.

[Markus (Gast)]

Hat keiner eine Idee warum es nicht funktionieren könnte? :(

Habe schon mehrere Varianten ausprobiert, aber er zählt meine 
Laufvariable einfach nicht hoch.

[Stefan B. (stefan)]

Du wurdest gebeten, den kompletten Quelltext zu schicken.
Das würde ich an deiner Stelle machen z.B. als Anhang.

Wenn du ein Projekt mit hochgeheimen Nebenfunktionen hast, kannst du ja 
diese entfernen. Dabei darauf achten, dass der Fehler nicht verschwindet 
bzw. wäre das Verschwinden des Fehlers bereits ein hilfreiches Indiz.

[Karl H. (kbuchegg)]

Hmm. Du hast den Auto Trigger auf den Compare Match vom Timer 0 gelegt.
D.h. der ADC wird mit der steigenden Flanke des zugehörigen Interrupt 
Flags gestartet.
Da du aber keine Compare Match ISR hast, wird dieses Compare Match 
Interrupt Flag nie zurückgesetzt und das Flag ist beim nächsten Compare 
Match schon gesetzt. Damit hast du aber keine steigende Flanke mehr und 
damit auch keinen Trigger für den ADC

Ich sagte doch: Hast du abgeklärt, ob die ADC-ISR regelmässig 
aufgerufen wird? Wenn dein Debug-Test nicht spezifisch genug ist, dann 
verwechselst du einen einmaligen Aufruf mit einem regelmässigen Aufruf. 
Die ISR wird offenbar nur einmal aufgerufen und dann nie wieder.

[spess53 (Gast)]

Hi

>Du hast den Auto Trigger auf den Compare Match vom Timer 0 gelegt.
>D.h. der ADC wird mit der steigenden Flanke des zugehörigen Interrupt
>Flags gestartet.
>Da du aber keine Compare Match ISR hast, wird dieses Compare Match...

Dessen braucht es auch nicht. Es reicht das Flag im ADC-Interrupt 
zurückzusetzen.

Datenblatt:
A conversion can thus be triggered without causing an interrupt. 
However, the Interrupt Flag must be cleared in order to trigger a new 
conversion at the next interrupt event.

MfG Spess

[Karl H. (kbuchegg)]

spess53 schrieb:
> Hi
>
>>Du hast den Auto Trigger auf den Compare Match vom Timer 0 gelegt.
>>D.h. der ADC wird mit der steigenden Flanke des zugehörigen Interrupt
>>Flags gestartet.
>>Da du aber keine Compare Match ISR hast, wird dieses Compare Match...
>
> Dessen braucht es auch nicht. Es reicht das Flag im ADC-Interrupt
> zurückzusetzen.

Ist auch eine Möglichkeit.
Aber auf jeden Fall muss er sich um das Flag kümmern.

> Datenblatt:
> A conversion can thus be triggered without causing an interrupt.
> However, the Interrupt Flag must be cleared in order to trigger a new
> conversion at the next interrupt event.

Den Teil hab ich wiederrum im DB überlesen und indirekt erschlossen :-)

[Markus (Gast)]

Danke Karl heinz, genau das war das Problem. Daran hatte ich überhaupt 
nicht mehr gedacht.

LG Markus

[Markus (Gast)]

Habe zu spät gesehen dass es schon neue Beträge gab, als ich meinen 
geschrieben hatte.

TIFR0 |= (1<<OCF0A);

Genauso habe ich es auch gemacht. Ich setze das Flag innerhalb des 
ADC-Interrupts einfach zurück.

[Karl H. (kbuchegg)]

Markus schrieb:
> Habe zu spät gesehen dass es schon neue Beträge gab, als ich meinen
> geschrieben hatte.
>
>

TIFR0 |= (1<<OCF0A);

>
> Genauso habe ich es auch gemacht. Ich setze das Flag innerhalb des
> ADC-Interrupts einfach zurück.

Falsch.
Interrupt Flags werden nicht so zurückgesetzt, sondern so

   TIFR0 = (1<<OCF0A);

[Markus (Gast)]

Beide Varianten funktionieren, zumindest bei mir.

Ich muss das Flag auf 1 setzen und dies tun beide Varianten. Der Status 
der anderen Flags dieses Registers interessieren mich nicht da ich sie 
nicht benötige. Verstehe nicht wirklich wo der Unterschied sein soll. 
Könntest du mich bitte aufklären? :)

[Karl H. (kbuchegg)]

Markus schrieb:
> Beide Varianten funktionieren, zumindest bei mir.
>
> Ich muss das Flag auf 1 setzen und dies tun beide Varianten. Der Status
> der anderen Flags dieses Registers interessieren mich nicht da ich sie
> nicht benötige. Verstehe nicht wirklich wo der Unterschied sein soll.
> Könntest du mich bitte aufklären? :)

Der Unteschied ist:

Bei deiner Variante werden ALLE Interrupt Flags in diesem Register 
gelöscht :-) Auch die, die an dieser Stelle gar nicht gelöscht
werden sollen.
Bei meiner nur das OCF0A Flag. So wie es sein soll.

(Interrupt Flags werden gelöscht, indem man an das Register eine Maske 
zuweist, die genau bei den Flags eine 1 aufweist, die zurückgesetzt 
werden sollen.

In
   TIFR0 =  TIFR0  |  (1<<OCF0A);
enthält die Maske aber eine 1 für jedes momentan gesetzte Interrupt Flag
)

[Markus (Gast)]

Du schiebst im Prinzip eine 1 an die Stelle von OCF0A und setzt damit 
das Flag zurück. Aber bei meiner Variante geschieht doch das gleiche?

Ich verknüpfe das gesamte Register und seinen aktuellen Wert per ODER 
mit einem Wert der genau an der Stelle von OCF0A eine 1 hat. Durch das 
ODER bleibt doch der Rest des Registers erhalten, nur OCF0A auf 1 
gesetzt. Durch die Verknüpfung der anderen Stellen mit einer 0 bleibt 
das alles erhalten?

Oder steh ich jetzt komplett auf dem Schlauch? :)

[Karl H. (kbuchegg)]

Markus schrieb:
> Du schiebst im Prinzip eine 1 an die Stelle von OCF0A und setzt damit
> das Flag zurück. Aber bei meiner Variante geschieht doch das gleiche?

Nein.
Du hast

   TIFR0 =  TIFR0  |  (1<<OCF0A);

der Teil rechts vom = hat eine 1 sowhl an der Stelle OCF0A als auch an 
allen Stellen in denen TIFR0 schon eine 1 hatte

> Ich verknüpfe das gesamte Register und seinen aktuellen Wert per ODER
> mit einem Wert der genau an der Stelle von OCF0A eine 1 hat.

Eben.
Genau das willst du nämlich NICHT!

> Durch das
> ODER bleibt doch der Rest des Registers erhalten, nur OCF0A auf 1
> gesetzt.

Richtig.
Und genau das willst du nicht.
Erinnere dich: Überall dort, wo du ein 1 Bit hast, wird das Interrupt 
Flag zurückgesetzt.

Angenommen TIFR0 enthalte  0b01001110
OCF0A sei meinetwegen Bit Nr 7 (spielt im Grunde keine Rolle)
D.h. TIFR0 | (1<<OCF0A) wird zu 0b11001110

Jetzt nimmst du das TIFR0          0b01001110

und setzt alle Bits auf 0
bei denen hier                     0b11001110
ein 1 Bit ist

Was kommt raus?                    0b00000000

Ähm.Da ist ein bischen mehr auf 0 gegangen. Nicht nur Bit 7.

Das sind keine normalen Zuweisungen. Der AVR verhält sich hier anders! 
Und nebenbei bemerkt, wenn das eine normale Zuweisung wäre, dann würde 
man das Flag ja wohl so löschen

  TIFR0 &= ~(1<<OCF0A);

> Oder steh ich jetzt komplett auf dem Schlauch? :)

Sieht so aus.

[Markus (Gast)]

Ach ja, natürlich. Hast recht, jetzt versteh ich was du meintest.

Vielen Dank nochmal :)