From a29a658ca42e5e48334c642ecbb8a70a417e7556 Mon Sep 17 00:00:00 2001 From: Yu-Han Yang Date: Thu, 23 Feb 2023 19:17:37 +0000 Subject: [PATCH] Handle GnssMeasurement registration according to HAL versions Also fix that if the newly merged request is passive, unregister should be called before returning true, to avoid measurements being reported without an active request. Bug: 270569009 Test: atest GnssMeasurementsProviderTest Change-Id: I9c53d60b2e8d896a90ae346bd9458ac1ed2aa429 --- .../location/gnss/GnssConfiguration.java | 2 + .../gnss/GnssMeasurementsProvider.java | 34 +++- services/core/jni/gnss/GnssConfiguration.cpp | 2 +- .../gnss/GnssMeasurementsProviderTest.java | 188 ++++++++++++++++++ 4 files changed, 215 insertions(+), 11 deletions(-) create mode 100644 services/tests/mockingservicestests/src/com/android/server/location/gnss/GnssMeasurementsProviderTest.java diff --git a/services/core/java/com/android/server/location/gnss/GnssConfiguration.java b/services/core/java/com/android/server/location/gnss/GnssConfiguration.java index 77cd673047292..a081dff9e62d0 100644 --- a/services/core/java/com/android/server/location/gnss/GnssConfiguration.java +++ b/services/core/java/com/android/server/location/gnss/GnssConfiguration.java @@ -92,6 +92,8 @@ public class GnssConfiguration { // Represents an HAL interface version. Instances of this class are created in the JNI layer // and returned through native methods. static class HalInterfaceVersion { + // mMajor being this value denotes AIDL HAL. In this case, mMinor denotes the AIDL version. + static final int AIDL_INTERFACE = 3; final int mMajor; final int mMinor; diff --git a/services/core/java/com/android/server/location/gnss/GnssMeasurementsProvider.java b/services/core/java/com/android/server/location/gnss/GnssMeasurementsProvider.java index 6c4c829b051d9..041f11d972fe3 100644 --- a/services/core/java/com/android/server/location/gnss/GnssMeasurementsProvider.java +++ b/services/core/java/com/android/server/location/gnss/GnssMeasurementsProvider.java @@ -21,6 +21,7 @@ import static android.app.AppOpsManager.OP_MONITOR_HIGH_POWER_LOCATION; import static com.android.server.location.gnss.GnssManagerService.D; import static com.android.server.location.gnss.GnssManagerService.TAG; +import android.annotation.NonNull; import android.annotation.Nullable; import android.app.AppOpsManager; import android.location.GnssMeasurementRequest; @@ -31,6 +32,7 @@ import android.os.IBinder; import android.stats.location.LocationStatsEnums; import android.util.Log; +import com.android.server.location.gnss.GnssConfiguration.HalInterfaceVersion; import com.android.server.location.gnss.hal.GnssNative; import com.android.server.location.injector.AppOpsHelper; import com.android.server.location.injector.Injector; @@ -115,16 +117,6 @@ public final class GnssMeasurementsProvider extends if (request.getIntervalMillis() == GnssMeasurementRequest.PASSIVE_INTERVAL) { return true; } - // The HAL doc does not specify if consecutive start() calls will be allowed. - // Some vendors may ignore the 2nd start() call if stop() is not called. - // Thus, here we always call stop() before calling start() to avoid being ignored. - if (mGnssNative.stopMeasurementCollection()) { - if (D) { - Log.d(TAG, "stopping gnss measurements"); - } - } else { - Log.e(TAG, "error stopping gnss measurements"); - } if (mGnssNative.startMeasurementCollection(request.isFullTracking(), request.isCorrelationVectorOutputsEnabled(), request.getIntervalMillis())) { @@ -138,6 +130,28 @@ public final class GnssMeasurementsProvider extends } } + @Override + protected boolean reregisterWithService(GnssMeasurementRequest old, + GnssMeasurementRequest request, + @NonNull Collection registrations) { + if (request.getIntervalMillis() == GnssMeasurementRequest.PASSIVE_INTERVAL) { + unregisterWithService(); + return true; + } + HalInterfaceVersion halInterfaceVersion = + mGnssNative.getConfiguration().getHalInterfaceVersion(); + boolean aidlV3Plus = halInterfaceVersion.mMajor == HalInterfaceVersion.AIDL_INTERFACE + && halInterfaceVersion.mMinor >= 3; + if (!aidlV3Plus) { + // The HAL doc does not specify if consecutive start() calls will be allowed. + // Some vendors may ignore the 2nd start() call if stop() is not called. + // Thus, here we always call stop() before calling start() to avoid being ignored. + // AIDL v3+ is free from this issue. + unregisterWithService(); + } + return registerWithService(request, registrations); + } + @Override protected void unregisterWithService() { if (mGnssNative.stopMeasurementCollection()) { diff --git a/services/core/jni/gnss/GnssConfiguration.cpp b/services/core/jni/gnss/GnssConfiguration.cpp index 3677641127f35..b57e451264a46 100644 --- a/services/core/jni/gnss/GnssConfiguration.cpp +++ b/services/core/jni/gnss/GnssConfiguration.cpp @@ -67,7 +67,7 @@ GnssConfiguration::GnssConfiguration(const sp& iGnssConfigur : mIGnssConfiguration(iGnssConfiguration) {} jobject GnssConfiguration::getVersion(JNIEnv* env) { - return createHalInterfaceVersionJavaObject(env, 3, 0); + return createHalInterfaceVersionJavaObject(env, 3, mIGnssConfiguration->getInterfaceVersion()); } jboolean GnssConfiguration::setEmergencySuplPdn(jint enable) { diff --git a/services/tests/mockingservicestests/src/com/android/server/location/gnss/GnssMeasurementsProviderTest.java b/services/tests/mockingservicestests/src/com/android/server/location/gnss/GnssMeasurementsProviderTest.java new file mode 100644 index 0000000000000..fd9dfe869d521 --- /dev/null +++ b/services/tests/mockingservicestests/src/com/android/server/location/gnss/GnssMeasurementsProviderTest.java @@ -0,0 +1,188 @@ +/* + * Copyright (C) 2023 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 static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.eq; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; + +import android.content.Context; +import android.location.GnssMeasurementRequest; +import android.location.IGnssMeasurementsListener; +import android.location.LocationManager; +import android.location.LocationManagerInternal; +import android.location.util.identity.CallerIdentity; +import android.os.IBinder; +import android.platform.test.annotations.Presubmit; + +import androidx.test.filters.SmallTest; +import androidx.test.runner.AndroidJUnit4; + +import com.android.server.LocalServices; +import com.android.server.location.gnss.hal.FakeGnssHal; +import com.android.server.location.gnss.hal.GnssNative; +import com.android.server.location.injector.FakeUserInfoHelper; +import com.android.server.location.injector.Injector; +import com.android.server.location.injector.TestInjector; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; + +import java.util.Objects; + +@Presubmit +@SmallTest +@RunWith(AndroidJUnit4.class) +public class GnssMeasurementsProviderTest { + private static final int CURRENT_USER = FakeUserInfoHelper.DEFAULT_USERID; + private static final CallerIdentity IDENTITY = CallerIdentity.forTest(CURRENT_USER, 1000, + "mypackage", "attribution", "listener"); + private static final GnssConfiguration.HalInterfaceVersion HIDL_V2_1 = + new GnssConfiguration.HalInterfaceVersion( + 2, 1); + private static final GnssConfiguration.HalInterfaceVersion AIDL_V3 = + new GnssConfiguration.HalInterfaceVersion( + GnssConfiguration.HalInterfaceVersion.AIDL_INTERFACE, 3); + private static final GnssMeasurementRequest ACTIVE_REQUEST = + new GnssMeasurementRequest.Builder().build(); + private static final GnssMeasurementRequest PASSIVE_REQUEST = + new GnssMeasurementRequest.Builder().setIntervalMillis( + GnssMeasurementRequest.PASSIVE_INTERVAL).build(); + private @Mock Context mContext; + private @Mock LocationManagerInternal mInternal; + private @Mock GnssConfiguration mMockConfiguration; + private @Mock GnssNative.GeofenceCallbacks mGeofenceCallbacks; + private @Mock IGnssMeasurementsListener mListener1; + private @Mock IGnssMeasurementsListener mListener2; + private @Mock IBinder mBinder1; + private @Mock IBinder mBinder2; + + private GnssNative mGnssNative; + + private GnssMeasurementsProvider mTestProvider; + + @Before + public void setUp() { + MockitoAnnotations.initMocks(this); + + doReturn(mBinder1).when(mListener1).asBinder(); + doReturn(mBinder2).when(mListener2).asBinder(); + doReturn(true).when(mInternal).isProviderEnabledForUser(eq(LocationManager.GPS_PROVIDER), + anyInt()); + LocalServices.addService(LocationManagerInternal.class, mInternal); + FakeGnssHal fakeGnssHal = new FakeGnssHal(); + GnssNative.setGnssHalForTest(fakeGnssHal); + Injector injector = new TestInjector(mContext); + mGnssNative = spy(Objects.requireNonNull( + GnssNative.create(injector, mMockConfiguration))); + mGnssNative.setGeofenceCallbacks(mGeofenceCallbacks); + mTestProvider = new GnssMeasurementsProvider(injector, mGnssNative); + mGnssNative.register(); + } + + @After + public void tearDown() { + LocalServices.removeServiceForTest(LocationManagerInternal.class); + } + + @Test + public void testAddListener_active() { + // add the active request + mTestProvider.addListener(ACTIVE_REQUEST, IDENTITY, mListener1); + verify(mGnssNative, times(1)).startMeasurementCollection( + eq(ACTIVE_REQUEST.isFullTracking()), + eq(ACTIVE_REQUEST.isCorrelationVectorOutputsEnabled()), + eq(ACTIVE_REQUEST.getIntervalMillis())); + + // remove the active request + mTestProvider.removeListener(mListener1); + verify(mGnssNative, times(1)).stopMeasurementCollection(); + } + + @Test + public void testAddListener_passive() { + // add the passive request + mTestProvider.addListener(PASSIVE_REQUEST, IDENTITY, mListener1); + verify(mGnssNative, never()).startMeasurementCollection(anyBoolean(), anyBoolean(), + anyInt()); + + // remove the passive request + mTestProvider.removeListener(mListener1); + verify(mGnssNative, times(1)).stopMeasurementCollection(); + } + + @Test + public void testReregister_aidlV3Plus() { + doReturn(AIDL_V3).when(mMockConfiguration).getHalInterfaceVersion(); + + // add the passive request + mTestProvider.addListener(PASSIVE_REQUEST, IDENTITY, mListener1); + verify(mGnssNative, never()).startMeasurementCollection(anyBoolean(), anyBoolean(), + anyInt()); + + // add the active request, reregister with the active request + mTestProvider.addListener(ACTIVE_REQUEST, IDENTITY, mListener2); + verify(mGnssNative, never()).stopMeasurementCollection(); + verify(mGnssNative, times(1)).startMeasurementCollection( + eq(ACTIVE_REQUEST.isFullTracking()), + eq(ACTIVE_REQUEST.isCorrelationVectorOutputsEnabled()), + eq(ACTIVE_REQUEST.getIntervalMillis())); + + // remove the active request, reregister with the passive request + mTestProvider.removeListener(mListener2); + verify(mGnssNative, times(1)).stopMeasurementCollection(); + + // remove the passive request + mTestProvider.removeListener(mListener1); + verify(mGnssNative, times(2)).stopMeasurementCollection(); + } + + @Test + public void testReregister_preAidlV3() { + doReturn(HIDL_V2_1).when(mMockConfiguration).getHalInterfaceVersion(); + + // add the passive request + mTestProvider.addListener(PASSIVE_REQUEST, IDENTITY, mListener1); + verify(mGnssNative, never()).startMeasurementCollection(anyBoolean(), anyBoolean(), + anyInt()); + + // add the active request, reregister with the active request + mTestProvider.addListener(ACTIVE_REQUEST, IDENTITY, mListener2); + verify(mGnssNative, times(1)).stopMeasurementCollection(); + verify(mGnssNative, times(1)).startMeasurementCollection( + eq(ACTIVE_REQUEST.isFullTracking()), + eq(ACTIVE_REQUEST.isCorrelationVectorOutputsEnabled()), + eq(ACTIVE_REQUEST.getIntervalMillis())); + + // remove the active request, reregister with the passive request + mTestProvider.removeListener(mListener2); + verify(mGnssNative, times(2)).stopMeasurementCollection(); + + // remove the passive request + mTestProvider.removeListener(mListener1); + verify(mGnssNative, times(3)).stopMeasurementCollection(); + } +}