From ef1af08b7a8ff43a07d64578ad0ae26dd362109e Mon Sep 17 00:00:00 2001 From: Evan Laird Date: Wed, 26 Apr 2023 17:21:29 -0400 Subject: [PATCH] Prefer the VCN subscription when filtering for CBRS This logic only matters in cases where there are exactly 2 subscriptions with the same `groupUuid` and at least one of them is opportunistic. In those cases, SystemUI was preferring to track only the subscriptionId associated with the `activeDataSubscriptionId` (per telephony) and filter out the other subscription. With this change, we now track the `vcnSubId` of the default network. `ConnectivityRepository` exposes the subscription ID of the current cellular VCN if it exists. In the case where one of the opportunistic networks is a VCN, we will prefer _that_ network rather than the current active data subscriptionId. The case that whis becomes relevant is any time the active data subscriptionId differs from the subscription id reported by the VcnTransportInfo. This case should only happen when telephony tries to switch the active data subscription and finds it not validatable. What happens then is that the VCN is still connected and we will continue to show the subscription for that connection rather than the active, which should switch back. Test: ConnectivityRepositoryImplTest Test: MobileIconsInteractorTest Bug: 222699760 Change-Id: I486ee0c323556407441de36284a1c22d74f3b836 --- .../interactor/MobileIconsInteractor.kt | 11 +- .../shared/ConnectivityInputLogger.kt | 4 + .../data/repository/ConnectivityRepository.kt | 109 +++++++++----- .../interactor/MobileIconsInteractorTest.kt | 46 ++++++ .../ConnectivityRepositoryImplTest.kt | 138 ++++++++++++++++++ .../repository/FakeConnectivityRepository.kt | 2 + 6 files changed, 271 insertions(+), 39 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractor.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractor.kt index eec91a0bca825..d42878c06bd78 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractor.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractor.kt @@ -155,7 +155,8 @@ constructor( combine( unfilteredSubscriptions, mobileConnectionsRepo.activeMobileDataSubscriptionId, - ) { unfilteredSubs, activeId -> + connectivityRepository.vcnSubId, + ) { unfilteredSubs, activeId, vcnSubId -> // Based on the old logic, if (unfilteredSubs.size != 2) { return@combine unfilteredSubs @@ -182,7 +183,13 @@ constructor( // return the non-opportunistic info return@combine if (info1.isOpportunistic) listOf(info2) else listOf(info1) } else { - return@combine if (info1.subscriptionId == activeId) { + // It's possible for the subId of the VCN to disagree with the active subId in + // cases where the system has tried to switch but found no connection. In these + // scenarios, VCN will always have the subId that we want to use, so use that + // value instead of the activeId reported by telephony + val subIdToKeep = vcnSubId ?: activeId + + return@combine if (info1.subscriptionId == subIdToKeep) { listOf(info1) } else { listOf(info2) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/shared/ConnectivityInputLogger.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/shared/ConnectivityInputLogger.kt index 82492babba46d..f05073860c415 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/shared/ConnectivityInputLogger.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/shared/ConnectivityInputLogger.kt @@ -61,6 +61,10 @@ constructor( model::messagePrinter, ) } + + fun logVcnSubscriptionId(subId: Int) { + buffer.log(TAG, LogLevel.DEBUG, { int1 = subId }, { "vcnSubId changed: $int1" }) + } } private const val TAG = "ConnectivityInputLogger" diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/shared/data/repository/ConnectivityRepository.kt b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/shared/data/repository/ConnectivityRepository.kt index 731f1e0284703..7076f345df973 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/shared/data/repository/ConnectivityRepository.kt +++ b/packages/SystemUI/src/com/android/systemui/statusbar/pipeline/shared/data/repository/ConnectivityRepository.kt @@ -27,6 +27,7 @@ import android.net.NetworkCapabilities.TRANSPORT_ETHERNET import android.net.NetworkCapabilities.TRANSPORT_WIFI import android.net.vcn.VcnTransportInfo import android.net.wifi.WifiInfo +import android.telephony.SubscriptionManager.INVALID_SUBSCRIPTION_ID import androidx.annotation.ArrayRes import androidx.annotation.VisibleForTesting import com.android.systemui.Dumpable @@ -50,10 +51,13 @@ import java.io.PrintWriter import javax.inject.Inject import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.channels.awaitClose +import kotlinx.coroutines.flow.SharedFlow import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.distinctUntilChanged +import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.onEach +import kotlinx.coroutines.flow.shareIn import kotlinx.coroutines.flow.stateIn /** @@ -66,6 +70,16 @@ interface ConnectivityRepository { /** Observable for which connection(s) are currently default. */ val defaultConnections: StateFlow + + /** + * Subscription ID of the [VcnTransportInfo] for the default connection. + * + * If the default network has a [VcnTransportInfo], then that transport info contains a subId of + * the VCN. When VCN is connected and default, this subId is what SystemUI will care about. In + * cases where telephony's activeDataSubscriptionId differs from this value, it is expected to + * eventually catch up and reflect what is represented here in the VcnTransportInfo. + */ + val vcnSubId: StateFlow } @SuppressLint("MissingPermission") @@ -118,24 +132,13 @@ constructor( initialValue = defaultHiddenIcons ) - @SuppressLint("MissingPermission") - override val defaultConnections: StateFlow = + private val defaultNetworkCapabilities: SharedFlow = conflatedCallbackFlow { val callback = object : ConnectivityManager.NetworkCallback(FLAG_INCLUDE_LOCATION_INFO) { override fun onLost(network: Network) { logger.logOnDefaultLost(network) - // The system no longer has a default network, so everything is - // non-default. - trySend( - DefaultConnectionModel( - Wifi(isDefault = false), - Mobile(isDefault = false), - CarrierMerged(isDefault = false), - Ethernet(isDefault = false), - isValidated = false, - ) - ) + trySend(null) } override fun onCapabilitiesChanged( @@ -143,30 +146,7 @@ constructor( networkCapabilities: NetworkCapabilities, ) { logger.logOnDefaultCapabilitiesChanged(network, networkCapabilities) - - val wifiInfo = - networkCapabilities.getMainOrUnderlyingWifiInfo(connectivityManager) - - val isWifiDefault = - networkCapabilities.hasTransport(TRANSPORT_WIFI) || wifiInfo != null - val isMobileDefault = - networkCapabilities.hasTransport(TRANSPORT_CELLULAR) - val isCarrierMergedDefault = wifiInfo?.isCarrierMerged == true - val isEthernetDefault = - networkCapabilities.hasTransport(TRANSPORT_ETHERNET) - - val isValidated = - networkCapabilities.hasCapability(NET_CAPABILITY_VALIDATED) - - trySend( - DefaultConnectionModel( - Wifi(isWifiDefault), - Mobile(isMobileDefault), - CarrierMerged(isCarrierMergedDefault), - Ethernet(isEthernetDefault), - isValidated, - ) - ) + trySend(networkCapabilities) } } @@ -174,6 +154,61 @@ constructor( awaitClose { connectivityManager.unregisterNetworkCallback(callback) } } + .shareIn(scope, SharingStarted.WhileSubscribed()) + + override val vcnSubId: StateFlow = + defaultNetworkCapabilities + .map { networkCapabilities -> + networkCapabilities?.run { + val subId = (transportInfo as? VcnTransportInfo)?.subId + // Never return an INVALID_SUBSCRIPTION_ID (-1) + if (subId != INVALID_SUBSCRIPTION_ID) { + subId + } else { + null + } + } + } + .distinctUntilChanged() + /* A note for logging: we use -2 here since -1 == INVALID_SUBSCRIPTION_ID */ + .onEach { logger.logVcnSubscriptionId(it ?: -2) } + .stateIn(scope, SharingStarted.Eagerly, null) + + @SuppressLint("MissingPermission") + override val defaultConnections: StateFlow = + defaultNetworkCapabilities + .map { networkCapabilities -> + if (networkCapabilities == null) { + // The system no longer has a default network, so everything is + // non-default. + DefaultConnectionModel( + Wifi(isDefault = false), + Mobile(isDefault = false), + CarrierMerged(isDefault = false), + Ethernet(isDefault = false), + isValidated = false, + ) + } else { + val wifiInfo = + networkCapabilities.getMainOrUnderlyingWifiInfo(connectivityManager) + + val isWifiDefault = + networkCapabilities.hasTransport(TRANSPORT_WIFI) || wifiInfo != null + val isMobileDefault = networkCapabilities.hasTransport(TRANSPORT_CELLULAR) + val isCarrierMergedDefault = wifiInfo?.isCarrierMerged == true + val isEthernetDefault = networkCapabilities.hasTransport(TRANSPORT_ETHERNET) + + val isValidated = networkCapabilities.hasCapability(NET_CAPABILITY_VALIDATED) + + DefaultConnectionModel( + Wifi(isWifiDefault), + Mobile(isMobileDefault), + CarrierMerged(isCarrierMergedDefault), + Ethernet(isEthernetDefault), + isValidated, + ) + } + } .distinctUntilChanged() .onEach { logger.logDefaultConnectionsChanged(it) } .stateIn(scope, SharingStarted.Eagerly, DefaultConnectionModel()) diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractorTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractorTest.kt index 6e1ab58db56d2..16104beb002c6 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractorTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/mobile/domain/interactor/MobileIconsInteractorTest.kt @@ -271,6 +271,52 @@ class MobileIconsInteractorTest : SysuiTestCase() { job.cancel() } + @Test + fun filteredSubscriptions_vcnSubId_agreesWithActiveSubId_usesActiveAkaVcnSub() = + testScope.runTest { + val (sub1, sub3) = + createSubscriptionPair( + subscriptionIds = Pair(SUB_1_ID, SUB_3_ID), + opportunistic = Pair(true, true), + grouped = true, + ) + connectionsRepository.setSubscriptions(listOf(sub1, sub3)) + connectionsRepository.setActiveMobileDataSubscriptionId(SUB_3_ID) + connectivityRepository.vcnSubId.value = SUB_3_ID + whenever(carrierConfigTracker.alwaysShowPrimarySignalBarInOpportunisticNetworkDefault) + .thenReturn(false) + + var latest: List? = null + val job = underTest.filteredSubscriptions.onEach { latest = it }.launchIn(this) + + assertThat(latest).isEqualTo(listOf(sub3)) + + job.cancel() + } + + @Test + fun filteredSubscriptions_vcnSubId_disagreesWithActiveSubId_usesVcnSub() = + testScope.runTest { + val (sub1, sub3) = + createSubscriptionPair( + subscriptionIds = Pair(SUB_1_ID, SUB_3_ID), + opportunistic = Pair(true, true), + grouped = true, + ) + connectionsRepository.setSubscriptions(listOf(sub1, sub3)) + connectionsRepository.setActiveMobileDataSubscriptionId(SUB_3_ID) + connectivityRepository.vcnSubId.value = SUB_1_ID + whenever(carrierConfigTracker.alwaysShowPrimarySignalBarInOpportunisticNetworkDefault) + .thenReturn(false) + + var latest: List? = null + val job = underTest.filteredSubscriptions.onEach { latest = it }.launchIn(this) + + assertThat(latest).isEqualTo(listOf(sub1)) + + job.cancel() + } + @Test fun activeDataConnection_turnedOn() = testScope.runTest { diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/shared/data/repository/ConnectivityRepositoryImplTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/shared/data/repository/ConnectivityRepositoryImplTest.kt index 661002d275b0f..fa4e91b68a5e3 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/shared/data/repository/ConnectivityRepositoryImplTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/shared/data/repository/ConnectivityRepositoryImplTest.kt @@ -24,6 +24,7 @@ import android.net.NetworkCapabilities.TRANSPORT_ETHERNET import android.net.NetworkCapabilities.TRANSPORT_WIFI import android.net.vcn.VcnTransportInfo import android.net.wifi.WifiInfo +import android.telephony.SubscriptionManager.INVALID_SUBSCRIPTION_ID import androidx.test.filters.SmallTest import com.android.systemui.SysuiTestCase import com.android.systemui.dump.DumpManager @@ -37,6 +38,7 @@ import com.android.systemui.statusbar.pipeline.shared.data.repository.Connectivi import com.android.systemui.tuner.TunerService import com.android.systemui.util.mockito.any import com.android.systemui.util.mockito.argumentCaptor +import com.android.systemui.util.mockito.eq import com.android.systemui.util.mockito.mock import com.google.common.truth.Truth.assertThat import kotlinx.coroutines.ExperimentalCoroutinesApi @@ -654,6 +656,139 @@ class ConnectivityRepositoryImplTest : SysuiTestCase() { job.cancel() } + @Test + fun vcnSubId_initiallyNull() { + assertThat(underTest.vcnSubId.value).isNull() + } + + @Test + fun vcnSubId_tracksVcnTransportInfo() = + testScope.runTest { + val vcnInfo = VcnTransportInfo(SUB_1_ID) + + var latest: Int? = null + val job = underTest.vcnSubId.onEach { latest = it }.launchIn(this) + + val capabilities = + mock().also { + whenever(it.hasTransport(TRANSPORT_CELLULAR)).thenReturn(true) + whenever(it.transportInfo).thenReturn(vcnInfo) + } + + getDefaultNetworkCallback().onCapabilitiesChanged(NETWORK, capabilities) + + assertThat(latest).isEqualTo(SUB_1_ID) + job.cancel() + } + + @Test + fun vcnSubId_filersOutInvalid() = + testScope.runTest { + val vcnInfo = VcnTransportInfo(INVALID_SUBSCRIPTION_ID) + + var latest: Int? = null + val job = underTest.vcnSubId.onEach { latest = it }.launchIn(this) + + val capabilities = + mock().also { + whenever(it.hasTransport(TRANSPORT_CELLULAR)).thenReturn(true) + whenever(it.transportInfo).thenReturn(vcnInfo) + } + + getDefaultNetworkCallback().onCapabilitiesChanged(NETWORK, capabilities) + + assertThat(latest).isNull() + job.cancel() + } + + @Test + fun vcnSubId_nullIfNoTransportInfo() = + testScope.runTest { + var latest: Int? = null + val job = underTest.vcnSubId.onEach { latest = it }.launchIn(this) + + val capabilities = + mock().also { + whenever(it.hasTransport(TRANSPORT_CELLULAR)).thenReturn(true) + whenever(it.transportInfo).thenReturn(null) + } + + getDefaultNetworkCallback().onCapabilitiesChanged(NETWORK, capabilities) + + assertThat(latest).isNull() + job.cancel() + } + + @Test + fun vcnSubId_nullIfVcnInfoIsNotCellular() = + testScope.runTest { + // If the underlying network of the VCN is a WiFi network, then there is no subId that + // could disagree with telephony's active data subscription id. + + var latest: Int? = null + val job = underTest.vcnSubId.onEach { latest = it }.launchIn(this) + + val wifiInfo = mock() + val vcnInfo = VcnTransportInfo(wifiInfo) + val capabilities = + mock().also { + whenever(it.hasTransport(TRANSPORT_CELLULAR)).thenReturn(true) + whenever(it.transportInfo).thenReturn(vcnInfo) + } + + getDefaultNetworkCallback().onCapabilitiesChanged(NETWORK, capabilities) + + assertThat(latest).isNull() + job.cancel() + } + + @Test + fun vcnSubId_changingVcnInfoIsTracked() = + testScope.runTest { + var latest: Int? = null + val job = underTest.vcnSubId.onEach { latest = it }.launchIn(this) + + val wifiInfo = mock() + val wifiVcnInfo = VcnTransportInfo(wifiInfo) + val sub1VcnInfo = VcnTransportInfo(SUB_1_ID) + val sub2VcnInfo = VcnTransportInfo(SUB_2_ID) + + val capabilities = + mock().also { + whenever(it.hasTransport(TRANSPORT_WIFI)).thenReturn(true) + whenever(it.transportInfo).thenReturn(wifiVcnInfo) + } + + // WIFI VCN info + getDefaultNetworkCallback().onCapabilitiesChanged(NETWORK, capabilities) + + assertThat(latest).isNull() + + // Cellular VCN info with subId 1 + whenever(capabilities.hasTransport(eq(TRANSPORT_CELLULAR))).thenReturn(true) + whenever(capabilities.transportInfo).thenReturn(sub1VcnInfo) + + getDefaultNetworkCallback().onCapabilitiesChanged(NETWORK, capabilities) + + assertThat(latest).isEqualTo(SUB_1_ID) + + // Cellular VCN info with subId 2 + whenever(capabilities.transportInfo).thenReturn(sub2VcnInfo) + + getDefaultNetworkCallback().onCapabilitiesChanged(NETWORK, capabilities) + + assertThat(latest).isEqualTo(SUB_2_ID) + + // No VCN anymore + whenever(capabilities.transportInfo).thenReturn(null) + + getDefaultNetworkCallback().onCapabilitiesChanged(NETWORK, capabilities) + + assertThat(latest).isNull() + + job.cancel() + } + @Test fun getMainOrUnderlyingWifiInfo_wifi_hasInfo() { val wifiInfo = mock() @@ -964,6 +1099,9 @@ class ConnectivityRepositoryImplTest : SysuiTestCase() { private const val SLOT_WIFI = "wifi" private const val SLOT_MOBILE = "mobile" + private const val SUB_1_ID = 1 + private const val SUB_2_ID = 2 + const val NETWORK_ID = 45 val NETWORK = mock().apply { whenever(this.getNetId()).thenReturn(NETWORK_ID) } } diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/shared/data/repository/FakeConnectivityRepository.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/shared/data/repository/FakeConnectivityRepository.kt index 9e825b7048512..8f28cc003d67f 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/shared/data/repository/FakeConnectivityRepository.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/pipeline/shared/data/repository/FakeConnectivityRepository.kt @@ -30,6 +30,8 @@ class FakeConnectivityRepository : ConnectivityRepository { override val defaultConnections: StateFlow = MutableStateFlow(DefaultConnectionModel()) + override val vcnSubId: MutableStateFlow = MutableStateFlow(null) + fun setForceHiddenIcons(hiddenIcons: Set) { _forceHiddenIcons.value = hiddenIcons }