# Code Optimisation [closed]

I have tried fading the RGB Led with different colour format and sequence. Working of Code :-

• When value of number is 1, the Red LED fades gradually and glow up with the delay interval of 10ms with fade amount 5.
• When value of number is 2, the Green LED works as above.
• The change of state from Red to green takes 4 seconds.

Blue Led Code can also be added to this and various other colour combinations can be made, which will complex the code and add up difficulty is debugging and finding errors because every combination should have different name for fading so that it doesn't conflict with other combinations.

Here is my code. However, I need some suggestions regarding quality of my code. Is it the efficient way to do code. How can I improve the code. Any constructive suggestions would be of great help.

``````int RED_LED = 12;           // the PWM pin the LED is attached to
int BLUE_LED = 14;
int GREEN_LED = 15;

int RedLedBrightness = 0;    // how bright the LED is
int GreenLedBrightness = 0;    // how bright the LED is

unsigned long previousMillis = 0;
const long interval = 10;

unsigned long previousMillis2 = 0;
const long interval2 = 4000;

int j = 0;

void setup()
{
pinMode(RED_LED, OUTPUT);
pinMode(BLUE_LED, OUTPUT);
pinMode(GREEN_LED, OUTPUT);

Serial.begin(115200)
}

void loop()
{
if (millis() - previousMillis2 >= interval2)
{
previousMillis2 = millis();

j += 1;
if (j > 2)
{
j = 1;
}
Serial.print("\n");
Serial.println(j);
}
SpecialFunction(j);
}

void SpecialFunction(int Number)
{
if (millis() - previousMillis >= interval)
{
previousMillis = millis();

if (Number == 1)
{
}

else if (Number == 2)
{
}
}
}

{
analogWrite(GREEN_LED, 0);

GreenLedBrightness = 0;    // how bright the LED

analogWrite(RED_LED, RedLedBrightness);

if (RedLedBrightness <= 0 || RedLedBrightness >= 1020)
{
}
}

{
analogWrite(RED_LED, 0);

RedLedBrightness = 0;

analogWrite(GREEN_LED, GreenLedBrightness);

if (GreenLedBrightness <= 0 || GreenLedBrightness >= 1020)
{
}
}
``````
• Perhaps if you were to write out, in plain English, what you want your program to achieve it may help. – Majenko Sep 25 '16 at 20:26
• @Majenko Please review my question again and my code as well, – Embedded Geek Sep 25 '16 at 20:33
• I have updated my answer. – Majenko Sep 25 '16 at 20:48
• `How can I improve the code` - this is not a reasonable question for this site. It is too broad. With all due respect to Majenko who is trying to help you, this is a very broad question. Plus, you keep editing the code so that answers don't make any sense. Please read How do I ask a good question?. – Nick Gammon Sep 26 '16 at 8:06

Without deciphering what all those functions are supposed to do, I can see a few fundamental flaws and inefficiencies:

1. Don't use string - especially if you're only then using it to convert into a char array. Just use the char array.
2. You're storing a number in a char array then comparing it to string constants. What is wrong with dealing with just numbers in the first place...?
3. This bit is wrong: `if (incomingByte = '#')` - you have an assignment (`=`) instead of a comparison (`==`) so it will always be true.
4. If you fix point 3 the rest of the code won't work, since your string will now always have `#` at the end (`3#` for instance) and the comparison against `3` (for example) will fail.

For your fading code there is a lot of redundancy in there. Personally I would use a bitfield indicating which LEDs are to fade and which aren't.

Basically you have a single value and OR together bits within it to indicate the enabled LEDs. For instance, assign the value `1` to red, `2` to green and `4` to blue. These correspond to `0b001`, `0b010` and `0b100` respectively.

When the state is changed, either by your loop or by an external serial signal, you reset all three colour brightnesses to 0 (to keep them synchronised) and set the right bits in the bitfield for the colours you want fading. For instance, for red you set the bitfield to 1. For cyan you set it to 6 (0b110). Then in your processing loop you just fade the colours that are enabled, and write all three values (the colour that isn't enabled will already be 0). This also has a side-effect that you now already have numbers assigned to the colours:

``````0 = 0b000 = All off
1 = 0b001 = Red
2 = 0b010 = Green
3 = 0b011 = Yellow
4 = 0b100 = Blue
5 = 0b101 = Magenta
6 = 0b110 = Cyan
7 = 0b111 = White
``````

In fact, since all the colours are fading together, you don't even need separate values for each colour - just one global brightness value.

Some code (untested) for you:

``````#define RED   0b001
#define GREEN 0b010
#define BLUE  0b100

int brightness = 0;
uint8_t enabled = 1;

const uint8_t RED_LED = 12;
const uint8_t BLUE_LED = 14;
const uint8_t GREEN_LED = 15;

unsigned long previousMillis = 0;
const long interval = 10;

unsigned long previousMillis2 = 0;
const long interval2 = 4000;

void setup() {
pinMode(RED_LED, OUTPUT);
pinMode(BLUE_LED, OUTPUT);
pinMode(GREEN_LED, OUTPUT);

Serial.begin(115200);
}

void loop()
{
if (millis() - previousMillis2 >= interval2) {
previousMillis2 = millis();
enabled ++;
if (enabled >= 8) enabled = 1;
Serial.println(enabled);
}

if (millis() - previousMillis >= interval) {
previousMillis = millis();
if (enabled & RED) {
analogWrite(RED_LED, brightness);
} else {
analogWrite(RED_LED, 0);
}
if (enabled & GREEN) {
analogWrite(GREEN_LED, brightness);
} else {
analogWrite(GREEN_LED, 0);
}
if (enabled & BLUE) {
analogWrite(BLUE_LED, brightness);
} else {
analogWrite(BLUE_LED, 0);
}
• I removed the serial part and I just want you to focus on rest of the code, which is necessary. I now used a loop to fill values into `j` after every 4 seconds. – Embedded Geek Sep 25 '16 at 20:14