Add channel index handling to NimBLEAdvertisedDevice and related structures #2
Conversation
…ctures - Introduced m_channelIndex in NimBLEAdvertisedDevice to store the primary advertising channel. - Implemented getChannel() method to retrieve the advertising channel. - Updated setChannelIndex() method for setting the channel index. - Modified handleGapEvent() in NimBLEScan to set the channel index from the discovery event. - Added channel_index field to ble_gap_ext_disc_desc and ble_gap_disc_desc structures. - Enhanced ble_hs_hci_evt_le_adv_rpt() to handle channel index in advertising reports.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe PR adds BLE advertising channel tracking to NimBLE by introducing a private channel index member and public getChannel() method to NimBLEAdvertisedDevice. It extends GAP report structures with channel_index fields, populates these in HCI event handlers, and updates the scanning example to display advertising channels. Changes
Sequence DiagramsequenceDiagram
participant HCI as HCI Event
participant Parser as ble_hs_hci_evt.c
participant GAP as GAP Struct
participant Scan as NimBLEScan
participant Device as NimBLEAdvertisedDevice
participant App as Example App
HCI->>Parser: LE Advertising Report
Parser->>Parser: Initialize channel_index = 0xFF
Parser->>Parser: Check if extra byte after RSSI
alt Extra byte exists & in bounds
Parser->>Parser: Read channel_index from report
end
Parser->>GAP: Populate ble_gap_disc_desc with channel_index
GAP->>Scan: Report discovered device
Scan->>Device: setChannelIndex(channel_index)
Device->>Device: Store m_channelIndex
App->>Device: getChannel()
Device->>Device: Map m_channelIndex (0–2) → BLE channel (37–39)
Device->>App: Return mapped channel or 0xFF
App->>App: Display advertising channel in scan results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Consume optional channel index when parsing legacy reports
The legacy advertising parser advances data by sizeof(rpt) + rpt->data_len + 1 before it checks for an optional channel byte. Because the increment on line 509 never includes the extra byte and sizeof(rpt) is just the pointer size, the subsequent conditional can never detect or skip the channel index. When controllers append the channel index (Bluetooth 5.4), that byte remains in the stream and is treated as the header of the next report, producing corrupted results or a length error. The parser needs to account for the extra byte and advance the cursor accordingly.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ble_gap_rx_ext_adv_report(&desc); | ||
|
|
There was a problem hiding this comment.
After reading desc.channel_index, the extended advertising loop still advances to the next report with report = (const void *)&report->data[report->data_len]; (lines 668‑670). When a controller actually appends the channel index immediately after the data, this pointer calculation leaves that byte in front of the next report so every subsequent iteration interprets the channel byte as part of the next header, yielding corrupted descriptors. The iterator should advance by the extra byte whenever one was consumed.
Useful? React with 👍 / 👎.
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/NimBLEAdvertisedDevice.cpp(2 hunks)src/NimBLEAdvertisedDevice.h(4 hunks)src/NimBLEScan.cpp(1 hunks)src/nimble/nimble/host/include/host/ble_gap.h(2 hunks)src/nimble/nimble/host/src/ble_hs_hci_evt.c(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/NimBLEAdvertisedDevice.h (1)
src/NimBLEAdvertisedDevice.cpp (2)
getChannel(831-837)getChannel(831-831)
🔇 Additional comments (13)
src/nimble/nimble/host/include/host/ble_gap.h (2)
407-408: LGTM!The addition of
channel_indexto the extended discovery descriptor is well-placed and documented. The 0xFF sentinel value appropriately indicates unavailable channel information.
440-441: LGTM!Consistent addition of
channel_indexto the legacy discovery descriptor, maintaining parity with the extended advertising descriptor.src/NimBLEScan.cpp (1)
133-133: LGTM!The channel index is properly propagated from the discovery descriptor to the advertised device, placed logically alongside RSSI and payload handling.
src/NimBLEAdvertisedDevice.h (4)
41-43: LGTM!Clear documentation of the
getChannel()method behavior, correctly identifying the BLE advertising channels (37–39) and the unknown sentinel value.
126-126: LGTM!Public accessor method properly declared.
156-156: LGTM!Private setter appropriately implemented as an inline assignment.
172-172: LGTM!Private member variable appropriately declared with correct type.
src/NimBLEAdvertisedDevice.cpp (2)
39-39: LGTM!Proper initialization of
m_channelIndexto the unknown sentinel value (0xFF) in the constructor.
827-837: LGTM!The channel index mapping is correct. BLE advertising channels (37, 38, 39) are properly derived from controller indices (0, 1, 2), with appropriate handling of unknown values.
src/nimble/nimble/host/src/ble_hs_hci_evt.c (4)
504-504: LGTM!Appropriate initialization of
channel_indexto the unknown sentinel value before processing advertising reports.
543-543: LGTM!Correct initialization for direct advertising reports, which do not contain channel index information in their standard format.
621-621: LGTM!Proper initialization of
channel_indexfor each extended advertising report before attempting to read the optional field.
659-666: The review comment's analysis of lines 659-666 is incorrectThe original concerns are based on faulty assumptions:
data_len < 255check is correct: Per Bluetooth specification, when data payload is less than maximum (255 bytes), the extended report format includes an optional channel index byte. The condition properly identifies when space exists for this extra byte.Bounds check is valid: The condition
(const uint8_t*)channel_ptr < (const uint8_t*)data + lencorrectly validates that the byte is within the event buffer before reading.Logic is sound: The code properly reads the channel index when available and within bounds.
However, there are critical issues in the same file that require attention:
- Lines 468, 509, 518 contain
sizeof(rpt)bugs (whererptis a pointer). These should besizeof(*rpt)to get the struct size, not the pointer size. This causes severe buffer offset miscalculations when parsing multiple reports.Likely an incorrect or invalid review comment.
…w-functionality Add workflow to build PlatformIO examples
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/nimble/nimble/host/src/ble_hs_hci_evt.c (2)
509-509: Critical: sizeof pointer vs sizeof structLine 509 uses
sizeof(rpt)which returns the size of the pointer (typically 4 or 8 bytes), not thestruct adv_report. This causes incorrect pointer arithmetic when advancing to the next report.Apply this diff:
- data += sizeof(rpt) + rpt->data_len + 1; + data += sizeof(*rpt) + rpt->data_len + 1;
659-670: Critical: Pointer advancement doesn't account for consumed channel byte.After reading
desc.channel_indexfrom*channel_ptr(line 664), the loop advances to the next report at line 670 using onlyreport->data_len. When the channel index byte is present and consumed, this leaves that byte positioned as the start of the next report, corrupting all subsequent report parsing.Apply this diff to advance past the channel byte when it was read:
+ uint8_t extra_bytes = 0; /* Channel index may follow the data if available */ if (report->data_len < 255) { const uint8_t *channel_ptr = &report->data[report->data_len]; /* Verify we're not reading past the event data */ if ((const uint8_t*)channel_ptr < (const uint8_t*)data + len) { desc.channel_index = *channel_ptr; + extra_bytes = 1; } } ble_gap_rx_ext_adv_report(&desc); - report = (const void *) &report->data[report->data_len]; + report = (const void *) &report->data[report->data_len + extra_bytes];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/platformio-examples.yml(1 hunks)examples/NimBLE_Scan_Continuous/NimBLE_Scan_Continuous.ino(2 hunks)src/nimble/nimble/host/src/ble_hs_hci_evt.c(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (5)
examples/NimBLE_Scan_Continuous/NimBLE_Scan_Continuous.ino (1)
20-27: LGTM! Clear demonstration of channel reporting.The callback correctly uses
getChannel()and handles both known channels (printing the channel number) and unknown channels (0xFF sentinel) appropriately. This provides a good example for users of the new channel awareness feature.src/nimble/nimble/host/src/ble_hs_hci_evt.c (3)
504-504: LGTM! Proper default initialization.Initializing
channel_indexto0xFF(unknown) is the correct default before attempting to read the optional channel byte.
543-543: LGTM! Proper default for direct advertising reports.Direct advertising reports don't include channel index data, so defaulting to
0xFFis appropriate.
621-621: LGTM! Proper default initialization for extended reports.Initializing
channel_indexto0xFFbefore attempting to read the optional channel byte is correct..github/workflows/platformio-examples.yml (1)
1-63: LGTM! Well-structured CI workflow.The workflow properly:
- Triggers on relevant file changes (src, examples, library properties)
- Caches PlatformIO for faster builds
- Iterates through all Arduino examples with proper error handling
- Uses appropriate error checking (
set -euo pipefail, zero-example detection)This ensures that examples like
NimBLE_Scan_Continuous.ino(which now uses the new channel reporting features) are validated in CI.
Summary by CodeRabbit
New Features
Tests & CI/CD