Übung mit RGB - Ist das sauber programmiert?

International Deutsch
1

Hallo!

Bin, was die Arduino-Programmierung angeht noch recht unerfahren; kenn mich nur einwenig mit C-programmierung aus.

Hab mir daher mal eine kleine Übungsaufgabe gestellt:

Eine RGB (gemeinsamer Pluspol) soll über einen Taster gesteuert werden.
Wenn man den Taster drückt, wechselt die RGB ihre Farbe nach diesem Schema:

RGB leuchtet rot --> drücke Taster --> RGB leuchtet grün --> drücke Taster --> RGB leuchtet blau --> drücke Taster --> wieder von vorne.

Hab schon alles programmiert und die Schaltung aufgebaut.
Würde gerne wissen, ob man da noch was verbessern könnte (vorallem im Quelltext) oder ob das so schon ideal ist.

Hier mal die Schaltung (mit Frintzing erstellt):

und der Quelltext von Hauptfunktion und Unterfunktion:

main:

/*Deklaration der Pins*/
int rot = 9;
int gruen = 10;
int blau = 11;
int taster = 2;

/*Deklaration und Initialisierung der Zählvariable*/
int i = 1;  

void setup()
{
    pinMode(rot,OUTPUT);
    pinMode(gruen,OUTPUT);
    pinMode(blau,OUTPUT);    
    pinMode(taster,INPUT);
}


void loop()
{   
    leuchten(rot,gruen,blau,i);       // Unterfunktion bringt RGB zum Leuchten
  
    if ( digitalRead(taster) == 1)     // Wenn Taster gedrückt wird soll nächste Farbe leuchten
     {      
       delay(50);                     // zum Entprellen des Tasters
       
       i++;                            // Erhöhung der Zählvariable
       if ( i > 3) i = 1;             // Falls Zählvariable größer als drei ist, soll vonvorne gezählt werden
       leuchten(rot,gruen,blau,i);       // Unterfunktion bringt neue Farbe zum leuchten
       

       do {                                 // Programm fährt erst fort, wenn Taster wieder losgelassen wurde
           digitalRead(taster);
       } while ( !(digitalRead(taster)==0));
       
     }     
     
     delay(50);         //zum Entprellen des Tasters
}

leuchten:

void leuchten(int rot, int gruen, int blau, int i)      
      
{            
      switch (i)      
      {        
         case 1: digitalWrite(rot,LOW);      // rote LED an
                 digitalWrite(gruen,HIGH);
                 digitalWrite(blau,HIGH);
                 break;
                 
         case 2: digitalWrite(rot,HIGH);    // grüne LED an
                 digitalWrite(gruen,LOW);
                 digitalWrite(blau,HIGH);
                 break;
                 
         case 3: digitalWrite(rot,HIGH);    // blaue LED an
                 digitalWrite(gruen,HIGH);
                 digitalWrite(blau,LOW);
                 break;
      }      
      return;      
}

Lässt sich da noch irgendetwas sauberer programmieren? Wenn ja, würde ich mich über Tipps freuen!

Gruß ser_u

2

1.) Konstanten wie Pin Definitionen kann man als "const" deklarieren. Dadurch wird es deutlicher und spart eventuell etwas RAM

2.) Bei digitalRead besser HIGH/LOW satt 0/1 verwenden. Der Code ist der gleiche, aber es ist lesbarer

3.) Bei Funktionen die keinen Rückgabewert haben spart man sich normalerweise das return am Ende

4.) "i" kannst du innerhalb von loop() deklarieren wenn du es "static" machst. Dann behält es seinen Wert von Iteration zu Iteration und ist dort deklariert wo man es verwendet

Das sind aber Kleinigkeiten :slight_smile:

3

An sich ist das schon recht sauber programmiert, finde ich.

Was ich anders machen würde:
a) mit

       do {                                 // Programm fährt erst fort, wenn Taster wieder losgelassen wurde
           digitalRead(taster);
       } while ( !(digitalRead(taster)==0));

blockierst du den Prozessor.
Das macht man eher nicht, Ziel ist es, dass die loop() möglichst oft durchlaufen wird. Wenn du nun eine weitere Funktion dazu packen würdest, würde die nicht ausgeführt werden, solange kein Taster gedrückt ist.
Lass den Teil einfach weg, in die IF geht er sowieso nur rein wenn der Taster gedrückt wurde.

b) die Funktion leuchten() rufst du 2mal auf: einmal in der Loop, einmal nach Tastendruck. Das schadet nicht, aber es reicht eigentlich, wenn du die Funktion nur nach einem Tastendruck aufrufst und die Ausgänge neu setzt.

c) Du hast die LED-Pins global definiert, übergibst sie aber in die Funktion.
Pins global definieren ist ok. Ich würde im Namen festhalten, dass es ein Pin ist, und, vor allem als Constante definieren. dann können die im Code nicht aus Versehen geändert werden.

/*Deklaration der Pins*/
const int LED_PIN_ROT = 9;
const int LED_PIN_GRUEN = 10;
const int LED_PIN_BLAU = 11;
const int TASTER_PIN = 2;

Und das übergeben an die Funktion kannst du dir dann sparen.

4

Hey, danke für eure Verbesserungsvorschläge!

In die Variablenbezeichnung "pin" einfügen und Pins als const int definieren ist sinnvoll, hab ich gleich mal umgesetzt.
Auch das mit HIGH und LOW statt 1 und 0 bei digitalRead klingt vernünftig.

zu Punkt a) von guntherb:
Das Problem ist, dass die Loop-Schleife so schnell durchlaufen wird, dass die Farbe mehr als einmal pro Tastendruck wechselt.
Genau diese Stelle hat mich übrigens auch am meisten gestört, mir ist kein anderer Weg eingefallen, das zu lösen.
Vielleicht hat hier jemand eine Idee?

zu Punkt b) von guntherb
An sich hast du Recht, hier war das Problem, dass beim Programmstart alle Pins auf Low sind, und somit alle Farben leuchten und sich mischen.
Die Unterfunktion hab ich am Anfang nochmal reingepackt, damit die RGB einen definierten Zustand hat (rot leuchtet).

zu Punkt c) von guntherb:
Danke für den Hinweis mit der Übergabe an die Unterfunktion. Habs geändert, ist jetzt übersichtlicher geworden!

zu Punkt 3) von Serenifly:
return; am Ende ist Lehrmeinung gewesen bei uns :slight_smile:
Habs mir so angewöhnt, finde ich auch intuitiver als wenn da nichts steht.

zu Punkt 4) von Serenifly
static muss ich mir mal anschauen. Davon höre ich das erste mal!

Danke nochmal für eure Hinweise, ich poste gleich mal die korrigierte Version.
Falls jemand noch Ratschläge hat, immer her damit :smiley:

Gruß, ser_u

5 
 do {                                 // Programm fährt erst fort, wenn Taster wieder losgelassen wurde
           digitalRead(taster);
       } while ( !(digitalRead(taster)==0));

Da du sowieso nur wartest, ist es egal, aber das innere digitalRead(taster) macht nichts.

while (digitalRead(taster) == HIGH) { }  //  Programm fährt erst fort, wenn Taster wieder losgelassen wurde

So, mit den const Deklarationen und mit weniger Funktionsparametern wird dein Code ein paar Byte kleiner und kommt mit weniger RAM aus.

6

So, hier die aktuellste Version vom kleinen Programm:

main

/*Deklaration der Pins*/
const int r_pin = 9;
const int g_pin = 10;
const int b_pin = 11;
const int taster_pin = 2;


/*Deklaration und Initialisierung der Zählvariable*/
int i = 1;  

void setup()
{
    pinMode(r_pin,OUTPUT);
    pinMode(g_pin,OUTPUT);
    pinMode(b_pin,OUTPUT);    
    pinMode(taster_pin,INPUT);
}


void loop()
{   
    leuchten(i);                             // Unterfunktion bringt RGB zum Leuchten
  
    if ( digitalRead(taster_pin) == HIGH)    // Wenn Taster gedrückt wird soll nächste Farbe leuchten
     {      
       delay(50);                            // zum Entprellen des Tasters
       
       i++;                                  // Erhöhung der Zählvariable
       if ( i > 3) i = 1;                    // Falls Zählvariable größer als drei ist, soll von vorne gezählt werden
      
       leuchten(i);                          // Unterfunktion bringt neue Farbe zum leuchten 
       

       while (digitalRead(taster_pin) == HIGH) { }  // Programm fährt erst fort, wenn Taster wieder losgelassen wurde
       
     }     
     
     delay(50);                              // zum Entprellen des Tasters
}

leuchten

void leuchten(int i)      
      
{            
      switch (i)      
      {        
         case 1: digitalWrite(r_pin,LOW);      // rote LED an
                 digitalWrite(g_pin,HIGH);
                 digitalWrite(b_pin,HIGH);
                 break;
                 
         case 2: digitalWrite(r_pin,HIGH);    // grüne LED an
                 digitalWrite(g_pin,LOW);
                 digitalWrite(b_pin,HIGH);
                 break;
                 
         case 3: digitalWrite(r_pin,HIGH);    // blaue LED an
                 digitalWrite(g_pin,HIGH);
                 digitalWrite(b_pin,LOW);
                 break;
      }      
      return;      
}
7

Danke für den Tip michael_x,
habs gleich mal übernommen.

Gruß ser_u

8

Alles brav befolgt. Hast ein Bienchen verdient :wink:

Bonus-Aufgaben:

  1. Nach Start sollen alle LED aus sein, bis der Taster das erste Mal gedrückt wird. ( In C fangen Zähler immer bei 0 an )
  2. Das ist ein großes Projekt von mehreren Leuten. Der Programmierer von Leuchten() glaubt nicht, dass der Programmierer von loop immer ordentlich arbeitet. Was passiert im jetzigen Zustand bei Leuchten(-1) ?

9 
         case 0: digitalWrite(r_pin,HIGH);      // keine LED an
                 digitalWrite(g_pin,HIGH);
                 digitalWrite(b_pin,HIGH);
                 break;

in die Unterfunktion einfügen

und i=0; initialisieren in main.

  1. hmm also -1 ist ja nicht definiert in switch, demzufolge würde nichts passieren bzw switch würde vom Programm übersprungen werden, denke ich.

Könnte zum Abfangen von solchen Fehlern zum Beispiel den default Fall noch anhängen in der Unterfunktion:

default: i=0;

Gruß

PS: Danke für dei Übungsaufgaben :smiley: