Need help to clean up my code

Hi,

This is my first Arduino project, to make a simple pwm- controller to the interior LED lights in my boat.
It is working as it should, but since this is the first time I write C code, I am sure that my code is very ineffective.

I would be very grateful if someone is able to check up my code and give me some hints of what I can do to clean it up.

/*
Two buttons for increasing and decreasing light. (pin 2 and 3)
 One potentiometer, selecting what PWM-stream to change (pin A0)
 One LED lits when the light change request is is more than 16 or less than 1 (pin 13)
 Two PWM controlled LED drivers (pin 9 and 10)
 
 */

const int analogOutPin1 = 9;      // Analog output pin that the PWM-controlled LED#1 is attached to
const int analogOutPin2 = 10;    // Analog output pin that the PWM-controlled LED#2 is attached to
const int buttonPinPlus = 3;       // the pin that the pushbutton plus is attached to
const int buttonPinMinus = 2;    // the pin that the pushbutton minus is attached to
const int ledPin = 13;              // the pin that the end-of-range LED is attached to

int outputValue1 = 0;             // value output to the PWM#1 (analog out)
int outputValue2 = 0;             // value output to the PWM#2 (analog out)
int buttonPushCounter1 = 8;   // counter1 for the number of button presses (light level LED driver 1) 
int buttonPushCounter2 = 8;   // counter2 for the number of button presses  (light level LED driver 2)
int buttonStatePlus = 0;         // current state of the button
int buttonStateMinus = 0;         // current state of the button
int lastButtonStatePlus = 0;     // previous state of the button
int lastButtonStateMinus = 0;     // previous state of the button

void setup() {
  pinMode(buttonPinPlus, INPUT);     // initialize the button pin plus as a input:
  pinMode(buttonPinMinus, INPUT);   // initialize the button pin minus as a input:
  pinMode(ledPin, OUTPUT);            // initialize the LED as an output:
  Serial.begin(9600);                      // initialize serial communication, 9600 bps:
}

void loop() {

  int sensorValue = analogRead(A0);    //read value from potentiometer

    // read the pushbuttons input pins:
  buttonStatePlus = digitalRead(buttonPinPlus);
  buttonStateMinus = digitalRead(buttonPinMinus);

  // compare the buttonState to its previous state
  if (buttonStatePlus != lastButtonStatePlus || buttonStateMinus != lastButtonStateMinus) {
    if (sensorValue < 512 ) { // if lower half of potentiometer
      if (buttonStatePlus == HIGH && buttonPushCounter1 < 16 ) { //button is pressed and counter1 <16
        buttonPushCounter1++; // increase buttonPushCounter1
      }
    }

    if (sensorValue > 512 ) { // if lower half of potentiometer
      if (buttonStatePlus == HIGH && buttonPushCounter2 < 16 ) { //button is pressed and counter2 <16
        buttonPushCounter2++; // increase buttonPushCounter2
      }
    }

    if (sensorValue < 512 ) { // if upper half of potentiometer
      if (buttonStateMinus == HIGH && buttonPushCounter1 > 1 ) { //button is pressed and counte1r >1
        buttonPushCounter1--; // decrease buttonPushCounter1
      }
    }

    if (sensorValue > 512 ) { // if upper half of potentiometer
      if (buttonStateMinus == HIGH && buttonPushCounter2 > 1 ) { //button is pressed and counter2 >1
        buttonPushCounter2--; // decrease buttonPushCounter2
      }
    }

  }
  if (buttonStatePlus == HIGH && buttonStateMinus == HIGH) { //if both buttons is pressed do:
    buttonPushCounter1 = 8; //set light1 to level eight
    buttonPushCounter2 = 8; //set light2 to level eight
  }

  //sets the outputValue1 according to buttonPushCounter1

  if (buttonPushCounter1 == 1){ 
    outputValue1 = 1;
  }
  if (buttonPushCounter1 == 2){ 
    outputValue1 = 4; 
  }
  if (buttonPushCounter1 == 3){ 
    outputValue1 = 9; 
  }
  if (buttonPushCounter1 == 4){ 
    outputValue1 = 14; 
  }
  if (buttonPushCounter1 == 5){ 
    outputValue1 = 20; 
  }
  if (buttonPushCounter1 == 6){ 
    outputValue1 = 27; 
  }
  if (buttonPushCounter1 == 7){ 
    outputValue1 = 35; 
  }
  if (buttonPushCounter1 == 8){ 
    outputValue1 = 44; 
  }
  if (buttonPushCounter1 == 9){ 
    outputValue1 = 55; 
  }
  if (buttonPushCounter1 == 10){ 
    outputValue1 = 69; 
  }
  if (buttonPushCounter1 == 11){ 
    outputValue1 = 86; 
  }
  if (buttonPushCounter1 == 12){ 
    outputValue1 = 107; 
  }
  if (buttonPushCounter1 == 13){ 
    outputValue1 = 133; 
  }
  if (buttonPushCounter1 == 14){ 
    outputValue1 = 165; 
  }
  if (buttonPushCounter1 == 15){ 
    outputValue1 = 206; 
  }
  if (buttonPushCounter1 == 16){ 
    outputValue1 = 255; 
  }

  //sets the outputValue2 according to buttonPushCounter2

  if (buttonPushCounter2 == 1){ 
    outputValue2 = 1;
  }
  if (buttonPushCounter2 == 2){ 
    outputValue2 = 4; 
  }
  if (buttonPushCounter2 == 3){ 
    outputValue2 = 9; 
  }
  if (buttonPushCounter2 == 4){ 
    outputValue2 = 14; 
  }
  if (buttonPushCounter2 == 5){ 
    outputValue2 = 20; 
  }
  if (buttonPushCounter2 == 6){ 
    outputValue2 = 27; 
  }
  if (buttonPushCounter2 == 7){ 
    outputValue2 = 35; 
  }
  if (buttonPushCounter2 == 8){ 
    outputValue2 = 44; 
  }
  if (buttonPushCounter2 == 9){ 
    outputValue2 = 55; 
  }
  if (buttonPushCounter2 == 10){ 
    outputValue2 = 69; 
  }
  if (buttonPushCounter2 == 11){ 
    outputValue2 = 86; 
  }
  if (buttonPushCounter2 == 12){ 
    outputValue2 = 107; 
  }
  if (buttonPushCounter2 == 13){ 
    outputValue2 = 133; 
  }
  if (buttonPushCounter2 == 14){ 
    outputValue2 = 165; 
  }
  if (buttonPushCounter2 == 15){ 
    outputValue2 = 206; 
  }
  if (buttonPushCounter2 == 16){ 
    outputValue2 = 255; 
  }


  // save the current state as the last state,
  //for next time through the loop
  lastButtonStatePlus = buttonStatePlus;
  lastButtonStateMinus = buttonStateMinus;

  //light end-of-range LED when outputValue is either 1 or 255
  if (sensorValue < 512) { //if lower half
    if (outputValue1 == 255 || outputValue1 == 1)  {
      digitalWrite(ledPin, HIGH);
    } 
    else {
      digitalWrite(ledPin, LOW);
    }
  }

  if (sensorValue >= 512) { //if upper half
    if (outputValue2 == 255 || outputValue2 == 1)  {
      digitalWrite(ledPin, HIGH);
    } 
    else {
      digitalWrite(ledPin, LOW);
    }
  }

  // set the brightness of outputpins
  analogWrite(analogOutPin1, outputValue1);          
  analogWrite(analogOutPin2, outputValue2);          

  // wait 10 milliseconds before the next loop
  // for the analog-to-digital converter to settle
  // after the last reading:
  delay(10);                    
}
 if (buttonPushCounter1 == 1){ 
    outputValue1 = 1;
  }
  if (buttonPushCounter1 == 2){ 
    outputValue1 = 4; 
  }
  if (buttonPushCounter1 == 3){ 
    outputValue1 = 9; 
  }
  if (buttonPushCounter1 == 4){ 
    outputValue1 = 14; 
  }
  if (buttonPushCounter1 == 5){ 
    outputValue1 = 20; 
  }
  if (buttonPushCounter1 == 6){ 
    outputValue1 = 27; 
  }
  if (buttonPushCounter1 == 7){ 
    outputValue1 = 35; 
  }
  if (buttonPushCounter1 == 8){ 
    outputValue1 = 44; 
  }
  if (buttonPushCounter1 == 9){ 
    outputValue1 = 55; 
  }
  if (buttonPushCounter1 == 10){ 
    outputValue1 = 69; 
  }
  if (buttonPushCounter1 == 11){ 
    outputValue1 = 86; 
  }
  if (buttonPushCounter1 == 12){ 
    outputValue1 = 107; 
  }
  if (buttonPushCounter1 == 13){ 
    outputValue1 = 133; 
  }
  if (buttonPushCounter1 == 14){ 
    outputValue1 = 165; 
  }
  if (buttonPushCounter1 == 15){ 
    outputValue1 = 206; 
  }
  if (buttonPushCounter1 == 16){ 
    outputValue1 = 255; 
  }

No-one likes typing that much:

const byte brightnessLookup [] = {0, 1, 4, 9, 14, 20, 27 etc };
// later
outputValue1 = brightnessLookup [buttonPushCounter1 & 0x1f] ;

AWOL:
No-one likes typing that much:

const byte brightnessLookup [] = {0, 1, 4, 9, 14, 20, 27 etc };

// later
outputValue1 = brightnessLookup [buttonPushCounter1 & 0x1f] ;

You are a TRUE hero, AWOL! <3

Well, maybe not, and you may need to do a little better than I indicated on the bounds-checking for the array, but you get the idea.

It works like a charm here. :slight_smile:

Do a little better on the bounds-checking, why?

Thanks a bunch!

Do a little better on the bounds-checking, why?

I haven't read your code very closely, but if buttonPushCounter1 gets bigger than the number of entries (or less than zero) in your array, the results may be unpredictable.

I googled for bounds checking and realized that now :wink:

The code however does not increases buttonpushcounters to more than 16 and not below 1 so I think that should be okay?

if (buttonStatePlus != lastButtonStatePlus || buttonStateMinus != lastButtonStateMinus) {
    if (sensorValue < 512 ) { // if lower half of potentiometer
      if (buttonStatePlus == HIGH && buttonPushCounter1 < 16 ) { //button is pressed and counter1 <16
        buttonPushCounter1++; // increase buttonPushCounter1
      }
    }

    if (sensorValue > 512 ) { // if lower half of potentiometer
      if (buttonStatePlus == HIGH && buttonPushCounter2 < 16 ) { //button is pressed and counter2 <16
        buttonPushCounter2++; // increase buttonPushCounter2
      }
    }

    if (sensorValue < 512 ) { // if upper half of potentiometer
      if (buttonStateMinus == HIGH && buttonPushCounter1 > 1 ) { //button is pressed and counte1r >1
        buttonPushCounter1--; // decrease buttonPushCounter1
      }
    }

    if (sensorValue > 512 ) { // if upper half of potentiometer
      if (buttonStateMinus == HIGH && buttonPushCounter2 > 1 ) { //button is pressed and counter2 >1
        buttonPushCounter2--; // decrease buttonPushCounter2
      }
    }

  }

Another question I have is about the reset light part below.
Is it possible to only activarte this if the buttons are pressed together for a period of two seconds?

 if (buttonStatePlus == HIGH && buttonStateMinus == HIGH) { //if both buttons is pressed do:
    buttonPushCounter1 = 8; //set light1 to level eight
    buttonPushCounter2 = 8; //set light2 to level eight
    lastButtonStatePlus = LOW;
    lastButtonStateMinus = LOW;
  }

Is it possible to only activarte this if the buttons are pressed together for a period of two seconds?

Yes. You need to keep track of the previous state of each button, so that you can detect a transition - from pressed to released or from released to pressed.

At each transition, record the time - timePressed1, timeReleased1, timePressed2, timeReleased2.

If, at any point you see that both switches are pressed, set a flag (both = true).

On each pass through loop, if both buttons are released, both is true, and the time that each button was held down (timeReleasedN - timePressedN) is greater than your threshold, perform the action.

The state is known. But I do not know how to record the time a button is pressed?

It would be even better if the reset kicked in without the need to release the two buttons, ie like it works now only adding a need to press down for two seconds instead of resetting immediately as it does now.

But I do not know how to record the time a button is pressed?

Use "millis()"

The state is known. But I do not know how to record the time a button is pressed?

Assuming the appropriate variables are defined, with the proper type:

  int currState = digitalRead(switchPin);
  if(currState != prevState)
  {
     if(currState == HIGH)
        timePressed = millis();
     else
     {
        timeReleased = millis();
        if(timeReleased - timePressed > someTime)
        {
          // Switch was held...
        }
        else
        {
          // Momentary press and release
        }
     }
  }

I have tried PaulS solution for some hours now, so far without any success.
Now I have removed that code so I can not post it here. :-/

I can not really see how the code is supposed to work, is my own comments is in the code correct?

int currState = digitalRead(switchPin); // set currState to the state of switchpin
  if(currState != prevState) //if currState is not the same as previously prevState do:
  {
     if(currState == HIGH) //when currState is high 
        timePressed = millis(); //set timePressed to the amount of milliseconds since the program started
     else 
     {
        timeReleased = millis(); //if currstate not high, set timeReleased to millis()
        if(timeReleased - timePressed > someTime) // if timeReleased minus timePressed > 2000 (my value)
        {
          // Switch was held... //code to execute goes here
        }
        else
        {
          // Momentary press and release //code for normal operations goes here
        }
     }
  }

is my own comments is in the code correct?

Yes, your comments are correct.

I can not really see how the code is supposed to work

It is first determining that a transition occurred - that is that a change from pressed to released or from released to pressed occurred.

If so, it checks if the switch was just pressed. If so, it records the time.
Otherwise, the switch was just released. So, it records the time.

Finally, it determines how long the switch was held down. The switch was either briefly pressed and released, or it was pressed, held for a while, then released.

The program assumes that HIGH means the switch is pressed. If that is not the case, the HIGH needs to be changed to LOW to make it behave correctly.

This is a bit strange.

When I use the code for one switch only, ie PaulS design direct everything works well.
If I change the code it does not work any longer. What is wrong with my code below?

const int switchPin1 = 3;
const int switchPin2 = 2;
int prevState1;
int prevState2;
unsigned long timePressed;
unsigned long timeReleased;

void setup() {
  pinMode(switchPin1, INPUT);  // initialize the button pin1 plus as a input:
    pinMode(switchPin2, INPUT);  // initialize the button pin2 plus as a input:
   Serial.begin(9600);
}

void loop () {
  

int currState1 = digitalRead(switchPin1); // set currState to the state of switchpin1
int currState2 = digitalRead(switchPin2); // set currState to the state of switchpin2

  if (currState1 != prevState1 || currState2 != prevState2) //if currState1 or -2  is not the same as previously prevState1 or -2 do:
  {

     if(currState1 == HIGH && currState2 == HIGH) { //when both currStates are high
             timePressed = millis(); //set timePressed to the amount of milliseconds since the program started
         Serial.println("buttons pressed");
     }
     else 
     {
        timeReleased = millis(); //if currstate1 & -2 not high, set timeReleased to millis()
        if(timeReleased - timePressed > 2000) // if timeReleased minus timePressed > 2000 (my value)
        {
          Serial.println("<2000");// Switch was held... //code to execute goes here
        }
        else
        {
          // Momentary press and release //code for normal operations goes here
        }
     }
  }
  prevState1 = currState1;
    prevState2 = currState2;
  
}

If I change the code it does not work any longer. What is wrong with my code below?

"It doesn't work" doesn't cut it here. The code does something. You need to tell us what that is.

You expect something else. You need to tell us what that is.

Then, we can either help you fix the code to do what you want, or change your expectations to match reality.

I'm going to guess it is going to be something to do with de Morgan's, but as Pauls said, we need some observations

Ok.

Bear with me then. :wink:

When I press only one button it writes "<2000" which it should not.
When I press booth buttons I get the "buttons pressed" message direct and not when releasing the buttons. After that it will not print the faulty <2000 message for approximately 2 secs.

When you push one button, this "did either button transition" code evaluates to true.

  if (currState1 != prevState1 || currState2 != prevState2)

So, this statement is evaluated:

     if(currState1 == HIGH && currState2 == HIGH) { //when both currStates are high

Well, both buttons are not pressed, so this is false. So, the else code is executed:

        timeReleased = millis(); //if currstate1 & -2 not high, set timeReleased to millis()
        if(timeReleased - timePressed > 2000)
        {
          Serial.println("<2000");// Switch was held... //code to execute goes here
        }
        else
        {
          // Momentary press and release //code for normal operations goes here
        }

Both buttons were never pressed at the same time, so timePressed was never set, so it is 0. If the Arduino has been running for more than 2 seconds, timeReleased will be greater than 2000, so the print statement will be executed. That prints less than 2000, which is clearly not true. Change the < to >.

If the Arduino has been running for less than two seconds, nothing happens. Probably ought to put something there.

Thank you for trailing that out PaulS!

But... How can I make it work then?

if (currState1 != prevState1 || currState2 != prevState2)

It would of course be better to have "and" instead of "or" here, but since I can not press both exactly at the same time && will never be true.

That prints less than 2000, which is clearly not true. Change the < to >.

My bad :blush: