Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

linux & macos: adapter power state handling #188

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

TirelessDev
Copy link
Contributor

A simple PR to address some of the issues brought up in #184. In particular, this adds a state change handler to the adapter which calls a provided callback when the powered state changes.

On macos this uses the existing CentralManagerDidUpdateState process that is already used to know when the adapter has been correctly enabled. On linux this watches for the Powered adapter D-Bus property to change.

This has been tested on an M1 Macbook Pro and a Raspberry Pi 4.

This PR modifies the existing API by adding (a *Adapter) SetStateChangeHandler(c func(newState AdapterState)). I also add some const states to provide generality between platforms, although these are probably unnecessary.

Unknowns:

  • Does bare-metal have any comparable functionality that could be linked in? Or would this be a no-op?
  • I do not have a windows machine handy so can not develop an implementation for that.

@xeanhort One additional note about #184, while this PR does not directly provide a mechanism to confirm if the adapter is scanning correctly or not. There is an adapter property called Discovering that can be watched to hopefully provide this insight. This can be easily integrated into the watching procedure added to adapter_linux.go in this PR.

If we can find a similar signal on the other platforms perhaps we can add it to the state change callback in a future PR.

@TirelessDev TirelessDev changed the base branch from release to dev August 20, 2023 03:35
@ansoni-san
Copy link
Contributor

ansoni-san commented Aug 20, 2023

Yay 🎉 Thank you so much for this.

Hmm, I am thinking how do we determine the initial adapter state on all platforms? Probably the library should always trigger an initial statechange event (after an Enable) based on whatever information it can get out of the platform.

The behaviour will need to belong to the library, as we can't trust the underlying implementations to behave the same.

@TirelessDev
Copy link
Contributor Author

TirelessDev commented Aug 20, 2023

On each platform, the Enable function will return an error if the adapter can't be turned on correctly. On macos this occurs after a timeout if the "Powered" signal doesn't arrive. This is the same powered signal used in this PR for the state-change callback. So at least on macos you could set up the callback before calling enable and expect to get the callback firing.

On linux the Enable function also enables a watch for the adapter property changes so the timing probably won't work as enable doesn't seem to actually turn anything on, it just fetches a reference to the D-Bus adapter object iiuc. In this case, what is probably more desirable is confirmation that the adapter is scanning correctly which can be confirmed by watching for the Discovering property on the adapter. However, there is not really an analog I can find on the macos side except for the isScanning property which looks to only be pollable but I will do some more digging.

I have no insight into the windows side, so if you are able to get something similar working on that end that would be awesome.

Also, I am not sure why tests are failing for some of the bare-metal targets. I need to dig into that as well.

@ansoni-san
Copy link
Contributor

ansoni-san commented Aug 20, 2023

So far my experience is that adapter.Enable does not fail when the adapter is disabled both on macOS and Windows.

Are you getting different behaviour? Maybe it's a different discussion 🤔

@TirelessDev
Copy link
Contributor Author

I can't speak for Windows, but on my M1 Macbook, if I start my application with Bluetooth turned off, calling Enable will fail after the 10-second timeout with the error timeout enabling CentralManager.

I have just tested on linux and I do not get an error when calling Enable with Bluetooth turned off. However, the new adapter state callback does fire when I eventually turn it on and from that you can kick things off correctly. The problem with this is that because there is no error the app can just assume everything is okay and continue to start scanning. Fortunately, the adapter.Scan function does return an error when you attempt to start it without the adapter actually being turned on. So as long as you catch that (make sure you subsequently call adapter.StopScan() to close the chan's) you should have a pretty robust solution.

@deadprogram deadprogram requested a review from aykevl August 25, 2023 06:48
@aykevl
Copy link
Member

aykevl commented Aug 29, 2023

Does bare-metal have any comparable functionality that could be linked in? Or would this be a no-op?

On bare metal, the Enable call directly enables the whole stack. An event system that waits until the stack is enabled doesn't make much sense there.

What actual problem are you trying to solve with this PR? I can think of a few:

  • Wait for Bluetooth to be enabled on startup.
  • Bluetooth is disabled, wait for the user to enable it.

For the first, if we know that Bluetooth is starting but not yet finished, I'd argue the proper way to fix it is not by introducing a new API but instead to make sure Enable waits until the adapter is fully enabled.

For the second, an event based system does make sense but I'm not sure this is actually the problem that you're trying to solve? Also, I'm not sure when that would be useful in practice.

@TirelessDev
Copy link
Contributor Author

In our case, we are building persistent Bluetooth gateway software that runs on MacOS and Linux to interface with our devices remotely. Think Bluetooth shell, SMP configuration, and OTA.

This software needs to be robust and persistent. On MacOS, and MacBooks in particular, having our software maintain functionality after the laptop has been closed and woken up again is critical, without the changes in this PR I wasn't able to find a way to capture the state changes that MacOS puts the Bluetooth adapter through. Less of a typical experience but still important is having the ability to respond to manual enabling and disabling of the adapter and having our software persist through such events.

@ansoni-san
Copy link
Contributor

I feel like there should be something stronger in this library's interface which allows a user to handle this in a normalized cross-platform way.

This issue currently happens with the connect / disconnect handler. We need a few more simple and explicit checks we can make so that we can ignore the underlying platform quirks. Also you don’t always receive these events depending on what happened.

So I think we should either:

  1. Provide explicit methods to check:
    - Is the adapter enabled or disabled (without having to call Enable as that is very messy as a probe)
    - Is this device connected or disconnected (without having to constantly probe by making useless calls again)

    These methods should belong to the relevant objects themselves. Otherwise you end up doing a lot of work with a dead device object, or disabled adapter, and then end up having to unwind everything. Also you cannot always unwind an entire user journey without pissing them off.

Or:

  1. Fire these events ourselves so that the behaviour is well defined (define these as library events so we can commit to enforcing some well-defined reliable behaviour)

We basically need to some way to find out the state (pull), and some way to be told the state (push). One without the other doesn't quite cover the cases.

@aykevl
Copy link
Member

aykevl commented Sep 2, 2023

On MacOS, and MacBooks in particular, having our software maintain functionality after the laptop has been closed and woken up again is critical, without the changes in this PR I wasn't able to find a way to capture the state changes that MacOS puts the Bluetooth adapter through.

Right. I assume these are pre-M1 laptops that fully shut down Bluetooth when suspended? I'm not sure about the newer ones but I think they keep Bluetooth alive when the lid is closed (at least that's what I experienced).

This seems like a good argument for adding some way to listen for changes. I hadn't considered suspend before.

Also, adding methods to check the current state seems reasonable. Those will probably be very simple.

Comment on lines +14 to +15
// AdapterStateUnknown is the state of the adaptor when it is unknown.
AdapterStateUnknown
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it actually possible for the state to be unknown, and under what circumstances can this happen?

If possible, I'd rather avoid this condition as it's just another special case.

If there are only two states, we can just use a boolean (enabled bool as a parameter for example).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They vary between OS's.

The MacOS implementation does define an unknown state along with a few others . On the linux side the Powered state that this PR watches only has on or off.

The Unknown was just an attempt to do 'something' with the extra ones that Core Bluetooth throws at us. I wasn't sure if the other states are merely transient and the adapter will always eventually fall into an on | off state or if there is a chance the states like CBManagerStateUnsupported may be a permanent latch.

@TirelessDev
Copy link
Contributor Author

Right. I assume these are pre-M1 laptops that fully shut down Bluetooth when suspended? I'm not sure about the newer ones but I think they keep Bluetooth alive when the lid is closed (at least that's what I experienced).

Actually, I am experiencing this on an M1, 14-inch Macbook Pro. I typically have it on power saving mode so that could be the explanation.

@TirelessDev
Copy link
Contributor Author

I have added some simple queries to get the adapter state on macOS and Linux.

@deadprogram
Copy link
Member

@deadprogram
Copy link
Member

Regarding nrf, we cannot default to the unknown state until Enable() is called and still use the same logic to determine if adaptor is powered on. So that is a little trickier to handle?

We could default to PoweredOn on nrf, but that seems inconsistent with the meaning on the other platforms.

@aykevl
Copy link
Member

aykevl commented Feb 17, 2024

Looking at this PR again, I think this would be a helpful feature.
@TirelessDev can you rebase this on the dev branch? It will likely require some relatively big changes for Linux (because I rewrote most of that on #216).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants