From b9e44e5007c57e6f0b148385a21a800a51a7f8e5 Mon Sep 17 00:00:00 2001 From: Robin Lee Date: Fri, 7 Feb 2020 04:18:20 +0100 Subject: Make LogicalLight @Nullable: part one (external) This helps to simplify the internal logic of LightsService to not have to worry about sending bad values down to the HAL if system services use lights that don't actually exist. Bug: 151113302 Test: atest google/perf/boottime/boottime-test Change-Id: I0512c71e376c64095270b8e70e9754fe0f81210e --- services/core/java/com/android/server/BatteryService.java | 3 +++ .../java/com/android/server/display/LocalDisplayAdapter.java | 8 ++++++-- .../core/java/com/android/server/lights/LightsManager.java | 4 +++- .../server/notification/NotificationManagerService.java | 12 +++++++++--- .../java/com/android/server/power/PowerManagerService.java | 4 +++- 5 files changed, 24 insertions(+), 7 deletions(-) diff --git a/services/core/java/com/android/server/BatteryService.java b/services/core/java/com/android/server/BatteryService.java index f1f5005b23db..8dd4fa6d8fd1 100644 --- a/services/core/java/com/android/server/BatteryService.java +++ b/services/core/java/com/android/server/BatteryService.java @@ -1101,6 +1101,9 @@ public final class BatteryService extends SystemService { * Synchronize on BatteryService. */ public void updateLightsLocked() { + if (mBatteryLight == null) { + return; + } final int level = mHealthInfo.batteryLevel; final int status = mHealthInfo.batteryStatus; if (level < mLowBatteryWarningLevel) { diff --git a/services/core/java/com/android/server/display/LocalDisplayAdapter.java b/services/core/java/com/android/server/display/LocalDisplayAdapter.java index 05a757bde179..8eb771046e6d 100644 --- a/services/core/java/com/android/server/display/LocalDisplayAdapter.java +++ b/services/core/java/com/android/server/display/LocalDisplayAdapter.java @@ -646,7 +646,9 @@ final class LocalDisplayAdapter extends DisplayAdapter { + "id=" + physicalDisplayId + ", state=" + Display.stateToString(state) + ")"); } - mBacklight.setVrMode(isVrEnabled); + if (mBacklight != null) { + mBacklight.setVrMode(isVrEnabled); + } } private void setDisplayState(int state) { @@ -708,7 +710,9 @@ final class LocalDisplayAdapter extends DisplayAdapter { BrightnessSynchronizer.brightnessFloatToInt(getContext(), brightness)); } - mBacklight.setBrightness(brightness); + if (mBacklight != null) { + mBacklight.setBrightness(brightness); + } Trace.traceCounter(Trace.TRACE_TAG_POWER, "ScreenBrightness", BrightnessSynchronizer.brightnessFloatToInt( diff --git a/services/core/java/com/android/server/lights/LightsManager.java b/services/core/java/com/android/server/lights/LightsManager.java index 521913a0c439..706c74137755 100644 --- a/services/core/java/com/android/server/lights/LightsManager.java +++ b/services/core/java/com/android/server/lights/LightsManager.java @@ -16,6 +16,7 @@ package com.android.server.lights; +import android.annotation.Nullable; import android.hardware.light.V2_0.Type; public abstract class LightsManager { @@ -30,7 +31,8 @@ public abstract class LightsManager { public static final int LIGHT_ID_COUNT = Type.COUNT; /** - * Returns the logical light with the given type. + * Returns the logical light with the given type, if it exists, or null. */ + @Nullable public abstract LogicalLight getLight(int id); } diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java index 69a5b35a5b12..cebf5b92d187 100755 --- a/services/core/java/com/android/server/notification/NotificationManagerService.java +++ b/services/core/java/com/android/server/notification/NotificationManagerService.java @@ -1548,7 +1548,9 @@ public class NotificationManagerService extends SystemService { } } else if (action.equals(Intent.ACTION_USER_PRESENT)) { // turn off LED when user passes through lock screen - mNotificationLight.turnOff(); + if (mNotificationLight != null) { + mNotificationLight.turnOff(); + } } else if (action.equals(Intent.ACTION_USER_SWITCHED)) { final int userId = intent.getIntExtra(Intent.EXTRA_USER_HANDLE, USER_NULL); mUserProfiles.updateCache(context); @@ -4179,7 +4181,7 @@ public class NotificationManagerService extends SystemService { @Override public int getInterruptionFilterFromListener(INotificationListener token) throws RemoteException { - synchronized (mNotificationLight) { + synchronized (mNotificationLock) { return mInterruptionFilter; } } @@ -6973,7 +6975,7 @@ public class NotificationManagerService extends SystemService { if (canShowLightsLocked(record, aboveThreshold)) { mLights.add(key); updateLightsLocked(); - if (mUseAttentionLight) { + if (mUseAttentionLight && mAttentionLight != null) { mAttentionLight.pulse(); } blink = true; @@ -8171,6 +8173,10 @@ public class NotificationManagerService extends SystemService { @GuardedBy("mNotificationLock") void updateLightsLocked() { + if (mNotificationLight == null) { + return; + } + // handle notification lights NotificationRecord ledNotification = null; while (ledNotification == null && !mLights.isEmpty()) { diff --git a/services/core/java/com/android/server/power/PowerManagerService.java b/services/core/java/com/android/server/power/PowerManagerService.java index 294deba459fe..6af03ce9533c 100644 --- a/services/core/java/com/android/server/power/PowerManagerService.java +++ b/services/core/java/com/android/server/power/PowerManagerService.java @@ -3496,7 +3496,9 @@ public final class PowerManagerService extends SystemService } // Control light outside of lock. - light.setFlashing(color, LogicalLight.LIGHT_FLASH_HARDWARE, (on ? 3 : 0), 0); + if (light != null) { + light.setFlashing(color, LogicalLight.LIGHT_FLASH_HARDWARE, (on ? 3 : 0), 0); + } } private void setDozeAfterScreenOffInternal(boolean on) { -- cgit v1.2.3-59-g8ed1b From 95d05138c549263a12aa00a37413c2e723e61263 Mon Sep 17 00:00:00 2001 From: Robin Lee Date: Fri, 7 Feb 2020 04:20:33 +0100 Subject: Make LogicalLight @Nullable: part two (internal) Primarily refactors populateAvailableLights() * breaks AIDL and HIDL logic into two small helpers * stops going and inventing fake lights if we have the AIDL HAL. Also reworks some LightsService internals * move "system light" definition into the light class * rename mLights -> mLightsByType (matching mLightsById) * put the traceEnd() for "setLightState" in one final block The way the HIDL HAL goes and invents fake lights hasn't been changed, to avoid breaking HALs that didn't implement getLights() before. Bug: 151113302 Test: atest LightsManagerTest Test: atest LightsServiceTest Test: atest google/perf/boottime/boottime-test Change-Id: I23545586a2fa44ec656cd4c58df61970ce6db979 --- .../com/android/server/lights/LightsService.java | 126 ++++++++++----------- 1 file changed, 58 insertions(+), 68 deletions(-) diff --git a/services/core/java/com/android/server/lights/LightsService.java b/services/core/java/com/android/server/lights/LightsService.java index a42dec8b575f..3c6e8d29cae0 100644 --- a/services/core/java/com/android/server/lights/LightsService.java +++ b/services/core/java/com/android/server/lights/LightsService.java @@ -52,8 +52,8 @@ public class LightsService extends SystemService { static final String TAG = "LightsService"; static final boolean DEBUG = false; - private LightImpl[] mLights = null; - private SparseArray mLightsById = null; + private final LightImpl[] mLightsByType = new LightImpl[LightsManager.LIGHT_ID_COUNT]; + private final SparseArray mLightsById = new SparseArray<>(); private ILights mVintfLights = null; @@ -96,8 +96,8 @@ public class LightsService extends SystemService { synchronized (LightsService.this) { final List lights = new ArrayList(); for (int i = 0; i < mLightsById.size(); i++) { - HwLight hwLight = mLightsById.valueAt(i).getHwLight(); - if (!isSystemLight(hwLight)) { + if (!mLightsById.valueAt(i).isSystemLight()) { + HwLight hwLight = mLightsById.valueAt(i).mHwLight; lights.add(new Light(hwLight.id, hwLight.ordinal, hwLight.type)); } } @@ -138,7 +138,7 @@ public class LightsService extends SystemService { synchronized (LightsService.this) { final LightImpl light = mLightsById.get(lightId); - if (light == null || isSystemLight(light.getHwLight())) { + if (light == null || light.isSystemLight()) { throw new IllegalArgumentException("Invalid light: " + lightId); } return new LightState(light.getColor()); @@ -184,9 +184,8 @@ public class LightsService extends SystemService { private void checkRequestIsValid(int[] lightIds) { for (int i = 0; i < lightIds.length; i++) { final LightImpl light = mLightsById.get(lightIds[i]); - final HwLight hwLight = light.getHwLight(); - Preconditions.checkState(light != null && !isSystemLight(hwLight), - "invalid lightId " + hwLight.id); + Preconditions.checkState(light != null && !light.isSystemLight(), + "Invalid lightId " + lightIds[i]); } } @@ -205,9 +204,8 @@ public class LightsService extends SystemService { } for (int i = 0; i < mLightsById.size(); i++) { LightImpl light = mLightsById.valueAt(i); - HwLight hwLight = light.getHwLight(); - if (!isSystemLight(hwLight)) { - LightState state = states.get(hwLight.id); + if (!light.isSystemLight()) { + LightState state = states.get(light.mHwLight.id); if (state != null) { light.setColor(state.getColor()); } else { @@ -385,26 +383,22 @@ public class LightsService extends SystemService { int brightnessMode) { Trace.traceBegin(Trace.TRACE_TAG_POWER, "setLightState(" + mHwLight.id + ", 0x" + Integer.toHexString(color) + ")"); - if (mVintfLights != null) { - HwLightState lightState = new HwLightState(); - lightState.color = color; - lightState.flashMode = (byte) mode; - lightState.flashOnMs = onMS; - lightState.flashOffMs = offMS; - lightState.brightnessMode = (byte) brightnessMode; - try { + try { + if (mVintfLights != null) { + HwLightState lightState = new HwLightState(); + lightState.color = color; + lightState.flashMode = (byte) mode; + lightState.flashOnMs = onMS; + lightState.flashOffMs = offMS; + lightState.brightnessMode = (byte) brightnessMode; mVintfLights.setLightState(mHwLight.id, lightState); - } catch (RemoteException | UnsupportedOperationException ex) { - Slog.e(TAG, "Failed issuing setLightState", ex); - } finally { - Trace.traceEnd(Trace.TRACE_TAG_POWER); - } - } else { - try { + } else { setLight_native(mHwLight.id, color, mode, onMS, offMS, brightnessMode); - } finally { - Trace.traceEnd(Trace.TRACE_TAG_POWER); } + } catch (RemoteException | UnsupportedOperationException ex) { + Slog.e(TAG, "Failed issuing setLightState", ex); + } finally { + Trace.traceEnd(Trace.TRACE_TAG_POWER); } } @@ -412,8 +406,14 @@ public class LightsService extends SystemService { return mVrModeEnabled && mUseLowPersistenceForVR; } - private HwLight getHwLight() { - return mHwLight; + /** + * Returns whether a light is system-use-only or should be accessible to + * applications using the {@link android.hardware.lights.LightsManager} API. + */ + private boolean isSystemLight() { + // LIGHT_ID_COUNT comes from the 2.0 HIDL HAL and only contains system lights. + // Newly-added lights are made available via the public LightsManager API. + return (0 <= mHwLight.type && mHwLight.type < LightsManager.LIGHT_ID_COUNT); } private int getColor() { @@ -451,36 +451,37 @@ public class LightsService extends SystemService { } private void populateAvailableLights(Context context) { - mLights = new LightImpl[LightsManager.LIGHT_ID_COUNT]; - mLightsById = new SparseArray<>(); - if (mVintfLights != null) { - try { - for (HwLight availableLight : mVintfLights.getLights()) { - LightImpl light = new LightImpl(context, availableLight); - int type = (int) availableLight.type; - if (0 <= type && type < mLights.length && mLights[type] == null) { - mLights[type] = light; - } - mLightsById.put(availableLight.id, light); - } - } catch (RemoteException ex) { - Slog.e(TAG, "Unable to get lights for initialization", ex); + populateAvailableLightsFromAidl(context); + } else { + populateAvailableLightsFromHidl(context); + } + + for (int i = mLightsById.size() - 1; i >= 0; i--) { + final int type = mLightsById.keyAt(i); + if (0 <= type && type < mLightsByType.length) { + mLightsByType[type] = mLightsById.valueAt(i); } } + } - // In the case where only the old HAL is available, all lights will be initialized here - for (int i = 0; i < mLights.length; i++) { - if (mLights[i] == null) { - // The ordinal can be anything if there is only 1 light of each type. Set it to 1. - HwLight light = new HwLight(); - light.id = (byte) i; - light.ordinal = 1; - light.type = (byte) i; - - mLights[i] = new LightImpl(context, light); - mLightsById.put(i, mLights[i]); + private void populateAvailableLightsFromAidl(Context context) { + try { + for (HwLight hwLight : mVintfLights.getLights()) { + mLightsById.put(hwLight.id, new LightImpl(context, hwLight)); } + } catch (RemoteException ex) { + Slog.e(TAG, "Unable to get lights from HAL", ex); + } + } + + private void populateAvailableLightsFromHidl(Context context) { + for (int i = 0; i < mLightsByType.length; i++) { + HwLight hwLight = new HwLight(); + hwLight.id = (byte) i; + hwLight.ordinal = 1; + hwLight.type = (byte) i; + mLightsById.put(hwLight.id, new LightImpl(context, hwLight)); } } @@ -505,25 +506,14 @@ public class LightsService extends SystemService { private final LightsManager mService = new LightsManager() { @Override public LogicalLight getLight(int lightType) { - if (mLights != null && 0 <= lightType && lightType < mLights.length) { - return mLights[lightType]; + if (mLightsByType != null && 0 <= lightType && lightType < mLightsByType.length) { + return mLightsByType[lightType]; } else { return null; } } }; - /** - * Returns whether a light is system-use-only or should be accessible to - * applications using the {@link android.hardware.lights.LightsManager} API. - */ - private static boolean isSystemLight(HwLight light) { - // LIGHT_ID_COUNT comes from the 2.0 HIDL HAL and only contains system - // lights. Newly added lights will be made available via the - // LightsManager API. - return 0 <= light.type && light.type < LightsManager.LIGHT_ID_COUNT; - } - static native void setLight_native(int light, int color, int mode, int onMS, int offMS, int brightnessMode); } -- cgit v1.2.3-59-g8ed1b