Help me to optimize my code

Projects Programming
1

Hi, I'm new in Arduino and in programming generally.
And here is my very first code, which changes the state of green & red leds when pressing the button. But i think there is the better way to do that. If so please show me how.

const int buttonPin = PUSH2;     // the number of the pushbutton pin
const int green =  GREEN_LED;      // the number of the LED pin
const int red =  RED_LED;
// variables will change:
int buttonState = 0;         // variable for reading the pushbutton status
int ledState = 0;

void setup() {
  pinMode(green, OUTPUT);
  pinMode(red, OUTPUT);  
  pinMode(buttonPin, INPUT_PULLUP);
  digitalWrite(green, HIGH);  
}
void loop(){
  buttonState = digitalRead(buttonPin);
  ledState    = digitalRead(green); 
  if (buttonState == LOW) {     
     if (ledState == HIGH) {
        while (buttonState == LOW){
        buttonState = digitalRead(buttonPin);    
       
        digitalWrite(green, LOW);
        digitalWrite(red, HIGH);
     }
      }
     
    if (ledState == LOW) {
        while (buttonState == LOW){
        buttonState = digitalRead(buttonPin);     
       digitalWrite(green, HIGH);
       digitalWrite(red, LOW);
      }
    }
  }
}

Best regards, wilfa

2

Did you leave out part? At some point you need to put real pin #s in, like in the first three lines.

otherwise I think these will all get assigned to pin 0:

const int buttonPin = PUSH2;     // the number of the pushbutton pin
const int green =  GREEN_LED;      // the number of the LED pin
const int red =  RED_LED;
3

const int buttonPin = PUSH2; // the number of the pushbutton pin
const int green = GREEN_LED; // the number of the LED pin
const int red = RED_LED;

const int buttonPin = 2;     // the number of the pushbutton pin
const int green =  13;      // the number of the LED pin
const int red =  14;

Is it better ?

4

Most of that loop looks redundant. Aren't you simply lighting one LED if the button is down, and the other if it is up?

5

You are doing something when the switch is pressed, which involves waiting for the switch to be released. It would be a lot better to detect the transitions, from released to pressed and from pressed to released, and doing something only when the correct transition occurs.

int currState;
int prevState = HIGH;

void loop()
{
   currState = digitalRead(buttonPin);
   if(currState != prevState)
   {
      // A transition occurred
      if(currState == LOW)
      {
         // from released to pressed
      }
      else
      {
         // from pressed to released
      }
   }
   prevState = currState;
}

Put your code to do stuff after the appropriate comment.

There is no need to call digitalWrite() in the while loop. The pins stay at the state you set them to, whether that is HIGH or LOW. They don't revert just because you turn your back.

6

I will try to explain better. When I press the button i want green led (pin13) to turn on, next time i press the same button i want green led to turn off and red led (pin14) to turn on. And next time green on and red off.

There is my new code. Fixed loops and added delay (Contact bounce)

const int buttonPin = 2;     // the number of the pushbutton pin
const int green =  13;      // the number of the LED pin
const int red =  14;
// variables will change:
int buttonState = 0;         // variable for reading the pushbutton status
int ledState = 0;

void setup() {
  pinMode(green, OUTPUT);
  pinMode(red, OUTPUT);  
  pinMode(buttonPin, INPUT_PULLUP);
  digitalWrite(green, HIGH);  
}
void loop(){
  buttonState = digitalRead(buttonPin);
  ledState    = digitalRead(green);
 delay(50);
  if (buttonState == LOW) {     
     if (ledState == HIGH) {
        digitalWrite(green, LOW);
        digitalWrite(red, HIGH);
        while (buttonState == LOW){
        buttonState = digitalRead(buttonPin);    
     }
      }
     delay(50);
    if (ledState == LOW) {
       digitalWrite(green, HIGH);
       digitalWrite(red, LOW);
        while (buttonState == LOW){
        buttonState = digitalRead(buttonPin);     
      }
    }
  }
}
7

When I press the button i want green led (pin13) to turn on, next time i press the same button i want green led to turn off and red led (pin14) to turn on. And next time green on and red off.

So, you want to count transitions from released to pressed.

Then, you want to do something based on the count:

int currState;
int prevState = HIGH;
byte count = 0;

void loop()
{
   currState = digitalRead(buttonPin);
   if(currState != prevState)
   {
      // A transition occurred
      if(currState == LOW)
      {
         // from released to pressed
         count++;
      }
      else
      {
         // from pressed to released
      }
   }
   prevState = currState;

   if(count > 2)
      count -= 2;

   switch(count)
   {
      case 0:
         // do nothing, as the switch has never been pressed
         break;
      case 1:
         // Turn red on and green off
         break;
      case 2:
         // Turn green on and red off
         break;
   }
}

count will contain 0, 1, 2, 3->1, 2, 3->1, etc.

8

Yes, your code was working perfect. Thanks.

But i have some more questions.

  1. Do i use c++ to write the program?
  2. And can i write the program in asm?
9
  1. Do i use c++ to write the program?

Normally, yes.

  1. And can i write the program in asm?

Yes. I don't recommend that, though, until you have a much better grasp of C and C++.

10

PaulS:

  1. And can i write the program in asm?

Yes. I don't recommend that, though, until you have a much better grasp of C and C++.

i know what you mean :slight_smile: But i think it is much more easier to understand asm then C (even if the code is longer).

Can i just write asm code inside C, or i must declare it?