esp32 mdns server (re)start on connectivity change

[kklem0]

Problem

What is being fixed? Examples:

• Fixes chip-device-ctrl - resolve command hangs with lock-app  #7705 chip-device-ctrl - resolve command hangs with lock-app.

Change overview

esp32 mdns server (re)start on connectivity change
Alternative solution of #7723
Worked with @DamKast for this change.

Testing

How was this tested? (at least one bullet point required)

• using lock-app on esp32

[CLAassistant]

All committers have signed the CLA.

[andy31415]

What is the difference between this and the 'StartServer' above?

The description seems the same except one says 'on network change'. When I look into the usage in Server.CPP it looks like one is 'not esp32' while the other is 'esp32'.

Could we combine them into one? If both esp32 and non-exp32 require some sort of implementation, it seems more natural to expose a single API rather than 2 APIs and ifdefing between them

[kklem0]

I was working on #7723 with @DamKast but not sure why the !CHIP_DEVICE_LAYER_TARGET_ESP32 was there at the first place, https://github.com/project-chip/connectedhomeip/blob/0bca292/src/app/server/Server.cpp#L531, but taking it out still doesn't make it work with ESP32, so we just take the parts needed out from https://github.com/project-chip/connectedhomeip/blob/3d2d9cb/examples/all-clusters-app/esp32/main/DeviceCallbacks.cpp and put it into server.cpp and mdns.cpp so it works for all ESP32 examples, we figured it StartServer() already works for all other platforms, no reason to keep restarting it on other platforms on network change, so there's that.

If you have some input of why there's !CHIP_DEVICE_LAYER_TARGET_ESP32 in server.cpp for mdns, we can look into it.

(It's probably more like)

#if CHIP_DEVICE_CONFIG_ENABLE_MDNS
#if !CHIP_DEVICE_LAYER_TARGET_ESP32
    app::Mdns::StartServer();
#else
    app::Mdns::StartServerOnNetworkChange();
#endif
#endif

[andy31415]

I am unsure about reasons - my guess is some interaction between esp32 using platform mdns and the rest using minmdns and platformmdns does not like restarting.

From an API perspective, I believe we should isolate the user of the API from platform differences. Even the 'do not call this method on some platform' seems dodgy. At most we can provide some parameter like StartServer(cause) and let platform code decide if that cause is sufficient for restart.

As it stants, we seem to fork off the same api (with a OnNetworkChangeSuffix) and call it in a platform dependent manner.

[DamKast]

What is your thought on this:

Instead of registering a handler catching network changes in Server.cpp (which is not platform specific), we could just restart the mDNS server when the connectivity change event is fired from the platform implementation.

diff --git a/src/platform/ESP32/ConnectivityManagerImpl.cpp b/src/platform/ESP32/ConnectivityManagerImpl.cpp
index 2f3e97c4..ea9c3018 100644
--- a/src/platform/ESP32/ConnectivityManagerImpl.cpp
+++ b/src/platform/ESP32/ConnectivityManagerImpl.cpp
@@ -41,6 +41,10 @@

 #include <type_traits>

+#if CHIP_DEVICE_CONFIG_ENABLE_MDNS
+#include <app/server/Mdns.h>
+#endif
+
 #if !CHIP_DEVICE_CONFIG_ENABLE_WIFI_STATION
 #error "WiFi Station support must be enabled when building for ESP32"
 #endif
@@ -687,6 +691,7 @@ void ConnectivityManagerImpl::OnStationConnected()
     event.Type                          = DeviceEventType::kWiFiConnectivityChange;
     event.WiFiConnectivityChange.Result = kConnectivity_Established;
     PlatformMgr().PostEvent(&event);
+    app::Mdns::StartServer();

     UpdateInternetConnectivityState();
 }

I tested it, and it seems to work at least for the lock-app. And that way, we don't need to introduce a StartServerOnNetworkChange(); API.

[kklem0]

Don't think you need #if CHIP_DEVICE_CONFIG_ENABLE_MDNS for the include, but need it for app::Mdns::StartServer();.

It's also not very clean but getting better. Can also call it in platform MdnsImpl.cpp.
It'll even be better if we can find out why platform mdns doesn't work without restarting on network change.

[kklem0]

@DamKast will follow this up with another PR.