Skip to content

Commit 0800251

Browse files
Bre77firstmate crewmate
andauthored
fix(vehicle): reset stale BLE reassembly frames (#58)
* fix(ble): reset reassembly buffer on stale inter-chunk gap ReassemblingBuffer only resynchronized on a protobuf decode failure, so a chunk dropped mid-message left a stale partial frame that got prepended to the next message, corrupting it until a lucky decode error resynced. Discard the partial when a chunk arrives more than 1s after the previous one, matching the reset semantics in Tesla's official Go SDK. * no-mistakes(document): Document BLE stale-frame reset --------- Co-authored-by: firstmate crewmate <crewmate@firstmate.local>
1 parent 759d6fa commit 0800251

4 files changed

Lines changed: 85 additions & 2 deletions

File tree

‎AGENTS.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,7 @@ Source protobuf definitions live in `proto/`; generated Python files live in `te
130130
- **`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.
131131
- **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.
132132
- **`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.
133+
- **`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).
133134

134135
## Maintaining this file
135136

‎docs/bluetooth_vehicles.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,12 @@ failures and response-wait `BluetoothTimeout` failures with one library error
101101
hierarchy, or catch `BluetoothTransportError` separately when you need to
102102
distinguish a transport failure from a vehicle timeout.
103103

104+
BLE response chunks are reassembled with the same stale-frame behavior as
105+
Tesla's vehicle-command BLE connector: if a partial frame sits idle for more
106+
than one second before the next chunk arrives, the partial frame is discarded
107+
before processing the new chunk. This prevents a dropped chunk from corrupting
108+
the next response, but it does not change command acknowledgement timeouts.
109+
104110
## Mutating Command Timeouts
105111

106112
A `BluetoothTimeout` from a mutating BLE command is inconclusive, not proof that

‎tesla_fleet_api/tesla/vehicle/bluetooth.py‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import asyncio
44
import hashlib
55
import struct
6+
import time
67
from random import randbytes
78
from typing import TYPE_CHECKING, Any, Callable, Generic, TypeVar
89

@@ -96,14 +97,20 @@ def prependLength(message: bytes) -> bytearray:
9697
return bytearray([len(message) >> 8, len(message) & 0xFF]) + message
9798

9899

100+
# A chunk arriving after this much silence means the prior partial frame was
101+
# abandoned mid-flight, not merely delayed.
102+
STALE_CHUNK_TIMEOUT = 1.0
103+
104+
99105
class ReassemblingBuffer:
100106
"""
101107
Reassembles BLE notification chunks into length-prefixed RoutableMessages.
102108
103109
Each message starts with a 2-byte length. One notification can contain part
104110
of a message, exactly one message, or multiple messages. If a message cannot
105111
be decoded, the buffer drops the current physical packet and resynchronizes
106-
at the next recorded packet boundary.
112+
at the next recorded packet boundary. A partial message is also discarded if
113+
the next chunk doesn't arrive within ``STALE_CHUNK_TIMEOUT``.
107114
"""
108115

109116
def __init__(self, callback: Callable[[RoutableMessage], None]):
@@ -117,6 +124,7 @@ def __init__(self, callback: Callable[[RoutableMessage], None]):
117124
self.expected_length: int | None = None
118125
self.packet_starts: list[int] = []
119126
self.callback = callback
127+
self._last_chunk_time: float | None = None
120128

121129
def receive_data(self, data: bytearray):
122130
"""
@@ -125,6 +133,17 @@ def receive_data(self, data: bytearray):
125133
Args:
126134
data: The received bytearray data.
127135
"""
136+
now = time.monotonic()
137+
if (
138+
self.buffer
139+
and self._last_chunk_time is not None
140+
and now - self._last_chunk_time > STALE_CHUNK_TIMEOUT
141+
):
142+
self.buffer = bytearray()
143+
self.expected_length = None
144+
self.packet_starts = []
145+
self._last_chunk_time = now
146+
128147
self.packet_starts.append(len(self.buffer))
129148
self.buffer.extend(data)
130149

‎tests/test_ble_reassembling_buffer.py‎

Lines changed: 58 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,13 @@
44
GATT) to lock down: a message split across multiple BLE notification
55
chunks, multiple complete messages delivered in a single chunk, and
66
resynchronization after a corrupted/oversized packet using the
7-
``packet_starts`` boundary tracking in ``discard_packet``.
7+
``packet_starts`` boundary tracking in ``discard_packet``. It also verifies
8+
that a stale partial frame is dropped after the BLE inter-chunk timeout while
9+
normal fast multi-chunk messages still reassemble.
810
"""
911

1012
from unittest import TestCase
13+
from unittest.mock import patch
1114

1215
from tesla_fleet_api.tesla.vehicle.bluetooth import ReassemblingBuffer, prependLength
1316
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:
114117
self.assertEqual(
115118
self.received[0].from_destination.domain, Domain.DOMAIN_VEHICLE_SECURITY
116119
)
120+
121+
def test_stale_partial_is_discarded_after_timeout(self) -> None:
122+
stale = RoutableMessage(
123+
from_destination=Destination(domain=Domain.DOMAIN_VEHICLE_SECURITY),
124+
request_uuid=b"0123456789abcdef",
125+
)
126+
stale_payload = framed(stale)
127+
128+
fresh = RoutableMessage(
129+
from_destination=Destination(domain=Domain.DOMAIN_INFOTAINMENT)
130+
)
131+
132+
with patch(
133+
"tesla_fleet_api.tesla.vehicle.bluetooth.time.monotonic"
134+
) as mock_monotonic:
135+
# Deliver only the first half of `stale` - a dropped chunk mid-message.
136+
mock_monotonic.return_value = 0.0
137+
self.buffer.receive_data(stale_payload[: len(stale_payload) // 2])
138+
self.assertEqual(self.received, [])
139+
140+
# The next chunk arrives well past the stale-chunk timeout: the
141+
# partial must be dropped, not prepended to the new message.
142+
mock_monotonic.return_value = 2.0
143+
self.buffer.receive_data(framed(fresh))
144+
145+
self.assertEqual(len(self.received), 1)
146+
self.assertEqual(
147+
self.received[0].from_destination.domain, Domain.DOMAIN_INFOTAINMENT
148+
)
149+
150+
def test_fast_multi_chunk_message_under_timeout_still_reassembles(self) -> None:
151+
msg = RoutableMessage(
152+
from_destination=Destination(domain=Domain.DOMAIN_VEHICLE_SECURITY),
153+
request_uuid=b"0123456789abcdef",
154+
)
155+
payload = framed(msg)
156+
chunk_size = 5
157+
158+
with patch(
159+
"tesla_fleet_api.tesla.vehicle.bluetooth.time.monotonic"
160+
) as mock_monotonic:
161+
# Each chunk arrives 0.1s after the previous one - well under the
162+
# stale-chunk timeout - so the partial must survive intact.
163+
clock = 0.0
164+
for i in range(0, len(payload), chunk_size):
165+
mock_monotonic.return_value = clock
166+
self.buffer.receive_data(payload[i : i + chunk_size])
167+
clock += 0.1
168+
169+
self.assertEqual(len(self.received), 1)
170+
self.assertEqual(
171+
self.received[0].from_destination.domain, Domain.DOMAIN_VEHICLE_SECURITY
172+
)
173+
self.assertEqual(self.received[0].request_uuid, b"0123456789abcdef")

0 commit comments

Comments
 (0)