Code for two bulb fading alternatively

Question

I have written this code

const int A = 3;
const int B = 5;
void setup() {
 pinMode(A, OUTPUT);
 pinMode(B, OUTPUT);
}
void loop() {
 for(int i=1;i<=255;i++)
 {
 int j =255-i;
 analogWrite(A,i);
 analogWrite(B,j);
 delay(100);
 }
 for(int i=255;i>0;i--)
 {
 int j =255-i;
 analogWrite(B,j);
 analogWrite(A,i);
 delay(100);
 }
}

Is this code correct? If yes then can it be modified in any way to make it better?

• st2000: Other than a little asymmetry, I don't think I see a problem (i goes from 1 to 255 while j goes from 0 to 254). Why? Is it not working?
• st2000: The pins you picked should work on most Arduino boards. But when asking these types of question you should list what Arduino board type you are using.
• AnT stands with Russia: Define "better". Your code uses delay, which is OK if your code has nothing else to do, but completely unacceptable if your code will be used in context of other code. Is this something that you want/need to make "better" or not?
• Gerben: Depending on what effect you want to achieve, you might want to add gamma-correction.

Answer 1

To keep the two loops more symmetrical, let the first loop go from 0 until 254: [0 .. 255) and the second loop from 255 to 1: [255 .. 0), so both iterations have 255 values each.

Also, it is quite common in programming to include the start element, and not include the end element (as in the values above: [start_element .. end_element).

Also, for the simple formula (255 - i), I would leave out the formula. And to make things more clear define constants for the min/max values (0,255) and the delay.

Than you get:

const int A = 3;
const int B = 5;
const int MIN_INTENSITY = 0;
const int MAX_INTENSITY = 255;
const int DELAY_TIME = 100;
void setup() {
 pinMode(A, OUTPUT);
 pinMode(B, OUTPUT);
}
void loop() 
{
 for(int i = MIN_INTENSITY; i < MAX_INTENSITY ; i++)
 {
 analogWrite(A, i);
 analogWrite(B, MAX_INTENSITY - i);
 delay(DELAY_TIME);
 }
 for(int i = MAX_INTENSITY ; i > MIN_INTENSITY; i--)
 {
 analogWrite(B, MAX_INTENSITY - i);
 analogWrite(A, i);
 delay(DELAY_TIME);
 }
}

By making a function and use arguments for the variable values, and create some better names for constants and variables you get this:

const int LED_1_PIN = 3;
const int LED_2_PIN = 5;
const int MIN_INTENSITY = 0;
const int MAX_INTENSITY = 255;
const int DELAY_TIME = 100;
void setup()
{
 pinMode(LED_1_PIN, OUTPUT);
 pinMode(LED_2_PIN, OUTPUT);
}
void loop() 
{
 fade(MIN_INTENSITY, MAX_INTENSITY, 1);
 fade(MAX_INTENSITY, MIN_INTENSITY, -1);
}
void fade(int startIntensity, int endIntensity, int stepValue)
{
 for(int intensity = startIntensity; intensity < endIntensity; intensity += stepValue)
 {
 analogWrite(LED_1_PIN, intensity);
 analogWrite(LED_2_PIN, MAX_INTENSITY - intensity);
 delay(DELAY_TIME);
 }
}