From 8ad07e271f1acb30746502fd69a9d6260832f877 Mon Sep 17 00:00:00 2001 From: Neil Fuller Date: Tue, 13 Dec 2022 14:49:06 +0000 Subject: [PATCH] Refactoring: Support alt. network time source Refactoring to support an alternative source of network time for passing to the GNSS component. The new implementation will be submitted in a follow-up. NtpTimeHelper has been replaced by NetworkTimeHelper in GnssLocationProvider. NetworkTimeHelper provides the stable interface between GnssLocationProvider and the original / alternative impl for what was NtpTimeHelper. NtpTimeHelper has been renamed NtpNetworkTimeHelper. These changes are not intended to change any behavior. There are some minor changes between the interaction between GnssLocationProvider and the NetworkTimeHelper class, but these are not expected to alter the runtime behavior. The NetworkTimeHelper.setPeriodicTimeInjectionMode() method touches a pre-existing bug: The method name reflected the effect of the method, which is the near-opposite of what the capability name would suggest. This appears to be due to an accidental logic inversion, not by intent. As can be seen in the changes for GnssLocationProvider: the enablePeriodicTimeInjection() method was called when mGnssNative.getCapabilities().hasOnDemandTime() was true. The existence of bug 73893222 supports the fact that there is a long-standing bug here. The intent with this commit is not to fix it or alter behavior, just to make it more obvious, as it is unclear if the current behavior is relied upon somewhere. Comments and field names have been improved to try to clarify the actual behavior. Bug: 73893222 Bug: 222295093 Test: atest services/robotests/src/com/android/server/location/gnss/NtpNetworkTimeHelperTest.java Change-Id: I0b1ba43a55ff531df343c022650e3f5721dda7f1 --- .../android/location/GnssCapabilities.java | 6 +- .../location/gnss/GnssLocationProvider.java | 43 ++++++----- .../location/gnss/GnssManagerService.java | 3 +- .../location/gnss/NetworkTimeHelper.java | 75 +++++++++++++++++++ ...eHelper.java => NtpNetworkTimeHelper.java} | 53 +++++++------ ...est.java => NtpNetworkTimeHelperTest.java} | 30 ++++---- 6 files changed, 147 insertions(+), 63 deletions(-) create mode 100644 services/core/java/com/android/server/location/gnss/NetworkTimeHelper.java rename services/core/java/com/android/server/location/gnss/{NtpTimeHelper.java => NtpNetworkTimeHelper.java} (83%) rename services/robotests/src/com/android/server/location/gnss/{NtpTimeHelperTest.java => NtpNetworkTimeHelperTest.java} (77%) diff --git a/location/java/android/location/GnssCapabilities.java b/location/java/android/location/GnssCapabilities.java index f9f9fa47df972..11b5833e35b3b 100644 --- a/location/java/android/location/GnssCapabilities.java +++ b/location/java/android/location/GnssCapabilities.java @@ -240,7 +240,11 @@ public final class GnssCapabilities implements Parcelable { } /** - * Returns {@code true} if GNSS chipset supports on demand time, {@code false} otherwise. + * Returns {@code true} if GNSS chipset requests periodic time signal injection from the + * platform in addition to on-demand and occasional time updates, {@code false} otherwise. + * + *

Note: The naming of this capability and the behavior it controls differ substantially. + * This is the result of a historic implementation bug, b/73893222. */ public boolean hasOnDemandTime() { return (mTopFlags & TOP_HAL_CAPABILITY_ON_DEMAND_TIME) != 0; diff --git a/services/core/java/com/android/server/location/gnss/GnssLocationProvider.java b/services/core/java/com/android/server/location/gnss/GnssLocationProvider.java index 282ad574a0ed8..262013ec8d961 100644 --- a/services/core/java/com/android/server/location/gnss/GnssLocationProvider.java +++ b/services/core/java/com/android/server/location/gnss/GnssLocationProvider.java @@ -113,9 +113,8 @@ import com.android.internal.util.FrameworkStatsLog; import com.android.internal.util.HexDump; import com.android.server.FgThread; import com.android.server.location.gnss.GnssSatelliteBlocklistHelper.GnssSatelliteBlocklistCallback; -import com.android.server.location.gnss.NtpTimeHelper.InjectNtpTimeCallback; +import com.android.server.location.gnss.NetworkTimeHelper.InjectTimeCallback; import com.android.server.location.gnss.hal.GnssNative; -import com.android.server.location.injector.Injector; import com.android.server.location.provider.AbstractLocationProvider; import java.io.FileDescriptor; @@ -138,7 +137,7 @@ import java.util.concurrent.TimeUnit; * {@hide} */ public class GnssLocationProvider extends AbstractLocationProvider implements - InjectNtpTimeCallback, GnssSatelliteBlocklistCallback, GnssNative.BaseCallbacks, + InjectTimeCallback, GnssSatelliteBlocklistCallback, GnssNative.BaseCallbacks, GnssNative.LocationCallbacks, GnssNative.SvStatusCallbacks, GnssNative.AGpsCallbacks, GnssNative.PsdsCallbacks, GnssNative.NotificationCallbacks, GnssNative.LocationRequestCallbacks, GnssNative.TimeCallbacks { @@ -307,7 +306,7 @@ public class GnssLocationProvider extends AbstractLocationProvider implements private boolean mSuplEsEnabled = false; private final LocationExtras mLocationExtras = new LocationExtras(); - private final NtpTimeHelper mNtpTimeHelper; + private final NetworkTimeHelper mNetworkTimeHelper; private final GnssSatelliteBlocklistHelper mGnssSatelliteBlocklistHelper; // Available only on GNSS HAL 2.0 implementations and later. @@ -398,7 +397,7 @@ public class GnssLocationProvider extends AbstractLocationProvider implements } } - public GnssLocationProvider(Context context, Injector injector, GnssNative gnssNative, + public GnssLocationProvider(Context context, GnssNative gnssNative, GnssMetrics gnssMetrics) { super(FgThread.getExecutor(), CallerIdentity.fromContext(context), PROPERTIES, Collections.emptySet()); @@ -470,7 +469,7 @@ public class GnssLocationProvider extends AbstractLocationProvider implements GnssLocationProvider.this::onNetworkAvailable, mHandler.getLooper(), mNIHandler); - mNtpTimeHelper = new NtpTimeHelper(mContext, mHandler.getLooper(), this); + mNetworkTimeHelper = NetworkTimeHelper.create(mContext, mHandler.getLooper(), this); mGnssSatelliteBlocklistHelper = new GnssSatelliteBlocklistHelper(mContext, mHandler.getLooper(), this); @@ -647,18 +646,19 @@ public class GnssLocationProvider extends AbstractLocationProvider implements } /** - * Implements {@link InjectNtpTimeCallback#injectTime} + * Implements {@link InjectTimeCallback#injectTime} */ @Override - public void injectTime(long time, long timeReference, int uncertainty) { - mGnssNative.injectTime(time, timeReference, uncertainty); + public void injectTime(long unixEpochTimeMillis, long elapsedRealtimeMillis, + int uncertaintyMillis) { + mGnssNative.injectTime(unixEpochTimeMillis, elapsedRealtimeMillis, uncertaintyMillis); } /** * Implements {@link GnssNetworkConnectivityHandler.GnssNetworkListener#onNetworkAvailable()} */ private void onNetworkAvailable() { - mNtpTimeHelper.onNetworkAvailable(); + mNetworkTimeHelper.onNetworkAvailable(); // Download only if supported, (prevents an unnecessary on-boot download) if (mSupportsPsds) { synchronized (mLock) { @@ -1145,7 +1145,7 @@ public class GnssLocationProvider extends AbstractLocationProvider implements if ("delete_aiding_data".equals(command)) { deleteAidingData(extras); } else if ("force_time_injection".equals(command)) { - requestUtcTime(); + demandUtcTimeInjection(); } else if ("force_psds_injection".equals(command)) { if (mSupportsPsds) { postWithWakeLockHeld(() -> handleDownloadPsdsData( @@ -1514,9 +1514,9 @@ public class GnssLocationProvider extends AbstractLocationProvider implements /* userResponse= */ 0); } - private void requestUtcTime() { - if (DEBUG) Log.d(TAG, "utcTimeRequest"); - postWithWakeLockHeld(mNtpTimeHelper::retrieveAndInjectNtpTime); + private void demandUtcTimeInjection() { + if (DEBUG) Log.d(TAG, "demandUtcTimeInjection"); + postWithWakeLockHeld(mNetworkTimeHelper::demandUtcTimeInjection); } @@ -1721,9 +1721,16 @@ public class GnssLocationProvider extends AbstractLocationProvider implements public void onCapabilitiesChanged(GnssCapabilities oldCapabilities, GnssCapabilities newCapabilities) { mHandler.post(() -> { - if (mGnssNative.getCapabilities().hasOnDemandTime()) { - mNtpTimeHelper.enablePeriodicTimeInjection(); - requestUtcTime(); + boolean useOnDemandTimeInjection = mGnssNative.getCapabilities().hasOnDemandTime(); + + // b/73893222: There is a historic bug on Android, which means that the capability + // "on demand time" is interpreted as "enable periodic injection" elsewhere but an + // on-demand injection is done here. GNSS developers may have come to rely on the + // periodic behavior, so it has been kept and all methods named to reflect what is + // actually done. "On demand" requests are supported regardless of the capability. + mNetworkTimeHelper.setPeriodicTimeInjectionMode(useOnDemandTimeInjection); + if (useOnDemandTimeInjection) { + demandUtcTimeInjection(); } restartLocationRequest(); @@ -1857,7 +1864,7 @@ public class GnssLocationProvider extends AbstractLocationProvider implements @Override public void onRequestUtcTime() { - requestUtcTime(); + demandUtcTimeInjection(); } @Override diff --git a/services/core/java/com/android/server/location/gnss/GnssManagerService.java b/services/core/java/com/android/server/location/gnss/GnssManagerService.java index 69385a92cab15..2174f4044ffd3 100644 --- a/services/core/java/com/android/server/location/gnss/GnssManagerService.java +++ b/services/core/java/com/android/server/location/gnss/GnssManagerService.java @@ -83,8 +83,7 @@ public class GnssManagerService { mGnssMetrics = new GnssMetrics(mContext, IBatteryStats.Stub.asInterface( ServiceManager.getService(BatteryStats.SERVICE_NAME)), mGnssNative); - mGnssLocationProvider = new GnssLocationProvider(mContext, injector, mGnssNative, - mGnssMetrics); + mGnssLocationProvider = new GnssLocationProvider(mContext, mGnssNative, mGnssMetrics); mGnssStatusProvider = new GnssStatusProvider(injector, mGnssNative); mGnssNmeaProvider = new GnssNmeaProvider(injector, mGnssNative); mGnssMeasurementsProvider = new GnssMeasurementsProvider(injector, mGnssNative); diff --git a/services/core/java/com/android/server/location/gnss/NetworkTimeHelper.java b/services/core/java/com/android/server/location/gnss/NetworkTimeHelper.java new file mode 100644 index 0000000000000..72d6f70145153 --- /dev/null +++ b/services/core/java/com/android/server/location/gnss/NetworkTimeHelper.java @@ -0,0 +1,75 @@ +/* + * Copyright (C) 2022 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.android.server.location.gnss; + +import android.content.Context; +import android.os.Looper; + +/** + * An abstraction for use by {@link GnssLocationProvider}. This class allows switching between + * implementations with a compile-time constant change, which is less risky than rolling back a + * whole class. When there is a single implementation again this class can be replaced by that + * implementation. + */ +abstract class NetworkTimeHelper { + + /** + * The callback interface used by {@link NetworkTimeHelper} to report the time to {@link + * GnssLocationProvider}. The callback can happen at any time using the thread associated with + * the looper passed to {@link #create(Context, Looper, InjectTimeCallback)}. + */ + interface InjectTimeCallback { + void injectTime(long unixEpochTimeMillis, long elapsedRealtimeMillis, + int uncertaintyMillis); + } + + /** + * Creates the {@link NetworkTimeHelper} instance for use by {@link GnssLocationProvider}. + */ + static NetworkTimeHelper create( + Context context, Looper looper, InjectTimeCallback injectTimeCallback) { + return new NtpNetworkTimeHelper(context, looper, injectTimeCallback); + } + + /** + * Sets the "on demand time injection" mode. + * + *

Called by {@link GnssLocationProvider} to set the expected time injection behavior. + * When {@code enablePeriodicTimeInjection == true}, the time helper should periodically send + * the time on an undefined schedule. The time can be injected at other times for other reasons + * as well as be requested via {@link #demandUtcTimeInjection()}. + * + * @param periodicTimeInjectionEnabled {@code true} if the GNSS implementation requires periodic + * time signals + */ + abstract void setPeriodicTimeInjectionMode(boolean periodicTimeInjectionEnabled); + + /** + * Requests an asynchronous time injection via {@link InjectTimeCallback#injectTime}, if a + * network time is available. {@link InjectTimeCallback#injectTime} may not be called if a + * network time is not available. + */ + abstract void demandUtcTimeInjection(); + + /** + * Notifies that network connectivity has been established. + * + *

Called by {@link GnssLocationProvider} when the device establishes a data network + * connection. + */ + abstract void onNetworkAvailable(); + +} diff --git a/services/core/java/com/android/server/location/gnss/NtpTimeHelper.java b/services/core/java/com/android/server/location/gnss/NtpNetworkTimeHelper.java similarity index 83% rename from services/core/java/com/android/server/location/gnss/NtpTimeHelper.java rename to services/core/java/com/android/server/location/gnss/NtpNetworkTimeHelper.java index e4bc78827269f..479dbdab8bd58 100644 --- a/services/core/java/com/android/server/location/gnss/NtpTimeHelper.java +++ b/services/core/java/com/android/server/location/gnss/NtpNetworkTimeHelper.java @@ -30,14 +30,11 @@ import com.android.internal.annotations.GuardedBy; import com.android.internal.annotations.VisibleForTesting; /** - * Handles inject NTP time to GNSS. - * - *

The client is responsible to call {@link #onNetworkAvailable()} when network is available - * for retrieving NTP Time. + * Handles injecting network time to GNSS by explicitly making NTP requests when needed. */ -class NtpTimeHelper { +class NtpNetworkTimeHelper extends NetworkTimeHelper { - private static final String TAG = "NtpTimeHelper"; + private static final String TAG = "NtpNetworkTimeHelper"; private static final boolean DEBUG = Log.isLoggable(TAG, Log.DEBUG); // states for injecting ntp @@ -71,23 +68,19 @@ class NtpTimeHelper { private final WakeLock mWakeLock; private final Handler mHandler; - private final InjectNtpTimeCallback mCallback; + private final InjectTimeCallback mCallback; // flags to trigger NTP when network becomes available // initialized to STATE_PENDING_NETWORK so we do NTP when the network comes up after booting @GuardedBy("this") private int mInjectNtpTimeState = STATE_PENDING_NETWORK; - // set to true if the GPS engine requested on-demand NTP time requests + // Enables periodic time injection in addition to injection for other reasons. @GuardedBy("this") - private boolean mOnDemandTimeInjection; - - interface InjectNtpTimeCallback { - void injectTime(long time, long timeReference, int uncertainty); - } + private boolean mPeriodicTimeInjection; @VisibleForTesting - NtpTimeHelper(Context context, Looper looper, InjectNtpTimeCallback callback, + NtpNetworkTimeHelper(Context context, Looper looper, InjectTimeCallback callback, NtpTrustedTime ntpTime) { mConnMgr = (ConnectivityManager) context.getSystemService(Context.CONNECTIVITY_SERVICE); mCallback = callback; @@ -97,14 +90,23 @@ class NtpTimeHelper { mWakeLock = powerManager.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, WAKELOCK_KEY); } - NtpTimeHelper(Context context, Looper looper, InjectNtpTimeCallback callback) { + NtpNetworkTimeHelper(Context context, Looper looper, InjectTimeCallback callback) { this(context, looper, callback, NtpTrustedTime.getInstance(context)); } - synchronized void enablePeriodicTimeInjection() { - mOnDemandTimeInjection = true; + @Override + synchronized void setPeriodicTimeInjectionMode(boolean periodicTimeInjectionEnabled) { + if (periodicTimeInjectionEnabled) { + mPeriodicTimeInjection = true; + } } + @Override + void demandUtcTimeInjection() { + retrieveAndInjectNtpTime(); + } + + @Override synchronized void onNetworkAvailable() { if (mInjectNtpTimeState == STATE_PENDING_NETWORK) { retrieveAndInjectNtpTime(); @@ -120,7 +122,7 @@ class NtpTimeHelper { return activeNetworkInfo != null && activeNetworkInfo.isConnected(); } - synchronized void retrieveAndInjectNtpTime() { + private synchronized void retrieveAndInjectNtpTime() { if (mInjectNtpTimeState == STATE_RETRIEVING_AND_INJECTING) { // already downloading data return; @@ -166,18 +168,15 @@ class NtpTimeHelper { if (DEBUG) { Log.d(TAG, String.format( - "onDemandTimeInjection=%s, refreshSuccess=%s, delay=%s", - mOnDemandTimeInjection, + "mPeriodicTimeInjection=%s, refreshSuccess=%s, delay=%s", + mPeriodicTimeInjection, refreshSuccess, delay)); } - // TODO(b/73893222): reconcile Capabilities bit 'on demand' name vs. de facto periodic - // injection. - if (mOnDemandTimeInjection || !refreshSuccess) { - /* Schedule next NTP injection. - * Since this is delayed, the wake lock is released right away, and will be held - * again when the delayed task runs. - */ + if (mPeriodicTimeInjection || !refreshSuccess) { + // Schedule next NTP injection. + // Since this is delayed, the wake lock is released right away, and will be held + // again when the delayed task runs. mHandler.postDelayed(this::retrieveAndInjectNtpTime, delay); } } diff --git a/services/robotests/src/com/android/server/location/gnss/NtpTimeHelperTest.java b/services/robotests/src/com/android/server/location/gnss/NtpNetworkTimeHelperTest.java similarity index 77% rename from services/robotests/src/com/android/server/location/gnss/NtpTimeHelperTest.java rename to services/robotests/src/com/android/server/location/gnss/NtpNetworkTimeHelperTest.java index e5a1cfeb55c21..4949091646ddd 100644 --- a/services/robotests/src/com/android/server/location/gnss/NtpTimeHelperTest.java +++ b/services/robotests/src/com/android/server/location/gnss/NtpNetworkTimeHelperTest.java @@ -26,7 +26,7 @@ import android.os.SystemClock; import android.platform.test.annotations.Presubmit; import android.util.NtpTrustedTime; -import com.android.server.location.gnss.NtpTimeHelper.InjectNtpTimeCallback; +import com.android.server.location.gnss.NetworkTimeHelper.InjectTimeCallback; import org.junit.Before; import org.junit.Test; @@ -41,16 +41,16 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; /** - * Unit tests for {@link NtpTimeHelper}. + * Unit tests for {@link NtpNetworkTimeHelper}. */ @RunWith(RobolectricTestRunner.class) @Presubmit -public class NtpTimeHelperTest { +public class NtpNetworkTimeHelperTest { private static final long MOCK_NTP_TIME = 1519930775453L; @Mock private NtpTrustedTime mMockNtpTrustedTime; - private NtpTimeHelper mNtpTimeHelper; + private NtpNetworkTimeHelper mNtpNetworkTimeHelper; private CountDownLatch mCountDownLatch; /** @@ -60,12 +60,12 @@ public class NtpTimeHelperTest { public void setUp() throws Exception { MockitoAnnotations.initMocks(this); mCountDownLatch = new CountDownLatch(1); - InjectNtpTimeCallback callback = + InjectTimeCallback callback = (time, timeReference, uncertainty) -> { assertThat(time).isEqualTo(MOCK_NTP_TIME); mCountDownLatch.countDown(); }; - mNtpTimeHelper = new NtpTimeHelper(RuntimeEnvironment.application, + mNtpNetworkTimeHelper = new NtpNetworkTimeHelper(RuntimeEnvironment.application, Looper.myLooper(), callback, mMockNtpTrustedTime); } @@ -74,13 +74,13 @@ public class NtpTimeHelperTest { * Verify that cached time is returned if cached age is low. */ @Test - public void handleInjectNtpTime_cachedAgeLow_injectTime() throws InterruptedException { + public void demandUtcTimeInjection_cachedAgeLow_injectTime() throws InterruptedException { NtpTrustedTime.TimeResult result = mock(NtpTrustedTime.TimeResult.class); - doReturn(NtpTimeHelper.NTP_INTERVAL - 1).when(result).getAgeMillis(); + doReturn(NtpNetworkTimeHelper.NTP_INTERVAL - 1).when(result).getAgeMillis(); doReturn(MOCK_NTP_TIME).when(result).getTimeMillis(); doReturn(result).when(mMockNtpTrustedTime).getCachedTimeResult(); - mNtpTimeHelper.retrieveAndInjectNtpTime(); + mNtpNetworkTimeHelper.demandUtcTimeInjection(); waitForTasksToBePostedOnHandlerAndRunThem(); assertThat(mCountDownLatch.await(2, TimeUnit.SECONDS)).isTrue(); @@ -90,14 +90,14 @@ public class NtpTimeHelperTest { * Verify that failed inject time and delayed inject time are handled properly. */ @Test - public void handleInjectNtpTime_injectTimeFailed_injectTimeDelayed() + public void demandUtcTimeInjection_injectTimeFailed_injectTimeDelayed() throws InterruptedException { NtpTrustedTime.TimeResult result1 = mock(NtpTrustedTime.TimeResult.class); - doReturn(NtpTimeHelper.NTP_INTERVAL + 1).when(result1).getAgeMillis(); + doReturn(NtpNetworkTimeHelper.NTP_INTERVAL + 1).when(result1).getAgeMillis(); doReturn(result1).when(mMockNtpTrustedTime).getCachedTimeResult(); doReturn(false).when(mMockNtpTrustedTime).forceRefresh(); - mNtpTimeHelper.retrieveAndInjectNtpTime(); + mNtpNetworkTimeHelper.demandUtcTimeInjection(); waitForTasksToBePostedOnHandlerAndRunThem(); assertThat(mCountDownLatch.await(2, TimeUnit.SECONDS)).isFalse(); @@ -106,15 +106,15 @@ public class NtpTimeHelperTest { doReturn(1L).when(result2).getAgeMillis(); doReturn(MOCK_NTP_TIME).when(result2).getTimeMillis(); doReturn(result2).when(mMockNtpTrustedTime).getCachedTimeResult(); - SystemClock.sleep(NtpTimeHelper.RETRY_INTERVAL); + SystemClock.sleep(NtpNetworkTimeHelper.RETRY_INTERVAL); waitForTasksToBePostedOnHandlerAndRunThem(); assertThat(mCountDownLatch.await(2, TimeUnit.SECONDS)).isTrue(); } /** - * Since a thread is created in {@link NtpTimeHelper#retrieveAndInjectNtpTime} and the task to - * be verified is posted in the thread, we have to wait for the task to be posted and then it + * Since a thread is created in {@link NtpNetworkTimeHelper#demandUtcTimeInjection} and the task + * to be verified is posted in the thread, we have to wait for the task to be posted and then it * can be run. */ private void waitForTasksToBePostedOnHandlerAndRunThem() throws InterruptedException {