Code Optimisation [closed]

Question

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

int RedFadeAmount = 5;
int GreenFadeAmount = 5;

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)
    {
      RedGradualChange();
    }

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

void RedGradualChange(void)
{
  analogWrite(GREEN_LED, 0);

  GreenLedBrightness = 0;    // how bright the LED
  GreenFadeAmount = 5;

  analogWrite(RED_LED, RedLedBrightness);
  RedLedBrightness = RedLedBrightness + RedFadeAmount;

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

void GreenGradualChange(void)
{
  analogWrite(RED_LED, 0);

  RedLedBrightness = 0;
  RedFadeAmount = 5;


  analogWrite(GREEN_LED, GreenLedBrightness);
  GreenLedBrightness = GreenLedBrightness + GreenFadeAmount;

  if (GreenLedBrightness <= 0 || GreenLedBrightness >= 1020)
  {
    GreenFadeAmount = -GreenFadeAmount;
  }
}

Answer 1

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.

You should learn more about how to input values through serial:

• https://hackingmajenkoblog.wordpress.com/2016/02/01/reading-serial-on-the-arduino/

---

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;
int fadeAmount = 5;

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);
        }
        brightness += fadeAmount;
        if (brightness > 1023) {
            brightness = 1023;
            fadeAmount = -fadeAmount;
        } else if (brightness < 0) {
            brightness = 0;
            fadeAmount = -fadeAmount;
        }
    }
}