All,
I have some code I was hoping someone could review to see if there is an optimal way to implement it. Basically, I am turning on/off a sign with a fade and flicker effect to simulate that it is not working properly. After effects occur the sign will stay on/off for a period of time (for testing purposes...it is 5 seconds). The fade-in/out is a randomized number to increase/decrease brightness, never to exceed 255 or go below 0. The flicker time is another randomized number and if the time does not exceed the current flicker interval, brightness will be zero, thus creating the flicker effect. The timer for the light to stay on/off occurs after the fade/flicker effects complete.
I did not create from scratch but modified code examples that are already out in the world. A lot of trial and error to get it to do what I wanted. A few nested ifs and repetitive code, and I am not sure if the code could be less cumbersome.
This is more of a learning exercise than a feature request or anything like that. Basically, I am exploring the art of the possible. Any guidance would be appreciated.
Please learn how to post code properly using code tags; below your code so people don't have to download it.
#define iPIN 9
unsigned long currentMillis = 0;
//GUITAR CONSTANTS
const int maxGuitarBrightness = 255;
const int minGuitarBrightness = 0;
const int minBrightIncrement = 1;
const int maxBrightIncrement = 20;
const int guitarOnInterval = 5000;
const int guitarFadeInterval = 50;
void guitarOperation() {
static bool guitarDirection = 0;
static int guitarBrightness = 0;
static unsigned long guitarOn = 0;
static unsigned long guitarFade = 0;
static unsigned long guitarFlicker = 0;
int guitarFlickerInterval = random (1, 250);
//-- has enough time passed for the guitar to turn on or off?
if (currentMillis - guitarOn >= guitarOnInterval) {
//-- it is time to turn on or off but there is a fade in or out preceding the actual on or off action
if (currentMillis - guitarFade >= guitarFadeInterval) {
//-- now, we are in the fade in/out loop and set the guitar fade accordingly
guitarFade = currentMillis;
//-- check which direction. If direction is 0, we need to turn on
if (guitarDirection == 0) {
//-- not only does guitar fade, it flickers as well to simulate a broken light. It will flicker throughout
//-- the fade in or out process. The flicker interval is random everytime to randomize pattern
if (currentMillis - guitarFlicker >= guitarFlickerInterval) {
guitarFlicker = currentMillis;
//-- brightness is randomized as well
guitarBrightness = guitarBrightness + random(minBrightIncrement, maxBrightIncrement);
//-- Max brightness will not exceed 255. Include the condition where it does equal zero so the other values get set
if (guitarBrightness >= maxGuitarBrightness) {
//-- set brightness to 0
guitarBrightness = maxGuitarBrightness;
//-- change direction of the fade
guitarDirection = 1;
//-- set time for guitar on/off here to ensure it stays on exclusive of flicker event
guitarOn = currentMillis;
}
analogWrite(iPIN, guitarBrightness);
//Serial.println(guitarBrightness);
}
else {
//-- the "off" feature which creates the flicker effect
analogWrite(iPIN, minGuitarBrightness);
}
}
//-- check which direction. If direction is 1, we need to turn off
else if (guitarDirection == 1) {
if (currentMillis - guitarFlicker >= guitarFlickerInterval) {
guitarFlicker = currentMillis;
guitarBrightness -= random(minBrightIncrement, maxBrightIncrement);
//-- Min brightness will not go below 0. Include the condition where it does equal zero so the other values get set
if (guitarBrightness <= 0) {
//-- set brightness to 0
guitarBrightness = minGuitarBrightness;
//-- change direction of the fade
guitarDirection = 0;
//-- set time for guitar on/off here to ensure it stays on exclusive of flicker event
guitarOn = currentMillis;
}
analogWrite(iPIN, guitarBrightness);
//Serial.println(guitarBrightness);
}
else {
//-- the "off" feature which creates the flicker effect
analogWrite(iPIN, minGuitarBrightness);
}
}
}
}
}
void setup()
{
//Serial.begin(9600);
pinMode(iPIN, OUTPUT);
analogWrite(iPIN, minGuitarBrightness);
}
void loop() {
currentMillis = millis();
guitarOperation();
}
Please read the post at the start of any forum , entitled "How to use this Forum".
OR http://forum.arduino.cc/index.php/topic,148850.0.html.
Then look down to item #7 about how to post your code.
It will be formatted in a scrolling window that makes it easier to read.
i don't see a need for so many timing condition checks -- you should only need one to determine when to do something and that action can determine the delay until the next time to do something (i.e. kinda like a state machine
i don't see a need for separate cases to handle direction. why not set you direction variable to either 1 or -1 and use it to update guitarBrightness = guitarDirection * random()
i don't see a need to call analogWrite() more than once -- call it once at the end of when you do something.
consider the following. i don't think it does what you want, but demonstrates ways to do things. long repetitive variable names are frowned upon
#if 0
# define iPIN 9
#else // my hardware
# define iPIN 13
#endif
//GUITAR CONSTANTS
const int maxBright = 155;
const int minBright = 0;
const int minIncrement = 1;
const int maxIncrement = 20;
unsigned long msecDelay = 3000;
int bright = 100;
int dir = 1;
void guitarOperation() {
static unsigned long msecLst = 0;
unsigned long msec = millis();
if ((msec - msecLst) > msecDelay) {
msecLst = msec;
bright += dir * random(minIncrement, maxIncrement);
if (bright > maxBright) {
bright = maxBright;
dir = -1;
}
else if (bright < minBright) {
bright = minBright;
dir = 1;
}
analogWrite (iPIN, bright);
msecDelay = random (1, 250);
Serial.println (bright);
}
}
void setup()
{
Serial.begin(115200);
pinMode(iPIN, OUTPUT);
}
void loop() {
guitarOperation();
}
You should declare 'guitarFlickerInterval' as 'unsigned int' rather than 'int'. That way you won't get warnings when comparing with unsigned values.
By picking a fresh value for 'guitarFlickerInterval' every time through loop() you are going to have many more short intervals than long intervals. The flicker ends the first time the random value is smaller than the current interval. If you want a random interval you could make it global, initialize it in setup() and pick a new value when the flicker ends.
I did not create from scratch but modified code examples that are already out in the world. A lot of trial and error to get it to do what I wanted. A few nested ifs and repetitive code, and I am not sure if the code could be less cumbersome.
This is more of a learning exercise than a feature request or anything like that. Basically, I am exploring the art of the possible. Any guidance would be appreciated.
I understand, but this is really not a good way to learn. If it works, then you have a working sketch and don't need to go any further. If you want to learn, you can get guidance here but it should not be your first stop. In that case, you should spend some time and drill down on anything that you don't understand, by doing the research from authoritative and original sources. Then try programming some ideas on your own, from scratch.
Also, when you "optimize" it's always focused on some aspect, because there are always trade offs. Do you want it to be faster, use less flash memory? Less RAM? More readable, more portable? There are so many ways to optimize.
