C++ / Arduino - Klassen und Objekte verschachteln. Wie geht's richtig?

[Marcus W. (marcusaw)]

Hallo,

Ich brauche ein Objekt, das zur Laufzeit erstellt wird, welches andere 
Objekte enthält, die zur Objekterstellung erstllt werden sollen. Das 
ganze soll 1-x LEDs ansteuern.

Schlauerle in mir sagt: klar, zwei Klassen - eine LED() und eine 
LEDController().

LEDController bekommt eine attach()-Methode in der LED-Objekte zu einer 
Liste hinzugefügt werden.

So weit, so theoretisch. Nur dann steige ich mit den Pointern aus.
Bei einer "modernen" Sprache (bitte nicht hauen) würde ich in etwa 
soetwas machen:
(VORSICHT! - PseudoCode)

public class LEDController
{
  int ledCount;
  LED LEDArray[10];
  public LEDController()
  {
    ledCount = 0;
  }

  public void attach(int _pin)
  {
    LEDArray[ledCount] = new LED(_pin);
  }

  public LED getChannel(int _channel)
  {
    return LEDArray[_channel];
  }


}

public class LED
{
  int _pin;
  public LED(int pin)
  {
    pinMode(pin, OUTPUT);
      _pin = pin;
  }

  public void blink()
  {
    digitalWrite(_pin, HIGH);
    delay(250);
    digitalWrite(_pin, LOW);
    delay(250);   
  } 
}


LEDController c1 = new LEDController();

void setup()
{
  c1.attach(D0);
  c1.attach(D1);
  c1.attach(D2);
}

void loop()
{
  c1.getChannel(0).blink();
}

In der richtigen Welt hab ich folgendes probiert:

test.ino

#include "ledcontroller.h"
#include "led.h"

LEDController myLedController;

void setup()
{
  
  myLedController.attach(LED(D1));

  
}

void loop()
{
  myLedController.getChannel(0)->blink();
  
}

LEDController.h

#ifndef ledController_h
#define ledController_h

#include "Arduino.h"
#include "led.h"


class LEDController
{
  public:
    LEDController();
    void attach(LED _led);
    LED getChannel(int _channel);

  private:
    LED _ledArray[10]{};
    int _ledcount;
};
#endif

LEDController.cpp

LEDController::LEDController()
{
  _ledcount = 0;
}

void LEDController::attach(LED _led)
{
  _ledArray[_ledcount] = _led;
  _ledcount++;
}

LED LEDController::getChannel(int _channel)
{
  return _ledArray[_channel];
}

LED.h

#ifndef led_h
#define led_h

#include "Arduino.h"

class LED
{
  public:
    LED(int pin);
    LED();
    void blink();
  private:
    int _pin;
};
#endif

LED.cpp

#include "Arduino.h"
#include "led.h"

LED::LED(int pin)
{
  pinMode(pin, OUTPUT);
  _pin = pin;
  _state = 0;
}

void LED::blink()
{
  digitalWrite(_pin, HIGH);
  delay(250);
  digitalWrite(_pin, LOW);
  delay(250);  
}

Aber - klar, funktioniert so nicht. Ich muss das irgendwie miteinander 
verschwurbeln, krieg es aber nicht hin... Ich glaube zu wissen, dass die 
Lösung Pointer hat - einfach nur, weil ich sogut wie keine habe :)

[Marcus W. (marcusaw)]

Ich hab jetzt eine - wahrscheinlich völlig ungeeignete, dennoch 
funktionierende Lösung:

#include <vector>

class LED
{
  private:
    int _LEDPin;

  public:
    LED (int newLEDPin)
    {
      _LEDPin = newLEDPin;
      pinMode(_LEDPin, OUTPUT);
      digitalWrite(_LEDPin, LOW);
    }

    void blink()
    {
      digitalWrite(_LEDPin,HIGH);
      delay(1000);
      digitalWrite(_LEDPin, LOW);
      delay(1000);
    }
};

class LEDController
{
  private:
    std::vector<LED> LEDList;
  
  public:
    void attach(int pin)
    {
      LEDList.push_back(LED(pin));
    }

    LED getChannel(int channel)
    {
      return LEDList[channel];
    }
};

LEDController myController;
void setup()
{
  myController.attach(D1);
  myController.attach(D2);
}
 
void loop()
{
  myController.getChannel(1).blink();  
}

[tictactoe (Gast)]

Ist ja schon nicht schlecht! Auf Pointer zu verzichten ist normalerweise 
genau richtig.

Wahrscheinlich ist dein Problem jetzt, dass du nicht nur eine LED 
blinken lassen willst, sondern alle gleichzeitig. Dazu ist deine blink() 
Routine ungeeignet, weil sie sich zwei Sekunden mit einer LED 
beschäftigt und der Zeit kann sonst nichts passieren. Du musst die 
Funktion in eine Einschalt- und eine Ausschaltfunktion aufteilen und die 
beiden von loop() in einer Schleife aufrufen.

[M.K. B. (mkbit)]

Marcus W. schrieb:
> Ich hab jetzt eine - wahrscheinlich völlig ungeeignete, dennoch
> funktionierende Lösung:

Ich finde die gar nicht so schlecht. Du verwendest keine Pointer, die zu 
Leak führen könnten.
Das Programm ist soweit funktional korrekt, folgende Tipps könnten es 
aber noch verbessern. Ich setze mal C++11 als Sprachversion voraus. Frag 
einfach, wenn etwas für dich noch nicht verständlich ist.

Du erzeugst immer eine Kopie der LED Klasse. Das ist jetzt nicht 
schlimm, weil eh nur der Integer kopiert wird, führt aber zu Fehlern 
wenn du dir einen Zustand in LED merken willst (z.B. für Toggle). 
Außerdem kann man LED auch ohne LEDPin anlegen.
Man kann das Kopieren von Objekten verbieten indem man den 
Kopierkonstruktor und den Zuweisungsoperator verbietet, allerdings 
werden diese vom Vektor benötigt, damit dieser sich vergrößern kann.

class LED
{
  private:
    int _LEDPin;

  public:
    LED() = delete; // LED kann nicht ohne Parameter angelegt werden
    LED (int newLEDPin)
      :_LEDPin(newLEDPin) // Initialisierungsliste (macht bei int keinen Unterschied, bei anderen Objekten schon)
    {
      pinMode(_LEDPin, OUTPUT);
      digitalWrite(_LEDPin, LOW);
    }

    void blink()
    {
      // ... Code unverändert
    }
};

Verhinderung der Kopie.

    void attach(int pin)
    {
      // Das Element wird direkt erzeugt und nicht erst lokal
      // erzeugt und dann kopiert. Aber selbst mit Zustand in LED 
      // wäre das kopieren kein Problem gewesen.
      LEDList.emplace_back(pin);
    }

    // Referenz zurückgeben auf das Element im Vektor. Damit 
    // wirkt sich eine Zustandsänderung auf das Element im Vektor
    // aus. Bei Kopie würde sich nur die Kopie ändern und das 
    // Element im Vektor bleibt unverändert.
    LED& getChannel(int channel)
    {
      // Wenn Channel außerhalb von LEDList liegt, dann ist das 
      // Verhalten undefiniert.
      return LEDList[channel];
    }

[Marcus W. (marcusaw)]

Das war auch schon mein ganzes Problem. Ich hab das & reingesetzt und 
schon funktioniert alles wie ich will.

Der ESP8266 hat total verückt gespielt, als ich das blinken auf 
non-blocking umgestellt habe, weil der vector 1000x umkopiert wurde.
Mir ist es dann total um die Ohren geflogen und "nowTime-lastTime >= 
2000" bzw "(240 - 0) >= 2000" hat aufeinmal 'true' ergeben.

Ein verdammtes Zeichen killt mir den ganzen Code! :)
Danke für die Hilfe. Die Initialisierung hab ich inzwischen auch 
umgestellt.

[Marcus W. (marcusaw)]

LED *getChannel(int channel)
{
  return &LEDList[channel];
}

// und dann 
myController.getChannel[0]->blink();

gegen

LED &getChannel(int channel)
{
  return LEDList[channel];
}

myController.getChannel[0].blink();

Was ist eigentlich "besser", bzw wo liegt der Unterschied?

[M.K. B. (mkbit)]

Marcus W. schrieb:
> Was ist eigentlich "besser", bzw wo liegt der Unterschied?

Für den Prozessor ist es kein Unterschied.

Bei einer Referenz ist der Zugriff immer gültig. Ein Pointer kann auch 0 
sein. Als Anwender der Funktion kann ich die Referenz einfach verwenden, 
den Pointer sollte ich prüfen.
Der Pointer ist sinnvoll, wenn du auch den Wert ungültig zurückgeben 
willst, z.B. bei einem Index, der ungültig ist.

Marcus W. schrieb:
> Der ESP8266 hat total verückt gespielt, als ich das blinken auf
> non-blocking umgestellt habe, weil der vector 1000x umkopiert wurde.

Das kann ich aus deinem Code so nicht schließen. Es wird ja in LED nur 
der int member kopiert. Im Fall der Referenz wir im Prinzip ein Pointer 
auf die LED, der normalerweise auch so groß wie ein int ist, erzeugt. 
Der Vektor wird niemals kopiert. Außerdem liegt beides auf dem Stack. 
Das dürfte normalerweise keinen Unterschied machen, außer es ist ein 
Debugbuild der beim Kopieren von Objekten noch mehr Checks einbaut.

[Marcus W. (marcusaw)]

Ich hatte das folgendermaßen:

class LED 
{ 
  
  private: 
    int       _pin;
    uint8_t   _state = LED_OFF; 
    
    unsigned long _last = 0;
    char _msg[50];

  public:
    enum { LED_OFF = LOW, LED_ON = HIGH };

    //LED (int pin):pin(pin),state(false){}
   
    LED (int pin)
    {
      _pin = pin;

      pinMode(_pin, OUTPUT);
      digitalWrite(_pin, _state);
    }
        
    void fade()
    {
      for (int i=0; i<_maxValue; i++)
      {
        analogWrite(_pin,i);
      }
    }

    void blink(int duration=1000)
    {
      unsigned long _now = millis();

      if (_now - _last >= duration)
      {
        Serial.print(_now - _last);
        Serial.print(":");

        _last = _now;
        _state = _state==LED_OFF ? LED_ON : LED_OFF;
        digitalWrite(_pin, _state);

        Serial.print(_now - _last);
        Serial.print(":");
      }
      
    }
};

LEDController Klasse:

class LEDController
{
  private:
    std::vector<LED> LEDList;
  
  public:
    LEDController()
    {
      LEDList.reserve(5);
    }

    
    void attach(int pin)
    {
      LEDList.emplace_back(pin);
    }

    LED getChannel(int channel)
    {
      return LEDList[channel];
    }
}

Wenn ich das so aufgespielt hab, triggert er im Millisekundentakt und 
printed mir dann nonsense wie

0:0 1:0 3:0 5:0 240:0 [...]

Im Moment wo (_now - _last) > 1000 ist, sollte er überhaupt nicht zum 
print kommen...

Setzte ich den Pointer, funktioniert alles wie erwartet.

[Christoph M. (mchris)]

Vielleicht wäre das noch was für's Thema:
https://learn.adafruit.com/multi-tasking-the-arduino-part-1/a-classy-solution

[äxl (Gast)]

Christoph M. schrieb:
> Vielleicht wäre das noch was für's Thema:
> https://learn.adafruit.com/multi-tasking-the-arduino-part-1/a-classy-solution

... danke (hab mitgeesen) ...

[dino (Gast)]

#include <vector>

woher kommt der? daher?:
https://github.com/mike-matera/ArduinoSTL

[Eric B. (beric)]

Ich würde das blinken dem Controller überlassen. Die LED kann ja nur ein 
oder aus sein. Das macht es auch einfacher verschiedene LEDs 
gleichzeitig blinken zu lassen.

[Eric B. (beric)]

Etwa so wie im Anhang.

EDIT: Ah, es fehlt den '#include <Arduino>' am Anfang, aber sonst müsste 
es kompilieren & laufen