0

enter image description here

I am attaching my code below :

int cwled=9;
int wwled=10;
int button=8;
int buttonpress;
long startTime=0;
long endTime;
long holdTime;
boolean previous = LOW;
boolean current;
int state=0;
int pot = A0;
boolean cwState = HIGH;
boolean wwState = LOW;
int brightness;
int counter = 1;
boolean pressB;

void setup(){
  pinMode(cwled,OUTPUT);
  pinMode(wwled,OUTPUT);
  pinMode(button,INPUT);
  Serial.begin(9600);

}

void loop() {
  if(state == 0){
    brightness= map(analogRead(pot),0,1023, 0, 255);
        analogWrite(cwled,brightness);
       // analogWrite(wwled,0);
        Serial.println("Initialized");
  }
  current= digitalRead(button);
  if(current == HIGH){
  pressDetection();
  state =1;
  }
}






void pressDetection(){
  pressB=digitalRead(button);
  if(pressB==HIGH && previous == LOW)
  {
    state =1;
    startTime = millis();
  }
  while(digitalRead(button) == HIGH){
    endTime = millis();  
    Serial.println("Inside loop");
  }
  previous = LOW;
  holdTime = endTime - startTime;
  if(holdTime> 500){
    longPress();
  }
  else{
    shortPress();
  }

  }

void shortPress(){
  cwState = !cwState;
  wwState = !wwState;
  //brightness= map(analogRead(pot),0,1023, 0, 255);
  if(cwState == HIGH && wwState == LOW){
   brightness= map(analogRead(pot),0,1023, 0, 255);
   digitalWrite(cwled,HIGH);
   digitalWrite(wwled,LOW);
   Serial.println("cwState");
   return;
  }
  if(cwState == LOW && wwState == HIGH){
  // brightness= map(analogRead(pot),0,1023, 0, 255);
   digitalWrite(wwled,HIGH);
   digitalWrite(cwled,LOW);
   Serial.println("wwState");
   return;
  }
 /*else if(cwState == HIGH && wwState == HIGH){
   //brightness= map(analogRead(pot),0,1023, 0, 255);
   digitalWrite(cwled,HIGH);
   digitalWrite(wwled,LOW);
  }*/
}



void longPress(){

   cwState =wwState = HIGH;

   //if(cwState == HIGH && wwState == HIGH){
 //  brightness= map(analogRead(pot),0,1023, 0, 255);
   digitalWrite(wwled,HIGH);
   digitalWrite(cwled,HIGH);
  cwState = !cwState;

 //}

}

I understand there are so many variables unused, this program needs a lot of improvising. My question is:

  1. How do I improve this code?
  2. How do I add dimming via pwm in this code?
  3. And if I have to add one more type of LED or say n number of LEDs and want to toggle between them what should be the algorithm.

My aim is to use a push button to switch between different types of LEDs (in my case between warmwhite LED and coldwhite LED, if Arduino registers long press off the push button all the LEDs light up, in my case both warmwhite and coldwhite LEDs turn on, and short press of push button toggles between warmwhite and coldwhite LED. The program by default switches on coldwhite LED first and after a long press, shortpress sets default LED to high which is coldwhite.

I am not a hardcore programmer but I have more than basic knowledge of OOPS programming. I am an ECE Engineer and I believe I have slightly more than basic understanding of OOPS and procedural programming. I am aware of C++, C and JAVA only. Any advice or help would be highly appreciated.

  • Since you never set state back to zero, it looks like your initial stuff (under the if) might as well be in setup as it will only be done once. – Nick Gammon Dec 22 '16 at 5:06
  • I deliberately did that. initially only coldwhite LED turns ON( which can be dimmed). I can remove that part but It's purpose is to initially start the system with coldwhite LED ON. – HarshChandola Dec 22 '16 at 5:32
  • "Improving you code" is too general of a question to ask here. I won't do it, but, unless you rephrase your question to something more specific, someone will likely down vote your question. (At the very least, you could add comments.) As for adding dimming, you are already using "analogwrite()". However, I'm not sure why, you later forsake the dimming feature and use "digitalwrite()". If you want to add LED I don't see what is stopping you. There are only some pins on an Arduino Uno which support PWM. Other than that you can add as many LED as you want. Just use normal low current LEDs. – st2000 Dec 22 '16 at 5:35
  • That analogWrite wasn't working properly. That is: it was not dimming in real-time, what I mean by that is suppose I switched to coldwhite Led, then when I'd change my potentiometer to set pwm to say 80 percent it'd not change the brightness , however when I'd toggle again to the warmwhite led it light up with 80% brightness and so on. By improving I mean can I use classes for it too? I want to make my code changeable, such that if I make amendments it'd not affect the previous performance – HarshChandola Dec 22 '16 at 5:44
  • Just want to clarify, your intention is add "dim" effect for transition? – dhimaspw Dec 22 '16 at 7:56
2

How do you improve your code?

Firstly there is nothing disastrously wrong with you code, it could do with a polish, but who's code couldn't?

In the dim and very distant past I was told "you write code once but read it many times" so try and make your code readable and clear.

  1. If the values aren't going to change then they should be constants or #defines.
  2. ALWAYS initialise variables. (Doesn't matter on an Arduino because the compiler does it for you, but its good practice).
  3. Boolean should be true or false.
  4. You are writing C/C++ not Java. Opening braces { should be on a new line. It makes the code easier to read and match up the braces.
  5. Unless your space bar is broken, put spaces after commas and either side of equals.
  6. Using function parameters and less global variables will make you code more reusable. Imagine the day when you start a new project and copy and paste functions from previous projects and finish the code in 15 minutes.
  7. One statement per line. cwState = wwState = HIGH;. In this case its not a problem, but you can get into states where side affects screw everything up. Also its easier to read.
  8. Comment the code as you go, because you'll never go back.
  9. Explain what each function does, because you'll have forgot by tomorrow.

I hope that helps you write better code and none of it has caused offence.

  • That was so helpful man. why would knowledge and/or advice cause offence. Very practical advice. yeah I was opening braces like they do it in JAVA. Thanks a lot. #define or constant thing is very helpful too. will definitely keep all these points in mind from now on. – HarshChandola Dec 23 '16 at 9:10
  • 10. Use parentesis around non-trivial expressions. Precedence rules can bite you. – user31481 Nov 17 '17 at 20:51
0

Putting aside the long press vs. short press, alternating the pwm generation is fairly simple.

  1. Use an adc interrupt to periodically read the pot and update a variable that holds the duty cycle.

  2. Use an interrupt to track the button input. In the isr, use a static variable to track which pwm pulse train is running. Upon button presses, update the variable and then use it to steer pwm output.

With this structure, your loop() is entirely empty and the functionalities (reading the pot, pwm generation and output steering) is done entirely in the background.

As to the long pushes, you can use a timer to track the button hold time and it returns not just the state of the button but also long vs. short presses.

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.