mdns: ethernet link flap doesn't restart mdns if DHCP in use. (IDFGH-12563)

karlp commented 2 months ago

Answers checklist.

[X] I have read the documentation for esp-protocols components and the issue is not addressed there.
[X] I have updated my esp-protocols branch (master or release) to the latest version and checked that the issue is present there.
[X] I have searched the issue tracker for a similar issue and not found a similar issue.

General issue report

When using DHCP on ethernet, If I pull the ethernet cable on an esp32 board, with MDNS advertising enabled[1], I will (correctly) get the event here https://github.com/espressif/esp-protocols/blob/mdns-v1.2.5/components/mdns/mdns.c#L4223-L4225, and the mdns service record will be withdrawn.

However, when I plug the cable back in again, I will (correctly) get an ethernet link up event (ETHERNET_EVENT_CONNECTED) in the mdns code, here: https://github.com/espressif/esp-protocols/blob/mdns-v1.2.5/components/mdns/mdns.c#L4216-L4222 but, because DHCP is not "stopped" the mdns service is never re-enabled.

For my own use cases, simply removing the check on DHCP being stopped "fixes" this issue, and re-stores mdns services when the link is restored. Receiving new DHCP leases still works correctly, as those come in via the IP_EVENT_ETH_GOT_IP event here https://github.com/espressif/esp-protocols/blob/mdns-v1.2.5/components/mdns/mdns.c#L4239-L4241 But that event is not triggered when the link simply flaps.

I'm not aware of any workaround beside restarting my application, or not using DHCP.

While the ethernet interface and the wifi interface both have the same code style here, I believe this is never seen on wifi, as the wifi WIFI_EVENT_STA_CONNECTED event is never seen without getting the IP_EVENT_STA_GOT_IP immediately afterwards. This is not the case on ethernet. The DHCP lease hasn't expired, the link just flapped.

Note, when using a static IP, this link flapping works fine, because DHCP is "stopped"

Solutions?

I'm not sure. This appears to stem from the change in https://github.com/espressif/esp-protocols/commit/f44c5694228eebebe25720aeadbc6157c8635314 However, I'm unconvinced that the solution is correct. IMO both dhcp and static IPs should work, and it's always broken to have this check on enable, but no check on disable?

The original behaviour of only enabling if dhcp was started seems worse, in that it might make dhcp work, but it would prevent static IPs from working at all? I can't track the reasoning for this check any further than ad8c92db52a in 2017 however. The corresponding commit in esp-idf is https://github.com/espressif/esp-idf/commit/4bddbc031cee83935c0e4df1dc72cc7000c97ba5 But there's no guidance there on why this check exists.

Can we just drop these checks? It "works for me" at least :) fixing both DHCP and Static IP configurations :)

[1] As per the examples... ie, something like this... with no specific interface handling, the default predefined ethernet interface is sufficient.

void app_start_mdns(void) {
    char *hostname = "hohostname";
    ESP_ERROR_CHECK(mdns_init());
    ESP_ERROR_CHECK(mdns_hostname_set(hostname));

    // structure with TXT records
    mdns_txt_item_t serviceTxtData[] = {
        {"vendor", "whatever"},
        {"model", "fun-model"},
    };

    ESP_ERROR_CHECK(mdns_service_add(NULL, "_http", "_tcp", 80, NULL, 0));
    ESP_ERROR_CHECK(mdns_service_add(NULL, "_whatever", "_tcp", 2222, serviceTxtData, ARRAY_SIZE(serviceTxtData)));
}

karlp commented 2 months ago

To clarify, the follow patch "fixes" it, but a) this makes ethernet and wifi inconsistent and b) fairly obviously, ipv6 probably still has an issue here... (unrelated, but... obvious now)

diff --git a/components/mdns/mdns.c b/components/mdns/mdns.c
index 5dcc40b..948e259 100644
--- a/components/mdns/mdns.c
+++ b/components/mdns/mdns.c
@@ -4229,11 +4229,7 @@ void mdns_preset_if_handle_system_event(void *arg, esp_event_base_t event_base,
         if (event_base == ETH_EVENT) {
             switch (event_id) {
             case ETHERNET_EVENT_CONNECTED:
-                if (!esp_netif_dhcpc_get_status(esp_netif_from_preset_if(MDNS_IF_ETH), &dcst)) {
-                    if (dcst == ESP_NETIF_DHCP_STOPPED) {
-                        post_mdns_enable_pcb(MDNS_IF_ETH, MDNS_IP_PROTOCOL_V4);
-                    }
-                }
+                post_mdns_enable_pcb(MDNS_IF_ETH, MDNS_IP_PROTOCOL_V4);
                 break;
             case ETHERNET_EVENT_DISCONNECTED:
                 post_mdns_disable_pcb(MDNS_IF_ETH, MDNS_IP_PROTOCOL_V4);

david-cermak commented 2 months ago

Hi @karlp

You can work this around by registering your own netif and configuring advertising actions per your needs (e.g. on link up event)

Here's how you register a netif

https://github.com/espressif/esp-protocols/blob/0b94d9ec47dff446259dc9ab1d7198e710c691d5/components/mdns/examples/query_advertise/main/mdns_example_main.c#L326-L329

and set up announcing events (typically in your event handlers):

https://github.com/espressif/esp-protocols/blob/0b94d9ec47dff446259dc9ab1d7198e710c691d5/components/mdns/examples/query_advertise/main/mdns_example_main.c#L330-L335

and sdkconfig:

https://github.com/espressif/esp-protocols/blob/0b94d9ec47dff446259dc9ab1d7198e710c691d5/components/mdns/examples/query_advertise/sdkconfig.ci.eth_custom_netif#L5-L8

espressif / esp-protocols