fix(ble): serialize connection parameter updates - #8
Merged
jeremiah-k merged 3 commits intoAug 29, 2026
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:
WalkthroughAdds a generation-aware BLE connection-parameter controller, integrates it with NimBLE callbacks and configuration flow, handles retries and session resets, and adds comprehensive controller tests. ChangesBLE connection parameter orchestration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant NimBLECallback
participant BluetoothPhoneAPI
participant ConnectionParamsUpdateController
participant BLEServer
NimBLECallback->>BluetoothPhoneAPI: Report connection
BluetoothPhoneAPI->>ConnectionParamsUpdateController: Admit connection and obtain generation
BluetoothPhoneAPI->>ConnectionParamsUpdateController: Queue requested mode
BluetoothPhoneAPI->>ConnectionParamsUpdateController: Service pending request
ConnectionParamsUpdateController-->>BluetoothPhoneAPI: Return connection-parameter request
BluetoothPhoneAPI->>BLEServer: Submit requestConnParams
NimBLECallback->>BluetoothPhoneAPI: Report update completion
BluetoothPhoneAPI->>ConnectionParamsUpdateController: Record interval and status
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jeremiah-k
force-pushed
the
fix/nimble-connection-parameter-serialization
branch
2 times, most recently
from
July 27, 2026 23:25
65b0999 to
500e3e7
Compare
jeremiah-k
force-pushed
the
fix/nimble-connection-parameter-serialization-review
branch
from
July 27, 2026 23:28
437bbc5 to
dafa583
Compare
jeremiah-k
force-pushed
the
fix/nimble-connection-parameter-serialization
branch
2 times, most recently
from
July 28, 2026 01:28
914ac5d to
d30c67e
Compare
![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
🧹 Nitpick comments (2)
test/test_ble_connection_params/test_main.cpp (1)
72-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReused output parameter reduces readability.
retiredis reused as the output parameter for the unrelatedbeginNextcall at Line 86 after already serving as the original in-flight request snapshot. Harmless here since the call returnsfalse, but a dedicated scratch variable would make the test easier to follow.Separately, this test only covers the reused-handle completion case where both sessions request the same mode (
INITIAL_HIGH_THROUGHPUT); it doesn't cover a stale completion for a different mode than the new session's in-flight request, which is the scenario flagged insrc/nimble/ConnectionParamsUpdateController.h(Lines 80-93).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_ble_connection_params/test_main.cpp` around lines 72 - 92, Improve test_retired_session_rejects_stale_submission_and_wrong_handle_completion by using a dedicated scratch ConnectionParamsRequest for the final beginNext(false) check instead of overwriting retired. Configure retired and current with different update modes, using the existing request fields and mode constants, so the reused-handle completion exercises a stale completion whose mode differs from the new session’s in-flight request. Preserve the assertions that stale submissions and completions do not resurrect or incorrectly consume the retired request.src/nimble/NimbleBluetooth.cpp (1)
440-487: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated/diverging connection-parameter magic numbers between
INITIAL_HIGH_THROUGHPUTandrequestHighThroughputConnection.Line 469 inlines
requestConnParams(request.connHandle, 6, 12, 0, 200)— same min/max interval and latency asrequestHighThroughputConnection()(Line 509:6, 12, 0, 600), but with a different, undocumented supervision timeout (200 vs 600). Unlike the other two modes, this case isn't routed through a dedicated helper with the descriptive parameter-rationale comment block used elsewhere, so the discrepancy looks accidental rather than a deliberate design choice.Consider extracting a shared helper (parameterized by timeout, or with a documented reason for the 200 vs 600 difference) to avoid future drift between these two similar requests.
♻️ Suggested consolidation
- case ConnectionParamsMode::INITIAL_HIGH_THROUGHPUT: - LOG_INFO("BLE requestInitialHighThroughputConnection"); - accepted = bleServer->requestConnParams(request.connHandle, 6, 12, 0, 200); - break; + case ConnectionParamsMode::INITIAL_HIGH_THROUGHPUT: + // NOTE: uses a shorter supervision timeout than steady-state high-throughput mode + // to detect a bad initial connection faster. See requestHighThroughputConnection(). + accepted = requestInitialHighThroughputConnection(request.connHandle); + break;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nimble/NimbleBluetooth.cpp` around lines 440 - 487, Consolidate the duplicated connection-parameter values used by updateConnectionParams for INITIAL_HIGH_THROUGHPUT and requestHighThroughputConnection into a shared helper or shared parameter definition. Preserve the intended supervision-timeout difference only if deliberate, and document its rationale; otherwise use one consistent timeout. Keep the existing mode-specific behavior while preventing future drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/nimble/NimbleBluetooth.cpp`:
- Around line 211-216: Track the active connection-parameter update generation
when setting requestedMode and updateInProgress, and pass or validate it in
onConnectionParamsUpdate/onUpdateComplete. Reject completion callbacks whose
generation no longer matches the active request, mirroring the stale-generation
handling in onSubmissionRejected(), while preserving wakeForConnectionParams()
only for valid completions.
---
Nitpick comments:
In `@src/nimble/NimbleBluetooth.cpp`:
- Around line 440-487: Consolidate the duplicated connection-parameter values
used by updateConnectionParams for INITIAL_HIGH_THROUGHPUT and
requestHighThroughputConnection into a shared helper or shared parameter
definition. Preserve the intended supervision-timeout difference only if
deliberate, and document its rationale; otherwise use one consistent timeout.
Keep the existing mode-specific behavior while preventing future drift.
In `@test/test_ble_connection_params/test_main.cpp`:
- Around line 72-92: Improve
test_retired_session_rejects_stale_submission_and_wrong_handle_completion by
using a dedicated scratch ConnectionParamsRequest for the final beginNext(false)
check instead of overwriting retired. Configure retired and current with
different update modes, using the existing request fields and mode constants, so
the reused-handle completion exercises a stale completion whose mode differs
from the new session’s in-flight request. Preserve the assertions that stale
submissions and completions do not resurrect or incorrectly consume the retired
request.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c9cd24a0-3c50-4c8c-97b3-8b366af37a67
📒 Files selected for processing (4)
src/nimble/ConnectionParamsUpdateController.hsrc/nimble/NimbleBluetooth.cpptest/native-suite-counttest/test_ble_connection_params/test_main.cpp
jeremiah-k
force-pushed
the
fix/nimble-connection-parameter-serialization
branch
from
July 28, 2026 10:12
d30c67e to
77969c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/pr_enforce_labels.yml:
- Line 13: Remove the job-level `if: false` guard from the PR label enforcement
workflow, either deleting the disabled job/workflow if it is no longer needed or
replacing the guard with an explicit repository variable condition that can
re-enable enforcement.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 32a45417-2990-482d-af5f-b14847ef83c6
📒 Files selected for processing (7)
.coderabbit.yaml.github/workflows/pr_enforce_labels.yml.gitignoresrc/nimble/ConnectionParamsUpdateController.hsrc/nimble/NimbleBluetooth.cpptest/native-suite-counttest/test_ble_connection_params/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- test/native-suite-count
- src/nimble/ConnectionParamsUpdateController.h
- test/test_ble_connection_params/test_main.cpp
- src/nimble/NimbleBluetooth.cpp
jeremiah-k
force-pushed
the
fix/nimble-connection-parameter-serialization
branch
2 times, most recently
from
July 28, 2026 10:24
81125c5 to
baaaa62
Compare
jeremiah-k
force-pushed
the
fix/nimble-connection-parameter-serialization-review
branch
from
July 28, 2026 10:26
0fb8390 to
d2b11d5
Compare
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/nimble/NimbleBluetooth.cpp`:
- Around line 467-470: Extract the inline INITIAL_HIGH_THROUGHPUT request from
the connection-parameter handling into a documented
requestInitialHighThroughputConnection helper, matching the existing
high-throughput helpers. Preserve the parameters (6, 12, 0, 200) and document
that the 2-second supervision timeout intentionally detects initial-connection
failures faster than the regular 6-second mode; update the case to call this
helper.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 87a418a3-62f5-4d3c-9d51-d7ca273d5944
📒 Files selected for processing (4)
src/nimble/ConnectionParamsUpdateController.hsrc/nimble/NimbleBluetooth.cpptest/native-suite-counttest/test_ble_connection_params/test_main.cpp
jeremiah-k
force-pushed
the
fix/nimble-connection-parameter-serialization
branch
2 times, most recently
from
July 28, 2026 17:41
27e5a6e to
902345e
Compare
jeremiah-k
force-pushed
the
fix/nimble-connection-parameter-serialization-review
branch
from
July 28, 2026 17:44
d2b11d5 to
a8623a6
Compare
jeremiah-k
force-pushed
the
fix/nimble-connection-parameter-serialization
branch
from
July 28, 2026 19:09
902345e to
aa6f88f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/pr_enforce_labels.yml:
- Line 3: Update the workflow_dispatch configuration to require a pull request
number input, then modify the label-fetching logic to use that input and
retrieve the PR labels through the GitHub API instead of relying on
context.payload.pull_request. Preserve the existing required-label enforcement
behavior for manually dispatched runs.
In `@src/nimble/ConnectionParamsUpdateController.h`:
- Around line 46-60: Update request() so the satisfied-mode early return only
clears desiredMode when no different pending mode exists; preserve an existing
non-NONE desiredMode instead of overwriting it. Keep the current validation and
new-mode assignment behavior unchanged for requests that are not already
satisfied.
- Around line 106-119: Update onUpdateComplete() to release the update lane when
status != 0 by clearing updateInProgress and requestedMode before returning. If
immediate release is not valid, add a bounded Throttle-based staleness timeout
that clears the lane without accepting arbitrary peer-initiated events, while
preserving the existing success-path completion logic.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 008357ae-251f-4240-b644-746a0ae40c3c
📒 Files selected for processing (7)
.coderabbit.yaml.github/workflows/pr_enforce_labels.yml.gitignoresrc/nimble/ConnectionParamsUpdateController.hsrc/nimble/NimbleBluetooth.cpptest/native-suite-counttest/test_ble_connection_params/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- test/native-suite-count
jeremiah-k
force-pushed
the
fix/nimble-connection-parameter-serialization
branch
from
July 28, 2026 19:23
aa6f88f to
7a8b4b7
Compare
Owner
Author
|
@sourcery-ai review |
Reviewer's GuideIntroduces a serialized, generation-aware controller for BLE connection parameter updates on NimBLE/ESP32, routing all high-throughput/low-power mode switches through a single main-task lane and backing it with unit tests. Sequence diagram for serialized BLE connection-parameter updatessequenceDiagram
actor User
participant BluetoothPhoneAPI
participant ConnectionParamsUpdateController as Controller
participant NimbleBluetoothServerCallback as ServerCallback
participant BLEServer
User->>BluetoothPhoneAPI: onConfigStart()
BluetoothPhoneAPI->>BluetoothPhoneAPI: requestConnectionParams(HIGH_THROUGHPUT)
BluetoothPhoneAPI->>Controller: request(currentGeneration(), HIGH_THROUGHPUT)
loop main_task
BluetoothPhoneAPI->>BluetoothPhoneAPI: updateConnectionParams()
BluetoothPhoneAPI->>Controller: beginNext()
alt [request available]
BluetoothPhoneAPI->>BLEServer: requestConnParams(connHandle, ...)
alt [submission rejected]
BluetoothPhoneAPI->>Controller: onSubmissionRejected(request)
end
end
end
BLEServer-->>ServerCallback: onConnParamsUpdate(connHandle, interval, status)
ServerCallback->>BluetoothPhoneAPI: onConnectionParamsUpdate(connHandle, interval, status)
BluetoothPhoneAPI->>Controller: onUpdateComplete(connHandle, interval, status)
alt [Controller returns true]
BluetoothPhoneAPI->>BluetoothPhoneAPI: wakeForConnectionParams()
end
User->>BluetoothPhoneAPI: onConfigComplete()
BluetoothPhoneAPI->>BluetoothPhoneAPI: requestConnectionParams(LOW_POWER)
BluetoothPhoneAPI->>Controller: request(currentGeneration(), LOW_POWER)
note over BluetoothPhoneAPI,Controller: New request coalesces and waits until previous update completes
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The interval thresholds used in
ConnectionParamsUpdateController::isSatisfied()(e.g.,<= 12,>= 24) should be tied to shared named constants or derived from the actualrequestHighThroughputConnection/requestLowerPowerConnectionparameters to avoid drift if the connection parameter presets are changed later. - In
ConnectionParamsUpdateController::onUpdateComplete, whenstatus != 0you leaveupdateInProgressset and never release the lane, which can permanently block future requests after an asynchronous failure; consider explicitly clearingupdateInProgress(and perhapsrequestedMode) on non-zero status and deciding whether to re-queue or drop the desired mode.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The interval thresholds used in `ConnectionParamsUpdateController::isSatisfied()` (e.g., `<= 12`, `>= 24`) should be tied to shared named constants or derived from the actual `requestHighThroughputConnection`/`requestLowerPowerConnection` parameters to avoid drift if the connection parameter presets are changed later.
- In `ConnectionParamsUpdateController::onUpdateComplete`, when `status != 0` you leave `updateInProgress` set and never release the lane, which can permanently block future requests after an asynchronous failure; consider explicitly clearing `updateInProgress` (and perhaps `requestedMode`) on non-zero status and deciding whether to re-queue or drop the desired mode.
## Individual Comments
### Comment 1
<location path="src/nimble/ConnectionParamsUpdateController.h" line_range="99-108" />
<code_context>
+ * request token. Treating every same-handle callback as our completion can therefore release the lane on Android's own
+ * priority update and submit a second peripheral procedure while controller work is still active.
+ */
+ bool onUpdateComplete(uint16_t connHandle, uint16_t interval, uint8_t status)
+ {
+ std::lock_guard<std::mutex> guard(stateMutex);
+ if (connHandle != connectionHandle) {
+ return false;
+ }
+
+ if (status == 0) {
+ currentInterval = interval;
+ }
+ if (!updateInProgress || status != 0 || !isSatisfied(requestedMode, currentInterval)) {
+ return false;
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Async failures keep the controller lane permanently blocked
In the non-zero `status` path, we return without clearing `updateInProgress` or `requestedMode`, so a failed controller/peer update can leave the lane stuck “in progress” and block `beginNext()` from ever admitting new requests. Please handle the failure case explicitly by resetting `updateInProgress` (and likely `requestedMode`) and deciding whether `desiredMode` should remain pending for retry or be dropped, similar to `onSubmissionRejected()`.
</issue_to_address>
### Comment 2
<location path="test/test_ble_connection_params/test_main.cpp" line_range="152-21" />
<code_context>
+ TEST_ASSERT_FALSE(controller.onUpdateComplete(1, 12, 0));
+}
+
+static void test_retired_session_request_cannot_target_reconnect()
+{
+ ConnectionParamsUpdateController controller;
+ controller.onConnected(1);
+ const uint32_t retiredGeneration = controller.currentGeneration();
+
+ controller.reset();
+ controller.onConnected(1);
+
+ TEST_ASSERT_FALSE(controller.request(retiredGeneration, ConnectionParamsMode::LOW_POWER));
+ TEST_ASSERT_FALSE(controller.beginNext().has_value());
+}
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for `reset()` while an update is in progress to cover mid-flight retirement
The existing tests only cover retirement before a new submission cycle starts. Please also add coverage for calling `reset()` while `updateInProgress` is true: after `onConnected`, queue a mode and call `beginNext()` (so `updateInProgress = true`), then call `reset()`. Finally, invoke `onSubmissionRejected` / `onUpdateComplete` with the old `ConnectionParamsRequest` and assert they return `false` and neither re-open the lane nor emit new requests. This will lock in the behavior for in-flight requests from a retired session.
Suggested implementation:
```cpp
ConnectionParamsUpdateController controller;
controller.onConnected(1);
TEST_ASSERT_TRUE(controller.request(controller.currentGeneration(), ConnectionParamsMode::HIGH_THROUGHPUT));
TEST_ASSERT_TRUE(controller.beginNext().has_value());
TEST_ASSERT_FALSE(controller.onUpdateComplete(1, 12, 1));
TEST_ASSERT_FALSE(controller.beginNext().has_value());
TEST_ASSERT_FALSE(controller.onUpdateComplete(1, 12, 0));
}
static void test_reset_mid_flight_retires_in_flight_requests()
{
ConnectionParamsUpdateController controller;
// Start a session and enqueue a request
controller.onConnected(1);
const uint32_t generation = controller.currentGeneration();
TEST_ASSERT_TRUE(controller.request(generation, ConnectionParamsMode::HIGH_THROUGHPUT));
// Begin processing the next request so that an update is in progress
auto inFlightOpt = controller.beginNext();
TEST_ASSERT_TRUE(inFlightOpt.has_value());
const ConnectionParamsRequest inFlight = *inFlightOpt;
// Reset the controller while the update is in progress
controller.reset();
// Callbacks for the old, in-flight request from the retired session must be ignored
TEST_ASSERT_FALSE(controller.onSubmissionRejected(inFlight));
TEST_ASSERT_FALSE(controller.onUpdateComplete(inFlight));
// No new work should be emitted and the lane must remain closed
TEST_ASSERT_FALSE(controller.beginNext().has_value());
}
using meshtastic::bluetooth::ConnectionParamsMode;
using meshtastic::bluetooth::ConnectionParamsRequest;
using meshtastic::bluetooth::ConnectionParamsUpdateController;
```
The above edit assumes the following overloads exist:
- `bool ConnectionParamsUpdateController::onSubmissionRejected(const ConnectionParamsRequest &request);`
- `bool ConnectionParamsUpdateController::onUpdateComplete(const ConnectionParamsRequest &request);`
If, instead, `onSubmissionRejected` / `onUpdateComplete` use a different signature (e.g. individual fields such as handle, generation, interval, latency, status flags), you should:
1. Replace the calls:
- `controller.onSubmissionRejected(inFlight);`
- `controller.onUpdateComplete(inFlight);`
with calls that match your actual API, using the values from `inFlight` (and any other appropriate constants) to represent the “old” request.
2. If there is a separate observable side-effect for “re-opening the lane” (e.g. a callback, flag, or counter), add assertions after `reset()` / the callback invocations to confirm that side-effect does not occur, in addition to the `beginNext()` assertions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
jeremiah-k
force-pushed
the
fix/nimble-connection-parameter-serialization
branch
from
July 28, 2026 20:09
7a8b4b7 to
a5b1394
Compare
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/nimble/ConnectionParamsUpdateController.h`:
- Around line 112-134: Update onUpdateComplete() to handle status == 0 callbacks
that leave isSatisfied(requestedMode, currentInterval) false: clear
requestedMode and updateInProgress while preserving desiredMode, then release
the blocking Throttle-based timeout using the existing time-based fallback
pattern so beginNext() can retry.
- Around line 105-111: Shorten the documentation comment immediately preceding
onUpdateComplete to no more than two lines while retaining only the essential
rationale for recording the negotiated interval and avoiding premature request
release; remove the extended explanation and any wording that restates the
implementation.
In `@src/nimble/NimbleBluetooth.cpp`:
- Around line 456-475: The request admission and GAP submission in the
connection-parameter scheduling flow must be atomic with session lifecycle
changes. Replace the separate connectionParamsController.beginNext(), mode
dispatch, and onSubmissionRejected() sequence with a controller-owned submission
operation that validates the request and invokes
requestHighThroughputConnection() or requestLowerPowerConnection() while
synchronized against onConnected() and reset(), preventing stale handle reuse
and untracked in-flight updates; add a regression test covering
disconnect/reset/reconnect handle reuse.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 477dd298-75bf-4b6e-a144-0f4ff4841682
📒 Files selected for processing (4)
src/nimble/ConnectionParamsUpdateController.hsrc/nimble/NimbleBluetooth.cpptest/native-suite-counttest/test_ble_connection_params/test_main.cpp
jeremiah-k
force-pushed
the
fix/nimble-connection-parameter-serialization
branch
2 times, most recently
from
July 28, 2026 21:07
a93ebdb to
e166df8
Compare
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Changed-file structure =='
ast-grep outline src/nimble/NimbleBluetooth.cpp --items all --match 'BluetoothPhoneAPI|onDisconnect|onConnect|requestConnectionParams|resetBleSessionState|resetConnectionParams' || true
ast-grep outline src/nimble/ConnectionParamsUpdateController.h --items all --match 'ConnectionParamsUpdateController|recordUpdate|onConnected|request|reset' || true
echo '== Controller integration and generation captures =='
rg -n -C 6 --glob 'src/nimble/NimbleBluetooth.cpp' \
'onBleConnected\s*\(|onConnectionParamsUpdate\s*\(|resetConnectionParams\s*\(|resetBleSessionState\s*\(|requestConnectionParams\s*\(|connectionParamsGeneration|currentGeneration\s*\(' \
src/nimble/NimbleBluetooth.cpp || true
echo '== Controller callback/reset conditions =='
rg -n -C 6 --glob 'src/nimble/ConnectionParamsUpdateController.h' \
'void\s+(onConnected|recordUpdate|reset)\s*\(|uint32_t\s+currentGeneration\s*\(|bool\s+request\s*\(|generation|connectionHandle|status' \
src/nimble/ConnectionParamsUpdateController.h || true
echo '== PR diff for relevant paths =='
git diff --unified=20 HEAD^ HEAD -- src/nimble/NimbleBluetooth.cpp src/nimble/ConnectionParamsUpdateController.h || trueLength of output: 30422 🤖 web_search executed:
Length of output: 12004
Keeping successful matching-handle peer updates able to refresh 🐇🧠 ✏️ Learnings added
This chat is LLM-generated and may be inaccurate. |
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/nimble/ConnectionParamsUpdateController.h`:
- Around line 120-140: Remove the unreachable reset assignments to requestedMode
and stateStartedAtMs inside the waitMs != 0 branch, since both are overwritten
immediately by the subsequent update flow. Preserve the remaining throttle check
and the final requestedMode, updateInProgress, and stateStartedAtMs assignments
in the surrounding controller logic.
In `@test/test_ble_connection_params/test_main.cpp`:
- Around line 140-152: Update
test_latest_mode_already_satisfied_at_timeout_needs_no_submission so its final
serviceAt call occurs at or after the actual lane expiry: LOW_POWER_SETTLE_MS
plus UPDATE_LANE_TIMEOUT_MS, accounting for the timing used by the
HIGH_THROUGHPUT request. Keep the existing no-submission assertion, ensuring
execution reaches the expiry-time isSatisfied check rather than the
lane-still-active early return.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 65f8d2fc-8d65-43f1-ba48-effabdec3213
📒 Files selected for processing (5)
src/mesh/Throttle.hsrc/nimble/ConnectionParamsUpdateController.hsrc/nimble/NimbleBluetooth.cpptest/native-suite-counttest/test_ble_connection_params/test_main.cpp
jeremiah-k
force-pushed
the
fix/nimble-connection-parameter-serialization
branch
2 times, most recently
from
July 30, 2026 01:41
b185a90 to
43ae0e1
Compare
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
jeremiah-k
force-pushed
the
fix/nimble-connection-parameter-serialization
branch
from
July 30, 2026 11:18
43ae0e1 to
79522c6
Compare
jeremiah-k
force-pushed
the
fix/nimble-connection-parameter-serialization-review
branch
from
July 30, 2026 11:18
0066705 to
42b530d
Compare
jeremiah-k
force-pushed
the
fix/nimble-connection-parameter-serialization
branch
from
July 30, 2026 12:51
79522c6 to
49a7049
Compare
jeremiah-k
force-pushed
the
fix/nimble-connection-parameter-serialization-review
branch
from
July 30, 2026 19:47
42b530d to
505f330
Compare
jeremiah-k
force-pushed
the
fix/nimble-connection-parameter-serialization
branch
from
July 30, 2026 19:47
49a7049 to
5d005e7
Compare
jeremiah-k
force-pushed
the
fix/nimble-connection-parameter-serialization-review
branch
from
July 31, 2026 12:34
505f330 to
ee53b73
Compare
jeremiah-k
force-pushed
the
fix/nimble-connection-parameter-serialization
branch
7 times, most recently
from
July 31, 2026 21:02
dc65b34 to
a857cca
Compare
jeremiah-k
force-pushed
the
fix/nimble-connection-parameter-serialization-review
branch
from
August 1, 2026 13:03
ee53b73 to
09dadfc
Compare
jeremiah-k
force-pushed
the
fix/nimble-connection-parameter-serialization
branch
from
August 1, 2026 13:03
a857cca to
6c6b98e
Compare
Route initial, high-throughput, and low-power connection parameter changes through one controller lane. Coalesce the latest desired mode while an update is active and submit it only after NimBLE reports the previous procedure complete. Guard session transitions with one mutex and require callers to present the session generation when queueing a mode. A callback from a retired connection therefore cannot queue low-power parameters for a newly connected phone, even when ESP32 reuses the connection handle. Use fully qualified controller types at their call sites rather than file-local aliases. This keeps the commit self-contained when neighboring BLE fixes add constants to the same namespace block. This avoids overlapping GAP update requests during back-to-back configuration handshakes, which the ESP32 host rejects as already in progress and which preceded an ld_acl controller assertion in hardware logs. Reset all lane state with the BLE session and keep controller calls on the existing main-task path.
Treat peer-selected connection parameters as observed link state instead of assuming every successful callback belongs to the firmware request. Release the single GAP lane only when the interval satisfies the active mode and doing so cannot immediately launch an opposite pending mode; ambiguous callbacks remain bounded by NimBLE's deadline plus cleanup margin. Submit under the session lock so disconnect or handle reuse cannot retarget the call, retry synchronous rejection after a bounded delay, and remove the redundant connection-time request so the central can establish its initial priority. Tests cover peer updates, wrong handles, failures, lane timeout, session retirement, atomic submission, and millisecond wraparound.
Serialize firmware-initiated connection-parameter procedures on newer ESP32-family controllers and preserve only the latest requested mode while a GAP update is active. Carry the connection generation captured by the NimBLE connect callback into main-task configuration requests so work from a retired BLE session cannot be admitted against a reconnect. Compile the lane and its state out of original ESP32 builds because locally initiated parameter procedures can panic inside the closed controller. Android already selects High and later Balanced priority, so the central owns connection timing on that target. Retain bounded rejected-submission backoff, wrap-safe lane recovery, and conservative callback correlation. An already-satisfied newest mode now cancels any older opposite request, preventing a deferred low-power transition from leaking into a new configuration phase.
jeremiah-k
force-pushed
the
fix/nimble-connection-parameter-serialization
branch
from
August 1, 2026 23:12
6c6b98e to
3c0624c
Compare
jeremiah-k
merged commit Aug 29, 2026
8ee17f9
into
fix/nimble-connection-parameter-serialization-review
3 checks passed
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.
This PR introduces a mutex-serialized “single lane” controller for BLE GAP connection-parameter updates, ensuring only one NimBLE connection-parameter procedure is in flight at a time. While an update is active, incoming mode requests (high-throughput vs low-power) are coalesced and the next submission is deferred until NimBLE reports completion; session teardown resets controller state to prevent stale callbacks from affecting a new connection. For
CONFIG_IDF_TARGET_ESP32, the controller workflow is compiled out and the new request/service hooks are no-ops.Key changes
Features
meshtastic::bluetooth::ConnectionParamsUpdateController(src/nimble/ConnectionParamsUpdateController.h)ConnectionParamsMode(NONE,HIGH_THROUGHPUT,LOW_POWER) and aConnectionParamsRequesttoken (connHandle,mode)Throttle::remainingTimespanMs)reset()and aPIO_UNIT_TESTING-onlyresetForTest(beforeLock)to validate atomicitysrc/nimble/NimbleBluetooth.cppto route connection-parameter sequencing through the controller on supported targetsonConfigStart()requestsHIGH_THROUGHPUT;onConfigComplete()requestsLOW_POWERviarequestConnectionParams(mode)runOnce()now returnsupdateConnectionParams(), which callsconnectionParamsController.servicePending(...)when enabledonConnectcaptures the active session viaonBleConnected(connHandle)onConnParamsUpdaterecords completion/status viaonConnectionParamsUpdate(connHandle, interval, status)resetBleSessionState()clears controller-held state viaresetConnectionParams()test/test_ble_connection_params/test_main.cpp)Throttle::remainingTimespanMs(...)helper (src/mesh/Throttle.h) for remaining-time/wrap-safe schedulingFixes
Refactors
Breaking changes / migration
No external API changes are expected for normal operation; this is an internal sequencing change plus new unit coverage. Note that for
CONFIG_IDF_TARGET_ESP32, the controller-based connection-parameter request/service path is currently compiled out (requests become no-ops).