From 54a4f8d06c164c0b106eca0966d2133b8f723987 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Mon, 9 Oct 2023 15:00:08 +0200 Subject: [PATCH 01/16] Do not use BLE_CONNECTION_OBJECT by internal implementation --- src/platform/Linux/BlePlatformConfig.h | 2 ++ src/platform/Linux/bluez/BluezConnection.cpp | 18 +++++++++--------- src/platform/Linux/bluez/BluezConnection.h | 10 +++++----- 3 files changed, 16 insertions(+), 14 deletions(-) diff --git a/src/platform/Linux/BlePlatformConfig.h b/src/platform/Linux/BlePlatformConfig.h index 4c1ffd4e66420a..1da215fa57e954 100644 --- a/src/platform/Linux/BlePlatformConfig.h +++ b/src/platform/Linux/BlePlatformConfig.h @@ -33,7 +33,9 @@ struct BluezConnection; } // namespace chip // ==================== Platform Adaptations ==================== +#define BLE_CONNECTION_OBJECT chip::DeviceLayer::Internal::BluezConnection * #define BLE_CONNECTION_UNINITIALIZED nullptr + // ========== Platform-specific Configuration Overrides ========= /* none so far */ diff --git a/src/platform/Linux/bluez/BluezConnection.cpp b/src/platform/Linux/bluez/BluezConnection.cpp index 142df8f1317202..5e0bdc33bae1dd 100644 --- a/src/platform/Linux/bluez/BluezConnection.cpp +++ b/src/platform/Linux/bluez/BluezConnection.cpp @@ -85,13 +85,13 @@ static BluezConnection::ConnectionDataBundle * MakeConnectionDataBundle(BLE_CONN const chip::System::PacketBufferHandle & apBuf) { auto * bundle = g_new(BluezConnection::ConnectionDataBundle, 1); - bundle->mpConn = static_cast(apConn); + bundle->mpConn = apConn; bundle->mpVal = g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE, apBuf->Start(), apBuf->DataLength() * sizeof(uint8_t), sizeof(uint8_t)); return bundle; } -CHIP_ERROR SendBluezIndication(BLE_CONNECTION_OBJECT apConn, chip::System::PacketBufferHandle apBuf) +CHIP_ERROR SendBluezIndication(BluezConnection * apConn, chip::System::PacketBufferHandle apBuf) { VerifyOrReturnError(!apBuf.IsNull(), CHIP_ERROR_INVALID_ARGUMENT, ChipLogError(DeviceLayer, "apBuf is NULL in %s", __func__)); return PlatformMgrImpl().GLibMatterContextInvokeSync(BluezC2Indicate, MakeConnectionDataBundle(apConn, apBuf)); @@ -114,9 +114,9 @@ static CHIP_ERROR BluezDisconnect(BluezConnection * conn) return CHIP_NO_ERROR; } -CHIP_ERROR CloseBluezConnection(BLE_CONNECTION_OBJECT apConn) +CHIP_ERROR CloseBluezConnection(BluezConnection * apConn) { - return PlatformMgrImpl().GLibMatterContextInvokeSync(BluezDisconnect, static_cast(apConn)); + return PlatformMgrImpl().GLibMatterContextInvokeSync(BluezDisconnect, apConn); } // BluezSendWriteRequest callbacks @@ -152,7 +152,7 @@ static CHIP_ERROR SendWriteRequestImpl(BluezConnection::ConnectionDataBundle * d return CHIP_NO_ERROR; } -CHIP_ERROR BluezSendWriteRequest(BLE_CONNECTION_OBJECT apConn, chip::System::PacketBufferHandle apBuf) +CHIP_ERROR BluezSendWriteRequest(BluezConnection * apConn, chip::System::PacketBufferHandle apBuf) { VerifyOrReturnError(!apBuf.IsNull(), CHIP_ERROR_INVALID_ARGUMENT, ChipLogError(DeviceLayer, "apBuf is NULL in %s", __func__)); return PlatformMgrImpl().GLibMatterContextInvokeSync(SendWriteRequestImpl, MakeConnectionDataBundle(apConn, apBuf)); @@ -200,9 +200,9 @@ static CHIP_ERROR SubscribeCharacteristicImpl(BluezConnection * connection) return CHIP_NO_ERROR; } -CHIP_ERROR BluezSubscribeCharacteristic(BLE_CONNECTION_OBJECT apConn) +CHIP_ERROR BluezSubscribeCharacteristic(BluezConnection * apConn) { - return PlatformMgrImpl().GLibMatterContextInvokeSync(SubscribeCharacteristicImpl, static_cast(apConn)); + return PlatformMgrImpl().GLibMatterContextInvokeSync(SubscribeCharacteristicImpl, apConn); } // BluezUnsubscribeCharacteristic callbacks @@ -231,9 +231,9 @@ static CHIP_ERROR UnsubscribeCharacteristicImpl(BluezConnection * connection) return CHIP_NO_ERROR; } -CHIP_ERROR BluezUnsubscribeCharacteristic(BLE_CONNECTION_OBJECT apConn) +CHIP_ERROR BluezUnsubscribeCharacteristic(BluezConnection * apConn) { - return PlatformMgrImpl().GLibMatterContextInvokeSync(UnsubscribeCharacteristicImpl, static_cast(apConn)); + return PlatformMgrImpl().GLibMatterContextInvokeSync(UnsubscribeCharacteristicImpl, apConn); } } // namespace Internal diff --git a/src/platform/Linux/bluez/BluezConnection.h b/src/platform/Linux/bluez/BluezConnection.h index 7375fbe79d915d..55ae1486138833 100644 --- a/src/platform/Linux/bluez/BluezConnection.h +++ b/src/platform/Linux/bluez/BluezConnection.h @@ -64,15 +64,15 @@ struct BluezConnection BluezEndpoint * mpEndpoint; }; -CHIP_ERROR SendBluezIndication(BLE_CONNECTION_OBJECT apConn, chip::System::PacketBufferHandle apBuf); -CHIP_ERROR CloseBluezConnection(BLE_CONNECTION_OBJECT apConn); +CHIP_ERROR SendBluezIndication(BluezConnection * apConn, chip::System::PacketBufferHandle apBuf); +CHIP_ERROR CloseBluezConnection(BluezConnection * apConn); /// Write to the CHIP RX characteristic on the remote peripheral device -CHIP_ERROR BluezSendWriteRequest(BLE_CONNECTION_OBJECT apConn, chip::System::PacketBufferHandle apBuf); +CHIP_ERROR BluezSendWriteRequest(BluezConnection * apConn, chip::System::PacketBufferHandle apBuf); /// Subscribe to the CHIP TX characteristic on the remote peripheral device -CHIP_ERROR BluezSubscribeCharacteristic(BLE_CONNECTION_OBJECT apConn); +CHIP_ERROR BluezSubscribeCharacteristic(BluezConnection * apConn); /// Unsubscribe from the CHIP TX characteristic on the remote peripheral device -CHIP_ERROR BluezUnsubscribeCharacteristic(BLE_CONNECTION_OBJECT apConn); +CHIP_ERROR BluezUnsubscribeCharacteristic(BluezConnection * apConn); } // namespace Internal } // namespace DeviceLayer From 0734f4813d9e98b4d0dd0d8dd417b403ee25d74f Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Tue, 10 Oct 2023 09:17:19 +0200 Subject: [PATCH 02/16] Wrap BluezConnection in a class --- src/platform/Linux/BLEManagerImpl.cpp | 25 +- src/platform/Linux/BlePlatformConfig.h | 2 +- src/platform/Linux/bluez/BluezConnection.cpp | 234 ++++++++++++++----- src/platform/Linux/bluez/BluezConnection.h | 71 ++++-- src/platform/Linux/bluez/BluezEndpoint.cpp | 146 +----------- 5 files changed, 248 insertions(+), 230 deletions(-) diff --git a/src/platform/Linux/BLEManagerImpl.cpp b/src/platform/Linux/BLEManagerImpl.cpp index 4e4e853ccdc69f..a08460972f36ad 100644 --- a/src/platform/Linux/BLEManagerImpl.cpp +++ b/src/platform/Linux/BLEManagerImpl.cpp @@ -344,20 +344,23 @@ void BLEManagerImpl::HandlePlatformSpecificBLEEvent(const ChipDeviceEvent * apEv uint16_t BLEManagerImpl::GetMTU(BLE_CONNECTION_OBJECT conId) const { - BluezConnection * connection = static_cast(conId); - return (connection != nullptr) ? connection->mMtu : 0; + VerifyOrReturnValue(conId != BLE_CONNECTION_UNINITIALIZED, 0, + ChipLogError(DeviceLayer, "BLE connection is not initialized in %s", __func__)); + return conId->mMtu; } bool BLEManagerImpl::SubscribeCharacteristic(BLE_CONNECTION_OBJECT conId, const ChipBleUUID * svcId, const ChipBleUUID * charId) { bool result = false; + VerifyOrExit(conId != BLE_CONNECTION_UNINITIALIZED, + ChipLogError(DeviceLayer, "BLE connection is not initialized in %s", __func__)); VerifyOrExit(Ble::UUIDsMatch(svcId, &CHIP_BLE_SVC_ID), ChipLogError(DeviceLayer, "SubscribeCharacteristic() called with invalid service ID")); VerifyOrExit(Ble::UUIDsMatch(charId, &ChipUUID_CHIPoBLEChar_TX), ChipLogError(DeviceLayer, "SubscribeCharacteristic() called with invalid characteristic ID")); - VerifyOrExit(BluezSubscribeCharacteristic(conId) == CHIP_NO_ERROR, + VerifyOrExit(conId->BluezSubscribeCharacteristic() == CHIP_NO_ERROR, ChipLogError(DeviceLayer, "BluezSubscribeCharacteristic() failed")); result = true; @@ -369,12 +372,14 @@ bool BLEManagerImpl::UnsubscribeCharacteristic(BLE_CONNECTION_OBJECT conId, cons { bool result = false; + VerifyOrExit(conId != BLE_CONNECTION_UNINITIALIZED, + ChipLogError(DeviceLayer, "BLE connection is not initialized in %s", __func__)); VerifyOrExit(Ble::UUIDsMatch(svcId, &CHIP_BLE_SVC_ID), ChipLogError(DeviceLayer, "UnsubscribeCharacteristic() called with invalid service ID")); VerifyOrExit(Ble::UUIDsMatch(charId, &ChipUUID_CHIPoBLEChar_TX), ChipLogError(DeviceLayer, "UnsubscribeCharacteristic() called with invalid characteristic ID")); - VerifyOrExit(BluezUnsubscribeCharacteristic(conId) == CHIP_NO_ERROR, + VerifyOrExit(conId->BluezUnsubscribeCharacteristic() == CHIP_NO_ERROR, ChipLogError(DeviceLayer, "BluezUnsubscribeCharacteristic() failed")); result = true; @@ -386,9 +391,11 @@ bool BLEManagerImpl::CloseConnection(BLE_CONNECTION_OBJECT conId) { bool result = false; + VerifyOrExit(conId != BLE_CONNECTION_UNINITIALIZED, + ChipLogError(DeviceLayer, "BLE connection is not initialized in %s", __func__)); ChipLogProgress(DeviceLayer, "Closing BLE GATT connection (con %p)", conId); - VerifyOrExit(CloseBluezConnection(conId) == CHIP_NO_ERROR, ChipLogError(DeviceLayer, "CloseBluezConnection() failed")); + VerifyOrExit(conId->CloseBluezConnection() == CHIP_NO_ERROR, ChipLogError(DeviceLayer, "CloseBluezConnection() failed")); result = true; exit: @@ -400,7 +407,9 @@ bool BLEManagerImpl::SendIndication(BLE_CONNECTION_OBJECT conId, const ChipBleUU { bool result = false; - VerifyOrExit(SendBluezIndication(conId, std::move(pBuf)) == CHIP_NO_ERROR, + VerifyOrExit(conId != BLE_CONNECTION_UNINITIALIZED, + ChipLogError(DeviceLayer, "BLE connection is not initialized in %s", __func__)); + VerifyOrExit(conId->SendBluezIndication(std::move(pBuf)) == CHIP_NO_ERROR, ChipLogError(DeviceLayer, "SendBluezIndication() failed")); result = true; @@ -413,12 +422,14 @@ bool BLEManagerImpl::SendWriteRequest(BLE_CONNECTION_OBJECT conId, const Ble::Ch { bool result = false; + VerifyOrExit(conId != BLE_CONNECTION_UNINITIALIZED, + ChipLogError(DeviceLayer, "BLE connection is not initialized in %s", __func__)); VerifyOrExit(Ble::UUIDsMatch(svcId, &CHIP_BLE_SVC_ID), ChipLogError(DeviceLayer, "SendWriteRequest() called with invalid service ID")); VerifyOrExit(Ble::UUIDsMatch(charId, &ChipUUID_CHIPoBLEChar_RX), ChipLogError(DeviceLayer, "SendWriteRequest() called with invalid characteristic ID")); - VerifyOrExit(BluezSendWriteRequest(conId, std::move(pBuf)) == CHIP_NO_ERROR, + VerifyOrExit(conId->BluezSendWriteRequest(std::move(pBuf)) == CHIP_NO_ERROR, ChipLogError(DeviceLayer, "BluezSendWriteRequest() failed")); result = true; diff --git a/src/platform/Linux/BlePlatformConfig.h b/src/platform/Linux/BlePlatformConfig.h index 1da215fa57e954..54640b707ab720 100644 --- a/src/platform/Linux/BlePlatformConfig.h +++ b/src/platform/Linux/BlePlatformConfig.h @@ -27,7 +27,7 @@ namespace chip { namespace DeviceLayer { namespace Internal { -struct BluezConnection; +class BluezConnection; } // namespace Internal } // namespace DeviceLayer } // namespace chip diff --git a/src/platform/Linux/bluez/BluezConnection.cpp b/src/platform/Linux/bluez/BluezConnection.cpp index 5e0bdc33bae1dd..7c75da7281d22f 100644 --- a/src/platform/Linux/bluez/BluezConnection.cpp +++ b/src/platform/Linux/bluez/BluezConnection.cpp @@ -40,69 +40,189 @@ namespace chip { namespace DeviceLayer { namespace Internal { -static CHIP_ERROR BluezC2Indicate(BluezConnection::ConnectionDataBundle * closure) +namespace { + +gboolean BluezIsServiceOnDevice(BluezGattService1 * aService, BluezDevice1 * aDevice) +{ + const auto * servicePath = bluez_gatt_service1_get_device(aService); + const auto * devicePath = g_dbus_proxy_get_object_path(G_DBUS_PROXY(aDevice)); + return strcmp(servicePath, devicePath) == 0 ? TRUE : FALSE; +} + +gboolean BluezIsCharOnService(BluezGattCharacteristic1 * aChar, BluezGattService1 * aService) +{ + const auto * charPath = bluez_gatt_characteristic1_get_service(aChar); + const auto * servicePath = g_dbus_proxy_get_object_path(G_DBUS_PROXY(aService)); + ChipLogDetail(DeviceLayer, "Char %s on service %s", charPath, servicePath); + return strcmp(charPath, servicePath) == 0 ? TRUE : FALSE; +} + +} // namespace + +BluezConnection::BluezConnection(BluezEndpoint * apEndpoint, BluezDevice1 * apDevice) : + mpEndpoint(apEndpoint), mpDevice(BLUEZ_DEVICE1(g_object_ref(apDevice))) +{ + mpPeerAddress = g_strdup(bluez_device1_get_address(apDevice)); + Init(); +} + +BluezConnection::~BluezConnection() +{ + if (mpDevice) + g_object_unref(mpDevice); + if (mpService) + g_object_unref(mpService); + if (mpC1) + g_object_unref(mpC1); + if (mpC2) + g_object_unref(mpC2); + if (mpPeerAddress) + g_free(mpPeerAddress); + if (mC1Channel.mWatchSource) + { + g_source_destroy(mC1Channel.mWatchSource); + g_source_unref(mC1Channel.mWatchSource); + } + if (mC1Channel.mpChannel) + g_io_channel_unref(mC1Channel.mpChannel); + if (mC2Channel.mWatchSource) + { + g_source_destroy(mC2Channel.mWatchSource); + g_source_unref(mC2Channel.mWatchSource); + } + if (mC2Channel.mpChannel) + g_io_channel_unref(mC2Channel.mpChannel); +} + +BluezConnection::ConnectionDataBundle::ConnectionDataBundle(BluezConnection * apConn, + const chip::System::PacketBufferHandle & apBuf) +{ + mpConn = apConn; + mData.reset(g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE, apBuf->Start(), apBuf->DataLength(), sizeof(uint8_t))); +} + +CHIP_ERROR BluezConnection::Init() +{ + // populate the service and the characteristics + GList * objects = nullptr; + GList * l; + + VerifyOrExit(mpEndpoint != nullptr, ChipLogError(DeviceLayer, "endpoint is NULL in %s", __func__)); + + if (!mpEndpoint->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)); + } + else + { + objects = g_dbus_object_manager_get_objects(mpEndpoint->mpObjMgr); + + for (l = objects; l != nullptr; l = l->next) + { + BluezObject * object = BLUEZ_OBJECT(l->data); + BluezGattService1 * service = bluez_object_get_gatt_service1(object); + + if (service != nullptr) + { + if ((BluezIsServiceOnDevice(service, mpDevice)) == TRUE && + (strcmp(bluez_gatt_service1_get_uuid(service), CHIP_BLE_UUID_SERVICE_STRING) == 0)) + { + mpService = service; + break; + } + g_object_unref(service); + } + } + + VerifyOrExit(mpService != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL service in %s", __func__)); + + for (l = objects; l != nullptr; l = l->next) + { + BluezObject * object = BLUEZ_OBJECT(l->data); + BluezGattCharacteristic1 * char1 = bluez_object_get_gatt_characteristic1(object); + + if (char1 != nullptr) + { + if ((BluezIsCharOnService(char1, mpService) == TRUE) && + (strcmp(bluez_gatt_characteristic1_get_uuid(char1), CHIP_PLAT_BLE_UUID_C1_STRING) == 0)) + { + mpC1 = char1; + } + else if ((BluezIsCharOnService(char1, mpService) == TRUE) && + (strcmp(bluez_gatt_characteristic1_get_uuid(char1), CHIP_PLAT_BLE_UUID_C2_STRING) == 0)) + { + mpC2 = char1; + } + else if ((BluezIsCharOnService(char1, mpService) == TRUE) && + (strcmp(bluez_gatt_characteristic1_get_uuid(char1), CHIP_PLAT_BLE_UUID_C3_STRING) == 0)) + { + mpC3 = char1; + } + else + { + g_object_unref(char1); + } + if ((mpC1 != nullptr) && (mpC2 != nullptr)) + { + break; + } + } + } + + VerifyOrExit(mpC1 != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL C1 in %s", __func__)); + VerifyOrExit(mpC2 != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL C2 in %s", __func__)); + } + +exit: + if (objects != nullptr) + g_list_free_full(objects, g_object_unref); + return CHIP_NO_ERROR; +} + +CHIP_ERROR BluezConnection::BluezC2Indicate(ConnectionDataBundle * data) { BluezConnection * conn = nullptr; GAutoPtr error; GIOStatus status; - const char * buf; size_t len, written; - VerifyOrExit(closure != nullptr, ChipLogError(DeviceLayer, "ConnectionDataBundle is NULL in %s", __func__)); - - conn = closure->mpConn; - VerifyOrExit(conn != nullptr, ChipLogError(DeviceLayer, "BluezConnection is NULL in %s", __func__)); + VerifyOrExit((conn = data->mpConn) != nullptr, ChipLogError(DeviceLayer, "BluezConnection is NULL in %s", __func__)); VerifyOrExit(conn->mpC2 != nullptr, ChipLogError(DeviceLayer, "FAIL: C2 Indicate: %s", "NULL C2")); if (bluez_gatt_characteristic1_get_notify_acquired(conn->mpC2) == TRUE) { - buf = (char *) g_variant_get_fixed_array(closure->mpVal, &len, sizeof(uint8_t)); + auto * buf = static_cast(g_variant_get_fixed_array(data->mData.get(), &len, sizeof(uint8_t))); VerifyOrExit(len <= static_cast(std::numeric_limits::max()), ChipLogError(DeviceLayer, "FAIL: buffer too large in %s", __func__)); status = g_io_channel_write_chars(conn->mC2Channel.mpChannel, buf, static_cast(len), &written, &MakeUniquePointerReceiver(error).Get()); - g_variant_unref(closure->mpVal); - closure->mpVal = nullptr; - VerifyOrExit(status == G_IO_STATUS_NORMAL, ChipLogError(DeviceLayer, "FAIL: C2 Indicate: %s", error->message)); } else { - bluez_gatt_characteristic1_set_value(conn->mpC2, closure->mpVal); - closure->mpVal = nullptr; + bluez_gatt_characteristic1_set_value(conn->mpC2, data->mData.release()); } exit: - if (closure != nullptr) - { - g_free(closure); - } - + Platform::Delete(data); return CHIP_NO_ERROR; } -static BluezConnection::ConnectionDataBundle * MakeConnectionDataBundle(BLE_CONNECTION_OBJECT apConn, - const chip::System::PacketBufferHandle & apBuf) -{ - auto * bundle = g_new(BluezConnection::ConnectionDataBundle, 1); - bundle->mpConn = apConn; - bundle->mpVal = - g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE, apBuf->Start(), apBuf->DataLength() * sizeof(uint8_t), sizeof(uint8_t)); - return bundle; -} - -CHIP_ERROR SendBluezIndication(BluezConnection * apConn, chip::System::PacketBufferHandle apBuf) +CHIP_ERROR BluezConnection::SendBluezIndication(chip::System::PacketBufferHandle apBuf) { + ConnectionDataBundle * bundle; VerifyOrReturnError(!apBuf.IsNull(), CHIP_ERROR_INVALID_ARGUMENT, ChipLogError(DeviceLayer, "apBuf is NULL in %s", __func__)); - return PlatformMgrImpl().GLibMatterContextInvokeSync(BluezC2Indicate, MakeConnectionDataBundle(apConn, apBuf)); + VerifyOrReturnError((bundle = Platform::New(this, apBuf)) != nullptr, CHIP_ERROR_NO_MEMORY); + return PlatformMgrImpl().GLibMatterContextInvokeSync(BluezC2Indicate, bundle); } -static CHIP_ERROR BluezDisconnect(BluezConnection * conn) +CHIP_ERROR BluezConnection::BluezDisconnect(BluezConnection * conn) { GAutoPtr error; gboolean success; - VerifyOrExit(conn != nullptr, ChipLogError(DeviceLayer, "conn is NULL in %s", __func__)); VerifyOrExit(conn->mpDevice != nullptr, ChipLogError(DeviceLayer, "FAIL: Disconnect: %s", "NULL Device")); ChipLogDetail(DeviceLayer, "%s peer=%s", __func__, bluez_device1_get_address(conn->mpDevice)); @@ -114,56 +234,55 @@ static CHIP_ERROR BluezDisconnect(BluezConnection * conn) return CHIP_NO_ERROR; } -CHIP_ERROR CloseBluezConnection(BluezConnection * apConn) +CHIP_ERROR BluezConnection::CloseBluezConnection() { - return PlatformMgrImpl().GLibMatterContextInvokeSync(BluezDisconnect, apConn); + return PlatformMgrImpl().GLibMatterContextInvokeSync(BluezDisconnect, this); } // BluezSendWriteRequest callbacks -static void SendWriteRequestDone(GObject * aObject, GAsyncResult * aResult, gpointer apConnection) +void BluezConnection::SendWriteRequestDone(GObject * aObject, GAsyncResult * aResult, gpointer apConnection) { BluezGattCharacteristic1 * c1 = BLUEZ_GATT_CHARACTERISTIC1(aObject); GAutoPtr error; gboolean success = bluez_gatt_characteristic1_call_write_value_finish(c1, aResult, &MakeUniquePointerReceiver(error).Get()); VerifyOrReturn(success == TRUE, ChipLogError(DeviceLayer, "FAIL: BluezSendWriteRequest : %s", error->message)); - BLEManagerImpl::HandleWriteComplete(static_cast(apConnection)); + BLEManagerImpl::HandleWriteComplete(static_cast(apConnection)); } -static CHIP_ERROR SendWriteRequestImpl(BluezConnection::ConnectionDataBundle * data) +CHIP_ERROR BluezConnection::SendWriteRequestImpl(ConnectionDataBundle * data) { GVariant * options = nullptr; GVariantBuilder optionsBuilder; - VerifyOrExit(data != nullptr, ChipLogError(DeviceLayer, "ConnectionDataBundle is NULL in %s", __func__)); - VerifyOrExit(data->mpConn != nullptr, ChipLogError(DeviceLayer, "BluezConnection is NULL in %s", __func__)); VerifyOrExit(data->mpConn->mpC1 != nullptr, ChipLogError(DeviceLayer, "C1 is NULL in %s", __func__)); g_variant_builder_init(&optionsBuilder, G_VARIANT_TYPE_ARRAY); g_variant_builder_add(&optionsBuilder, "{sv}", "type", g_variant_new_string("request")); options = g_variant_builder_end(&optionsBuilder); - bluez_gatt_characteristic1_call_write_value(data->mpConn->mpC1, data->mpVal, options, nullptr, SendWriteRequestDone, + bluez_gatt_characteristic1_call_write_value(data->mpConn->mpC1, data->mData.release(), options, nullptr, SendWriteRequestDone, data->mpConn); exit: - g_free(data); + Platform::Delete(data); return CHIP_NO_ERROR; } -CHIP_ERROR BluezSendWriteRequest(BluezConnection * apConn, chip::System::PacketBufferHandle apBuf) +CHIP_ERROR BluezConnection::BluezSendWriteRequest(chip::System::PacketBufferHandle apBuf) { + ConnectionDataBundle * bundle; VerifyOrReturnError(!apBuf.IsNull(), CHIP_ERROR_INVALID_ARGUMENT, ChipLogError(DeviceLayer, "apBuf is NULL in %s", __func__)); - return PlatformMgrImpl().GLibMatterContextInvokeSync(SendWriteRequestImpl, MakeConnectionDataBundle(apConn, apBuf)); + VerifyOrReturnError((bundle = Platform::New(this, apBuf)) != nullptr, CHIP_ERROR_NO_MEMORY); + return PlatformMgrImpl().GLibMatterContextInvokeSync(SendWriteRequestImpl, bundle); } // BluezSubscribeCharacteristic callbacks -static void OnCharacteristicChanged(GDBusProxy * aInterface, GVariant * aChangedProperties, const gchar * const * aInvalidatedProps, - gpointer apConnection) +void BluezConnection::OnCharacteristicChanged(GDBusProxy * aInterface, GVariant * aChangedProperties, + const gchar * const * aInvalidatedProps, gpointer apConnection) { - BLE_CONNECTION_OBJECT connection = static_cast(apConnection); GAutoPtr dataValue(g_variant_lookup_value(aChangedProperties, "Value", G_VARIANT_TYPE_BYTESTRING)); VerifyOrReturn(dataValue != nullptr); @@ -171,10 +290,11 @@ static void OnCharacteristicChanged(GDBusProxy * aInterface, GVariant * aChanged auto buffer = g_variant_get_fixed_array(dataValue.get(), &bufferLen, sizeof(uint8_t)); VerifyOrReturn(buffer != nullptr, ChipLogError(DeviceLayer, "Characteristic value has unexpected type")); - BLEManagerImpl::HandleTXCharChanged(connection, static_cast(buffer), bufferLen); + BLEManagerImpl::HandleTXCharChanged(static_cast(apConnection), static_cast(buffer), + bufferLen); } -static void SubscribeCharacteristicDone(GObject * aObject, GAsyncResult * aResult, gpointer apConnection) +void BluezConnection::SubscribeCharacteristicDone(GObject * aObject, GAsyncResult * aResult, gpointer apConnection) { BluezGattCharacteristic1 * c2 = BLUEZ_GATT_CHARACTERISTIC1(aObject); GAutoPtr error; @@ -182,13 +302,12 @@ static void SubscribeCharacteristicDone(GObject * aObject, GAsyncResult * aResul VerifyOrReturn(success == TRUE, ChipLogError(DeviceLayer, "FAIL: BluezSubscribeCharacteristic : %s", error->message)); - BLEManagerImpl::HandleSubscribeOpComplete(static_cast(apConnection), true); + BLEManagerImpl::HandleSubscribeOpComplete(static_cast(apConnection), true); } -static CHIP_ERROR SubscribeCharacteristicImpl(BluezConnection * connection) +CHIP_ERROR BluezConnection::SubscribeCharacteristicImpl(BluezConnection * connection) { BluezGattCharacteristic1 * c2 = nullptr; - VerifyOrExit(connection != nullptr, ChipLogError(DeviceLayer, "BluezConnection is NULL in %s", __func__)); VerifyOrExit(connection->mpC2 != nullptr, ChipLogError(DeviceLayer, "C2 is NULL in %s", __func__)); c2 = BLUEZ_GATT_CHARACTERISTIC1(connection->mpC2); @@ -200,14 +319,14 @@ static CHIP_ERROR SubscribeCharacteristicImpl(BluezConnection * connection) return CHIP_NO_ERROR; } -CHIP_ERROR BluezSubscribeCharacteristic(BluezConnection * apConn) +CHIP_ERROR BluezConnection::BluezSubscribeCharacteristic() { - return PlatformMgrImpl().GLibMatterContextInvokeSync(SubscribeCharacteristicImpl, apConn); + return PlatformMgrImpl().GLibMatterContextInvokeSync(SubscribeCharacteristicImpl, this); } // BluezUnsubscribeCharacteristic callbacks -static void UnsubscribeCharacteristicDone(GObject * aObject, GAsyncResult * aResult, gpointer apConnection) +void BluezConnection::UnsubscribeCharacteristicDone(GObject * aObject, GAsyncResult * aResult, gpointer apConnection) { BluezGattCharacteristic1 * c2 = BLUEZ_GATT_CHARACTERISTIC1(aObject); GAutoPtr error; @@ -217,12 +336,11 @@ static void UnsubscribeCharacteristicDone(GObject * aObject, GAsyncResult * aRes // Stop listening to the TX characteristic changes g_signal_handlers_disconnect_by_data(c2, apConnection); - BLEManagerImpl::HandleSubscribeOpComplete(static_cast(apConnection), false); + BLEManagerImpl::HandleSubscribeOpComplete(static_cast(apConnection), false); } -static CHIP_ERROR UnsubscribeCharacteristicImpl(BluezConnection * connection) +CHIP_ERROR BluezConnection::UnsubscribeCharacteristicImpl(BluezConnection * connection) { - VerifyOrExit(connection != nullptr, ChipLogError(DeviceLayer, "BluezConnection is NULL in %s", __func__)); VerifyOrExit(connection->mpC2 != nullptr, ChipLogError(DeviceLayer, "C2 is NULL in %s", __func__)); bluez_gatt_characteristic1_call_stop_notify(connection->mpC2, nullptr, UnsubscribeCharacteristicDone, connection); @@ -231,9 +349,9 @@ static CHIP_ERROR UnsubscribeCharacteristicImpl(BluezConnection * connection) return CHIP_NO_ERROR; } -CHIP_ERROR BluezUnsubscribeCharacteristic(BluezConnection * apConn) +CHIP_ERROR BluezConnection::BluezUnsubscribeCharacteristic() { - return PlatformMgrImpl().GLibMatterContextInvokeSync(UnsubscribeCharacteristicImpl, apConn); + return PlatformMgrImpl().GLibMatterContextInvokeSync(UnsubscribeCharacteristicImpl, this); } } // namespace Internal diff --git a/src/platform/Linux/bluez/BluezConnection.h b/src/platform/Linux/bluez/BluezConnection.h index 55ae1486138833..154f9f33f35eca 100644 --- a/src/platform/Linux/bluez/BluezConnection.h +++ b/src/platform/Linux/bluez/BluezConnection.h @@ -34,45 +34,70 @@ namespace Internal { struct BluezEndpoint; -struct BluezConnection +class BluezConnection { - +public: struct IOChannel { GIOChannel * mpChannel; GSource * mWatchSource; }; + BluezConnection(BluezEndpoint * apEndpoint, BluezDevice1 * apDevice); + ~BluezConnection(); + + CHIP_ERROR SendBluezIndication(chip::System::PacketBufferHandle apBuf); + CHIP_ERROR CloseBluezConnection(); + + /// Write to the CHIP RX characteristic on the remote peripheral device + CHIP_ERROR BluezSendWriteRequest(chip::System::PacketBufferHandle apBuf); + /// Subscribe to the CHIP TX characteristic on the remote peripheral device + CHIP_ERROR BluezSubscribeCharacteristic(); + /// Unsubscribe from the CHIP TX characteristic on the remote peripheral device + CHIP_ERROR BluezUnsubscribeCharacteristic(); + +private: struct ConnectionDataBundle { + ConnectionDataBundle(BluezConnection *, const chip::System::PacketBufferHandle &); + ~ConnectionDataBundle() = default; + BluezConnection * mpConn; - GVariant * mpVal; + GAutoPtr mData; }; - char * mpPeerAddress; - BluezDevice1 * mpDevice; - BluezGattService1 * mpService; - BluezGattCharacteristic1 * mpC1; - BluezGattCharacteristic1 * mpC2; - // additional data characteristics - BluezGattCharacteristic1 * mpC3; + CHIP_ERROR Init(); + + static CHIP_ERROR BluezC2Indicate(ConnectionDataBundle * data); + static CHIP_ERROR BluezDisconnect(BluezConnection * apConn); + + static void SendWriteRequestDone(GObject * aObject, GAsyncResult * aResult, gpointer apConn); + static CHIP_ERROR SendWriteRequestImpl(ConnectionDataBundle * data); + + static void OnCharacteristicChanged(GDBusProxy * aInterface, GVariant * aChangedProperties, + const gchar * const * aInvalidatedProps, gpointer apConn); + static void SubscribeCharacteristicDone(GObject * aObject, GAsyncResult * aResult, gpointer apConn); + static CHIP_ERROR SubscribeCharacteristicImpl(BluezConnection * apConn); + + static void UnsubscribeCharacteristicDone(GObject * aObject, GAsyncResult * aResult, gpointer apConn); + static CHIP_ERROR UnsubscribeCharacteristicImpl(BluezConnection * apConn); - bool mIsNotify; - uint16_t mMtu; - struct IOChannel mC1Channel; - struct IOChannel mC2Channel; BluezEndpoint * mpEndpoint; -}; + BluezDevice1 * mpDevice; -CHIP_ERROR SendBluezIndication(BluezConnection * apConn, chip::System::PacketBufferHandle apBuf); -CHIP_ERROR CloseBluezConnection(BluezConnection * apConn); +public: + char * mpPeerAddress; + BluezGattService1 * mpService = nullptr; + BluezGattCharacteristic1 * mpC1 = nullptr; + BluezGattCharacteristic1 * mpC2 = nullptr; + // additional data characteristics + BluezGattCharacteristic1 * mpC3 = nullptr; -/// Write to the CHIP RX characteristic on the remote peripheral device -CHIP_ERROR BluezSendWriteRequest(BluezConnection * apConn, chip::System::PacketBufferHandle apBuf); -/// Subscribe to the CHIP TX characteristic on the remote peripheral device -CHIP_ERROR BluezSubscribeCharacteristic(BluezConnection * apConn); -/// Unsubscribe from the CHIP TX characteristic on the remote peripheral device -CHIP_ERROR BluezUnsubscribeCharacteristic(BluezConnection * apConn); + bool mIsNotify = false; + uint16_t mMtu = 0; + struct IOChannel mC1Channel = { 0 }; + struct IOChannel mC2Channel = { 0 }; +}; } // namespace Internal } // namespace DeviceLayer diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index d6f3196a6b877c..f9e9d6b775a72a 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -593,136 +593,6 @@ 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 gboolean BluezIsServiceOnDevice(BluezGattService1 * aService, BluezDevice1 * aDevice) -{ - return strcmp(bluez_gatt_service1_get_device(aService), g_dbus_proxy_get_object_path(G_DBUS_PROXY(aDevice))) == 0 ? TRUE - : FALSE; -} - -static gboolean BluezIsCharOnService(BluezGattCharacteristic1 * aChar, BluezGattService1 * aService) -{ - ChipLogDetail(DeviceLayer, "Char1 %s", bluez_gatt_characteristic1_get_service(aChar)); - ChipLogDetail(DeviceLayer, "Char1 %s", g_dbus_proxy_get_object_path(G_DBUS_PROXY(aService))); - return strcmp(bluez_gatt_characteristic1_get_service(aChar), g_dbus_proxy_get_object_path(G_DBUS_PROXY(aService))) == 0 ? TRUE - : FALSE; -} - -static void BluezConnectionInit(BluezConnection * apConn) -{ - // populate the service and the characteristics - GList * objects = nullptr; - GList * l; - BluezEndpoint * endpoint = nullptr; - - VerifyOrExit(apConn != nullptr, ChipLogError(DeviceLayer, "Bluez connection is NULL in %s", __func__)); - - endpoint = apConn->mpEndpoint; - VerifyOrExit(endpoint != nullptr, ChipLogError(DeviceLayer, "endpoint is NULL in %s", __func__)); - - if (!endpoint->mIsCentral) - { - apConn->mpService = BLUEZ_GATT_SERVICE1(g_object_ref(apConn->mpEndpoint->mpService)); - apConn->mpC1 = BLUEZ_GATT_CHARACTERISTIC1(g_object_ref(endpoint->mpC1)); - apConn->mpC2 = BLUEZ_GATT_CHARACTERISTIC1(g_object_ref(endpoint->mpC2)); - } - else - { - objects = g_dbus_object_manager_get_objects(endpoint->mpObjMgr); - - for (l = objects; l != nullptr; l = l->next) - { - BluezObject * object = BLUEZ_OBJECT(l->data); - BluezGattService1 * service = bluez_object_get_gatt_service1(object); - - if (service != nullptr) - { - if ((BluezIsServiceOnDevice(service, apConn->mpDevice)) == TRUE && - (strcmp(bluez_gatt_service1_get_uuid(service), CHIP_BLE_UUID_SERVICE_STRING) == 0)) - { - apConn->mpService = service; - break; - } - g_object_unref(service); - } - } - - VerifyOrExit(apConn->mpService != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL service in %s", __func__)); - - for (l = objects; l != nullptr; l = l->next) - { - BluezObject * object = BLUEZ_OBJECT(l->data); - BluezGattCharacteristic1 * char1 = bluez_object_get_gatt_characteristic1(object); - - if (char1 != nullptr) - { - if ((BluezIsCharOnService(char1, apConn->mpService) == TRUE) && - (strcmp(bluez_gatt_characteristic1_get_uuid(char1), CHIP_PLAT_BLE_UUID_C1_STRING) == 0)) - { - apConn->mpC1 = char1; - } - else if ((BluezIsCharOnService(char1, apConn->mpService) == TRUE) && - (strcmp(bluez_gatt_characteristic1_get_uuid(char1), CHIP_PLAT_BLE_UUID_C2_STRING) == 0)) - { - apConn->mpC2 = char1; - } - else if ((BluezIsCharOnService(char1, apConn->mpService) == TRUE) && - (strcmp(bluez_gatt_characteristic1_get_uuid(char1), CHIP_PLAT_BLE_UUID_C3_STRING) == 0)) - { - apConn->mpC3 = char1; - } - else - { - g_object_unref(char1); - } - if ((apConn->mpC1 != nullptr) && (apConn->mpC2 != nullptr)) - { - break; - } - } - } - - VerifyOrExit(apConn->mpC1 != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL C1 in %s", __func__)); - VerifyOrExit(apConn->mpC2 != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL C2 in %s", __func__)); - } - -exit: - if (objects != nullptr) - g_list_free_full(objects, g_object_unref); -} - -static void BluezOTConnectionDestroy(BluezConnection * aConn) -{ - if (aConn) - { - if (aConn->mpDevice) - g_object_unref(aConn->mpDevice); - if (aConn->mpService) - g_object_unref(aConn->mpService); - if (aConn->mpC1) - g_object_unref(aConn->mpC1); - if (aConn->mpC2) - g_object_unref(aConn->mpC2); - if (aConn->mpPeerAddress) - g_free(aConn->mpPeerAddress); - if (aConn->mC1Channel.mWatchSource) - { - g_source_destroy(aConn->mC1Channel.mWatchSource); - g_source_unref(aConn->mC1Channel.mWatchSource); - } - if (aConn->mC1Channel.mpChannel) - g_io_channel_unref(aConn->mC1Channel.mpChannel); - if (aConn->mC2Channel.mWatchSource) - { - g_source_destroy(aConn->mC2Channel.mWatchSource); - g_source_unref(aConn->mC2Channel.mWatchSource); - } - if (aConn->mC2Channel.mpChannel) - g_io_channel_unref(aConn->mC2Channel.mpChannel); - - g_free(aConn); - } -} - static BluezGattCharacteristic1 * BluezCharacteristicCreate(BluezGattService1 * aService, const char * aCharName, const char * aUUID, GDBusObjectManagerServer * aRoot) { @@ -801,7 +671,7 @@ static void UpdateConnectionTable(BluezDevice1 * apDevice, BluezEndpoint & aEndp BLEManagerImpl::CHIPoBluez_ConnectionClosed(connection); // 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. - BluezOTConnectionDestroy(connection); + chip::Platform::Delete(connection); g_hash_table_remove(aEndpoint.mpConnMap, objectPath); return; } @@ -814,11 +684,7 @@ static void UpdateConnectionTable(BluezDevice1 * apDevice, BluezEndpoint & aEndp if (connection == nullptr && bluez_device1_get_connected(apDevice) && (!aEndpoint.mIsCentral || bluez_device1_get_services_resolved(apDevice))) { - connection = g_new0(BluezConnection, 1); - connection->mpPeerAddress = g_strdup(bluez_device1_get_address(apDevice)); - connection->mpDevice = static_cast(g_object_ref(apDevice)); - connection->mpEndpoint = &aEndpoint; - BluezConnectionInit(connection); + connection = chip::Platform::New(&aEndpoint, apDevice); aEndpoint.mpPeerDevicePath = g_strdup(objectPath); g_hash_table_insert(aEndpoint.mpConnMap, aEndpoint.mpPeerDevicePath, connection); @@ -865,11 +731,7 @@ static void BluezHandleNewDevice(BluezDevice1 * device, BluezEndpoint * apEndpoi ChipLogError(DeviceLayer, "FAIL: connection already tracked: conn: %p new device: %s", conn, g_dbus_proxy_get_object_path(G_DBUS_PROXY(device)))); - conn = g_new0(BluezConnection, 1); - conn->mpPeerAddress = g_strdup(bluez_device1_get_address(device)); - conn->mpDevice = static_cast(g_object_ref(device)); - conn->mpEndpoint = apEndpoint; - BluezConnectionInit(conn); + 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); @@ -1383,6 +1245,8 @@ static CHIP_ERROR ConnectDeviceImpl(ConnectParams * apParams) CHIP_ERROR ConnectDevice(BluezDevice1 & aDevice, BluezEndpoint * apEndpoint) { auto params = chip::Platform::New(&aDevice, apEndpoint); + VerifyOrReturnError(params != nullptr, CHIP_ERROR_NO_MEMORY); + if (PlatformMgrImpl().GLibMatterContextInvokeSync(ConnectDeviceImpl, params) != CHIP_NO_ERROR) { ChipLogError(Ble, "Failed to schedule ConnectDeviceImpl() on CHIPoBluez thread"); From 0cf96116d957d49418e03e7cb195b6c815ae029b Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Fri, 13 Oct 2023 15:11:48 +0200 Subject: [PATCH 03/16] Store const ref to connection in ConnectionDataBundle --- src/platform/Linux/bluez/BluezConnection.cpp | 31 +++++++++----------- src/platform/Linux/bluez/BluezConnection.h | 4 +-- 2 files changed, 16 insertions(+), 19 deletions(-) diff --git a/src/platform/Linux/bluez/BluezConnection.cpp b/src/platform/Linux/bluez/BluezConnection.cpp index 7c75da7281d22f..247e2be4c190a8 100644 --- a/src/platform/Linux/bluez/BluezConnection.cpp +++ b/src/platform/Linux/bluez/BluezConnection.cpp @@ -94,12 +94,11 @@ BluezConnection::~BluezConnection() g_io_channel_unref(mC2Channel.mpChannel); } -BluezConnection::ConnectionDataBundle::ConnectionDataBundle(BluezConnection * apConn, - const chip::System::PacketBufferHandle & apBuf) -{ - mpConn = apConn; - mData.reset(g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE, apBuf->Start(), apBuf->DataLength(), sizeof(uint8_t))); -} +BluezConnection::ConnectionDataBundle::ConnectionDataBundle(const BluezConnection & aConn, + const chip::System::PacketBufferHandle & aBuf) : + mConn(aConn), + mData(g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE, aBuf->Start(), aBuf->DataLength(), sizeof(uint8_t))) +{} CHIP_ERROR BluezConnection::Init() { @@ -183,26 +182,24 @@ CHIP_ERROR BluezConnection::Init() CHIP_ERROR BluezConnection::BluezC2Indicate(ConnectionDataBundle * data) { - BluezConnection * conn = nullptr; GAutoPtr error; GIOStatus status; size_t len, written; - VerifyOrExit((conn = data->mpConn) != nullptr, ChipLogError(DeviceLayer, "BluezConnection is NULL in %s", __func__)); - VerifyOrExit(conn->mpC2 != nullptr, ChipLogError(DeviceLayer, "FAIL: C2 Indicate: %s", "NULL C2")); + VerifyOrExit(data->mConn.mpC2 != nullptr, ChipLogError(DeviceLayer, "FAIL: C2 Indicate: %s", "NULL C2")); - if (bluez_gatt_characteristic1_get_notify_acquired(conn->mpC2) == TRUE) + if (bluez_gatt_characteristic1_get_notify_acquired(data->mConn.mpC2) == TRUE) { auto * buf = static_cast(g_variant_get_fixed_array(data->mData.get(), &len, sizeof(uint8_t))); VerifyOrExit(len <= static_cast(std::numeric_limits::max()), ChipLogError(DeviceLayer, "FAIL: buffer too large in %s", __func__)); - status = g_io_channel_write_chars(conn->mC2Channel.mpChannel, buf, static_cast(len), &written, + status = g_io_channel_write_chars(data->mConn.mC2Channel.mpChannel, buf, static_cast(len), &written, &MakeUniquePointerReceiver(error).Get()); VerifyOrExit(status == G_IO_STATUS_NORMAL, ChipLogError(DeviceLayer, "FAIL: C2 Indicate: %s", error->message)); } else { - bluez_gatt_characteristic1_set_value(conn->mpC2, data->mData.release()); + bluez_gatt_characteristic1_set_value(data->mConn.mpC2, data->mData.release()); } exit: @@ -214,7 +211,7 @@ CHIP_ERROR BluezConnection::SendBluezIndication(chip::System::PacketBufferHandle { ConnectionDataBundle * bundle; VerifyOrReturnError(!apBuf.IsNull(), CHIP_ERROR_INVALID_ARGUMENT, ChipLogError(DeviceLayer, "apBuf is NULL in %s", __func__)); - VerifyOrReturnError((bundle = Platform::New(this, apBuf)) != nullptr, CHIP_ERROR_NO_MEMORY); + VerifyOrReturnError((bundle = Platform::New(*this, apBuf)) != nullptr, CHIP_ERROR_NO_MEMORY); return PlatformMgrImpl().GLibMatterContextInvokeSync(BluezC2Indicate, bundle); } @@ -256,14 +253,14 @@ CHIP_ERROR BluezConnection::SendWriteRequestImpl(ConnectionDataBundle * data) GVariant * options = nullptr; GVariantBuilder optionsBuilder; - VerifyOrExit(data->mpConn->mpC1 != nullptr, ChipLogError(DeviceLayer, "C1 is NULL in %s", __func__)); + VerifyOrExit(data->mConn.mpC1 != nullptr, ChipLogError(DeviceLayer, "C1 is NULL in %s", __func__)); g_variant_builder_init(&optionsBuilder, G_VARIANT_TYPE_ARRAY); g_variant_builder_add(&optionsBuilder, "{sv}", "type", g_variant_new_string("request")); options = g_variant_builder_end(&optionsBuilder); - bluez_gatt_characteristic1_call_write_value(data->mpConn->mpC1, data->mData.release(), options, nullptr, SendWriteRequestDone, - data->mpConn); + bluez_gatt_characteristic1_call_write_value(data->mConn.mpC1, data->mData.release(), options, nullptr, SendWriteRequestDone, + const_cast(&data->mConn)); exit: Platform::Delete(data); @@ -274,7 +271,7 @@ CHIP_ERROR BluezConnection::BluezSendWriteRequest(chip::System::PacketBufferHand { ConnectionDataBundle * bundle; VerifyOrReturnError(!apBuf.IsNull(), CHIP_ERROR_INVALID_ARGUMENT, ChipLogError(DeviceLayer, "apBuf is NULL in %s", __func__)); - VerifyOrReturnError((bundle = Platform::New(this, apBuf)) != nullptr, CHIP_ERROR_NO_MEMORY); + VerifyOrReturnError((bundle = Platform::New(*this, apBuf)) != nullptr, CHIP_ERROR_NO_MEMORY); return PlatformMgrImpl().GLibMatterContextInvokeSync(SendWriteRequestImpl, bundle); } diff --git a/src/platform/Linux/bluez/BluezConnection.h b/src/platform/Linux/bluez/BluezConnection.h index 154f9f33f35eca..75e57a11158942 100644 --- a/src/platform/Linux/bluez/BluezConnection.h +++ b/src/platform/Linux/bluez/BluezConnection.h @@ -59,10 +59,10 @@ class BluezConnection private: struct ConnectionDataBundle { - ConnectionDataBundle(BluezConnection *, const chip::System::PacketBufferHandle &); + ConnectionDataBundle(const BluezConnection &, const chip::System::PacketBufferHandle &); ~ConnectionDataBundle() = default; - BluezConnection * mpConn; + const BluezConnection & mConn; GAutoPtr mData; }; From a734c16e194a81282245ad45918453872758e59a Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Tue, 10 Oct 2023 09:19:54 +0200 Subject: [PATCH 04/16] Simplify get connection MTU method --- src/platform/Linux/BLEManagerImpl.cpp | 9 ++++++--- src/platform/Linux/bluez/BluezConnection.h | 2 ++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/platform/Linux/BLEManagerImpl.cpp b/src/platform/Linux/BLEManagerImpl.cpp index a08460972f36ad..1a18c2f01178f0 100644 --- a/src/platform/Linux/BLEManagerImpl.cpp +++ b/src/platform/Linux/BLEManagerImpl.cpp @@ -344,9 +344,12 @@ void BLEManagerImpl::HandlePlatformSpecificBLEEvent(const ChipDeviceEvent * apEv uint16_t BLEManagerImpl::GetMTU(BLE_CONNECTION_OBJECT conId) const { - VerifyOrReturnValue(conId != BLE_CONNECTION_UNINITIALIZED, 0, - ChipLogError(DeviceLayer, "BLE connection is not initialized in %s", __func__)); - return conId->mMtu; + uint16_t mtu = 0; + VerifyOrExit(conId != BLE_CONNECTION_UNINITIALIZED, + ChipLogError(DeviceLayer, "BLE connection is not initialized in %s", __func__)); + mtu = conId->GetMTU(); +exit: + return mtu; } bool BLEManagerImpl::SubscribeCharacteristic(BLE_CONNECTION_OBJECT conId, const ChipBleUUID * svcId, const ChipBleUUID * charId) diff --git a/src/platform/Linux/bluez/BluezConnection.h b/src/platform/Linux/bluez/BluezConnection.h index 75e57a11158942..8999da9d4b18f8 100644 --- a/src/platform/Linux/bluez/BluezConnection.h +++ b/src/platform/Linux/bluez/BluezConnection.h @@ -49,6 +49,8 @@ class BluezConnection CHIP_ERROR SendBluezIndication(chip::System::PacketBufferHandle apBuf); CHIP_ERROR CloseBluezConnection(); + uint16_t GetMTU() const { return mMtu; } + /// Write to the CHIP RX characteristic on the remote peripheral device CHIP_ERROR BluezSendWriteRequest(chip::System::PacketBufferHandle apBuf); /// Subscribe to the CHIP TX characteristic on the remote peripheral device From f9de24e27e4e12414b7ff818079541baef95276d Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Tue, 10 Oct 2023 10:08:51 +0200 Subject: [PATCH 05/16] Add proxy method for getting device address --- src/platform/Linux/BLEManagerImpl.cpp | 4 +-- src/platform/Linux/bluez/BluezConnection.cpp | 8 +++-- src/platform/Linux/bluez/BluezConnection.h | 12 +++++-- src/platform/Linux/bluez/BluezEndpoint.cpp | 34 +++++++++++--------- 4 files changed, 34 insertions(+), 24 deletions(-) diff --git a/src/platform/Linux/BLEManagerImpl.cpp b/src/platform/Linux/BLEManagerImpl.cpp index 1a18c2f01178f0..aa8ff993bdb887 100644 --- a/src/platform/Linux/BLEManagerImpl.cpp +++ b/src/platform/Linux/BLEManagerImpl.cpp @@ -566,12 +566,12 @@ void BLEManagerImpl::HandleTXCharCCCDWrite(BLE_CONNECTION_OBJECT conId) // whether the client is enabling or disabling indications. { ChipDeviceEvent event; - event.Type = connection->mIsNotify ? DeviceEventType::kCHIPoBLESubscribe : DeviceEventType::kCHIPoBLEUnsubscribe; + event.Type = connection->IsNotifyAcquired() ? DeviceEventType::kCHIPoBLESubscribe : DeviceEventType::kCHIPoBLEUnsubscribe; event.CHIPoBLESubscribe.ConId = connection; PlatformMgr().PostEventOrDie(&event); } - ChipLogProgress(DeviceLayer, "CHIPoBLE %s received", connection->mIsNotify ? "subscribe" : "unsubscribe"); + ChipLogProgress(DeviceLayer, "CHIPoBLE %s received", connection->IsNotifyAcquired() ? "subscribe" : "unsubscribe"); exit: if (err != CHIP_NO_ERROR) diff --git a/src/platform/Linux/bluez/BluezConnection.cpp b/src/platform/Linux/bluez/BluezConnection.cpp index 247e2be4c190a8..6765311c2dae56 100644 --- a/src/platform/Linux/bluez/BluezConnection.cpp +++ b/src/platform/Linux/bluez/BluezConnection.cpp @@ -62,7 +62,6 @@ gboolean BluezIsCharOnService(BluezGattCharacteristic1 * aChar, BluezGattService BluezConnection::BluezConnection(BluezEndpoint * apEndpoint, BluezDevice1 * apDevice) : mpEndpoint(apEndpoint), mpDevice(BLUEZ_DEVICE1(g_object_ref(apDevice))) { - mpPeerAddress = g_strdup(bluez_device1_get_address(apDevice)); Init(); } @@ -76,8 +75,6 @@ BluezConnection::~BluezConnection() g_object_unref(mpC1); if (mpC2) g_object_unref(mpC2); - if (mpPeerAddress) - g_free(mpPeerAddress); if (mC1Channel.mWatchSource) { g_source_destroy(mC1Channel.mWatchSource); @@ -236,6 +233,11 @@ CHIP_ERROR BluezConnection::CloseBluezConnection() return PlatformMgrImpl().GLibMatterContextInvokeSync(BluezDisconnect, this); } +const char * BluezConnection::GetPeerAddress() const +{ + return bluez_device1_get_address(mpDevice); +} + // BluezSendWriteRequest callbacks void BluezConnection::SendWriteRequestDone(GObject * aObject, GAsyncResult * aResult, gpointer apConnection) diff --git a/src/platform/Linux/bluez/BluezConnection.h b/src/platform/Linux/bluez/BluezConnection.h index 8999da9d4b18f8..c187eeb9711231 100644 --- a/src/platform/Linux/bluez/BluezConnection.h +++ b/src/platform/Linux/bluez/BluezConnection.h @@ -49,7 +49,13 @@ class BluezConnection CHIP_ERROR SendBluezIndication(chip::System::PacketBufferHandle apBuf); CHIP_ERROR CloseBluezConnection(); + const char * GetPeerAddress() const; + uint16_t GetMTU() const { return mMtu; } + void SetMTU(uint16_t aMtu) { mMtu = aMtu; } + + bool IsNotifyAcquired() const { return mNotifyAcquired; } + void SetNotifyAcquired(bool aNotifyAcquired) { mNotifyAcquired = aNotifyAcquired; } /// Write to the CHIP RX characteristic on the remote peripheral device CHIP_ERROR BluezSendWriteRequest(chip::System::PacketBufferHandle apBuf); @@ -87,16 +93,16 @@ class BluezConnection BluezEndpoint * mpEndpoint; BluezDevice1 * mpDevice; + bool mNotifyAcquired = false; + uint16_t mMtu = 0; + public: - char * mpPeerAddress; BluezGattService1 * mpService = nullptr; BluezGattCharacteristic1 * mpC1 = nullptr; BluezGattCharacteristic1 * mpC2 = nullptr; // additional data characteristics BluezGattCharacteristic1 * mpC3 = nullptr; - bool mIsNotify = false; - uint16_t mMtu = 0; struct IOChannel mC1Channel = { 0 }; struct IOChannel mC2Channel = { 0 }; }; diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index f9e9d6b775a72a..517ca6f653dfe1 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -325,10 +325,10 @@ static gboolean BluezCharacteristicWriteFD(GIOChannel * aChannel, GIOCondition a VerifyOrExit(!(aCond & (G_IO_ERR | G_IO_NVAL)), ChipLogError(DeviceLayer, "INFO: socket error in %s", __func__)); VerifyOrExit(aCond == G_IO_IN, ChipLogError(DeviceLayer, "FAIL: error in %s", __func__)); - ChipLogDetail(DeviceLayer, "c1 %s mtu, %d", __func__, conn->mMtu); + ChipLogDetail(DeviceLayer, "c1 %s mtu, %d", __func__, conn->GetMTU()); - buf = g_new(uint8_t, conn->mMtu); - len = read(g_io_channel_unix_get_fd(aChannel), buf, conn->mMtu); + buf = g_new(uint8_t, conn->GetMTU()); + len = read(g_io_channel_unix_get_fd(aChannel), buf, conn->GetMTU()); VerifyOrExit(len > 0, ChipLogError(DeviceLayer, "FAIL: short read in %s (%zd)", __func__, len)); // Casting len to size_t is safe, since we ensured that it's not negative. @@ -368,6 +368,7 @@ static gboolean BluezCharacteristicAcquireWrite(BluezGattCharacteristic1 * aChar #endif // CHIP_ERROR_LOGGING BluezConnection * conn = nullptr; GAutoPtr option_mtu; + uint16_t mtu; BluezEndpoint * endpoint = static_cast(apEndpoint); @@ -391,9 +392,9 @@ static gboolean BluezCharacteristicAcquireWrite(BluezGattCharacteristic1 * aChar } VerifyOrReturnValue( - g_variant_lookup(aOptions, "mtu", "q", &conn->mMtu), FALSE, - ChipLogError(DeviceLayer, "FAIL: No MTU in options in %s", __func__); + g_variant_lookup(aOptions, "mtu", "q", &mtu), FALSE, ChipLogError(DeviceLayer, "FAIL: No MTU in options in %s", __func__); g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.InvalidArguments", "MTU negotiation failed")); + conn->SetMTU(mtu); channel = g_io_channel_unix_new(fds[0]); g_io_channel_set_encoding(channel, nullptr, nullptr); @@ -408,7 +409,7 @@ static gboolean BluezCharacteristicAcquireWrite(BluezGattCharacteristic1 * aChar bluez_gatt_characteristic1_set_write_acquired(aChar, TRUE); - Bluez_gatt_characteristic1_complete_acquire_write_with_fd(aInvocation, fds[1], conn->mMtu); + Bluez_gatt_characteristic1_complete_acquire_write_with_fd(aInvocation, fds[1], conn->GetMTU()); close(fds[1]); return TRUE; @@ -434,6 +435,7 @@ static gboolean BluezCharacteristicAcquireNotify(BluezGattCharacteristic1 * aCha #endif // CHIP_ERROR_LOGGING BluezConnection * conn = nullptr; GAutoPtr option_mtu; + uint16_t mtu; BluezEndpoint * endpoint = static_cast(apEndpoint); VerifyOrReturnValue(endpoint != nullptr, FALSE, ChipLogError(DeviceLayer, "endpoint is NULL in %s", __func__)); @@ -449,10 +451,10 @@ static gboolean BluezCharacteristicAcquireNotify(BluezGattCharacteristic1 * aCha conn != nullptr, FALSE, g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.Failed", "No Chipoble connection")); - VerifyOrReturnValue(g_variant_lookup(aOptions, "mtu", "q", &conn->mMtu), FALSE, { - ChipLogError(DeviceLayer, "FAIL: No MTU in options in %s", __func__); - g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.InvalidArguments", "MTU negotiation failed"); - }); + VerifyOrReturnValue( + g_variant_lookup(aOptions, "mtu", "q", &mtu), FALSE, ChipLogError(DeviceLayer, "FAIL: No MTU in options in %s", __func__); + g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.InvalidArguments", "MTU negotiation failed");); + conn->SetMTU(mtu); if (socketpair(AF_UNIX, SOCK_SEQPACKET | SOCK_NONBLOCK | SOCK_CLOEXEC, 0, fds) < 0) { @@ -478,10 +480,10 @@ static gboolean BluezCharacteristicAcquireNotify(BluezGattCharacteristic1 * aCha bluez_gatt_characteristic1_set_notify_acquired(aChar, TRUE); // same reply as for AcquireWrite - Bluez_gatt_characteristic1_complete_acquire_write_with_fd(aInvocation, fds[1], conn->mMtu); + Bluez_gatt_characteristic1_complete_acquire_write_with_fd(aInvocation, fds[1], conn->GetMTU()); close(fds[1]); - conn->mIsNotify = true; + conn->SetNotifyAcquired(true); BLEManagerImpl::HandleTXCharCCCDWrite(conn); return TRUE; @@ -517,7 +519,7 @@ static gboolean BluezCharacteristicStartNotify(BluezGattCharacteristic1 * aChar, { bluez_gatt_characteristic1_complete_start_notify(aChar, aInvocation); bluez_gatt_characteristic1_set_notifying(aChar, TRUE); - conn->mIsNotify = true; + conn->SetNotifyAcquired(true); BLEManagerImpl::HandleTXCharCCCDWrite(conn); } isSuccess = true; @@ -555,7 +557,7 @@ static gboolean BluezCharacteristicStopNotify(BluezGattCharacteristic1 * aChar, bluez_gatt_characteristic1_complete_start_notify(aChar, aInvocation); bluez_gatt_characteristic1_set_notifying(aChar, FALSE); } - conn->mIsNotify = false; + conn->SetNotifyAcquired(false); isSuccess = true; @@ -688,7 +690,7 @@ static void UpdateConnectionTable(BluezDevice1 * apDevice, BluezEndpoint & aEndp aEndpoint.mpPeerDevicePath = g_strdup(objectPath); g_hash_table_insert(aEndpoint.mpConnMap, aEndpoint.mpPeerDevicePath, connection); - ChipLogDetail(DeviceLayer, "New BLE connection: conn %p, device %s, path %s", connection, connection->mpPeerAddress, + ChipLogDetail(DeviceLayer, "New BLE connection: conn %p, device %s, path %s", connection, connection->GetPeerAddress(), aEndpoint.mpPeerDevicePath); BLEManagerImpl::HandleNewConnection(connection); @@ -735,7 +737,7 @@ static void BluezHandleNewDevice(BluezDevice1 * device, BluezEndpoint * apEndpoi apEndpoint->mpPeerDevicePath = g_strdup(g_dbus_proxy_get_object_path(G_DBUS_PROXY(device))); g_hash_table_insert(apEndpoint->mpConnMap, apEndpoint->mpPeerDevicePath, conn); - ChipLogDetail(DeviceLayer, "BLE device connected: conn %p, device %s, path %s", conn, conn->mpPeerAddress, + ChipLogDetail(DeviceLayer, "BLE device connected: conn %p, device %s, path %s", conn, conn->GetPeerAddress(), apEndpoint->mpPeerDevicePath); exit: From c29fdd4ed41b27567d3a34f853e11c9e0c66e041 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Fri, 13 Oct 2023 15:37:41 +0200 Subject: [PATCH 06/16] Allocate ConnectionDataBundle on stack --- src/platform/Linux/bluez/BluezConnection.cpp | 29 ++++++++------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/src/platform/Linux/bluez/BluezConnection.cpp b/src/platform/Linux/bluez/BluezConnection.cpp index 6765311c2dae56..abf6637958ac54 100644 --- a/src/platform/Linux/bluez/BluezConnection.cpp +++ b/src/platform/Linux/bluez/BluezConnection.cpp @@ -180,18 +180,15 @@ CHIP_ERROR BluezConnection::Init() CHIP_ERROR BluezConnection::BluezC2Indicate(ConnectionDataBundle * data) { GAutoPtr error; - GIOStatus status; size_t len, written; - VerifyOrExit(data->mConn.mpC2 != nullptr, ChipLogError(DeviceLayer, "FAIL: C2 Indicate: %s", "NULL C2")); - if (bluez_gatt_characteristic1_get_notify_acquired(data->mConn.mpC2) == TRUE) { auto * buf = static_cast(g_variant_get_fixed_array(data->mData.get(), &len, sizeof(uint8_t))); VerifyOrExit(len <= static_cast(std::numeric_limits::max()), ChipLogError(DeviceLayer, "FAIL: buffer too large in %s", __func__)); - status = g_io_channel_write_chars(data->mConn.mC2Channel.mpChannel, buf, static_cast(len), &written, - &MakeUniquePointerReceiver(error).Get()); + auto status = g_io_channel_write_chars(data->mConn.mC2Channel.mpChannel, buf, static_cast(len), &written, + &MakeUniquePointerReceiver(error).Get()); VerifyOrExit(status == G_IO_STATUS_NORMAL, ChipLogError(DeviceLayer, "FAIL: C2 Indicate: %s", error->message)); } else @@ -200,16 +197,16 @@ CHIP_ERROR BluezConnection::BluezC2Indicate(ConnectionDataBundle * data) } exit: - Platform::Delete(data); return CHIP_NO_ERROR; } CHIP_ERROR BluezConnection::SendBluezIndication(chip::System::PacketBufferHandle apBuf) { - ConnectionDataBundle * bundle; VerifyOrReturnError(!apBuf.IsNull(), CHIP_ERROR_INVALID_ARGUMENT, ChipLogError(DeviceLayer, "apBuf is NULL in %s", __func__)); - VerifyOrReturnError((bundle = Platform::New(*this, apBuf)) != nullptr, CHIP_ERROR_NO_MEMORY); - return PlatformMgrImpl().GLibMatterContextInvokeSync(BluezC2Indicate, bundle); + VerifyOrReturnError(mpC2 != nullptr, CHIP_ERROR_INTERNAL, ChipLogError(DeviceLayer, "C2 is NULL in %s", __func__)); + + ConnectionDataBundle bundle(*this, apBuf); + return PlatformMgrImpl().GLibMatterContextInvokeSync(BluezC2Indicate, &bundle); } CHIP_ERROR BluezConnection::BluezDisconnect(BluezConnection * conn) @@ -252,29 +249,25 @@ void BluezConnection::SendWriteRequestDone(GObject * aObject, GAsyncResult * aRe CHIP_ERROR BluezConnection::SendWriteRequestImpl(ConnectionDataBundle * data) { - GVariant * options = nullptr; GVariantBuilder optionsBuilder; - VerifyOrExit(data->mConn.mpC1 != nullptr, ChipLogError(DeviceLayer, "C1 is NULL in %s", __func__)); - g_variant_builder_init(&optionsBuilder, G_VARIANT_TYPE_ARRAY); g_variant_builder_add(&optionsBuilder, "{sv}", "type", g_variant_new_string("request")); - options = g_variant_builder_end(&optionsBuilder); + auto options = g_variant_builder_end(&optionsBuilder); bluez_gatt_characteristic1_call_write_value(data->mConn.mpC1, data->mData.release(), options, nullptr, SendWriteRequestDone, const_cast(&data->mConn)); -exit: - Platform::Delete(data); return CHIP_NO_ERROR; } CHIP_ERROR BluezConnection::BluezSendWriteRequest(chip::System::PacketBufferHandle apBuf) { - ConnectionDataBundle * bundle; VerifyOrReturnError(!apBuf.IsNull(), CHIP_ERROR_INVALID_ARGUMENT, ChipLogError(DeviceLayer, "apBuf is NULL in %s", __func__)); - VerifyOrReturnError((bundle = Platform::New(*this, apBuf)) != nullptr, CHIP_ERROR_NO_MEMORY); - return PlatformMgrImpl().GLibMatterContextInvokeSync(SendWriteRequestImpl, bundle); + VerifyOrReturnError(mpC1 != nullptr, CHIP_ERROR_INTERNAL, ChipLogError(DeviceLayer, "C1 is NULL in %s", __func__)); + + ConnectionDataBundle bundle(*this, apBuf); + return PlatformMgrImpl().GLibMatterContextInvokeSync(SendWriteRequestImpl, &bundle); } // BluezSubscribeCharacteristic callbacks From 698506760011b985253f8a607fa7ab6d3496e92a Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Mon, 16 Oct 2023 09:05:25 +0200 Subject: [PATCH 07/16] Add destructor to IOChannel --- src/platform/Linux/bluez/BluezConnection.cpp | 28 ++++++++------------ src/platform/Linux/bluez/BluezConnection.h | 7 +++-- 2 files changed, 16 insertions(+), 19 deletions(-) diff --git a/src/platform/Linux/bluez/BluezConnection.cpp b/src/platform/Linux/bluez/BluezConnection.cpp index abf6637958ac54..189f68d7a5718e 100644 --- a/src/platform/Linux/bluez/BluezConnection.cpp +++ b/src/platform/Linux/bluez/BluezConnection.cpp @@ -67,28 +67,26 @@ BluezConnection::BluezConnection(BluezEndpoint * apEndpoint, BluezDevice1 * apDe BluezConnection::~BluezConnection() { - if (mpDevice) - g_object_unref(mpDevice); + g_object_unref(mpDevice); if (mpService) g_object_unref(mpService); if (mpC1) g_object_unref(mpC1); if (mpC2) g_object_unref(mpC2); - if (mC1Channel.mWatchSource) +} + +BluezConnection::IOChannel::~IOChannel() +{ + if (mWatchSource != nullptr) { - g_source_destroy(mC1Channel.mWatchSource); - g_source_unref(mC1Channel.mWatchSource); + g_source_destroy(mWatchSource); + g_source_unref(mWatchSource); } - if (mC1Channel.mpChannel) - g_io_channel_unref(mC1Channel.mpChannel); - if (mC2Channel.mWatchSource) + if (mpChannel != nullptr) { - g_source_destroy(mC2Channel.mWatchSource); - g_source_unref(mC2Channel.mWatchSource); + g_io_channel_unref(mpChannel); } - if (mC2Channel.mpChannel) - g_io_channel_unref(mC2Channel.mpChannel); } BluezConnection::ConnectionDataBundle::ConnectionDataBundle(const BluezConnection & aConn, @@ -103,8 +101,6 @@ CHIP_ERROR BluezConnection::Init() GList * objects = nullptr; GList * l; - VerifyOrExit(mpEndpoint != nullptr, ChipLogError(DeviceLayer, "endpoint is NULL in %s", __func__)); - if (!mpEndpoint->mIsCentral) { mpService = BLUEZ_GATT_SERVICE1(g_object_ref(mpEndpoint->mpService)); @@ -214,9 +210,7 @@ CHIP_ERROR BluezConnection::BluezDisconnect(BluezConnection * conn) GAutoPtr error; gboolean success; - VerifyOrExit(conn->mpDevice != nullptr, ChipLogError(DeviceLayer, "FAIL: Disconnect: %s", "NULL Device")); - - ChipLogDetail(DeviceLayer, "%s peer=%s", __func__, bluez_device1_get_address(conn->mpDevice)); + ChipLogDetail(DeviceLayer, "%s peer=%s", __func__, conn->GetPeerAddress()); success = bluez_device1_call_disconnect_sync(conn->mpDevice, nullptr, &MakeUniquePointerReceiver(error).Get()); VerifyOrExit(success == TRUE, ChipLogError(DeviceLayer, "FAIL: Disconnect: %s", error->message)); diff --git a/src/platform/Linux/bluez/BluezConnection.h b/src/platform/Linux/bluez/BluezConnection.h index c187eeb9711231..e51ba80b0e1cd3 100644 --- a/src/platform/Linux/bluez/BluezConnection.h +++ b/src/platform/Linux/bluez/BluezConnection.h @@ -39,6 +39,9 @@ class BluezConnection public: struct IOChannel { + IOChannel() = default; + ~IOChannel(); + GIOChannel * mpChannel; GSource * mWatchSource; }; @@ -103,8 +106,8 @@ class BluezConnection // additional data characteristics BluezGattCharacteristic1 * mpC3 = nullptr; - struct IOChannel mC1Channel = { 0 }; - struct IOChannel mC2Channel = { 0 }; + IOChannel mC1Channel = { 0 }; + IOChannel mC2Channel = { 0 }; }; } // namespace Internal From dffd16b193b62dc6904801931ada6dd2c86fef3b Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Mon, 16 Oct 2023 09:26:30 +0200 Subject: [PATCH 08/16] Remove Bluez from public method names --- src/platform/Linux/BLEManagerImpl.cpp | 15 ++--- src/platform/Linux/bluez/BluezConnection.cpp | 70 ++++++++++---------- src/platform/Linux/bluez/BluezConnection.h | 12 ++-- 3 files changed, 48 insertions(+), 49 deletions(-) diff --git a/src/platform/Linux/BLEManagerImpl.cpp b/src/platform/Linux/BLEManagerImpl.cpp index aa8ff993bdb887..9efadbab675f01 100644 --- a/src/platform/Linux/BLEManagerImpl.cpp +++ b/src/platform/Linux/BLEManagerImpl.cpp @@ -363,8 +363,7 @@ bool BLEManagerImpl::SubscribeCharacteristic(BLE_CONNECTION_OBJECT conId, const VerifyOrExit(Ble::UUIDsMatch(charId, &ChipUUID_CHIPoBLEChar_TX), ChipLogError(DeviceLayer, "SubscribeCharacteristic() called with invalid characteristic ID")); - VerifyOrExit(conId->BluezSubscribeCharacteristic() == CHIP_NO_ERROR, - ChipLogError(DeviceLayer, "BluezSubscribeCharacteristic() failed")); + VerifyOrExit(conId->SubscribeCharacteristic() == CHIP_NO_ERROR, ChipLogError(DeviceLayer, "SubscribeCharacteristic() failed")); result = true; exit: @@ -382,8 +381,8 @@ bool BLEManagerImpl::UnsubscribeCharacteristic(BLE_CONNECTION_OBJECT conId, cons VerifyOrExit(Ble::UUIDsMatch(charId, &ChipUUID_CHIPoBLEChar_TX), ChipLogError(DeviceLayer, "UnsubscribeCharacteristic() called with invalid characteristic ID")); - VerifyOrExit(conId->BluezUnsubscribeCharacteristic() == CHIP_NO_ERROR, - ChipLogError(DeviceLayer, "BluezUnsubscribeCharacteristic() failed")); + VerifyOrExit(conId->UnsubscribeCharacteristic() == CHIP_NO_ERROR, + ChipLogError(DeviceLayer, "UnsubscribeCharacteristic() failed")); result = true; exit: @@ -398,7 +397,7 @@ bool BLEManagerImpl::CloseConnection(BLE_CONNECTION_OBJECT conId) ChipLogError(DeviceLayer, "BLE connection is not initialized in %s", __func__)); ChipLogProgress(DeviceLayer, "Closing BLE GATT connection (con %p)", conId); - VerifyOrExit(conId->CloseBluezConnection() == CHIP_NO_ERROR, ChipLogError(DeviceLayer, "CloseBluezConnection() failed")); + VerifyOrExit(conId->CloseConnection() == CHIP_NO_ERROR, ChipLogError(DeviceLayer, "CloseConnection() failed")); result = true; exit: @@ -412,8 +411,7 @@ bool BLEManagerImpl::SendIndication(BLE_CONNECTION_OBJECT conId, const ChipBleUU VerifyOrExit(conId != BLE_CONNECTION_UNINITIALIZED, ChipLogError(DeviceLayer, "BLE connection is not initialized in %s", __func__)); - VerifyOrExit(conId->SendBluezIndication(std::move(pBuf)) == CHIP_NO_ERROR, - ChipLogError(DeviceLayer, "SendBluezIndication() failed")); + VerifyOrExit(conId->SendIndication(std::move(pBuf)) == CHIP_NO_ERROR, ChipLogError(DeviceLayer, "SendIndication() failed")); result = true; exit: @@ -432,8 +430,7 @@ bool BLEManagerImpl::SendWriteRequest(BLE_CONNECTION_OBJECT conId, const Ble::Ch VerifyOrExit(Ble::UUIDsMatch(charId, &ChipUUID_CHIPoBLEChar_RX), ChipLogError(DeviceLayer, "SendWriteRequest() called with invalid characteristic ID")); - VerifyOrExit(conId->BluezSendWriteRequest(std::move(pBuf)) == CHIP_NO_ERROR, - ChipLogError(DeviceLayer, "BluezSendWriteRequest() failed")); + VerifyOrExit(conId->SendWriteRequest(std::move(pBuf)) == CHIP_NO_ERROR, ChipLogError(DeviceLayer, "SendWriteRequest() failed")); result = true; exit: diff --git a/src/platform/Linux/bluez/BluezConnection.cpp b/src/platform/Linux/bluez/BluezConnection.cpp index 189f68d7a5718e..c490326878db87 100644 --- a/src/platform/Linux/bluez/BluezConnection.cpp +++ b/src/platform/Linux/bluez/BluezConnection.cpp @@ -173,6 +173,32 @@ CHIP_ERROR BluezConnection::Init() return CHIP_NO_ERROR; } +CHIP_ERROR BluezConnection::BluezDisconnect(BluezConnection * conn) +{ + GAutoPtr error; + gboolean success; + + ChipLogDetail(DeviceLayer, "%s peer=%s", __func__, conn->GetPeerAddress()); + + success = bluez_device1_call_disconnect_sync(conn->mpDevice, nullptr, &MakeUniquePointerReceiver(error).Get()); + VerifyOrExit(success == TRUE, ChipLogError(DeviceLayer, "FAIL: Disconnect: %s", error->message)); + +exit: + return CHIP_NO_ERROR; +} + +CHIP_ERROR BluezConnection::CloseConnection() +{ + return PlatformMgrImpl().GLibMatterContextInvokeSync(BluezDisconnect, this); +} + +const char * BluezConnection::GetPeerAddress() const +{ + return bluez_device1_get_address(mpDevice); +} + +// SendIndication callbacks + CHIP_ERROR BluezConnection::BluezC2Indicate(ConnectionDataBundle * data) { GAutoPtr error; @@ -196,7 +222,7 @@ CHIP_ERROR BluezConnection::BluezC2Indicate(ConnectionDataBundle * data) return CHIP_NO_ERROR; } -CHIP_ERROR BluezConnection::SendBluezIndication(chip::System::PacketBufferHandle apBuf) +CHIP_ERROR BluezConnection::SendIndication(chip::System::PacketBufferHandle apBuf) { VerifyOrReturnError(!apBuf.IsNull(), CHIP_ERROR_INVALID_ARGUMENT, ChipLogError(DeviceLayer, "apBuf is NULL in %s", __func__)); VerifyOrReturnError(mpC2 != nullptr, CHIP_ERROR_INTERNAL, ChipLogError(DeviceLayer, "C2 is NULL in %s", __func__)); @@ -205,31 +231,7 @@ CHIP_ERROR BluezConnection::SendBluezIndication(chip::System::PacketBufferHandle return PlatformMgrImpl().GLibMatterContextInvokeSync(BluezC2Indicate, &bundle); } -CHIP_ERROR BluezConnection::BluezDisconnect(BluezConnection * conn) -{ - GAutoPtr error; - gboolean success; - - ChipLogDetail(DeviceLayer, "%s peer=%s", __func__, conn->GetPeerAddress()); - - success = bluez_device1_call_disconnect_sync(conn->mpDevice, nullptr, &MakeUniquePointerReceiver(error).Get()); - VerifyOrExit(success == TRUE, ChipLogError(DeviceLayer, "FAIL: Disconnect: %s", error->message)); - -exit: - return CHIP_NO_ERROR; -} - -CHIP_ERROR BluezConnection::CloseBluezConnection() -{ - return PlatformMgrImpl().GLibMatterContextInvokeSync(BluezDisconnect, this); -} - -const char * BluezConnection::GetPeerAddress() const -{ - return bluez_device1_get_address(mpDevice); -} - -// BluezSendWriteRequest callbacks +// SendWriteRequest callbacks void BluezConnection::SendWriteRequestDone(GObject * aObject, GAsyncResult * aResult, gpointer apConnection) { @@ -237,7 +239,7 @@ void BluezConnection::SendWriteRequestDone(GObject * aObject, GAsyncResult * aRe GAutoPtr error; gboolean success = bluez_gatt_characteristic1_call_write_value_finish(c1, aResult, &MakeUniquePointerReceiver(error).Get()); - VerifyOrReturn(success == TRUE, ChipLogError(DeviceLayer, "FAIL: BluezSendWriteRequest : %s", error->message)); + VerifyOrReturn(success == TRUE, ChipLogError(DeviceLayer, "FAIL: SendWriteRequest : %s", error->message)); BLEManagerImpl::HandleWriteComplete(static_cast(apConnection)); } @@ -255,7 +257,7 @@ CHIP_ERROR BluezConnection::SendWriteRequestImpl(ConnectionDataBundle * data) return CHIP_NO_ERROR; } -CHIP_ERROR BluezConnection::BluezSendWriteRequest(chip::System::PacketBufferHandle apBuf) +CHIP_ERROR BluezConnection::SendWriteRequest(chip::System::PacketBufferHandle apBuf) { VerifyOrReturnError(!apBuf.IsNull(), CHIP_ERROR_INVALID_ARGUMENT, ChipLogError(DeviceLayer, "apBuf is NULL in %s", __func__)); VerifyOrReturnError(mpC1 != nullptr, CHIP_ERROR_INTERNAL, ChipLogError(DeviceLayer, "C1 is NULL in %s", __func__)); @@ -264,7 +266,7 @@ CHIP_ERROR BluezConnection::BluezSendWriteRequest(chip::System::PacketBufferHand return PlatformMgrImpl().GLibMatterContextInvokeSync(SendWriteRequestImpl, &bundle); } -// BluezSubscribeCharacteristic callbacks +// SubscribeCharacteristic callbacks void BluezConnection::OnCharacteristicChanged(GDBusProxy * aInterface, GVariant * aChangedProperties, const gchar * const * aInvalidatedProps, gpointer apConnection) @@ -286,7 +288,7 @@ void BluezConnection::SubscribeCharacteristicDone(GObject * aObject, GAsyncResul GAutoPtr error; gboolean success = bluez_gatt_characteristic1_call_write_value_finish(c2, aResult, &MakeUniquePointerReceiver(error).Get()); - VerifyOrReturn(success == TRUE, ChipLogError(DeviceLayer, "FAIL: BluezSubscribeCharacteristic : %s", error->message)); + VerifyOrReturn(success == TRUE, ChipLogError(DeviceLayer, "FAIL: SubscribeCharacteristic : %s", error->message)); BLEManagerImpl::HandleSubscribeOpComplete(static_cast(apConnection), true); } @@ -305,12 +307,12 @@ CHIP_ERROR BluezConnection::SubscribeCharacteristicImpl(BluezConnection * connec return CHIP_NO_ERROR; } -CHIP_ERROR BluezConnection::BluezSubscribeCharacteristic() +CHIP_ERROR BluezConnection::SubscribeCharacteristic() { return PlatformMgrImpl().GLibMatterContextInvokeSync(SubscribeCharacteristicImpl, this); } -// BluezUnsubscribeCharacteristic callbacks +// UnsubscribeCharacteristic callbacks void BluezConnection::UnsubscribeCharacteristicDone(GObject * aObject, GAsyncResult * aResult, gpointer apConnection) { @@ -318,7 +320,7 @@ void BluezConnection::UnsubscribeCharacteristicDone(GObject * aObject, GAsyncRes GAutoPtr error; gboolean success = bluez_gatt_characteristic1_call_write_value_finish(c2, aResult, &MakeUniquePointerReceiver(error).Get()); - VerifyOrReturn(success == TRUE, ChipLogError(DeviceLayer, "FAIL: BluezUnsubscribeCharacteristic : %s", error->message)); + VerifyOrReturn(success == TRUE, ChipLogError(DeviceLayer, "FAIL: UnsubscribeCharacteristic : %s", error->message)); // Stop listening to the TX characteristic changes g_signal_handlers_disconnect_by_data(c2, apConnection); @@ -335,7 +337,7 @@ CHIP_ERROR BluezConnection::UnsubscribeCharacteristicImpl(BluezConnection * conn return CHIP_NO_ERROR; } -CHIP_ERROR BluezConnection::BluezUnsubscribeCharacteristic() +CHIP_ERROR BluezConnection::UnsubscribeCharacteristic() { return PlatformMgrImpl().GLibMatterContextInvokeSync(UnsubscribeCharacteristicImpl, this); } diff --git a/src/platform/Linux/bluez/BluezConnection.h b/src/platform/Linux/bluez/BluezConnection.h index e51ba80b0e1cd3..b4eb11f6bd7917 100644 --- a/src/platform/Linux/bluez/BluezConnection.h +++ b/src/platform/Linux/bluez/BluezConnection.h @@ -49,9 +49,6 @@ class BluezConnection BluezConnection(BluezEndpoint * apEndpoint, BluezDevice1 * apDevice); ~BluezConnection(); - CHIP_ERROR SendBluezIndication(chip::System::PacketBufferHandle apBuf); - CHIP_ERROR CloseBluezConnection(); - const char * GetPeerAddress() const; uint16_t GetMTU() const { return mMtu; } @@ -60,12 +57,15 @@ class BluezConnection bool IsNotifyAcquired() const { return mNotifyAcquired; } void SetNotifyAcquired(bool aNotifyAcquired) { mNotifyAcquired = aNotifyAcquired; } + CHIP_ERROR SendIndication(chip::System::PacketBufferHandle apBuf); /// Write to the CHIP RX characteristic on the remote peripheral device - CHIP_ERROR BluezSendWriteRequest(chip::System::PacketBufferHandle apBuf); + CHIP_ERROR SendWriteRequest(chip::System::PacketBufferHandle apBuf); /// Subscribe to the CHIP TX characteristic on the remote peripheral device - CHIP_ERROR BluezSubscribeCharacteristic(); + CHIP_ERROR SubscribeCharacteristic(); /// Unsubscribe from the CHIP TX characteristic on the remote peripheral device - CHIP_ERROR BluezUnsubscribeCharacteristic(); + CHIP_ERROR UnsubscribeCharacteristic(); + + CHIP_ERROR CloseConnection(); private: struct ConnectionDataBundle From 46bdc2044fdab85c7f47ed4f8489d42ee2af0b56 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Mon, 16 Oct 2023 09:53:00 +0200 Subject: [PATCH 09/16] Move IO channels creation to BluezConnection --- src/platform/Linux/bluez/BluezConnection.cpp | 36 ++++++++++++++- src/platform/Linux/bluez/BluezConnection.h | 35 ++++++++------ src/platform/Linux/bluez/BluezEndpoint.cpp | 48 ++++---------------- 3 files changed, 66 insertions(+), 53 deletions(-) diff --git a/src/platform/Linux/bluez/BluezConnection.cpp b/src/platform/Linux/bluez/BluezConnection.cpp index c490326878db87..5fa81445e10ca7 100644 --- a/src/platform/Linux/bluez/BluezConnection.cpp +++ b/src/platform/Linux/bluez/BluezConnection.cpp @@ -197,9 +197,41 @@ const char * BluezConnection::GetPeerAddress() const return bluez_device1_get_address(mpDevice); } +void BluezConnection::SetupWriteCallback(int aSocketFd, CharCallbackFunc aCallback) +{ + auto channel = g_io_channel_unix_new(aSocketFd); + g_io_channel_set_encoding(channel, nullptr, nullptr); + g_io_channel_set_close_on_unref(channel, TRUE); + g_io_channel_set_buffered(channel, FALSE); + + auto watchSource = g_io_create_watch(channel, static_cast(G_IO_HUP | G_IO_IN | G_IO_ERR | G_IO_NVAL)); + g_source_set_callback(watchSource, G_SOURCE_FUNC(aCallback), this, nullptr); + + mC1Channel.mpChannel = channel; + mC1Channel.mWatchSource = watchSource; + + PlatformMgrImpl().GLibMatterContextAttachSource(watchSource); +} + +void BluezConnection::SetupNotifyCallback(int aSocketFd, CharCallbackFunc aCallback) +{ + auto channel = g_io_channel_unix_new(aSocketFd); + g_io_channel_set_encoding(channel, nullptr, nullptr); + g_io_channel_set_close_on_unref(channel, TRUE); + g_io_channel_set_buffered(channel, FALSE); + + auto watchSource = g_io_create_watch(channel, static_cast(G_IO_HUP | G_IO_ERR | G_IO_NVAL)); + g_source_set_callback(watchSource, G_SOURCE_FUNC(aCallback), this, nullptr); + + mC2Channel.mpChannel = channel; + mC2Channel.mWatchSource = watchSource; + + PlatformMgrImpl().GLibMatterContextAttachSource(watchSource); +} + // SendIndication callbacks -CHIP_ERROR BluezConnection::BluezC2Indicate(ConnectionDataBundle * data) +CHIP_ERROR BluezConnection::SendIndicationImpl(ConnectionDataBundle * data) { GAutoPtr error; size_t len, written; @@ -228,7 +260,7 @@ CHIP_ERROR BluezConnection::SendIndication(chip::System::PacketBufferHandle apBu VerifyOrReturnError(mpC2 != nullptr, CHIP_ERROR_INTERNAL, ChipLogError(DeviceLayer, "C2 is NULL in %s", __func__)); ConnectionDataBundle bundle(*this, apBuf); - return PlatformMgrImpl().GLibMatterContextInvokeSync(BluezC2Indicate, &bundle); + return PlatformMgrImpl().GLibMatterContextInvokeSync(SendIndicationImpl, &bundle); } // SendWriteRequest callbacks diff --git a/src/platform/Linux/bluez/BluezConnection.h b/src/platform/Linux/bluez/BluezConnection.h index b4eb11f6bd7917..b485221fc174b1 100644 --- a/src/platform/Linux/bluez/BluezConnection.h +++ b/src/platform/Linux/bluez/BluezConnection.h @@ -37,15 +37,6 @@ struct BluezEndpoint; class BluezConnection { public: - struct IOChannel - { - IOChannel() = default; - ~IOChannel(); - - GIOChannel * mpChannel; - GSource * mWatchSource; - }; - BluezConnection(BluezEndpoint * apEndpoint, BluezDevice1 * apDevice); ~BluezConnection(); @@ -57,6 +48,14 @@ class BluezConnection bool IsNotifyAcquired() const { return mNotifyAcquired; } void SetNotifyAcquired(bool aNotifyAcquired) { mNotifyAcquired = aNotifyAcquired; } + using CharCallbackFunc = gboolean(GIOChannel *, GIOCondition, BluezConnection *); + + /// Setup callback for receiving data from the CHIP TX characteristic on the remote peripheral device + void SetupWriteCallback(int aSocketFd, CharCallbackFunc aCallback); + /// Setup callback for receiving HUP event on the notification channel + void SetupNotifyCallback(int aSocketFd, CharCallbackFunc aCallback); + + /// Send indication to the CHIP RX characteristic on the remote peripheral device CHIP_ERROR SendIndication(chip::System::PacketBufferHandle apBuf); /// Write to the CHIP RX characteristic on the remote peripheral device CHIP_ERROR SendWriteRequest(chip::System::PacketBufferHandle apBuf); @@ -68,6 +67,15 @@ class BluezConnection CHIP_ERROR CloseConnection(); private: + struct IOChannel + { + IOChannel() = default; + ~IOChannel(); + + GIOChannel * mpChannel; + GSource * mWatchSource; + }; + struct ConnectionDataBundle { ConnectionDataBundle(const BluezConnection &, const chip::System::PacketBufferHandle &); @@ -79,9 +87,10 @@ class BluezConnection CHIP_ERROR Init(); - static CHIP_ERROR BluezC2Indicate(ConnectionDataBundle * data); static CHIP_ERROR BluezDisconnect(BluezConnection * apConn); + static CHIP_ERROR SendIndicationImpl(ConnectionDataBundle * data); + static void SendWriteRequestDone(GObject * aObject, GAsyncResult * aResult, gpointer apConn); static CHIP_ERROR SendWriteRequestImpl(ConnectionDataBundle * data); @@ -99,15 +108,15 @@ class BluezConnection bool mNotifyAcquired = false; uint16_t mMtu = 0; + IOChannel mC1Channel = { 0 }; + IOChannel mC2Channel = { 0 }; + public: BluezGattService1 * mpService = nullptr; BluezGattCharacteristic1 * mpC1 = nullptr; BluezGattCharacteristic1 * mpC2 = nullptr; // additional data characteristics BluezGattCharacteristic1 * mpC3 = nullptr; - - IOChannel mC1Channel = { 0 }; - IOChannel mC2Channel = { 0 }; }; } // namespace Internal diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index 517ca6f653dfe1..0296e0fe3285ed 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -310,32 +310,28 @@ static gboolean BluezCharacteristicWriteValueError(BluezGattCharacteristic1 * aC return TRUE; } -static gboolean BluezCharacteristicWriteFD(GIOChannel * aChannel, GIOCondition aCond, gpointer apEndpoint) +static gboolean BluezCharacteristicWriteFD(GIOChannel * aChannel, GIOCondition aCond, BluezConnection * apConn) { GVariant * newVal; uint8_t * buf = nullptr; ssize_t len; bool isSuccess = false; - BluezConnection * conn = static_cast(apEndpoint); - - VerifyOrExit(conn != nullptr, ChipLogError(DeviceLayer, "No CHIP Bluez connection in %s", __func__)); - VerifyOrExit(!(aCond & G_IO_HUP), ChipLogError(DeviceLayer, "INFO: socket disconnected in %s", __func__)); VerifyOrExit(!(aCond & (G_IO_ERR | G_IO_NVAL)), ChipLogError(DeviceLayer, "INFO: socket error in %s", __func__)); VerifyOrExit(aCond == G_IO_IN, ChipLogError(DeviceLayer, "FAIL: error in %s", __func__)); - ChipLogDetail(DeviceLayer, "c1 %s mtu, %d", __func__, conn->GetMTU()); + ChipLogDetail(DeviceLayer, "c1 %s mtu, %d", __func__, apConn->GetMTU()); - buf = g_new(uint8_t, conn->GetMTU()); - len = read(g_io_channel_unix_get_fd(aChannel), buf, conn->GetMTU()); + buf = g_new(uint8_t, apConn->GetMTU()); + len = read(g_io_channel_unix_get_fd(aChannel), buf, apConn->GetMTU()); VerifyOrExit(len > 0, ChipLogError(DeviceLayer, "FAIL: short read in %s (%zd)", __func__, len)); // Casting len to size_t is safe, since we ensured that it's not negative. newVal = g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE, buf, static_cast(len), sizeof(uint8_t)); - bluez_gatt_characteristic1_set_value(conn->mpC1, newVal); - BLEManagerImpl::HandleRXCharWrite(conn, buf, static_cast(len)); + bluez_gatt_characteristic1_set_value(apConn->mpC1, newVal); + BLEManagerImpl::HandleRXCharWrite(apConn, buf, static_cast(len)); isSuccess = true; exit: @@ -352,7 +348,7 @@ static void Bluez_gatt_characteristic1_complete_acquire_write_with_fd(GDBusMetho g_object_unref(fd_list); } -static gboolean bluezCharacteristicDestroyFD(GIOChannel * aChannel, GIOCondition aCond, gpointer apEndpoint) +static gboolean bluezCharacteristicDestroyFD(GIOChannel *, GIOCondition, BluezConnection *) { return G_SOURCE_REMOVE; } @@ -361,8 +357,6 @@ static gboolean BluezCharacteristicAcquireWrite(BluezGattCharacteristic1 * aChar GVariant * aOptions, gpointer apEndpoint) { int fds[2] = { -1, -1 }; - GIOChannel * channel; - GSource * watchSource; #if CHIP_ERROR_LOGGING char * errStr; #endif // CHIP_ERROR_LOGGING @@ -396,17 +390,7 @@ static gboolean BluezCharacteristicAcquireWrite(BluezGattCharacteristic1 * aChar g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.InvalidArguments", "MTU negotiation failed")); conn->SetMTU(mtu); - channel = g_io_channel_unix_new(fds[0]); - g_io_channel_set_encoding(channel, nullptr, nullptr); - g_io_channel_set_close_on_unref(channel, TRUE); - g_io_channel_set_buffered(channel, FALSE); - conn->mC1Channel.mpChannel = channel; - - watchSource = g_io_create_watch(channel, static_cast(G_IO_HUP | G_IO_IN | G_IO_ERR | G_IO_NVAL)); - g_source_set_callback(watchSource, G_SOURCE_FUNC(BluezCharacteristicWriteFD), conn, nullptr); - PlatformMgrImpl().GLibMatterContextAttachSource(watchSource); - conn->mC1Channel.mWatchSource = watchSource; - + conn->SetupWriteCallback(fds[0], BluezCharacteristicWriteFD); bluez_gatt_characteristic1_set_write_acquired(aChar, TRUE); Bluez_gatt_characteristic1_complete_acquire_write_with_fd(aInvocation, fds[1], conn->GetMTU()); @@ -428,8 +412,6 @@ static gboolean BluezCharacteristicAcquireNotify(BluezGattCharacteristic1 * aCha GVariant * aOptions, gpointer apEndpoint) { int fds[2] = { -1, -1 }; - GIOChannel * channel; - GSource * watchSource; #if CHIP_ERROR_LOGGING char * errStr; #endif // CHIP_ERROR_LOGGING @@ -466,24 +448,14 @@ static gboolean BluezCharacteristicAcquireNotify(BluezGattCharacteristic1 * aCha return FALSE; } - channel = g_io_channel_unix_new(fds[0]); - g_io_channel_set_encoding(channel, nullptr, nullptr); - g_io_channel_set_close_on_unref(channel, TRUE); - g_io_channel_set_buffered(channel, FALSE); - conn->mC2Channel.mpChannel = channel; - - watchSource = g_io_create_watch(channel, static_cast(G_IO_HUP | G_IO_ERR | G_IO_NVAL)); - g_source_set_callback(watchSource, G_SOURCE_FUNC(bluezCharacteristicDestroyFD), conn, nullptr); - PlatformMgrImpl().GLibMatterContextAttachSource(watchSource); - conn->mC1Channel.mWatchSource = watchSource; - + conn->SetupNotifyCallback(fds[0], bluezCharacteristicDestroyFD); bluez_gatt_characteristic1_set_notify_acquired(aChar, TRUE); + conn->SetNotifyAcquired(true); // same reply as for AcquireWrite Bluez_gatt_characteristic1_complete_acquire_write_with_fd(aInvocation, fds[1], conn->GetMTU()); close(fds[1]); - conn->SetNotifyAcquired(true); BLEManagerImpl::HandleTXCharCCCDWrite(conn); return TRUE; From 282dd72e6e203b7111f1566323dba08a6beb4e38 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Mon, 16 Oct 2023 10:16:53 +0200 Subject: [PATCH 10/16] Fix notify callback setup for additional advertising --- src/platform/Linux/bluez/BluezConnection.cpp | 20 +++++++++++++++++--- src/platform/Linux/bluez/BluezConnection.h | 5 ++++- src/platform/Linux/bluez/BluezEndpoint.cpp | 9 +++++++-- 3 files changed, 28 insertions(+), 6 deletions(-) diff --git a/src/platform/Linux/bluez/BluezConnection.cpp b/src/platform/Linux/bluez/BluezConnection.cpp index 5fa81445e10ca7..5e238950907d99 100644 --- a/src/platform/Linux/bluez/BluezConnection.cpp +++ b/src/platform/Linux/bluez/BluezConnection.cpp @@ -74,6 +74,10 @@ BluezConnection::~BluezConnection() g_object_unref(mpC1); if (mpC2) g_object_unref(mpC2); +#if CHIP_ENABLE_ADDITIONAL_DATA_ADVERTISING + if (mpC3) + g_object_unref(mpC2); +#endif } BluezConnection::IOChannel::~IOChannel() @@ -213,7 +217,7 @@ void BluezConnection::SetupWriteCallback(int aSocketFd, CharCallbackFunc aCallba PlatformMgrImpl().GLibMatterContextAttachSource(watchSource); } -void BluezConnection::SetupNotifyCallback(int aSocketFd, CharCallbackFunc aCallback) +void BluezConnection::SetupNotifyCallback(int aSocketFd, CharCallbackFunc aCallback, bool aAdditionalAdvertising) { auto channel = g_io_channel_unix_new(aSocketFd); g_io_channel_set_encoding(channel, nullptr, nullptr); @@ -223,8 +227,18 @@ void BluezConnection::SetupNotifyCallback(int aSocketFd, CharCallbackFunc aCallb auto watchSource = g_io_create_watch(channel, static_cast(G_IO_HUP | G_IO_ERR | G_IO_NVAL)); g_source_set_callback(watchSource, G_SOURCE_FUNC(aCallback), this, nullptr); - mC2Channel.mpChannel = channel; - mC2Channel.mWatchSource = watchSource; +#if CHIP_ENABLE_ADDITIONAL_DATA_ADVERTISING + if (aAdditionalAdvertising) + { + mC3Channel.mpChannel = channel; + mC3Channel.mWatchSource = watchSource; + } + else +#endif + { + mC2Channel.mpChannel = channel; + mC2Channel.mWatchSource = watchSource; + } PlatformMgrImpl().GLibMatterContextAttachSource(watchSource); } diff --git a/src/platform/Linux/bluez/BluezConnection.h b/src/platform/Linux/bluez/BluezConnection.h index b485221fc174b1..693aba6a156296 100644 --- a/src/platform/Linux/bluez/BluezConnection.h +++ b/src/platform/Linux/bluez/BluezConnection.h @@ -53,7 +53,7 @@ class BluezConnection /// Setup callback for receiving data from the CHIP TX characteristic on the remote peripheral device void SetupWriteCallback(int aSocketFd, CharCallbackFunc aCallback); /// Setup callback for receiving HUP event on the notification channel - void SetupNotifyCallback(int aSocketFd, CharCallbackFunc aCallback); + void SetupNotifyCallback(int aSocketFd, CharCallbackFunc aCallback, bool aAdditionalAdvertising = false); /// Send indication to the CHIP RX characteristic on the remote peripheral device CHIP_ERROR SendIndication(chip::System::PacketBufferHandle apBuf); @@ -110,6 +110,9 @@ class BluezConnection IOChannel mC1Channel = { 0 }; IOChannel mC2Channel = { 0 }; +#if CHIP_ENABLE_ADDITIONAL_DATA_ADVERTISING + IOChannel mC3Channel = { 0 }; +#endif public: BluezGattService1 * mpService = nullptr; diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index 0296e0fe3285ed..6d770f2f2bc361 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -415,13 +415,18 @@ static gboolean BluezCharacteristicAcquireNotify(BluezGattCharacteristic1 * aCha #if CHIP_ERROR_LOGGING char * errStr; #endif // CHIP_ERROR_LOGGING - BluezConnection * conn = nullptr; + BluezConnection * conn = nullptr; + bool isAdditionalAdvertising = false; 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); +#endif + if (bluez_gatt_characteristic1_get_notifying(aChar)) { g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.NotPermitted", "Already notifying"); @@ -448,7 +453,7 @@ static gboolean BluezCharacteristicAcquireNotify(BluezGattCharacteristic1 * aCha return FALSE; } - conn->SetupNotifyCallback(fds[0], bluezCharacteristicDestroyFD); + conn->SetupNotifyCallback(fds[0], bluezCharacteristicDestroyFD, isAdditionalAdvertising); bluez_gatt_characteristic1_set_notify_acquired(aChar, TRUE); conn->SetNotifyAcquired(true); From fa5c5aca5311dd96e98305f695ac137c381b9024 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Mon, 16 Oct 2023 10:19:24 +0200 Subject: [PATCH 11/16] Fix FD leak in case of g_variant_lookup failure --- src/platform/Linux/bluez/BluezEndpoint.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index 6d770f2f2bc361..3f90e489db41d7 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -375,6 +375,11 @@ static gboolean BluezCharacteristicAcquireWrite(BluezGattCharacteristic1 * aChar ChipLogDetail(DeviceLayer, "BluezCharacteristicAcquireWrite is called, conn: %p", conn); + VerifyOrReturnValue( + g_variant_lookup(aOptions, "mtu", "q", &mtu), FALSE, ChipLogError(DeviceLayer, "FAIL: No MTU in options in %s", __func__); + g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.InvalidArguments", "MTU negotiation failed")); + conn->SetMTU(mtu); + if (socketpair(AF_UNIX, SOCK_SEQPACKET | SOCK_NONBLOCK | SOCK_CLOEXEC, 0, fds) < 0) { #if CHIP_ERROR_LOGGING @@ -385,11 +390,6 @@ static gboolean BluezCharacteristicAcquireWrite(BluezGattCharacteristic1 * aChar return FALSE; } - VerifyOrReturnValue( - g_variant_lookup(aOptions, "mtu", "q", &mtu), FALSE, ChipLogError(DeviceLayer, "FAIL: No MTU in options in %s", __func__); - g_dbus_method_invocation_return_dbus_error(aInvocation, "org.bluez.Error.InvalidArguments", "MTU negotiation failed")); - conn->SetMTU(mtu); - conn->SetupWriteCallback(fds[0], BluezCharacteristicWriteFD); bluez_gatt_characteristic1_set_write_acquired(aChar, TRUE); From f4a55114a4ac50893d46d80914233cf8d14fc7f8 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Mon, 16 Oct 2023 13:33:05 +0200 Subject: [PATCH 12/16] Move IO channel handlers to BluezConnection class --- src/platform/Linux/BLEManagerImpl.cpp | 28 ++++--------- src/platform/Linux/bluez/BluezConnection.cpp | 44 ++++++++++++++++++-- src/platform/Linux/bluez/BluezConnection.h | 25 +++++------ src/platform/Linux/bluez/BluezEndpoint.cpp | 38 +---------------- 4 files changed, 63 insertions(+), 72 deletions(-) diff --git a/src/platform/Linux/BLEManagerImpl.cpp b/src/platform/Linux/BLEManagerImpl.cpp index 9efadbab675f01..bf0f4eb2f909bd 100644 --- a/src/platform/Linux/BLEManagerImpl.cpp +++ b/src/platform/Linux/BLEManagerImpl.cpp @@ -552,30 +552,18 @@ void BLEManagerImpl::CHIPoBluez_ConnectionClosed(BLE_CONNECTION_OBJECT conId) void BLEManagerImpl::HandleTXCharCCCDWrite(BLE_CONNECTION_OBJECT conId) { - CHIP_ERROR err = CHIP_NO_ERROR; - - BluezConnection * connection = static_cast(conId); - - VerifyOrExit(connection != nullptr, ChipLogError(DeviceLayer, "Connection is NULL in HandleTXCharCCCDWrite")); - VerifyOrExit(connection->mpC2 != nullptr, ChipLogError(DeviceLayer, "C2 is NULL in HandleTXCharCCCDWrite")); + VerifyOrReturn(conId != BLE_CONNECTION_UNINITIALIZED, + ChipLogError(DeviceLayer, "BLE connection is not initialized in %s", __func__)); // Post an event to the Chip queue to process either a CHIPoBLE Subscribe or Unsubscribe based on // whether the client is enabling or disabling indications. - { - ChipDeviceEvent event; - event.Type = connection->IsNotifyAcquired() ? DeviceEventType::kCHIPoBLESubscribe : DeviceEventType::kCHIPoBLEUnsubscribe; - event.CHIPoBLESubscribe.ConId = connection; - PlatformMgr().PostEventOrDie(&event); - } - - ChipLogProgress(DeviceLayer, "CHIPoBLE %s received", connection->IsNotifyAcquired() ? "subscribe" : "unsubscribe"); + ChipDeviceEvent event; + event.Type = conId->IsNotifyAcquired() ? DeviceEventType::kCHIPoBLESubscribe : DeviceEventType::kCHIPoBLEUnsubscribe; + event.CHIPoBLESubscribe.ConId = conId; + PlatformMgr().PostEventOrDie(&event); -exit: - if (err != CHIP_NO_ERROR) - { - ChipLogError(DeviceLayer, "HandleTXCharCCCDWrite() failed: %s", ErrorStr(err)); - // TODO: fail connection - } + ChipLogProgress(DeviceLayer, "CHIPoBLE %s received", + (event.Type == DeviceEventType::kCHIPoBLESubscribe) ? "subscribe" : "unsubscribe"); } void BLEManagerImpl::HandleTXComplete(BLE_CONNECTION_OBJECT conId) diff --git a/src/platform/Linux/bluez/BluezConnection.cpp b/src/platform/Linux/bluez/BluezConnection.cpp index 5e238950907d99..bafd27be1e7bff 100644 --- a/src/platform/Linux/bluez/BluezConnection.cpp +++ b/src/platform/Linux/bluez/BluezConnection.cpp @@ -151,11 +151,13 @@ CHIP_ERROR BluezConnection::Init() { mpC2 = char1; } +#if CHIP_ENABLE_ADDITIONAL_DATA_ADVERTISING else if ((BluezIsCharOnService(char1, mpService) == TRUE) && (strcmp(bluez_gatt_characteristic1_get_uuid(char1), CHIP_PLAT_BLE_UUID_C3_STRING) == 0)) { mpC3 = char1; } +#endif else { g_object_unref(char1); @@ -201,7 +203,36 @@ const char * BluezConnection::GetPeerAddress() const return bluez_device1_get_address(mpDevice); } -void BluezConnection::SetupWriteCallback(int aSocketFd, CharCallbackFunc aCallback) +gboolean BluezConnection::WriteHandlerCallback(GIOChannel * aChannel, GIOCondition aCond, BluezConnection * apConn) +{ + uint8_t * buf = nullptr; + bool isSuccess = false; + GVariant * newVal; + ssize_t len; + + VerifyOrExit(!(aCond & G_IO_HUP), ChipLogError(DeviceLayer, "INFO: socket disconnected in %s", __func__)); + VerifyOrExit(!(aCond & (G_IO_ERR | G_IO_NVAL)), ChipLogError(DeviceLayer, "INFO: socket error in %s", __func__)); + VerifyOrExit(aCond == G_IO_IN, ChipLogError(DeviceLayer, "FAIL: error in %s", __func__)); + + ChipLogDetail(DeviceLayer, "C1 %s MTU: %d", __func__, apConn->GetMTU()); + + buf = g_new(uint8_t, apConn->GetMTU()); + len = read(g_io_channel_unix_get_fd(aChannel), buf, apConn->GetMTU()); + VerifyOrExit(len > 0, ChipLogError(DeviceLayer, "FAIL: short read in %s (%zd)", __func__, len)); + + // Casting len to size_t is safe, since we ensured that it's not negative. + newVal = g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE, buf, static_cast(len), sizeof(uint8_t)); + + bluez_gatt_characteristic1_set_value(apConn->mpC1, newVal); + BLEManagerImpl::HandleRXCharWrite(apConn, buf, static_cast(len)); + isSuccess = true; + +exit: + g_free(buf); + return isSuccess ? G_SOURCE_CONTINUE : G_SOURCE_REMOVE; +} + +void BluezConnection::SetupWriteHandler(int aSocketFd) { auto channel = g_io_channel_unix_new(aSocketFd); g_io_channel_set_encoding(channel, nullptr, nullptr); @@ -209,7 +240,7 @@ void BluezConnection::SetupWriteCallback(int aSocketFd, CharCallbackFunc aCallba g_io_channel_set_buffered(channel, FALSE); auto watchSource = g_io_create_watch(channel, static_cast(G_IO_HUP | G_IO_IN | G_IO_ERR | G_IO_NVAL)); - g_source_set_callback(watchSource, G_SOURCE_FUNC(aCallback), this, nullptr); + g_source_set_callback(watchSource, G_SOURCE_FUNC(WriteHandlerCallback), this, nullptr); mC1Channel.mpChannel = channel; mC1Channel.mWatchSource = watchSource; @@ -217,7 +248,12 @@ void BluezConnection::SetupWriteCallback(int aSocketFd, CharCallbackFunc aCallba PlatformMgrImpl().GLibMatterContextAttachSource(watchSource); } -void BluezConnection::SetupNotifyCallback(int aSocketFd, CharCallbackFunc aCallback, bool aAdditionalAdvertising) +gboolean BluezConnection::NotifyHandlerCallback(GIOChannel *, GIOCondition, BluezConnection *) +{ + return G_SOURCE_REMOVE; +} + +void BluezConnection::SetupNotifyHandler(int aSocketFd, bool aAdditionalAdvertising) { auto channel = g_io_channel_unix_new(aSocketFd); g_io_channel_set_encoding(channel, nullptr, nullptr); @@ -225,7 +261,7 @@ void BluezConnection::SetupNotifyCallback(int aSocketFd, CharCallbackFunc aCallb g_io_channel_set_buffered(channel, FALSE); auto watchSource = g_io_create_watch(channel, static_cast(G_IO_HUP | G_IO_ERR | G_IO_NVAL)); - g_source_set_callback(watchSource, G_SOURCE_FUNC(aCallback), this, nullptr); + g_source_set_callback(watchSource, G_SOURCE_FUNC(NotifyHandlerCallback), this, nullptr); #if CHIP_ENABLE_ADDITIONAL_DATA_ADVERTISING if (aAdditionalAdvertising) diff --git a/src/platform/Linux/bluez/BluezConnection.h b/src/platform/Linux/bluez/BluezConnection.h index 693aba6a156296..a3d4e04de0c896 100644 --- a/src/platform/Linux/bluez/BluezConnection.h +++ b/src/platform/Linux/bluez/BluezConnection.h @@ -48,12 +48,10 @@ class BluezConnection bool IsNotifyAcquired() const { return mNotifyAcquired; } void SetNotifyAcquired(bool aNotifyAcquired) { mNotifyAcquired = aNotifyAcquired; } - using CharCallbackFunc = gboolean(GIOChannel *, GIOCondition, BluezConnection *); - /// Setup callback for receiving data from the CHIP TX characteristic on the remote peripheral device - void SetupWriteCallback(int aSocketFd, CharCallbackFunc aCallback); + void SetupWriteHandler(int aSocketFd); /// Setup callback for receiving HUP event on the notification channel - void SetupNotifyCallback(int aSocketFd, CharCallbackFunc aCallback, bool aAdditionalAdvertising = false); + void SetupNotifyHandler(int aSocketFd, bool aAdditionalAdvertising = false); /// Send indication to the CHIP RX characteristic on the remote peripheral device CHIP_ERROR SendIndication(chip::System::PacketBufferHandle apBuf); @@ -89,6 +87,9 @@ class BluezConnection static CHIP_ERROR BluezDisconnect(BluezConnection * apConn); + static gboolean WriteHandlerCallback(GIOChannel * aChannel, GIOCondition aCond, BluezConnection * apConn); + static gboolean NotifyHandlerCallback(GIOChannel * aChannel, GIOCondition aCond, BluezConnection * apConn); + static CHIP_ERROR SendIndicationImpl(ConnectionDataBundle * data); static void SendWriteRequestDone(GObject * aObject, GAsyncResult * aResult, gpointer apConn); @@ -108,18 +109,18 @@ class BluezConnection bool mNotifyAcquired = false; uint16_t mMtu = 0; - IOChannel mC1Channel = { 0 }; - IOChannel mC2Channel = { 0 }; -#if CHIP_ENABLE_ADDITIONAL_DATA_ADVERTISING - IOChannel mC3Channel = { 0 }; -#endif + BluezGattService1 * mpService = nullptr; -public: - BluezGattService1 * mpService = nullptr; BluezGattCharacteristic1 * mpC1 = nullptr; + IOChannel mC1Channel = { 0 }; + BluezGattCharacteristic1 * mpC2 = nullptr; - // additional data characteristics + IOChannel mC2Channel = { 0 }; + +#if CHIP_ENABLE_ADDITIONAL_DATA_ADVERTISING BluezGattCharacteristic1 * mpC3 = nullptr; + IOChannel mC3Channel = { 0 }; +#endif }; } // namespace Internal diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index 3f90e489db41d7..b4f519ced3ce9d 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -310,35 +310,6 @@ static gboolean BluezCharacteristicWriteValueError(BluezGattCharacteristic1 * aC return TRUE; } -static gboolean BluezCharacteristicWriteFD(GIOChannel * aChannel, GIOCondition aCond, BluezConnection * apConn) -{ - GVariant * newVal; - uint8_t * buf = nullptr; - ssize_t len; - bool isSuccess = false; - - VerifyOrExit(!(aCond & G_IO_HUP), ChipLogError(DeviceLayer, "INFO: socket disconnected in %s", __func__)); - VerifyOrExit(!(aCond & (G_IO_ERR | G_IO_NVAL)), ChipLogError(DeviceLayer, "INFO: socket error in %s", __func__)); - VerifyOrExit(aCond == G_IO_IN, ChipLogError(DeviceLayer, "FAIL: error in %s", __func__)); - - ChipLogDetail(DeviceLayer, "c1 %s mtu, %d", __func__, apConn->GetMTU()); - - buf = g_new(uint8_t, apConn->GetMTU()); - len = read(g_io_channel_unix_get_fd(aChannel), buf, apConn->GetMTU()); - VerifyOrExit(len > 0, ChipLogError(DeviceLayer, "FAIL: short read in %s (%zd)", __func__, len)); - - // Casting len to size_t is safe, since we ensured that it's not negative. - newVal = g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE, buf, static_cast(len), sizeof(uint8_t)); - - bluez_gatt_characteristic1_set_value(apConn->mpC1, newVal); - BLEManagerImpl::HandleRXCharWrite(apConn, buf, static_cast(len)); - isSuccess = true; - -exit: - g_free(buf); - return isSuccess ? G_SOURCE_CONTINUE : G_SOURCE_REMOVE; -} - static void Bluez_gatt_characteristic1_complete_acquire_write_with_fd(GDBusMethodInvocation * invocation, int fd, guint16 mtu) { GUnixFDList * fd_list = g_unix_fd_list_new(); @@ -348,11 +319,6 @@ static void Bluez_gatt_characteristic1_complete_acquire_write_with_fd(GDBusMetho g_object_unref(fd_list); } -static gboolean bluezCharacteristicDestroyFD(GIOChannel *, GIOCondition, BluezConnection *) -{ - return G_SOURCE_REMOVE; -} - static gboolean BluezCharacteristicAcquireWrite(BluezGattCharacteristic1 * aChar, GDBusMethodInvocation * aInvocation, GVariant * aOptions, gpointer apEndpoint) { @@ -390,7 +356,7 @@ static gboolean BluezCharacteristicAcquireWrite(BluezGattCharacteristic1 * aChar return FALSE; } - conn->SetupWriteCallback(fds[0], BluezCharacteristicWriteFD); + conn->SetupWriteHandler(fds[0]); bluez_gatt_characteristic1_set_write_acquired(aChar, TRUE); Bluez_gatt_characteristic1_complete_acquire_write_with_fd(aInvocation, fds[1], conn->GetMTU()); @@ -453,7 +419,7 @@ static gboolean BluezCharacteristicAcquireNotify(BluezGattCharacteristic1 * aCha return FALSE; } - conn->SetupNotifyCallback(fds[0], bluezCharacteristicDestroyFD, isAdditionalAdvertising); + conn->SetupNotifyHandler(fds[0], isAdditionalAdvertising); bluez_gatt_characteristic1_set_notify_acquired(aChar, TRUE); conn->SetNotifyAcquired(true); From c5522423e9e8ac93cc191162e53c4cf7ee6a05fb Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Mon, 16 Oct 2023 14:06:36 +0200 Subject: [PATCH 13/16] Manage IOChannel members with GAutoPtr --- src/platform/GLibTypeDeleter.h | 11 +++++++++ src/platform/Linux/bluez/BluezConnection.cpp | 24 ++++++++------------ src/platform/Linux/bluez/BluezConnection.h | 4 ++-- 3 files changed, 22 insertions(+), 17 deletions(-) diff --git a/src/platform/GLibTypeDeleter.h b/src/platform/GLibTypeDeleter.h index 565403566f60a8..2a9269a97e851b 100644 --- a/src/platform/GLibTypeDeleter.h +++ b/src/platform/GLibTypeDeleter.h @@ -60,6 +60,11 @@ struct GErrorDeleter void operator()(GError * object) { g_error_free(object); } }; +struct GIOChannelDeleter +{ + void operator()(GIOChannel * object) { g_io_channel_unref(object); } +}; + struct GVariantDeleter { void operator()(GVariant * object) { g_variant_unref(object); } @@ -110,6 +115,12 @@ struct GAutoPtrDeleter using deleter = GErrorDeleter; }; +template <> +struct GAutoPtrDeleter +{ + using deleter = GIOChannelDeleter; +}; + template <> struct GAutoPtrDeleter { diff --git a/src/platform/Linux/bluez/BluezConnection.cpp b/src/platform/Linux/bluez/BluezConnection.cpp index bafd27be1e7bff..86c267b42e4739 100644 --- a/src/platform/Linux/bluez/BluezConnection.cpp +++ b/src/platform/Linux/bluez/BluezConnection.cpp @@ -83,14 +83,8 @@ BluezConnection::~BluezConnection() BluezConnection::IOChannel::~IOChannel() { if (mWatchSource != nullptr) - { - g_source_destroy(mWatchSource); - g_source_unref(mWatchSource); - } - if (mpChannel != nullptr) - { - g_io_channel_unref(mpChannel); - } + // Make sure the source is detached before destroying the channel. + g_source_destroy(mWatchSource.get()); } BluezConnection::ConnectionDataBundle::ConnectionDataBundle(const BluezConnection & aConn, @@ -242,8 +236,8 @@ void BluezConnection::SetupWriteHandler(int aSocketFd) auto watchSource = g_io_create_watch(channel, static_cast(G_IO_HUP | G_IO_IN | G_IO_ERR | G_IO_NVAL)); g_source_set_callback(watchSource, G_SOURCE_FUNC(WriteHandlerCallback), this, nullptr); - mC1Channel.mpChannel = channel; - mC1Channel.mWatchSource = watchSource; + mC1Channel.mChannel.reset(channel); + mC1Channel.mWatchSource.reset(watchSource); PlatformMgrImpl().GLibMatterContextAttachSource(watchSource); } @@ -266,14 +260,14 @@ void BluezConnection::SetupNotifyHandler(int aSocketFd, bool aAdditionalAdvertis #if CHIP_ENABLE_ADDITIONAL_DATA_ADVERTISING if (aAdditionalAdvertising) { - mC3Channel.mpChannel = channel; - mC3Channel.mWatchSource = watchSource; + mC3Channel.mChannel.reset(channel); + mC3Channel.mWatchSource.reset(watchSource); } else #endif { - mC2Channel.mpChannel = channel; - mC2Channel.mWatchSource = watchSource; + mC2Channel.mChannel.reset(channel); + mC2Channel.mWatchSource.reset(watchSource); } PlatformMgrImpl().GLibMatterContextAttachSource(watchSource); @@ -291,7 +285,7 @@ CHIP_ERROR BluezConnection::SendIndicationImpl(ConnectionDataBundle * data) auto * buf = static_cast(g_variant_get_fixed_array(data->mData.get(), &len, sizeof(uint8_t))); VerifyOrExit(len <= static_cast(std::numeric_limits::max()), ChipLogError(DeviceLayer, "FAIL: buffer too large in %s", __func__)); - auto status = g_io_channel_write_chars(data->mConn.mC2Channel.mpChannel, buf, static_cast(len), &written, + auto status = g_io_channel_write_chars(data->mConn.mC2Channel.mChannel.get(), buf, static_cast(len), &written, &MakeUniquePointerReceiver(error).Get()); VerifyOrExit(status == G_IO_STATUS_NORMAL, ChipLogError(DeviceLayer, "FAIL: C2 Indicate: %s", error->message)); } diff --git a/src/platform/Linux/bluez/BluezConnection.h b/src/platform/Linux/bluez/BluezConnection.h index a3d4e04de0c896..2d7762840dfb6d 100644 --- a/src/platform/Linux/bluez/BluezConnection.h +++ b/src/platform/Linux/bluez/BluezConnection.h @@ -70,8 +70,8 @@ class BluezConnection IOChannel() = default; ~IOChannel(); - GIOChannel * mpChannel; - GSource * mWatchSource; + GAutoPtr mChannel; + GAutoPtr mWatchSource; }; struct ConnectionDataBundle From 0c1373d8bcaf3de7c288f46444a40b793dfd1877 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Mon, 16 Oct 2023 15:05:44 +0200 Subject: [PATCH 14/16] Use on-stack array for reading characteristic --- src/platform/Linux/bluez/BluezConnection.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/platform/Linux/bluez/BluezConnection.cpp b/src/platform/Linux/bluez/BluezConnection.cpp index 86c267b42e4739..2d4286a4fe6069 100644 --- a/src/platform/Linux/bluez/BluezConnection.cpp +++ b/src/platform/Linux/bluez/BluezConnection.cpp @@ -199,7 +199,7 @@ const char * BluezConnection::GetPeerAddress() const gboolean BluezConnection::WriteHandlerCallback(GIOChannel * aChannel, GIOCondition aCond, BluezConnection * apConn) { - uint8_t * buf = nullptr; + uint8_t buf[512 /* characteristic max size per BLE specification */]; bool isSuccess = false; GVariant * newVal; ssize_t len; @@ -210,8 +210,7 @@ gboolean BluezConnection::WriteHandlerCallback(GIOChannel * aChannel, GIOConditi ChipLogDetail(DeviceLayer, "C1 %s MTU: %d", __func__, apConn->GetMTU()); - buf = g_new(uint8_t, apConn->GetMTU()); - len = read(g_io_channel_unix_get_fd(aChannel), buf, apConn->GetMTU()); + len = read(g_io_channel_unix_get_fd(aChannel), buf, sizeof(buf)); VerifyOrExit(len > 0, ChipLogError(DeviceLayer, "FAIL: short read in %s (%zd)", __func__, len)); // Casting len to size_t is safe, since we ensured that it's not negative. @@ -222,7 +221,6 @@ gboolean BluezConnection::WriteHandlerCallback(GIOChannel * aChannel, GIOConditi isSuccess = true; exit: - g_free(buf); return isSuccess ? G_SOURCE_CONTINUE : G_SOURCE_REMOVE; } From 1c2b99e0e2e5d9470edb636539f7bd67ffed0761 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Mon, 16 Oct 2023 15:11:30 +0200 Subject: [PATCH 15/16] Document FD closing behavior by setup methods --- src/platform/Linux/bluez/BluezConnection.h | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/platform/Linux/bluez/BluezConnection.h b/src/platform/Linux/bluez/BluezConnection.h index 2d7762840dfb6d..3a64ac931220a8 100644 --- a/src/platform/Linux/bluez/BluezConnection.h +++ b/src/platform/Linux/bluez/BluezConnection.h @@ -48,9 +48,21 @@ class BluezConnection bool IsNotifyAcquired() const { return mNotifyAcquired; } void SetNotifyAcquired(bool aNotifyAcquired) { mNotifyAcquired = aNotifyAcquired; } - /// Setup callback for receiving data from the CHIP TX characteristic on the remote peripheral device + /** + * @brief Setup callback for receiving data from the CHIP TX characteristic on + * the remote peripheral device. + * + * @note This function takes the ownership of the passed file descriptor and + * will close it when the connection is closed. + */ void SetupWriteHandler(int aSocketFd); - /// Setup callback for receiving HUP event on the notification channel + + /** + * @brief Setup callback for receiving HUP event on the notification channel. + * + * @note This function takes the ownership of the passed file descriptor and + * will close it when the connection is closed. + */ void SetupNotifyHandler(int aSocketFd, bool aAdditionalAdvertising = false); /// Send indication to the CHIP RX characteristic on the remote peripheral device @@ -113,10 +125,8 @@ class BluezConnection BluezGattCharacteristic1 * mpC1 = nullptr; IOChannel mC1Channel = { 0 }; - BluezGattCharacteristic1 * mpC2 = nullptr; IOChannel mC2Channel = { 0 }; - #if CHIP_ENABLE_ADDITIONAL_DATA_ADVERTISING BluezGattCharacteristic1 * mpC3 = nullptr; IOChannel mC3Channel = { 0 }; From 7fd333c9af2d4854ef8fccbe7692ae06e5fdbf1f Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Mon, 16 Oct 2023 21:09:15 +0200 Subject: [PATCH 16/16] Fix GSource deleter --- src/platform/GLibTypeDeleter.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/platform/GLibTypeDeleter.h b/src/platform/GLibTypeDeleter.h index 2a9269a97e851b..93ff40f61c111d 100644 --- a/src/platform/GLibTypeDeleter.h +++ b/src/platform/GLibTypeDeleter.h @@ -65,6 +65,11 @@ struct GIOChannelDeleter void operator()(GIOChannel * object) { g_io_channel_unref(object); } }; +struct GSourceDeleter +{ + void operator()(GSource * object) { g_source_unref(object); } +}; + struct GVariantDeleter { void operator()(GVariant * object) { g_variant_unref(object); } @@ -124,7 +129,7 @@ struct GAutoPtrDeleter template <> struct GAutoPtrDeleter { - using deleter = GObjectDeleter; + using deleter = GSourceDeleter; }; template <>