[thekurtovic]

Resolves cppcheck warnings noExplicitConstructor, useInitializationList, and passedByValue.

Summary by CodeRabbit

• API Changes
  • Constructors for Bluetooth addresses, UUIDs, clients, characteristics, services, connection information, HID devices, extended advertisements, and attribute values now require explicit construction.
  • Attribute-value constructors accept strings and byte vectors by const reference.
  • Local and remote services support characteristic lookup using 16-bit or 32-bit UUIDs.
• Compatibility
  • Code that relied on implicit conversions to these types may need to construct the target type explicitly.

[thekurtovic]

There's one warning I wasn't able to resolve.
NimBLEUUID(const ble_uuid_any_t& uuid);

If I make that constructor explicit then I get this compile error.

Log

managed_components/esp-nimble-cpp/src/NimBLERemoteValueAttribute.h: In constructor 'NimBLERemoteValueAttribute::NimBLERemoteValueAttribute(const ble_uuid_any_t&, uint16_t)':
managed_components/esp-nimble-cpp/src/NimBLERemoteValueAttribute.h:189:107: error: no matching function for call to 'NimBLEAttribute::NimBLEAttribute(const ble_uuid_any_t&, uint16_t&)'
  189 |     NimBLERemoteValueAttribute(const ble_uuid_any_t& uuid, uint16_t handle) : NimBLEAttribute(uuid, handle) {}
      |                                                                                                           ^
In file included from managed_components/esp-nimble-cpp/src/NimBLERemoteService.h:24,
                 from managed_components/esp-nimble-cpp/src/NimBLEDevice.h:261:
managed_components/esp-nimble-cpp/src/NimBLEAttribute.h:48:5: note: candidate: 'NimBLEAttribute::NimBLEAttribute(const NimBLEUUID&, uint16_t)'
   48 |     NimBLEAttribute(const NimBLEUUID& uuid, uint16_t handle) : m_uuid{uuid}, m_handle{handle} {}
      |     ^~~~~~~~~~~~~~~
managed_components/esp-nimble-cpp/src/NimBLEAttribute.h:48:39: note:   no known conversion for argument 1 from 'const ble_uuid_any_t' to 'const NimBLEUUID&'
   48 |     NimBLEAttribute(const NimBLEUUID& uuid, uint16_t handle) : m_uuid{uuid}, m_handle{handle} {}
      |                     ~~~~~~~~~~~~~~~~~~^~~~
managed_components/esp-nimble-cpp/src/NimBLEAttribute.h:29:7: note: candidate: 'constexpr NimBLEAttribute::NimBLEAttribute(const NimBLEAttribute&)'
   29 | class NimBLEAttribute {

---

Also NimBLEHIDDevice needs to be updated to be compliant with this change.
For example: m_deviceInfoSvc = server->createService(deviceInfoSvcUuid);

Do you think it's better to just create a new constructor which can take a uint16_t, or change such instances like this?
m_deviceInfoSvc = server->createService(NimBLEUUID(deviceInfoSvcUuid));

I've gone with the latter (though I excluded the file from the commit for now), though adding the new constructor would probably make it easier to read NimBLEHIDDevice.

Edit: Or this

static constexpr uint16_t deviceInfoSvcUuid = 0x180a;
static const NimBLEUUID deviceInfoSvcUuid = NimBLEUUID((uint16_t)0x180a);

[h2zero]

I think m_deviceInfoSvc = server->createService(NimBLEUUID(deviceInfoSvcUuid)); is probably fine here as it's an internally managed class so application code isn't affected, still quite readable as well.

[thekurtovic]

Would this be considered a breaking change? Any applications making use of these implicit conversions will fail to compile, though the changes should be easy to make.
I'll make a separate PR for NimBLEHIDDevice.

[h2zero]

It might be, depending on the application.

I would suggest adding the HID changes to this PR so we can test the builds at least. Maybe add some more build tests to find where it may fail so it can be documented?

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

• Last reviewed commit: fb0574f

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Arr, public constructors now require explicit conversions. NimBLEAttValue string and vector constructors take const references. Affected call sites construct NimBLEUUID or NimBLEAddress objects directly. Local and remote services add integer-UUID characteristic lookup overloads.

Changes: BLE conversions and characteristic lookup

Layer / File(s)	Summary
Explicit constructor declarations src/NimBLEUUID.h, src/NimBLE2904.h, src/NimBLEAddress.h, src/NimBLEAttValue.h, src/NimBLECharacteristic.h, src/NimBLEConnInfo.h, src/NimBLEService.h, src/NimBLEClient.h, src/NimBLEExtAdvertising.h, src/NimBLEHIDDevice.h	Arr, constructors across these public types are now explicit. NimBLEAttValue string and vector constructors now accept const references. NimBLEUUID constructor declarations are reordered.
Explicit UUID and address call sites src/NimBLEHIDDevice.cpp, src/NimBLERemoteService.cpp, src/NimBLERemoteValueAttribute.h, src/NimBLEClient.cpp	Affected call sites now construct NimBLEUUID objects for UUID arguments and NimBLEAddress objects for disconnect-event fallback lookups. Existing UUIDs, properties, lookup order, and null checks remain unchanged.
Integer-UUID characteristic lookup src/NimBLEService.h, src/NimBLEService.cpp, src/NimBLERemoteService.h, src/NimBLERemoteService.cpp	Local and remote service classes add characteristic lookup overloads for 16-bit and 32-bit UUIDs. The overloads convert each integer UUID to NimBLEUUID and delegate to existing lookup methods.

Priority: Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Refactor

Suggested reviewers: h2zero

Merge Risk: Minimal · up to fb057

No actionable merge-blocking issue is established by the available evidence. This assessment is limited to the supplied characteristic-lookup changes.

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly identifies the main change: adding the explicit keyword to constructors across the codebase.
Docstring Coverage		Docstring coverage is 84.21% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentati…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 84.21% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 19 functions across 16 files.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Arr, UUIDs now sail explicit and clear,
Two integer lookup routes draw near.
Addresses and values mind their types,
While services chart the matching stripes.
The BLE constructors keep conversions in sight,
And report UUIDs are wrapped right.}

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