8000
Skip to content

[ble_device_base] Bind BLEHub to the build's tracker at compile time - #18181

Merged
bdraco merged 79 commits into
devfrom
esp32-hub-devirtualize
Aug 9, 2026
Merged

[ble_device_base] Bind BLEHub to the build's tracker at compile time#18181
bdraco merged 79 commits into
devfrom
esp32-hub-devirtualize

Conversation

@bdraco
@bdraco bdraco commented Aug 8, 2026 •
edited
Loading
Copy link
Copy Markdown
Member

What does this implement/fix?

Replaces the abstract BLEHub interface with a compile time binding, chained on #18179. Exactly one tracker exists per build, so ble_device_base/ble_hub_impl.h now selects it with a using BLEHub = ... alias (each tracker's codegen emits a USE_*_BLE_TRACKER define) and the virtual class is deleted. Every hub call becomes a direct, inlinable member call; the trackers drop their vtable for the contract; ble_hub.h keeps the shared types and documents the duck typed method surface, which a tracker missing a method fails at the consumer's call site.

What falls out:

  • get_capabilities() is now static constexpr on every tracker, so capability branches in consumers fold at compile time.
  • The proxy's esp32 parent_() downcast and the set_parent/set_ble_hub split are gone; one setter, no cast.
  • The scanner state push slot moved from the deleted base into the esp32 tracker, and the proxy registers it only where USE_BLE_SCANNER_STATE_CALLBACK is emitted (esp32); the polled hubs no longer carry an unused 8 byte slot.
  • The advertisement trigger constructors in ble_device_base/automation.h are templated on the hub type, so the header also builds with no tracker present (host unit tests).
  • The documented surface is enforced: a BLEHubContract concept in ble_hub.h is static_asserted where ble_hub_impl.h binds the alias, so a tracker missing a method fails one clear assert instead of a distant call site. The old scan mode host test became tautological without the base class and is deleted in favor of that compile time check.
  • defines.h now mirrors codegen: the scanner state define sits in the esp32 arm only, and the LibreTiny block selects one tracker per build (ln882x gets its real hub, bk72xx stands in for hub-less chips) so each tidy environment analyzes the capabilities it ships.

This should land before 2026.8.0 ships: the BLEHub contract is new in this cycle and has never been released, so reshaping it now is free while doing it later would be a breaking change.

Not breaking for users or external components; the interface never shipped and there are no external hubs or platforms.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • New developer-facing feature (adds functionality for component developers; no end-user configuration change)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected) — policy
  • Developer breaking change (an API change that could break external components) — policy
  • Undocumented C++ API change (removal or change of undocumented public methods that lambda users may depend on) — policy
  • Code quality improvements to existing code or addition of tests
  • Other

Related issue or feature (if applicable):

  • fixes

Pull request in esphome.io with documentation (if applicable):

  • esphome/esphome.io#<esphome.io PR number goes here>

Pull request in developers.esphome.io with developer documentation (if applicable):

  • esphome/developers.esphome.io#<developers.esphome.io PR number goes here>

Test Environment

  • ESP32
  • ESP32 IDF
  • ESP8266
  • RP2040/RP2350
  • BK72xx
  • RTL87xx
  • LN882x
  • nRF52840

Example entry for config.yaml:

# Example config.yaml

Checklist:

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works (under tests/ folder).

If user exposed functionality or configuration variables are added/changed:

bdraco added 15 commits August 8, 2026 00:00
@bdraco
Move the esp32 proxy advertisement path to the hub raw callback
de9074f
@bdraco
Pin the removed listener slot
5296539
@bdraco
Carry the address as uint64 in RawAdvertisement and unify the proxy s…
a6f81f3 
…etup
@bdraco
Deliver scanner state through a hub push callback
3e98081
@bdraco
Retire the raw listener path and parser-type enum from the tracker
67cc43c
@bdraco
Address review: one tracker pointer, portable format, doc fixes
47804f8
@bdraco
Merge branch 'esp32-proxy-hub-advertisements' into esp32-proxy-hub-sc…
7f44b9b 
…anner-state

# Conflicts:
#	esphome/components/bluetooth_proxy/bluetooth_proxy.cpp
@bdraco
Merge branch 'esp32-proxy-hub-scanner-state' into esp32-tracker-parse…
e671a60 
…r-cleanup
@bdraco
Simplify scanner state: aliased enum, define-gated slot, one guarded …
d9f0f40 
…sender
@bdraco
Merge branch 'esp32-proxy-hub-scanner-state' into esp32-tracker-parse…
11fc8e4 
…r-cleanup
@bdraco
Monotonic parse flag, client-scoped opt-out, drop dead include
0e5d902
@bdraco
Retire the scanner-state listener interface
6b2938c
@bdraco
Remove redundant access specifier
b092f64
@bdraco
Merge branch 'esp32-tracker-parser-cleanup' into esp32-tracker-retire…
c0c3afe 
…-scanner-listener
@bdraco
Bind BLEHub to the build's tracker at compile time
ab458ff
@bdraco
bdraco commented Aug 8, 2026
Copy link
Copy Markdown
Member Author

@bluetoothbot review

@esphome
esphome Bot commented Aug 8, 2026
Copy link
Copy Markdown
Contributor

To use the changes from this PR as an external component, add the following to your ESPHome configuration YAML file:

external_components:
  - source: github://pr#18181
    components: [bk72xx_ble_tracker, ble_device_base, bluetooth_proxy, esp32_ble_tracker, ln882h_ble_tracker, rp2_ble_tracker]
    refresh: 1h

(Added by the PR bot)

@bdraco bdraco added this to the 2026.8.0 milestone Aug 8, 2026
@bdraco
bdraco commented Aug 9, 2026
Copy link
Copy Markdown
Member Author

@bluetoothbot review

@bluetoothbot
bluetoothbot commented Aug 9, 2026 •
edited
Loading
Copy link
Copy Markdown
Contributor

Previous review — superseded by a newer review below.

bluetoothbot
bluetoothbot approved these changes Aug 9, 2026
View reviewed changes
@bluetoothbot bluetoothbot left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

No blocking issues found — ready to merge.

@bluetoothbot
bluetoothbot commented Aug 9, 2026 •
edited
Loading
Copy link
Copy Markdown
Contributor

Previous review — superseded by a newer review below.

bluetoothbot
bluetoothbot approved these changes Aug 9, 2026
View reviewed changes
@bluetoothbot bluetoothbot left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

No blocking issues found — ready to merge.

@bdraco bdraco mentioned this pull request Aug 9, 2026
Merged
19 tasks
@bluetoothbot
bluetoothbot commented Aug 9, 2026 •
edited
Loading
Copy link
Copy Markdown
Contributor

Previous review — superseded by a newer review below.

bluetoothbot
bluetoothbot approved these changes Aug 9, 2026
View reviewed changes
@bluetoothbot bluetoothbot left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

No blocking issues found — ready to merge.

@bluetoothbot
bluetoothbot commented Aug 9, 2026 •
edited
Loading
Copy link
Copy Markdown
Contributor

Previous review — superseded by a newer review below.

bluetoothbot
bluetoothbot approved these changes Aug 9, 2026
View reviewed changes
@bluetoothbot bluetoothbot left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

No blocking issues found — ready to merge.

@bdraco
bdraco marked this pull request as draft August 9, 2026 04:09
@bluetoothbot
bluetoothbot commented Aug 9, 2026 •
edited
Loading
Copy link
Copy Markdown
Contributor

Previous review — superseded by a newer review below.

bluetoothbot
bluetoothbot approved these changes Aug 9, 2026
View reviewed changes
@bluetoothbot bluetoothbot left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

No blocking issues found — ready to merge.

@bluetoothbot
bluetoothbot commented Aug 9, 2026
Copy link
Copy Markdown
Contributor

PR Review — [ble_device_base] Bind BLEHub to the build's tracker at compile time

Approve — the devirtualization is correct and consistently applied, and this round closes the last actionable nit from the previous review. Only non-blocking notes remain.

Specific strengths, each re-verified against the tree this round rather than carried forward on trust:

  • The "exactly one tracker per build" premise actually holds at the config layer: bk72xx_ble declares DEPENDENCIES = ["bk72xx"] and ln882h_ble declares ["ln882x"], so the two LibreTiny arms are mutually exclusive, and esp32/rp2 are exclusive by platform. No config can light two USE_*_BLE_TRACKER arms and silently take the first.
  • The concept is real enforcement, not decoration. writer.py:238-255 puts every header of every used component into the generated esphome.h, and all four trackers AUTO_LOAD ble_device_base, so ble_hub_impl.h and its static_assert compile into every firmware that ships a tracker — broader than the pure-virtual base it replaces. ESP32BLETracker::get_scanner_state() returns ble_device_base::ScannerState via the using alias at esp32_ble_tracker.h:90, so the std::same_as check under USE_BLE_SCANNER_STATE_CALLBACK is satisfiable.
  • C++20 concepts are not new ground for this component — ble_device_base/ble_gatt_client.h:98,120 already ships two, so no toolchain risk on the LibreTiny/RP2 arms.
  • Include-guard alignment holds for the defines.h stand-in: bk72xx_ble_tracker.h is gated on USE_LIBRETINY and bk72xx_ble.h on USE_BK72XX_BLE, both of which the LibreTiny block defines for every LibreTiny tidy env — so the rtl87xx envs really can parse the bk72xx arm.
  • The four new USE_*_BLE_TRACKER names collide with nothing pre-existing (grep across esphome/, script/, tests/ returns only the PR's own sites), and ble_hub_impl.h's ESPHOME_BLE_HUB_TYPE is a type macro, so ci-custom.py's lint_no_defines (integer-literal only) does not fire.
  • The push/poll split is expressed once and completely: send_polled_scanner_state_, last_scan_running_, the loop() poll, the mode report and subscribe_api_connection all key off USE_BLE_SCANNER_STATE_CALLBACK, and esp32_ble_tracker.cpp:390 guards the invoke under the same macro. Behaviour is byte-for-byte what #ifndef USE D7A7 _ESP32 produced before.
  • get_capabilities() has exactly one consumer (bluetooth_proxy.h:155, in the #else // !USE_ESP32 arm of get_feature_flags() const), which the static constexpr form makes legal and folds to a constant. No stale instance call site anywhere in esphome/ or tests/.
  • The new ln882h_tracker.yaml matches its sibling fixtures exactly — same slotcount- name prefix, canonical generic-ln882h board (ln882x/boards.py:18), and a valid dependency chain through ln882h_ble → ln882x.

Resolved since the previous round:

  • test_every_tracker_emits_its_alias_define's docstring now states the real safety net ("fails any build containing a BLEHub consumer — today bluetooth_proxy, which CI compiles or tidy-parses on every tracker platform") instead of the earlier overclaim. Confirmed accurate: with no matching arm, ble_hub_impl.h expands to an empty header and only a translation unit naming the alias breaks.

Remaining, both non-blocking:

  • Two #ifndef USE_BLE_SCANNER_STATE_CALLBACK guards in the !USE_ESP32 arm of bluetooth_proxy.cpp cannot be false today; the mode-report one encodes an assumption a future push hub could break silently. Documented in ble_hub.h, so this stays a note.
  • test_raw_callback.cpp's FakeHub comment still claims to pin "the emit path every tracker implements", which no longer follows now that the class inherits nothing.

✅ Resolved since last review (1)

Previously-flagged issues verified fixed
  • tests/component_tests/ble_device_base/test_hub_binding.py:222 Docstring overstates how a missing ladder arm fails

🟢 Suggestions

1. Mode-change report is compiled out for push hubs, but a push only fires on a state transition
esphome/components/bluetooth_proxy/bluetooth_proxy.cpp:608-616

Carried unchanged from the previous round; re-verified against the current diff and still accurate.

This guard makes the neutral bluetooth_scanner_set_mode() skip its state report whenever the build has a push hub, on the assumption that the push path reports the restart's transitions instead.

That holds only when the mode change actually restarts the scan. A future push hub that applies the mode while the scanner is idle, or in place with no STOPPING/STARTING/RUNNING cycle, emits no transition, so the subscriber never learns the mode changed. Same for a refused request: today the unconditional send_polled_scanner_state_() is what carries the real, unchanged mode back to Home Assistant, and under the push define that path disappears too.

Re-verified the preprocessor structure this round rather than re-reading the prior note: bluetooth_proxy.cpp:469 opens #ifdef USE_ESP32, line 482 is #else // !USE_ESP32, and line 609 closes it — so both new guards in this arm (loop()'s poll and this one) sit in the non-esp32 half, while USE_BLE_SCANNER_STATE_CALLBACK is now emitted only by _to_code_esp32 (bluetooth_proxy/__init__.py, the sole emitter in the tree). Neither #ifndef can be false today — they are placeholders for a hub that does not exist yet, not live branches.

The PR already added the matching requirement to ble_hub.h ("A push hub must emit a transition for every accepted or refused mode request"), which covers the documentation half. The belt-and-braces alternative is keeping the report unconditional and letting the change detector dedupe.

#ifndef USE_BLE_SCANNER_STATE_CALLBACK
  if (this->api_connection_ != nullptr) {
    this->send_polled_scanner_state_();
  }
#endif

Checklist

  • Exactly one tracker per build is enforceable, not just asserted
  • defines.h mirror matches codegen on every tidy environment
  • BLEHubContract satisfied by every tracker it can bind to
  • Contract enforced on every build that ships a tracker
  • Tracker header platform guards match their alias arm's define
  • C++20 concepts supported on every toolchain the header reaches
  • New tracker headers add no unresolvable SDK include to proxy consumers
  • New USE_*_BLE_TRACKER defines collide with nothing existing
  • Push/poll compile-time split expressed consistently
  • No stranded references to compiled-out members
  • Non-proxy BLE consumers unaffected by removing the BLEHub class
  • Templated trigger constructors cannot resolve to the wrong overload
  • Dropped ble_hub.h include from automation.h breaks no transitive consumer
  • No clang-tidy or ci-custom regression from the new macro and devirtualized methods
  • New test fixtures follow the sibling files' conventions and board ids
  • Test comments describe the enforcement that actually exists
  • No user-facing or external-component breaking change
  • Diff matches the PR description; no scope creep
  • Compile-time define carries a single, accurate meaning — suggestion ＃1
ℹ️ Triage summary

1 pre-existing finding(s) on unchanged code suppressed (freeze).

Automated review by Kōan (Claude) HEAD=97bbcf5 7 min 25s

bluetoothbot
bluetoothbot approved these changes Aug 9, 2026
View reviewed changes
@bluetoothbot bluetoothbot left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

No blocking issues found — ready to merge.

@bdraco
bdraco commented Aug 9, 2026
Copy link
Copy Markdown
Member Author

decline the remaining bot suggestion. Its going to get refactored anyways in future PRs

@bdraco
bdraco marked this pull request as ready for review August 9, 2026 05:23
@bdraco
bdraco commented Aug 9, 2026
Copy link
Copy Markdown
Member Author

thanks

@bdraco
bdraco merged commit e9f4289 into dev Aug 9, 2026
68 checks passed
@bdraco
bdraco deleted the esp32-hub-devirtualize branch August 9, 2026 13:57
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 11, 2026
@jesserockz jesserockz removed this from the 2026.8.0 milestone Aug 12, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@swoboda1337 swoboda1337 swoboda1337 approved these changes
@jesserockz jesserockz Awaiting requested review from jesserockz jesserockz is a code owner
+1 more reviewer
@bluetoothbot bluetoothbot bluetoothbot approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

by-code-owner code-quality component: bk72xx_ble_tracker component: ble_device_base component: bluetooth_proxy component: esp32_ble_tracker component: ln882h_ble_tracker component: rp2_ble_tracker core has-tests stacked-pr

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@bdraco @bluetoothbot @swoboda1337 @jesserockz
0