Skip to content

Expose async_scanner_devices_by_address from the bluetooth api#83733

Merged
bdraco merged 9 commits into
home-assistant:devfrom
dbuezas:feat/expose-async_get_discovered_devices_and_advertisement_data_by_address
Jan 9, 2023
Merged

Expose async_scanner_devices_by_address from the bluetooth api#83733
bdraco merged 9 commits into
home-assistant:devfrom
dbuezas:feat/expose-async_get_discovered_devices_and_advertisement_data_by_address

Conversation

@dbuezas
Copy link
Copy Markdown
Contributor

@dbuezas dbuezas commented Dec 10, 2022

Proposed change

Exposes async_scanner_devices_by_address in the bluetooth api so that integrations can decide to use a different bluetooth adapter if they wish so.
This is useful in two cases:

  1. Skipping BTProxy as backend for devices that require pairing
  2. Falling back to a second local adapter if all connection slots of the best available adapter are full.

Both are arguably best solved by fully implementing extra functionality in the core, but since those are non-trivial tasks, this may serve to "unbreak" integrations in the meantime

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@home-assistant
Copy link
Copy Markdown
Contributor

Hi dbuezas

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@home-assistant
Copy link
Copy Markdown
Contributor

Hey there @bdraco, mind taking a look at this pull request as it has been labeled with an integration (bluetooth) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of bluetooth can trigger bot actions by commenting:

  • @home-assistant close Closes the issue.
  • @home-assistant rename Awesome new title Change the title of the issue.
  • @home-assistant reopen Reopen the issue.
  • @home-assistant unassign bluetooth Removes the current integration label and assignees on the issue, add the integration domain after the command.

@home-assistant
Copy link
Copy Markdown
Contributor

Hi dbuezas

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

Comment thread homeassistant/components/bluetooth/api.py Outdated
@bdraco
Copy link
Copy Markdown
Member

bdraco commented Dec 10, 2022

We will need docs for this as well

https://developers.home-assistant.io/docs/bluetooth?_highlight=blue

and a test

@dbuezas
Copy link
Copy Markdown
Contributor Author

dbuezas commented Dec 11, 2022

I'll hold this until I find out if this is indeed useful to connect to a specific adapter.

I thought this was working, but after inspection, I see that the path of device I pass to establish_connection and client._backend._device_path differ. This seems to be because both freshen_device inside the retry_connector and `self._async_get_best_available_backend_and_device()´ inside core/homeassistant/components/bluetooth/wrappers.py want to change the adapter to the one with highest rssi (irrespective of what path is in the passed ble_device).

Co-authored-by: J. Nick Koston <nick@koston.org>
@dbuezas
Copy link
Copy Markdown
Contributor Author

dbuezas commented Dec 16, 2022

I added the docs, but I'm clueless regarding how to add the tests and it would take me more time than I have at hand to find figure out all the details.

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Jan 7, 2023

@dbuezas Do you still need this exposed given the local adapter connection management is now built-in?

@dbuezas
Copy link
Copy Markdown
Contributor Author

dbuezas commented Jan 8, 2023

@bdraco yes, or a way to feed back that there were communication problems. In my use case connection may succeed, but communication can still fail due to pairing issues with a specific adapter.
Regardless, your recent work is very impressive, I'm sure a lot of other non pairing integrations became even more robust! 👏

@dbuezas
Copy link
Copy Markdown
Contributor Author

dbuezas commented Jan 8, 2023

This is what happens with the device that requires pairing with pass key:
Screenshot_20230108-082518
On each scan, if the "wrong" (not paired) adapter is picked, connection succeeds, but regardless of communication retries, the update scan fails.
Because of that, I switch the adapter each time communication fails (even if connection works)

@bdraco bdraco mentioned this pull request Jan 8, 2023
19 tasks
…_address -> async_scanner_devices_by_address and wrap up the result in a dataclass
@bdraco
Copy link
Copy Markdown
Member

bdraco commented Jan 8, 2023

Since we are exposing this as a public api, I'm going to rename async_get_scanner_discovered_devices_and_advertisement_data_by_address -> async_scanner_devices_by_address and wrap up the result in a dataclass so it should be a lot easier to read

@bdraco bdraco changed the title Expose async_get_discovered_devices_and_advertisement_data_by_address… Expose async_scanner_devices_by_address from the bluetooth api Jan 8, 2023
Comment thread homeassistant/components/bluetooth/models.py
@bdraco
Copy link
Copy Markdown
Member

bdraco commented Jan 8, 2023

I added the tests for you.

I also renamed it and wrapped it up in a dataclass to make it more developer friendly since we are going to expose this in a public API.

That probably means some hasattr calls to make it work with old versions in your code though but should prevent future breakage.

@dbuezas
Copy link
Copy Markdown
Contributor Author

dbuezas commented Jan 8, 2023

That's awesome! Thank you!

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Jan 8, 2023

I'm going to get #85448 merged and than fix conflicts here.

Comment thread homeassistant/components/bluetooth/wrappers.py
@bdraco
Copy link
Copy Markdown
Member

bdraco commented Jan 8, 2023

I think this is all good to go now. I need to do some functional testing just to be sure (the tests should cover this 100% though)

@dbuezas
Copy link
Copy Markdown
Contributor Author

dbuezas commented Jan 8, 2023

Thank you for this!

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Jan 8, 2023

I think I screwed up the merge conflict here or on my integration branch

2023-01-08 23:16:12.340 ERROR (MainThread) [homeassistant.components.xiaomi_ble] C4:7C:8D:6D:77:E4: Failure while polling
Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/components/bluetooth/active_update_processor.py", line 120, in _async_poll
    update = await self._async_poll_data(self._last_service_info)
  File "/usr/src/homeassistant/homeassistant/components/bluetooth/active_update_processor.py", line 113, in _async_poll_data
    return await self._poll_method(last_service_info)
  File "/usr/src/homeassistant/homeassistant/components/xiaomi_ble/__init__.py", line 92, in _async_poll
    return await data.async_poll(connectable_device)
  File "/usr/local/lib/python3.10/site-packages/xiaomi_ble/parser.py", line 1586, in async_poll
    client = await establish_connection(
  File "/usr/local/lib/python3.10/site-packages/bleak_retry_connector/__init__.py", line 342, in establish_connection
    await client.connect(
  File "/usr/src/homeassistant/homeassistant/components/bluetooth/wrappers.py", line 235, in connect
    wrapped_backend = self._async_get_best_available_backend_and_device(manager)
  File "/usr/src/homeassistant/homeassistant/components/bluetooth/wrappers.py", line 334, in _async_get_best_available_backend_and_device
    ble_device.details,
NameError: name 'ble_device' is not defined

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Jan 8, 2023

It was just wrong on my integration branch. But I tweaked it to avoid a dict lookup

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Jan 8, 2023

Looks good

path
19_connections

@bdraco bdraco merged commit 112b2c2 into home-assistant:dev Jan 9, 2023
tronikos pushed a commit to tronikos/home-assistant-core that referenced this pull request Jan 9, 2023
…assistant#83733)

Co-authored-by: J. Nick Koston <nick@koston.org>
fixes undefined
shbatm pushed a commit to shbatm/home-assistant-core that referenced this pull request Jan 9, 2023
…assistant#83733)

Co-authored-by: J. Nick Koston <nick@koston.org>
fixes undefined
@github-actions github-actions Bot locked and limited conversation to collaborators Jan 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

No mechanism available for integrations to chose the BT adapter they want to use.

2 participants