Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

esp32 mdns server (re)start on connectivity change #7749

Closed
wants to merge 2 commits into from
Closed

esp32 mdns server (re)start on connectivity change #7749

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0faa894
esp32 mdns server start at connectivity change
kklem0 Jun 19, 2021
72032a9
all-clusters-app/esp32 remove custom mdns logic
kklem0 Jun 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 0 additions & 15 deletions examples/all-clusters-app/esp32/main/DeviceCallbacks.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,8 @@
#include <app/Command.h>
#include <app/common/gen/attribute-id.h>
#include <app/common/gen/cluster-id.h>
#include <app/server/Mdns.h>
#include <app/util/basic-types.h>
#include <app/util/util.h>
#include <lib/mdns/Advertiser.h>
#include <support/CodeUtils.h>

static const char * TAG = "app-devicecallbacks";
Expand All @@ -60,17 +58,6 @@ void DeviceCallbacks::DeviceEventCallback(const ChipDeviceEvent * event, intptr_
case DeviceEventType::kSessionEstablished:
OnSessionEstablished(event);
break;
case DeviceEventType::kInterfaceIpAddressChanged:
if ((event->InterfaceIpAddressChanged.Type == InterfaceIpChangeType::kIpV4_Assigned) ||
(event->InterfaceIpAddressChanged.Type == InterfaceIpChangeType::kIpV6_Assigned))
{
// MDNS server restart on any ip assignment: if link local ipv6 is configured, that
// will not trigger a 'internet connectivity change' as there is no internet
// connectivity. MDNS still wants to refresh its listening interfaces to include the
// newly selected address.
chip::app::Mdns::StartServer();
}
break;
}

ESP_LOGI(TAG, "Current free heap: %zu\n", heap_caps_get_free_size(MALLOC_CAP_8BIT));
Expand Down Expand Up @@ -110,7 +97,6 @@ void DeviceCallbacks::OnInternetConnectivityChange(const ChipDeviceEvent * event
{
ESP_LOGI(TAG, "Server ready at: %s:%d", event->InternetConnectivityChange.address, CHIP_PORT);
wifiLED.Set(true);
chip::app::Mdns::StartServer();
}
else if (event->InternetConnectivityChange.IPv4 == kConnectivity_Lost)
{
Expand All @@ -120,7 +106,6 @@ void DeviceCallbacks::OnInternetConnectivityChange(const ChipDeviceEvent * event
if (event->InternetConnectivityChange.IPv6 == kConnectivity_Established)
{
ESP_LOGI(TAG, "IPv6 Server ready...");
chip::app::Mdns::StartServer();
}
else if (event->InternetConnectivityChange.IPv6 == kConnectivity_Lost)
{
Expand Down
27 changes: 27 additions & 0 deletions src/app/server/Mdns.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,33 @@ void StartServer()
}
}

void NetworkChangeEventHandler(const chip::DeviceLayer::ChipDeviceEvent * event, intptr_t)
{
switch (event->Type)
{
case chip::DeviceLayer::DeviceEventType::kInternetConnectivityChange:
if ((event->InternetConnectivityChange.IPv4 == chip::DeviceLayer::kConnectivity_Established) ||
(event->InternetConnectivityChange.IPv6 == chip::DeviceLayer::kConnectivity_Established))
{
chip::app::Mdns::StartServer();
}
break;
#if CHIP_DEVICE_CONFIG_ENABLE_THREAD
case chip::DeviceLayer::DeviceEventType::kThreadStateChange:
VerifyOrReturn(event->ThreadStateChange.AddressChanged);
chip::app::Mdns::StartServer();
break;
#endif
default:
break;
}
}

void StartServerOnNetworkChange()
{
chip::DeviceLayer::PlatformMgr().AddEventHandler(NetworkChangeEventHandler, {});
}

#if CHIP_ENABLE_ROTATING_DEVICE_ID
CHIP_ERROR GenerateRotatingDeviceId(char rotatingDeviceIdHexBuffer[], size_t rotatingDeviceIdHexBufferSize)
{
Expand Down
3 changes: 3 additions & 0 deletions src/app/server/Mdns.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ CHIP_ERROR Advertise(bool commissionableNode);
/// (Re-)starts the minmdns server
void StartServer();

/// (Re-)starts the minmdns server on network change
void StartServerOnNetworkChange();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@DamKast DamKast Jun 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


CHIP_ERROR GenerateRotatingDeviceId(char rotatingDeviceIdHexBuffer[], size_t rotatingDeviceIdHexBufferSize);

} // namespace Mdns
Expand Down
2 changes: 2 additions & 0 deletions src/app/server/Server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,6 +530,8 @@ void InitServer(AppDelegate * delegate)
// ESP32 examples have a custom logic for enabling DNS-SD
#if CHIP_DEVICE_CONFIG_ENABLE_MDNS && !CHIP_DEVICE_LAYER_TARGET_ESP32
app::Mdns::StartServer();
#elif CHIP_DEVICE_CONFIG_ENABLE_MDNS && CHIP_DEVICE_LAYER_TARGET_ESP32
app::Mdns::StartServerOnNetworkChange();
#endif

gCallbacks.SetSessionMgr(&gSessions);
Expand Down