From 83bdcfd2452b8b348b0748cc30e5ee967903eb4f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joris=20Pelgr=C3=B6m?= Date: Sat, 19 Feb 2022 19:48:07 +0100 Subject: [PATCH] Improve Wear OS error handling on home, check websocket connection state (#2261) * Websocket: make connected Job Deferred to get errors - Switch the job used for monitoring whether a connection has been established to use Deferred, which will allow us to get the exception that caused the job to be cancelled (and also makes the code a little easier) * Websocket: add variable for state - Add a variable to the websocket repository to monitor the state of the connection. This will allow the app to respond to handled exceptions. * More Websocket race condition fixes - If multiple outgoing messages are pending before the connection is opened, if we receive an invalid auth message or onFailure callback this will be processed last. The connected job is still 'completed', so it shouldn't return true but return if it was successful. This will ensure that any messages after the first one that triggered the failure, will be ignored until the job is reset. - Set the connection state to authenticating before sending the message, to make sure that we don't accidentally override a very quick response * Handle error states in HomeView - Add a state for the loading progress to the view model in order to be able to update the home view with errors if necessary - If loading the main data failed because of authentication, reset the session like in the phone app and send the user back to onboarding - Fix the app getting stuck on a loading screen, even if everything loaded correctly, if there are no supported entity domains in the response --- .../data/websocket/WebSocketRepository.kt | 1 + .../common/data/websocket/WebSocketState.kt | 5 + .../websocket/impl/WebSocketRepositoryImpl.kt | 29 +++-- common/src/main/res/values/strings.xml | 3 + .../companion/android/home/HomePresenter.kt | 3 + .../android/home/HomePresenterImpl.kt | 11 +- .../companion/android/home/MainViewModel.kt | 106 ++++++++++++------ .../companion/android/home/views/HomeView.kt | 5 +- .../companion/android/home/views/MainView.kt | 59 ++++++++-- 9 files changed, 165 insertions(+), 57 deletions(-) create mode 100644 common/src/main/java/io/homeassistant/companion/android/common/data/websocket/WebSocketState.kt diff --git a/common/src/main/java/io/homeassistant/companion/android/common/data/websocket/WebSocketRepository.kt b/common/src/main/java/io/homeassistant/companion/android/common/data/websocket/WebSocketRepository.kt index 8ee7d2f5e74..98066fb8513 100644 --- a/common/src/main/java/io/homeassistant/companion/android/common/data/websocket/WebSocketRepository.kt +++ b/common/src/main/java/io/homeassistant/companion/android/common/data/websocket/WebSocketRepository.kt @@ -15,6 +15,7 @@ import kotlinx.coroutines.flow.Flow @ExperimentalCoroutinesApi interface WebSocketRepository { + fun getConnectionState(): WebSocketState? suspend fun sendPing(): Boolean suspend fun getConfig(): GetConfigResponse? suspend fun getStates(): List>? diff --git a/common/src/main/java/io/homeassistant/companion/android/common/data/websocket/WebSocketState.kt b/common/src/main/java/io/homeassistant/companion/android/common/data/websocket/WebSocketState.kt new file mode 100644 index 00000000000..1d8c578e45f --- /dev/null +++ b/common/src/main/java/io/homeassistant/companion/android/common/data/websocket/WebSocketState.kt @@ -0,0 +1,5 @@ +package io.homeassistant.companion.android.common.data.websocket + +enum class WebSocketState { + AUTHENTICATING, ACTIVE, CLOSED_AUTH, CLOSED_OTHER +} diff --git a/common/src/main/java/io/homeassistant/companion/android/common/data/websocket/impl/WebSocketRepositoryImpl.kt b/common/src/main/java/io/homeassistant/companion/android/common/data/websocket/impl/WebSocketRepositoryImpl.kt index f2ba5cf418c..f52e2a6863e 100644 --- a/common/src/main/java/io/homeassistant/companion/android/common/data/websocket/impl/WebSocketRepositoryImpl.kt +++ b/common/src/main/java/io/homeassistant/companion/android/common/data/websocket/impl/WebSocketRepositoryImpl.kt @@ -14,6 +14,7 @@ import io.homeassistant.companion.android.common.data.integration.ServiceData import io.homeassistant.companion.android.common.data.integration.impl.entities.EntityResponse import io.homeassistant.companion.android.common.data.url.UrlRepository import io.homeassistant.companion.android.common.data.websocket.WebSocketRepository +import io.homeassistant.companion.android.common.data.websocket.WebSocketState import io.homeassistant.companion.android.common.data.websocket.impl.entities.AreaRegistryResponse import io.homeassistant.companion.android.common.data.websocket.impl.entities.AreaRegistryUpdatedEvent import io.homeassistant.companion.android.common.data.websocket.impl.entities.DeviceRegistryResponse @@ -26,6 +27,7 @@ import io.homeassistant.companion.android.common.data.websocket.impl.entities.Ge import io.homeassistant.companion.android.common.data.websocket.impl.entities.SocketResponse import io.homeassistant.companion.android.common.data.websocket.impl.entities.StateChangedEvent import kotlinx.coroutines.CancellableContinuation +import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi @@ -78,8 +80,9 @@ class WebSocketRepositoryImpl @Inject constructor( private val responseCallbackJobs = mutableMapOf>() private val id = AtomicLong(1) private var connection: WebSocket? = null + private var connectionState: WebSocketState? = null private val connectedMutex = Mutex() - private var connected = Job() + private var connected = CompletableDeferred() private val eventSubscriptionMutex = Mutex() private val eventSubscriptionFlow = mutableMapOf>() private var eventSubscriptionProducerScope = mutableMapOf>() @@ -87,6 +90,8 @@ class WebSocketRepositoryImpl @Inject constructor( private var notificationFlow: Flow>? = null private var notificationProducerScope: ProducerScope>? = null + override fun getConnectionState(): WebSocketState? = connectionState + override suspend fun sendPing(): Boolean { val socketResponse = sendMessage( mapOf( @@ -256,7 +261,7 @@ class WebSocketRepositoryImpl @Inject constructor( private suspend fun connect(): Boolean { connectedMutex.withLock { if (connection != null && connected.isCompleted) { - return true + return !connected.isCancelled } val url = urlRepository.getUrl() @@ -275,6 +280,7 @@ class WebSocketRepositoryImpl @Inject constructor( this ).also { // Preemptively send auth + connectionState = WebSocketState.AUTHENTICATING it.send( mapper.writeValueAsString( mapOf( @@ -288,9 +294,7 @@ class WebSocketRepositoryImpl @Inject constructor( // Wait up to 30 seconds for auth response return true == withTimeoutOrNull(30000) { return@withTimeoutOrNull try { - connected.join() - if (connected.isCancelled) throw AuthorizationException() - true + connected.await() } catch (e: Exception) { Log.e(TAG, "Unable to authenticate", e) false @@ -327,10 +331,13 @@ class WebSocketRepositoryImpl @Inject constructor( if (response?.result != null) mapper.convertValue(response.result) else null private fun handleAuthComplete(successful: Boolean) { - if (successful) - connected.complete() - else - connected.completeExceptionally(Exception("Authentication Error")) + if (successful) { + connectionState = WebSocketState.ACTIVE + connected.complete(true) + } else { + connectionState = WebSocketState.CLOSED_AUTH + connected.completeExceptionally(AuthorizationException()) + } } private fun handleMessage(response: SocketResponse) { @@ -378,8 +385,10 @@ class WebSocketRepositoryImpl @Inject constructor( private fun handleClosingSocket() { ioScope.launch { connectedMutex.withLock { - connected = Job() + connected = CompletableDeferred() connection = null + if (connectionState != WebSocketState.CLOSED_AUTH) + connectionState = WebSocketState.CLOSED_OTHER } } // If we still have flows flowing diff --git a/common/src/main/res/values/strings.xml b/common/src/main/res/values/strings.xml index 2766ff7d9f0..394e9a4872b 100644 --- a/common/src/main/res/values/strings.xml +++ b/common/src/main/res/values/strings.xml @@ -171,6 +171,7 @@ Current state and attribute of any entity It appears that your authorization was revoked, please reconnect to Home Assistant. Unable to connect to Home Assistant. + Error loading entities Unable to connect to Home Assistant. Unable to communicate with Home Assistant because of a SSL error. Please ensure your certificate is valid. Please check to ensure you have the mobile_app\nintegration enabled on your home assistant instance. @@ -329,6 +330,8 @@ Please fill out the form first You have not received any notifications yet No Notifications + No Supported Entities + Check the documentation for more information When you add a widget to your home screen, you will see it here No Widgets Found None diff --git a/wear/src/main/java/io/homeassistant/companion/android/home/HomePresenter.kt b/wear/src/main/java/io/homeassistant/companion/android/home/HomePresenter.kt index 14bbb81c5b2..a207a21775b 100644 --- a/wear/src/main/java/io/homeassistant/companion/android/home/HomePresenter.kt +++ b/wear/src/main/java/io/homeassistant/companion/android/home/HomePresenter.kt @@ -1,6 +1,7 @@ package io.homeassistant.companion.android.home import io.homeassistant.companion.android.common.data.integration.Entity +import io.homeassistant.companion.android.common.data.websocket.WebSocketState import io.homeassistant.companion.android.common.data.websocket.impl.entities.AreaRegistryResponse import io.homeassistant.companion.android.common.data.websocket.impl.entities.AreaRegistryUpdatedEvent import io.homeassistant.companion.android.common.data.websocket.impl.entities.DeviceRegistryResponse @@ -17,9 +18,11 @@ interface HomePresenter { fun onViewReady() suspend fun onEntityClicked(entityId: String, state: String) fun onLogoutClicked() + fun onInvalidAuthorization() fun onFinish() suspend fun isConnected(): Boolean + fun getWebSocketState(): WebSocketState? suspend fun getEntities(): List>? suspend fun getEntityUpdates(): Flow>? diff --git a/wear/src/main/java/io/homeassistant/companion/android/home/HomePresenterImpl.kt b/wear/src/main/java/io/homeassistant/companion/android/home/HomePresenterImpl.kt index 2d8a21d027c..842b0276514 100644 --- a/wear/src/main/java/io/homeassistant/companion/android/home/HomePresenterImpl.kt +++ b/wear/src/main/java/io/homeassistant/companion/android/home/HomePresenterImpl.kt @@ -8,6 +8,7 @@ import io.homeassistant.companion.android.common.data.integration.DeviceRegistra import io.homeassistant.companion.android.common.data.integration.Entity import io.homeassistant.companion.android.common.data.integration.IntegrationRepository import io.homeassistant.companion.android.common.data.websocket.WebSocketRepository +import io.homeassistant.companion.android.common.data.websocket.WebSocketState import io.homeassistant.companion.android.common.data.websocket.impl.entities.AreaRegistryResponse import io.homeassistant.companion.android.common.data.websocket.impl.entities.AreaRegistryUpdatedEvent import io.homeassistant.companion.android.common.data.websocket.impl.entities.DeviceRegistryResponse @@ -101,7 +102,11 @@ class HomePresenterImpl @Inject constructor( ) } - override fun onLogoutClicked() { + override fun onInvalidAuthorization() = finishSession() + + override fun onLogoutClicked() = finishSession() + + private fun finishSession() { mainScope.launch { authenticationUseCase.revokeSession() view.displayOnBoarding() @@ -132,6 +137,10 @@ class HomePresenterImpl @Inject constructor( return integrationUseCase.isRegistered() } + override fun getWebSocketState(): WebSocketState? { + return webSocketUseCase.getConnectionState() + } + override suspend fun getAreaRegistry(): List? { return webSocketUseCase.getAreaRegistry() } diff --git a/wear/src/main/java/io/homeassistant/companion/android/home/MainViewModel.kt b/wear/src/main/java/io/homeassistant/companion/android/home/MainViewModel.kt index 04141efaed2..816ebfbc13e 100644 --- a/wear/src/main/java/io/homeassistant/companion/android/home/MainViewModel.kt +++ b/wear/src/main/java/io/homeassistant/companion/android/home/MainViewModel.kt @@ -1,6 +1,7 @@ package io.homeassistant.companion.android.home import android.app.Application +import android.util.Log import androidx.compose.runtime.mutableStateListOf import androidx.compose.runtime.mutableStateMapOf import androidx.compose.runtime.mutableStateOf @@ -10,6 +11,7 @@ import androidx.lifecycle.viewModelScope import dagger.hilt.android.lifecycle.HiltViewModel import io.homeassistant.companion.android.HomeAssistantApplication import io.homeassistant.companion.android.common.data.integration.Entity +import io.homeassistant.companion.android.common.data.websocket.WebSocketState import io.homeassistant.companion.android.common.data.websocket.impl.entities.AreaRegistryResponse import io.homeassistant.companion.android.common.data.websocket.impl.entities.DeviceRegistryResponse import io.homeassistant.companion.android.common.data.websocket.impl.entities.EntityRegistryResponse @@ -27,6 +29,14 @@ import javax.inject.Inject @HiltViewModel class MainViewModel @Inject constructor(application: Application) : AndroidViewModel(application) { + companion object { + const val TAG = "MainViewModel" + } + + enum class LoadingState { + LOADING, READY, ERROR + } + private lateinit var homePresenter: HomePresenter val app = getApplication() private val favoritesDao = AppDatabase.getInstance(app.applicationContext).favoritesDao() @@ -38,6 +48,7 @@ class MainViewModel @Inject constructor(application: Application) : AndroidViewM // TODO: This is bad, do this instead: https://stackoverflow.com/questions/46283981/android-viewmodel-additional-arguments fun init(homePresenter: HomePresenter) { this.homePresenter = homePresenter + loadSettings() loadEntities() getFavorites() getSensors() @@ -68,6 +79,8 @@ class MainViewModel @Inject constructor(application: Application) : AndroidViewM var entityListFilter: (Entity<*>) -> Boolean = { true } // settings + var loadingState = mutableStateOf(LoadingState.LOADING) + private set var isHapticEnabled = mutableStateOf(false) private set var isToastEnabled = mutableStateOf(false) @@ -90,7 +103,7 @@ class MainViewModel @Inject constructor(application: Application) : AndroidViewM var sensors = mutableStateListOf() - private fun loadEntities() { + private fun loadSettings() { viewModelScope.launch { if (!homePresenter.isConnected()) { return@launch @@ -101,49 +114,76 @@ class MainViewModel @Inject constructor(application: Application) : AndroidViewM isShowShortcutTextEnabled.value = homePresenter.getShowShortcutText() templateTileContent.value = homePresenter.getTemplateTileContent() templateTileRefreshInterval.value = homePresenter.getTemplateTileRefreshInterval() + } + } - homePresenter.getAreaRegistry()?.let { - areaRegistry = it - areas.addAll(it) + fun loadEntities() { + viewModelScope.launch { + if (!homePresenter.isConnected()) { + return@launch } - deviceRegistry = homePresenter.getDeviceRegistry() - entityRegistry = homePresenter.getEntityRegistry() - homePresenter.getEntities()?.forEach { - if (supportedDomains().contains(it.entityId.split(".")[0])) { - entities[it.entityId] = it + try { + // Load initial state + loadingState.value = LoadingState.LOADING + homePresenter.getAreaRegistry()?.let { + areaRegistry = it + areas.addAll(it) } - } - updateEntityDomains() - - viewModelScope.launch { - homePresenter.getEntityUpdates()?.collect { + deviceRegistry = homePresenter.getDeviceRegistry() + entityRegistry = homePresenter.getEntityRegistry() + homePresenter.getEntities()?.forEach { if (supportedDomains().contains(it.entityId.split(".")[0])) { entities[it.entityId] = it - updateEntityDomains() } } - } - viewModelScope.launch { - homePresenter.getAreaRegistryUpdates()?.collect { - areaRegistry = homePresenter.getAreaRegistry() - areas.clear() - areaRegistry?.let { - areas.addAll(it) + updateEntityDomains() + + // Finished initial load, update state + val webSocketState = homePresenter.getWebSocketState() + if (webSocketState == WebSocketState.CLOSED_AUTH) { + homePresenter.onInvalidAuthorization() + return@launch + } + loadingState.value = if (webSocketState == WebSocketState.ACTIVE) { + LoadingState.READY + } else { + LoadingState.ERROR + } + + // Listen for updates + viewModelScope.launch { + homePresenter.getEntityUpdates()?.collect { + if (supportedDomains().contains(it.entityId.split(".")[0])) { + entities[it.entityId] = it + updateEntityDomains() + } } - updateEntityDomains() } - } - viewModelScope.launch { - homePresenter.getDeviceRegistryUpdates()?.collect { - deviceRegistry = homePresenter.getDeviceRegistry() - updateEntityDomains() + viewModelScope.launch { + homePresenter.getAreaRegistryUpdates()?.collect { + areaRegistry = homePresenter.getAreaRegistry() + areas.clear() + areaRegistry?.let { + areas.addAll(it) + } + updateEntityDomains() + } } - } - viewModelScope.launch { - homePresenter.getEntityRegistryUpdates()?.collect { - entityRegistry = homePresenter.getEntityRegistry() - updateEntityDomains() + viewModelScope.launch { + homePresenter.getDeviceRegistryUpdates()?.collect { + deviceRegistry = homePresenter.getDeviceRegistry() + updateEntityDomains() + } + } + viewModelScope.launch { + homePresenter.getEntityRegistryUpdates()?.collect { + entityRegistry = homePresenter.getEntityRegistry() + updateEntityDomains() + } } + } catch (e: Exception) { + Log.e(TAG, "Exception while loading entities", e) + loadingState.value = LoadingState.ERROR } } } diff --git a/wear/src/main/java/io/homeassistant/companion/android/home/views/HomeView.kt b/wear/src/main/java/io/homeassistant/companion/android/home/views/HomeView.kt index 1ab200f6e04..16fa711a522 100644 --- a/wear/src/main/java/io/homeassistant/companion/android/home/views/HomeView.kt +++ b/wear/src/main/java/io/homeassistant/companion/android/home/views/HomeView.kt @@ -56,7 +56,9 @@ fun LoadHomePage( val context = LocalContext.current WearAppTheme { - if (mainViewModel.entities.isNullOrEmpty() && mainViewModel.favoriteEntityIds.isNullOrEmpty()) { + if (mainViewModel.loadingState.value == MainViewModel.LoadingState.LOADING && + mainViewModel.favoriteEntityIds.isNullOrEmpty() + ) { Column { ListHeader(id = commonR.string.loading) Chip( @@ -83,6 +85,7 @@ fun LoadHomePage( mainViewModel, mainViewModel.favoriteEntityIds, { id, state -> mainViewModel.toggleEntity(id, state) }, + mainViewModel::loadEntities, { swipeDismissableNavController.navigate(SCREEN_SETTINGS) }, { lists, order, filter -> mainViewModel.entityLists.clear() diff --git a/wear/src/main/java/io/homeassistant/companion/android/home/views/MainView.kt b/wear/src/main/java/io/homeassistant/companion/android/home/views/MainView.kt index 51414cf3e47..80d0ba6d8ab 100644 --- a/wear/src/main/java/io/homeassistant/companion/android/home/views/MainView.kt +++ b/wear/src/main/java/io/homeassistant/companion/android/home/views/MainView.kt @@ -5,6 +5,7 @@ import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.Spacer import androidx.compose.foundation.layout.fillMaxWidth import androidx.compose.foundation.layout.height +import androidx.compose.foundation.layout.padding import androidx.compose.runtime.Composable import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf @@ -23,6 +24,7 @@ import androidx.compose.ui.unit.dp import androidx.wear.compose.material.Chip import androidx.wear.compose.material.ChipDefaults import androidx.wear.compose.material.ExperimentalWearMaterialApi +import androidx.wear.compose.material.MaterialTheme import androidx.wear.compose.material.PositionIndicator import androidx.wear.compose.material.Scaffold import androidx.wear.compose.material.ScalingLazyListState @@ -46,6 +48,7 @@ fun MainView( mainViewModel: MainViewModel, favoriteEntityIds: List, onEntityClicked: (String, String) -> Unit, + onRetryLoadEntitiesClicked: () -> Unit, onSettingsClicked: () -> Unit, onTestClicked: (entityLists: Map>>, listOrder: List, filter: (Entity<*>) -> (Boolean)) -> Unit, isHapticEnabled: Boolean, @@ -125,20 +128,52 @@ fun MainView( } } } - if (mainViewModel.entities.isEmpty()) { - item { - Column { - ListHeader(id = commonR.string.loading) - Chip( - label = { + item { + Column { + when (mainViewModel.loadingState.value) { + MainViewModel.LoadingState.LOADING -> { + ListHeader(id = commonR.string.loading) + Chip( + label = { + Text( + text = stringResource(commonR.string.loading_entities), + textAlign = TextAlign.Center + ) + }, + onClick = { /* No op */ }, + colors = ChipDefaults.primaryChipColors() + ) + } + MainViewModel.LoadingState.READY -> { + if (mainViewModel.entities.isEmpty()) { Text( - text = stringResource(commonR.string.loading_entities), - textAlign = TextAlign.Center + text = stringResource(commonR.string.no_supported_entities), + textAlign = TextAlign.Center, + style = MaterialTheme.typography.title3, + modifier = Modifier.fillMaxWidth().padding(top = 32.dp) ) - }, - onClick = { /* No op */ }, - colors = ChipDefaults.primaryChipColors() - ) + Text( + text = stringResource(commonR.string.no_supported_entities_summary), + textAlign = TextAlign.Center, + style = MaterialTheme.typography.body2, + modifier = Modifier.fillMaxWidth().padding(top = 8.dp) + ) + } + } + MainViewModel.LoadingState.ERROR -> { + ListHeader(id = commonR.string.error_loading_entities) + Chip( + label = { + Text( + text = stringResource(commonR.string.retry), + textAlign = TextAlign.Center, + modifier = Modifier.fillMaxWidth() + ) + }, + onClick = onRetryLoadEntitiesClicked, + colors = ChipDefaults.primaryChipColors() + ) + } } } }