Test

[h2zero]

Summary by CodeRabbit

Release Notes

• New Features

  • Added library version query API via getVersion()
  • Added passkey display callback for custom pairing workflows on client
  • Added passkey entry callback for custom pairing workflows on server

• Bug Fixes

  • Improved error handling for GATT server startup failures with proper logging

• Chores

  • Services now start through the server startup process instead of individually

[coderabbitai]

📝 Walkthrough

Walkthrough

Arrr, this PR be restructurin' how NimBLE services be startin' themselves, matey! The swashbuckling changes remove explicit service start calls from examples and library code, add proper error handlin' for GATT server startup, introduce version macros for trackin' library releases, add new passkey display callbacks for pairing flows, and refactor service startup to flow through the server instead o' bein' called independently. Yarrr!

Changes

Cohort / File(s)	Summary
Example Code Updates examples/Bluetooth_5/NimBLE_extended_server/main/main.cpp, examples/Bluetooth_5/NimBLE_multi_advertiser/main/main.cpp, examples/NimBLE_Server/main/main.cpp	Removed explicit pService->start() calls from application initialization, movin' service startup responsibility to the server's start sequence.
Versioning Infrastructure src/NimBLECppVersion.h	New header introducin' compile-time version macros (major, minor, patch) and a semantic version string NIMBLE_CPP_VERSION_STR for library version trackin' and diagnostics.
Device Initialization & Logging src/NimBLEDevice.cpp, src/NimBLEDevice.h	Added public getVersion() method exposin' library version; removed internal GATT stack initialization calls (ble_gatts_reset(), GATT/GAP service init) from server creation; enhanced startup logging with version and completion messages.
Server Startup Refactoring src/NimBLEServer.cpp, src/NimBLEServer.h	Changed start() and resetGATT() return types from void to bool to signal success/failure; refactored reset logic to check service change conditions; added onPassKeyEntry() callback for passkey injection during pairing.
Service Startup Refactoring src/NimBLEService.cpp, src/NimBLEService.h	Renamed internal start() logic to start_internal(); deprecated public start() method (now returns true without effect) with documentation clarifyin' that services start via NimBLEServer::start(); added cleanup on registration errors.
Client Pairing Enhancements src/NimBLEClient.cpp, src/NimBLEClient.h	Added new onPassKeyDisplay() callback to handle passkey display requests during pairing; wired BLE_SM_IOACT_DISP path to construct and inject passkey via callback before injection.
Advertising Error Handling src/NimBLEAdvertising.cpp	Enhanced start() to check GATT server start return value; logs error and returns false if GATT startup fails, preventin' advertisin' from proceedin' on failure.
Stream Service Cleanup src/NimBLEStream.cpp	Removed explicit pSvc->start() call in NimBLEStreamServer::begin(), delegatin' service startup to the server initialization flow.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

This PR be involvin' scattered changes across multiple files with mixed complexity, matey. Ye be havin' return-type signature changes in NimBLEServer and NimBLEService, refactorin' of startup logic from distributed to centralized server control, new callback hooks for pairing flows, and systematic removal o' service start calls from examples. While each individual change be straightforward, the breadth and interconnected nature o' the startup sequence modifications—particularly the control-flow shift from independent service starts to server-mediated ones—demand careful reasonin' about potential impact on initialization order and error propagation. No complex algorithm changes here, but the architectural refactorin' requires solid understandin' of the service/server lifecycle. Yarrr!

Possibly related PRs

• Add BLE stream classes. #384: Related to NimBLEStream implementation and service startup behavior; this PR's removal o' explicit pService->start() calls directly affects the stream classes and their initialization patterns introduced in that PR, matey!

Poem

⚓ Hoist the sails o' startup refactor,
Services now bow to server's master,
Error checks and version traces,
Pairing flows with callback graces,
A tidy ship, from bow to aft, arr! 🏴‍☠️

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The pull request title 'Test' is vague and generic, failing to convey meaningful information about the substantial changes made across multiple files and the core functionality affected.	Replace 'Test' with a descriptive title that captures the main change, such as 'Refactor service startup lifecycle and add passkey display callback' or similar to reflect the primary objectives.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	Docstring coverage is 91.67% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch test

📝 Coding Plan

• Generate coding plan for human review comments

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/NimBLEServer.cpp (1)

272-320: ⚠️ Potential issue | 🔴 Critical

Ahoy! Critical issue spotted in yer extended advertising, ye salty sea dog!

While the start() return value changes be sound, not all callers be handlin' the new boolean properly! The extended advertising be ignorin' the return value at src/NimBLEExtAdvertising.cpp:74:

pServer->start(); // make sure the GATT server is ready before advertising

This be sailin' blind! Contrast with src/NimBLEAdvertising.cpp:200, which handles it proper-like:

if (pServer != nullptr && !pServer->start()) { // make sure the GATT server is ready before advertising
    NIMBLE_LOGE(LOG_TAG, "Failed to start GATT server");
    return false;
}

Extended advertising must check the return value and handle failure before proceeding with configuration. The GATT server could be sinkin' without ye knowin' it!

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NimBLEServer.cpp` around lines 272 - 320, The call to pServer->start() in
NimBLEExtAdvertising (where extended advertising prepares to start) ignores the
boolean return and can proceed when GATT start failed; update the logic around
the call in NimBLEExtAdvertising.cpp (the spot currently calling
pServer->start() to "make sure the GATT server is ready") to mirror
NimBLEAdvertising.cpp's pattern: check pServer != nullptr and if
pServer->start() returns false, log an error via NIMBLE_LOGE (similar message
"Failed to start GATT server") and return false from the extended advertising
start/config routine so advertising setup is aborted on GATT start failure.

🧹 Nitpick comments (1)

src/NimBLEClient.cpp (1)

1213-1222: Ahoy! The magic number 123456 be sailin' through yer code, matey!

Arr, this passkey check be usin' the hardcoded value 123456 to determine whether to invoke the callback. While this be consistent with the server-side implementation in NimBLEServer.cpp (lines 668-669), it be a bit fragile, ye scurvy dog! If a landlubber intentionally sets their passkey to 123456, the callback will still be invoked unnecessarily.

Consider definin' a named constant like NIMBLE_DEFAULT_PASSKEY to make the intent clearer across the seven seas of yer codebase.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NimBLEClient.cpp` around lines 1213 - 1222, Replace the magic literal
123456 used in the BLE_SM_IOACT_DISP handling with a named constant (e.g.,
NIMBLE_DEFAULT_PASSKEY) to make intent explicit and avoid accidental collisions;
declare NIMBLE_DEFAULT_PASSKEY in an appropriate shared header or at the top of
NimBLEClient.cpp and update the check in the BLE_SM_IOACT_DISP branch where
pkey.passkey (from NimBLEDevice::getSecurityPasskey()) is compared and used to
decide whether to call pClient->m_pClientCallbacks->onPassKeyDisplay(peerInfo)
so the same symbolic constant is used consistently with the server-side
implementation.

🤖 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/NimBLEService.h`:
- Around line 45-51: The deprecated NimBLEService::start() currently returns
true unconditionally which hides real startup failures; update this shim to call
and return the actual boolean result from the server start routine instead of
always true — locate NimBLEService::start() and change it to invoke
NimBLEServer::start() (or the appropriate server instance/owner start method
tied to this service) and return that boolean so existing service->start() call
sites reflect real success/failure.

---

Outside diff comments:
In `@src/NimBLEServer.cpp`:
- Around line 272-320: The call to pServer->start() in NimBLEExtAdvertising
(where extended advertising prepares to start) ignores the boolean return and
can proceed when GATT start failed; update the logic around the call in
NimBLEExtAdvertising.cpp (the spot currently calling pServer->start() to "make
sure the GATT server is ready") to mirror NimBLEAdvertising.cpp's pattern: check
pServer != nullptr and if pServer->start() returns false, log an error via
NIMBLE_LOGE (similar message "Failed to start GATT server") and return false
from the extended advertising start/config routine so advertising setup is
aborted on GATT start failure.

---

Nitpick comments:
In `@src/NimBLEClient.cpp`:
- Around line 1213-1222: Replace the magic literal 123456 used in the
BLE_SM_IOACT_DISP handling with a named constant (e.g., NIMBLE_DEFAULT_PASSKEY)
to make intent explicit and avoid accidental collisions; declare
NIMBLE_DEFAULT_PASSKEY in an appropriate shared header or at the top of
NimBLEClient.cpp and update the check in the BLE_SM_IOACT_DISP branch where
pkey.passkey (from NimBLEDevice::getSecurityPasskey()) is compared and used to
decide whether to call pClient->m_pClientCallbacks->onPassKeyDisplay(peerInfo)
so the same symbolic constant is used consistently with the server-side
implementation.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: aad66044-0c59-4978-b924-2e3a2aee33a5

📥 Commits

Reviewing files that changed from the base of the PR and between 22e7d6f and 05c21fa.

📒 Files selected for processing (14)

• examples/Bluetooth_5/NimBLE_extended_server/main/main.cpp
• examples/Bluetooth_5/NimBLE_multi_advertiser/main/main.cpp
• examples/NimBLE_Server/main/main.cpp
• src/NimBLEAdvertising.cpp
• src/NimBLEClient.cpp
• src/NimBLEClient.h
• src/NimBLECppVersion.h
• src/NimBLEDevice.cpp
• src/NimBLEDevice.h
• src/NimBLEServer.cpp
• src/NimBLEServer.h
• src/NimBLEService.cpp
• src/NimBLEService.h
• src/NimBLEStream.cpp

💤 Files with no reviewable changes (3)

• examples/Bluetooth_5/NimBLE_multi_advertiser/main/main.cpp
• examples/NimBLE_Server/main/main.cpp
• examples/Bluetooth_5/NimBLE_extended_server/main/main.cpp