marian-craciunescu / ESP32Ping

Ping library for ESP32 Arduino core
GNU Lesser General Public License v2.1
110 stars 58 forks source link

ESP32 Ping.ping(remote_host_IP) not working fine #20

Closed akansha104 closed 4 years ago

akansha104 commented 4 years ago

Hi, I want ESP32 server to ping my local host(PC) on same network. But ESP32 is always successfully pinging irrespective of remote host in network or not. I am using <ESP32Ping .h library. I am attaching code:

#include <WiFi.h>
#include <ESP32Ping.h>

const char* ssid     = "*****";
const char* password = "******";

const IPAddress remote_ip(192,168,43,197);

void setup() {
  Serial.begin(115200);
  delay(10);

  // We start by connecting to a WiFi network

  Serial.println();
  Serial.println("Connecting to WiFi");

  WiFi.begin(ssid, password);

  while (WiFi.status() != WL_CONNECTED) {
    delay(100);
    Serial.print(".");
  }

  Serial.println();
  Serial.print("WiFi connected with ip ");  
  Serial.println(WiFi.localIP());

  Serial.print("Pinging ip ");
  Serial.println(remote_ip);

}

void loop() { 
  int ret = Ping.ping(remote_ip,10 );
  Serial.println(ret);
  i  Serial.println(avg_time_ms);
 nt avg_time_ms = Ping.averageTime();
    Serial.println("Success!!");
  } else {
    if(ret) {
   Serial.println("Error :(");
  }}

Please suggest me how to resolve this Regards

varandaas commented 4 years ago

I can confirm this behavior. Have exactly the same problem

andrea1984 commented 4 years ago

Same here, this library doesn't work. The debug statistics are wrong too:

DEBUG: ping reply total_count = 10 resp_time = 0 seqno = 0 timeout_count = 931264192 bytes = 1074308350 total_bytes = 10 total_time = -2146594692 ping_err = 1

marian-craciunescu commented 4 years ago

i think this is a regression introduced after merging @adynis pull request. I don't have at the moment a ESP32 board to properly test the modificaiton.I took his proof. I think I can do a revert (or you could try to use another library version , prior to 1.3) In the mean time.

andrea1984 commented 4 years ago

I solved the problem modifing your library and ping.cpp library, removing the bugs. Now the code is much simpler and, most of all, working well.

adynis commented 4 years ago

@marian-craciunescu : wait wait, do not revert yet :) Please allow me to test also, I didn't see these comments until now. In this evening/night I will test and comment here.

PS. There were also some existing other pull requests from me, I will check if the issue was already solved by my other pull requests, since in my overall copy of the code it was working fine after many tests on ESP32-wroom-32D

andrea1984 commented 4 years ago

If you are planning to fix the bugs, I can give you some hint: 1) ping.cpp - 336: local variable pingresp cannot be passed to an external function. Big bug here. 2) Function pointers are totally useless, because the ping_start function is synchronous, so the best thing to do is add the ping results in the ping_option pointer, removing the fixed and useless data and adding the relevant data as transmitted and received packet, and average time.

Those two are the most relevant fixes that I made to the library. I don't need call ping function async, but if you need I think the easier way is to use a timer.

marian-craciunescu commented 4 years ago

@andrea1984 since you already did the work,can you raise a PR ?

marian-craciunescu commented 4 years ago
  1. ping.cpp - 336: local variable pingresp cannot be passed to an external function. Big bug here. that is a local variable which is passed as a pointer to the function.The Local variable will exist in the that scope and still can be used by the called functionrecv_function()

  2. Function pointers are totally useless, because the ping_start function is synchronous, so the best thing to do is add the ping results in the ping_option pointer, removing the fixed and useless data and adding the relevant data as transmitted and received packet, and average time.

marian-craciunescu commented 4 years ago

I can also merge @adynis work (Since it was left in somewhat of a limbo). @andrea1984 it would be great if you can also test it .

adynis commented 4 years ago

Oky, I did some tests, short conclusion : if you merge PullRequest #15 (which includes issue #8 and issue #9) then also the problem mentioned by @akansha104 is solved !

For more details on the results shown after merging pr #15 , pleasee check https://github.com/marian-craciunescu/ESP32Ping/issues/9#issuecomment-614313475 where I've added some examples.

@andrea1984 : 1.) It works, like @marian-craciunescu mentioned, it's no technical problem. Right now, in master, the issue is different (see issue #8 and eventually issue #9). 2.) I'm not sure I understand, but I think you can check my pullRequest #15 which works fine with transmitting all the information through pingresp.

andrea1984 commented 4 years ago

Sorry, my fault for the first issue. Yes is not a bug my first sentence on ping.cpp. I was thinking that in some way the function worked async, but obviously not. Those callback functions are not really callback because there is not any async mechanism. For this reason are completely useless, and what I've done is completely remove them, because all the result ping data can be returned when the ping function (ping_start) is executed.

marian-craciunescu commented 4 years ago

@adynis @akansha104 @andrea1984 @varandaas can you give a test to version 1.5 of the library. I've ordered a new ESP32 board I willl receive it in 2-3 weeks

adynis commented 4 years ago

Thanks for the merge; it corresponds now to the code I was initially using. I re-did some quick checks now for a valid host and an invalid host and it looks like expected. The only minor observation I did: the README.md (change int to float). I did a pull request with only this change. Of course that additional feedback from others would be very useful !

akansha104 commented 4 years ago

@adynis @akansha104 @andrea1984 @varandaas can you give a test to version 1.5 of the library. I've ordered a new ESP32 board I willl receive it in 2-3 weeks

Yeah sure i will check with with the updated library version. Thanks Akansha

marian-craciunescu commented 4 years ago

Hey Guys any updates on the testing ? @akansha104 @andrea1984 @varandaas ?

varandaas commented 4 years ago

Yeah, did some testing yesterday and deployed the code on 3 devices. So far so good, seems solved 👍

Thank you so much, guys!

marian-craciunescu commented 4 years ago

one more tester and I will close this issue

andrea1984 commented 4 years ago

Yes it works