Forum: Compiler & IDEs Compiler Optimization Level (-O1) ändert Ablauflogik?


von Mike (Gast)


Lesenswert?

Hallo zusammen,

ich habe im Compiler Optimization Level (-O1) aktiviert, damit mein 
Programm auf den ATtiny2313 passt. Allerdings scheint der Compiler dann 
ein paar IF-Abfragen rauszuoptimieren, so dass sich die Ablauflogik 
komplett ändert - kann das sein?!? Das wäre aber blöd - wie geht man 
damit um? Das würde ja die Fehlersuche verdammt schwer machen...

Hier der Programmausschnitt:

<c>...
        if (startbit_ok==2) {
          zaehler_phasen++;
          counts_pro_phase[zaehler_phasen]=interrupt_count;
          signal_changed=0;
          interrupt_count=0;
          }

        if ((startbit_ok==1) && (interrupt_count>=300) && 
(interrupt_count<=400)) {
          interrupt_count=0;
          signal_changed=0;
          startbit_ok=2;}

        if ((startbit_ok<=1) && (interrupt_count>=700) && 
(interrupt_count<=800)) {
          interrupt_count=0;
          signal_changed=0;
          startbit_ok=1;}
</c>

interrupt_count ist nie >200, so dass die Bedingungen 2 und 3 nie wahr 
werden sollten - aber trotzdem setzt das Programm startbit_ok=2 - aber 
nur mit (-O1). Ohne Optimization verhält es sich wie erwartet...

VG Mike

: Verschoben durch Admin
von Karl H. (kbuchegg)


Lesenswert?

Da ist doch sicher wieder in irgendeiner Form ein Interrupt bzw. eine 
ISR involviert und einigen deiner Variablen fehlt die volatile Kennung.

Zumindest stinkt das Fehlersymptom danach.

FAQ: Was hat es mit volatile auf sich

Oder aber, was auch noch sein könnte: kein atomarer Zugriff auf 16-Bit 
Variablen, so dass es zu Auslesefehler kommt.

von Mike (Gast)


Lesenswert?

Karl Heinz Buchegger schrieb:
> Da ist doch sicher wieder in irgendeiner Form ein Interrupt bzw. eine
>
> ISR involviert und einigen deiner Variablen fehlt die volatile Kennung.

ISR ist involviert, aber die gemeinsam genutzten Variablen sind 
volatile. Hier ein etwas größerer Programmausschnitt:

volatile uint16_t interrupt_count;
volatile uint8_t signal_changed;

...

if (signal_changed!=0)
      {
        cli();

        if (startbit_ok==2) {
          zaehler_phasen++;
          counts_pro_phase[zaehler_phasen]=interrupt_count;
          signal_changed=0;
          interrupt_count=0;
          }

        // Anfangsrauschen unterdrücken --> Aufzeichnung beginnen mit 
Startbit Pulse

        if ((startbit_ok==1) && (interrupt_count>=300) && 
(interrupt_count<=400)) {
          interrupt_count=0;
          signal_changed=0;
          startbit_ok=2;}

        if ((startbit_ok<=1) && (interrupt_count>=700) && 
(interrupt_count<=800)) {
          interrupt_count=0;
          signal_changed=0;
          startbit_ok=1;}

        sei();
      }

...

ISR(TIMER0_COMPA_vect)
{
  interrupt_count++;
}

ISR(PCINT_vect)
{
  signal_changed=1;
  timer_config_start;
}

Karl Heinz Buchegger schrieb:
> FAQ: Was hat es mit volatile auf sich

Hab ich auf Anhieb keinen Hinweis gefunden, aber lese es nachher 
intensiver.

Karl Heinz Buchegger schrieb:
> Oder aber, was auch noch sein könnte: kein atomarer Zugriff auf 16-Bit
>
> Variablen, so dass es zu Auslesefehler kommt.

Das könnte sein - was heißt genau "atomarer Zugriff" - also ich habe den 
ganzen Programmblock in ein cli() ... sei() eingeschlossen - ist das der 
falsche Ansatz?

von Mike (Gast)


Angehängte Dateien:

Lesenswert?

Basierend auf Deinen Hinweisen habe ich nochmal etwas rumprobiert - und 
bin jetzt noch mehr verwirrt...:

- Die beiden Variablen, die in der ISR verwendet werden, waren schon 
vorher als volatile deklariert.
- Zum Test habe ich dann auch alle anderen Variablen (auf Verdacht) mit 
volatile versehen --> dann wurde scheinbar weniger wegoptimiert und das 
IF-Statement hat sich "wie erwartet" verhalten. Da aber das eigentliche 
Programm trotzdem nicht gelaufen ist, bin ich nochmal zurück zum Anfang 
(Code siehe Anlage):

Programm funktioniert (zumindest scheinbar = d. h. IR-Signal wird 
aufgezeichnet und dann angezeigt) mit folgender Deklaration:

int main(void)
{
  // Variablen
  uint8_t modus;
...

Ergänze ich bei "modus" ein "volatile", so funktioniert das Programm 
nicht mehr (d. h. weder Kontroll-LED blinkt nach Aufzeichnung noch wird 
etwas angezeigt)

int main(void)
{
  // Variablen
  volatile uint8_t modus;
...

Ist das so zu interpretieren, dass das Programm im ersten Fall nur den 
Anschein macht, so zu funktionieren wie gewollt bzw. nur deshalb läuft, 
weil die Optimierung einen "Logikfehler" rausoptimiert? Bin verwirrt...

von Peter II (Gast)


Lesenswert?

bist du sicher das es überhaupt sinnvoll geht?

wenn ich sehe wie lange du die ISR sperrst:
1
cli();
2
PORTB&=!(1<<PB2);
3
_delay_ms(1000);
4
PORTB|=(1<<PB2);
5
_delay_ms(1000);
6
7
for (i=1;i<=70;i++)
8
{
9
displayZahl(counts_pro_phase[i],dauer);
10
   _delay_ms(200);
11
}
12
      
13
zaehler_phasen=0;
14
interrupt_count=0;
15
modus=0;
16
sei();

da verpasst du doch sehr viele von den Timer ISR - ob das ok ist, kann 
ich nicht beurteile, aber sinnvoll finde ich das nicht.

von Fabian O. (xfr)


Lesenswert?

Lokale Variablen einer Funktion volatile zu machen ist sinnlos. Darauf 
kann man eh nicht von außerhalb zugreifen.

Du muss lokale Variablen wie modus allerdings mit einem Wert 
initialisieren, bevor Du sie liest! Sonst haben sie einen undefinierten 
Wert. Davor sollte der Compiler aber auch warnen, wenn Du mit -Wall 
kompilierst ...

von Mike (Gast)


Lesenswert?

es funktioniert jetzt :-) danke!!

Fabian O. schrieb:
> Du muss lokale Variablen wie modus allerdings mit einem Wert
>
> initialisieren, bevor Du sie liest! Sonst haben sie einen undefinierten
>
> Wert. Davor sollte der Compiler aber auch warnen, wenn Du mit -Wall
>
> kompilierst ...

Das war wohl der Fehler. Bin davon ausgegangen, dass der Initialwert 0 
wäre, wenn nichts anderes angegeben wird (Anfängerfehler...).

Fabian O. schrieb:
> Lokale Variablen einer Funktion volatile zu machen ist sinnlos. Darauf
>
> kann man eh nicht von außerhalb zugreifen.

Ok, verstanden. Aber irgendwie scheint es doch etwas zu "bewirken", hat 
zumindest den Fehler der fehlenden Initialisierung (s. o.) "aufgedeckt":

- ohne volatile und ohne Initialisierung ging es (zumindest dem Anschein 
nach und auch wiederholbar)
- mit volatile und ohne Initialisierung ging es nicht
- und schließlich mit volatile und Initialisierung geht es wieder :-)

Irgendwie komisch, dass das Programm trotz des "undefinierten" Zustands 
der Variablen (wiederholbar) funktioniert hat - war dann entweder Glück 
(bzw. Pech, je nachdem wie man's sieht) oder Compiler hat das irgendwie 
ausgebügelt (so lange kein volatile drin war)

Peter II schrieb:
> da verpasst du doch sehr viele von den Timer ISR - ob das ok ist, kann
>
> ich nicht beurteile, aber sinnvoll finde ich das nicht.

Ich habe mir nur einen "Versuchsaufbau" gebastelt, bei dem zunächst kurz 
das IR-Signal empfangen wird (Modus = 0) und dann "in Ruhe" über's 
Display angezeigt wird (Modus = 1) - wollte nur prüfen, ob die Signale 
(meines gebastelten Senders) korrekt empfangen werden (z. B. Anzahl und 
Länge Signalphasen). Während der Anzeigephase soll nichts mehr empfangen 
werden - ob das sinnvoll ist, kann man drüber streiten ;-) ist nur zum 
Testen, hat keinen tieferen Sinn/Anwendungsfall...

VG & danke
Mike

von Fabian O. (xfr)


Lesenswert?

Mike schrieb:
> Irgendwie komisch, dass das Programm trotz des "undefinierten" Zustands
> der Variablen (wiederholbar) funktioniert hat - war dann entweder Glück
> (bzw. Pech, je nachdem wie man's sieht) oder Compiler hat das irgendwie
> ausgebügelt (so lange kein volatile drin war)

Der Compiler kann die Variable ohne volatile z.B. in einem 
Prozessor-Register mit Initialwert 0 gehalten haben. Mit dem volatile 
hast Du ihn dagegen zu einem Speicherzugriff gezwungen, wo dann ohne 
Initialisierung offenbar keine 0 oder 1 drinnen stand.

von Mike (Gast)


Lesenswert?

Danke.

Noch ein komisches "Phänomen":

Version 1 des Code (ohne Filter Startbit):

cli();
zaehler_phasen++;
counts_pro_phase[zaehler_phasen]=interrupt_count;
signal_changed=0;
interrupt_count=0;
sei();

liefert folgende Phasendauern (Wert 1 = Pulse / Wert 2 = Pause):

2     --> Noise bei Sendebeginn?
75 36 (Startbit)
15 41 (Datenbit 1)
15 40 (Datenbit 2)
usw.

Version 2 des Code (hier versuche ich das Startbit (und Noise bei 
Sendebeginn) herauszufiltern - Anmerkung: Ziel ist später eine State 
Machine, ohne Aufzeichnung aller Phasendauern):

cli();
// jetzt kommen die Datenbits --> Aufzeichnung beginnen
if (startbit_ok==2){
  zaehler_phasen++;
  counts_pro_phase[zaehler_phasen]=interrupt_count;
  signal_changed=0;
  interrupt_count=0;}
// Startbit Pulse abwarten
if ((startbit_ok<=1) && (interrupt_count>=70) && (interrupt_count<=80)){
  startbit_ok=1;
  signal_changed=0;
  interrupt_count=0;}
// Starbit Pase abwarten
if ((startbit_ok==1) && (interrupt_count>=30) && (interrupt_count<=40)){
  startbit_ok=2;
  signal_changed=0;
  interrupt_count=0;}
sei();

liefert folgende Phasendauern:

13    --> wo kommt das her?? also Startbit mit 75 und 36 wurden 
rausgefiltert, aber eine zusätzliche "Phase" mit 13 Counts (eigentlich 
nur durch Signaländerung am Pin möglich) wurde "eingeschmuggelt"

15 41 (Datenbit 1)
15 40 (Datenbit 2)
usw.

Also bei Startbit Pause von 36 (Testwert!) setze ich starbit_ok=2, und 
dann kommt eigentlich das erste Datenbit mit Pulse = 15... wo kommt da 
die 13 zwischendrin her? Hat jemand ne Idee?

von Mike (Gast)


Angehängte Dateien:

Lesenswert?

Ok, mit einer kleinen Änderung funktioniert es jetzt. Ich mache jetzt in 
der ISR eine Unterscheidung, ob Signaländerung An>Aus oder Aus>An 
vorliegt.
@ Karl-Heinz: Mit der Abfrage von PINB geht das dann auch :-)
Keine Ahnung, warum das einen Unterschied macht und dadurch die ominöse 
oben beschriebene "13" nicht mehr auftritt, aber sei's drum.

Wie könnte es anders sein, es gibt ein neues Problem... der beigefügte 
Code funktioniert bei genau einem Durchlauf, danach nicht mehr. 
Programmlogik ist wie folgt:

- Ports, Interrupts und Variablen initialisieren
- Endlosschleife
--> Modus 0 = Empfang und Aufzeichnung IR-Sequenz (über Pin Change und 
Timer Interrupt)
--> Modus 1 = Anzeige der empfangenen Sequenz auf LED Display, danach 
wieder Ports, Interrupts und Variablen initialisieren (Ports und 
Interrupts nur auf Verdacht, m. E. eigentlich nicht notwendig)

Beim ersten Durchlauf funktioniert das wunderbar, es werden genau die 
Signallängen angezeigt, die ich sende und erwarte. Sende ich das Signal 
erneut, passiert nix mehr (kommt zumindest nicht mehr von Modus 0 in 
Modus 1). Durch die Initialisierungen am Ende von Modus 1 würde ich 
erwarten, dass das Programm genau im gleichen Zustand ist wie beim 
ersten Durchlauf. Wenn ich auf den Reset-Button klicke, geht's dann 
wieder - strange

PS: Anbei noch ein Foto meiner Versuchsanordnung, damit Ihr Euch ein 
besseres Bild machen könnt, was ich hier überhaupt treibe.

von Karl H. (kbuchegg)


Lesenswert?

Wie formulier ichs diplomatisch?

Dein ganzer Aufbau mit der Anzeige, den 100-tausend delays .... das ist 
alles mies.

7-Segment Anzeige macht man komplett anders. Dazu wird ein Timer 
abgestellt, der im Hintergrund laufend die Anzeige Stelle für Stelle 
multiplext.



Abgesehen davon.
Ich finde deinen Code sehr unübersichtlich. Für mich ist es schwer mich 
darin zurechtzufinden. Aber ich geh mal suchen, was mir sonst noch so 
auffällt.

von Karl H. (kbuchegg)


Lesenswert?

Am Anfang von 'Modus 1' hast du einen cli(). Aber du hebst den nie 
wieder auf. Da fehlt ein sei().

von Karl H. (kbuchegg)


Lesenswert?

Und jetzt schreibst du 100 mal:
1
In C wird bei 0 angefangen zu zählen und das hat Konsequenzen!



1
  uint8_t counts_pro_phase[70];                      
2
3
  for (i=1;i<=70;i++)
4
  {
5
    displayZahl(counts_pro_phase[i],dauer);

No.
In C wird bei 0 angefangen zu zählen! Ein Array, das du mit einer Länge 
von 4 dimensionierst

  int werte[4];

besteht auch aus 4 Werten, die die Indizes

  werte[0], werte[1], werte[2], werte[3]

tragen. Zähl nach: es sind 4 Stück - genau so viele wie angefordert 
wurden.
D.h. der höchste Index in einem Array ist IMMER um 1 kleiner als diese 
Dimensionierungsangabe!

Daher sieht eine for-Schleife, mit der man alle Indizes eines Arrays 
durchgeht, IMMER so aus

   for( i = 0; i < DIMENSIONIERUNGSANGABE; i++ )

in diesem Fall dann eben

    for( i = 0; i < 4; i++ )

oder wenn man ein wenig mehr Erfahrung hat

    for( i = 0; i < sizeof( Array ) / sizeof( *Array ); i++ )

(vergiss die letzte Form momentan wieder)

Die entscheidenden Punkte sind

  die Laufvariable wird mit 0 initialisiert
  der Vergleich lautet auf 'kleiner'
  der Vergleich wird gegen die definierte Länge des Arrays durchgeführt
  die Laufvariable wird um 1 erhöht

Sobald eine Schleife, die offensichtlich nicht nach diesem Schema gebaut 
ist, wie zb

  for (i=1;i<=70;i++)

auftaucht, dann müssen bei dir sofort die Alarmglocken schrillen und du 
musst dich fragen, ob das so stimmen kann, bzw. ob es einen speziellen 
Grund gibt, warum eine derartige Schleife nicht nach dem Standard-Muster 
gebaut wurde.
Gestandene C-Programmierer denken über
  for( i = 0; i < NR_ARRAY_ENTRIES; i++ )
    array[i] ....
gar nicht länger nach. Das ist ein optisches Muster, das einem 
C-Programmierer auf einen Blick erzählt, was hier passiert. Die for ist 
nach Schema 'F' aufgebaut und das i wird in der Schleife als Arrayindex 
benutzt. Die einzige Frage lautet 'Stimmt NR_ARRAY_ENTRIES mit der 
definierten Größe des Arrays überein' und wenn das mit 'ja' beantwortet 
wird, dann ist die Analyse gegessen, denn dann ist alles klar: In dieser 
Schleife wird irgendetwas mit jedem einzelnen Element des Arrays 
gemacht.


Also:
Wir fangen bei 0 zu zählen an!
Und das ist auch gut so! Denn es vereinfacht viele Dinge.
Das hat aber auch Konsequenzen.


Und eine davon lautet: Du brauchst ein C Buch für die allgemeine 
Einführung in C.

von Mike (Gast)


Lesenswert?

Karl Heinz Buchegger schrieb:
> Dein ganzer Aufbau mit der Anzeige, den 100-tausend delays .... das ist
>
> alles mies.
>
>
>
> 7-Segment Anzeige macht man komplett anders. Dazu wird ein Timer
>
> abgestellt, der im Hintergrund laufend die Anzeige Stelle für Stelle
>
> multiplext.

Ja, war mein erster Versuch für das Multiplexing der LED-Anzeige - werde 
das überarbeiten, wenn der IR-Empfang funktioniert - für den Moment ist 
nicht schön (oder sogar mies ;-), aber es tut

Karl Heinz Buchegger schrieb:
> Ich finde deinen Code sehr unübersichtlich. Für mich ist es schwer mich
>
> darin zurechtzufinden.

Warum? Abgesehen von der etwas eigenwilligen LED-Implementierung ist es 
doch nicht so schlecht strukturiert... hast Du einen konkreten Tip zur 
Verbesserung? will ja lernen... die Mischung aus "State Machine" 
(Rausfiltern Startbit) und Aufzeichnung Phasenlängen ist nur ein 
Zwischenstand, am Schluss möchte ich nur noch die 4 Datenbytes 
aufzeichnen, vielleicht wird dann die Struktur auch besser.

Karl Heinz Buchegger schrieb:
> Am Anfang von 'Modus 1' hast du einen cli(). Aber du hebst den nie
>
> wieder auf. Da fehlt ein sei().

Das ist in der Routine "initPortsInterrupts" am Schluss - ich hatte es 
"sicherheitshalber" ein zweites Mal ins Hauptprogramm gemacht, aber 
keine Änderung. Das sollte nicht das Problem sein?

Noch ein Hinweis: Ich hatte am Anfang von Modus 0 zum Test ein 
LED-Blinken eingebaut. Dieses wird auch beim zweiten Durchlauf (nach 
Wechsel Modus 1 auf Modus 0) durchgeführt. D. h. dass danach aber der 
Empfang und Auswertung des Signals nicht mehr so läuft wie beim ersten 
Durchlauf.

Hast Du noch eine Idee?

von Mike (Gast)


Lesenswert?

Karl Heinz Buchegger schrieb:
> Daher sieht eine for-Schleife, mit der man alle Indizes eines Arrays
>
> durchgeht, IMMER so aus
>
>
>
>    for( i = 0; i < DIMENSIONIERUNGSANGABE; i++ )

mann, noch ein dummer Fehler. Vielen Dank für die ausführliche 
Erläuterung!

Ich habe das geändert, aber leider auch ohne Erfolg - gleiches 
Programmverhalten - hast Du noch eine Idee? Sorry, aber ich suche und 
suche und finde es einfach nicht...

Karl Heinz Buchegger schrieb:
> Die entscheidenden Punkte sind
>
>
>
>   die Laufvariable wird mit 0 initialisiert
>
>   der Vergleich lautet auf 'kleiner'
>
>   der Vergleich wird gegen die definierte Länge des Arrays durchgeführt
>
>   die Laufvariable wird um 1 erhöht

Aber ich könnte auch auf eine Zahl kleiner der definierten Länge des 
Arrays gemacht werden, oder? Wenn ich z. B. nur 10 von 20 definierten 
Elementen loopen möchte - oder macht das auch Probleme? Dann versteh 
ich's nicht mehr ;-)

Karl Heinz Buchegger schrieb:
> Du brauchst ein C Buch für die allgemeine
>
> Einführung in C.

Werde ich mir zulegen (und lesen!), versprochen.

Karl Heinz Buchegger schrieb:
> Und jetzt schreibst du 100 mal:In C wird bei 0 angefangen zu zählen und das hat 
Konsequenzen!

In C wird bei 0 angefangen zu zählen und das hat Konsequenzen!
In C wird bei 0 angefangen zu zählen und das hat Konsequenzen!
In C wird bei 0 angefangen zu zählen und das hat Konsequenzen!
In C wird bei 0 angefangen zu zählen und das hat Konsequenzen!
In C wird bei 0 angefangen zu zählen und das hat Konsequenzen!
In C wird bei 0 angefangen zu zählen und das hat Konsequenzen!
In C wird bei 0 angefangen zu zählen und das hat Konsequenzen!
In C wird bei 0 angefangen zu zählen und das hat Konsequenzen!
In C wird bei 0 angefangen zu zählen und das hat Konsequenzen!
In C wird bei 0 angefangen zu zählen und das hat Konsequenzen!

... die restlichen 90x schreib ich von Hand, sonst wär's ja keine Strafe 
;-)

von Lötlackl *. (pappnase) Benutzerseite


Lesenswert?

@Mike
falscher Ansatz!

Ich hätte es eher so gemacht. ;-)
1
for(uint8_t i = 0; i < 100; i++)
2
{
3
  printf("In C wird bei 0 angefangen zu zählen und das hat Konsequenzen!\r\n");
4
}

von Stefan (Gast)


Lesenswert?

Nein, wenn schon, dann richtig:
1
for(uint8_t i = 1; i <= 100; i++)
2
{
3
  printf("In C wird bei 0 angefangen zu zählen und das hat Konsequenzen!\r\n");
4
}

;-)

von Patrick D. (oldbug) Benutzerseite


Lesenswert?

Nu verwirr den armen Kerl doch nicht wieder!
Karl-Heinz hat's grad noch so hingebogen!

von Mike (Gast)


Lesenswert?

spottet nur ;-))

von Frank M. (ukw) (Moderator) Benutzerseite


Lesenswert?

Mike schrieb:
> ich habe im Compiler Optimization Level (-O1) aktiviert, damit mein
> Programm auf den ATtiny2313 passt.

Benutze besser -Os.

von Karl H. (kbuchegg)


Lesenswert?

** Lötlackl schrieb:
> @Mike
> falscher Ansatz!
>
> Ich hätte es eher so gemacht. ;-)
>
1
> for(uint8_t i = 0; i < 100; i++)
2
> {
3
>   printf("In C wird bei 0 angefangen zu zählen und das hat
4
> Konsequenzen!\r\n");
5
> }


#Das# ist eines C-Programmierers würdig und das lass ich auch gelten :-)

von Karl H. (kbuchegg)


Angehängte Dateien:

Lesenswert?

Ich hol mir den Code mal als C-File ins Attachment, damit man auch was 
sieht :-)

von Karl H. (kbuchegg)


Lesenswert?

OK.
Was mir aufgefallen ist
1
...
2
      // Initialisierung
3
      initPortsInterrupts();
4
5
      modus=0;
6
      signal_changed=0;
7
      interrupt_count=0;
8
      zaehler_phasen=0;
9
      startbit_ok=0;
10
...
Ich denke es wäre sinnvoll, ZUERST die Variablen auf 0 zu setzen und 
erst DANN die Interrupts freizugeben.


Ähm
1
  if (PINB &= (1<<PB0)){
was'n das?   &=


Dein Code ist schauderhaft. Alleine dein { } Schema - da krieg ich die 
Krise. Das ist alles so .... irgendwie alles in einer Wurst dahin, da 
ist auf den ersten Blick keine Struktur zu erkennen; logische Blöcke, 
die zusammengehören.

von Mike (Gast)


Lesenswert?

Karl Heinz Buchegger schrieb:
> Ich denke es wäre sinnvoll, ZUERST die Variablen auf 0 zu setzen und
>
> erst DANN die Interrupts freizugeben.

ok, gemacht

Karl Heinz Buchegger schrieb:
> Ähm  if (PINB &= (1<<PB0)){
>
> was'n das?   &=

Hey, wer hat denn da das "=" eingeschmuggelt, Frechheit ;-) habe es in 
(PINB & (1<<PB0)) geändert.

Jetzt die schlechte Nachricht: Löst immer noch nicht das Problem 
verzweifel

Karl Heinz Buchegger schrieb:
> Dein Code ist schauderhaft. Alleine dein { } Schema - da krieg ich die
>
> Krise.

Meinst Du jetzt optisch oder logisch? Vermutlich beides ;-) im Sinne der 
Motivation sag doch bitte statt "schauderhaft" vielleicht 
"optimierungsbedürftig" oder "ausbaufähig" oder "im Rahmen der 
Möglichkeiten sein Bestes gegeben" - habe doch auch Gefühle ;-)

von Karl H. (kbuchegg)


Lesenswert?

Da dein Code darauf angewiesen ist, dass der Timer erst dann gestartet 
wird, wenn die erste Flanke am Pin-Change Pin kommt: Wo stoppst du den 
Timer eigentlich wieder?

Denn sonst hast du das Problem, dass beim Moduswechsel von 1 auf 0 der 
Timer schon fleissig interrupt_counts zählt.


Persönlich bin ich der Ansicht, dass dieses Timer_starten / 
Timer_stoppen sowieso Murks ist, aber seis drumm.
Viel einfacher ist es den Timer einfach durchlaufen zu lassen und mit 
einer globalen Variable der ISR mitzuteilen, ob sie momentan zählen soll 
oder nicht.

von Mike (Gast)


Lesenswert?

Karl Heinz Buchegger schrieb:
> Da dein Code darauf angewiesen ist, dass der Timer erst dann gestartet
>
> wird, wenn die erste Flanke am Pin-Change Pin kommt: Wo stoppst du den
>
> Timer eigentlich wieder?
>
>
>
> Denn sonst hast du das Problem, dass beim Moduswechsel von 1 auf 0 der
>
> Timer schon fleissig interrupt_counts zählt.

Bingo!! :-)) Danke, danke, danke - das hatte ich übersehen

Karl Heinz Buchegger schrieb:
> Persönlich bin ich der Ansicht, dass dieses Timer_starten /
>
> Timer_stoppen sowieso Murks ist, aber seis drumm.
>
> Viel einfacher ist es den Timer einfach durchlaufen zu lassen und mit
>
> einer globalen Variable der ISR mitzuteilen, ob sie momentan zählen soll
>
> oder nicht.

Hab's dann gleich so gemacht, und es funktioniert.

So, da es jetzt läuft, gehe ich jetzt ans Aufräumen und Aufhübschen des 
Codes - dann lad ich ihn nochmal zum "Review" hoch, Du wirst mit den 
Ohren schlackern und Tränen der Freude in den Augen haben ;-)

von Patrick D. (oldbug) Benutzerseite


Lesenswert?

Zum hübschmachen gibt's tools ;)

von Mike (Gast)


Lesenswert?

Patrick Dohmen schrieb:
> Zum hübschmachen gibt's tools ;)

Jetzt wollt ich Karl-Heinz echte Handarbeit liefern, aber wenn's so was 
von der Stange auch tut ;-)

Aber im Ernst: ich benutze Atmel Studio und habe dort mal auf Edit > 
Format Document geklickt - aber sooo viel anders/besser sieht's dann 
nicht aus - welches Tool empfiehlst Du? ...und abgesehen vom { } Schema, 
ist es echt so schlimm?

von Mike (Gast)


Lesenswert?

PS: Habe gerade gesehen, dass Karl Heinz sich ohne Bindestrich schreibt, 
sorry - hab's eben nicht so mit der Formatierung ;-)

von (prx) A. K. (prx)


Lesenswert?

Du sollst den Code hübsch machen, nicht den Karl Heinz. :-)

von Lötlackl *. (pappnase) Benutzerseite


Lesenswert?

Mike schrieb:
> ...und abgesehen vom { } Schema

welches so grausig anzusehen ist:
1
  if (PINB & (1<<PB0)){
2
    signal_changed=1; }    // Eingang ist High, d. h. kein Empfang am TSOP --> Wechsel An>Aus --> Ende/Dauer Pulse-Phase
3
  else {
4
    signal_changed=2; }    // Eingang ist Low, d. h. Empfang am TSOP --> Wechsel Aus>An --> Ende/Dauer Pause-Phase

wenn schon „One True Brace Style“, dann so:
1
  if (PINB & (1<<PB0)) {
2
    signal_changed=1;     // Eingang ist High, d. h. kein Empfang am TSOP --> Wechsel An>Aus --> Ende/Dauer Pulse-Phase
3
  } else {
4
    signal_changed=2;     // Eingang ist Low, d. h. Empfang am TSOP --> Wechsel Aus>An --> Ende/Dauer Pause-Phase
5
  }
oder aber im K&R-Style.
1
  if (PINB & (1<<PB0))
2
  {
3
    signal_changed=1;     // Eingang ist High, d. h. kein Empfang am TSOP --> Wechsel An>Aus --> Ende/Dauer Pulse-Phase
4
  }
5
  else
6
  {
7
    signal_changed=2;     // Eingang ist Low, d. h. Empfang am TSOP --> Wechsel Aus>An --> Ende/Dauer Pause-Phase
8
  }

von Fabian O. (xfr)


Lesenswert?

Mike schrieb:
> Aber im Ernst: ich benutze Atmel Studio und habe dort mal auf Edit >
> Format Document geklickt - aber sooo viel anders/besser sieht's dann
> nicht aus - welches Tool empfiehlst Du? ...und abgesehen vom { } Schema,
> ist es echt so schlimm?

Als ersten Schritt könntest Du es mal hier einfügen:
http://prettydiff.com/

Sieht dann schon ganz anders aus, braucht aber noch etwas händische 
Nacharbeit. Immerhin sind dann die Klammern und Leerzeichen schon mal 
ordentlich gesetzt.

von troll (Gast)


Lesenswert?

A. K. schrieb:
> Du sollst den Code hübsch machen, nicht den Karl Heinz. :-)

Bei dem ist eh nichts mehr zu retten...

duckundweg

SCNR

von Frank M. (ukw) (Moderator) Benutzerseite


Lesenswert?

Fabian O. schrieb:
> Als ersten Schritt könntest Du es mal hier einfügen:
> http://prettydiff.com/

Das Ding ist ziemlich kaputt. Aus
1
  if (PINB & (1<<PB0)){
2
    signal_changed=1; }    // Eingang ist High, d. h. kein Empfang am TSOP --> Wechsel An>Aus --> Ende/Dauer Pulse-Phase
3
  else {
4
    signal_changed=2; }    // Eingang ist Low, d. h. Empfang am TSOP --> Wechsel Aus>An --> Ende/Dauer Pause-Phase

macht das Teil:
1
if (PINB & (1 << PB0)) {
2
    signal_changed = 1;
3
} // Eingang ist High, d. h. kein Empfang am TSOP --> Wechsel An>Aus --> Ende/Dauer Pulse-Phase else {
4
    signal_changed = 2;
5
} // Eingang ist Low, d. h. Empfang am TSOP --> Wechsel Aus>An --> Ende/Dauer Pause-Phase

Das heisst: das Schlüsselwort "else" landet im Kommentar! Das kann fatal 
sein, wenn man das nicht merkt! Glücklicherweise hat das Ding auch noch 
die geschweifte Klammer in den Kommentar gezogen, so dass der Compiler 
wenigstens einen Syntax-Error ausgibt.

> Sieht dann schon ganz anders aus, braucht aber noch etwas händische
> Nacharbeit. Immerhin sind dann die Klammern und Leerzeichen schon mal
> ordentlich gesetzt.

Die Klammern sind eben nicht ordentlich gesetzt, im Kommentar haben 
sie wohl eher nichts zu suchen ;-)

Fazit: Das Ding verfälscht die Programmlogik und erzeugt auch noch 
nicht-compilierbaren Code.

EDIT:

Wenn man den Style "Allman (ANSI) Style — Sets opening curly braces on a 
new line." einschaltet, kommt sogar compilierbarer Code raus, aber das 
else ist immer noch im Kommentar. Folge: Das Programm macht nicht mehr 
das, was es ursprünglich sollte:

1
if (PINB & (1 << PB0))
2
{
3
    signal_changed = 1;
4
} // Eingang ist High, d. h. kein Empfang am TSOP --> Wechsel An>Aus --> Ende/Dauer Pulse-Phase else
5
{
6
    signal_changed = 2;
7
} // Eingang ist Low, d. h. Empfang am TSOP --> Wechsel Aus>An --> Ende/Dauer Pause-Phase

Aua.

von Fabian O. (xfr)


Lesenswert?

Stimmt schon, es macht doch einige Fehler. Die Defines am Anfang verhaut 
es auch, außerdem kennt es die bitweise Schreibweise von Literalen wie 
0b00000101 nicht.

Naja, dann also neue Empfehlung: Einmal reinkopieren, anschauen wie es 
prinzipiell aussehen soll und dann von Hand umbauen ... ;-)

von Mike (Gast)


Lesenswert?

Frank M. schrieb:
> Das Programm macht nicht mehr das, was es ursprünglich sollte:

... zumindest das bekomm ich auch von Hand hin ;-)

von Fabian O. (xfr)


Lesenswert?

Die Defines mit den LED-Segmenten würde ich mir in der Form übrigens 
sparen und gleich in die Funktion packen:
1
void aktiviereSegmente(uint8_t ziffer)
2
{
3
  switch (ziffer) {
4
    case 0:
5
      PORTD = (1<<PD2) | (1<<PD3) | (1<<PD4) | (1<<PD5);
6
      PORTA = (1<<PA0) | (1<<PA1);
7
      break;
8
    case 1:
9
      PORTD = (1<<PD3) | (1<<PD4);
10
      PORTA = 0;
11
      break;
12
    case 2:
13
      PORTD = (1<<PD5) | (1<<PD4) | (1<<PD2) | (1<<PD1);
14
      PORTA = (1<<PA0);
15
      break;
16
    case 3:
17
      PORTD = (1<<PD5) | (1<<PD4) | (1<<PD3) | (1<<PD2) | (1<<PD1);
18
      PORTA = 0;
19
      break;
20
    case 4:
21
      PORTD = (1<<PD4) | (1<<PD3) | (1<<PD1);
22
      PORTA = (1<<PA1);
23
      break;
24
    case 5:
25
      PORTD = (1<<PD5) | (1<<PD3) | (1<<PD2) | (1<<PD1);
26
      PORTA = (1<<PA1);
27
      break;
28
    case 6:
29
      PORTD = (1<<PD5) | (1<<PD3) | (1<<PD2) | (1<<PD1);
30
      PORTA = (1<<PA1) | (1<<PA0);
31
      break;
32
    case 7:
33
      PORTD = (1<<PD3) | (1<<PD4) | (1<<PD5);
34
      PORTA = 0;
35
      break;
36
    case 8:
37
      PORTD = (1<<PD1) | (1<<PD2) | (1<<PD3) | (1<<PD4) | (1<<PD5);
38
      PORTA = (1<<PA0) | (1<<PA1);
39
      break;
40
    case 9:
41
      PORTD = (1<<PD1) | (1<<PD2) | (1<<PD3) | (1<<PD4) | (1<<PD5);
42
      PORTA = (1<<PA1);
43
      break;
44
    case 10:
45
      PORTD = 0;
46
      PORTA = 0;
47
      break;
48
  }
49
}

Wenn mans kompakter haben möchte, kann man die Werte auch in einer 
Tabelle im Programmspeicher ablegen. Aber die Defines mit den ganzen 
Anweisungen, die man nur genau einmal in genau der Reihenfolge 
verwendet, bringen keinen Mehrwert.

von Mike (Gast)


Lesenswert?

Fabian O. schrieb:
> Wenn mans kompakter haben möchte, kann man die Werte auch in einer
>
> Tabelle im Programmspeicher ablegen.

Gute Idee. Wäre das der richtige Ansatz (und die richtige Syntax?):

const uint8_t segmente[] =
{
PORTASegmente, PORTDSegmente,   // für Ziffer 0
PORTASegmente, PORTDSegmente,   // für Ziffer 1
PORTASegmente, PORTDSegmente,   // für Ziffer 2
...};

und Zugriff dann über
PORTA = segmente[ziffer*2]
PORTD = segmente[ziffer*2+1]

ich habe noch nie mit Tabellen im Programmspeicher gearbeitet, daher die 
Frage

von Fabian O. (xfr)


Lesenswert?

Man könnte es z.B. so machen:
1
#include <avr/pgmspace.h>
2
3
#define SEG_O  (1<<PD5) // Segment oben
4
#define SEG_OR (1<<PD4) // Segment oben rechts
5
#define SEG_UR (1<<PD3) // Segment unten rechts
6
// ...
7
8
typedef struct {
9
  uint8_t portA;
10
  uint8_t portD;
11
} segmente_t;
12
13
static const PROGMEM segmente_t segmente_array[] = {
14
  // ...                        // Ziffer 0
15
  {0, SEG_OR | SEG_UR},         // Ziffer 1
16
  // ...
17
  {0, SEG_O | SEG_OR | SEG_UR}, // Ziffer 7
18
  // ...
19
  {0, 0},                       // aus
20
}
21
22
void aktiviereSegmente(uint8_t ziffer)
23
{
24
  segmente_t segmente;
25
  memcpy_P(&segmente, segmente_array + ziffer, sizeof(segmente_t));
26
  PORTA = segmente.portA;
27
  PORTD = segmente.portD;
28
}

Beim Befüllen des Arrays musst Du halt aufpassen, welches Segment zu 
welchem Port gehört. Da gäbs sicher auch noch Tricks für, aber das würde 
zu weit führen. Wenn Dir das mit den Defines zu umständlich ist, kannst 
Du natürlich in das Array auch direkt die Werte in binärer oder 
hexadezimaler Schreibweise eintragen.

von Mike (Gast)


Lesenswert?

Vielen Dank, werde ich so einbauen!

Fabian O. schrieb:
> memcpy_P(&segmente, segmente_array + ziffer, sizeof(segmente_t));

Diese Syntax muss ich mir nochmal genauer anschauen, um sie zu 
verstehen...
Die <avr/pgmspace.h> Doku ist da vermutlich die richtige Quelle.

Karl Heinz Buchegger schrieb:
> 7-Segment Anzeige macht man komplett anders. Dazu wird ein Timer
>
> abgestellt, der im Hintergrund laufend die Anzeige Stelle für Stelle
>
> multiplext.

Und das stell ich auch um, dann sollte die LED-Anzeige "sauber" sein :-)

von Fabian O. (xfr)


Lesenswert?

Mike schrieb:
>> memcpy_P(&segmente, segmente_array + ziffer, sizeof(segmente_t));
>
> Diese Syntax muss ich mir nochmal genauer anschauen, um sie zu
> verstehen...

Die Funktion macht das Gleiche wie das normale memcpy: Sie kopiert eine 
Anzahl an Bytes von einer Speicheradresse zur anderen. memcpy_P kopiert 
nur nicht vom RAM ins RAM sondern vom Programmspeicher (Flash) ins RAM.

Der erste Parameter sagt, wohin kopiert werden soll: An die Adresse, an 
der die Variable segmente im RAM steht:
1
&segmente

Der zweite Parameter sagt, woher die Daten kommen sollen: Von der 
Adresse, an der die Segmente im Programmspeicher stehen:
1
segmente_array + ziffer
Die Segmente für Ziffer 0 stehen dort, wo das Array beginnt. Die für 
Ziffer 1 an der nächsten Stelle usw.. C rechnet die Adresse entsprechend 
der Größe eines Arrayelements selber richtig aus (Zeigerarithmetik). 
Vielleicht ist es so klarer:
1
&(segmente_array[ziffer])

Der dritte Parameter sagt, wie viele Bytes kopiert werden. So viele, wie 
die Struktur groß ist:
1
sizeof(segmente_t)
Das wären in diesem Fall zwei Bytes.

von Patrick D. (oldbug) Benutzerseite


Lesenswert?

Hups, hab den Thread aus den Augen verloren!

Ich dachte aber eher an sowas wie indent, findet sich sicherlich in 
den tools, die dem avr-gcc beiliegen.

von Da D. (dieter)


Lesenswert?

Stefan schrieb:
> for(uint8_t i = 1; i <= 100; i++)
> {
>   printf("In C wird bei 0 angefangen zu zählen und das hat Konsequenzen!\r\n");
> }

Die Ironie in diesem Codestückchen gefällt mir :-)

von Mike (Gast)


Lesenswert?

Mir auch :-) noch besser fand ich:

A. K. schrieb:
> Du sollst den Code hübsch machen, nicht den Karl Heinz. :-)

@ Fabian: Danke für die tolle Erklärung - werde das am Wochenende 
einbauen.

VG Mike

von Mike (Gast)


Angehängte Dateien:

Lesenswert?

So, habe nun Eure Empfehlungen und Verbesserungsvorschläge umgesetzt 
(ich hoffe richtig - auf jeden Fall läuft's wie gewünscht):

- LED Mulitplexing über Timer
- Speicherung LED-Segmente in Programmspeicher
- State Machine zur Dekodierung des IR Signals
- Code-Formatierung zur Schonung von Karl Heinz' Augen ;-)

Anbei das Ergebnis - besser bekomm ich's mit dem aktuellen 
Erfahrungsschatz nicht hin...

Nochmal danke an Euren Support!

von Mike (Gast)


Lesenswert?

Jetzt gibt es doch noch ein Problemchen:

Mit diesen Parametern läuft das Programm einwandfrei und "stabil":

#define startbit_pulse (3000/dauer_count)
#define startbit_pause (1500/dauer_count)
...
#define stopbit_pause (3000/dauer_count)

Ändere ich die Parameter auf die "richtigen NEC-Werte", so läuft das 
Programm nur noch sporadisch (manchmal wird Ergebnis angezeigt, d. h. 
State Machine duchlaufen - manchmal nicht):

#define startbit_pulse (9000/dauer_count)
#define startbit_pause (4500/dauer_count)
...
#define stopbit_pause (4500/dauer_count)

Und das, obwohl die gemessene Anzahl der Interrupt-Counts ziemlich genau 
dem erwarteten Ergebnis entspricht (z. B. bei Startbit Pulse 226 Counts 
gemessen, 225 rechnerisch). Gibt es dafür eine logische Erklärung?? Sehr 
merkwürdig....

PS: Ist das eigentlich normal, dass sobald ein Problem gelöst ist, ein 
neues Auftritt? :-(

Bitte melde dich an um einen Beitrag zu schreiben. Anmeldung ist kostenlos und dauert nur eine Minute.
Bestehender Account
Schon ein Account bei Google/GoogleMail? Keine Anmeldung erforderlich!
Mit Google-Account einloggen
Noch kein Account? Hier anmelden.