IPD-172858 Add delay between GATT refresh and discover calls

Add delay between refresh and discover call since refresh call returns immediately and we want to give it enough time for subsequent discovery call to always succeed
SRP: refactor discover call out in its own class
Testing: Unit test + manually test GG host app + from MFA via composite build(but it did not go through GG refresh call as service change indication worked after FWUP test)

* commit '45ee8dd2d2354f108e7b14418524859a053832c8':
  IPD-172858 Add delay between GATT refresh and discover calls

platform/android/goldengate/GoldenGateConnectionManager/src/main/java/com/fitbit/goldengate/hub/LinkupWithPeerHubHandler.kt

@@ -2,11 +2,12 @@ package com.fitbit.goldengate.hub
 
 import com.fitbit.bluetooth.fbgatt.rx.GattCharacteristicSubscriptionStatus
 import com.fitbit.bluetooth.fbgatt.rx.client.BitGattPeer
+import com.fitbit.bluetooth.fbgatt.rx.client.GattServiceDiscoverer
 import com.fitbit.bluetooth.fbgatt.rx.client.GattServiceRefresher
 import com.fitbit.bluetooth.fbgatt.rx.client.PeerGattServiceSubscriber
 import com.fitbit.goldengate.bt.gatt.server.services.gattlink.GattlinkService
 import com.fitbit.goldengate.bt.gatt.server.services.gattlink.listeners.TransmitCharacteristicSubscriptionListener
-import com.fitbit.bluetooth.fbgatt.rx.dumpServices
+import com.fitbit.goldengate.node.GATT_SERVICE_REFRESH_DELAY_IN_SEC
 import com.fitbit.goldengate.node.Linkup
 import com.fitbit.linkcontroller.services.configuration.ClientPreferredConnectionConfigurationCharacteristic
 import com.fitbit.linkcontroller.services.configuration.ClientPreferredConnectionModeCharacteristic
@@ -29,7 +30,8 @@ internal class LinkupWithPeerHubHandler(
     private val transmitCharacteristicSubscriptionListener: TransmitCharacteristicSubscriptionListener = TransmitCharacteristicSubscriptionListener.instance,
     private val linkUpTimeoutSeconds: Long = LINK_UP_TIMEOUT_SECONDS,
     private val timeoutScheduler: Scheduler = Schedulers.computation(),
-    private val gattServiceRefresher: GattServiceRefresher = GattServiceRefresher()
+    private val gattServiceRefresher: GattServiceRefresher = GattServiceRefresher(),
+    private val gattServiceDiscoverer: GattServiceDiscoverer = GattServiceDiscoverer()
 ): Linkup {
 
     /**
@@ -38,57 +40,55 @@ internal class LinkupWithPeerHubHandler(
      *
      * It is required that Node is already connected
      *
-     * @param peripheral a connected Node
+     * @param peer a connected Node
      * @return Emits [Completable] if connected, error otherwise
      */
-    override fun link(central: BitGattPeer): Completable {
-        return central.connect().ignoreElement()
-            .andThen(refreshServices(central))
-            .andThen(discoverServices(central))
-            .andThen(subscribeToPreferredConnectionConfigurationCharacteristic(central))
-            .andThen(subscribeToPreferredConnectionModeCharacteristic(central))
-            .andThen(subscribeToGeneralPurposeCommandCharacteristic(central))
-            .andThen(waitForGattlinkSubscription(central))
-            .doOnComplete { Timber.d("Local Gattlink service and remote Link Configuration Service on Hub: ${central.fitbitDevice.address} have been subscribed") }
-            .doOnError { Timber.e(it, "Local Gattlink service and remote Link Configuration Service on Hub: ${central.fitbitDevice.address} haven't been subscribed") }
+    override fun link(peer: BitGattPeer): Completable {
+        return peer.connect().ignoreElement()
+            .andThen(refreshServices(peer))
+            .andThen(Completable.timer(GATT_SERVICE_REFRESH_DELAY_IN_SEC, TimeUnit.SECONDS))
+            .andThen(discoverServices(peer))
+            .andThen(subscribeToPreferredConnectionConfigurationCharacteristic(peer))
+            .andThen(subscribeToPreferredConnectionModeCharacteristic(peer))
+            .andThen(subscribeToGeneralPurposeCommandCharacteristic(peer))
+            .andThen(waitForGattlinkSubscription(peer))
+            .doOnComplete { Timber.d("Local Gattlink service and remote Link Configuration Service on Hub: ${peer.fitbitDevice.address} have been subscribed") }
+            .doOnError { Timber.e(it, "Local Gattlink service and remote Link Configuration Service on Hub: ${peer.fitbitDevice.address} haven't been subscribed") }
             .timeout(linkUpTimeoutSeconds, TimeUnit.SECONDS, timeoutScheduler)
             .doOnError { Timber.e(it, "Failed to complete linkup handler within timeout") }
     }
 
-    private fun discoverServices(central: BitGattPeer): Completable = Completable.defer {
-        central.discoverServices()
-            .doOnSuccess { dumpServices(central.services) }
-            .doOnError { Timber.w(it, "Error discovering services on $central") }
-            .ignoreElement()
+    private fun discoverServices(peer: BitGattPeer): Completable = Completable.defer {
+        gattServiceDiscoverer.discover(peer.gattConnection).ignoreElement()
     }
 
-    private fun subscribeToPreferredConnectionConfigurationCharacteristic(central: BitGattPeer): Completable {
+    private fun subscribeToPreferredConnectionConfigurationCharacteristic(peer: BitGattPeer): Completable {
         return Completable.defer {
             Timber.d("<1> subscribe ClientPreferredConnectionConfigurationCharacteristic")
             peerGattServiceSubscriber.subscribe(
-                central,
+                peer,
                 LinkConfigurationService.uuid,
                 ClientPreferredConnectionConfigurationCharacteristic.uuid
             )
         }
     }
 
-    private fun subscribeToPreferredConnectionModeCharacteristic(central: BitGattPeer): Completable {
+    private fun subscribeToPreferredConnectionModeCharacteristic(peer: BitGattPeer): Completable {
         return Completable.defer {
             Timber.d("<2> subscribe ClientPreferredConnectionModeCharacteristic")
             peerGattServiceSubscriber.subscribe(
-                central,
+                peer,
                 LinkConfigurationService.uuid,
                 ClientPreferredConnectionModeCharacteristic.uuid
             )
         }
     }
 
-    private fun subscribeToGeneralPurposeCommandCharacteristic(central: BitGattPeer): Completable {
+    private fun subscribeToGeneralPurposeCommandCharacteristic(peer: BitGattPeer): Completable {
         return Completable.defer {
             Timber.d("<3> subscribe GeneralPurposeCommandCharacteristic")
             peerGattServiceSubscriber.subscribe(
-                central,
+                peer,
                 LinkConfigurationService.uuid,
                 GeneralPurposeCommandCharacteristic.uuid
             )
@@ -106,7 +106,5 @@ internal class LinkupWithPeerHubHandler(
     private fun refreshServices(peer: BitGattPeer): Completable =
         gattServiceRefresher
             .refresh(peer.gattConnection)
-            .doOnError { Timber.w(it, "Fail to refresh services on $peer") }
-            .doOnComplete { Timber.d("Successfully refresh services on $peer") }
             .onErrorComplete()
 }

platform/android/goldengate/GoldenGateConnectionManager/src/main/java/com/fitbit/goldengate/node/LinkupWithPeerNodeHandler.kt

@@ -4,14 +4,14 @@
 package com.fitbit.goldengate.node
 
 import com.fitbit.bluetooth.fbgatt.rx.client.BitGattPeer
+import com.fitbit.bluetooth.fbgatt.rx.client.GattServiceDiscoverer
 import com.fitbit.bluetooth.fbgatt.rx.client.GattServiceRefresher
 import com.fitbit.bluetooth.fbgatt.rx.client.PeerGattServiceSubscriber
 import com.fitbit.goldengate.bt.gatt.client.services.GattDatabaseValidator
 import com.fitbit.goldengate.bt.gatt.client.services.GenericAttributeService
 import com.fitbit.goldengate.bt.gatt.server.services.gattlink.FitbitGattlinkService
 import com.fitbit.goldengate.bt.gatt.server.services.gattlink.GattlinkService
 import com.fitbit.goldengate.bt.gatt.server.services.gattlink.TransmitCharacteristic
-import com.fitbit.bluetooth.fbgatt.rx.dumpServices
 import io.reactivex.Completable
 import io.reactivex.Flowable
 import io.reactivex.Scheduler
@@ -25,13 +25,17 @@ import java.util.concurrent.atomic.AtomicInteger
 internal const val MAX_RETRY_ATTEMPTS = 3
 internal const val LINK_UP_TIMEOUT_SECONDS = 60L
 
+// add delay after GATT refresh and before discovery as refresh is a async call, and we want to give it little time to complete b4 calling discovery again
+internal const val GATT_SERVICE_REFRESH_DELAY_IN_SEC = 2L
+
 /**
  * Utility class to establish a link with given CONNECTED Node
  */
 internal class LinkupWithPeerNodeHandler(
     private val peerGattServiceSubscriber: PeerGattServiceSubscriber = PeerGattServiceSubscriber(),
     private val gattDatabaseValidator: GattDatabaseValidator = GattDatabaseValidator(),
     private val gattServiceRefresher: GattServiceRefresher = GattServiceRefresher(),
+    private val gattServiceDiscoverer: GattServiceDiscoverer = GattServiceDiscoverer(),
     private val maxRetryAttempts: Int = MAX_RETRY_ATTEMPTS,
     private val linkUpTimeoutSeconds: Long = LINK_UP_TIMEOUT_SECONDS,
     private val timeoutScheduler: Scheduler = Schedulers.computation()
@@ -73,17 +77,16 @@ internal class LinkupWithPeerNodeHandler(
                         Flowable.timer(retryCount * 2000L, TimeUnit.MILLISECONDS, Schedulers.computation())
                     }
             }
-            .onErrorResumeNext {
-                Timber.e("Failed to validate the gatt service")
+            .onErrorResumeNext { error ->
+                Timber.e(error, "Failed to validate the gatt service")
                 refreshServices(peer)
-                    .andThen(discoverServices(peer)) }
+                    .andThen(Completable.timer(GATT_SERVICE_REFRESH_DELAY_IN_SEC, TimeUnit.SECONDS))
+                    .andThen(discoverServices(peer))
+            }
     }
 
     private fun discoverServices(peer: BitGattPeer): Completable = Completable.defer {
-        peer.discoverServices()
-            .doOnSuccess { dumpServices(peer.services) }
-            .doOnError { Timber.w(it, "Error discovering services on $peer") }
-            .ignoreElement()
+        gattServiceDiscoverer.discover(peer.gattConnection).ignoreElement()
     }
 
     private fun subscribeToGenericAttribute(peer: BitGattPeer): Completable {
@@ -121,8 +124,5 @@ internal class LinkupWithPeerNodeHandler(
     }
 
     private fun refreshServices(peer: BitGattPeer): Completable =
-        gattServiceRefresher
-            .refresh(peer.gattConnection)
-            .doOnError { Timber.w(it, "Fail to refresh services on $peer") }
-            .doOnComplete { Timber.d("Successfully refresh services on $peer") }
+        gattServiceRefresher.refresh(peer.gattConnection)
 }

platform/android/goldengate/GoldenGateConnectionManager/src/test/java/com/fitbit/goldengate/hub/LinkupWithPeerHubHandlerTest.kt

@@ -6,19 +6,16 @@ import com.fitbit.bluetooth.fbgatt.GattConnection
 import com.fitbit.bluetooth.fbgatt.rx.GattCharacteristicSubscriptionStatus
 import com.fitbit.bluetooth.fbgatt.rx.GattServiceNotFoundException
 import com.fitbit.bluetooth.fbgatt.rx.client.BitGattPeer
+import com.fitbit.bluetooth.fbgatt.rx.client.GattServiceDiscoverer
 import com.fitbit.bluetooth.fbgatt.rx.client.GattServiceRefresher
 import com.fitbit.bluetooth.fbgatt.rx.client.PeerGattServiceSubscriber
 import com.fitbit.goldengate.bt.gatt.server.services.gattlink.listeners.TransmitCharacteristicSubscriptionListener
+import com.fitbit.goldengate.node.GATT_SERVICE_REFRESH_DELAY_IN_SEC
 import com.fitbit.linkcontroller.services.configuration.ClientPreferredConnectionConfigurationCharacteristic
 import com.fitbit.linkcontroller.services.configuration.ClientPreferredConnectionModeCharacteristic
 import com.fitbit.linkcontroller.services.configuration.GeneralPurposeCommandCharacteristic
 import com.fitbit.linkcontroller.services.configuration.LinkConfigurationService
-import com.nhaarman.mockitokotlin2.any
-import com.nhaarman.mockitokotlin2.doReturn
-import com.nhaarman.mockitokotlin2.mock
-import com.nhaarman.mockitokotlin2.times
-import com.nhaarman.mockitokotlin2.verify
-import com.nhaarman.mockitokotlin2.whenever
+import com.nhaarman.mockitokotlin2.*
 import io.reactivex.Completable
 import io.reactivex.Observable
 import io.reactivex.Single
@@ -47,13 +44,15 @@ class LinkupWithPeerHubHandlerTest {
     private val linkUpTimeoutSeconds = 60L
     private val testScheduler = TestScheduler()
     private val mockGattServiceRefresher = mock<GattServiceRefresher>()
+    private val mockGattServiceDiscoverer = mock<GattServiceDiscoverer>()
 
     private val handler = LinkupWithPeerHubHandler(
         mockPeerGattServiceSubscriber,
         mockTransmitCharacteristicSubscriptionListener,
         linkUpTimeoutSeconds,
         timeoutScheduler = testScheduler,
-        gattServiceRefresher = mockGattServiceRefresher
+        gattServiceRefresher = mockGattServiceRefresher,
+        gattServiceDiscoverer = mockGattServiceDiscoverer
     )
 
     @Before
@@ -78,7 +77,10 @@ class LinkupWithPeerHubHandlerTest {
 
         val testObserver = handler.link(mockCentral).test()
 
-        verify(mockCentral).discoverServices()
+        // 2 sec for delay between refresh+discover call
+        testScheduler.advanceTimeBy(GATT_SERVICE_REFRESH_DELAY_IN_SEC, TimeUnit.SECONDS)
+
+        verify(mockGattServiceDiscoverer).discover(mockGattConnection)
         verify(mockPeerGattServiceSubscriber)
             .subscribe(mockCentral, LinkConfigurationService.uuid, ClientPreferredConnectionConfigurationCharacteristic.uuid)
         verify(mockPeerGattServiceSubscriber)
@@ -98,7 +100,10 @@ class LinkupWithPeerHubHandlerTest {
 
         val testObserver = handler.link(mockCentral).test()
 
-        verify(mockCentral).discoverServices()
+        // 2 sec for delay between refresh+discover call
+        testScheduler.advanceTimeBy(GATT_SERVICE_REFRESH_DELAY_IN_SEC, TimeUnit.SECONDS)
+
+        verify(mockGattServiceDiscoverer).discover(mockGattConnection)
         verify(mockPeerGattServiceSubscriber, times(0))
             .subscribe(mockCentral, LinkConfigurationService.uuid, ClientPreferredConnectionConfigurationCharacteristic.uuid)
 
@@ -115,7 +120,10 @@ class LinkupWithPeerHubHandlerTest {
 
         val testObserver = handler.link(mockCentral).test()
 
-        verify(mockCentral).discoverServices()
+        // 2 sec for delay between refresh+discover call
+        testScheduler.advanceTimeBy(GATT_SERVICE_REFRESH_DELAY_IN_SEC, TimeUnit.SECONDS)
+
+        verify(mockGattServiceDiscoverer).discover(mockGattConnection)
         verify(mockPeerGattServiceSubscriber)
             .subscribe(mockCentral, LinkConfigurationService.uuid, ClientPreferredConnectionConfigurationCharacteristic.uuid)
 
@@ -137,7 +145,7 @@ class LinkupWithPeerHubHandlerTest {
 
         testScheduler.advanceTimeBy(LINK_UP_TIMEOUT_SECONDS, TimeUnit.SECONDS)
 
-        verify(mockCentral).discoverServices()
+        verify(mockGattServiceDiscoverer).discover(mockGattConnection)
         verify(mockPeerGattServiceSubscriber)
             .subscribe(mockCentral, LinkConfigurationService.uuid, ClientPreferredConnectionConfigurationCharacteristic.uuid)
         verify(mockPeerGattServiceSubscriber)
@@ -161,7 +169,10 @@ class LinkupWithPeerHubHandlerTest {
 
         val testObserver = handler.link(mockCentral).test()
 
-        verify(mockCentral).discoverServices()
+        // 2 sec for delay between refresh+discover call
+        testScheduler.advanceTimeBy(GATT_SERVICE_REFRESH_DELAY_IN_SEC, TimeUnit.SECONDS)
+
+        verify(mockGattServiceDiscoverer).discover(mockGattConnection)
         verify(mockPeerGattServiceSubscriber)
             .subscribe(mockCentral, LinkConfigurationService.uuid, ClientPreferredConnectionConfigurationCharacteristic.uuid)
         verify(mockPeerGattServiceSubscriber)
@@ -175,10 +186,10 @@ class LinkupWithPeerHubHandlerTest {
     }
 
     private fun mockDiscoverServicesSuccess(services: List<BluetoothGattService> = emptyList()) =
-        whenever(mockCentral.discoverServices()).thenReturn(Single.just(services))
+        whenever(mockGattServiceDiscoverer.discover(mockGattConnection)).thenReturn(Single.just(services))
 
     private fun mockDiscoverServicesFailure() =
-        whenever(mockCentral.discoverServices()).thenReturn(Single.error(RuntimeException("Failed")))
+        whenever(mockGattServiceDiscoverer.discover(mockGattConnection)).thenReturn(Single.error(RuntimeException("Failed")))
 
     private fun mockLinkConfigurationServiceNotFound() =
         whenever(mockPeerGattServiceSubscriber.subscribe(