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