feat!: take address and connection parameters in GapCentral::Connect - #1310
Closed
gfs (gabrielsantosphilips) wants to merge 6 commits into
Closed
feat!: take address and connection parameters in GapCentral::Connect#1310gfs (gabrielsantosphilips) wants to merge 6 commits into
gfs (gabrielsantosphilips) wants to merge 6 commits into
Conversation
…nect Connect took a MAC address and an address type as separate arguments, and gave the caller no say over the link layer parameters of the connection it was about to open. The connection parameters were only reachable afterwards, through the peripheral side of the interface, so a central had to accept whatever its implementation hard-coded. That is a problem for the supervision timeout in particular, because a peripheral without a hardware ECC engine can block for well over a second while computing LE Secure Connections key material and gets supervised out by a central that assumes a short timeout. Connect now takes the address as a single GapAddress, the requested GapConnectionParameters, and the initiating timeout. Gap.proto follows the same shape: ConnectionParameters now describes the link layer parameters, and the Connect argument is renamed to ConnectRequest. BREAKING CHANGE: GapCentral::Connect takes a GapAddress and GapConnectionParameters instead of a MAC address and address type, and the Connect rpc argument is renamed from ConnectionParameters to ConnectRequest with initiatingTimeoutInMs moved to field 3.
Contributor
Dependency ReviewThe following issues were found:
Snapshot WarningsConsider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. License Issuesexternal/args/CMakeLists.txt
external/segger_rtt/CMakeLists.txt
external/crypto/micro-ecc/CMakeLists.txt
external/crypto/mbedtls/CMakeLists.txt
external/crypto/tiny-aes128/CMakeLists.txt
osal/freertos/CMakeLists.txt
infra/syntax/CMakeLists.txt
external/protobuf/CMakeLists.txt
osal/threadx/CMakeLists.txt
cmake/emil_test_helpers.cmake
lwip/lwip/CMakeLists.txt
OpenSSF ScorecardScorecard details
Scanned Files
|
Copilot started reviewing on behalf of
gfs (gabrielsantosphilips)
August 18, 2026 06:41
View session
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the BLE GAP Central connect API to allow callers to provide both the target address (as a GapAddress) and requested link-layer connection parameters up front, and aligns the RPC schema accordingly. This enables centrals to avoid hard-coded connection settings (notably supervision timeout) that can be incompatible with slow peripherals during LE Secure Connections operations.
Changes:
- Changed
GapCentral::Connectto take aGapAddressplusGapConnectionParameters(and initiating timeout) instead of separate MAC + address type. - Updated the
GapCentralRPC inGap.protoby introducingConnectRequestand redefiningConnectionParametersas the link-layer parameter container. - Updated mocks, decorators, and unit tests to match and validate the new forwarding behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| services/ble/test/TestGapCentral.cpp | Updates decorator test expectations and adds a matcher for comparing GapConnectionParameters. |
| services/ble/test_doubles/GapCentralMock.hpp | Updates the mock Connect signature to the new API. |
| services/ble/Gap.proto | Introduces ConnectRequest, defines link-layer ConnectionParameters, and updates GapCentral.Connect RPC signature. |
| services/ble/Gap.hpp | Updates the GapCentral interface and decorator declaration to the new Connect signature. |
| services/ble/Gap.cpp | Updates GapCentralDecorator::Connect to forward the new arguments to the subject. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Copilot started reviewing on behalf of
gfs (gabrielsantosphilips)
August 18, 2026 06:44
View session
Contributor
✅
|
| Descriptor | Linter | Files | Fixed | Errors | Max errors | Warnings | Elapsed time |
|---|---|---|---|---|---|---|---|
| ✅ ACTION | actionlint | 11 | 0 | 0 | 0.06s | ||
| ✅ ACTION | zizmor | 11 | 0 | 0 | 0 | 6.05s | |
| ✅ CPP | clang-format | 1100 | 8 | 0 | 0 | 7.05s | |
| ✅ DOCKERFILE | hadolint | 2 | 0 | 0 | 0.04s | ||
| ✅ JSON | jsonlint | 7 | 0 | 0 | 0.09s | ||
| ✅ JSON | prettier | 7 | 0 | 0 | 0 | 0.46s | |
| markdownlint | 8 | 0 | 5 | 0 | 0.82s | ||
| ✅ MARKDOWN | markdown-table-formatter | 8 | 0 | 0 | 0 | 0.17s | |
| betterleaks | yes | 1 | 5 | 1.55s | |||
| ✅ REPOSITORY | checkov | yes | no | no | 33.87s | ||
| ✅ REPOSITORY | git_diff | yes | no | no | 0.04s | ||
| ✅ REPOSITORY | grype | yes | no | no | 68.51s | ||
| ✅ REPOSITORY | ls-lint | yes | no | no | 0.01s | ||
| ✅ REPOSITORY | osv-scanner | yes | no | no | 1.17s | ||
| ✅ REPOSITORY | secretlint | yes | no | no | 25.86s | ||
| ✅ REPOSITORY | syft | yes | no | no | 1.79s | ||
| ✅ REPOSITORY | trivy | yes | no | no | 11.51s | ||
| ✅ REPOSITORY | trivy-sbom | yes | no | no | 0.21s | ||
| ✅ REPOSITORY | trufflehog | yes | no | no | 3.59s | ||
| lychee | 140 | 1 | 0 | 111.15s | |||
| prettier | 21 | 1 | 1 | 0 | 0.73s | ||
| ✅ YAML | v8r | 21 | 0 | 0 | 5.08s | ||
| ✅ YAML | yamllint | 21 | 0 | 0 | 1.01s |
Detailed Issues
⚠️ REPOSITORY / betterleaks - 1 error
warning: private-key has detected secret for file services/network/CertificatesMbedTls.cpp.
┌─ services/network/CertificatesMbedTls.cpp:125:21
│
125 │ stream << "HIDDEN_BY_MEGALINTER\r\n";
│ ╰────────────────────────────────────────────────^
warning: private-key has detected secret for file services/network/test_doubles/Certificates.cpp.
┌─ services/network/test_doubles/Certificates.cpp:56:15
│
56 │ "HIDDEN_BY_MEGALINTER\r\n";
│ ╰──────────────────────────────────────────^
warning: private-key has detected secret for file services/network/test_doubles/Certificates.cpp.
┌─ services/network/test_doubles/Certificates.cpp:108:15
│
108 │ "HIDDEN_BY_MEGALINTER\r\n";
│ ╰──────────────────────────────────────────^
warning: generic-api-key has detected secret for file external/crypto/tiny-aes128/TinyAes.c.
┌─ external/crypto/tiny-aes128/TinyAes.c:17:4
│
17 │ key:
│ ╭────^
18 │ │ 2b7e151628aed2a6abf7158809cf4f3c
│ ╰────────────────────────────────────^
warning: generic-api-key has detected secret for file services/network/WebSocket.cpp.
┌─ services/network/WebSocket.cpp:81:64
│
81 │ headers.push_back(services::HttpHeader("Sec-Websocket-Key", "AQIDBAUGBbgJCgsMDQ4PEC=="));
│ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
warning: 5 warnings emitted
⚠️ SPELL / lychee - 1 error
📝 Summary
---------------------
🔍 Total..........697
🔗 Unique.........661
✅ Successful.....691
⏳ Timeouts.........0
🔀 Redirected.....310
👻 Excluded.........5
❓ Unknown..........0
🚫 Errors...........1
⛔ Unsupported......1
Errors in external/protoc/CMakeLists.txt
[404] https://github.com/protocolbuffers/protobuf/releases/download/v$%7Bprotobuf_tag%7D/protoc-$%7Bprotobuf_version%7D-$%7Bos_postfix%7D.zip (at 18:13) | Rejected status code: 404 Not Found
Hint: Followed 310 redirects. You might want to consider replacing redirecting URLs with the resolved URLs. Use verbose mode (`-v`/`-vv`) to see redirection details.
Hint: You can configure accepted/rejected response codes with `-a` or `--accept`
⚠️ MARKDOWN / markdownlint - 5 errors
.github/instructions/microtest.instructions.md:7 error MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading [Context: "## Google Test Suite Coding Ru..."]
external/crypto/tiny-aes128/README.md:1 error MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading [Context: "### Tiny AES128 in C"]
external/crypto/tiny-aes128/README.md:29 error MD046/code-block-style Code block style [Expected: fenced; Actual: indented]
external/crypto/tiny-aes128/README.md:39 error MD046/code-block-style Code block style [Expected: fenced; Actual: indented]
external/crypto/tiny-aes128/README.md:49 error MD046/code-block-style Code block style [Expected: fenced; Actual: indented]
⚠️ YAML / prettier - 1 error
[error] Explicitly specified pattern "documents/modules/ROOT/examples/clangformat.yaml" is a symbolic link.
.clusterfuzzlite/project.yaml 34ms (unchanged)
.github/dependabot.yml 10ms (unchanged)
.github/workflows/ci.yml 43ms (unchanged)
.github/workflows/dependency-scanner.yml 6ms (unchanged)
.github/workflows/documentation.yml 8ms (unchanged)
.github/workflows/fuzzing-batch.yml 5ms (unchanged)
.github/workflows/fuzzing-cron.yml 4ms (unchanged)
.github/workflows/fuzzing-pr.yml 5ms (unchanged)
.github/workflows/linting-formatting.yml 12ms (unchanged)
.github/workflows/release-please.yml 6ms (unchanged)
.github/workflows/security.yml 4ms (unchanged)
.github/workflows/static-analysis.yml 11ms (unchanged)
.github/workflows/validate-pr.yml 14ms (unchanged)
.ls-lint.yml 2ms
.mega-linter.yml 3ms (unchanged)
antora-playbook-branch.yml 3ms (unchanged)
antora-playbook-site.yml 3ms (unchanged)
documents/antora.yml 1ms (unchanged)
documents/supplemental-ui/ui.yml 1ms (unchanged)
mull.yml 1ms (unchanged)
Notices
REPOSITORY_GITLEAKS, REPOSITORY_KICS. See Removed linters to find their replacements.
See detailed reports in MegaLinter artifacts
Your project could benefit from a custom flavor, which would allow you to run only the linters you need, and thus improve runtime performances. (Skip this info by defining FLAVOR_SUGGESTIONS: false)
- Documentation: Custom Flavors
- Command:
npx mega-linter-runner@10.0.0 --custom-flavor-setup --custom-flavor-linters ACTION_ACTIONLINT,ACTION_ZIZMOR,CPP_CLANG_FORMAT,DOCKERFILE_HADOLINT,JSON_JSONLINT,JSON_PRETTIER,MARKDOWN_MARKDOWNLINT,MARKDOWN_MARKDOWN_TABLE_FORMATTER,REPOSITORY_CHECKOV,REPOSITORY_GIT_DIFF,REPOSITORY_BETTERLEAKS,REPOSITORY_GRYPE,REPOSITORY_LS_LINT,REPOSITORY_OSV_SCANNER,REPOSITORY_SECRETLINT,REPOSITORY_SYFT,REPOSITORY_TRIVY,REPOSITORY_TRIVY_SBOM,REPOSITORY_TRUFFLEHOG,SPELL_LYCHEE,YAML_PRETTIER,YAML_YAMLLINT,YAML_V8R
Show us your support by starring ⭐ the repository
| AddressWithType addressWithType = 1; | ||
|
|
||
| // The link layer parameters requested for this connection. | ||
| ConnectionParameters connectionParameters = 2; |
heinwessels (heinwessels-philips)
requested changes
Aug 18, 2026
| // Link layer connection parameters, as defined in Bluetooth Core Specification v6.3, Vol 6, Part B, Section 4.5.1. | ||
| message ConnectionParameters | ||
| { | ||
| // Minimum and maximum connection interval, in units of 1.25 ms. |
Contributor
There was a problem hiding this comment.
Maybe we could add sensible defaults to these values? I could imagine that an customer would see this and not know what to fill it. Similar to what we did for AdvertisementMode. This is something a customer requested.
Same goes for the other values in ConnectionParameters.
Additionally, do we need this change now to fix the bug your solving? Or could it be planned with the upcoming changes to reduce breaking changes? MiquelJayson-Philips
Contributor
Author
There was a problem hiding this comment.
This will be taken offline due to Philips policy.
supervisorTimeoutMs claimed to be milliseconds while every implementation feeds it to a controller api that takes units of 10 ms, and that mismatch already produced a real defect: GapPeripheralTiCC35 divided the value by ten while its own central did not. slaveLatency uses terminology the core specification replaced with peripheral, and the interval fields were abbreviated to the point of hiding that they are multipliers as well. The names now state the unit, so callers no longer need a comment to use them: minConnIntMultiplier -> minConnectionIntervalMultiplier maxConnIntMultiplier -> maxConnectionIntervalMultiplier slaveLatency -> peripheralLatency supervisorTimeoutMs -> supervisionTimeoutMultiplier BREAKING CHANGE: the members of GapConnectionParameters are renamed, and the Connect rpc field supervisionTimeoutInMs is renamed to supervisionTimeoutMultiplier.
Copilot started reviewing on behalf of
gfs (gabrielsantosphilips)
August 18, 2026 07:43
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
services/ble/test/TestGapPeripheral.cpp:70
- The lambda body indentation is inconsistent here (the EXPECT_* lines are over-indented). This looks like it bypassed the usual clang-format style and makes the test harder to read/maintain.
{
EXPECT_EQ(param.maxConnectionIntervalMultiplier, connParam.maxConnectionIntervalMultiplier);
EXPECT_EQ(param.minConnectionIntervalMultiplier, connParam.minConnectionIntervalMultiplier);
EXPECT_EQ(param.peripheralLatency, connParam.peripheralLatency);
EXPECT_EQ(param.supervisionTimeoutMultiplier, connParam.supervisionTimeoutMultiplier);
…onnect-parameters # Conflicts: # services/ble/Gap.proto
Copilot started reviewing on behalf of
gfs (gabrielsantosphilips)
August 18, 2026 08:03
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (3)
services/ble/Gap.proto:425
- In ConnectRequest, field tag 2 now has a different wire type/meaning than the previous Connect request message (tag 2 used to be initiatingTimeoutInMs). If an older client still calls method_id 4 during a rolling upgrade, its varint timeout on tag 2 will be dropped when parsed as the new length-delimited ConnectionParameters message, implicitly turning the timeout into 0 (wait indefinitely). To make mixed-version deployments safer, keep initiatingTimeoutInMs on tag 2 and add connectionParameters on a new tag (e.g. 3).
// The link layer parameters requested for this connection.
ConnectionParameters connectionParameters = 2;
services/ble/test/TestGapPeripheralIntervalDecorator.cpp:77
- The renamed fields are fine, but these updated checks still use EXPECT_EQ. This test file already depends on gMock, so switching to EXPECT_THAT + matchers will improve diagnostics and align with the repository’s Google Test conventions (and should be applied to the other similar blocks in this file too).
EXPECT_EQ(connParam.minConnectionIntervalMultiplier, 6);
EXPECT_EQ(connParam.maxConnectionIntervalMultiplier, 6);
EXPECT_EQ(connParam.peripheralLatency, 0);
EXPECT_EQ(connParam.supervisionTimeoutMultiplier, 500);
services/ble/test/TestGapPeripheral.cpp:70
- These updated assertions use EXPECT_EQ, but this file already uses EXPECT_THAT elsewhere and the project’s Google Test rules prefer EXPECT_THAT for clearer diagnostics. Converting these comparisons will keep the style consistent and improve failure messages.
EXPECT_EQ(param.maxConnectionIntervalMultiplier, connParam.maxConnectionIntervalMultiplier);
EXPECT_EQ(param.minConnectionIntervalMultiplier, connParam.minConnectionIntervalMultiplier);
EXPECT_EQ(param.peripheralLatency, connParam.peripheralLatency);
EXPECT_EQ(param.supervisionTimeoutMultiplier, connParam.supervisionTimeoutMultiplier);
Contributor
Author
|
Copilot fix this issue in this PR creating a new commit: ❌ Linted [REPOSITORY] files with [git_diff]: Found 1 error(s) and 0 warning(s) - (0.1s) (expand for details)
services/ble/test/TestGapPeripheral.cpp:68: trailing whitespace.
services/ble/test/TestGapPeripheral.cpp:69: trailing whitespace.
services/ble/test/TestGapPeripheral.cpp:70: trailing whitespace.
|
…al.cpp Co-authored-by: gabrielsantosphilips <118445638+gabrielsantosphilips@users.noreply.github.com>
Contributor
Fixed in fdf61c3. The trailing whitespace was actually CR characters — |
Copilot started reviewing on behalf of
gfs (gabrielsantosphilips)
August 18, 2026 08:27
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
services/ble/Gap.proto:425
- In protobuf, field number 2 previously held initiatingTimeoutInMs (varint), but now field 2 is reused for connectionParameters (message). If an older client talks to a newer server (or vice versa), this will silently drop the timeout and/or parameters due to the wire-type mismatch, leading to unexpected behavior (e.g. default timeout 0 = wait indefinitely). To reduce operational risk, keep initiatingTimeoutInMs on tag 2 and put connectionParameters on a new tag.
// The link layer parameters requested for this connection.
ConnectionParameters connectionParameters = 2;
Copilot started reviewing on behalf of
gfs (gabrielsantosphilips)
August 18, 2026 08:33
View session
|
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
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.
Connect took a MAC address and an address type as separate arguments, and gave the caller no say over the link layer parameters of the connection it was about to open. The connection parameters were only reachable afterwards, through the peripheral side of the interface, so a central had to accept whatever its implementation hard-coded. That is a problem for the supervision timeout in particular, because a peripheral without a hardware ECC engine can block for well over a second while computing LE Secure Connections key material and gets supervised out by a central that assumes a short timeout.
Connect now takes the address as a single GapAddress, the requested GapConnectionParameters, and the initiating timeout. Gap.proto follows the same shape: ConnectionParameters now describes the link layer parameters, and the Connect argument is renamed to ConnectRequest.
BREAKING CHANGE: GapCentral::Connect takes a GapAddress and GapConnectionParameters instead of a MAC address and address type, and the Connect rpc argument is renamed from ConnectionParameters to ConnectRequest with initiatingTimeoutInMs moved to field 3.