From b13dcde78b52f591e9dd02ff3774915288365586 Mon Sep 17 00:00:00 2001 From: Lais Andrade Date: Tue, 18 Oct 2022 16:49:26 +0100 Subject: [PATCH] Allow upcoming vibration when current one if being cancelled The vibration cancellation at VibratorManagerService is asynchronous and might take some time before it sets the current Vibration status to the cancelled one requested. If a new binder call to vibrate arrived before the status is updated then the new vibration might be ignored for the current one, even though it's already being cancelled. Add a check to the cancel signal sent to the VibrationStepConductor before deciding to ignore new vibrations in favor of the ongoing. Bug: 249032223 Test: VibratorManagerServiceTest Change-Id: I8151d9ee0e30b7115e9cb3d6bcd272da2806b45c Merged-In: I8151d9ee0e30b7115e9cb3d6bcd272da2806b45c (cherry picked from commit 546dd659054de251ff64f4b3b4c9c72f0fe514d4) --- .../vibrator/VibrationStepConductor.java | 13 +++++++ .../vibrator/VibratorManagerService.java | 4 +- .../FakeVibratorControllerProvider.java | 31 +++++++++------ .../server/vibrator/VibrationThreadTest.java | 7 ++-- .../vibrator/VibratorManagerServiceTest.java | 38 +++++++++++++++++++ 5 files changed, 75 insertions(+), 18 deletions(-) diff --git a/services/core/java/com/android/server/vibrator/VibrationStepConductor.java b/services/core/java/com/android/server/vibrator/VibrationStepConductor.java index 0799b955b6f14..9c455dbb53207 100644 --- a/services/core/java/com/android/server/vibrator/VibrationStepConductor.java +++ b/services/core/java/com/android/server/vibrator/VibrationStepConductor.java @@ -65,6 +65,9 @@ final class VibrationStepConductor implements IBinder.DeathRecipient { public final DeviceVibrationEffectAdapter deviceEffectAdapter; public final VibrationThread.VibratorManagerHooks vibratorManagerHooks; + // Not guarded by lock because they're not modified by this conductor, it's used here only to + // check immutable attributes. The status and other mutable states are changed by the service or + // by the vibrator steps. private final Vibration mVibration; private final SparseArray mVibrators = new SparseArray<>(); @@ -405,6 +408,16 @@ final class VibrationStepConductor implements IBinder.DeathRecipient { } } + /** Returns true if a cancellation signal was sent via {@link #notifyCancelled}. */ + public boolean wasNotifiedToCancel() { + if (Build.IS_DEBUGGABLE) { + expectIsVibrationThread(false); + } + synchronized (mLock) { + return mSignalCancel != null; + } + } + @GuardedBy("mLock") private boolean hasPendingNotifySignalLocked() { if (Build.IS_DEBUGGABLE) { diff --git a/services/core/java/com/android/server/vibrator/VibratorManagerService.java b/services/core/java/com/android/server/vibrator/VibratorManagerService.java index 97275583ac367..14693599c3ece 100644 --- a/services/core/java/com/android/server/vibrator/VibratorManagerService.java +++ b/services/core/java/com/android/server/vibrator/VibratorManagerService.java @@ -852,8 +852,8 @@ public class VibratorManagerService extends IVibratorManagerService.Stub { } Vibration currentVibration = mCurrentVibration.getVibration(); - if (currentVibration.hasEnded()) { - // Current vibration is finishing up, it should not block incoming vibrations. + if (currentVibration.hasEnded() || mCurrentVibration.wasNotifiedToCancel()) { + // Current vibration has ended or is cancelling, should not block incoming vibrations. return null; } diff --git a/services/tests/servicestests/src/com/android/server/vibrator/FakeVibratorControllerProvider.java b/services/tests/servicestests/src/com/android/server/vibrator/FakeVibratorControllerProvider.java index 235849c1cd8b5..c484f457faea6 100644 --- a/services/tests/servicestests/src/com/android/server/vibrator/FakeVibratorControllerProvider.java +++ b/services/tests/servicestests/src/com/android/server/vibrator/FakeVibratorControllerProvider.java @@ -53,7 +53,8 @@ final class FakeVibratorControllerProvider { private boolean mIsAvailable = true; private boolean mIsInfoLoadSuccessful = true; - private long mLatency; + private long mOnLatency; + private long mOffLatency; private int mOffCount; private int mCapabilities; @@ -97,7 +98,7 @@ final class FakeVibratorControllerProvider { public long on(long milliseconds, long vibrationId) { recordEffectSegment(vibrationId, new StepSegment(VibrationEffect.DEFAULT_AMPLITUDE, /* frequencyHz= */ 0, (int) milliseconds)); - applyLatency(); + applyLatency(mOnLatency); scheduleListener(milliseconds, vibrationId); return milliseconds; } @@ -105,12 +106,13 @@ final class FakeVibratorControllerProvider { @Override public void off() { mOffCount++; + applyLatency(mOffLatency); } @Override public void setAmplitude(float amplitude) { mAmplitudes.add(amplitude); - applyLatency(); + applyLatency(mOnLatency); } @Override @@ -121,7 +123,7 @@ final class FakeVibratorControllerProvider { } recordEffectSegment(vibrationId, new PrebakedSegment((int) effect, false, (int) strength)); - applyLatency(); + applyLatency(mOnLatency); scheduleListener(EFFECT_DURATION, vibrationId); return EFFECT_DURATION; } @@ -141,7 +143,7 @@ final class FakeVibratorControllerProvider { duration += EFFECT_DURATION + primitive.getDelay(); recordEffectSegment(vibrationId, primitive); } - applyLatency(); + applyLatency(mOnLatency); scheduleListener(duration, vibrationId); return duration; } @@ -154,7 +156,7 @@ final class FakeVibratorControllerProvider { recordEffectSegment(vibrationId, primitive); } recordBraking(vibrationId, braking); - applyLatency(); + applyLatency(mOnLatency); scheduleListener(duration, vibrationId); return duration; } @@ -193,10 +195,10 @@ final class FakeVibratorControllerProvider { return mIsInfoLoadSuccessful; } - private void applyLatency() { + private void applyLatency(long latencyMillis) { try { - if (mLatency > 0) { - Thread.sleep(mLatency); + if (latencyMillis > 0) { + Thread.sleep(latencyMillis); } } catch (InterruptedException e) { } @@ -240,10 +242,15 @@ final class FakeVibratorControllerProvider { /** * Sets the latency this controller should fake for turning the vibrator hardware on or setting - * it's vibration amplitude. + * the vibration amplitude. */ - public void setLatency(long millis) { - mLatency = millis; + public void setOnLatency(long millis) { + mOnLatency = millis; + } + + /** Sets the latency this controller should fake for turning the vibrator off. */ + public void setOffLatency(long millis) { + mOffLatency = millis; } /** Set the capabilities of the fake vibrator hardware. */ diff --git a/services/tests/servicestests/src/com/android/server/vibrator/VibrationThreadTest.java b/services/tests/servicestests/src/com/android/server/vibrator/VibrationThreadTest.java index 0551bfc70bdad..42a2c10a24d6c 100644 --- a/services/tests/servicestests/src/com/android/server/vibrator/VibrationThreadTest.java +++ b/services/tests/servicestests/src/com/android/server/vibrator/VibrationThreadTest.java @@ -1118,7 +1118,7 @@ public class VibrationThreadTest { // 25% of the first waveform step will be spent on the native on() call. // 25% of each waveform step will be spent on the native setAmplitude() call.. - mVibratorProviders.get(VIBRATOR_ID).setLatency(stepDuration / 4); + mVibratorProviders.get(VIBRATOR_ID).setOnLatency(stepDuration / 4); mVibratorProviders.get(VIBRATOR_ID).setCapabilities(IVibrator.CAP_AMPLITUDE_CONTROL); int stepCount = totalDuration / stepDuration; @@ -1149,7 +1149,7 @@ public class VibrationThreadTest { fakeVibrator.setSupportedEffects(VibrationEffect.EFFECT_CLICK); long latency = 5_000; // 5s - fakeVibrator.setLatency(latency); + fakeVibrator.setOnLatency(latency); long vibrationId = 1; VibrationEffect effect = VibrationEffect.get(VibrationEffect.EFFECT_CLICK); @@ -1163,8 +1163,7 @@ public class VibrationThreadTest { // fail at waitForCompletion(cancellingThread). Thread cancellingThread = new Thread( () -> conductor.notifyCancelled( - new Vibration.EndInfo( - Vibration.Status.CANCELLED_BY_USER), + new Vibration.EndInfo(Vibration.Status.CANCELLED_BY_USER), /* immediate= */ false)); cancellingThread.start(); diff --git a/services/tests/servicestests/src/com/android/server/vibrator/VibratorManagerServiceTest.java b/services/tests/servicestests/src/com/android/server/vibrator/VibratorManagerServiceTest.java index fe0a79c2d944f..039e159a347bb 100644 --- a/services/tests/servicestests/src/com/android/server/vibrator/VibratorManagerServiceTest.java +++ b/services/tests/servicestests/src/com/android/server/vibrator/VibratorManagerServiceTest.java @@ -817,6 +817,39 @@ public class VibratorManagerServiceTest { verify(mBatteryStatsMock).noteVibratorOn(UID, 5000); // The second vibration shouldn't have recorded that the vibrators were turned on. verify(mBatteryStatsMock, times(1)).noteVibratorOn(anyInt(), anyLong()); + // No segment played is the prebaked CLICK from the second vibration. + assertFalse(mVibratorProviders.get(1).getAllEffectSegments().stream() + .anyMatch(PrebakedSegment.class::isInstance)); + // Clean up repeating effect. + service.cancelVibrate(VibrationAttributes.USAGE_FILTER_MATCH_ALL, service); + } + + @Test + public void vibrate_withOngoingRepeatingVibrationBeingCancelled_playsAfterPreviousIsCancelled() + throws Exception { + mockVibrators(1); + FakeVibratorControllerProvider fakeVibrator = mVibratorProviders.get(1); + fakeVibrator.setOffLatency(50); // Add latency so cancellation is slow. + fakeVibrator.setCapabilities(IVibrator.CAP_AMPLITUDE_CONTROL); + fakeVibrator.setSupportedEffects(VibrationEffect.EFFECT_CLICK); + VibratorManagerService service = createSystemReadyService(); + + VibrationEffect repeatingEffect = VibrationEffect.createWaveform( + new long[]{10, 10_000}, new int[]{255, 0}, 1); + vibrate(service, repeatingEffect, ALARM_ATTRS); + + // VibrationThread will start this vibration async, wait until the off waveform step. + assertTrue(waitUntil(s -> fakeVibrator.getOffCount() > 0, service, TEST_TIMEOUT_MILLIS)); + + // Cancel vibration right before requesting a new one. + // This should trigger slow IVibrator.off before setting the vibration status to cancelled. + service.cancelVibrate(VibrationAttributes.USAGE_FILTER_MATCH_ALL, service); + vibrateAndWaitUntilFinished(service, VibrationEffect.get(VibrationEffect.EFFECT_CLICK), + ALARM_ATTRS); + + // Check that second vibration was played. + assertTrue(fakeVibrator.getAllEffectSegments().stream() + .anyMatch(PrebakedSegment.class::isInstance)); } @Test @@ -867,6 +900,11 @@ public class VibratorManagerServiceTest { // The second vibration shouldn't have recorded that the vibrators were turned on. verify(mBatteryStatsMock, times(1)).noteVibratorOn(anyInt(), anyLong()); + // The second vibration shouldn't have played any prebaked segment. + assertFalse(mVibratorProviders.get(1).getAllEffectSegments().stream() + .anyMatch(PrebakedSegment.class::isInstance)); + // Clean up long effect. + service.cancelVibrate(VibrationAttributes.USAGE_FILTER_MATCH_ALL, service); } @Test