Feat/ble improvements - #2823
Open
pkuehnel wants to merge 21 commits into
Open
Conversation
The mode a previous process left on the home battery devices is unknown after startup, e.g. when the container was killed instead of shut down gracefully or when the mode job never ran at all. As restoring normal was limited to a tracked hold or charge, such a leftover hold was never released and the battery could neither charge nor discharge. Normal is now written once after startup as long as no other mode is required. Failed writes are retried as the flag is only set on success. Also adds a test that all jobs in the scheduling jobs namespace are registered for dependency injection: an unregistered job throws on every fire, which sets its trigger to error state, so it silently never runs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replaces the remaining raw new HttpClient() allocations with IHttpClientFactory clients. Per request timeouts move from HttpClient.Timeout to a CancellationTokenSource, as the timeout of a pooled client must not be changed per call. Auth handling of the generic JSON REST templates is fixed as part of this: CreateHttpClient put Basic auth and API tokens on DefaultRequestHeaders, which would leak the credentials of one configuration onto another configuration's requests once the client is shared. ApplyAuthHeaders now sets them per HttpRequestMessage instead. Adds a named BLE client (120 s backstop, per call timeouts unchanged at 60/30/29/10/5 s) and moves TeslaBleService onto it. The four sites that build a client with a custom HttpClientHandler are left as they are. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The BLE container logged its detailed car communication based on the UseDebugBle environment variable, so enabling it meant editing the container configuration and restarting it. It is now a normal TSC base configuration setting that is sent along with every command. Workers pick the change up lazily per adapter: a worker started with a different setting is restarted on its next request, as the log level of the used library is global per process. Workers on other adapters keep serving with their own setting until they are used again. The keep warm restart and the liveness probe pass no setting at all so they never cause a restart of their own. Bumps the lockstep compatibility version to 2.41.0 as the container API gained a query parameter. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The beacon scan already existed on IBleService and BleController but had no entry point on the support page, where the other BLE diagnostics live. Next to the presence answer the result carries the advertisement counters of the scan window, so a car that is really away can be told apart from a Bluetooth radio that hears nothing at all. Uses GetBeaconScanResultForVin, which deliberately sends no keep warm seconds, so a manual check never moves the container's warm window. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The BLE refresh ran inline at the start of ChargingValueJob, so every car that answered slowly (or was absent and cost a full scan window plus connect timeouts) delayed the charging value calculation by that much. It is now a job of its own, BleDataRefreshJob, running every BleDataRefreshIntervalSeconds (default 13 s, configurable). Charging values are calculated from the most recent refresh instead of waiting for one. Adds BleReadCoordinator so a car is never read from two places at once, and RefreshSingleCarData for callers that need an up to date state right away rather than at the next scheduled run. The away confirmation of BlePresenceStateService becomes time based: it counted consecutive misses, which silently tied "how long must a car be gone" to the poll interval and would have dropped it from about 2.5 minutes to about 65 seconds at the new cadence. It now requires an uninterrupted 2.5 minutes of misses, plus at least two observed misses so a poller that stalled longer than that cannot confirm a car as away on its first poll after resuming. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Requests to the infotainment system reset a car's standby timer and keep it awake, which was verified on a real car: the VCSEC body controller poll alone does not, the infotainment charge state poll does. So a BLE data collection car parked at home never fell asleep. An idle car now enters a sleep window during which only the infotainment poll is withheld, so the standby timer can run out. Presence detection (beacon scan) and the VCSEC read keep running, so a car that is driven away is still noticed while it is trying to sleep. A window starts once all doors, frunk and trunk have been closed and unchanged, nobody is in the car and neither the plugged in state nor the charge limit changed for the stability period. When the window elapses the car is polled once and, if still idle, a new window starts. The window is cancelled whenever the car is no longer idle: a charge command is sent, the car leaves home, or the user cancels the attempt on the home page. The home page shows the phase with a countdown and lets the user start an attempt right away or cancel a running one. Defaults: 13 minutes window, 5 minutes stability, both configurable, 0 disables the feature. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nothing in the sleep window's stability signature changes while a car charges steadily: doors stay closed, nobody is in the car, the plugged in state and the charge limit do not move. After the stability period the car therefore entered a sleep window and TSC stopped reading its charge state for the whole window, so it could not see the car finish, stop or get unplugged. The reset on charge commands does not cover this: SetAmp returns early without sending anything while the target current is unchanged, which is exactly the steady state case. A charging car is definitely awake and not trying to sleep, so its window state is cleared and the poll is not fed into the state machine. Also adds the missing coverage for RefreshSingleCarData. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Adds TeslaSolarCharger.Protobuf, which generates message types from Tesla's own protos rather than duplicating them in hand written DTOs. The protos are vendored verbatim from teslamotors/vehicle-command at the ref already pinned as VEHICLE_COMMAND_REF in the BLE API Dockerfile, so the container's Go build and TSC always decode the same wire format. Generated code is not committed, Grpc.Tools emits it into obj at build time. The server Dockerfile needs the new csproj before its restore step; its build stage is amd64 only, so protoc never has to exist for arm/v7. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… strings Replaces the hand written BLE DTOs with Tesla's generated messages. protojson omits every field that holds its proto3 default, so a closed closure is never serialized at all - the reason a closed up car was reported as open or occupied. Generated types make that structural rather than a rule someone has to remember: an absent field decodes to the enum's zero value, and that value is CLOSURESTATE_CLOSED, so absent and closed need no special casing. Also drops GetChargingStateName, which decoded a protobuf oneof by reading the JSON property name out of a JObject. The charging state is now a real oneof case. Charge values use explicit presence, so a reported 0 A is distinguishable from a field the car never sent. BleProtoJson is the only place that builds a JsonParser and always ignores unknown fields: the parser is strict by default and Tesla adds fields to these messages every few months, which would otherwise break decoding in the field rather than degrading gracefully. Note InvalidJsonException does not derive from InvalidProtocolBufferException, so both are caught. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
syncTeslaProtos checks upstream weekly and opens a single pull request that updates the vendored protos and VEHICLE_COMMAND_REF together, gated by a full build and test run so a breaking upstream change fails in a pull request rather than on someone's car. It checks out and targets develop explicitly, because a scheduled run would otherwise use the default branch and carry master's whole state into the pull request. protoDriftCheck fails any pull request where the vendored protos no longer match the pinned ref, which is the only way the container's Go build and TSC's generated C# can end up on different versions of the wire format. Note both triggers only fire once the workflow file reaches the default branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Verified against a real car with its driver door open: protojson sends only the closure that is not closed, and vehicleLockState disappears entirely because VEHICLELOCKSTATE_UNLOCKED is 0. Both fixtures previously carried that field, a shape the container can never emit - the same omission rule that caused the original defect, one field further along. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…configurable Charging commands were suspended from the very first missed beacon scan until the next hit. That is right for a healthy radio, where misses are rare, but a weak link misses scans on a car standing right next to the adapter - measured on a real setup: 12 misses to 3 hits over three and a half minutes with the car awake and plugged in. Presence then stays uncertain nearly all the time, GetTargetValue keeps returning null, and the car is never regulated while the "BLE connection is unstable" reason is shown permanently. Adds BleMissesBeforePresenceUncertain to the base configuration (default 2, range 1 to 100, empty uses the default). 1 restores the previous behaviour. Away detection is deliberately untouched: it still needs its own confirmation duration and at least two misses, so a car that really left is caught on the same schedule as before. Only the interim suspension is relaxed. The miss counter was capped at MinimumMissesToConfirmAway + 1, which would have made any tolerance above 3 unreachable and the setting silently useless; the cap is now well above the configurable maximum and a test pins that. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Measured on a real car: a parked Tesla advertises only about every twelve seconds, so the three second scan window found it in 4 of 18 attempts even at -65 dBm with the car metres away. The radio was fine throughout - it decoded 54 to 105 advertisements per scan from a nearby device - the car is simply quiet. Every miss then blocks charging control, which is what made the car look permanently out of range while plugged in. The Go worker already accepted windowMs per request, but neither the container API nor TSC exposed it, so everything ran on the container's own short default. This plumbs it through: BleBeaconScanWindowSeconds in the base configuration (default 7, range 1 to 60) travels to the container as a query parameter, and the container's and daemon's own defaults move to 7 for direct callers. A longer window is close to free: the scan is cancelled the moment every car has been heard, so it only ever runs to its full length when a car really is away. The refresh interval help text now says to keep the two in proportion. Bumps the lockstep compatibility version, as both sides have to move together for the setting to have any effect. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Presence is decided from one bit per scan, but the container reports RSSI, how far into the window the car was heard, and how many other advertisements the radio picked up. All of it was parsed and dropped at the point the bool was extracted. It is now kept per car, which is the difference between "BLE feels unstable" and knowing whether the car is quiet or the adapter is deaf. The Support page gains a per car beacon history: hit rate, average signal and longest run of misses, a strip of one box per scan so the pattern is visible at a glance, and the per scan detail below it. Presence detection itself is untouched - this only records what the scan saw. The store is bounded by count and by age, because the poll interval is configurable and a count alone would cover minutes on a fast interval and hours on a slow one. Age is measured against the newest sample rather than the wall clock so a stalled poller cannot silently empty the history. Reset deliberately keeps the history: it is diagnostic, and it is most useful exactly when a car was just reset because it looked away. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…d scan A car standing in the garage was heard in only every third 7 s scan window (31 % of 200 recorded scans, measured on car 11), which made TeslaSolarCharger suspend its charging commands while the car answered every command it was sent. The radio was sampling a signal the car emits rarely and ignoring one that is continuously available. The worker now runs one long living scan whenever nothing else needs the adapter, records per car when it was last heard, and answers presence from that memory instead of listening for a window. beaconScan and the pre-connect gate both read the same registry, so the whole worker has one radio path left instead of three, and an absent car no longer occupies the adapter at all. Three properties of the stack decided the design: - go-ble's Dial does not stop scanning first and controllers reject LE Create Connection while a scan runs, so stopping the scan before a connect is a correctness duty. The radio arbiter registers a waiter before it cancels the running scan and refuses to re-arm while any waiter exists: a command waits for one scan disable round trip at most and can never be starved. - Every device.Scan call reallocates go-ble's advertisement history and an unpaired scan response makes it drop the rest of the HCI event, so the scan is never restarted on a timer, only on demand or by the deafness watchdog. - An established connection holds no lock, so scanning alongside an open link is possible; whether it is harmless is a hardware question, hence -scan-while-connected. A car whose local name only travels in the scan response is heard as a nameless advertisement most of the time, so an address confirmed by a named advertisement counts for it until the binding expires. namedCount against addressCount measures how much that is worth. Presence is also recorded when a command reaches the car, which closes the circular dependency that started this: presence gated the command, and the command would have proven presence. Adds Debug/ScannerStatus as the instrument (per car cadence, duty cycle, advertisement rates, worker CPU) and Debug/SetScannerMode so the scan modes can be compared on real hardware without a redeploy. The TSC wire contract is unchanged, so the lockstep version stays where it is; beaconScan now answers "heard within the max age" and an unchanged TSC gets a strictly better signal. 824 tests green, go build/vet/test -race green in the image build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e build The release image is built for linux/amd64, linux/arm64 and linux/arm/v7, and the race detector does not exist on 32 bit arm at all, so the build failed with "-race is not supported on linux/arm". arm64 is excluded as well: that leg is QEMU emulated on the amd64 runner, where the race detector is slow and unreliable. amd64 is the native leg, so the check still runs once per build and the other legs run the same tests without it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Only scans that had already ended were counted, so a scanner sitting in one long uninterrupted scan - which is its normal state - reported a duty cycle falling towards zero and paused time it never spent. Measured on real hardware: 54.5 % duty and 105 s "paused" while advertisements kept arriving at 84/s and neither a command nor a watchdog restart had happened. That number is what the whole rework has to be judged by, so the snapshot now adds the time since the running scan started, and a test with an injected clock pins both halves: an uninterrupted scan reports no paused time, and time handed to a command counts as paused without counting the ended scan twice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.