[Feat] Wait for connections to establish before callback, retry on fail#413
Merged
[Feat] Wait for connections to establish before callback, retry on fail#413
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 — Adds a delayed “connect established” stage with per-client timer and pending flag, extracts GAP connect into Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Client as NimBLEClient
participant Stack as BLE_Stack
participant Timer as ConnectEstTimer
App->>Client: connect(address,...)
activate Client
Client->>Client: reset pending & retry state
Client->>Stack: startConnectionAttempt()
deactivate Client
Stack->>Client: BLE_GAP_EVENT_CONNECT (success)
activate Client
Client->>Client: set m_connectCallbackPending = true
Client->>Timer: startConnectEstablishedTimer(conn_itvl)
Client->>Stack: exchangeMTU() (fire-and-forget)
deactivate Client
Stack->>Client: subsequent GAP events (notify/mtu/encrypt/identity/phy/others)
activate Client
Client->>Client: completeConnectEstablished()
Client->>App: onConnect(this)
Client->>Timer: stop timer
deactivate Client
alt Disconnect with establishment-fail (0x3e)
Stack->>Client: BLE_GAP_EVENT_DISCONNECT (0x3e)
activate Client
alt retries left
Client->>Client: increment retry count
Client->>Stack: startConnectionAttempt() (retry)
else
Client->>App: onConnectFail()/onDisconnect
end
deactivate Client
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 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.
🧹 Nitpick comments (1)
src/NimBLEClient.cpp (1)
72-73: Arr, a wee formatting barnacle on the hull!Matey, line 73 be havin' inconsistent indentation - it starts with 4 spaces instead of 6 spaces like the other member initializers. Not a functional issue, but it be lookin' a bit scruffy.
🧹 Proposed fix for alignment
m_connStatus{DISCONNECTED}, m_connectCallbackPending{false}, - m_connectFailRetryCount{0}, + m_connectFailRetryCount{0}, # if MYNEWT_VAL(BLE_EXT_ADV)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEClient.cpp` around lines 72 - 73, The member initializer list in NimBLEClient.cpp has inconsistent indentation: m_connectCallbackPending is indented with 6 spaces but m_connectFailRetryCount starts with 4, making the initializer list misaligned; update the whitespace so m_connectFailRetryCount aligns with the other initializers in the constructor initializer list (adjust indentation to match m_connectCallbackPending and the surrounding initializers).
🤖 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 72-73: The member initializer list in NimBLEClient.cpp has
inconsistent indentation: m_connectCallbackPending is indented with 6 spaces but
m_connectFailRetryCount starts with 4, making the initializer list misaligned;
update the whitespace so m_connectFailRetryCount aligns with the other
initializers in the constructor initializer list (adjust indentation to match
m_connectCallbackPending and the surrounding initializers).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 264a0adc-72f6-4813-a616-f23b74a59e20
📒 Files selected for processing (2)
src/NimBLEClient.cppsrc/NimBLEClient.h
h2zero
force-pushed
the
feat/conn-reattempt
branch
from
98c5653 to
cf5ac86
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.h (1)
104-111:⚠️ Potential issue | 🟡 MinorAye, the retry default should anchor itself to the
Configstruct, not just the constructor, ye scallywag!Line 110 currently leaves
connectFailRetryCountwithout a default member initializer. While the constructor seedsm_config.connectFailRetryCount = 1(line 85), a caller who builds a freshConfig{}and passes it tosetConfig()will hand over a zero retry budget, silently disabling retries. The fix is to add a default member initializer so the default survives any way the struct is constructed:struct Config { uint8_t deleteCallbacks : 1; // Delete the callback object when the client is deleted. uint8_t deleteOnDisconnect : 1; // Delete the client when disconnected. uint8_t deleteOnConnectFail : 1; // Delete the client when a connection attempt fails. uint8_t asyncConnect : 1; // Connect asynchronously. uint8_t exchangeMTU : 1; // Exchange MTU after connection. - uint8_t connectFailRetryCount; // Number of retries for 0x3e (connection establishment) failures. + uint8_t connectFailRetryCount = 1; // Number of retries for 0x3e failures; 0 disables retries. };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEClient.h` around lines 104 - 111, The Config struct's connectFailRetryCount lacks a default member initializer so constructing Config{} yields zero retries; update the struct definition (struct Config) to give connectFailRetryCount a default value (e.g., = 1) so any caller who creates a Config or passes it to setConfig() gets the intended default retry count and you no longer rely solely on m_config being set in the NimBLEClient constructor.
🤖 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.h`:
- Around line 104-111: The Config struct's connectFailRetryCount lacks a default
member initializer so constructing Config{} yields zero retries; update the
struct definition (struct Config) to give connectFailRetryCount a default value
(e.g., = 1) so any caller who creates a Config or passes it to setConfig() gets
the intended default retry count and you no longer rely solely on m_config being
set in the NimBLEClient constructor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a2dfddf2-9f2c-43e1-9675-84e28df9903c
📒 Files selected for processing (2)
src/NimBLEClient.cppsrc/NimBLEClient.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/NimBLEClient.cpp
h2zero
force-pushed
the
feat/conn-reattempt
branch
from
cf5ac86 to
7a51823
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.h`:
- Around line 127-129: An armed connect-established callout started by
startConnectEstablishedTimer is not being stopped on all terminal
BLE_GAP_EVENT_CONNECT paths, which can later invoke
connectEstablishedTimerCb/completeConnectEstablished on a stale this; update the
BLE_GAP_EVENT_CONNECT handling in NimBLEClient.cpp to call ble_npl_callout_stop
(the same call used on disconnect paths) before any early return/termination,
ensuring the callout is always stopped and that any pointer/state is nulled or
guarded to prevent callbacks after the client is torn down.
🪄 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: cb7355a6-be4a-4966-9463-531cea0b1d8f
📒 Files selected for processing (2)
src/NimBLEClient.cppsrc/NimBLEClient.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/NimBLEClient.cpp
h2zero
force-pushed
the
feat/conn-reattempt
branch
from
7a51823 to
4a138e3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/NimBLEClient.h (2)
104-110: Make the retry default opt-in, matey.Arr, Line 110 changes behavior for callers that never touch
Config: a0x3enow gets one hidden reattempt before the final failure path runs. That be a noticeable compatibility shift for synchronousconnect()and for apps timingonConnectFail(). I’d default this to0and let applications opt in if they want automatic retries.⚓ Proposed tweak
- uint8_t connectFailRetryCount{1}; // Number of retries for 0x3e (connection establishment) failures. + uint8_t connectFailRetryCount{0}; // Number of retries for 0x3e (connection establishment) failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEClient.h` around lines 104 - 110, The Config struct in NimBLEClient.h currently sets connectFailRetryCount{1}, which silently changes default behavior; change the default to 0 (connectFailRetryCount{0}) so retries are opt-in, update the inline comment to reflect "default 0, opt-in for automatic retries" and ensure any code that relies on an implicit retry (e.g., in connect() or onConnectFail() flows) continues to read this field so callers can opt-in explicitly via Config::connectFailRetryCount when they want retries.
124-134: Update the public callback docs for this new phase, matey.Arr, these helpers add a second “connected but not yet reported” state, but
NimBLEClientCallbacks::onConnect,onConnectFail, andonDisconnectstill read like the old immediate GAP-event flow. SincecompleteConnectEstablished()insrc/NimBLEClient.cpp:943-974now defersonConnect(), and terminal0x3eis routed throughonConnectFail(), the header should say that plainly so downstream users do not debug the wrong ordering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEClient.h` around lines 124 - 134, Update the public callback documentation in NimBLEClient.h to reflect the new “connected but not yet reported” phase introduced by completeConnectEstablished(): state that onConnect is deferred until completeConnectEstablished()/connectEstablishedTimerCb signals the connection is fully established, that terminal disconnect reason 0x3e (and similar terminal cases handled in handleGapEvent/completeConnectEstablished) will be reported via onConnectFail rather than a normal onConnect flow, and clarify ordering semantics for NimBLEClientCallbacks::onConnect, onConnectFail, and onDisconnect so downstream users understand the deferred-reporting and failure routing.
🤖 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.h`:
- Around line 104-110: The Config struct in NimBLEClient.h currently sets
connectFailRetryCount{1}, which silently changes default behavior; change the
default to 0 (connectFailRetryCount{0}) so retries are opt-in, update the inline
comment to reflect "default 0, opt-in for automatic retries" and ensure any code
that relies on an implicit retry (e.g., in connect() or onConnectFail() flows)
continues to read this field so callers can opt-in explicitly via
Config::connectFailRetryCount when they want retries.
- Around line 124-134: Update the public callback documentation in
NimBLEClient.h to reflect the new “connected but not yet reported” phase
introduced by completeConnectEstablished(): state that onConnect is deferred
until completeConnectEstablished()/connectEstablishedTimerCb signals the
connection is fully established, that terminal disconnect reason 0x3e (and
similar terminal cases handled in handleGapEvent/completeConnectEstablished)
will be reported via onConnectFail rather than a normal onConnect flow, and
clarify ordering semantics for NimBLEClientCallbacks::onConnect, onConnectFail,
and onDisconnect so downstream users understand the deferred-reporting and
failure routing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c83c501-0b35-4ea0-ab77-44af1dec208b
📒 Files selected for processing (2)
src/NimBLEClient.cppsrc/NimBLEClient.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/NimBLEClient.cpp
h2zero
force-pushed
the
feat/conn-reattempt
branch
2 times, most recently
from
531a15e to
df98835
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 958-969: completeConnectEstablished currently invokes
m_pClientCallbacks->onConnect inline while GAP handlers (callers that continue
to dereference pClient after this helper returns) may still be running; this can
cause use-after-free if onConnect disconnects or deletes the client. Change
completeConnectEstablished (and any similar helper) to not call onConnect
inline: clear m_connectCallbackPending, stop the connect timer, then schedule a
deferred invocation (e.g., use an existing ble_npl_callout or post a task/event)
that runs after the current GAP event unwinds to call
m_pClientCallbacks->onConnect(this) and perform
NimBLEUtils::taskRelease(*m_pTaskData,0) there; alternatively update all callers
that dereference pClient after completeConnectEstablished returns to avoid
touching pClient when it returns true. Ensure the deferred path cleans up the
same state and preserves the current semantics of m_connectCallbackPending.
- Around line 1045-1059: The blocking connect() wait can return early while
startConnectionAttempt()/delayed-establish retries are still in-flight; update
connect() to extend or restart its wait whenever a retry path is initiated
(detect when startConnectionAttempt(...) returns 0 or when you set m_connStatus
back to CONNECTING and m_connectFailRetryCount is incremented) so that it waits
for the final outcome signaled by completeConnectEstablished()/onConnect() or
the ultimate failure; implement this by resetting/recomputing the
condition-variable timeout (or setting a pending-retry flag) inside the retry
branch that currently returns 0, and ensure connect() checks that flag (or
inspects m_connectFailRetryCount vs m_config.connectFailRetries and
BLE_HS_CONN_HANDLE_NONE) to block until the retry sequence and the post-connect
grace window complete.
🪄 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: 234fccd5-824b-4d66-8d06-09f0aa9d1c08
📒 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
feat/conn-reattempt
branch
from
df98835 to
cc8486e
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 991-998: The m_config.connectFailRetries field is only 3 bits
(0–7) but setConnectRetries currently allows 8, which will overflow; change the
clamp to cap at 7 (use std::min<uint8_t>(numRetries, 7U)) inside
NimBLEClient::setConnectRetries and update the function comment to state "Max is
7, Default is 2" so documentation matches the actual storage limit; ensure you
reference/set m_config.connectFailRetries only after clamping.
🪄 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: 7e6bf5a3-cd01-490a-9163-609fc0520132
📒 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
feat/conn-reattempt
branch
from
cc8486e to
7ad0dfc
Compare
h2zero
changed the title
h2zero
changed the title
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 (2)
src/NimBLEClient.cpp (2)
1073-1089:⚠️ Potential issue | 🟠 MajorArr, the new 0x3e connect-fail path skips
deleteOnConnectFailfor async callers.Lines 1073-1077 now surface terminal 0x3e as
onConnectFail, but Lines 1081-1089 only self-delete offdeleteOnDisconnect. In async mode (pTaskData == nullptr),setSelfDelete(false, true)no longer works for this failure path.🛠️ Proposed fix
- if (pClient->m_config.deleteOnDisconnect) { + const bool shouldDelete = pClient->m_config.deleteOnDisconnect || + (rc == connEstablishFailReason && pClient->m_config.deleteOnConnectFail); + if (shouldDelete) { // If we are set to self delete on disconnect but we have a task waiting on the connection // completion we will set the flag to delete on connect fail instead of deleting here // to prevent segmentation faults or double deleting if (pTaskData != nullptr && rc == connEstablishFailReason) { pClient->m_config.deleteOnConnectFail = true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEClient.cpp` around lines 1073 - 1089, The connect-fail path (rc == connEstablishFailReason) triggers onConnectFail but the subsequent self-delete logic only checks m_config.deleteOnDisconnect, so async callers (pTaskData == nullptr) can't use setSelfDelete(false, true); update the post-disconnect handling in NimBLEClient.cpp to honor m_config.deleteOnConnectFail for the connEstablishFailReason case: if rc == connEstablishFailReason and pClient->m_config.deleteOnConnectFail is true and pTaskData is nullptr then call NimBLEDevice::deleteClient(pClient); keep the existing behavior that when pTaskData != nullptr and rc == connEstablishFailReason you set pClient->m_config.deleteOnConnectFail = true instead of deleting immediately.
1005-1008:⚠️ Potential issue | 🔴 CriticalArr, refresh the waiting task after rebinding the disconnect client.
Disconnect events are already special-cased because
argcan point at the wrong client.pTaskDatais copied before Lines 1015-1022 fix uppClient, so Lines 1396-1398 can release the wrong waiter—or never release the real blockingconnect()at all.🛠️ Proposed fix
if (pClient == nullptr) { NIMBLE_LOGE(LOG_TAG, "Disconnected client not found, conn_handle=%d", event->disconnect.conn.conn_handle); return 0; } + + pTaskData = pClient->m_pTaskData;Also applies to: 1015-1022
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEClient.cpp` around lines 1005 - 1008, The code copies pTaskData from pClient before the rebinding logic that can replace pClient when handling a disconnect, which can cause the wrong waiter to be signaled; in NimBLEClient::handleGapEvent adjust the order so pTaskData is refreshed after any rebinding/realignment of pClient (i.e., move or duplicate the assignment of NimBLETaskData* pTaskData = pClient->m_pTaskData to occur after the block that fixes up pClient or re-read pTaskData right before the code that releases the connect waiter), and ensure you null-check pClient and pTaskData before using them so the real blocking connect() waiter is always released for the correct client.
♻️ Duplicate comments (1)
src/NimBLEClient.cpp (1)
296-299:⚠️ Potential issue | 🔴 CriticalArr, this wait budget still only covers one connect attempt.
startConnectionAttempt()reusesm_connectTimeouton every 0x3e retry, but Lines 297-299 only budget onem_connectTimeout. With retries enabled, a blockingconnect()can time out and cancel a live retry, or hit the Line 301 success fallback beforecompleteConnectEstablished()fires.🛠️ Proposed fix
- // Wait for the connect timeout time +1 second +retry time for the connection to complete + const uint32_t attempts = static_cast<uint32_t>(m_config.connectFailRetries) + 1U; + const uint32_t establishWindowMs = connIntervalToMs(m_connParams.itvl_max) * 7U; + // Wait for all connection attempts plus the post-connect establishment window. if (!NimBLEUtils::taskWait( taskData, - m_connectTimeout + 1000 + connIntervalToMs(m_connParams.itvl_max) * 7 * m_config.connectFailRetries)) { + attempts * (static_cast<uint32_t>(m_connectTimeout) + establishWindowMs) + 1000U)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEClient.cpp` around lines 296 - 299, The wait in NimBLEUtils::taskWait only budgets a single m_connectTimeout so it can abort while startConnectionAttempt is performing multiple 0x3e retry connection attempts; update the computed timeout passed to NimBLEUtils::taskWait to include the retry budget (e.g., multiply or add m_connectTimeout for each retry in m_config.connectFailRetries) in addition to the existing connIntervalToMs(m_connParams.itvl_max) * 7 * m_config.connectFailRetries and +1000 so the wait covers every connect attempt and only falls back to the Line 301 success case after completeConnectEstablished can fire.
🤖 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.h`:
- Line 111: The default retry count for the new 0x3e flow is wrong: change the
initializer for the bitfield connectFailRetries in class/struct NimBLEClient
from 2 to 1 so callers that never call setConnectRetries() get a single retry as
intended; update any adjacent comment to reflect "1 retry" if present and ensure
setConnectRetries() still overrides this default behavior.
---
Outside diff comments:
In `@src/NimBLEClient.cpp`:
- Around line 1073-1089: The connect-fail path (rc == connEstablishFailReason)
triggers onConnectFail but the subsequent self-delete logic only checks
m_config.deleteOnDisconnect, so async callers (pTaskData == nullptr) can't use
setSelfDelete(false, true); update the post-disconnect handling in
NimBLEClient.cpp to honor m_config.deleteOnConnectFail for the
connEstablishFailReason case: if rc == connEstablishFailReason and
pClient->m_config.deleteOnConnectFail is true and pTaskData is nullptr then call
NimBLEDevice::deleteClient(pClient); keep the existing behavior that when
pTaskData != nullptr and rc == connEstablishFailReason you set
pClient->m_config.deleteOnConnectFail = true instead of deleting immediately.
- Around line 1005-1008: The code copies pTaskData from pClient before the
rebinding logic that can replace pClient when handling a disconnect, which can
cause the wrong waiter to be signaled; in NimBLEClient::handleGapEvent adjust
the order so pTaskData is refreshed after any rebinding/realignment of pClient
(i.e., move or duplicate the assignment of NimBLETaskData* pTaskData =
pClient->m_pTaskData to occur after the block that fixes up pClient or re-read
pTaskData right before the code that releases the connect waiter), and ensure
you null-check pClient and pTaskData before using them so the real blocking
connect() waiter is always released for the correct client.
---
Duplicate comments:
In `@src/NimBLEClient.cpp`:
- Around line 296-299: The wait in NimBLEUtils::taskWait only budgets a single
m_connectTimeout so it can abort while startConnectionAttempt is performing
multiple 0x3e retry connection attempts; update the computed timeout passed to
NimBLEUtils::taskWait to include the retry budget (e.g., multiply or add
m_connectTimeout for each retry in m_config.connectFailRetries) in addition to
the existing connIntervalToMs(m_connParams.itvl_max) * 7 *
m_config.connectFailRetries and +1000 so the wait covers every connect attempt
and only falls back to the Line 301 success case after
completeConnectEstablished can fire.
🪄 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: ff5bd3f7-2f27-4e1b-a74f-2cda7512feda
📒 Files selected for processing (2)
src/NimBLEClient.cppsrc/NimBLEClient.h
h2zero
force-pushed
the
feat/conn-reattempt
branch
3 times, most recently
from
0b8a4eb to
c6901eb
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/NimBLEClient.h (1)
166-168: Optional hardening: initialize new state fields in-class too.Arr matey, constructor init is present, but in-class defaults would guard future constructor additions from accidental uninitialized state.
🛠️ Suggested refactor
- bool m_connectCallbackPending; - uint8_t m_connectFailRetryCount; + bool m_connectCallbackPending{false}; + uint8_t m_connectFailRetryCount{0};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEClient.h` around lines 166 - 168, Initialize the new member fields directly in-class to harden against future constructor changes: set m_connectEstablishedTimer to {} (already shown), and add explicit in-class defaults for m_connectCallbackPending (e.g., = false) and m_connectFailRetryCount (e.g., = 0) so the members m_connectEstablishedTimer, m_connectCallbackPending and m_connectFailRetryCount in the NimBLEClient class are always initialized even if constructors change.
🤖 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.h`:
- Around line 166-168: Initialize the new member fields directly in-class to
harden against future constructor changes: set m_connectEstablishedTimer to {}
(already shown), and add explicit in-class defaults for m_connectCallbackPending
(e.g., = false) and m_connectFailRetryCount (e.g., = 0) so the members
m_connectEstablishedTimer, m_connectCallbackPending and m_connectFailRetryCount
in the NimBLEClient class are always initialized even if constructors change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d13f98c-509f-4346-9748-30b08608fe59
📒 Files selected for processing (2)
src/NimBLEClient.cppsrc/NimBLEClient.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/NimBLEClient.cpp
* Delay onConnect until connection is likely established using a timer (7x conn interval), with early completion on peer activity. * Suppress onDisconnect for 0x3e and route terminal 0x3e failures to onConnectFail. * Add configurable 0x3e reconnect attempts via `setConnectRetries` or client config (default 2). * Retry logic suppresses callbacks while retrying; callbacks fire only on final outcome. * Refactor connect-start path into a shared helper used by initial connect and retries.
h2zero
force-pushed
the
feat/conn-reattempt
branch
from
c6901eb to
882ff11
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/NimBLEClient.cpp (1)
954-958: Arr, yer defensive codin' be adequate but could use a wee polish!The
ble_npl_time_ms_to_ticksreturn code ain't bein' checked, matey. While yer initialization ofwaitTicks = 1and the subsequent zero-check provide a safe fallback, checkin' the return code would be more shipshape for diagnosing any conversion failures in rough seas.🔧 Optional: Check conversion return code
ble_npl_time_t waitTicks = 1; - ble_npl_time_ms_to_ticks(waitMs, &waitTicks); + if (ble_npl_time_ms_to_ticks(waitMs, &waitTicks) != 0) { + NIMBLE_LOGW(LOG_TAG, "Failed to convert %lu ms to ticks, using 1 tick", (unsigned long)waitMs); + waitTicks = 1; + } if (waitTicks == 0) { waitTicks = 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEClient.cpp` around lines 954 - 958, The code calls ble_npl_time_ms_to_ticks(waitMs, &waitTicks) but doesn't check its return value; update the call site to capture the function's return code, validate it (e.g., on error fall back to a safe value like 1), and log or propagate the error as appropriate; specifically modify the block around ble_npl_time_ms_to_ticks to store its return, check for non-success, set waitTicks = 1 if conversion failed or returned 0, and emit an error/debug message referencing waitMs and the failing return code so failures are diagnosable.
🤖 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 954-958: The code calls ble_npl_time_ms_to_ticks(waitMs,
&waitTicks) but doesn't check its return value; update the call site to capture
the function's return code, validate it (e.g., on error fall back to a safe
value like 1), and log or propagate the error as appropriate; specifically
modify the block around ble_npl_time_ms_to_ticks to store its return, check for
non-success, set waitTicks = 1 if conversion failed or returned 0, and emit an
error/debug message referencing waitMs and the failing return code so failures
are diagnosable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df133a01-4422-41d5-a5cc-0241ca63e8fd
📒 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
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.
setConnectRetriesor client config (default 2).Summary by CodeRabbit
New Features
Bug Fixes