diff --git a/custom_components/mysmartbike_ble/const.py b/custom_components/mysmartbike_ble/const.py index 32a09b9..e2fa638 100644 --- a/custom_components/mysmartbike_ble/const.py +++ b/custom_components/mysmartbike_ble/const.py @@ -53,3 +53,8 @@ VOLATILE_FIELDS: Final[dict[str, tuple[str, ...]]] = { "battery_secondary": ("current", "is_charging"), "ebm": ("status", "accel_y", "accel_z"), } + +# A link that survived at least this long is worth reconnecting immediately when +# it drops; anything shorter is left to the regular poll so a bike that cannot +# hold a connection does not spin in a reconnect loop. +MIN_LINK_SECONDS_FOR_FAST_RECONNECT: Final = 5.0 diff --git a/custom_components/mysmartbike_ble/coordinator.py b/custom_components/mysmartbike_ble/coordinator.py index addf0c1..6b0d0e7 100644 --- a/custom_components/mysmartbike_ble/coordinator.py +++ b/custom_components/mysmartbike_ble/coordinator.py @@ -35,6 +35,7 @@ from .const import ( PROTOCOL_REQUEST_MESSAGE, CLOSE_MESSAGE, SCAN_INTERVAL, + MIN_LINK_SECONDS_FOR_FAST_RECONNECT, STORAGE_SAVE_DELAY, STORAGE_VERSION, RESTORE_STATE_KEYS, @@ -84,6 +85,9 @@ class MySmartBikeCoordinator(DataUpdateCoordinator[dict[str, Any]]): self._last_seen: datetime | None = None self._save_armed = False self._unreachable_reason: str | None = None + self._connecting = False + self._advertisements_seen = 0 + self._connected_since: float | None = None self._store: Store[dict[str, Any]] = Store( hass, STORAGE_VERSION, storage_key(entry) ) @@ -213,15 +217,36 @@ class MySmartBikeCoordinator(DataUpdateCoordinator[dict[str, Any]]): BluetoothScanningMode.ACTIVE, ) + @callback + def _async_claim_connect(self) -> bool: + """Reserve the right to run one connect attempt. + + Home Assistant invokes the bluetooth callback on *every* advertisement, + and these bikes advertise several times a second. Without a claim, a + disconnected bike would queue hundreds of connect tasks behind + `_connect_lock`, each running a full `establish_connection` retry cycle + against a proxy that is already struggling to hold the link. + """ + if self._is_connected or self._manual_disconnect or self._connecting: + return False + self._connecting = True + return True + @callback def _async_device_appeared( self, service_info: BluetoothServiceInfoBleak, change: BluetoothChange ) -> None: """Handle the bike showing up in range.""" - if self._is_connected or self._manual_disconnect: + self._advertisements_seen += 1 + if not self._async_claim_connect(): return + _LOGGER.debug( + "Advertisement from %s (%d seen) - connecting now", + self._address, + self._advertisements_seen, + ) self._entry.async_create_background_task( - self.hass, self._async_try_connect(), f"{DOMAIN} connect {self._address}" + self.hass, self._async_run_connect(), f"{DOMAIN} connect {self._address}" ) @callback @@ -239,6 +264,25 @@ class MySmartBikeCoordinator(DataUpdateCoordinator[dict[str, Any]]): self._is_connected = False self.async_update_listeners() + # Don't sit out the poll interval. These bikes stop advertising after an + # unexpected link loss, so the advertisement watch does not fire, and + # every notification has just pushed the poll timer another 30s out - + # leaving the bike disconnected far longer than necessary. + held_for = ( + self.hass.loop.time() - self._connected_since + if self._connected_since is not None + else 0.0 + ) + self._connected_since = None + if held_for < MIN_LINK_SECONDS_FOR_FAST_RECONNECT: + # A link that died almost immediately would spin; let the poll retry. + return + if not self._async_claim_connect(): + return + self._entry.async_create_background_task( + self.hass, self._async_run_connect(), f"{DOMAIN} reconnect {self._address}" + ) + async def async_first_connect(self) -> None: """Attempt the initial connection without blocking setup.""" if self._manual_disconnect: @@ -250,6 +294,12 @@ class MySmartBikeCoordinator(DataUpdateCoordinator[dict[str, Any]]): await self._async_try_connect() async def _async_try_connect(self) -> None: + """Run a connect attempt unless one is already in flight.""" + if not self._async_claim_connect(): + return + await self._async_run_connect() + + async def _async_run_connect(self) -> None: """Connect, reporting the expected 'bike is off' failures once each.""" try: await self._connect() @@ -257,6 +307,8 @@ class MySmartBikeCoordinator(DataUpdateCoordinator[dict[str, Any]]): self._async_report_unreachable(str(ex)) except Exception as ex: # noqa: BLE001 self._async_report_unreachable(f"Connection attempt failed: {ex}") + finally: + self._connecting = False def _async_report_unreachable(self, reason: str) -> None: """Log why we cannot connect - once per distinct reason. @@ -376,6 +428,13 @@ class MySmartBikeCoordinator(DataUpdateCoordinator[dict[str, Any]]): state["rssi"] = None state["last_seen"] = self._last_seen + _LOGGER.debug( + "%s: connected=%s advertisements_seen=%s rssi=%s", + self._address, + self._is_connected, + self._advertisements_seen, + state["rssi"], + ) return state async def _connect(self) -> None: @@ -418,6 +477,7 @@ class MySmartBikeCoordinator(DataUpdateCoordinator[dict[str, Any]]): return self._is_connected = True + self._connected_since = self.hass.loop.time() self._unreachable_reason = None _LOGGER.debug("Connected to %s", self._address) diff --git a/tests/components/mysmartbike_ble/test_restore.py b/tests/components/mysmartbike_ble/test_restore.py index 545348f..106ae50 100644 --- a/tests/components/mysmartbike_ble/test_restore.py +++ b/tests/components/mysmartbike_ble/test_restore.py @@ -269,7 +269,12 @@ async def test_reconnects_when_bike_appears( """An advertisement triggers a connect instead of waiting for the poll.""" await setup_offline(hass, restore_config_entry) coordinator = restore_config_entry.runtime_data - coordinator._is_connected = False + await wait_connected(hass, coordinator) + + # Drop the link the way bleak reports one + coordinator._async_client_disconnected(coordinator._client) + await hass.async_block_till_done() + assert coordinator.is_connected is False with patch.object( coordinator, "_connect", new_callable=AsyncMock @@ -446,3 +451,89 @@ async def test_unreachable_warning_is_not_repeated( await hass.async_block_till_done() assert "turn on the bike" not in caplog.text + + +async def test_advertisement_storm_starts_one_connect_attempt( + hass: HomeAssistant, + restore_config_entry: MockConfigEntry, + mock_bleak_client, + mock_device_in_range, + mock_bluetooth_service_info, +) -> None: + """Home Assistant fires the callback on every advertisement. + + These bikes advertise several times a second, so an unguarded callback + would queue a connect task per advertisement, all serialising behind + _connect_lock and each hammering the proxy with a full retry cycle. + """ + await setup_offline(hass, restore_config_entry) + coordinator = restore_config_entry.runtime_data + await wait_connected(hass, coordinator) + coordinator._async_client_disconnected(coordinator._client) + await hass.async_block_till_done() + + started = 0 + release = asyncio.Event() + + async def slow_connect() -> None: + nonlocal started + started += 1 + await release.wait() + + with patch.object(coordinator, "_connect", side_effect=slow_connect): + for _ in range(50): + coordinator._async_device_appeared(mock_bluetooth_service_info, None) + await asyncio.sleep(0) + await hass.async_block_till_done() + + assert started == 1, f"{started} connect attempts for 50 advertisements" + + release.set() + await hass.async_block_till_done() + + # ...and the claim is released, so a later advertisement can still connect + assert coordinator._connecting is False + + +async def test_fast_reconnect_after_established_link_drops( + hass: HomeAssistant, + restore_config_entry: MockConfigEntry, + mock_bleak_client, + mock_device_in_range, +) -> None: + """A dropped link reconnects at once instead of waiting out the poll. + + These bikes stop advertising after an unexpected disconnect, so the + advertisement watch never fires, and every notification has just pushed the + poll timer another SCAN_INTERVAL out. + """ + await setup_offline(hass, restore_config_entry) + coordinator = restore_config_entry.runtime_data + await wait_connected(hass, coordinator) + + # Pretend the link had been up comfortably longer than the spin guard + coordinator._connected_since = hass.loop.time() - 120 + + with patch.object(coordinator, "_connect", new_callable=AsyncMock) as mock_connect: + coordinator._async_client_disconnected(coordinator._client) + await hass.async_block_till_done() + mock_connect.assert_called_once() + + +async def test_no_fast_reconnect_when_link_died_immediately( + hass: HomeAssistant, + restore_config_entry: MockConfigEntry, + mock_bleak_client, + mock_device_in_range, +) -> None: + """A link that collapses at once must not spin; the poll retries instead.""" + await setup_offline(hass, restore_config_entry) + coordinator = restore_config_entry.runtime_data + await wait_connected(hass, coordinator) + + coordinator._connected_since = hass.loop.time() # just connected + + with patch.object(coordinator, "_connect", new_callable=AsyncMock) as mock_connect: + coordinator._async_client_disconnected(coordinator._client) + await hass.async_block_till_done() + mock_connect.assert_not_called()