Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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 <Arduino.h>

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))) {
    memset(ledStates, 0x00, length);
    ledCount = length;
  } else {  // malloc failed
    ledCount = 0;
  }
}

void HypnoDisc::latchDown(void) {
  // Starts the
  digitalWrite(latchPin, LOW);
}

void HypnoDisc::latchUp(void) {
  // Stop clocking in data
  digitalWrite(latchPin, HIGH);
}

void HypnoDisc::addLight(void) {
  // Adds a light at the start of the array
  ledStates[0] = pwmMaxLevel;
}

boolean HypnoDisc::allDotsLanded(void) {
  // Indicates that all dots are stacked at the end of the array.
  // This is also true for a completely empty array.
  boolean endOfLanded = false;
  for (byte i = ledCount; i-- > 0;) {
    if (ledStates[i] < pwmMaxLevel) {
      endOfLanded = true;
    } else if (ledStates[i] > 0 && endOfLanded) {
      return false;
    }
  }
  return true;
}

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;
}

boolean HypnoDisc::discFull(void) {
  // Indicated whether the disc is fully filled with active dots.
  for (byte i = ledCount; i-- > 0;) {
    if (ledStates[i] < pwmMaxLevel) return false;
  }
  return true;
}

void HypnoDisc::clockwiseDrop(void) {
  // Spins the output LEDs clockwise.
  // Lights at the end of the array remain there, and additional Lights that
  // fall down are added to a growing arc of max-brightness LEDs.
  byte current, next;
  for (current = ledCount; --current > 0;) {
    next = current - 1;
    if (ledStates[current] < pwmMaxLevel) {
      ledStates[current] = ledStates[next];
      ledStates[next] >>= 1;
    } else if (ledStates[next] < pwmMaxLevel) {
      ledStates[next] >>= 1;
    }
  }
}

void HypnoDisc::clockwiseSpin(void) {
  // Spins the output LEDs clockwise.
  // Lights at the end of the array are moved to the start, making them go round.
  byte current, next;
  byte lastValue = ledStates[ledCount - 1];
  for (current = ledCount; --current > 0;) {
    next = current - 1;
    ledStates[current] = ledStates[next];
    ledStates[next] >>= 1;
  }
  ledStates[0] = max(ledStates[0], lastValue);
  ledStates[ledCount - 1] = max(ledStates[ledCount - 1], lastValue >> 1);
}

void HypnoDisc::clockwiseWipe(void) {
  // Spins the output LEDs clockwise.
  // Lights at the end of the array are moved off the disc entirely.
  byte current, next;
  for (current = ledCount; --current > 0;) {
    next = current - 1;
    ledStates[current] = ledStates[next];
    ledStates[next] >>= 1;
  }
}

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 < ledCount / 8; i++) {
    for (j = 0; j < 8; j++) {
      bitWrite(shiftData, j, (ledStates[i * 8 + j] > pwmStep));
    }
    shiftOut(dataPin, clockPin, LSBFIRST, shiftData);
  }
  latchUp();
  pwmStep = ++pwmStep % pwmMaxLevel;
}

HypnoDisc.ino

/* Flying dots on a circular board */
#include "hypno.h"

const byte updateInterval = 30;
const byte ringSize = 24;

HypnoDisc disc = HypnoDisc(ringSize);

void setup() {
  disc.begin();
}

void loop() {
  // Creates a set of
  byte divisors[] = {12, 8, 6};
  for (byte div = 0; div < sizeof(divisors); div++) {
    for (byte i = disc.ledCount * 4; i-- > 0;) {
      if (i % divisors[div] == 0) {
        disc.addLight();
      }
      disc.clockwiseSpin();
      updateFor(updateInterval);
    }
    wipe();
  }
}

void updateFor(byte timeout) {
  // Update (PWM) the outputs without movement for `timeout` milliseconds.
  long currentMillis = millis();
  while (millis() - currentMillis < timeout) {
    disc.updateLights();
  }
}

void wipe(void) {
  // Wipe the output disc clean one step at a time.
  while (!disc.discEmpty()) {
    disc.clockwiseWipe();
    updateFor(updateInterval);
  }
}
share|improve this question
    
I am going to have to get back into Arduino again! I miss this stuff –  Malachi Oct 31 '13 at 15:04

1 Answer 1

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<byte>, 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<byte> 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 < ledCount / 8; i++) {
    for (j = 0; j < 8; j++) {
      bitWrite(shiftData, j, (ledStates[i * 8 + j] > 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<ledCount; i++)
        for (byte j=0; j<8; j++)
            bitWrite(shiftData, j, (ledStates[i*8+j] > 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.

share|improve this answer
    
That's a lot of useful pointers, thanks Jerry. The use of vector<byte> makes things a lot nicer and the initializer list makes things better. That has allowed me to make the pins used for the disc const as well. Unfortunately, allof is not implemented in either the Arduino stdlib or the StandardCPlusPlus library, so it now uses an iterator and some inline code. –  Elmer Mar 13 at 21:52
    
I'm not sure how you meant the struct latch block to work. putting the latch on the class HypnoDisc the compiler complained a lot about using non-static members. I currently have a method HypnoDisc::toggleLatch() that returns an instance of my struct latch. The latch is initialized with the latchPin and the ctor/dtor toggle the pin state. Is there a nicer way of doing that? –  Elmer Mar 13 at 21:55

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.