[Bugfix] Incorrect Client connection state tracking and self delete#1123
[Bugfix] Incorrect Client connection state tracking and self delete#1123
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:
📝 WalkthroughWalkthroughArrr — This PR adds a private Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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
🤖 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 297-303: The error path calls NimBLEDevice::deleteClient(this)
while later accessing member fields, causing a use-after-free; change the order
so you update m_connStatus (set to DISCONNECTED) and m_lastErr = rc before
potentially calling NimBLEDevice::deleteClient(this), or alternatively defer
calling deleteClient until after you've stopped accessing any members (e.g.,
capture needed state or set a local flag and call deleteClient after leaving the
method). Ensure this change is applied to the error label and any early return
paths where m_config.deleteOnConnectFail is checked (references:
m_config.deleteOnConnectFail, NimBLEDevice::deleteClient, m_connStatus,
m_lastErr, CONNECTING/DISCONNECTED).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7834cec-9b49-4daa-a144-46a61da36c29
📒 Files selected for processing (2)
src/NimBLEClient.cppsrc/NimBLEClient.h
h2zero
force-pushed
the
bugfix/client-connection-state
branch
from
2eaabc9 to
814842e
Compare
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 1266-1268: The current change makes NimBLEClient::isConnected()
return true for CONNECTING and DISCONNECTING too; restore the original semantics
by making isConnected() return m_connStatus == CONNECTED only, and add a new
helper (e.g., isConnectingOrConnected() or isConnectedOrInTransition()) that
returns m_connStatus != DISCONNECTED; then update call sites that need the
broader “not disconnected” semantics (search for uses in NimBLEScan,
getConnectedClients, getDisconnectedClient, deleteClient, NimBLEL2CAPChannel,
getRssi, retrieveServices) to call the new helper while leaving callers that
require a stable connected state using isConnected().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 255181ed-ab76-4138-a632-dae28899f516
📒 Files selected for processing (2)
src/NimBLEClient.cppsrc/NimBLEClient.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/NimBLEClient.h
h2zero
force-pushed
the
bugfix/client-connection-state
branch
from
814842e to
d049396
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4f1645c1-d23c-451e-ae0f-35a18fe27262
📒 Files selected for processing (3)
src/NimBLEClient.cppsrc/NimBLEClient.hsrc/NimBLEDevice.cpp
h2zero
force-pushed
the
bugfix/client-connection-state
branch
from
d049396 to
e3dd9fe
Compare
There was a problem hiding this comment.
Pull request overview
This PR improves central-role client lifecycle/state tracking in the NimBLE C++ layer so APIs that look for “disconnected” clients don’t accidentally return clients that are mid-connect, and so self-delete on connection failures is handled more consistently.
Changes:
- Introduces an explicit
ConnStatusstate machine inNimBLEClientand uses it to gate “connected/disconnected/connecting/disconnecting” behavior. - Updates
NimBLEDeviceclient selection/deletion logic to rely on the new connection status rather thanisConnected()/task state. - Normalizes some preprocessor indentation in
NimBLEDevice.cpp.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/NimBLEDevice.cpp | Uses m_connStatus for delete/disconnected-client selection and minor preprocessor formatting tweaks. |
| src/NimBLEClient.h | Adds ConnStatus enum and m_connStatus field for explicit connection state tracking. |
| src/NimBLEClient.cpp | Implements state transitions across connect/disconnect/GAP events and updates connection-dependent checks accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
h2zero
force-pushed
the
bugfix/client-connection-state
branch
from
e3dd9fe to
20eb2cc
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
20eb2cc to
2149162
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/NimBLEClient.cpp (1)
1356-1356: Avast! There be a stray#floatin' in the code, ye scallywag!This lone
#on line 1356 be a valid "null directive" in C/C++ (does nothin'), but it looks like debris from a previous edit. Should be swabbed off the deck fer cleanliness!🧹 Proposed fix to clean up the debris
-#🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEClient.cpp` at line 1356, Remove the stray null-preprocessor directive ('#') introduced in NimBLEClient.cpp (a lone '#' token with no directive) by deleting that line; ensure the dangling '#' is not left in any function or class scope (e.g., near any NimBLEClient-related functions) and rebuild to verify there are no remaining stray preprocessor tokens or compile warnings.
🤖 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`:
- Line 1356: Remove the stray null-preprocessor directive ('#') introduced in
NimBLEClient.cpp (a lone '#' token with no directive) by deleting that line;
ensure the dangling '#' is not left in any function or class scope (e.g., near
any NimBLEClient-related functions) and rebuild to verify there are no remaining
stray preprocessor tokens or compile warnings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 11fb8dce-823e-4144-8671-2f9b8eb629c2
📒 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/NimBLEDevice.cpp
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
Summary by CodeRabbit