Atmega/Arduino - Input Capture funktioniert nicht

[Andreas S. (igel1)]

Liebe AVR'ler,

ich steh grad irgendwie auf der Leitung - finde den Fehler nicht:

Ich möchte mit untenstehendem Code einen Puls, der
zwischen ca. 16us - 30 us schwankt, vermessen.

Dazu möchte ich nicht die pulseIn() Funktion von Arduino verwenden (die 
hat nur 1us Auflösung), sondern den Input Capture Mode von Timer1 meines 
Arduino Nano Clones (hat einen Atmega328p).

Leider erhalte ich auf meinem OLED-Display "kippende" Werte angezeigt:
z.B.  abwechselnd 14 und 65522,
oder 13 und 65523,
ab und an auch schon mal nur 0.

Neben den kippenden Werten wundert mich auch, dass ich nicht Werte um 
16us/(1/16Hz)= 254 herum erhalte.

Bitte schaut Euch einmal den Code an - ich bin irgendwie zu blind, den 
Fehler zu finden.

Viele Grüße

Igel1

#include <Arduino.h>
#include <U8g2lib.h>
#include <SPI.h>
#include <Wire.h>

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


// Set pinout
int pin = 8;                 // pin for puls input from oscillator

// Constructor - for OLED display library - see u8g2 documentation
U8G2_SSD1309_128X64_NONAME0_F_4W_HW_SPI u8g2(U8G2_R0, 10, 9, 7);  

volatile uint16_t rising_capture = 0;
volatile uint16_t falling_capture = 0;
volatile uint16_t pulse_width = 0;
volatile bool rising_edge = true;
volatile bool isFinished = false;

char buffer[32];


void setup() {
  pinMode(pin, INPUT);
  u8g2.begin();                // start the u8g2 library
  u8g2.setBitmapMode(1);
  u8g2.setFont(u8g2_font_helvB08_tr);
}

void loop() {
    pulse_width = 0;
    falling_capture = 0;
    rising_capture = 0;
    isFinished = false;

    TCCR1B |= (1 << ICES1);       // input capture set for rising edge
    TCCR1B |= (1 << CS10);        // no prescaler
    TIMSK1 |= (1 << ICIE1);       // input capture interrupt enable
   
    while(isFinished == false) {};
    
    pulse_width = falling_capture - rising_capture;
    
    u8g2.clearBuffer();
    sprintf(buffer, "Pulse: %u", pulse_width);
    u8g2.drawStr(10, 64, buffer);
    u8g2.sendBuffer();
    delay(1000);
}


ISR(TIMER1_CAPT_vect)
{
    switch(rising_edge){
        case true:
            //TCNT1 = 0;             // reset the timer counter 
            rising_capture = ICR1;   // take the input capture value and
                                     // save it in rising_capture 
            TCCR1B &= ~(1 << ICES1); // set input capture for falling edge
            rising_edge = false;     // make rising_edge variable false 
                                     // so next time it will react to 
                                     // case false
            break;
        
        case false:
            falling_capture = ICR1; // take input capture value and 
                                    // save it in falling edge
            TCCR1B |= (1 << ICES1); // set input capture for rising 
                                    // edge back
            rising_edge = true;     // make rising edge true for next 
                                    // pulse
            TIMSK1 &= ~(1 << ICIE1); // disable input capture interrupt
            isFinished = true;       // mark pulse measurement finished
            break;
    }
}

[Arduino F. (Firma: Gast) (arduinof)]

Arduino:
Der Timer ist für PWM vorbereitet

Man sollte bevor man ihn neu konfiguriert alle wichtigen Register auf 
Null setzen.
z.B.
TCCR1A = 0;
TCCR1B = 0;
in setup()

[Helmut H. (helmuth)]

Andreas S. schrieb:
> Leider erhalte ich auf meinem OLED-Display "kippende" Werte angezeigt:
> z.B.  abwechselnd 14 und 65522,

Da TCNT1 nicht auf 0 gesetzt wird (ist auskommentiert) tritt das 
beschrieben Verhalten auf, wenn der Zähler während der Messung 
überläuft.

 pulse_width = falling_capture - rising_capture;

Dann ist die Differenz negativ, das ist unsigned 655xx.

Andreas S. schrieb:
> Neben den kippenden Werten wundert mich auch, dass ich nicht Werte um
> 16us/(1/16Hz)= 254 herum erhalte.

siehe Vorredner, TCCR1B muss erst auf 0 gesetzt werden, wahrscheinlich 
sind CS11 oder CS12 gesetzt, das ergibt dann einen anderen Prescaler.

   TCCR1B |= (1 << CS10);        // no prescaler

[Falk B. (falk)]

Andreas S. schrieb:
> Neben den kippenden Werten wundert mich auch, dass ich nicht Werte um
> 16us/(1/16Hz)= 254 herum erhalte.
>
> Bitte schaut Euch einmal den Code an - ich bin irgendwie zu blind, den
> Fehler zu finden.

Dein Ansatz ist richtig, die Umsetzng in einigen wichtigen Details 
nicht.
Variabelen, welche in einer ISR und im Hauptprogramm gelesen oder 
geschrieben werden, müssen sowohl volatile deklariert werden (hast du) 
als auch im Hauptprogramm atomar gelesen und geschrieben werden. SIehe 
Interrupt. Wnn Variablen aber NUR in der ISR benutzt werden, müssen 
sie nicht volatile sein.

Die Initialisierung des Timers macht man EINMALIG in setup() und nicht 
dauernd in loop()! Bei einer Bool-Variablen nimmt man im Normalfall kein 
Switch, denn außer true und false gibt es da nix, ein if/else reicht.
Bei Input Capture läßt man den Zähler in den meisten Fällen immer 
laufen. Man muss ihn NICH auf Null setzen, um die Pulsbreite zu 
bestimmen! Die Differenz falling_capture-rising_capture stimmt auch beim 
Überlauf!

TCCR1B &= ~(1 << ICES1); // set input capture for falling edge

Das kann schief gehen. Du invertierst hier das Bit. Damit kann man aber 
Pech haben, wenn sie deine Logik desynchronisiert. Hier ist es besser, 
die Bits explizit zu setzen. Das Gleiche gilt für deine Variable 
rising_edge. Hier is es besser und einfacher, die aktuellen Bits des 
Steuerregisters zu nutzen. Am Ende kann man die Pulsbreite auch gleich 
in der ISR berechnen, so eine einfache Differenz ist sehr schnell.

#include <Arduino.h>
#include <U8g2lib.h>
#include <SPI.h>
#include <Wire.h>
#include <avr/io.h>
#include <util/delay.h>
#include <util/atomic.h>
#include <avr/interrupt.h>

// Set pinout

int pin = 8;                 // pin for puls input from oscillator

// Constructor - for OLED display library - see u8g2 documentation

U8G2_SSD1309_128X64_NONAME0_F_4W_HW_SPI u8g2(U8G2_R0, 10, 9, 7);  
volatile uint16_t pulse_width = 0;
volatile bool isFinished = false;
char buffer[32];

void setup() {

  pinMode(pin, INPUT);
  u8g2.begin();                // start the u8g2 library
  u8g2.setBitmapMode(1);
  u8g2.setFont(u8g2_font_helvB08_tr);

  isFinished = false;
  TCCR1A  = 0;
  TCCR1B  = (1<<ICNC1) | (1<<ICES1) | (1<<CS11);
  TIMSK1 |= (1<<ICIE1);

}

void loop() {
    uint16_t tmp;
 
    isFinished = false;
    while(!isFinished);

    ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
        tmp = pulse_width;
    }

    u8g2.clearBuffer();
    sprintf(buffer, "Pulse: %u", tmp);
    u8g2.drawStr(10, 64, buffer);
    u8g2.sendBuffer();
    delay(1000);
}

ISR(TIMER1_CAPT_vect)
{
    uint16_t tr, tf;

    if (TCCR1B & (1<<ICES1) ) {             // rising edge
        tr = ICR1;
        TCCR1B  = (1<<ICNC1) | (1<<CS11);   // next trigger on falling edge
    } else {                                // falling edge
        tf = ICR1;
        TCCR1B  = (1<<ICNC1) | (1<<ICES1) | (1<<CS11);  // next trigger on rising edge
        pulse_width = tf-tr;
        isFinished = true;
    }
}

[Falk B. (falk)]

Helmut H. schrieb:
> Da TCNT1 nicht auf 0 gesetzt wird (ist auskommentiert) tritt das
> beschrieben Verhalten auf, wenn der Zähler während der Messung
> überläuft. pulse_width = falling_capture - rising_capture;
> Dann ist die Differenz negativ, das ist unsigned 655xx.

Nein, eben nicht. Im Falle des Zählerüberlauf stimmt die Differenz 
trotzdem!

rising_capture = 65000
Zählerüberlauf
falling_capture = 1000
pulse_width = falling_capture - rising_capture
            = 1000 - 65000 = -64000 = 1536;

Die -64000 erzeugen selber wieder einen Überlauf, wodurch daraus ein 
positives, richtiges Ergebnis wird! Einzige Randbedingung. Die Differenz 
zwischen den Werten darf nicht größer als der halbe Zählerumfang sein, 
als max. 32767!

[Mi N. (msx)]

Andreas S. schrieb:
> Ich möchte mit untenstehendem Code einen Puls, der
> zwischen ca. 16us - 30 us schwankt, vermessen.

Da kommt es aber auch noch auf das Tastverhältnis bzw. Wiederholfrequenz 
an.
Wenn nur 1 µs Abstand besteht, kann es nicht klappen, da die ISR nicht 
hinterherkommt.
Vielleicht läßt Du Dir die +PW und -PW anzeigen, um mehr Durchblick zu 
erhalten.

Arduino F. schrieb:
> Arduino:
> Der Timer ist für PWM vorbereitet

Bist Du Dir da sicher? Soweit ich es verfolgt habe, wird erst dann 
'vorbereitet', wenn PWM auch verwendet wird.

[Falk B. (falk)]

Falk B. schrieb:
> TCCR1B &= ~(1 << ICES1); // set input capture for falling edge
>
> Das kann schief gehen. Du invertierst hier das Bit.

Was hab ich denn da für einen Unsinn geschrieben? War wohl noch zu früh 
am Morgen! Es ist natürlich ein korrektes Bitlöschen!

[Veit D. (devil-elec)]

Mi N. schrieb:

> Arduino F. schrieb:
>> Arduino:
>> Der Timer ist für PWM vorbereitet
>
> Bist Du Dir da sicher? Soweit ich es verfolgt habe, wird erst dann
> 'vorbereitet', wenn PWM auch verwendet wird.

Hallo,

die Timer werden teilweise mittels setup() vorbereitet. Nur das was mit 
dem Pin direkt zu tun hat noch nicht. Viele Worte ohne Sinn, schau dir 
den Testcode an was wann passiert.

#include <Streaming.h>  // https://github.com/janelia-arduino/Streaming
Stream &out {Serial};

const uint8_t pinOutM {12};   // OC1B Arduino Mega2560
const uint8_t pinOutU {10};   // OC1B Arduino Uno R3 

void setup(void)
{
  Serial.begin(9600);
  Serial.println(F("\nuC Reset ####\n"));  
  pinMode(pinOutM, OUTPUT);
  pinMode(pinOutU, OUTPUT);
  readT1Register(Serial);
  analogWrite(pinOutM, 128);
  analogWrite(pinOutU, 128);
  readT1Register(Serial);
}

void loop(void)
{ }

void readT1Register (Stream &out)
{
  out << "TCCR1A " << _BIN(TCCR1A) << endl;
  out << "TCCR1B " << _BIN(TCCR1B) << endl;
  out << "TCCR1C " << _BIN(TCCR1C) << endl;
  out << "TIMSK1 " << _BIN(TIMSK1) << endl;
  out << "TIFR1  " << _BIN(TIFR1)  << endl;
  out << "ICR1   " << ICR1  << endl;
  out << "OCR1A  " << OCR1A << endl;
  out << "OCR1B  " << OCR1B << endl;
  out << "OCR1C  " << OCR1C << endl;
  out.println();
}

[Arduino F. (Firma: Gast) (arduinof)]

Mi N. schrieb:
> Bist Du Dir da sicher?

Aber sowas von!

Der Beweis: 
https://github.com/arduino/ArduinoCore-avr/blob/master/cores/arduino/wiring.c#L297

[Andreas S. (igel1)]

Erst einmal 1000 Dank an Euch alle, die Ihr Euch so viele Mühen mit 
Euren Antworten gemacht habt - einfach toll.

Auf die Schnelle habe ich  Arduino F.'s Hinweis ausprobiert und die 
Steuerregister genullt:

TCCR1A = 0;
TCCR1B = 0;

Das hat die Werte zumindest schon mal in die richtige Region gebracht - 
nämlich zwischen ca. 240 und 1200 (je nach Frequenz meines Oszillators, 
den ich an Pin 8 (nach Arduino-Zählweise) hängen habe - Tastverhältnis 
ist ca. 50%).

Trotzdem flippen die Werte noch irgendwie herum - beim aktuell 
eingestellten Oszillatorwert/Eingangssignal z.B. zwischen

1189,
0,
394,
784

Da das alles in der Region von Vielfachen von 394 liegt, habe ich die 
Vermutung, dass der Capture nicht alle Flanken richtig mitbekommt.

Leider habe ich aktuell Null Zeit und kann Eure weiteren Tipps erst 
heute Abend oder morgen in Ruhe lesen und ausprobieren.

Dann werde ich mir nochmals die Register genau anschauen, ob da ggf. 
noch etwas genullt werden muss und ich werde Falk's und Veit D.'s 
Hinweise nochmals genau lesen und umsetzen.

Daher: bitte habt noch etwas Geduld - ich werde hier in jedem Fall 
berichten, wie die Sache weitergeht.

Viele Grüße

Igel1

[Sebastian W. (wangnick)]

Falk B. schrieb:
> uint16_t tr

Falk B. schrieb:
> Was hab ich denn da für einen Unsinn geschrieben? War wohl noch zu früh
> am Morgen!

LG, Sebastian

[Mi N. (msx)]

Arduino F. schrieb:
> Aber sowas von!
>
> Der Beweis:
> 
https://github.com/arduino/ArduinoCore-avr/blob/master/cores/arduino/wiring.c#L297

Gut. Wiring.c soll intern sogar noch vor setup() aufgerufen werden. Das 
soll der 'normale' Benutzer aber garnicht wissen.
Vermeiden kann man all dies, indem man kein setup() aufruft und main() 
verwendet ;-)

Letztens hatte ich Arduino für den RP2040 am Wickel, wo die 
Initialisierungen wohl erst nach Bedarf zur Laufzeit erfolgen. Da die 
ISR-Vektoren im RAM liegen ist das kein Problem.

[Falk B. (falk)]

Falk B. schrieb:
> ISR(TIMER1_CAPT_vect)
> {
>     uint16_t tr, tf;

Da fehlt ein static, aua. Und beim Prescaler hab ich auch noch nen 
Fehler drin. Eher so

#include <Arduino.h>
#include <U8g2lib.h>
#include <SPI.h>
#include <Wire.h>
#include <avr/io.h>
#include <util/delay.h>
#include <util/atomic.h>
#include <avr/interrupt.h>

// Set pinout

int pin = 8;                 // pin for puls input from oscillator

// Constructor - for OLED display library - see u8g2 documentation

U8G2_SSD1309_128X64_NONAME0_F_4W_HW_SPI u8g2(U8G2_R0, 10, 9, 7);  
volatile uint16_t pulse_width;
volatile bool isFinished;
char buffer[32];

void setup() {

  pinMode(pin, INPUT);
  u8g2.begin();                // start the u8g2 library
  u8g2.setBitmapMode(1);
  u8g2.setFont(u8g2_font_helvB08_tr);

  TCCR1A  = 0;
  TCCR1B  = (1<<ICNC1) | (1<<ICES1) | (1<<CS10);
  TIMSK1 |= (1<<ICIE1);
}

void loop() {

    uint16_t tmp;

    isFinished = false;
    while(!isFinished);

    ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
        tmp = pulse_width;
    }
    u8g2.clearBuffer();
    sprintf(buffer, "Pulse: %u", tmp);
    u8g2.drawStr(10, 64, buffer);
    u8g2.sendBuffer();
    delay(1000);
}

ISR(TIMER1_CAPT_vect)
{
    static uint16_t tr;
    uint16_t tf;

    if (TCCR1B & (1<<ICES1) ) {             // rising edge
        tr = ICR1;
        TCCR1B  = (1<<ICNC1) | (1<<CS10);   // next trigger on falling edge
    } else {                                // falling edge
        tf = ICR1;
        TCCR1B  = (1<<ICNC1) | (1<<ICES1) | (1<<CS10);  // next trigger on rising edge
        pulse_width = tf-tr;
        isFinished = true;
    }
}

[Andreas S. (igel1)]

Wow - hier fliegen einem ja die gebratenen Gänse in den Mund  (... die 
alle von Falk ferngesteuert werden ...).

Fertiger, mundgerechter Code - das ist ja mehr als Service!

Bei so viel Engagement musste ich dann doch noch etwas Zeit von unserem 
Ausflug abknappsen:

Habe Falks Code schnell kopiert, kompiliert und gebrannt -> läuft out of 
the box wie am Schnürchen. Toll!

Respekt an Falk (aber auch alle anderen für Ihre super Hinweise). Den 
Rest arbeite ich in Ruhe heute Abend oder morgen durch.

Viele Grüße und nochmals dickes Dankeschön!

Igel1

[Falk B. (falk)]

Hmm, obwohl es so funktioniert und ich so auch schon ein Projet gemacht 
habe, muss man nach der Doku von Atmel/Microchip das Flag ICF1 im 
Register TIFR1 löschen, wenn die Flanke vom ICP1 geändert wird.
Also sicherheitshalber noch ein

TIFR1 = (1<<ICF1);

Ans Ende der ISR.

Siehe Datenblatt ATmega 328

16.6.3 Using the Input Capture Unit

"Measurement of an external signal’s duty cycle requires that the 
trigger edge is changed after each capture. Changing the edge sensing 
must be done as early as possible after the ICR1 Register has been read. 
After a change of the edge, the Input Capture Flag (ICF1) must be 
cleared by software  writing a logical one to the I/O bit location). For 
measuring frequency only, the clearing of the ICF1 Flag is not required 
(if an interrupt handler is used)."

[Andreas S. (igel1)]

Danke, Falk, für den Nachtrag - ist eingebaut.

Ich habe auch nochmals all Eure Anmerkungen schön durchgelesen und 
nachvollzogen - inklusive Link von Falk.

Danke für die Tipps - sie haben mir gut aufs Pferd geholfen!

Viele Grüße

Igel1