Globales struct; Änderung der Daten innerhalb einer ISR manchmal nicht wirksam

[Tobias G. (t-g-laeser)]

Hallo,

- µC: ATxmega128A1, 32MHz intern,
- globales Struct mit allen wichtigen Programmdaten
- Sekundentakt von Echtzeituhr mit sauberen Flanken

In vielen Funktionen greife ich auf mein globales Daten-Struct zurück. 
Das besteht aus 256 Byte Daten die beim Start aus dem Flash geladen 
werden und dort regelmäßig geloggt werden.

volatile struct aktive_daten{

  // .... paar Arrays etc..

  // Flag Byte 0
  volatile uint8_t Flag_SQW_Falling:1;
  volatile uint8_t Flag_SQW_Rising:1;
  volatile uint8_t Flag_sekunde_ungerade:1;
  volatile uint8_t Flag_Betriebszustand_aktiv:1;
  volatile uint8_t Flag_Betriebszustand_S3:1;
  volatile uint8_t Flag005:1;
  volatile uint8_t Flag006:1;
  volatile uint8_t Flag_Fehlfunktion:1;

  // .... insgesamt 256 Byte

}tgl_daten;

==> Probleme treten bei der ISR auf..:

Kommt nun mein Sekundentakt mit steigender Flanke:

ISR(PORTF_INT0_vect){
  PORTE.OUTTGL = (1 << PIN0);  PIN0 toggeln
  tgl_daten.Flag_SQW_Rising = 1;
}

sollte in der Main-Schleife entsprechend der Code ausgeführt werden:

  do{
    if(tgl_daten.Flag_SQW_Rising){
      tgl_daten.Flag_SQW_Rising = 0;

      // Irgendeinen kurzen Code ausführen
      PORTE.OUTTGL = (1 << PIN1);  PIN1 toggeln

    }
  }while(1);

... Allerdings passiert es in unregelmäßigen Abständen, dass der Code in 
der Main-Schleife micht ausgeführt wird.
PIN0 Toggelt jede Sekunde, PIN1 lässt ab und an einen aus.

Ich bin mir sicher, dass keine weiteren ISR dazwischenfunken.
Eigentlich dachte ich es würde helfen alles mit "volatile " (wie oben 
gezeigt) zu erweitern, aber auch das half nicht weiter.

Wenn ich das ganze mit einer

volatile uint8_t test;

 versuche, tritt das Problem eher selten ( / nie? - Bin mir grad nicht 
sicher) auf.

Ist jemand ein solches Problem bekannt?

Danke für jede ernst gemeinte Antwort.

Mit freundlichen Grüßen
Tobias

[Dr. G. Reed (Gast)]

Ich mach das im Prinzip auch so, nur dass ich statt ganzen Bytes nur 
jeweils ein Bit in einem flag-register während  der ISR setze.

Das Flag-register ist auch als Volatile uin8_t definiert.

Mir ist noch nicht aufgefallen, dass da was verloren ginge!

[Karl H. (kbuchegg)]

Was hast du noch in der Hauptschleife.
Kann es sein, dass deine Hauptschleife ab und an auch mal 1 Sekunde für 
einen Durchlauf braucht?

Annahme:
 tgl_daten.Flag_SQW_Rising  ist gesetzt. Und zwar schon seit längerer 
Zeit.

    if(tgl_daten.Flag_SQW_Rising){

ja, ist der Fall. ALso gehts ins if hinein

Aber wie der Teufel so will, kommt dir jetzt ein entsprechender 
Interrupt in die Quere. Die ISR wird aufgerufen und 
tgl_daten.Flag_SQW_Rising wird 'nochmal' gesetzt (obwohl es schon 
gesetzt ist).
Die ISR ist fertig, es geht zurück in die Hauptschleife. DOrt passiert

      tgl_daten.Flag_SQW_Rising = 0;

Ooops. Das Flag zurücksetzen sollte eigentlich die Antwort auf
    if(tgl_daten.Flag_SQW_Rising){
gewesen sein. Durch das Rücksetzen wird allerdings der zwischendruch 
aufgetretene Interrupt auch gleich mitgelöscht.


-> sei() und cli() verwenden, damit zwischen Abfrage und Rücksetzen kein 
Interrupt dazwischenfunken kann.

  do{

    cli();   // ****
    if(tgl_daten.Flag_SQW_Rising){
      tgl_daten.Flag_SQW_Rising = 0;
      sei();  // ****

      // Irgendeinen kurzen Code ausführen
      PORTE.OUTTGL = (1 << PIN1);  PIN1 toggeln

    }
    sei();  // ****

  }while(1);

[Stefan E. (sternst)]

Und was ist mit den ganzen anderen Flags in dem Bit-Feld? Die werden 
doch sicher auch irgendwo in main geschrieben, oder? Auch wenn es wie 
separate Variablen aussieht, so ist doch der Zugriff auf sie nicht 
atomar. Wenn z.B. irgendwo in main nach Flag_sekunde_ungerade 
geschrieben wird und da der Interrupt ungünstig rein fällt, geht dir das 
Schreiben nach Flag_SQW_Rising in der ISR "verloren".

[Dr. G. Reed (Gast)]

Du kannst ja mal nachschauen, was der Compiler für Assembler Code 
erzeugt, dann siehst Du genau was los ist...

[Dosmo (Gast)]

Tobias Gläser schrieb:
> volatile struct aktive_daten{
>
>   // .... paar Arrays etc..
>
>   // Flag Byte 0
>   volatile uint8_t Flag_SQW_Falling:1;
>   volatile uint8_t Flag_SQW_Rising:1;
>   volatile uint8_t Flag_sekunde_ungerade:1;
>   volatile uint8_t Flag_Betriebszustand_aktiv:1;
>   volatile uint8_t Flag_Betriebszustand_S3:1;
>   volatile uint8_t Flag005:1;
>   volatile uint8_t Flag006:1;
>   volatile uint8_t Flag_Fehlfunktion:1;
>
>   // .... insgesamt 256 Byte
>
> }tgl_daten;

Es reicht, daß die Struktur "volatile" ist, Du mußt nicht noch alle 
einzelnen Elemente "volatile" machen.

Ich vermute, daß das Problem ist, daß das Setzen und Löschen einzelner 
Bits in einem Datenwort aus einer "Read-Modify-Write"-Sequenz besteht. 
D.h. Du hast mehrere Assemblerbefehle:
1. Datenwort aus dem RAM in ein Register lesen
2. Das Register manipulieren (Bit setzen oder löschen)
3. Register zurück ins Datenwort schreiben.

In Deinem Fall besteht also die Zeile

tgl_daten.Flag_SQW_Rising = 0;

aus drei mind. Assemblerbefehlen. Wie Stefan Ernst richtig schreibt, 
sind diese nicht atomar, d.h. der Interrupt kann in diese Sequenz 
hineinfunken.

Probier mal folgendes:

  do{    
    if(tgl_daten.Flag_SQW_Rising){
      cli();
      tgl_daten.Flag_SQW_Rising = 0;
      sei();
      asm volatile ("nop");
      // Irgendeinen kurzen Code ausführen
      PORTE.OUTTGL = (1 << PIN1);  PIN1 toggeln
    }
  }while(1);

[(prx) A. K. (prx)]

Einfacher wäre es in diesem Fall freilich, wenn man aus dem Bit ein Byte 
macht. Dann sind die beteiligten Operationen atomar und der cli/sei-Kram 
entfällt.

[Tobias G. (t-g-laeser)]

Hallo und vielen Dank für Eure Antworten.

Karl Heinz Buchegger schrieb:
> Was hast du noch in der Hauptschleife.
> Kann es sein, dass deine Hauptschleife ab und an auch mal 1 Sekunde für
> einen Durchlauf braucht?

Aktuell habe ich alles auskommentiert was noch verzögern könnte, dazu 
lasse ich noch ein Pin einfach toggeln, der etwa mit 1 kHz anzeigt dass 
die Main-Schleife nicht hängt.
Du hast es anschaulich beschrieben, also hab ich das mal so getestet.
Es bleibt also folgender Code übrig:

ISR(PORTF_INT0_vect){
  PORTE.OUTTGL = (1 << PIN0);
  tgl_daten.Flag_SQW_Rising = 1;
}

  do{
    cli();
    if(tgl_daten.Flag_SQW_Rising){
      tgl_daten.Flag_SQW_Rising = 0;
      PORTE.OUTTGL = (1 << PIN3);
    }
    sei();
    PORTE.OUTTGL = (1 << PIN1);
    _delay_ms(1);
  }while(1);

Am 4-Kanal-Oszi sehe ich nun sauber das Eingangssignal (1 Hz), die ISR 
(0,5 Hz), die Funktion (0,5 Hz) und die Main-Schleife (515 Hz)

Ein Problem besteht nun jedoch noch... Ab und an wird die ISR nun 
garnicht aufgerufen oder sogar mal doppelt.
Doppelt erkenne ich daran, dass das ISR-Signal auf einem Wert bleibt 
(doppelt getoggelt) jedoch in der Main das Bit nur einmal sich ändert, 
so wie es sein sollte.
Seltsam hierbei ist aber, das die 1Hz der Echtzeituhr sehr saubere 
Flanken hat, also woran könnte das noch liegen?

Stefan Ernst schrieb:
> Und was ist mit den ganzen anderen Flags in dem Bit-Feld? Die werden
> doch sicher auch irgendwo in main geschrieben, oder? Auch wenn es wie
> separate Variablen aussieht, so ist doch der Zugriff auf sie *nicht*
> atomar. Wenn z.B. irgendwo in main nach Flag_sekunde_ungerade
> geschrieben wird und da der Interrupt ungünstig rein fällt, geht dir das
> Schreiben nach Flag_SQW_Rising in der ISR "verloren".

Daran hab ich nun nicht gedacht, aber momentan bei dem Test werden keine 
anderen Variablen benutzt. Später benötige ich fast alle 160 Flags und 
darf nicht mehr als 20 Byte verwenden, damit mein Struct nicht zu groß 
wird (Wegen der Page-Ablage im Flash). Aber ich behalts mal für später 
im Hinterkopf.

Den Code von Dosmo werde ich auch noch testen, aber er scheint auf das 
gleiche hinaus zu laufen, jedoch ist die if-Abfrage außerhalb des 
cli-sei. Benötigt diese dann nicht auch min. 2 Befehle in Assembler?

A. K. schrieb:
> Einfacher wäre es in diesem Fall freilich, wenn man aus dem Bit ein Byte
> macht. Dann sind die beteiligten Operationen atomar und der cli/sei-Kram
> entfällt.

Einen solchen einfacheren weg fände ich schön aber wie ließe sich das 
bewerkstelligen ohne für 160 Flags mehr Speicher zu belegen? Ein Byte 
mit 8 verschiedenen Maskierungen? In einem einzelnem Befehl wird das 
dann wohl auch nicht funktionieren, oder täusche ich mich da?

Schonmal Danke für Eure Hilfe,
mit freundlichen Grüßen
Tobias

P.S.: Nun warte ich schon seit 10 Minuten auf die fehlende ISR um ein 
Bild anzuhängen, aber wie es halt immer so ist kommt der Fehler nun 
nicht. Deshalb hab ich als 3. Bild ein älteres angehängt, wo ich den 
1kHz-Takt noch nicht drin hatte.

*EDIT*: Die Struktur habe ich wie Dosmo geschrieben hat nur noch als 
ganzes volatile gesetzt. Nicht mehr alle einzelnen Bits intern.

[Karl H. (kbuchegg)]

Tobias Gläser schrieb:

> Den Code von Dosmo werde ich auch noch testen, aber er scheint auf das
> gleiche hinaus zu laufen, jedoch ist die if-Abfrage außerhalb des
> cli-sei. Benötigt diese dann nicht auch min. 2 Befehle in Assembler?

Die braucht es sowieso.
Und genau das ist auch der Grund für die cli/sei.
Bessere CPUs, die von Haus aus Unterstützung für Multiprocessing bzw. 
Multithreading mitbringen haben auf CPU Ebene eine Instruktion: Teste 
ein Bit und setzte es zurück wenn gesetzt.
Es ist wichtig, dass diese komplette Operation nicht unterbrochen werden 
kann!
Hat die CPU keine derartige ununterbrechbare Operation, dann muss man 
das auf dem AVR eben händisch durch cli/sei machen. Aber der Schutz muss 
sich über die komplette Operation "Testen und rücksetzen" erstrecken. 
Nur so ist (zumindest diese) die Gefahr komplett gebannt.

> Einen solchen einfacheren weg fände ich schön aber wie ließe sich das
> bewerkstelligen ohne für 160 Flags mehr Speicher zu belegen? Ein Byte
> mit 8 verschiedenen Maskierungen?

Genau das hast du im Prinzip jetzt auch. Nur hast du es in einer struct 
versteckt und der Compiler erledigt für dich das Ausmaskieren und 
zurechtschieben.

[(prx) A. K. (prx)]

Karl Heinz Buchegger schrieb:

> Bessere CPUs, die von Haus aus Unterstützung für Multiprocessing bzw.
> Multithreading mitbringen haben auf CPU Ebene eine Instruktion: Teste
> ein Bit und setzte es zurück wenn gesetzt.

Das ist ein möglicher Weg. Es gibt auch andere Wege, effizientere, 
weshalb dieses Test-and-Set nicht überall existiert.

[Tobias G. (t-g-laeser)]

Guten Morgen!

Habe nun das Ding weiter am laufen, geschützt mit der cli-sei-Sache.
Doppelte Interrupts werden gefiltert, fehlende Interrupts konnte ich 
nicht rekonstruieren, werden mir aber mithilfe eines Software-Watchdog 
in Priorität 3 (ATxmega hat 3 Prioritätsebenen) durch einen "Piep" 
angezeigt (Mit Testschleife gehts, aber von selber kam der Fehler nun 
nicht mehr).
=> Danke für die Hilfen oben, das war wohl der richtige Ansatz!

Nun möchte ich es so umbauen, dass das 256-Byte-Struct nur in der 
Main-Schleife (und deren Unterfunktionen) bearbeitet/gelesen wird und 
die ISR eine eigene globale Variable bekommt. So spare ich mir auf 
Kosten des Speichers hunderte cli-sei später.

Jedoch möchte ich das nicht für meine ganzen ADC-/DAC-Sachen, 
UART/I2C/SPI und so auch noch machen müssen.

Überlegung:
Wenn diese Datem im selben Struct sind, jedoch mit eigenen Bytes / 
Arrays (keine gemeinsamen Flags o.ä.) - Bekomme ich dann Probleme mit 
meinen anderen Sachen im Struct?

Beispiel:
( Main ) lese/schreibe (ungeschützt) ein Bit-Flag im 5. Byte des Structs
(=> ISR) lese/schreibe (ungeschützt) Byte 20 im Struct
( Main ) Mit dem 5. Byte weiter machen
( Main ) (cli)20. Byte auswerten(sei)

Bekomme ich da ein Problem? Sollte ich das reine Struct der 
Main-Schleife von dem mit ISR trennen? Also so, dass beide zusammen 
dennoch ihre 256 Bytes haben, aber die eine mit cli-se und die andere 
ohne, oder darf ich das so zusammen lassen?

[Karl H. (kbuchegg)]

Tobias Gläser schrieb:

> Überlegung:
> Wenn diese Datem im selben Struct sind, jedoch mit eigenen Bytes /
> Arrays (keine gemeinsamen Flags o.ä.) - Bekomme ich dann Probleme mit
> meinen anderen Sachen im Struct?

Nein.

Aber:
Du solltest dir wirklich softwaretechnisch überlegen, wie sinnvoll es 
ist, alles in eine einzige struct zu quetschen. Das ist nicht wirklich 
Sinn einer struct.
In einer struct sammelt man die Dinge zusammen, die thematisch 
zusammengehören. Uhrzeit, Fehlerstatus, UART, .... das alles hat 
thematisch gesehen nicht wirklich etwas miteinander zu tun. Es ist für 
mich sehr zweifelhaft, ob das alles in erster Linie überhaupt in einer 
gemeinsamen struct sein sollte.

Bisher hatte ich diese Krot akzeptiert und nichts gesagt, weil ich 
dachte du müsstest auf jedes Byte Speicher aufpassen. D.h. Bits 
zusammenquetschen wo nur geht.
Da das aber nicht der Fall zu sein scheint ... O-Ton
> So spare ich mir auf Kosten des Speichers hunderte cli-sei später.
... ist aber auch der Grund für das Zudrücken beider Hühneraugen 
weggefallen.

[Dosmo (Gast)]

Ich bin nicht sicher, ob ich verstanden habe, was Du tun willst, aber 
ich würde die Variablen, die zwischen Main und ISR ausgetauscht werden, 
in einer separate Struktur zusammenfassen und diese Struktur dann nur 
unter Interruptsperre lesen und schreiben.
Dann hat alles seine Ordnung und Du kommst nicht durcheinander. 
Ansonsten suchst Du ewig und drei Tage nach komplizierten Bugs, die sich 
durch einen strukturieren Ansatz vermeiden ließen.

[Tobias G. (t-g-laeser)]

Karl Heinz Buchegger schrieb:
> Nein

Das bedeutet das Beispiel würde funktionieren? Dann würde ich mir 
mangels Übersicht zwar überall anders viele Beine stellen, aber die 
Zugriffe an sich werden nicht verletzt?

Karl Heinz Buchegger schrieb:
> In einer struct sammelt man die Dinge zusammen, die thematisch
> zusammengehören. Uhrzeit, Fehlerstatus, UART, .... das alles hat
> thematisch gesehen nicht wirklich etwas miteinander zu tun. Es ist für
> mich sehr zweifelhaft, ob das alles in erster Linie überhaupt in einer
> gemeinsamen struct sein sollte.

Da muss ich Dir natürlich Recht geben. Der Sinn dieses zusammenführendem 
Struct ist eigentlich der, dass ich ihn hauptsächlich für die 
Speicherverwaltung und den regelmäßigen Log benötige. So ist es mir ein 
einfaches, einfach das ganze Struct ohne irgendwelche Zusatzgedanken in 
den Flash zu schieben und muss nicht erst Schleifen konstruieren um die 
einzelnen Streucts nacheinander ins Flash zu laden.
Die ADC/DAC, UART, ... haben noch eigene Structs, jedoch ist in dem 
"Haupt-Struct" die Sammlung aller gefilterten Mittelwerte von den ADC 
und den externen Sensoren etc. Dazu eben die Flags die ich mitsichern 
möchte usw.

Dosmo schrieb:
> Ich bin nicht sicher, ob ich verstanden habe, was Du tun willst, aber

Das hast Du. ;)

Dosmo schrieb:
> ich würde die Variablen, die zwischen Main und ISR ausgetauscht werden,
> in einer separate Struktur zusammenfassen und diese Struktur dann nur
> unter Interruptsperre lesen und schreiben.
> Dann hat alles seine Ordnung und Du kommst nicht durcheinander.
> Ansonsten suchst Du ewig und drei Tage nach komplizierten Bugs, die sich
> durch einen strukturieren Ansatz vermeiden ließen.

Der Einwand ist gut, die Daten sind im Struct zwar gut sortiert, aber 
eine saubere Auftrennung in 2 Structs wäre schon möglich.