LED Sequencer and an apparent timing issue (delays appear to solve the problem)

All-

So I'm working on LED Sequencer firmware for my Lilypad clone with an on-board TLC5940. The idea is that the user creates "banks" of one (or more) LEDs and an interval for each bank. At the desired time, the bank will turn on (or off) as needed.

These banks have already been defined in EEPROM (this is a stripped-down version of my code; the actual software supports a "program mode" where it will allow the user to configure the EEPROM serially via a Visual Basic .net program) and are read in at run-time.

I setup a pattern where I defined 6 banks of 2 LEDs, each firing at 1/2, 1, 2, 4, 8, and 16 seconds (a 6bit binary counter). The interval is defined in 1/10 seconds, with a max of 9999 (16 minutes and some change).

THE PROBLEM: The first bank turns on, but doesn't turn off after 1/2 second. It only blinks for 1/2 second every 10 seconds or so (if that). The other 5 banks work fine.

THE (APPARENT) SOLUTION: I added debugging statements to figure out why it didn't work and guess what - the problem disappeared!

Finally I messed around with various combinations to discover that a "delay(2)" in the addEvent method seems to be the easiest and solution with less impact (adding the debugging code throws off the animation).

So uncommenting the define for EFFIN_BUGGY_CODE OR DEBUG_CHECK will cause the problem to disappear.

Also if I change the delay at the bottom of loop() to 25 millis or more the problem doesn't appear either (but the animation isn't as smooth).

Basically adding sufficient debug code to see what is going on causes the problem to disappear.

So I'm assuming this is some sort of timing issue but it could be my buggy code, hence the reason for my post.

First up, the main program

// Simple LED Sequencer code
#include "Events.h"
#include <Tlc5940.h>
#include <EEPROM.h>

#define MAX_BRIGHTNESS      40               // max LED brightness
#define EEPROM_BASE        100               // Start of bank data
#define SIZE_OF_EEPROM       4               // size of bank data in EEPROM

Events sequenceList;                         // event queue

void setup() {    
  int bankOffset = 0;
 
  Serial.begin(9600); 
  
  sequenceList.init();                       // Initialize event list

  // Fetch data from EEPROM.  Start w/ number of banks
  int numberBanks = EEPROM.read(EEPROM_BASE);

  // For each bank, pull sequence info 
  for (int indx=0; indx < numberBanks; indx++) {
    // Pull the data for each bank from the EEPROM
      
    // First Determine the offset into EEPROM
    int spot   = EEPROM_BASE + 1 + (SIZE_OF_EEPROM * indx);

    int bankNo     = EEPROM.read(spot);
    int numberLEDs = EEPROM.read(spot + 1);

    // handle interval a bit differently since it's really an integer 
    int interval   = EEPROM.read(spot + 2) << 8;       // high byte
        interval  += EEPROM.read(spot + 3);            // low byte

    // Now add these entries into the event list and let's go!
    sequenceList.addEvent( interval, bankNo, bankOffset, numberLEDs );
      
    // adjust bank offset
    bankOffset += numberLEDs;
  }

  // Initialize TLC5940 and turn all channels off
  Tlc.init(0);  
}

// Main program loop
void loop() {
  // Pretty much idle here waiting for the next event to expire.  We toggle
  // the LED state, delete the event, and re-add it with a new eventTime.
  if (sequenceList.checkEvent()) {
    // Timer expired!  Extract details
    int bankNumber = sequenceList.getBankNumber();
    int bankOffset = sequenceList.getBankOffset();
    int interval   = sequenceList.getInterval();
    int numberLEDs = sequenceList.getNumberLEDs();
    boolean lights = false;

    // Now delete and re-add the event back so we can keep things on schedule
    sequenceList.deleteEvent();
    sequenceList.addEvent(interval, bankNumber, bankOffset, numberLEDs);

    // Check first LED in bank to see if we need to turn everything off or on
    if (Tlc.get(bankOffset) == 0)
      lights = true;

    // Update the bank as needed
    for (int indx = bankOffset; indx < (bankOffset + numberLEDs); indx++) {
      if (lights)
        Tlc.set(indx, MAX_BRIGHTNESS);
      else
        Tlc.set(indx, 0);
    }

    // Update the display
    Tlc.update();
  }
  else {
    // Let's kill some time here 
    delay(2);
  }
}

Here is the included "Events" class.

// Events 
// Simple event handler for LED sequencer
#include <Wprogram.h>

// Total number of events possible 
#define MAX_EVENTS 16

class Events {
private:
  // Event structure
  typedef struct {
    unsigned long eventTime;                     // Event deadline
    int interval;                                // Actual event time period 
    int bankNumber;                              // bank number
    int bankOffset;                              // bank offset (0-16)
    int numberLEDs;                              // Total number of LEDs in bank
  } event;

  event eventList[MAX_EVENTS];                   // Current event list

  int numberEvents;                              // Number events in list
public:
  void init(void);                               // initialize the object

  // Add an event to the list
  boolean addEvent(int interval,
                   int bankNumber,
                   int bankOffset,
                   int numberLEDs);

  // Check to see if the current event has expired
  boolean checkEvent(void);

  // Fetch event details
  int getInterval(void)       { return eventList[0].interval;} 
  int getBankNumber(void)     { return eventList[0].bankNumber;} 
  int getBankOffset(void)     { return eventList[0].bankOffset;}
  int getNumberLEDs(void)     { return eventList[0].numberLEDs;}

  // How many events are in the queue?
  int getNumberEvents(void)  {  return numberEvents;}

  // Delete the current event
  boolean deleteEvent(void);
  
  void dumpEvents(void);
};

// 
// Class implementation follows
//

// Initialize object
void Events::init(void) {
  numberEvents      = 0;
}

// Add an event to the queue (ok, it's an array).  Note this will insert items
// in chronological order of expiration
boolean Events::addEvent(int interval, int bankNumber, int bankOffset, int numberLEDs) {
  int indx = 0;
  boolean found = false;
  boolean done = false;
  unsigned long eventTime = millis() + (unsigned long)interval * 100UL;

  // Make sure there is room at the inn
  if (numberEvents == MAX_EVENTS)
    return false;

#define EFFIN_BUGGY_CODE 1
#ifdef EFFIN_BUGGY_CODE
  // I can't figure out how to make this work consistently without a small delay
  // here. 
  delay(2);
#endif

  // Locate the position in the array
  for (indx = 0; (indx < numberEvents) && !found; indx++) {
    found = (eventTime < eventList[indx].eventTime);
  }

  if (found) {
    // Found a spot in the array.  Move the existing events up one spot

    // First adjust indx (since it will be off by one)
    indx--;

    // Now move items up one to leave a spot
    for (int spot=numberEvents; spot > indx; spot--) {
      eventList[spot] = eventList[spot - 1];
    }

    // Now insert the new event
    eventList[indx].interval         = interval;
    eventList[indx].eventTime        = eventTime;
    eventList[indx].bankNumber       = bankNumber;
    eventList[indx].bankOffset       = bankOffset;
    eventList[indx].numberLEDs       = numberLEDs;
  }
  else {
    // didn't find it.  Simply add to the end of the list
    eventList[numberEvents].interval   = interval;
    eventList[numberEvents].eventTime  = eventTime;
    eventList[numberEvents].bankNumber = bankNumber;
    eventList[numberEvents].bankOffset = bankOffset;
    eventList[numberEvents].numberLEDs = numberLEDs;
  }

  // Bump event counter
  numberEvents++;

  return true;
}

// Check to see if the current event has expired
boolean Events::checkEvent(void) {
  if (numberEvents == 0)
    return false;

//#define DEBUG_CHECK 1
#ifdef DEBUG_CHECK
  if (millis() >= eventList[0].eventTime) {
    if (eventList[0].bankNumber == 1) {
      Serial.print("expired @ ");
      Serial.println(millis());
    }
  }
#endif  
  return (millis() >= eventList[0].eventTime);
}

// Delete the current event
boolean Events::deleteEvent(void) {
  // make sure the list is populated
  if (numberEvents == 0)
    return false;

  // If there was only one we are done here
  if (--numberEvents == 0)
    return true;

  // Move the rest down a bit
  for (int indx=1; indx <= numberEvents; indx++) {
    eventList[indx - 1] = eventList[indx];
  }

  return true;
}

Ideas? Things I can do better? So far, the only thing I've thought of to improve the code's speed would be to keep the current LED state in the event structure (since I'm sure it's faster to pull out a boolean and flip it than to query the TLC5940 over SPI).

Thanks in advance,

Brad (KF7FER)

PS Oh yeah, I've tried this with faster animations (1/10, 1/5, 2/5, 4/5, 1.6, and 3.2 seconds) as well as slower (1,2, 4, 8, 16, and 32 seconds) and the problem persists.

I'd suggest that some of your names are misleading:

#define SIZE_OF_EEPROM       4               // size of bank data in EEPROM

My EEPROM is much larger than that. SIZE_OF_BANK might be a better name.

    int spot   = EEPROM_BASE + 1 + (SIZE_OF_EEPROM * indx);

spot? How about startAddress?

Your class should have a constructor that also calls init(), so that you can be assured that init() gets called.

You deleteEvent() method always removes the last event. I don't like that design. It should be able to remove the nth event.

Your checkEvents() method returns true if any event occurred. It should return -1 for no event, and n for the nth event.

Your other methods assume that the first event in the list is the event that fired.

Really, I think your problem is in the Events class. You need to know which event occurred, so that you can get the data for that event. Deleting the event, and re-adding it, is silly. Just add a method to reset the nth event's eventTime value.

PaulS:
I'd suggest that some of your names are misleading:

I agree. I'll have to work on that.

Your class should have a constructor that also calls init(), so that you can be assured that init() gets called.

Of course (he says after doing a bit of reading I should have done before on the library page). Thanks!

You deleteEvent() method always removes the last event. I don't like that design. It should be able to remove the nth event.

Actually it's the first event that gets removed (indx=0), but I get your point.

Your checkEvents() method returns true if any event occurred. It should return -1 for no event, and n for the nth event.

Actually it only returns true if the first event has occurred.

Your other methods assume that the first event in the list is the event that fired.

Well, the implementation may be a bit naive, but the list is maintained in ascending chronological order so it will always be the first event that fired.

The idea was to make it inexpensive to check to see if an event has expired (you'll do that many times per second) and moving the "cost" to the add since you'll be doing that less often.

I do see a danger of too deep a list and some of the end events never being serviced but I think that won't typically be an issue given the limits on event time (1/10 second) and the fact that there isn't much need to have more than a few events firing more than once per second.

Really, I think your problem is in the Events class. You need to know which event occurred, so that you can get the data for that event. Deleting the event, and re-adding it, is silly. Just add a method to reset the nth event's eventTime value.

In retrospect, I agree about deleting and re-adding it. No need to physically maintain chronological order when I could simply use a pointer in each structure and maintain that in the proper order. One thought behind delete and re-adding was that perhaps the new event being added would be different than the previous event (or not even needed).

I should modify deleteEvent to re-order the event back into the list when the event fires (and possibly re-name the method since it isn't deleting anymore).

Thanks for the help,

Brad (KF7FER)

EDIT: Was a stupid idea to do the re-schedule in checkEvent since I have to pull the data first