Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 04f7f34a authored by Sundeep Ghuman's avatar Sundeep Ghuman
Browse files

Remove double cache eviction logic.

WifiTracker and AccessPoint both maintain their own caches of
ScanResults and dictate their own grouping logic. This leads to various
issues, such as dropping APs during network selection. Remove the double
cache eviction logic, making WifiTracker the source of truth for
evicting and grouping ScanResults.

Consolidate on AccessPoint time based eviction logic. In place swap of
logic, with subsequent wifi tracker clean up and cache rekeying to
follow in later CLs. Logic was left as close to original location for
reviewer ease and will be rearranged in following CLs.

Moves existing key generation logic from AccessPointPreference into
AccessPoint.java

BUG: 64989100
Test: runtest runtest --path
frameworks/base/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/AccessPointTest.java
runtest --path
frameworks/base/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/WifiTrackerTest.java
make RunSettingsLibRoboTests
make RunSettingsRoboTests
Manual inspection of WifiSettings jitter when changing networks / moving
across floors.

Change-Id: Id30a34b08e14fc8da2c9d29dfcb5d1e8973cc18c
parent aa31e19a
Loading
Loading
Loading
Loading
+135 −102
Original line number Diff line number Diff line
@@ -32,7 +32,6 @@ import android.net.NetworkKey;
import android.net.NetworkScoreManager;
import android.net.NetworkScorerAppData;
import android.net.ScoredNetwork;
import android.net.WifiKey;
import android.net.wifi.IWifiManager;
import android.net.wifi.ScanResult;
import android.net.wifi.WifiConfiguration;
@@ -43,6 +42,7 @@ import android.net.wifi.WifiManager;
import android.net.wifi.WifiNetworkScoreCache;
import android.net.wifi.hotspot2.PasspointConfiguration;
import android.os.Bundle;
import android.os.Parcelable;
import android.os.RemoteException;
import android.os.ServiceManager;
import android.os.SystemClock;
@@ -52,6 +52,7 @@ import android.text.Spannable;
import android.text.SpannableString;
import android.text.TextUtils;
import android.text.style.TtsSpan;
import android.util.ArraySet;
import android.util.Log;

import com.android.internal.annotations.VisibleForTesting;
@@ -60,11 +61,11 @@ import com.android.settingslib.R;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashMap;
import java.util.Iterator;
import java.util.Map;
import java.util.Objects;
import java.util.concurrent.ConcurrentHashMap;
import java.util.Set;
import java.util.concurrent.atomic.AtomicInteger;


@@ -91,6 +92,9 @@ public class AccessPoint implements Comparable<AccessPoint> {
     */
    public static final int HIGHER_FREQ_5GHZ = 5900;

    /** The key which identifies this AccessPoint grouping. */
    private String mKey;

    @IntDef({Speed.NONE, Speed.SLOW, Speed.MODERATE, Speed.FAST, Speed.VERY_FAST})
    @Retention(RetentionPolicy.SOURCE)
    public @interface Speed {
@@ -116,14 +120,8 @@ public class AccessPoint implements Comparable<AccessPoint> {
        int VERY_FAST = 30;
    }

    /**
     * Experimental: we should be able to show the user the list of BSSIDs and bands
     *  for that SSID.
     *  For now this data is used only with Verbose Logging so as to show the band and number
     *  of BSSIDs on which that network is seen.
     */
    private final ConcurrentHashMap<String, ScanResult> mScanResultCache =
            new ConcurrentHashMap<String, ScanResult>(32);
    /** The underlying set of scan results comprising this AccessPoint. */
    private final ArraySet<ScanResult> mScanResults = new ArraySet<>();

    /**
     * Map of BSSIDs to scored networks for individual bssids.
@@ -133,17 +131,13 @@ public class AccessPoint implements Comparable<AccessPoint> {
     */
    private final Map<String, TimestampedScoredNetwork> mScoredNetworkCache = new HashMap<>();

    /** Maximum age of scan results to hold onto while actively scanning. **/
    private static final long MAX_SCAN_RESULT_AGE_MILLIS = 25000;

    static final String KEY_NETWORKINFO = "key_networkinfo";
    static final String KEY_WIFIINFO = "key_wifiinfo";
    static final String KEY_SCANRESULT = "key_scanresult";
    static final String KEY_SSID = "key_ssid";
    static final String KEY_SECURITY = "key_security";
    static final String KEY_SPEED = "key_speed";
    static final String KEY_PSKTYPE = "key_psktype";
    static final String KEY_SCANRESULTCACHE = "key_scanresultcache";
    static final String KEY_SCANRESULTS = "key_scanresults";
    static final String KEY_SCOREDNETWORKCACHE = "key_scorednetworkcache";
    static final String KEY_CONFIG = "key_config";
    static final String KEY_FQDN = "key_fqdn";
@@ -216,7 +210,10 @@ public class AccessPoint implements Comparable<AccessPoint> {

    public AccessPoint(Context context, Bundle savedState) {
        mContext = context;

        if (savedState.containsKey(KEY_CONFIG)) {
            mConfig = savedState.getParcelable(KEY_CONFIG);
        }
        if (mConfig != null) {
            loadConfig(mConfig);
        }
@@ -236,12 +233,11 @@ public class AccessPoint implements Comparable<AccessPoint> {
        if (savedState.containsKey(KEY_NETWORKINFO)) {
            mNetworkInfo = savedState.getParcelable(KEY_NETWORKINFO);
        }
        if (savedState.containsKey(KEY_SCANRESULTCACHE)) {
            ArrayList<ScanResult> scanResultArrayList =
                    savedState.getParcelableArrayList(KEY_SCANRESULTCACHE);
            mScanResultCache.clear();
            for (ScanResult result : scanResultArrayList) {
                mScanResultCache.put(result.BSSID, result);
        if (savedState.containsKey(KEY_SCANRESULTS)) {
            Parcelable[] scanResults = savedState.getParcelableArray(KEY_SCANRESULTS);
            mScanResults.clear();
            for (Parcelable result : scanResults) {
                mScanResults.add((ScanResult) result);
            }
        }
        if (savedState.containsKey(KEY_SCOREDNETWORKCACHE)) {
@@ -268,8 +264,10 @@ public class AccessPoint implements Comparable<AccessPoint> {
        }
        update(mConfig, mInfo, mNetworkInfo);

        // Do not evict old scan results on initial creation
        // Calculate required fields
        updateKey();
        updateRssi();

        mId = sLastId.incrementAndGet();
    }

@@ -295,30 +293,75 @@ public class AccessPoint implements Comparable<AccessPoint> {
        copyFrom(other);
    }

    AccessPoint(Context context, ScanResult result) {
    AccessPoint(Context context, Collection<ScanResult> results) {
        mContext = context;
        initWithScanResult(result);

        if (results.isEmpty()) {
            throw new IllegalArgumentException("Cannot construct with an empty ScanResult list");
        }
        mScanResults.addAll(results);

        // Information derived from scan results
        ScanResult firstResult = results.iterator().next();
        ssid = firstResult.SSID;
        bssid = firstResult.BSSID;
        security = getSecurity(firstResult);
        if (security == SECURITY_PSK) {
            pskType = getPskType(firstResult);
        }
        updateKey();
        updateRssi();

        // Passpoint Info
        mIsCarrierAp = firstResult.isCarrierAp;
        mCarrierApEapType = firstResult.carrierApEapType;
        mCarrierName = firstResult.carrierName;

        mId = sLastId.incrementAndGet();
    }

    @VisibleForTesting void loadConfig(WifiConfiguration config) {
        ssid = (config.SSID == null ? "" : removeDoubleQuotes(config.SSID));
        bssid = config.BSSID;
        security = getSecurity(config);
        updateKey();
        networkId = config.networkId;
        mConfig = config;
    }

    /** Updates {@link #mKey} and should only called upon object creation/initialization. */
    private void updateKey() {
        // TODO(sghuman): Consolidate Key logic on ScanResultMatchInfo

        StringBuilder builder = new StringBuilder();

        if (TextUtils.isEmpty(getSsidStr())) {
            builder.append(getBssid());
        } else {
            builder.append(getSsidStr());
        }

        builder.append(',').append(getSecurity());
        mKey = builder.toString();
    }

    /**
     * Copy accesspoint information. NOTE: We do not copy tag information because that is never
     * set on the internal copy.
     * @param that
     */
    void copyFrom(AccessPoint that) {
        that.evictOldScanResults();
        this.ssid = that.ssid;
        this.bssid = that.bssid;
        this.security = that.security;
        this.mKey = that.mKey;
        this.networkId = that.networkId;
        this.pskType = that.pskType;
        this.mConfig = that.mConfig; //TODO: Watch out, this object is mutated.
        this.mRssi = that.mRssi;
        this.mInfo = that.mInfo;
        this.mNetworkInfo = that.mNetworkInfo;
        this.mScanResultCache.clear();
        this.mScanResultCache.putAll(that.mScanResultCache);
        this.mScanResults.clear();
        this.mScanResults.addAll(that.mScanResults);
        this.mScoredNetworkCache.clear();
        this.mScoredNetworkCache.putAll(that.mScoredNetworkCache);
        this.mId = that.mId;
@@ -426,7 +469,7 @@ public class AccessPoint implements Comparable<AccessPoint> {

        if (WifiTracker.sVerboseLogging) {
            builder.append(",rssi=").append(mRssi);
            builder.append(",scan cache size=").append(mScanResultCache.size());
            builder.append(",scan cache size=").append(mScanResults.size());
        }

        return builder.append(')').toString();
@@ -468,7 +511,7 @@ public class AccessPoint implements Comparable<AccessPoint> {
     */
    private boolean updateScores(WifiNetworkScoreCache scoreCache, long maxScoreCacheAgeMillis) {
        long nowMillis = SystemClock.elapsedRealtime();
        for (ScanResult result : mScanResultCache.values()) {
        for (ScanResult result : mScanResults) {
            ScoredNetwork score = scoreCache.getScoredNetwork(result);
            if (score == null) {
                continue;
@@ -555,7 +598,7 @@ public class AccessPoint implements Comparable<AccessPoint> {
                mIsScoredNetworkMetered |= score.meteredHint;
            }
        } else {
            for (ScanResult result : mScanResultCache.values()) {
            for (ScanResult result : mScanResults) {
                ScoredNetwork score = scoreCache.getScoredNetwork(result);
                if (score == null) {
                    continue;
@@ -566,19 +609,21 @@ public class AccessPoint implements Comparable<AccessPoint> {
        return oldMetering == mIsScoredNetworkMetered;
    }

    private void evictOldScanResults() {
        long nowMs = SystemClock.elapsedRealtime();
        for (Iterator<ScanResult> iter = mScanResultCache.values().iterator(); iter.hasNext(); ) {
            ScanResult result = iter.next();
            // result timestamp is in microseconds
            if (nowMs - result.timestamp / 1000 > MAX_SCAN_RESULT_AGE_MILLIS) {
                iter.remove();
            }
    public static String getKey(ScanResult result) {
        StringBuilder builder = new StringBuilder();

        if (TextUtils.isEmpty(result.SSID)) {
            builder.append(result.BSSID);
        } else {
            builder.append(result.SSID);
        }

        builder.append(',').append(getSecurity(result));
        return builder.toString();
    }

    public boolean matches(ScanResult result) {
        return ssid.equals(result.SSID) && security == getSecurity(result);
    public String getKey() {
        return mKey;
    }

    public boolean matches(WifiConfiguration config) {
@@ -622,9 +667,12 @@ public class AccessPoint implements Comparable<AccessPoint> {
        return mRssi;
    }

    public ConcurrentHashMap<String, ScanResult> getScanResults() {
        return mScanResultCache;
    }
    /**
     * Returns the underlying scan result set.
     *
     * <p>Callers should not modify this set.
     */
    public Set<ScanResult> getScanResults() { return mScanResults; }

    public Map<String, TimestampedScoredNetwork> getScoredNetworkCache() {
        return mScoredNetworkCache;
@@ -645,7 +693,7 @@ public class AccessPoint implements Comparable<AccessPoint> {
        }

        int rssi = UNREACHABLE_RSSI;
        for (ScanResult result : mScanResultCache.values()) {
        for (ScanResult result : mScanResults) {
            if (result.level > rssi) {
                rssi = result.level;
            }
@@ -853,7 +901,6 @@ public class AccessPoint implements Comparable<AccessPoint> {
        }

        if (WifiTracker.sVerboseLogging) {
            evictOldScanResults();
            summary.append(WifiUtils.buildLoggingSummary(this, config));
        }

@@ -950,28 +997,6 @@ public class AccessPoint implements Comparable<AccessPoint> {
        mConfig.allowedKeyManagement.set(KeyMgmt.NONE);
    }

    void loadConfig(WifiConfiguration config) {
        ssid = (config.SSID == null ? "" : removeDoubleQuotes(config.SSID));
        bssid = config.BSSID;
        security = getSecurity(config);
        networkId = config.networkId;
        mConfig = config;
    }

    private void initWithScanResult(ScanResult result) {
        ssid = result.SSID;
        bssid = result.BSSID;
        security = getSecurity(result);
        if (security == SECURITY_PSK)
            pskType = getPskType(result);

        mScanResultCache.put(result.BSSID, result);
        updateRssi();
        mIsCarrierAp = result.isCarrierAp;
        mCarrierApEapType = result.carrierApEapType;
        mCarrierName = result.carrierName;
    }

    public void saveWifiState(Bundle savedState) {
        if (ssid != null) savedState.putString(KEY_SSID, getSsidStr());
        savedState.putInt(KEY_SECURITY, security);
@@ -979,9 +1004,8 @@ public class AccessPoint implements Comparable<AccessPoint> {
        savedState.putInt(KEY_PSKTYPE, pskType);
        if (mConfig != null) savedState.putParcelable(KEY_CONFIG, mConfig);
        savedState.putParcelable(KEY_WIFIINFO, mInfo);
        evictOldScanResults();
        savedState.putParcelableArrayList(KEY_SCANRESULTCACHE,
                new ArrayList<ScanResult>(mScanResultCache.values()));
        savedState.putParcelableArray(KEY_SCANRESULTS,
                mScanResults.toArray(new Parcelable[mScanResults.size()]));
        savedState.putParcelableArrayList(KEY_SCOREDNETWORKCACHE,
                new ArrayList<>(mScoredNetworkCache.values()));
        if (mNetworkInfo != null) {
@@ -1003,24 +1027,32 @@ public class AccessPoint implements Comparable<AccessPoint> {
    }

    /**
     * Update the AP with the given scan result.
     * Sets {@link #mScanResults} to the given collection.
     *
     * @param result the ScanResult to add to the AccessPoint scan cache
     * @param evictOldScanResults whether stale scan results should be removed
     *         from the cache during this update process
     * @return true if the scan result update caused a change in state which would impact ranking
     *     or AccessPoint rendering (e.g. wifi level, security)
     * @param scanResults a collection of scan results to add to the internal set
     * @throws IllegalArgumentException if any of the given ScanResults did not belong to this AP
     */
    boolean update(ScanResult result, boolean evictOldScanResults) {
        if (matches(result)) {
            int oldLevel = getLevel();
    void setScanResults(Collection<ScanResult> scanResults) {

        // Validate scan results are for current AP only
        String key = getKey();
        for (ScanResult result : scanResults) {
            String scanResultKey = AccessPoint.getKey(result);
            if (!mKey.equals(scanResultKey)) {
                throw new IllegalArgumentException(
                        String.format("ScanResult %s\nkey of %s did not match current AP key %s",
                                      result, scanResultKey, key));
            }
        }


            /* Add or update the scan result for the BSSID */
            mScanResultCache.put(result.BSSID, result);
            if (evictOldScanResults) evictOldScanResults();
        int oldLevel = getLevel();
        mScanResults.clear();
        mScanResults.addAll(scanResults);
        updateRssi();
        int newLevel = getLevel();

        // If newLevel is 0, there will be no displayed Preference since the AP is unreachable
        if (newLevel > 0 && newLevel != oldLevel) {
            // Only update labels on visible rssi changes
            updateSpeed();
@@ -1028,24 +1060,25 @@ public class AccessPoint implements Comparable<AccessPoint> {
                mAccessPointListener.onLevelChanged(this);
            }
        }
            // This flag only comes from scans, is not easily saved in config
            if (security == SECURITY_PSK) {
                pskType = getPskType(result);
            }

        if (mAccessPointListener != null) {
            mAccessPointListener.onAccessPointChanged(this);
        }

        if (!scanResults.isEmpty()) {
            ScanResult result = scanResults.iterator().next();

            // This flag only comes from scans, is not easily saved in config
            if (security == SECURITY_PSK) {
                pskType = getPskType(result);
            }

            // The carrier info in the ScanResult is set by the platform based on the SSID and will
            // always be the same for all matching scan results.
            mIsCarrierAp = result.isCarrierAp;
            mCarrierApEapType = result.carrierApEapType;
            mCarrierName = result.carrierName;

            return true;
        }
        return false;
    }

    /** Attempt to update the AccessPoint and return true if an update occurred. */
+0 −13
Original line number Diff line number Diff line
@@ -77,19 +77,6 @@ public class AccessPointPreference extends TwoTargetPreference {
    private int mDefaultIconResId;
    private int mWifiSpeed = Speed.NONE;

    public static String generatePreferenceKey(AccessPoint accessPoint) {
        StringBuilder builder = new StringBuilder();

        if (TextUtils.isEmpty(accessPoint.getSsidStr())) {
            builder.append(accessPoint.getBssid());
        } else {
            builder.append(accessPoint.getSsidStr());
        }

        builder.append(',').append(accessPoint.getSecurity());
        return builder.toString();
    }

    @Nullable
    private static StateListDrawable getFrictionStateListDrawable(Context context) {
        TypedArray frictionSld;
+7 −5
Original line number Diff line number Diff line
@@ -23,6 +23,7 @@ import android.net.wifi.ScanResult;
import android.net.wifi.WifiConfiguration;
import android.net.wifi.WifiInfo;
import android.os.Bundle;
import android.os.Parcelable;
import android.support.annotation.Keep;

import com.android.settingslib.wifi.AccessPoint.Speed;
@@ -58,7 +59,7 @@ public class TestAccessPointBuilder {
    private String mCarrierName = null;

    Context mContext;
    private ArrayList<ScanResult> mScanResultCache;
    private ArrayList<ScanResult> mScanResults;
    private ArrayList<TimestampedScoredNetwork> mScoredNetworkCache;

    @Keep
@@ -84,8 +85,9 @@ public class TestAccessPointBuilder {
        if (mProviderFriendlyName != null) {
            bundle.putString(AccessPoint.KEY_PROVIDER_FRIENDLY_NAME, mProviderFriendlyName);
        }
        if (mScanResultCache != null) {
            bundle.putParcelableArrayList(AccessPoint.KEY_SCANRESULTCACHE, mScanResultCache);
        if (mScanResults != null) {
            bundle.putParcelableArray(AccessPoint.KEY_SCANRESULTS,
                    mScanResults.toArray(new Parcelable[mScanResults.size()]));
        }
        if (mScoredNetworkCache != null) {
            bundle.putParcelableArrayList(AccessPoint.KEY_SCOREDNETWORKCACHE, mScoredNetworkCache);
@@ -229,8 +231,8 @@ public class TestAccessPointBuilder {
        return this;
    }

    public TestAccessPointBuilder setScanResultCache(ArrayList<ScanResult> scanResultCache) {
        mScanResultCache = scanResultCache;
    public TestAccessPointBuilder setScanResults(ArrayList<ScanResult> scanResults) {
        mScanResults = scanResults;
        return this;
    }

+72 −34

File changed.

Preview size limit exceeded, changes collapsed.

+1 −1
Original line number Diff line number Diff line
@@ -109,7 +109,7 @@ public class WifiUtils {

        // TODO: sort list by RSSI or age
        long nowMs = SystemClock.elapsedRealtime();
        for (ScanResult result : accessPoint.getScanResults().values()) {
        for (ScanResult result : accessPoint.getScanResults()) {
            if (result.frequency >= AccessPoint.LOWER_FREQ_5GHZ
                    && result.frequency <= AccessPoint.HIGHER_FREQ_5GHZ) {
                // Strictly speaking: [4915, 5825]
Loading