From 26465d267aa8ff969d359b575559d9954ebb607d Mon Sep 17 00:00:00 2001 From: Wyatt Riley Date: Mon, 12 Feb 2018 13:44:24 -0800 Subject: [PATCH] Handling concurrent location & SV status update Move handling of location and SV status updates to existing handler thread. Change SV status JNI output for thread safety. Synchronize updates to LocationExtras (many entry points) Adding some DeleteLocalRef cleanups Bug: 72481417 Fixes: 72949246 Fixes: 73090867 Test: GPS works on device, and adb shell dumpsys location shows extras Change-Id: I3eda8884597ad1cdf5aec6d1b7c33ab49401410f --- .../server/location/GnssLocationProvider.java | 149 ++++++++++-------- ...d_server_location_GnssLocationProvider.cpp | 110 ++++++------- 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 9d2a8e28a3a06..0dab528bb99d6 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 ""; } @@ -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 c2771e4b931f9..0c11bc110c77f 100644 --- a/services/core/jni/com_android_server_location_GnssLocationProvider.cpp +++ b/services/core/jni/com_android_server_location_GnssLocationProvider.cpp @@ -393,10 +393,7 @@ struct GnssCallback : public IGnssCallback { // New in 1.1 Return gnssNameCb(const android::hardware::hidl_string& name) override; - static GnssSvInfo sGnssSvList[static_cast( - 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; }; @@ -412,11 +409,8 @@ Return GnssCallback::gnssNameCb(const android::hardware::hidl_string& name return Void(); } -IGnssCallback::GnssSvInfo GnssCallback::sGnssSvList[static_cast( - android::hardware::gnss::V1_0::GnssMax::SVS_COUNT)]; const char* GnssCallback::sNmeaString = nullptr; size_t GnssCallback::sNmeaStringLength = 0; -size_t GnssCallback::sGnssSvListSize = 0; Return GnssCallback::gnssLocationCb(const GnssLocation& location) { JNIEnv* env = getJniEnv(); @@ -430,6 +424,7 @@ Return GnssCallback::gnssLocationCb(const GnssLocation& location) { boolToJbool(hasLatLong), jLocation); checkAndClearExceptionFromCallback(env, __FUNCTION__); + env->DeleteLocalRef(jLocation); return Void(); } @@ -443,20 +438,55 @@ Return GnssCallback::gnssStatusCb(const IGnssCallback::GnssStatusValue sta Return GnssCallback::gnssSvStatusCb(const IGnssCallback::GnssSvStatus& svStatus) { JNIEnv* env = getJniEnv(); - sGnssSvListSize = svStatus.numSvs; - if (sGnssSvListSize > static_cast( + uint32_t listSize = svStatus.numSvs; + if (listSize > static_cast( 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(android::hardware::gnss::V1_0::GnssMax::SVS_COUNT)); - sGnssSvListSize = static_cast(android::hardware::gnss::V1_0::GnssMax::SVS_COUNT); + listSize = static_cast(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(info.constellation) << CONSTELLATION_TYPE_SHIFT_WIDTH) | + static_cast(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(listSize), svidWithFlagArray, cn0Array, elevArray, azimArray, + carrierFreqArray); + checkAndClearExceptionFromCallback(env, __FUNCTION__); return Void(); } @@ -575,6 +605,7 @@ Return GnssGeofenceCallback::gnssGeofenceTransitionCb( timestamp); checkAndClearExceptionFromCallback(env, __FUNCTION__); + env->DeleteLocalRef(jLocation); return Void(); } @@ -590,6 +621,7 @@ Return GnssGeofenceCallback::gnssGeofenceStatusCb( status, jLocation); checkAndClearExceptionFromCallback(env, __FUNCTION__); + env->DeleteLocalRef(jLocation); return Void(); } @@ -1063,7 +1095,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"); @@ -1369,49 +1401,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(info.constellation) << CONSTELLATION_TYPE_SHIFT_WIDTH) | - static_cast(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(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; @@ -2057,9 +2046,6 @@ static const JNINativeMethod sMethods[] = { {"native_delete_aiding_data", "(I)V", reinterpret_cast(android_location_GnssLocationProvider_delete_aiding_data)}, - {"native_read_sv_status", - "([I[F[F[F[F)I", - reinterpret_cast(android_location_GnssLocationProvider_read_sv_status)}, {"native_read_nmea", "([BI)I", reinterpret_cast( android_location_GnssLocationProvider_read_nmea)}, {"native_inject_time", "(JJI)V", reinterpret_cast(