Fix for Issue #283: Invalid Weave BLE scan response from nRF5 Device Layer

Jay Logue · Jay logue · commit eec889d8a80c · 2019-07-12T14:54:34.000-07:00
-- Corrected a mistake in the encoding of the Weave BLE device identification block that
gets sent as BLE service advertisement data by the nRF52840.  The previous version of the
code omitted two fields, which caused considerable heartburn for the Nest mobile
application.

-- Corrected another minor mistake in the encoding of the device identification block on the
ESP32 which resulted in the wrong data block type being sent.  This bug didn’t seem to
bother the mobile application.

-- Created a common data structure for encoding and decoding a Weave BLE device
identification block and refactored the nRF5 and ESP32 code bases to use this structure.
diff --git a/src/adaptations/device-layer/ESP32/BLEManagerImpl.cpp b/src/adaptations/device-layer/ESP32/BLEManagerImpl.cpp
@@ -24,6 +24,7 @@
 
 #include <Weave/DeviceLayer/internal/WeaveDeviceLayerInternal.h>
 #include <Weave/DeviceLayer/internal/BLEManager.h>
+#include <BleLayer/WeaveBleServiceData.h>
 #include <new>
 
 #if WEAVE_DEVICE_CONFIG_ENABLE_WOBLE
@@ -45,17 +46,11 @@ namespace Internal {
 
 namespace {
 
-struct WeaveServiceData
+
+struct ESP32WeaveServiceData
 {
     uint8_t ServiceUUID[2];
-    uint8_t DataBlockLen;
-    uint8_t DataBlockType;
-    uint8_t DataBlockMajorVersion;
-    uint8_t DataBlockMinorVersion;
-    uint8_t DeviceVendorId[2];
-    uint8_t DeviceProductId[2];
-    uint8_t DeviceId[8];
-    uint8_t PairingStatus;
+    WeaveBLEDeviceIdentificationInfo DeviceIdInfo;
 };
 
 const uint16_t WoBLEAppId = 0x235A;
@@ -562,7 +557,7 @@ WEAVE_ERROR BLEManagerImpl::ConfigureAdvertisingData(void)
 {
     WEAVE_ERROR err;
     esp_ble_adv_data_t advertData;
-    WeaveServiceData weaveServiceData;
+    ESP32WeaveServiceData weaveServiceData;
 
     // If a custom device name has not been specified, generate a Nest-standard name based on the
     // bottom digits of the Weave device id.
@@ -604,16 +599,11 @@ WEAVE_ERROR BLEManagerImpl::ConfigureAdvertisingData(void)
         ExitNow();
     }
 
-    // Construct the Weave Service Data to be sent in the scan response packet.
+    // Construct the Weave BLE Service Data to be sent in the scan response packet.  On the ESP32,
+    // the buffer given to esp_ble_gap_config_adv_data() must begin with the Weave BLE service UUID.
     memcpy(weaveServiceData.ServiceUUID, ShortUUID_WoBLEService, sizeof(weaveServiceData.ServiceUUID));
-    weaveServiceData.DataBlockLen = 16;
-    weaveServiceData.DataBlockType = 0x16;
-    weaveServiceData.DataBlockMajorVersion = 0;
-    weaveServiceData.DataBlockMinorVersion = 1;
-    Encoding::LittleEndian::Put16(weaveServiceData.DeviceVendorId, (uint16_t)WEAVE_DEVICE_CONFIG_DEVICE_VENDOR_ID);
-    Encoding::LittleEndian::Put16(weaveServiceData.DeviceProductId, (uint16_t)WEAVE_DEVICE_CONFIG_DEVICE_PRODUCT_ID);
-    Encoding::LittleEndian::Put64(weaveServiceData.DeviceId, FabricState.LocalNodeId);
-    weaveServiceData.PairingStatus = ConfigurationMgr().IsPairedToAccount() ? 1 : 0;
+    err = ConfigurationMgr().GetBLEDeviceIdentificationInfo(weaveServiceData.DeviceIdInfo);
+    SuccessOrExit(err);
 
     // Configure the contents of the scan response packet.
     memset(&advertData, 0, sizeof(advertData));
diff --git a/src/adaptations/device-layer/include/Weave/DeviceLayer/ConfigurationManager.h b/src/adaptations/device-layer/include/Weave/DeviceLayer/ConfigurationManager.h
@@ -28,6 +28,12 @@
 #include <Weave/Profiles/device-description/DeviceDescription.h>
 #include <Weave/Profiles/network-provisioning/NetworkProvisioning.h>
 
+namespace nl {
+namespace Ble {
+struct WeaveBLEDeviceIdentificationInfo;
+}
+}
+
 namespace nl {
 namespace Weave {
 namespace DeviceLayer {
@@ -98,6 +104,8 @@ class ConfigurationManager
 
     WEAVE_ERROR GetWiFiAPSSID(char * buf, size_t bufSize);
 
+    WEAVE_ERROR GetBLEDeviceIdentificationInfo(Ble::WeaveBLEDeviceIdentificationInfo & deviceIdInfo);
+
     bool IsServiceProvisioned();
     bool IsPairedToAccount();
     bool IsMemberOfFabric();
@@ -362,6 +370,11 @@ inline WEAVE_ERROR ConfigurationManager::GetWiFiAPSSID(char * buf, size_t bufSiz
     return static_cast<ImplClass*>(this)->_GetWiFiAPSSID(buf, bufSize);
 }
 
+inline WEAVE_ERROR ConfigurationManager::GetBLEDeviceIdentificationInfo(Ble::WeaveBLEDeviceIdentificationInfo & deviceIdInfo)
+{
+    return static_cast<ImplClass*>(this)->_GetBLEDeviceIdentificationInfo(deviceIdInfo);
+}
+
 inline bool ConfigurationManager::IsServiceProvisioned()
 {
     return static_cast<ImplClass*>(this)->_IsServiceProvisioned();
diff --git a/src/adaptations/device-layer/include/Weave/DeviceLayer/internal/GenericConfigurationManagerImpl.h b/src/adaptations/device-layer/include/Weave/DeviceLayer/internal/GenericConfigurationManagerImpl.h
@@ -85,6 +85,7 @@ class GenericConfigurationManagerImpl
     WEAVE_ERROR _GetDeviceDescriptorTLV(uint8_t * buf, size_t bufSize, size_t & encodedLen);
     WEAVE_ERROR _GetQRCodeString(char * buf, size_t bufSize);
     WEAVE_ERROR _GetWiFiAPSSID(char * buf, size_t bufSize);
+    WEAVE_ERROR _GetBLEDeviceIdentificationInfo(Ble::WeaveBLEDeviceIdentificationInfo & deviceIdInfo);
     bool _IsServiceProvisioned();
     bool _IsMemberOfFabric();
     bool _IsPairedToAccount();
diff --git a/src/adaptations/device-layer/include/Weave/DeviceLayer/internal/GenericConfigurationManagerImpl.ipp b/src/adaptations/device-layer/include/Weave/DeviceLayer/internal/GenericConfigurationManagerImpl.ipp
@@ -27,6 +27,7 @@
 
 #include <Weave/DeviceLayer/internal/WeaveDeviceLayerInternal.h>
 #include <Weave/DeviceLayer/internal/GenericConfigurationManagerImpl.h>
+#include <BleLayer/WeaveBleServiceData.h>
 
 
 namespace nl {
@@ -615,6 +616,32 @@ exit:
     return err;
 }
 
+template<class ImplClass>
+WEAVE_ERROR GenericConfigurationManagerImpl<ImplClass>::_GetBLEDeviceIdentificationInfo(Ble::WeaveBLEDeviceIdentificationInfo & deviceIdInfo)
+{
+    WEAVE_ERROR err;
+    uint16_t id;
+
+    deviceIdInfo.Init();
+
+    err = Impl()->_GetVendorId(id);
+    SuccessOrExit(err);
+    deviceIdInfo.SetVendorId(id);
+
+    err = Impl()->_GetProductId(id);
+    SuccessOrExit(err);
+    deviceIdInfo.SetProductId(id);
+
+    deviceIdInfo.SetDeviceId(FabricState.LocalNodeId);
+
+    deviceIdInfo.PairingStatus = Impl()->_IsPairedToAccount()
+        ? Ble::WeaveBLEDeviceIdentificationInfo::kPairingStatus_Paired
+        : Ble::WeaveBLEDeviceIdentificationInfo::kPairingStatus_Unpaired;
+
+exit:
+    return err;
+}
+
 template<class ImplClass>
 bool GenericConfigurationManagerImpl<ImplClass>::_IsServiceProvisioned()
 {
diff --git a/src/adaptations/device-layer/nRF5/BLEManagerImpl.cpp b/src/adaptations/device-layer/nRF5/BLEManagerImpl.cpp
@@ -24,6 +24,7 @@
 
 #include <Weave/DeviceLayer/internal/WeaveDeviceLayerInternal.h>
 #include <Weave/DeviceLayer/internal/BLEManager.h>
+#include <BleLayer/WeaveBleServiceData.h>
 #include <new>
 
 #if WEAVE_DEVICE_CONFIG_ENABLE_WOBLE
@@ -49,16 +50,6 @@ namespace Internal {
 
 namespace {
 
-struct WeaveServiceData
-{
-    uint8_t MajorVersion;
-    uint8_t MinorVersion;
-    uint8_t DeviceVendorId[2];
-    uint8_t DeviceProductId[2];
-    uint8_t DeviceId[8];
-    uint8_t PairingStatus;
-};
-
 const uint16_t UUID16_WoBLEService = 0xFEAF;
 const ble_uuid_t UUID_WoBLEService = { UUID16_WoBLEService, BLE_UUID_TYPE_BLE };
 
@@ -519,7 +510,7 @@ WEAVE_ERROR BLEManagerImpl::EncodeAdvertisingData(ble_gap_adv_data_t & gapAdvDat
     WEAVE_ERROR err = WEAVE_NO_ERROR;
     ble_advdata_t advData;
     ble_advdata_service_data_t serviceData;
-    WeaveServiceData weaveServiceData;
+    WeaveBLEDeviceIdentificationInfo deviceIdInfo;
 
     // Form the contents of the advertising packet.
     memset(&advData, 0, sizeof(advData));
@@ -533,20 +524,16 @@ WEAVE_ERROR BLEManagerImpl::EncodeAdvertisingData(ble_gap_adv_data_t & gapAdvDat
     err = ble_advdata_encode(&advData, mAdvDataBuf, &gapAdvData.adv_data.len);
     SuccessOrExit(err);
 
-    // Construct the Weave Service Data structure that will be sent in the scan response packet.
-    memset(&weaveServiceData, 0, sizeof(weaveServiceData));
-    weaveServiceData.MajorVersion = 0;
-    weaveServiceData.MinorVersion = 1;
-    Encoding::LittleEndian::Put16(weaveServiceData.DeviceVendorId, (uint16_t)WEAVE_DEVICE_CONFIG_DEVICE_VENDOR_ID);
-    Encoding::LittleEndian::Put16(weaveServiceData.DeviceProductId, (uint16_t)WEAVE_DEVICE_CONFIG_DEVICE_PRODUCT_ID);
-    Encoding::LittleEndian::Put64(weaveServiceData.DeviceId, FabricState.LocalNodeId);
-    weaveServiceData.PairingStatus = ConfigurationMgr().IsPairedToAccount() ? 1 : 0;
+    // Initialize the Weave BLE Device Identification Information block that will be sent as payload
+    // within the BLE service advertisement data.
+    err = ConfigurationMgr().GetBLEDeviceIdentificationInfo(deviceIdInfo);
+    SuccessOrExit(err);
 
     // Form the contents of the scan response packet.
     memset(&serviceData, 0, sizeof(serviceData));
     serviceData.service_uuid = UUID16_WoBLEService;
-    serviceData.data.size = sizeof(weaveServiceData);
-    serviceData.data.p_data = (uint8_t *)&weaveServiceData;
+    serviceData.data.size = sizeof(deviceIdInfo);
+    serviceData.data.p_data = (uint8_t *)&deviceIdInfo;
     memset(&advData, 0, sizeof(advData));
     advData.name_type = BLE_ADVDATA_NO_NAME;
     advData.include_appearance = false;
diff --git a/src/ble/WeaveBleServiceData.h b/src/ble/WeaveBleServiceData.h
@@ -0,0 +1,111 @@
+/*
+ *
+ *    Copyright (c) 2019 Google LLC.
+ *    All rights reserved.
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+/**
+ *    @file
+ *          Definitions for Weave BLE service advertisement data.
+ */
+
+#ifndef WEAVE_BLE_SERVICE_DATA_H
+#define WEAVE_BLE_SERVICE_DATA_H
+
+namespace nl {
+namespace Ble {
+
+/**
+ * Weave data block types that may appear with Weave BLE service advertisement data.
+ */
+enum WeaveBLEServiceDataType
+{
+    kWeaveBLEServiceDataType_DeviceIdentificationInfo       = 0x01,
+    kWeaveBLEServiceDataType_TokenIdentificationInfo        = 0x02,
+};
+
+/**
+ * Weave BLE Device Identification Information Block
+ *
+ * Defines the over-the-air encoded format of the device identification information block that appears
+ * within Weave BLE service advertisement data.
+ */
+struct WeaveBLEDeviceIdentificationInfo
+{
+    enum
+    {
+        kMajorVersion           = 0,
+        kMinorVersion           = 1,
+    };
+
+    enum
+    {
+        kPairingStatus_Unpaired = 0,
+        kPairingStatus_Paired   = 1,
+    };
+
+    uint8_t BlockLen;
+    uint8_t BlockType;
+    uint8_t MajorVersion;
+    uint8_t MinorVersion;
+    uint8_t DeviceVendorId[2];
+    uint8_t DeviceProductId[2];
+    uint8_t DeviceId[8];
+    uint8_t PairingStatus;
+
+    void Init()
+    {
+        memset(this, 0, sizeof(*this));
+        BlockLen = sizeof(*this) - sizeof(BlockLen); // size of all fields EXCEPT BlockLen
+        BlockType = kWeaveBLEServiceDataType_DeviceIdentificationInfo;
+        MajorVersion = kMajorVersion;
+        MinorVersion = kMinorVersion;
+    }
+
+    uint16_t GetVendorId(void)
+    {
+        return nl::Weave::Encoding::LittleEndian::Get16(DeviceVendorId);
+    }
+
+    void SetVendorId(uint16_t vendorId)
+    {
+        nl::Weave::Encoding::LittleEndian::Put16(DeviceVendorId, vendorId);
+    }
+
+    uint16_t GetProductId(void)
+    {
+        return nl::Weave::Encoding::LittleEndian::Get16(DeviceProductId);
+    }
+
+    void SetProductId(uint16_t productId)
+    {
+        nl::Weave::Encoding::LittleEndian::Put16(DeviceProductId, productId);
+    }
+
+    uint64_t GetDeviceId(void)
+    {
+        return nl::Weave::Encoding::LittleEndian::Get64(DeviceId);
+    }
+
+    void SetDeviceId(uint64_t deviceId)
+    {
+        nl::Weave::Encoding::LittleEndian::Put64(DeviceId, deviceId);
+    }
+} __attribute__((packed));
+
+} /* namespace Ble */
+} /* namespace nl */
+
+#endif // WEAVE_BLE_SERVICE_DATA_H
diff --git a/src/include/Makefile.am b/src/include/Makefile.am
@@ -81,6 +81,7 @@ $(nl_public_BleLayer_ble_source_dirstem)/BleLayer.h \
 $(nl_public_BleLayer_ble_source_dirstem)/BlePlatformDelegate.h \
 $(nl_public_BleLayer_ble_source_dirstem)/BleUUID.h \
 $(nl_public_BleLayer_ble_source_dirstem)/WoBle.h \
+$(nl_public_BleLayer_ble_source_dirstem)/WeaveBleServiceData.h \
 $(NULL)
 
 dist_ble_ble_HEADERS = $(addprefix ../,$(nl_public_BleLayer_ble_header_sources))
diff --git a/src/include/Makefile.in b/src/include/Makefile.in
@@ -717,6 +717,7 @@ $(nl_public_BleLayer_ble_source_dirstem)/BleLayer.h \
 $(nl_public_BleLayer_ble_source_dirstem)/BlePlatformDelegate.h \
 $(nl_public_BleLayer_ble_source_dirstem)/BleUUID.h \
 $(nl_public_BleLayer_ble_source_dirstem)/WoBle.h \
+$(nl_public_BleLayer_ble_source_dirstem)/WeaveBleServiceData.h \
 $(NULL)
 
 dist_ble_ble_HEADERS = $(addprefix ../,$(nl_public_BleLayer_ble_header_sources))
