1

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?

  • 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 May 20 at 13:10
  • 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. – st2000 May 20 at 13:19
  • 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? – AnT May 20 at 16:49
  • Depending on what effect you want to achieve, you might want to add gamma-correction. – Gerben May 20 at 19:09
0

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);
  }
}

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.