Characteristic Aggregate Format Descriptor (0x2905) support for Peripheral device#1054
Characteristic Aggregate Format Descriptor (0x2905) support for Peripheral device#1054srgg wants to merge 1 commit into
Conversation
ccf45e6 to
ec85ea8
Compare
|
This looks really good, only issue I have is the changes to the core files. This would make the api incompatible with the esp-idf version of NimBLE, maybe there's another way. |
|
@h2zero I don’t have much experience with ESP-IDF yet. Could you provide more details about the issue so I can explore alternative ways to address it? Honestly, I was hesitant to modify ble_gatts directly, since changes are introduced there from time to time. At the moment, the approach I took felt like the most straightforward way to fix the problem. |
|
I try to avoid modifying the stack code unless it's to address something that won't break the API with the esp-idf repo. I maintain a separate cpp wrapper for esp-idf: https://github.com/h2zero/esp-nimble-cpp since the NimBLE stack is included already in esp-idf, whereas in this repo it is provided. So in this case the 2905 class would not work with esp-idf as it would not have those core changes that are needed and the API would no longer be the same for both repos. |
1ab9dfd to
dc01636
Compare
|
@srgg I think I have a solution to the stack change requirements for this. Please see #1107 That should simplify this PR and class quite a bit. |
dc01636 to
5e4ce61
Compare
📝 WalkthroughWalkthroughAdds pirate-ready support for the Bluetooth Characteristic Aggregate Format descriptor (UUID 0x2905), including descriptor creation, 2904 handle aggregation, configurable capacity, and post-startup value initialization. ChangesAggregate Format descriptor
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Application
participant NimBLECharacteristic
participant NimBLEServer
participant NimBLE2905
Application->>NimBLECharacteristic: create2905()
NimBLECharacteristic-->>Application: NimBLE2905*
NimBLECharacteristic->>NimBLE2905: add2904Descriptor()
NimBLEServer->>NimBLEServer: ble_gatts_start()
NimBLEServer->>NimBLE2905: initValue()
NimBLE2905-->>NimBLEServer: aggregate 2904 handles
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Ports the 0x2905 Characteristic Aggregate Format feature (from feat/aggregate-format-2905 @ dc01636, 2.3.6-based) onto NimBLE-Arduino 2.5.0. Only the feature is carried: NimBLE2905 descriptor class, NimBLECharacteristic::create2905(), the 0x2905 branch in createDescriptor(), the NIMBLE_MAX_AGGREGATE_FORMAT_DESCRIPTORS knob, and the post-start initValue() pass in NimBLEServer::start(). The fork's descriptor-handle-assignment fix (Phase-1 + the ble_gatts_find_dsc dsc_arg change) is intentionally dropped: 2.5.0 already contains upstream h2zero#1107 ("Properly set attribute handles"), which owns duplicate-safe handle assignment via the GATT register callback. So the host-C is left untouched.
5e4ce61 to
1ff1c68
Compare
|
@h2zero, excuse me for the long pause; it has been rebased |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/NimBLE2905.cpp`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4860e599-d3ad-442a-8e16-e41e6c3dcf15
📒 Files selected for processing (6)
src/NimBLE2905.cppsrc/NimBLE2905.hsrc/NimBLECharacteristic.cppsrc/NimBLECharacteristic.hsrc/NimBLEServer.cppsrc/nimconfig.h
| 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 |
There was a problem hiding this comment.
🩺 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.
| 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.
2 participants
Summary
This PR adds full support for the Characteristic Aggregate Format Descriptor (UUID
0x2905) when operating as a BLE peripheral. Correct implementation requires reliable handle mapping for multiple Presentation Format descriptors (0x2904), which previously failed due to duplicate handle assignment.This PR also introduces PlatformIO build support for the existing examples and includes a usage demonstration of this new capability in
NimBLE_Server.ino.Problem
Descriptor handle resolution used UUID-only lookup (
ble_gatts_find_dsc()), which meant:0x2905) could not assemble a correct referenced handle listThe BLE specification requires each referenced descriptor to have a unique handle for valid aggregation.
Fix
ble_gatt_dsc_defAdditional Enhancements
They still function as regular Arduino sketches
and can now also be built and flashed using PlatformIO:
NimBLE_Server.inoexample to include a practical usage example of the Aggregate Format descriptor feature:Impact
0x2904)Summary by CodeRabbit