Ability to pick tinygo vs go-ble impl by zlymeda · Pull Request #400 · teslamotors/vehicle-command

zlymeda · 2025-05-03T22:01:32Z

Description

Resurrecting the work done in this MR: #373
Having 2 implementations for ble. go-ble and tinygo or another custom impl

if useTinyGo {
    adapter, err = tinygo.NewAdapter(btAdapter)
} else {
    adapter, err = goble.NewAdapter(btAdapter)
    if err != nil && goble.IsAdapterError(err) {
	logger.Print(goble.AdapterErrorHelpMessage(err))
	return
    }
}

Also forked go-ble to https://github.com/zlymeda/go-ble see go-ble/ble@master...zlymeda:go-ble:master

replaced github.com/JuulLabs-OSS/cbgo with github.com/tinygo-org/cbgo
merged fix Resolve blocking channel
merged fix use RxMtu() for long read
added updated go + deps

I can only test on linux, so if someone can test on MacOs as well as Windows (using tinygo) that'd be great.

I compiled tesla-control and used both tinygo and go-ble. I was able to control my tesla using both (on llinux).

Type of 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

Breaking changes

The ble package now needs and adapter implementation.

adapter, _ := tinygo.NewAdapter("")
// or adapter, _ := goble.NewAdapter("")

// takes extra adapter argument
_, err := ble.ScanVehicleBeacon(ctx, vin, adapter)

// takes extra adapter argument
conn, err := ble.NewConnection(ctx, vin, adapter)

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.

Lenart12 · 2025-05-05T13:10:49Z

pkg/connector/ble/goble/ble.go

+)
+
+func init() {
+	iface.RegisterAdapter(adapter{})


There is no way to register this (or bluez) adapter after the fact, meaning a decision to use one or another during runtime would currently be impossible. Maybe you can add a "UseAdapter" function, or make the adapter public.

Also, maybe we can use go-ble by default - if impl is nil during InitAdapter call, load go-ble adapter.

ok, switched it up a bit.

moved some types to iface subpackage to prevent circular imports

top-level ble pkg is now more like a facade and for registering the adapters

go-ble is now default, no need for naked imports

also updated the description, as the breaking change is now gone, I used aliases to resolve that

sethterashima · 2025-05-06T17:13:47Z

pkg/sign/sign.go

+// [MessageForVehicle].
 //
 // The function overwrites the audience ("aud") and issuer ("iss") JWT claims.
-func SignMessageForFleet(privateKey authentication.ECDHPrivateKey, app string, message jwt.MapClaims) (string, error) {


Please avoid unnecessary breaking API changes

will revert and disable linter rule

An anti-stutter linter rule would be nice, since it's a common slip. Is there a way to disable the rule for these specific functions?

sethterashima · 2025-05-06T17:15:40Z

internal/authentication/native.go

 	if sk.D.Cmp(elliptic.P256().Params().N) >= 0 {
 		return nil
 	}
-	x, y := sk.PublicKey.Curve.ScalarBaseMult(privateScalar)


Are these changes required by the Go uprev? The authentication package is used internally and would ideally stay compatible with other Go versions.

this changed was "forced" by the linter. alternatively we can turn off that rule. it was some

QF1008: could remove embedded field "PublicKey" from selector (staticcheck) x, y := sk.PublicKey.Curve.ScalarBaseMult(privateScalar) ^

let me know if you'd rather disable that rule

.github/workflows/build.yml

sethterashima · 2025-05-06T17:26:41Z

pkg/connector/ble/iface/adapter.go

@@ -0,0 +1,14 @@
+package iface


To me it seems more natural to put the shared code in pkg/connector/ble rather than a separate iface directory. Any objections to moving it back?

...also would limit API breakage.

yes I had it like that first. @Lenart12 suggested using go-ble as a default, but the problem was cyclic import.
eg. go-ble wants to use ScanResult from pkg/connector/ble and pkg/connector/ble wanted to set go-ble as a default impl.

I am using this as a library and I didn't notice any breaking change. I was able to compile without a change.

I can move it back, but then library user will have to pick the impl

sethterashima · 2025-05-06T17:32:14Z

pkg/connector/ble/iface/connection.go

+			blockLength = len(out)
+		}
+
+		if err := c.writer.WriteCharacteristic(out[:blockLength], blockLength); err != nil {


The context should be passed on to the writer.

sethterashima · 2025-05-06T17:32:43Z

pkg/connector/ble/iface/connection.go

+)
+
+type Writer interface {
+	WriteCharacteristic(bytes []byte, length int) error


Why a separate length argument?

an oversight :)

sethterashima · 2025-05-06T17:41:13Z

pkg/connector/ble/iface/connection.go

+)
+
+type Writer interface {
+	WriteCharacteristic(bytes []byte, length int) error


This creates a bit of a hacky almost circular dependency. I think it would be cleaner to have the interface expose methods for for both reading (or exposing a channel for reading packets) and writing; the implmentation should be self-contained.

I will try to re-think this. first step was to separate it out, but now I got more familiar with both versions, I will try to make it better

…and fix issues found by the linter

…oving some types to iface to prevent circular dependencies, top-level ble pkg is more like a facade

zlymeda · 2025-05-25T20:31:48Z

finally had some time to work on this.

pls take a look and let me know, I ran few basic tests, but will test more in upcoming days

sethterashima · 2025-05-27T23:22:21Z

pkg/cache/example_test.go

 import (
 	"context"
 	"fmt"
+	"github.com/teslamotors/vehicle-command/pkg/connector/ble/goble"


Nit: group with other local imports

sethterashima · 2025-05-27T23:22:49Z

pkg/cli/config.go

 	"errors"
 	"flag"
 	"fmt"
+	"github.com/teslamotors/vehicle-command/pkg/connector/ble"


Group with other local imports

sethterashima · 2025-05-27T23:24:39Z

pkg/cli/config.go

 		flag.StringVar(&c.Backend.FileDir, "keyring-file-dir", keyringDirectory, "keyring `directory` for file-backed keyring types")
 		flag.BoolVar(&c.Debug, "keyring-debug", false, "Enable keyring debug logging")
 	}
+	if c.Flags.isSet(FlagBLE) && c.Flags.isSet(FlagVIN) {


I don't think we'd ever require FlagBLE without FlagVIN, but why is FlagVIN made explicitly required here?

ok, I can remove it

sethterashima · 2025-05-27T23:25:28Z

pkg/cli/config.go

 		flag.BoolVar(&c.Debug, "keyring-debug", false, "Enable keyring debug logging")
 	}
+	if c.Flags.isSet(FlagBLE) && c.Flags.isSet(FlagVIN) {
+		flag.BoolVar(&c.BtTinyGo, "tinygo", false, "Use tinygo ble impl (go-ble is the default.")


God help us if we end up needing more than two adapter implementations, but thoughts on bool vs string here?

yes, will do

updated to use enum

sethterashima · 2025-05-27T23:38:03Z

pkg/connector/ble/goble/adapter.go

+
+	"github.com/teslamotors/vehicle-command/pkg/connector/ble"
+	"github.com/teslamotors/vehicle-command/pkg/protocol"
+	goble "github.com/zlymeda/go-ble"


Why change to a fork?

This specific fork uses tinygo, so why use both this and tinygo directly?

I wrote it in the description:

forked go-ble to zlymeda/go-ble see go-ble/ble@master...zlymeda:go-ble:master

replaced github.com/JuulLabs-OSS/cbgo with github.com/tinygo-org/cbgo

merged fix Resolve blocking channel

merged fix use RxMtu() for long read
added updated go + deps

that fork does not use tinygo, it uses only cbgo part of tinygo (which is for MacOS), same as this repo did using the replace

replace github.com/JuulLabs-OSS/cbgo => github.com/tinygo-org/cbgo v0.0.4

without that I was getting issues (using the replace + using tinygo), it resulted in an error:

go mod tidy go: github.com/tinygo-org/cbgo@v0.0.4 used for two different module paths (github.com/JuulLabs-OSS/cbgo and github.com/tinygo-org/cbgo)

so in order to fix that I "moved" the replace directly into the fork

is there anything else I can do? did I answer all the questions? pls let me know

sethterashima · 2025-06-17T22:17:55Z

go install ./cmd/...
# github.com/teslamotors/vehicle-command/pkg/connector/ble/tinygo
pkg/connector/ble/tinygo/adapter.go:122:3: unknown field MACAddress in struct literal of type bluetooth.Address
pkg/connector/ble/tinygo/device_darwin.go:22:9: not enough return values
	have (*bluetooth.Adapter)
	want (*bluetooth.Adapter, error)
make: *** [install] Error 1

git rev-parse HEAD
f8b08efe9eff5ab22d1722e57a9449e8e1054814

sethterashima · 2025-06-17T22:19:39Z

examples/ble/main.go

 	flag.StringVar(&privateKeyFile, "key", "", "Private key `file` for authorizing commands (PEM PKCS8 NIST-P256)")
 	flag.StringVar(&vin, "vin", "", "Vehicle Identification Number (`VIN`) of the car")
 	flag.BoolVar(&debug, "debug", false, "Enable debugging of TX/RX BLE packets")
+	flag.BoolVar(&useTinyGo, "tinygo", false, "Use tinygo ble impl (go-ble is the default")


Use tinygo BLE implementation instead of go-ble

sethterashima · 2025-06-17T22:23:41Z

pkg/cli/config.go

 		flag.BoolVar(&c.Debug, "keyring-debug", false, "Enable keyring debug logging")
 	}
+	if c.Flags.isSet(FlagBLE) {
+		flag.Var(&c.BtImpl, "bt-impl", "ble impl to use: goble/tinygo")


BLE implementation to use. Allowed values are "tinygo" and "goble" (default).

zlymeda · 2025-06-19T07:13:44Z

go install ./cmd/...
# github.com/teslamotors/vehicle-command/pkg/connector/ble/tinygo
pkg/connector/ble/tinygo/adapter.go:122:3: unknown field MACAddress in struct literal of type bluetooth.Address
pkg/connector/ble/tinygo/device_darwin.go:22:9: not enough return values
	have (*bluetooth.Adapter)
	want (*bluetooth.Adapter, error)
make: *** [install] Error 1

git rev-parse HEAD
f8b08efe9eff5ab22d1722e57a9449e8e1054814

this should be fixed now

sethterashima · 2025-06-24T00:40:49Z

On MacOS, using tinygo:

tesla-control -ble -debug -bt-impl tinygo ping

2025-06-23T17:21:59-07:00 [debug] Connecting over BLE...                                                                                                                                                                                                            
2025-06-23T17:22:02-07:00 [warn ] ble: failed to stop scan: not calling Scan function                                                                                                                                                                               
2025-06-23T17:22:02-07:00 [info ] Connecting to car...                                                                                                                                                                                                              
2025-06-23T17:22:02-07:00 [info ] Starting dispatcher service...                                                                                                                                                                                                    
2025-06-23T17:22:02-07:00 [info ] Securing connection...                                                                                                                                                                                                            
2025-06-23T17:22:02-07:00 [info ] Session for DOMAIN_INFOTAINMENT loaded from cache                                                                                                                                                                                 
2025-06-23T17:22:02-07:00 [info ] Session for DOMAIN_VEHICLE_SECURITY loaded from cache
...
2025-06-23T17:23:19-07:00 [warn ] [421ee3ca2f5315349589acfcb1e1b9c3] Terminal transmission error: timeout on Write()

On an ubuntu system, both BT implementations work, but tinygo results in a short but notable delay between when the tesla-control finishes its work and when the program terminates (note the timestamps):

35a6eded15336bacd0cbf037afa53c971345e7fd60db6bcdbf845b02b9e1f53f1eadddbaa89d6a244a220a0ce8512e8cf7d3830bfd04e9e610011a105a1c2ef0a785f387cad3b2c3311109a0920310dd450015070888088fcb19f395fdab849a0310d238f5b6a38d75f63b4af79acf224373
{
  "climateState": {
    "driverTempSetting": 24,
     // snip
  }
}
2025-06-23T17:40:17-07:00 [debug] RX: 320208003a020802521f1a1d12160a14de47322bad058717c1279f58160c974abd57b8ca1802220101
2025-06-23T17:40:18-07:00 [debug] RX: 320208003a020802521f1a1d12160a14de47322bad058717c1279f58160c974abd57b8ca1802220101

When I tried to reprodce the above, I got this error spammed. So in addition to the error itself, there's the secondary issue of writing to stderr in a pretty tight loop:

2025-06-23T17:39:06-07:00 [warn ] BLE connection attempt failed: Method "Get" with signature "ss" on interface "org.freedesktop.DBus.Properties" doesn't exist

2025-06-23T17:39:06-07:00 [warn ] BLE connection attempt failed: Method "Get" with signature "ss" on interface "org.freedesktop.DBus.Properties" doesn't exist

2025-06-23T17:39:06-07:00 [warn ] BLE connection attempt failed: Method "Get" with signature "ss" on interface "org.freedesktop.DBus.Properties" doesn't exist

2025-06-23T17:39:06-07:00 [warn ] BLE connection attempt failed: Method "Get" with signature "ss" on interface "org.freedesktop.DBus.Properties" doesn't exist

2025-06-23T17:39:06-07:00 [warn ] BLE connection attempt failed: Method "Get" with signature "ss" on interface "org.freedesktop.DBus.Properties" doesn't exist

zlymeda · 2025-07-11T19:21:51Z

Hi,

the tight loop comes from here:

pkg/connector/ble/ble.go

for {
  conn, err := tryToConnect(ctx, vin, beacon, adapter)
  if err == nil {
	  return conn, nil
  }
  
  log.Warning("BLE connection attempt failed: %+v", err)
  if err := ctx.Err(); err != nil {
	  if lastError != nil {
		  return nil, lastError
	  }
	  return nil, err
  }
  lastError = err
}

maybe we could add small sleep like 100ms between tries?

I dont have any MacOS device, so I can't really debug the issue. Is there more relevant logs?

I also have ubuntu and when trying to get the climate state, I dont have any significant delay:

2025-07-11T21:14:57+02:00 [debug] RX: 321212106995d7e885079386657599c25aea6e863a020803529501ab23f3d1eeb089fc3d9dbde84a25a6730966e06ef58a998241fc99e0b8eb8d6cb7fb732aa43cc3475af7254c11fd6fe271c7154c1e349d6e2684bb60487798efdbaee27c2ceae06c87ef6461626f7dfa250bdeacde3bac903e31f19320c1998d299e39f0aed13afff37f5ea189f9266e943a77bd11f6187336a9b11dfab065b801482765efd93a1163b03a77a8d09ea5cd41abb2da6a244a220a0cc8b2b819a925a5d93d113ff710011a10c6b522ee735548945da47d21a06d9402920310686998498171de51694b55d46416e4309a03108e61941e3ec24983f6e83c185a6b24f8
{
  "climateState":  {
    "insideTempCelsius":  22.6,
// snip
}
2025-07-11T21:14:57+02:00 [debug] RX: 320208003a020802521f1a1d12160a1457e52e846dcf362ba45d16a07463c1c6b6f6b7c01802220101
2025-07-11T21:14:59+02:00 [debug] RX: 320208003a020802521f1a1d12160a1457e52e846dcf362ba45d16a07463c1c6b6f6b7c01802220101

The issue with "BLE connection attempt failed: Method "Get" with signature "ss" on interface "org.freedesktop.DBus.Properties" doesn't exist"
seems to be intermittent. It seems the scan was successful, but when trying to use the beacon, it fails. however, it works on another try.

I am using both tinygo and goble for my automations and both are running relatively without problems. One is on raspberry pi 3 running debian, another is a HP mini PC also running debian.
I use tinygo on raps PI and goble on the HP mini. sometimes I need to restart the rasp. pi, the ble adapter stops responding. that happens like once in a month, not sure why..

zlymeda mentioned this pull request May 3, 2025

Replace go-ble/ble with tinygo-org/bluetooth #373

Closed

8 tasks

zlymeda force-pushed the main branch from 2eaedc9 to d7af7f3 Compare May 3, 2025 22:28

zlymeda marked this pull request as ready for review May 3, 2025 22:41

Lenart12 reviewed May 5, 2025

View reviewed changes

zlymeda force-pushed the main branch from bec7ed1 to b7381d6 Compare May 5, 2025 20:47

sethterashima reviewed May 6, 2025

View reviewed changes

.github/workflows/build.yml Show resolved Hide resolved

sethterashima reviewed May 6, 2025

View reviewed changes

zlymeda added 7 commits May 24, 2025 20:20

replace go-ble

65e42ea

ble: extract common code and have go-ble + tinygo implementations

3e597ed

upgrade to go 1.24.2; upgrade linter to 2.1.5, migrate linter config …

5f00d5b

…and fix issues found by the linter

tinygo: fix InitAdapter not setting the global device

ad9f2af

test: fix race condition

a7ee89d

ble: use goble by default, allow switching the impl during runtime, m…

e594dd0

…oving some types to iface to prevent circular dependencies, top-level ble pkg is more like a facade

review changes

c4bd391

zlymeda force-pushed the main branch 2 times, most recently from 26d4324 to 7ef178c Compare May 25, 2025 20:24

Redesign

48118e2

zlymeda force-pushed the main branch from 7ef178c to 48118e2 Compare May 25, 2025 20:27

close: make sure it can be called multiple times without seg. fault

eb22be8

sethterashima reviewed May 27, 2025

View reviewed changes

cli: use enum for ble impl

cede070

iainbullock mentioned this pull request Jun 2, 2025

Add option to choose the controller in case there are more than one tesla-local-control/tesla-local-control-addon#114

Open

zlymeda added 2 commits June 17, 2025 12:03

tinygo adapter: fix the case when device.Scan returns err

97e163a

mod: updates

f8b08ef

sethterashima reviewed Jun 17, 2025

View reviewed changes

zlymeda added 2 commits June 19, 2025 09:11

flags: update descriptions

4811b91

fix: MacOS uses UUID instead of MACAddress

cd6da64

Conversation

zlymeda commented May 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Breaking changes

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zlymeda commented May 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sethterashima commented Jun 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zlymeda commented Jun 19, 2025

Uh oh!

sethterashima commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zlymeda commented Jul 11, 2025

Uh oh!

Reviewers

Assignees

zlymeda commented May 3, 2025 •

edited

Loading

sethterashima commented Jun 24, 2025 •

edited

Loading