Conversation
h2zero
force-pushed
the
scan-response-timer
branch
4 times, most recently
from
64bae68 to
f47363e
Compare
h2zero
force-pushed
the
scan-response-timer
branch
3 times, most recently
from
96d0ef0 to
b8438ac
Compare
h2zero
force-pushed
the
scan-response-timer
branch
from
b8438ac to
bdb4021
Compare
There was a problem hiding this comment.
Pull request overview
Adds an optional (and default-enabled) scan-response timeout to ensure active scanning still produces onResult callbacks even when a peripheral never sends a scan response.
Changes:
- Introduces
CONFIG_NIMBLE_CPP_SCAN_RSP_TIMEOUTconfiguration (default 30ms, 0 to disable). - Adds a host-queue callout timer to trigger a “dummy scan response” path when the timeout elapses.
- Extends scan state tracking by storing a per-device timestamp used by the timeout logic.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
src/nimconfig.h |
Adds user-facing config and default for scan response timeout. |
src/NimBLEScan.h |
Adds scan-response timeout timer callback declaration and callout member. |
src/NimBLEScan.cpp |
Implements timer logic and enqueues a dummy scan response event to complete results. |
src/NimBLEAdvertisedDevice.h |
Adds per-device timestamp storage used for timeout scheduling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
h2zero
force-pushed
the
scan-response-timer
branch
from
bdb4021 to
1f164c2
Compare
|
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:
📝 WalkthroughWalkthroughArrr — Added scan-response timeout handling, FIFO waiting list for scannable devices, per-device timestamp and intrusive-list pointer ( Changes
Sequence Diagram(s)sequenceDiagram
participant GAP as GAP Event
participant Scanner as NimBLEScan
participant Timer as SR Timer
participant Device as AdvertisedDevice
GAP->>Scanner: advertisement received
Scanner->>Device: create/update (set m_time)
alt active scan & device scannable
Scanner->>Scanner: addWaitingDevice(Device)
Scanner->>Timer: ensure timer running for head
end
alt scan response received
GAP->>Scanner: scan response
Scanner->>Scanner: recordSrTime(Device)
Scanner->>Scanner: removeWaitingDevice(Device)
Scanner->>Scanner: onResult(Device)
else timeout expires
Timer->>Scanner: srTimerCb()
Scanner->>Scanner: select overdue head device
Scanner->>Scanner: schedule forceResultCallback(Device)
Scanner->>Scanner: force onResult(Device) (mark responded)
end
GAP->>Scanner: BLE_GAP_EVENT_DISC_COMPLETE
Scanner->>Timer: stop callout
Scanner->>Scanner: force remaining results, clear waiting list
Scanner->>Scanner: onScanEnd()
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/NimBLEScan.cpp (2)
76-114: The timer callback logic be sound, but there be a minor navigation error!The algorithm fer findin' the oldest device awaitin' a scan response and schedulin' the next timeout be correct. The unsigned arithmetic fer time comparison handles wrap-around properly - good seamanship!
However, I be spottin' a wee formatting issue that might make future crew members scratch their heads:
🔧 Minor spacing fix at line 84
The comparison logic and timer restart be correct - the elapsed time calculation using unsigned subtraction handles wrap-around properly. Just a tiny style nit:
At line 256 (in the
handleGapEventfunction, not shown in this range but related), there be a missing space:} else if (isLegacyAdv && advertisedDevice->isScannable() &&!ble_npl_callout_is_active(&pScan->m_srTimer)) {Should have a space before the
!:} else if (isLegacyAdv && advertisedDevice->isScannable() && !ble_npl_callout_is_active(&pScan->m_srTimer)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEScan.cpp` around lines 76 - 114, Fix the minor spacing nit in the conditional inside handleGapEvent: add a space before the negation operator in the condition that references advertisedDevice->isScannable() and !ble_npl_callout_is_active(&pScan->m_srTimer) so it reads "... && advertisedDevice->isScannable() && !ble_npl_callout_is_active(&pScan->m_srTimer))". Locate the condition in the handleGapEvent function (related to NimBLEScan and m_srTimer / advertisedDevice) and adjust the spacing for readability only.
255-259: Startin' the timer when we spot a scannable device!This be the trigger that starts the whole timeout mechanism. Only startin' the timer when it ain't already active prevents unnecessary resets.
However, there be a wee formatting barnacle on line 256:
🔧 Missing space before negation operator
- } else if (isLegacyAdv && advertisedDevice->isScannable() &&!ble_npl_callout_is_active(&pScan->m_srTimer)) { + } else if (isLegacyAdv && advertisedDevice->isScannable() && !ble_npl_callout_is_active(&pScan->m_srTimer)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEScan.cpp` around lines 255 - 259, The condition in NimBLEScan.cpp uses "&&!ble_npl_callout_is_active(&pScan->m_srTimer)" which lacks a space before the negation operator; update the conditional in the block that starts the scan-response timer (the check involving isLegacyAdv, advertisedDevice->isScannable(), and ble_npl_callout_is_active(&pScan->m_srTimer)) to use "&& !ble_npl_callout_is_active(...)" for correct/consistent formatting.
🤖 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/NimBLEScan.cpp`:
- Around line 76-114: Fix the minor spacing nit in the conditional inside
handleGapEvent: add a space before the negation operator in the condition that
references advertisedDevice->isScannable() and
!ble_npl_callout_is_active(&pScan->m_srTimer) so it reads "... &&
advertisedDevice->isScannable() &&
!ble_npl_callout_is_active(&pScan->m_srTimer))". Locate the condition in the
handleGapEvent function (related to NimBLEScan and m_srTimer / advertisedDevice)
and adjust the spacing for readability only.
- Around line 255-259: The condition in NimBLEScan.cpp uses
"&&!ble_npl_callout_is_active(&pScan->m_srTimer)" which lacks a space before the
negation operator; update the conditional in the block that starts the
scan-response timer (the check involving isLegacyAdv,
advertisedDevice->isScannable(), and
ble_npl_callout_is_active(&pScan->m_srTimer)) to use "&&
!ble_npl_callout_is_active(...)" for correct/consistent formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9adbc458-9132-4c41-b106-1f6e9dd365e8
📒 Files selected for processing (6)
src/NimBLEAdvertisedDevice.cppsrc/NimBLEAdvertisedDevice.hsrc/NimBLEScan.cppsrc/NimBLEScan.hsrc/nimconfig.hsrc/syscfg/syscfg.h
h2zero
force-pushed
the
scan-response-timer
branch
from
1f164c2 to
542fd41
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/NimBLEScan.h (1)
99-111: Arrr, this timeout hook changes theonResult()contract.With the synthetic scan-response path in
src/NimBLEScan.cpp, callers can now seeonResult()without any real scan-response payload. Please note that in the public callback docs so mates don’t assumeonResult()always means “complete data.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEScan.h` around lines 99 - 111, The public callback contract for NimBLEScanCallbacks::onResult must be updated to state that the synthetic scan-response path in NimBLEScan.cpp (triggered by NimBLEScan::srTimerCb / ble_npl_callout m_srTimer) can invoke onResult() without a real scan-response payload; update the onResult() documentation/comments in the NimBLEScanCallbacks declaration and any public API docs to explicitly say callers cannot assume onResult() always carries complete scan-response data and must handle missing/empty scan-response fields.
🤖 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/NimBLEScan.cpp`:
- Around line 103-106: The synthetic EXT_ADV lookup fails because only the
address is copied into dummyDesc when a scan response times out; on BLE_EXT_ADV
builds matching uses both address and disc.sid. Update the timeout handling so
that after memcpy(&dummyDesc.addr, curDev->getAddress().getBase(), ...) you also
set dummyDesc.sid to the advertising SID from the discovered device (use the
existing field/source used elsewhere, e.g. the discovered record's disc.sid or
the SID accessor on curDev) so the subsequent lookup (the code that matches by
address and disc.sid) finds the existing entry and avoids the "scan response
without advertisement" path.
- Around line 82-119: In NimBLEScan::srTimerCb, avoid enqueueing a dummy
scan-response for curDev unless its timeout has actually elapsed: compute
elapsed = now - curDev->m_time and only memcpy to dummyDesc.addr, log and
ble_npl_eventq_put(&dummySrTimerEvent) when elapsed >= srTimeoutTicks; otherwise
calculate remaining = srTimeoutTicks - elapsed and call
ble_npl_callout_reset(&pScan->m_srTimer, remaining) (similarly handle the case
where remaining==0 set to 1), and keep the existing nextDev logic to schedule
the next expiry using m_time and srTimeoutTicks.
---
Nitpick comments:
In `@src/NimBLEScan.h`:
- Around line 99-111: The public callback contract for
NimBLEScanCallbacks::onResult must be updated to state that the synthetic
scan-response path in NimBLEScan.cpp (triggered by NimBLEScan::srTimerCb /
ble_npl_callout m_srTimer) can invoke onResult() without a real scan-response
payload; update the onResult() documentation/comments in the NimBLEScanCallbacks
declaration and any public API docs to explicitly say callers cannot assume
onResult() always carries complete scan-response data and must handle
missing/empty scan-response fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0737c67e-0af2-4017-b273-93c6e765d1bb
📒 Files selected for processing (6)
src/NimBLEAdvertisedDevice.cppsrc/NimBLEAdvertisedDevice.hsrc/NimBLEScan.cppsrc/NimBLEScan.hsrc/nimconfig.hsrc/syscfg/syscfg.h
✅ Files skipped from review due to trivial changes (2)
- src/nimconfig.h
- src/syscfg/syscfg.h
🚧 Files skipped from review as they are similar to previous changes (2)
- src/NimBLEAdvertisedDevice.h
- src/NimBLEAdvertisedDevice.cpp
h2zero
force-pushed
the
scan-response-timer
branch
from
542fd41 to
6420308
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/NimBLEScan.cpp (1)
103-121:⚠️ Potential issue | 🔴 CriticalArrr, re-arm
curDevwhen its timeout has not actually elapsed.If a duplicate advertisement refreshes
curDev->m_time, Line 103 can be false even thoughcurDevis still the oldest pending device. This branch then schedulesnextDevinstead, or nothing at all, so the oldest advertiser never gets its timeout callback andm_maxResults == 0can start leaking again.🏴☠️ Proposed fix
- // Add the event to the host queue - if (curDev && now - curDev->m_time >= srTimeoutTicks) { + // Add the event to the host queue. + if (curDev) { + auto elapsed = now - curDev->m_time; + if (elapsed < srTimeoutTicks) { + ble_npl_callout_reset(&pScan->m_srTimer, srTimeoutTicks - elapsed); + return; + } # if MYNEWT_VAL(BLE_EXT_ADV) dummyDesc.sid = curDev->getSetId(); # endif memcpy(&dummyDesc.addr, curDev->getAddress().getBase(), sizeof(dummyDesc.addr)); NIMBLE_LOGI(LOG_TAG, "Scan response timeout for: %s", curDev->getAddress().toString().c_str()); ble_npl_eventq_put(nimble_port_get_dflt_eventq(), &dummySrTimerEvent); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEScan.cpp` around lines 103 - 121, The current logic only fires dummySrTimerEvent when curDev has actually timed out and otherwise schedules nextDev, which lets curDev slip if its m_time was refreshed; change the branch for curDev to re-arm its timeout when now - curDev->m_time < srTimeoutTicks by computing remaining = srTimeoutTicks - (now - curDev->m_time) (clamp to >=1) and calling ble_npl_callout_reset(&pScan->m_srTimer, remaining) (or otherwise schedule dummySrTimerEvent for that remaining delay) so the oldest pending device (curDev) always gets a future timeout even if its timestamp was updated; use the existing symbols curDev, m_time, srTimeoutTicks, dummySrTimerEvent and pScan->m_srTimer to implement this.
🤖 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/NimBLEScan.cpp`:
- Around line 88-99: srTimerCb() currently ignores devices once m_callbackSent
>= 2 even if a fresh legacy advertisement only updates m_time, so devices never
re-enter the “waiting for scan response” path; when the legacy-advertisement
handling code that updates dev->m_time (the block that stores a fresh legacy
advertisement) runs, also reset or decrement dev->m_callbackSent (e.g., set to 0
or to a value <2) for that device so srTimerCb() will reconsider it and re-arm
the scan-response timeout and eventual onResult() callout; update the
legacy-advertisement update site and ensure you only reset m_callbackSent for
devices that truly started a new advertisement cycle (don’t touch devices that
legitimately completed two full callbacks).
---
Duplicate comments:
In `@src/NimBLEScan.cpp`:
- Around line 103-121: The current logic only fires dummySrTimerEvent when
curDev has actually timed out and otherwise schedules nextDev, which lets curDev
slip if its m_time was refreshed; change the branch for curDev to re-arm its
timeout when now - curDev->m_time < srTimeoutTicks by computing remaining =
srTimeoutTicks - (now - curDev->m_time) (clamp to >=1) and calling
ble_npl_callout_reset(&pScan->m_srTimer, remaining) (or otherwise schedule
dummySrTimerEvent for that remaining delay) so the oldest pending device
(curDev) always gets a future timeout even if its timestamp was updated; use the
existing symbols curDev, m_time, srTimeoutTicks, dummySrTimerEvent and
pScan->m_srTimer to implement this.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a50b5f99-a57d-423d-82fb-efaad4b6af74
📒 Files selected for processing (6)
src/NimBLEAdvertisedDevice.cppsrc/NimBLEAdvertisedDevice.hsrc/NimBLEScan.cppsrc/NimBLEScan.hsrc/nimconfig.hsrc/syscfg/syscfg.h
✅ Files skipped from review due to trivial changes (1)
- src/nimconfig.h
🚧 Files skipped from review as they are similar to previous changes (4)
- src/NimBLEScan.h
- src/syscfg/syscfg.h
- src/NimBLEAdvertisedDevice.h
- src/NimBLEAdvertisedDevice.cpp
h2zero
force-pushed
the
scan-response-timer
branch
from
6420308 to
0c61601
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/NimBLEScan.cpp (2)
97-117:⚠️ Potential issue | 🔴 CriticalArrr, re-arm the timeout for
curDevbefore ye look atnextDev.When Lines 98-116 see the oldest pending device but it has not actually reached
m_srTimeoutTicksyet, the callback falls through without resettingm_srTimerfor that device. That can leave the timer idle after a refreshed advertisement or after the previously oldest device already completed, soonResult()never fires for the still-pending entry.🏴☠️ Proposed fix
- // Add the event to the host queue - if (curDev && now - curDev->m_time >= pScan->m_srTimeoutTicks) { + if (curDev) { + auto elapsed = now - curDev->m_time; + if (elapsed < pScan->m_srTimeoutTicks) { + auto remaining = pScan->m_srTimeoutTicks - elapsed; + ble_npl_callout_reset(&pScan->m_srTimer, remaining ? remaining : 1); + return; + } + + // Add the event to the host queue # if MYNEWT_VAL(BLE_EXT_ADV) dummyDesc.sid = curDev->getSetId(); # endif memcpy(&dummyDesc.addr, curDev->getAddress().getBase(), sizeof(dummyDesc.addr)); NIMBLE_LOGI(LOG_TAG, "Scan response timeout for: %s", curDev->getAddress().toString().c_str()); ble_npl_eventq_put(nimble_port_get_dflt_eventq(), &dummySrTimerEvent); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEScan.cpp` around lines 97 - 117, The timer re-arm logic fails to reset pScan->m_srTimer for curDev when curDev is still pending, causing onResult() to never fire for that entry; fix by moving/resetting the sr timer for curDev before evaluating nextDev: when curDev exists and has not yet reached pScan->m_srTimeoutTicks, compute the remaining timeout and call ble_npl_callout_reset(&pScan->m_srTimer, remaining) (using curDev->m_time and pScan->m_srTimeoutTicks) so the dummySrTimerEvent/dummyDesc handling still posts the event when it elapses, then proceed to compute/reset for nextDev as currently implemented.
83-95:⚠️ Potential issue | 🟠 MajorArrr, keep one explicit “waiting for scan response” state.
Lines 83-95 treat any
m_callbackSent < 2device as timer-eligible, while Lines 223-229 reopen duplicates by zeroingm_callbackSent. That includes EXT_ADV entries that bail out at Lines 235-237 withm_callbackSent == 0, and it also makes repeated legacy advertisements fireonDiscovered()again. Track onlym_callbackSent == 1insrTimerCb(), and reopen completed active-scannable devices back to1, not0.🏴☠️ Proposed fix
- if (dev->m_callbackSent < 2 && dev->isScannable()) { + if (dev->m_callbackSent == 1 && dev->isScannable()) { if (!curDev || (now - dev->m_time > now - curDev->m_time)) { nextDev = curDev; curDev = dev; continue; } @@ NIMBLE_LOGI(LOG_TAG, "Duplicate; updated: %s", advertisedAddress.toString().c_str()); // Restart scan-response timeout when we see a new non-scan-response // legacy advertisement during active scanning for a scannable device. - advertisedDevice->m_time = ble_npl_time_get(); - advertisedDevice->m_callbackSent = 0; - if (advertisedDevice->isScannable() && !ble_npl_callout_is_active(&pScan->m_srTimer)) { - ble_npl_callout_reset(&pScan->m_srTimer, pScan->m_srTimeoutTicks); + if (!pScan->m_scanParams.passive && advertisedDevice->isScannable()) { + advertisedDevice->m_time = ble_npl_time_get(); + if (advertisedDevice->m_callbackSent > 1) { + advertisedDevice->m_callbackSent = 1; + } + if (!ble_npl_callout_is_active(&pScan->m_srTimer)) { + ble_npl_callout_reset(&pScan->m_srTimer, pScan->m_srTimeoutTicks); + } }Also applies to: 223-229
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEScan.cpp` around lines 83 - 95, In srTimerCb() (the loop iterating pScan->m_scanResults.m_deviceVec) change the eligibility check from "m_callbackSent < 2 && dev->isScannable()" to only consider devices with m_callbackSent == 1 and dev->isScannable(), and update the reopen logic where completed active-scannable devices are currently reset to 0 (the code around the block that zeroes m_callbackSent) so that such devices are reopened by setting m_callbackSent back to 1 instead of 0; this ensures only the explicit "waiting for scan response" state (m_callbackSent == 1) is scheduled by srTimerCb and prevents legacy/EXT_ADV entries from being retriggered incorrectly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/NimBLEScan.cpp`:
- Around line 97-117: The timer re-arm logic fails to reset pScan->m_srTimer for
curDev when curDev is still pending, causing onResult() to never fire for that
entry; fix by moving/resetting the sr timer for curDev before evaluating
nextDev: when curDev exists and has not yet reached pScan->m_srTimeoutTicks,
compute the remaining timeout and call ble_npl_callout_reset(&pScan->m_srTimer,
remaining) (using curDev->m_time and pScan->m_srTimeoutTicks) so the
dummySrTimerEvent/dummyDesc handling still posts the event when it elapses, then
proceed to compute/reset for nextDev as currently implemented.
- Around line 83-95: In srTimerCb() (the loop iterating
pScan->m_scanResults.m_deviceVec) change the eligibility check from
"m_callbackSent < 2 && dev->isScannable()" to only consider devices with
m_callbackSent == 1 and dev->isScannable(), and update the reopen logic where
completed active-scannable devices are currently reset to 0 (the code around the
block that zeroes m_callbackSent) so that such devices are reopened by setting
m_callbackSent back to 1 instead of 0; this ensures only the explicit "waiting
for scan response" state (m_callbackSent == 1) is scheduled by srTimerCb and
prevents legacy/EXT_ADV entries from being retriggered incorrectly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ebc4748-bd8a-4938-b806-58918d86f28b
📒 Files selected for processing (4)
src/NimBLEAdvertisedDevice.hsrc/NimBLEScan.cppsrc/NimBLEScan.hsrc/nimconfig.h
✅ Files skipped from review due to trivial changes (1)
- src/nimconfig.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/NimBLEAdvertisedDevice.h
h2zero
force-pushed
the
scan-response-timer
branch
from
32724c5 to
ca46c60
Compare
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/NimBLEScan.cpp (1)
142-144: Arr, deinitializin' a static from an instance be a bit peculiar, matey.
dummySrTimerEventbe a module-level static, but it's bein' deinitialized in the instance destructor. While this be safe in practice sinceNimBLEScanbe effectively a singleton (created only byNimBLEDevice), andble_npl_event_deinitbe a no-op on FreeRTOS, it be architecturally unusual. If the singleton assumption ever changes, this could cause issues.Consider movin' the event initialization/deinitialization to match the static's lifetime, or document the singleton assumption.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEScan.cpp` around lines 142 - 144, The destructor in NimBLEScan currently calls ble_npl_event_deinit(&dummySrTimerEvent) even though dummySrTimerEvent is a module-level static; instead, either move the init/deinit of dummySrTimerEvent out of the NimBLEScan instance lifecycle to module scope (e.g., perform ble_npl_event_init/ble_npl_event_deinit in a module-level init/cleanup pair or a singleton manager), or remove the instance deinit and document the singleton assumption clearly; update references around m_srTimer and ble_npl_callout_deinit so only instance-owned resources are deinitialized in the NimBLEScan destructor and module-owned dummySrTimerEvent is handled by module-level code (or a static refcount) to avoid tearing down a static from an instance.
🤖 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/NimBLEScan.h`:
- Around line 126-148: In toString() the std::string is pre-sized to 400 then
populated via snprintf leaving the string length at 400 with embedded nulls; fix
by capturing snprintf's return (the written/required length) and then resize the
std::string to that actual character count (handling negative return as zero and
clamp to buffer size if it equals/exceeds the buffer) so the returned string
contains only the formatted text.
---
Nitpick comments:
In `@src/NimBLEScan.cpp`:
- Around line 142-144: The destructor in NimBLEScan currently calls
ble_npl_event_deinit(&dummySrTimerEvent) even though dummySrTimerEvent is a
module-level static; instead, either move the init/deinit of dummySrTimerEvent
out of the NimBLEScan instance lifecycle to module scope (e.g., perform
ble_npl_event_init/ble_npl_event_deinit in a module-level init/cleanup pair or a
singleton manager), or remove the instance deinit and document the singleton
assumption clearly; update references around m_srTimer and
ble_npl_callout_deinit so only instance-owned resources are deinitialized in the
NimBLEScan destructor and module-owned dummySrTimerEvent is handled by
module-level code (or a static refcount) to avoid tearing down a static from an
instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a437b2f1-e9d4-47bd-b44e-7651e133f582
📒 Files selected for processing (4)
src/NimBLEAdvertisedDevice.cppsrc/NimBLEAdvertisedDevice.hsrc/NimBLEScan.cppsrc/NimBLEScan.h
🚧 Files skipped from review as they are similar to previous changes (2)
- src/NimBLEAdvertisedDevice.cpp
- src/NimBLEAdvertisedDevice.h
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/NimBLEScan.cpp (1)
205-212:⚠️ Potential issue | 🟠 MajorBlimey! This be causin' duplicate onDiscovered callbacks, matey!
Resettin'
m_callbackSent = 0at line 208 means the check at line 224 (if (!advertisedDevice->m_callbackSent)) will pass again, causin'onDiscovered()to be called multiple times for the same device on each duplicate advertisement.If the intent be to allow the SR timeout path to work again, consider settin' it to
1instead of0, so only the SR timeout/result callback can fire again without re-triggerin' the discovery callback:🏴☠️ Proposed fix to prevent duplicate onDiscovered calls
// Restart scan-response timeout when we see a new non-scan-response // legacy advertisement during active scanning for a scannable device. advertisedDevice->m_time = ble_npl_time_get(); - advertisedDevice->m_callbackSent = 0; + advertisedDevice->m_callbackSent = 1; if (pScan->m_srTimeoutTicks && advertisedDevice->isScannable() && !ble_npl_callout_is_active(&pScan->m_srTimer)) { ble_npl_callout_reset(&pScan->m_srTimer, pScan->m_srTimeoutTicks); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEScan.cpp` around lines 205 - 212, The code resets advertisedDevice->m_callbackSent to 0 which allows the later check (!advertisedDevice->m_callbackSent) to re-fire onDiscovered for duplicate adverts; change the reset to set advertisedDevice->m_callbackSent = 1 so the discovery callback won't be re-triggered while still restarting the scan-response timer (keep the ble_npl_callout_reset(&pScan->m_srTimer, pScan->m_srTimeoutTicks) logic intact) and ensure this change is applied where advertisedDevice->m_time and the SR timer handling occur (references: advertisedDevice->m_time, advertisedDevice->m_callbackSent, advertisedDevice->isScannable(), pScan->m_srTimeoutTicks, pScan->m_srTimer).
🧹 Nitpick comments (1)
src/NimBLEScan.cpp (1)
434-436: A minor navigation concern, captain!The stats be reset at the very start of
start(), even if the scan be already in progress andrestartbefalse. This means ye might lose stats from an ongoing scan even when not actually restarting it.Consider movin' the reset to after the restart check, or only resettin' when
!isContinue:🏴☠️ Suggested adjustment
bool NimBLEScan::start(uint32_t duration, bool isContinue, bool restart) { - m_stats.reset(); - NIMBLE_LOGD(LOG_TAG, ">> start: duration=%" PRIu32, duration); if (isScanning()) { if (restart) { NIMBLE_LOGI(LOG_TAG, "Scan already in progress, restarting it"); if (!stop()) { return false; } if (!isContinue) { clearResults(); + m_stats.reset(); } } } else { // Don't clear results while scanning is active if (!isContinue) { clearResults(); + m_stats.reset(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEScan.cpp` around lines 434 - 436, In NimBLEScan::start, m_stats.reset() is called unconditionally at the top which clears stats even when a scan is already running and restart == false; move or conditionally execute the reset so stats are only cleared when restarting or starting a new non-continuous scan (e.g., after the restart check or guarded by if (restart || !isContinue))—update the logic around NimBLEScan::start and references to m_stats.reset to preserve ongoing scan statistics when restart is false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/NimBLEScan.cpp`:
- Around line 205-212: The code resets advertisedDevice->m_callbackSent to 0
which allows the later check (!advertisedDevice->m_callbackSent) to re-fire
onDiscovered for duplicate adverts; change the reset to set
advertisedDevice->m_callbackSent = 1 so the discovery callback won't be
re-triggered while still restarting the scan-response timer (keep the
ble_npl_callout_reset(&pScan->m_srTimer, pScan->m_srTimeoutTicks) logic intact)
and ensure this change is applied where advertisedDevice->m_time and the SR
timer handling occur (references: advertisedDevice->m_time,
advertisedDevice->m_callbackSent, advertisedDevice->isScannable(),
pScan->m_srTimeoutTicks, pScan->m_srTimer).
---
Nitpick comments:
In `@src/NimBLEScan.cpp`:
- Around line 434-436: In NimBLEScan::start, m_stats.reset() is called
unconditionally at the top which clears stats even when a scan is already
running and restart == false; move or conditionally execute the reset so stats
are only cleared when restarting or starting a new non-continuous scan (e.g.,
after the restart check or guarded by if (restart || !isContinue))—update the
logic around NimBLEScan::start and references to m_stats.reset to preserve
ongoing scan statistics when restart is false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 200d34f6-d189-4d57-944c-da260800bfa0
📒 Files selected for processing (4)
src/NimBLEAdvertisedDevice.cppsrc/NimBLEAdvertisedDevice.hsrc/NimBLEScan.cppsrc/NimBLEScan.h
💤 Files with no reviewable changes (1)
- src/NimBLEAdvertisedDevice.cpp
✅ Files skipped from review due to trivial changes (1)
- src/NimBLEAdvertisedDevice.h
h2zero
force-pushed
the
scan-response-timer
branch
from
11530a8 to
b4dc2d4
Compare
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/NimBLEAdvertisedDevice.h`:
- Line 167: The field m_pNextWaiting must be initialized to the "not in list"
sentinel (this) for every NimBLEAdvertisedDevice construction because
addWaitingDevice() and removeWaitingDevice() rely on m_pNextWaiting == this;
update every constructor of class NimBLEAdvertisedDevice (including the public
default constructor) to set m_pNextWaiting = this in their initializer or body,
or alternatively remove/delete the default constructor if default-constructed
instances must be disallowed; ensure changes reference m_pNextWaiting, the
NimBLEAdvertisedDevice constructors, and the functions
addWaitingDevice/removeWaitingDevice so the invariant holds everywhere.
In `@src/NimBLEScan.cpp`:
- Around line 338-342: The loop over pScan->m_scanResults.m_deviceVec calls
pScan->m_pScanCallbacks->onResult(dev) which can mutate/clear m_scanResults and
invalidate the iterator; fix by snapshotting the pending devices before invoking
callbacks (e.g. build a small vector of pointers/refs from
pScan->m_scanResults.m_deviceVec for devices where dev->isScannable() &&
dev->m_callbackSent < 2), update pScan->m_stats.incMissedSrCount() while
building the snapshot, then after the iteration over the snapshot call
m_pScanCallbacks->onResult(dev) for each saved device so the internal container
is not mutated during traversal.
- Around line 117-195: The waiting-list (m_pWaitingListHead, m_pWaitingListTail,
and each device's m_pNextWaiting) is mutated from multiple contexts
(addWaitingDevice, removeWaitingDevice, clearWaitingList, resetWaitingTimer) and
races with srTimerCb/handleGapEvent and public callers like
erase()/clearResults; serialize these mutations onto one task or add a
protection primitive around all accesses (e.g., a mutex or run-on-host-queue
wrapper). Concretely, ensure every call-site that reads or writes the waiting
list (addWaitingDevice, removeWaitingDevice, clearWaitingList, resetWaitingTimer
and the srTimerCb/handleGapEvent callbacks) acquires the same lock or posts the
operation to the NimBLE host queue before touching
m_pWaitingListHead/m_pWaitingListTail/m_pNextWaiting, and release/return only
after the list work is complete so the intrusive list cannot be concurrently
modified.
In `@src/NimBLEScan.h`:
- Around line 126-147: The toString() formatter prints srMinMs/srMaxMs as
UINT32_MAX when srCount == 0; update toString() to clamp the displayed SR
min/max/avg values when srCount is zero (e.g., compute displayMin = srCount ?
srMinMs : 0, displayMax = srCount ? srMaxMs : 0, displayAvg = srCount ?
srTotalMs / srCount : 0) and pass those display* variables into snprintf instead
of srMinMs/srMaxMs/srTotalMs so the output shows 0 (or another chosen sentinel)
rather than 4294967295 when there are no samples.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f4f80ad1-1f1f-4577-baca-a8430f1bbfa5
📒 Files selected for processing (4)
src/NimBLEAdvertisedDevice.cppsrc/NimBLEAdvertisedDevice.hsrc/NimBLEScan.cppsrc/NimBLEScan.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/NimBLEAdvertisedDevice.cpp
h2zero
force-pushed
the
scan-response-timer
branch
from
0527f1f to
84f557f
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (5)
src/NimBLEScan.cpp (3)
57-74:⚠️ Potential issue | 🔴 CriticalArrr, the timeout can still fire against the wrong head.
If the current head gets a real scan response at Lines 269-273, ye remove it from the FIFO but leave the old callout armed. When that deadline hits,
srTimerCb()at Lines 57-74 times out whatever device is now at the head without checking its own elapsed wait. Re-arm on head removal here, or guardsrTimerCb()withnow - curDev->m_time >= m_srTimeoutTicks.🏴☠️ Possible patch
void NimBLEScan::srTimerCb(ble_npl_event* event) { auto* pScan = NimBLEDevice::getScan(); auto curDev = pScan->m_pWaitingListHead; if (curDev == nullptr) { ble_npl_callout_stop(&pScan->m_srTimer); return; } + + ble_npl_time_t elapsed = ble_npl_time_get() - curDev->m_time; + if (elapsed < pScan->m_srTimeoutTicks) { + ble_npl_time_t remaining = pScan->m_srTimeoutTicks - elapsed; + ble_npl_callout_reset(&pScan->m_srTimer, remaining == 0 ? 1 : remaining); + return; + } NIMBLE_LOGI(LOG_TAG, "Scan response timeout for: %s", curDev->getAddress().toString().c_str()); @@ if (event_type == BLE_HCI_ADV_RPT_EVTYPE_SCAN_RSP) { pScan->m_stats.recordSrTime(ble_npl_time_get() - advertisedDevice->m_time); NIMBLE_LOGI(LOG_TAG, "Scan response from: %s", advertisedAddress.toString().c_str()); - pScan->removeWaitingDevice(advertisedDevice); + bool wasHead = pScan->m_pWaitingListHead == advertisedDevice; + pScan->removeWaitingDevice(advertisedDevice); + if (wasHead) { + pScan->resetWaitingTimer(); + } } else {Also applies to: 269-273
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEScan.cpp` around lines 57 - 74, srTimerCb can fire for the wrong device because the head may have been removed after a real scan response but the callout remained armed; update srTimerCb (which reads m_pWaitingListHead) to verify the timed-out device actually exceeded m_srTimeoutTicks by comparing now - curDev->m_time >= m_srTimeoutTicks before treating it as timed out, and/or ensure removeWaitingDevice/resetWaitingTimer re-arms or stops the m_srTimer/callout when the head is removed so the stale deadline cannot fire for a new head (refer to NimBLEScan::srTimerCb, NimBLEScan::removeWaitingDevice, NimBLEScan::resetWaitingTimer, member m_srTimeoutTicks and device m_time).
332-337:⚠️ Potential issue | 🟠 MajorArrr, this scan-end fallback still calls user code while sailin'
m_deviceVec.Line 333 iterates the internal vector and Line 336 calls
onResult(), so a callback that erases or clears results blows up the loop. It also leavesm_callbackSentunchanged, so the same stale entry can be emitted again on laterstart(..., true)scans. Snapshot the pending devices first, mark each one complete, then invoke callbacks from the snapshot.🏴☠️ Possible patch
case BLE_GAP_EVENT_DISC_COMPLETE: { pScan->clearWaitingList(); - for (const auto& dev : pScan->m_scanResults.m_deviceVec) { - if (dev->isScannable() && dev->m_callbackSent < 2) { - pScan->m_stats.incMissedSrCount(); - pScan->m_pScanCallbacks->onResult(dev); - } - } + std::vector<NimBLEAdvertisedDevice*> pending{}; + pending.reserve(pScan->m_scanResults.m_deviceVec.size()); + for (const auto& dev : pScan->m_scanResults.m_deviceVec) { + if (dev->isScannable() && dev->m_callbackSent < 2) { + pScan->m_stats.incMissedSrCount(); + dev->m_callbackSent = 2; + pending.push_back(dev); + } + } + for (const auto& dev : pending) { + pScan->m_pScanCallbacks->onResult(dev); + } NIMBLE_LOGD(LOG_TAG, "discovery complete; reason=%d", event->disc_complete.reason);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEScan.cpp` around lines 332 - 337, The loop in NimBLEScan.cpp iterates pScan->m_scanResults.m_deviceVec and invokes pScan->m_pScanCallbacks->onResult(dev) directly which can mutate/clear the vector and re-emit stale entries; instead, first snapshot the pending devices (filter devices where isScannable() && m_callbackSent < 2) into a local list, then for each snapshot entry set its m_callbackSent to the completed value (e.g., mark as sent/2) and increment stats via pScan->m_stats.incMissedSrCount(), and only after marking/incrementing invoke pScan->m_pScanCallbacks->onResult(...) on the snapshot so the original m_scanResults.m_deviceVec is not mutated during iteration and entries won’t be re-emitted on later start(..., true) calls.
108-191:⚠️ Potential issue | 🔴 CriticalArrr, the waiting FIFO still be touched from multiple contexts without a guard.
handleGapEvent()andsrTimerCb()mutatem_pWaitingListHead/m_pWaitingListTailon the host queue, while Lines 597, 619-620, 635-636, and 689-695 let public user-callable paths mutate the same intrusive list. Ye already wrap the result-vector swap in a critical section at Lines 693-695, but the waiting FIFO itself still has none. A concurrenterase()/clearResults()can unlink or free the current head while the host task is walking it.Also applies to: 597-597, 619-620, 635-636, 689-695
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEScan.cpp` around lines 108 - 191, The waiting FIFO (m_pWaitingListHead / m_pWaitingListTail) is accessed from host callbacks (handleGapEvent, srTimerCb) and public methods (addWaitingDevice, removeWaitingDevice, clearWaitingList, resetWaitingTimer, erase(), clearResults()) without synchronization; wrap every mutation and traversal of the intrusive list in the same critical section used for the results-vector swap (i.e., the existing critical lock used around the result swap) to prevent concurrent unlink/free races — specifically, add enter/exit critical section calls at the start/end of addWaitingDevice, removeWaitingDevice, clearWaitingList, resetWaitingTimer and around any code paths in erase() / clearResults() that touch m_pWaitingListHead or m_pWaitingListTail, ensuring sentinel updates (pDev->m_pNextWaiting = pDev) occur while holding the lock.src/NimBLEScan.h (1)
126-147:⚠️ Potential issue | 🟡 MinorArrr, zero matched scan responses still print a bogus minimum.
When
srCount == 0, Line 141 formatssrMinMswhile it still holdsUINT32_MAX, so the stats string saysmin=4294967295. Clamp the displayed minimum to0orn/awhen there be no samples.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEScan.h` around lines 126 - 147, In NimBLEScan::toString() the SR min value prints srMinMs even when srCount == 0 (srMinMs still UINT32_MAX); update the formatting logic in toString() to conditionally use a safe display value (e.g., 0 or "n/a") when srCount is zero instead of srMinMs, and ensure the same conditional approach used for avg (srCount ? srTotalMs / srCount : 0) is applied to the min (and max if desired) so the snprintf call uses the computed safe value(s) rather than raw srMinMs.src/NimBLEAdvertisedDevice.h (1)
161-167:⚠️ Potential issue | 🟡 MinorArrr, the self-pointer sentinel still ain't guaranteed for default-constructed devices.
Line 167 says
thisbe the “not in list” state, but Line 47 still default-constructsm_pNextWaiting{}to null.addWaitingDevice()andremoveWaitingDevice()both key offm_pNextWaiting == this, so a default-constructedNimBLEAdvertisedDevicestarts in the wrong state. Please initialize the sentinel in every constructor, or delete the default constructor if that state be unsupported.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEAdvertisedDevice.h` around lines 161 - 167, The sentinel pointer m_pNextWaiting must be initialized to point to the object itself so addWaitingDevice() and removeWaitingDevice() can detect the "not in list" state; update NimBLEAdvertisedDevice so that every constructor (including the default constructor) sets m_pNextWaiting = this (or change the member initializer to m_pNextWaiting{this}) or remove the default constructor if that state is unsupported, ensuring existing code paths in addWaitingDevice() and removeWaitingDevice() behave correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/NimBLEAdvertisedDevice.h`:
- Around line 161-167: The sentinel pointer m_pNextWaiting must be initialized
to point to the object itself so addWaitingDevice() and removeWaitingDevice()
can detect the "not in list" state; update NimBLEAdvertisedDevice so that every
constructor (including the default constructor) sets m_pNextWaiting = this (or
change the member initializer to m_pNextWaiting{this}) or remove the default
constructor if that state is unsupported, ensuring existing code paths in
addWaitingDevice() and removeWaitingDevice() behave correctly.
In `@src/NimBLEScan.cpp`:
- Around line 57-74: srTimerCb can fire for the wrong device because the head
may have been removed after a real scan response but the callout remained armed;
update srTimerCb (which reads m_pWaitingListHead) to verify the timed-out device
actually exceeded m_srTimeoutTicks by comparing now - curDev->m_time >=
m_srTimeoutTicks before treating it as timed out, and/or ensure
removeWaitingDevice/resetWaitingTimer re-arms or stops the m_srTimer/callout
when the head is removed so the stale deadline cannot fire for a new head (refer
to NimBLEScan::srTimerCb, NimBLEScan::removeWaitingDevice,
NimBLEScan::resetWaitingTimer, member m_srTimeoutTicks and device m_time).
- Around line 332-337: The loop in NimBLEScan.cpp iterates
pScan->m_scanResults.m_deviceVec and invokes
pScan->m_pScanCallbacks->onResult(dev) directly which can mutate/clear the
vector and re-emit stale entries; instead, first snapshot the pending devices
(filter devices where isScannable() && m_callbackSent < 2) into a local list,
then for each snapshot entry set its m_callbackSent to the completed value
(e.g., mark as sent/2) and increment stats via
pScan->m_stats.incMissedSrCount(), and only after marking/incrementing invoke
pScan->m_pScanCallbacks->onResult(...) on the snapshot so the original
m_scanResults.m_deviceVec is not mutated during iteration and entries won’t be
re-emitted on later start(..., true) calls.
- Around line 108-191: The waiting FIFO (m_pWaitingListHead /
m_pWaitingListTail) is accessed from host callbacks (handleGapEvent, srTimerCb)
and public methods (addWaitingDevice, removeWaitingDevice, clearWaitingList,
resetWaitingTimer, erase(), clearResults()) without synchronization; wrap every
mutation and traversal of the intrusive list in the same critical section used
for the results-vector swap (i.e., the existing critical lock used around the
result swap) to prevent concurrent unlink/free races — specifically, add
enter/exit critical section calls at the start/end of addWaitingDevice,
removeWaitingDevice, clearWaitingList, resetWaitingTimer and around any code
paths in erase() / clearResults() that touch m_pWaitingListHead or
m_pWaitingListTail, ensuring sentinel updates (pDev->m_pNextWaiting = pDev)
occur while holding the lock.
In `@src/NimBLEScan.h`:
- Around line 126-147: In NimBLEScan::toString() the SR min value prints srMinMs
even when srCount == 0 (srMinMs still UINT32_MAX); update the formatting logic
in toString() to conditionally use a safe display value (e.g., 0 or "n/a") when
srCount is zero instead of srMinMs, and ensure the same conditional approach
used for avg (srCount ? srTotalMs / srCount : 0) is applied to the min (and max
if desired) so the snprintf call uses the computed safe value(s) rather than raw
srMinMs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8e4e73b5-3b4c-4890-a347-f4c5edcd265e
📒 Files selected for processing (4)
src/NimBLEAdvertisedDevice.cppsrc/NimBLEAdvertisedDevice.hsrc/NimBLEScan.cppsrc/NimBLEScan.h
✅ Files skipped from review due to trivial changes (1)
- src/NimBLEAdvertisedDevice.cpp
h2zero
force-pushed
the
scan-response-timer
branch
from
84f557f to
0c11810
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/NimBLEScan.h (1)
126-147:⚠️ Potential issue | 🟡 MinorArrr, zero scan responses still print a bogus minimum, ye scallywag!
When
srCount == 0, the formatter printssrMinMsasUINT32_MAX(4294967295) which be mighty confusin' for sailors tryin' to tune their scan parameters. The average be handled with a ternary, but the min/max fields be not.🏴☠️ Proposed fix to clamp min/max when no samples exist
snprintf(out.data(), out.size(), "Scan stats:\n" " Devices seen : %" PRIu32 "\n" " Duplicate advs : %" PRIu32 "\n" " Scan responses : %" PRIu32 "\n" " SR timing (ms) : min=%" PRIu32 ", max=%" PRIu32 ", avg=%" PRIu64 "\n" " Orphaned SR : %" PRIu32 "\n" " Missed SR : %" PRIu32 "\n", devCount, dupCount, srCount, - srMinMs, - srMaxMs, + srCount ? srMinMs : 0, + srCount ? srMaxMs : 0, srCount ? srTotalMs / srCount : 0, orphanedSrCount, missedSrCount);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEScan.h` around lines 126 - 147, The toString() formatter prints srMinMs/srMaxMs even when srCount==0, showing UINT32_MAX; update the snprintf arguments in NimBLEScan::toString() to use conditional/clamped values for the min and max (e.g., use srCount ? srMinMs : 0 and srCount ? srMaxMs : 0) and keep the existing ternary for average (srCount ? srTotalMs / srCount : 0) so min/max/avg are sensible when srCount is zero; ensure the order of arguments passed to snprintf matches the format placeholders (devCount, dupCount, srCount, minVal, maxVal, avgVal, orphanedSrCount, missedSrCount).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/NimBLEScan.h`:
- Around line 126-147: The toString() formatter prints srMinMs/srMaxMs even when
srCount==0, showing UINT32_MAX; update the snprintf arguments in
NimBLEScan::toString() to use conditional/clamped values for the min and max
(e.g., use srCount ? srMinMs : 0 and srCount ? srMaxMs : 0) and keep the
existing ternary for average (srCount ? srTotalMs / srCount : 0) so min/max/avg
are sensible when srCount is zero; ensure the order of arguments passed to
snprintf matches the format placeholders (devCount, dupCount, srCount, minVal,
maxVal, avgVal, orphanedSrCount, missedSrCount).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce089f21-4caa-444a-812f-03bb983bf59d
📒 Files selected for processing (4)
src/NimBLEAdvertisedDevice.cppsrc/NimBLEAdvertisedDevice.hsrc/NimBLEScan.cppsrc/NimBLEScan.h
✅ Files skipped from review due to trivial changes (1)
- src/NimBLEAdvertisedDevice.cpp
h2zero
force-pushed
the
scan-response-timer
branch
2 times, most recently
from
11b6053 to
cb43e11
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
This PR adds scan-response timeout handling to the BLE scanning flow so onResult() is delivered within a bounded time even when devices don’t respond to active scan requests, and it adds debug-only scan diagnostics to help tune scanning parameters.
Changes:
- Introduces a per-scan FIFO “waiting list” and a callout timer to force-deliver pending scan results when scan responses don’t arrive in time.
- Adds debug-level scan statistics (device counts, duplicate advs, scan-response timing, missed/orphaned scan responses).
- Extends
NimBLEAdvertisedDevicewith timestamp and intrusive waiting-list linkage used by the scan-response timeout mechanism.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/NimBLEScan.h |
Adds scan-response timeout API, debug stats struct/string formatting, and new timer/waiting-list members. |
src/NimBLEScan.cpp |
Implements timeout callout + waiting-list management and integrates it into GAP scan event handling. |
src/NimBLEAdvertisedDevice.h |
Adds time and intrusive-list pointer fields used by the scan-response timeout workflow. |
src/NimBLEAdvertisedDevice.cpp |
Initializes the intrusive-list sentinel pointer in the adv-device constructor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
h2zero
force-pushed
the
scan-response-timer
branch
from
cb43e11 to
7a5a25a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
h2zero
force-pushed
the
scan-response-timer
branch
5 times, most recently
from
e627df2 to
eab5e5d
Compare
This ensures that the callback will be called within the configured time (in ms) when devices fail to respond to a scan response request within that time. * Adds stats when debug logging to help tune the scan response timeout/scan parameters.
h2zero
force-pushed
the
scan-response-timer
branch
from
eab5e5d to
a5e88fc
Compare
This ensures that the callback will be called within the configured time (in ms) when devices fail to respond to a scan response request within that time.
Closes #591
Summary by CodeRabbit
New Features
Bug Fixes
Behavior