From 049fef0a90f93da037d9afd12e448980482f9279 Mon Sep 17 00:00:00 2001 From: tianfeng-yang <130436698+tianfeng-yang@users.noreply.github.com> Date: Fri, 19 Jan 2024 17:10:04 +0800 Subject: [PATCH] [Linux] Device scanning status using state machine and fix error status (#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 --- src/platform/Linux/BLEManagerImpl.cpp | 7 +- .../Linux/bluez/ChipDeviceScanner.cpp | 91 ++++++++++--------- src/platform/Linux/bluez/ChipDeviceScanner.h | 20 +++- 3 files changed, 71 insertions(+), 47 deletions(-) diff --git a/src/platform/Linux/BLEManagerImpl.cpp b/src/platform/Linux/BLEManagerImpl.cpp index 10a17be7607735..e46e9c5d21edf2 100644 --- 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 index 1670dfc082bebd..3a3115a9ae4783 100644 --- 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(this)); if (err != CHIP_NO_ERROR) @@ -143,7 +140,9 @@ 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; } @@ -151,37 +150,35 @@ CHIP_ERROR ChipDeviceScanner::StartScan(System::Clock::Timeout timeout) void ChipDeviceScanner::TimerExpiredCallback(chip::System::Layer * layer, void * appState) { ChipDeviceScanner * chipDeviceScanner = static_cast(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 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 index 16b8c91682650c..4785a737869dda 100644 --- 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