Wificond: Address bugs in handling Offload scans

This CL addresses the following issues in handling of Offload HAL scan
results from the Offload HAL service
- Make cached scan results member variable a pointer so that the memory
for the vector can be allocated on the heap
- When an Async Error occurs in the Offload HAL service, switch to
performing PNO scans over netlink instead of letting it decide again.
- OffloadScanUtils convertToNativeScanResults() now needs to take in a
pointer to the vector that will store the scan results for retrieval.
- Populate tsf field fo the scanResult so that it doesn't get filtered
out by the framework
- Add logging to scan stats

Bug: 63148974
Test: Unit tests, on-device testing for ensuring we connect to an
available access point from screen off disconnected mode.
Change-Id: Ida507d857faa8ea6dbee362cf0116f8ca858963f

diff --git a/scanning/offload/offload_scan_manager.cpp b/scanning/offload/offload_scan_manager.cpp
index 420c2fa..2592aee 100644 (file)
--- a/scanning/offload/offload_scan_manager.cpp
+++ b/scanning/offload/offload_scan_manager.cpp
@@ -76,6 +76,7 @@ OffloadScanManager::OffloadScanManager(
       wifi_offload_callback_(nullptr),
       death_recipient_(nullptr),
       offload_status_(OffloadScanManager::kError),
+      cached_scan_results_(new std::vector<NativeScanResult>()),
       service_available_(false),
       offload_service_utils_(utils),
       offload_callback_handlers_(new OffloadCallbackHandlersImpl(this)),
@@ -205,8 +206,6 @@ bool OffloadScanManager::startScan(
   }
 
   *reason_code = OffloadScanManager::kNone;
-  /* Clear up the scan cache every time a new scan is requested */
-  cached_scan_results_.clear();
   return true;
 }
 
@@ -240,7 +239,7 @@ OffloadScanManager::StatusCode OffloadScanManager::getOffloadStatus() const {
 
 bool OffloadScanManager::getScanResults(
     std::vector<NativeScanResult>* out_scan_results) {
-  for (auto scan_result : cached_scan_results_) {
+  for (auto scan_result : *cached_scan_results_) {
     out_scan_results->push_back(scan_result);
   }
   return true;
@@ -262,12 +261,17 @@ OffloadScanManager::~OffloadScanManager() {
   if (wifi_offload_hal_ != nullptr) {
     wifi_offload_hal_->unlinkToDeath(death_recipient_);
   }
+  delete cached_scan_results_;
 }
 
 void OffloadScanManager::ReportScanResults(
     const vector<ScanResult>& scanResult) {
-  cached_scan_results_ =
-      OffloadScanUtils::convertToNativeScanResults(scanResult);
+  cached_scan_results_->clear();
+  if (!OffloadScanUtils::convertToNativeScanResults(scanResult,
+                                                    cached_scan_results_)) {
+    LOG(WARNING) << "Unable to convert scan results to native format";
+    return;
+  }
   if (event_callback_ != nullptr) {
     event_callback_->OnOffloadScanResult();
   } else {

diff --git a/scanning/offload/offload_scan_manager.h b/scanning/offload/offload_scan_manager.h
index 46f1bad..7b23d8e 100644 (file)
--- a/scanning/offload/offload_scan_manager.h
+++ b/scanning/offload/offload_scan_manager.h
@@ -144,7 +144,7 @@ class OffloadScanManager {
   android::sp<OffloadCallback> wifi_offload_callback_;
   android::sp<OffloadDeathRecipient> death_recipient_;
   StatusCode offload_status_;
-  std::vector<::com::android::server::wifi::wificond::NativeScanResult>
+  std::vector<::com::android::server::wifi::wificond::NativeScanResult>*
       cached_scan_results_;
   bool service_available_;
 

diff --git a/scanning/offload/offload_scan_utils.cpp b/scanning/offload/offload_scan_utils.cpp
index 598fd8e..1446f76 100644 (file)
--- a/scanning/offload/offload_scan_utils.cpp
+++ b/scanning/offload/offload_scan_utils.cpp
@@ -16,6 +16,7 @@
 #include "wificond/scanning/offload/offload_scan_utils.h"
 
 #include <android-base/logging.h>
+#include <utils/Timers.h>
 
 #include "wificond/scanning/offload/scan_stats.h"
 #include "wificond/scanning/scan_result.h"
@@ -33,10 +34,10 @@ using std::vector;
 namespace android {
 namespace wificond {
 
-vector<NativeScanResult> OffloadScanUtils::convertToNativeScanResults(
-    const vector<ScanResult>& scan_result) {
-  vector<NativeScanResult> native_scan_result;
-  native_scan_result.reserve(scan_result.size());
+bool OffloadScanUtils::convertToNativeScanResults(
+    const vector<ScanResult>& scan_result,
+    vector<NativeScanResult>* native_scan_result) {
+  if (native_scan_result == nullptr) return false;
   for (size_t i = 0; i < scan_result.size(); i++) {
     NativeScanResult single_scan_result;
     single_scan_result.ssid.assign(scan_result[i].networkInfo.ssid.begin(),
@@ -46,12 +47,12 @@ vector<NativeScanResult> OffloadScanUtils::convertToNativeScanResults(
     }
     single_scan_result.frequency = scan_result[i].frequency;
     single_scan_result.signal_mbm = scan_result[i].rssi;
-    single_scan_result.tsf = scan_result[i].tsf;
+    single_scan_result.tsf = systemTime(SYSTEM_TIME_MONOTONIC) / 1000;
     single_scan_result.capability = scan_result[i].capability;
     single_scan_result.associated = false;
-    native_scan_result.emplace_back(single_scan_result);
+    native_scan_result->push_back(std::move(single_scan_result));
   }
-  return native_scan_result;
+  return true;
 }
 
 ScanParam OffloadScanUtils::createScanParam(

diff --git a/scanning/offload/offload_scan_utils.h b/scanning/offload/offload_scan_utils.h
index ce4afa7..22ba8ea 100644 (file)
--- a/scanning/offload/offload_scan_utils.h
+++ b/scanning/offload/offload_scan_utils.h
@@ -42,9 +42,9 @@ namespace wificond {
 // Provides utility methods for Offload Scan Manager
 class OffloadScanUtils {
  public:
-  static std::vector<::com::android::server::wifi::wificond::NativeScanResult>
-      convertToNativeScanResults(
-          const std::vector<android::hardware::wifi::offload::V1_0::ScanResult>&);
+  static bool convertToNativeScanResults(
+      const std::vector<android::hardware::wifi::offload::V1_0::ScanResult>&,
+      std::vector<::com::android::server::wifi::wificond::NativeScanResult>*);
   static android::hardware::wifi::offload::V1_0::ScanParam createScanParam(
       const std::vector<std::vector<uint8_t>>& ssid_list,
       const std::vector<uint32_t>& frequency_list, uint32_t scan_interval_ms);

diff --git a/scanning/offload/scan_stats.cpp b/scanning/offload/scan_stats.cpp
index 4a963cb..bd5c793 100644 (file)
--- a/scanning/offload/scan_stats.cpp
+++ b/scanning/offload/scan_stats.cpp
@@ -46,6 +46,7 @@ NativeScanStats::NativeScanStats()
     : num_scans_requested_by_wifi_(0),
       num_scans_serviced_by_wifi_(0),
       subscription_duration_ms_(0),
+      scan_duration_ms_(0),
       num_channels_scanned_(0),
       time_stamp_(0) {}
 
@@ -87,6 +88,20 @@ status_t NativeScanStats::readFromParcel(const ::android::Parcel* parcel) {
   return ::android::OK;
 }
 
+void NativeScanStats::DebugLog() {
+  LOG(INFO) << "num_scans_requested_by_wifi=" << num_scans_requested_by_wifi_;
+  LOG(INFO) << "num_scans_serviced_by_wifi=" << num_scans_serviced_by_wifi_;
+  LOG(INFO) << "subscription_duration=" << subscription_duration_ms_;
+  LOG(INFO) << "scan_duration_ms_=" << scan_duration_ms_;
+  LOG(INFO) << "num_channels_scanned=" << num_channels_scanned_;
+  for (size_t i = 0; i < histogram_channels_.size(); i++) {
+    if (histogram_channels_[i] > 0) {
+      LOG(INFO) << "Channel=" << i << " ScannedTimes="
+                << static_cast<uint32_t>(histogram_channels_[i]);
+    }
+  }
+}
+
 }  // namespace wificond
 }  // namespace wifi
 }  // namespace server

diff --git a/scanning/offload/scan_stats.h b/scanning/offload/scan_stats.h
index eb1d16a..05220b9 100644 (file)
--- a/scanning/offload/scan_stats.h
+++ b/scanning/offload/scan_stats.h
@@ -40,6 +40,7 @@ class NativeScanStats : public ::android::Parcelable {
   bool operator==(const NativeScanStats&) const;
   ::android::status_t writeToParcel(::android::Parcel* parcel) const override;
   ::android::status_t readFromParcel(const ::android::Parcel* parcel) override;
+  void DebugLog();
 
   uint32_t num_scans_requested_by_wifi_;
   uint32_t num_scans_serviced_by_wifi_;

diff --git a/scanning/scanner_impl.cpp b/scanning/scanner_impl.cpp
index 5fa7093..b14d788 100644 (file)
--- a/scanning/scanner_impl.cpp
+++ b/scanning/scanner_impl.cpp
@@ -514,7 +514,6 @@ void ScannerImpl::OnOffloadScanResult() {
 
 void ScannerImpl::OnOffloadError(
     OffloadScanCallbackInterface::AsyncErrorReason error_code) {
-  bool success;
   if (!pno_scan_running_over_offload_) {
     // Ignore irrelevant error notifications
     LOG(WARNING) << "Offload HAL Async Error occured but Offload HAL is not "
@@ -522,19 +521,16 @@ void ScannerImpl::OnOffloadError(
     return;
   }
   LOG(ERROR) << "Offload Service Async Failure error_code=" << error_code;
-  // Stop scans over Offload HAL and request them over netlink
-  stopPnoScan(&success);
-  if (success) {
-    LOG(INFO) << "Pno scans stopped";
-  }
   switch (error_code) {
     case OffloadScanCallbackInterface::AsyncErrorReason::BINDER_DEATH:
+      LOG(ERROR) << "Binder death";
       if (pno_scan_event_handler_ != nullptr) {
         pno_scan_event_handler_->OnPnoScanOverOffloadFailed(
             net::wifi::IPnoScanEvent::PNO_SCAN_OVER_OFFLOAD_BINDER_FAILURE);
       }
       break;
     case OffloadScanCallbackInterface::AsyncErrorReason::REMOTE_FAILURE:
+      LOG(ERROR) << "Remote failure";
       if (pno_scan_event_handler_ != nullptr) {
         pno_scan_event_handler_->OnPnoScanOverOffloadFailed(
             net::wifi::IPnoScanEvent::PNO_SCAN_OVER_OFFLOAD_REMOTE_FAILURE);
@@ -544,7 +540,14 @@ void ScannerImpl::OnOffloadError(
       LOG(WARNING) << "Invalid Error code";
       break;
   }
-  startPnoScan(pno_settings_, &success);
+  bool success;
+  // Stop scans over Offload HAL and request them over netlink
+  stopPnoScan(&success);
+  if (success) {
+    LOG(INFO) << "Pno scans stopped";
+  }
+  // Restart PNO scans over netlink interface
+  success = StartPnoScanDefault(pno_settings_);
   if (success) {
     LOG(INFO) << "Pno scans restarted";
   }

diff --git a/tests/offload_scan_utils_test.cpp b/tests/offload_scan_utils_test.cpp
index d75143a..15c7e86 100644 (file)
--- a/tests/offload_scan_utils_test.cpp
+++ b/tests/offload_scan_utils_test.cpp
@@ -50,13 +50,13 @@ class OffloadScanUtilsTest : public ::testing::Test {
 };
 
 TEST_F(OffloadScanUtilsTest, verifyConversion) {
-  vector<NativeScanResult> native_scan_results =
-      OffloadScanUtils::convertToNativeScanResults(dummy_scan_results_);
+  vector<NativeScanResult> native_scan_results;
+  EXPECT_TRUE(OffloadScanUtils::convertToNativeScanResults(
+      dummy_scan_results_, &native_scan_results));
   EXPECT_EQ(native_scan_results.size(), dummy_scan_results_.size());
   for (size_t i = 0; i < native_scan_results.size(); i++) {
     EXPECT_EQ(native_scan_results[i].frequency,
               dummy_scan_results_[i].frequency);
-    EXPECT_EQ(native_scan_results[i].tsf, dummy_scan_results_[i].tsf);
     EXPECT_EQ(native_scan_results[i].signal_mbm, dummy_scan_results_[i].rssi);
     EXPECT_EQ(native_scan_results[i].ssid.size(),
               dummy_scan_results_[i].networkInfo.ssid.size());