Skip to content

Use bleak-retry-connector and write-only move loop for BLE proxy reliability#41

Draft
pedropombeiro wants to merge 2 commits into
abmantis:mainfrom
pedropombeiro:pr/smooth-move-to-target
Draft

Use bleak-retry-connector and write-only move loop for BLE proxy reliability#41
pedropombeiro wants to merge 2 commits into
abmantis:mainfrom
pedropombeiro:pr/smooth-move-to-target

Conversation

@pedropombeiro
Copy link
Copy Markdown
Contributor

Summary

Replace the raw BleakClient.connect() path with bleak_retry_connector.establish_connection() and rewrite move_to_target to eliminate GATT reads during movement, building on the wakeup ordering fix from #40.

Connection improvements

  • add bleak-retry-connector as a dependency
  • use establish_connection() with BleakClientWithServiceCache for proper connection caching
  • introduce ManagedIdasenDesk subclass to avoid creating an unused BleakClient in __init__ and cleanly adopt the externally-established client
  • accept the current BLEDevice in ConnectionManager.connect() so device-swap and reconnect paths stay aligned

Write-only move loop

  • send target height writes every 400ms with response=False (fire-and-forget)
  • detect arrival via BLE notifications from monitor() instead of polling get_height_and_speed()
  • add 15-second safety timeout to prevent infinite move loops

Why

The upstream move_to_target interleaves GATT reads with writes during movement. BLE has a single ATT channel per connection — reads block the channel until the response arrives. Over Bluetooth proxies (HA → WiFi → ESP32 → BLE → desk), read latency is 50–200ms normally but can spike to seconds on timeout/retry, creating unpredictable gaps between reference input writes. The desk interprets these gaps as "stop commanding movement" and decelerates or stops.

By removing reads from the loop, the write cadence becomes deterministic and the ATT channel stays free for outgoing commands. Height data is already pushed to us via the existing monitor() BLE notification subscription.

Ref: #38 (comment: #38 (comment))

Validation

  • verified locally against Home Assistant + ESPHome Bluetooth proxy
  • confirmed the BleakClient.connect() called without bleak-retry-connector warning is gone
  • confirmed the desk reconnects with ESPHome proxy logs showing Connecting v3 with cache
  • confirmed smooth desk movement without interruptions over BLE proxy
  • 51 tests pass

Supersedes #39. Related to home-assistant/core#155912.

Replace the raw BleakClient.connect() path with
bleak_retry_connector.establish_connection(), building on the wakeup
ordering fix from abmantis#40.

- add bleak-retry-connector as a dependency
- use establish_connection() with BleakClientWithServiceCache for
  proper connection caching
- introduce ManagedIdasenDesk subclass to avoid creating an unused
  BleakClient in __init__ and cleanly adopt the externally-established
  client
- accept the current BLEDevice in ConnectionManager.connect() so
  device-swap and reconnect paths stay aligned
Rewrite ManagedIdasenDesk.move_to_target to eliminate GATT reads during
movement. Send target height writes every 200ms with acknowledged
delivery and detect arrival via BLE notifications from the monitor()
subscription instead of polling get_height_and_speed().

The previous implementation interleaved GATT reads with writes. BLE has
a single ATT channel per connection -- reads block the channel until the
response arrives. Over Bluetooth proxies, read latency can spike to
seconds on timeout/retry, creating unpredictable gaps between reference
input writes that cause the desk to stop moving.

Send explicit stop commands (COMMAND_STOP + REFERENCE_INPUT_STOP) when
the move loop exits to cleanly terminate desk movement.

Add a 30-second safety timeout to prevent infinite move loops.

Ref: abmantis#38 (comment)
@pedropombeiro pedropombeiro force-pushed the pr/smooth-move-to-target branch from 715bfc0 to a0e00f3 Compare April 14, 2026 10:34
@abmantis
Copy link
Copy Markdown
Owner

Hey! This is doing too much for a single PR.
Can you please split each feature/fix/change into separate PRs? Also, have you tried suggesting the improvements to IdasenDesk upstream?
It would be cleaner than extending the class here like that, and could be an improvement to the upstream library.

@abmantis abmantis marked this pull request as draft May 18, 2026 21:28
@pedropombeiro
Copy link
Copy Markdown
Contributor Author

Hey! This is doing too much for a single PR. Can you please split each feature/fix/change into separate PRs? Also, have you tried suggesting the improvements to IdasenDesk upstream? It would be cleaner than extending the class here like that, and could be an improvement to the upstream library.

Thanks for the review!

I've split the move-loop change off and proposed it upstream in newAM/idasen#489 — that's a small, self-contained fix that makes move_to_target tolerant of transient speed == 0 readings and reduces the write interval. Once that lands and is released, the desk-stuttering symptom is fixed at the source, and this PR can drop the move_to_target work entirely.

That leaves this PR scoped to the BLE connection path:

  • add bleak-retry-connector as a dependency
  • use establish_connection() with BleakClientWithServiceCache
  • introduce ManagedIdasenDesk only to skip the unused BleakClient in __init__ and adopt an externally-established client via set_client()
  • accept the current BLEDevice in ConnectionManager.connect() so device-swap and reconnect paths stay aligned

I'll trim this PR down to just the connection-path changes once we agree.

@abmantis
Copy link
Copy Markdown
Owner

abmantis commented May 19, 2026

introduce ManagedIdasenDesk only to skip the unused BleakClient in init and adopt an externally-established client via set_client()

I just don't like this part very much 😄 . Can we open another PR to upstream, adding an argument client : BleakClient | None = None to IdasenDesk.__init__()? If it is None, it keeps the existing behavior (create a BleakClient). Otherwise the code gets very fragile to changes in upstream.

@pedropombeiro
Copy link
Copy Markdown
Contributor Author

Good call — that's a cleaner design. I'll open a follow-up upstream PR on newAM/idasen to add client: BleakClient | None = None to IdasenDesk.__init__(). When it's None, the existing behavior is preserved; otherwise the injected client is used as-is.

Once that lands and is released, I'll:

  • drop ManagedIdasenDesk from this PR entirely (use IdasenDesk(..., client=injected_client) directly in ConnectionManager)
  • drop the move-loop changes (already fixed upstream in newAM/idasen#489, released as idasen 0.13.0)
  • bump the idasen floor to whichever upstream version contains the client argument

Will update here once the upstream PR is filed.

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