Bug 2011498 - Add an accelerometer API, implementation, and shake detection

[MatthewTighe]

No description provided.

[github-actions]

Warning

The base branch is currently set to main. Please Edit this PR and set the base to autoland.

[github-actions]

View this pull request in Lando to land it once approved.

[MatthewTighe]

Couple notes in here for follow-ups. My main priority is to get this landing to unblock an end-to-end test of an early implementation.

[MatthewTighe]

I will remove this before landing, just wanted an example of the API

[MatthewTighe]

We will follow-up on the sampling delay here - this may be too high of a rate for battery reasons and we can likely do something intelligent in terms of increasing the sample rate as necessary.

[segunfamisa]

Thanks for the PR.

This PR is definitely shaping up nicely, thank you.

I have some comments, I highlighted the ones that about naming that are nitpicky - so, please feel free to not pay too much attention to them.

[segunfamisa]

❓ question:

Do we need to do anything if there's no accelerometer sensor? I think we may need to throw an error that will be handled downstream?

I don't know how we envisage the usage of the sample stream, but we could accidentally have an observer waiting indefinitely if we don't emit anything 🤔

It sounds like we should complete it - either successfully empty or with an error, if we don't have a sensor

[MatthewTighe]

How are you imagining that to look? We don't have a way to complete the flow on the publishing side, so I think this would mean that we'd need to change the AccelerometerEventSource to be a Flow<SensorResult> so that consumers could cancel their collection if we had a SensorResult.Failed.

Is there anything they could do to recover from this case? For our specific use-case, I don't think the resulting behavior is different - we just don't get shake detection.

[segunfamisa]

Yeah. Let's keep this and see if we need to revisit

[segunfamisa]

I want to propose using robolectric shadows here instead of mocking to be able to write the tests properly and without mocks.

Here's a sample test I was able to get working:

@RunWith(AndroidJUnit4::class)
@Config(sdk = [28])
class LifecycleAwareSensorManagerEventFlowTest {

    private val testDispatcher = StandardTestDispatcher()

    // from AndroidX
    private lateinit var lifecycleOwner: TestLifecycleOwner

    private lateinit var sensorManager: SensorManager
    private lateinit var shadowSensorManager: ShadowSensorManager

    private lateinit var eventFlow: LifecycleAwareSensorManagerEventFlow

    @Before
    fun setUp() {
        val context = ApplicationProvider.getApplicationContext<Context>()
        sensorManager = context.getSystemService(Context.SENSOR_SERVICE) as SensorManager
        shadowSensorManager = Shadows.shadowOf(sensorManager)
        shadowSensorManager.addSensor(ShadowSensor.newInstance(Sensor.TYPE_ACCELEROMETER))

        eventFlow = LifecycleAwareSensorManagerEventFlow(
            sensorManager = sensorManager,
            logger = { },
        )

        lifecycleOwner = TestLifecycleOwner(Lifecycle.State.INITIALIZED)
        lifecycleOwner.lifecycle.addObserver(eventFlow)
    }

    @Test
    fun `when lifecycle is resumed, sensor events are emitted`() = runTest(testDispatcher) {
        // given the lifecycle is resumed
        lifecycleOwner.handleLifecycleEvent(Lifecycle.Event.ON_RESUME)

        // and we are observing the event flow
        val samples = mutableListOf<Accelerometer.Sample>()
        val eventsJob = launch(testDispatcher) {
            eventFlow.flowEvents()
                .collect { samples.add(it) }
        }

        // when we emit two sensor events
        emitSensorEvent(floatArrayOf(1f, 2f, 3f, 4f))
        emitSensorEvent(floatArrayOf(1f, 2f, 3f, 4f))

        testDispatcher.scheduler.advanceUntilIdle()

        // then we observe two emissions
        assertEquals("Two sensor events are observed", 2, samples.size)
        eventsJob.cancel()
    }

    @Test
    fun `when lifecycle is paused, no sensor events are emitted`() = runTest(testDispatcher) {
        // given that we are observing the events
        val samples = mutableListOf<Accelerometer.Sample>()
        val eventsJob = launch(testDispatcher) {
            eventFlow.flowEvents()
                .collect { samples.add(it) }
        }

        // and two events were emitted when the life cycle was resumed
        lifecycleOwner.handleLifecycleEvent(Lifecycle.Event.ON_RESUME)
        emitSensorEvent(floatArrayOf(1f, 2f, 3f, 4f))
        emitSensorEvent(floatArrayOf(1f, 2f, 3f, 4f))

        testDispatcher.scheduler.advanceUntilIdle()

        // when the lifecycle is paused
        lifecycleOwner.handleLifecycleEvent(Lifecycle.Event.ON_PAUSE)

        // and we emit another event
        emitSensorEvent(floatArrayOf(1f, 2f, 3f, 4f))

        testDispatcher.scheduler.advanceUntilIdle()

        // then we observe two emissions
        assertEquals("Expected that only the two prior sensor events were emitted", 2, samples.size)
        eventsJob.cancel()
    }

    private fun emitSensorEvent(values: FloatArray = floatArrayOf(1f, 2f, 3f, 4f)) {
        val sensor = sensorManager.getDefaultSensor(Sensor.TYPE_ACCELEROMETER)
        shadowSensorManager.sendSensorEventToListeners(
            SensorEventBuilder.newBuilder()
                .setSensor(requireNotNull(sensor))
                .setValues(values)
                .setTimestamp(1234)
                .build(),
        )
    }

[MatthewTighe]

I am not exactly sure why, but applying these tests for me was racy because the launched collection didn't start in time. I had to runCurrent on the test dispatcher to ensure collection started eagerly

[segunfamisa]

👌

[segunfamisa]

Are we going to have the sampling happen later? Or it has already happened somewhere? 😅

[segunfamisa]

What if we make this a class, and we provided Low, High and Medium values as this:

@JvmInline
value class ShakeSensitivity(val threshold: Float) {
  companion object {
    val Low = ShakeSensitivity(threshold = 3.2f)
    val Medium = ShakeSensitivity(threshold = 2.7f)
    ....
  }
}

The benefit of this idea is that it gives room for consumers of this class to be able to use their own in-between sensitivity thresholds without us having to change our API, and we don't have to manually map the given sensitivity into a threshold in the next() function besides the default ones we've defined.

[MatthewTighe]

Love this idea!

[segunfamisa]

Thanks a lot. Changes look great to me. ✅

[lando-prod-mozilla]

Pull request closed by commit c63d58e