sei() innerhalb eines ISR()

[D a v i d K. (oekel)]

Hi,

ich verwende gerade jede Menge I2C Aufrufe. Eine davon sogar im ISR

uint8_t ks;
...
ISR( TIMER0_OVF_vect) {
    ks = I2C_ReadRegister(HT16K33, 0x41);
    ...//do a little bit more
}

Nun habe ich pauschal alle I2c_* Funktionen mit einem cli()/sei(); 
umschlossen.

uint8_t I2C_ReadRegister(uint8_t busAddr, uint8_t deviceRegister) {
  cli();
  uint8_t data = 0;
  i2c_start(busAddr); // send device address
  i2c_write(deviceRegister); // set register pointer
  i2c_start(busAddr + I2C_READ); // restart as a read operation
  data = i2c_readNak(); // read the register data
  i2c_stop(); // stop
  sei();
  return data;
}

Früher gab es ja noch SIGNAL und INTERRUPT (<-- heutige ISR)
SIGNAL hat dabei ja Interrupts während der IR-Routine erlaubt.

Meine Frage wäre daher nun, ob ich mir künstlich wieder ein SIGNAL 
gebaut habe, dadurch dass ich in der ISR einen Funktionsaufruf verwende 
der sei() enthält?

Oder wird das I-Bit unter keinen umständen verändert, solange ich mich 
in der ISR befinde und bin somit safe?

Grüße Oekel

[Falk B. (falk)]

@ D a v i d K. (oekel)

>ich verwende gerade jede Menge I2C Aufrufe. Eine davon sogar im ISR

Naja, hoffentlich ist die ISR-Frequenz nicht zu hoch.

>Nun habe ich pauschal alle I2c_* Funktionen mit einem cli()/sei();
>umschlossen.

Warum? Das ist im Normalfall nicht nötig. I2C ist synchron und du nutzt 
ja das I2C Modul deines AVRs. Da darf so ein I2C Zugriff auch mal durch 
einen ISR unterbrochen werden.

>Früher gab es ja noch SIGNAL und INTERRUPT (<-- heutige ISR)
>SIGNAL hat dabei ja Interrupts während der IR-Routine erlaubt.

Alter Kram. Vergiss das.

>Meine Frage wäre daher nun, ob ich mir künstlich wieder ein SIGNAL
>gebaut habe, dadurch dass ich in der ISR einen Funktionsaufruf verwende
>der sei() enthält?

Nun ja, selbst wenn man eine Funktion atomar ausführen will, sollte man 
im Normalfall die Funktionen aus util/atomic.h nutzen. Denn die können 
am Ende des atomaren Blocks den vorherigen Zustand des I-Bit wieder 
herstellen. Sprioch, wenn es vorher gelöscht war, bleibt es gelöscht!

#include <util/atomic.h>

    ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
        // atomarer Funktionsblock, Interrupts sind gesperrt
    }

Siehe Doku der libc.

>Oder wird das I-Bit unter keinen umständen verändert, solange ich mich
>in der ISR befinde und bin somit safe?

Nö. Ein sei() setzt das I-Bit. Immer.

[jz23 (Gast)]

Du kannst sei() natürlich auch innerhalb einer ISR verwenden. Du 
solltest aber auf jeden Fall vor sei() dafür sorgen, dass der aktuelle 
Interrupt (Also bei dir der Timer0 Overflow) deaktiviert ist, weil sonst 
die Gefahr besteht, dass der Stack überläuft.

Also nach dem Schema

ISR( TIMER0_OVF_vect) {
// whatever
TIMSK &= ~(1<<TOIE0);
sei();
// noch mehr Code
cli();
TIMSK |= 1<<TOIE0;
return;
}

Bei dir ist das aber eigentlich gar nicht sinnvoll, weil du ja keine 
Interrupts während deines Timer-Overflows benötigst (Zumindest macht 
dein Codeschnipsel den Anschein). Ich würde an deiner Stelle eher das 
Statusregister speichern und anschließend wiederherstellen.

Aus dem GCC Tutorial:

I2C(){
uint8_t tmp_sreg = SREG;
cli();
// mach was
SREG = tmp_sreg;

[D a v i d K. (oekel)]

Falk B. schrieb:
> @ D a v i d K. (oekel)
>
>>ich verwende gerade jede Menge I2C Aufrufe. Eine davon sogar im ISR
>
> Naja, hoffentlich ist die ISR-Frequenz nicht zu hoch.
Teiler: 1024

> Nun ja, selbst wenn man eine Funktion atomar ausführen will, sollte man
> im Normalfall die Funktionen aus util/atomic.h nutzen. Denn die können
> am Ende des atomaren Blocks den vorherigen Zustand des I-Bit wieder
> herstellen. Sprioch, wenn es vorher gelöscht war, bleibt es gelöscht!
>
>

> #include <util/atomic.h>
> 
>     ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
>         // atomarer Funktionsblock, Interrupts sind gesperrt
>     }
>

Mein theoretisches Szenario wäre das sich das ganze wie folgt mal michen 
könnte:

void I2C_WriteRegister(uint8_t busAddr, uint8_t deviceRegister, uint8_t data) {
  cli();
  i2c_start(busAddr);
  i2c_write(deviceRegister);
        //...nun kommt der ISR..

        //uint8_t I2C_ReadRegister(uint8_t busAddr, uint8_t deviceRegister) {
  cli();
  uint8_t data = 0;
  i2c_start(busAddr); // send device address
  i2c_write(deviceRegister); // set register pointer
  i2c_start(busAddr + I2C_READ); // restart as a read operation
  data = i2c_readNak(); // read the register data
  i2c_stop(); // stop
  //sei();
        //}

        //...weiter gehts
  i2c_write(data);
  i2c_stop();
  sei();
}

Sprich es kommt im besten Fall Müll auf dem Display an (dafür verwende 
ich das "I2C_WriteRegister") im Schlimmsten Fall hängt er sich auf? 
(darartigen Fall habe ich gerade sporadisch in der Praxis und schiebe es 
auf den ISR.
(konkret läuft eine Endlosschleife mit Testausgaben: 40-100x ok. dann 
steht alles...)

[A. S. (Gast)]

cli und sei in Funktion vermeiden. Maximal im Hauptstrang. oder vorher 
fragen, ob Interrupts gesperrt sind. Falls ja, dann nachher nicht 
freigegeben. Erfordert aber genaue Überlegungen.

Auch braucht man oft getrennte Funktionen für gleiche Aufgaben: eine für 
den Aufruf aus Interrupts.

[D a v i d K. (oekel)]

PPS:

folgenden minimal-Wert für "multi" habe ich gerade per TryAndError für 
mich ermittelt:

void timer0_init() {

  TCCR0 = (CS00 || CS02); /* Takt 1024 */

  //"Timer/Counter Interrupt Mask" (TIMSK)
  //"Timer/Counter Interrupt Enable"(TOIE0)
  TIMSK |= (1 << TOIE0);

}
uint16_t multi = 0;
ISR( TIMER0_OVF_vect) {

  if (multi == 0) {
    multi = 350;

    if(SW2){
      PORTC ^= ( 1 << PC6 );
      SW2 = false;
    }
    updateSwitches();
  }
  multi--;
}

void updateSwitches() {

  if (SW1 || SW2 || SW3)
    return;
  //uint16_t ks = Key_ReadCom(uint8_t ks);
  uint8_t ks = I2C_ReadRegister(HT16K33, 0x41);
  SW1 = (ks & 0b00010000);
  SW2 = (ks & 0b00001000);
  SW3 = (ks & 0b00000100);
}

Wieso ich allerdings hier schon 1024*350 Takte für brauche erschließt 
sich mir nicht.

[D a v i d K. (oekel)]

Achim S. schrieb:
> cli und sei in Funktion vermeiden. Maximal im Hauptstrang.
Guter Einwand...

[Falk B. (falk)]

@D a v i d K. (oekel)

>Mein theoretisches Szenario wäre das sich das ganze wie folgt mal michen
>könnte:

Stimmt. Dann braucht man eine atomare Sicherung. Aber in den meisten 
Fällen ist es besser, keinerlei I2C Zugriffe in der ISR zu machen und 
statt dessen alle Zugriffe nur als normale Funktion in der 
Hauptschleife.

https://www.mikrocontroller.net/articles/Interrupt#Steuersignale_zwischen_ISR_und_Hauptprogramm

Das hat auch den Vorteil, daß man sehr langsame Funktionen nutzen kann, 
die in einer ISR nichts zu suchen haben.

[Sascha W. (sascha-w)]

Hallo,

das scheit so nicht zu passen
 TCCR0 = (CS00 || CS02); /* Takt 1024 */

CS00 = 0
CS02 = 2

ergibt TCCR0=2   => CLK/8

Sascha

[D a v i d K. (oekel)]

Sascha W. schrieb:
> Hallo,
>
> das scheit so nicht zu passen
>  TCCR0 = (CS00 || CS02); /* Takt 1024 */

Autsch!
TCCR0 = (1 << CS00) | (1 << CS02); /* Takt 1024 */

[D a v i d K. (oekel)]

Falk B. schrieb:

> Aber in den meisten
> Fällen ist es besser, keinerlei I2C Zugriffe in der ISR zu machen und
> statt dessen alle Zugriffe nur als normale Funktion in der
> Hauptschleife.

Hatte ich auch ursprünglich nicht vor. Doch wie die Flanken eines 
Tasters am HT16K33 rechtzeitig erkennen, wenn ich nicht ständig auf dem 
I2C-Bus pulle?

[Falk B. (falk)]

@ D a v i d K. (oekel)

>Hatte ich auch ursprünglich nicht vor. Doch wie die Flanken eines
>Tasters am HT16K33 rechtzeitig erkennen, wenn ich nicht ständig auf dem
>I2C-Bus pulle?

Man kann die periodische Abfrage (vulgo "pulle") innerhalb einer 
Funktion im Hauptprogramm machen. Wie das geht, steht in meinem Link.

[D a v i d K. (oekel)]

Falk B. schrieb:
> @ D a v i d K. (oekel)

> Man kann die periodische Abfrage (vulgo "pulle") innerhalb einer
> Funktion im Hauptprogramm machen. Wie das geht, steht in meinem Link.

Danke, hab es jetzt genau so gelöst.
Die Hauptroutine wirkte anfänglich so riesig, dass ich vermutet habe 
"dort muss unbedingt zwischengesprungen werden". Aber tatsächlich wird 
jeder REST-Zweig mit 5-6 if-Abfragen erreicht und die vorgelagerte

if(flag)
  alter_ISR_Teil();
else
  REST();

stört den "REST-Ablauf" nicht wirklich.
Endlich wieder Determinismus. Danke!

PS: In der Theorie hatte ich das alles schon gelesen (lese ja viel der 
hiesigen Wiki-Artikel). In der Praxis muss man aber immer wieder drauf 
gestoßen werden, um seinen Code zu strukturieren.

[Peter D. (peda)]

D a v i d K. schrieb:
> Nun habe ich pauschal alle I2c_* Funktionen mit einem cli()/sei();
> umschlossen.

"Pauschal" ist immer ein schlechter Programmierstil. Man muß sich bei 
jeder Anweisung auch was gedacht haben. Insbesondere Anweisungen 
bezüglich der Interruptlogik sollte man nur mit Bedacht einfügen.

[D a v i d K. (oekel)]

Letzte Nachricht von mir diesbezüglich:

Gibt es einen Grund wieso man einen Eröffnungspost negativ bewertet?
Antworten zu markieren finde ich ja logisch, aber "dumme Fragen" gibt es 
nicht und die Höflichkeit gebietet es doch nachzuhaken oder des Weges zu 
gehen!?