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

4 files changed, 215 insertions, 11 deletions

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 77cd67304729..a081dff9e62d 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 6c4c829b051d..041f11d972fe 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())) {
@@ -139,6 +131,28 @@ public final class GnssMeasurementsProvider extends
     }
 
     @Override
+    protected boolean reregisterWithService(GnssMeasurementRequest old,
+            GnssMeasurementRequest request,
+            @NonNull Collection<GnssListenerRegistration> 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()) {
             if (D) {
diff --git a/services/core/jni/gnss/GnssConfiguration.cpp b/services/core/jni/gnss/GnssConfiguration.cpp
index 3677641127f3..b57e451264a4 100644
--- a/services/core/jni/gnss/GnssConfiguration.cpp
+++ b/services/core/jni/gnss/GnssConfiguration.cpp
@@ -67,7 +67,7 @@ GnssConfiguration::GnssConfiguration(const sp<IGnssConfiguration>& 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 000000000000..fd9dfe869d52
--- /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();
+    }
+}