From 12b6388255c0431c92b8009d8776599cd3f3df6b Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Tue, 3 Oct 2023 10:47:32 +0200 Subject: [PATCH 01/25] Wrap BluezEndpoint in a class --- src/platform/Linux/BLEManagerImpl.cpp | 28 ++--- src/platform/Linux/BLEManagerImpl.h | 5 +- src/platform/Linux/bluez/BluezEndpoint.cpp | 130 +++++++++------------ src/platform/Linux/bluez/BluezEndpoint.h | 52 +++++---- 4 files changed, 103 insertions(+), 112 deletions(-) diff --git a/src/platform/Linux/BLEManagerImpl.cpp b/src/platform/Linux/BLEManagerImpl.cpp index 194598b8581ff6..96f1cf012cd30a 100644 --- a/src/platform/Linux/BLEManagerImpl.cpp +++ b/src/platform/Linux/BLEManagerImpl.cpp @@ -65,7 +65,7 @@ void HandleConnectTimeout(chip::System::Layer *, void * apEndpoint) { assert(apEndpoint != nullptr); - CancelConnect(static_cast(apEndpoint)); + static_cast(apEndpoint)->CancelConnect(); BLEManagerImpl::HandleConnectFailed(CHIP_ERROR_TIMEOUT); } @@ -106,7 +106,7 @@ void BLEManagerImpl::_Shutdown() // Stop advertising and free resources. mBLEAdvertisement.Shutdown(); // Release BLE connection resources (unregister from BlueZ). - ShutdownBluezBleLayer(mpEndpoint); + mEndpoint.reset(); mFlags.Clear(Flags::kBluezBLELayerInitialized); } @@ -571,16 +571,16 @@ void BLEManagerImpl::DriveBLEState() // Initializes the Bluez BLE layer if needed. if (mServiceMode == ConnectivityManager::kCHIPoBLEServiceMode_Enabled && !mFlags.Has(Flags::kBluezBLELayerInitialized)) { - err = InitBluezBleLayer(mAdapterId, mIsCentral, nullptr, mDeviceName, mpEndpoint); - SuccessOrExit(err); + mEndpoint = Platform::MakeUnique(mAdapterId, mIsCentral); + VerifyOrExit(mEndpoint, ChipLogError(DeviceLayer, "FAIL: memory allocation in %s", __func__)); + SuccessOrExit(err = mEndpoint->Init(nullptr, mDeviceName)); mFlags.Set(Flags::kBluezBLELayerInitialized); } // Register the CHIPoBLE application with the Bluez BLE layer if needed. if (!mIsCentral && mServiceMode == ConnectivityManager::kCHIPoBLEServiceMode_Enabled && !mFlags.Has(Flags::kAppRegistered)) { - err = BluezGattsAppRegister(mpEndpoint); - SuccessOrExit(err); + SuccessOrExit(err = mEndpoint->BluezGattAppRegister()); mFlags.Set(Flags::kControlOpInProgress); ExitNow(); } @@ -597,7 +597,7 @@ void BLEManagerImpl::DriveBLEState() // Configure advertising data if it hasn't been done yet. if (!mFlags.Has(Flags::kAdvertisingConfigured)) { - err = mBLEAdvertisement.Init(mpEndpoint, mBLEAdvType, mpBLEAdvUUID, mBLEAdvDurationMs); + err = mBLEAdvertisement.Init(mEndpoint.get(), mBLEAdvType, mpBLEAdvUUID, mBLEAdvDurationMs); SuccessOrExit(err); mFlags.Set(Flags::kAdvertisingConfigured); } @@ -653,14 +653,14 @@ void BLEManagerImpl::InitiateScan(BleScanState scanType) return; } - if (mpEndpoint == nullptr) + if (!mEndpoint) { BleConnectionDelegate::OnConnectionError(mBLEScanConfig.mAppState, CHIP_ERROR_INCORRECT_STATE); ChipLogError(Ble, "BLE Layer is not yet initialized"); return; } - if (mpEndpoint->mpAdapter == nullptr) + if (mEndpoint->mpAdapter == nullptr) { BleConnectionDelegate::OnConnectionError(mBLEScanConfig.mAppState, CHIP_ERROR_INCORRECT_STATE); ChipLogError(Ble, "No adapter available for new connection establishment"); @@ -669,7 +669,7 @@ void BLEManagerImpl::InitiateScan(BleScanState scanType) mBLEScanConfig.mBleScanState = scanType; - CHIP_ERROR err = mDeviceScanner.Init(mpEndpoint->mpAdapter, this); + CHIP_ERROR err = mDeviceScanner.Init(mEndpoint->mpAdapter, this); if (err != CHIP_NO_ERROR) { mBLEScanConfig.mBleScanState = BleScanState::kNotScanning; @@ -691,7 +691,7 @@ void BLEManagerImpl::InitiateScan(BleScanState scanType) void BLEManagerImpl::CleanScanConfig() { if (mBLEScanConfig.mBleScanState == BleScanState::kConnecting) - DeviceLayer::SystemLayer().CancelTimer(HandleConnectTimeout, mpEndpoint); + DeviceLayer::SystemLayer().CancelTimer(HandleConnectTimeout, mEndpoint.get()); mBLEScanConfig.mBleScanState = BleScanState::kNotScanning; } @@ -713,7 +713,7 @@ void BLEManagerImpl::NewConnection(BleLayer * bleLayer, void * appState, const S CHIP_ERROR BLEManagerImpl::CancelConnection() { if (mBLEScanConfig.mBleScanState == BleScanState::kConnecting) - CancelConnect(mpEndpoint); + mEndpoint->CancelConnect(); // If in discovery mode, stop scan. else if (mBLEScanConfig.mBleScanState != BleScanState::kNotScanning) mDeviceScanner.StopScan(); @@ -777,12 +777,12 @@ void BLEManagerImpl::OnDeviceScanned(BluezDevice1 & device, const chip::Ble::Chi mBLEScanConfig.mBleScanState = BleScanState::kConnecting; chip::DeviceLayer::PlatformMgr().LockChipStack(); - DeviceLayer::SystemLayer().StartTimer(kConnectTimeout, HandleConnectTimeout, mpEndpoint); + DeviceLayer::SystemLayer().StartTimer(kConnectTimeout, HandleConnectTimeout, mEndpoint.get()); chip::DeviceLayer::PlatformMgr().UnlockChipStack(); mDeviceScanner.StopScan(); - ConnectDevice(device, mpEndpoint); + mEndpoint->ConnectDevice(device); } void BLEManagerImpl::OnScanComplete() diff --git a/src/platform/Linux/BLEManagerImpl.h b/src/platform/Linux/BLEManagerImpl.h index 2f7cf8fe4f8ae5..e717c94832ecd3 100644 --- a/src/platform/Linux/BLEManagerImpl.h +++ b/src/platform/Linux/BLEManagerImpl.h @@ -27,6 +27,7 @@ #include #include +#include #include #include "bluez/BluezAdvertisement.h" @@ -185,8 +186,8 @@ class BLEManagerImpl final : public BLEManager, uint32_t mAdapterId = 0; char mDeviceName[kMaxDeviceNameLength + 1]; - bool mIsCentral = false; - BluezEndpoint * mpEndpoint = nullptr; + bool mIsCentral = false; + Platform::UniquePtr mEndpoint; BluezAdvertisement mBLEAdvertisement; ChipAdvType mBLEAdvType = ChipAdvType::BLUEZ_ADV_TYPE_UNDIRECTED_CONNECTABLE_SCANNABLE; diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index f905422bbf84b2..052e252cd84c3b 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -500,40 +500,6 @@ static BluezConnection * GetBluezConnectionViaDevice(BluezEndpoint * apEndpoint) return retval; } -static void EndpointCleanup(BluezEndpoint * apEndpoint) -{ - if (apEndpoint != nullptr) - { - g_free(apEndpoint->mpOwningName); - g_free(apEndpoint->mpAdapterName); - g_free(apEndpoint->mpAdapterAddr); - g_free(apEndpoint->mpRootPath); - g_free(apEndpoint->mpServicePath); - if (apEndpoint->mpObjMgr != nullptr) - g_object_unref(apEndpoint->mpObjMgr); - if (apEndpoint->mpAdapter != nullptr) - g_object_unref(apEndpoint->mpAdapter); - if (apEndpoint->mpDevice != nullptr) - g_object_unref(apEndpoint->mpDevice); - if (apEndpoint->mpRoot != nullptr) - g_object_unref(apEndpoint->mpRoot); - if (apEndpoint->mpService != nullptr) - g_object_unref(apEndpoint->mpService); - if (apEndpoint->mpC1 != nullptr) - g_object_unref(apEndpoint->mpC1); - if (apEndpoint->mpC2 != nullptr) - g_object_unref(apEndpoint->mpC2); - if (apEndpoint->mpC3 != nullptr) - g_object_unref(apEndpoint->mpC3); - if (apEndpoint->mpConnMap != nullptr) - g_hash_table_destroy(apEndpoint->mpConnMap); - g_free(apEndpoint->mpPeerDevicePath); - if (apEndpoint->mpConnectCancellable != nullptr) - g_object_unref(apEndpoint->mpConnectCancellable); - g_free(apEndpoint); - } -} - #if CHIP_ENABLE_ADDITIONAL_DATA_ADVERTISING static void UpdateAdditionalDataCharacteristic(BluezGattCharacteristic1 * characteristic) { @@ -682,66 +648,84 @@ static CHIP_ERROR StartupEndpointBindings(BluezEndpoint * endpoint) return CHIP_NO_ERROR; } -CHIP_ERROR BluezGattsAppRegister(BluezEndpoint * apEndpoint) +BluezEndpoint::BluezEndpoint(uint32_t aAdapterId, bool aIsCentral) : mAdapterId(aAdapterId), mIsCentral(aIsCentral) +{ + mpConnMap = g_hash_table_new(g_str_hash, g_str_equal); +} + +BluezEndpoint::~BluezEndpoint() +{ + Shutdown(); + g_free(mpOwningName); + g_free(mpAdapterName); + g_free(mpAdapterAddr); + g_free(mpRootPath); + g_free(mpServicePath); + if (mpConnMap != nullptr) + g_hash_table_destroy(mpConnMap); + g_free(mpPeerDevicePath); + if (mpConnectCancellable != nullptr) + g_object_unref(mpConnectCancellable); +} + +CHIP_ERROR BluezEndpoint::BluezGattAppRegister() { - CHIP_ERROR err = PlatformMgrImpl().GLibMatterContextInvokeSync(BluezPeripheralRegisterApp, apEndpoint); + CHIP_ERROR err = PlatformMgrImpl().GLibMatterContextInvokeSync(BluezPeripheralRegisterApp, this); VerifyOrReturnError(err == CHIP_NO_ERROR, CHIP_ERROR_INCORRECT_STATE, ChipLogError(Ble, "Failed to schedule BluezPeripheralRegisterApp() on CHIPoBluez thread")); return err; } -CHIP_ERROR InitBluezBleLayer(uint32_t aAdapterId, bool aIsCentral, const char * apBleAddr, const char * apBleName, - BluezEndpoint *& apEndpoint) +CHIP_ERROR BluezEndpoint::Init(const char * apBleAddr, const char * apBleName) { - BluezEndpoint * endpoint = g_new0(BluezEndpoint, 1); - CHIP_ERROR err = CHIP_NO_ERROR; + CHIP_ERROR err; if (apBleAddr != nullptr) - endpoint->mpAdapterAddr = g_strdup(apBleAddr); + mpAdapterAddr = g_strdup(apBleAddr); - endpoint->mpConnMap = g_hash_table_new(g_str_hash, g_str_equal); - endpoint->mAdapterId = aAdapterId; - endpoint->mIsCentral = aIsCentral; - - if (!aIsCentral) + if (!mIsCentral) { - endpoint->mpAdapterName = g_strdup(apBleName); + mpAdapterName = g_strdup(apBleName); } else { - endpoint->mpConnectCancellable = g_cancellable_new(); + mpConnectCancellable = g_cancellable_new(); } - err = PlatformMgrImpl().GLibMatterContextInvokeSync(StartupEndpointBindings, endpoint); - VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(DeviceLayer, "Failed to schedule endpoint initialization")); - -exit: - if (err == CHIP_NO_ERROR) - { - apEndpoint = endpoint; - ChipLogDetail(DeviceLayer, "PlatformBlueZInit init success"); - } - else - { - EndpointCleanup(endpoint); - } + err = PlatformMgrImpl().GLibMatterContextInvokeSync(StartupEndpointBindings, this); + VerifyOrReturnError(err == CHIP_NO_ERROR, err, ChipLogError(DeviceLayer, "Failed to schedule endpoint initialization")); - return err; + ChipLogDetail(DeviceLayer, "PlatformBlueZInit init success"); + return CHIP_NO_ERROR; } -CHIP_ERROR ShutdownBluezBleLayer(BluezEndpoint * apEndpoint) +void BluezEndpoint::Shutdown() { - VerifyOrReturnError(apEndpoint != nullptr, CHIP_ERROR_INVALID_ARGUMENT); // Run endpoint cleanup on the CHIPoBluez thread. This is necessary because the // cleanup function releases the D-Bus manager client object, which handles D-Bus // signals. Otherwise, we will face race condition when the D-Bus signal is in // the middle of being processed when the cleanup function is called. - return PlatformMgrImpl().GLibMatterContextInvokeSync( - +[](BluezEndpoint * endpoint) { - EndpointCleanup(endpoint); + PlatformMgrImpl().GLibMatterContextInvokeSync( + +[](BluezEndpoint * self) { + if (self->mpObjMgr != nullptr) + g_object_unref(self->mpObjMgr); + if (self->mpAdapter != nullptr) + g_object_unref(self->mpAdapter); + if (self->mpDevice != nullptr) + g_object_unref(self->mpDevice); + if (self->mpRoot != nullptr) + g_object_unref(self->mpRoot); + if (self->mpService != nullptr) + g_object_unref(self->mpService); + if (self->mpC1 != nullptr) + g_object_unref(self->mpC1); + if (self->mpC2 != nullptr) + g_object_unref(self->mpC2); + if (self->mpC3 != nullptr) + g_object_unref(self->mpC3); return CHIP_NO_ERROR; }, - apEndpoint); + this); } // ConnectDevice callbacks @@ -809,9 +793,9 @@ static CHIP_ERROR ConnectDeviceImpl(ConnectParams * apParams) return CHIP_NO_ERROR; } -CHIP_ERROR ConnectDevice(BluezDevice1 & aDevice, BluezEndpoint * apEndpoint) +CHIP_ERROR BluezEndpoint::ConnectDevice(BluezDevice1 & aDevice) { - auto params = chip::Platform::New(&aDevice, apEndpoint); + auto params = chip::Platform::New(&aDevice, this); VerifyOrReturnError(params != nullptr, CHIP_ERROR_NO_MEMORY); if (PlatformMgrImpl().GLibMatterContextInvokeSync(ConnectDeviceImpl, params) != CHIP_NO_ERROR) @@ -824,10 +808,10 @@ CHIP_ERROR ConnectDevice(BluezDevice1 & aDevice, BluezEndpoint * apEndpoint) return CHIP_NO_ERROR; } -void CancelConnect(BluezEndpoint * apEndpoint) +void BluezEndpoint::CancelConnect() { - assert(apEndpoint->mpConnectCancellable != nullptr); - g_cancellable_cancel(apEndpoint->mpConnectCancellable); + assert(mpConnectCancellable != nullptr); + g_cancellable_cancel(mpConnectCancellable); } } // namespace Internal diff --git a/src/platform/Linux/bluez/BluezEndpoint.h b/src/platform/Linux/bluez/BluezEndpoint.h index 712cbc9ad5657f..1b2e3f028a466e 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.h +++ b/src/platform/Linux/bluez/BluezEndpoint.h @@ -60,17 +60,31 @@ namespace chip { namespace DeviceLayer { namespace Internal { -struct BluezEndpoint +class BluezEndpoint { - char * mpOwningName; // Bus owning name +public: + BluezEndpoint(uint32_t aAdapterId, bool aIsCentral); + ~BluezEndpoint(); + + CHIP_ERROR Init(const char * apBleAddr, const char * apBleName); + void Shutdown(); + + CHIP_ERROR BluezGattAppRegister(); + + CHIP_ERROR ConnectDevice(BluezDevice1 & aDevice); + void CancelConnect(); + +public: + // Bus owning name + char * mpOwningName = nullptr; // Adapter properties - char * mpAdapterName; - char * mpAdapterAddr; + char * mpAdapterName = nullptr; + char * mpAdapterAddr = nullptr; // Paths for objects published by this service - char * mpRootPath; - char * mpServicePath; + char * mpRootPath = nullptr; + char * mpServicePath = nullptr; // Objects (interfaces) subscribed to by this service GDBusObjectManager * mpObjMgr = nullptr; @@ -78,29 +92,21 @@ struct BluezEndpoint BluezDevice1 * mpDevice = nullptr; // Objects (interfaces) published by this service - GDBusObjectManagerServer * mpRoot; - BluezGattService1 * mpService; - BluezGattCharacteristic1 * mpC1; - BluezGattCharacteristic1 * mpC2; + GDBusObjectManagerServer * mpRoot = nullptr; + BluezGattService1 * mpService = nullptr; + BluezGattCharacteristic1 * mpC1 = nullptr; + BluezGattCharacteristic1 * mpC2 = nullptr; // additional data characteristics - BluezGattCharacteristic1 * mpC3; + BluezGattCharacteristic1 * mpC3 = nullptr; // map device path to the connection - GHashTable * mpConnMap; - uint32_t mAdapterId; - bool mIsCentral; - char * mpPeerDevicePath; + GHashTable * mpConnMap = nullptr; + uint32_t mAdapterId = 0; + bool mIsCentral = false; + char * mpPeerDevicePath = nullptr; GCancellable * mpConnectCancellable = nullptr; }; -CHIP_ERROR InitBluezBleLayer(uint32_t aAdapterId, bool aIsCentral, const char * apBleAddr, const char * apBleName, - BluezEndpoint *& apEndpoint); -CHIP_ERROR ShutdownBluezBleLayer(BluezEndpoint * apEndpoint); -CHIP_ERROR BluezGattsAppRegister(BluezEndpoint * apEndpoint); - -CHIP_ERROR ConnectDevice(BluezDevice1 & aDevice, BluezEndpoint * apEndpoint); -void CancelConnect(BluezEndpoint * apEndpoint); - } // namespace Internal } // namespace DeviceLayer } // namespace chip From 0092b021fbfdd310f8fd1fa5f4be7b68dff759aa Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Tue, 10 Oct 2023 16:24:05 +0200 Subject: [PATCH 02/25] Change pointer to reference in handle new device --- src/platform/Linux/bluez/BluezEndpoint.cpp | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index 052e252cd84c3b..0ca016a0936781 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -362,13 +362,9 @@ static void BluezSignalInterfacePropertiesChanged(GDBusObjectManagerClient * aMa UpdateConnectionTable(device, *endpoint); } -static void BluezHandleNewDevice(BluezDevice1 * device, BluezEndpoint * apEndpoint) +static void BluezHandleNewDevice(BluezDevice1 * device, BluezEndpoint & aEndpoint) { - VerifyOrExit(apEndpoint != nullptr, ChipLogError(DeviceLayer, "endpoint is NULL in %s", __func__)); - if (apEndpoint->mIsCentral) - { - return; - } + VerifyOrReturn(!aEndpoint.mIsCentral); // We need to handle device connection both this function and BluezSignalInterfacePropertiesChanged // When a device is connected for first time, this function will be triggered. @@ -378,17 +374,17 @@ static void BluezHandleNewDevice(BluezDevice1 * device, BluezEndpoint * apEndpoi VerifyOrExit(bluez_device1_get_connected(device), ChipLogError(DeviceLayer, "FAIL: device is not connected")); conn = static_cast( - g_hash_table_lookup(apEndpoint->mpConnMap, g_dbus_proxy_get_object_path(G_DBUS_PROXY(device)))); + g_hash_table_lookup(aEndpoint.mpConnMap, g_dbus_proxy_get_object_path(G_DBUS_PROXY(device)))); VerifyOrExit(conn == nullptr, ChipLogError(DeviceLayer, "FAIL: connection already tracked: conn: %p new device: %s", conn, g_dbus_proxy_get_object_path(G_DBUS_PROXY(device)))); - conn = chip::Platform::New(apEndpoint, device); - apEndpoint->mpPeerDevicePath = g_strdup(g_dbus_proxy_get_object_path(G_DBUS_PROXY(device))); - g_hash_table_insert(apEndpoint->mpConnMap, apEndpoint->mpPeerDevicePath, conn); + conn = chip::Platform::New(&aEndpoint, device); + aEndpoint.mpPeerDevicePath = g_strdup(g_dbus_proxy_get_object_path(G_DBUS_PROXY(device))); + g_hash_table_insert(aEndpoint.mpConnMap, aEndpoint.mpPeerDevicePath, conn); ChipLogDetail(DeviceLayer, "BLE device connected: conn %p, device %s, path %s", conn, conn->GetPeerAddress(), - apEndpoint->mpPeerDevicePath); + aEndpoint.mpPeerDevicePath); exit: return; @@ -399,12 +395,11 @@ static void BluezSignalOnObjectAdded(GDBusObjectManager * aManager, GDBusObject // TODO: right now we do not handle addition/removal of adapters // Primary focus here is to handle addition of a device GAutoPtr device(bluez_object_get_device1(BLUEZ_OBJECT(aObject))); - VerifyOrReturn(device.get() != nullptr); if (BluezIsDeviceOnAdapter(device.get(), endpoint->mpAdapter) == TRUE) { - BluezHandleNewDevice(device.get(), endpoint); + BluezHandleNewDevice(device.get(), *endpoint); } } From e69bf48887b677814ca1d719093455263d500ba3 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Thu, 12 Oct 2023 08:53:11 +0200 Subject: [PATCH 03/25] BluezEndpoint: Move some static functions to class --- src/platform/Linux/bluez/BluezEndpoint.cpp | 128 +++++++-------------- src/platform/Linux/bluez/BluezEndpoint.h | 23 ++++ 2 files changed, 67 insertions(+), 84 deletions(-) diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index 0ca016a0936781..024dafc0d9db29 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -79,8 +79,6 @@ namespace Internal { constexpr uint16_t kMaxConnectRetries = 4; -static BluezConnection * GetBluezConnectionViaDevice(BluezEndpoint * apEndpoint); - static gboolean BluezCharacteristicReadValue(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInvocation, GVariant * aOptions) { @@ -115,7 +113,7 @@ static gboolean BluezCharacteristicAcquireWrite(BluezGattCharacteristic1 * aChar VerifyOrReturnValue(endpoint != nullptr, FALSE, ChipLogError(DeviceLayer, "endpoint is NULL in %s", __func__)); - conn = GetBluezConnectionViaDevice(endpoint); + conn = endpoint->GetBluezConnectionViaDevice(); VerifyOrReturnValue( conn != nullptr, FALSE, g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.Failed", "No Chipoble connection")); @@ -180,7 +178,7 @@ static gboolean BluezCharacteristicAcquireNotify(BluezGattCharacteristic1 * aCha return FALSE; } - conn = GetBluezConnectionViaDevice(endpoint); + conn = endpoint->GetBluezConnectionViaDevice(); VerifyOrReturnValue( conn != nullptr, FALSE, g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.Failed", "No Chipoble connection")); @@ -226,7 +224,7 @@ static gboolean BluezCharacteristicConfirm(BluezGattCharacteristic1 * aChar, GDB gpointer apClosure) { BluezEndpoint * endpoint = static_cast(apClosure); - BluezConnection * conn = GetBluezConnectionViaDevice(endpoint); + BluezConnection * conn = endpoint->GetBluezConnectionViaDevice(); ChipLogDetail(Ble, "Indication confirmation, %p", conn); BLEManagerImpl::HandleTXComplete(conn); @@ -410,14 +408,14 @@ static void BluezSignalOnObjectRemoved(GDBusObjectManager * aManager, GDBusObjec // for Characteristic1, or GattService -- handle here via calling otPlatTobleHandleDisconnected, or ignore. } -static BluezGattService1 * BluezServiceCreate(BluezEndpoint * endpoint) +BluezGattService1 * BluezEndpoint::BluezServiceCreate() { BluezObjectSkeleton * object; BluezGattService1 * service; - endpoint->mpServicePath = g_strdup_printf("%s/service", endpoint->mpRootPath); - ChipLogDetail(DeviceLayer, "CREATE service object at %s", endpoint->mpServicePath); - object = bluez_object_skeleton_new(endpoint->mpServicePath); + mpServicePath = g_strdup_printf("%s/service", mpRootPath); + ChipLogDetail(DeviceLayer, "CREATE service object at %s", mpServicePath); + object = bluez_object_skeleton_new(mpServicePath); service = bluez_gatt_service1_skeleton_new(); bluez_gatt_service1_set_uuid(service, "0xFFF6"); @@ -426,24 +424,22 @@ static BluezGattService1 * BluezServiceCreate(BluezEndpoint * endpoint) // includes -- unclear whether required. Might be filled in later bluez_object_skeleton_set_gatt_service1(object, service); - g_dbus_object_manager_server_export(endpoint->mpRoot, G_DBUS_OBJECT_SKELETON(object)); + g_dbus_object_manager_server_export(mpRoot, G_DBUS_OBJECT_SKELETON(object)); g_object_unref(object); return service; } -static void bluezObjectsSetup(BluezEndpoint * apEndpoint) +void BluezEndpoint::BluezObjectsSetup() { GList * objects = nullptr; GList * l; char * expectedPath = nullptr; - VerifyOrExit(apEndpoint != nullptr, ChipLogError(DeviceLayer, "endpoint is NULL in %s", __func__)); - - expectedPath = g_strdup_printf("%s/hci%d", BLUEZ_PATH, apEndpoint->mAdapterId); - objects = g_dbus_object_manager_get_objects(apEndpoint->mpObjMgr); + expectedPath = g_strdup_printf("%s/hci%d", BLUEZ_PATH, mAdapterId); + objects = g_dbus_object_manager_get_objects(mpObjMgr); - for (l = objects; l != nullptr && apEndpoint->mpAdapter == nullptr; l = l->next) + for (l = objects; l != nullptr && mpAdapter == nullptr; l = l->next) { BluezObject * object = BLUEZ_OBJECT(l->data); GList * interfaces; @@ -456,52 +452,46 @@ static void bluezObjectsSetup(BluezEndpoint * apEndpoint) { // we found the adapter BluezAdapter1 * adapter = BLUEZ_ADAPTER1(ll->data); char * addr = const_cast(bluez_adapter1_get_address(adapter)); - if (apEndpoint->mpAdapterAddr == nullptr) // no adapter address provided, bind to the hci indicated by nodeid + if (mpAdapterAddr == nullptr) // no adapter address provided, bind to the hci indicated by nodeid { if (strcmp(g_dbus_proxy_get_object_path(G_DBUS_PROXY(adapter)), expectedPath) == 0) { - apEndpoint->mpAdapter = static_cast(g_object_ref(adapter)); + mpAdapter = static_cast(g_object_ref(adapter)); } } else { - if (strcmp(apEndpoint->mpAdapterAddr, addr) == 0) + if (strcmp(mpAdapterAddr, addr) == 0) { - apEndpoint->mpAdapter = static_cast(g_object_ref(adapter)); + mpAdapter = static_cast(g_object_ref(adapter)); } } } } g_list_free_full(interfaces, g_object_unref); } - VerifyOrExit(apEndpoint->mpAdapter != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL apEndpoint->mpAdapter in %s", __func__)); - bluez_adapter1_set_powered(apEndpoint->mpAdapter, TRUE); + VerifyOrExit(mpAdapter != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL mpAdapter in %s", __func__)); + bluez_adapter1_set_powered(mpAdapter, TRUE); // Setting "Discoverable" to False on the adapter and to True on the advertisement convinces // Bluez to set "BR/EDR Not Supported" flag. Bluez doesn't provide API to do that explicitly // and the flag is necessary to force using LE transport. - bluez_adapter1_set_discoverable(apEndpoint->mpAdapter, FALSE); + bluez_adapter1_set_discoverable(mpAdapter, FALSE); exit: g_list_free_full(objects, g_object_unref); g_free(expectedPath); } -static BluezConnection * GetBluezConnectionViaDevice(BluezEndpoint * apEndpoint) +BluezConnection * BluezEndpoint::GetBluezConnectionViaDevice() { - BluezConnection * retval = - static_cast(g_hash_table_lookup(apEndpoint->mpConnMap, apEndpoint->mpPeerDevicePath)); - // ChipLogError(DeviceLayer, "acquire connection object %p in (%s)", retval, __func__); - return retval; + return static_cast(g_hash_table_lookup(mpConnMap, mpPeerDevicePath)); } #if CHIP_ENABLE_ADDITIONAL_DATA_ADVERTISING static void UpdateAdditionalDataCharacteristic(BluezGattCharacteristic1 * characteristic) { - if (characteristic == nullptr) - { - return; - } + VerifyOrReturn(characteristic != nullptr); // Construct the TLV for the additional data GVariant * cValue = nullptr; @@ -543,7 +533,7 @@ static void UpdateAdditionalDataCharacteristic(BluezGattCharacteristic1 * charac } #endif -static void BluezPeripheralObjectsSetup(BluezEndpoint * endpoint) +void BluezEndpoint::BluezPeripheralObjectsSetup() { static const char * const c1_flags[] = { "write", nullptr }; @@ -552,7 +542,9 @@ static void BluezPeripheralObjectsSetup(BluezEndpoint * endpoint) static const char * const c3_flags[] = { "read", nullptr }; #endif - endpoint->mpService = BluezServiceCreate(endpoint); + BluezEndpoint * endpoint = this; + + endpoint->mpService = BluezServiceCreate(); // C1 characteristic endpoint->mpC1 = BluezCharacteristicCreate(endpoint->mpService, "c1", CHIP_PLAT_BLE_UUID_C1_STRING, endpoint->mpRoot); bluez_gatt_characteristic1_set_flags(endpoint->mpC1, c1_flags); @@ -589,31 +581,20 @@ static void BluezPeripheralObjectsSetup(BluezEndpoint * endpoint) #endif } -static void BluezOnBusAcquired(GDBusConnection * aConn, const gchar * aName, gpointer apClosure) +void BluezEndpoint::BluezOnBusAcquired(GDBusConnection * aConn, const char * aName) { - BluezEndpoint * endpoint = static_cast(apClosure); - VerifyOrExit(endpoint != nullptr, ChipLogError(DeviceLayer, "endpoint is NULL in %s", __func__)); - ChipLogDetail(DeviceLayer, "TRACE: Bus acquired for name %s", aName); + VerifyOrReturn(!mIsCentral, ); - if (!endpoint->mIsCentral) - { - endpoint->mpRootPath = g_strdup_printf("/chipoble/%04x", getpid() & 0xffff); - endpoint->mpRoot = g_dbus_object_manager_server_new(endpoint->mpRootPath); - g_dbus_object_manager_server_set_connection(endpoint->mpRoot, aConn); - - BluezPeripheralObjectsSetup(endpoint); - } + mpRootPath = g_strdup_printf("/chipoble/%04x", getpid() & 0xffff); + mpRoot = g_dbus_object_manager_server_new(mpRootPath); + g_dbus_object_manager_server_set_connection(mpRoot, aConn); -exit: - return; + BluezPeripheralObjectsSetup(); } -static CHIP_ERROR StartupEndpointBindings(BluezEndpoint * endpoint) +CHIP_ERROR BluezEndpoint::StartupEndpointBindings(BluezEndpoint * endpoint) { - VerifyOrReturnError(endpoint != nullptr, CHIP_ERROR_INVALID_ARGUMENT, - ChipLogError(DeviceLayer, "endpoint is NULL in %s", __func__)); - GAutoPtr err; GAutoPtr conn(g_bus_get_sync(G_BUS_TYPE_SYSTEM, nullptr, &MakeUniquePointerReceiver(err).Get())); VerifyOrReturnError(conn != nullptr, CHIP_ERROR_INTERNAL, @@ -624,7 +605,7 @@ static CHIP_ERROR StartupEndpointBindings(BluezEndpoint * endpoint) else endpoint->mpOwningName = g_strdup_printf("C-%04x", getpid() & 0xffff); - BluezOnBusAcquired(conn.get(), endpoint->mpOwningName, endpoint); + endpoint->BluezOnBusAcquired(conn.get(), endpoint->mpOwningName); GDBusObjectManager * manager = g_dbus_object_manager_client_new_sync( conn.get(), G_DBUS_OBJECT_MANAGER_CLIENT_FLAGS_NONE, BLUEZ_INTERFACE, "/", bluez_object_manager_client_get_proxy_type, @@ -634,7 +615,7 @@ static CHIP_ERROR StartupEndpointBindings(BluezEndpoint * endpoint) ChipLogError(DeviceLayer, "FAIL: Error getting object manager client: %s", err->message)); endpoint->mpObjMgr = manager; - bluezObjectsSetup(endpoint); + endpoint->BluezObjectsSetup(); g_signal_connect(manager, "object-added", G_CALLBACK(BluezSignalOnObjectAdded), endpoint); g_signal_connect(manager, "object-removed", G_CALLBACK(BluezSignalOnObjectRemoved), endpoint); @@ -725,27 +706,13 @@ void BluezEndpoint::Shutdown() // ConnectDevice callbacks -struct ConnectParams +void BluezEndpoint::ConnectDeviceDone(GObject * aObject, GAsyncResult * aResult, gpointer apParams) { - ConnectParams(BluezDevice1 * device, BluezEndpoint * endpoint) : - mDevice(reinterpret_cast(g_object_ref(device))), mEndpoint(endpoint), mNumRetries(0) - {} - ~ConnectParams() { g_object_unref(mDevice); } - BluezDevice1 * mDevice; - BluezEndpoint * mEndpoint; - uint16_t mNumRetries; -}; - -static void ConnectDeviceDone(GObject * aObject, GAsyncResult * aResult, gpointer apParams) -{ - BluezDevice1 * device = BLUEZ_DEVICE1(aObject); - GAutoPtr error; - gboolean success = bluez_device1_call_connect_finish(device, aResult, &MakeUniquePointerReceiver(error).Get()); + BluezDevice1 * device = BLUEZ_DEVICE1(aObject); ConnectParams * params = static_cast(apParams); + GAutoPtr error; - assert(params != nullptr); - - if (!success) + if (!bluez_device1_call_connect_finish(device, aResult, &MakeUniquePointerReceiver(error).Get())) { ChipLogError(DeviceLayer, "FAIL: ConnectDevice : %s (%d)", error->message, error->code); @@ -760,7 +727,7 @@ static void ConnectDeviceDone(GObject * aObject, GAsyncResult * aResult, gpointe g_clear_error(&MakeUniquePointerReceiver(error).Get()); bluez_device1_call_disconnect_sync(device, nullptr, &MakeUniquePointerReceiver(error).Get()); - bluez_device1_call_connect(device, params->mEndpoint->mpConnectCancellable, ConnectDeviceDone, params); + bluez_device1_call_connect(device, params->mEndpoint.mpConnectCancellable, ConnectDeviceDone, params); return; } @@ -774,23 +741,16 @@ static void ConnectDeviceDone(GObject * aObject, GAsyncResult * aResult, gpointe chip::Platform::Delete(params); } -static CHIP_ERROR ConnectDeviceImpl(ConnectParams * apParams) +CHIP_ERROR BluezEndpoint::ConnectDeviceImpl(ConnectParams * apParams) { - BluezDevice1 * device = apParams->mDevice; - BluezEndpoint * endpoint = apParams->mEndpoint; - - assert(device != nullptr); - assert(endpoint != nullptr); - - g_cancellable_reset(endpoint->mpConnectCancellable); - bluez_device1_call_connect(device, endpoint->mpConnectCancellable, ConnectDeviceDone, apParams); - + g_cancellable_reset(apParams->mEndpoint.mpConnectCancellable); + bluez_device1_call_connect(apParams->mpDevice, apParams->mEndpoint.mpConnectCancellable, ConnectDeviceDone, apParams); return CHIP_NO_ERROR; } CHIP_ERROR BluezEndpoint::ConnectDevice(BluezDevice1 & aDevice) { - auto params = chip::Platform::New(&aDevice, this); + auto params = chip::Platform::New(*this, &aDevice); VerifyOrReturnError(params != nullptr, CHIP_ERROR_NO_MEMORY); if (PlatformMgrImpl().GLibMatterContextInvokeSync(ConnectDeviceImpl, params) != CHIP_NO_ERROR) diff --git a/src/platform/Linux/bluez/BluezEndpoint.h b/src/platform/Linux/bluez/BluezEndpoint.h index 1b2e3f028a466e..f142be61e2de44 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.h +++ b/src/platform/Linux/bluez/BluezEndpoint.h @@ -74,7 +74,30 @@ class BluezEndpoint CHIP_ERROR ConnectDevice(BluezDevice1 & aDevice); void CancelConnect(); +private: + struct ConnectParams + { + ConnectParams(const BluezEndpoint & aEndpoint, BluezDevice1 * apDevice) : mEndpoint(aEndpoint), mpDevice(apDevice) {} + ~ConnectParams() = default; + + const BluezEndpoint & mEndpoint; + BluezDevice1 * mpDevice; + uint16_t mNumRetries = 0; + }; + + BluezGattService1 * BluezServiceCreate(); + void BluezOnBusAcquired(GDBusConnection * aConn, const char * aName); + void BluezPeripheralObjectsSetup(); + void BluezObjectsSetup(); + + static CHIP_ERROR StartupEndpointBindings(BluezEndpoint * endpoint); + + static void ConnectDeviceDone(GObject * aObject, GAsyncResult * aResult, gpointer apParams); + static CHIP_ERROR ConnectDeviceImpl(ConnectParams * apParams); + public: + BluezConnection * GetBluezConnectionViaDevice(); + // Bus owning name char * mpOwningName = nullptr; From d6a9e9206f1ca8b6ef3c2a937dc65176f75f3cae Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Wed, 25 Oct 2023 14:13:47 +0200 Subject: [PATCH 04/25] Remove unnecessary memory allocation --- src/platform/Linux/bluez/BluezEndpoint.cpp | 23 +++++++++------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index 024dafc0d9db29..aae0d7d25e95fe 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -432,26 +432,20 @@ BluezGattService1 * BluezEndpoint::BluezServiceCreate() void BluezEndpoint::BluezObjectsSetup() { - GList * objects = nullptr; - GList * l; - char * expectedPath = nullptr; + char expectedPath[32]; + snprintf(expectedPath, sizeof(expectedPath), BLUEZ_PATH "/hci%u", mAdapterId); - expectedPath = g_strdup_printf("%s/hci%d", BLUEZ_PATH, mAdapterId); - objects = g_dbus_object_manager_get_objects(mpObjMgr); - - for (l = objects; l != nullptr && mpAdapter == nullptr; l = l->next) + GList * objects = g_dbus_object_manager_get_objects(mpObjMgr); + for (auto l = objects; l != nullptr && mpAdapter == nullptr; l = l->next) { BluezObject * object = BLUEZ_OBJECT(l->data); - GList * interfaces; - GList * ll; - interfaces = g_dbus_object_get_interfaces(G_DBUS_OBJECT(object)); - for (ll = interfaces; ll != nullptr; ll = ll->next) + GList * interfaces = g_dbus_object_get_interfaces(G_DBUS_OBJECT(object)); + for (auto ll = interfaces; ll != nullptr; ll = ll->next) { if (BLUEZ_IS_ADAPTER1(ll->data)) { // we found the adapter BluezAdapter1 * adapter = BLUEZ_ADAPTER1(ll->data); - char * addr = const_cast(bluez_adapter1_get_address(adapter)); if (mpAdapterAddr == nullptr) // no adapter address provided, bind to the hci indicated by nodeid { if (strcmp(g_dbus_proxy_get_object_path(G_DBUS_PROXY(adapter)), expectedPath) == 0) @@ -461,7 +455,7 @@ void BluezEndpoint::BluezObjectsSetup() } else { - if (strcmp(mpAdapterAddr, addr) == 0) + if (strcmp(bluez_adapter1_get_address(adapter), mpAdapterAddr) == 0) { mpAdapter = static_cast(g_object_ref(adapter)); } @@ -470,7 +464,9 @@ void BluezEndpoint::BluezObjectsSetup() } g_list_free_full(interfaces, g_object_unref); } + VerifyOrExit(mpAdapter != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL mpAdapter in %s", __func__)); + bluez_adapter1_set_powered(mpAdapter, TRUE); // Setting "Discoverable" to False on the adapter and to True on the advertisement convinces @@ -480,7 +476,6 @@ void BluezEndpoint::BluezObjectsSetup() exit: g_list_free_full(objects, g_object_unref); - g_free(expectedPath); } BluezConnection * BluezEndpoint::GetBluezConnectionViaDevice() From b91344f4d34b91959f69e4823f4255babf79bebd Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Wed, 25 Oct 2023 14:53:23 +0200 Subject: [PATCH 05/25] Make GATT service setup function names more descriptive --- src/platform/Linux/bluez/BluezEndpoint.cpp | 73 +++++++++++----------- src/platform/Linux/bluez/BluezEndpoint.h | 9 ++- 2 files changed, 41 insertions(+), 41 deletions(-) diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index aae0d7d25e95fe..223a74a7d14c45 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -243,25 +243,23 @@ static gboolean BluezIsDeviceOnAdapter(BluezDevice1 * aDevice, BluezAdapter1 * a return strcmp(bluez_device1_get_adapter(aDevice), g_dbus_proxy_get_object_path(G_DBUS_PROXY(aAdapter))) == 0 ? TRUE : FALSE; } -static BluezGattCharacteristic1 * BluezCharacteristicCreate(BluezGattService1 * aService, const char * aCharName, - const char * aUUID, GDBusObjectManagerServer * aRoot) +BluezGattCharacteristic1 * BluezEndpoint::CreateGattCharacteristic(BluezGattService1 * aService, const char * aCharName, + const char * aUUID) { const char * servicePath = g_dbus_object_get_object_path(g_dbus_interface_get_object(G_DBUS_INTERFACE(aService))); - char * charPath = g_strdup_printf("%s/%s", servicePath, aCharName); + GAutoPtr charPath(g_strdup_printf("%s/%s", servicePath, aCharName)); BluezObjectSkeleton * object; BluezGattCharacteristic1 * characteristic; - ChipLogDetail(DeviceLayer, "Create characteristic object at %s", charPath); - object = bluez_object_skeleton_new(charPath); + ChipLogDetail(DeviceLayer, "Create characteristic object at %s", charPath.get()); + object = bluez_object_skeleton_new(charPath.get()); characteristic = bluez_gatt_characteristic1_skeleton_new(); bluez_gatt_characteristic1_set_uuid(characteristic, aUUID); bluez_gatt_characteristic1_set_service(characteristic, servicePath); bluez_object_skeleton_set_gatt_characteristic1(object, characteristic); - g_dbus_object_manager_server_export(aRoot, G_DBUS_OBJECT_SKELETON(object)); - - g_free(charPath); + g_dbus_object_manager_server_export(mpRoot, G_DBUS_OBJECT_SKELETON(object)); g_object_unref(object); return characteristic; @@ -408,7 +406,7 @@ static void BluezSignalOnObjectRemoved(GDBusObjectManager * aManager, GDBusObjec // for Characteristic1, or GattService -- handle here via calling otPlatTobleHandleDisconnected, or ignore. } -BluezGattService1 * BluezEndpoint::BluezServiceCreate() +BluezGattService1 * BluezEndpoint::CreateGattService(const char * aUUID) { BluezObjectSkeleton * object; BluezGattService1 * service; @@ -418,7 +416,7 @@ BluezGattService1 * BluezEndpoint::BluezServiceCreate() object = bluez_object_skeleton_new(mpServicePath); service = bluez_gatt_service1_skeleton_new(); - bluez_gatt_service1_set_uuid(service, "0xFFF6"); + bluez_gatt_service1_set_uuid(service, aUUID); // device is only valid for remote services bluez_gatt_service1_set_primary(service, TRUE); @@ -430,7 +428,7 @@ BluezGattService1 * BluezEndpoint::BluezServiceCreate() return service; } -void BluezEndpoint::BluezObjectsSetup() +void BluezEndpoint::SetupAdapter() { char expectedPath[32]; snprintf(expectedPath, sizeof(expectedPath), BLUEZ_PATH "/hci%u", mAdapterId); @@ -528,7 +526,7 @@ static void UpdateAdditionalDataCharacteristic(BluezGattCharacteristic1 * charac } #endif -void BluezEndpoint::BluezPeripheralObjectsSetup() +void BluezEndpoint::SetupGattService() { static const char * const c1_flags[] = { "write", nullptr }; @@ -537,40 +535,39 @@ void BluezEndpoint::BluezPeripheralObjectsSetup() static const char * const c3_flags[] = { "read", nullptr }; #endif - BluezEndpoint * endpoint = this; + mpService = CreateGattService("0xFFF6"); - endpoint->mpService = BluezServiceCreate(); // C1 characteristic - endpoint->mpC1 = BluezCharacteristicCreate(endpoint->mpService, "c1", CHIP_PLAT_BLE_UUID_C1_STRING, endpoint->mpRoot); - bluez_gatt_characteristic1_set_flags(endpoint->mpC1, c1_flags); + mpC1 = CreateGattCharacteristic(mpService, "c1", CHIP_PLAT_BLE_UUID_C1_STRING); + bluez_gatt_characteristic1_set_flags(mpC1, c1_flags); - g_signal_connect(endpoint->mpC1, "handle-read-value", G_CALLBACK(BluezCharacteristicReadValue), endpoint); - g_signal_connect(endpoint->mpC1, "handle-acquire-write", G_CALLBACK(BluezCharacteristicAcquireWrite), endpoint); - g_signal_connect(endpoint->mpC1, "handle-acquire-notify", G_CALLBACK(BluezCharacteristicAcquireNotifyError), nullptr); - g_signal_connect(endpoint->mpC1, "handle-confirm", G_CALLBACK(BluezCharacteristicConfirmError), nullptr); + g_signal_connect(mpC1, "handle-read-value", G_CALLBACK(BluezCharacteristicReadValue), this); + g_signal_connect(mpC1, "handle-acquire-write", G_CALLBACK(BluezCharacteristicAcquireWrite), this); + g_signal_connect(mpC1, "handle-acquire-notify", G_CALLBACK(BluezCharacteristicAcquireNotifyError), nullptr); + g_signal_connect(mpC1, "handle-confirm", G_CALLBACK(BluezCharacteristicConfirmError), nullptr); - endpoint->mpC2 = BluezCharacteristicCreate(endpoint->mpService, "c2", CHIP_PLAT_BLE_UUID_C2_STRING, endpoint->mpRoot); - bluez_gatt_characteristic1_set_flags(endpoint->mpC2, c2_flags); - g_signal_connect(endpoint->mpC2, "handle-read-value", G_CALLBACK(BluezCharacteristicReadValue), endpoint); - g_signal_connect(endpoint->mpC2, "handle-acquire-write", G_CALLBACK(BluezCharacteristicAcquireWriteError), nullptr); - g_signal_connect(endpoint->mpC2, "handle-acquire-notify", G_CALLBACK(BluezCharacteristicAcquireNotify), endpoint); - g_signal_connect(endpoint->mpC2, "handle-confirm", G_CALLBACK(BluezCharacteristicConfirm), endpoint); + mpC2 = CreateGattCharacteristic(mpService, "c2", CHIP_PLAT_BLE_UUID_C2_STRING); + bluez_gatt_characteristic1_set_flags(mpC2, c2_flags); + g_signal_connect(mpC2, "handle-read-value", G_CALLBACK(BluezCharacteristicReadValue), this); + g_signal_connect(mpC2, "handle-acquire-write", G_CALLBACK(BluezCharacteristicAcquireWriteError), nullptr); + g_signal_connect(mpC2, "handle-acquire-notify", G_CALLBACK(BluezCharacteristicAcquireNotify), this); + g_signal_connect(mpC2, "handle-confirm", G_CALLBACK(BluezCharacteristicConfirm), this); - ChipLogDetail(DeviceLayer, "CHIP BTP C1 %s", bluez_gatt_characteristic1_get_service(endpoint->mpC1)); - ChipLogDetail(DeviceLayer, "CHIP BTP C2 %s", bluez_gatt_characteristic1_get_service(endpoint->mpC2)); + ChipLogDetail(DeviceLayer, "CHIP BTP C1 %s", bluez_gatt_characteristic1_get_service(mpC1)); + ChipLogDetail(DeviceLayer, "CHIP BTP C2 %s", bluez_gatt_characteristic1_get_service(mpC2)); #if CHIP_ENABLE_ADDITIONAL_DATA_ADVERTISING ChipLogDetail(DeviceLayer, "CHIP_ENABLE_ADDITIONAL_DATA_ADVERTISING is TRUE"); // Additional data characteristics - endpoint->mpC3 = BluezCharacteristicCreate(endpoint->mpService, "c3", CHIP_PLAT_BLE_UUID_C3_STRING, endpoint->mpRoot); - bluez_gatt_characteristic1_set_flags(endpoint->mpC3, c3_flags); - g_signal_connect(endpoint->mpC3, "handle-read-value", G_CALLBACK(BluezCharacteristicReadValue), endpoint); - g_signal_connect(endpoint->mpC3, "handle-acquire-write", G_CALLBACK(BluezCharacteristicAcquireWriteError), nullptr); - g_signal_connect(endpoint->mpC3, "handle-acquire-notify", G_CALLBACK(BluezCharacteristicAcquireNotify), endpoint); - g_signal_connect(endpoint->mpC3, "handle-confirm", G_CALLBACK(BluezCharacteristicConfirm), endpoint); + mpC3 = CreateGattCharacteristic(mpService, "c3", CHIP_PLAT_BLE_UUID_C3_STRING); + bluez_gatt_characteristic1_set_flags(mpC3, c3_flags); + g_signal_connect(mpC3, "handle-read-value", G_CALLBACK(BluezCharacteristicReadValue), this); + g_signal_connect(mpC3, "handle-acquire-write", G_CALLBACK(BluezCharacteristicAcquireWriteError), nullptr); + g_signal_connect(mpC3, "handle-acquire-notify", G_CALLBACK(BluezCharacteristicAcquireNotify), this); + g_signal_connect(mpC3, "handle-confirm", G_CALLBACK(BluezCharacteristicConfirm), this); // update the characteristic value - UpdateAdditionalDataCharacteristic(endpoint->mpC3); - ChipLogDetail(DeviceLayer, "CHIP BTP C3 %s", bluez_gatt_characteristic1_get_service(endpoint->mpC3)); + UpdateAdditionalDataCharacteristic(mpC3); + ChipLogDetail(DeviceLayer, "CHIP BTP C3 %s", bluez_gatt_characteristic1_get_service(mpC3)); #else ChipLogDetail(DeviceLayer, "CHIP_ENABLE_ADDITIONAL_DATA_ADVERTISING is FALSE"); #endif @@ -585,7 +582,7 @@ void BluezEndpoint::BluezOnBusAcquired(GDBusConnection * aConn, const char * aNa mpRoot = g_dbus_object_manager_server_new(mpRootPath); g_dbus_object_manager_server_set_connection(mpRoot, aConn); - BluezPeripheralObjectsSetup(); + SetupGattService(); } CHIP_ERROR BluezEndpoint::StartupEndpointBindings(BluezEndpoint * endpoint) @@ -610,7 +607,7 @@ CHIP_ERROR BluezEndpoint::StartupEndpointBindings(BluezEndpoint * endpoint) ChipLogError(DeviceLayer, "FAIL: Error getting object manager client: %s", err->message)); endpoint->mpObjMgr = manager; - endpoint->BluezObjectsSetup(); + endpoint->SetupAdapter(); g_signal_connect(manager, "object-added", G_CALLBACK(BluezSignalOnObjectAdded), endpoint); g_signal_connect(manager, "object-removed", G_CALLBACK(BluezSignalOnObjectRemoved), endpoint); diff --git a/src/platform/Linux/bluez/BluezEndpoint.h b/src/platform/Linux/bluez/BluezEndpoint.h index f142be61e2de44..f241e130110f48 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.h +++ b/src/platform/Linux/bluez/BluezEndpoint.h @@ -85,10 +85,13 @@ class BluezEndpoint uint16_t mNumRetries = 0; }; - BluezGattService1 * BluezServiceCreate(); + void SetupAdapter(); + void SetupGattService(); + + BluezGattService1 * CreateGattService(const char * aUUID); + BluezGattCharacteristic1 * CreateGattCharacteristic(BluezGattService1 * aService, const char * aCharName, const char * aUUID); + void BluezOnBusAcquired(GDBusConnection * aConn, const char * aName); - void BluezPeripheralObjectsSetup(); - void BluezObjectsSetup(); static CHIP_ERROR StartupEndpointBindings(BluezEndpoint * endpoint); From 1a93eff7dbfccc1ff7d835fd4004d960cd865c45 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Wed, 25 Oct 2023 14:55:27 +0200 Subject: [PATCH 06/25] Do not pass magic string to CreateGattService --- src/platform/Linux/bluez/BluezEndpoint.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index 223a74a7d14c45..8d14482a149ed7 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -535,7 +535,7 @@ void BluezEndpoint::SetupGattService() static const char * const c3_flags[] = { "read", nullptr }; #endif - mpService = CreateGattService("0xFFF6"); + mpService = CreateGattService(CHIP_BLE_UUID_SERVICE_SHORT_STRING); // C1 characteristic mpC1 = CreateGattCharacteristic(mpService, "c1", CHIP_PLAT_BLE_UUID_C1_STRING); From f6ebe92ec8a5515c1b42e6885ece616fdddd747d Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Wed, 25 Oct 2023 16:25:01 +0200 Subject: [PATCH 07/25] More class encapsulation --- src/platform/Linux/bluez/BluezEndpoint.cpp | 6 +++--- src/platform/Linux/bluez/BluezEndpoint.h | 8 +++++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index 8d14482a149ed7..6b348097674e2b 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -282,7 +282,7 @@ static void BluezPeripheralRegisterAppDone(GObject * aObject, GAsyncResult * aRe ChipLogDetail(DeviceLayer, "BluezPeripheralRegisterAppDone done"); } -static CHIP_ERROR BluezPeripheralRegisterApp(BluezEndpoint * endpoint) +CHIP_ERROR BluezEndpoint::BluezGattAppRegisterImpl(BluezEndpoint * endpoint) { GDBusObject * adapter; BluezGattManager1 * gattMgr; @@ -638,9 +638,9 @@ BluezEndpoint::~BluezEndpoint() CHIP_ERROR BluezEndpoint::BluezGattAppRegister() { - CHIP_ERROR err = PlatformMgrImpl().GLibMatterContextInvokeSync(BluezPeripheralRegisterApp, this); + CHIP_ERROR err = PlatformMgrImpl().GLibMatterContextInvokeSync(BluezGattAppRegisterImpl, this); VerifyOrReturnError(err == CHIP_NO_ERROR, CHIP_ERROR_INCORRECT_STATE, - ChipLogError(Ble, "Failed to schedule BluezPeripheralRegisterApp() on CHIPoBluez thread")); + ChipLogError(Ble, "Failed to schedule BluezGattAppRegister() on CHIPoBluez thread")); return err; } diff --git a/src/platform/Linux/bluez/BluezEndpoint.h b/src/platform/Linux/bluez/BluezEndpoint.h index f241e130110f48..a747f3aebd8009 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.h +++ b/src/platform/Linux/bluez/BluezEndpoint.h @@ -85,15 +85,17 @@ class BluezEndpoint uint16_t mNumRetries = 0; }; + static CHIP_ERROR StartupEndpointBindings(BluezEndpoint * endpoint); + void BluezOnBusAcquired(GDBusConnection * aConn, const char * aName); + void SetupAdapter(); void SetupGattService(); BluezGattService1 * CreateGattService(const char * aUUID); BluezGattCharacteristic1 * CreateGattCharacteristic(BluezGattService1 * aService, const char * aCharName, const char * aUUID); + BluezLEAdvertisement1 * CreateLEAdvertisement(); - void BluezOnBusAcquired(GDBusConnection * aConn, const char * aName); - - static CHIP_ERROR StartupEndpointBindings(BluezEndpoint * endpoint); + static CHIP_ERROR BluezGattAppRegisterImpl(BluezEndpoint * endpoint); static void ConnectDeviceDone(GObject * aObject, GAsyncResult * aResult, gpointer apParams); static CHIP_ERROR ConnectDeviceImpl(ConnectParams * apParams); From d2a32f317a3171578c3039b8b5281687d252fad7 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Fri, 27 Oct 2023 09:46:13 +0200 Subject: [PATCH 08/25] Reduce number of signals on the D-Bus during initialization --- src/platform/Linux/BLEManagerImpl.cpp | 2 +- src/platform/Linux/bluez/BluezEndpoint.cpp | 21 ++++++++++++--------- src/platform/Linux/bluez/BluezEndpoint.h | 6 +++--- 3 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/platform/Linux/BLEManagerImpl.cpp b/src/platform/Linux/BLEManagerImpl.cpp index 96f1cf012cd30a..979f9bfa68510f 100644 --- a/src/platform/Linux/BLEManagerImpl.cpp +++ b/src/platform/Linux/BLEManagerImpl.cpp @@ -580,7 +580,7 @@ void BLEManagerImpl::DriveBLEState() // Register the CHIPoBLE application with the Bluez BLE layer if needed. if (!mIsCentral && mServiceMode == ConnectivityManager::kCHIPoBLEServiceMode_Enabled && !mFlags.Has(Flags::kAppRegistered)) { - SuccessOrExit(err = mEndpoint->BluezGattAppRegister()); + SuccessOrExit(err = mEndpoint->RegisterGattApplication()); mFlags.Set(Flags::kControlOpInProgress); ExitNow(); } diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index 6b348097674e2b..a550a30a7bfea7 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -282,7 +282,7 @@ static void BluezPeripheralRegisterAppDone(GObject * aObject, GAsyncResult * aRe ChipLogDetail(DeviceLayer, "BluezPeripheralRegisterAppDone done"); } -CHIP_ERROR BluezEndpoint::BluezGattAppRegisterImpl(BluezEndpoint * endpoint) +CHIP_ERROR BluezEndpoint::RegisterGattApplicationImpl(BluezEndpoint * endpoint) { GDBusObject * adapter; BluezGattManager1 * gattMgr; @@ -573,16 +573,18 @@ void BluezEndpoint::SetupGattService() #endif } -void BluezEndpoint::BluezOnBusAcquired(GDBusConnection * aConn, const char * aName) +void BluezEndpoint::SetupGattServer(GDBusConnection * aConn) { - ChipLogDetail(DeviceLayer, "TRACE: Bus acquired for name %s", aName); - VerifyOrReturn(!mIsCentral, ); + VerifyOrReturn(!mIsCentral); mpRootPath = g_strdup_printf("/chipoble/%04x", getpid() & 0xffff); mpRoot = g_dbus_object_manager_server_new(mpRootPath); - g_dbus_object_manager_server_set_connection(mpRoot, aConn); SetupGattService(); + + // Set connection after the service is set up in order to reduce the number + // of signals sent on the bus. + g_dbus_object_manager_server_set_connection(mpRoot, aConn); } CHIP_ERROR BluezEndpoint::StartupEndpointBindings(BluezEndpoint * endpoint) @@ -596,8 +598,9 @@ CHIP_ERROR BluezEndpoint::StartupEndpointBindings(BluezEndpoint * endpoint) endpoint->mpOwningName = g_strdup_printf("%s", endpoint->mpAdapterName); else endpoint->mpOwningName = g_strdup_printf("C-%04x", getpid() & 0xffff); + ChipLogDetail(DeviceLayer, "TRACE: Bus acquired for name %s", endpoint->mpOwningName); - endpoint->BluezOnBusAcquired(conn.get(), endpoint->mpOwningName); + endpoint->SetupGattServer(conn.get()); GDBusObjectManager * manager = g_dbus_object_manager_client_new_sync( conn.get(), G_DBUS_OBJECT_MANAGER_CLIENT_FLAGS_NONE, BLUEZ_INTERFACE, "/", bluez_object_manager_client_get_proxy_type, @@ -636,11 +639,11 @@ BluezEndpoint::~BluezEndpoint() g_object_unref(mpConnectCancellable); } -CHIP_ERROR BluezEndpoint::BluezGattAppRegister() +CHIP_ERROR BluezEndpoint::RegisterGattApplication() { - CHIP_ERROR err = PlatformMgrImpl().GLibMatterContextInvokeSync(BluezGattAppRegisterImpl, this); + CHIP_ERROR err = PlatformMgrImpl().GLibMatterContextInvokeSync(RegisterGattApplicationImpl, this); VerifyOrReturnError(err == CHIP_NO_ERROR, CHIP_ERROR_INCORRECT_STATE, - ChipLogError(Ble, "Failed to schedule BluezGattAppRegister() on CHIPoBluez thread")); + ChipLogError(Ble, "Failed to schedule RegisterGattApplication() on CHIPoBluez thread")); return err; } diff --git a/src/platform/Linux/bluez/BluezEndpoint.h b/src/platform/Linux/bluez/BluezEndpoint.h index a747f3aebd8009..df378290feca39 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.h +++ b/src/platform/Linux/bluez/BluezEndpoint.h @@ -69,7 +69,7 @@ class BluezEndpoint CHIP_ERROR Init(const char * apBleAddr, const char * apBleName); void Shutdown(); - CHIP_ERROR BluezGattAppRegister(); + CHIP_ERROR RegisterGattApplication(); CHIP_ERROR ConnectDevice(BluezDevice1 & aDevice); void CancelConnect(); @@ -86,16 +86,16 @@ class BluezEndpoint }; static CHIP_ERROR StartupEndpointBindings(BluezEndpoint * endpoint); - void BluezOnBusAcquired(GDBusConnection * aConn, const char * aName); void SetupAdapter(); + void SetupGattServer(GDBusConnection * aConn); void SetupGattService(); BluezGattService1 * CreateGattService(const char * aUUID); BluezGattCharacteristic1 * CreateGattCharacteristic(BluezGattService1 * aService, const char * aCharName, const char * aUUID); BluezLEAdvertisement1 * CreateLEAdvertisement(); - static CHIP_ERROR BluezGattAppRegisterImpl(BluezEndpoint * endpoint); + static CHIP_ERROR RegisterGattApplicationImpl(BluezEndpoint * endpoint); static void ConnectDeviceDone(GObject * aObject, GAsyncResult * aResult, gpointer apParams); static CHIP_ERROR ConnectDeviceImpl(ConnectParams * apParams); From 0160ca6e187c96bb949d7b79a9968979b26f365d Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Fri, 27 Oct 2023 10:01:05 +0200 Subject: [PATCH 09/25] Keep D-Bus callbacks in BluezEndpoint class --- src/platform/Linux/bluez/BluezEndpoint.cpp | 49 ++++++++++------------ src/platform/Linux/bluez/BluezEndpoint.h | 10 ++++- 2 files changed, 32 insertions(+), 27 deletions(-) diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index a550a30a7bfea7..a7a7bb8327037b 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -308,10 +308,10 @@ CHIP_ERROR BluezEndpoint::RegisterGattApplicationImpl(BluezEndpoint * endpoint) } /// Update the table of open BLE connections whenever a new device is spotted or its attributes have changed. -static void UpdateConnectionTable(BluezDevice1 * apDevice, BluezEndpoint & aEndpoint) +void BluezEndpoint::UpdateConnectionTable(BluezDevice1 * apDevice) { const char * objectPath = g_dbus_proxy_get_object_path(G_DBUS_PROXY(apDevice)); - BluezConnection * connection = static_cast(g_hash_table_lookup(aEndpoint.mpConnMap, objectPath)); + BluezConnection * connection = static_cast(g_hash_table_lookup(mpConnMap, objectPath)); if (connection != nullptr && !bluez_device1_get_connected(apDevice)) { @@ -320,34 +320,33 @@ static void UpdateConnectionTable(BluezDevice1 * apDevice, BluezEndpoint & aEndp // TODO: the connection object should be released after BLEManagerImpl finishes cleaning up its resources // after the disconnection. Releasing it here doesn't cause any issues, but it's error-prone. chip::Platform::Delete(connection); - g_hash_table_remove(aEndpoint.mpConnMap, objectPath); + g_hash_table_remove(mpConnMap, objectPath); return; } - if (connection == nullptr && !bluez_device1_get_connected(apDevice) && aEndpoint.mIsCentral) + if (connection == nullptr && !bluez_device1_get_connected(apDevice) && mIsCentral) { return; } if (connection == nullptr && bluez_device1_get_connected(apDevice) && - (!aEndpoint.mIsCentral || bluez_device1_get_services_resolved(apDevice))) + (!mIsCentral || bluez_device1_get_services_resolved(apDevice))) { - connection = chip::Platform::New(&aEndpoint, apDevice); - aEndpoint.mpPeerDevicePath = g_strdup(objectPath); - g_hash_table_insert(aEndpoint.mpConnMap, aEndpoint.mpPeerDevicePath, connection); + connection = chip::Platform::New(this, apDevice); + mpPeerDevicePath = g_strdup(objectPath); + g_hash_table_insert(mpConnMap, mpPeerDevicePath, connection); ChipLogDetail(DeviceLayer, "New BLE connection: conn %p, device %s, path %s", connection, connection->GetPeerAddress(), - aEndpoint.mpPeerDevicePath); + mpPeerDevicePath); BLEManagerImpl::HandleNewConnection(connection); } } -static void BluezSignalInterfacePropertiesChanged(GDBusObjectManagerClient * aManager, GDBusObjectProxy * aObject, - GDBusProxy * aInterface, GVariant * aChangedProperties, - const gchar * const * aInvalidatedProps, gpointer apClosure) +void BluezEndpoint::BluezSignalInterfacePropertiesChanged(GDBusObjectManagerClient * aManager, GDBusObjectProxy * aObject, + GDBusProxy * aInterface, GVariant * aChangedProperties, + const char * const * aInvalidatedProps, BluezEndpoint * endpoint) { - BluezEndpoint * endpoint = static_cast(apClosure); VerifyOrReturn(endpoint != nullptr, ChipLogError(DeviceLayer, "endpoint is NULL in %s", __func__)); VerifyOrReturn(endpoint->mpAdapter != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL endpoint->mpAdapter in %s", __func__)); VerifyOrReturn(strcmp(g_dbus_proxy_get_interface_name(aInterface), DEVICE_INTERFACE) == 0, ); @@ -355,12 +354,12 @@ static void BluezSignalInterfacePropertiesChanged(GDBusObjectManagerClient * aMa BluezDevice1 * device = BLUEZ_DEVICE1(aInterface); VerifyOrReturn(BluezIsDeviceOnAdapter(device, endpoint->mpAdapter)); - UpdateConnectionTable(device, *endpoint); + endpoint->UpdateConnectionTable(device); } -static void BluezHandleNewDevice(BluezDevice1 * device, BluezEndpoint & aEndpoint) +void BluezEndpoint::HandleNewDevice(BluezDevice1 * device) { - VerifyOrReturn(!aEndpoint.mIsCentral); + VerifyOrReturn(!mIsCentral); // We need to handle device connection both this function and BluezSignalInterfacePropertiesChanged // When a device is connected for first time, this function will be triggered. @@ -369,24 +368,22 @@ static void BluezHandleNewDevice(BluezDevice1 * device, BluezEndpoint & aEndpoin BluezConnection * conn; VerifyOrExit(bluez_device1_get_connected(device), ChipLogError(DeviceLayer, "FAIL: device is not connected")); - conn = static_cast( - g_hash_table_lookup(aEndpoint.mpConnMap, g_dbus_proxy_get_object_path(G_DBUS_PROXY(device)))); + conn = static_cast(g_hash_table_lookup(mpConnMap, g_dbus_proxy_get_object_path(G_DBUS_PROXY(device)))); VerifyOrExit(conn == nullptr, ChipLogError(DeviceLayer, "FAIL: connection already tracked: conn: %p new device: %s", conn, g_dbus_proxy_get_object_path(G_DBUS_PROXY(device)))); - conn = chip::Platform::New(&aEndpoint, device); - aEndpoint.mpPeerDevicePath = g_strdup(g_dbus_proxy_get_object_path(G_DBUS_PROXY(device))); - g_hash_table_insert(aEndpoint.mpConnMap, aEndpoint.mpPeerDevicePath, conn); + conn = chip::Platform::New(this, device); + mpPeerDevicePath = g_strdup(g_dbus_proxy_get_object_path(G_DBUS_PROXY(device))); + g_hash_table_insert(mpConnMap, mpPeerDevicePath, conn); - ChipLogDetail(DeviceLayer, "BLE device connected: conn %p, device %s, path %s", conn, conn->GetPeerAddress(), - aEndpoint.mpPeerDevicePath); + ChipLogDetail(DeviceLayer, "BLE device connected: conn %p, device %s, path %s", conn, conn->GetPeerAddress(), mpPeerDevicePath); exit: return; } -static void BluezSignalOnObjectAdded(GDBusObjectManager * aManager, GDBusObject * aObject, BluezEndpoint * endpoint) +void BluezEndpoint::BluezSignalOnObjectAdded(GDBusObjectManager * aManager, GDBusObject * aObject, BluezEndpoint * endpoint) { // TODO: right now we do not handle addition/removal of adapters // Primary focus here is to handle addition of a device @@ -395,11 +392,11 @@ static void BluezSignalOnObjectAdded(GDBusObjectManager * aManager, GDBusObject if (BluezIsDeviceOnAdapter(device.get(), endpoint->mpAdapter) == TRUE) { - BluezHandleNewDevice(device.get(), *endpoint); + endpoint->HandleNewDevice(device.get()); } } -static void BluezSignalOnObjectRemoved(GDBusObjectManager * aManager, GDBusObject * aObject, gpointer apClosure) +void BluezEndpoint::BluezSignalOnObjectRemoved(GDBusObjectManager * aManager, GDBusObject * aObject, BluezEndpoint * endpoint) { // TODO: for Device1, lookup connection, and call otPlatTobleHandleDisconnected // for Adapter1: unclear, crash if this pertains to our adapter? at least null out the endpoint->mpAdapter. diff --git a/src/platform/Linux/bluez/BluezEndpoint.h b/src/platform/Linux/bluez/BluezEndpoint.h index df378290feca39..a7f4ff78342250 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.h +++ b/src/platform/Linux/bluez/BluezEndpoint.h @@ -93,7 +93,15 @@ class BluezEndpoint BluezGattService1 * CreateGattService(const char * aUUID); BluezGattCharacteristic1 * CreateGattCharacteristic(BluezGattService1 * aService, const char * aCharName, const char * aUUID); - BluezLEAdvertisement1 * CreateLEAdvertisement(); + + void HandleNewDevice(BluezDevice1 * aDevice); + void UpdateConnectionTable(BluezDevice1 * aDevice); + + static void BluezSignalOnObjectAdded(GDBusObjectManager * aManager, GDBusObject * aObject, BluezEndpoint * endpoint); + static void BluezSignalOnObjectRemoved(GDBusObjectManager * aManager, GDBusObject * aObject, BluezEndpoint * endpoint); + static void BluezSignalInterfacePropertiesChanged(GDBusObjectManagerClient * aManager, GDBusObjectProxy * aObject, + GDBusProxy * aInterface, GVariant * aChangedProperties, + const char * const * aInvalidatedProps, BluezEndpoint * endpoint); static CHIP_ERROR RegisterGattApplicationImpl(BluezEndpoint * endpoint); From c2d12e427f88e03b67b2fd540b75bcc66adaf603 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Fri, 27 Oct 2023 10:41:12 +0200 Subject: [PATCH 10/25] Keep signal callbacks in BluezEndpoint class --- src/platform/Linux/bluez/BluezEndpoint.cpp | 93 ++++++++++------------ src/platform/Linux/bluez/BluezEndpoint.h | 26 ++++-- 2 files changed, 61 insertions(+), 58 deletions(-) diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index a7a7bb8327037b..e6d13e8bb8b318 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -65,6 +65,7 @@ #include #include #include +#include #include #include #include @@ -79,12 +80,11 @@ namespace Internal { constexpr uint16_t kMaxConnectRetries = 4; -static gboolean BluezCharacteristicReadValue(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInvocation, - GVariant * aOptions) +gboolean BluezEndpoint::BluezCharacteristicReadValue(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInvocation, + GVariant * aOptions, BluezEndpoint * self) { - GVariant * val; - ChipLogDetail(DeviceLayer, "Received BluezCharacteristicReadValue"); - val = bluez_gatt_characteristic1_get_value(aChar); + ChipLogDetail(DeviceLayer, "Received %s", __func__); + GVariant * val = bluez_gatt_characteristic1_get_value(aChar); bluez_gatt_characteristic1_complete_read_value(aChar, aInvocation, val); return TRUE; } @@ -98,8 +98,8 @@ static void Bluez_gatt_characteristic1_complete_acquire_write_with_fd(GDBusMetho g_object_unref(fd_list); } -static gboolean BluezCharacteristicAcquireWrite(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInvocation, - GVariant * aOptions, gpointer apEndpoint) +gboolean BluezEndpoint::BluezCharacteristicAcquireWrite(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInvocation, + GVariant * aOptions, BluezEndpoint * self) { int fds[2] = { -1, -1 }; #if CHIP_ERROR_LOGGING @@ -109,11 +109,7 @@ static gboolean BluezCharacteristicAcquireWrite(BluezGattCharacteristic1 * aChar GAutoPtr option_mtu; uint16_t mtu; - BluezEndpoint * endpoint = static_cast(apEndpoint); - - VerifyOrReturnValue(endpoint != nullptr, FALSE, ChipLogError(DeviceLayer, "endpoint is NULL in %s", __func__)); - - conn = endpoint->GetBluezConnectionViaDevice(); + conn = self->GetBluezConnectionViaDevice(); VerifyOrReturnValue( conn != nullptr, FALSE, g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.Failed", "No Chipoble connection")); @@ -147,14 +143,14 @@ static gboolean BluezCharacteristicAcquireWrite(BluezGattCharacteristic1 * aChar static gboolean BluezCharacteristicAcquireWriteError(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInvocation, GVariant * aOptions) { - ChipLogDetail(DeviceLayer, "BluezCharacteristicAcquireWriteError is called"); + ChipLogDetail(DeviceLayer, "Received %s", __func__); g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.NotSupported", "AcquireWrite for characteristic is unsupported"); return TRUE; } -static gboolean BluezCharacteristicAcquireNotify(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInvocation, - GVariant * aOptions, gpointer apEndpoint) +gboolean BluezEndpoint::BluezCharacteristicAcquireNotify(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInvocation, + GVariant * aOptions, BluezEndpoint * self) { int fds[2] = { -1, -1 }; #if CHIP_ERROR_LOGGING @@ -165,11 +161,8 @@ static gboolean BluezCharacteristicAcquireNotify(BluezGattCharacteristic1 * aCha GAutoPtr option_mtu; uint16_t mtu; - BluezEndpoint * endpoint = static_cast(apEndpoint); - VerifyOrReturnValue(endpoint != nullptr, FALSE, ChipLogError(DeviceLayer, "endpoint is NULL in %s", __func__)); - #if CHIP_ENABLE_ADDITIONAL_DATA_ADVERTISING - isAdditionalAdvertising = (aChar == endpoint->mpC3); + isAdditionalAdvertising = (aChar == self->mpC3); #endif if (bluez_gatt_characteristic1_get_notifying(aChar)) @@ -178,7 +171,7 @@ static gboolean BluezCharacteristicAcquireNotify(BluezGattCharacteristic1 * aCha return FALSE; } - conn = endpoint->GetBluezConnectionViaDevice(); + conn = self->GetBluezConnectionViaDevice(); VerifyOrReturnValue( conn != nullptr, FALSE, g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.Failed", "No Chipoble connection")); @@ -214,17 +207,16 @@ static gboolean BluezCharacteristicAcquireNotify(BluezGattCharacteristic1 * aCha static gboolean BluezCharacteristicAcquireNotifyError(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInvocation, GVariant * aOptions) { - ChipLogDetail(DeviceLayer, "TRACE: AcquireNotify is called"); + ChipLogDetail(DeviceLayer, "Received %s", __func__); g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.NotSupported", "AcquireNotify for characteristic is unsupported"); return TRUE; } -static gboolean BluezCharacteristicConfirm(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInvocation, - gpointer apClosure) +gboolean BluezEndpoint::BluezCharacteristicConfirm(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInvocation, + BluezEndpoint * self) { - BluezEndpoint * endpoint = static_cast(apClosure); - BluezConnection * conn = endpoint->GetBluezConnectionViaDevice(); + BluezConnection * conn = self->GetBluezConnectionViaDevice(); ChipLogDetail(Ble, "Indication confirmation, %p", conn); BLEManagerImpl::HandleTXComplete(conn); @@ -282,16 +274,16 @@ static void BluezPeripheralRegisterAppDone(GObject * aObject, GAsyncResult * aRe ChipLogDetail(DeviceLayer, "BluezPeripheralRegisterAppDone done"); } -CHIP_ERROR BluezEndpoint::RegisterGattApplicationImpl(BluezEndpoint * endpoint) +CHIP_ERROR BluezEndpoint::RegisterGattApplicationImpl(BluezEndpoint * self) { GDBusObject * adapter; BluezGattManager1 * gattMgr; GVariantBuilder optionsBuilder; GVariant * options; - VerifyOrExit(endpoint->mpAdapter != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL endpoint->mpAdapter in %s", __func__)); + VerifyOrExit(self->mpAdapter != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL mpAdapter in %s", __func__)); - adapter = g_dbus_interface_get_object(G_DBUS_INTERFACE(endpoint->mpAdapter)); + adapter = g_dbus_interface_get_object(G_DBUS_INTERFACE(self->mpAdapter)); VerifyOrExit(adapter != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL adapter in %s", __func__)); gattMgr = bluez_object_get_gatt_manager1(BLUEZ_OBJECT(adapter)); @@ -300,7 +292,7 @@ CHIP_ERROR BluezEndpoint::RegisterGattApplicationImpl(BluezEndpoint * endpoint) g_variant_builder_init(&optionsBuilder, G_VARIANT_TYPE("a{sv}")); options = g_variant_builder_end(&optionsBuilder); - bluez_gatt_manager1_call_register_application(gattMgr, endpoint->mpRootPath, options, nullptr, BluezPeripheralRegisterAppDone, + bluez_gatt_manager1_call_register_application(gattMgr, self->mpRootPath, options, nullptr, BluezPeripheralRegisterAppDone, nullptr); exit: @@ -345,16 +337,15 @@ void BluezEndpoint::UpdateConnectionTable(BluezDevice1 * apDevice) void BluezEndpoint::BluezSignalInterfacePropertiesChanged(GDBusObjectManagerClient * aManager, GDBusObjectProxy * aObject, GDBusProxy * aInterface, GVariant * aChangedProperties, - const char * const * aInvalidatedProps, BluezEndpoint * endpoint) + const char * const * aInvalidatedProps, BluezEndpoint * self) { - VerifyOrReturn(endpoint != nullptr, ChipLogError(DeviceLayer, "endpoint is NULL in %s", __func__)); - VerifyOrReturn(endpoint->mpAdapter != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL endpoint->mpAdapter in %s", __func__)); + VerifyOrReturn(self->mpAdapter != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL mpAdapter in %s", __func__)); VerifyOrReturn(strcmp(g_dbus_proxy_get_interface_name(aInterface), DEVICE_INTERFACE) == 0, ); BluezDevice1 * device = BLUEZ_DEVICE1(aInterface); - VerifyOrReturn(BluezIsDeviceOnAdapter(device, endpoint->mpAdapter)); + VerifyOrReturn(BluezIsDeviceOnAdapter(device, self->mpAdapter)); - endpoint->UpdateConnectionTable(device); + self->UpdateConnectionTable(device); } void BluezEndpoint::HandleNewDevice(BluezDevice1 * device) @@ -383,23 +374,23 @@ void BluezEndpoint::HandleNewDevice(BluezDevice1 * device) return; } -void BluezEndpoint::BluezSignalOnObjectAdded(GDBusObjectManager * aManager, GDBusObject * aObject, BluezEndpoint * endpoint) +void BluezEndpoint::BluezSignalOnObjectAdded(GDBusObjectManager * aManager, GDBusObject * aObject, BluezEndpoint * self) { // TODO: right now we do not handle addition/removal of adapters // Primary focus here is to handle addition of a device GAutoPtr device(bluez_object_get_device1(BLUEZ_OBJECT(aObject))); VerifyOrReturn(device.get() != nullptr); - if (BluezIsDeviceOnAdapter(device.get(), endpoint->mpAdapter) == TRUE) + if (BluezIsDeviceOnAdapter(device.get(), self->mpAdapter) == TRUE) { - endpoint->HandleNewDevice(device.get()); + self->HandleNewDevice(device.get()); } } -void BluezEndpoint::BluezSignalOnObjectRemoved(GDBusObjectManager * aManager, GDBusObject * aObject, BluezEndpoint * endpoint) +void BluezEndpoint::BluezSignalOnObjectRemoved(GDBusObjectManager * aManager, GDBusObject * aObject, BluezEndpoint * self) { // TODO: for Device1, lookup connection, and call otPlatTobleHandleDisconnected - // for Adapter1: unclear, crash if this pertains to our adapter? at least null out the endpoint->mpAdapter. + // for Adapter1: unclear, crash if this pertains to our adapter? at least null out the self->mpAdapter. // for Characteristic1, or GattService -- handle here via calling otPlatTobleHandleDisconnected, or ignore. } @@ -537,12 +528,12 @@ void BluezEndpoint::SetupGattService() // C1 characteristic mpC1 = CreateGattCharacteristic(mpService, "c1", CHIP_PLAT_BLE_UUID_C1_STRING); bluez_gatt_characteristic1_set_flags(mpC1, c1_flags); - g_signal_connect(mpC1, "handle-read-value", G_CALLBACK(BluezCharacteristicReadValue), this); g_signal_connect(mpC1, "handle-acquire-write", G_CALLBACK(BluezCharacteristicAcquireWrite), this); g_signal_connect(mpC1, "handle-acquire-notify", G_CALLBACK(BluezCharacteristicAcquireNotifyError), nullptr); g_signal_connect(mpC1, "handle-confirm", G_CALLBACK(BluezCharacteristicConfirmError), nullptr); + // C2 characteristic mpC2 = CreateGattCharacteristic(mpService, "c2", CHIP_PLAT_BLE_UUID_C2_STRING); bluez_gatt_characteristic1_set_flags(mpC2, c2_flags); g_signal_connect(mpC2, "handle-read-value", G_CALLBACK(BluezCharacteristicReadValue), this); @@ -584,20 +575,20 @@ void BluezEndpoint::SetupGattServer(GDBusConnection * aConn) g_dbus_object_manager_server_set_connection(mpRoot, aConn); } -CHIP_ERROR BluezEndpoint::StartupEndpointBindings(BluezEndpoint * endpoint) +CHIP_ERROR BluezEndpoint::StartupEndpointBindings(BluezEndpoint * self) { GAutoPtr err; GAutoPtr conn(g_bus_get_sync(G_BUS_TYPE_SYSTEM, nullptr, &MakeUniquePointerReceiver(err).Get())); VerifyOrReturnError(conn != nullptr, CHIP_ERROR_INTERNAL, ChipLogError(DeviceLayer, "FAIL: get bus sync in %s, error: %s", __func__, err->message)); - if (endpoint->mpAdapterName != nullptr) - endpoint->mpOwningName = g_strdup_printf("%s", endpoint->mpAdapterName); + if (self->mpAdapterName != nullptr) + self->mpOwningName = g_strdup_printf("%s", self->mpAdapterName); else - endpoint->mpOwningName = g_strdup_printf("C-%04x", getpid() & 0xffff); - ChipLogDetail(DeviceLayer, "TRACE: Bus acquired for name %s", endpoint->mpOwningName); + self->mpOwningName = g_strdup_printf("C-%04x", getpid() & 0xffff); + ChipLogDetail(DeviceLayer, "TRACE: Bus acquired for name %s", self->mpOwningName); - endpoint->SetupGattServer(conn.get()); + self->SetupGattServer(conn.get()); GDBusObjectManager * manager = g_dbus_object_manager_client_new_sync( conn.get(), G_DBUS_OBJECT_MANAGER_CLIENT_FLAGS_NONE, BLUEZ_INTERFACE, "/", bluez_object_manager_client_get_proxy_type, @@ -606,12 +597,12 @@ CHIP_ERROR BluezEndpoint::StartupEndpointBindings(BluezEndpoint * endpoint) VerifyOrReturnError(manager != nullptr, CHIP_ERROR_INTERNAL, ChipLogError(DeviceLayer, "FAIL: Error getting object manager client: %s", err->message)); - endpoint->mpObjMgr = manager; - endpoint->SetupAdapter(); + self->mpObjMgr = manager; + self->SetupAdapter(); - g_signal_connect(manager, "object-added", G_CALLBACK(BluezSignalOnObjectAdded), endpoint); - g_signal_connect(manager, "object-removed", G_CALLBACK(BluezSignalOnObjectRemoved), endpoint); - g_signal_connect(manager, "interface-proxy-properties-changed", G_CALLBACK(BluezSignalInterfacePropertiesChanged), endpoint); + g_signal_connect(manager, "object-added", G_CALLBACK(BluezSignalOnObjectAdded), self); + g_signal_connect(manager, "object-removed", G_CALLBACK(BluezSignalOnObjectRemoved), self); + g_signal_connect(manager, "interface-proxy-properties-changed", G_CALLBACK(BluezSignalInterfacePropertiesChanged), self); return CHIP_NO_ERROR; } diff --git a/src/platform/Linux/bluez/BluezEndpoint.h b/src/platform/Linux/bluez/BluezEndpoint.h index a7f4ff78342250..7c5e2c224f42da 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.h +++ b/src/platform/Linux/bluez/BluezEndpoint.h @@ -52,6 +52,7 @@ #include #include +#include #include "BluezConnection.h" #include "Types.h" @@ -85,7 +86,7 @@ class BluezEndpoint uint16_t mNumRetries = 0; }; - static CHIP_ERROR StartupEndpointBindings(BluezEndpoint * endpoint); + static CHIP_ERROR StartupEndpointBindings(BluezEndpoint * self); void SetupAdapter(); void SetupGattServer(GDBusConnection * aConn); @@ -96,21 +97,32 @@ class BluezEndpoint void HandleNewDevice(BluezDevice1 * aDevice); void UpdateConnectionTable(BluezDevice1 * aDevice); + BluezConnection * GetBluezConnectionViaDevice(); + + static gboolean BluezAdvertisingRelease(BluezLEAdvertisement1 * aAdv, GDBusMethodInvocation * aInvocation, + BluezEndpoint * self); + + static gboolean BluezCharacteristicReadValue(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInvocation, + GVariant * aOptions, BluezEndpoint * self); + static gboolean BluezCharacteristicAcquireWrite(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInvocation, + GVariant * aOptions, BluezEndpoint * self); + static gboolean BluezCharacteristicAcquireNotify(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInvocation, + GVariant * aOptions, BluezEndpoint * self); + static gboolean BluezCharacteristicConfirm(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInvocation, + BluezEndpoint * self); - static void BluezSignalOnObjectAdded(GDBusObjectManager * aManager, GDBusObject * aObject, BluezEndpoint * endpoint); - static void BluezSignalOnObjectRemoved(GDBusObjectManager * aManager, GDBusObject * aObject, BluezEndpoint * endpoint); + static void BluezSignalOnObjectAdded(GDBusObjectManager * aManager, GDBusObject * aObject, BluezEndpoint * self); + static void BluezSignalOnObjectRemoved(GDBusObjectManager * aManager, GDBusObject * aObject, BluezEndpoint * self); static void BluezSignalInterfacePropertiesChanged(GDBusObjectManagerClient * aManager, GDBusObjectProxy * aObject, GDBusProxy * aInterface, GVariant * aChangedProperties, - const char * const * aInvalidatedProps, BluezEndpoint * endpoint); + const char * const * aInvalidatedProps, BluezEndpoint * self); - static CHIP_ERROR RegisterGattApplicationImpl(BluezEndpoint * endpoint); + static CHIP_ERROR RegisterGattApplicationImpl(BluezEndpoint * self); static void ConnectDeviceDone(GObject * aObject, GAsyncResult * aResult, gpointer apParams); static CHIP_ERROR ConnectDeviceImpl(ConnectParams * apParams); public: - BluezConnection * GetBluezConnectionViaDevice(); - // Bus owning name char * mpOwningName = nullptr; From db0030800c3e58d0ff0aee270988b3c78e0ca605 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Fri, 27 Oct 2023 11:23:38 +0200 Subject: [PATCH 11/25] Get rid of explicit passing of this pointer --- src/platform/Linux/bluez/BluezEndpoint.cpp | 37 +++++++++++----------- src/platform/Linux/bluez/BluezEndpoint.h | 4 +-- 2 files changed, 21 insertions(+), 20 deletions(-) diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index e6d13e8bb8b318..9ffc1ecb3c7b96 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -274,16 +274,16 @@ static void BluezPeripheralRegisterAppDone(GObject * aObject, GAsyncResult * aRe ChipLogDetail(DeviceLayer, "BluezPeripheralRegisterAppDone done"); } -CHIP_ERROR BluezEndpoint::RegisterGattApplicationImpl(BluezEndpoint * self) +CHIP_ERROR BluezEndpoint::RegisterGattApplicationImpl() { GDBusObject * adapter; BluezGattManager1 * gattMgr; GVariantBuilder optionsBuilder; GVariant * options; - VerifyOrExit(self->mpAdapter != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL mpAdapter in %s", __func__)); + VerifyOrExit(mpAdapter != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL mpAdapter in %s", __func__)); - adapter = g_dbus_interface_get_object(G_DBUS_INTERFACE(self->mpAdapter)); + adapter = g_dbus_interface_get_object(G_DBUS_INTERFACE(mpAdapter)); VerifyOrExit(adapter != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL adapter in %s", __func__)); gattMgr = bluez_object_get_gatt_manager1(BLUEZ_OBJECT(adapter)); @@ -292,8 +292,7 @@ CHIP_ERROR BluezEndpoint::RegisterGattApplicationImpl(BluezEndpoint * self) g_variant_builder_init(&optionsBuilder, G_VARIANT_TYPE("a{sv}")); options = g_variant_builder_end(&optionsBuilder); - bluez_gatt_manager1_call_register_application(gattMgr, self->mpRootPath, options, nullptr, BluezPeripheralRegisterAppDone, - nullptr); + bluez_gatt_manager1_call_register_application(gattMgr, mpRootPath, options, nullptr, BluezPeripheralRegisterAppDone, nullptr); exit: return CHIP_NO_ERROR; @@ -575,20 +574,20 @@ void BluezEndpoint::SetupGattServer(GDBusConnection * aConn) g_dbus_object_manager_server_set_connection(mpRoot, aConn); } -CHIP_ERROR BluezEndpoint::StartupEndpointBindings(BluezEndpoint * self) +CHIP_ERROR BluezEndpoint::StartupEndpointBindings() { GAutoPtr err; GAutoPtr conn(g_bus_get_sync(G_BUS_TYPE_SYSTEM, nullptr, &MakeUniquePointerReceiver(err).Get())); VerifyOrReturnError(conn != nullptr, CHIP_ERROR_INTERNAL, ChipLogError(DeviceLayer, "FAIL: get bus sync in %s, error: %s", __func__, err->message)); - if (self->mpAdapterName != nullptr) - self->mpOwningName = g_strdup_printf("%s", self->mpAdapterName); + if (mpAdapterName != nullptr) + mpOwningName = g_strdup_printf("%s", mpAdapterName); else - self->mpOwningName = g_strdup_printf("C-%04x", getpid() & 0xffff); - ChipLogDetail(DeviceLayer, "TRACE: Bus acquired for name %s", self->mpOwningName); + mpOwningName = g_strdup_printf("C-%04x", getpid() & 0xffff); + ChipLogDetail(DeviceLayer, "TRACE: Bus acquired for name %s", mpOwningName); - self->SetupGattServer(conn.get()); + SetupGattServer(conn.get()); GDBusObjectManager * manager = g_dbus_object_manager_client_new_sync( conn.get(), G_DBUS_OBJECT_MANAGER_CLIENT_FLAGS_NONE, BLUEZ_INTERFACE, "/", bluez_object_manager_client_get_proxy_type, @@ -597,12 +596,12 @@ CHIP_ERROR BluezEndpoint::StartupEndpointBindings(BluezEndpoint * self) VerifyOrReturnError(manager != nullptr, CHIP_ERROR_INTERNAL, ChipLogError(DeviceLayer, "FAIL: Error getting object manager client: %s", err->message)); - self->mpObjMgr = manager; - self->SetupAdapter(); + mpObjMgr = manager; + SetupAdapter(); - g_signal_connect(manager, "object-added", G_CALLBACK(BluezSignalOnObjectAdded), self); - g_signal_connect(manager, "object-removed", G_CALLBACK(BluezSignalOnObjectRemoved), self); - g_signal_connect(manager, "interface-proxy-properties-changed", G_CALLBACK(BluezSignalInterfacePropertiesChanged), self); + g_signal_connect(manager, "object-added", G_CALLBACK(BluezSignalOnObjectAdded), this); + g_signal_connect(manager, "object-removed", G_CALLBACK(BluezSignalOnObjectRemoved), this); + g_signal_connect(manager, "interface-proxy-properties-changed", G_CALLBACK(BluezSignalInterfacePropertiesChanged), this); return CHIP_NO_ERROR; } @@ -629,7 +628,8 @@ BluezEndpoint::~BluezEndpoint() CHIP_ERROR BluezEndpoint::RegisterGattApplication() { - CHIP_ERROR err = PlatformMgrImpl().GLibMatterContextInvokeSync(RegisterGattApplicationImpl, this); + CHIP_ERROR err = PlatformMgrImpl().GLibMatterContextInvokeSync( + +[](BluezEndpoint * self) { return self->RegisterGattApplicationImpl(); }, this); VerifyOrReturnError(err == CHIP_NO_ERROR, CHIP_ERROR_INCORRECT_STATE, ChipLogError(Ble, "Failed to schedule RegisterGattApplication() on CHIPoBluez thread")); return err; @@ -651,7 +651,8 @@ CHIP_ERROR BluezEndpoint::Init(const char * apBleAddr, const char * apBleName) mpConnectCancellable = g_cancellable_new(); } - err = PlatformMgrImpl().GLibMatterContextInvokeSync(StartupEndpointBindings, this); + err = PlatformMgrImpl().GLibMatterContextInvokeSync( + +[](BluezEndpoint * self) { return self->StartupEndpointBindings(); }, this); VerifyOrReturnError(err == CHIP_NO_ERROR, err, ChipLogError(DeviceLayer, "Failed to schedule endpoint initialization")); ChipLogDetail(DeviceLayer, "PlatformBlueZInit init success"); diff --git a/src/platform/Linux/bluez/BluezEndpoint.h b/src/platform/Linux/bluez/BluezEndpoint.h index 7c5e2c224f42da..e81e3c52736a54 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.h +++ b/src/platform/Linux/bluez/BluezEndpoint.h @@ -86,7 +86,7 @@ class BluezEndpoint uint16_t mNumRetries = 0; }; - static CHIP_ERROR StartupEndpointBindings(BluezEndpoint * self); + CHIP_ERROR StartupEndpointBindings(); void SetupAdapter(); void SetupGattServer(GDBusConnection * aConn); @@ -117,7 +117,7 @@ class BluezEndpoint GDBusProxy * aInterface, GVariant * aChangedProperties, const char * const * aInvalidatedProps, BluezEndpoint * self); - static CHIP_ERROR RegisterGattApplicationImpl(BluezEndpoint * self); + CHIP_ERROR RegisterGattApplicationImpl(); static void ConnectDeviceDone(GObject * aObject, GAsyncResult * aResult, gpointer apParams); static CHIP_ERROR ConnectDeviceImpl(ConnectParams * apParams); From b567a250fe6f305a3d7cf9f97471cceaaa9ecf5b Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Fri, 27 Oct 2023 11:40:19 +0200 Subject: [PATCH 12/25] Keep data of BluezEndpoint private --- src/platform/Linux/BLEManagerImpl.cpp | 4 ++-- src/platform/Linux/bluez/BluezEndpoint.h | 6 +++++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/platform/Linux/BLEManagerImpl.cpp b/src/platform/Linux/BLEManagerImpl.cpp index 979f9bfa68510f..b9af868dd6749e 100644 --- a/src/platform/Linux/BLEManagerImpl.cpp +++ b/src/platform/Linux/BLEManagerImpl.cpp @@ -660,7 +660,7 @@ void BLEManagerImpl::InitiateScan(BleScanState scanType) return; } - if (mEndpoint->mpAdapter == nullptr) + if (mEndpoint->GetAdapter() == nullptr) { BleConnectionDelegate::OnConnectionError(mBLEScanConfig.mAppState, CHIP_ERROR_INCORRECT_STATE); ChipLogError(Ble, "No adapter available for new connection establishment"); @@ -669,7 +669,7 @@ void BLEManagerImpl::InitiateScan(BleScanState scanType) mBLEScanConfig.mBleScanState = scanType; - CHIP_ERROR err = mDeviceScanner.Init(mEndpoint->mpAdapter, this); + CHIP_ERROR err = mDeviceScanner.Init(mEndpoint->GetAdapter(), this); if (err != CHIP_NO_ERROR) { mBLEScanConfig.mBleScanState = BleScanState::kNotScanning; diff --git a/src/platform/Linux/bluez/BluezEndpoint.h b/src/platform/Linux/bluez/BluezEndpoint.h index e81e3c52736a54..1c9d84e859518d 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.h +++ b/src/platform/Linux/bluez/BluezEndpoint.h @@ -70,6 +70,8 @@ class BluezEndpoint CHIP_ERROR Init(const char * apBleAddr, const char * apBleName); void Shutdown(); + BluezAdapter1 * GetAdapter() const { return mpAdapter; } + CHIP_ERROR RegisterGattApplication(); CHIP_ERROR ConnectDevice(BluezDevice1 & aDevice); @@ -122,7 +124,6 @@ class BluezEndpoint static void ConnectDeviceDone(GObject * aObject, GAsyncResult * aResult, gpointer apParams); static CHIP_ERROR ConnectDeviceImpl(ConnectParams * apParams); -public: // Bus owning name char * mpOwningName = nullptr; @@ -153,6 +154,9 @@ class BluezEndpoint bool mIsCentral = false; char * mpPeerDevicePath = nullptr; GCancellable * mpConnectCancellable = nullptr; + + // Allow BluezConnection to access our private members + friend class BluezConnection; }; } // namespace Internal From 7064ad4a89c5ae8ec09da83c02408f55a00ccbc3 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Fri, 27 Oct 2023 11:44:35 +0200 Subject: [PATCH 13/25] Fixup for naming convention --- src/platform/Linux/bluez/BluezAdvertisement.h | 2 +- src/platform/Linux/bluez/BluezConnection.h | 2 +- src/platform/Linux/bluez/BluezEndpoint.cpp | 4 ++-- src/platform/Linux/bluez/BluezEndpoint.h | 9 ++++++--- 4 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/platform/Linux/bluez/BluezAdvertisement.h b/src/platform/Linux/bluez/BluezAdvertisement.h index 7fc01e027d38e0..cf65313a351e49 100644 --- a/src/platform/Linux/bluez/BluezAdvertisement.h +++ b/src/platform/Linux/bluez/BluezAdvertisement.h @@ -33,7 +33,7 @@ namespace chip { namespace DeviceLayer { namespace Internal { -struct BluezEndpoint; +class BluezEndpoint; class BluezAdvertisement { diff --git a/src/platform/Linux/bluez/BluezConnection.h b/src/platform/Linux/bluez/BluezConnection.h index 3a64ac931220a8..7bf91103e95ebf 100644 --- a/src/platform/Linux/bluez/BluezConnection.h +++ b/src/platform/Linux/bluez/BluezConnection.h @@ -32,7 +32,7 @@ namespace chip { namespace DeviceLayer { namespace Internal { -struct BluezEndpoint; +class BluezEndpoint; class BluezConnection { diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index 9ffc1ecb3c7b96..85c6bc19f9a1a6 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -112,7 +112,7 @@ gboolean BluezEndpoint::BluezCharacteristicAcquireWrite(BluezGattCharacteristic1 conn = self->GetBluezConnectionViaDevice(); VerifyOrReturnValue( conn != nullptr, FALSE, - g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.Failed", "No Chipoble connection")); + g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.Failed", "No CHIPoBLE connection")); ChipLogDetail(DeviceLayer, "BluezCharacteristicAcquireWrite is called, conn: %p", conn); @@ -174,7 +174,7 @@ gboolean BluezEndpoint::BluezCharacteristicAcquireNotify(BluezGattCharacteristic conn = self->GetBluezConnectionViaDevice(); VerifyOrReturnValue( conn != nullptr, FALSE, - g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.Failed", "No Chipoble connection")); + g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.Failed", "No CHIPoBLE connection")); VerifyOrReturnValue( g_variant_lookup(aOptions, "mtu", "q", &mtu), FALSE, ChipLogError(DeviceLayer, "FAIL: No MTU in options in %s", __func__); diff --git a/src/platform/Linux/bluez/BluezEndpoint.h b/src/platform/Linux/bluez/BluezEndpoint.h index 1c9d84e859518d..6f8d0ec5b95821 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.h +++ b/src/platform/Linux/bluez/BluezEndpoint.h @@ -54,6 +54,7 @@ #include #include +#include "BluezAdvertisement.h" #include "BluezConnection.h" #include "Types.h" @@ -101,8 +102,8 @@ class BluezEndpoint void UpdateConnectionTable(BluezDevice1 * aDevice); BluezConnection * GetBluezConnectionViaDevice(); - static gboolean BluezAdvertisingRelease(BluezLEAdvertisement1 * aAdv, GDBusMethodInvocation * aInvocation, - BluezEndpoint * self); + static gboolean BluezAdvertisementRelease(BluezLEAdvertisement1 * aAdv, GDBusMethodInvocation * aInvocation, + BluezEndpoint * self); static gboolean BluezCharacteristicReadValue(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInvocation, GVariant * aOptions, BluezEndpoint * self); @@ -155,7 +156,9 @@ class BluezEndpoint char * mpPeerDevicePath = nullptr; GCancellable * mpConnectCancellable = nullptr; - // Allow BluezConnection to access our private members + // Allow BluezAdvertisement and BluezConnection to access our private members + // TODO: Fix this tight coupling + friend class BluezAdvertisement; friend class BluezConnection; }; From fe7642d551fc95d11f3741037f2e28fd1f5e554a Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Fri, 27 Oct 2023 12:57:34 +0200 Subject: [PATCH 14/25] Get rid of static class members --- src/platform/Linux/bluez/BluezEndpoint.cpp | 94 +++++++++++++++------- src/platform/Linux/bluez/BluezEndpoint.h | 27 +++---- 2 files changed, 75 insertions(+), 46 deletions(-) diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index 85c6bc19f9a1a6..050091b97a3722 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -81,7 +81,7 @@ namespace Internal { constexpr uint16_t kMaxConnectRetries = 4; gboolean BluezEndpoint::BluezCharacteristicReadValue(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInvocation, - GVariant * aOptions, BluezEndpoint * self) + GVariant * aOptions) { ChipLogDetail(DeviceLayer, "Received %s", __func__); GVariant * val = bluez_gatt_characteristic1_get_value(aChar); @@ -99,7 +99,7 @@ static void Bluez_gatt_characteristic1_complete_acquire_write_with_fd(GDBusMetho } gboolean BluezEndpoint::BluezCharacteristicAcquireWrite(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInvocation, - GVariant * aOptions, BluezEndpoint * self) + GVariant * aOptions) { int fds[2] = { -1, -1 }; #if CHIP_ERROR_LOGGING @@ -109,7 +109,7 @@ gboolean BluezEndpoint::BluezCharacteristicAcquireWrite(BluezGattCharacteristic1 GAutoPtr option_mtu; uint16_t mtu; - conn = self->GetBluezConnectionViaDevice(); + conn = GetBluezConnectionViaDevice(); VerifyOrReturnValue( conn != nullptr, FALSE, g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.Failed", "No CHIPoBLE connection")); @@ -150,7 +150,7 @@ static gboolean BluezCharacteristicAcquireWriteError(BluezGattCharacteristic1 * } gboolean BluezEndpoint::BluezCharacteristicAcquireNotify(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInvocation, - GVariant * aOptions, BluezEndpoint * self) + GVariant * aOptions) { int fds[2] = { -1, -1 }; #if CHIP_ERROR_LOGGING @@ -171,7 +171,7 @@ gboolean BluezEndpoint::BluezCharacteristicAcquireNotify(BluezGattCharacteristic return FALSE; } - conn = self->GetBluezConnectionViaDevice(); + conn = GetBluezConnectionViaDevice(); VerifyOrReturnValue( conn != nullptr, FALSE, g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.Failed", "No CHIPoBLE connection")); @@ -213,14 +213,11 @@ static gboolean BluezCharacteristicAcquireNotifyError(BluezGattCharacteristic1 * return TRUE; } -gboolean BluezEndpoint::BluezCharacteristicConfirm(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInvocation, - BluezEndpoint * self) +gboolean BluezEndpoint::BluezCharacteristicConfirm(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInvocation) { - BluezConnection * conn = self->GetBluezConnectionViaDevice(); - + BluezConnection * conn = GetBluezConnectionViaDevice(); ChipLogDetail(Ble, "Indication confirmation, %p", conn); BLEManagerImpl::HandleTXComplete(conn); - return TRUE; } @@ -336,15 +333,15 @@ void BluezEndpoint::UpdateConnectionTable(BluezDevice1 * apDevice) void BluezEndpoint::BluezSignalInterfacePropertiesChanged(GDBusObjectManagerClient * aManager, GDBusObjectProxy * aObject, GDBusProxy * aInterface, GVariant * aChangedProperties, - const char * const * aInvalidatedProps, BluezEndpoint * self) + const char * const * aInvalidatedProps) { - VerifyOrReturn(self->mpAdapter != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL mpAdapter in %s", __func__)); + VerifyOrReturn(mpAdapter != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL mpAdapter in %s", __func__)); VerifyOrReturn(strcmp(g_dbus_proxy_get_interface_name(aInterface), DEVICE_INTERFACE) == 0, ); BluezDevice1 * device = BLUEZ_DEVICE1(aInterface); - VerifyOrReturn(BluezIsDeviceOnAdapter(device, self->mpAdapter)); + VerifyOrReturn(BluezIsDeviceOnAdapter(device, mpAdapter)); - self->UpdateConnectionTable(device); + UpdateConnectionTable(device); } void BluezEndpoint::HandleNewDevice(BluezDevice1 * device) @@ -373,20 +370,20 @@ void BluezEndpoint::HandleNewDevice(BluezDevice1 * device) return; } -void BluezEndpoint::BluezSignalOnObjectAdded(GDBusObjectManager * aManager, GDBusObject * aObject, BluezEndpoint * self) +void BluezEndpoint::BluezSignalOnObjectAdded(GDBusObjectManager * aManager, GDBusObject * aObject) { // TODO: right now we do not handle addition/removal of adapters // Primary focus here is to handle addition of a device GAutoPtr device(bluez_object_get_device1(BLUEZ_OBJECT(aObject))); VerifyOrReturn(device.get() != nullptr); - if (BluezIsDeviceOnAdapter(device.get(), self->mpAdapter) == TRUE) + if (BluezIsDeviceOnAdapter(device.get(), mpAdapter) == TRUE) { - self->HandleNewDevice(device.get()); + HandleNewDevice(device.get()); } } -void BluezEndpoint::BluezSignalOnObjectRemoved(GDBusObjectManager * aManager, GDBusObject * aObject, BluezEndpoint * self) +void BluezEndpoint::BluezSignalOnObjectRemoved(GDBusObjectManager * aManager, GDBusObject * aObject) { // TODO: for Device1, lookup connection, and call otPlatTobleHandleDisconnected // for Adapter1: unclear, crash if this pertains to our adapter? at least null out the self->mpAdapter. @@ -527,18 +524,34 @@ void BluezEndpoint::SetupGattService() // C1 characteristic mpC1 = CreateGattCharacteristic(mpService, "c1", CHIP_PLAT_BLE_UUID_C1_STRING); bluez_gatt_characteristic1_set_flags(mpC1, c1_flags); - g_signal_connect(mpC1, "handle-read-value", G_CALLBACK(BluezCharacteristicReadValue), this); - g_signal_connect(mpC1, "handle-acquire-write", G_CALLBACK(BluezCharacteristicAcquireWrite), this); + g_signal_connect(mpC1, "handle-read-value", + G_CALLBACK(+[](BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInv, GVariant * aOpt, + BluezEndpoint * self) { return self->BluezCharacteristicReadValue(aChar, aInv, aOpt); }), + this); + g_signal_connect(mpC1, "handle-acquire-write", + G_CALLBACK(+[](BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInv, GVariant * aOpt, + BluezEndpoint * self) { return self->BluezCharacteristicAcquireWrite(aChar, aInv, aOpt); }), + this); g_signal_connect(mpC1, "handle-acquire-notify", G_CALLBACK(BluezCharacteristicAcquireNotifyError), nullptr); g_signal_connect(mpC1, "handle-confirm", G_CALLBACK(BluezCharacteristicConfirmError), nullptr); // C2 characteristic mpC2 = CreateGattCharacteristic(mpService, "c2", CHIP_PLAT_BLE_UUID_C2_STRING); bluez_gatt_characteristic1_set_flags(mpC2, c2_flags); - g_signal_connect(mpC2, "handle-read-value", G_CALLBACK(BluezCharacteristicReadValue), this); + g_signal_connect(mpC2, "handle-read-value", + G_CALLBACK(+[](BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInv, GVariant * aOpt, + BluezEndpoint * self) { return self->BluezCharacteristicReadValue(aChar, aInv, aOpt); }), + this); g_signal_connect(mpC2, "handle-acquire-write", G_CALLBACK(BluezCharacteristicAcquireWriteError), nullptr); - g_signal_connect(mpC2, "handle-acquire-notify", G_CALLBACK(BluezCharacteristicAcquireNotify), this); - g_signal_connect(mpC2, "handle-confirm", G_CALLBACK(BluezCharacteristicConfirm), this); + g_signal_connect(mpC2, "handle-acquire-notify", + G_CALLBACK(+[](BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInv, GVariant * aOpt, + BluezEndpoint * self) { return self->BluezCharacteristicAcquireNotify(aChar, aInv, aOpt); }), + this); + g_signal_connect(mpC2, "handle-confirm", + G_CALLBACK(+[](BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInv, BluezEndpoint * self) { + return self->BluezCharacteristicConfirm(aChar, aInv); + }), + this); ChipLogDetail(DeviceLayer, "CHIP BTP C1 %s", bluez_gatt_characteristic1_get_service(mpC1)); ChipLogDetail(DeviceLayer, "CHIP BTP C2 %s", bluez_gatt_characteristic1_get_service(mpC2)); @@ -548,10 +561,21 @@ void BluezEndpoint::SetupGattService() // Additional data characteristics mpC3 = CreateGattCharacteristic(mpService, "c3", CHIP_PLAT_BLE_UUID_C3_STRING); bluez_gatt_characteristic1_set_flags(mpC3, c3_flags); - g_signal_connect(mpC3, "handle-read-value", G_CALLBACK(BluezCharacteristicReadValue), this); + g_signal_connect(mpC3, "handle-read-value", + G_CALLBACK(+[](BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInv, GVariant * aOpt, + BluezEndpoint * self) { return self->BluezCharacteristicReadValue(aChar, aInv, aOpt); }), + this); g_signal_connect(mpC3, "handle-acquire-write", G_CALLBACK(BluezCharacteristicAcquireWriteError), nullptr); - g_signal_connect(mpC3, "handle-acquire-notify", G_CALLBACK(BluezCharacteristicAcquireNotify), this); - g_signal_connect(mpC3, "handle-confirm", G_CALLBACK(BluezCharacteristicConfirm), this); + g_signal_connect(mpC3, "handle-acquire-notify", + G_CALLBACK(+[](BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInv, GVariant * aOpt, + BluezEndpoint * self) { return self->BluezCharacteristicAcquireNotify(aChar, aInv, aOpt); }), + this); + g_signal_connect(mpC3, "handle-confirm", + G_CALLBACK(+[](BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInv, BluezEndpoint * self) { + return self->BluezCharacteristicConfirm(aChar, aInv); + }), + this); + // update the characteristic value UpdateAdditionalDataCharacteristic(mpC3); ChipLogDetail(DeviceLayer, "CHIP BTP C3 %s", bluez_gatt_characteristic1_get_service(mpC3)); @@ -599,9 +623,21 @@ CHIP_ERROR BluezEndpoint::StartupEndpointBindings() mpObjMgr = manager; SetupAdapter(); - g_signal_connect(manager, "object-added", G_CALLBACK(BluezSignalOnObjectAdded), this); - g_signal_connect(manager, "object-removed", G_CALLBACK(BluezSignalOnObjectRemoved), this); - g_signal_connect(manager, "interface-proxy-properties-changed", G_CALLBACK(BluezSignalInterfacePropertiesChanged), this); + g_signal_connect(manager, "object-added", G_CALLBACK(+[](GDBusObjectManager * aMgr, GDBusObject * aObj, BluezEndpoint * self) { + return self->BluezSignalOnObjectAdded(aMgr, aObj); + }), + this); + g_signal_connect(manager, "object-removed", + G_CALLBACK(+[](GDBusObjectManager * aMgr, GDBusObject * aObj, BluezEndpoint * self) { + return self->BluezSignalOnObjectRemoved(aMgr, aObj); + }), + this); + g_signal_connect(manager, "interface-proxy-properties-changed", + G_CALLBACK(+[](GDBusObjectManagerClient * aMgr, GDBusObjectProxy * aObj, GDBusProxy * aIface, + GVariant * aChangedProps, const char * const * aInvalidatedProps, BluezEndpoint * self) { + return self->BluezSignalInterfacePropertiesChanged(aMgr, aObj, aIface, aChangedProps, aInvalidatedProps); + }), + this); return CHIP_NO_ERROR; } diff --git a/src/platform/Linux/bluez/BluezEndpoint.h b/src/platform/Linux/bluez/BluezEndpoint.h index 6f8d0ec5b95821..79f2356e6c4d9c 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.h +++ b/src/platform/Linux/bluez/BluezEndpoint.h @@ -102,23 +102,16 @@ class BluezEndpoint void UpdateConnectionTable(BluezDevice1 * aDevice); BluezConnection * GetBluezConnectionViaDevice(); - static gboolean BluezAdvertisementRelease(BluezLEAdvertisement1 * aAdv, GDBusMethodInvocation * aInvocation, - BluezEndpoint * self); - - static gboolean BluezCharacteristicReadValue(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInvocation, - GVariant * aOptions, BluezEndpoint * self); - static gboolean BluezCharacteristicAcquireWrite(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInvocation, - GVariant * aOptions, BluezEndpoint * self); - static gboolean BluezCharacteristicAcquireNotify(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInvocation, - GVariant * aOptions, BluezEndpoint * self); - static gboolean BluezCharacteristicConfirm(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInvocation, - BluezEndpoint * self); - - static void BluezSignalOnObjectAdded(GDBusObjectManager * aManager, GDBusObject * aObject, BluezEndpoint * self); - static void BluezSignalOnObjectRemoved(GDBusObjectManager * aManager, GDBusObject * aObject, BluezEndpoint * self); - static void BluezSignalInterfacePropertiesChanged(GDBusObjectManagerClient * aManager, GDBusObjectProxy * aObject, - GDBusProxy * aInterface, GVariant * aChangedProperties, - const char * const * aInvalidatedProps, BluezEndpoint * self); + gboolean BluezCharacteristicReadValue(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInv, GVariant * aOptions); + gboolean BluezCharacteristicAcquireWrite(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInv, GVariant * aOptions); + gboolean BluezCharacteristicAcquireNotify(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInv, GVariant * aOptions); + gboolean BluezCharacteristicConfirm(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInv); + + void BluezSignalOnObjectAdded(GDBusObjectManager * aManager, GDBusObject * aObject); + void BluezSignalOnObjectRemoved(GDBusObjectManager * aManager, GDBusObject * aObject); + void BluezSignalInterfacePropertiesChanged(GDBusObjectManagerClient * aManager, GDBusObjectProxy * aObject, + GDBusProxy * aInterface, GVariant * aChangedProperties, + const char * const * aInvalidatedProps); CHIP_ERROR RegisterGattApplicationImpl(); From a667125d728cc62fdaa235b3d3847519f8934244 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Fri, 27 Oct 2023 16:43:21 +0200 Subject: [PATCH 15/25] Keep register app done callback in class --- src/platform/Linux/bluez/BluezEndpoint.cpp | 9 +++++++-- src/platform/Linux/bluez/BluezEndpoint.h | 1 + 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index 050091b97a3722..b05c4c9648ff09 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -254,7 +254,7 @@ BluezGattCharacteristic1 * BluezEndpoint::CreateGattCharacteristic(BluezGattServ return characteristic; } -static void BluezPeripheralRegisterAppDone(GObject * aObject, GAsyncResult * aResult, gpointer apClosure) +void BluezEndpoint::RegisterGattApplicationDone(GObject * aObject, GAsyncResult * aResult) { GAutoPtr error; BluezGattManager1 * gattMgr = BLUEZ_GATT_MANAGER1(aObject); @@ -289,7 +289,12 @@ CHIP_ERROR BluezEndpoint::RegisterGattApplicationImpl() g_variant_builder_init(&optionsBuilder, G_VARIANT_TYPE("a{sv}")); options = g_variant_builder_end(&optionsBuilder); - bluez_gatt_manager1_call_register_application(gattMgr, mpRootPath, options, nullptr, BluezPeripheralRegisterAppDone, nullptr); + bluez_gatt_manager1_call_register_application( + gattMgr, mpRootPath, options, nullptr, + +[](GObject * aObj, GAsyncResult * aResult, void * self) { + reinterpret_cast(self)->RegisterGattApplicationDone(aObj, aResult); + }, + this); exit: return CHIP_NO_ERROR; diff --git a/src/platform/Linux/bluez/BluezEndpoint.h b/src/platform/Linux/bluez/BluezEndpoint.h index 79f2356e6c4d9c..5c8bd18c939bbf 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.h +++ b/src/platform/Linux/bluez/BluezEndpoint.h @@ -113,6 +113,7 @@ class BluezEndpoint GDBusProxy * aInterface, GVariant * aChangedProperties, const char * const * aInvalidatedProps); + void RegisterGattApplicationDone(GObject * aObject, GAsyncResult * aResult); CHIP_ERROR RegisterGattApplicationImpl(); static void ConnectDeviceDone(GObject * aObject, GAsyncResult * aResult, gpointer apParams); From 5c7c0cccdd6b337d4a01402a70fb5ff4af70e556 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Sat, 28 Oct 2023 08:34:26 +0200 Subject: [PATCH 16/25] Fix error handling when allocating BluezEndpoint --- src/platform/Linux/BLEManagerImpl.cpp | 5 ++--- src/platform/Linux/bluez/BluezEndpoint.cpp | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/platform/Linux/BLEManagerImpl.cpp b/src/platform/Linux/BLEManagerImpl.cpp index b9af868dd6749e..99faae5a838566 100644 --- a/src/platform/Linux/BLEManagerImpl.cpp +++ b/src/platform/Linux/BLEManagerImpl.cpp @@ -571,8 +571,7 @@ void BLEManagerImpl::DriveBLEState() // Initializes the Bluez BLE layer if needed. if (mServiceMode == ConnectivityManager::kCHIPoBLEServiceMode_Enabled && !mFlags.Has(Flags::kBluezBLELayerInitialized)) { - mEndpoint = Platform::MakeUnique(mAdapterId, mIsCentral); - VerifyOrExit(mEndpoint, ChipLogError(DeviceLayer, "FAIL: memory allocation in %s", __func__)); + VerifyOrExit((mEndpoint = Platform::MakeUnique(mAdapterId, mIsCentral)), err = CHIP_ERROR_NO_MEMORY); SuccessOrExit(err = mEndpoint->Init(nullptr, mDeviceName)); mFlags.Set(Flags::kBluezBLELayerInitialized); } @@ -653,7 +652,7 @@ void BLEManagerImpl::InitiateScan(BleScanState scanType) return; } - if (!mEndpoint) + if (!mFlags.Has(Flags::kBluezBLELayerInitialized)) { BleConnectionDelegate::OnConnectionError(mBLEScanConfig.mAppState, CHIP_ERROR_INCORRECT_STATE); ChipLogError(Ble, "BLE Layer is not yet initialized"); diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index b05c4c9648ff09..e7adc4e7d4a87e 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -162,7 +162,7 @@ gboolean BluezEndpoint::BluezCharacteristicAcquireNotify(BluezGattCharacteristic uint16_t mtu; #if CHIP_ENABLE_ADDITIONAL_DATA_ADVERTISING - isAdditionalAdvertising = (aChar == self->mpC3); + isAdditionalAdvertising = (aChar == mpC3); #endif if (bluez_gatt_characteristic1_get_notifying(aChar)) From 6902b2b5b5a6527d1f69af3028cdc8cdc4e472d2 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Mon, 30 Oct 2023 09:17:55 +0100 Subject: [PATCH 17/25] Do not use assert() in the code --- src/platform/Linux/bluez/BluezEndpoint.cpp | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index e7adc4e7d4a87e..53547a7bf346b5 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -45,7 +45,6 @@ #include "BluezEndpoint.h" -#include #include #include #include @@ -618,32 +617,31 @@ CHIP_ERROR BluezEndpoint::StartupEndpointBindings() SetupGattServer(conn.get()); - GDBusObjectManager * manager = g_dbus_object_manager_client_new_sync( + mpObjMgr = g_dbus_object_manager_client_new_sync( conn.get(), 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 */, nullptr /* cancellable */, &MakeUniquePointerReceiver(err).Get()); - VerifyOrReturnError(manager != nullptr, CHIP_ERROR_INTERNAL, + VerifyOrReturnError(mpObjMgr != nullptr, CHIP_ERROR_INTERNAL, ChipLogError(DeviceLayer, "FAIL: Error getting object manager client: %s", err->message)); - mpObjMgr = manager; - SetupAdapter(); - - g_signal_connect(manager, "object-added", G_CALLBACK(+[](GDBusObjectManager * aMgr, GDBusObject * aObj, BluezEndpoint * self) { + g_signal_connect(mpObjMgr, "object-added", G_CALLBACK(+[](GDBusObjectManager * aMgr, GDBusObject * aObj, BluezEndpoint * self) { return self->BluezSignalOnObjectAdded(aMgr, aObj); }), this); - g_signal_connect(manager, "object-removed", + g_signal_connect(mpObjMgr, "object-removed", G_CALLBACK(+[](GDBusObjectManager * aMgr, GDBusObject * aObj, BluezEndpoint * self) { return self->BluezSignalOnObjectRemoved(aMgr, aObj); }), this); - g_signal_connect(manager, "interface-proxy-properties-changed", + g_signal_connect(mpObjMgr, "interface-proxy-properties-changed", G_CALLBACK(+[](GDBusObjectManagerClient * aMgr, GDBusObjectProxy * aObj, GDBusProxy * aIface, GVariant * aChangedProps, const char * const * aInvalidatedProps, BluezEndpoint * self) { return self->BluezSignalInterfacePropertiesChanged(aMgr, aObj, aIface, aChangedProps, aInvalidatedProps); }), this); + SetupAdapter(); + return CHIP_NO_ERROR; } @@ -790,7 +788,7 @@ CHIP_ERROR BluezEndpoint::ConnectDevice(BluezDevice1 & aDevice) void BluezEndpoint::CancelConnect() { - assert(mpConnectCancellable != nullptr); + VerifyOrDie(mpConnectCancellable != nullptr); g_cancellable_cancel(mpConnectCancellable); } From 43cfaeb2572547efde4d4e3b3fcb0c2dc43ecbea Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Mon, 30 Oct 2023 10:06:48 +0100 Subject: [PATCH 18/25] Fix crash when trying to read characteristic without prior write --- src/platform/Linux/bluez/BluezEndpoint.cpp | 16 +++++++++------- src/platform/Linux/bluez/BluezEndpoint.h | 3 ++- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index 53547a7bf346b5..26e6cae99de51a 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -232,7 +232,7 @@ static gboolean BluezIsDeviceOnAdapter(BluezDevice1 * aDevice, BluezAdapter1 * a } BluezGattCharacteristic1 * BluezEndpoint::CreateGattCharacteristic(BluezGattService1 * aService, const char * aCharName, - const char * aUUID) + const char * aUUID, const char * const * aFlags) { const char * servicePath = g_dbus_object_get_object_path(g_dbus_interface_get_object(G_DBUS_INTERFACE(aService))); GAutoPtr charPath(g_strdup_printf("%s/%s", servicePath, aCharName)); @@ -244,8 +244,13 @@ BluezGattCharacteristic1 * BluezEndpoint::CreateGattCharacteristic(BluezGattServ characteristic = bluez_gatt_characteristic1_skeleton_new(); bluez_gatt_characteristic1_set_uuid(characteristic, aUUID); + bluez_gatt_characteristic1_set_flags(characteristic, aFlags); bluez_gatt_characteristic1_set_service(characteristic, servicePath); + // Initialize value to empty array, so it can be read without prior write from the client side. + auto value = g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE, nullptr, 0, sizeof(uint8_t)); + bluez_gatt_characteristic1_set_value(characteristic, value); + bluez_object_skeleton_set_gatt_characteristic1(object, characteristic); g_dbus_object_manager_server_export(mpRoot, G_DBUS_OBJECT_SKELETON(object)); g_object_unref(object); @@ -526,8 +531,7 @@ void BluezEndpoint::SetupGattService() mpService = CreateGattService(CHIP_BLE_UUID_SERVICE_SHORT_STRING); // C1 characteristic - mpC1 = CreateGattCharacteristic(mpService, "c1", CHIP_PLAT_BLE_UUID_C1_STRING); - bluez_gatt_characteristic1_set_flags(mpC1, c1_flags); + mpC1 = CreateGattCharacteristic(mpService, "c1", CHIP_PLAT_BLE_UUID_C1_STRING, c1_flags); g_signal_connect(mpC1, "handle-read-value", G_CALLBACK(+[](BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInv, GVariant * aOpt, BluezEndpoint * self) { return self->BluezCharacteristicReadValue(aChar, aInv, aOpt); }), @@ -540,8 +544,7 @@ void BluezEndpoint::SetupGattService() g_signal_connect(mpC1, "handle-confirm", G_CALLBACK(BluezCharacteristicConfirmError), nullptr); // C2 characteristic - mpC2 = CreateGattCharacteristic(mpService, "c2", CHIP_PLAT_BLE_UUID_C2_STRING); - bluez_gatt_characteristic1_set_flags(mpC2, c2_flags); + mpC2 = CreateGattCharacteristic(mpService, "c2", CHIP_PLAT_BLE_UUID_C2_STRING, c2_flags); g_signal_connect(mpC2, "handle-read-value", G_CALLBACK(+[](BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInv, GVariant * aOpt, BluezEndpoint * self) { return self->BluezCharacteristicReadValue(aChar, aInv, aOpt); }), @@ -563,8 +566,7 @@ void BluezEndpoint::SetupGattService() #if CHIP_ENABLE_ADDITIONAL_DATA_ADVERTISING ChipLogDetail(DeviceLayer, "CHIP_ENABLE_ADDITIONAL_DATA_ADVERTISING is TRUE"); // Additional data characteristics - mpC3 = CreateGattCharacteristic(mpService, "c3", CHIP_PLAT_BLE_UUID_C3_STRING); - bluez_gatt_characteristic1_set_flags(mpC3, c3_flags); + mpC3 = CreateGattCharacteristic(mpService, "c3", CHIP_PLAT_BLE_UUID_C3_STRING, c3_flags); g_signal_connect(mpC3, "handle-read-value", G_CALLBACK(+[](BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInv, GVariant * aOpt, BluezEndpoint * self) { return self->BluezCharacteristicReadValue(aChar, aInv, aOpt); }), diff --git a/src/platform/Linux/bluez/BluezEndpoint.h b/src/platform/Linux/bluez/BluezEndpoint.h index 5c8bd18c939bbf..4331af136c519d 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.h +++ b/src/platform/Linux/bluez/BluezEndpoint.h @@ -96,7 +96,8 @@ class BluezEndpoint void SetupGattService(); BluezGattService1 * CreateGattService(const char * aUUID); - BluezGattCharacteristic1 * CreateGattCharacteristic(BluezGattService1 * aService, const char * aCharName, const char * aUUID); + BluezGattCharacteristic1 * CreateGattCharacteristic(BluezGattService1 * aService, const char * aCharName, const char * aUUID, + const char * const * aFlags); void HandleNewDevice(BluezDevice1 * aDevice); void UpdateConnectionTable(BluezDevice1 * aDevice); From 7edddec97b1a91bcd77a006e234bb8c0541a7052 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Mon, 30 Oct 2023 10:12:48 +0100 Subject: [PATCH 19/25] Do not use pointer for BluezEndpoint --- src/platform/Linux/BLEManagerImpl.cpp | 27 ++++++++++---------- src/platform/Linux/BLEManagerImpl.h | 2 +- src/platform/Linux/bluez/BluezEndpoint.cpp | 29 ++++++++++++---------- src/platform/Linux/bluez/BluezEndpoint.h | 4 +-- 4 files changed, 32 insertions(+), 30 deletions(-) diff --git a/src/platform/Linux/BLEManagerImpl.cpp b/src/platform/Linux/BLEManagerImpl.cpp index 99faae5a838566..2f7857c83a3336 100644 --- a/src/platform/Linux/BLEManagerImpl.cpp +++ b/src/platform/Linux/BLEManagerImpl.cpp @@ -32,7 +32,6 @@ * platform//BLEManagerImpl.h after defining interface class. */ #include "platform/internal/BLEManager.h" -#include #include #include @@ -63,8 +62,7 @@ const ChipBleUUID ChipUUID_CHIPoBLEChar_TX = { { 0x18, 0xEE, 0x2E, 0xF5, 0x26, 0 void HandleConnectTimeout(chip::System::Layer *, void * apEndpoint) { - assert(apEndpoint != nullptr); - + VerifyOrDie(apEndpoint != nullptr); static_cast(apEndpoint)->CancelConnect(); BLEManagerImpl::HandleConnectFailed(CHIP_ERROR_TIMEOUT); } @@ -105,8 +103,10 @@ void BLEManagerImpl::_Shutdown() mDeviceScanner.Shutdown(); // Stop advertising and free resources. mBLEAdvertisement.Shutdown(); + // Make sure that the endpoint is not used by the timer. + DeviceLayer::SystemLayer().CancelTimer(HandleConnectTimeout, &mEndpoint); // Release BLE connection resources (unregister from BlueZ). - mEndpoint.reset(); + mEndpoint.Shutdown(); mFlags.Clear(Flags::kBluezBLELayerInitialized); } @@ -571,15 +571,14 @@ void BLEManagerImpl::DriveBLEState() // Initializes the Bluez BLE layer if needed. if (mServiceMode == ConnectivityManager::kCHIPoBLEServiceMode_Enabled && !mFlags.Has(Flags::kBluezBLELayerInitialized)) { - VerifyOrExit((mEndpoint = Platform::MakeUnique(mAdapterId, mIsCentral)), err = CHIP_ERROR_NO_MEMORY); - SuccessOrExit(err = mEndpoint->Init(nullptr, mDeviceName)); + SuccessOrExit(err = mEndpoint.Init(mAdapterId, mIsCentral, nullptr, mDeviceName)); mFlags.Set(Flags::kBluezBLELayerInitialized); } // Register the CHIPoBLE application with the Bluez BLE layer if needed. if (!mIsCentral && mServiceMode == ConnectivityManager::kCHIPoBLEServiceMode_Enabled && !mFlags.Has(Flags::kAppRegistered)) { - SuccessOrExit(err = mEndpoint->RegisterGattApplication()); + SuccessOrExit(err = mEndpoint.RegisterGattApplication()); mFlags.Set(Flags::kControlOpInProgress); ExitNow(); } @@ -596,7 +595,7 @@ void BLEManagerImpl::DriveBLEState() // Configure advertising data if it hasn't been done yet. if (!mFlags.Has(Flags::kAdvertisingConfigured)) { - err = mBLEAdvertisement.Init(mEndpoint.get(), mBLEAdvType, mpBLEAdvUUID, mBLEAdvDurationMs); + err = mBLEAdvertisement.Init(&mEndpoint, mBLEAdvType, mpBLEAdvUUID, mBLEAdvDurationMs); SuccessOrExit(err); mFlags.Set(Flags::kAdvertisingConfigured); } @@ -659,7 +658,7 @@ void BLEManagerImpl::InitiateScan(BleScanState scanType) return; } - if (mEndpoint->GetAdapter() == nullptr) + if (mEndpoint.GetAdapter() == nullptr) { BleConnectionDelegate::OnConnectionError(mBLEScanConfig.mAppState, CHIP_ERROR_INCORRECT_STATE); ChipLogError(Ble, "No adapter available for new connection establishment"); @@ -668,7 +667,7 @@ void BLEManagerImpl::InitiateScan(BleScanState scanType) mBLEScanConfig.mBleScanState = scanType; - CHIP_ERROR err = mDeviceScanner.Init(mEndpoint->GetAdapter(), this); + CHIP_ERROR err = mDeviceScanner.Init(mEndpoint.GetAdapter(), this); if (err != CHIP_NO_ERROR) { mBLEScanConfig.mBleScanState = BleScanState::kNotScanning; @@ -690,7 +689,7 @@ void BLEManagerImpl::InitiateScan(BleScanState scanType) void BLEManagerImpl::CleanScanConfig() { if (mBLEScanConfig.mBleScanState == BleScanState::kConnecting) - DeviceLayer::SystemLayer().CancelTimer(HandleConnectTimeout, mEndpoint.get()); + DeviceLayer::SystemLayer().CancelTimer(HandleConnectTimeout, &mEndpoint); mBLEScanConfig.mBleScanState = BleScanState::kNotScanning; } @@ -712,7 +711,7 @@ void BLEManagerImpl::NewConnection(BleLayer * bleLayer, void * appState, const S CHIP_ERROR BLEManagerImpl::CancelConnection() { if (mBLEScanConfig.mBleScanState == BleScanState::kConnecting) - mEndpoint->CancelConnect(); + mEndpoint.CancelConnect(); // If in discovery mode, stop scan. else if (mBLEScanConfig.mBleScanState != BleScanState::kNotScanning) mDeviceScanner.StopScan(); @@ -776,12 +775,12 @@ void BLEManagerImpl::OnDeviceScanned(BluezDevice1 & device, const chip::Ble::Chi mBLEScanConfig.mBleScanState = BleScanState::kConnecting; chip::DeviceLayer::PlatformMgr().LockChipStack(); - DeviceLayer::SystemLayer().StartTimer(kConnectTimeout, HandleConnectTimeout, mEndpoint.get()); + DeviceLayer::SystemLayer().StartTimer(kConnectTimeout, HandleConnectTimeout, &mEndpoint); chip::DeviceLayer::PlatformMgr().UnlockChipStack(); mDeviceScanner.StopScan(); - mEndpoint->ConnectDevice(device); + mEndpoint.ConnectDevice(device); } void BLEManagerImpl::OnScanComplete() diff --git a/src/platform/Linux/BLEManagerImpl.h b/src/platform/Linux/BLEManagerImpl.h index e717c94832ecd3..76859726745925 100644 --- a/src/platform/Linux/BLEManagerImpl.h +++ b/src/platform/Linux/BLEManagerImpl.h @@ -187,7 +187,7 @@ class BLEManagerImpl final : public BLEManager, uint32_t mAdapterId = 0; char mDeviceName[kMaxDeviceNameLength + 1]; bool mIsCentral = false; - Platform::UniquePtr mEndpoint; + BluezEndpoint mEndpoint; BluezAdvertisement mBLEAdvertisement; ChipAdvType mBLEAdvType = ChipAdvType::BLUEZ_ADV_TYPE_UNDIRECTED_CONNECTABLE_SCANNABLE; diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index 26e6cae99de51a..74af39b15158db 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -647,7 +647,7 @@ CHIP_ERROR BluezEndpoint::StartupEndpointBindings() return CHIP_NO_ERROR; } -BluezEndpoint::BluezEndpoint(uint32_t aAdapterId, bool aIsCentral) : mAdapterId(aAdapterId), mIsCentral(aIsCentral) +BluezEndpoint::BluezEndpoint() { mpConnMap = g_hash_table_new(g_str_hash, g_str_equal); } @@ -655,16 +655,7 @@ BluezEndpoint::BluezEndpoint(uint32_t aAdapterId, bool aIsCentral) : mAdapterId( BluezEndpoint::~BluezEndpoint() { Shutdown(); - g_free(mpOwningName); - g_free(mpAdapterName); - g_free(mpAdapterAddr); - g_free(mpRootPath); - g_free(mpServicePath); - if (mpConnMap != nullptr) - g_hash_table_destroy(mpConnMap); - g_free(mpPeerDevicePath); - if (mpConnectCancellable != nullptr) - g_object_unref(mpConnectCancellable); + g_hash_table_destroy(mpConnMap); } CHIP_ERROR BluezEndpoint::RegisterGattApplication() @@ -676,14 +667,17 @@ CHIP_ERROR BluezEndpoint::RegisterGattApplication() return err; } -CHIP_ERROR BluezEndpoint::Init(const char * apBleAddr, const char * apBleName) +CHIP_ERROR BluezEndpoint::Init(uint32_t aAdapterId, bool aIsCentral, const char * apBleAddr, const char * apBleName) { CHIP_ERROR err; + mAdapterId = aAdapterId; + mIsCentral = aIsCentral; + if (apBleAddr != nullptr) mpAdapterAddr = g_strdup(apBleAddr); - if (!mIsCentral) + if (!aIsCentral) { mpAdapterName = g_strdup(apBleName); } @@ -724,9 +718,18 @@ void BluezEndpoint::Shutdown() g_object_unref(self->mpC2); if (self->mpC3 != nullptr) g_object_unref(self->mpC3); + if (self->mpConnectCancellable != nullptr) + g_object_unref(self->mpConnectCancellable); return CHIP_NO_ERROR; }, this); + + g_free(mpOwningName); + g_free(mpAdapterName); + g_free(mpAdapterAddr); + g_free(mpRootPath); + g_free(mpServicePath); + g_free(mpPeerDevicePath); } // ConnectDevice callbacks diff --git a/src/platform/Linux/bluez/BluezEndpoint.h b/src/platform/Linux/bluez/BluezEndpoint.h index 4331af136c519d..9cf207109fe3ab 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.h +++ b/src/platform/Linux/bluez/BluezEndpoint.h @@ -65,10 +65,10 @@ namespace Internal { class BluezEndpoint { public: - BluezEndpoint(uint32_t aAdapterId, bool aIsCentral); + BluezEndpoint(); ~BluezEndpoint(); - CHIP_ERROR Init(const char * apBleAddr, const char * apBleName); + CHIP_ERROR Init(uint32_t aAdapterId, bool aIsCentral, const char * apBleAddr, const char * apBleName); void Shutdown(); BluezAdapter1 * GetAdapter() const { return mpAdapter; } From 1163c2fe503cb9960c6b54af368e772082ceea00 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Mon, 13 Nov 2023 12:02:04 +0100 Subject: [PATCH 20/25] Get rid of BluezEndpoint pointers --- src/platform/Linux/BLEManagerImpl.cpp | 2 +- .../Linux/bluez/BluezAdvertisement.cpp | 10 ++++------ src/platform/Linux/bluez/BluezAdvertisement.h | 2 +- src/platform/Linux/bluez/BluezConnection.cpp | 18 +++++++++--------- src/platform/Linux/bluez/BluezConnection.h | 5 ++--- src/platform/Linux/bluez/BluezEndpoint.cpp | 4 ++-- src/platform/Linux/bluez/BluezEndpoint.h | 2 +- 7 files changed, 20 insertions(+), 23 deletions(-) diff --git a/src/platform/Linux/BLEManagerImpl.cpp b/src/platform/Linux/BLEManagerImpl.cpp index 2f7857c83a3336..4756892cf0a364 100644 --- a/src/platform/Linux/BLEManagerImpl.cpp +++ b/src/platform/Linux/BLEManagerImpl.cpp @@ -595,7 +595,7 @@ void BLEManagerImpl::DriveBLEState() // Configure advertising data if it hasn't been done yet. if (!mFlags.Has(Flags::kAdvertisingConfigured)) { - err = mBLEAdvertisement.Init(&mEndpoint, mBLEAdvType, mpBLEAdvUUID, mBLEAdvDurationMs); + err = mBLEAdvertisement.Init(mEndpoint, mBLEAdvType, mpBLEAdvUUID, mBLEAdvDurationMs); SuccessOrExit(err); mFlags.Set(Flags::kAdvertisingConfigured); } diff --git a/src/platform/Linux/bluez/BluezAdvertisement.cpp b/src/platform/Linux/bluez/BluezAdvertisement.cpp index 16d590d6bab80d..6dcb24e9bbf37a 100644 --- a/src/platform/Linux/bluez/BluezAdvertisement.cpp +++ b/src/platform/Linux/bluez/BluezAdvertisement.cpp @@ -133,20 +133,18 @@ CHIP_ERROR BluezAdvertisement::InitImpl() return CHIP_NO_ERROR; } -CHIP_ERROR BluezAdvertisement::Init(BluezEndpoint * apEndpoint, ChipAdvType aAdvType, const char * aAdvUUID, +CHIP_ERROR BluezAdvertisement::Init(const BluezEndpoint & aEndpoint, ChipAdvType aAdvType, const char * aAdvUUID, uint32_t aAdvDurationMs) { GAutoPtr rootPath; CHIP_ERROR err; - VerifyOrExit(apEndpoint != nullptr, err = CHIP_ERROR_INCORRECT_STATE; - ChipLogError(DeviceLayer, "FAIL: NULL endpoint in %s", __func__)); VerifyOrExit(mpAdv == nullptr, err = CHIP_ERROR_INCORRECT_STATE; ChipLogError(DeviceLayer, "FAIL: BLE advertisement already initialized in %s", __func__)); - mpRoot = reinterpret_cast(g_object_ref(apEndpoint->mpRoot)); - mpAdapter = reinterpret_cast(g_object_ref(apEndpoint->mpAdapter)); - mpAdapterName = g_strdup(apEndpoint->mpAdapterName); + mpRoot = reinterpret_cast(g_object_ref(aEndpoint.mpRoot)); + mpAdapter = reinterpret_cast(g_object_ref(aEndpoint.GetAdapter())); + mpAdapterName = g_strdup(aEndpoint.GetAdapterName()); g_object_get(G_OBJECT(mpRoot), "object-path", &MakeUniquePointerReceiver(rootPath).Get(), nullptr); mpAdvPath = g_strdup_printf("%s/advertising", rootPath.get()); diff --git a/src/platform/Linux/bluez/BluezAdvertisement.h b/src/platform/Linux/bluez/BluezAdvertisement.h index cf65313a351e49..6ae2974d706b35 100644 --- a/src/platform/Linux/bluez/BluezAdvertisement.h +++ b/src/platform/Linux/bluez/BluezAdvertisement.h @@ -41,7 +41,7 @@ class BluezAdvertisement BluezAdvertisement() = default; ~BluezAdvertisement() { Shutdown(); } - CHIP_ERROR Init(BluezEndpoint * apEndpoint, ChipAdvType aAdvType, const char * aAdvUUID, uint32_t aAdvDurationMs); + CHIP_ERROR Init(const BluezEndpoint & aEndpoint, ChipAdvType aAdvType, const char * aAdvUUID, uint32_t aAdvDurationMs); void Shutdown(); /// Start BLE advertising. diff --git a/src/platform/Linux/bluez/BluezConnection.cpp b/src/platform/Linux/bluez/BluezConnection.cpp index 2d4286a4fe6069..39b5bb0bdb8ba6 100644 --- a/src/platform/Linux/bluez/BluezConnection.cpp +++ b/src/platform/Linux/bluez/BluezConnection.cpp @@ -59,10 +59,10 @@ gboolean BluezIsCharOnService(BluezGattCharacteristic1 * aChar, BluezGattService } // namespace -BluezConnection::BluezConnection(BluezEndpoint * apEndpoint, BluezDevice1 * apDevice) : - mpEndpoint(apEndpoint), mpDevice(BLUEZ_DEVICE1(g_object_ref(apDevice))) +BluezConnection::BluezConnection(const BluezEndpoint & aEndpoint, BluezDevice1 * apDevice) : + mpDevice(BLUEZ_DEVICE1(g_object_ref(apDevice))) { - Init(); + Init(aEndpoint); } BluezConnection::~BluezConnection() @@ -93,21 +93,21 @@ BluezConnection::ConnectionDataBundle::ConnectionDataBundle(const BluezConnectio mData(g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE, aBuf->Start(), aBuf->DataLength(), sizeof(uint8_t))) {} -CHIP_ERROR BluezConnection::Init() +CHIP_ERROR BluezConnection::Init(const BluezEndpoint & aEndpoint) { // populate the service and the characteristics GList * objects = nullptr; GList * l; - if (!mpEndpoint->mIsCentral) + if (!aEndpoint.mIsCentral) { - mpService = BLUEZ_GATT_SERVICE1(g_object_ref(mpEndpoint->mpService)); - mpC1 = BLUEZ_GATT_CHARACTERISTIC1(g_object_ref(mpEndpoint->mpC1)); - mpC2 = BLUEZ_GATT_CHARACTERISTIC1(g_object_ref(mpEndpoint->mpC2)); + mpService = BLUEZ_GATT_SERVICE1(g_object_ref(aEndpoint.mpService)); + mpC1 = BLUEZ_GATT_CHARACTERISTIC1(g_object_ref(aEndpoint.mpC1)); + mpC2 = BLUEZ_GATT_CHARACTERISTIC1(g_object_ref(aEndpoint.mpC2)); } else { - objects = g_dbus_object_manager_get_objects(mpEndpoint->mpObjMgr); + objects = g_dbus_object_manager_get_objects(aEndpoint.mpObjMgr); for (l = objects; l != nullptr; l = l->next) { diff --git a/src/platform/Linux/bluez/BluezConnection.h b/src/platform/Linux/bluez/BluezConnection.h index 7bf91103e95ebf..fa8a6392fa1ec5 100644 --- a/src/platform/Linux/bluez/BluezConnection.h +++ b/src/platform/Linux/bluez/BluezConnection.h @@ -37,7 +37,7 @@ class BluezEndpoint; class BluezConnection { public: - BluezConnection(BluezEndpoint * apEndpoint, BluezDevice1 * apDevice); + BluezConnection(const BluezEndpoint & aEndpoint, BluezDevice1 * apDevice); ~BluezConnection(); const char * GetPeerAddress() const; @@ -95,7 +95,7 @@ class BluezConnection GAutoPtr mData; }; - CHIP_ERROR Init(); + CHIP_ERROR Init(const BluezEndpoint & aEndpoint); static CHIP_ERROR BluezDisconnect(BluezConnection * apConn); @@ -115,7 +115,6 @@ class BluezConnection static void UnsubscribeCharacteristicDone(GObject * aObject, GAsyncResult * aResult, gpointer apConn); static CHIP_ERROR UnsubscribeCharacteristicImpl(BluezConnection * apConn); - BluezEndpoint * mpEndpoint; BluezDevice1 * mpDevice; bool mNotifyAcquired = false; diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index 74af39b15158db..e455d16b818dbf 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -329,7 +329,7 @@ void BluezEndpoint::UpdateConnectionTable(BluezDevice1 * apDevice) if (connection == nullptr && bluez_device1_get_connected(apDevice) && (!mIsCentral || bluez_device1_get_services_resolved(apDevice))) { - connection = chip::Platform::New(this, apDevice); + connection = chip::Platform::New(*this, apDevice); mpPeerDevicePath = g_strdup(objectPath); g_hash_table_insert(mpConnMap, mpPeerDevicePath, connection); @@ -369,7 +369,7 @@ void BluezEndpoint::HandleNewDevice(BluezDevice1 * device) ChipLogError(DeviceLayer, "FAIL: connection already tracked: conn: %p new device: %s", conn, g_dbus_proxy_get_object_path(G_DBUS_PROXY(device)))); - conn = chip::Platform::New(this, device); + conn = chip::Platform::New(*this, device); mpPeerDevicePath = g_strdup(g_dbus_proxy_get_object_path(G_DBUS_PROXY(device))); g_hash_table_insert(mpConnMap, mpPeerDevicePath, conn); diff --git a/src/platform/Linux/bluez/BluezEndpoint.h b/src/platform/Linux/bluez/BluezEndpoint.h index 9cf207109fe3ab..aa83998acd528b 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.h +++ b/src/platform/Linux/bluez/BluezEndpoint.h @@ -54,7 +54,6 @@ #include #include -#include "BluezAdvertisement.h" #include "BluezConnection.h" #include "Types.h" @@ -72,6 +71,7 @@ class BluezEndpoint void Shutdown(); BluezAdapter1 * GetAdapter() const { return mpAdapter; } + const char * GetAdapterName() const { return mpAdapterName; } CHIP_ERROR RegisterGattApplication(); From d052a692f9c71443f2b6096198c1287e244e1715 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Mon, 13 Nov 2023 12:02:51 +0100 Subject: [PATCH 21/25] Fix double-free when destroying BluezEndpoint --- src/platform/Linux/bluez/BluezEndpoint.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index e455d16b818dbf..af110771fcaf89 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -654,7 +654,6 @@ BluezEndpoint::BluezEndpoint() BluezEndpoint::~BluezEndpoint() { - Shutdown(); g_hash_table_destroy(mpConnMap); } From e8e785d152898d12fa3ff9c148aa367d021fbbf8 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Mon, 13 Nov 2023 13:22:14 +0100 Subject: [PATCH 22/25] Use c++ map for storing BlueConnection objects --- src/platform/Linux/bluez/BluezEndpoint.cpp | 36 ++++++++++------------ src/platform/Linux/bluez/BluezEndpoint.h | 17 +++++----- 2 files changed, 26 insertions(+), 27 deletions(-) diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index af110771fcaf89..8b334a770b7642 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -308,16 +308,16 @@ CHIP_ERROR BluezEndpoint::RegisterGattApplicationImpl() void BluezEndpoint::UpdateConnectionTable(BluezDevice1 * apDevice) { const char * objectPath = g_dbus_proxy_get_object_path(G_DBUS_PROXY(apDevice)); - BluezConnection * connection = static_cast(g_hash_table_lookup(mpConnMap, objectPath)); + BluezConnection * connection = GetBluezConnection(objectPath); if (connection != nullptr && !bluez_device1_get_connected(apDevice)) { ChipLogDetail(DeviceLayer, "Bluez disconnected"); BLEManagerImpl::CHIPoBluez_ConnectionClosed(connection); + mConnMap.erase(objectPath); // TODO: the connection object should be released after BLEManagerImpl finishes cleaning up its resources // after the disconnection. Releasing it here doesn't cause any issues, but it's error-prone. chip::Platform::Delete(connection); - g_hash_table_remove(mpConnMap, objectPath); return; } @@ -329,9 +329,9 @@ void BluezEndpoint::UpdateConnectionTable(BluezDevice1 * apDevice) if (connection == nullptr && bluez_device1_get_connected(apDevice) && (!mIsCentral || bluez_device1_get_services_resolved(apDevice))) { - connection = chip::Platform::New(*this, apDevice); - mpPeerDevicePath = g_strdup(objectPath); - g_hash_table_insert(mpConnMap, mpPeerDevicePath, connection); + connection = chip::Platform::New(*this, apDevice); + mpPeerDevicePath = g_strdup(objectPath); + mConnMap[mpPeerDevicePath] = connection; ChipLogDetail(DeviceLayer, "New BLE connection: conn %p, device %s, path %s", connection, connection->GetPeerAddress(), mpPeerDevicePath); @@ -364,14 +364,14 @@ void BluezEndpoint::HandleNewDevice(BluezDevice1 * device) BluezConnection * conn; VerifyOrExit(bluez_device1_get_connected(device), ChipLogError(DeviceLayer, "FAIL: device is not connected")); - conn = static_cast(g_hash_table_lookup(mpConnMap, g_dbus_proxy_get_object_path(G_DBUS_PROXY(device)))); + conn = GetBluezConnection(g_dbus_proxy_get_object_path(G_DBUS_PROXY(device))); VerifyOrExit(conn == nullptr, ChipLogError(DeviceLayer, "FAIL: connection already tracked: conn: %p new device: %s", conn, g_dbus_proxy_get_object_path(G_DBUS_PROXY(device)))); - conn = chip::Platform::New(*this, device); - mpPeerDevicePath = g_strdup(g_dbus_proxy_get_object_path(G_DBUS_PROXY(device))); - g_hash_table_insert(mpConnMap, mpPeerDevicePath, conn); + conn = chip::Platform::New(*this, device); + mpPeerDevicePath = g_strdup(g_dbus_proxy_get_object_path(G_DBUS_PROXY(device))); + mConnMap[mpPeerDevicePath] = conn; ChipLogDetail(DeviceLayer, "BLE device connected: conn %p, device %s, path %s", conn, conn->GetPeerAddress(), mpPeerDevicePath); @@ -469,9 +469,15 @@ void BluezEndpoint::SetupAdapter() g_list_free_full(objects, g_object_unref); } +BluezConnection * BluezEndpoint::GetBluezConnection(const char * aPath) +{ + auto it = mConnMap.find(aPath); + return (it != mConnMap.end()) ? it->second : nullptr; +} + BluezConnection * BluezEndpoint::GetBluezConnectionViaDevice() { - return static_cast(g_hash_table_lookup(mpConnMap, mpPeerDevicePath)); + return GetBluezConnection(mpPeerDevicePath); } #if CHIP_ENABLE_ADDITIONAL_DATA_ADVERTISING @@ -647,16 +653,6 @@ CHIP_ERROR BluezEndpoint::StartupEndpointBindings() return CHIP_NO_ERROR; } -BluezEndpoint::BluezEndpoint() -{ - mpConnMap = g_hash_table_new(g_str_hash, g_str_equal); -} - -BluezEndpoint::~BluezEndpoint() -{ - g_hash_table_destroy(mpConnMap); -} - CHIP_ERROR BluezEndpoint::RegisterGattApplication() { CHIP_ERROR err = PlatformMgrImpl().GLibMatterContextInvokeSync( diff --git a/src/platform/Linux/bluez/BluezEndpoint.h b/src/platform/Linux/bluez/BluezEndpoint.h index aa83998acd528b..c01976f6d3d873 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.h +++ b/src/platform/Linux/bluez/BluezEndpoint.h @@ -46,6 +46,8 @@ #pragma once #include +#include +#include #include #include @@ -64,8 +66,8 @@ namespace Internal { class BluezEndpoint { public: - BluezEndpoint(); - ~BluezEndpoint(); + BluezEndpoint() = default; + ~BluezEndpoint() = default; CHIP_ERROR Init(uint32_t aAdapterId, bool aIsCentral, const char * apBleAddr, const char * apBleName); void Shutdown(); @@ -101,6 +103,7 @@ class BluezEndpoint void HandleNewDevice(BluezDevice1 * aDevice); void UpdateConnectionTable(BluezDevice1 * aDevice); + BluezConnection * GetBluezConnection(const char * aPath); BluezConnection * GetBluezConnectionViaDevice(); gboolean BluezCharacteristicReadValue(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInv, GVariant * aOptions); @@ -120,10 +123,13 @@ class BluezEndpoint static void ConnectDeviceDone(GObject * aObject, GAsyncResult * aResult, gpointer apParams); static CHIP_ERROR ConnectDeviceImpl(ConnectParams * apParams); + bool mIsCentral = false; + // Bus owning name char * mpOwningName = nullptr; // Adapter properties + uint32_t mAdapterId = 0; char * mpAdapterName = nullptr; char * mpAdapterAddr = nullptr; @@ -144,12 +150,9 @@ class BluezEndpoint // additional data characteristics BluezGattCharacteristic1 * mpC3 = nullptr; - // map device path to the connection - GHashTable * mpConnMap = nullptr; - uint32_t mAdapterId = 0; - bool mIsCentral = false; - char * mpPeerDevicePath = nullptr; + std::unordered_map mConnMap; GCancellable * mpConnectCancellable = nullptr; + char * mpPeerDevicePath = nullptr; // Allow BluezAdvertisement and BluezConnection to access our private members // TODO: Fix this tight coupling From 73a7a9107d9436eb5fe68360ac11e0356aaffd34 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Mon, 13 Nov 2023 14:35:20 +0100 Subject: [PATCH 23/25] Decouple BLE adv with BluezEndpoint private members --- src/platform/Linux/BLEManagerImpl.h | 1 - src/platform/Linux/bluez/BluezAdvertisement.cpp | 2 +- src/platform/Linux/bluez/BluezEndpoint.h | 5 ++--- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/platform/Linux/BLEManagerImpl.h b/src/platform/Linux/BLEManagerImpl.h index 76859726745925..5cd3ed2460adf9 100644 --- a/src/platform/Linux/BLEManagerImpl.h +++ b/src/platform/Linux/BLEManagerImpl.h @@ -27,7 +27,6 @@ #include #include -#include #include #include "bluez/BluezAdvertisement.h" diff --git a/src/platform/Linux/bluez/BluezAdvertisement.cpp b/src/platform/Linux/bluez/BluezAdvertisement.cpp index 6dcb24e9bbf37a..19212fbe717475 100644 --- a/src/platform/Linux/bluez/BluezAdvertisement.cpp +++ b/src/platform/Linux/bluez/BluezAdvertisement.cpp @@ -142,7 +142,7 @@ CHIP_ERROR BluezAdvertisement::Init(const BluezEndpoint & aEndpoint, ChipAdvType VerifyOrExit(mpAdv == nullptr, err = CHIP_ERROR_INCORRECT_STATE; ChipLogError(DeviceLayer, "FAIL: BLE advertisement already initialized in %s", __func__)); - mpRoot = reinterpret_cast(g_object_ref(aEndpoint.mpRoot)); + mpRoot = reinterpret_cast(g_object_ref(aEndpoint.GetGattApplicationObjectManager())); mpAdapter = reinterpret_cast(g_object_ref(aEndpoint.GetAdapter())); mpAdapterName = g_strdup(aEndpoint.GetAdapterName()); diff --git a/src/platform/Linux/bluez/BluezEndpoint.h b/src/platform/Linux/bluez/BluezEndpoint.h index c01976f6d3d873..b6aee62cbeb09d 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.h +++ b/src/platform/Linux/bluez/BluezEndpoint.h @@ -76,6 +76,7 @@ class BluezEndpoint const char * GetAdapterName() const { return mpAdapterName; } CHIP_ERROR RegisterGattApplication(); + GDBusObjectManagerServer * GetGattApplicationObjectManager() const { return mpRoot; } CHIP_ERROR ConnectDevice(BluezDevice1 & aDevice); void CancelConnect(); @@ -154,9 +155,7 @@ class BluezEndpoint GCancellable * mpConnectCancellable = nullptr; char * mpPeerDevicePath = nullptr; - // Allow BluezAdvertisement and BluezConnection to access our private members - // TODO: Fix this tight coupling - friend class BluezAdvertisement; + // Allow BluezConnection to access our private members friend class BluezConnection; }; From b95b171cb53295ef3eb65f1f75e1205db6aa704a Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Mon, 13 Nov 2023 16:32:00 +0100 Subject: [PATCH 24/25] Prevent double free when calling shutdown twice --- src/platform/Linux/bluez/BluezEndpoint.cpp | 8 +++++++- src/platform/Linux/bluez/BluezEndpoint.h | 3 ++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index 8b334a770b7642..4c315f11a7d581 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -685,12 +685,16 @@ CHIP_ERROR BluezEndpoint::Init(uint32_t aAdapterId, bool aIsCentral, const char +[](BluezEndpoint * self) { return self->StartupEndpointBindings(); }, this); VerifyOrReturnError(err == CHIP_NO_ERROR, err, ChipLogError(DeviceLayer, "Failed to schedule endpoint initialization")); - ChipLogDetail(DeviceLayer, "PlatformBlueZInit init success"); + ChipLogDetail(DeviceLayer, "BlueZ integration init success"); + mIsInitialized = true; + return CHIP_NO_ERROR; } void BluezEndpoint::Shutdown() { + VerifyOrReturn(mIsInitialized); + // Run endpoint cleanup on the CHIPoBluez thread. This is necessary because the // cleanup function releases the D-Bus manager client object, which handles D-Bus // signals. Otherwise, we will face race condition when the D-Bus signal is in @@ -725,6 +729,8 @@ void BluezEndpoint::Shutdown() g_free(mpRootPath); g_free(mpServicePath); g_free(mpPeerDevicePath); + + mIsInitialized = false; } // ConnectDevice callbacks diff --git a/src/platform/Linux/bluez/BluezEndpoint.h b/src/platform/Linux/bluez/BluezEndpoint.h index b6aee62cbeb09d..24b7ad140cde65 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.h +++ b/src/platform/Linux/bluez/BluezEndpoint.h @@ -124,7 +124,8 @@ class BluezEndpoint static void ConnectDeviceDone(GObject * aObject, GAsyncResult * aResult, gpointer apParams); static CHIP_ERROR ConnectDeviceImpl(ConnectParams * apParams); - bool mIsCentral = false; + bool mIsCentral = false; + bool mIsInitialized = false; // Bus owning name char * mpOwningName = nullptr; From 833df1e9b7869bcb8c17ee14194e3935c33a3256 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Tue, 14 Nov 2023 11:31:36 +0100 Subject: [PATCH 25/25] Keep SuccessOrExit in one line with function call --- src/platform/Linux/BLEManagerImpl.cpp | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/platform/Linux/BLEManagerImpl.cpp b/src/platform/Linux/BLEManagerImpl.cpp index 4756892cf0a364..196dd480872f84 100644 --- a/src/platform/Linux/BLEManagerImpl.cpp +++ b/src/platform/Linux/BLEManagerImpl.cpp @@ -595,15 +595,13 @@ void BLEManagerImpl::DriveBLEState() // Configure advertising data if it hasn't been done yet. if (!mFlags.Has(Flags::kAdvertisingConfigured)) { - err = mBLEAdvertisement.Init(mEndpoint, mBLEAdvType, mpBLEAdvUUID, mBLEAdvDurationMs); - SuccessOrExit(err); + SuccessOrExit(err = mBLEAdvertisement.Init(mEndpoint, mBLEAdvType, mpBLEAdvUUID, mBLEAdvDurationMs)); mFlags.Set(Flags::kAdvertisingConfigured); } // Start advertising. This is an asynchronous step. BLE manager will be notified of // advertising start completion via a call to NotifyBLEPeripheralAdvStartComplete. - err = mBLEAdvertisement.Start(); - SuccessOrExit(err); + SuccessOrExit(err = mBLEAdvertisement.Start()); mFlags.Set(Flags::kControlOpInProgress); ExitNow(); } @@ -614,8 +612,7 @@ void BLEManagerImpl::DriveBLEState() { if (mFlags.Has(Flags::kAdvertising)) { - err = mBLEAdvertisement.Stop(); - SuccessOrExit(err); + SuccessOrExit(err = mBLEAdvertisement.Stop()); mFlags.Set(Flags::kControlOpInProgress); ExitNow();