blink-without-delay slowing things by factor 10 ?

Using Arduino LEDs and Multiplexing
1

The code below works in conjunction with my High Power RGB LED shield http://ledshield.wordpress.com. The shield is connected to the Arduino compatible Teensy3 per I2C bus.
The core routine is an adapted 3D Bresenham algorithm to with the three coordinates being red, green, blue. each time through the main loop the algorithm advances one pixel in 3D RGB space (I guess in that case it's really a voxel ). I apologize for the lack of commention/documenting. This is still under heavy development.

When out-commenting the blink-without-delay code marked as such in the main loop (at the very end) the fading algorithm runs substantially faster than with it enabled (the value for "interval" is set to 0). The rough measurement for an 8-bit fade from r,g,b 0,0,0 --> 255,255,255 appears to be dependent on which frequency I am running the I2C bus with. Going from 100kHz to 2.4MHz results in a 10x increase. When the blink-without-delay code is enabled there is no increase in execution speed beyond 4ookHz.

Is there an alternative method to mills() or am I using this incorrectly or inefficiently ?

Without blink-without-delay code

100 kHz --> 337 ms
400 kHz --> 97 ms
600 kHz --> 75 ms
800 kHz --> 58 ms
1000 kHz --> 53 ms
1200 kHz --> 47 ms
1500 kHz --> 42 ms
2000 kHz --> 36 ms
2400 kHz --> 33 ms

with blink-without-delay

100kHz --> 337 ms
400kHz --> 256 ms
600kHz --> 256 ms
800kHz --> 256 ms
1000kHz --> 256 ms
1200kHz --> 256 ms
1500kHz --> 256 ms
2000kHz --> 256 ms
2400kHz --> 256 ms

#include "i2c_t3.h"
#include "HPRGB2.h"

long interval = 0;
long previousMillis = 0;
long previousMillisTimer = 0;
int current;

class RGBFader: 
public HPRGB {
public:
  RGBFader (uint8_t mcp4728ID = 0x00, uint8_t pca9685ID = 0x00);
  void randomLineInit(void);
  void randomLine(void);
  void rgbFade(uint8_t x1, uint8_t y1, uint8_t z1,uint8_t x2,uint8_t y2,uint8_t z2);
  
private:
  int16_t  xd, yd, zd;
  uint8_t  x, y, z;
  uint16_t ax, ay, az;
  int8_t   sx, sy, sz;
  int16_t  dx, dy, dz;
  uint8_t  x1, y1, z1;
  uint8_t  x2, y2, z2;
  uint8_t  r, g, b;
  boolean newLine;
  boolean (RGBFader::*pLine)();
  uint16_t MAX(uint16_t a, uint16_t b);
  uint16_t ABS(int16_t a);
  int8_t ZSGN(int16_t a);
  boolean lineXdominant(void);
  boolean lineYdominant(void);
  boolean lineZdominant(void);
};

RGBFader::RGBFader(uint8_t mcp4728ID, uint8_t pca9685ID)
{
  _mcp4728ID = mcp4728ID;
  _mcp4728_address = (MCP4728_BASE_ADDR | _mcp4728ID);
  _pca9685ID = pca9685ID;
  _pca9685_address = (PCA9685_BASE_ADDR | _pca9685ID);
}

/* find maximum of a and b */
inline uint16_t RGBFader::MAX(uint16_t a, uint16_t b) { 
  return (a > b) ? a : b;
}

/* absolute value of a */
inline uint16_t RGBFader::ABS(int16_t a) { 
  return (a < 0) ? -a : a;
}

/* take sign of a, either -1, 0, or 1 */
inline int8_t RGBFader::ZSGN(int16_t a) { 
  return (a < 0) ? -1 : a > 0 ? 1 : 0;
} 

void RGBFader::randomLineInit(void){

    uint8_t rand;
    rand = random (1, 4);
    
    x1 = 0;
    y1 = 0;
    z1 = 0;
    
    if (rand==1) { x2=255; } else { x2= random(0, 256); };
    if (rand==2) { y2=255; } else { y2= random(0, 256); };
    if (rand==3) { z2=255; } else { z2= random(0, 256); };

    x = x1;
    y = y1;
    z = z1;
    
    dx = x2 - x1;
    dy = y2 - y1;
    dz = z2 - z1;
      
    ax = ABS(dx) << 1;
    ay = ABS(dy) << 1;
    az = ABS(dz) << 1;

    sx = ZSGN(dx);
    sy = ZSGN(dy);
    sz = ZSGN(dz);

    if (ax >= MAX(ay, az)){                 /* x dominant */
      yd = ay - (ax >> 1);
      zd = az - (ax >> 1);
      pLine = &RGBFader::lineXdominant;
    }
    else if (ay >= MAX(ax, az)){            /* y dominant */
      xd = ax - (ay >> 1);
      zd = az - (ay >> 1);
      pLine = &RGBFader::lineYdominant;
    }
    else if (az >= MAX(ax, ay)){            /* z dominant */
      xd = ax - (az >> 1);
      yd = ay - (az >> 1);
      pLine = &RGBFader::lineZdominant;
    }
}

void RGBFader::rgbFade(uint8_t x1, uint8_t y1, uint8_t z1, uint8_t x2, uint8_t y2, uint8_t z2){
    newLine = (this->*pLine) ();
    if (newLine){
      
    unsigned long currentMillis = micros();            
    Serial.println(currentMillis-previousMillisTimer);
    previousMillisTimer = currentMillis;
    
    x = x1;
    y = y1;
    z = z1;

    dx = x2 - x1;
    dy = y2 - y1;
    dz = z2 - z1;

    ax = ABS(dx) << 1;
    ay = ABS(dy) << 1;
    az = ABS(dz) << 1;

    sx = ZSGN(dx);
    sy = ZSGN(dy);
    sz = ZSGN(dz);

    if (ax >= MAX(ay, az)){                 /* x dominant */
      yd = ay - (ax >> 1);
      zd = az - (ax >> 1);
      pLine = &RGBFader::lineXdominant;
    }
    else if (ay >= MAX(ax, az)){            /* y dominant */
      xd = ax - (ay >> 1);
      zd = az - (ay >> 1);
      pLine = &RGBFader::lineYdominant;
    }
    else if (az >= MAX(ax, ay)){            /* z dominant */
      xd = ax - (az >> 1);
      yd = ay - (az >> 1);
      pLine = &RGBFader::lineZdominant;
    }
  }
}

void RGBFader::randomLine(void){
  newLine = (this->*pLine) ();   
  
    if (newLine){
    int rand;
    rand = random (1, 4);
    
    x1 = x2;
    y1 = y2;
    z1 = z2;
    
    if (rand==1) { x2=0; } else { x2= random(0, 256); };
    if (rand==2) { y2=0; } else { y2= random(0, 256); };
    if (rand==3) { z2=0; } else { z2= random(0, 256); };

    x = x1;
    y = y1;
    z = z1;

    dx = x2 - x1;
    dy = y2 - y1;
    dz = z2 - z1;

    ax = ABS(dx) << 1;
    ay = ABS(dy) << 1;
    az = ABS(dz) << 1;

    sx = ZSGN(dx);
    sy = ZSGN(dy);
    sz = ZSGN(dz);

    if (ax >= MAX(ay, az)){                 /* x dominant */
      yd = ay - (ax >> 1);
      zd = az - (ax >> 1);
      pLine = &RGBFader::lineXdominant;
    }
    else if (ay >= MAX(ax, az)){            /* y dominant */
      xd = ax - (ay >> 1);
      zd = az - (ay >> 1);
      pLine = &RGBFader::lineYdominant;
    }
    else if (az >= MAX(ax, ay)){            /* z dominant */
      xd = ax - (az >> 1);
      yd = ay - (az >> 1);
      pLine = &RGBFader::lineZdominant;
    }
  }
}

boolean RGBFader::lineXdominant(void){
 
  goToRGB(x, y, z);
  if (x == x2)
  {
    return 1;
  }
  if (yd >= 0)
  {
    y += sy;
    yd -= ax;
  }
  if (zd >= 0)
  {
    z += sz;
    zd -= ax;
  }
  x += sx;
  yd += ay;
  zd += az;
  return 0;
}


boolean RGBFader::lineYdominant(void){
    
  goToRGB(x, y, z);
  if (y == y2){
    return true;
  }
  if (xd >= 0){
    x += sx;
    xd -= ay;
  }
  if (zd >= 0){
    z += sz;
    zd -= ay;
  }
  y += sy;
  xd += ax;
  zd += az;
  return false;
  
}

boolean RGBFader::lineZdominant(void){
    
  goToRGB(x, y, z);
  if (z == z2)
  {
    return true;
  }
  if (xd >= 0)
  {
    x += sx;
    xd -= az;
  }
  if (yd >= 0)
  {
    y += sy;
    yd -= az;
  }
  z += sz;
  xd += ax;
  yd += ay;
  return false;

}

RGBFader ledOne;// default mcp4728 id(0) and default PCA9685 id(0)

void setup()
{
  Serial.begin(115200);
   
  ledOne.begin();
  ledOne.setCurrent(100,100,100); // set maximum current for channel 1-3 (mA)
  ledOne.setFreq(330);            // operation frequency of the LED driver (KHz)
  ledOne.setPWMFrequency(120);
  ledOne.eepromWrite();           // write current settings to EEPROM
  delay(100);                     // wait for EEPROM writing
  
  
//test current-settings read function  
  Serial.print("Max current - channel one   :");
  Serial.println(ledOne.getCurrent(1));
  Serial.print("Max current - channel two   :");
  Serial.println(ledOne.getCurrent(2));
  Serial.print("Max current - channel three :");
  Serial.println(ledOne.getCurrent(3));
  
//test current-switch-frequency read function
  Serial.print("Operating Frequency :");
  Serial.println(ledOne.getFreq());
}

void loop()
{
  unsigned long currentMillis = millis();            // if out commented
  if(currentMillis - previousMillis > interval) {    // code runs
    previousMillis = currentMillis;                     // substantially  
    ledOne.rgbFade(0,0,0,255,255,255);
  }                                                             // faster
}
2

Wondering: Did you really intend to use micros() here?

   unsigned long currentMillis = micros();

-br

3

Well..I did intend to use micros() at some point just to see if it would make any difference, but it did not, so I have changed it back already.

What does however seem to be slowing down the process is this line:

previousMillis = currentMillis;

There seem to be two reasons for that.

  1. Both are of a different type "unsigned long" and "long"
  2. They are 32 bit values

Changing the unsigned long to a long - while not the end solution - already showed improved execution time. My idea is to use pointers instead of copying bytes. I yet have to try that out.

4

have you timed how long this call takes - ledOne.rgbFade(0,0,0,255,255,255);

the millis() stuff is used correctly and takes less than 1 ms to check.

void RGBFader::rgbFade(uint8_t x1, uint8_t y1, uint8_t z1, uint8_t x2, uint8_t y2, uint8_t z2){
newLine = (this->*pLine) ();
if (newLine){

unsigned long currentMillis = micros();
Serial.println(currentMillis-previousMillisTimer);
previousMillisTimer = currentMillis;

these 3 lines are not needed and serial output is relative slow (depends on baudrate)

5

The measurement values I provided in the starting post are for a complete fade using the ledOne(0,0,0,255,255,25) 256 times to cross the color cube from all-off to all-on. So I have not measured the speed of the routine directly e.g. by switching a digital pin on-off and using my oscilloscope but for comparative purposes its good enough.

The three lines you marked as "not needed" do just that.

However, I believe that really does not answer the question why eliminating/out-commenting that one line of code:

previousMillis = currentMillis;

Speeds up the code by several factors.

Even more puzzling is that I use this same code in the rgbFade Method and it does not seem to have the same effect there. the only difference is that I don't use it within the if(){} structure.

6

This issue would be a lot easier to understand if the code were a whole lot simpler. In the interests of simplifying it, here's the "Blink Without Delay" sketch from the examples in 1.5.2, without most of the comments:

const int ledPin =  13;         // the number of the LED pin
int ledState = LOW;             // ledState used to set the LED
long previousMillis = 0;        // will store last time LED was updated
long interval = 1000;           // interval at which to blink (milliseconds)

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

void loop()
{
  unsigned long currentMillis = millis();
  if(currentMillis - previousMillis > interval) {
    previousMillis = currentMillis;   
    if (ledState == LOW)
      ledState = HIGH;
    else
      ledState = LOW;
    digitalWrite(ledPin, ledState);
  }
}

If this line

    previousMillis = currentMillis;

is deleted, here's what happens:

  • The value of previousMillis is always 0.
  • The conditional evaluates as true when the millis() returns a value of 1 or greater. That may happen the first time through loop(), or later, but not more than 1 millisecond later.
  • After that, the value of currentMillis will always be greater than 1, at least until millis() rolls over to zero, about 50 days from now.
  • The conditional will always evaluate as true, and the LED state will be inverted every time loop() executes - very often indeed.

Visually, the LED will appear to be on continuously, at a lower brightness than it has when it's actually on continuously. Testing this code, that's exactly what I see. When the line is part of the program, the LED blinks on for about a second, off for about a second, and repeats, as expected.

A slower execution speed isn't an unintentional artifact of the "blink without delay" code - that's its purpose. Slower, and with reasonably precise timing.

I won't be poring over your "adapted 3D Bresenham algorithm" to figure out how it works. But, based on the data that you present, I'll bet that the time interval that you're measuring and reporting corresponds to about 256 calls to rgbFade(). If that's so, then I'd interpret your data like this: At I2C speeds of 100kHZ and lower, the delays associated with the I2C operations take longer than a millisecond, and they're limiting the execution speed; at I2C speeds of 400 kHz or more, I2C operations take less than a millisecond, and the intentional delays implemented by the "blink without delay" code, as you call it, become the limiting factor.

An easy way to test this theory is to change the value of interval to, say, 1, and see if the intervals make sense as you change the I2C speed. I'd bet that all of the speeds that you listed would give you intervals of 512 milliseconds.

Headroom:
... am I using this incorrectly ... ?

If you didn't want to wait 1 millisecond between calls to rgbFade(), then I would think, "Yes," and that you probably didn't want to use the "blink without delay" code at all.

7

@tmd,

Thanks! You are absolutely correct and your bullet points explained it very clearly. It pains me that the value of 256 ms really stared me in the face and I still did not get it. I got the same explanation on the Teensy forum using a lot less words :slight_smile:

8

Headroom:
I got the same explanation on the Teensy forum using a lot less words

I think he's trying to say that I'm verbose. If I were that kind of smart aleck, I'd feel compelled to point out that I managed to demonstrate the same behavior with a few hundred fewer lines of code.

But, I'm not a smart aleck, I'm a curmudgeon. So I won't mention it.