Characteristic Aggregate Format Descriptor (0x2905) support for Peripheral device - #1054
Characteristic Aggregate Format Descriptor (0x2905) support for Peripheral device#1054srgg wants to merge 2 commits 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 |
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
|
Thanks @srgg, looks good, I think we should do as code rabbit suggests here. |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/NimBLE2905.cpp (1)
29-30: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winArrr, make the configured range and error message agree.
The guard accepts values through
255, but the#errorsays the maximum be128. Thus configurations from129through255compile while contradicting the diagnostic. Update the message to255, or tighten the condition to> 128if that be the intended limit.🛠️ Suggested fix
- `#error` "NIMBLE_MAX_AGGREGATE_FORMAT_DESCRIPTORS must be between 1 and 128" + `#error` "NIMBLE_MAX_AGGREGATE_FORMAT_DESCRIPTORS must be between 1 and 255"🤖 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 29 - 30, Align the NIMBLE_MAX_AGGREGATE_FORMAT_DESCRIPTORS validation in the preprocessor guard with its diagnostic: either change the `#error` message maximum to 255 to match the current accepted range, or change the upper-bound condition to 128 if that is the intended limit. Ensure the condition and message describe the same configured range.
🤖 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.
Outside diff comments:
In `@src/NimBLE2905.cpp`:
- Around line 29-30: Align the NIMBLE_MAX_AGGREGATE_FORMAT_DESCRIPTORS
validation in the preprocessor guard with its diagnostic: either change the
`#error` message maximum to 255 to match the current accepted range, or change the
upper-bound condition to 128 if that is the intended limit. Ensure the condition
and message describe the same configured range.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5135aaa4-3603-4929-a944-25752bfe302a
📒 Files selected for processing (1)
src/NimBLE2905.cpp
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