Noob here, programming check/ guru wanted

Hi all,

First off, i am a noob at this (programming with arduino) and would like some advise or checkup for improving my code i've put together. (was intentionally to work with micropython, but esp-now not yet implemented)

The project i am working on is: waterlevel measurement with esp-now

for this project i have used 2 esp32 dev board and one relay board, the 2 esp32 boards are connected to each other with esp-now, the 'master' send a measured distance using the ultrasonic sensor HR-SR04 and then the 'slave' uses this value to make some of the relays on or off.

The project itself work with this code but i think i could use some tweaking:

Sender code:

#include <esp_now.h>
#include <WiFi.h>

uint8_t broadcastAddress[] = {0x24, 0x62, 0xAB, 0xD5, 0x15, 0x30};  //mac-adress ontvanger
int trigPin = 16;    // Trigger
int echoPin = 17;    // Echo
int redled = 2;     // Connection ok
long duration, cm, inches;



void setup() {

  Serial.begin(115200);
  WiFi.mode(WIFI_STA);
  Serial.println();
  Serial.println(WiFi.macAddress());
  if (esp_now_init() != ESP_OK) {
    Serial.println("Error initializing ESP-NOW");
    return;
  }
  // register peer
  esp_now_peer_info_t peerInfo;
  memcpy(peerInfo.peer_addr, broadcastAddress, 6);
  peerInfo.channel = 0;
  peerInfo.encrypt = false;
  if (esp_now_add_peer(&peerInfo) != ESP_OK) {
    Serial.println("Failed to add peer");
    return;
  }
  //Define inputs and outputs
  pinMode(trigPin, OUTPUT);
  pinMode(echoPin, INPUT);
  pinMode(redled, OUTPUT);
}



void loop()  {

  // The sensor is triggered by a HIGH pulse of 10 or more microseconds.
  // Give a short LOW pulse beforehand to ensure a clean HIGH pulse:
  digitalWrite(trigPin, redled, LOW);
  delayMicroseconds(5);
  digitalWrite(trigPin, HIGH);
  delayMicroseconds(10);
  digitalWrite(trigPin, LOW);

  // Read the signal from the sensor: a HIGH pulse whose
  // duration is the time (in microseconds) from the sending
  // of the ping to the reception of its echo off of an object.
  pinMode(echoPin, INPUT);
  duration = pulseIn(echoPin, HIGH);

  // Convert the time into a distance
  cm = (duration / 2) / 29.1;   // Divide by 29.1 or multiply by 0.0343
  inches = (duration / 2) / 74; // Divide by 74 or multiply by 0.0135

  Serial.print(inches);
  Serial.print("in, ");
  Serial.print(cm);
  Serial.print("cm");
  Serial.println();

  delay(5000);
  {
    // send data

    esp_err_t result = esp_now_send(broadcastAddress, (uint8_t *) &cm, sizeof(int));

    if (result == ESP_OK) {
      Serial.println("Sent with success");
      digitalWrite(redled, HIGH);
    }
    else {
      Serial.println("Error sending the data");
      digitalWrite(redled, LOW);
    }

  }
}

Receiver code:

#include <esp_now.h>
#include <WiFi.h>

int high = 16;  // Low level
int low = 17;  // High level
int fault = 18; // Wrong reading


void onReceiveData(const uint8_t *mac, const uint8_t *cm, int len) {

  Serial.print("Received from MAC: ");

  for (int i = 0; i < 6; i++) {

    Serial.printf("%02X", mac[i]);
    if (i < 5)Serial.print(":");
  }

  int * messagePointer = (int*)cm;
  Serial.println();
  Serial.println(*messagePointer);
  pinMode(high, OUTPUT);
  pinMode(low, OUTPUT);
  pinMode(fault, OUTPUT);
  digitalWrite(high, LOW);
  digitalWrite(low, LOW);

  if (*cm < 15)
    digitalWrite(high, HIGH);
  else
    digitalWrite(high, LOW);

  if (*cm > 30)
    digitalWrite(low, HIGH);
  else
    digitalWrite(low, LOW);

  if (*cm == 0)
    digitalWrite(fault, HIGH);
  else
    digitalWrite(fault, LOW);

}

void setup() {
  Serial.begin(115200);
  WiFi.mode(WIFI_STA);
  if (esp_now_init() != ESP_OK) {
    Serial.println("Error initializing ESP-NOW");
    return;
  }
  esp_now_register_recv_cb(onReceiveData);
  pinMode(high, OUTPUT);
  pinMode(low, OUTPUT);
}

void loop() {}}

Like i said this works but if the sender fails the receiver does not notice this, is there a way to program this ? Also i don't think it runs stable, first boot takes a while and need the restart sometimes...

digitalWrite(trigPin, redled, LOW); Oops.

  duration = pulseIn(echoPin, HIGH);

The 'pulseIn()' function returns 0 if no echo was detected. This means the object is either VERY close (less than 2 or 3 cm) or too far (echo is not strong enough to detect).

By using Logging library - ESP32 - — ESP-IDF Programming Guide latest documentation to print things, instead of serial, will give a performance increase; #include "sdkconfig.h".

Use ESP time counter for millis()/micros() instead of Arduino

oid fDo_AudioReadFreq( void *pvParameters )
{
  int FreqVal[7];
  const int NOISE = 10; // noise that you want to chop off
  const int A_D_ConversionBits = 4096; // arduino use 1024, ESP32 use 4096
  Analyzer Audio = Analyzer( 5, 15, 36 );//Strobe pin ->15  RST pin ->2 Analog Pin ->36
  Audio.Init(); // start the audio analyzer
  int64_t EndTime = esp_timer_get_time();
  int64_t StartTime = esp_timer_get_time(); //gets time in uSeconds like Arduino Micros
  for (;;)
  {
    xEventGroupWaitBits (eg, evtDo_AudioReadFreq, pdTRUE, pdTRUE, portMAX_DELAY);
    EndTime = esp_timer_get_time() - StartTime;
    log_i( "TimeSpentOnTasks: %d", EndTime );
    Audio.ReadFreq(FreqVal);
    for (int i = 0; i < 7; i++)
    {
      FreqVal[i] = constrain( FreqVal[i], NOISE, A_D_ConversionBits );
      FreqVal[i] = map( FreqVal[i], NOISE, A_D_ConversionBits, 0, 255 );
      // log_i( "Freq %d Value: %d", i, FreqVal[i]);//used for debugging and Freq choosing
    }
    xQueueSend( xQ_LED_Info, ( void * ) &FreqVal, 0 );
    StartTime = esp_timer_get_time();
  }
  vTaskDelete( NULL );
} // fDo_ AudioReadFreq( void *pvParameters )

esp_timer_get_time() is a micro second clock that rolls over in 200ish years. and saves a bit of time with code execution.

Using the ESP GPIO API to read GPIO pins instead of digital read will improve performance.

      if ( !AlreadyOn )
      {
        REG_WRITE(GPIO_OUT_W1TS_REG, BIT15);
        AlreadyOn = true;
      } else {
        REG_WRITE(GPIO_OUT_W1TC_REG, BIT15);
        AlreadyOn = false;
      }

See the ESP32 GPIO API. You can use the Arduino expressions to setup the digital pins and use the ESP32 to read/write the GPIO pins.

Example of using ESP32 GPIO API to setup GPIO

 gpio_config_t io_cfg = {};
  io_cfg.mode = GPIO_MODE_OUTPUT;
  //bit mask of the pins to set
  io_cfg.pin_bit_mask = ( (1ULL << GPIO_NUM_0) | (1ULL << GPIO_NUM_15) | (1ULL << GPIO_NUM_2) );
  //configure GPIO with the given settings
  gpio_config(&io_cfg);
  REG_WRITE(GPIO_OUT_W1TS_REG, BIT0);
  REG_WRITE(GPIO_OUT_W1TC_REG, BIT12);
  REG_WRITE(GPIO_OUT_W1TC_REG, BIT15);

Use freeRTOS, the built in OS of the ESP32.

For pulse in, look into using the ESP32 PCNT API, Pulse Counter (PCNT) - ESP32 - — ESP-IDF Programming Guide latest documentation. The PCNT runs independently of the mcu.

Use vTaskDelay, non-blocking. Meaning if you use freeRTOS any vTaskDelay will allow other tasks to run during the delay time. See freeRTOS documentation. If you use vTaskDelay with only one declared task, loop() is a declared task, the you get blocking.

When declaring a structure on the ESP32 like this esp_now_peer_info_t peerInfo; always declare the structure like this

esp_now_peer_info_t peerInfo = {};

When the structure is assigned memory, the esp will block off a section of memory for the structure. The ESP does NOT clear the ram. If the memory location were previously used, the the previous information is brought into the structure. You may not declare or use all the fields of the structure but when the structure is used, those uncleared memory fields will be used as setting for the thing to do.

If the structure is created with the {} then the ram is cleared of previous data.

long duration, cm, inches; why do you need a 64 bit signed integer?
And why

cm = (duration / 2) / 29.1;   // Divide by 29.1 or multiply by 0.0343
  inches = (duration / 2) / 74; // Divide by 74 or multiply by 0.0135

do you use floats in your long integer calculations?

This delay(5000); is really bad practice, use freeRTOS and get rid of code execution blocking.

And something wrong here "void loop() {}}"

Appreciate the feedback!!!

But like i said, noob here. Will look into the things said.

TheMemberFormerlyKnownAsAWOL:

digitalWrite(trigPin, redled, LOW);

Oops.

Yeah , changed that after uploaded the code so guess thats no good, changed it:

digitalWrite(trigPin, LOW);
digitalWrite(redpin, LOW);

johnwasser:

  duration = pulseIn(echoPin, HIGH);

The 'pulseIn()' function returns 0 if no echo was detected. This means the object is either VERY close (less than 2 or 3 cm) or too far (echo is not strong enough to detect).

Don't see any problem here, if it to close or to far i would make a fault for it so i know something is wrong.

Idahowalker:
By using Logging library - ESP32 - — ESP-IDF Programming Guide latest documentation to print things, instead of serial, will give a performance increase; #include "sdkconfig.h".

Use ESP time counter for millis()/micros() instead of Arduino

oid fDo_AudioReadFreq( void *pvParameters )

{
 int FreqVal[7];
 const int NOISE = 10; // noise that you want to chop off
 const int A_D_ConversionBits = 4096; // arduino use 1024, ESP32 use 4096
 Analyzer Audio = Analyzer( 5, 15, 36 );//Strobe pin ->15  RST pin ->2 Analog Pin ->36
 Audio.Init(); // start the audio analyzer
 int64_t EndTime = esp_timer_get_time();
 int64_t StartTime = esp_timer_get_time(); //gets time in uSeconds like Arduino Micros
 for (;:wink:
 {
   xEventGroupWaitBits (eg, evtDo_AudioReadFreq, pdTRUE, pdTRUE, portMAX_DELAY);
   EndTime = esp_timer_get_time() - StartTime;
   log_i( "TimeSpentOnTasks: %d", EndTime );
   Audio.ReadFreq(FreqVal);
   for (int i = 0; i < 7; i++)
   {
     FreqVal[i] = constrain( FreqVal[i], NOISE, A_D_ConversionBits );
     FreqVal[i] = map( FreqVal[i], NOISE, A_D_ConversionBits, 0, 255 );
     // log_i( "Freq %d Value: %d", i, FreqVal[i]);//used for debugging and Freq choosing
   }
   xQueueSend( xQ_LED_Info, ( void * ) &FreqVal, 0 );
   StartTime = esp_timer_get_time();
 }
 vTaskDelete( NULL );
} // fDo_ AudioReadFreq( void *pvParameters )



esp_timer_get_time() is a micro second clock that rolls over in 200ish years. and saves a bit of time with code execution.


Using the ESP GPIO API to read GPIO pins instead of digital read will improve performance.



if ( !AlreadyOn )
     {
       REG_WRITE(GPIO_OUT_W1TS_REG, BIT15);
       AlreadyOn = true;
     } else {
       REG_WRITE(GPIO_OUT_W1TC_REG, BIT15);
       AlreadyOn = false;
     }



See the ESP32 GPIO API. You can use the Arduino expressions to setup the digital pins and use the ESP32 to read/write the GPIO pins.

Example of using ESP32 GPIO API to setup GPIO


gpio_config_t io_cfg = {};
 io_cfg.mode = GPIO_MODE_OUTPUT;
 //bit mask of the pins to set
 io_cfg.pin_bit_mask = ( (1ULL << GPIO_NUM_0) | (1ULL << GPIO_NUM_15) | (1ULL << GPIO_NUM_2) );
 //configure GPIO with the given settings
 gpio_config(&io_cfg);
 REG_WRITE(GPIO_OUT_W1TS_REG, BIT0);
 REG_WRITE(GPIO_OUT_W1TC_REG, BIT12);
 REG_WRITE(GPIO_OUT_W1TC_REG, BIT15);





**Use freeRTOS**, the built in OS of the ESP32.

For pulse in, look into using the ESP32 PCNT API, https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/peripherals/pcnt.html. The PCNT runs independently of the mcu.

Use vTaskDelay, non-blocking. Meaning if you use freeRTOS any vTaskDelay will allow other tasks to run during the delay time. See freeRTOS documentation. If you use vTaskDelay with only one declared task, loop() is a declared task, the you get blocking.

Euhm okay will give that a shot,...(remember noob here :slight_smile:
stil learning also on phone not so easy..)

Idahowalker:
When declaring a structure on the ESP32 like this esp_now_peer_info_t peerInfo; always declare the structure like this

esp_now_peer_info_t peerInfo = {};

When the structure is assigned memory, the esp will block off a section of memory for the structure. The ESP does NOT clear the ram. If the memory location were previously used, the the previous information is brought into the structure. You may not declare or use all the fields of the structure but when the structure is used, those uncleared memory fields will be used as setting for the thing to do.

If the structure is created with the {} then the ram is cleared of previous data.

long duration, cm, inches; why do you need a 64 bit signed integer?
And why

cm = (duration / 2) / 29.1;   // Divide by 29.1 or multiply by 0.0343

inches = (duration / 2) / 74; // Divide by 74 or multiply by 0.0135



do you use floats in your long integer calculations?

This delay(5000); is really bad practice, use freeRTOS and get rid of code execution blocking.

And something wrong here "void loop() {}}"

Most of my code consist of copy paste from somewhere else, these 2 sepperatly(esp-now en measurement with hr-sr04) exist so i tried to combine/ rewrite them, with succes only i think the code looks like s...

Anyway like the beginning, thx and i really appreciate the feedback!!!

(Reply with phone sry for any typo's/ wrongs?)