Volatile array element oder struct element?

[Karsten K. (karsten42)]

Moin Moin,

Auch nach lesen von diversen Threads werde ich nicht gnaz schlau daraus 
ob ich mich darauf verlassen kann, das ein Element in einem Struct oder 
Elemente eines Array wirklich volatile sind wenn ich diese 
folgendermaßen definiere:

MCU: AT90CAN128
gcc version 4.3.3 (WinAVR 20100110)
Optimizer: -Os

//variant a
volatile uint8_t active = 0;

// variant b
typedef struct {
  volatile uint8_t active;
  can_buffer_t buffer;      
  can_t list[CAN_TX_BUFFER_SIZE];  
} txbuf_t;

//variant c
volatile active[SJA_MAX];

txbux_t txbuf[SJA_MAX];


// later in code
ISR(INT0_vect)  {

...

// This is working
active = 0;

// This two variant´s do not work
active[0] = 0;
txbuf.active[0] = 0;
....
}

Sobald ich die Variable "active" als Element eines Array oder einer 
Struktur nutze funktioniert anscheinend die Deklaration volatile nicht, 
da in beiden Fällen der code nicht mehr funktioniert.Natürlich könnte es 
auch nur ein "seiteneffekt" sein. Ich möchte mir aber wirklich sicher 
sein, dass die deklaration und der Zugriff 100% richtig sind bevor ich 
anfange wilder fiese Geister zu suchen.

Wo liegt hier der Haken in der Deklaration oder Nutzung der Variablen?

[Achim K. (aks)]

txbux_t txbuf[SJA_MAX];
...
txbuf.active[0] = 0;

Ich verstehe nicht ganz, wie dass tut. Ist das Array nicht txbuf?
Dann müßte es doch

txbuf[0].active = 0;

sein. Oder meint "don't work", dass es nicht compiliert?

[Mike (Gast)]

Und bei Variante c fehlt der Datentyp..

[Karsten K. (karsten42)]

Arrrrghhhh!
Ich habe den code abgeschrieben weil es sonst zu viel wäre. Tippfuhler!
Mea maxima culpa!

Richtig ist natürlich:

//variant c
volatile uin8_t array = 0;


// This two variant´s do not work
active[0] = 0;
txbuf[0].active = 0;

Compilieren tut es ( wenn man den originalen code nutzt, aua!). die 
Funktion die ich mit der variablen "active" steuere funktionier jedoch 
nur, wenn ich die variante "c" nutze. Sie zeigt an, ob ein 
CAN-Controller ( SJA1000 ) noch gerade dabei ist die CAN nachricht zu 
senden und somit weitere Daten in einen Buffer zwischengespeichert 
werden müssen.

Danke für´s aufmerksame lesen.

Gruß
Karsten

[Achim K. (aks)]

OK, jetzt sieht es besser aus.
Meine Erfahrungen mit Variante b waren bis jetzt ganz gut.
Zeigst Du uns auch noch die andere Stelle, wo Du den Wert liest?

[Karsten K. (karsten42)]

Hallo,

Damit nun nicht noch einmal Karstens Krummfinger dazwischen kommen der 
ganze code für den SJA1000 im Anhang.

Es sind zwei SJA1000 anzusteuern, die jeweils mit einem TX buffer 
ausgestattet sind und diesen über interrupt "selbst" abarbeiten. Dabei 
wird aber nur eine sende Funktion genutzt die als Parameter den 
entsprechenden SJA übergeben bekommt der die message versenden soll.

Alles nicht ganz einfach, die Funktion _send_message() reentrant zu 
halten...

Gruß
Karsten

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

Ob Member oder separate Variable ist bei "volatile" egal. Nur ist die 
Programmlogik hier nicht identisch, denn in einem Fall ist es eine 
Variable für alle, im anderen Fall eine pro Puffer.

[Karsten K. (karsten42)]

Hallo,

Damit die Funktion sja_send_message() für beide SJA´s funktioniert muss 
natürlich die Variable "active" abhängig vom Übergabewert ( x ) pro 
buffer gesetzt werden. txbuf[x].active = Y
Weil das aber nicht funktioniert, habe ich für den einen SJA eine feste 
Variable ausprobiert ( volatile uint8_t active ). Im Code ist der 
ursprüngliche Versuch auskommentiert.

Weil es mit der direckten volatile variable active funktioniert, gehe 
ich davon aus, dass ich etwas falsch mache wenn ich ein volatiles 
Element aus der struct txbuf_t nutze. Zum Test hatte ich auch einfach 
mal nur ein arra versucht wie oben beschrieben. Aber auch dort 
funktioniert der code nicht mehr.

Jetzt könnte ich zwei "active" variablen nutzen und mit "wilden if()" 
testen ob nun für das setzten und lesen die Eine oder Andere nutzen 
muss. Das ist aber wieder alles nicht atomic und macht dann sicher 
Probleme.

gruß
Karsten

[Karl H. (kbuchegg)]

Karsten K. schrieb:
> Hallo,
>
> Damit die Funktion sja_send_message() für beide SJA´s funktioniert muss
> natürlich die Variable "active" abhängig vom Übergabewert ( x ) pro
> buffer gesetzt werden. txbuf[x].active = Y


Jo, grundsätzlich richtig

> Alles nicht ganz einfach, die Funktion _send_message() reentrant zu halten...

Teil deines 'alles nicht ganz einfach' dürfte darin liegen, dass du eine 
globale sja_add Variable hast, die angibt auf welchen SJA die Low-Level 
Funktion operieren sollen.
Hättest du dir die Variable gespart und statt dessen den Low-Level 
Funktionen ein zusätliches Argument gegeben, welches ihnen jeweil 
angibt, welcher SJA gemeint ist, dann hätte sich dein 'alles nicht so 
einfach' in Luft aufgelöst. Solche 'Umschaltvariablen' inklusive 
Sichern/Rücksetzen an allen möglichen und unmöglichen Codestellen sieht 
nur auf den ersten Blick einfach aus. Tatsächlich hat so ein Vorgehen 
ungemeines Sprengpotential. Hat sich in der Vergangenheit oft genug 
bewiesen. Übergib einer Funktion alle Parameter, die sie zur Arbeit 
benötigt - das kostet dich einmalig ein paar Minuten um die vorhandenen 
Funktionen (die mit nur einem Gerät arbeiten) entsprechend aufzubohren 
aber auf lange Sicht ist das die wartungstechnisch vernünftigste Lösung.


> Weil es mit der direckten volatile variable active funktioniert, gehe
> ich davon aus, dass ich etwas falsch mache wenn ich ein volatiles
> Element aus der struct txbuf_t nutze.

Ich hab ihn (den Fehler) zwar noch nicht entdeckt, ich denke aber, dass 
den sja_add irgendwo den falschen Wert hat.



PS: Bei Code in diesen Größenordnungen ist ein vergessenes volatile 
'nicht so schlimm'. Natürlich ist es nach wie vor ein Fehler, aber 
alleine durch die Codegröße und den Datenumfang schafft es der Compiler 
sowieso nicht, die entsprechendn Variablen ständig in Registern zu 
halten, so dass sich ein fälschlicherweise vergessenes volatile nicht 
auswirkt, weil der Compiler sowieso keine andere Wahl hat, solange das 
ganze nicht in Schleifen vorkommt.

[Karsten K. (karsten42)]

Hallo Karl Heinz,

Ja, du hast recht! Ich war(bin) auch nicht so sehr mit sja_add 
zufrieden, hatte aber gescheut die sja_write() und sja_read() umzubauen.

O.K. ich werde deinen Vorschlag beherzigen und das ganze ohne globales 
Schalten realisieren. Das wird dann hoffentlich betriebssicherer.
Das Problem mit der Variablen "active" wird aber bestehen bleiben. Es 
sei denn, der Fehler liegt tatsächlich am globalen sja_add.

By the Way:
Von der "Schönheit" her würde ich die Makros
ENTER_CRITICAL_SECTION und LEAVE_CRIRICAL_SECTION für die Funktion 
_send_message() gern innerhalb der Funktion definieren.

Mir ist nur nicht klar, ob dies negative Auswirkungen hat.

uint8_t
_send_message(const can_t *msg)  {

ENTER_CRITICAL_SECTION
...
LEAVE_CRIRICAL_SECTION
}

// same as ??????
LEAVE_CRIRICAL_SECTION
_send_message(&msg);
LEAVE_CRIRICAL_SECTION

Herzlichen Dank für deine Einschätzung und deinen Tipp!

Gruß
Karsten

[Karl H. (kbuchegg)]

Karsten K. schrieb:

> Ja, du hast recht! Ich war(bin) auch nicht so sehr mit sja_add
> zufrieden, hatte aber gescheut die sja_write() und sja_read() umzubauen.

Das sind 2 Funktionen, denen du ein zusätzlicxhes Argument gibst.
Dann jagst du den Compiler drüber und der sucht dir alle Aufrufe (in 
Form von Fehlermeldungen) raus, an denen beim Aufruf dieses Argument 
ergänzt werden muss. Es wird Fälle geben, in denen der Aufruf in einer 
Funktion steckt, die dann selbst wieder ein zusätzliches Argument 
bekommt und es einfach nur weitergibt.

Alles in allem ist der Umbau (beim gezeigten Code) in 10 Minuten 
abgeschlossen. Alles kein Problem.


> Schalten realisieren. Das wird dann hoffentlich betriebssicherer.
> Das Problem mit der Variablen "active" wird aber bestehen bleiben.

Ich denke, dass das ein Nebensachauplatz ist.
Dein 'funktioniert nicht' ist nicht unbedingt auf diese active Variable 
zurückzuführen. Wie schon gesagt: du hast (im gezeigten Code) keine 
Warteschleifen, die auf eine Veränderung dieser Variablen warten würden. 
Durch die Codegröße und die Anzahl der Variablen sowie die Aufteilung in 
Funktionen hat der Compiler sowieso keine Wahl - er kann diese Active 
Variable nicht die ganze Zeit in einem Register halten.

[Karl H. (kbuchegg)]

Die Logik in

uint8_t
sja_send_message(const can_t *msg, const uint8_t sja)  {

ist mir noch unklar.
Ich denke da stimmt was nicht.

[Karl H. (kbuchegg)]

Karl Heinz schrieb:
> Die Logik in
>
>

> uint8_t
> sja_send_message(const can_t *msg, const uint8_t sja)  {
>

>
> ist mir noch unklar.
> Ich denke da stimmt was nicht.

Der ganze erste Teil der Funktion ist mir nicht koscher.

Ich würds so angehen

uint8_t
sja_send_message(const can_t *msg, const uint8_t sja)  {

  ENTER_CRITICAL_SECTION;

  // kann direkt über den SJA gesendet werden, oder muss die msg
  // gequed werden
  if(!(sja_read(SR, sja) & (1<<TBS)))  {

    // SJA ist nicht frei, msg muss in die Queue
    can_t *buf = can_buffer_get_enqueue_ptr(&txbuf[sja].buffer); 
    
    if (buf == NULL)  {
      return 9;    // buffer full
    }
    
    // copy message to the buffer and queue it
    memcpy(buf, msg, sizeof(can_t));
    can_buffer_enqueue(&txbuf[sja].buffer);

  } else {

    // SJA ist frei. die msg kann direkt abgesetzt werden
    _send_message(msg, sja);
  }
  LEAVE_CRITICAL_SECTION;

  return(2);
}

PS: jetzt seh ich hier auch, was mich noch stört.
Der Member 'buffer' hat einen schlechten Namen. Das ist offenbar ein 
Member, den die Queue Routinen benötigen, und in denen die sich 
INformation darüber ablegen, welche Einträge in

   can_t list[CAN_TX_BUFFER_SIZE];

benutzt sind und welche nicht.

Also eine Control-Variable.

Dann würde ich die Member so benennen

typedef struct  {

  volatile uint8_t active;        // Flag set if transmission is in progress
  can_buffer_t     QueueControl;
  can_t            QueueBuffer[CAN_TX_BUFFER_SIZE];

} txbuf_t;

das macht dann den Code beim reinen Lesen logischer

    // SJA ist nicht frei, msg muss in die Queue
    can_t *QueueItem = can_buffer_get_enqueue_ptr( &txbuf[sja].QueueControl ); 
    
    if (QueueItem == NULL)  {
      return 9;    // buffer full
    }
    
    // copy message to the Queue Element and queue it
    memcpy( QueueItem, msg, sizeof(can_t) );
    can_buffer_enqueue( &txbuf[sja].QueueControl );

ich finde diese Version leichter zu lesen, weil man nicht dauernd um die 
Ecke denken muss. Mit dem Begriff 'buffer' verbindet man ja intuitiv 
etwas - nämlich ein Array in dem etwas gespeichert wird. Nur ist hier 
der 'buffer' eben kein Buffer in diesem Sinne, sondern einfach nur ein 
Hilfselement der Queue Routinen.

-> man kann durch geschickte Wahl von Variablennamen sich das Leben 
leichter oder schwerer machen.


Das hier

   can_buffer_enqueue( &txbuf[sja].QueueControl );

sieht auch eigenartig aus, aber ok.
Das Prinzip ist mir, denk ich klar.
Zuerst besorgt man sich mit
    can_buffer_get_enqueue_ptr
einen Pointer auf ein Queue Element, das man benutzen kann. 
can_buffer_enqueue 'aktiviert' dann dieses Element in der Queue. Das 
geht deswegen, weil sich die Queue Routinen irgendwo merken, welchen 
Pointer sie als letztes beim get_ptr rausgerückt haben.
Finde ich nicht gut gemacht. Da ist schon wieder eine Race Condition 
drinnen. Zwischen der get_ptr Funktion und enqueue darf kein Interrupt 
auftreten, der seinerseits etwas in diese Queue zu stellen versucht.

Solche implizten Zusammenhänge, quer über Funktionsaufrufe, sind 
gefährlich! Da ist schon so mancher reingefallen. Es ist verführereisch 
zb in der Programmierung einer linearen Liste ein sog. activeElement in 
die Datenstruktur mit aufzunehmen. Auf lange Sicht gesehen ist das aber 
parktisch immer ein 'pain in the ass'.

[Achim K. (aks)]

return 9;

ohne

LEAVE_CRITICAL_SECTION;

?

[Karl H. (kbuchegg)]

Achim K. schrieb:
>

> return 9;
>

> ohne
>

> LEAVE_CRITICAL_SECTION;
>

> ?

Wo du recht hast, hast du recht.
Der ist mir entgangen.

[Karsten K. (karsten42)]

Hallo KarlHeinz,

Eine menge Info die ich ersteinmal nachvollziehen muss. Den Umbau habe 
ich tatsächich in 10 Minuten geschafft, herzlichen Dank.

Weiters sehe ich mir jetzt an ( war zwischendurch beim Bäcker :-).  Im 
Augenblick kracht das ganze sogar ab und zu mal. In sofern, dass der 
AT90CAN128 einen Reset ausführt! Die Resetleitung ist´s nicht; da hatte 
ich schon mal den reset-IC aus dem OLIMEX Testboard erwischt als der 
sporatisch 500ns lang Reset erzeugt hat!

O.K. Schritt für Schritt nach dem alten Cäsar: Teile und herrsche!

Ich melde mich gern zurück.

Gruß
Karsten

[Karsten K. (karsten42)]

Hallo, nochmal  ich :-)

Ich habe gerade beschlossen die Steckbrettvariante komplett über den 
haufen zu schmeissen; irgendwie muss ich mich zumindest auf eine 
variable in diesem projekt verlassen können ( Wackelkontackte, lange 
leitungen usw. ). Unberechenbare Resets sind ja kein Spass mehr.

Also ein kleines reboot :-(

Zu den Buffern:
Nun, index geführte Buffer listen sind vielleicht nicht der letzte 
schrei, aber auch eine (doppelt) gelinkte Liste muss beim Zugriff 
"gesichert" werden. Der verwendete Code ist in allen entscheidenen durch 
ATOMIC_BLOCK makros gesichert.

Falls aber eine benutzbare Bufferwverwaltung für die kleinen AVR´s zur 
"Hand" ist und ich diese nutzen darf, wäre ich höchst begeistert diese 
zu bekommen.

Herzlichsten Dank nochmals für die geniale Hilfe und den Anstoß nochmals 
am Grunde zu beginnen. Am Ende ist es dann doch einfacher als ewiges 
rungefrickel mit labilem Code.

Gruß
Karsten