[Linux] Device scanning status using state machine and fix error stat…

…us (#31412) * [Linux] Modify the device scanning status to a state machine * Restyled by clang-format * [Linux] Use a mutex to avoid potential race conditions with BLE scanners. * [Linux] use std::lock_guard instead of manually unlock * [Linux] fix bug: mutex is nonmoveable * [Linux] Turn scanning on and off in the same thread(ChipStack) --------- Co-authored-by: Restyled.io <[email protected]>
project-chip · Feb 21, 2024 · 63482aa · 63482aa
1 parent 8aada7e
commit 63482aa
Show file tree

Hide file tree

Showing 3 changed files with 71 additions and 47 deletions.
diff --git a/src/platform/Linux/BLEManagerImpl.cpp b/src/platform/Linux/BLEManagerImpl.cpp
@@ -776,11 +776,14 @@ void BLEManagerImpl::OnDeviceScanned(BluezDevice1 & device, const chip::Ble::Chi
     mBLEScanConfig.mBleScanState = BleScanState::kConnecting;
 
     chip::DeviceLayer::PlatformMgr().LockChipStack();
+    // We StartScan in the ChipStack thread.
+    // StopScan should also be performed in the ChipStack thread.
+    // At the same time, the scan timer also needs to be canceled in the ChipStack thread.
+    mDeviceScanner.StopScan();
+    // Stop scanning and then start connecting timer
     DeviceLayer::SystemLayer().StartTimer(kConnectTimeout, HandleConnectTimeout, &mEndpoint);
     chip::DeviceLayer::PlatformMgr().UnlockChipStack();
 
-    mDeviceScanner.StopScan();
-
     mEndpoint.ConnectDevice(device);
 }
 

diff --git a/src/platform/Linux/bluez/ChipDeviceScanner.cpp b/src/platform/Linux/bluez/ChipDeviceScanner.cpp
@@ -80,23 +80,17 @@ CHIP_ERROR ChipDeviceScanner::Init(BluezAdapter1 * adapter, ChipDeviceScannerDel
         },
         this));
 
-    mIsInitialized = true;
+    mScannerState = ChipDeviceScannerState::SCANNER_INITIALIZED;
+
     return CHIP_NO_ERROR;
 }
 
 void ChipDeviceScanner::Shutdown()
 {
-    VerifyOrReturn(mIsInitialized);
+    VerifyOrReturn(mScannerState != ChipDeviceScannerState::SCANNER_UNINITIALIZED);
 
     StopScan();
 
-    // mTimerExpired should only be set to true in the TimerExpiredCallback, which means we are in that callback
-    // right now so there is no need to cancel the timer.
-    if (!mTimerExpired)
-    {
-        chip::DeviceLayer::SystemLayer().CancelTimer(TimerExpiredCallback, this);
-    }
-
     // Release resources on the glib thread. This is necessary because the D-Bus manager client
     // object handles D-Bus signals. Otherwise, we might face a race when the manager object is
     // released during a D-Bus signal being processed.
@@ -112,29 +106,32 @@ void ChipDeviceScanner::Shutdown()
         },
         this);
 
-    mIsInitialized = false;
+    mScannerState = ChipDeviceScannerState::SCANNER_UNINITIALIZED;
 }
 
 CHIP_ERROR ChipDeviceScanner::StartScan(System::Clock::Timeout timeout)
 {
     assertChipStackLockedByCurrentThread();
-    VerifyOrReturnError(mIsInitialized, CHIP_ERROR_INCORRECT_STATE);
-    VerifyOrReturnError(!mIsScanning, CHIP_ERROR_INCORRECT_STATE);
+    VerifyOrReturnError(mScannerState != ChipDeviceScannerState::SCANNER_SCANNING, CHIP_ERROR_INCORRECT_STATE);
+    VerifyOrReturnError(mTimerState == ScannerTimerState::TIMER_CANCELED, CHIP_ERROR_INCORRECT_STATE);
 
-    mIsScanning = true; // optimistic, to allow all callbacks to check this
     if (PlatformMgrImpl().GLibMatterContextInvokeSync(MainLoopStartScan, this) != CHIP_NO_ERROR)
     {
         ChipLogError(Ble, "Failed to schedule BLE scan start.");
-        mIsScanning = false;
-        return CHIP_ERROR_INTERNAL;
-    }
 
-    if (!mIsScanning)
-    {
-        ChipLogError(Ble, "Failed to start BLE scan.");
+        ChipDeviceScannerDelegate * delegate = this->mDelegate;
+        // callback is explicitly allowed to delete the scanner (hence no more
+        // references to 'self' here)
+        delegate->OnScanComplete();
+
         return CHIP_ERROR_INTERNAL;
     }
 
+    // Here need to set the Bluetooth scanning status immediately.
+    // So that if the timer fails to start in the next step,
+    // calling StopScan will be effective.
+    mScannerState = ChipDeviceScannerState::SCANNER_SCANNING;
+
     CHIP_ERROR err = chip::DeviceLayer::SystemLayer().StartTimer(timeout, TimerExpiredCallback, static_cast<void *>(this));
 
     if (err != CHIP_NO_ERROR)
@@ -143,45 +140,45 @@ CHIP_ERROR ChipDeviceScanner::StartScan(System::Clock::Timeout timeout)
         StopScan();
         return err;
     }
-    mTimerExpired = false;
+    mTimerState = ScannerTimerState::TIMER_STARTED;
+
+    ChipLogDetail(Ble, "ChipDeviceScanner has started scanning!");
 
     return CHIP_NO_ERROR;
 }
 
 void ChipDeviceScanner::TimerExpiredCallback(chip::System::Layer * layer, void * appState)
 {
     ChipDeviceScanner * chipDeviceScanner = static_cast<ChipDeviceScanner *>(appState);
-    chipDeviceScanner->mTimerExpired      = true;
+    chipDeviceScanner->mTimerState        = ScannerTimerState::TIMER_EXPIRED;
     chipDeviceScanner->mDelegate->OnScanError(CHIP_ERROR_TIMEOUT);
     chipDeviceScanner->StopScan();
 }
 
 CHIP_ERROR ChipDeviceScanner::StopScan()
 {
-    VerifyOrReturnError(mIsScanning, CHIP_NO_ERROR);
-    VerifyOrReturnError(!mIsStopping, CHIP_NO_ERROR);
-
-    mIsStopping = true;
-    g_cancellable_cancel(mCancellable); // in case we are currently running a scan
+    assertChipStackLockedByCurrentThread();
+    VerifyOrReturnError(mScannerState == ChipDeviceScannerState::SCANNER_SCANNING, CHIP_NO_ERROR);
 
-    if (mObjectAddedSignal)
+    if (PlatformMgrImpl().GLibMatterContextInvokeSync(MainLoopStopScan, this) != CHIP_NO_ERROR)
     {
-        g_signal_handler_disconnect(mManager, mObjectAddedSignal);
-        mObjectAddedSignal = 0;
+        ChipLogError(Ble, "Failed to schedule BLE scan stop.");
+        return CHIP_ERROR_INTERNAL;
     }
 
-    if (mInterfaceChangedSignal)
-    {
-        g_signal_handler_disconnect(mManager, mInterfaceChangedSignal);
-        mInterfaceChangedSignal = 0;
-    }
+    // Stop scanning and return to initialization state
+    mScannerState = ChipDeviceScannerState::SCANNER_INITIALIZED;
 
-    if (PlatformMgrImpl().GLibMatterContextInvokeSync(MainLoopStopScan, this) != CHIP_NO_ERROR)
+    ChipLogDetail(Ble, "ChipDeviceScanner has stopped scanning!");
+
+    if (mTimerState == ScannerTimerState::TIMER_STARTED)
     {
-        ChipLogError(Ble, "Failed to schedule BLE scan stop.");
-        return CHIP_ERROR_INTERNAL;
+        chip::DeviceLayer::SystemLayer().CancelTimer(TimerExpiredCallback, this);
     }
 
+    // Reset timer status
+    mTimerState = ScannerTimerState::TIMER_CANCELED;
+
     ChipDeviceScannerDelegate * delegate = this->mDelegate;
     // callback is explicitly allowed to delete the scanner (hence no more
     // references to 'self' here)
@@ -194,12 +191,26 @@ CHIP_ERROR ChipDeviceScanner::MainLoopStopScan(ChipDeviceScanner * self)
 {
     GAutoPtr<GError> error;
 
+    g_cancellable_cancel(self->mCancellable); // in case we are currently running a scan
+
+    if (self->mObjectAddedSignal)
+    {
+        g_signal_handler_disconnect(self->mManager, self->mObjectAddedSignal);
+        self->mObjectAddedSignal = 0;
+    }
+
+    if (self->mInterfaceChangedSignal)
+    {
+        g_signal_handler_disconnect(self->mManager, self->mInterfaceChangedSignal);
+        self->mInterfaceChangedSignal = 0;
+    }
+
     if (!bluez_adapter1_call_stop_discovery_sync(self->mAdapter, nullptr /* not cancellable */,
                                                  &MakeUniquePointerReceiver(error).Get()))
     {
         ChipLogError(Ble, "Failed to stop discovery %s", error->message);
+        return CHIP_ERROR_INTERNAL;
     }
-    self->mIsScanning = false;
 
     return CHIP_NO_ERROR;
 }
@@ -304,9 +315,7 @@ CHIP_ERROR ChipDeviceScanner::MainLoopStartScan(ChipDeviceScanner * self)
     if (!bluez_adapter1_call_start_discovery_sync(self->mAdapter, self->mCancellable, &MakeUniquePointerReceiver(error).Get()))
     {
         ChipLogError(Ble, "Failed to start discovery: %s", error->message);
-
-        self->mIsScanning = false;
-        self->mDelegate->OnScanComplete();
+        return CHIP_ERROR_INTERNAL;
     }
 
     return CHIP_NO_ERROR;

diff --git a/src/platform/Linux/bluez/ChipDeviceScanner.h b/src/platform/Linux/bluez/ChipDeviceScanner.h
@@ -75,6 +75,20 @@ class ChipDeviceScanner
     CHIP_ERROR StopScan();
 
 private:
+    enum ChipDeviceScannerState
+    {
+        SCANNER_UNINITIALIZED,
+        SCANNER_INITIALIZED,
+        SCANNER_SCANNING
+    };
+
+    enum ScannerTimerState
+    {
+        TIMER_CANCELED,
+        TIMER_STARTED,
+        TIMER_EXPIRED
+    };
+
     static void TimerExpiredCallback(chip::System::Layer * layer, void * appState);
     static CHIP_ERROR MainLoopStartScan(ChipDeviceScanner * self);
     static CHIP_ERROR MainLoopStopScan(ChipDeviceScanner * self);
@@ -96,11 +110,9 @@ class ChipDeviceScanner
     ChipDeviceScannerDelegate * mDelegate = nullptr;
     gulong mObjectAddedSignal             = 0;
     gulong mInterfaceChangedSignal        = 0;
-    bool mIsInitialized                   = false;
-    bool mIsScanning                      = false;
-    bool mIsStopping                      = false;
+    ChipDeviceScannerState mScannerState  = ChipDeviceScannerState::SCANNER_UNINITIALIZED;
     /// Used to track if timer has already expired and doesn't need to be canceled.
-    bool mTimerExpired = false;
+    ScannerTimerState mTimerState = ScannerTimerState::TIMER_CANCELED;
 };
 
 } // namespace Internal