From 47669db5537aad976fba751bc1b924e1f2735566 Mon Sep 17 00:00:00 2001 From: firstmate crewmate Date: Fri, 10 Jul 2026 13:00:49 +1000 Subject: [PATCH 1/3] perf(ble): return early on terminal VCSEC actuation acks A VCSEC actuation (RKE/closure/wake via _sendVehicleSecurity) replies with a single bare ACK and no data frame, whereas a VCSEC read replies with a bare ACK then a data frame. _send cannot tell the two apart at transport level, so every successful actuation waited out the full _ack_followup_timeout (~2s) before returning, and a lost ack blocked the full 5s outer timeout. The caller knows what it sent, so thread an explicit expects_data hint from _sendVehicleSecurity (expects_data=False) through _command into _send. With expects_data=False, _send returns immediately on the matching terminal ack and, on a lost ack, raises BluetoothTimeout after a shorter _actuation_timeout (2s) instead of _default_timeout (5s) - the verify-by-state contract makes a longer wait pointless. Reads, infotainment commands, handshake and pair keep expects_data=True and today's exact behavior. The WAIT/fault retry threads the hint through unchanged. Tests drive the real _send state machine (immediate terminal-ack return, read follow-up wait unchanged, shortened lost-ack timeout for actuations, default timeout preserved for reads) plus the expects_data threading and unchanged WAIT retry over the mocked transport. --- AGENTS.md | 1 + tesla_fleet_api/tesla/vehicle/bluetooth.py | 37 +++++++-- tesla_fleet_api/tesla/vehicle/commands.py | 46 ++++++++--- tesla_fleet_api/tesla/vehicle/signed.py | 7 +- tests/test_ble_expects_data.py | 89 ++++++++++++++++++++++ tests/test_ble_send_transport.py | 83 ++++++++++++++++++++ 6 files changed, 247 insertions(+), 16 deletions(-) create mode 100644 tests/test_ble_expects_data.py diff --git a/AGENTS.md b/AGENTS.md index 02e2ae5..1d7e120 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -129,6 +129,7 @@ Source protobuf definitions live in `proto/`; generated Python files live in `te - **BLE mutating-command timeout is inconclusive - never assume "the write didn't land"**: this corrects an earlier version of this note, which observed `adjust_volume` write timeouts on one rig leaving the car unmutated and concluded a write timeout means the write never landed. Later live testing disproved that as a general rule: `door_unlock` and `door_lock` each raised `BluetoothTimeout` (~5.5-5.9s) yet both physically executed - a VCSEC state read after each confirmed the lock state had flipped. Reads (state readers, `wake_up()`'s effect) came back reliably all night; only mutating VCSEC/RKE actions showed this false-negative pattern, most likely because the vehicle doesn't reliably return an ack `_send()` observes within the timeout, not because the write failed. Treat a `BluetoothTimeout` from any mutating BLE command as **inconclusive, not failure**: snapshot state before acting, then verify the outcome with a follow-up state read whenever a mutation times out. Never blind-retry a non-idempotent command (toggles like `media_toggle_playback`, volume steps, schedule add/remove) on timeout alone - see the retry-double-execution entry below for why the library's own retry has the same exposure. The `adjust_volume`/`TimeoutAPIError` root cause from the original observation is still unexplained; it just isn't evidence that timed-out writes never land. - **`wake_up()`'s own ACK is an unreliable signal - almost always a false-negative timeout**: live-verified 9/9 across two independent sessions - `wake_up()` raises `BluetoothTimeout` regardless of whether the vehicle was asleep, already awake, freshly connected, or held open for minutes, yet the wake (or no-op) reliably took effect - an immediate follow-up state read on the same connection succeeds. Never treat a `wake_up()` `BluetoothTimeout` as command failure; call it best-effort (catch and ignore) and confirm readiness by retrying a cheap INFO read instead (see the boot-delay gotcha above for why the first INFO read still needs its own retry/backoff). Hold one connection across a whole batch of related commands rather than reconnecting between each - reconnecting costs ~123% more per operation with no demonstrated wake-preservation benefit from the connection alone. - **The signed-command retry in `Commands._command` can double-execute a mutating command**: on an `OPERATIONSTATUS_WAIT` reply or an `INCORRECT_EPOCH`/`INVALID_TOKEN` fault, `_command` (`commands.py`) re-signs and re-sends the identical command, bounded at 3 attempts then a clean `{"result": False, "reason": "Too many retries"}` - the cap itself is safe and doesn't loop. Live-verified deterministically: a WAIT-then-OK sequence produces 2 physical sends of the same command on the wire. Combined with the mutating-timeout-is-inconclusive gotcha above (a command can execute despite a WAIT/fault reply), this retry is a latent double-apply window. Harmless for a naturally idempotent command (lock/unlock), a real correctness risk for toggles and step commands (`media_toggle_playback`, `media_volume_up`/`down`, schedule add/remove) - verify those by absolute state after the call, never by counting invocations or trusting the retry to be safe. +- **`expects_data` splits BLE reply-waiting: VCSEC actuations return on the terminal ack**: a VCSEC read replies with a bare ACK **then** a data frame, but a VCSEC actuation (RKE/closure/wake via `_sendVehicleSecurity`) replies with a **single bare ACK only** - `_send` cannot tell the two apart at transport level, so the caller declares it. `_sendVehicleSecurity` passes `expects_data=False` down through `_command` into `_send` (`commands.py`/`bluetooth.py`); everything else (VCSEC reads via `_getVehicleSecurity`, all infotainment via `_send/_getInfotainment`, `_handshake`, `pair`) keeps the default `expects_data=True`. With `expects_data=False`, `_send` returns immediately on the matching ACK instead of waiting out `_ack_followup_timeout` (was a flat ~2s tax on every VCSEC actuation), and on a lost ack it raises `BluetoothTimeout` after the shorter `_actuation_timeout` (2s) rather than `_default_timeout` (5s) - the verify-by-state contract makes a longer wait pointless. Infotainment actuations never paid the tax (their `actionStatus` rides in `protobuf_message_as_bytes`, so `_send` returns on that data frame). The WAIT/fault retry threads `expects_data` through unchanged, so the double-execute exposure noted above is unaffected. - **`navigation_gps_request`'s `order` param is a raw int, not a callable enum**: `commands.py` used to build it as `NavigationGpsRequest.RemoteNavTripOrder(order)`, treating the protobuf nested-enum wrapper (`EnumTypeWrapper`) as if it were a callable Python `enum.IntEnum` class - it isn't, so every call raised `TypeError` before any message was sent (found live during PR-8; this method had never been exercised over BLE before). Fixed to pass `order=order` directly (matching the working sibling `navigation_gps_destination_request`), which protobuf accepts as a bare int for an enum field at runtime. - **`ReassemblingBuffer` resets on a >1s inter-chunk gap, not just on decode failure**: `bluetooth.py`'s `ReassemblingBuffer.receive_data` discards any in-progress partial frame if the next chunk arrives more than `STALE_CHUNK_TIMEOUT` (1s) after the previous one, mirroring Tesla's official Go SDK (`teslamotors/vehicle-command`, `pkg/connector/ble/ble.go`'s `rxTimeout`). Without this, a chunk dropped mid-message left a stale partial in the buffer that got prepended to the next message, corrupting it until a lucky decode failure resynced. This is a frame-integrity hardening, not a fix for the separate ack-loss behavior documented above (that's a stalled/silent link, which no buffer-side reset can recover). diff --git a/tesla_fleet_api/tesla/vehicle/bluetooth.py b/tesla_fleet_api/tesla/vehicle/bluetooth.py index 8a2a451..9c441be 100644 --- a/tesla_fleet_api/tesla/vehicle/bluetooth.py +++ b/tesla_fleet_api/tesla/vehicle/bluetooth.py @@ -221,6 +221,10 @@ class VehicleBluetooth(Commands[BluetoothParentT], Generic[BluetoothParentT]): _buffer: ReassemblingBuffer _auth_method = "aes" _ack_followup_timeout: float = 2 + _default_timeout: float = 5 + # A lost actuation ack is inconclusive; the contract is verify-by-state, so + # a shorter wait than a data read's before raising loses nothing. + _actuation_timeout: float = 2 def __init__( self, @@ -355,9 +359,22 @@ def _on_message(self, msg: RoutableMessage) -> None: queue.put_nowait(msg) async def _send( - self, msg: RoutableMessage, requires: str, timeout: int = 5 + self, + msg: RoutableMessage, + requires: str, + expects_data: bool = True, + *, + timeout: float | None = None, ) -> RoutableMessage: - """Serialize a message and send to the vehicle and wait for a response.""" + """Serialize a message and send to the vehicle and wait for a response. + + When ``expects_data`` is False the reply is a single terminal ack (a + VCSEC actuation, no data frame follows), so the ack is returned as soon + as it arrives and a shorter total timeout applies. + """ + + if timeout is None: + timeout = self._default_timeout if expects_data else self._actuation_timeout domain = msg.to_destination.domain async with self._sessions[domain].lock: @@ -397,6 +414,10 @@ async def _send( # Some commands (e.g. RKE wake/lock) only return an ACK with no data. # Wait briefly for a follow-up data response before returning the ACK. if msg.uuid and resp.request_uuid == msg.uuid: + if not expects_data: + # A VCSEC actuation's ack is terminal; no data + # frame follows, so return without the wait. + return resp LOGGER.debug( "Received ACK for our request, waiting briefly for data follow-up" ) @@ -549,11 +570,17 @@ async def query_version(self) -> int | None: return None async def _command( - self, domain: Domain, command: bytes, attempt: int = 0 + self, + domain: Domain, + command: bytes, + attempt: int = 0, + expects_data: bool = True, ) -> dict[str, Any]: """Serialize a message and send to the signed command endpoint.""" await self.connect_if_needed() - return await super()._command(domain, command, attempt) + return await super()._command( + domain, command, attempt, expects_data=expects_data + ) async def pair( self, @@ -577,7 +604,7 @@ async def pair( protobuf_message_as_bytes=request.SerializeToString(), uuid=randbytes(16), ) - resp = await self._send(msg, "protobuf_message_as_bytes", timeout) + resp = await self._send(msg, "protobuf_message_as_bytes", timeout=timeout) respMsg = FromVCSECMessage.FromString(resp.protobuf_message_as_bytes) if respMsg.commandStatus.whitelistOperationStatus.whitelistOperationInformation: if ( diff --git a/tesla_fleet_api/tesla/vehicle/commands.py b/tesla_fleet_api/tesla/vehicle/commands.py index 0cca9d2..392206a 100644 --- a/tesla_fleet_api/tesla/vehicle/commands.py +++ b/tesla_fleet_api/tesla/vehicle/commands.py @@ -331,8 +331,15 @@ def shared_key(self, vehicleKey: bytes) -> bytes: return hashlib.sha1(exchange).digest()[:16] @abstractmethod - async def _send(self, msg: RoutableMessage, requires: str) -> RoutableMessage: - """Transmit the message to the vehicle.""" + async def _send( + self, msg: RoutableMessage, requires: str, expects_data: bool = True + ) -> RoutableMessage: + """Transmit the message to the vehicle. + + ``expects_data`` is False when the reply is a single terminal ack with + no following data frame (a VCSEC actuation), letting a framing + transport return on that ack instead of waiting out a data follow-up. + """ raise NotImplementedError def validate_msg(self, msg: RoutableMessage) -> None: @@ -352,7 +359,11 @@ def validate_msg(self, msg: RoutableMessage) -> None: raise exception async def _command( - self, domain: Domain, command: bytes, attempt: int = 0 + self, + domain: Domain, + command: bytes, + attempt: int = 0, + expects_data: bool = True, ) -> dict[str, Any]: """Serialize a message and send to the signed command endpoint. @@ -360,6 +371,9 @@ async def _command( the identical command (bounded at 3 attempts) - for a non-idempotent command that window can apply it twice if the first attempt actually executed despite the WAIT/fault reply. + + ``expects_data`` is threaded to ``_send`` and every retry; it is False + for a VCSEC actuation, whose reply is a bare terminal ack. """ session = self._sessions[domain] if not session.ready: @@ -374,7 +388,9 @@ async def _command( raise ValueError(f"Unknown auth method: {self._auth_method}") try: - resp = await self._send(msg, "protobuf_message_as_bytes") + resp = await self._send( + msg, "protobuf_message_as_bytes", expects_data=expects_data + ) except ( # TeslaFleetMessageFaultInvalidSignature, TeslaFleetMessageFaultIncorrectEpoch, @@ -384,7 +400,9 @@ async def _command( if attempt > 3: # We tried 3 times, give up, raise the error raise e - return await self._command(domain, command, attempt) + return await self._command( + domain, command, attempt, expects_data=expects_data + ) if ( resp.signedMessageStatus.operation_status @@ -396,7 +414,9 @@ async def _command( return {"response": {"result": False, "reason": "Too many retries"}} async with session.lock: await sleep(2) - return await self._command(domain, command, attempt) + return await self._command( + domain, command, attempt, expects_data=expects_data + ) if resp.HasField("protobuf_message_as_bytes"): # decrypt @@ -498,7 +518,9 @@ async def _command( } async with session.lock: await sleep(2) - return await self._command(domain, command, attempt) + return await self._command( + domain, command, attempt, expects_data=expects_data + ) elif ( vcsec.commandStatus.operationStatus == OperationStatus_E.OPERATIONSTATUS_ERROR @@ -652,9 +674,15 @@ async def _commandAes( ) async def _sendVehicleSecurity(self, command: UnsignedMessage) -> dict[str, Any]: - """Sign and send a message to Infotainment computer.""" + """Sign and send an actuation to the Vehicle Security computer. + + A VCSEC actuation replies with a single terminal ack and no data frame, + so ``expects_data=False`` lets ``_send`` return on that ack. + """ return await self._command( - Domain.DOMAIN_VEHICLE_SECURITY, command.SerializeToString() + Domain.DOMAIN_VEHICLE_SECURITY, + command.SerializeToString(), + expects_data=False, ) async def _getVehicleSecurity(self, command: UnsignedMessage) -> VehicleStatus: diff --git a/tesla_fleet_api/tesla/vehicle/signed.py b/tesla_fleet_api/tesla/vehicle/signed.py index 8523939..4ab7771 100644 --- a/tesla_fleet_api/tesla/vehicle/signed.py +++ b/tesla_fleet_api/tesla/vehicle/signed.py @@ -27,9 +27,12 @@ def __init__(self, parent: SignedParentT, vin: str): super(Commands, self).__init__(parent, vin) - async def _send(self, msg: RoutableMessage, requires: str) -> RoutableMessage: + async def _send( + self, msg: RoutableMessage, requires: str, expects_data: bool = True + ) -> RoutableMessage: """Serialize a message and send to the signed command endpoint.""" - # requires isnt used because Fleet API messages are singular + # requires and expects_data are unused: Fleet API replies are singular, + # delivered whole in one response with no separate terminal ack frame. async with self._sessions[msg.to_destination.domain].lock: json = await self.signed_command( diff --git a/tests/test_ble_expects_data.py b/tests/test_ble_expects_data.py new file mode 100644 index 0000000..f58b52c --- /dev/null +++ b/tests/test_ble_expects_data.py @@ -0,0 +1,89 @@ +"""Tests that the ``expects_data`` hint is threaded from the command layer into +``_send`` and preserved across the WAIT retry. + +Uses the mocked-transport base (``_send`` is an ``AsyncMock``) to inspect the +flag each command passes without a real BLE connection. A VCSEC actuation +replies with a bare terminal ack, so it must send ``expects_data=False``; reads +and infotainment actions expect a data frame and keep ``expects_data=True``. +""" + +from __future__ import annotations + +from unittest.mock import AsyncMock, patch + +from tesla_fleet_api.tesla.vehicle.proto.universal_message_pb2 import ( + Destination, + Domain, + MessageStatus, + OperationStatus_E, + RoutableMessage, +) +from tesla_fleet_api.tesla.vehicle.proto.vcsec_pb2 import VehicleStatus + +from ble_mocked_transport import ( + MockedBleTransportTestCase, + infotainment_action_ok_reply, + vcsec_ok_reply, + vcsec_vehicle_status_reply, +) + + +def _expects_data(send: AsyncMock) -> bool: + """Return the ``expects_data`` kwarg of the most recent ``_send`` call.""" + await_args = send.await_args + assert await_args is not None + return await_args.kwargs["expects_data"] + + +def vcsec_wait_reply() -> RoutableMessage: + """A canned VCSEC reply asking the caller to retry (OPERATIONSTATUS_WAIT).""" + return RoutableMessage( + from_destination=Destination(domain=Domain.DOMAIN_VEHICLE_SECURITY), + signedMessageStatus=MessageStatus( + operation_status=OperationStatus_E.OPERATIONSTATUS_WAIT + ), + ) + + +class ExpectsDataFlagTests(MockedBleTransportTestCase): + async def test_vcsec_actuation_sends_expects_data_false(self) -> None: + vehicle, send = self.make_vehicle() + send.return_value = vcsec_ok_reply() + + await vehicle.door_lock() + + self.assertFalse(_expects_data(send)) + + async def test_infotainment_action_sends_expects_data_true(self) -> None: + vehicle, send = self.make_vehicle() + send.return_value = infotainment_action_ok_reply() + + await vehicle.charge_start() + + self.assertTrue(_expects_data(send)) + + async def test_vcsec_read_sends_expects_data_true(self) -> None: + vehicle, send = self.make_vehicle() + send.return_value = vcsec_vehicle_status_reply(VehicleStatus()) + + await vehicle.vehicle_state() + + self.assertTrue(_expects_data(send)) + + +class ExpectsDataRetryTests(MockedBleTransportTestCase): + async def test_wait_reply_retries_and_preserves_expects_data(self) -> None: + vehicle, send = self.make_vehicle() + send.side_effect = [vcsec_wait_reply(), vcsec_ok_reply()] + + # Patch out the 2s inter-retry sleep so the retry path runs instantly. + with patch( + "tesla_fleet_api.tesla.vehicle.commands.sleep", AsyncMock() + ) as sleep: + result = await vehicle.door_lock() + + self.assertEqual(result, {"response": {"result": True, "reason": ""}}) + self.assertEqual(send.await_count, 2) + sleep.assert_awaited_once() + for call in send.await_args_list: + self.assertFalse(call.kwargs["expects_data"]) diff --git a/tests/test_ble_send_transport.py b/tests/test_ble_send_transport.py index c51b2c4..f0a7c94 100644 --- a/tests/test_ble_send_transport.py +++ b/tests/test_ble_send_transport.py @@ -9,6 +9,7 @@ from __future__ import annotations +import asyncio from unittest import IsolatedAsyncioTestCase from unittest.mock import AsyncMock, MagicMock, patch @@ -126,6 +127,88 @@ async def fake_write(_uuid, _payload, _response): self.assertEqual(result, data) +class SendActuationEarlyReturnTests(IsolatedAsyncioTestCase): + """expects_data=False: a bare terminal actuation ack returns immediately.""" + + async def test_terminal_ack_returns_without_waiting_followup(self) -> None: + vehicle = _make_vehicle() + # A large follow-up window would dominate if the early return regressed; + # wait_for below fails fast instead of hanging the suite. + vehicle._ack_followup_timeout = 30 + msg = _outgoing() + ack = RoutableMessage( + from_destination=Destination(domain=DOMAIN), request_uuid=msg.uuid + ) + + async def fake_write(_uuid, _payload, _response): + vehicle._queues[DOMAIN].put_nowait(ack) + + vehicle.client.write_gatt_char = AsyncMock(side_effect=fake_write) + + result = await asyncio.wait_for( + vehicle._send(msg, "protobuf_message_as_bytes", expects_data=False), + timeout=1.0, + ) + + self.assertEqual(result, ack) + + async def test_read_still_waits_followup_when_expects_data(self) -> None: + # Contrast: with expects_data (a read), the same bare ack is NOT terminal + # - _send waits the follow-up window before returning it, unchanged. + vehicle = _make_vehicle() + vehicle._ack_followup_timeout = 0.05 + msg = _outgoing() + ack = RoutableMessage( + from_destination=Destination(domain=DOMAIN), request_uuid=msg.uuid + ) + + async def fake_write(_uuid, _payload, _response): + vehicle._queues[DOMAIN].put_nowait(ack) + + vehicle.client.write_gatt_char = AsyncMock(side_effect=fake_write) + + result = await vehicle._send( + msg, "protobuf_message_as_bytes", expects_data=True + ) + + self.assertEqual(result, ack) + + +class SendActuationTimeoutTests(IsolatedAsyncioTestCase): + """A lost actuation ack raises after the short actuation timeout, not the default.""" + + async def test_lost_actuation_ack_uses_short_timeout(self) -> None: + vehicle = _make_vehicle() + vehicle._actuation_timeout = 0.05 + vehicle._default_timeout = 5 + msg = _outgoing() + vehicle.client.write_gatt_char = AsyncMock() # nothing enqueued + + # wait_for(1.0) would trip first (asyncio.TimeoutError) if _send wrongly + # waited the 5s default; BluetoothTimeout proves the short path ran. + with self.assertRaises(BluetoothTimeout): + await asyncio.wait_for( + vehicle._send(msg, "protobuf_message_as_bytes", expects_data=False), + timeout=1.0, + ) + + async def test_read_keeps_default_timeout(self) -> None: + vehicle = _make_vehicle() + vehicle._actuation_timeout = 0.05 + vehicle._default_timeout = 5 + msg = _outgoing() + vehicle.client.write_gatt_char = AsyncMock() # nothing enqueued + + # A read must ignore the short actuation timeout: _send is still waiting + # on the default at 0.3s, so the outer wait_for fires instead of the + # short-path BluetoothTimeout. + with self.assertRaises(asyncio.TimeoutError): + await asyncio.wait_for( + vehicle._send(msg, "protobuf_message_as_bytes", expects_data=True), + timeout=0.3, + ) + + class SendQueueHygieneTests(IsolatedAsyncioTestCase): async def test_stale_queued_message_is_discarded_before_send(self) -> None: vehicle = _make_vehicle() From a14fcbf7cbbe4e14ff32e3daa03f939263d0067f Mon Sep 17 00:00:00 2001 From: firstmate crewmate Date: Fri, 10 Jul 2026 13:09:50 +1000 Subject: [PATCH 2/3] no-mistakes(document): Document BLE actuation ack behavior --- AGENTS.md | 4 ++-- docs/bluetooth_vehicles.md | 6 ++++++ tesla_fleet_api/tesla/vehicle/commands.py | 2 +- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 1d7e120..225ae49 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -126,8 +126,8 @@ Source protobuf definitions live in `proto/`; generated Python files live in `te - **`set_scheduled_departure`'s `preconditioning_enabled`/`off_peak_charging_enabled` args are dead**: live-verified - `ScheduledDepartureAction` (`proto/car_server.proto`) has no fields for them, only `preconditioning_times`/`off_peak_charging_times` (weekday-recurrence only, no on/off). Passing `preconditioning_enabled=False` has no effect on the vehicle's observed state. Not a library bug to "fix" without a wider protocol capability; document, don't rely on these args to gate the feature. - **`charge_standard()` rejects `already_standard`**: live-verified - calling it while `charge_state().charge_limit_soc` already equals `charge_limit_soc_std` gets `{"result": False, "reason": "already_standard"}` rather than a no-op success. Not a library bug; callers/tests exercising this command need the limit to actually differ from the std preset first (e.g. via `charge_max_range()` or `set_charge_limit()`). - **BLE media state-observability gotcha**: `MediaState.now_playing_artist/title` and all of `MediaDetailState` (`now_playing_album/station/source_string/elapsed/duration`) were observed empty/zero on the test car while Spotify was actively `Playing` at nonzero volume - these legacy fields are apparently only populated for certain sources (e.g. USB/Bluetooth), not Spotify. Don't assume `media_next_track`/`media_prev_track`/`media_next_fav`/`media_prev_fav` are state-observable via these readers; verify by ACK (`{"result": True}`) and pair with the inverse command when the fingerprint doesn't change. `audio_volume`/`media_playback_status` (for `adjust_volume`/`media_volume_up`/`media_volume_down`/`media_toggle_playback`) were populated correctly and are reliable provers. -- **BLE mutating-command timeout is inconclusive - never assume "the write didn't land"**: this corrects an earlier version of this note, which observed `adjust_volume` write timeouts on one rig leaving the car unmutated and concluded a write timeout means the write never landed. Later live testing disproved that as a general rule: `door_unlock` and `door_lock` each raised `BluetoothTimeout` (~5.5-5.9s) yet both physically executed - a VCSEC state read after each confirmed the lock state had flipped. Reads (state readers, `wake_up()`'s effect) came back reliably all night; only mutating VCSEC/RKE actions showed this false-negative pattern, most likely because the vehicle doesn't reliably return an ack `_send()` observes within the timeout, not because the write failed. Treat a `BluetoothTimeout` from any mutating BLE command as **inconclusive, not failure**: snapshot state before acting, then verify the outcome with a follow-up state read whenever a mutation times out. Never blind-retry a non-idempotent command (toggles like `media_toggle_playback`, volume steps, schedule add/remove) on timeout alone - see the retry-double-execution entry below for why the library's own retry has the same exposure. The `adjust_volume`/`TimeoutAPIError` root cause from the original observation is still unexplained; it just isn't evidence that timed-out writes never land. -- **`wake_up()`'s own ACK is an unreliable signal - almost always a false-negative timeout**: live-verified 9/9 across two independent sessions - `wake_up()` raises `BluetoothTimeout` regardless of whether the vehicle was asleep, already awake, freshly connected, or held open for minutes, yet the wake (or no-op) reliably took effect - an immediate follow-up state read on the same connection succeeds. Never treat a `wake_up()` `BluetoothTimeout` as command failure; call it best-effort (catch and ignore) and confirm readiness by retrying a cheap INFO read instead (see the boot-delay gotcha above for why the first INFO read still needs its own retry/backoff). Hold one connection across a whole batch of related commands rather than reconnecting between each - reconnecting costs ~123% more per operation with no demonstrated wake-preservation benefit from the connection alone. +- **BLE mutating-command timeout is inconclusive - never assume "the write didn't land"**: this corrects an earlier version of this note, which observed `adjust_volume` write timeouts on one rig leaving the car unmutated and concluded a write timeout means the write never landed. Later live testing disproved that as a general rule: `door_unlock` and `door_lock` each raised `BluetoothTimeout` yet both physically executed - a VCSEC state read after each confirmed the lock state had flipped. Reads (state readers, `wake_up()`'s effect) came back reliably all night; only mutating VCSEC/RKE actions showed this false-negative pattern, most likely because the vehicle doesn't reliably return an ack `_send()` observes within the timeout, not because the write failed. Treat a `BluetoothTimeout` from any mutating BLE command as **inconclusive, not failure**: snapshot state before acting, then verify the outcome with a follow-up state read whenever a mutation times out. Never blind-retry a non-idempotent command (toggles like `media_toggle_playback`, volume steps, schedule add/remove) on timeout alone - see the retry-double-execution entry below for why the library's own retry has the same exposure. The `adjust_volume`/`TimeoutAPIError` root cause from the original observation is still unexplained; it just isn't evidence that timed-out writes never land. VCSEC actuations now use the shorter `_actuation_timeout` when their terminal ack is lost. +- **`wake_up()` is best-effort; confirm readiness with an INFO read**: `wake_up()` is a VCSEC actuation, so a terminal ack now returns promptly when observed, but a `BluetoothTimeout` is still only an inconclusive wake signal, not command failure. Call it best-effort (catch and ignore timeout) and confirm readiness by retrying a cheap INFO read instead (see the boot-delay gotcha above for why the first INFO read still needs its own retry/backoff). Hold one connection across a whole batch of related commands rather than reconnecting between each - reconnecting costs ~123% more per operation with no demonstrated wake-preservation benefit from the connection alone. - **The signed-command retry in `Commands._command` can double-execute a mutating command**: on an `OPERATIONSTATUS_WAIT` reply or an `INCORRECT_EPOCH`/`INVALID_TOKEN` fault, `_command` (`commands.py`) re-signs and re-sends the identical command, bounded at 3 attempts then a clean `{"result": False, "reason": "Too many retries"}` - the cap itself is safe and doesn't loop. Live-verified deterministically: a WAIT-then-OK sequence produces 2 physical sends of the same command on the wire. Combined with the mutating-timeout-is-inconclusive gotcha above (a command can execute despite a WAIT/fault reply), this retry is a latent double-apply window. Harmless for a naturally idempotent command (lock/unlock), a real correctness risk for toggles and step commands (`media_toggle_playback`, `media_volume_up`/`down`, schedule add/remove) - verify those by absolute state after the call, never by counting invocations or trusting the retry to be safe. - **`expects_data` splits BLE reply-waiting: VCSEC actuations return on the terminal ack**: a VCSEC read replies with a bare ACK **then** a data frame, but a VCSEC actuation (RKE/closure/wake via `_sendVehicleSecurity`) replies with a **single bare ACK only** - `_send` cannot tell the two apart at transport level, so the caller declares it. `_sendVehicleSecurity` passes `expects_data=False` down through `_command` into `_send` (`commands.py`/`bluetooth.py`); everything else (VCSEC reads via `_getVehicleSecurity`, all infotainment via `_send/_getInfotainment`, `_handshake`, `pair`) keeps the default `expects_data=True`. With `expects_data=False`, `_send` returns immediately on the matching ACK instead of waiting out `_ack_followup_timeout` (was a flat ~2s tax on every VCSEC actuation), and on a lost ack it raises `BluetoothTimeout` after the shorter `_actuation_timeout` (2s) rather than `_default_timeout` (5s) - the verify-by-state contract makes a longer wait pointless. Infotainment actuations never paid the tax (their `actionStatus` rides in `protobuf_message_as_bytes`, so `_send` returns on that data frame). The WAIT/fault retry threads `expects_data` through unchanged, so the double-execute exposure noted above is unaffected. - **`navigation_gps_request`'s `order` param is a raw int, not a callable enum**: `commands.py` used to build it as `NavigationGpsRequest.RemoteNavTripOrder(order)`, treating the protobuf nested-enum wrapper (`EnumTypeWrapper`) as if it were a callable Python `enum.IntEnum` class - it isn't, so every call raised `TypeError` before any message was sent (found live during PR-8; this method had never been exercised over BLE before). Fixed to pass `order=order` directly (matching the working sibling `navigation_gps_destination_request`), which protobuf accepts as a bare int for an enum field at runtime. diff --git a/docs/bluetooth_vehicles.md b/docs/bluetooth_vehicles.md index 7e1851d..a953ed0 100644 --- a/docs/bluetooth_vehicles.md +++ b/docs/bluetooth_vehicles.md @@ -115,6 +115,12 @@ acknowledgement does not reach the client. For commands that change vehicle state, snapshot the relevant state before acting and verify the outcome with a follow-up state read after any timeout. +VCSEC actuations, such as RKE, closure, and wake requests, return as soon as +their terminal acknowledgement arrives because no data frame follows it. If that +acknowledgement is lost, they use a shorter response timeout than reads; the +timeout is still inconclusive and should be handled with the same verify-by-state +pattern. + Do not blind-retry non-idempotent commands, such as media toggles, volume steps, or schedule add/remove operations, on timeout alone. The signed-command retry inside the library can also re-send an identical command after a WAIT status or diff --git a/tesla_fleet_api/tesla/vehicle/commands.py b/tesla_fleet_api/tesla/vehicle/commands.py index 392206a..94013d1 100644 --- a/tesla_fleet_api/tesla/vehicle/commands.py +++ b/tesla_fleet_api/tesla/vehicle/commands.py @@ -686,7 +686,7 @@ async def _sendVehicleSecurity(self, command: UnsignedMessage) -> dict[str, Any] ) async def _getVehicleSecurity(self, command: UnsignedMessage) -> VehicleStatus: - """Sign and send a message to Infotainment computer.""" + """Sign and send a read request to the Vehicle Security computer.""" reply = await self._command( Domain.DOMAIN_VEHICLE_SECURITY, command.SerializeToString() ) From d7f9127a8e9b032524877620170dd7624e3a151e Mon Sep 17 00:00:00 2001 From: firstmate crewmate Date: Fri, 10 Jul 2026 13:11:26 +1000 Subject: [PATCH 3/3] no-mistakes(lint): Fix lint formatting --- tesla_fleet_api/tesla/vehicle/signed.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tesla_fleet_api/tesla/vehicle/signed.py b/tesla_fleet_api/tesla/vehicle/signed.py index 4ab7771..8092fdf 100644 --- a/tesla_fleet_api/tesla/vehicle/signed.py +++ b/tesla_fleet_api/tesla/vehicle/signed.py @@ -15,10 +15,11 @@ SignedParentT = TypeVar("SignedParentT", bound="TeslaFleetApi") -class VehicleSigned(Commands[SignedParentT], VehicleFleet[SignedParentT], Generic[SignedParentT]): # pyright: ignore[reportIncompatibleMethodOverride] +class VehicleSigned( # pyright: ignore[reportIncompatibleMethodOverride] + Commands[SignedParentT], VehicleFleet[SignedParentT], Generic[SignedParentT] +): """Class describing the Tesla Fleet API vehicle endpoints and commands for a specific vehicle with command signing.""" - _auth_method = "hmac" def __init__(self, parent: SignedParentT, vin: str): @@ -26,7 +27,6 @@ def __init__(self, parent: SignedParentT, vin: str): super().__init__(parent, vin) super(Commands, self).__init__(parent, vin) - async def _send( self, msg: RoutableMessage, requires: str, expects_data: bool = True ) -> RoutableMessage: