diff --git a/src/controller/python/chip/ble/LinuxImpl.cpp b/src/controller/python/chip/ble/LinuxImpl.cpp index c33dddd1325dcc..6218d9c66d4c83 100644 --- a/src/controller/python/chip/ble/LinuxImpl.cpp +++ b/src/controller/python/chip/ble/LinuxImpl.cpp @@ -1,3 +1,20 @@ +/* + * + * Copyright (c) 2020-2023 Project CHIP Authors + * All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ #include #include @@ -76,7 +93,8 @@ class ScannerDelegateImpl : public ChipDeviceScannerDelegate mScanCallback(scanCallback), mCompleteCallback(completeCallback), mErrorCallback(errorCallback) {} - void SetScanner(std::unique_ptr scanner) { mScanner = std::move(scanner); } + CHIP_ERROR ScannerInit(BluezAdapter1 * adapter) { return mScanner.Init(adapter, this); } + CHIP_ERROR ScannerStartScan(chip::System::Clock::Timeout timeout) { return mScanner.StartScan(timeout); } void OnDeviceScanned(BluezDevice1 & device, const chip::Ble::ChipBLEDeviceIdentificationInfo & info) override { @@ -106,7 +124,7 @@ class ScannerDelegateImpl : public ChipDeviceScannerDelegate } private: - std::unique_ptr mScanner; + ChipDeviceScanner mScanner; PyObject * const mContext; const DeviceScannedCallback mScanCallback; const ScanCompleteCallback mCompleteCallback; @@ -123,23 +141,13 @@ extern "C" void * pychip_ble_start_scanning(PyObject * context, void * adapter, std::unique_ptr delegate = std::make_unique(context, scanCallback, completeCallback, errorCallback); - std::unique_ptr scanner = ChipDeviceScanner::Create(static_cast(adapter), delegate.get()); + CHIP_ERROR err = delegate->ScannerInit(static_cast(adapter)); + VerifyOrReturnError(err == CHIP_NO_ERROR, nullptr); - if (!scanner) - { - return nullptr; - } - - CHIP_ERROR err = CHIP_NO_ERROR; chip::DeviceLayer::PlatformMgr().LockChipStack(); - err = scanner->StartScan(chip::System::Clock::Milliseconds32(timeoutMs)); + err = delegate->ScannerStartScan(chip::System::Clock::Milliseconds32(timeoutMs)); chip::DeviceLayer::PlatformMgr().UnlockChipStack(); - if (err != CHIP_NO_ERROR) - { - return nullptr; - } - - delegate->SetScanner(std::move(scanner)); + VerifyOrReturnError(err == CHIP_NO_ERROR, nullptr); return delegate.release(); } diff --git a/src/platform/Linux/BLEManagerImpl.cpp b/src/platform/Linux/BLEManagerImpl.cpp index bf0f4eb2f909bd..01a93a29b8a1ed 100644 --- a/src/platform/Linux/BLEManagerImpl.cpp +++ b/src/platform/Linux/BLEManagerImpl.cpp @@ -102,7 +102,7 @@ CHIP_ERROR BLEManagerImpl::_Init() void BLEManagerImpl::_Shutdown() { // ensure scan resources are cleared (e.g. timeout timers) - mDeviceScanner.reset(); + mDeviceScanner.Shutdown(); // Release BLE connection resources (unregister from BlueZ). ShutdownBluezBleLayer(mpEndpoint); mFlags.Clear(Flags::kBluezBLELayerInitialized); @@ -689,10 +689,10 @@ void BLEManagerImpl::InitiateScan(BleScanState scanType) return; } - mDeviceScanner = Internal::ChipDeviceScanner::Create(mpEndpoint->mpAdapter, this); mBLEScanConfig.mBleScanState = scanType; - if (!mDeviceScanner) + CHIP_ERROR err = mDeviceScanner.Init(mpEndpoint->mpAdapter, this); + if (err != CHIP_NO_ERROR) { mBLEScanConfig.mBleScanState = BleScanState::kNotScanning; BleConnectionDelegate::OnConnectionError(mBLEScanConfig.mAppState, CHIP_ERROR_INTERNAL); @@ -700,7 +700,7 @@ void BLEManagerImpl::InitiateScan(BleScanState scanType) return; } - CHIP_ERROR err = mDeviceScanner->StartScan(kNewConnectionScanTimeout); + err = mDeviceScanner.StartScan(kNewConnectionScanTimeout); if (err != CHIP_NO_ERROR) { mBLEScanConfig.mBleScanState = BleScanState::kNotScanning; @@ -738,7 +738,7 @@ CHIP_ERROR BLEManagerImpl::CancelConnection() CancelConnect(mpEndpoint); // If in discovery mode, stop scan. else if (mBLEScanConfig.mBleScanState != BleScanState::kNotScanning) - mDeviceScanner->StopScan(); + mDeviceScanner.StopScan(); return CHIP_NO_ERROR; } @@ -811,7 +811,7 @@ void BLEManagerImpl::OnDeviceScanned(BluezDevice1 & device, const chip::Ble::Chi DeviceLayer::SystemLayer().StartTimer(kConnectTimeout, HandleConnectTimeout, mpEndpoint); chip::DeviceLayer::PlatformMgr().UnlockChipStack(); - mDeviceScanner->StopScan(); + mDeviceScanner.StopScan(); ConnectDevice(device, mpEndpoint); } diff --git a/src/platform/Linux/BLEManagerImpl.h b/src/platform/Linux/BLEManagerImpl.h index 2cc764b742cf13..458bac267db832 100644 --- a/src/platform/Linux/BLEManagerImpl.h +++ b/src/platform/Linux/BLEManagerImpl.h @@ -204,7 +204,7 @@ class BLEManagerImpl final : public BLEManager, char mDeviceName[kMaxDeviceNameLength + 1]; bool mIsCentral = false; BluezEndpoint * mpEndpoint = nullptr; - std::unique_ptr mDeviceScanner; + ChipDeviceScanner mDeviceScanner; }; /** diff --git a/src/platform/Linux/bluez/ChipDeviceScanner.cpp b/src/platform/Linux/bluez/ChipDeviceScanner.cpp index 050df86dabc9ef..1670dfc082bebd 100644 --- a/src/platform/Linux/bluez/ChipDeviceScanner.cpp +++ b/src/platform/Linux/bluez/ChipDeviceScanner.cpp @@ -33,43 +33,6 @@ namespace Internal { namespace { -// Helper context for creating GDBusObjectManager with -// chip::DeviceLayer::GLibMatterContextInvokeSync() -struct GDBusCreateObjectManagerContext -{ - GDBusObjectManager * object = nullptr; - // Cancellable passed to g_dbus_object_manager_client_new_for_bus_sync() - // which later can be used to cancel the scan operation. - GCancellable * cancellable = nullptr; - - GDBusCreateObjectManagerContext() : cancellable(g_cancellable_new()) {} - ~GDBusCreateObjectManagerContext() - { - g_object_unref(cancellable); - if (object != nullptr) - { - g_object_unref(object); - } - } -}; - -CHIP_ERROR MainLoopCreateObjectManager(GDBusCreateObjectManagerContext * context) -{ - // When creating D-Bus proxy object, the thread default context must be initialized. Otherwise, - // all D-Bus signals will be delivered to the GLib global default main context. - VerifyOrDie(g_main_context_get_thread_default() != nullptr); - - GAutoPtr err; - context->object = g_dbus_object_manager_client_new_for_bus_sync( - G_BUS_TYPE_SYSTEM, G_DBUS_OBJECT_MANAGER_CLIENT_FLAGS_NONE, BLUEZ_INTERFACE, "/", - bluez_object_manager_client_get_proxy_type, nullptr /* unused user data in the Proxy Type Func */, - nullptr /* destroy notify */, context->cancellable, &MakeUniquePointerReceiver(err).Get()); - VerifyOrReturnError(context->object != nullptr, CHIP_ERROR_INTERNAL, - ChipLogError(Ble, "Failed to get DBUS object manager for device scanning: %s", err->message)); - - return CHIP_NO_ERROR; -} - /// Retrieve CHIP device identification info from the device advertising data bool BluezGetChipDeviceInfo(BluezDevice1 & aDevice, chip::Ble::ChipBLEDeviceIdentificationInfo & aDeviceInfo) { @@ -89,18 +52,42 @@ bool BluezGetChipDeviceInfo(BluezDevice1 & aDevice, chip::Ble::ChipBLEDeviceIden } // namespace -ChipDeviceScanner::ChipDeviceScanner(GDBusObjectManager * manager, BluezAdapter1 * adapter, GCancellable * cancellable, - ChipDeviceScannerDelegate * delegate) : - mManager(manager), - mAdapter(adapter), mCancellable(cancellable), mDelegate(delegate) +CHIP_ERROR ChipDeviceScanner::Init(BluezAdapter1 * adapter, ChipDeviceScannerDelegate * delegate) { - g_object_ref(mAdapter); - g_object_ref(mCancellable); - g_object_ref(mManager); + + // Make this function idempotent by shutting down previously initialized state if any. + Shutdown(); + + mAdapter = BLUEZ_ADAPTER1(g_object_ref(adapter)); + mCancellable = g_cancellable_new(); + mDelegate = delegate; + + // Create the D-Bus object manager client object on the glib thread, so that all D-Bus signals + // will be delivered to the glib thread. + ReturnErrorOnFailure(PlatformMgrImpl().GLibMatterContextInvokeSync( + +[](ChipDeviceScanner * self) { + // When creating D-Bus proxy object, the thread default context must be initialized. + VerifyOrDie(g_main_context_get_thread_default() != nullptr); + + GAutoPtr err; + self->mManager = g_dbus_object_manager_client_new_for_bus_sync( + G_BUS_TYPE_SYSTEM, G_DBUS_OBJECT_MANAGER_CLIENT_FLAGS_NONE, BLUEZ_INTERFACE, "/", + bluez_object_manager_client_get_proxy_type, nullptr /* unused user data in the Proxy Type Func */, + nullptr /* destroy notify */, self->mCancellable, &MakeUniquePointerReceiver(err).Get()); + VerifyOrReturnError(self->mManager != nullptr, CHIP_ERROR_INTERNAL, + ChipLogError(Ble, "Failed to get D-Bus object manager for device scanning: %s", err->message)); + return CHIP_NO_ERROR; + }, + this)); + + mIsInitialized = true; + return CHIP_NO_ERROR; } -ChipDeviceScanner::~ChipDeviceScanner() +void ChipDeviceScanner::Shutdown() { + VerifyOrReturn(mIsInitialized); + StopScan(); // mTimerExpired should only be set to true in the TimerExpiredCallback, which means we are in that callback @@ -110,34 +97,29 @@ ChipDeviceScanner::~ChipDeviceScanner() chip::DeviceLayer::SystemLayer().CancelTimer(TimerExpiredCallback, this); } - g_object_unref(mManager); - g_object_unref(mCancellable); - g_object_unref(mAdapter); - - mManager = nullptr; - mAdapter = nullptr; - mCancellable = nullptr; - mDelegate = nullptr; -} - -std::unique_ptr ChipDeviceScanner::Create(BluezAdapter1 * adapter, ChipDeviceScannerDelegate * delegate) -{ - GDBusCreateObjectManagerContext context; - CHIP_ERROR err; - - err = PlatformMgrImpl().GLibMatterContextInvokeSync(MainLoopCreateObjectManager, &context); - VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Ble, "Failed to create BLE object manager")); - - return std::make_unique(context.object, adapter, context.cancellable, delegate); - -exit: - return std::unique_ptr(); + // 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. + PlatformMgrImpl().GLibMatterContextInvokeSync( + +[](ChipDeviceScanner * self) { + if (self->mManager != nullptr) + g_object_unref(self->mManager); + if (self->mAdapter != nullptr) + g_object_unref(self->mAdapter); + if (self->mCancellable != nullptr) + g_object_unref(self->mCancellable); + return CHIP_NO_ERROR; + }, + this); + + mIsInitialized = false; } CHIP_ERROR ChipDeviceScanner::StartScan(System::Clock::Timeout timeout) { assertChipStackLockedByCurrentThread(); - ReturnErrorCodeIf(mIsScanning, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(mIsInitialized, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(!mIsScanning, CHIP_ERROR_INCORRECT_STATE); mIsScanning = true; // optimistic, to allow all callbacks to check this if (PlatformMgrImpl().GLibMatterContextInvokeSync(MainLoopStartScan, this) != CHIP_NO_ERROR) @@ -169,15 +151,16 @@ CHIP_ERROR ChipDeviceScanner::StartScan(System::Clock::Timeout timeout) void ChipDeviceScanner::TimerExpiredCallback(chip::System::Layer * layer, void * appState) { ChipDeviceScanner * chipDeviceScanner = static_cast(appState); - chipDeviceScanner->MarkTimerExpired(); + chipDeviceScanner->mTimerExpired = true; chipDeviceScanner->mDelegate->OnScanError(CHIP_ERROR_TIMEOUT); chipDeviceScanner->StopScan(); } CHIP_ERROR ChipDeviceScanner::StopScan() { - ReturnErrorCodeIf(!mIsScanning, CHIP_NO_ERROR); - ReturnErrorCodeIf(mIsStopping, CHIP_NO_ERROR); + VerifyOrReturnError(mIsScanning, CHIP_NO_ERROR); + VerifyOrReturnError(!mIsStopping, CHIP_NO_ERROR); + mIsStopping = true; g_cancellable_cancel(mCancellable); // in case we are currently running a scan diff --git a/src/platform/Linux/bluez/ChipDeviceScanner.h b/src/platform/Linux/bluez/ChipDeviceScanner.h index ef8830a6fac820..16b8c91682650c 100644 --- a/src/platform/Linux/bluez/ChipDeviceScanner.h +++ b/src/platform/Linux/bluez/ChipDeviceScanner.h @@ -20,7 +20,6 @@ #include #include -#include #include #include @@ -53,15 +52,18 @@ class ChipDeviceScannerDelegate class ChipDeviceScanner { public: - /// NOTE: prefer to use the ::Create method instead direct constructor calling. - ChipDeviceScanner(GDBusObjectManager * manager, BluezAdapter1 * adapter, GCancellable * cancellable, - ChipDeviceScannerDelegate * delegate); - + ChipDeviceScanner() = default; ChipDeviceScanner(ChipDeviceScanner &&) = default; ChipDeviceScanner(const ChipDeviceScanner &) = delete; ChipDeviceScanner & operator=(const ChipDeviceScanner &) = delete; - ~ChipDeviceScanner(); + ~ChipDeviceScanner() { Shutdown(); } + + /// Initialize the scanner. + CHIP_ERROR Init(BluezAdapter1 * adapter, ChipDeviceScannerDelegate * delegate); + + /// Release any resources associated with the scanner. + void Shutdown(); /// Initiate a scan for devices, with the given timeout /// @@ -72,15 +74,6 @@ class ChipDeviceScanner /// Stop any currently running scan CHIP_ERROR StopScan(); - /// Should only be called by TimerExpiredCallback. - void MarkTimerExpired() { mTimerExpired = true; } - - /// Create a new device scanner - /// - /// Convenience method to allocate any required variables. - /// On success, maintains a reference to the provided adapter. - static std::unique_ptr Create(BluezAdapter1 * adapter, ChipDeviceScannerDelegate * delegate); - private: static void TimerExpiredCallback(chip::System::Layer * layer, void * appState); static CHIP_ERROR MainLoopStartScan(ChipDeviceScanner * self); @@ -103,6 +96,7 @@ class ChipDeviceScanner ChipDeviceScannerDelegate * mDelegate = nullptr; gulong mObjectAddedSignal = 0; gulong mInterfaceChangedSignal = 0; + bool mIsInitialized = false; bool mIsScanning = false; bool mIsStopping = false; /// Used to track if timer has already expired and doesn't need to be canceled.