From 27dfe10689aad1b70c533d0cefb999d10b38d9ed Mon Sep 17 00:00:00 2001 From: geeksville Date: Sun, 7 Jul 2024 04:50:47 -0700 Subject: [PATCH] Fix BLE logging on nrf52 (#4244) * allow ble logrecords to be fetched either by NOTIFY or INDICATE ble types This allows 'lossless' log reading. If client has requested INDICATE (rather than NOTIFY) each log record emitted via log() will have to fetched by the client device before the meshtastic node can continue. * Fix serious problem with nrf52 BLE logging. When doing notifies of LogRecords it is important to use the binary write routines - writing using the 'string' write won't work. Because protobufs can contain \0 nuls inside of them which if being parsed as a string will cause only a portion of the protobuf to be sent. I noticed this because some log messages were not getting through. --------- Co-authored-by: Ben Meadors --- src/RedirectablePrint.cpp | 2 +- src/platform/nrf52/NRF52Bluetooth.cpp | 22 +++++++++++++++------- src/platform/nrf52/NRF52Bluetooth.h | 2 +- 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/src/RedirectablePrint.cpp b/src/RedirectablePrint.cpp index 01e5a34a7..9c3dcdc98 100644 --- a/src/RedirectablePrint.cpp +++ b/src/RedirectablePrint.cpp @@ -206,7 +206,7 @@ void RedirectablePrint::log_to_ble(const char *logLevel, const char *format, va_ #ifdef ARCH_ESP32 nimbleBluetooth->sendLog(buffer, size); #elif defined(ARCH_NRF52) - nrf52Bluetooth->sendLog(reinterpret_cast(buffer)); + nrf52Bluetooth->sendLog(buffer, size); #endif delete[] message; delete[] buffer; diff --git a/src/platform/nrf52/NRF52Bluetooth.cpp b/src/platform/nrf52/NRF52Bluetooth.cpp index 56d7ed167..57035a7c3 100644 --- a/src/platform/nrf52/NRF52Bluetooth.cpp +++ b/src/platform/nrf52/NRF52Bluetooth.cpp @@ -74,11 +74,16 @@ void onCccd(uint16_t conn_hdl, BLECharacteristic *chr, uint16_t cccd_value) LOG_INFO("CCCD Updated: %u\n", cccd_value); // Check the characteristic this CCCD update is associated with in case // this handler is used for multiple CCCD records. + + // According to the GATT spec: cccd value = 0x0001 means notifications are enabled + // and cccd value = 0x0002 means indications are enabled + if (chr->uuid == fromNum.uuid || chr->uuid == logRadio.uuid) { - if (chr->notifyEnabled(conn_hdl)) { - LOG_INFO("fromNum 'Notify' enabled\n"); + auto result = cccd_value == 2 ? chr->indicateEnabled(conn_hdl) : chr->notifyEnabled(conn_hdl); + if (result) { + LOG_INFO("Notify/Indicate enabled\n"); } else { - LOG_INFO("fromNum 'Notify' disabled\n"); + LOG_INFO("Notify/Indicate disabled\n"); } } } @@ -176,7 +181,7 @@ void setupMeshService(void) toRadio.setWriteCallback(onToRadioWrite, false); toRadio.begin(); - logRadio.setProperties(CHR_PROPS_NOTIFY | CHR_PROPS_READ); + logRadio.setProperties(CHR_PROPS_INDICATE | CHR_PROPS_NOTIFY | CHR_PROPS_READ); logRadio.setPermission(secMode, SECMODE_NO_ACCESS); logRadio.setMaxLen(512); logRadio.setCccdWriteCallback(onCccd); @@ -334,9 +339,12 @@ void NRF52Bluetooth::onPairingCompleted(uint16_t conn_handle, uint8_t auth_statu screen->endAlert(); } -void NRF52Bluetooth::sendLog(const char *logMessage) +void NRF52Bluetooth::sendLog(const uint8_t *logMessage, size_t length) { - if (!isConnected() || strlen(logMessage) > 512) + if (!isConnected() || length > 512) return; - logRadio.notify(logMessage); + if (logRadio.indicateEnabled()) + logRadio.indicate(logMessage, (uint16_t)length); + else + logRadio.notify(logMessage, (uint16_t)length); } \ No newline at end of file diff --git a/src/platform/nrf52/NRF52Bluetooth.h b/src/platform/nrf52/NRF52Bluetooth.h index 0d621dda2..2229163f8 100644 --- a/src/platform/nrf52/NRF52Bluetooth.h +++ b/src/platform/nrf52/NRF52Bluetooth.h @@ -13,7 +13,7 @@ class NRF52Bluetooth : BluetoothApi void clearBonds(); bool isConnected(); int getRssi(); - void sendLog(const char *logMessage); + void sendLog(const uint8_t *logMessage, size_t length); private: static void onConnectionSecured(uint16_t conn_handle);