Skip to content

feat: support ble l2cap connections - #1808

Draft
wiomoc wants to merge 1 commit into
esphome:mainfrom
wiomoc:feat-l2cap-socket
Draft

feat: support ble l2cap connections#1808
wiomoc wants to merge 1 commit into
esphome:mainfrom
wiomoc:feat-l2cap-socket

Conversation

@wiomoc

@wiomoc wiomoc commented Jun 28, 2026

Copy link
Copy Markdown

What does this implement/fix?

https://github.com/orgs/esphome/discussions/3607
early draft ...

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Code quality improvements to existing code or addition of tests
  • Other

Related issue or feature (if applicable):

  • fixes

Pull request in esphome:

Checklist:

  • The code change is tested and works locally.
  • If api.proto was modified, a linked pull request has been made to esphome with the same changes.
  • Tests have been added to verify that the new code works (under tests/ folder).

@wiomoc
wiomoc force-pushed the feat-l2cap-socket branch from 4fcdd26 to 69bff84 Compare July 4, 2026 11:00
@wiomoc
wiomoc force-pushed the feat-l2cap-socket branch 2 times, most recently from 942cf4f to 505720a Compare July 6, 2026 19:27
@esphbot

esphbot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

PR Review — feat: support ble l2cap connections

Promising early draft of BLE/L2CAP transport, but not mergeable — an import-time crash affecting all platforms, a log-name regression that hits TCP too, and frame truncation that corrupts the protocol.

What's solid: the transport abstraction is the right shape — extracting an abstract APIConnection with a _connect_socket_connect/_create_protocol seam and pushing the TCP socket logic down into a ZCAPIConnection subclass is a clean way to add a second transport without touching the hot read/frame paths. The L2CAPSocket ctypes wrapper (sockaddr_l2 layout, bdaddr byte-reversal, EINPROGRESS handling) is a reasonable low-level approach, and the params dataclass split cleanly separates BLE vs zeroconf connection fields. The linked upstream esphome PR is referenced per repo policy.

  • CRITICAL: ctypes.util.find_library is called but only import ctypes was added — ctypes.util is never imported, so connection.py fails to import on every platform (TCP users included).
  • CRITICAL: _set_log_name now passes a single string to build_log_name, which iterates it as a list of addresses — corrupts log names on both TCP and BLE paths.
  • CRITICAL: L2CAPSocket.send truncates frames to the MTU instead of fragmenting — silent data loss that desyncs the length-prefixed protocol.
  • WARNING: HOST_RESOLVED state guard commented out (weakens TCP invariant); except BaseException wraps CancelledError into SocketAPIError, breaking cancellation; main.py debug script with a hardcoded PSK committed to root; reliance on private loop._create_connection_transport.
  • No tests added (checklist unticked) for the new socket/MTU/params logic; main.py scratch harness should be removed before merge.

🔴 Blocking

1. `ctypes.util` is used but never imported — breaks importing the whole package
aioesphomeapi/connection.py:1180-1181

The diff adds import ctypes but references ctypes.util.find_library("c") in the L2CAPSocket class body:

libc = ctypes.CDLL(ctypes.util.find_library("c"), use_errno=True)

import ctypes does not make the ctypes.util submodule available — you must import ctypes.util (or from ctypes.util import find_library) explicitly. Because libc = ... runs at class-definition time, this executes the moment connection.py is imported.

Why it matters: connection.py is imported by the entire package (client.py -> connection). This AttributeError: module 'ctypes' has no attribute 'util' fires on every platform, TCP users included, not just Linux/BLE. The library would fail to import at all. I confirmed there is no import ctypes.util anywhere in the tree.

Fix: add import ctypes.util (or from ctypes.util import find_library), and lazily build libc inside __init__/a module guard so non-Linux platforms that will never use L2CAP don't load libc at import.

libc = ctypes.CDLL(ctypes.util.find_library("c"), use_errno=True)
2. `build_log_name` now receives a single string instead of a list — corrupts log names for TCP and BLE
aioesphomeapi/client_base.py:444-446

The change passes a single string to build_log_name:

self._params.address
if hasattr(self._params, "address")
else self._params.addresses[0],

But build_log_name(name, addresses, connected_address) treats its second argument as a list — it does for address in addresses: and return name or addresses[0] (see util.py:83-89). The original code passed self._params.addresses (the whole list); this changes it to addresses[0], a single string.

Why it matters: iterating a string yields its characters, so build_log_name will loop over "1","9","2",... for "192.168.1.5" and addresses[0] becomes the first character. This is a regression for the TCP path too, not just BLE — every connection's log_name becomes garbage, and the case-insensitive name matching logic mis-fires.

Fix: pass a list in both branches, e.g. [self._params.address] if isinstance(self._params, BLEConnectionParams) else self._params.addresses. Prefer isinstance over hasattr for the discrimination.

self._params.address
if hasattr(self._params, "address")
else self._params.addresses[0],
3. `L2CAPSocket.send` silently truncates frames to the MTU — corrupts the wire protocol
aioesphomeapi/connection.py:1210-1215
def send(self, data):
    # truncate data to MTU size to avoid "Message too long" error
    if len(data) > self._mtu:
        data = data[: self._mtu]
    return super().send(data)

The native API is a length-prefixed protobuf frame protocol. Dropping the tail of any frame larger than the MTU (default 128, negotiated BT_SNDMTU) means the receiver gets a truncated frame and the stream desynchronizes — device_info, list_entities, or any large state message would be silently mangled.

Why it matters: truncation isn't a workaround for "Message too long" — it's data loss. asyncio's transport also assumes send consumes the bytes it reports; returning a short count without the transport re-buffering the remainder loses data even for the framing layer. This will manifest as intermittent, hard-to-debug protocol errors as soon as a frame exceeds the MTU.

Fix: fragment across multiple send() calls (loop over MTU-sized chunks) rather than truncating, or ensure the frame helper's writelines/write path chunks to _mtu. For a SEQPACKET/L2CAP CoC socket, respect the negotiated MTU by segmenting, never dropping.

if len(data) > self._mtu:
    data = data[: self._mtu]
return super().send(data)

🟡 Important

1. State-machine invariant commented out in `start_connection`
aioesphomeapi/connection.py:594-598

The HOST_RESOLVED precondition check is commented out rather than made conditional:

# if self.connection_state is not CONNECTION_STATE_HOST_RESOLVED:
#    raise RuntimeError(
#        "Connection must be in HOST_RESOLVED state to start connection"
#    )

This was disabled because BLE never enters HOST_RESOLVED (it has no resolve step). But commenting it out drops the guard for the TCP path too, weakening a real invariant that catches out-of-order lifecycle calls.

Why it matters: the TCP connection state machine relied on this to fail loudly if start_connection is called before resolution. Silently removing it means a misuse now proceeds into _connect_socket_connect with an empty _addrs_info.

Fix: restore the check for the ZC path (e.g. override or gate on connection type / expected precondition), rather than leaving dead commented code. Delete the comment block once the real guard is in place.

# if self.connection_state is not CONNECTION_STATE_HOST_RESOLVED:
#    raise RuntimeError(
2. `except BaseException` in BLE connect wraps CancelledError into SocketAPIError
aioesphomeapi/connection.py:1400-1405
except TimeoutError:
    raise TimeoutAPIError(f"Timeout while connecting to {addr}")
except BaseException as e:
    raise SocketAPIError(f"Error connecting to {addr}: {e}") from e

except BaseException catches asyncio.CancelledError (and KeyboardInterrupt) and re-raises them as SocketAPIError. The TCP path deliberately preserves CancelledError (see the _raise_fatal_connection_exception handling) so that asyncio.timeout / TaskGroup cancellation semantics are honored.

Why it matters: converting CancelledError to SocketAPIError breaks cooperative cancellation — a caller cancelling the connect coroutine (timeout, shutdown, reconnect abort) will see a bogus socket error instead of the cancellation propagating, and the surrounding interrupt()/timeout machinery can hang or mis-report.

Also: the raise TimeoutAPIError(...) lacks from (loses the chained cause; ruff B904).

Fix: catch (OSError, Exception) and let CancelledError propagate (or explicitly except CancelledError: raise), and add from err on the timeout re-raise.

except BaseException as e:
    raise SocketAPIError(f"Error connecting to {addr}: {e}") from e
3. Debug harness `main.py` committed to repo root with a hardcoded PSK
main.py:1-43

main.py is a manual test script (hardcoded device MAC, switch_command busy-loop, commented-out log subscription) that doesn't belong in the package. It also embeds a real-looking noise_psk credential:

noise_psk="iOZqtvw31Yy6sasRl5h2DElG2VDlqW2WjJEKObVN8bg="

Why it matters: committing a scratch script to the repo root pollutes the package layout and ships a busy-loop example that hammers a switch every 0.5s. The embedded PSK, even for the author's own test device, is a credential in version control — git log keeps it forever.

Fix: drop main.py from the PR (add to .gitignore locally). If an example is wanted, put a credential-free one under an examples/ dir. This is expected for an early draft — just don't let it reach a merge-ready state.

noise_psk="iOZqtvw31Yy6sasRl5h2DElG2VDlqW2WjJEKObVN8bg="
4. Reliance on private asyncio APIs (`_create_connection_transport`, `_create_connection_transport`)
aioesphomeapi/connection.py:1160-1170

BLEAPIConnection._create_protocol calls self._loop._create_connection_transport(...) with ssl_handshake_timeout/ssl_shutdown_timeout kwargs, and the connect path manually drives add_writer/remove_writer around a raw non-blocking connect.

Why it matters: _create_connection_transport is a private CPython asyncio method whose signature has changed across 3.11/3.12/3.13 (the ssl_shutdown_timeout parameter, in particular, was added/moved). Depending on it makes the BLE path silently break on Python version bumps — and this repo targets 3.11+ across several minors.

Fix: prefer loop.connect_accepted_socket / loop.create_connection(sock=...) where possible, or at minimum guard the private-API call with a version check and a test matrix note. Worth a comment explaining why the standard create_connection(sock=...) path (used by the ZC subclass) can't be reused here.

_, protocol = await self._loop._create_connection_transport(
    self._socket, protocol_factory, None, None,
    ssl_handshake_timeout=None, ssl_shutdown_timeout=None,
)

🟢 Suggestions

1. Bare `except:` in socket-cleanup path
aioesphomeapi/connection.py:1385-1390
try:
    ...
except:
    sock.close()
    raise

Bare except: (ruff E722) catches KeyboardInterrupt/SystemExit. It does re-raise, so behavior is mostly fine, but the project runs ruff in CI and this will fail lint.

Fix: use except BaseException: if you truly want to close on any exit, or better a try/finally that closes only on the error path. Given the outer handler already wraps errors, except Exception: is usually the right scope here.

except:
    sock.close()
    raise
2. `start_resolve_host` BLE branch is a no-op that couples to the disabled state check
aioesphomeapi/client.py:337-355

In the else (BLE) branch of start_resolve_host, a BLEAPIConnection is constructed but no coroutine is executed — the method returns without advancing the connection to HOST_RESOLVED. This is why the HOST_RESOLVED guard in connection.start_connection had to be commented out (see the separate finding there).

Why it matters: the two changes are entangled — the BLE path works only because a TCP-path invariant was globally disabled. That's fragile and makes the state machine harder to reason about.

Suggestion: model BLE explicitly. Either give BLEAPIConnection a start_resolve_host that transitions straight to HOST_RESOLVED (no-op resolve), keeping the guard intact for both paths, or introduce a distinct BLE lifecycle. Naming: ZCAPIConnection/ZCConnectionParams ("ZC" = zeroconf) reads as an abbreviation; consider TCPAPIConnection/TCPConnectionParams for clarity since the distinction is really transport (TCP vs L2CAP), not discovery.

else:
    self._connection = BLEAPIConnection(...)
    # no coro executed; state never reaches HOST_RESOLVED

Checklist

  • Package imports cleanly on all platforms — critical #1
  • Log-name construction unchanged for existing TCP path — critical #2
  • Wire protocol integrity preserved (no frame truncation) — critical #3
  • Connection state-machine invariants intact — warning #1
  • Cancellation semantics preserved in connect path — warning #2
  • No hardcoded secrets / scratch files committed — warning #3
  • No reliance on private/unstable stdlib APIs — warning #4
  • No bare except / lint-clean — suggestion #1
  • Tests added for new functionality — suggestion #2

To rebase specific severity levels, mention me: @esphbot rebase critical (fixes 🔴 only), @esphbot rebase important (fixes 🔴 + 🟡), or just @esphbot rebase for all.


Silent Failure Analysis

🟠 **HIGH** — silent data truncation
aioesphomeapi/connection.py:1290-1296

Risk: On this SEQPACKET/L2CAP socket the bytes past the MTU are silently dropped rather than chunked or flushed, so oversized protobuf frames are corrupted on the wire with no error surfaced to the caller.

def send(self, data: bytes | bytearray | memoryview) -> int:
    # truncate data to MTU size to avoid "Message too long" error
    if len(data) > self._mtu:
        data = data[: self._mtu]
    return super().send(data)

Fix: Loop to send the remaining bytes across multiple packets (or raise if a single message exceeds the negotiated MTU) instead of discarding the tail.

🟠 **HIGH** — dead error branch / removed validation
aioesphomeapi/connection.py:596-600

Risk: Commenting out the state guard means a connection whose resolve step failed or was skipped (e.g. TCP path where start_resolve_host never ran) proceeds into socket connect in an invalid state instead of failing loudly.

# if self.connection_state is not CONNECTION_STATE_HOST_RESOLVED:
#    raise RuntimeError(
#        "Connection must be in HOST_RESOLVED state to start connection"
#    )

Fix: Restore a state check that accepts the valid pre-connect states for both TCP and BLE, rather than deleting the guard entirely.

🟡 **MEDIUM** — resource leak on error path
aioesphomeapi/connection.py:1400-1420

Risk: If the asyncio_timeout fires (or await fut is cancelled) before writable() runs, the registered writer is never removed via loop.remove_writer, leaving a dangling callback on a closed/reused fd.

loop.add_writer(sock.fileno(), writable)
await fut
...
except:
    sock.close()
    raise

Fix: Wrap the await in try/finally that calls loop.remove_writer(sock.fileno()) before closing the socket.

🟡 **MEDIUM** — silent security check bypass
aioesphomeapi/connection.py:416-425

Risk: For BLE (Noise) connections the expected_mac verification is silently forced to None, disabling MAC identity checking without any log or error, so a mismatched/spoofed device is accepted.

expected_mac=self._params.expected_mac
if isinstance(self._params, ZCConnectionParams)
else None,

Fix: Give BLEConnectionParams an expected_mac (or explicitly document and log that MAC verification is intentionally unsupported on BLE).

🟡 **MEDIUM** — silent no-op on control path
aioesphomeapi/client.py:345-355

Risk: The BLE branch constructs the connection but never advances connection_state past INITIALIZED, and because the HOST_RESOLVED guard in start_connection was commented out, the missing step is invisible instead of being enforced or logged.

else:
    self._connection = BLEAPIConnection(...)
    # no _execute_connection_coro / resolve call

Fix: Explicitly transition the BLE connection to a defined ready state (or run a BLE-specific prepare step) so the lifecycle is observable rather than an implicit skip.

🟡 **MEDIUM** — lost exception context
aioesphomeapi/connection.py:1425-1428

Risk: Re-raising without from drops the original traceback/cause, hiding the underlying failure (e.g. which syscall timed out) during diagnosis.

except TimeoutError:
    raise TimeoutAPIError(f"Timeout while connecting to {addr}")

Fix: Use raise TimeoutAPIError(...) from err to preserve the original exception chain.


Automated review by Kōan (Claude) HEAD=942cf4f 5 min

@esphbot esphbot 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.

Blocking issues found.

  • ctypes.util is used but never imported — breaks importing the whole package
  • build_log_name now receives a single string instead of a list — corrupts log names for TCP and BLE
  • L2CAPSocket.send silently truncates frames to the MTU — corrupts the wire protocol
  • State-machine invariant commented out in start_connection
  • except BaseException in BLE connect wraps CancelledError into SocketAPIError
  • Debug harness main.py committed to repo root with a hardcoded PSK
  • Reliance on private asyncio APIs (_create_connection_transport, _create_connection_transport)

@wiomoc
wiomoc force-pushed the feat-l2cap-socket branch 4 times, most recently from 9f9f410 to 5bdcd51 Compare July 6, 2026 21:08
@wiomoc
wiomoc force-pushed the feat-l2cap-socket branch 3 times, most recently from 5caf1d5 to 56e0f76 Compare July 9, 2026 23:06
@codecov

codecov Bot commented Jul 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 64.28571% with 85 lines in your changes missing coverage. Please review.
✅ Project coverage is 98.04%. Comparing base (99aabeb) to head (1c04d35).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
aioesphomeapi/connection.py 60.65% 72 Missing ⚠️
aioesphomeapi/client_base.py 40.00% 12 Missing ⚠️
aioesphomeapi/client.py 80.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##              main    #1808      +/-   ##
===========================================
- Coverage   100.00%   98.04%   -1.96%     
===========================================
  Files           26       26              
  Lines         4204     4347     +143     
===========================================
+ Hits          4204     4262      +58     
- Misses           0       85      +85     

☔ View full report in Codecov by Harness.
📢 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.

@wiomoc
wiomoc force-pushed the feat-l2cap-socket branch from 56e0f76 to fe165d9 Compare July 9, 2026 23:26
@wiomoc
wiomoc force-pushed the feat-l2cap-socket branch from fe165d9 to 1c04d35 Compare July 9, 2026 23:36
@codspeed-hq

codspeed-hq Bot commented Jul 9, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 56 untouched benchmarks


Comparing wiomoc:feat-l2cap-socket (1c04d35) with main (61f3e47)

Open in CodSpeed

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