Conversation
There was a problem hiding this comment.
Pull request overview
This PR follows up on #6459 by moving Google Play Services activity/sleep intent processing out of a long-running BroadcastReceiver path and into WorkManager, to reduce background ANR risk while still updating Activity-related sensors.
Changes:
- Introduces
ActivitySensorBroadcastReceiver(lightweight intent parsing) plusActivitySensorWorker/SleepSensorWorker(DB updates viaSensorManager.onSensorUpdated) - Refactors
ActivitySensorManagerto no longer be aBroadcastReceiverand updates pending intents to target the new receiver - Updates manifests to register the new receiver and adds unit tests for the new workers
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| automotive/src/main/AndroidManifest.xml | Switches activity updates receiver class to ActivitySensorBroadcastReceiver |
| app/src/main/AndroidManifest.xml | Switches activity updates receiver class to ActivitySensorBroadcastReceiver |
| app/src/minimal/kotlin/io/homeassistant/companion/android/sensors/ActivitySensorManager.kt | Keeps minimal flavor Activity sensor manager as a no-op SensorManager (no GMS) |
| app/src/full/kotlin/io/homeassistant/companion/android/sensors/ActivitySensorManager.kt | Removes receiver responsibilities; routes pending intents to the new broadcast receiver |
| app/src/full/kotlin/io/homeassistant/companion/android/sensors/ActivitySensorBroadcastReceiver.kt | New receiver that extracts GMS intent data and enqueues workers |
| app/src/full/kotlin/io/homeassistant/companion/android/sensors/worker/ActivitySensorWorker.kt | New worker to update detected activity sensor state/attributes |
| app/src/full/kotlin/io/homeassistant/companion/android/sensors/worker/SleepSensorWorker.kt | New worker to update sleep classify/segment sensor state/attributes |
| app/src/testFull/kotlin/io/homeassistant/companion/android/sensors/worker/ActivitySensorWorkerTest.kt | Adds unit tests for ActivitySensorWorker |
| app/src/testFull/kotlin/io/homeassistant/companion/android/sensors/worker/SleepSensorWorkerTest.kt | Adds unit tests for SleepSensorWorker |
| <receiver | ||
| android:name=".sensors.ActivitySensorManager" | ||
| android:name=".sensors.ActivitySensorBroadcastReceiver" | ||
| android:enabled="true" | ||
| android:exported="false"> |
There was a problem hiding this comment.
ActivitySensorBroadcastReceiver is declared in the main manifest, but the class only exists in the full source set. This means the minimal flavor APK (and any variant that does not include app/src/full) will ship a manifest component whose class is missing, leading to a runtime crash if the receiver is ever instantiated. Consider moving this <receiver> declaration into app/src/full/AndroidManifest.xml (and/or adding a no-op ActivitySensorBroadcastReceiver implementation in app/src/minimal).
| if (result.probableActivities[1].type == DetectedActivity.RUNNING) return "running" | ||
| if (result.probableActivities[1].type == DetectedActivity.WALKING) return "walking" | ||
| return "on_foot" |
There was a problem hiding this comment.
getSubActivity assumes probableActivities has at least 2 entries ([1]). ActivityRecognitionResult.probableActivities is not guaranteed to contain 2+ items, so this can throw IndexOutOfBoundsException and crash the receiver. Use getOrNull(1) (or a size check) and fall back to "on_foot" when the second entry is missing.
| if (result.probableActivities[1].type == DetectedActivity.RUNNING) return "running" | |
| if (result.probableActivities[1].type == DetectedActivity.WALKING) return "walking" | |
| return "on_foot" | |
| val subActivity = result.probableActivities.getOrNull(1) ?: return "on_foot" | |
| return when (subActivity.type) { | |
| DetectedActivity.RUNNING -> "running" | |
| DetectedActivity.WALKING -> "walking" | |
| else -> "on_foot" | |
| } |
|
|
||
| val confidenceKeys = inputData.getStringArray(KEY_CONFIDENCE_KEYS) ?: emptyArray() | ||
| val confidenceValues = inputData.getIntArray(KEY_CONFIDENCE_VALUES) ?: intArrayOf() | ||
| val confidences = confidenceKeys.zip(confidenceValues.toTypedArray()).toMap() |
There was a problem hiding this comment.
The confidence map is reconstructed from two arrays via zip(...). If the arrays ever differ in length (or become out of sync), this will silently drop entries and/or mis-associate keys and values. Consider validating the lengths (and logging if mismatched) and/or persisting the confidences as a single serialized payload (e.g., JSON) to avoid ordering issues.
| val confidences = confidenceKeys.zip(confidenceValues.toTypedArray()).toMap() | |
| val confidences: Map<String, Int> = | |
| if (confidenceKeys.size != confidenceValues.size) { | |
| Timber.w( | |
| "Mismatched activity confidence arrays: keys=${confidenceKeys.size}, values=${confidenceValues.size}", | |
| ) | |
| emptyMap() | |
| } else { | |
| confidenceKeys.indices.associate { index -> | |
| confidenceKeys[index] to confidenceValues[index] | |
| } | |
| } |
| fun enqueue( | ||
| context: Context, | ||
| classifyData: ClassifyData?, | ||
| segmentData: SegmentData?, | ||
| ) { | ||
| val builder = Data.Builder() | ||
|
|
||
| if (classifyData != null) { | ||
| builder.putInt(KEY_CLASSIFY_CONFIDENCE, classifyData.confidence) | ||
| builder.putInt(KEY_CLASSIFY_LIGHT, classifyData.light) | ||
| builder.putInt(KEY_CLASSIFY_MOTION, classifyData.motion) | ||
| builder.putLong(KEY_CLASSIFY_TIMESTAMP, classifyData.timestampMillis) | ||
| } | ||
|
|
||
| if (segmentData != null) { | ||
| builder.putLong(KEY_SEGMENT_DURATION, segmentData.durationMillis) | ||
| builder.putLong(KEY_SEGMENT_START, segmentData.startMillis) | ||
| builder.putLong(KEY_SEGMENT_END, segmentData.endMillis) | ||
| builder.putString(KEY_SEGMENT_STATUS, segmentData.status) | ||
| } | ||
|
|
||
| val request = OneTimeWorkRequestBuilder<SleepSensorWorker>() | ||
| .setInputData(builder.build()) | ||
| .build() | ||
|
|
||
| WorkManager.getInstance(context).enqueue(request) | ||
| } |
There was a problem hiding this comment.
enqueue() uses WorkManager.enqueue() with no uniqueness policy. Sleep classify events can arrive in bursts, so this can enqueue many redundant workers that all do DB work. Consider enqueueUniqueWork (e.g., keep only the latest sleep update) and using context.applicationContext when obtaining the WorkManager instance.
| assertEquals(ListenableWorker.Result.success(), result) | ||
| coVerify(exactly = 1) { sensorDao.update(match { it.state == "85" }) } | ||
| coVerify(exactly = 1) { SensorReceiver.updateAllSensors(any()) } | ||
| } |
There was a problem hiding this comment.
SensorReceiver.updateAllSensors is a non-suspending function, but the test verifies it with coVerify. This can cause false negatives or runtime errors in MockK; use verify for non-suspend functions.
| mockkStatic(EntryPointAccessors::class) | ||
| val entryPoint = mockk<SensorManager.SensorManagerEntryPoint>() | ||
| every { | ||
| EntryPointAccessors.fromApplication(context, SensorManager.SensorManagerEntryPoint::class.java) | ||
| } returns entryPoint | ||
| every { entryPoint.sensorDao() } returns sensorDao | ||
| every { entryPoint.serverManager() } returns serverManager | ||
|
|
||
| coEvery { serverManager.servers() } returns listOf(server) | ||
|
|
||
| mockkObject(SensorReceiver) | ||
| justRun { SensorReceiver.updateAllSensors(any()) } | ||
| } |
There was a problem hiding this comment.
This test uses mockkStatic(EntryPointAccessors::class) / mockkObject(SensorReceiver) but never un-mocks them. Because these mocks are global, they can leak into other tests in the same JVM and cause unexpected failures. Add an @AfterEach that calls unmockkStatic(EntryPointAccessors::class) and unmockkObject(SensorReceiver) (or unmockkAll() if appropriate).
| <receiver | ||
| android:name=".sensors.ActivitySensorManager" | ||
| android:name=".sensors.ActivitySensorBroadcastReceiver" | ||
| android:enabled="true" | ||
| android:exported="false"> | ||
| <intent-filter> |
There was a problem hiding this comment.
ActivitySensorBroadcastReceiver is referenced from the automotive main manifest, but the class is only present in the full source set. Since the automotive module defines both full and minimal source sets, the minimal variant will have a manifest receiver whose class is missing, which can crash if the receiver is instantiated. Move this receiver declaration to the automotive full manifest (or provide a minimal no-op receiver implementation).
| * | ||
| * Extracts data synchronously from the intent and enqueues an | ||
| * [ActivitySensorWorker] or [SleepSensorWorker] for the actual sensor | ||
| * update work, to keep the BroadcastReceiver lifecycle bellow 10s. |
There was a problem hiding this comment.
Typo in KDoc: "bellow 10s" should be "below 10s".
| * update work, to keep the BroadcastReceiver lifecycle bellow 10s. | |
| * update work, to keep the BroadcastReceiver lifecycle below 10s. |
| fun enqueue( | ||
| context: Context, | ||
| activity: String, | ||
| confidences: Map<String, Int>, | ||
| ) { | ||
| val data = Data.Builder() | ||
| .putString(KEY_ACTIVITY, activity) | ||
| .putStringArray(KEY_CONFIDENCE_KEYS, confidences.keys.toTypedArray()) | ||
| .putIntArray(KEY_CONFIDENCE_VALUES, confidences.values.toIntArray()) | ||
| .build() | ||
|
|
||
| val request = OneTimeWorkRequestBuilder<ActivitySensorWorker>() | ||
| .setInputData(data) | ||
| .build() | ||
|
|
||
| WorkManager.getInstance(context).enqueue(request) | ||
| } |
There was a problem hiding this comment.
enqueue() uses WorkManager.enqueue() with no uniqueness policy. Activity recognition can be very chatty, so this risks building an unbounded backlog of one-time work, increasing battery/CPU and delaying the latest state. Consider switching to enqueueUniqueWork with a stable name and ExistingWorkPolicy.REPLACE (or another policy that keeps only the latest update), and prefer WorkManager.getInstance(context.applicationContext) to avoid holding onto a receiver context.
| mockkStatic(EntryPointAccessors::class) | ||
| val entryPoint = mockk<SensorManager.SensorManagerEntryPoint>() | ||
| every { | ||
| EntryPointAccessors.fromApplication(context, SensorManager.SensorManagerEntryPoint::class.java) | ||
| } returns entryPoint | ||
| every { entryPoint.sensorDao() } returns sensorDao | ||
| every { entryPoint.serverManager() } returns serverManager | ||
|
|
||
| coEvery { serverManager.servers() } returns listOf(server) | ||
| } |
There was a problem hiding this comment.
This test uses mockkStatic(EntryPointAccessors::class) but never un-mocks it. Static mocks are global and can leak into other tests in the same JVM. Add an @AfterEach that calls unmockkStatic(EntryPointAccessors::class) (or unmockkAll() if appropriate).
| @@ -0,0 +1,125 @@ | |||
| package io.homeassistant.companion.android.sensors | |||
There was a problem hiding this comment.
If it's necessary to separate the receiver from the SensorManager, it might be best to move it to a more specific package like you've done for workers. Keep the main package for sensors.
There was a problem hiding this comment.
It is not necessary but I prefer to have a clear separation between the two, to keep a clear separation of responsibilities.
Summary
Follow up after #6459 to try to improve the situation toward our background ANRs, this PR delegate the update of the ActivitySensors to a worker.
Checklist
Any other notes
Mostly made with Claude (especially the tests)