Refactor BLE connecting to allow scanning for vehicle presence #353

Lenart12 · 2025-01-18T16:24:54Z

Description

This PR splits the ble.tryToConnect method in to two parts, by factoring out its first step in which we scan for a suitable tesla BLE advertisement (beacon). This is achieved by putting the code in a new ble.scanVehicleBeacon method along with other smaller refactorings. This allows us to expose the new method with ble.ScanVehicleBeacon.

The end user can then use a different (shorter) timeout to check for the presence of the vehicle before deciding if it wants to try to connect, since if a beacon got detected it would make sense to use a longer timeout to connect, or even do multiple retries, and if it wasn't detected allow for faster error handling.

Example

// Old way
conn, err := ble.NewConnection(ctx, vin)

// New way

// Either try to connect immediatly
conn, err := ble.NewConnection(ctx, vin)
// Or
scanCtx, scanCancel = context.withTimeout(ctx, 1*time.Second)
defer scanCancel()
beacon, err := ble.ScanVehicleBeacon(scanCtx, vin)
if err != nil {
  handleErr(err)
}
conn, err := ble.NewConnectionToBleTarget(ctx, vin, beacon)

Type of change

Please select all options that apply to this change:

New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Bug fix (non-breaking change which fixes an issue)
Documentation update

Checklist:

Confirm you have completed the following steps:

My code follows the style of this project.
I have performed a self-review of my code.
I have made corresponding updates to the documentation.
I have added/updated unit tests to cover my changes.

sethterashima · 2025-01-22T21:06:55Z

pkg/connector/ble/ble.go

+	go func() {
+		select {
+		case <-ctx.Done():
+			cancel()


Since ctx2 is created from ctx, ctx2 will be terminated automatically when ctx is done.

sethterashima · 2025-01-22T21:10:51Z

pkg/connector/ble/ble.go

+	ch := make(chan Advertisement)
+	fn := func(a Advertisement) {
+		ln := a.LocalName()
+		if ln != localName {


I know this this is copy/pasted, but we should just if a.LocalName() != localName.

sethterashima · 2025-01-22T21:18:58Z

pkg/connector/ble/ble.go

+			return
+		}
+		cancel()
+		ch <- a


A bit worried about creating something like go-ble/ble#118 here. I don't believe the interface defines whether or not blocking behavior in fn is allowed. If device.Scan blocks on fn, then there's a deadlock because ch is unbuffered and nothing reads from it until device.Scan finishes. On the other hand, if fn runs asynchronously then we could leak a go routine if two BLE devices with localName respond with advertisements, because the second will block indefinitely while waiting for ch to clear.

Edit: For context, go-ble/ble@c1142dc changed the behavior of Scan on MacOS from non-blocking to blocking, so being agnostic to this requirement would be great, and would probably allow us to uprev the ble package.

I would need help with this change 😅. I'm new to Go and also don't have a Mac to test.

I based this on your changes: Lenart12#1

Tested on Linux and MacOS.

sethterashima · 2025-01-22T21:25:03Z

pkg/connector/ble/ble.go

 	var lastError error
 	for {
-		conn, err := tryToConnect(ctx, vin)
+		conn, err := tryToConnect(ctx, vin, target)


Results in redundant calls to VehicleLocalName. Call it once in NewConnectionToBleTarget and pass the result to tryToConnect instead of the VIN.

I think the only way its called twice is when user is calling first ScanVehicleBeacon and then NewConnectionToBleTarget. If calling NewConnection (or NewConnectionToBleTarget(ctx, vin, nil)) it is only generated once in tryToConnect and passed to scanVehicleBeacon. I think avoiding double generation can be solved only by requiring the user to generate the local name and passing it to both the calls, replacing the vin argument in both ScanVehicleBeacon and TryToConnectToBleTarget. Did you mean this?

sethterashima · 2025-01-22T21:38:52Z

pkg/connector/ble/ble.go

 		return nil, ErrMaxConnectionsExceeded
 	}

-	log.Debug("Connecting to BLE beacon %s...", client.Addr())
+	log.Debug("Dialing to %s (%s)...", target.Addr(), localName)


Is connecting with target.Addr() significantly faster than scanning? If so, then it might be worth modifying the API to accept target.Addr() instead of target. This could allow clients to cache target.Addr() to use for future connections.

We'd need to consider being able to recover in cases where target.Addr() changes due to a VCSEC replacement, as well as detecting ErrMaxConnectionsExceeded. But optmizing for the common case could be useful.

Good idea, but If we would count on "longer term" address caching it would make sense to also seperate this part of the tryToConnect method to its own function, since it adds back the same pain point this PR is trying to address. Thoughts?

I'm fine punting that improvement to a different PR. The reason I proposed it because implementing it as part of this one would help limit API churn.

sethterashima · 2025-01-22T21:42:04Z

pkg/connector/ble/ble.go

 	var lastError error
 	for {
-		conn, err := tryToConnect(ctx, vin)
+		conn, err := tryToConnect(ctx, vin, target)


If target.Connectable() is false, we're just going to fail in a tight loop. Should return immediately.

sethterashima · 2025-01-24T18:59:41Z

pkg/connector/ble/ble.go

@@ -146,61 +230,51 @@ func NewConnection(ctx context.Context, vin string) (*Connection, error) {
 	}
 }

-func tryToConnect(ctx context.Context, vin string) (*Connection, error) {
+func tryToConnect(ctx context.Context, vin string, target Advertisement) (*Connection, error, bool) {


This function fails the recently merged linter checks because error isn't the final return value. Please fix and rebase off of the latest version.

Fixed linters

sethterashima · 2025-01-24T19:01:34Z

This likely has the side effect of fixing #272 by removing the direct call to device.Connect.

…eacon

The MacOS BLE Scan() method blocks until all queued instances of the callback function finish. This created a deadlock, since the callback function would try to write a matching advertisement to an unbuffered channel, and the unbuffered channel would not be read until Scan() completed. To fix the problem, this commit makes the channel buffered, allowing the callback function to succeed on the first match. The callback handles subsequent matches by detecting that the scan context has been canceled and returning. This commit also uprevs the BLE package. This was previously impossible because we relied on the device.Connect method, which is broken upstream on MacOS.

sethterashima · 2025-01-28T00:31:42Z

Thanks, @Lenart12 ! I think we ended up killing a couple of very annoying birds in the process with this one.

pkuehnel · 2025-01-28T00:43:23Z

Is updating the tesla-control CLI docs planned? I am not sure how I should check for the presence of the car. Also, how to check change the timeout and what is the default timeout?

Lenart12 mentioned this pull request Jan 18, 2025

Add BT information wimaha/TeslaBleHttpProxy#78

Open

sethterashima reviewed Jan 22, 2025

View reviewed changes

sethterashima reviewed Jan 24, 2025

View reviewed changes

Lenart12 and others added 6 commits January 27, 2025 22:40

Refactor BLE connecting to allow scanning for vehicle presence

edcf69a

Remove unnecessary goroutine for context cancellation in scanVehicleB…

7abc52f

…eacon

Simplify local name check in scanVehicleBeacon function

2527064

Do not retry BLE connecting to vehicle in certain cases

3c91d3d

Fix linter error in ble.tryToConnect

0231440

Lenart12 force-pushed the main branch from 01237bb to 0231440 Compare January 27, 2025 21:44

Lenart12 requested a review from sethterashima January 27, 2025 21:51

sethterashima approved these changes Jan 28, 2025

View reviewed changes

sethterashima merged commit 142e78e into teslamotors:main Jan 28, 2025
1 check passed

sethterashima mentioned this pull request Jan 28, 2025

ble.NewConnection sometimes never returns #272

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor BLE connecting to allow scanning for vehicle presence #353

Refactor BLE connecting to allow scanning for vehicle presence #353

Lenart12 commented Jan 18, 2025 •

edited

Loading

sethterashima Jan 22, 2025

sethterashima Jan 22, 2025

sethterashima Jan 22, 2025 •

edited

Loading

Lenart12 Jan 22, 2025

sethterashima Jan 23, 2025

sethterashima Jan 22, 2025

Lenart12 Jan 22, 2025

sethterashima Jan 22, 2025

Lenart12 Jan 22, 2025

sethterashima Jan 22, 2025

sethterashima Jan 22, 2025

sethterashima Jan 24, 2025

Lenart12 Jan 27, 2025

sethterashima commented Jan 24, 2025 •

edited

Loading

sethterashima commented Jan 28, 2025

pkuehnel commented Jan 28, 2025 •

edited

Loading

Refactor BLE connecting to allow scanning for vehicle presence #353

Refactor BLE connecting to allow scanning for vehicle presence #353

Conversation

Lenart12 commented Jan 18, 2025 • edited Loading

Description

Type of change

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sethterashima Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sethterashima commented Jan 24, 2025 • edited Loading

sethterashima commented Jan 28, 2025

pkuehnel commented Jan 28, 2025 • edited Loading

Lenart12 commented Jan 18, 2025 •

edited

Loading

sethterashima Jan 22, 2025 •

edited

Loading

sethterashima commented Jan 24, 2025 •

edited

Loading

pkuehnel commented Jan 28, 2025 •

edited

Loading