Hi, I have updated Tim Bartlett's NeoCandle - http://timbartlett.net/ledcandle/ and https://github.com/timpear/NeoCandle/tree/master/NeoCandle to work with

Hi, I have updated Tim Bartlett’s NeoCandle - http://timbartlett.net/ledcandle/ and https://github.com/timpear/NeoCandle/tree/master/NeoCandle to work with FastLED and also without using delay() so that the code is more flexible.

I have one question, the code now uses currentMillis() and previousMillis() in three separate locations, would it be better to use different variable names on each occasion? I want to use this code as an object in a bigger project if that matters.

Also any suggestions for how to improve the code gratefully received. I know I have declared currentmillis() 3 times which should probably be altered.

Moved code to pastebin - http://pastebin.com/fbNFyuj4

Hi Rob,
To answer specifically this question…
would it be better to use different variable names on each occasion?
In my humble opinion, the simple answer is no. Because in your case, all 3 occurrences are limited in scope to their current for loop.
On the other hand, I believe that you are incorrectly using the millis() function.
I am only guessing that you intended to stay in a loop for a specific duration but the way I see it, the currentMillis and previousMillis variables are not really useful in your posted sketch !
You probably want to use a while loop !?
previousMillis = millis();
while(currentMillis - previousMillis < fRep){
currentMillis = millis();
do what you want here…
}

@JP_Roy That’s very useful thanks. I always struggle with the difference between ‘if’ and 'whil’e statements … all i want is a delay so i think you’re right, a ‘do while’ statement seems appropriate, even though the code works as intended. Would it be processor resources that are the main reason for doing it your way?

Hi @Rob_Hilken ,
I am not an experienced programmer and actually can’t say if one method is more efficient with resources.
Many experienced programmers do not like using delays() in their sketches. They consider the time spinning around inside a delay() as wasted time. In many cases they are right about that but, in my humble opinion, unless you know you could be doing something useful in the time that is otherwise wasted in a delay(), I see no reason to use something else like a while loop.
Also note that there is significant differences between if and while, I suggest you carefully go through the Arduino reference to understand them both well.

@JP_Roy I’ve tried using your ‘while’ code but it isn’t working. the light just stays on and does not flicker, whereas it works fine using my original ‘if’ statement. Why do you think using ‘while’ statement should work better?

redPx, grnPx, bluePx are dynamic and changed each time the loops is run, but in a ‘while’ statement they hold their value which isn’t what I want.

@JP_Roy Thanks for your advice. It’s critical that I don’t use delay as these will become objects in a much larger sketch… there will be many things happening at once so getting rid of the delay is critical… I have been getting my head around Finite State Machines and Object Oriented Programming to get it to come together as it’s an interactive art project with multiple inputs and sensors. I understand the basics of if statements and while statements but sometimes it’s tricky to see which is more efficient as they will often both do similar things (IF this is true then DO this vs WHILE this is true then DO this). In my case I think the IF statement is better as I do not want to get stuck inside a while loop

@Rob_Hilken The few lines of code I gave you are incomplete and only provided as a guideline, not a complete solution. All I can say for sure is that the code you posted made incorrect usage of the millis() function.
Here’s my quick analysis of that…
In the 3 places you use millis(), the fist 2 lines set previous millis and current millis
previousMillis = 0;
unsigned long currentMillis = millis();
the next line is…
if (currentMillis - previousMillis > fRep) {
currentMillis - 0 will always be greater than fRep so the rest of the if statement gets executed in all cases. You could just eliminate these lines completely and your code should run just the same !!
Since I do not know your intention for using these statements, I could only assume you wanted to loop through some code for a certain amount of milliseconds set by fRep.
I did not go through a complete analysis of your code, I only spotted an error in the actual part that you were asking a specific question about.

@JP_Roy What you said is true, the code will always be executed on the first loop, but then later in the loop I use previousMillis = currentMillis so that the timer is updated. It’s the same way the blink without delay tutorial works. Am I missing something?

@JP_Roy Thanks very much for your push to make me examine the code fully. I am adding serialprint to every step to see what is going on and you are correct, while it looks like it is working, some very strange things are happening! I will persevere and get to the bottom of it. Many thanks for your pointers :slight_smile:

@JP_Roy Thanks for your advice. I have eventually got it all working exactly as planned by using a state machine and switch statements. A good learning curve to go through!