-
-
Notifications
You must be signed in to change notification settings - Fork 966
Add error screen no connection on Automotive #6857
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
dcf0d45
16db884
037591c
707dba9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| package io.homeassistant.companion.android.vehicle | ||
|
|
||
| import dagger.Binds | ||
| import dagger.Module | ||
| import dagger.hilt.InstallIn | ||
| import dagger.hilt.components.SingletonComponent | ||
| import io.homeassistant.companion.android.common.data.servers.ServerManager | ||
| import javax.inject.Inject | ||
| import javax.inject.Singleton | ||
| import kotlin.coroutines.cancellation.CancellationException | ||
| import kotlin.time.Duration | ||
| import kotlin.time.Duration.Companion.seconds | ||
| import kotlinx.coroutines.ExperimentalCoroutinesApi | ||
| import kotlinx.coroutines.currentCoroutineContext | ||
| import kotlinx.coroutines.delay | ||
| import kotlinx.coroutines.flow.Flow | ||
| import kotlinx.coroutines.flow.distinctUntilChanged | ||
| import kotlinx.coroutines.flow.flow | ||
| import kotlinx.coroutines.flow.transformLatest | ||
| import kotlinx.coroutines.isActive | ||
| import timber.log.Timber | ||
|
|
||
| sealed interface ConnectionAvailability { | ||
| data object Available : ConnectionAvailability | ||
| data object Unavailable : ConnectionAvailability | ||
| } | ||
|
|
||
| interface ConnectionAvailabilityMonitor { | ||
| fun observeAvailability(): Flow<ConnectionAvailability> | ||
| } | ||
|
|
||
| internal val GRACE_PERIOD: Duration = 10.seconds | ||
|
|
||
| internal val HEALTHY_POLL_INTERVAL: Duration = 15.seconds | ||
|
|
||
| internal val DEGRADED_POLL_INTERVAL: Duration = 1.seconds | ||
|
Comment on lines
+32
to
+36
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This probably should only be |
||
|
|
||
| @Singleton | ||
| internal class ConnectionAvailabilityMonitorImpl @Inject constructor(private val serverManager: ServerManager) : | ||
| ConnectionAvailabilityMonitor { | ||
|
|
||
| @OptIn(ExperimentalCoroutinesApi::class) | ||
| override fun observeAvailability(): Flow<ConnectionAvailability> { | ||
| return haReachableFlow() | ||
| .transformLatest { reachable -> | ||
| if (reachable) { | ||
| emit(ConnectionAvailability.Available) | ||
| } else { | ||
| delay(GRACE_PERIOD) | ||
| emit(ConnectionAvailability.Unavailable) | ||
| } | ||
| } | ||
| .distinctUntilChanged() | ||
| } | ||
|
|
||
| private fun haReachableFlow(): Flow<Boolean> = flow { | ||
| while (currentCoroutineContext().isActive) { | ||
| val reachable = isHaReachable() | ||
| emit(reachable) | ||
| delay(if (reachable) HEALTHY_POLL_INTERVAL else DEGRADED_POLL_INTERVAL) | ||
| } | ||
| }.distinctUntilChanged() | ||
|
|
||
| private suspend fun isHaReachable(): Boolean { | ||
| return try { | ||
| if (!serverManager.isRegistered()) { | ||
| true | ||
| } else { | ||
| serverManager.webSocketRepository().sendPing() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are only checking the current active server, it needs to be documented, in the class documentation for instance. |
||
| } | ||
| } catch (e: CancellationException) { | ||
| throw e | ||
| } catch (e: Exception) { | ||
| Timber.w(e, "Failed to ping WebSocket") | ||
| false | ||
| } | ||
|
cddu33 marked this conversation as resolved.
|
||
| } | ||
| } | ||
|
|
||
| @Module | ||
| @InstallIn(SingletonComponent::class) | ||
| internal abstract class ConnectionAvailabilityMonitorModule { | ||
| @Binds | ||
| @Singleton | ||
| abstract fun bindConnectionAvailabilityMonitor( | ||
| impl: ConnectionAvailabilityMonitorImpl, | ||
| ): ConnectionAvailabilityMonitor | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| package io.homeassistant.companion.android.vehicle | ||
|
|
||
| import androidx.car.app.CarContext | ||
| import androidx.car.app.Screen | ||
| import androidx.car.app.model.Action | ||
| import androidx.car.app.model.CarColor | ||
| import androidx.car.app.model.CarIcon | ||
| import androidx.car.app.model.MessageTemplate | ||
| import androidx.car.app.model.Template | ||
| import com.mikepenz.iconics.IconicsDrawable | ||
| import com.mikepenz.iconics.typeface.library.community.material.CommunityMaterial | ||
| import com.mikepenz.iconics.utils.sizeDp | ||
| import com.mikepenz.iconics.utils.toAndroidIconCompat | ||
| import io.homeassistant.companion.android.common.R | ||
| import io.homeassistant.companion.android.util.vehicle.getHeaderBuilder | ||
|
|
||
| class NoConnectionScreen(context: CarContext) : Screen(context) { | ||
|
|
||
| override fun onGetTemplate(): Template { | ||
| val icon = CarIcon.Builder( | ||
| IconicsDrawable(carContext, CommunityMaterial.Icon3.cmd_wifi_off).apply { | ||
| sizeDp = 64 | ||
| }.toAndroidIconCompat(), | ||
| ) | ||
| .setTint(CarColor.DEFAULT) | ||
| .build() | ||
|
|
||
| return MessageTemplate.Builder(carContext.getString(R.string.error_connection_failed_no_network)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are assuming that when we don't have a WebSocket connection we have no network. It might not be true, the server might simply be down. If you check to the new error screen when we are not able to reach out to the server we show a way to test what is currently wrong check
We could replicate this or simply change the message to say that the connection to the server is broken maybe? @jpelgrom might have another opinion on the wording used.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Replicating the entire screen feels like a bit much for a car, but a general header "unable to connect" and a short sentence describing the reason, like the main app, would be nice I think. We also don't have to discuss the wording in that case ;)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree that we don't need to replicate, but the message and the logo should not say that there are no network available that's potentially not the issue, so it should be more generic. |
||
| .setHeader( | ||
| carContext.getHeaderBuilder( | ||
| title = R.string.aa_no_connection_title, | ||
| action = Action.APP_ICON, | ||
| ).build(), | ||
| ) | ||
| .setIcon(icon) | ||
| .build() | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,111 @@ | ||||||
| package io.homeassistant.companion.android.vehicle | ||||||
|
|
||||||
| import app.cash.turbine.test | ||||||
| import io.homeassistant.companion.android.common.data.servers.ServerManager | ||||||
| import io.homeassistant.companion.android.common.data.websocket.WebSocketRepository | ||||||
| import io.homeassistant.companion.android.testing.unit.ConsoleLogExtension | ||||||
| import io.homeassistant.companion.android.testing.unit.MainDispatcherJUnit5Extension | ||||||
| import io.mockk.coEvery | ||||||
| import io.mockk.mockk | ||||||
| import kotlin.time.Duration.Companion.seconds | ||||||
| import kotlinx.coroutines.ExperimentalCoroutinesApi | ||||||
| import kotlinx.coroutines.test.advanceTimeBy | ||||||
| import kotlinx.coroutines.test.runTest | ||||||
| import org.junit.jupiter.api.Assertions.assertEquals | ||||||
| import org.junit.jupiter.api.Test | ||||||
| import org.junit.jupiter.api.extension.ExtendWith | ||||||
|
|
||||||
| @OptIn(ExperimentalCoroutinesApi::class) | ||||||
| @ExtendWith(MainDispatcherJUnit5Extension::class, ConsoleLogExtension::class) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I don't think you need the extension for the MainDispatcher here. |
||||||
| class ConnectionAvailabilityMonitorTest { | ||||||
|
|
||||||
| private val serverManager: ServerManager = mockk(relaxed = true) | ||||||
|
|
||||||
| private fun createMonitor(): ConnectionAvailabilityMonitor = ConnectionAvailabilityMonitorImpl(serverManager) | ||||||
|
|
||||||
| private fun stubPings(vararg results: Boolean) { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
A stub is something dumb, here it's real mocking. |
||||||
| val webSocketRepository = mockk<WebSocketRepository>() | ||||||
| coEvery { webSocketRepository.sendPing() } returnsMany results.toList() | ||||||
|
cddu33 marked this conversation as resolved.
Outdated
|
||||||
| coEvery { serverManager.webSocketRepository() } returns webSocketRepository | ||||||
| } | ||||||
|
|
||||||
| @Test | ||||||
| fun `Given no server registered when observing then emits Available`() = runTest { | ||||||
| coEvery { serverManager.isRegistered() } returns false | ||||||
|
|
||||||
| createMonitor().observeAvailability().test { | ||||||
| assertEquals(ConnectionAvailability.Available, awaitItem()) | ||||||
| cancelAndConsumeRemainingEvents() | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| @Test | ||||||
| fun `Given server registered and ping succeeds when observing then emits Available`() = runTest { | ||||||
| coEvery { serverManager.isRegistered() } returns true | ||||||
| stubPings(true) | ||||||
|
|
||||||
| createMonitor().observeAvailability().test { | ||||||
| assertEquals(ConnectionAvailability.Available, awaitItem()) | ||||||
| cancelAndConsumeRemainingEvents() | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| @Test | ||||||
| fun `Given server registered and ping fails when grace period elapses then emits Unavailable`() = runTest { | ||||||
| coEvery { serverManager.isRegistered() } returns true | ||||||
| stubPings(false) | ||||||
|
|
||||||
| createMonitor().observeAvailability().test { | ||||||
| advanceTimeBy(GRACE_PERIOD - 1.seconds) | ||||||
| expectNoEvents() | ||||||
| advanceTimeBy(2.seconds) | ||||||
| assertEquals(ConnectionAvailability.Unavailable, awaitItem()) | ||||||
| cancelAndConsumeRemainingEvents() | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| @Test | ||||||
| fun `Given ping recovers during grace period when polling then emits Available without Unavailable`() = runTest { | ||||||
| coEvery { serverManager.isRegistered() } returns true | ||||||
| stubPings(false, true) | ||||||
|
|
||||||
| createMonitor().observeAvailability().test { | ||||||
| advanceTimeBy(DEGRADED_POLL_INTERVAL + 1.seconds) | ||||||
| assertEquals(ConnectionAvailability.Available, awaitItem()) | ||||||
| advanceTimeBy(GRACE_PERIOD) | ||||||
| expectNoEvents() | ||||||
| cancelAndConsumeRemainingEvents() | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| @Test | ||||||
| fun `Given ping goes from success to failure when grace elapses then emits Unavailable`() = runTest { | ||||||
| coEvery { serverManager.isRegistered() } returns true | ||||||
| stubPings(true, false, false) | ||||||
|
|
||||||
| createMonitor().observeAvailability().test { | ||||||
| assertEquals(ConnectionAvailability.Available, awaitItem()) | ||||||
| advanceTimeBy(HEALTHY_POLL_INTERVAL + GRACE_PERIOD + 1.seconds) | ||||||
| assertEquals(ConnectionAvailability.Unavailable, awaitItem()) | ||||||
| cancelAndConsumeRemainingEvents() | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| @Test | ||||||
| fun `Given monitor is Unavailable when ping recovers then emits Available`() = runTest { | ||||||
| coEvery { serverManager.isRegistered() } returns true | ||||||
| val webSocketRepository = mockk<WebSocketRepository>() | ||||||
| coEvery { webSocketRepository.sendPing() } returns false | ||||||
| coEvery { serverManager.webSocketRepository() } returns webSocketRepository | ||||||
|
|
||||||
| createMonitor().observeAvailability().test { | ||||||
| advanceTimeBy(GRACE_PERIOD + 1.seconds) | ||||||
| assertEquals(ConnectionAvailability.Unavailable, awaitItem()) | ||||||
|
|
||||||
| coEvery { webSocketRepository.sendPing() } returns true | ||||||
| advanceTimeBy(DEGRADED_POLL_INTERVAL + 1.seconds) | ||||||
| assertEquals(ConnectionAvailability.Available, awaitItem()) | ||||||
| cancelAndConsumeRemainingEvents() | ||||||
| } | ||||||
| } | ||||||
| } | ||||||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not make this specific to vehicle, also it is a public API so I expect to see documentation on the public part of this class (explaining the behavior not the implementation).