[Bugfix] Missing notification data when length > 255 bytes#403
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughArr: client notification flow now finds the matching remote characteristic, assembles multi-fragment Changes
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/NimBLEClient.cpp (1)
1054-1077: Optional DRY-up: reuse the handle lookup helper.Arr matey, this block re-implements characteristic lookup that
NimBLEClient::getCharacteristic(uint16_t)already provides. Reusin’ it would reduce traversal duplication and maintenance risk.♻️ Suggested simplification
- 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; - } - ... - NimBLERemoteCharacteristic* pChr = nullptr; - for (const auto& chr : svc->m_vChars) { - if (chr->getHandle() == event->notify_rx.attr_handle) { - pChr = chr; - break; - } - } - if (pChr == nullptr) { - continue; - } + NimBLERemoteCharacteristic* pChr = pClient->getCharacteristic(event->notify_rx.attr_handle); + if (pChr == nullptr) { + return 0; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEClient.cpp` around lines 1054 - 1077, This block duplicates characteristic lookup logic; replace the inner service+characteristic traversal with a call to NimBLEClient::getCharacteristic(uint16_t) using event->notify_rx.attr_handle to find the characteristic. Specifically, in the loop over pClient->m_svcVec (and before logging "Got Notification..."), call getCharacteristic(event->notify_rx.attr_handle) to obtain a NimBLERemoteCharacteristic* (check for nullptr) and then proceed with the existing notification handling; remove the nested for-loop that searches svc->m_vChars and its manual assignment to pChr to avoid duplicated traversal.
🤖 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 1054-1077: This block duplicates characteristic lookup logic;
replace the inner service+characteristic traversal with a call to
NimBLEClient::getCharacteristic(uint16_t) using event->notify_rx.attr_handle to
find the characteristic. Specifically, in the loop over pClient->m_svcVec (and
before logging "Got Notification..."), call
getCharacteristic(event->notify_rx.attr_handle) to obtain a
NimBLERemoteCharacteristic* (check for nullptr) and then proceed with the
existing notification handling; remove the nested for-loop that searches
svc->m_vChars and its manual assignment to pChr to avoid duplicated traversal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 587ed991-f1fb-426d-b5d7-4a616b8789d4
📒 Files selected for processing (1)
src/NimBLEClient.cpp
h2zero
force-pushed
the
bugfix/notify-data-len
branch
2 times, most recently
from
5933942 to
c348a7d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/NimBLEClient.cpp (1)
1057-1074: Hoist this handle lookup to one code path, matey.This reimplements
NimBLEClient::getCharacteristic(uint16_t)and can drift over time. Reusing the helper (or checking both start/end bounds) keeps lookup logic consistent and cheaper to maintain.⚓ 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_LOGD(LOG_TAG, "Got Notification for characteristic %s", pChr->toString().c_str()); ... - - break; }🤖 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, The handle lookup loop duplicates NimBLEClient::getCharacteristic(uint16_t); replace the manual iteration over pClient->m_svcVec and per-service m_vChars with a single call to pClient->getCharacteristic(event->notify_rx.attr_handle), assign the returned NimBLERemoteCharacteristic* to pChr, and then proceed to the existing notification handling (including the NIMBLE_LOGD call and null check). If you prefer to keep inline logic instead, make it consistent by checking both service start and end bounds (using service->getStartHandle() and service->getEndHandle()) before scanning m_vChars to match getCharacteristic’s behavior.
🤖 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: The handle lookup loop duplicates
NimBLEClient::getCharacteristic(uint16_t); replace the manual iteration over
pClient->m_svcVec and per-service m_vChars with a single call to
pClient->getCharacteristic(event->notify_rx.attr_handle), assign the returned
NimBLERemoteCharacteristic* to pChr, and then proceed to the existing
notification handling (including the NIMBLE_LOGD call and null check). If you
prefer to keep inline logic instead, make it consistent by checking both service
start and end bounds (using service->getStartHandle() and
service->getEndHandle()) before scanning m_vChars to match getCharacteristic’s
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4afae33b-58e9-4692-bd26-f55aeaaf4803
📒 Files selected for processing (1)
src/NimBLEClient.cpp
h2zero
force-pushed
the
bugfix/notify-data-len
branch
from
c348a7d to
3762eaa
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/NimBLEClient.cpp (1)
1063-1083: Arrr, avoid partial cache writes when fragment assembly fails.Right now
pChr->m_valueis mutated as fragments arrive; on error, a partial value can remain cached. Assemble in a temporary and assign only after full validation.🏴☠️ Proposed refactor
- auto len = event->notify_rx.om->om_len; - if (pChr->m_value.setValue(event->notify_rx.om->om_data, len)) { + NimBLEAttValue assembled{}; + auto len = event->notify_rx.om->om_len; + if (assembled.setValue(event->notify_rx.om->om_data, len)) { os_mbuf* next; next = SLIST_NEXT(event->notify_rx.om, om_next); while (next != NULL) { - pChr->m_value.append(next->om_data, next->om_len); - if (pChr->m_value.length() != len + next->om_len) { + assembled.append(next->om_data, next->om_len); + if (assembled.length() != len + next->om_len) { rc = BLE_ATT_ERR_INVALID_ATTR_VALUE_LEN; break; } len += next->om_len; next = SLIST_NEXT(next, om_next); } } else { rc = BLE_ATT_ERR_INVALID_ATTR_VALUE_LEN; } if (rc != 0) { // This should never happen NIMBLE_LOGE(LOG_TAG, "notification value error; exceeds limit"); return rc; } + + pChr->m_value.setValue(assembled.getValue().data(), assembled.length());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEClient.cpp` around lines 1063 - 1083, The code currently writes fragments directly into pChr->m_value during assembly and may leave a partial value on error; instead, allocate or use a temporary container (e.g., a local os_mbuf-like buffer or a temporary m_value copy) and perform setValue/append operations against that temporary using event->notify_rx.om, SLIST_NEXT, setValue, append and length checks; only if assembly and length validation succeed, assign or swap the temporary into pChr->m_value atomically; on any error return BLE_ATT_ERR_INVALID_ATTR_VALUE_LEN without mutating the original pChr->m_value.
🤖 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-1083: The code currently writes fragments directly into
pChr->m_value during assembly and may leave a partial value on error; instead,
allocate or use a temporary container (e.g., a local os_mbuf-like buffer or a
temporary m_value copy) and perform setValue/append operations against that
temporary using event->notify_rx.om, SLIST_NEXT, setValue, append and length
checks; only if assembly and length validation succeed, assign or swap the
temporary into pChr->m_value atomically; on any error return
BLE_ATT_ERR_INVALID_ATTR_VALUE_LEN without mutating the original pChr->m_value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b25ea70-b836-42be-9790-659489939cf9
📒 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
3762eaa to
52598b1
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/NimBLEClient.cpp (1)
1086-1090: Arr, ye spotted treasure to plunder later, matey!The
const_casthere be a bit like patchin' yer hull with barnacles - it works but ain't pretty. The TODO comment shows ye be aware this callback interface needs modernizin' to useNimBLEAttValuedirectly.Would ye like me to hoist the flag and open an issue to track this here technical debt fer a future voyage?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEClient.cpp` around lines 1086 - 1090, The code is using const_cast to pass raw pointer/length into pChr->m_notifyCallback; instead update the notify callback to accept a NimBLEAttValue (or const NimBLEAttValue&) instead of raw data/length, update the callback type/registration where m_notifyCallback is defined, and change the call site to pass pChr->m_value directly (removing const_cast and uses of m_value.getValue().data() / m_value.length()); ensure all places that set or implement m_notifyCallback are updated to the new signature.
🤖 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 1086-1090: The code is using const_cast to pass raw pointer/length
into pChr->m_notifyCallback; instead update the notify callback to accept a
NimBLEAttValue (or const NimBLEAttValue&) instead of raw data/length, update the
callback type/registration where m_notifyCallback is defined, and change the
call site to pass pChr->m_value directly (removing const_cast and uses of
m_value.getValue().data() / m_value.length()); ensure all places that set or
implement m_notifyCallback are updated to the new signature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ff6387e9-db49-4a87-867d-63de49a86cc6
📒 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