[Bugfix] Incorrect Client connection state tracking and self delete#411
[Bugfix] Incorrect Client connection state tracking and self delete#411
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, added a private Changes
Sequence Diagram(s)sequenceDiagram
participant App as "App"
participant Device as "NimBLEDevice"
participant Client as "NimBLEClient"
participant GAP as "BLE GAP/Controller"
App->>Device: request connect client
Device->>Client: connect(params)
Client->>Client: set m_connStatus = CONNECTING
Client->>GAP: gap_connect()
GAP-->>Client: BLE_GAP_EVENT_CONNECT / BLE_GAP_EVENT_DISCONNECT / error
alt BLE_GAP_EVENT_CONNECT (success)
Client->>Client: set m_connStatus = CONNECTED\nstore m_connHandle
Client-->>Device: notify connected
else BLE_GAP_EVENT_CONNECT (failure) or error
Client->>Client: set m_connStatus = DISCONNECTED\nclear m_connHandle
Client-->>Device: notify failed
end
App->>Client: operations (getRssi, retrieveServices, setPeerAddress)
Client->>Client: gate ops on m_connStatus (e.g., require CONNECTED)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
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/NimBLEClient.cpp (1)
355-364:⚠️ Potential issue | 🟠 MajorDon’t strand
BLE_HS_ENOTCONNclients inDISCONNECTING.Arrr, if
ble_gap_terminate()reportsBLE_HS_ENOTCONN, there may be no follow-up GAP event to move the state again. That leaves a fully disconnected client stuck as busy and can block laterconnect()orsetPeerAddress()calls. Keepm_connStatusasDISCONNECTEDfor that branch.🏴☠️ Suggested fix
- m_connStatus = DISCONNECTING; + m_connStatus = (rc == BLE_HS_ENOTCONN) ? DISCONNECTED : DISCONNECTING;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEClient.cpp` around lines 355 - 364, The disconnect path in NimBLEClient::disconnect() leaves m_connStatus as DISCONNECTING when ble_gap_terminate() returns BLE_HS_ENOTCONN, which can strand a client; change the logic so that when rc == BLE_HS_ENOTCONN you set m_connStatus = DISCONNECTED (and still record m_lastErr as appropriate) instead of DISCONNECTING, while preserving the existing logging and behavior for other error codes (keep handling for BLE_HS_EALREADY and non-zero rc unchanged).
🤖 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 1266-1267: Change NimBLEClient::isConnected() to return true only
when m_connStatus == CONNECTED (not CONNECTING) to ensure established links
only; add a separate predicate (e.g., NimBLEClient::isInUse() or isActive())
that implements the previous “not DISCONNECTED” semantics (or checks
m_connHandle != BLE_HS_CONN_HANDLE_NONE) for reuse/disconnected-client logic,
and update call sites that relied on the old behavior (notably the
BLE_GAP_EVENT_CONNECT handling and the synchronous wait path) to use the new
isInUse()/m_connHandle check where appropriate while leaving event/connection
logic to use the stricter isConnected().
---
Outside diff comments:
In `@src/NimBLEClient.cpp`:
- Around line 355-364: The disconnect path in NimBLEClient::disconnect() leaves
m_connStatus as DISCONNECTING when ble_gap_terminate() returns BLE_HS_ENOTCONN,
which can strand a client; change the logic so that when rc == BLE_HS_ENOTCONN
you set m_connStatus = DISCONNECTED (and still record m_lastErr as appropriate)
instead of DISCONNECTING, while preserving the existing logging and behavior for
other error codes (keep handling for BLE_HS_EALREADY and non-zero rc unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c8e107fa-673d-4350-8007-ea86f1ea35e2
📒 Files selected for processing (2)
src/NimBLEClient.cppsrc/NimBLEClient.h
h2zero
force-pushed
the
bugfix/client-connection-state
branch
from
7f99a5d to
ab86717
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/NimBLEClient.cpp (1)
974-982:⚠️ Potential issue | 🟠 MajorArr, publish
DISCONNECTEDbefore firing the failure/disconnect callbacks.Right now
onConnectFail()/onDisconnect()still run while the client state says the link be alive. If callback code checksisConnected()or kicks off GATT work there, it can observe the stale pre-disconnect state. Move theDISCONNECTEDtransition before those callbacks so the new state machine stays truthful inside the callback too.🏴☠️ Minimal fix
pClient->m_terminateFailCount = 0; pClient->m_asyncSecureAttempt = 0; + pClient->m_connStatus = DISCONNECTED; // Don't call the disconnect callback if we are waiting for a connection to complete and it fails if (rc == (BLE_HS_ERR_HCI_BASE + BLE_ERR_CONN_ESTABLISHMENT) && pClient->m_config.asyncConnect) { pClient->m_pClientCallbacks->onConnectFail(pClient, rc); } else { pClient->m_pClientCallbacks->onDisconnect(pClient, rc); } - pClient->m_connStatus = DISCONNECTED; pClient->m_connHandle = BLE_HS_CONN_HANDLE_NONE;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEClient.cpp` around lines 974 - 982, Move the connection state update before invoking callbacks: set pClient->m_connStatus = DISCONNECTED and pClient->m_connHandle = BLE_HS_CONN_HANDLE_NONE prior to calling pClient->m_pClientCallbacks->onConnectFail(...) or onDisconnect(...). Specifically, in the block that checks rc == (BLE_HS_ERR_HCI_BASE + BLE_ERR_CONN_ESTABLISHMENT) && pClient->m_config.asyncConnect, update m_connStatus and m_connHandle first, then call onConnectFail; otherwise update them first and then call onDisconnect, so callbacks observe the true disconnected state.
♻️ Duplicate comments (1)
src/NimBLEClient.cpp (1)
185-189:⚠️ Potential issue | 🔴 CriticalArr, split “connected” from “in use.”
isConnected()now returns true forCONNECTINGandDISCONNECTING, but the guards here only blockCONNECTED/CONNECTING. That leaves aDISCONNECTINGclient reusable before its old GAP disconnect lands, so the late event can clobber the new attempt or miss the client entirely. KeepisConnected()strict toCONNECTED, and gate reuse / peer-address mutation onm_connStatus != DISCONNECTED(or a dedicated helper).🏴☠️ Local fix
- if (m_connStatus == CONNECTED || m_connStatus == CONNECTING) { + if (m_connStatus != DISCONNECTED) { NIMBLE_LOGE(LOG_TAG, "Client already connected/connecting"); rc = BLE_HS_EREJECT; goto error; } ... - if (m_connStatus == CONNECTED || m_connStatus == CONNECTING) { + if (m_connStatus != DISCONNECTED) { NIMBLE_LOGE(LOG_TAG, "Cannot set peer address while connected/connecting"); return false; } ... bool NimBLEClient::isConnected() const { - return m_connStatus != DISCONNECTED; + return m_connStatus == CONNECTED; }Also applies to: 580-582, 1265-1266
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEClient.cpp` around lines 185 - 189, The guard that blocks reuse/mutation currently checks CONNECTED/CONNECTING but must also block DISCONNECTING; update the conditional around m_connStatus (the block that logs "Client already connected/connecting" and returns BLE_HS_EREJECT) to treat any non-DISCONNECTED state as in-use (i.e., reject when m_connStatus != DISCONNECTED) and ensure isConnected() remains strict (return true only for CONNECTED); apply the same fix to the other similar guards that check CONNECTED/CONNECTING (all uses that gate reuse or peer-address mutation) so DISCONNECTING is treated as in-use too.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/NimBLEClient.cpp`:
- Around line 974-982: Move the connection state update before invoking
callbacks: set pClient->m_connStatus = DISCONNECTED and pClient->m_connHandle =
BLE_HS_CONN_HANDLE_NONE prior to calling
pClient->m_pClientCallbacks->onConnectFail(...) or onDisconnect(...).
Specifically, in the block that checks rc == (BLE_HS_ERR_HCI_BASE +
BLE_ERR_CONN_ESTABLISHMENT) && pClient->m_config.asyncConnect, update
m_connStatus and m_connHandle first, then call onConnectFail; otherwise update
them first and then call onDisconnect, so callbacks observe the true
disconnected state.
---
Duplicate comments:
In `@src/NimBLEClient.cpp`:
- Around line 185-189: The guard that blocks reuse/mutation currently checks
CONNECTED/CONNECTING but must also block DISCONNECTING; update the conditional
around m_connStatus (the block that logs "Client already connected/connecting"
and returns BLE_HS_EREJECT) to treat any non-DISCONNECTED state as in-use (i.e.,
reject when m_connStatus != DISCONNECTED) and ensure isConnected() remains
strict (return true only for CONNECTED); apply the same fix to the other similar
guards that check CONNECTED/CONNECTING (all uses that gate reuse or peer-address
mutation) so DISCONNECTING is treated as in-use too.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a5440372-da29-4749-977b-d7f654f814dd
📒 Files selected for processing (2)
src/NimBLEClient.cppsrc/NimBLEClient.h
✅ Files skipped from review due to trivial changes (1)
- src/NimBLEClient.h
h2zero
force-pushed
the
bugfix/client-connection-state
branch
from
ab86717 to
63dcdf2
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/NimBLEDevice.cpp (1)
362-374:⚠️ Potential issue | 🟠 MajorArrr, keep
DISCONNECTINGon the deferred-delete path.Once
disconnect()succeeds,NimBLEClient::disconnect()moves the client toDISCONNECTINGbefore the GAP disconnect event lands. In that window, Line 367 skips both guarded branches and drops into the rawdelete, which can remove the client fromm_pClientsbefore the disconnect-completion path runs. TreatDISCONNECTINGthe same asCONNECTED: armdeleteOnDisconnect, but only calldisconnect()if the state is stillCONNECTED.🏴☠️ Proposed fix
- if (clt->m_connStatus == NimBLEClient::CONNECTED) { + if (clt->m_connStatus == NimBLEClient::CONNECTED || + clt->m_connStatus == NimBLEClient::DISCONNECTING) { clt->m_config.deleteOnDisconnect = true; - if (!clt->disconnect()) { + if (clt->m_connStatus == NimBLEClient::CONNECTED && !clt->disconnect()) { break; } } else if (pClient->m_connStatus == NimBLEClient::CONNECTING) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEDevice.cpp` around lines 362 - 374, The current removal path deletes a NimBLEClient prematurely because DISCONNECTING falls through to raw delete; change the logic in the block that checks clt->m_connStatus so DISCONNECTING is treated like CONNECTED by setting clt->m_config.deleteOnDisconnect = true, but only invoke clt->disconnect() when clt->m_connStatus is actually CONNECTED (do not call disconnect() when already DISCONNECTING); keep the existing CONNECTING branch (setting deleteOnConnectFail and calling cancelConnect()) and only delete the object in the final else if neither CONNECTED/DISCONNECTING nor CONNECTING.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/NimBLEDevice.cpp`:
- Around line 362-374: The current removal path deletes a NimBLEClient
prematurely because DISCONNECTING falls through to raw delete; change the logic
in the block that checks clt->m_connStatus so DISCONNECTING is treated like
CONNECTED by setting clt->m_config.deleteOnDisconnect = true, but only invoke
clt->disconnect() when clt->m_connStatus is actually CONNECTED (do not call
disconnect() when already DISCONNECTING); keep the existing CONNECTING branch
(setting deleteOnConnectFail and calling cancelConnect()) and only delete the
object in the final else if neither CONNECTED/DISCONNECTING nor CONNECTING.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 49242007-61f1-4261-ac49-1219a6315540
📒 Files selected for processing (3)
src/NimBLEClient.cppsrc/NimBLEClient.hsrc/NimBLEDevice.cpp
✅ Files skipped from review due to trivial changes (1)
- src/NimBLEClient.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/NimBLEClient.cpp
h2zero
force-pushed
the
bugfix/client-connection-state
branch
2 times, most recently
from
4f75a21 to
8aecbbf
Compare
This adds better client state tracking so that functions like NimBLEDevice::getDisconnectedClient get a more accurate state and will not return a connecting client. This also fixes the client self delete on connection error where function call errors did not delete the client
h2zero
force-pushed
the
bugfix/client-connection-state
branch
from
8aecbbf to
c06b3cb
Compare
1 participant
This adds better client state tracking so that functions like NimBLEDevice::getDisconnectedClient get a more accurate state and will not return a connecting client.
This also fixes the client self delete on connection error where function call errors did not delete the client
Fixes #335
Fixes #336
Summary by CodeRabbit
Bug Fixes
Refactor