[Bugfix] HCI response to disconnect of unknown ID should return success.#414
[Bugfix] HCI response to disconnect of unknown ID should return success.#414
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAvast: standardized HCI error checks to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
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 docstrings
🧪 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/NimBLEServer.cpp (1)
336-343:⚠️ Potential issue | 🔴 CriticalArrr, matey! The server be needin' cleanup when the controller says it knows not the connection.
Line 338 treats
BLE_ERR_UNK_CONN_IDas success, but this be leavin'm_connectedPeersstale forever. When the controller reports it knows not the connection, there'll be noBLE_GAP_EVENT_DISCONNECTevent comin' back to clean up yer bookkeeping. This meansgetConnectedCount(),getPeerDevices(),advertiseOnDisconnect, and the optionalm_pClient->m_connHandlemirror stay stale. The client handles this right by settin'm_connStatus = DISCONNECTEDon the spot (line 387); the server should do the same form_connectedPeersbefore returnin'true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEServer.cpp` around lines 336 - 343, NimBLEServer::disconnect currently treats BLE_ERR_UNK_CONN_ID as a non-error and returns true without cleaning up state; update disconnect(uint16_t connHandle, uint8_t reason) so when ble_gap_terminate returns BLE_HS_HCI_ERR(BLE_ERR_UNK_CONN_ID) you remove the peer from m_connectedPeers (and perform the same per-peer cleanup you would on BLE_GAP_EVENT_DISCONNECT) and update any mirror state (e.g., clear the peer's m_connHandle, set its connection status to DISCONNECTED) before returning true so getConnectedCount(), getPeerDevices(), advertiseOnDisconnect and related logic reflect the disconnection.
🤖 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 381-387: When ble_gap_terminate(...) returns BLE_ERR_UNK_CONN_ID
the code sets m_connStatus to DISCONNECTED without firing the
BLE_GAP_EVENT_DISCONNECT cleanup path, which can orphan clients when
deleteOnDisconnect is true; modify the post-terminate branch in NimBLEClient.cpp
(where BLE_ERR_UNK_CONN_ID is checked) to perform the same cleanup the GAP
disconnect handler does: detect the BLE_ERR_UNK_CONN_ID fast path, check
this->deleteOnDisconnect (and any relevant client flags), and invoke the same
cleanup sequence used by the GAP handler (or call the same helper used by that
handler) so delete-on-disconnect clients are freed immediately rather than
relying on an event. Ensure m_lastErr and m_connStatus are still set
appropriately and reuse existing cleanup logic to avoid duplication.
---
Outside diff comments:
In `@src/NimBLEServer.cpp`:
- Around line 336-343: NimBLEServer::disconnect currently treats
BLE_ERR_UNK_CONN_ID as a non-error and returns true without cleaning up state;
update disconnect(uint16_t connHandle, uint8_t reason) so when ble_gap_terminate
returns BLE_HS_HCI_ERR(BLE_ERR_UNK_CONN_ID) you remove the peer from
m_connectedPeers (and perform the same per-peer cleanup you would on
BLE_GAP_EVENT_DISCONNECT) and update any mirror state (e.g., clear the peer's
m_connHandle, set its connection status to DISCONNECTED) before returning true
so getConnectedCount(), getPeerDevices(), advertiseOnDisconnect and related
logic reflect the disconnection.
🪄 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: f943e235-e0ba-400f-b17a-09b83939accb
📒 Files selected for processing (2)
src/NimBLEClient.cppsrc/NimBLEServer.cpp
src/NimBLEClient.cpp
Outdated
| if (rc != 0 && rc != BLE_HS_ENOTCONN && rc != BLE_HS_EALREADY && rc != BLE_HS_HCI_ERR(BLE_ERR_UNK_CONN_ID)) { | ||
| NIMBLE_LOGE(LOG_TAG, "ble_gap_terminate failed: rc=%d %s", rc, NimBLEUtils::returnCodeToString(rc)); | ||
| m_lastErr = rc; | ||
| return false; | ||
| } | ||
|
|
||
| m_connStatus = (rc == BLE_HS_ENOTCONN) ? DISCONNECTED : DISCONNECTING; | ||
| m_connStatus = (rc == BLE_HS_ENOTCONN || rc == BLE_HS_HCI_ERR(BLE_ERR_UNK_CONN_ID)) ? DISCONNECTED : DISCONNECTING; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== NimBLEClient::disconnect ==\n'
sed -n '375,389p' src/NimBLEClient.cpp
printf '\n== NimBLEDevice::deleteClient ==\n'
sed -n '352,400p' src/NimBLEDevice.cpp
printf '\n== deleteOnDisconnect usages ==\n'
rg -n -C2 'deleteOnDisconnect|deleteClient\(' src/NimBLEClient.cpp src/NimBLEDevice.cppRepository: h2zero/esp-nimble-cpp
Length of output: 5154
🏁 Script executed:
rg -n 'deleteOnDisconnect|BLE_GAP_EVENT_DISCONNECT|BLE_ERR_UNK_CONN_ID' src/NimBLEClient.cpp src/NimBLEDevice.cpp -A 3 -B 1Repository: h2zero/esp-nimble-cpp
Length of output: 3169
🏁 Script executed:
# Check if there's cleanup that happens outside GAP events
rg -n 'BLE_GAP_EVENT_DISCONNECT' src/ -A 30 | head -100Repository: h2zero/esp-nimble-cpp
Length of output: 6851
🏁 Script executed:
# Look for any task or timer-based cleanup
rg -n 'deinit|cleanup|delete.*client|purge' src/NimBLEDevice.cpp -i | head -20Repository: h2zero/esp-nimble-cpp
Length of output: 681
🏁 Script executed:
# Check if there are any comments about BLE_ERR_UNK_CONN_ID behavior
rg -n 'BLE_ERR_UNK_CONN_ID' src/ -B 3 -A 3Repository: h2zero/esp-nimble-cpp
Length of output: 2008
🏁 Script executed:
# Check what deleteOnDisconnect does when client is already DISCONNECTED
rg -n 'DISCONNECTED.*deleteOnDisconnect|deleteOnDisconnect.*DISCONNECTED' src/ -iRepository: h2zero/esp-nimble-cpp
Length of output: 276
🏁 Script executed:
# Search for any cleanup path that handles deleteOnDisconnect outside GAP event
rg -n 'deleteOnDisconnect' src/ -B 2 -A 5Repository: h2zero/esp-nimble-cpp
Length of output: 4568
Arrr, the BLE_ERR_UNK_CONN_ID fast path creates an orphaned client, matey!
Aye, when ble_gap_terminate() returns BLE_ERR_UNK_CONN_ID (line 387), the client transitions straight to DISCONNECTED without ever firing a BLE_GAP_EVENT_DISCONNECT event. This be a problem: if NimBLEDevice::deleteClient() sets deleteOnDisconnect=true and calls disconnect(), the flag be set but the GAP handler that checks it (line 1085 in NimBLEClient.cpp) will never run. The client be left orphaned with cleanup forever deferred.
Either add explicit cleanup at line 387 when BLE_ERR_UNK_CONN_ID be detected (check deleteOnDisconnect and clean up like the GAP handler does), or ensure NimBLEDevice::deleteClient() handles the DISCONNECTED state directly without relying on the event path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NimBLEClient.cpp` around lines 381 - 387, When ble_gap_terminate(...)
returns BLE_ERR_UNK_CONN_ID the code sets m_connStatus to DISCONNECTED without
firing the BLE_GAP_EVENT_DISCONNECT cleanup path, which can orphan clients when
deleteOnDisconnect is true; modify the post-terminate branch in NimBLEClient.cpp
(where BLE_ERR_UNK_CONN_ID is checked) to perform the same cleanup the GAP
disconnect handler does: detect the BLE_ERR_UNK_CONN_ID fast path, check
this->deleteOnDisconnect (and any relevant client flags), and invoke the same
cleanup sequence used by the GAP handler (or call the same helper used by that
handler) so delete-on-disconnect clients are freed immediately rather than
relying on an event. Ensure m_lastErr and m_connStatus are still set
appropriately and reuse existing cleanup logic to avoid duplication.
h2zero
force-pushed
the
bugfix/disconnect-unk-conn-id
branch
from
4e27a4f to
69cff9a
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/NimBLEClient.cpp (1)
381-387:⚠️ Potential issue | 🟠 MajorArrr,
BLE_ERR_UNK_CONN_IDstill leaves the client stuck inDISCONNECTING.Line 381 now treats
BLE_HS_HCI_ERR(BLE_ERR_UNK_CONN_ID)as success, but Line 387 still maps it toDISCONNECTING.connect()only acceptsDISCONNECTEDat Lines 261-264, so this path now depends on a later GAP disconnect event to restore state. If that event never arrives for an unknown handle, the client stays wedged and reconnects get rejected. I’d treatBLE_HS_HCI_ERR(BLE_ERR_UNK_CONN_ID)the same asBLE_HS_ENOTCONNin the local state machine.🏴☠️ Minimal state fix
- m_connStatus = rc == BLE_HS_ENOTCONN ? DISCONNECTED : DISCONNECTING; + m_connStatus = (rc == BLE_HS_ENOTCONN || rc == BLE_HS_HCI_ERR(BLE_ERR_UNK_CONN_ID)) + ? DISCONNECTED + : DISCONNECTING;#!/bin/bash set -euo pipefail client_file="$(fd -p 'NimBLEClient\.cpp$' src | head -n1)" device_file="$(fd -p 'NimBLEDevice\.cpp$' src | head -n1)" printf '\n== disconnect() success path ==\n' sed -n '379,388p' "$client_file" printf '\n== connect() precondition ==\n' sed -n '255,265p' "$client_file" printf '\n== all m_connStatus transitions in NimBLEClient.cpp ==\n' rg -n -C2 'm_connStatus\s*=' "$client_file" printf '\n== deleteClient / deleteOnDisconnect handling ==\n' rg -n -C3 'deleteClient\s*\(|deleteOnDisconnect|deleteOnConnectFail' "$device_file" "$client_file"Expected result: ye should see the unknown-connection success path landing in
DISCONNECTING,connect()only allowingDISCONNECTED, and whether any separate delete-on-disconnect cleanup exists outside this branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEClient.cpp` around lines 381 - 387, The code treats BLE_HS_HCI_ERR(BLE_ERR_UNK_CONN_ID) as a non-error return from ble_gap_terminate() but still sets m_connStatus to DISCONNECTING, which can leave the client wedged; update the disconnect() success-path in NimBLEClient.cpp so that when rc equals BLE_HS_ENOTCONN OR BLE_HS_HCI_ERR(BLE_ERR_UNK_CONN_ID) you set m_connStatus = DISCONNECTED (same as the ENOTCONN case) and preserve m_lastErr handling, so connect() (which checks for DISCONNECTED) will allow reconnection immediately.
🤖 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/NimBLEClient.cpp`:
- Around line 381-387: The code treats BLE_HS_HCI_ERR(BLE_ERR_UNK_CONN_ID) as a
non-error return from ble_gap_terminate() but still sets m_connStatus to
DISCONNECTING, which can leave the client wedged; update the disconnect()
success-path in NimBLEClient.cpp so that when rc equals BLE_HS_ENOTCONN OR
BLE_HS_HCI_ERR(BLE_ERR_UNK_CONN_ID) you set m_connStatus = DISCONNECTED (same as
the ENOTCONN case) and preserve m_lastErr handling, so connect() (which checks
for DISCONNECTED) will allow reconnection immediately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bd099957-ae8e-4cd2-891c-8f6cd7eaf01c
📒 Files selected for processing (2)
src/NimBLEClient.cppsrc/NimBLEServer.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/NimBLEServer.cpp
* Update macro use to use conversion macros.
h2zero
force-pushed
the
bugfix/disconnect-unk-conn-id
branch
from
69cff9a to
5f8a1be
Compare
1 participant
Summary by CodeRabbit