optimize code

Hi,
The following sketch is a clock based on 60 LPD6803 RGB led’s. The code has not been stable and that can have something to do with the interrupts. I changed the SQW from 0 (pin2) to 1 (pin3) and that seems to do the trick for now. I commented the sketch and would like you all (…) to have a look. I’m a newbie on programming Arduino and would like to know if I’m doing okay. Please look for inefficient coding and improve my work. I also included the circuit. Thank you!

LPD6803_Clock.pde (5.44 KB)

It helps to have stable hardware first. I notice no decoupling in that circuit. With that many LEDs you defiantly need some.

these shift-registers don't need decoupling

Rubbish, there isn't a active component on the planet that dosn't need decoupling, what makes you say that?

Well I am on my iPad at the moment so looking at code especially as a download isn't easy. I will take a look tomorrow.

What makes me say that is over 40 years in the business has taught me that instability is always down to noise due to lack of decoupling. And that is dealing with professional engineeres who know about decoupling.

Have you measured the noise on your supply with a good scope? If you never use decoupling then you have never produced reliable circuits. You might think they function but you have probbly not tested them sufficiently.

If a circuit changes the ammount of current it draws, it changes the voltage on the power rail due to the finite impedance of the power supply. It is that variation in voltage you need to minimise by using decoupling capacitors. Variations in the power rail voltage lead to intermittent operation.

You can argue about using decoupling caps, or you can use them. Guess which one isn't a waste of time .

As for code you have a else if that looks like this: " else if (h = 12) { h = 0; } // same here but do we need it?"

Certainly you meant h==12 and not h=12.

You never explained what "stable" meant.

I can recommend the use of capacitors. I have 5 logic chips controlling an LCD, a tiny bit of noise on the line caused an output enable to trigger later than expected. My DSO quad set to AC coupling clearly showed the noise on rare occasions causing the rise time to be longer than usual ( noisy peak after setting high ), so the EN was triggered to late which skewed the read results.

Instantly fixed with a capacitor on all 5 chips. Cost about $0.67 to fix the problem.

Well I had a look at the code and I think the problem is that you are spending too much time in the interrupt service routine (ISR).

interrupts(); // start all interupts again. why do we need this???

It is needed to re-enable the interrupts inside an interrupt service routine, so the routine can itself be interrupted. This is a technique that has the potential for disaster because if you don't get out fast enough you could find the stack overflowing. You are calling the I2C routines from the ISR which requires interrupts themselves.

I would take the bulk of the ISR out and just use the ISR to set a flag. Then in the loop() you look at the flag and when you find it set you do the reading of the RTC and updating of the LED states and finally clear the flag.

The resistors marked ? should be 4K7 pull up resistors. These are needed on an I2C bus even though the Arduino enables the internal pull up resistors because these are too high. See:- http://www.dsscircuits.com/articles/effects-of-varying-i2c-pull-up-resistors.html This could be the cause of the ISR taking too long.

I have looked at these strips and they actually contain some small surface mount decoupling capacitors. Therefore the high frequency noise is taken care of. However, I would add some bulk decoupling to the 5V line driving the strip in the order of a 100uF capacitor.

LED circuits are particularly in need of decoupling because of the high currents being switched. Standard logic loads only take a few mA for a few ns to charge up the signal lines, whereas with LEDs you might be switching 50mA, 100mA, 1A or more at logic speeds. Its the fast switching that causes the problem.

Lets look at 100mA of LED load being switched simultaneously from high-speed CMOS. Thats perhaps a switching time of 5 to 10nS, which is a rate-of-change of current of 10 to 20 MILLION amps per second. Even small stray inductances in the PCB layout will lead to large induced voltages at this rate of current change. 10cm of standard PCB trace can be about 0.1uH, which taking that current step will create 2V transient while the current changes. So if you don't have a decoupling capacitor within 10cm of the chip doing the switching its supply will drop (or try to drop) from 5V to 1V (2V drop on each of the supply traces). The chip may malfunction (it might not, but that's luck, not design). I'd suggest caps within 1cm of each chip. 0.1uF ceramic for each chip plus 22uF or more on the board if such large loads are involved.

The decoupling capacitors soak up the extra demand for current at different timescales (0.1uF on the nanosecond level, 22uF on the microsecond level, the power supply itself does the adjustment at slower timescales).

If you have a remote power supply I'd add 100+uF or more to allow for the even slower response of a remote supply.

This is the code rearranged so that the ISR time is minimised as described in my earlier post.
As I don’t have the LPD6803 library I have not managed to compile or test it but it should be close.

// LPD6803 lib from ladyada
#include "LPD6803.h"

// LPD6802 pins
int dataPin = 11;       // 'white' wire
int clockPin = 13;      // 'green' wire
// Don't forget to connect 'black' to ground and 'red' to +5V/3A.

// LDR pin
int LDRPin = A2;
volatile int refreshFlag =0;

// Timer 1 is also used by the strip to send pixel clocks.
#include <TimerOne.h>

// Set the first variable to the NUMBER of pixels.
LPD6803 strip = LPD6803(60, dataPin, clockPin);

//wire and rtc
#include <Wire.h>
#include "RTClib.h"

// Set RTC. To be replaced by a Chronodot.
RTC_DS1307 RTC;
//SDA = A4
//SCL = A5
//SQW = 3

// Define second, minute, hour.
int s;
int m;
int h;

// Brightness fade up (1 or 2). Depends on LDR reading.
int z;

// Set brightness mapping to min 6, max 31.
int MinBright = 6;
int MaxBright = 31;
int brightness;

// LDR
int LDRValue;

int SQW = 3;
  
void setup() {
  
  Serial.begin(57600);
  pinMode(SQW, INPUT);
  
  //start wire and rtc
  Wire.begin();
  RTC.begin();
  
    RTC.adjust(DateTime(__DATE__, __TIME__));     //to be replaced by fysical set-buttons  
  // set 1Hz SQW on DS1307 pin 7, we will be using this for interrupt 1 on arduino pin 3
  Wire.beginTransmission(0x68);              // write the control register
  Wire.send(0x07);                           // register address 07H)
  Wire.send(0x90);                           // 0x90=1Hz, 0x91=4kHz, 0x92=8kHz, 0x93=32kHz
  Wire.endTransmission();
  
  // The Arduino needs to clock out the data to the pixels
  // this happens in interrupt timer 1, we can change how often
  // to call the interrupt. setting CPUmax to 100 will take nearly all the
  // time to do the pixel updates and a nicer/faster display, 
  // especially with strands of over 100 dots.
  strip.setCPUmax(80);  // up this if the strand flickers or is slow
  
  // Start up the LED counter
  strip.begin();
  
  // Update the strip, to start they are all 'off'
  strip.show();
  
  //Attach SQW from DS1307 to Arduino pin 3 and call function clock
  attachInterrupt(1, clock, FALLING);

}

// Empty loop, all is done by the SQW 1Hz interupt on pin 3
void loop () {
 if(refreshFlag) {
   refreshFlag = 0; // reset the flag
   refresh();
 }
  }
  
void clock(){  // interrupt service routine
  refreshFlag = 1; // time to refresh the display
}

void refresh() {

  //Get current time
  DateTime now = RTC.now();  
  
  // mapping hour 24 => 12 => 60 is all the data we need.
  m = now.minute(), DEC;
  s = now.second(), DEC;
  h = now.hour(), DEC;
  if (h >= 12) { h = h - 12; }  // dealing with the 24hr format from RTC reading
  else if (h = 12) { h = 0; }   // same here but do we need it?
  h = map(h, 0, 12, 0, 60);
  
  if ( m < 6 ) { h == h; }      // slowly move the hour hand in between two hours during the hour
  else if ( m < 18 ) { h = h + 1; }
  else if ( m < 30 ) { h = h + 2; }
  else if ( m < 42 ) { h = h + 3; }
  else if ( m < 54 ) { h = h + 4; }
  
    // Get LDR vaulue and set brightness  
    LDRValue = analogRead(LDRPin);
    brightness = map(LDRValue, 0, 1023, MaxBright, MinBright);
  
    // set increase step size and treshold value
    if ( brightness < 20 ) {
      z = 1;
    } else {
      z = 2;
    }
    
    // clear previous output as seconds, minutes and hours pass by
    strip.setPixelColor((h - 1), 0, 0, 0);
    strip.setPixelColor((m - 1), 0, 0, 0);
    strip.setPixelColor((s - 2 ), 0, 0, 0);  //trailing led #1
    strip.setPixelColor((s - 3 ), 0, 0, 0);  //trailing led #2
    
    // clear transistion from 59 -> 0 minutes & hours
    if (s == 0) { strip.setPixelColor((58), 0, 0, 0); }
    if (s == 1) { strip.setPixelColor((59), 0, 0, 0); }  
    if (m == 0) { strip.setPixelColor((59), 0, 0, 0); }
    
    //start fade up/down seconds
    for (int y = 1; y < brightness; y = y + z) {
  
         strip.setPixelColor((s - 1 ), (brightness - y), 0, 0);
         if (s == 0) { strip.setPixelColor(59, (brightness - y), 0, 0); }  
         strip.setPixelColor(s, brightness, 0, 0); 
         strip.setPixelColor((s + 1 ), y, 0, 0);
         if (s == 59) { strip.setPixelColor(0, y, 0, 0); } 
        
         strip.setPixelColor(m, 0, brightness, 0);     //turn on led according to minute at level brightness
         strip.setPixelColor(h, 0, 0, brightness);     //turn on led according to hours at level brightness
         
         // second equals minute
         if ((s + 1) == m) { strip.setPixelColor(s + 1, y, brightness, 0); } 
         if (s == m) { strip.setPixelColor(s, brightness, brightness, 0); } 
         if ((s - 1) == m) { strip.setPixelColor(s - 1, (brightness - y), brightness, 0); } 
         
         // second equals hour
         if ((s + 1) == h) { strip.setPixelColor(s + 1, y, 0, brightness); } 
         if (s == h) { strip.setPixelColor(s, brightness, 0, brightness); }
         if ((s - 1) == h) { strip.setPixelColor(s - 1, (brightness - y), 0, brightness); } 
     
         // update strip
         strip.show();
         
         delay(105 - (2 * y));
     }
   // Debug   
    //Serial.print(now.year(), DEC);
    //Serial.print('/');
    //Serial.print(now.month(), DEC);
    //Serial.print('/');
    //Serial.print(now.day(), DEC);
    //Serial.print(' ');
    //Serial.print(now.hour(), DEC);
    //Serial.print(':');
    //Serial.print(now.minute(), DEC);
    //Serial.print(':');
    //Serial.print(now.second(), DEC);
    //Serial.println();
    //Serial.print("raw LDR reading = ");
    //Serial.println(LDRValue);     // the raw analog reading
    //Serial.print("brightness level = ");
    //Serial.println(brightness);     // the calculated level
    //Serial.println(); 
}

Anybody notice that there aren't any loading caps on the crystal? That would lead to low level clock and maybe some "instability"??

It is easy to have too little capacitance and difficult to have too much. A value of 100uF should be fine for starters.

The theory behind this experiment is that lowering the brightness doesn’t cause such high spikes when the IC’s switch.

As the brightness is controlled by PWM it makes absolutely no difference to the spikes if it runs bright or dim, the peak current is still the same. It is only the average current that is lower.

Any chance of a photo?

can’t tell if your ISR v2 is doing better then before. It runs for sure!

If it runs then it is doing better because your system will not fall in a heap if it fails to make an update.

Have you tried changing this:-

strip.setCPUmax(80);  // up this if the strand flickers or is slow

With the ISR done like I showed you, you can safely push this.

Also put some of the large caps on the arduino power supply, not just along the LED strip.

That is not how switch/case works. It may compile, but it doesn't do what you intend.

‘<6’ is multi character constant, not a comparison.
A perfectly legal C constant, which is why it compiles, but “case ‘<6’:” would only fire if “m” had the value ‘<6’ ( I’m posting on a tablet with no hex calculator, but it would be a value in excess of 8192, if my memory of the ASCIi table serves me correctly)

Probably.
But make sure you understand the difference between = and ==.
“h == h;” is a bit of a waste of time.

RogerMoerdijk:
does not doe the same thing when I leave the “h == h;” out.

Yes it does. I bet you’re leaving MORE than just the “h==h;” out. The following will do the same as the posted code.

  if ( m <= 6 ) { }      // slowly move the hour hand in between two hours during the hour
  else if ( m <= 18 ) { h = h + 1; }
  else if ( m <= 30 ) { h = h + 2; }
  else if ( m <= 42 ) { h = h + 3; }
  else if ( m <= 54 ) { h = h + 4; }

A far simpler approach to this whole problem is to combine hour AND minute, and then map it to the hour-hand position. e.g.

h = map(h*60 + m,0,12*60,0,60);

There are two things wrong with this:-

if ( m <= 6 ) { h == h; }

First of all the double equals should only be used in an if statement as it does logical comparison so you should have:-

if ( m <= 6 ) { h = h; }

But what is that saying? Make h equal to h, well it already is so why bother doing anything?

This has already been pointed out, but seemingly ignored.

Well, I did some experimenting with leaving that piece out, but the code doesn’t work properly if I do so.

  if ( m <= 6 ) { h = h; }      //shorthand led stays on full hour for 6 minutes
  else if ( m <= 18 ) { h = h + 1; }  //on minute 7 move to the next led
  else if ( m <= 30 ) { h = h + 2; }  //on minute 19 move to the next led
  else if ( m <= 42 ) { h = h + 3; }  //on minute 31 move to the next led
  else if ( m <= 53 ) { h = h + 4; }  //on minute 43 move to the next led
  else if ( m >= 54 ) { h = h + 5; }  //on minute 54 move to the next full hour and stay there until 6 minutes after that full hour

The assignment of h to h is silly. That line can go away. Your trigger points are not logically defined. 6, 18, 30, 42, 53?

You can calculate an increment for h:

int inc = (m+6)/12;

For values of m up to 5, inc will be 0. For values between 6 and 17, inc will be 1. For values from 18 to 29, inc will be 2. For values from 30 to 41, inc will be 3. For values from 42 to 53, inc will be 4. For values from 54 to 59, inc will be 5.

Then,

h += inc.

Much simpler than a bunch of if statements.