Problem mit if Abfrage in C

[Jörg S. (mitchell)]

Hallo,
ich bin gerade dabei mich mit C-Programmierung zu befassen.
Dazu habe ich mir ein Test-Programm geschrieben.
Teste zur Zeit mit einem Tiny2313, 3 LEDs an PB0-PB2 und 3 Schalter an 
PD0-PD2.
LED sind gegen  +5V geschaltet, Die Pullup für die Schalter sind 
aktiviert.
Wird einer der Schalter betätigt ( schaltet gegen GND) soll die 
entsprechende LED an gehen.
Schalter 1 und 2 sollen statische Signale und Schalter 3 soll ein 1Hz 
Signal simulieren.
SW1 und SW2 funktioniert die Abfrage, die LEDs werden entsprechend 
angeschaltet.

Bei SW3 soll die LED angehen, dann soll in einer Schleife getestet 
werden, ob der SW3 noch betätigt ist, da es pulsiert.
Wenn ca. 1s lang kein Signal kommt wird der Block verlassen.

Bei der Abfrage von SW3 komme ich nur bis zu der Anweisung "PORTB = 
0b11111010 ;  // LED1 und LED3 an".
Das Z = 0 und alles danach wird einfach ignoriert.
Warum?
Es steht doch in dem Anweisungsblock mit { und }.
Was habe ich falsch gemacht oder übersehen?

Gruß Jörg

[Peter II (Gast)]

Jörg S. schrieb:
> Was habe ich falsch gemacht oder übersehen?

es wird ignoriert weil es keine Bedeutung hat. Es ist weg optimiert 
wurden.

[Frank B. (f-baer)]

Z als volatile deklarieren, dann sollte es gehen.
Der Compiler optimiert die for-Schleife weg.

[Lothar M. (Firma: Titel) (lkmiller) (Moderator)]

Jörg S. schrieb:
> Dazu habe ich mir ein Test-Programm geschrieben
Meine C-Programme enden auf *.c
Warum endet deines auf *.txt?

Jörg S. schrieb:
> Was habe ich falsch gemacht oder übersehen?
Hier aändert sich der Wert von x nicht mehr, weil der Port nicht mehr 
eingelesen wird:
        if (( x & 0x07 ) == 3) { // ist SW3 noch an?
Und weil x vorher == 2 war, wird die Bedingung nie erfüllt und 
wegoptimiert.

[Ingo L. (corrtexx)]

  int x = 0;
  int z = 0;

Du benötigst hier keine "int" (16-Bit), eine 8Bit Variable tut es auch.

[Ralf G. (ralg)]

zusätzlich (reine Geschmackssache):
Mir gefallen solche 
'wenn-dies-dann-das-wenn-anders-dann-so-wenn-ja-warum-nicht-Vergleiche' 
überhaupt nicht.

switch (PORTD & 0x07)
{
    case 6:
        //  ...
        break;
    case 4:
        //  ...
        break;
    case 3:
        //  ...
        break;
    case 2:
        //  ...
        break;
// eventuell:
    default:
        //  ...
        break;
}

... find' ich übersichtlicher.

[Jörg S. (mitchell)]

Hallo,
danke für die schnelle Rückmeldung.

Peter II schrieb:
> Jörg S. schrieb:
>> Was habe ich falsch gemacht oder übersehen?
>
> es wird ignoriert weil es keine Bedeutung hat. Es ist weg optimiert
> wurden.

warum?
Bin gerade erst beim lernen und verstehe es nicht.


Frank B. schrieb:
> Z als volatile deklarieren, dann sollte es gehen.
> Der Compiler optimiert die for-Schleife weg.

Wo, vor dem Z in der Abfrage?


Lothar M. schrieb:
> Jörg S. schrieb:
>> Dazu habe ich mir ein Test-Programm geschrieben
> Meine C-Programme enden auf *.c
> Warum endet deines auf *.txt?
>
> Jörg S. schrieb:
>> Was habe ich falsch gemacht oder übersehen?
> Hier aändert sich der Wert von x nicht mehr, weil der Port nicht mehr
> eingelesen wird:
>         if (( x & 0x07 ) == 3) { // ist SW3 noch an?
> Und weil x vorher == 2 war, wird die Bedingung nie erfüllt und
> wegoptimiert.

1. ich habe es in eine .txt kopiert, das nächste mal nehme ich gleich 
die .c

2. bis dahin komme ich nicht, aber ich werde das ändern auf 2 und 
testen.

Gruß Jörg

[Peter II (Gast)]

Jörg S. schrieb:
> Bin gerade erst beim lernen und verstehe es nicht.

welche Bedeutung (bzw Auswirkung) soll denn der code nach deiner Meinung 
haben?

[Karl H. (kbuchegg)]

Jörg S. schrieb:

> warum?
> Bin gerade erst beim lernen und verstehe es nicht.

weil, wenn

    x & 0x07

wie weiter vorne gefordert den Wert 2 haben muss, damit dieser Teilzweig 
überhaupt betreten wird, dann kann derselbe Ausdruck x & 0x07 nicht den 
Wert 3 haben, wenn zwischendurch x keine Möglichkeit hatte seinen Wert 
zu verändern. Von da ausgehend, kann man aber das Pferd jetzt von hinten 
aufzäumen. Wenn der Ausdruck nicht 3 ergeben kann, dann kann er das auch 
nicht, wenn man das in einer Schleife immer wieder tut. Die Schleife 
macht also effektiv nichts. Eine Schleife die nichts tut, braucht 
keiner. Gibt es keine Schleife mehr, dann gibt es auch keinen Grund für 
eine Variable namens z, die sonst nirgends mehr vorkommt.

d.h. ausgehend von diesem Code

    else if (( x & 0x07 ) == 2)  {      // ist SW1 und SW3 close?
      PORTB = 0b11111010 ;        // LED1 und LED3 an
      z = 0 ;            // Zähler zurück setzen
        for (; z<=4; z++) {
        //_delay_ms(200);      // warte 200ms
          if (( x & 0x07 ) == 3) {  // ist SW3 noch an?
            z = 0;      // ja, Zähler zurück setzen
          }
        }
      }

und der simplen Beobachtung, dass

           if (( x & 0x07 ) == 3)

niemals wahr sein kann, ist bis auf

      PORTB = 0b11111010 ;        //

alles toter Code, der nichts bewirkt ausser Rechenzeit zu verbrauchen.
Also hat ihn der Compiler rausgeworfen.

[Sebastian V. (sebi_s)]

Frank B. schrieb:
> Z als volatile deklarieren, dann sollte es gehen.
> Der Compiler optimiert die for-Schleife weg.

Volatile bringt hier gar nichts. Man sollte einfach PIND anstelle von x 
testen. Oder man ließt vor jedem Test in der for-Schleife nochmal den 
aktuellen Wert von PIND und schreibt ihn in die x Variable.

[Yalu X. (yalu) (Moderator)]

Jörg S. schrieb:
> Lothar M. schrieb:
>> Hier aändert sich der Wert von x nicht mehr, weil der Port nicht mehr
>> eingelesen wird:
>>         if (( x & 0x07 ) == 3) { // ist SW3 noch an?
>> Und weil x vorher == 2 war, wird die Bedingung nie erfüllt und
>> wegoptimiert.
>
> ...
>
> 2. bis dahin komme ich nicht, aber ich werde das ändern auf 2 und
> testen.

Ich glaube, du möchtest eher das x durch PIND ersetzen, damit der Port
an dieser Stelle (d.h. in jedem Schleifendurchlauf) erneut gelesen wird
und so auf das Loslassen der Taste reagiert werden kann.

[Jörg S. (mitchell)]

Hallo Ralf,
das mit Case habe ich auch schon getestet.
Aber nur mit 3 statischen Signalen.
Das funktioniert, auch bei If mit drei statischen Signalen.
Aber ich mochte das bei dem 3. Signal ein pulsierendes abgefragt wird 
und erst wenn dieses nicht mehr anliegt, die Abfrage verlassen.

Da kommt die selbe Problematik wie beim If.
Aber auch das werde ich mit dem Case testen.

[Karl H. (kbuchegg)]

Du möchtest da jetzt keine Zahlenspielchen spielen, sondern du möchtest 
deinen COde ordentlich schreiben.

Ob ein Bit in einem Byte auf 0 ist oder nicht, testet man mit dem 
Vergleich

    !( Byte & ( 1 << Bitnummer ) )

dieser Ausdruck ergibt wahr, wenn das Bit auf 0 ist. Bei dir bedeutet 
das, dass die entsprechende Taste gedrückt ist.

Und genau so handhaben wir das dann auch im Programm.

Da ich mal davon ausgehe, dass es erst mal nicht besonders kritisch ist, 
dass die Bitabfragen tatsächlich gleichzeitig erfolgen (denn: so genau 
kannst du deine Tasten sowieso nicht händisch betätigen, dass wirklich 
beide Tasten in genau derselben Femtosekunde gleichzeitig schalten), 
schlage ich erst mal vor, du machst dir dafür ein Makro

#define KEY_PRESSED( Port, Bit )  ( !(( Port) & ( 1 << (Bit) )) )

und du machst dir auch ein paar Makros für deine Hardwarebezeichnungen

#define SWITCH_1     PD0
#define SWITCH_2     PD1
#define SWITCH_3     PD2

dann schreibt sich nämlich dieser Aspekt des COdes als:

  while(1)
  {
    if  (KEY_PRESSED( PIND, SWITCH_1 )) {        
      PORTB = 0b11111110 ;        // LED1 an
    }
    else if (KEY_PRESSED( PIND, SWITCH_1 ) &&
             KEY_PRESSED( PIND, SWITCH_2 ) ) {        
      PORTB = 0b11111100 ;        // LED1 und LED2 aon
    }
    else if (KEY_PRESSED( PIND, SWITCH_1 ) &&
             KEY_PRESSED( PIND, SWITCH_3 ) )  {      
      PORTB = 0b11111010 ;        // LED1 und LED3 an
      z = 0 ;            // Zähler zurück setzen
        for (; z<=4; z++) {
        //_delay_ms(200);      // warte 200ms
          if ( KEY_PRESSED( PIND, SWITCH_3 ) {
            z = 0;      // ja, Zähler zurück setzen
          }
        }
      }
    else {
      PORTB = 255;
    }
  }

nicht nur macht diese Modifikation einige Kommentare überflüssig, das 
Programm ist jetzt auch deutlich leichter zu lesen und zu verstehen. Und 
als Nebeneffekt verschwindet dann auch dein Logikfehler.

Und dasselbe machst du auch mit deinen LED
* erst mal ersetzt du die direkten Portzuweisungen durch die 
entsprechenden Bitoperationen. Und dann denkst du dir Namen für dein LED 
aus und machst dir Makros dafür. Wann immer du versucht bist, so etwas 
zu schreiben

      PORTB = 0b11111110 ;        // LED1 an

musst du dich sofort fragen, warum du da einen Kommentar brauchst. Wozu 
einen Kommentar schreiben, wenn du genausogut zb

     CLR_BIT( PORTB, LED1 );

schreiben kannst, oder noch besser

     LED_ON( LED1 );

was genau dasselbe aussagt, was bei dir im Kommentar steht. Alles was du 
dazu brauchst, ist nur ein wenig Makro-Magie, so dass sich durch die 
Makro.Textersetzungen dann wieder

     PORTB &= ~( 1 << PB0 );

ergibt, was genau das Bit 0 am Port B auf 0 setzt und sonst nichts 
anderes tut.

[Jörg S. (mitchell)]

Hallo Karl Heinz,
danke für die anschauliche Erklärung, das hilft mir mehr als nur 
Begriffe.

das werde ich gleich mal testen

Gruß Jörg

[Karl H. (kbuchegg)]

Jörg S. schrieb:

> das werde ich gleich mal testen

Ich hatte noch eine Klammerung vergessen.
So ist das Makro richtig

#define KEY_PRESSED( Port, Bit )  ( !((Port) & ( 1 << (Bit) )) )

[Jörg S. (mitchell)]

Hallo Karl Heinz,

ich habe jetzt einen Teil umgesetzt.

Die Tasten sind soweit klar.
aber bei den LEDs habe ich jetzt schon wieder ein Problem bzw. etwas 
nicht verstanden.

     LED_ON( LED1 );

dazu habe ich das definiert :
#define LED_ON( Bit )  ( (PORTB) &= ~(1 << (bit) ) )
#define LED1  PB0
#define LED2  PB1
#define LED3  PB2

und erhalte das:

Error  1  'bit' undeclared (first use in this function)

Gruß Jörg

[Yalu X. (yalu) (Moderator)]

Groß- Kleinschreibung beachten in C.

[Jörg S. (mitchell)]

Hallo,
genau, das passiert mir immer wieder, danke.
Jetzt werde ich das ganze mal testen.

Gruß Jörg

[Karl H. (kbuchegg)]

:-)
Und ich hoffe, du hast dir auch überlegt, dass eine leuchtende LED nicht 
von alleine ausgeht.

   LED_ON( LED1 );

ist kein vollwertiger Ersatz für

      PORTB = 0b11111110 ;        // LED1 an

selbst wenn der Kommentar dies suggeriert.
Denn die letzte Anweisung macht noch mehr als nur die LED1 
einzuschalten. Sie schaltet auch alle anderen ab bzw. setzt sogar die 
Portpins an denen gar nichts hängt auf 1. Davon steht aber nichts im 
Kommentar!

(Und das ist auch einer der Gründe, warum du möglichst ohne derartige 
Komentare auskommen sollst. Denn meistens sind die sowieso falsch bzw. 
erzählen nicht die ganze Wahrheit. Sich auf sie zu verlassen ist daher 
sowieso oft Harakiri. Man kommt daher ohne exakte Analyse des Source 
Codes sowieso nicht aus. Ein

      LED_ON( LED1 );
      LED_OFF( LED2 );
      LED_OFF( LED3 );

hingegen erzählt mir auch ohne Kommentar die ganze Wahrheit und ist 
dabei recht einfach zu lesen und zu analysieren. Vor allen Dingen wenn 
die LED dann nicht LED1, LED2 und LED3 heissen, sondern sprechende Namen 
tragen, wie zb ERROR_LED oder READY_LED oder ...

[Jörg S. (mitchell)]

Hallo,
und schon wieder ein neues Problem mit den Makros.
Ich komme nicht mehr aus der ersten IF raus, da es immer wahr ist.
Auch wenn die beiden anderen Tasten betätigt werden, ist der Taster 1 
bereits betätigt und lande bei der ersten IF.

Taster 1 ist so zu sagen der Hauptschalter und ist immer vor den anderen 
betätigt.
Es soll getestet werden:

ist nur SW1 on
 -Anweisung
ist SW1 und SW2 on
 -Anweisung
ist SW1 und SW3 oder SW1 und SW2 und SW3 on (SW2 ist hier egal)
 -Anweisung
sonst
 -Anweisung

das hat ohne die Makros funktioniert nur eben nicht die verschachtele 
Anweisung beim 3.Fall mit der Schleife.

Gruß Jörg

[Jörg S. (mitchell)]

Hallo,
ja, das LED_OFF Makro fehlt noch.

Wenn man gerade C lernt, ist das mit dem Makros noch komplizierter als 
die direkte Zuweisung.
Aber man möchte es auch gleich richtig lernen.

Gruß Jörg

[Karl H. (kbuchegg)]

Jörg S. schrieb:

> ist nur SW1 on

'nur' bedeutet in diesem Zusammenhang
* Taster1 ist betätigt    UND
* Taster2 ist nicht betätigt    UND
* Taster3 ist nicht betätigt

In Codeform

    if  (  KEY_PRESSED( PIND, SWITCH_1 ) &&
         ! KEY_PRESSED( PIND, SWITCH_2 ) &&
         ! KEY_PRESSED( PIND, SWITCH_3 ) )

schreib was du meinst. Aber überleg dir vorher, ob das was du meinst 
auch die Realität wiederspiegelt.

Und ja:
wir sind im täglichen Leben recht schlampig was strenge Logik angeht. 
Wir haben unsere Code-Wörter, die der Person gegenüber signalisieren, 
was er sich dazudenken muss.

[Jörg S. (mitchell)]

Hallo,
alles klar, danke.

[Karl H. (kbuchegg)]

Jörg S. schrieb:

> Taster 1 ist so zu sagen der Hauptschalter und ist immer vor den anderen
> betätigt.

Wenn es sich um einen Hauptschalter handelt, dann formulier das auch 
genau so

   if( KEY_PRESSED( PIND, SWITCH_1 ) )    // Hauptschalter auf "ein" ?
   {

     if( KEY_PRESSED( PIND, SWITCH_2 ) )
       ...

     if( KEY_PRESSED( PIND, SWITCH_3 ) )
       ...
   }

Ist der Hauptschalter nicht auf 'ein', dann interessieren dich auch die 
anderen Schalter nicht mehr. Wie im täglichen Leben. Ist im Auto die 
Zündung ausgeschaltet, dann interessiert dich die Stellung des 
Scheibenwischerschalters nicht weiter. Erst mal geht es um die Frage: 
Ist die Zündung überhaupt ein oder nicht? Und nur dann, wenn die 'ein' 
ist, erst dann willst du die Stellung des Scheibenwischerschalters 
wissen um zu entscheiden, ob gewischt werden sollte oder nicht. Aber 
solange die Zündung aus ist, funktioniert nicht sehr viel im Auto.

[Jörg S. (mitchell)]

Hallo,
so ist es noch besser.

Gruß Jörg

[Jörg S. (mitchell)]

Hallo Karl Heinz,
noch mal danke für die Hilfe.
Es funktioniert jetzt so wie es soll.
Auch die Schleife um das pulsierende Signal anzuzeigen ohne das die LED 
mit blinkt funktioniert jetzt.
Das Ausschalten der LEDs habe ich in die else Anweisung gepackt.

Der Dateiname stimmt aber nicht mehr, es gibt keine else if mehr.

Gruß Jörg

[Karl H. (kbuchegg)]

Kleiner Tip.

Das hier

      if (KEY_PRESSED( PIND, SWITCH_3)) {      
      LED_ON( LED3 );
      z = 0 ;                      // Zähler zurück setzen
        for (; z<=4; z++) {
        _delay_ms(200);                // warte 200ms
          if (KEY_PRESSED( PIND, SWITCH_3)) {    // ist SW3 noch an?
            z = 0;                // ja, Zähler zurück setzen
          }
        }
      }

lässt sich auch deutlich einfacher und ohne 'von hinten durch die Brust 
ins Auge'-Programmierung erreichen.

Was ist denn der Sinn der Sache?
Die LED3 soll eingeschaltet werden.
Danach soll solange gewartet werden, bis die Taste losgelassen wird. 
Weiter geht es erst, wenn die Taste 4 mal hintereinander als losgelassen 
gesehen wird, was sich in Summe bei 200ms pro Schleifendurchlauf zu 
800ms aufsummiert.
D.h. der Nettoeffekt ist: Die LED wird auf jeden Fall 800ms brennen, 
möglicherweise auch länger wenn die Taste gedrückt bleibt. Erst danach 
geht  es dann weiter (was ist eigentlich, wenn bei gedrückter Taste 3 
der Hauptschalter losgelassen wird? Im Moment kümmert das nicht - was 
aber eigentlich am Sinn eines Hauptschalters vorbei geht). 'Weiter geht' 
bedeutet aber in jedem Fall, dass die LED 3 wieder abgeschaltet wird.

Wie auch immer. Das hier

      if (KEY_PRESSED( PIND, SWITCH_3)) {      
        LED_ON( LED3 );
        delay_ms( 800 ); 
        while( KEY_PRESSED( PIND, SWITCH_3) )
          ;
        LED_OFF( LED3 );
      }

macht daher offensichtlich genau dasselbe. Bei deutlich einfacherem 
Code.

Edit:
stimmt nicht ganz. Im Original kann ich die Taste zum richtigen 
Zeitpunkt zwischendurch loslassen und wieder drücken, ohne dass es deine 
Warte auf Taste loslassen Schleife abbricht, was bei dieser Version 
nicht geht. Auf der anderen Seite habe ich dafür ganze 8 Zehntel 
Sekunden Zeit, was nicht wahnsinnig viel ist. Aber wer weiss: vielleicht 
bist du ja gerade darauf aus: Schnell genug (aber auch nicht zu schnell) 
auf die Taste hämmern, lässt die LED weiterleuchten.
Das hätte sich dann allerdings einen Kommentar verdient.

[Jörg S. (mitchell)]

Hallo,
da hast du Recht, wenn SW1 geöffnet wird, bleibt LED3 an.

Das habe ich gleich geändert und jetzt geht die LED3 nach Ablauf der 
Schleife auch aus.

SW3 simuliert ein pulsierendes Signal mit ca. 1HZ, kommt nicht von einem 
Quarz, daher ca.
Die LED soll aber nicht im Takt blinken, sondern dauernd leuchten bis 
das Signal nicht mehr anliegt.

Das hatte ich weiter oben schon mal erwähnt.

Gruß Jörg