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.

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 lock-app mdns server start at connectivity change #7723

Merged
merged 1 commit into from
Jun 21, 2021

Conversation

DamKast
Copy link
Contributor

@DamKast DamKast commented Jun 17, 2021

Problem

What is being fixed? Examples:

Change overview

esp32 lock-app mdns server start at connectivity change

Testing

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

  • using lock-app on esp32

@CLAassistant
Copy link

CLAassistant commented Jun 17, 2021

CLA assistant check
All committers have signed the CLA.

@dhrishi
Copy link
Contributor

dhrishi commented Jun 17, 2021

Hi @DamKast Thanks for the fix! The same issue is also seen with examples/temperature-measurement-app/esp32 as well. Would you mind making the exact same changes there?

@dhrishi dhrishi requested a review from gjc13 June 17, 2021 15:44
@@ -51,6 +52,18 @@ void DeviceCallbacks::DeviceEventCallback(const ChipDeviceEvent * event, intptr_
case DeviceEventType::kSessionEstablished:
OnSessionEstablished(event);
break;

case DeviceEventType::kInterfaceIpAddressChanged:
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be simpler or clearer to place all chip::app::Mdns::StartServer(); calls in DeviceEventCallback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how it is done for the all-clusters-app example. So I am not sure that moving it in DeviceEventCallback would be clearer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, OK. I mean it's just moving the calls one function deeper.

No problem.

@kklem0
Copy link

kklem0 commented Jun 18, 2021

Me and @DamKast are looking into if it makes more sense to move it into src/app/server/Server.cpp or even src/app/server/Mdns.cpp to be more generic.
Probably an #elsif in

// ESP32 examples have a custom logic for enabling DNS-SD
#if CHIP_DEVICE_CONFIG_ENABLE_MDNS && !CHIP_DEVICE_LAYER_TARGET_ESP32
    app::Mdns::StartServer();
#endif

Or in app::Mdns::StartServer().

Thoughts?

@kklem0
Copy link

kklem0 commented Jun 18, 2021

To elaborate my last comment, what do you guys think of this approach instead?
I can get @DamKast to polish it for this PR if it's good.

diff --git a/src/app/server/Mdns.cpp b/src/app/server/Mdns.cpp
index 3bab44a9..157c4b5d 100644
--- a/src/app/server/Mdns.cpp
+++ b/src/app/server/Mdns.cpp
@@ -272,6 +272,21 @@ void StartServer()
     }
 }
 
+void NetworkChangeEventHandler(const chip::DeviceLayer::ChipDeviceEvent * event, intptr_t)
+{
+    VerifyOrReturn(event->Type == chip::DeviceLayer::DeviceEventType::kInternetConnectivityChange);
+
+    if (event->InternetConnectivityChange.IPv4 == chip::DeviceLayer::kConnectivity_Established || event->InternetConnectivityChange.IPv6 == chip::DeviceLayer::kConnectivity_Established)
+    {
+        chip::app::Mdns::StartServer();
+    }
+}
+
+void StartServerOnNetworkChange()
+{
+    chip::DeviceLayer::PlatformMgr().AddEventHandler(NetworkChangeEventHandler, {});
+}
+
 #if CHIP_ENABLE_ROTATING_DEVICE_ID
 CHIP_ERROR GenerateRotatingDeviceId(char rotatingDeviceIdHexBuffer[], size_t rotatingDeviceIdHexBufferSize)
 {
diff --git a/src/app/server/Mdns.h b/src/app/server/Mdns.h
index 7c13968f..e6f1ad15 100644
--- a/src/app/server/Mdns.h
+++ b/src/app/server/Mdns.h
@@ -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();
+
 CHIP_ERROR GenerateRotatingDeviceId(char rotatingDeviceIdHexBuffer[], size_t rotatingDeviceIdHexBufferSize);
 
 } // namespace Mdns
diff --git a/src/app/server/Server.cpp b/src/app/server/Server.cpp
index 4198882d..b6e6bdf8 100644
--- a/src/app/server/Server.cpp
+++ b/src/app/server/Server.cpp
@@ -527,9 +527,10 @@ void InitServer(AppDelegate * delegate)
 #endif
     }
 
-// 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);

@woody-apple woody-apple merged commit 7ddf8be into project-chip:master Jun 21, 2021
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

chip-device-ctrl - resolve command hangs with lock-app
9 participants