# simplify code

hi how can I simplify the following code?
with a for loop...

`````` if (buttonState == HIGH)
{
myArray = 2;
}
if (buttonState != lastButtonState)
{
if (buttonState == LOW)
{
myArray = 0;
}
// Delay a little bit to avoid bouncing
delay(5);
}
// save the current state as the last state, for next time through the loop
lastButtonState = buttonState;
//************************************************************************************
if (buttonState == HIGH)
{
myArray = 3;
}
if (buttonState != lastButtonState)
{
if (buttonState == LOW)
{
myArray = 0;
}
// Delay a little bit to avoid bouncing
delay(5);
}
// save the current state as the last state, for next time through the loop
lastButtonState = buttonState;
``````

hi how can I simplify the following code?
with a for loop...

Yes. Do you really need help writing a for statement?

Use the Bounce2 library, then put it in a for loop.

PaulS:
Yes. Do you really need help writing a for statement?

yes i need help writing the for loop, i tried unsuccesful

``````bool buttonStates[] = {buttonState, buttonState, lastButtonState, lastButtonState};

for ( byte x = 0 ; x < 2 ; x ++ ) {
if ( buttonStates[x] != buttonStates[x+2] ) {
if ( buttonStates[x] == LOW) {
myArray = 0;
}
// Delay a little bit to avoid bouncing
delay(5);
}
// save the current state as the last state, for next time through the loop
buttonStates[x+2] = buttonStates[x];

}
``````

destiny2008:
yes i need help writing the for loop, i tried unsuccesful

It will be easier to provide useful help if you post the program (complete program, please) that represents your best attempt. That way we can see what you are having difficulty with.

...R

Robin2:
It will be easier to provide useful help if you post the program (complete program, please) that represents your best attempt. That way we can see what you are having difficulty with.

…R

here is the working full code:.

``````#include <nRF24L01.h>
#include <printf.h>
#include <RF24.h>
#include <RF24_config.h>

const uint64_t pipe = 0xE8E8F0F0E1LL;

const uint8_t buttonPin[] = {2, 3, 4};
uint8_t myArray;

bool buttonState[] = {0, 0, 0};       // current state of the button
bool lastButtonState[] = {0, 0, 0};   // previous state of the button

void setup(void)
{
Serial.begin(115200);
printf_begin();
for (uint8_t Pin = 2; Pin < 4 ; Pin++)
{
pinMode(Pin, INPUT);
}
}

void loop(void)
{
for (uint8_t j = 0; j <= 2; j++)
{
}

//**********************************************************************
if (buttonState == HIGH)
{
myArray = 1;
}
if (buttonState != lastButtonState)
{
if (buttonState == LOW)
{
myArray = 0;
}
// Delay a little bit to avoid bouncing
delay(5);
}
// save the current state as the last state, for next time through the loop
lastButtonState = buttonState;
//**************************************************************************************
if (buttonState == HIGH)
{
myArray = 2;
}
if (buttonState != lastButtonState)
{
if (buttonState == LOW)
{
myArray = 0;
}
// Delay a little bit to avoid bouncing
delay(5);
}
// save the current state as the last state, for next time through the loop
lastButtonState = buttonState;
//************************************************************************************
if (buttonState == HIGH)
{
myArray = 3;
}
if (buttonState != lastButtonState)
{
if (buttonState == LOW)
{
myArray = 0;
}
// Delay a little bit to avoid bouncing
delay(5);
}
// save the current state as the last state, for next time through the loop
lastButtonState = buttonState;
}
``````

here is the working full code:.

With a properly written for statement and body that is clearly too short.

Move the first block of code after the for statement body into the body of the for statement, and make the necessary edits (to use j instead of 0). Then, delete the repetitive code.

Unconditionally sending one piece of data when the pin is HIGH and conditionally (when the pin becomes LOW) sending another piece of data does not seem like the right thing to do.

PaulS:
With a properly written for statement and body that is clearly too short.

Move the first block of code after the for statement body into the body of the for statement, and make the necessary edits (to use j instead of 0). Then, delete the repetitive code.

Unconditionally sending one piece of data when the pin is HIGH and conditionally (when the pin becomes LOW) sending another piece of data does not seem like the right thing to do.

do you have a better way to do it?

do you have a better way to do it?

Yes. Do what?

PaulS:
Yes. Do what?

"sending another piece of data is not the right thing to do"

destiny2008:
“sending another piece of data is not the right thing to do”

Here’s what I think you should have:

``````void loop(void)
{
for (uint8_t j = 0; j <= 2; j++)
{
if (buttonState[j] != lastButtonState[j])
{
if (buttonState[j] == LOW)
{
myArray = 0;
}
else
{
myArray = 1;
}
}
lastButtonState[j] = buttonState[j];
}
}
``````

This will send one value when the switch changes state. The value that it sends will depend on the state that was changed to.

You could even simplify this, since the value to send corresponds to the new state:

``````void loop(void)
{
for (uint8_t j = 0; j <= 2; j++)
{
if (buttonState[j] != lastButtonState[j])
{
}
lastButtonState[j] = buttonState[j];
}
}
``````

PaulS:
You could even simplify this, since the value to send corresponds to the new state:

``````void loop(void)
``````

{
for (uint8_t j = 0; j <= 2; j++)
{
if (buttonState[j] != lastButtonState[j])
{
}
lastButtonState[j] = buttonState[j];
}
}

the following works but all 3 buttons repond to the same LED . is it because of myArray missing?

here is my rx code , is it correct?

``````const uint8_t LED[] = {2, 3, 4};

uint8_t myArray;
``````
``````void loop()
{

{

//Light up the correct LEDs
for (uint8_t j = 1; j <= 3; j++)
{
if (myArray == j) {
digitalWrite(LED[j-1], HIGH);
}
if (myArray == 0) {
digitalWrite(LED[j-1], LOW);

}
}
}
}
``````

the following works but all 3 buttons repond to the same LED . is it because of myArray missing?

No, it is because you are not sending enough information. In addition to the state of the pin, you need to send the pin index, so you know to set the state of pin[index] to the appropriate value.

You'll need to restore myArray, but make it hold two elements - the index (j) and the state (buttonState[j]).

On the receiver, you need to size the array appropriately, too, and use both pieces of data.

PaulS:
No, it is because you are not sending enough information. In addition to the state of the pin, you need to send the pin index, so you know to set the state of pin[index] to the appropriate value.

You’ll need to restore myArray, but make it hold two elements - the index (j) and the state (buttonState[j]).

On the receiver, you need to size the array appropriately, too, and use both pieces of data.

this is what is working on tx for me :
can it be any simpler?

``````void loop(void)
{
for (uint8_t j = 0; j <= 2; j++)
{
if (buttonState[j] == HIGH)
{
myArray = (j + 1);
}
if (buttonState[j] != lastButtonState[j])
{
if (buttonState[j] == LOW)
{
myArray = 0;
}
delay(5);
}
lastButtonState[j] = buttonState[j];
}
}
``````
``````	  buttonState[i] = digitalRead(buttonPin[i]);
if (buttonState[j] == HIGH)
``````

Store a value in the ith position, and then check the value in the jth position. Hmmm...

can it be any simpler?

Simpler? It is far too early to be talking about simplify code that is WRONG.

You have gone back to sending a value when the pin IS high and another when the pin BECOMES low.

That is WRONG!

PaulS:

``````	  buttonState[i] = digitalRead(buttonPin[i]);
``````

if (buttonState[j] == HIGH)

``````

Store a value in the ith position, and then check the value in the jth position. Hmmm...
Simpler? It is far too early to be talking about simplify code that is WRONG.

You have gone back to sending a value when the pin IS high and another when the pin BECOMES low.

That is WRONG!
``````

sorry posted before the fix :

``````	  buttonState[j] = digitalRead(buttonPin[j]);
if (buttonState[j] == HIGH)
``````

but don’t see another way not to send another LOW and have it instruct the RX to turn led off.
unless you have an example tx and rx to turn it off without sending a high/low radio write, I don’t see it sorry.
not an expert at this.

You should ONLY send data when there has been a state change. The switch became pressed, so send one value. The switch became released, so send a different value. That's the way the code in reply #12 worked.

You can't just add the index value and the to-be state to form the value to send. index = 0 and state = 1 adds up to 1. So does index = 1 and state = 0. So, the receiver wouldn't know what to do to what pin.

You could, if you insist on sending just one byte, use value = index * 10 + state, which would work until you have more than 9 pins.

What's so hard about sending two bytes - one containing the index and one containing the state?

PaulS:
You should ONLY send data when there has been a state change. The switch became pressed, so send one value. The switch became released, so send a different value. That's the way the code in reply #12 worked.

You can't just add the index value and the to-be state to form the value to send. index = 0 and state = 1 adds up to 1. So does index = 1 and state = 0. So, the receiver wouldn't know what to do to what pin.

You could, if you insist on sending just one byte, use value = index * 10 + state, which would work until you have more than 9 pins.

What's so hard about sending two bytes - one containing the index and one containing the state?

so at the end of the day you will need only 1 radio write statement doing all this? or 2 ?

so at the end of the day you will need only 1 radio write statement doing all this?

Yes.

The statement will write 2 bytes, though.