Merge "Handling concurrent location & SV status update"

2 files changed, 133 insertions, 126 deletions

diff --git a/services/core/java/com/android/server/location/GnssLocationProvider.java b/services/core/java/com/android/server/location/GnssLocationProvider.java
index 9d2a8e28a3a0..0dab528bb99d 100644
--- a/services/core/java/com/android/server/location/GnssLocationProvider.java
+++ b/services/core/java/com/android/server/location/GnssLocationProvider.java
@@ -215,6 +215,8 @@ public class GnssLocationProvider implements LocationProviderInterface {
     private static final int REQUEST_SUPL_CONNECTION = 14;
     private static final int RELEASE_SUPL_CONNECTION = 15;
     private static final int REQUEST_LOCATION = 16;
+    private static final int REPORT_LOCATION = 17; // HAL reports location
+    private static final int REPORT_SV_STATUS = 18; // HAL reports SV status
 
     // Request setid
     private static final int AGPS_RIL_REQUEST_SETID_IMSI = 1;
@@ -266,7 +268,7 @@ public class GnssLocationProvider implements LocationProviderInterface {
         }
     }
 
-    // Simple class to hold stats reported in the Extras Bundle
+    // Threadsafe class to hold stats reported in the Extras Bundle
     private static class LocationExtras {
         private int mSvCount;
         private int mMeanCn0;
@@ -278,9 +280,11 @@ public class GnssLocationProvider implements LocationProviderInterface {
         }
 
         public void set(int svCount, int meanCn0, int maxCn0) {
-            mSvCount = svCount;
-            mMeanCn0 = meanCn0;
-            mMaxCn0 = maxCn0;
+            synchronized(this) {
+                mSvCount = svCount;
+                mMeanCn0 = meanCn0;
+                mMaxCn0 = maxCn0;
+            }
             setBundle(mBundle);
         }
 
@@ -291,14 +295,18 @@ public class GnssLocationProvider implements LocationProviderInterface {
         // Also used by outside methods to add to other bundles
         public void setBundle(Bundle extras) {
             if (extras != null) {
-                extras.putInt("satellites", mSvCount);
-                extras.putInt("meanCn0", mMeanCn0);
-                extras.putInt("maxCn0", mMaxCn0);
+                synchronized (this) {
+                    extras.putInt("satellites", mSvCount);
+                    extras.putInt("meanCn0", mMeanCn0);
+                    extras.putInt("maxCn0", mMaxCn0);
+                }
             }
         }
 
         public Bundle getBundle() {
-            return mBundle;
+            synchronized (this) {
+                return new Bundle(mBundle);
+            }
         }
     }
 
@@ -411,7 +419,6 @@ public class GnssLocationProvider implements LocationProviderInterface {
     private final Context mContext;
     private final NtpTrustedTime mNtpTime;
     private final ILocationManager mILocationManager;
-    private Location mLocation = new Location(LocationManager.GPS_PROVIDER);
     private final LocationExtras mLocationExtras = new LocationExtras();
     private final GnssStatusListenerHelper mListenerHelper;
     private final GnssMeasurementsProvider mGnssMeasurementsProvider;
@@ -758,8 +765,6 @@ public class GnssLocationProvider implements LocationProviderInterface {
         mNtpTime = NtpTrustedTime.getInstance(context);
         mILocationManager = ilocationManager;
 
-        mLocation.setExtras(mLocationExtras.getBundle());
-
         // Create a wake lock
         mPowerManager = (PowerManager) mContext.getSystemService(Context.POWER_SERVICE);
         mWakeLock = mPowerManager.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, WAKELOCK_KEY);
@@ -1726,6 +1731,10 @@ public class GnssLocationProvider implements LocationProviderInterface {
      * called from native code to update our position.
      */
     private void reportLocation(boolean hasLatLong, Location location) {
+        sendMessage(REPORT_LOCATION, hasLatLong ? 1 : 0, location);
+    }
+
+    private void handleReportLocation(boolean hasLatLong, Location location) {
         if (location.hasSpeed()) {
             mItarSpeedLimitExceeded = location.getSpeed() > ITAR_SPEED_LIMIT_METERS_PER_SECOND;
         }
@@ -1739,18 +1748,15 @@ public class GnssLocationProvider implements LocationProviderInterface {
 
         if (VERBOSE) Log.v(TAG, "reportLocation " + location.toString());
 
-        synchronized (mLocation) {
-            mLocation = location;
-            // It would be nice to push the elapsed real-time timestamp
-            // further down the stack, but this is still useful
-            mLocation.setElapsedRealtimeNanos(SystemClock.elapsedRealtimeNanos());
-            mLocation.setExtras(mLocationExtras.getBundle());
+        // It would be nice to push the elapsed real-time timestamp
+        // further down the stack, but this is still useful
+        location.setElapsedRealtimeNanos(SystemClock.elapsedRealtimeNanos());
+        location.setExtras(mLocationExtras.getBundle());
 
-            try {
-                mILocationManager.reportLocation(mLocation, false);
-            } catch (RemoteException e) {
-                Log.e(TAG, "RemoteException calling reportLocation");
-            }
+        try {
+            mILocationManager.reportLocation(location, false);
+        } catch (RemoteException e) {
+            Log.e(TAG, "RemoteException calling reportLocation");
         }
 
         mGnssMetrics.logReceivedLocationStatus(hasLatLong);
@@ -1835,54 +1841,73 @@ public class GnssLocationProvider implements LocationProviderInterface {
         }
     }
 
+    // Helper class to carry data to handler for reportSvStatus
+    private static class SvStatusInfo {
+        public int mSvCount;
+        public int[] mSvidWithFlags;
+        public float[] mCn0s;
+        public float[] mSvElevations;
+        public float[] mSvAzimuths;
+        public float[] mSvCarrierFreqs;
+    }
+
     /**
      * called from native code to update SV info
      */
-    private void reportSvStatus() {
-        int svCount = native_read_sv_status(mSvidWithFlags,
-                mCn0s,
-                mSvElevations,
-                mSvAzimuths,
-                mSvCarrierFreqs);
+    private void reportSvStatus(int svCount, int[] svidWithFlags, float[] cn0s,
+            float[] svElevations, float[] svAzimuths, float[] svCarrierFreqs) {
+        SvStatusInfo svStatusInfo = new SvStatusInfo();
+        svStatusInfo.mSvCount = svCount;
+        svStatusInfo.mSvidWithFlags = svidWithFlags;
+        svStatusInfo.mCn0s = cn0s;
+        svStatusInfo.mSvElevations = svElevations;
+        svStatusInfo.mSvAzimuths = svAzimuths;
+        svStatusInfo.mSvCarrierFreqs = svCarrierFreqs;
+
+        sendMessage(REPORT_SV_STATUS, 0, svStatusInfo);
+    }
+
+    private void handleReportSvStatus(SvStatusInfo info) {
         mListenerHelper.onSvStatusChanged(
-                svCount,
-                mSvidWithFlags,
-                mCn0s,
-                mSvElevations,
-                mSvAzimuths,
-                mSvCarrierFreqs);
+                info.mSvCount,
+                info.mSvidWithFlags,
+                info.mCn0s,
+                info.mSvElevations,
+                info.mSvAzimuths,
+                info.mSvCarrierFreqs);
 
         // Log CN0 as part of GNSS metrics
-        mGnssMetrics.logCn0(mCn0s, svCount);
+        mGnssMetrics.logCn0(info.mCn0s, info.mSvCount);
 
         if (VERBOSE) {
-            Log.v(TAG, "SV count: " + svCount);
+            Log.v(TAG, "SV count: " + info.mSvCount);
         }
         // Calculate number of satellites used in fix.
         int usedInFixCount = 0;
         int maxCn0 = 0;
         int meanCn0 = 0;
-        for (int i = 0; i < svCount; i++) {
-            if ((mSvidWithFlags[i] & GnssStatus.GNSS_SV_FLAGS_USED_IN_FIX) != 0) {
+        for (int i = 0; i < info.mSvCount; i++) {
+            if ((info.mSvidWithFlags[i] & GnssStatus.GNSS_SV_FLAGS_USED_IN_FIX) != 0) {
                 ++usedInFixCount;
-                if (mCn0s[i] > maxCn0) {
-                    maxCn0 = (int) mCn0s[i];
+                if (info.mCn0s[i] > maxCn0) {
+                    maxCn0 = (int) info.mCn0s[i];
                 }
-                meanCn0 += mCn0s[i];
+                meanCn0 += info.mCn0s[i];
             }
             if (VERBOSE) {
-                Log.v(TAG, "svid: " + (mSvidWithFlags[i] >> GnssStatus.SVID_SHIFT_WIDTH) +
-                        " cn0: " + mCn0s[i] +
-                        " elev: " + mSvElevations[i] +
-                        " azimuth: " + mSvAzimuths[i] +
-                        " carrier frequency: " + mSvCarrierFreqs[i] +
-                        ((mSvidWithFlags[i] & GnssStatus.GNSS_SV_FLAGS_HAS_EPHEMERIS_DATA) == 0
+                Log.v(TAG, "svid: " + (info.mSvidWithFlags[i] >> GnssStatus.SVID_SHIFT_WIDTH) +
+                        " cn0: " + info.mCn0s[i] +
+                        " elev: " + info.mSvElevations[i] +
+                        " azimuth: " + info.mSvAzimuths[i] +
+                        " carrier frequency: " + info.mSvCarrierFreqs[i] +
+                        ((info.mSvidWithFlags[i] & GnssStatus.GNSS_SV_FLAGS_HAS_EPHEMERIS_DATA) == 0
                                 ? "  " : " E") +
-                        ((mSvidWithFlags[i] & GnssStatus.GNSS_SV_FLAGS_HAS_ALMANAC_DATA) == 0
+                        ((info.mSvidWithFlags[i] & GnssStatus.GNSS_SV_FLAGS_HAS_ALMANAC_DATA) == 0
                                 ? "  " : " A") +
-                        ((mSvidWithFlags[i] & GnssStatus.GNSS_SV_FLAGS_USED_IN_FIX) == 0
+                        ((info.mSvidWithFlags[i] & GnssStatus.GNSS_SV_FLAGS_USED_IN_FIX) == 0
                                 ? "" : "U") +
-                        ((mSvidWithFlags[i] & GnssStatus.GNSS_SV_FLAGS_HAS_CARRIER_FREQUENCY) == 0
+                        ((info.mSvidWithFlags[i] &
+                                GnssStatus.GNSS_SV_FLAGS_HAS_CARRIER_FREQUENCY) == 0
                                 ? "" : "F"));
             }
         }
@@ -2479,6 +2504,12 @@ public class GnssLocationProvider implements LocationProviderInterface {
                 case INITIALIZE_HANDLER:
                     handleInitialize();
                     break;
+                case REPORT_LOCATION:
+                    handleReportLocation(msg.arg1 == 1, (Location) msg.obj);
+                    break;
+                case REPORT_SV_STATUS:
+                    handleReportSvStatus((SvStatusInfo) msg.obj);
+                    break;
             }
             if (msg.arg2 == 1) {
                 // wakelock was taken for this message, release it
@@ -2798,6 +2829,10 @@ public class GnssLocationProvider implements LocationProviderInterface {
                 return "SUBSCRIPTION_OR_SIM_CHANGED";
             case INITIALIZE_HANDLER:
                 return "INITIALIZE_HANDLER";
+            case REPORT_LOCATION:
+                return "REPORT_LOCATION";
+            case REPORT_SV_STATUS:
+                return "REPORT_SV_STATUS";
             default:
                 return "<Unknown>";
         }
@@ -2862,15 +2897,6 @@ public class GnssLocationProvider implements LocationProviderInterface {
         }
     }
 
-    // for GPS SV statistics
-    private static final int MAX_SVS = 64;
-
-    // preallocated arrays, to avoid memory allocation in reportStatus()
-    private int mSvidWithFlags[] = new int[MAX_SVS];
-    private float mCn0s[] = new float[MAX_SVS];
-    private float mSvElevations[] = new float[MAX_SVS];
-    private float mSvAzimuths[] = new float[MAX_SVS];
-    private float mSvCarrierFreqs[] = new float[MAX_SVS];
     // preallocated to avoid memory allocation in reportNmea()
     private byte[] mNmeaBuffer = new byte[120];
 
@@ -2899,11 +2925,6 @@ public class GnssLocationProvider implements LocationProviderInterface {
 
     private native void native_delete_aiding_data(int flags);
 
-    // returns number of SVs
-    // mask[0] is ephemeris mask and mask[1] is almanac mask
-    private native int native_read_sv_status(int[] prnWithFlags, float[] cn0s, float[] elevations,
-            float[] azimuths, float[] carrierFrequencies);
-
     private native int native_read_nmea(byte[] buffer, int bufferSize);
 
     private native void native_inject_best_location(
diff --git a/services/core/jni/com_android_server_location_GnssLocationProvider.cpp b/services/core/jni/com_android_server_location_GnssLocationProvider.cpp
index 266f89b063b3..0a1da5719d3e 100644
--- a/services/core/jni/com_android_server_location_GnssLocationProvider.cpp
+++ b/services/core/jni/com_android_server_location_GnssLocationProvider.cpp
@@ -395,10 +395,7 @@ struct GnssCallback : public IGnssCallback {
     // New in 1.1
     Return<void> gnssNameCb(const android::hardware::hidl_string& name) override;
 
-    static GnssSvInfo sGnssSvList[static_cast<uint32_t>(
-            android::hardware::gnss::V1_0::GnssMax::SVS_COUNT)];
-    static size_t sGnssSvListSize;
-
+    // TODO(b/73306084): Reconsider allocation cost vs threadsafety on these statics
     static const char* sNmeaString;
     static size_t sNmeaStringLength;
 };
@@ -414,11 +411,8 @@ Return<void> GnssCallback::gnssNameCb(const android::hardware::hidl_string& name
     return Void();
 }
 
-IGnssCallback::GnssSvInfo GnssCallback::sGnssSvList[static_cast<uint32_t>(
-        android::hardware::gnss::V1_0::GnssMax::SVS_COUNT)];
 const char* GnssCallback::sNmeaString = nullptr;
 size_t GnssCallback::sNmeaStringLength = 0;
-size_t GnssCallback::sGnssSvListSize = 0;
 
 Return<void> GnssCallback::gnssLocationCb(const GnssLocation& location) {
     JNIEnv* env = getJniEnv();
@@ -432,6 +426,7 @@ Return<void> GnssCallback::gnssLocationCb(const GnssLocation& location) {
                         boolToJbool(hasLatLong),
                         jLocation);
     checkAndClearExceptionFromCallback(env, __FUNCTION__);
+    env->DeleteLocalRef(jLocation);
     return Void();
 }
 
@@ -445,20 +440,55 @@ Return<void> GnssCallback::gnssStatusCb(const IGnssCallback::GnssStatusValue sta
 Return<void> GnssCallback::gnssSvStatusCb(const IGnssCallback::GnssSvStatus& svStatus) {
     JNIEnv* env = getJniEnv();
 
-    sGnssSvListSize = svStatus.numSvs;
-    if (sGnssSvListSize > static_cast<uint32_t>(
+    uint32_t listSize = svStatus.numSvs;
+    if (listSize > static_cast<uint32_t>(
             android::hardware::gnss::V1_0::GnssMax::SVS_COUNT)) {
-        ALOGD("Too many satellites %zd. Clamps to %u.", sGnssSvListSize,
+        ALOGD("Too many satellites %u. Clamps to %u.", listSize,
               static_cast<uint32_t>(android::hardware::gnss::V1_0::GnssMax::SVS_COUNT));
-        sGnssSvListSize = static_cast<uint32_t>(android::hardware::gnss::V1_0::GnssMax::SVS_COUNT);
+        listSize = static_cast<uint32_t>(android::hardware::gnss::V1_0::GnssMax::SVS_COUNT);
     }
 
-    // Copy GNSS SV info into sGnssSvList, if any.
-    if (svStatus.numSvs > 0) {
-        memcpy(sGnssSvList, svStatus.gnssSvList.data(), sizeof(GnssSvInfo) * sGnssSvListSize);
+    jintArray svidWithFlagArray = env->NewIntArray(listSize);
+    jfloatArray cn0Array = env->NewFloatArray(listSize);
+    jfloatArray elevArray = env->NewFloatArray(listSize);
+    jfloatArray azimArray = env->NewFloatArray(listSize);
+    jfloatArray carrierFreqArray = env->NewFloatArray(listSize);
+
+    jint* svidWithFlags = env->GetIntArrayElements(svidWithFlagArray, 0);
+    jfloat* cn0s = env->GetFloatArrayElements(cn0Array, 0);
+    jfloat* elev = env->GetFloatArrayElements(elevArray, 0);
+    jfloat* azim = env->GetFloatArrayElements(azimArray, 0);
+    jfloat* carrierFreq = env->GetFloatArrayElements(carrierFreqArray, 0);
+
+    /*
+     * Read GNSS SV info.
+     */
+    for (size_t i = 0; i < listSize; ++i) {
+        enum ShiftWidth: uint8_t {
+            SVID_SHIFT_WIDTH = 8,
+            CONSTELLATION_TYPE_SHIFT_WIDTH = 4
+        };
+
+        const IGnssCallback::GnssSvInfo& info = svStatus.gnssSvList.data()[i];
+        svidWithFlags[i] = (info.svid << SVID_SHIFT_WIDTH) |
+            (static_cast<uint32_t>(info.constellation) << CONSTELLATION_TYPE_SHIFT_WIDTH) |
+            static_cast<uint32_t>(info.svFlag);
+        cn0s[i] = info.cN0Dbhz;
+        elev[i] = info.elevationDegrees;
+        azim[i] = info.azimuthDegrees;
+        carrierFreq[i] = info.carrierFrequencyHz;
     }
 
-    env->CallVoidMethod(mCallbacksObj, method_reportSvStatus);
+    env->ReleaseIntArrayElements(svidWithFlagArray, svidWithFlags, 0);
+    env->ReleaseFloatArrayElements(cn0Array, cn0s, 0);
+    env->ReleaseFloatArrayElements(elevArray, elev, 0);
+    env->ReleaseFloatArrayElements(azimArray, azim, 0);
+    env->ReleaseFloatArrayElements(carrierFreqArray, carrierFreq, 0);
+
+    env->CallVoidMethod(mCallbacksObj, method_reportSvStatus,
+            static_cast<jint>(listSize), svidWithFlagArray, cn0Array, elevArray, azimArray,
+            carrierFreqArray);
+
     checkAndClearExceptionFromCallback(env, __FUNCTION__);
     return Void();
 }
@@ -577,6 +607,7 @@ Return<void> GnssGeofenceCallback::gnssGeofenceTransitionCb(
                         timestamp);
 
     checkAndClearExceptionFromCallback(env, __FUNCTION__);
+    env->DeleteLocalRef(jLocation);
     return Void();
 }
 
@@ -592,6 +623,7 @@ Return<void> GnssGeofenceCallback::gnssGeofenceStatusCb(
                         status,
                         jLocation);
     checkAndClearExceptionFromCallback(env, __FUNCTION__);
+    env->DeleteLocalRef(jLocation);
     return Void();
 }
 
@@ -1086,7 +1118,7 @@ static void android_location_GnssLocationProvider_class_init_native(JNIEnv* env,
     method_reportLocation = env->GetMethodID(clazz, "reportLocation",
             "(ZLandroid/location/Location;)V");
     method_reportStatus = env->GetMethodID(clazz, "reportStatus", "(I)V");
-    method_reportSvStatus = env->GetMethodID(clazz, "reportSvStatus", "()V");
+    method_reportSvStatus = env->GetMethodID(clazz, "reportSvStatus", "(I[I[F[F[F[F)V");
     method_reportAGpsStatus = env->GetMethodID(clazz, "reportAGpsStatus", "(II[B)V");
     method_reportNmea = env->GetMethodID(clazz, "reportNmea", "(J)V");
     method_setEngineCapabilities = env->GetMethodID(clazz, "setEngineCapabilities", "(I)V");
@@ -1392,49 +1424,6 @@ static void android_location_GnssLocationProvider_delete_aiding_data(JNIEnv* /*
     }
 }
 
-/*
- * This enum is used by the read_sv_status method to combine the svid,
- * constellation and svFlag fields.
- */
-enum ShiftWidth: uint8_t {
-    SVID_SHIFT_WIDTH = 8,
-    CONSTELLATION_TYPE_SHIFT_WIDTH = 4
-};
-
-static jint android_location_GnssLocationProvider_read_sv_status(JNIEnv* env, jobject /* obj */,
-        jintArray svidWithFlagArray, jfloatArray cn0Array, jfloatArray elevArray,
-        jfloatArray azumArray, jfloatArray carrierFreqArray) {
-    /*
-     * This method should only be called from within a call to reportSvStatus.
-     */
-    jint* svidWithFlags = env->GetIntArrayElements(svidWithFlagArray, 0);
-    jfloat* cn0s = env->GetFloatArrayElements(cn0Array, 0);
-    jfloat* elev = env->GetFloatArrayElements(elevArray, 0);
-    jfloat* azim = env->GetFloatArrayElements(azumArray, 0);
-    jfloat* carrierFreq = env->GetFloatArrayElements(carrierFreqArray, 0);
-
-    /*
-     * Read GNSS SV info.
-     */
-    for (size_t i = 0; i < GnssCallback::sGnssSvListSize; ++i) {
-        const IGnssCallback::GnssSvInfo& info = GnssCallback::sGnssSvList[i];
-        svidWithFlags[i] = (info.svid << SVID_SHIFT_WIDTH) |
-            (static_cast<uint32_t>(info.constellation) << CONSTELLATION_TYPE_SHIFT_WIDTH) |
-            static_cast<uint32_t>(info.svFlag);
-        cn0s[i] = info.cN0Dbhz;
-        elev[i] = info.elevationDegrees;
-        azim[i] = info.azimuthDegrees;
-        carrierFreq[i] = info.carrierFrequencyHz;
-    }
-
-    env->ReleaseIntArrayElements(svidWithFlagArray, svidWithFlags, 0);
-    env->ReleaseFloatArrayElements(cn0Array, cn0s, 0);
-    env->ReleaseFloatArrayElements(elevArray, elev, 0);
-    env->ReleaseFloatArrayElements(azumArray, azim, 0);
-    env->ReleaseFloatArrayElements(carrierFreqArray, carrierFreq, 0);
-    return static_cast<jint>(GnssCallback::sGnssSvListSize);
-}
-
 static void android_location_GnssLocationProvider_agps_set_reference_location_cellid(
         JNIEnv* /* env */, jobject /* obj */, jint type, jint mcc, jint mnc, jint lac, jint cid) {
     IAGnssRil::AGnssRefLocation location;
@@ -2080,9 +2069,6 @@ static const JNINativeMethod sMethods[] = {
     {"native_delete_aiding_data",
             "(I)V",
             reinterpret_cast<void*>(android_location_GnssLocationProvider_delete_aiding_data)},
-    {"native_read_sv_status",
-            "([I[F[F[F[F)I",
-            reinterpret_cast<void *>(android_location_GnssLocationProvider_read_sv_status)},
     {"native_read_nmea", "([BI)I", reinterpret_cast<void *>(
             android_location_GnssLocationProvider_read_nmea)},
     {"native_inject_time", "(JJI)V", reinterpret_cast<void *>(