8000
Skip to content

[esp32_ble] Migrate the BLE count machinery to the shared slot counter factory - #18059

Merged
bdraco merged 9 commits into
devfrom
esp32_ble_slot_counters
Aug 5, 2026
Merged

[esp32_ble] Migrate the BLE count machinery to the shared slot counter factory#18059
bdraco merged 9 commits into
devfrom
esp32_ble_slot_counters

Conversation

@bdraco
@bdraco bdraco commented Aug 4, 2026
Copy link
Copy Markdown
Member

What does this implement/fix?

Moves the esp32 BLE count machinery onto the shared slot counter factory from #18057. The five esp32_ble handler counters lived in a module level dataclass, which survives between compiles when the host process stays alive (device-builder, dashboard); they now live in CORE.data through the factory, which clears between runs. On the old code each compile in such a process grew every handler count by one, so later builds oversized the StaticCallbackManager storage; the counts now start from zero every compile. The esp32_ble_tracker listener and client counters move to factory instances as well, and its FINAL job now only handles the feature coupling.

The tracker's scanner state listeners also move from std::vector to the codegen sized StaticVector pattern; bluetooth_proxy is the only subscriber and now requests a slot from its to_code, so a build without it carries no scanner state listener storage at all.

All define names are unchanged; no in tree consumer components need changes. The tracker's add_scanner_state_listener method is now compiled only when a codegen registration requests a slot. That method has only ever served bluetooth_proxy (added for it in #8649, reshaped in #12156) and the proxy now registers through the new esp32_ble_tracker.register_scanner_state_listener codegen helper, which any future consumer should use as well.

Chained on #18057.

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):

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

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

Test Environment

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

Example entry for config.yaml:

# Example config.yaml

bluetooth_proxy:
  active: true

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
bdraco commented Aug 4, 2026
Copy link
Copy Markdown
Member Author

@bluetoothbot review

@codecov
codecov Bot commented Aug 4, 2026
Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.11%. Comparing base (1bae91a) to head (2855922).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##              dev   #18059   +/-   ##
=======================================
  Coverage   87.11%   87.11%           
=======================================
  Files          63       63           
  Lines       14601    14601           
  Branches     2201     2201           
=======================================
  Hits        12720    12720           
  Misses       1574     1574           
  Partials      307      307           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
8000

@bdraco
bdraco force-pushed the ble_listener_slot_factory branch from 5e62c66 to b0ef638 Compare August 4, 2026 05:16
@esphome
esphome Bot commented Aug 4, 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#18059
    components: [bluetooth_proxy, esp32_ble, esp32_ble_tracker]
    refresh: 1h

(Added by the PR bot)

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

@bluetoothbot review

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

Previous review — superseded by a newer review below.

@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 force-pushed the ble_listener_slot_factory branch from b0ef638 to 1047b01 Compare August 4, 2026 05:50
@bdraco
bdraco force-pushed the esp32_ble_slot_counters branch from c5a67be to bdc00c0 Compare August 4, 2026 05:50
@bdraco
bdraco commented Aug 4, 2026
Copy link
Copy Markdown
Member Author

@bluetoothbot review

@bdraco
bdraco force-pushed the ble_listener_slot_factory branch from 1047b01 to 9b37520 Compare August 4, 2026 06:12
@bdraco
bdraco force-pushed the esp32_ble_slot_counters branch from bdc00c0 to 11dad94 Compare August 4, 2026 06:12
@bdraco
bdraco commented Aug 4, 2026
Copy link
Copy Markdown
Member Author

@bluetoothbot review

@codspeed-hq
codspeed-hq Bot commented Aug 4, 2026
Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 153 untouched benchmarks


Comparing esp32_ble_slot_counters (2855922) with dev (1f5df20)1

Open in CodSpeed

Footnotes

  1. No successful run was found on dev (1bae91a) during the generation of this report, so 1f5df20 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@bluetoothbot
Copy link
Copy Markdown
Contributor

@bdraco: @bluetoothbot review

Re-reviewed at 11dad94. Only unrelated test_libretiny.py churn since last pass, so all three prior nits remain: stale comment at esp32_ble/__init__.py:605, slot_counter inside the noqa: F401 re-export block, and undeclared guarding of add_scanner_state_listener. No new issues. Verified automation.h genuinely uses std::vector, and on_scanner_state early-returns on null api_connection_, so codegen-time registration cannot read configured_scan_active_ before setup(). Still merge-ready.

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

Previous review — superseded by a newer review below.

@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 4, 2026
Copy link
Copy Markdown
Member Author

fixed in fdd2da3

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

@bluetoothbot review

@bluetoothbot
Copy link
Copy Markdown
Contributor

@bdraco: fixed in fdd2da3

Confirmed. All three resolved.

  • Stale CORE.add_job comment gone. Import narrowing correct — dataclass and CoroPriority had no other uses.
  • slot_counter now via ble_device_base.slot_counter(...), outside noqa: F401 block. Matches tracker.
  • Header doc names register_scanner_state_listener. API box ticked.

One nit left, non-blocking: proxy test could also assert handler counts stay "1", pinning cross-compile reset directly.

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

Previous review — superseded by a newer review below.

@bdraco
bdraco requested a review from a team as a code owner August 4, 2026 20:08
Copilot AI lite review requested due to automatic review settings August 4, 2026 20:08
@esphome
esphome Bot commented Aug 4, 2026
Copy link
Copy Markdown
Contributor

👋 Hi there! This PR modifies 8 file(s) with codeowners.

@jesserockz, @Rapsssito, @esphome/core - As codeowner(s) of the affected files, your review would be appreciated! 🙏

Note: Automatic review request may have failed, but you're still welcome to review.

Copilot AI 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.

Pull request overview

This PR migrates ESP32 BLE handler/listener slot counting to the shared cg.slot_counter() factory so slot counts are stored in CORE.data (reset between runs) instead of module-level state that can leak across compiles in long-lived processes. It also makes the ESP32 BLE tracker’s scanner-state listener storage codegen-sized (and fully compiled out when unused) while keeping existing define names stable for in-tree consumers.

Changes:

  • Replaced esp32_ble handler registration counters and FINAL define-emission job with per-handler cg.slot_counter() requests.
  • Migrated esp32_ble_tracker listener/client counts (and new scanner-state listener count) to cg.slot_counter(), and switched scanner-state listeners from std::vector to codegen-sized StaticVector guarded by a define.
  • Updated bluetooth_proxy to register as a scanner-state listener via a new esp32_ble_tracker.register_scanner_state_listener() helper, and added component tests + configs to validate define emission and cross-compile count resets.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/component_tests/ble_device_base/test_slot_counter.py Adds assertions for ESP32 BLE handler/listener defines and verifies counts reset between compiles.
tests/component_tests/ble_device_base/config/esp32_tracker_only.yaml Adds a minimal ESP32 IDF tracker-only config for slot counter tests.
tests/component_tests/ble_device_base/config/esp32_bluetooth_proxy.yaml Adds a proxy config to validate scanner-state/listener/client slot define emission.
esphome/core/defines.h Adds the new scanner-state listener count define for static-analysis/IDE builds.
esphome/components/esp32_ble/init.py Switches handler count machinery to cg.slot_counter() requests; removes module-level dataclass counters and FINAL job.
esphome/components/esp32_ble_tracker/esp32_ble_tracker.h Guards scanner-state listener API/storage behind a define and uses StaticVector instead of std::vector.
esphome/components/esp32_ble_tracker/esp32_ble_tracker.cpp Wraps scanner-state listener dispatch in the new compile-time guard.
esphome/components/esp32_ble_tracker/automation.h Adds <vector> include (no longer inherited from the main tracker header).
esphome/components/esp32_ble_tracker/init.py Migrates listener/client counting to cg.slot_counter() and adds register_scanner_state_listener() helper.
esphome/components/bluetooth_proxy/bluetooth_proxy.cpp Removes direct runtime registration of the scanner-state listener from setup().
esphome/components/bluetooth_proxy/init.py Registers the proxy’s scanner-state listener via the new codegen helper.

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

@bluetoothbot review

@bdraco
bdraco force-pushed the esp32_ble_slot_counters branch from 4522f45 to 668792a Compare August 4, 2026 21:05
@bluetoothbot
Copy link
Copy Markdown
Contributor

@bdraco: > ###

Understood. Dropped, not re-raised. Grep confirms bluetooth_proxy sole in-tree caller. No blocking findings remain.

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

Previous review — superseded by a newer review below.

@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 enabled auto-merge (squash) August 4, 2026 22:47
@bdraco
bdraco commented Aug 4, 2026
Copy link
Copy Markdown
Member Author

Thanks

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

@bluetoothbot review

@bdraco
bdraco force-pushed the esp32_ble_slot_counters branch from 668792a to 2855922 Compare August 4, 2026 22:47
@bluetoothbot
Copy link
Copy Markdown
Contributor

PR Review — [esp32_ble] Migrate the BLE count machinery to the shared slot counter factory

Merge-ready. Independently re-verified against the tree; no blocking issues, and nothing new since the last pass.

What this PR gets right, specifically:

  • The cg.slot_counter factory (esphome/cpp_helpers.py:158-198) keeps counts in CORE.data and schedules its FINAL emit job on the first request, so removing CORE.add_job(_add_ble_handler_defines) from esp32_ble.to_code loses nothing — and the request-after-emit ValueError means an undercount fails loudly instead of silently truncating a StaticVector.

  • Every caller of the five register_*_event_handler helpers (esp32_ble_beacon:74, esp32_ble_server:595-596, esp32_ble_tracker:221-224) runs inside a to_code coroutine, and the tracker's remaining FINAL job _add_ble_features only reads counts via cg.get_slot_count. No path can trip the request-after-emit guard.

  • test_counts_reset_between_compiles is a genuine regression test for the leak the PR descri 10BC8 bes — the module-level HandlerCounts() was not reachable by CORE.reset(), so the second compile reported 2 on base.

  • The #ifdef ESPHOME_ESP32_BLE_TRACKER_SCANNER_STATE_LISTENER_COUNT guard is applied consistently on all three sides (method, member, dispatch loop in esp32_ble_tracker.cpp:446), while BLEScannerStateListener itself stays unguarded — required, since bluetooth_proxy.h:57 inherits it unconditionally.

  • Moving the proxy subscription from BluetoothProxy::setup() into codegen is ordering-safe and strictly earlier: cg.add statements land in generated setup() before App.setup(), and on_scanner_state early-returns while api_connection_ is null, so no send can happen before configured_scan_active_ is captured.

  • Dropping the transitive <vector> from esp32_ble_tracker.h is fully paired: I resolved the include graph for every .cpp/.h in esphome/ that uses std::vector, with and without the removal, and automation.h is the only translation unit that regresses — which this PR fixes.

  • No blocking findings.

  • The one prior suggestion (silent StaticVector::push_back drop for external C++ callers of add_scanner_state_listener) stays dropped per @bdraco: "Decline, there are no external callers." Repo-wide grep confirms bluetooth_proxy is still the sole in-tree caller.

  • Scope matches the description exactly; no unrelated changes.



Checklist

  • Handler/listener/client counts identical to base for the same configs
  • Counts reset between compiles in a long-lived host process
  • All slot requests originate from to_code, never from a post-FINAL job
  • No dangling references to removed dataclasses or CORE.data key
  • No unused imports left by the removed FINAL job (CoroPriority kept where still used)
  • Removed transitive include breaks no translation unit (full include-graph check)
  • Guarded C++ members have no remaining unguarded uses
  • Listener registration ordering safe after moving from setup() to codegen
  • New tests assert observable codegen output, not source inspection
  • Test isolation: autouse reset_core plus explicit CORE.reset() in the cross-compile test
  • New define registered in esphome/core/defines.h for static analysis
  • Diff scope matches the PR description; type-of-change boxes match the diff

Automated review by Kōan (Claude) HEAD=2855922 4 min 1s

@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 merged commit ad18bbc into dev Aug 5, 2026
70 of 71 checks passed
@bdraco
bdraco deleted the esp32_ble_slot_counters branch August 5, 2026 00:54
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 6, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

0