Naked ISR hier ok?

[Ingo L. (corrtexx)]

Hallo,

ich habe ein paar Zeilen Code, es spielt sich absolut alles in der ISR 
ab, von daher ist die Frage, ob die ISR hier "naked" sein darf. Der 
Pro-/Epilog einer normalen ISR macht schon ganz ordentlich Laufzeit bei 
einem Systemtakt von 128kHz.

int main ( void )
{
  _delay_ms( 100 );

  // Pulse Output
  PULSE_HIGH();
  PULSE_DDR |= (1<<PULSE_PIN);
  
  // Disable AD-Comp
  ACSR |= (1<<ACD);
  
  // Unused Pin low
  DDRB |= (1<<PB2) | (1<<PB3) | (1<<PB4);
  PORTB &= ~( (1<<PB2) | (1<<PB3) | (1<<PB4) );
  
  //Setup AVR
  OCR0A = 111;
  TCCR0A |= (1<<WGM01);      // CTC
  TCCR0B |= (1<<CS00);      // Prescaler = 1
  TIMSK0 |= (1<<OCIE0A);      // Interrupt enabler
  
  //External Interrupt
  PORTB |= (1<<PB1);          // internal Pullup enable
  MCUCR |= (1<<ISC01);        // Trigger on falling edge
  CLEAR_SENSOR_FLAG();
  
  // Global Interrupt enable
  sei();
  
    /* Nice weather... */
    while ( "Cycling" );
}


ISR ( TIMEBASE_1MS, ISR_NAKED )
{
  static uint16_t TimeCounter = 0;
  static uint16_t RoundTrip[2] = {0,0};
  static uint16_t TimeDifference = PULSE_PERIOD_MAX_MS;
  static uint16_t Pulsegenerator = UINT16_MAX;
  static uint16_t TunedOutputPeriod = PULSE_PERIOD_MAX_MS;
  static uint16_t Timeout = ZERO_TIME_DETECT;

  // Nullerkennung
  if ( Timeout ) Timeout--;

  // Impuls Erfassung  
  TimeCounter++;
  
  if ( SENSOR_TRIGGERD ){
    Timeout = ZERO_TIME_DETECT;
    CLEAR_SENSOR_FLAG();
    RoundTrip[NEW] = TimeCounter;
    TimeDifference = RoundTrip[NEW] - RoundTrip[OLD];
    RoundTrip[OLD] = RoundTrip[NEW];
  }

  // Impuls Ausgabe
  if ( TimeDifference < PULSE_PERIOD_MIN_MS ){ // abgeregelt
    TunedOutputPeriod = PULSE_PERIOD_MIN_MS;
  }else{// unmanipuliert
    TunedOutputPeriod = TimeDifference;
  }
  
  if ( TunedOutputPeriod > PULSE_PERIOD_MAX_MS ) TunedOutputPeriod = PULSE_PERIOD_MAX_MS; 
  
  if ( Pulsegenerator >= TunedOutputPeriod ) Pulsegenerator = 0;
  else Pulsegenerator++;
  
  if ( Pulsegenerator < PULSE_DURATION_MS && Timeout ) PULSE_LOW();
  else PULSE_HIGH();

  reti();
}

Funktionieren tut es, aber gibts hier irgendwelche Fallstricke?

[Oliver S. (oliverso)]

Rein theoretisch sollte die Endlosschleife im Hauptprogramm zu einem 
einfachen jmp zusammenschrumpfen, dem es egal ist, ob die ISR 
zwischenzeitlich Register oder Flags ändert.

Rein praktisch solltest du dir den generierten Assemblercode anschauen, 
ob das auch so tatsächlich ist.

Oliver

[Ingo L. (corrtexx)]

 8c:  ff cf         rjmp  .-2        ; 0x8c <main+0x44>

ja, passiert

[Bernd K. (prof7bit)]

Ich würde mir überlegen ob es in diesem konkreten Fall nicht sogar 
sinnvoller wäre komplett auf den Interrupt zu verzichten und 
stattdessen in der main-loop das entsprechende Timer-Bit zu pollen und 
alles in der main zu machen.

[Ingo L. (corrtexx)]

Bernd K. schrieb:
> Ich würde mir überlegen ob es in diesem konkreten Fall nicht sogar
> sinnvoller wäre komplett auf den Interrupt zu verzichten und
> stattdessen in der main-loop das entsprechende Timer-Bit zu pollen und
> alles in der main zu machen.
Wo wäre da der Vorteil gegenüber der jetzigen Lösung? Ich würde sie in 
diesem Fall sogar als absolut gleichwertig sehen

[Peter D. (peda)]

Ingo L. schrieb:
> Wo wäre da der Vorteil gegenüber der jetzigen Lösung? Ich würde sie in
> diesem Fall sogar als absolut gleichwertig sehen

Nö, die Loop hat mehrere Vorteile:
Bit testen, löschen und zurück springen geht schneller als 
Interrupteinsprung und Return.
Der Compiler merkt, daß die Loop nie verlassen wird und
wird viele Variablen in Registern halten.

Ingo L. schrieb:
>     while ( "Cycling" );

Ist jetzt nicht so die feine Art, einen Stringpointer als Bool zu 
vergewaltigen.
C erlaubt viele Schweinereien, man sollte sie aber nicht grundlos 
verwenden.
Heutzutage ist es hip, der Lesbarkeit den Vorzug zu geben.

[Bernd K. (prof7bit)]

Peter D. schrieb:
>>     while ( "Cycling" );
>
> C erlaubt viele Schweinereien, man sollte sie aber nicht grundlos
> verwenden.
> Heutzutage ist es hip, der Lesbarkeit den Vorzug zu geben.

Also wenn lesbar dann so:

while ("my guitar gently weeps");

[Niklas G. (erlkoenig)]

Peter D. schrieb:
> Bit testen, löschen und zurück springen geht schneller als
> Interrupteinsprung und Return.

Dafür kann man hier keinen Power Down Modus nutzen; wenn man Interrupts 
nimmt, kann man in der while-Schleife einen solchen aktivieren um 
Energie zu sparen. Beim AVR braucht das aber ggf. Register-Zugriffe...

[Bernd K. (prof7bit)]

Ingo L. schrieb:
>> stattdessen in der main-loop das entsprechende Timer-Bit zu pollen und
>> alles in der main zu machen.
> Wo wäre da der Vorteil gegenüber der jetzigen Lösung? Ich würde sie in
> diesem Fall sogar als absolut gleichwertig sehen

Konkretes Beispiel:

Ich hab hier einen aktuellen Fall wo ich eine 80kHz PLL auf einem Silabs 
8051 (EFM8BB10) implementierte. Der erste Versuch verwendete den 
ADC-Interrupt (der dann mit 320kHz kam) und fast die ganze CPU-Zeit ging 
dafür drauf in den Interrupt hinein und wieder hinaus zu springen, 
zusätzlich noch der Overhead bis 4 zu zählen und jedesmal zu schauen 
welche der 4 Werte (I, Q, -I, -Q) jetzt gerade gesampelt wurde. Ich war 
schon bei 80% CPU allein vom Grundgerüst ohne schon irgendwas anderes zu 
machen als nur Samples einzulesen und zuzuordnen. Die Zeit hat nicht 
mehr gereicht noch irgendwas sinnvolles damit zu machen, es ging so 
nicht.

Im zweiten Anlauf machte ich eine main-loop die pro Durchlauf 4 mal auf 
das conversion complete bit wartet, danach jeweils ein bisschen was tut 
(erst dies, dann das, eben alles was einmal pro Periode zu tun ist schön 
verteilt), die Zeit reicht jetzt gut aus für alles und könnte jetzt 
sogar bis 100kHz gehen wenn ich wollte.

[egberto (Gast)]

Wenn das mal richtig läuft, kannst du den kompletten Code ja unter 
"Fahrzeugelektronik" Thema E-Bike Tuning posten ;-)


Grüße,

egberto

[Thomas S. (thschl)]

Bernd K. schrieb:
> while ("my guitar gently weeps");

dann lieber was sinnvolles und  my_boobs_gently_weeps als TRUE 
definieren

[Jörg W. (dl8dtl) (Moderator)]

Niklas G. schrieb:
> Beim AVR braucht das aber ggf. Register-Zugriffe...

Wenn man sleep_mode() nimmt, ja, weil das ja das Äquivalent zu

  sleep_enable();
  sleep_cpu();
  sleep_disable();

ist. Wenn man jedoch auf die (eigentlich empfohlene) Abfolge verzichtet 
und das SE-Bit immer gesetzt lässt:

  sleep_enable();

  while("awake")
    sleep_cpu();

dann ist das ja nur ein inline asm für den SLEEP-Befehl, der sich 
natürlich nicht weiter um Register kümmert.

[Thomas E. (thomase)]

Niklas G. schrieb:
> Dafür kann man hier keinen Power Down Modus nutzen; wenn man Interrupts
> nimmt, kann man in der while-Schleife einen solchen aktivieren um
> Energie zu sparen.

Guter Einwand. Dumm nur, daß der Timer dann nicht läuft.

[Niklas G. (erlkoenig)]

Thomas E. schrieb:
> Guter Einwand. Dumm nur, daß der Timer dann nicht läuft.

Kommt auf den konkreten Modus an... Im am wenigsten sparsamen Modus 
("Idle"?) ist die Peripherie noch komplett an, nur der Prozessor ist 
aus, der ja aber typischerweise der größte Verbraucher ist.

[Thomas E. (thomase)]

Niklas G. schrieb:
> Kommt auf den konkreten Modus an... Im am wenigsten sparsamen Modus
> ("Idle"?

Vorhin wolltest du noch Power Down.

[Niklas G. (erlkoenig)]

Thomas E. schrieb:
> Vorhin wolltest du noch Power Down.

Ich meinte damit allgemein "einen der Sparmodi", nicht konkret den einen 
"Power Down" Mode. Habe nicht genau im Kopf welcher beim AVR jetzt was 
macht.

[Thomas E. (thomase)]

Niklas G. schrieb:
> Ich meinte damit allgemein "einen der Sparmodi",

Dann schreib das auch. Das heißt Sleep Mode. Sind wir hier bei 
gutefrage.net?

[Niklas G. (erlkoenig)]

Thomas E. schrieb:
> Dann schreib das auch. Das heißt Sleep Mode. Sind wir hier bei
> gutefrage.net?

Verzeih dass ich nicht jeden Controller-spezifischen Begriff auswendig 
weiß. Die Grundaussage ist das was zählt.

[Thomas E. (thomase)]

Niklas G. schrieb:
> Verzeih dass ich nicht jeden Controller-spezifischen Begriff auswendig
> weiß. Die Grundaussage ist das was zählt.

Nein, das heißt immer Sleep Mode.

[Niklas G. (erlkoenig)]

Thomas E. schrieb:
> Nein, das heißt immer Sleep Mode.

Bei den STM32F103 heißt das "Low-power modes". "Sleep mode" ist beim 
STM32F103 das was beim AVR der "Idle" ist.

[Peter D. (peda)]

Bernd K. schrieb:
> Also wenn lesbar dann so:
>
> while ("my guitar gently weeps");

Nein.
Entweder 1 oder true.

[Jörg W. (dl8dtl) (Moderator)]

Peter D. schrieb:
> Bernd K. schrieb:
>> Also wenn lesbar dann so:
>>
>> while ("my guitar gently weeps");
>
> Nein.
> Entweder 1 oder true.

Humordetektor kaputt?

Peter, du solltest doch nun wirklich alt genug sein, um den 
entsprechenden Beatles-Titel noch zu kennen …

[Ingo L. (corrtexx)]

egberto schrieb:
> Wenn das mal richtig läuft, kannst du den kompletten Code ja unter
> "Fahrzeugelektronik" Thema E-Bike Tuning posten ;-)
Funktioniert doch, ich habe die CPU jetzt auf 300kHz laufen, damit 
schaffe ich 2kHz Timerinterrupt.

int main ( void )
{
  _delay_ms( 100 );
  
  CLKPR = (1<<CLKPCE);
  CLKPR = (1<<CLKPS2);    // /16
  
  OSCCAL = 0x63;
  // Pulse Output
  PULSE_HIGH();
  PULSE_DDR |= (1<<PULSE_PIN);
  
  // Disable AD-Comp
  ACSR |= (1<<ACD);
  
  // Unused Pin low
  DDRB |= (1<<PB2) | (1<<PB3) | (1<<PB4);
  PORTB &= ~( (1<<PB2) | (1<<PB3) | (1<<PB4) );
  
  // Setup AVR
  OCR0A = 17;
  TCCR0A |= (1<<WGM01);      // CTC
  TCCR0B |= (1<<CS01);      // Prescaler = 1
  
  // External Interrupt
  PORTB |= (1<<PB1);          // internal Pullup enable
  MCUCR |= (1<<ISC01);        // Trigger on falling edge
  CLEAR_SENSOR_FLAG();

  
    /* Nice weather... */
    while ( "Cycling" ){
    if ( TIFR0 & (1<<OCF0A) ){
      #if DEBUG_MODE == 1
        PULSE_HIGH();
      #endif
      
      // Clear TimerFlag    
      TIFR0 |= (1<<OCF0A);
  
      // Nullerkennung
      if ( Timeout ) Timeout--;

      // Impuls Erfassung
      TimeCounter++;
  
      if ( SENSOR_TRIGGERD ){
        Timeout = ZERO_TIME_DETECT;
        CLEAR_SENSOR_FLAG();
        RoundTrip[NEW] = TimeCounter;
        TimeDifference = RoundTrip[NEW] - RoundTrip[OLD];
        RoundTrip[OLD] = RoundTrip[NEW];
      }

      // Impuls Ausgabe
      if ( TimeDifference < PULSE_PERIOD_MIN_MS ){ // abgeregelt
        TunedOutputPeriod = PULSE_PERIOD_MIN_MS;
      }else{// unmanipuliert
        TunedOutputPeriod = TimeDifference;
      }
  
      if ( TunedOutputPeriod > PULSE_PERIOD_MAX_MS ) TunedOutputPeriod = PULSE_PERIOD_MAX_MS;
  
      if ( Pulsegenerator >= TunedOutputPeriod ) Pulsegenerator = 0;
      else Pulsegenerator++;
      
      #if DEBUG_MODE == OFF
        if ( Pulsegenerator < PULSE_DURATION_MS && Timeout ) PULSE_LOW();
        else PULSE_HIGH();
      #elif DEBUG_MODE == ON
        if ( Pulsegenerator < PULSE_DURATION_MS && Timeout )PORTB &= ~(1<<PB1);
        else PORTB |= (1<<PB1); 
        PULSE_LOW();
      #endif
    }
  }
}

[Egberto (Gast)]

Na so richtig komplett ist es ja nun nicht.....trotzdem vielen Dank.

[Ingo L. (corrtexx)]

Egberto schrieb:
> Na so richtig komplett ist es ja nun nicht.....trotzdem vielen
> Dank.
Richtig, das hier fehlt noch:

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

#define TIMEBASE_1MS      TIM0_COMPA_vect
#define PULSE_DDR        DDRB
#define PULSE_PORT        PORTB
#define PULSE_PIN        PB0
#define PULSE_LOW()        (PULSE_PORT &= ~(1<<PULSE_PIN))
#define PULSE_HIGH()      (PULSE_PORT |= (1<<PULSE_PIN))

#define INTERRUPT_FREQ_KHZ    2

#define PULSE_PERIOD_MIN_MS    359 * INTERRUPT_FREQ_KHZ    // 22km/h
#define PULSE_PERIOD_MAX_MS    7898 * INTERRUPT_FREQ_KHZ    // 1km/h
#define PULSE_DURATION_MS    2 * INTERRUPT_FREQ_KHZ

#define ZERO_TIME_DETECT    8000*INTERRUPT_FREQ_KHZ

#define SENSOR_TRIGGERD      (GIFR & (1<<INTF0))
#define CLEAR_SENSOR_FLAG()    (GIFR |= (1<<INTF0))

#define NEW            1
#define OLD            0

#define ON            1
#define OFF            0

#define DEBUG_MODE        OFF

static uint16_t TimeCounter = 0;
static uint16_t RoundTrip[2] = {0,0};
static uint16_t TimeDifference = PULSE_PERIOD_MAX_MS;
static uint16_t Pulsegenerator = UINT16_MAX;
static uint16_t TunedOutputPeriod = PULSE_PERIOD_MAX_MS;
static uint16_t Timeout = ZERO_TIME_DETECT;

[egberto (Gast)]

Vielen Dank.

Nur für mein Verständnis...eine (leere) Interruproutine für den ext. 
Interrupt braucht man nicht?

Debouncing (oder ist da ein Hallsensor dran)?

Grüße,

egberto

[Ingo L. (corrtexx)]

egberto schrieb:
> Nur für mein Verständnis...eine (leere) Interruproutine für den ext.
> Interrupt braucht man nicht?
Nicht, wenn man sich mit einem Fehler von maximal 1ms in der Umlaufzeit 
zufrieden gibt.

egberto schrieb:
> Debouncing (oder ist da ein Hallsensor dran)?
Gut das du es sagst, ein Reed-Kontakt prellt natürlich auch... Bau ich 
gleich mal ein

[Dennis Restle (Gast)]

Der Code in der ISR hat den Vorteil, dass er immer exakt x Mikrosekunden 
nach dem Bit stattfindet. Bei einem Polling hast du einen Jitter von 
mehreren Taktzyklen.

[Ingo L. (corrtexx)]

Dennis Restle schrieb:
> Der Code in der ISR hat den Vorteil, dass er immer exakt x Mikrosekunden
> nach dem Bit stattfindet. Bei einem Polling hast du einen Jitter von
> mehreren Taktzyklen.
Der aber bei einer Zeitauflösung von 500µs und einer 
Ausführungsgeschwindigkeit von 3 1/3µs pro Takt nicht auffällt...
Hier die Entprellte Version:

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

#define TIMEBASE_1MS      TIM0_COMPA_vect
#define PULSE_DDR        DDRB
#define PULSE_PORT        PORTB
#define PULSE_PIN        PB0
#define PULSE_LOW()        (PULSE_PORT &= ~(1<<PULSE_PIN))
#define PULSE_HIGH()      (PULSE_PORT |= (1<<PULSE_PIN))

#define INTERRUPT_FREQ_KHZ    2

#define PULSE_PERIOD_MIN_MS    359 * INTERRUPT_FREQ_KHZ    // 22km/h
#define PULSE_PERIOD_MAX_MS    7898 * INTERRUPT_FREQ_KHZ    // 1km/h
#define PULSE_DURATION_MS    2 * INTERRUPT_FREQ_KHZ

#define ZERO_TIME_DETECT    8000*INTERRUPT_FREQ_KHZ

#define SENSOR_TRIGGERD      (GIFR & (1<<INTF0))
#define CLEAR_SENSOR_FLAG()    (GIFR |= (1<<INTF0))

#define NEW            1
#define OLD            0

#define ON            1
#define OFF            0

#define DEBOUNCE_TIME_MS    100 * INTERRUPT_FREQ_KHZ  

#define DEBUG_MODE        OFF

static uint16_t TimeCounter = 0;
static uint16_t RoundTrip[2] = {0,0};
static uint16_t TimeDifference = PULSE_PERIOD_MAX_MS;
static uint16_t Pulsegenerator = UINT16_MAX;
static uint16_t TunedOutputPeriod = PULSE_PERIOD_MAX_MS;
static uint16_t Timeout = ZERO_TIME_DETECT;
static uint8_t Debounce = DEBOUNCE_TIME_MS;

int main ( void )
{
  _delay_ms( 100 );
  
  CLKPR = (1<<CLKPCE);
  CLKPR = (1<<CLKPS2);    // /16
  
  OSCCAL = 0x63;
  // Pulse Output
  PULSE_HIGH();
  PULSE_DDR |= (1<<PULSE_PIN);
  
  // Disable AD-Comp
  ACSR |= (1<<ACD);
  
  // Unused Pin low
  DDRB |= (1<<PB2) | (1<<PB3) | (1<<PB4);
  PORTB &= ~( (1<<PB2) | (1<<PB3) | (1<<PB4) );
  
  // Setup AVR
  OCR0A = 17;
  TCCR0A |= (1<<WGM01);      // CTC
  TCCR0B |= (1<<CS01);      // Prescaler = 8
  
  // External Interrupt
  PORTB |= (1<<PB1);          // internal Pullup enable
  MCUCR |= (1<<ISC01);        // Trigger on falling edge
  CLEAR_SENSOR_FLAG();

  
    /* Nice weather... */
    while ( "Cycling" ){
    if ( TIFR0 & (1<<OCF0A) ){
      #if DEBUG_MODE == 1
        PULSE_HIGH();
      #endif
      
      if ( Debounce ) Debounce--;
      if ( Debounce == 1 ) CLEAR_SENSOR_FLAG();
      
      // Clear TimerFlag    
      TIFR0 |= (1<<OCF0A);
  
      // Nullerkennung
      if ( Timeout ) Timeout--;

      // Impuls Erfassung
      TimeCounter++;
  
      if ( SENSOR_TRIGGERD && !Debounce ){
        Timeout = ZERO_TIME_DETECT;
        Debounce = DEBOUNCE_TIME_MS;
        RoundTrip[NEW] = TimeCounter;
        TimeDifference = RoundTrip[NEW] - RoundTrip[OLD];
        RoundTrip[OLD] = RoundTrip[NEW];
      }

      // Impuls Ausgabe
      if ( TimeDifference < PULSE_PERIOD_MIN_MS ){ // abgeregelt
        TunedOutputPeriod = PULSE_PERIOD_MIN_MS;
      }else{// unmanipuliert
        TunedOutputPeriod = TimeDifference;
      }
  
      if ( TunedOutputPeriod > PULSE_PERIOD_MAX_MS ) TunedOutputPeriod = PULSE_PERIOD_MAX_MS;
  
      if ( Pulsegenerator >= TunedOutputPeriod ) Pulsegenerator = 0;
      else Pulsegenerator++;
      
      #if DEBUG_MODE == OFF
        if ( Pulsegenerator < PULSE_DURATION_MS && Timeout ) PULSE_LOW();
        else PULSE_HIGH();
      #elif DEBUG_MODE == ON
        if ( Pulsegenerator < PULSE_DURATION_MS && Timeout )PORTB &= ~(1<<PB1);
        else PORTB |= (1<<PB1); 
        PULSE_LOW();
      #endif
    }
  }
}

[egberto (Gast)]

mir ging es nur darum, ob da nicht ein zusätzliches

EMPTY_INTERRUPT(INT0_vect);

sauberer wäre.

Grüße,

egberto

[Ingo L. (corrtexx)]

egberto schrieb:
> mir ging es nur darum, ob da nicht ein zusätzliches
>
> EMPTY_INTERRUPT(INT0_vect);
>
> sauberer wäre.
Wozu denn?

[egberto (Gast)]

Nach meinem Verständnis versucht der Mikrocontroller bei Interrupt doch 
über den entsprechenden Vector die zugehörige Routine anzuspringen - 
oder ist das nicht immer so?

[Ingo L. (corrtexx)]

egberto schrieb:
> Nach meinem Verständnis versucht der Mikrocontroller bei Interrupt doch
> über den entsprechenden Vector die zugehörige Routine anzuspringen -
> oder ist das nicht immer so?
Wenn man ihm das sagt ( hier wäre es: GIMSK |= (1<<INT0) ), dann schon. 
Da ich das aber nicht mache, benötige ich auch die dazugehörige ISR 
nicht...

[Jörg W. (dl8dtl) (Moderator)]

Er hat ja nun aber gar keinen Interrupt mehr aktiviert.

[Ingo L. (corrtexx)]

Jörg W. schrieb:
> Er hat ja nun aber gar keinen Interrupt mehr aktiviert.
Schneller ;)

[egberto (Gast)]

ah ja, die Codeänderung hatte ich nicht beachtet....