Skip to content

Commit a1af07a

Browse files
Bre77firstmate crewmate
andauthored
fix(vehicle): align BLE and cloud command parity (#60)
* fix(vehicle): keep 0.0 homelink coordinates on the REST path trigger_homelink on the Fleet REST path gated lat/lon with a truthy "if lat and lon" check, silently dropping a valid 0.0 coordinate. The BLE signed-command path already guards with "is not None" and forwards 0.0 correctly, so the two transports built different instructions from the same call. Match the REST path to the BLE semantics and lock the parity in with a cross-transport test. * fix(vehicle): validate BLE adjust_volume range for transport parity The Fleet REST adjust_volume rejected out-of-range volumes with a ValueError before building the request; the BLE signed-command path forwarded any float to the vehicle unchecked. Same call, different behavior. Apply the identical 0.0-11.0 guard on the BLE path so both transports reject the same inputs, and extend the cross-transport parity test. * docs(vehicle): record cloud-vs-BLE cross-transport parity findings Document the parity contract between the Fleet REST and BLE signed-command paths, the non-bug form differences that must not be "fixed", and the two open divergences (clear_pin_to_drive_admin action mapping, navigation_gps_request order signature) left for live verification. * no-mistakes(document): Sync command docs * docs(vehicle): revert trigger_homelink docstring wording A docstring should not announce a bugfix; the fix speaks for itself. Restore the original trigger_homelink docstring in both commands.py and fleet.py. --------- Co-authored-by: firstmate crewmate <crewmate@firstmate.local>
1 parent 628bfae commit a1af07a

6 files changed

Lines changed: 137 additions & 3 deletions

File tree

‎AGENTS.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,7 @@ Source protobuf definitions live in `proto/`; generated Python files live in `te
132132
- **`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.
133133
- **`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.
134134
- **`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).
135+
- **Cross-transport parity (cloud REST `VehicleFleet` vs BLE `Commands`)**: the same-named command on both paths should build a semantically equivalent instruction from identical args - a divergence there is a bug, but response *bodies* legitimately differ (REST JSON dict vs decoded protobuf) and are not. `tests/test_cross_transport_parity.py` locks the equivalence in with mocked-both-transports tests. Known **non-bug FORM differences** (do not "fix"): `set_scheduled_departure`'s `preconditioning_enabled`/`off_peak_charging_enabled` (no proto fields), `window_control` lat/lon and `navigation_sc_request` `id` (no proto fields), `navigation_request`'s `type`/`locale`/`timestamp_ms` (REST share-intent framing), and `media_volume_up` (no Tesla REST endpoint - BLE-only; cloud raises volume via `adjust_volume`). Two **open divergences left unfixed** pending live verification: `clear_pin_to_drive_admin` builds `DrivingClearSpeedLimitPinAction` (speed-limit PIN, not PIN-to-Drive - suspected mismapping, security-sensitive), and `navigation_gps_request`'s `order` is required on BLE but optional on cloud (signature mismatch; null-order wire semantics undecided).
135136

136137
## Maintaining this file
137138

‎docs/bluetooth_vehicles.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -309,6 +309,9 @@ commands:
309309
- `media_next_fav()`
310310
- `media_prev_fav()`
311311

312+
`adjust_volume(volume)` accepts absolute volume values from `0.0` through
313+
`11.0`, matching the Fleet API command validation.
314+
312315
These commands return the signed-command acknowledgement, not a media-state
313316
diff. For verification, prefer `media_state().audio_volume` and
314317
`media_state().media_playback_status` where they apply. Track identity fields

‎docs/fleet_api_signed_commands.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,9 @@ actually executed despite the WAIT/fault reply. Verify commands such as media
187187
toggles, volume steps, and schedule add/remove operations by reading absolute
188188
state after the call instead of relying on the number of send attempts.
189189

190+
`adjust_volume(volume)` accepts absolute volume values from `0.0` through
191+
`11.0`, matching the Fleet API command validation.
192+
190193
## Flash Lights
191194

192195
You can flash the lights of a specific vehicle using its VIN:

‎tesla_fleet_api/tesla/vehicle/commands.py‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -756,7 +756,9 @@ async def actuate_trunk(self, which_trunk: Trunk | str) -> dict[str, Any]:
756756
raise ValueError("Invalid trunk.")
757757

758758
async def adjust_volume(self, volume: float) -> dict[str, Any]:
759-
"""Adjusts vehicle media playback volume."""
759+
"""Adjusts vehicle media playback volume from 0.0 to 11.0."""
760+
if volume < 0.0 or volume > 11.0:
761+
raise ValueError("Volume must a number from 0.0 to 11.0")
760762
return await self._sendInfotainment(
761763
Action(
762764
vehicleAction=VehicleAction(

‎tesla_fleet_api/tesla/vehicle/fleet.py‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ async def actuate_trunk(self, which_trunk: Trunk | str) -> dict[str, Any]:
4242
)
4343

4444
async def adjust_volume(self, volume: float) -> dict[str, Any]:
45-
"""Adjusts vehicle media playback volume."""
45+
"""Adjusts vehicle media playback volume from 0.0 to 11.0."""
4646
if volume < 0.0 or volume > 11.0:
4747
raise ValueError("Volume must a number from 0.0 to 11.0")
4848
return await self._request(
@@ -582,7 +582,8 @@ async def trigger_homelink(
582582
data: dict[str, str | float] = {}
583583
if token:
584584
data["token"] = token
585-
if lat and lon:
585+
# Guard on None, not truthiness: lat/lon of 0.0 are valid coordinates.
586+
if lat is not None and lon is not None:
586587
data["lat"] = lat
587588
data["lon"] = lon
588589
return await self._request(
Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
"""Cross-transport parity: the same Python call must build semantically
2+
equivalent vehicle instructions over the Fleet REST (cloud) path and the BLE
3+
signed-command (protobuf) path.
4+
5+
Response bodies legitimately differ in FORM (cloud returns a REST JSON dict,
6+
BLE returns decoded protobuf), so these tests assert on the *outbound
7+
instruction* each transport constructs from identical arguments - that is where
8+
a parameter-handling divergence between the two code paths would show up.
9+
10+
The cloud path is exercised through ``VehicleFleet`` with ``_request`` mocked;
11+
the BLE path through ``VehicleBluetooth`` with ``_send`` mocked (see
12+
``ble_mocked_transport``).
13+
"""
14+
15+
from __future__ import annotations
16+
17+
from typing import Any, cast
18+
from unittest.mock import AsyncMock, MagicMock
19+
20+
from tesla_fleet_api.tesla.vehicle.bluetooth import VehicleBluetooth
21+
from tesla_fleet_api.tesla.vehicle.fleet import VehicleFleet
22+
from tesla_fleet_api.tesla.vehicle.proto.car_server_pb2 import Action, VehicleAction
23+
from tesla_fleet_api.tesla.vehicle.proto.universal_message_pb2 import RoutableMessage
24+
25+
from ble_mocked_transport import (
26+
MockedBleTransportTestCase,
27+
decrypt_sent_command,
28+
infotainment_action_ok_reply,
29+
)
30+
31+
32+
def _make_fleet_vehicle(vin: str) -> tuple[VehicleFleet[Any], AsyncMock]:
33+
"""A ``VehicleFleet`` whose ``_request`` is mocked to capture the REST call."""
34+
parent = MagicMock()
35+
request = AsyncMock(return_value={"response": {"result": True}})
36+
parent._request = request # pyright: ignore[reportAttributeAccessIssue]
37+
return VehicleFleet(parent, vin), request
38+
39+
40+
def _sent_vehicle_action(
41+
vehicle: VehicleBluetooth[Any], send: AsyncMock
42+
) -> VehicleAction:
43+
"""Decrypt the signed command the BLE transport was about to send."""
44+
assert send.await_args is not None
45+
sent_msg = cast("RoutableMessage", send.await_args.args[0])
46+
plaintext = decrypt_sent_command(vehicle, sent_msg)
47+
return Action.FromString(plaintext).vehicleAction
48+
49+
50+
class TriggerHomelinkParityTests(MockedBleTransportTestCase):
51+
"""``trigger_homelink`` must carry the given coordinates on both transports.
52+
53+
Regression: the cloud path used a truthy ``if lat and lon`` check that
54+
silently dropped a valid ``0.0`` coordinate, while BLE (``is not None``)
55+
kept it - a genuine parameter-handling divergence.
56+
"""
57+
58+
async def test_zero_coordinates_survive_on_both_transports(self) -> None:
59+
# Cloud (REST)
60+
cloud, request = _make_fleet_vehicle(self.VIN)
61+
await cloud.trigger_homelink(token="tok", lat=0.0, lon=0.0)
62+
assert request.await_args is not None
63+
cloud_json = request.await_args.kwargs["json"]
64+
self.assertEqual(cloud_json["lat"], 0.0)
65+
self.assertEqual(cloud_json["lon"], 0.0)
66+
self.assertEqual(cloud_json["token"], "tok")
67+
68+
# BLE (signed protobuf)
69+
ble, send = self.make_vehicle()
70+
send.return_value = infotainment_action_ok_reply()
71+
await ble.trigger_homelink(token="tok", lat=0.0, lon=0.0)
72+
action = _sent_vehicle_action(ble, send).vehicleControlTriggerHomelinkAction
73+
self.assertEqual(action.location.latitude, 0.0)
74+
self.assertEqual(action.location.longitude, 0.0)
75+
self.assertEqual(action.token, "tok")
76+
77+
async def test_omitted_coordinates_absent_on_both_transports(self) -> None:
78+
cloud, request = _make_fleet_vehicle(self.VIN)
79+
await cloud.trigger_homelink(token="tok")
80+
assert request.await_args is not None
81+
cloud_json = request.await_args.kwargs["json"]
82+
self.assertNotIn("lat", cloud_json)
83+
self.assertNotIn("lon", cloud_json)
84+
85+
ble, send = self.make_vehicle()
86+
send.return_value = infotainment_action_ok_reply()
87+
await ble.trigger_homelink(token="tok")
88+
action = _sent_vehicle_action(ble, send).vehicleControlTriggerHomelinkAction
89+
# No location submessage set when coordinates are omitted.
90+
self.assertFalse(action.HasField("location"))
91+
92+
93+
class AdjustVolumeParityTests(MockedBleTransportTestCase):
94+
"""``adjust_volume`` must reject the same out-of-range values on both
95+
transports.
96+
97+
Regression: the cloud path validated ``0.0 <= volume <= 11.0`` and raised
98+
``ValueError``; the BLE path forwarded any float to the car unchecked - a
99+
parameter-validation divergence.
100+
"""
101+
102+
async def test_out_of_range_rejected_on_both_transports(self) -> None:
103+
for bad in (-1.0, 11.5):
104+
cloud, request = _make_fleet_vehicle(self.VIN)
105+
with self.assertRaises(ValueError):
106+
await cloud.adjust_volume(bad)
107+
request.assert_not_awaited()
108+
109+
ble, send = self.make_vehicle()
110+
with self.assertRaises(ValueError):
111+
await ble.adjust_volume(bad)
112+
send.assert_not_awaited()
113+
114+
async def test_in_range_sends_absolute_volume_on_both_transports(self) -> None:
115+
cloud, request = _make_fleet_vehicle(self.VIN)
116+
await cloud.adjust_volume(5.0)
117+
assert request.await_args is not None
118+
self.assertEqual(request.await_args.kwargs["json"], {"volume": 5.0})
119+
120+
ble, send = self.make_vehicle()
121+
send.return_value = infotainment_action_ok_reply()
122+
await ble.adjust_volume(5.0)
123+
action = _sent_vehicle_action(ble, send)
124+
self.assertAlmostEqual(action.mediaUpdateVolume.volume_absolute_float, 5.0)

0 commit comments

Comments
 (0)