diff --git a/AGENTS.md b/AGENTS.md index d6beff3..02e2ae5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -130,6 +130,7 @@ Source protobuf definitions live in `proto/`; generated Python files live in `te - **`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. - **`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). ## Maintaining this file diff --git a/docs/bluetooth_vehicles.md b/docs/bluetooth_vehicles.md index 7e56eb0..7e1851d 100644 --- a/docs/bluetooth_vehicles.md +++ b/docs/bluetooth_vehicles.md @@ -101,6 +101,12 @@ failures and response-wait `BluetoothTimeout` failures with one library error hierarchy, or catch `BluetoothTransportError` separately when you need to distinguish a transport failure from a vehicle timeout. +BLE response chunks are reassembled with the same stale-frame behavior as +Tesla's vehicle-command BLE connector: if a partial frame sits idle for more +than one second before the next chunk arrives, the partial frame is discarded +before processing the new chunk. This prevents a dropped chunk from corrupting +the next response, but it does not change command acknowledgement timeouts. + ## Mutating Command Timeouts A `BluetoothTimeout` from a mutating BLE command is inconclusive, not proof that diff --git a/tesla_fleet_api/tesla/vehicle/bluetooth.py b/tesla_fleet_api/tesla/vehicle/bluetooth.py index 8c4fb7d..8a2a451 100644 --- a/tesla_fleet_api/tesla/vehicle/bluetooth.py +++ b/tesla_fleet_api/tesla/vehicle/bluetooth.py @@ -3,6 +3,7 @@ import asyncio import hashlib import struct +import time from random import randbytes from typing import TYPE_CHECKING, Any, Callable, Generic, TypeVar @@ -96,6 +97,11 @@ def prependLength(message: bytes) -> bytearray: return bytearray([len(message) >> 8, len(message) & 0xFF]) + message +# A chunk arriving after this much silence means the prior partial frame was +# abandoned mid-flight, not merely delayed. +STALE_CHUNK_TIMEOUT = 1.0 + + class ReassemblingBuffer: """ Reassembles BLE notification chunks into length-prefixed RoutableMessages. @@ -103,7 +109,8 @@ class ReassemblingBuffer: Each message starts with a 2-byte length. One notification can contain part of a message, exactly one message, or multiple messages. If a message cannot be decoded, the buffer drops the current physical packet and resynchronizes - at the next recorded packet boundary. + at the next recorded packet boundary. A partial message is also discarded if + the next chunk doesn't arrive within ``STALE_CHUNK_TIMEOUT``. """ def __init__(self, callback: Callable[[RoutableMessage], None]): @@ -117,6 +124,7 @@ def __init__(self, callback: Callable[[RoutableMessage], None]): self.expected_length: int | None = None self.packet_starts: list[int] = [] self.callback = callback + self._last_chunk_time: float | None = None def receive_data(self, data: bytearray): """ @@ -125,6 +133,17 @@ def receive_data(self, data: bytearray): Args: data: The received bytearray data. """ + now = time.monotonic() + if ( + self.buffer + and self._last_chunk_time is not None + and now - self._last_chunk_time > STALE_CHUNK_TIMEOUT + ): + self.buffer = bytearray() + self.expected_length = None + self.packet_starts = [] + self._last_chunk_time = now + self.packet_starts.append(len(self.buffer)) self.buffer.extend(data) diff --git a/tests/test_ble_reassembling_buffer.py b/tests/test_ble_reassembling_buffer.py index d47c51b..d1d9371 100644 --- a/tests/test_ble_reassembling_buffer.py +++ b/tests/test_ble_reassembling_buffer.py @@ -4,10 +4,13 @@ GATT) to lock down: a message split across multiple BLE notification chunks, multiple complete messages delivered in a single chunk, and resynchronization after a corrupted/oversized packet using the -``packet_starts`` boundary tracking in ``discard_packet``. +``packet_starts`` boundary tracking in ``discard_packet``. It also verifies +that a stale partial frame is dropped after the BLE inter-chunk timeout while +normal fast multi-chunk messages still reassemble. """ from unittest import TestCase +from unittest.mock import patch from tesla_fleet_api.tesla.vehicle.bluetooth import ReassemblingBuffer, prependLength from tesla_fleet_api.tesla.vehicle.proto.universal_message_pb2 import ( @@ -114,3 +117,57 @@ def test_oversized_length_header_discards_and_resyncs(self) -> None: self.assertEqual( self.received[0].from_destination.domain, Domain.DOMAIN_VEHICLE_SECURITY ) + + def test_stale_partial_is_discarded_after_timeout(self) -> None: + stale = RoutableMessage( + from_destination=Destination(domain=Domain.DOMAIN_VEHICLE_SECURITY), + request_uuid=b"0123456789abcdef", + ) + stale_payload = framed(stale) + + fresh = RoutableMessage( + from_destination=Destination(domain=Domain.DOMAIN_INFOTAINMENT) + ) + + with patch( + "tesla_fleet_api.tesla.vehicle.bluetooth.time.monotonic" + ) as mock_monotonic: + # Deliver only the first half of `stale` - a dropped chunk mid-message. + mock_monotonic.return_value = 0.0 + self.buffer.receive_data(stale_payload[: len(stale_payload) // 2]) + self.assertEqual(self.received, []) + + # The next chunk arrives well past the stale-chunk timeout: the + # partial must be dropped, not prepended to the new message. + mock_monotonic.return_value = 2.0 + self.buffer.receive_data(framed(fresh)) + + self.assertEqual(len(self.received), 1) + self.assertEqual( + self.received[0].from_destination.domain, Domain.DOMAIN_INFOTAINMENT + ) + + def test_fast_multi_chunk_message_under_timeout_still_reassembles(self) -> None: + msg = RoutableMessage( + from_destination=Destination(domain=Domain.DOMAIN_VEHICLE_SECURITY), + request_uuid=b"0123456789abcdef", + ) + payload = framed(msg) + chunk_size = 5 + + with patch( + "tesla_fleet_api.tesla.vehicle.bluetooth.time.monotonic" + ) as mock_monotonic: + # Each chunk arrives 0.1s after the previous one - well under the + # stale-chunk timeout - so the partial must survive intact. + clock = 0.0 + for i in range(0, len(payload), chunk_size): + mock_monotonic.return_value = clock + self.buffer.receive_data(payload[i : i + chunk_size]) + clock += 0.1 + + self.assertEqual(len(self.received), 1) + self.assertEqual( + self.received[0].from_destination.domain, Domain.DOMAIN_VEHICLE_SECURITY + ) + self.assertEqual(self.received[0].request_uuid, b"0123456789abcdef")