From 39b2dab19d6d8cddadc24824b95b1abd2960ac8b Mon Sep 17 00:00:00 2001 From: Lais Andrade Date: Thu, 7 Oct 2021 15:08:44 +0100 Subject: [PATCH] Change VibratorInfo reload logic in VibratorManagerService The VibratorController.getInfo method is reloading the VibratorInfo on every call when at least one of the read calls to the vibrator HAL has failed. This can force a device to enter a request loop when the HAL has a broken getter implementation, and also has no control on how frequently it requests HAL data. This update will reload the VibratorInfo only once from VibratorManagerService.systemReady, to give the controller a chance to fetch the HAL data before the service becomes available to the system. After this point that info (including defaults for failed fields) will be considered the device vibrator profile and can be cached by client calls and apps. This is also adding more logs on failing to load primitive durations, to help finding the root cause for the failed cts. Bug: 195595741 Test: VibratorControllerTest and VibratorManagerServiceTest Change-Id: Idd9457300c49d7e240f4d9a6326fdbe995e7089b --- core/java/android/os/SystemVibrator.java | 11 ++- core/java/android/os/Vibrator.java | 6 +- core/java/android/os/VibratorInfo.java | 3 +- .../server/vibrator/VibratorController.java | 81 +++++++++++++------ .../vibrator/VibratorManagerService.java | 28 ++++++- ...oid_server_vibrator_VibratorController.cpp | 2 +- .../FakeVibratorControllerProvider.java | 11 ++- .../vibrator/VibratorManagerServiceTest.java | 41 +++++++++- 8 files changed, 149 insertions(+), 34 deletions(-) diff --git a/core/java/android/os/SystemVibrator.java b/core/java/android/os/SystemVibrator.java index 0c3debb1b54ec..a243453a02d1a 100644 --- a/core/java/android/os/SystemVibrator.java +++ b/core/java/android/os/SystemVibrator.java @@ -51,6 +51,7 @@ public class SystemVibrator extends Vibrator { mRegisteredListeners = new ArrayMap<>(); private final Object mLock = new Object(); + @GuardedBy("mLock") private AllVibratorsInfo mVibratorInfo; @UnsupportedAppUsage @@ -73,7 +74,15 @@ public class SystemVibrator extends Vibrator { int[] vibratorIds = mVibratorManager.getVibratorIds(); VibratorInfo[] vibratorInfos = new VibratorInfo[vibratorIds.length]; for (int i = 0; i < vibratorIds.length; i++) { - vibratorInfos[i] = mVibratorManager.getVibrator(vibratorIds[i]).getInfo(); + Vibrator vibrator = mVibratorManager.getVibrator(vibratorIds[i]); + if (vibrator instanceof NullVibrator) { + Log.w(TAG, "Vibrator manager service not ready; " + + "Info not yet available for vibrator: " + vibratorIds[i]); + // This should never happen after the vibrator manager service is ready. + // Skip caching this vibrator until then. + return VibratorInfo.EMPTY_VIBRATOR_INFO; + } + vibratorInfos[i] = vibrator.getInfo(); } return mVibratorInfo = new AllVibratorsInfo(vibratorInfos); } diff --git a/core/java/android/os/Vibrator.java b/core/java/android/os/Vibrator.java index feffcbd960976..aa9028e97faf5 100644 --- a/core/java/android/os/Vibrator.java +++ b/core/java/android/os/Vibrator.java @@ -165,7 +165,11 @@ public abstract class Vibrator { return ctx != null ? ctx.getResources().getFloat(resId) : defaultValue; } - /** @hide */ + /** + * Get the info describing this vibrator. + * + * @hide + */ protected VibratorInfo getInfo() { return VibratorInfo.EMPTY_VIBRATOR_INFO; } diff --git a/core/java/android/os/VibratorInfo.java b/core/java/android/os/VibratorInfo.java index 486f9f139e0a7..cf51496ac0118 100644 --- a/core/java/android/os/VibratorInfo.java +++ b/core/java/android/os/VibratorInfo.java @@ -461,7 +461,8 @@ public class VibratorInfo implements Parcelable { int supportedPrimitivesCount = mSupportedPrimitives.size(); String[] names = new String[supportedPrimitivesCount]; for (int i = 0; i < supportedPrimitivesCount; i++) { - names[i] = VibrationEffect.Composition.primitiveToString(mSupportedPrimitives.keyAt(i)); + names[i] = VibrationEffect.Composition.primitiveToString(mSupportedPrimitives.keyAt(i)) + + "(" + mSupportedPrimitives.valueAt(i) + "ms)"; } return names; } diff --git a/services/core/java/com/android/server/vibrator/VibratorController.java b/services/core/java/com/android/server/vibrator/VibratorController.java index efccd57d2fc3e..4b7fd904e32d4 100644 --- a/services/core/java/com/android/server/vibrator/VibratorController.java +++ b/services/core/java/com/android/server/vibrator/VibratorController.java @@ -41,12 +41,11 @@ final class VibratorController { private final Object mLock = new Object(); private final NativeWrapper mNativeWrapper; - private final VibratorInfo.Builder mVibratorInfoBuilder; @GuardedBy("mLock") private VibratorInfo mVibratorInfo; @GuardedBy("mLock") - private boolean mVibratorInfoLoaded; + private boolean mVibratorInfoLoadSuccessful; @GuardedBy("mLock") private final RemoteCallbackList mVibratorStateListeners = new RemoteCallbackList<>(); @@ -73,10 +72,16 @@ final class VibratorController { NativeWrapper nativeWrapper) { mNativeWrapper = nativeWrapper; mNativeWrapper.init(vibratorId, listener); - mVibratorInfoBuilder = new VibratorInfo.Builder(vibratorId); - mVibratorInfoLoaded = mNativeWrapper.getInfo(SUGGESTED_FREQUENCY_SAFE_RANGE, - mVibratorInfoBuilder); - mVibratorInfo = mVibratorInfoBuilder.build(); + VibratorInfo.Builder vibratorInfoBuilder = new VibratorInfo.Builder(vibratorId); + mVibratorInfoLoadSuccessful = mNativeWrapper.getInfo(SUGGESTED_FREQUENCY_SAFE_RANGE, + vibratorInfoBuilder); + mVibratorInfo = vibratorInfoBuilder.build(); + + if (!mVibratorInfoLoadSuccessful) { + Slog.e(TAG, + "Vibrator controller initialization failed to load some HAL info for vibrator " + + vibratorId); + } } /** Register state listener for this vibrator. */ @@ -108,15 +113,33 @@ final class VibratorController { } } + /** Reruns the query to the vibrator to load the {@link VibratorInfo}, if not yet successful. */ + public void reloadVibratorInfoIfNeeded() { + synchronized (mLock) { + if (mVibratorInfoLoadSuccessful) { + return; + } + int vibratorId = mVibratorInfo.getId(); + VibratorInfo.Builder vibratorInfoBuilder = new VibratorInfo.Builder(vibratorId); + mVibratorInfoLoadSuccessful = mNativeWrapper.getInfo(SUGGESTED_FREQUENCY_SAFE_RANGE, + vibratorInfoBuilder); + mVibratorInfo = vibratorInfoBuilder.build(); + if (!mVibratorInfoLoadSuccessful) { + Slog.e(TAG, "Failed retry of HAL getInfo for vibrator " + vibratorId); + } + } + } + + /** Checks if the {@link VibratorInfo} was loaded from the vibrator hardware successfully. */ + boolean isVibratorInfoLoadSuccessful() { + synchronized (mLock) { + return mVibratorInfoLoadSuccessful; + } + } + /** Return the {@link VibratorInfo} representing the vibrator controlled by this instance. */ public VibratorInfo getVibratorInfo() { synchronized (mLock) { - if (!mVibratorInfoLoaded) { - // Try to load the vibrator metadata that has failed in the last attempt. - mVibratorInfoLoaded = mNativeWrapper.getInfo(SUGGESTED_FREQUENCY_SAFE_RANGE, - mVibratorInfoBuilder); - mVibratorInfo = mVibratorInfoBuilder.build(); - } return mVibratorInfo; } } @@ -164,7 +187,9 @@ final class VibratorController { * @return true if this vibrator has this capability, false otherwise */ public boolean hasCapability(long capability) { - return mVibratorInfo.hasCapability(capability); + synchronized (mLock) { + return mVibratorInfo.hasCapability(capability); + } } /** Return {@code true} if the underlying vibrator is currently available, false otherwise. */ @@ -178,10 +203,10 @@ final class VibratorController { *

This will affect the state of {@link #isUnderExternalControl()}. */ public void setExternalControl(boolean externalControl) { - if (!mVibratorInfo.hasCapability(IVibrator.CAP_EXTERNAL_CONTROL)) { - return; - } synchronized (mLock) { + if (!mVibratorInfo.hasCapability(IVibrator.CAP_EXTERNAL_CONTROL)) { + return; + } mIsUnderExternalControl = externalControl; mNativeWrapper.setExternalControl(externalControl); } @@ -192,10 +217,10 @@ final class VibratorController { * if given {@code effect} is {@code null}. */ public void updateAlwaysOn(int id, @Nullable PrebakedSegment prebaked) { - if (!mVibratorInfo.hasCapability(IVibrator.CAP_ALWAYS_ON_CONTROL)) { - return; - } synchronized (mLock) { + if (!mVibratorInfo.hasCapability(IVibrator.CAP_ALWAYS_ON_CONTROL)) { + return; + } if (prebaked == null) { mNativeWrapper.alwaysOnDisable(id); } else { @@ -268,10 +293,10 @@ final class VibratorController { * do not support the input or a negative number if the operation failed. */ public long on(PrimitiveSegment[] primitives, long vibrationId) { - if (!mVibratorInfo.hasCapability(IVibrator.CAP_COMPOSE_EFFECTS)) { - return 0; - } synchronized (mLock) { + if (!mVibratorInfo.hasCapability(IVibrator.CAP_COMPOSE_EFFECTS)) { + return 0; + } long duration = mNativeWrapper.compose(primitives, vibrationId); if (duration > 0) { mCurrentAmplitude = -1; @@ -290,10 +315,10 @@ final class VibratorController { * @return The duration of the effect playing, or 0 if unsupported. */ public long on(RampSegment[] primitives, long vibrationId) { - if (!mVibratorInfo.hasCapability(IVibrator.CAP_COMPOSE_PWLE_EFFECTS)) { - return 0; - } synchronized (mLock) { + if (!mVibratorInfo.hasCapability(IVibrator.CAP_COMPOSE_PWLE_EFFECTS)) { + return 0; + } int braking = mVibratorInfo.getDefaultBraking(); long duration = mNativeWrapper.composePwle(primitives, braking, vibrationId); if (duration > 0) { @@ -327,6 +352,7 @@ final class VibratorController { synchronized (mLock) { return "VibratorController{" + "mVibratorInfo=" + mVibratorInfo + + ", mVibratorInfoLoadSuccessful=" + mVibratorInfoLoadSuccessful + ", mIsVibrating=" + mIsVibrating + ", mCurrentAmplitude=" + mCurrentAmplitude + ", mIsUnderExternalControl=" + mIsUnderExternalControl @@ -393,10 +419,15 @@ final class VibratorController { * allocated and returned by {@link #nativeInit(int, OnVibrationCompleteListener)}. */ private static native long getNativeFinalizer(); + private static native boolean isAvailable(long nativePtr); + private static native long on(long nativePtr, long milliseconds, long vibrationId); + private static native void off(long nativePtr); + private static native void setAmplitude(long nativePtr, float amplitude); + private static native long performEffect(long nativePtr, long effect, long strength, long vibrationId); diff --git a/services/core/java/com/android/server/vibrator/VibratorManagerService.java b/services/core/java/com/android/server/vibrator/VibratorManagerService.java index 239a112f31447..3a3ce5b894a1a 100644 --- a/services/core/java/com/android/server/vibrator/VibratorManagerService.java +++ b/services/core/java/com/android/server/vibrator/VibratorManagerService.java @@ -128,6 +128,8 @@ public class VibratorManagerService extends IVibratorManagerService.Stub { private VibrationThread mNextVibration; @GuardedBy("mLock") private ExternalVibrationHolder mCurrentExternalVibration; + @GuardedBy("mLock") + private boolean mServiceReady; private final VibrationSettings mVibrationSettings; private final VibrationScaler mVibrationScaler; @@ -201,6 +203,9 @@ public class VibratorManagerService extends IVibratorManagerService.Stub { mWakeLock = pm.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, "*vibrator*"); mWakeLock.setReferenceCounted(true); + // Load vibrator hardware info. The vibrator ids and manager capabilities are loaded only + // once and assumed unchanged for the lifecycle of this service. Each individual vibrator + // can still retry loading each individual vibrator hardware spec once more at systemReady. mCapabilities = mNativeWrapper.getCapabilities(); int[] vibratorIds = mNativeWrapper.getVibratorIds(); if (vibratorIds == null) { @@ -235,6 +240,11 @@ public class VibratorManagerService extends IVibratorManagerService.Stub { Slog.v(TAG, "Initializing VibratorManager service..."); Trace.traceBegin(Trace.TRACE_TAG_VIBRATOR, "systemReady"); try { + // Will retry to load each vibrator's info, if any request have failed. + for (int i = 0; i < mVibrators.size(); i++) { + mVibrators.valueAt(i).reloadVibratorInfoIfNeeded(); + } + mVibrationSettings.onSystemReady(); mInputDeviceDelegate.onSystemReady(); @@ -243,6 +253,9 @@ public class VibratorManagerService extends IVibratorManagerService.Stub { // Will update settings and input devices. updateServiceState(); } finally { + synchronized (mLock) { + mServiceReady = true; + } Slog.v(TAG, "VibratorManager service initialized"); Trace.traceEnd(Trace.TRACE_TAG_VIBRATOR); } @@ -256,8 +269,19 @@ public class VibratorManagerService extends IVibratorManagerService.Stub { @Override // Binder call @Nullable public VibratorInfo getVibratorInfo(int vibratorId) { - VibratorController controller = mVibrators.get(vibratorId); - return controller == null ? null : controller.getVibratorInfo(); + final VibratorController controller = mVibrators.get(vibratorId); + if (controller == null) { + return null; + } + final VibratorInfo info = controller.getVibratorInfo(); + synchronized (mLock) { + if (mServiceReady) { + return info; + } + } + // If the service is not ready and the load was unsuccessful then return null while waiting + // for the service to be ready. It will retry to load the complete info from the HAL. + return controller.isVibratorInfoLoadSuccessful() ? info : null; } @Override // Binder call diff --git a/services/core/jni/com_android_server_vibrator_VibratorController.cpp b/services/core/jni/com_android_server_vibrator_VibratorController.cpp index 9029fe7cca667..546b075ea0be1 100644 --- a/services/core/jni/com_android_server_vibrator_VibratorController.cpp +++ b/services/core/jni/com_android_server_vibrator_VibratorController.cpp @@ -443,7 +443,7 @@ static jboolean vibratorGetInfo(JNIEnv* env, jclass /* clazz */, jlong ptr, env->CallObjectMethod(vibratorInfoBuilder, sVibratorInfoBuilderClassInfo.setFrequencyMapping, frequencyMapping); - return info.checkAndLogFailure("vibratorGetInfo") ? JNI_FALSE : JNI_TRUE; + return info.isFailedLogged("vibratorGetInfo") ? JNI_FALSE : JNI_TRUE; } static const JNINativeMethod method_table[] = { 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 378304d024e53..777e3f4e4a010 100644 --- a/services/tests/servicestests/src/com/android/server/vibrator/FakeVibratorControllerProvider.java +++ b/services/tests/servicestests/src/com/android/server/vibrator/FakeVibratorControllerProvider.java @@ -51,6 +51,7 @@ final class FakeVibratorControllerProvider { private final FakeNativeWrapper mNativeWrapper; private boolean mIsAvailable = true; + private boolean mIsInfoLoadSuccessful = true; private long mLatency; private int mOffCount; @@ -172,7 +173,7 @@ final class FakeVibratorControllerProvider { infoBuilder.setFrequencyMapping(new VibratorInfo.FrequencyMapping(mMinFrequency, mResonantFrequency, mFrequencyResolution, suggestedFrequencyRange, mMaxAmplitudes)); - return true; + return mIsInfoLoadSuccessful; } private void applyLatency() { @@ -212,6 +213,14 @@ final class FakeVibratorControllerProvider { mIsAvailable = false; } + /** + * Sets the result for the method that loads the {@link VibratorInfo}, for faking a vibrator + * that fails to load some of the hardware data. + */ + public void setVibratorInfoLoadSuccessful(boolean successful) { + mIsInfoLoadSuccessful = successful; + } + /** * Sets the latency this controller should fake for turning the vibrator hardware on or setting * it's vibration amplitude. 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 5e25bc59c6a5b..6118169f1d9b8 100644 --- a/services/tests/servicestests/src/com/android/server/vibrator/VibratorManagerServiceTest.java +++ b/services/tests/servicestests/src/com/android/server/vibrator/VibratorManagerServiceTest.java @@ -287,7 +287,26 @@ public class VibratorManagerServiceTest { } @Test - public void getVibratorInfo_withExistingVibratorId_returnsHalInfoForVibrator() { + public void getVibratorInfo_vibratorFailedLoadBeforeSystemReady_returnsNull() { + mockVibrators(1); + mVibratorProviders.get(1).setVibratorInfoLoadSuccessful(false); + assertNull(createService().getVibratorInfo(1)); + } + + @Test + public void getVibratorInfo_vibratorFailedLoadAfterSystemReady_returnsInfoForVibrator() { + mockVibrators(1); + mVibratorProviders.get(1).setVibratorInfoLoadSuccessful(false); + mVibratorProviders.get(1).setResonantFrequency(123.f); + VibratorInfo info = createSystemReadyService().getVibratorInfo(1); + + assertNotNull(info); + assertEquals(1, info.getId()); + assertEquals(123.f, info.getResonantFrequency(), 0.01 /*tolerance*/); + } + + @Test + public void getVibratorInfo_vibratorSuccessfulLoadBeforeSystemReady_returnsInfoForVibrator() { mockVibrators(1); FakeVibratorControllerProvider vibrator = mVibratorProviders.get(1); vibrator.setCapabilities(IVibrator.CAP_COMPOSE_EFFECTS, IVibrator.CAP_AMPLITUDE_CONTROL); @@ -295,7 +314,7 @@ public class VibratorManagerServiceTest { vibrator.setSupportedPrimitives(VibrationEffect.Composition.PRIMITIVE_CLICK); vibrator.setResonantFrequency(123.f); vibrator.setQFactor(Float.NaN); - VibratorInfo info = createSystemReadyService().getVibratorInfo(1); + VibratorInfo info = createService().getVibratorInfo(1); assertNotNull(info); assertEquals(1, info.getId()); @@ -312,6 +331,24 @@ public class VibratorManagerServiceTest { assertTrue(Float.isNaN(info.getQFactor())); } + @Test + public void getVibratorInfo_vibratorFailedThenSuccessfulLoad_returnsNullThenInfo() { + mockVibrators(1); + mVibratorProviders.get(1).setVibratorInfoLoadSuccessful(false); + + VibratorManagerService service = createService(); + assertNull(createService().getVibratorInfo(1)); + + mVibratorProviders.get(1).setVibratorInfoLoadSuccessful(true); + mVibratorProviders.get(1).setResonantFrequency(123.f); + service.systemReady(); + + VibratorInfo info = createService().getVibratorInfo(1); + assertNotNull(info); + assertEquals(1, info.getId()); + assertEquals(123.f, info.getResonantFrequency(), 0.01 /*tolerance*/); + } + @Test public void registerVibratorStateListener_callbacksAreTriggered() throws Exception { mockVibrators(1);