Merge pull request #9350 from dhalbert/fix-espressif-ping

espressif: fix returning ping time and pinging too often

Author: dhalbert

ports/espressif/common-hal/wifi/Radio.c

@@ -533,32 +533,56 @@ void common_hal_wifi_radio_set_ipv4_address_ap(wifi_radio_obj_t *self, mp_obj_t
     common_hal_wifi_radio_start_dhcp_server(self); // restart access point DHCP
 }
 
+static void ping_success_cb(esp_ping_handle_t hdl, void *args) {
+    wifi_radio_obj_t *self = (wifi_radio_obj_t *)args;
+    esp_ping_get_profile(hdl, ESP_PING_PROF_TIMEGAP, &self->ping_elapsed_time, sizeof(self->ping_elapsed_time));
+}
+
 mp_int_t common_hal_wifi_radio_ping(wifi_radio_obj_t *self, mp_obj_t ip_address, mp_float_t timeout) {
     esp_ping_config_t ping_config = ESP_PING_DEFAULT_CONFIG();
     ipaddress_ipaddress_to_esp_idf(ip_address, &ping_config.target_addr);
     ping_config.count = 1;
 
+    // We must fetch ping information using the callback mechanism, because the session storage is freed when
+    // the ping session is done, even before esp_ping_delete_session().
+    esp_ping_callbacks_t ping_callbacks = {
+        .on_ping_success = ping_success_cb,
+        .cb_args = (void *)self,
+    };
+
     size_t timeout_ms = timeout * 1000;
 
+    // ESP-IDF creates a task to do the ping session. It shuts down when done, but only after a one second delay.
+    // Calling common_hal_wifi_radio_ping() too fast will cause resource exhaustion.
     esp_ping_handle_t ping;
-    CHECK_ESP_RESULT(esp_ping_new_session(&ping_config, NULL, &ping));
+    if (esp_ping_new_session(&ping_config, &ping_callbacks, &ping) != ESP_OK) {
+        // Wait for old task to go away and then try again.
+        // Empirical testing shows we have to wait at least two seconds, despite the task
+        // having a one-second timeout.
+        common_hal_time_delay_ms(2000);
+        // Return if interrupted now, to show the interruption as KeyboardInterrupt instead of the
+        // IDF error.
+        if (mp_hal_is_interrupted()) {
+            return (uint32_t)(-1);
+        }
+        CHECK_ESP_RESULT(esp_ping_new_session(&ping_config, &ping_callbacks, &ping));
+    }
+
     esp_ping_start(ping);
 
-    uint32_t received = 0;
-    uint32_t total_time_ms = 0;
+    // Use all ones as a flag that the elapsed time was not set (ping failed or timed out).
+    self->ping_elapsed_time = (uint32_t)(-1);
+
     uint32_t start_time = common_hal_time_monotonic_ms();
-    while (received == 0 && (common_hal_time_monotonic_ms() - start_time < timeout_ms) && !mp_hal_is_interrupted()) {
+    while ((self->ping_elapsed_time == (uint32_t)(-1)) &&
+           (common_hal_time_monotonic_ms() - start_time < timeout_ms) &&
+           !mp_hal_is_interrupted()) {
         RUN_BACKGROUND_TASKS;
-        esp_ping_get_profile(ping, ESP_PING_PROF_DURATION, &total_time_ms, sizeof(total_time_ms));
-        esp_ping_get_profile(ping, ESP_PING_PROF_REPLY, &received, sizeof(received));
-    }
-    uint32_t elapsed_time = 0xffffffff;
-    if (received > 0) {
-        esp_ping_get_profile(ping, ESP_PING_PROF_TIMEGAP, &elapsed_time, sizeof(elapsed_time));
     }
+    esp_ping_stop(ping);
     esp_ping_delete_session(ping);
 
-    return elapsed_time;
+    return (mp_int_t)self->ping_elapsed_time;
 }
 
 void common_hal_wifi_radio_gc_collect(wifi_radio_obj_t *self) {

ports/espressif/common-hal/wifi/Radio.h

@@ -32,15 +32,16 @@ typedef struct {
     esp_netif_ip_info_t ip_info;
     esp_netif_dns_info_t dns_info;
     esp_netif_t *netif;
+    uint32_t ping_elapsed_time;
+    wifi_config_t ap_config;
+    esp_netif_ip_info_t ap_ip_info;
+    esp_netif_t *ap_netif;
     bool started;
     bool ap_mode;
     bool sta_mode;
     uint8_t retries_left;
     uint8_t starting_retries;
     uint8_t last_disconnect_reason;
-    wifi_config_t ap_config;
-    esp_netif_ip_info_t ap_ip_info;
-    esp_netif_t *ap_netif;
 } wifi_radio_obj_t;
 
 extern void common_hal_wifi_radio_gc_collect(wifi_radio_obj_t *self);

shared-bindings/wifi/Radio.c

@@ -698,7 +698,12 @@ MP_PROPERTY_GETTER(wifi_radio_ap_info_obj,
 //|         self, ip: ipaddress.IPv4Address, *, timeout: Optional[float] = 0.5
 //|     ) -> Optional[float]:
 //|         """Ping an IP to test connectivity. Returns echo time in seconds.
-//|         Returns None when it times out."""
+//|         Returns None when it times out.
+//|
+//|         **Limitations:** On Espressif, calling `ping()` multiple times rapidly
+//|         exhausts available resources after several calls. Rather than failing at that point, `ping()`
+//|         will wait two seconds for enough resources to be freed up before proceeding.
+//|         """
 //|         ...
 //|
 static mp_obj_t wifi_radio_ping(size_t n_args, const mp_obj_t *pos_args, mp_map_t *kw_args) {