Replace go-ble/ble with tinygo-org/bluetooth#373
Replace go-ble/ble with tinygo-org/bluetooth#373Lenart12 wants to merge 20 commits intoteslamotors:mainfrom
Conversation
This was causing issues if multiple scans were being requested one after another quickly.
790fba7 to
7ba27d5
Compare
|
Tested this on macOS 14.6.1 using a variety of |
|
Plesantly suprised it worked without changes, I was unable to test on mac so It only worked "in theory". Great :^) |
|
Ah... scan is still getting stuck 🤬 was testing overnight and it locked up. Not much logs to investigate it unfortunately. Edit: Here we can see scan is stopped but code from library never finishes, and connection status commands start stacking up. It should show Working example: This seems to me that it is an issue with the bluetooth library... (and/or by other applications using the same adapter) EDIT: Another event now with btmon logs... Happened at 17:20:14 By looking at the logs It seems to me that the issues is that another program restarted the adapter while the scan is active. Probably the library doesn't handle such cases yet. |
|
Alas it was not library bug. What happened:
|
pkg/connector/ble/device_darwin.go
Outdated
| device, err := darwin.NewDevice() | ||
| if err != nil { | ||
| return nil, err | ||
| func IsAdapterError(err error) bool { |
There was a problem hiding this comment.
Function causes make linters to fail:
pkg/connector/ble/device_darwin.go:8:21: unused-parameter: parameter 'err' seems to be unused, consider removing or renaming it as _ (revive)
I.e., change to IsAdapterError(_ error) bool {
examples/ble/main.go
Outdated
| return | ||
| } | ||
|
|
||
| // For simplcity, allow 30 seconds to wake up the vehicle, connect to it, |
There was a problem hiding this comment.
Might as well fix "simplicity" typo next time you do a push
| } | ||
| device, err := adapter.Connect(target.Address, params) | ||
| if err != nil && !connectionCancelled { | ||
| errorCh <- err |
There was a problem hiding this comment.
I think there's still a race condition here; if we enter into this if branch, and then the main thread sets connectionCancelled = true, we'll be blocked trying to write to errorCh and no one will ever unblock us by reading from it. Similarly, there's a race with the else if branch. We could solve both of these problems by making the errorCh and deviceCh channels buffered, which will allow the goroutine to write to them even if no one is listening:
deviceCh := make(chan bluetooth.Device, 1)
errorCh := make(chan error, 1)
This should also obviate teh need to use connectionCancelled, which is a bit sus anyway since AFAIK reading/writing to non-atomic booleans isn't well-defined behavior.
|
This is just about ready to merge, but I'm about to head out on a vacation with limited availability. @patrickdemers6, if I'm not around and the responses to the above comments look good, I think we can merge. |
|
Still want to fix an error before merging inside |
|
I want to wait on tinygo-org/bluetooth#342 before merging |
|
Hey @sethterashima I have a questing if this is a regression, or not... If I keep the connection open for longer than 30s the car closes connection and if that happens, when i try to open the connection next time it will fail. If I close the connection before 30s it seems to not fail. I don't know if this is issue with tesla, bluez, or tinygo-org/bluetooth, maybe you will know... In the attachment I sent bluetooth and dbus capture you can open with wireshark. You can check packet 628 that fails and then 919 retries and succedes. Again 3062, 3265 (failed twice) and 3547 EDIT: Actually I think this is unrelated to the "30s timeout"... need to figure out what is causing this EDIT: I acually think this is an bluez issue (related ukBaz/BLE_GATT#9) as it is happening even in bluetoothctl tool. I think I need to implement the same workaroundin the bluetooth library. |
|
Hi @Lenart12 I've been watching this with interest because the current main branch 3.0.3 bluetooth library in use will go off with the fairies after 1-2 weeks of otherwise stable use, and only way to bring it back it a cold boot. I've been testing your PR on a raspberry pi 4 and whenever I try any BLE commands, it hard locks up and tesla-control will not return, can't be killed :( only way to bring it back is a power cycle... Sometimes, the very first command works, but then second one locks it hard. |
|
@iainwhyte Could you run with |
|
Also while monitoring with |
Changed to branch tinygo-bluetooth to get the monitor.sh and now it seems to working perfectly (doh!). Attached the bluetooth capture of me waking and honking horn, in case its still of interest. I will run this branch for a week and see how it goes. Thanks! |
| return | ||
| } | ||
|
|
||
| if result.LocalName() == localName { |
There was a problem hiding this comment.
This code has the same issues described in #373 (comment).
|
I know this is taking a while to get in, but I want you to know that your work here is appreciated, @Lenart12. I think we're in the homestretch. |
|
I think this approach wont work. As much as I hoped that BlueZ would solve much of the issues some people are experiencing in reality it seems that it is not stable enough to use reliably atleast on my setup (RPi 4B+). In some cases it blocks up the whole adapter for 30+ seconds.... (by failing to make a connection to the car) This is an issue with BlueZ not with tinygo-org/bluetooth and as such we can't really fix it here or in the library, (only by changing hardware). I guess the best way to support as many people would be to use both libraries and allow the user to select whichever one they want, but then that seems like a waste and too much complexity. @sethterashima what do you think should be done as fully switching the project to this library will break things for some people depending on the setup. I have not heard much feedback on which one most people prefer. |
|
It's discouraging that Golang BLE support is finicky. But thanks for your work here. Hopefully in the not to distant future the upstream issues will be resolved and we can blow the dust off of this PR. |
|
I have been using the tinygo ble version for some time and it worked well. I switched to the latest main branch that uses go-ble, and after a while the library wasn't able to "down" the hci, was getting connection timeout - actually I had to restart my raspberry pi in order to make it work again. reverted again to tinygo, as it seems more stable (at least for me). I think it'd be worth to be able to pick the implementation by a library user. I could draft a PR if you'd be interested. |
|
Having some system to choose the BT library would be great for me. Right now, I have a Homeasistant addon using this library, and I need to include two compiled versions of the program. One with the current implementation, and one with tinygo-org library. If instead we could select it perhaps during adapter initialization that would be great. Maybe even allowing to pass an implemented interface to allow users of the library to have complete HAL. |
|
I started on it, but found some issues when trying to use both go-ble and tinygo at the same time. removing the replace works, but that means cbgo for go-ble is used from The cleanest would be to have different impl packages, one for tinygo and one for go-ble. And this repo would be extracted as a library and the current repo would have runnable commands and the examples (picking one ble impl by default) What do you think would be the best course of action to support multiple ble impls? I have experimented with the |
|
another idea is to have this repo as-is, using go-ble as default (for now at least), but still implement interfaces to be able to replace the impl. and tinygo version would be a different repo and then whoever uses this repo as a library can use the default or use the tinygo one (if they choose to) |
|
Would it be easier to fork go-ble/ble and change the cbgo dependency there. That repo is unmaintained anyways... |
|
created a draft today: #400 still WIP, need to test it |
Description
As per title... works great with my setup (RPi 3B+, Debian 12).
New stuff:
Small example:
Closes #372
Please select all options that apply to this change:
Checklist:
Confirm you have completed the following steps: