unverständliche Optimierung beim Schreiben von PORTs

[pschyrum (Gast)]

heyo,

gerade bin ich bei einem Projekt, in dem ein Tiny24 Blinkfolgen ausgeben 
soll - auf zwei PORTS verteilt. Die zu schreibenden Bytes werden 
nacheinander generiert - die ersten 6 Bits gehen an PORTA:0-5, die 
höchsten beiden an PORTB:0-1.

 while(1) {
    if (..) {
      byteA = calculate_byte(message);
      PORTB = byteA>>6;
      PORTA = byteA;
      bits.set = 1;
    } 
    
    if (..) {
      PORTA = 0;
      PORTB = 0;
    }
 }

Wenn ich den Code compiliere und übertrage, funktioniert das nur ohne 
Optimierung (-O0) wie gewünscht.
Bei -Os schaltet er grundsätzlich alle LEDs an und gibt Müll aus.
Wenn ich nur jeweils einen PORT beschreibe mit dem gewünschten Byte, und 
den anderen mit einem festem Wert, geht das allerdings.
Sobald ich beide PORTs mit dem gleichen Byte beschreibe, geht es nicht.
Schreibe ich auf den ersten PORT ein Byte, hole mir das nächste und 
schreibe die höchsten beiden Bits auf den anderen PORT - geht das.

Der Assembler-Code macht eigentlich auch das richtige:

 218:   9c df           rcall   .-200           ; 0x152 <calculate_byte>
 21a:   80 93 60 00     sts     0x0060, r24
 21e:   80 91 60 00     lds     r24, 0x0060
 222:   82 95           swap    r24
 224:   86 95           lsr     r24
 226:   86 95           lsr     r24
 228:   83 70           andi    r24, 0x03       ; 3
 22a:   88 bb           out     0x18, r24       ; 24
 22c:   80 91 60 00     lds     r24, 0x0060
 230:   8b bb           out     0x1b, r24       ; 27

0x18 und 0x1b sind die Portregister.

Ich habe noch einen Hardware PWM auf PB2 - aber der sollte ja unabhängig 
davon sein, oder?

TCCR0A = (1<<COM0A1) | (1<<WGM01) | (1<<WGM00);
  // prescaler = 8   --> CLK 1Mhz; 255us
TCCR0B = (1<<CS01);

Interrupts sind keine aktiv.

Ich hab echt keine Ahnung woran das liegen könnte - und würde mich 
freuen über Ideen und Vorschläge.
Kann gern auch noch mehr Code posten - bin aber irgendwie sicher, dass 
es an der Optimierung liegt.

Vielen Dank schonmal
Benni

[Ralf G. (ralg)]

pschyrum schrieb:
> Kann gern auch noch mehr Code posten - bin aber irgendwie sicher, dass
> es an der Optimierung liegt.

Immer ein compilerbares Minimalprogramm, das den Fehler enthält! Fast 
immer liegt der Fehler woanders.

[Oliver (Gast)]

pschyrum schrieb:
> Der Assembler-Code macht eigentlich auch das richtige:

Tja, in dem Fall ist dein Tiny kaputt. Was anderes kommt ja nicht in 
Frage...

Oliver

[Karl H. (kbuchegg)]

pschyrum schrieb:

> Ich hab echt keine Ahnung woran das liegen könnte - und würde mich
> freuen über Ideen und Vorschläge.
> Kann gern auch noch mehr Code posten - bin aber irgendwie sicher, dass
> es an der Optimierung liegt.

Das kann schon sein. Aber nicht an dieser Stelle. Wie du ja selber 
siehst, hat der Compiler exakt das umgesetzt, was du programmiert hast.

Du solltest dir im Übrigen solche Rundumschlag-Portzugriffe abgewöhnen. 
Wenn du nur die Bits 0 bis 5 beeinflussen willst, dann tu auch genau 
das. Wir wissen nicht, was sonst noch so alles an den restlichen 
Port-Pins hängt, und was daher noch alles ungewollt umgestellt wird.


      PORTB = ( PORTB & ~0x03 ) | ((byteA>>6) & 0x03);
      PORTA = ( PORTA & ~0x3F ) | (byteA & 0x3F );

so ist es sauber. Es werden nur die tatsächlich benutzten Bits 
verändert. Alle anderen bleiben gleich. Und wenn du auch noch Interrupts 
im Spiel hast, dann sollte das ganze dann auch noch unter Interrupt 
Sperre laufen (ok, hast du nicht).

Und byteA sollte besser ein unsigned char sein oder ein uint8_t.

[pschyrum (Gast)]

woah! Stark, das ging ja extrem schnell.
Besten Dank für die Antworten!

Kritik am Code ist immer erwünscht :), habe das mal soweit angepasst und 
alles unwesentliche entfernt.

#include <avr/io.h>      
#define F_CPU 8000000UL

// in ms
#define REFRESH_RATE 50
// max chars - be aware of free SRAM 
#define MAX_CHARS 10
// led-out time in ms
#define DISTANCE 1000
// clarity of LEDS :: 0 to 255
#define CLARITY 120

const __flash uint8_t letter_A[] = {5, 0b00011111,
                     0b01101000,
                     0b10001000,
                     0b01101000,
                     0b00011111};
const __flash uint8_t letter_B[] = {3, 0b11111111,
                     0b10010001,
                     0b01101110};
const __flash uint8_t * const __flash accessDef[] = {letter_A, 
                           letter_B};
// booleans
typedef struct {
  // bit that signals whether char is set
  unsigned set: 1;
} singleBits;

void read_message(uint8_t *message) {
  // initalizing with B(1) A(0) B(1) and 'end' (255)
  message[0] = 1;
  message[1] = 0;
  message[2] = 1;
  message[3] = 255;
}

uint8_t calculate_byte(uint8_t* message) {        
  static uint8_t charIndex = 0;
  static uint8_t messageIndex = 0; 
  static uint8_t charLength = 0;
  uint8_t  nextByte;  
  // still message to come
  if (message[messageIndex] != 255) {
    // no length given yet 
    if (charLength == 0) {
      charLength = accessDef[message[messageIndex]][0];
    }
    // read next byte
    charIndex += 1;
    nextByte = accessDef[message[messageIndex]][charIndex];
    // last byte -> go to next char
    if (charIndex == charLength) {
      charLength = 0;
      charIndex = 0;
      messageIndex++;
    }
  } else {
    nextByte = 0;
    messageIndex = 0;
  }
  return nextByte;
}
  
int main (void) {      
  
  // port definitions - PB(0-4) set as output
    DDRA = (1<<PA0) | (1<<PA1) | (1<<PA2) | (1<<PA3) | (1<<PA4) | (1<<PA5);       
    DDRB = (1<<PB0) | (1<<PB1) | (1<<PB2); 
    // pwm settings
    OCR0A = CLARITY;
    
    // disable all output
  PORTA = (PORTA & ~0x3F);
  PORTB = (PORTB & ~0x03);
  
  // Timer0 settings
  // clear OC0A on Compare match (COM0A1) and FAST PWM (WGM01 WGM00)
  TCCR0A = (1<<COM0A1) | (1<<WGM01) | (1<<WGM00);
  // prescaler = 8   --> CLK 1Mhz; 255us
  TCCR0B = (1<<CS01);     
  // set Timer0 value to 0
  TCNT0 = 0;    
  
  uint8_t message[MAX_CHARS];
  uint16_t counter = 0;
  uint8_t byteA;
  singleBits bits;
  bits.set = 0;
  
  // get message
  read_message(message);
  
  while(1) {
    
    // switch on leds
    if ((counter == DISTANCE*4) & (bits.set == 0)) {
      byteA = calculate_byte(message);
      // 2 highest bits to PORTB
      PORTB = (PORTB & ~0x03) | ((byteA>>6) & (0x03));
      // 6 remaining bits to PORTA
      PORTA = (PORTA & ~0x3F) | (byteA & 0x3F);
      bits.set = 1;
    }
    
    // switch of leds
    if (counter == 4*REFRESH_RATE + DISTANCE*4) {
      PORTA = (PORTA & ~0x3F);
      PORTB = (PORTB & ~0x03);
      counter = 0;
      bits.set = 0;
    }
    
    if (TIFR0 & (1<<TOV0)) {
        // set overflow flag
        TIFR0 |= (1<<TOV0);
        counter += 1;
    }
  }  
}

Die Beschaltung ist eigentlich super einfach - die LEDS werden mit der 
Anoden an jew. einen Pin, mit der Katode alle zusammen an' Kollektor vom 
Transistor, der von PB2 angesteuert wird.

Bin schon gespannt was da falsch sein könnte ..
Besten Dank nochmal!

mfg
Benni

[Felix P. (fixxl)]

Zwei kleine Anmerkungen, auch wenn sie auf die Funktion des Programms 
keinen Einfluss haben dürften:

if ((counter == DISTANCE*4) & (bits.set == 0))

Das wird zwar funktionieren, weil durch die '==' entweder 0 oder 1 links 
und rechts des &-Operators steht, aber die logische Verknüpfung sollte 
dennoch mit && gemacht werden.

// set overflow flag
   TIFR0 |= (1<<TOV0);

Du willst das Flag hier sicher löschen, gesetzt wird es ja durch den 
Timer Overflow von selbst. Um es zu löschen, sollte die Anweisung

// clear overflow flag
   TIFR0 = (1<<TOV0);

lauten, also ohne die "Veroderung". Spielt in deinem speziellen Fall 
keine große Rolle, da du keine anderen Flags aus dem TIFR0-Register 
betrachtest, durch die Veroderung würden die aber - sofern sie gesetzt 
sind - durch deinen Befehl gelöscht werden.

[Hmm (Gast)]

Felix Pflaum schrieb:
> Zwei kleine Anmerkungen, auch wenn sie auf die Funktion des Programms
> keinen Einfluss haben dürften:
>
>

if ((counter == DISTANCE*4) & (bits.set == 0))

> Das wird zwar funktionieren, weil durch die '==' entweder 0 oder 1 links
> und rechts des &-Operators steht, aber die logische Verknüpfung sollte
> dennoch mit && gemacht werden.
>
>
>

// set overflow flag
>    TIFR0 |= (1<<TOV0);

> Du willst das Flag hier sicher löschen, gesetzt wird es ja durch den
> Timer Overflow von selbst. Um es zu löschen, sollte die Anweisung
>

// clear overflow flag
>    TIFR0 = (1<<TOV0);

> lauten, also ohne die "Veroderung". Spielt in deinem speziellen Fall
> keine große Rolle, da du keine anderen Flags aus dem TIFR0-Register
> betrachtest, durch die Veroderung würden die aber - sofern sie gesetzt
> sind - durch deinen Befehl gelöscht werden.

Ich fürchte das hast Du Dich vertan.
Es ist genau umgekehrt. Die ursprüngliche Anweisung

  // set overflow flag
  TIFR0 |= (1<<TOV0);

würde andere Bits al TOV0, sofern sie gesetzt sind auch gesetzt lassen 
und sofern sie gelöscht sind, sie auch gelöscht lassen.

Deine neue Anweisung würde aber Bits löschen die gesetzt sind.

Abgesehen davon ist der ursprüngliche (also nicht von Dir stammende) 
Kommentar falsch. Mit dem schreiben einer 1 wird das TOV0-Flag gelöscht 
und nicht gesetzt.

[Felix P. (fixxl)]

Hmm schrieb:
> Ich fürchte das hast Du Dich vertan.
> Es ist genau umgekehrt. Die ursprüngliche Anweisung
>   // set overflow flag
>   TIFR0 |= (1<<TOV0);
> würde andere Bits al TOV0, sofern sie gesetzt sind auch gesetzt lassen
> und sofern sie gelöscht sind, sie auch gelöscht lassen.
>
> Deine neue Anweisung würde aber Bits löschen die gesetzt sind.
>
> Abgesehen davon ist der ursprüngliche (also nicht von Dir stammende)
> Kommentar falsch. Mit dem schreiben einer 1 wird das TOV0-Flag gelöscht
> und nicht gesetzt.

Nein, habe ich nicht. Interrupt-Flags löscht man tatsächlich ohne die 
Oder-Anweisung. Nachzulesen ist das u.a. hier:
http://www.mikrocontroller.net/articles/AVR_Checkliste#Flag_richtig_gel.C3.B6scht.3F

Hintergrund: Man löscht das Flag durch Schreiben einer 1 ins Register. 
Nun ist die Anweisung

TIFR0 |= (1<<TOV0);

gleichbedeutend mit:

TIFR0 = TIFR0 | (1<<TOV0);

Man schreibt damit eine 1 an jede Stelle, an der vorher schon eine 1 
stand und zusätzlich auch an die durch TOV0 bezeichnete Stelle, löscht 
also alle gesetzten Flags.

Schreibt man hingegen eine 0 ins Register, wo vorher eine 1 stand, 
bleibt die 1 stehen. Das Flag-Register verhält sich hier einfach anders 
als die PORT- oder DDR-Register.

[Hmm (Gast)]

@  Felix Pflaum (fixxl)

>Nein, habe ich nicht.

Tatsächlich. Du hast das ganz richtig beschrieben. Ich lag falsch. 
Sorry.

[Josef D. (jogedua)]

Felix Pflaum schrieb:
> Nun ist die Anweisung

TIFR0 |= (1<<TOV0);

> gleichbedeutend mit:

TIFR0 = TIFR0 | (1<<TOV0);

das stimmt wohl nur bei ausgeschalteter Optimierung.
Mit Optimierung wird doch

TIFR0 |= (1<<TOV0);

durch einen Bit-Setz-Befehl (sbi) ersetzt, wenn das möglich ist.
In diesem Fall würde sogar das erwartete Ergebnis eintreten und das 
Programm korrekt arbeiten; böse Falle.

[pschyrum (Gast)]

hey,

danke schonma' für die Antworten.
Oops, dann ohne das '|', wieder was gelernt!
Das 'clear flag' ist natürlich der richtige Kommentar ;) - ebenso müsste 
es 'brightness'heißen nicht 'clarity' :P

Irgendwelche Ideen, was falsch sein könnte?

Gerade habe ich noch etwas rumprobiert: Wenn ich

message[0] = 1;

setze, dann geht gar nichts.
Setze ich

message[0] = 0;

 geht die erste Blinkfolge, die folgenden aber nicht.
Setze ich nur PORTA oder nur PORTB gehen alle Blinkfolgen richtig, egal 
was in message[0] steht.

Hö?
Faszinierend wie mich der avrgcc austrickst ;)

Noch irgendwelche Hinweise, Ideen? Würde mich sehr freuen!

mfg
Benni

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

Josef D. schrieb:
> In diesem Fall würde sogar das erwartete Ergebnis eintreten und das
> Programm korrekt arbeiten; böse Falle.

Ist im Datenblatt nicht explizit ausgewiesen.  SBI verhält sich in
den allermeisten Fällen schon genauso wie PORT = PORT | bit, also
als komplettes read-modify-write, einschließlich der entsprechenden
Seiteneffekte.

Ob das genau hier auch so ist, müsste man im Zweifelsfalle
ausprobieren.

[Josef D. (jogedua)]

Das scheint sogar je nach CPU unterschiedlich zu sein
(Eine Fußnote aus dem Datenblatt zum AtMega328, doc827-rev1209, S.535):

"3. Some of the Status Flags are cleared by writing a logical one to 
them. Note that, unlike most other AVRs, the CBI and SBI
instructions will only operate on the specified bit, and can therefore 
be used on registers containing such Status Flags. The
CBI and SBI instructions work with registers 0x00 to 0x1F only."

und hängt vielleicht hiermit zusammen (S. 78):

"13.2.2 Toggling the Pin
Writing a logic one to PINxn toggles the value of PORTxn, independent on 
the value of DDRxn.
Note that the SBI instruction can be used to toggle one single bit in a 
port."

[Felix P. (fixxl)]

pschyrum schrieb:
> Noch irgendwelche Hinweise, Ideen? Würde mich sehr freuen!
Was genau passiert in der Funktion calculateByte(...)? Kommt da das 
heraus, was herauskommen soll?

Wenn ich "message" mit {1, 0, 1, 255} initialisiere und die Funktion 
calculateByte(message) 16-mal aufrufe, erhalte ich als Ergebnis: 255 145 
110 31 104 136 104 31 255 145 110 0 255 145 110 31.

[pschyrum (Gast)]

hey,

nun, eigentlich kommt schon das raus was soll - vorausgesetzt ich 
schreibe das Ergebnis nur auf einen Port.

calculate_byte funktioniert super einfach - die Adresse von message wird 
übergeben - für jeden Eintrag wird das entsprechende Array aus Bytes 
durchgegangen und pro Aufruf ein Byte übergeben.

Ich habs mal sehr ausführlich dokumentiert.

// Arrays, die die jeweiligen Muster repräsentieren.
// Der erste Wert ist jeweils die Breite des Muster bzw. die Länge
// des Arrays - 1
const __flash uint8_t letter_A[] = {5, 0b00011111,
                     0b01101000,
                     0b10001000,
                     0b01101000,
                     0b00011111};
const __flash uint8_t letter_B[] = {3, 0b11111111,
                     0b10010001,
                     0b01101110};
// Array, dass die Adressen der Muster abspeichert
// Zugriff erfolgt somit über zweimaliges dereferenzieren:
// accessDef[0][0] entspricht 1. Eintrag letter_A - also Arraylänge - 1
// accessDef[Muster][Byte]
const __flash uint8_t * const __flash accessDef[] = {letter_A, 
                           letter_B};
                           
uint8_t calculate_byte(uint8_t* message) {        
  // Variablen benötigt fürs Durchlaufen durch die Nachricht.
  // gibt das aktuelle Byte im Buchstaben an
  static uint8_t charIndex = 0;
  // gibt den aktuellen Buchstaben in der Botschaft an
  static uint8_t messageIndex = 0; 
  // Breite des Musters des aktuellen Buchstabens
  static uint8_t charLength = 0;
  uint8_t  nextByte;  
  
  // wenn das Ende der Botschaft nicht erreicht wurde
  if (message[messageIndex] != 255) {
    // die Länge des Buchstabens noch nicht bekannt ist
    if (charLength == 0) {
      // den 1. Eintrag des aktuellen Buchstabens holen
      charLength = accessDef[message[messageIndex]][0];
    }
    // read next byte
    // auf das nächste Byte zeigen
    charIndex += 1;
    // Byte abholen
    nextByte = accessDef[message[messageIndex]][charIndex];
    // wenn die Breite bereits dem Index entspricht, abbrechen
    if (charIndex == charLength) {
      // Buchstabenbreite muss wieder berechnet werden
      charLength = 0;
      // Rücksetzen
      charIndex = 0;
      // nächster Buchstabe soll begonnen werden
      messageIndex++;
    }
  } else {
    // im Falle dass die Botschaft geendet hat
    nextByte = 0;
    // zurück zum ersten Buchstaben
    messageIndex = 0;
  }
  return nextByte;
}

Könnte das auch an der avrgcc Version liegen? Oder am (noch) nicht 
optimalen __flash ?

Dankbar für weitere Hinweise.

[Karl H. (kbuchegg)]

pschyrum schrieb:

> Könnte das auch an der avrgcc Version liegen? Oder am (noch) nicht
> optimalen __flash ?

Ich weiß nicht, wie gut __flash implementiert ist und ob da noch Fehler 
drinnen sind. Dein zugriff geht ja auch einmal indirekt übers Flash.

Probiers halt aus. Nimm das __flash raus und sieh nach, ob sich was 
ändert.

[pschyrum (Gast)]

ich hab mir grad die von Felix geposteten Dezimalzahlen ins Binäre 
konvertiert und angeschaut - entspricht genau den richtigen Bitmuster 
-.-

Zwischendurch habe ich noch etwas weiterprogrammiert, an einer anderen 
Stelle. Nach dem Übertragen hat nicht nur diese Funktion (analog - 
digital Konvertierung die die Blinkfrequenz anpasst) funktioniert - nein 
es ging auch plötzlich der Rest.
Der neu hinzugekommene Teil nimmt allerdings in keinster Weise Einfluss 
auf das vorhandene.

Da ich dann vollkommen verwirrt war, habe ich den hier von mir 
geposteten Code wieder kompiliert und übertragen. Er ging. HMPF.
avrgcc müsste eigentlich den gleichen Assemblercode erzeugt haben - da 
sich nichts geändert hat. Trotzdem geht es. Die zusätzliche Beschaltung 
habe ich rückgängig gemacht.
Ich hätte ja gesagt - Tiny  Programmiergerät  Schaltung selbst kaputt. 
Aber ohne Optimierung ging es zuvor immer.

Jetzt hab ich grad mal meine avrgcc Version gecheckt - bin etwas 
erstaunt über das prerelease..
"gcc-Version 4.8.0 20130502 (prerelease) (GCC)"

Fehler dort zu suchen?

Danke in jedem Fall für all die Mühe!

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

pschyrum schrieb:
> Fehler dort zu suchen?

Sehr wahrscheinlich nicht.  Inzwischen ist 4.8.0 allerdings auch
freigegeben worden, kannst du also auch upgraden, wenn du willst.

[Oliver (Gast)]

Josef D. schrieb:
> Note that, unlike most other AVRs, the CBI and SBI
> instructions will only operate on the specified bit, and can therefore
> be used on registers containing such Status Flags.

Mir ist noch kein AVR untergekommen, bei dem das nicht so wäre.

Oliver

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

Oliver schrieb:
> Mir ist noch kein AVR untergekommen, bei dem das nicht so wäre.

Das war anfangs anders: "Some of the status flags are cleared by writing 
a logical one to them. Note that the CBI and SBI instructions will 
operate on all bits in the I/O register, writing a one back into any 
flag read as set, thus clearing the flag."

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

Josef D. schrieb:
> Das scheint sogar je nach CPU unterschiedlich zu sein

Streng genommen darf bei CPUs mit der neuen Eigenschaft von SBI/SBI ein 
C Compiler eine |= Operation bei volatilem linken Operanden nicht mehr 
mit SBI implementieren. Da hat man wohl einen 8051-Kämpen ran gelassen, 
die sind diese Form klammheimlicher Standardverletzung gewohnt.

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

A. K. schrieb:
> Da hat man wohl einen 8051-Kämpen ran gelassen,
> die sind diese Form klammheimlicher Standardverletzung gewohnt.

Was hast du denn heute geraucht? ;-)

Der AVR-GCC hat einfach keine Kennung davon, dass sich da
irgendwelche CPUs subtil anders verhalten.  Schließlich erzählt
Atmel das ja auch niemandem von denen, die da am GCC basteln.
Das Einzige, bei dem dann klar war, dass es deutliche(re)
Unterschiede gibt, ist die Xmega-Serie.  Alle vorangegangenen
CPUs implementieren im GCC mehr oder minder das gleiche CPU-Modell
(modulo der verschiedenen Speichergrößen und davon abhängigen
Befehlsvielfalt bzw. der CPUs, die MUL und word-Befehle enthalten).

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

Jörg Wunsch schrieb:
> Was hast du denn heute geraucht? ;-)

Das Kraut heisst "Pedant". Keine Sorge, das vergeht wieder. ;-)

> Der AVR-GCC hat einfach keine Kennung davon, dass sich da
> irgendwelche CPUs subtil anders verhalten.

Keine blasse Ahnung von der Maschine haben ist für einen Compilerbauer 
eine äusserst schwache Entschuldigung. ;-)

Bei den Compilern für 51er ist das ausserdem Vorsatz. Da wars nie 
anders: R-M-W Befehle lesen das Ausgaberegister (vgl PORTx), 
aufgedröselt wird jedoch den Portzustand (vgl PINx) gelesen. Bei (fast) 
Open Drain Ports ist der Unterschied recht bedeutsam.

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

A. K. schrieb:
> Keine blasse Ahnung von der Maschine haben ist für einen Compilerbauer
> eine äusserst schwache Entschuldigung.

"Keine blasse Ahnung" ist aber auch 'ne mächtige Übertreibung
dafür, dass offenbar einzelne CPUs (und dann teilweise auch noch
nur auf einzelnen Ports) anders implementiert worden sind als
andere.

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

Jörg Wunsch schrieb:
> "Keine blasse Ahnung" ist aber auch 'ne mächtige Übertreibung

Ja. ;-)