Skip to content

feat: add async_address_reachability_diagnostics for unreachable devices - #517

Merged
bdraco merged 8 commits into
mainfrom
add-address-reachability-diagnostics
May 29, 2026
Merged

feat: add async_address_reachability_diagnostics for unreachable devices#517
bdraco merged 8 commits into
mainfrom
add-address-reachability-diagnostics

Conversation

@bdraco

@bdraco bdraco commented May 29, 2026

Copy link
Copy Markdown
Member

When a device cannot be found, callers only get a bare "not found" with no way to tell why. This adds BluetoothManager.async_address_reachability_diagnostics(address, intent), a read only, side effect free helper that returns a human readable summary tailored to what the caller needs.

The intent comes from a new BluetoothReachabilityIntent enum; PASSIVE_ADVERTISEMENT and ACTIVE_ADVERTISEMENT (scan response, treated the same as passive for now) only report whether the device is being seen, while CONNECTION also reports connectable history, whether a connectable path exists, per scanner failures, in progress connections and slot allocations.

Every result includes a scanner availability summary; how many scanners are registered, scanning and connectable. It calls out the common failure where scanners pause scanning while connecting, so if they are all stuck retrying connections no advertisements can be received at all and the device vanishes; in that case it advises adding more adapters or proxies since the available ones are overloaded.

The wrappers "No backend with an available connection slot" BleakError now embeds the connection intent summary so the cause shows up in logs.

Meant to be surfaced by Home Assistant (switchbot reports "Could not find Switchbot ... with address X" with no reason today); see home-assistant/core#170232.

@codecov

codecov Bot commented May 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.24%. Comparing base (fa10a13) to head (3904a5f).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #517      +/-   ##
==========================================
+ Coverage   98.19%   98.24%   +0.05%     
==========================================
  Files          15       15              
  Lines        2605     2682      +77     
  Branches      367      385      +18     
==========================================
+ Hits         2558     2635      +77     
  Misses         21       21              
  Partials       26       26              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@codspeed-hq

codspeed-hq Bot commented May 29, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 24 untouched benchmarks
⏩ 5 skipped benchmarks1


Comparing add-address-reachability-diagnostics (3904a5f) with main (fa10a13)

Open in CodSpeed

Footnotes

  1. 5 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@bluetoothbot

bluetoothbot commented May 29, 2026

Copy link
Copy Markdown
Contributor

PR Review — feat: add async_address_reachability_diagnostics for unreachable devices

Solid, well-scoped diagnostics-first change in the spirit of the prior PR #416 learning. The new method is explicitly read-only, side-effect free, documented as cold-path-only, and the returned string is documented as unstable so callers don't grow a parsing habit. The try/except Exception wrap around the diagnostics call in wrappers.py is the right shape: it uses _LOGGER.exception (no swallowed traceback), only ever appends to msg, and has its own dedicated test (test_no_backend_error_survives_diagnostics_failure) proving the original BleakError is never masked. The new public BaseHaScanner accessors (connections_in_progress, connection_failures, connecting_count) cleanly replace the underscore-prefixed access in wrappers.py without needing .pxd changes (they're pure-Python wrappers / a property over an existing cdef public field). Test coverage is comprehensive — connectable-in-history, non-connectable-only, pure advertisement intent, never-seen, no-connectable-scanners, out-of-slots, history-without-scanner-cache, all-paused-connecting, and stopped-but-not-connecting all have explicit cases. BluetoothReachabilityIntent is exported in __all__. Only two small nits, both non-blocking: a redundant filter/allocation lookup in the cold path, and the easy-to-confuse naming between the two _connecting* accessors.


🟢 Suggestions

1. Duplicate iteration / repeated get_allocations() in cold path (`src/habluetooth/manager.py`, L1080-1115)

Minor: _append_connection_diagnostics filters devices into connectable_devices and non_connectable_devices with two separate list comprehensions, then for each connectable device calls scanner.get_allocations() once here to build reported, and again for the same scanner in the detail loop in async_address_reachability_diagnostics. This is a documented cold path (only called on the BleakError branch), so the perf cost is negligible — but a single pass that captures both the partition and the allocation result per scanner would also remove the risk of the two calls disagreeing if anything mutates allocations between them. Purely a polish suggestion, not blocking.

connectable_devices = [d for d in devices if d.scanner.connectable]
non_connectable_devices = [d for d in devices if not d.scanner.connectable]
...
2. `connecting_count` vs `connections_in_progress()` are easy to confuse (`src/habluetooth/base_scanner.py`, L270-281)

These are two related-but-different counters now both exposed publicly with similar-looking names:

  • connecting_count (property) = self._connecting, the scanning-pause counter from the connecting() context manager
  • connections_in_progress() (method) = sum of _connect_in_progress.values(), per-address in-flight attempts

The docstrings call this out clearly, but the names alone don't, and a future caller is likely to grab whichever shows up first in autocomplete. Optional rename to something like paused_for_connecting / scan_pause_count for the property would make the distinction self-documenting; not blocking since the docstrings disambiguate.

@property
def connecting_count(self) -> int:
    """..."""
    return self._connecting

Checklist

  • No SQL/command injection or shell interpolation of user input
  • No hardcoded secrets or credentials
  • No unsafe deserialization
  • No path traversal risk
  • Input validation at boundaries (enum-typed intent param)
  • No bare except / silent exception swallowing
  • Cleanup in error paths
  • No resource leaks
  • Error messages don't leak sensitive internals
  • No N+1 / repeated I/O in hot loops (cold path, documented) — suggestion #1
  • No unbounded collections
  • Edge case coverage (unknown, history-only, all-busy, stopped)
  • Tests assert observable behavior, not source code presence
  • Cython .pxd updated where needed (no cdef field/method changes here)
  • New public API exported in all

Summary

Solid, well-scoped diagnostics-first change in the spirit of the prior PR #416 learning. The new method is explicitly read-only, side-effect free, documented as cold-path-only, and the returned string is documented as unstable so callers don't grow a parsing habit. The try/except Exception wrap around the diagnostics call in wrappers.py is the right shape: it uses _LOGGER.exception (no swallowed traceback), only ever appends to msg, and has its own dedicated test (test_no_backend_error_survives_diagnostics_failure) proving the original BleakError is never masked. The new public BaseHaScanner accessors (connections_in_progress, connection_failures, connecting_count) cleanly replace the underscore-prefixed access in wrappers.py without needing .pxd changes (they're pure-Python wrappers / a property over an existing cdef public field). Test coverage is comprehensive — connectable-in-history, non-connectable-only, pure advertisement intent, never-seen, no-connectable-scanners, out-of-slots, history-without-scanner-cache, all-paused-connecting, and stopped-but-not-connecting all have explicit cases. BluetoothReachabilityIntent is exported in __all__. Only two small nits, both non-blocking: a redundant filter/allocation lookup in the cold path, and the easy-to-confuse naming between the two _connecting* accessors.


Automated review by Kōan80f6c1b
bd3918b
dd829fd
55aff19
f6d7d59
d950a5d
a330206
3904a5f

@bdraco
bdraco marked this pull request as ready for review May 29, 2026 15:31
@bdraco
bdraco merged commit 2429ce8 into main May 29, 2026
42 checks passed
@bdraco
bdraco deleted the add-address-reachability-diagnostics branch May 29, 2026 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants