[thekurtovic]

Resolves cppcheck warnings noExplicitConstructor, useInitializationList, and passedByValue.

Summary by CodeRabbit

• API Changes
  • Constructors for Bluetooth addresses, clients, characteristics, services, connection information, HID devices, extended advertisements, and attribute values now require explicit construction, preventing unintended implicit conversions.
  • Attribute-value constructors accept strings and byte vectors by const reference.
• Compatibility
  • Code relying 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]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: e3ab60a8-cf64-4f84-a6b3-300e644a1e02

Commits

Reviewing files that changed from the base of the PR and between 6ab0376 and 6fbcb7e.

Files selected for processing: 6

• src/NimBLEAttValue.h
• src/NimBLEClient.cpp
• src/NimBLEClient.h
• src/NimBLEExtAdvertising.h
• src/NimBLEHIDDevice.h
• src/NimBLEUUID.h

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.

Walkthrough

Arr, public constructors now require explicit conversions. NimBLEAttValue string and vector constructors take const references. Affected call sites now construct NimBLEUUID or NimBLEAddress objects directly.

Changes: Explicit BLE conversions

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 without signature changes.
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.

Priority: Low

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

Change: Other

Suggested reviewers: h2zero

Merge Risk: Minimal · up to 6fbcb

No merge-blocking issue is established. Explicit constructors intentionally restrict implicit conversions, so applications relying on those conversions may need call-site updates.

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		Arr, the title be clear and on target. "Add explicit keyword to constructors" names the main change: the constructors across the NimBLE headers now carry the explicit keyword to block implicit convers…
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: Title check

Explanation

Arr, the title be clear and on target. "Add explicit keyword to constructors" names the main change: the constructors across the NimBLE headers now carry the explicit keyword to block implicit conversions. The title is short, specific, and matches the PR objectives, which target the noExplicitConstructor warnings. Aye, it does not list the supporting edits in NimBLEHIDDevice.cpp or NimBLEAttValue.h, but it need not do so.

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 14 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, conversions now sail with a clear type,
UUIDs are wrapped at each call site,
Const references pass strings and vectors along,
The lookup order stays steady and strong,
Explicit constructors keep the course right.}

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