Uncertain where to post this, is this coding correct?

This is my first writing of arduino code and I'd like to know how well I've done.
The program is used to do two things:

  1. fade one LED up and down in brightness.
  2. Change the color of a multi color led from green to red.
    This is to be loaded onto an arduino nano and more will likely be added.
    Please tell me whether it is done correct or any tendencies that could compromise my coding at a more advanced level.
const int fadingled = 9; //fading led pin
const int brightness = 0; //brightness of the led
const int fadeamount = 5 //how much the led fades
const int colorchange1 = 11; //red dual color pin
const int colorchange2 = 12; //green dual color pin
int colorledstate1 = LOW; //red led state on/off
int colorledstate2 = LOW; //green led state on/off
long previousmillis = 0; //last time LED was updated
long interval = 500; //time between flashes

void setup()   {
  pinMode(fadingled,OUTPUT); //declare all LED's as outputs
  pinMode(colorchange1,OUTPUT);
  pinMode(colorchange2,OUTPUT);
}

void loop()   {
  analogWrite(led, brightness); //set the brightness to the LED
  brightness = brightness + fadeamount; //add or subtract to the brightness based on fadeamount
  if (brightness == 0 || brightness == 255) {
    fadeamount = -fadeamount;
  }
  unsigned long currentmillis = millis();
  if currentmillis - previousmillis > interval)  { //if it has been more than 500 milliseconds
    previousmillis = currentmillis;
    //turn the led off if it's on and vice versa
    if (colorledstate1 == LOW)  {
      colorledstate1 = HIGH;  }
    else {
      colorledstate1 = LOW;  }
      //if the red LED is on, turn the green LED off and vice versa
  if (colorledstate1 == LOW)  {
    colorledstate2 = HIGH;  
  }
  else {
    colorledstate2 = LOW;
  }

"Programming Questions ?"

I'm a big fan of 'if it does what you want, its right.' :slight_smile:

:stuck_out_tongue:
Arduino hasn't showed up yet. I want to get my code done and ready to go before it gets here.
Of course I bought two ]:smiley:

Well It's not working now :frowning:
I put one LED to go high in the setup just so I know that it is running and it is, but it's never going through the loop.

const int fadingled = 9; //fading led pin
int brightness = 0; //brightness of the led
int fadeamount = 5; //how much the led fades
const int colorchange1 = 11; //red dual color pin
const int colorchange2 = 12; //green dual color pin
int colorledstate1 = LOW; //red led state on/off
int colorledstate2 = LOW; //green led state on/off
long previousmillis = 0; //last time LED was updated
long interval = 500; //time between flashes

void setup()   {
  pinMode(fadingled,OUTPUT); //declare all LED's as outputs
  pinMode(colorchange1,OUTPUT);
  pinMode(colorchange2,OUTPUT);
  analogWrite(colorledstate1, HIGH);
}

void loop()   {
  analogWrite(fadingled, brightness); //set the brightness to the LED
  brightness = brightness + fadeamount; //add or subtract to the brightness based on fadeamount
  if (brightness == 0 || brightness == 255) {
    fadeamount = -fadeamount;
  }
  unsigned long currentmillis = millis();
  if (currentmillis - previousmillis > interval)  { //if it has been more than 500 milliseconds
    previousmillis = currentmillis;
    //turn the led off if it's on and vice versa
    if (colorledstate1 == LOW)  {
      colorledstate1 = HIGH;  }
    else {
      colorledstate1 = LOW;  }
      //if the red LED is on, turn the green LED off and vice versa
  if (colorledstate1 == LOW)  {
    colorledstate2 = HIGH;  
  }
  else {
    colorledstate2 = LOW;
  }
  }
}
analogWrite(colorledstate1, HIGH);

colorledstate1 is not a pin,
try writing to one of your defined pins

const int colorchange1 = 11; //red dual color pin
const int colorchange2 = 12; //green dual color pin

You're right, I missed that. Either way, the led still goes high during the setup.
Modified code, still not looping though

const int fadingled = 9; //fading led pin
int brightness = 0; //brightness of the led
int fadeamount = 1; //how much the led fades
const int colorchange1 = 11; //red dual color pin
const int colorchange2 = 12; //green dual color pin
int colorledstate1 = LOW; //red led state on/off
int colorledstate2 = LOW; //green led state on/off
long previousmillis = 0; //last time LED was updated
long interval = 500; //time between flashes

void setup()   {
  pinMode(fadingled,OUTPUT); //declare all LED's as outputs
  pinMode(colorchange1,OUTPUT);
  pinMode(colorchange2,OUTPUT);
  analogWrite(colorchange1, HIGH);
}

void loop()   {
  analogWrite(colorchange1,colorledstate1); //write to the LED's
  analogWrite(colorchange2,colorledstate2); 
  analogWrite(fadingled, brightness); //set the brightness to the LED
  brightness = brightness + fadeamount; //add or subtract to the brightness based on fadeamount
  if (brightness == 0 || brightness == 255) {
    fadeamount = -fadeamount;
  }
  unsigned long currentmillis = millis();
  if (currentmillis - previousmillis > interval)  { //if it has been more than 500 milliseconds
    previousmillis = currentmillis;
    //turn the led off if it's on and vice versa
    if (colorledstate1 == LOW)  {
      colorledstate1 = HIGH;  }
    else {
      colorledstate1 = LOW;  }
      //if the red LED is on, turn the green LED off and vice versa
  if (colorledstate1 == LOW)  {
    colorledstate2 = HIGH;  
  }
  else {
    colorledstate2 = LOW;
  }
  }
}

packocrayons:
Modified code, still not looping though

Well, it is looping in the sense that loop() is being called, but presumably the code inside loop() isn't doing what you intended.

analogWrite(pin, # from 0 to 255);
I don't see you writing a number to the pins.
Try changing the low, high you have there now to something either is, or represents, a number.

Here's the modified code, the color change is working now but the fading LED just stays solid, it stays at a certain brightness depending on the value of fadeamount.
Edit: I forgot to put a delay, it's all working now.

const int fadingled = 9; //fading led pin
int brightness = 1; //brightness of the led
int fadeamount = 1; //how much the led fades
const int colorchange1 = 11; //red dual color pin
const int colorchange2 = 12; //green dual color pin
int colorledstate1 = LOW; //red led state on/off
int colorledstate2 = LOW; //green led state on/off
long previousmillis = 0; //last time LED was updated
long interval = 500; //time between flashes

void setup()   {
  pinMode(fadingled,OUTPUT); //declare all LED's as outputs
  pinMode(colorchange1,OUTPUT);
  pinMode(colorchange2,OUTPUT);
  }

void loop()   {
  digitalWrite(colorchange1,colorledstate1); //write to the LED's
  digitalWrite(colorchange2,colorledstate2); 
  analogWrite(fadingled, brightness); //set the brightness to the LED
  brightness = brightness += fadeamount; //add or subtract to the brightness based on fadeamount
  if (brightness == 0 || brightness == 255) {
    fadeamount = -fadeamount;
  }
  unsigned long currentmillis = millis();
  if (currentmillis - previousmillis > interval)  { //if it has been more than 500 milliseconds
    previousmillis = currentmillis;
    //turn the led off if it's on and vice versa
    if (colorledstate1 == LOW)  {
      colorledstate1 = HIGH;  }
    else {
      colorledstate1 = LOW;  }
      //if the red LED is on, turn the green LED off and vice versa
  if (colorledstate1 == LOW)  {
    colorledstate2 = HIGH;  
  }
  else {
    colorledstate2 = LOW;
  }
  }
}

Well, that would indicate this isn't doing what you want:

  brightness = brightness += fadeamount; //add or subtract to the brightness based on fadeamount
  if (brightness == 0 || brightness == 255) {
    fadeamount = -fadeamount;
  }

Try putting a Serial.print and see what's happening to brightness & fadeamount.

It's doing what I want now. All I need to do is figure out how to get around using the delay.
Thanks everyone for the help.