Code funktioniert nur manchmal

[Doc_Arduino]

Hallo,

combie, meinste nicht auch das dein goto mit einem switch case nicht deutlich klarer lesbar wäre? Bist du nicht der der lesbaren Code vorzieht ...     duck und weg     Ist nur Spass, ist dein Code.

Computed goto kenne ich nicht und befinde mich damit außerhalb meines Wissensstandes. Interessant zu wissen.

if hatte ich schon einmal gegen switch case antreten lassen. Wenn ich mich recht erinnere wurde if mit jedem Vergleich langsamer und switch case war immer konstant gleich schnell/langsam, nur abhängig von der Summe der case Vergleiche. if kann man dafür wiederum von Hand optimieren in dem man die Reihenfolge der Vergleiche vertauscht. Welcher Vergleich wahrscheinlich öfterer zuschlägt wie andere.

Meine Bedenken sind das die Gefahr besteht, wenn das hier Neulinge lesen, die gewöhnen sich goto an weil es bequem ist und dann endet das in Unübersichtlichkeit. Selbst mit einer Batchdatei (Windows) erlebt. Zwangsweise goto zur Ablaufsteuerung verwendet, bin bald blöde gewurden im wilden Gehüpfe.

Ich sage mal so. Ich weiß jetzt das es hier und da sinnvoll sein kann. Nur selbst habe ich es noch nie benötigt und ich denke ich werde es auch nicht benötigen.     Stand heute.  Ich befasse mich dann doch lieber immer weiter mit leserlicheren Code zu schreiben für mich und für andere. Auch meine switch case kann ich hier und da noch aufräumen. Es hat eben jeder seine Herangehensweise. Ich nehme gern neue Dinge zur Kenntnis, schau mir das an und entscheide dann für mich alleine ob ich das anwende/verwende oder auch nicht. In dem Fall hier wie man sieht nicht.

Zitat aus "Shooter".   "langsam ist präzise und präzise ist schnell"    

In diesem Sinne allen einen ruhigen Sonntag.

[Serenifly]

if hatte ich schon einmal gegen switch case antreten lassen. Wenn ich mich recht erinnere wurde if mit jedem Vergleich langsamer und switch case war immer konstant gleich schnell/langsam,

Selbstverständlich. Die Zeit ist konstant weil switch/case i.d.R. als Sprungtabelle umgesetzt wird. Allerdings gehen die schnellen RJMPs (relative jump) "nur" +/- 2kB.

[Doc_Arduino]

Hallo,

noch einer wach.  Mit 2kB meinst du den Speicherbereich in dem gesprungen wird oder darf die angelegte Sprungtabelle nicht 2kB belegten Speicher überschreiten?

[combie]

combie, meinste nicht auch das dein goto mit einem switch case nicht deutlich klarer lesbar wäre? Bist du nicht der der lesbaren Code vorzieht ...     duck und weg      Ist nur Spass, ist dein Code.

Dann zeige doch mal wie du einen solchen Doppelblinker bauen würdest...
Dann können wir das nebeneinander halten und an der Sache die "Schönheit", "Eleganz", "Wartungsfreundlichkeit" und "Übersichtlichkeit" bewerten.

Vielleicht kann ich ja von dir noch was lernen!

[Whandall]

Ich finde eine Version mit switch und enum ist besser zu lesen, aber schau selbst

Code: [Select]

void run(const bool blinkAnforderung) {
  switch (aktuell) {
    case Start:
      if (!blinkAnforderung)
        return;
      digitalWrite(ledPin, HIGH);
      aktuell = Hell;
      zeitMerker = millis();
    case Hell:
      if (millis() - zeitMerker < hellPhase)
        return;
      digitalWrite(ledPin, LOW);
      aktuell = Dunkel;
      zeitMerker = millis();
    case Dunkel:
      if (millis() - zeitMerker >= dunkelPhase)
        aktuell = Start;
  }
}


Hier noch mal alles zusammen

Code: [Select]

enum Zustand {
  Start,
  Hell,
  Dunkel,
};

const byte taster = 2;

class Blinker
{
  protected:
    const byte ledPin;
    const unsigned long hellPhase;
    const unsigned long dunkelPhase;
    unsigned long zeitMerker = 0;
    Zustand aktuell = Start;

  public:
    Blinker(const byte ledPin, const unsigned long hellPhase, const unsigned long dunkelPhase)
      : ledPin(ledPin), hellPhase(hellPhase), dunkelPhase(dunkelPhase) {}

    void run(const bool blinkAnforderung) {
      switch (aktuell) {
        case Start:
          if (!blinkAnforderung)
            return;
          digitalWrite(ledPin, HIGH);
          aktuell = Hell;
          zeitMerker = millis();
        case Hell:
          if (millis() - zeitMerker < hellPhase)
            return;
          digitalWrite(ledPin, LOW);
          aktuell = Dunkel;
          zeitMerker = millis();
        case Dunkel:
          if (millis() - zeitMerker >= dunkelPhase)
            aktuell = Start;
      }
    }

    void init() {
      pinMode(ledPin, OUTPUT);
    }
};

Blinker blinkGruppe[] = { // {pin,hellzeit,dunkelzeit}
  {13, 100, 500},
  {12, 500, 1000},
};

void setup() {
  pinMode(taster, INPUT_PULLUP);
  for (Blinker &blinker : blinkGruppe)
    blinker.init();
}

void loop() {
  bool blinkAnforderung = !digitalRead(taster);
  for (Blinker &blinker : blinkGruppe)
    blinker.run(blinkAnforderung);
}

Ich habe so wenig wie möglich geändert.
Was nicht heissen soll, dass ich "einen solchen Doppelblinker" so schreiben würde...

[combie]

Was nicht heissen soll, dass ich "einen solchen Doppelblinker" so schreiben würde...

Wie würdest du ihn schreiben?

Und ja, so wie deins sehen eigentlich alle meine hier je geposteten Blink Klassen aus.

Mit einer Ausnahme:
Den enum würde ich nicht global machen, weil er nicht global gebraucht wird.

Und vermutlich würde ich meine "neue" CombieLib für IO und Timer einsetzen.
Aber das Argument gilt noch nicht, weil die Lib noch nicht fertig ist.

Also die von mir "verbesserte" Version:

Code: [Select]



const byte taster = 2;

class Blinker
{
  protected:
    enum Zustand {  Start,  Hell,  Dunkel,};
    const byte ledPin;
    const unsigned long hellPhase;
    const unsigned long dunkelPhase;
    unsigned long zeitMerker = 0;
    Zustand aktuell = Start;

  public:
    Blinker(const byte ledPin, const unsigned long hellPhase, const unsigned long dunkelPhase)
      : ledPin(ledPin), hellPhase(hellPhase), dunkelPhase(dunkelPhase) {}

    void run(const bool blinkAnforderung) {
      switch (aktuell) {
        case Start:
          if (!blinkAnforderung)
            return;
          digitalWrite(ledPin, HIGH);
          aktuell = Hell;
          zeitMerker = millis();
        case Hell:
          if (millis() - zeitMerker < hellPhase)
            return;
          digitalWrite(ledPin, LOW);
          aktuell = Dunkel;
          zeitMerker = millis();
        case Dunkel:
          if (millis() - zeitMerker >= dunkelPhase)
            aktuell = Start;
      }
    }

    void init() {
      pinMode(ledPin, OUTPUT);
    }
};

Blinker blinkGruppe[] = { // {pin,hellzeit,dunkelzeit}
  {13, 100, 500},
  {12, 500, 1000},
};

void setup() {
  pinMode(taster, INPUT_PULLUP);
  for (Blinker &blinker : blinkGruppe)
    blinker.init();
}

void loop() {
  bool blinkAnforderung = !digitalRead(taster);
  for (Blinker &blinker : blinkGruppe)
    blinker.run(blinkAnforderung);
}

Die Schachtelungstiefe ist allerdings um eins höher, als bei der Goto Version.
Auch werden zwei Sprachkonstrukte genutzt, statt nur eins.
Aber beides ist kein KO Kriterium, für diese Variante.

Ansonsten, wie schon gesagt:

Wie würdest du ihn schreiben?

Jetzt frei, ohne dich an meine Vorlage zu halten.
Nur, identische Funktion, also gleiche Aufgabe.

Denn:

Vielleicht kann ich ja von dir noch was lernen!

[Whandall]

Deine run Funktion benutzt mehr unsaubere und/oder exotische Sprachelemente als eine Version mit enum und switch.

Code: [Select]

    void *sprungZiel = 0; // zustandsmerker
      if (!sprungZiel) sprungZiel = && Start;
      goto *sprungZiel;
      sprungZiel = 0;

Wenn das deinem Kodestil entspricht, kannst du noch viel lernen, egal von wem. 

[combie]

du noch viel lernen, egal von wem.

Genau!

Damit ich dich richtig verstehe...
Kritik ja.
Zeigen nein.

[Whandall]

Damit ich dich richtig verstehe...
Kritik ja.
Zeigen nein.

Ich habe dir gerade gezeigt wie man es einfacher verständlich und weniger exotisch formulieren kann.

[combie]

Du sprachest:

Was nicht heissen soll, dass ich "einen solchen Doppelblinker" so schreiben würde...

Und ich fragte darauf hin:

Wie würdest du ihn schreiben?

Aber ist ok, wenn du nicht magst, dann lass es...

[Whandall]

Ich habe schon verstanden, dass du vom Thema ablenken willst.

Es geht hier um goto, &&, void* versus struct und enum.

Da ist - meiner Ansicht nach - enum und switch zu bevorzugen,
was ich mit meinem Beispiel versucht habe deutlich zu machen.

Wenn du anderer Ansicht bist, von mir aus.

[Doc_Arduino]

Hallo,

goto umgewandelt in switch case, ich finde das ist besser lesbar, jetzt könnte man noch die Logikwechsel entwirren, mal sehen ob ich das auch noch hinbekomme ...

Vergleichstest ergeben beide Sketche verhalten sich gleich. Zufällig, war keine Absicht, ist der switch case Sketch um 14 Byte kleiner. Ehrlich gesagt dachte ich der wird etwas größer. Egal.

Code: [Select]


const byte taster = 2;

class Blinker
{
  protected:
  const byte ledPin;
  const unsigned long hellPhase;
  const unsigned long dunkelPhase;
  unsigned long zeitMerker = 0;
  typedef enum {START, HELLPHASE, DUNKELPHASE} state;    // Steuerzustände
  state sprungZiel = START;

  public:
  Blinker(const byte ledPin,const unsigned long hellPhase,const unsigned long dunkelPhase): ledPin(ledPin),hellPhase(hellPhase),dunkelPhase(dunkelPhase){}

  void run(const bool blinkAnforderung)
  {
    switch (sprungZiel) {
      case START:
                  if(!blinkAnforderung) break;
                  digitalWrite(ledPin,HIGH);
                  zeitMerker = millis();
                  sprungZiel = HELLPHASE;
                  break;
  
      case HELLPHASE:
                  if(millis()-zeitMerker<hellPhase) break;
                  digitalWrite(ledPin,LOW);
                  zeitMerker = millis();
                  sprungZiel = DUNKELPHASE;
                  break;
 
      case DUNKELPHASE:
                  if(millis()-zeitMerker<dunkelPhase) break;
                  sprungZiel = START;
                  break;
                  
      default:    digitalWrite(ledPin,LOW);         // sollte nie eintreten
                  break;          
    }            
  }
 
  void init()
  {
    pinMode(ledPin,OUTPUT);
  }
};


Blinker blinkGruppe[] = { // {pin,hellzeit,dunkelzeit}
                          {30,100, 500},
                          {31,500,1000},
                        };




void setup()
{
  pinMode(taster,INPUT_PULLUP);
  for(Blinker &blinker:blinkGruppe) blinker.init();
}

void loop()
{
  bool blinkAnforderung = !digitalRead(taster);
  for(Blinker &blinker:blinkGruppe) blinker.run(blinkAnforderung);
}

Edit:
ihr habt ja schon ohne mich weitergemacht, habt ihr die breaks in den cases absichtlich weggelassen? Der Code rammelt doch ohne break durch? Gleich einmal testen ...
Edit 2:
okay, wegen der jeweiligen if Abfrage wird das durchrammeln verhindert  
Gefällt mir auch noch nicht so richtig, der Status darf erst bei Bedarf weitergeschalten werden und sollte dann auch ausgeführt werden ohne zusätzliche Hinderung, mal sehen ob mir da nochwas einfällt ...

[Whandall]

ihr habt ja schon ohne mich weitergemacht, habt ihr die breaks in den cases absichtlich weggelassen? Der Code rammelt doch ohne break durch? Gleich einmal testen ...

Ich habe versucht möglichst wenig zu verändern und der originale Kode fiel durch, also habe ich das so gelassen.
Wenn man breaks benutzt wiirde es wahrscheinlich einen Hauch schneller, aber auch grösser werden.

[Doc_Arduino]

Hallo,

so wenig wie möglich ändern war auch meine erste Herangehensweise. Man muss sich erstmal reindenken.
Zufrieden war ich mit dem switch case noch nicht. 
Meine neue Version ist nun auch 6 Byte größer. Egal. Ich finde es noch besser lesbar. Man sieht was wann passiert.
Da man naturgemäß mit positiver Logik besser klar kommt, habe ich die positive Logik, die combie in loop mit der Tasterabfrage schon macht in switch case weitergeführt. Die Zustände namentlich machen jetzt auch das was sie aussagen. Das war mein Ziel.
Bsp. der Zustand AUS bleibt aus, solange wie nicht erneut geblinkt werden soll, erst dann ändert sich der Zustand.
EINSCHALTEN schaltet wirklich nur ein und übergibt in den nächsten Zustand.
Danach bleibt der Zustand HELLPHASE solange erhalten wie er gültig ist, macht demnach auch hier genau das was der Name sagt usw.

Alle wieder versöhnt?

switch default könnte man hier weglassen, da alle Zustände behandelt werden, das prüft ja der Compiler.

Code: [Select]

const byte taster = 2;

class Blinker
{
  protected:
  const byte ledPin;
  const unsigned long hellPhase;
  const unsigned long dunkelPhase;
  unsigned long zeitMerker = 0;
  typedef enum {AUS, EINSCHALTEN, HELLPHASE, DUNKELPHASE} state;    // Steuerzustände
  state sprungZiel = AUS;

  public:
  Blinker(const byte ledPin,const unsigned long hellPhase,const unsigned long dunkelPhase): ledPin(ledPin),hellPhase(hellPhase),dunkelPhase(dunkelPhase){}

  void run(const bool blinkAnforderung)
  {
   
    switch (sprungZiel) {
      case AUS:   
                  if(blinkAnforderung == true) {
                    sprungZiel = EINSCHALTEN;
                  }
                  break;
                 
      case EINSCHALTEN:
                  digitalWrite(ledPin,HIGH);
                  zeitMerker = millis();
                  sprungZiel = HELLPHASE;
                  break;
                 
      case HELLPHASE:
                  if(millis()-zeitMerker > hellPhase) {
                    digitalWrite(ledPin,LOW);
                    zeitMerker = millis();
                    sprungZiel = DUNKELPHASE;
                  }
                  break;
   
      case DUNKELPHASE:
                  if(millis()-zeitMerker > dunkelPhase) {
                    sprungZiel = AUS;
                  } 
                  break;
                 
      default:    digitalWrite(ledPin,LOW);         // sollte nie eintreten
                  break;         
    }           
  }
 
  void init()
  {
    pinMode(ledPin,OUTPUT);
  }
};


Blinker blinkGruppe[] = { // {pin,hellzeit,dunkelzeit}
                          {30,100, 500},
                          {31,500,1000},
                        };




void setup()
{
  pinMode(taster,INPUT_PULLUP);
  for(Blinker &blinker:blinkGruppe) blinker.init();
}

void loop()
{
  bool blinkAnforderung = !digitalRead(taster);
  for(Blinker &blinker:blinkGruppe) blinker.run(blinkAnforderung);
}


[combie]

habt ihr die breaks in den cases absichtlich weggelassen?

Ich, nicht absichtlich.
Eher Faulheit.

Ein break bzw. return ist bei dem Ablauf, an der Stelle, nicht nötig.
(Edit: wie du ja gerade selber bemerkt hast)

Und was nicht nötig ist, lasse ich gerne weg....
Mein Fokus liegt da eher auf sprechende Bezeichner, möglichst flache Struktur, einfache Bedingungen.
u.s.w.