[Android] Fix discover Commissionable Device API issue when discovere…

…d multiple devices (project-chip#32459) * Fix Android Commissionable Device API when searched multiple devices * Fix Android Commissionable Device API when searched multiple devices * Fix from comment, fix edge case issue * Restyled by google-java-format --------- Co-authored-by: Restyled.io <[email protected]>
kurvaSai · Mar 12, 2024 · 9a947d8 · 9a947d8
1 parent a31de1d
commit 9a947d8
Show file tree

Hide file tree

Showing 5 changed files with 51 additions and 35 deletions.
diff --git a/examples/android/CHIPTool/app/src/main/java/com/google/chip/chiptool/ChipClient.kt b/examples/android/CHIPTool/app/src/main/java/com/google/chip/chiptool/ChipClient.kt
@@ -74,7 +74,10 @@ object ChipClient {
           AndroidBleManager(context),
           PreferencesKeyValueStoreManager(context),
           PreferencesConfigurationManager(context),
-          NsdManagerServiceResolver(context),
+          NsdManagerServiceResolver(
+            context,
+            NsdManagerServiceResolver.NsdManagerResolverAvailState()
+          ),
           NsdManagerServiceBrowser(context),
           ChipMdnsCallbackImpl(),
           DiagnosticDataProviderImpl(context)

diff --git a/src/controller/AbstractDnssdDiscoveryController.cpp b/src/controller/AbstractDnssdDiscoveryController.cpp
@@ -35,7 +35,8 @@ void AbstractDnssdDiscoveryController::OnNodeDiscovered(const chip::Dnssd::Disco
             continue;
         }
         if (strcmp(discoveredNode.resolutionData.hostName, nodeData.resolutionData.hostName) == 0 &&
-            discoveredNode.resolutionData.port == nodeData.resolutionData.port)
+            discoveredNode.resolutionData.port == nodeData.resolutionData.port &&
+            discoveredNode.resolutionData.ipAddress == nodeData.resolutionData.ipAddress)
         {
             discoveredNode = nodeData;
             if (mDeviceDiscoveryDelegate != nullptr)

diff --git a/src/platform/android/java/chip/platform/NsdManagerServiceBrowser.java b/src/platform/android/java/chip/platform/NsdManagerServiceBrowser.java
@@ -34,10 +34,19 @@ public class NsdManagerServiceBrowser implements ServiceBrowser {
   private final NsdManager nsdManager;
   private MulticastLock multicastLock;
   private Handler mainThreadHandler;
+  private final long timeout;
 
   private HashMap<Long, NsdManagerDiscovery> callbackMap;
 
   public NsdManagerServiceBrowser(Context context) {
+    this(context, BROWSE_SERVICE_TIMEOUT);
+  }
+
+  /**
+   * @param context application context
+   * @param timeout Timeout value in case there is no response after calling browse
+   */
+  public NsdManagerServiceBrowser(Context context, long timeout) {
     this.nsdManager = (NsdManager) context.getSystemService(Context.NSD_SERVICE);
     this.mainThreadHandler = new Handler(Looper.getMainLooper());
 
@@ -46,6 +55,7 @@ public NsdManagerServiceBrowser(Context context) {
             .createMulticastLock("chipBrowseMulticastLock");
     this.multicastLock.setReferenceCounted(true);
     callbackMap = new HashMap<>();
+    this.timeout = timeout;
   }
 
   @Override
@@ -62,7 +72,7 @@ public void run() {
           }
         };
     startDiscover(serviceType, callbackHandle, contextHandle, chipMdnsCallback);
-    mainThreadHandler.postDelayed(timeoutRunnable, BROWSE_SERVICE_TIMEOUT);
+    mainThreadHandler.postDelayed(timeoutRunnable, timeout);
   }
 
   public void startDiscover(

diff --git a/src/platform/android/java/chip/platform/NsdManagerServiceResolver.java b/src/platform/android/java/chip/platform/NsdManagerServiceResolver.java
@@ -22,13 +22,14 @@
 import android.net.nsd.NsdServiceInfo;
 import android.net.wifi.WifiManager;
 import android.net.wifi.WifiManager.MulticastLock;
-import android.os.Handler;
-import android.os.Looper;
 import android.util.Log;
 import androidx.annotation.Nullable;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
 import java.util.concurrent.locks.Condition;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
@@ -38,30 +39,38 @@ public class NsdManagerServiceResolver implements ServiceResolver {
   private static final long RESOLVE_SERVICE_TIMEOUT = 30000;
   private final NsdManager nsdManager;
   private MulticastLock multicastLock;
-  private Handler mainThreadHandler;
   private List<NsdManager.RegistrationListener> registrationListeners = new ArrayList<>();
   private final CopyOnWriteArrayList<String> mMFServiceName = new CopyOnWriteArrayList<>();
   @Nullable private final NsdManagerResolverAvailState nsdManagerResolverAvailState;
+  private final long timeout;
 
   /**
    * @param context application context
    * @param nsdManagerResolverAvailState Passing NsdManagerResolverAvailState allows
    *     NsdManagerServiceResolver to synchronize on the usage of NsdManager's resolveService() API
+   * @param timeout Timeout value in case there is no response after calling resolve
    */
   public NsdManagerServiceResolver(
-      Context context, @Nullable NsdManagerResolverAvailState nsdManagerResolverAvailState) {
+      Context context,
+      @Nullable NsdManagerResolverAvailState nsdManagerResolverAvailState,
+      long timeout) {
     this.nsdManager = (NsdManager) context.getSystemService(Context.NSD_SERVICE);
-    this.mainThreadHandler = new Handler(Looper.getMainLooper());
 
     this.multicastLock =
         ((WifiManager) context.getSystemService(Context.WIFI_SERVICE))
             .createMulticastLock("chipMulticastLock");
     this.multicastLock.setReferenceCounted(true);
     this.nsdManagerResolverAvailState = nsdManagerResolverAvailState;
+    this.timeout = timeout;
   }
 
   public NsdManagerServiceResolver(Context context) {
-    this(context, null);
+    this(context, null, RESOLVE_SERVICE_TIMEOUT);
+  }
+
+  public NsdManagerServiceResolver(
+      Context context, @Nullable NsdManagerResolverAvailState nsdManagerResolverAvailState) {
+    this(context, nsdManagerResolverAvailState, RESOLVE_SERVICE_TIMEOUT);
   }
 
   @Override
@@ -82,6 +91,10 @@ public void resolve(
             + serviceType
             + "'");
 
+    if (nsdManagerResolverAvailState != null) {
+      nsdManagerResolverAvailState.acquireResolver();
+    }
+
     Runnable timeoutRunnable =
         new Runnable() {
           @Override
@@ -92,28 +105,29 @@ public void run() {
             Log.d(TAG, "resolve: Timing out");
             if (multicastLock.isHeld()) {
               multicastLock.release();
+            }
 
-              if (nsdManagerResolverAvailState != null) {
-                nsdManagerResolverAvailState.signalFree();
-              }
+            if (nsdManagerResolverAvailState != null) {
+              nsdManagerResolverAvailState.signalFree();
             }
           }
         };
 
+    ScheduledFuture<?> resolveTimeoutExecutor =
+        Executors.newSingleThreadScheduledExecutor()
+            .schedule(timeoutRunnable, timeout, TimeUnit.MILLISECONDS);
+
     NsdServiceFinderAndResolver serviceFinderResolver =
         new NsdServiceFinderAndResolver(
             this.nsdManager,
             serviceInfo,
             callbackHandle,
             contextHandle,
             chipMdnsCallback,
-            timeoutRunnable,
             multicastLock,
-            mainThreadHandler,
+            resolveTimeoutExecutor,
             nsdManagerResolverAvailState);
     serviceFinderResolver.start();
-
-    mainThreadHandler.postDelayed(timeoutRunnable, RESOLVE_SERVICE_TIMEOUT);
   }
 
   @Override

diff --git a/src/platform/android/java/chip/platform/NsdServiceFinderAndResolver.java b/src/platform/android/java/chip/platform/NsdServiceFinderAndResolver.java
@@ -21,7 +21,6 @@
 import android.net.nsd.NsdManager;
 import android.net.nsd.NsdServiceInfo;
 import android.net.wifi.WifiManager.MulticastLock;
-import android.os.Handler;
 import android.util.Log;
 import androidx.annotation.Nullable;
 import java.util.concurrent.Executors;
@@ -38,9 +37,8 @@ class NsdServiceFinderAndResolver implements NsdManager.DiscoveryListener {
   private final long callbackHandle;
   private final long contextHandle;
   private final ChipMdnsCallback chipMdnsCallback;
-  private final Runnable timeoutRunnable;
   private final MulticastLock multicastLock;
-  private final Handler mainThreadHandler;
+  private final ScheduledFuture<?> resolveTimeoutExecutor;
 
   @Nullable
   private final NsdManagerServiceResolver.NsdManagerResolverAvailState nsdManagerResolverAvailState;
@@ -53,18 +51,16 @@ public NsdServiceFinderAndResolver(
       final long callbackHandle,
       final long contextHandle,
       final ChipMdnsCallback chipMdnsCallback,
-      final Runnable timeoutRunnable,
       final MulticastLock multicastLock,
-      final Handler mainThreadHandler,
+      final ScheduledFuture<?> resolveTimeoutExecutor,
       final NsdManagerServiceResolver.NsdManagerResolverAvailState nsdManagerResolverAvailState) {
     this.nsdManager = nsdManager;
     this.targetServiceInfo = targetServiceInfo;
     this.callbackHandle = callbackHandle;
     this.contextHandle = contextHandle;
     this.chipMdnsCallback = chipMdnsCallback;
-    this.timeoutRunnable = timeoutRunnable;
     this.multicastLock = multicastLock;
-    this.mainThreadHandler = mainThreadHandler;
+    this.resolveTimeoutExecutor = resolveTimeoutExecutor;
     this.nsdManagerResolverAvailState = nsdManagerResolverAvailState;
   }
 
@@ -101,16 +97,9 @@ public void onServiceFound(NsdServiceInfo service) {
 
       if (stopDiscoveryRunnable.cancel(false)) {
         nsdManager.stopServiceDiscovery(this);
-        if (multicastLock.isHeld()) {
-          multicastLock.release();
-        }
       }
 
-      if (nsdManagerResolverAvailState != null) {
-        nsdManagerResolverAvailState.acquireResolver();
-      }
-
-      resolveService(service, callbackHandle, contextHandle, chipMdnsCallback, timeoutRunnable);
+      resolveService(service, callbackHandle, contextHandle, chipMdnsCallback);
     } else {
       Log.d(TAG, "onServiceFound: found service not a target for resolution, ignoring " + service);
     }
@@ -120,8 +109,7 @@ private void resolveService(
       NsdServiceInfo serviceInfo,
       final long callbackHandle,
       final long contextHandle,
-      final ChipMdnsCallback chipMdnsCallback,
-      Runnable timeoutRunnable) {
+      final ChipMdnsCallback chipMdnsCallback) {
     this.nsdManager.resolveService(
         serviceInfo,
         new NsdManager.ResolveListener() {
@@ -152,7 +140,7 @@ public void onResolveFailed(NsdServiceInfo serviceInfo, int errorCode) {
                 nsdManagerResolverAvailState.signalFree();
               }
             }
-            mainThreadHandler.removeCallbacks(timeoutRunnable);
+            resolveTimeoutExecutor.cancel(false);
           }
 
           @Override
@@ -188,7 +176,7 @@ public void onServiceResolved(NsdServiceInfo serviceInfo) {
                 nsdManagerResolverAvailState.signalFree();
               }
             }
-            mainThreadHandler.removeCallbacks(timeoutRunnable);
+            resolveTimeoutExecutor.cancel(false);
           }
         });
   }