[Bugfix] Missing notification data when length > 255 bytes#1113
Merged
[Bugfix] Missing notification data when length > 255 bytes#1113
Conversation
📝 WalkthroughWalkthroughArr, BLE GAP notification handling now looks up the characteristic by attribute handle, assembles fragmented mbuf payloads into a single value, validates total length (returns ATT error on mismatch/allocation failures), and invokes the characteristic notify callback only when assembly succeeds. Non-matching notifications are skipped. Changes
Sequence Diagram(s)sequenceDiagram
participant Peripheral as Peripheral (BLE Controller)
participant GAP as NimBLE GAP handler
participant Chr as NimBLERemoteCharacteristic
participant App as App notifyCallback
Peripheral->>GAP: BLE_GAP_EVENT_NOTIFY_RX (mbuf chain, attr_handle)
GAP->>GAP: find characteristic by attr_handle
alt characteristic not found
GAP-->>Peripheral: ignore / skip notification
else characteristic found
GAP->>GAP: iterate mbuf chain, assemble payload, track total length
alt length mismatch or alloc fail
GAP-->>Peripheral: return BLE_ATT_ERR_INVALID_ATTR_VALUE_LEN
GAP->>Chr: do not invoke callback (error state)
else assembly success
GAP->>Chr: set assembled value
GAP->>App: invoke notifyCallback(value, length)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
h2zero
force-pushed
the
bugfix/notify-data-len
branch
from
6d9c7cc to
e46d741
Compare
![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 the current code and only fix it if needed.
Inline comments:
In `@src/NimBLEClient.cpp`:
- Around line 1095-1106: When reassembly fails (rc != 0) do not set an empty
value on pChr->m_value and do not invoke pChr->m_notifyCallback, because
zero-length payloads are valid BLE notifications; instead bail out early on
error (or invoke a distinct error callback if you add one) so the application
can detect the failure. Locate the block handling rc in NimBLEClient.cpp
(references: rc, pChr->m_value, pChr->m_notifyCallback,
event->notify_rx.indication) and change it to skip calling m_notifyCallback when
rc != 0 (or call a new explicit error handler you add, e.g.,
pChr->m_notifyErrorCallback) rather than passing a zero-length value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 208409c8-5445-444c-abb5-0a10e1d5962f
📒 Files selected for processing (1)
src/NimBLEClient.cpp
h2zero
force-pushed
the
bugfix/notify-data-len
branch
from
e46d741 to
50a3a32
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/NimBLEClient.cpp (1)
1057-1074: Consider usin’getCharacteristic()to avoid duplicate lookup logic, matey.This search path repeats handle-matching logic already centralized in
NimBLEClient::getCharacteristic(uint16_t). Reusing it would reduce drift risk.🏴☠️ Suggested refactor
- NimBLERemoteCharacteristic* pChr = nullptr; - for (const auto& svc : pClient->m_svcVec) { - // Dont waste cycles searching services without this handle in its range - if (svc->getEndHandle() < event->notify_rx.attr_handle) { - continue; - } - - for (const auto& chr : svc->m_vChars) { - if (chr->getHandle() == event->notify_rx.attr_handle) { - NIMBLE_LOGD(LOG_TAG, "Got Notification for characteristic %s", chr->toString().c_str()); - pChr = chr; - break; - } - } - - if (pChr == nullptr) { - continue; - } + NimBLERemoteCharacteristic* pChr = pClient->getCharacteristic(event->notify_rx.attr_handle); + if (pChr == nullptr) { + NIMBLE_LOGE(LOG_TAG, "unknown handle: %d", event->notify_rx.attr_handle); + return BLE_ATT_ERR_INVALID_HANDLE; + } + NIMBLE_LOGD(LOG_TAG, "Got Notification for characteristic %s", pChr->toString().c_str()); auto len = event->notify_rx.om->om_len; if (pChr->m_value.setValue(event->notify_rx.om->om_data, len)) { @@ - break; - } - - if (pChr == nullptr) { - NIMBLE_LOGE(LOG_TAG, "unknown handle: %d", event->notify_rx.attr_handle); - rc = BLE_ATT_ERR_INVALID_HANDLE; - } + return rc;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEClient.cpp` around lines 1057 - 1074, Replace the manual nested service/characteristic lookup with the existing helper: call pClient->getCharacteristic(event->notify_rx.attr_handle) (NimBLEClient::getCharacteristic(uint16_t)) to obtain the NimBLERemoteCharacteristic* instead of iterating svc->m_vChars; assign the result to pChr, check for nullptr and log/handle notifications as before (use the same NIMBLE_LOGD call with pChr->toString() when non-null). This removes the duplicated handle-matching logic and centralizes lookup in NimBLEClient::getCharacteristic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/NimBLEClient.cpp`:
- Around line 1057-1074: Replace the manual nested service/characteristic lookup
with the existing helper: call
pClient->getCharacteristic(event->notify_rx.attr_handle)
(NimBLEClient::getCharacteristic(uint16_t)) to obtain the
NimBLERemoteCharacteristic* instead of iterating svc->m_vChars; assign the
result to pChr, check for nullptr and log/handle notifications as before (use
the same NIMBLE_LOGD call with pChr->toString() when non-null). This removes the
duplicated handle-matching logic and centralizes lookup in
NimBLEClient::getCharacteristic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4060d1b0-58a4-4d1d-870a-6c94d48fb737
📒 Files selected for processing (1)
src/NimBLEClient.cpp
When the ACL buffer is less than the MTU, the data arrives in more than one mbuf. This combines the data from the mbuf chain and stores it before calling the appliation callback, ensuring it has all the data
h2zero
force-pushed
the
bugfix/notify-data-len
branch
from
50a3a32 to
f23d2b2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/NimBLEClient.cpp (1)
1063-1064: Arr, consider a defensive null check fer the mbuf, savvy?The code be accessin'
event->notify_rx.om->om_lenandom_datawithout first checkin' ifombe NULL. While a NULL mbuf be rare in these waters (an exceptional situation per the BLE spec), a defensive check would prevent any potential kraken attacks (crashes) from sinkin' the ship.🛡️ Proposed defensive check
NIMBLE_LOGD(LOG_TAG, "Notify Received for handle: %d", event->notify_rx.attr_handle); NimBLERemoteCharacteristic* pChr = pClient->getCharacteristic(event->notify_rx.attr_handle); if (pChr == nullptr) { NIMBLE_LOGW(LOG_TAG, "unknown handle: %d", event->notify_rx.attr_handle); return BLE_ATT_ERR_INVALID_HANDLE; } + if (event->notify_rx.om == nullptr) { + NIMBLE_LOGE(LOG_TAG, "Notification mbuf is null"); + return BLE_ATT_ERR_UNLIKELY; + } + auto len = event->notify_rx.om->om_len;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEClient.cpp` around lines 1063 - 1064, The code dereferences event->notify_rx.om without checking for NULL before reading om_len/om_data; add a defensive null check for event->notify_rx.om at the start of the notification handling block (the code that reads auto len = event->notify_rx.om->om_len and calls pChr->m_value.setValue), and if NULL, handle gracefully (e.g., log via the existing logger, skip processing/return early or mark the characteristic update as failed) to avoid crashes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/NimBLEClient.cpp`:
- Around line 1063-1064: The code dereferences event->notify_rx.om without
checking for NULL before reading om_len/om_data; add a defensive null check for
event->notify_rx.om at the start of the notification handling block (the code
that reads auto len = event->notify_rx.om->om_len and calls
pChr->m_value.setValue), and if NULL, handle gracefully (e.g., log via the
existing logger, skip processing/return early or mark the characteristic update
as failed) to avoid crashes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 280e868e-8481-4d08-95d0-d47caa49c118
📒 Files selected for processing (1)
src/NimBLEClient.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When the ACL buffer is less than the MTU, the data arrives in more than one mbuf. This combines the data from the mbuf chain and stores it before calling the appliation callback, ensuring it has all the data
Summary by CodeRabbit