Any advice on my code

Hello, i am posting my esp8266, EspNow code so that more experimented people could advice if something is wrong.
i have a board with 4 buttons that send some data to another board that receives it and based on the data corresponding to a button, it powers one of the relays with 1 sec delay between state changes also only one relay at the time is allowed to be powered. the button on the receivers board also act like a transmitter by changing the data received into what it needs to power the according relay. d button enter maintenance mode so that no relay can be powered unless that mode goes of by the same tx board who activated it based on id number

/*
 * TRANSMITTER
 */

//libraries
#include <ESP8266WiFi.h>
#include <espnow.h>

//esp8266 transmitter object
uint8_t broadcastAddress[] = {0xCC, 0x50, 0xE3, 0x3C, 0xA8, 0x4B}; //adress for master receiver
#define BOARD_ID 1

//esp data to send
typedef struct struct_message {
  int id;
  char letter;
}struct_message;
struct_message espDataA;
struct_message espDataB;
struct_message espDataC;
struct_message espDataD;

//button variables
//pins; D1-GPIO5,D2-GPIO4,D5-GPIO14,D6-GPIO12,D7-GPIO13
const int buttonA = 5; // GPIO 5 = D1
const int buttonB = 4; // GPIO 4 = D2
const int buttonC = 14; // GPIO 14 = D5
const int buttonD = 12; // GPIO 12 = D6
int buttonStateA = HIGH;
int lastButtonStateA = HIGH;
int buttonStateB = HIGH;
int lastButtonStateB = HIGH;
int buttonStateC = HIGH;
int lastButtonStateC = HIGH;
int buttonStateD = HIGH;
int lastButtonStateD = HIGH;
unsigned long lastDebounceTime = 0;
unsigned long debounceDelay = 50;

//esp send data function
void OnDataSent(uint8_t *mac_addr, uint8_t sendStatus) {
  if (sendStatus == 0) {
    Serial.println("Delivery success");
  }
  else {
    Serial.println("Delivery fail");
  }
}

void setup() {

  //radio temporary
  Serial.begin(115200);

  //buttons initialization
  pinMode(buttonA, INPUT_PULLUP);
  pinMode(buttonB, INPUT_PULLUP);
  pinMode(buttonC, INPUT_PULLUP);
  pinMode(buttonD, INPUT_PULLUP);

  //data to send
  espDataA.letter = 'A';
  espDataB.letter = 'B';
  espDataC.letter = 'C';
  espDataD.letter = 'D';
  espDataA.id = BOARD_ID;
  espDataB.id = BOARD_ID;
  espDataC.id = BOARD_ID;
  espDataD.id = BOARD_ID;
  
  //Set device as a Wi-Fi Station
  WiFi.mode(WIFI_STA);
  WiFi.disconnect();
  
  // Init ESP-NOW
  if (esp_now_init() != 0) {
    Serial.println("Error initializing ESP-NOW");
    Serial.flush();
    ESP.restart();
    return;
  }
  esp_now_set_self_role(ESP_NOW_ROLE_CONTROLLER);
  esp_now_register_send_cb(OnDataSent);
  esp_now_add_peer(broadcastAddress, ESP_NOW_ROLE_SLAVE, 1, NULL, 0);
  ESP.wdtDisable();
}

void loop() {

  //feed watchdog
  ESP.wdtFeed();

  //get buttons values
  int readingA = digitalRead(buttonA);
  int readingB = digitalRead(buttonB);
  int readingC = digitalRead(buttonC);
  int readingD = digitalRead(buttonD);

  //debounce
  if ((readingA != lastButtonStateA) || (readingB != lastButtonStateB) || (readingC != lastButtonStateC) || (readingD != lastButtonStateD)) {
    lastDebounceTime = millis();
  }

  //execute if button pressed
  if ((millis() - lastDebounceTime) > debounceDelay) {

    if (readingA != buttonStateA) {
      buttonStateA = readingA;
      if (buttonStateA == LOW) {
        esp_now_send(broadcastAddress, (uint8_t *) &espDataA, sizeof(espDataA));
      }
    } else if (readingB != buttonStateB) {
      buttonStateB = readingB;
      if (buttonStateB == LOW) {
        esp_now_send(broadcastAddress, (uint8_t *) &espDataB, sizeof(espDataB));
      }
    } else if (readingC != buttonStateC) {
      buttonStateC = readingC;
      if (buttonStateC == LOW) {
        esp_now_send(broadcastAddress, (uint8_t *) &espDataC, sizeof(espDataC));
      }
    } else if (readingD != buttonStateD) {
      buttonStateD = readingD;
      if (buttonStateD == LOW) {
        esp_now_send(broadcastAddress, (uint8_t *) &espDataD, sizeof(espDataD));
      }
    }

  }

  //update button state
  lastButtonStateA = readingA;
  lastButtonStateB = readingB;
  lastButtonStateC = readingC;
  lastButtonStateD = readingD;

}
/*
 * RECEIVER
 */
 
//libraries
#include <ESP8266WiFi.h>
#include <espnow.h>

//board id
#define BOARD_ID 0

//esp data to receive
typedef struct struct_message {
  int id;
  char letter;
} struct_message;
struct_message espData;
boolean ReceivedData = false;

//relays variables
//pins; D1-GPIO5,D2-GPIO4,D5-GPIO14,D6-GPIO12,D7-GPIO13
const int relayA = 5; // GPIO 5 = D1
const int relayB = 4; // GPIO 4 = D2
const int relayC = 14; // GPIO 14 = D5
boolean RelayState = LOW;
char wichRelayState = 'O';

//button variables
const int mentenance = 12; // GPIO 12 = D6
int buttonStateMentenance = HIGH;
int lastButtonStateMentenance = HIGH;
unsigned long lastDebounceTime = 0;
unsigned long debounceDelay = 50;

//variables for mentenance
const int ledPin = 13; // GPIO 13 = D7
int ledState = HIGH;
boolean mentenanceOn = true;

//main board relays button
int pressCount = 0;
unsigned long lastTimePress = 0;
unsigned long Delay = 1500;
int wichIdPressedD = -1;

//function for receiving
void OnDataRecv(uint8_t * mac_addr, uint8_t *incomingData, uint8_t len) {
  memcpy(&espData, incomingData, sizeof(espData));
  ReceivedData = true;
  Serial.printf("Board ID %d \n", espData.id);
  Serial.printf("letter value: %d \n", espData.letter);
  Serial.println();
}

void delayFct (){
    ESP.wdtFeed();
    delay(1000);
  }

void setup() {

  // Initialize Serial Monitor
  Serial.begin(115200);

  //relays initialization
  digitalWrite(relayA, LOW);
  digitalWrite(relayB, LOW);
  digitalWrite(relayC, LOW);

  //led initialize
  digitalWrite(ledPin, ledState);

  pinMode(relayA, OUTPUT); // OUTPUT 5V
  pinMode(relayB, OUTPUT);
  pinMode(relayC, OUTPUT);

  //buttons initialization
  pinMode(mentenance, INPUT_PULLUP);

  //led initialize
  pinMode(ledPin, OUTPUT);

  espData.letter = 'O';
  espData.id = -1;

  //Set device as a Wi-Fi Station
  WiFi.mode(WIFI_STA);
  WiFi.disconnect();

  // Init ESP-NOW
  if (esp_now_init() != 0) {
    Serial.println("Error initializing ESP-NOW");
    Serial.flush();
    ESP.restart();
    return;
  }

  // Once ESPNow is successfully Init, we will register for recv CB to
  // get recv packer info
  esp_now_set_self_role(ESP_NOW_ROLE_SLAVE);
  esp_now_register_recv_cb(OnDataRecv);
  ESP.wdtDisable();
}

void loop() {

  //feed watchdog
  ESP.wdtFeed();

  //get buttons values
  int readingMentenance = digitalRead(mentenance);

  //debounce
  if (readingMentenance != lastButtonStateMentenance) {
    lastDebounceTime = millis();
  }

  //execute if button pressed
  if ((millis() - lastDebounceTime) > debounceDelay) {

    if (readingMentenance != buttonStateMentenance) {
      buttonStateMentenance = readingMentenance;
      if (buttonStateMentenance == LOW) {
        pressCount ++;
        lastTimePress = millis();
      }
    }

    if (((millis() - lastTimePress) > Delay) && (pressCount > 0)) {
      if (pressCount == 1) {
        espData.letter = 'A';
        ReceivedData = true;
        espData.id = BOARD_ID;
      } else if (pressCount == 2) {
        espData.letter = 'B';
        ReceivedData = true;
        espData.id = BOARD_ID;
      } else if (pressCount == 3) {
        espData.letter = 'C';
        ReceivedData = true;
        espData.id = BOARD_ID;
      } else if (pressCount == 4) {
        espData.letter = 'D';
        ReceivedData = true;
        espData.id = BOARD_ID;
      }
      pressCount = 0;
    }

  }

  //if we got new data, activate the relays
  if (ReceivedData == true) {

    if (espData.letter == 'D') {

      if (mentenanceOn == false) {

        wichIdPressedD = espData.id;
        mentenanceOn = !mentenanceOn;
        digitalWrite(ledPin, !ledState);

        if (RelayState == LOW) {
          delayFct();
        } else if (RelayState == HIGH) {
          digitalWrite(relayA, LOW);
          digitalWrite(relayB, LOW);
          digitalWrite(relayC, LOW);
          RelayState = LOW;
          delayFct();
        }

      } else {

        if ((wichIdPressedD == espData.id) || (wichIdPressedD == -1)) {
          mentenanceOn = !mentenanceOn;
          digitalWrite(ledPin, !ledState);
          delayFct();
        }
      }

    } else {

      if (mentenanceOn == false) {
        switch (espData.letter) {

          case 'A':
            if (RelayState == LOW) {
              digitalWrite(relayA, HIGH);
              RelayState = HIGH;
              delayFct();

            } else if (RelayState == HIGH) {

              switch (wichRelayState) {
                case 'A':
                  digitalWrite(relayA, LOW);
                  delayFct();
                  RelayState = LOW;
                  break;

                case 'B':
                  digitalWrite(relayB, LOW);
                  delayFct();
                  digitalWrite(relayA, HIGH);
                  RelayState = HIGH;
                  break;

                case 'C':
                  digitalWrite(relayC, LOW);
                  delayFct();
                  digitalWrite(relayA, HIGH);
                  RelayState = HIGH;
                  break;

              }
            }
            wichRelayState = 'A';
            break;

          case 'B':
            if (RelayState == LOW) {

              digitalWrite(relayB, HIGH);
              RelayState = HIGH;
              delayFct();

            } else if (RelayState == HIGH) {

              switch (wichRelayState) {
                case 'A':
                  digitalWrite(relayA, LOW);
                  delayFct();
                  digitalWrite(relayB, HIGH);
                  RelayState = HIGH;
                  break;

                case 'B':
                  digitalWrite(relayB, LOW);
                  delayFct();
                  RelayState = LOW;
                  break;

                case 'C':
                  digitalWrite(relayC, LOW);
                  delayFct();
                  digitalWrite(relayB, HIGH);
                  RelayState = HIGH;
                  break;

              }
            }
            wichRelayState = 'B';
            break;

          case 'C':
            if (RelayState == LOW) {

              digitalWrite(relayC, HIGH);
              RelayState = HIGH;
              delayFct();

            } else if (RelayState == HIGH) {

              switch (wichRelayState) {
                case 'A':
                  digitalWrite(relayA, LOW);
                  delayFct();
                  digitalWrite(relayC, HIGH);
                  RelayState = HIGH;
                  break;

                case 'B':
                  digitalWrite(relayB, LOW);
                  delayFct();
                  digitalWrite(relayC, HIGH);
                  RelayState = HIGH;
                  break;

                case 'C':
                  digitalWrite(relayC, LOW);
                  delayFct();
                  RelayState = LOW;
                  break;

              }
            }
            wichRelayState = 'C';
            break;

          default:
            espData.letter = 'O';
            espData.id = -1;
            break;
        }
      } else {
        //mentenace is ON and D was not pressed
      }
    }

    ReceivedData = false;
  }

  //update button state
  lastButtonStateMentenance = readingMentenance;
}

First advice:
write down a description that uses much more sentences to explain the functionality. From some sentences I assume there is only one receiver from other sentences there might be multiple receivers

or as an alternative describe an example how your devices work together.
There is one device with 4 buttons
let's name this device the 4buttonESP
you press button "B" on the 4buttonESP
4buttonESP will send a "B" over ESP-NOW

the (one or the mutliple-receiver-devices??) receive the "B"
what happens then??? Describe it step by step

Second advice: don't nest your code inside loop.
divide your code into functions where each function does one senseful thing.

There are too many elses with too less comments what each else means
to follow your code.

If you don't get more answers it is very likely because it is hard to analyse the functionality.

best regards Stefan

Indent and spacing not according to the rest of the sketch :scream:

void delayFct (){
    ESP.wdtFeed();
    delay(1000);
  }

A library for debouncing would make the sketch easier to read.

I agree with StefanL38, the code with the buttons and the code with the if-switch-case-if-switch-something does not feel right.

i understand it is hard to read but it i am not that experimented in writing arduino code , i am happy that it works so far. i can explaing it better ; what is wrong with this?, i was worried about if wtd is feed in delay lib so i did that anyway just in case
void delayFct (){ ESP.wdtFeed(); delay(1000); }

so i have one esp8266 board as the only one receiver who controls some contactors. is has attached to it 3 relays, relayA , relayB, relayC, a push button called mentenance and an led.
when the board boots up it enters in maintenance mode that means all relays turn of (if the aren't already), the led turn on and mentenanceOn variable turn is true. in this stage the board won't trigger any relay even if it get a signal to do so. it is waiting for signal D wich will unlock the maintenance mode, turn led of.. and now relays can trigger.
after debouncing and counting how many times in the Delay interval , the button on this board was pressed, if it was pressed once it will change espData.letter to A and espData.id to the id of this board wich is BOARD_ID 0 also anouncing that new data has received or in this case induced by the button ReceivedData = true.
each board has it's individual id and mac address. if the button it's pressed 4 times it will change that data to D which enter maintenance mode but now since the mode was off and want to turn on, it remembers which board id send the signal wichIdPressedD = 0 so when the mode should go off, only the same board can turn off the maintenance based on the id (who turned it On can only turn it Off exception first time when wichIdPressedD = -1).
if i received data ReceivedData == true;
it checks if the letter is D in witch case the maintenance would change to it's opposite state as described. otherwise if mainten.. is On and D was not pressed do nothing
if it is off then look for witch letter has been received let say A and based on that; if the relayA state is off just turn it on and RelayState = HIGH and remember wichRelayState is on ? = 'A'; changes. if it is already turned On that means i sended the signal to stop it so turn the relay off and wait 1 sec so the contactors won't go crazy if someone spams the buttons :))
if relayB is On and i send C then turn rlB off wait a second and turn relayC on, wait 1sec so that no relays won't be turned on at the same time

for the transmitter code i can have multiple boards with different id and 4 buttons each A,B,C,D

just confusing

one board?

or

multiple boards

???

you are mixing halfway writing about code with halfway writing general functionality.

This is a way of trying to describe it that I don't understand.

If english is not your native language you should write down the description in your native language and let do google-translate the translation.

make a decision if you want to describe the functionality by

avoiding each and every

programming term
or

pure code

as pure code makes everything clear what you mean instead of a fuzzy, foggy writing about

how many times did happen what and

which

delay?

best regards Stefan

a common problem is doing cut&paste duplicating code.

here's some code that scans multiple buttons

// check multiple buttons and toggle LEDs

enum { Off = HIGH, On = LOW };

byte pinsLed [] = { 10, 11, 12 };
byte pinsBut [] = { A1, A2, A3 };
#define N_BUT   sizeof(pinsBut)

byte butState [N_BUT];

// -----------------------------------------------------------------------------
int
chkButtons ()
{
    for (unsigned n = 0; n < sizeof(pinsBut); n++)  {
        byte but = digitalRead (pinsBut [n]);

        if (butState [n] != but)  {
            butState [n] = but;

            delay (10);     // debounce

            if (On == but)
                return n;
        }
    }
    return -1;
}

// -----------------------------------------------------------------------------
void
loop ()
{
    switch (chkButtons ())  {
    case 2:
        digitalWrite (pinsLed [2], ! digitalRead (pinsLed [2]));
        break;

    case 1:
        digitalWrite (pinsLed [1], ! digitalRead (pinsLed [1]));
        break;

    case 0:
        digitalWrite (pinsLed [0], ! digitalRead (pinsLed [0]));
        break;
    }
}

// -----------------------------------------------------------------------------
void
setup ()
{
    Serial.begin (9600);

    for (unsigned n = 0; n < sizeof(pinsBut); n++)  {
        pinMode (pinsBut [n], INPUT_PULLUP);
        butState [n] = digitalRead (pinsBut [n]);
    }

    for (unsigned n = 0; n < sizeof(pinsLed); n++)  {
        digitalWrite (pinsLed [n], Off);
        pinMode      (pinsLed [n], OUTPUT);
    }
}

in the above, couldn't the button press simply set a variable to the character to send and there only be one of the following using that variable

       esp_now_send(broadcastAddress, (uint8_t *) &espDataD, sizeof(espDataD));

not really sure what the code is trying to do. since we have code to see "how" is does things, it would be more helpful to read "what" the code is trying to do. what do the relays control?

in the above

  • if the "if" condition tests for LOW there's no need for an else if testing for HIGH, a simple else will do
  • i think the logic could probably be simply
delay (1000);
digitalWrite(wichRelay, LOW);
  • don't believe this code is that critical that it requires a watch dog timer (what should it do after a reset)?

Hi, @alex0512343
Can you draw a basic block diagram of your project, showing how many controllers you have and what is connected to them.
We do not need a full schematic at this time, just a block diagram explaining the basic layout of your project.

Thanks.. Tom... :grinning: :+1: :coffee: :australia: