Feedback on Arduino class for LED circle animations

I'm fairly new to Arduino and C++ in general, coming from a heavy Python background. The code below is functional, but my lack of C++ knowledge is keeping me from spotting any errors in style, idioms and other good practices.

Not all of the library code is used by the example, as I tried to keep that to the minimum (the functions there are simply more of the same).

While this is one of my first steps into embedded programming, please point out all that you believe is wrong or need improving. I'd rather know what to improve than continue with a false sense of accomplishment :-)

hypno.h

#include 

class HypnoDisc {
  public:
    HypnoDisc(
        byte ledCount,
        byte pwmLevels = 5,
        byte latch = 7,
        byte clock = 6,
        byte data = 5);
    void
      begin(void),
      addLight(void),
      clockwiseDrop(void),
      clockwiseSpin(void),
      clockwiseWipe(void),
      updateLights(void);
    boolean
      allDotsLanded(void),
      discEmpty(void),
      discFull(void);
    byte
      ledCount;

  private:
    byte
      clockPin, latchPin, dataPin,
      pwmMaxLevel,
      pwmStep,
      *ledStates;
    void
      latchDown(void),
      latchUp(void),
      setLength(byte length);
};

hypno.cpp

```
#include "hypno.h"

HypnoDisc::HypnoDisc(byte ledCountInit, byte pwmLevels, byte latch, byte clock, byte data) {
setLength(ledCountInit);
pwmMaxLevel = pow(2, (pwmLevels - 1));
pwmStep = 0;
latchPin = latch;
clockPin = clock;
dataPin = data;
}

void HypnoDisc::begin(void) {
// Initiates the disc and sets the output to all-dark
pinMode(latchPin, OUTPUT);
pinMode(clockPin, OUTPUT);
pinMode(dataPin, OUTPUT);
updateLights();
}

void HypnoDisc::setLength(byte length) {
// Sets the number of output LEDs on the controller, so that animations on the
// disc run as expected (with the wrap where it should be).
if (ledStates != NULL) {
free(ledStates);
}
if (NULL != (ledStates = (byte *)malloc(length))) {

I'm going to preface this with the fact that I'm not quite sure what (if any) restrictions the Arduino may impose on the C++ you can use. Since you've tagged it as C++, I'm going to assume you can use C++ as you would on any other platform.

void
  begin(void),
  addLight(void),
  clockwiseDrop(void),
  clockwiseSpin(void),
  clockwiseWipe(void),
  updateLights(void);

I don't like this at all. I'd (strongly) prefer:

void begin();
void addList():
void clockwiseDrop();
void clockwiseSpin();
void clockwiseWipe():
void updateLights();

This isn't (at least according to the tag), so the (void) in the parameter list to specify that they take no parameters is entirely unnecessary.

HypnoDisc::HypnoDisc(byte ledCountInit, byte pwmLevels, byte latch, byte clock, byte data) {
  setLength(ledCountInit);
  pwmMaxLevel = pow(2, (pwmLevels - 1));
  pwmStep = 0;
  latchPin = latch;
  clockPin = clock;
  dataPin = data;
}

It looks like these should be handled in a member initializer list instead of assignments in the body of the ctor. The one that initially looks like an exception is the call to setlength, but from the looks of things, that should really be as well--but with ledcount and ledstates replaced by a std::vector, at which point it can be initialized in the initializer list, just like the rest. For the moment, I'll assume you've done this.

HypnoDisc::HypnoDisc(byte ledCount, byte pwmLevels, byte latch, byte clock, byte data) 
   : ledstates(ledcount), 
     pwmMaxLevel(1 << pwmLevels), 
     pwmStep(0), 
     latchPin(latch),
     clockPin(clock)
     dataPin(data)
{ }

As mentioned above, setLength:

void HypnoDisc::setLength(byte length) {
  // Sets the number of output LEDs on the controller, so that animations on the
  // disc run as expected (with the wrap where it should be).
  if (ledStates != NULL) {
    free(ledStates);
  }
  if (NULL != (ledStates = (byte *)malloc(length))) {
    memset(ledStates, 0x00, length);
    ledCount = length;
  } else {  // malloc failed
    ledCount = 0;
  }
}

...should be replaced with something like std::vector ledStates;. If you can't use std::vector, you should still really do a vector-like class of your own instead of trying to embed its functionality into the parent class.

boolean HypnoDisc::discEmpty(void) {
  // Indicates whether the disc is completely empty (trails count as non-empty).
  for (byte i = ledCount; i-- > 0;) {
    if (ledStates[i] > 0) return false;
  }
  return true;
}

Assuming the previous modification, this can be done with:

return std::allof(ledStates.begin(), ledStates.end(), [](byte b) {return b == 0; });

Likewise with discFull.

void HypnoDisc::updateLights(void) {
  // Writes out the current LED-states with simple software PWM
  // This should be called often for good brightness control (>500Hz ideally)
  latchDown();
  byte i, j, shiftData;
  for (i = 0; i  pwmStep));
    }
    shiftOut(dataPin, clockPin, LSBFIRST, shiftData);
  }
  latchUp();
  pwmStep = ++pwmStep % pwmMaxLevel;
}

Based on the usage pattern, I think I'd move latchUp and latchDown into the ctor and dtor of a separate class:

struct latch {
   latch() { /* same code as your current `latchDown` */ }
   ~latch() { /* same as your current `latchUp */ }
};

Then updateLights simplifies a little bit to:

void HypnoDisc::updateLights() {
    latch l;
    for (byte i=0; i pwmStep));
}

It's also worth at least considering writing a little byteWrite function that handles calling bitWrite in a loop for an entire byte's worth of data.

StackExchange Code Review Q#19148, answer score: 2