Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 82 additions & 0 deletions src/NimBLE2905.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
/*
* Copyright 2020-2025 Ryan Powell <ryan@nable-embedded.io> and
* esp-nimble-cpp, NimBLE-Arduino contributors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#include "NimBLE2905.h"
#include "NimBLELog.h"

#include "NimBLECharacteristic.h"
#if CONFIG_BT_NIMBLE_ENABLED && MYNEWT_VAL(BLE_ROLE_PERIPHERAL)

// Define default if not already defined
#ifndef NIMBLE_MAX_AGGREGATE_FORMAT_DESCRIPTORS
#define NIMBLE_MAX_AGGREGATE_FORMAT_DESCRIPTORS 5 // Default value
#else
// Ensure the value is within a valid range
#if NIMBLE_MAX_AGGREGATE_FORMAT_DESCRIPTORS < 1 || NIMBLE_MAX_AGGREGATE_FORMAT_DESCRIPTORS > 255
#error "NIMBLE_MAX_AGGREGATE_FORMAT_DESCRIPTORS must be between 1 and 128"
#endif
#endif

static const char* LOG_TAG = "NimBLE2905";

NimBLE2905::NimBLE2905(NimBLECharacteristic* pChr)
: NimBLEDescriptor(NimBLEUUID(static_cast<uint16_t>(0x2905)), BLE_GATT_CHR_F_READ, NIMBLE_MAX_AGGREGATE_FORMAT_DESCRIPTORS * sizeof(uint16_t), pChr) {
} // NimBLE2905

void NimBLE2905::initValue() {
const size_t count = m_vAggregatedDescriptors.size();
uint16_t aggregatedHandles[count];

for (size_t i = 0; i < count; ++i) {
auto* desc = m_vAggregatedDescriptors[i];
uint16_t handle = desc->getHandle();

if (handle == 0) {
NIMBLE_LOGE(LOG_TAG, "Failed to initialize value: presentation format descriptor handle is not initialized");
return;
}

aggregatedHandles[i] = desc->getHandle();
} // initValue

setValue(reinterpret_cast<const uint8_t*>(aggregatedHandles), sizeof(aggregatedHandles));

// Presentation formats no longer needed, let's free some memory
m_vAggregatedDescriptors.clear();
m_vAggregatedDescriptors.shrink_to_fit();
} // initValue
Comment on lines +40 to +61

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Refactor to remove VLA and prevent memory hoarding on early return.

Shiver me timbers! Ye be usin' a Variable-Length Array (VLA) for aggregatedHandles, which be a compiler extension and not standard C++. Strict compilers will throw ye overboard! Worse yet, if ye hit a handle that's zero, yer early return skips the memory cleanup below, leavin' ye hoarding memory like a greedy pirate.

Let's patch both leaks by using a standard array sized to the maximum capacity and skippin' invalid handles with a continue instead of abandonin' ship!

🛠️ Proposed refactor for `initValue`
 void NimBLE2905::initValue() {
-    const size_t count = m_vAggregatedDescriptors.size();
-    uint16_t aggregatedHandles[count];
-
-    for (size_t i = 0; i < count; ++i) {
-        auto* desc = m_vAggregatedDescriptors[i];
-        uint16_t handle = desc->getHandle();
-
-        if (handle == 0) {
-            NIMBLE_LOGE(LOG_TAG, "Failed to initialize value: presentation format descriptor handle is not initialized");
-            return;
-        }
-
-        aggregatedHandles[i] = desc->getHandle();
-    } // initValue
-
-    setValue(reinterpret_cast<const uint8_t*>(aggregatedHandles), sizeof(aggregatedHandles));
+    uint16_t aggregatedHandles[NIMBLE_MAX_AGGREGATE_FORMAT_DESCRIPTORS];
+    size_t validCount = 0;
+
+    for (auto* desc : m_vAggregatedDescriptors) {
+        uint16_t handle = desc->getHandle();
+
+        if (handle == 0) {
+            NIMBLE_LOGE(LOG_TAG, "Failed to initialize value: presentation format descriptor handle is not initialized");
+            continue;
+        }
+
+        aggregatedHandles[validCount++] = handle;
+    }
+
+    if (validCount > 0) {
+        setValue(reinterpret_cast<const uint8_t*>(aggregatedHandles), validCount * sizeof(uint16_t));
+    }
 
     // Presentation formats no longer needed, let's free some memory
     m_vAggregatedDescriptors.clear();
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
void NimBLE2905::initValue() {
const size_t count = m_vAggregatedDescriptors.size();
uint16_t aggregatedHandles[count];
for (size_t i = 0; i < count; ++i) {
auto* desc = m_vAggregatedDescriptors[i];
uint16_t handle = desc->getHandle();
if (handle == 0) {
NIMBLE_LOGE(LOG_TAG, "Failed to initialize value: presentation format descriptor handle is not initialized");
return;
}
aggregatedHandles[i] = desc->getHandle();
} // initValue
setValue(reinterpret_cast<const uint8_t*>(aggregatedHandles), sizeof(aggregatedHandles));
// Presentation formats no longer needed, let's free some memory
m_vAggregatedDescriptors.clear();
m_vAggregatedDescriptors.shrink_to_fit();
} // initValue
void NimBLE2905::initValue() {
uint16_t aggregatedHandles[NIMBLE_MAX_AGGREGATE_FORMAT_DESCRIPTORS];
size_t validCount = 0;
for (auto* desc : m_vAggregatedDescriptors) {
uint16_t handle = desc->getHandle();
if (handle == 0) {
NIMBLE_LOGE(LOG_TAG, "Failed to initialize value: presentation format descriptor handle is not initialized");
continue;
}
aggregatedHandles[validCount++] = handle;
}
if (validCount > 0) {
setValue(reinterpret_cast<const uint8_t*>(aggregatedHandles), validCount * sizeof(uint16_t));
}
// Presentation formats no longer needed, let's free some memory
m_vAggregatedDescriptors.clear();
m_vAggregatedDescriptors.shrink_to_fit();
} // initValue
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/NimBLE2905.cpp` around lines 40 - 61, Update NimBLE2905::initValue to
replace the VLA aggregatedHandles with a standard fixed-capacity array, and size
the value passed to setValue for the number of valid entries. When a descriptor
handle is zero, skip it with continue rather than returning, while preserving
the cleanup that clears and shrinks m_vAggregatedDescriptors on every path.



/**
* @brief Add presentation format descriptor.
* @param [in] presentationFormat The 2904 descriptor to aggregate.
*/
void NimBLE2905::add2904Descriptor(const NimBLE2904* presentationFormat) {
if (presentationFormat == nullptr) {
NIMBLE_LOGE(LOG_TAG, "Failed to add presentation format descriptor: nullptr");
return;
}

if (m_vAggregatedDescriptors.size() < NIMBLE_MAX_AGGREGATE_FORMAT_DESCRIPTORS) {
m_vAggregatedDescriptors.push_back(presentationFormat);
} else {
NIMBLE_LOGE(LOG_TAG, "Failed to add presentation format descriptor: maximum capacity reached");
}
} // add2904Descriptor


#endif // CONFIG_BT_NIMBLE_ENABLED && MYNEWT_VAL(BLE_ROLE_PERIPHERAL)
56 changes: 56 additions & 0 deletions src/NimBLE2905.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/*
* Copyright 2020-2025 Ryan Powell <ryan@nable-embedded.io> and
* esp-nimble-cpp, NimBLE-Arduino contributors.
*
* 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.
*/

#ifndef NIMBLE_CPP_2905_H_
#define NIMBLE_CPP_2905_H_

#include "syscfg/syscfg.h"
#if CONFIG_BT_NIMBLE_ENABLED && MYNEWT_VAL(BLE_ROLE_PERIPHERAL)

# include "NimBLEDescriptor.h"

/**
* @brief Characteristic Aggregate Format descriptor (UUID: 0x2905).
*
* @details Contains an ordered list of handles referencing 0x2904 Presentation Format
* descriptors that define the parent characteristic’s value.
*/
class NimBLE2905 : public NimBLEDescriptor {
public:
NimBLE2905(NimBLECharacteristic* pChr = nullptr);

void add2904Descriptor(const NimBLE2904* presentationFormat);
private:

/**
* @brief Descriptor for the Characteristic Aggregate Format (UUID: 0x2905).
*
* @details Contains an ordered list of handles referencing the 0x2904
* Presentation Format descriptors that define the parent characteristic's value.
*
* @see NimBLEServer::start()
*/
void initValue();

friend class NimBLECharacteristic;
friend class NimBLEServer;

std::vector<const NimBLE2904*> m_vAggregatedDescriptors;
}; // NimBLE2904

#endif // CONFIG_BT_NIMBLE_ENABLED && MYNEWT_VAL(BLE_ROLE_PERIPHERAL)
#endif // NIMBLE_CPP_2904_H_
14 changes: 14 additions & 0 deletions src/NimBLECharacteristic.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
# endif

# include "NimBLE2904.h"
# include "NimBLE2905.h"
# include "NimBLEDevice.h"
# include "NimBLELog.h"

Expand Down Expand Up @@ -86,6 +87,9 @@ NimBLEDescriptor* NimBLECharacteristic::createDescriptor(const NimBLEUUID& uuid,
if (uuid == NimBLEUUID(static_cast<uint16_t>(0x2904))) {
NIMBLE_LOGW(LOG_TAG, "0x2904 descriptor should be created with create2904()");
pDescriptor = create2904();
} else if (uuid == NimBLEUUID(static_cast<uint16_t>(0x2905))) {
NIMBLE_LOGW(LOG_TAG, "0x2905 descriptor should be created with create2905()");
pDescriptor = create2905();
} else {
pDescriptor = new NimBLEDescriptor(uuid, properties, maxLen, this);
}
Expand All @@ -104,6 +108,16 @@ NimBLE2904* NimBLECharacteristic::create2904() {
return pDescriptor;
} // create2904

/**
* @brief Create a Characteristic Aggregate Format Descriptor for this characteristic.
* @return A pointer to a NimBLE2905 descriptor.
*/
NimBLE2905* NimBLECharacteristic::create2905() {
NimBLE2905* pDescriptor = new NimBLE2905(this);
addDescriptor(pDescriptor);
return pDescriptor;
} // create2905

/**
* @brief Add a descriptor to the characteristic.
* @param [in] pDescriptor A pointer to the descriptor to add.
Expand Down
2 changes: 2 additions & 0 deletions src/NimBLECharacteristic.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ class NimBLEService;
class NimBLECharacteristic;
class NimBLEDescriptor;
class NimBLE2904;
class NimBLE2905;

# include "NimBLELocalValueAttribute.h"

Expand Down Expand Up @@ -69,6 +70,7 @@ class NimBLECharacteristic : public NimBLELocalValueAttribute {
uint32_t properties = NIMBLE_PROPERTY::READ | NIMBLE_PROPERTY::WRITE,
uint16_t maxLen = BLE_ATT_ATTR_MAX_LEN);
NimBLE2904* create2904();
NimBLE2905* create2905();
NimBLEDescriptor* getDescriptorByUUID(const char* uuid, uint16_t index = 0) const;
NimBLEDescriptor* getDescriptorByUUID(const NimBLEUUID& uuid, uint16_t index = 0) const;
NimBLEDescriptor* getDescriptorByHandle(uint16_t handle) const;
Expand Down
14 changes: 14 additions & 0 deletions src/NimBLEServer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "NimBLEServer.h"
#if CONFIG_BT_NIMBLE_ENABLED && MYNEWT_VAL(BLE_ROLE_PERIPHERAL)

# include "NimBLE2905.h"
# include "NimBLEDevice.h"
# include "NimBLELog.h"

Expand Down Expand Up @@ -317,6 +318,19 @@ bool NimBLEServer::start() {
}
# endif

// Populate any Aggregate Format (0x2905) descriptors now that every attribute handle has been
// assigned during ble_gatts_start() (via the GATT register callback). A 0x2905 value is the
// ordered list of its aggregated 0x2904 presentation-format descriptor handles.
for (const auto& svc : m_svcVec) {
for (const auto& chr : svc->getCharacteristics()) {
for (auto& desc : chr->m_vDescriptors) {
if (desc->getUUID() == NimBLEUUID(static_cast<uint16_t>(0x2905))) {
static_cast<NimBLE2905*>(desc)->initValue();
}
}
}
}

// If the services have changed indicate it now
if (m_svcChanged) {
m_svcChanged = false;
Expand Down
3 changes: 3 additions & 0 deletions src/nimconfig.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
* Arduino User Options *
**********************************************/

/** @brief Uncomment to change the maximum number of aggregated presentation format descriptors; 5 by default. */
// #define NIMBLE_MAX_AGGREGATE_FORMAT_DESCRIPTORS 5

/** @brief Un-comment to change the number of simultaneous connections (esp controller max is 9) */
// #define MYNEWT_VAL_BLE_MAX_CONNECTIONS 3

Expand Down
Loading