Allow parsing to happen in PassiveBluetoothProcessorCoordinator #76384

Jc2k · 2022-08-07T12:20:04Z

Breaking change

This changes the PassiveBluetoothProcessorCoordinator and PassiveBluetoothDataProcessor bluetooth API's introduced in 2022.8.

PassiveBluetoothProcessorCoordinator now takes a mandatory update_method callback that receives bluetooth advertisements (in the form of BluetoothServiceInfoBleak) and returns the data that should be handed off to any subscribed PassiveBluetoothDataProcessor.
PassiveBluetoothDataProcessor still takes an update_method, but instead of a BluetoothServiceInfoBleak, it now takes receives the data from PassiveBluetoothProcessorCoordinator's update_method

This change is to help integrations where:

a single advertisement contains data for multiple platforms and only needs parsing once
where you need to parse advertisements to know what platforms to load

Proposed change

As per discussion in #76342, slight refactor so that the bluetooth coordinator can drive the parsers. This is useful:

Where 2 platforms in an integration use the same parser (e.g. sensor and binary_sensor contained in the same advertisement)
There is non-platform logic (like encryption and triggering reauth flows) that makes sense to pull out of the platform.
Where you need to parse the advertisements to know which platforms to load

If i rebase #76342 on top of this then I can implement an ActiveBluetoothProcessorCoordinator that polls characteristics at the coordinator and makes the data available to multiple platforms the same way passsive data is handled (DataProcessor.async_update) (in the PR as it is now, I have to do this in the DataProcessor which probhibits sharing collected data between platforms).

Crucially, each platform is still responsible for its own entities - the coordinator does not know about Home Assistant entities.

There is a default update_method for the coordinator - that just passes through the BluetotohServiceInfo, so other integrations carry on working as they are for now. I intend for that to be temporary, unless people think its useful.

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

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

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:

Documentation added/updated for www.home-assistant.io

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.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

probot-home-assistant · 2022-08-07T12:20:10Z

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!
_{^{(message by CodeOwnersMention)}}

probot-home-assistant · 2022-08-07T12:20:13Z

Hey there @bdraco, mind taking a look at this pull request as it has been labeled with an integration (sensorpush) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

probot-home-assistant · 2022-08-07T12:20:13Z

Hey there @bdraco, mind taking a look at this pull request as it has been labeled with an integration (moat) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

probot-home-assistant · 2022-08-07T12:20:13Z

Hey there @bdraco, mind taking a look at this pull request as it has been labeled with an integration (inkbird) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

probot-home-assistant · 2022-08-07T12:20:14Z

Hey there @bdraco, mind taking a look at this pull request as it has been labeled with an integration (govee_ble) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

probot-home-assistant · 2022-08-07T12:20:14Z

Hey there @Ernst79, mind taking a look at this pull request as it has been labeled with an integration (xiaomi_ble) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

bdraco · 2022-08-07T20:02:44Z

homeassistant/components/bluetooth/passive_update_processor.py

@@ -52,7 +52,9 @@ class PassiveBluetoothDataUpdate(Generic[_T]):
    )


-class PassiveBluetoothProcessorCoordinator(BasePassiveBluetoothCoordinator):
+class PassiveBluetoothProcessorCoordinator(


Could you subclass the coordinator and overload async_handle_bluetooth_event instead?

Hmm, I think this behaviour makes sense as the default, which is why I didn't do it in a subclass. If you disagree I am happy to do it in a subclass instead. But i'd be submitting an ActiveBluetoothProcessorCoordinator seperately for xiaomi, and I think this behaviour especially makes sense for any passive sensor that has binary sensor and sensor payloads in the same broadcast, and any passive sensor that has dynamic platforms. (So we'd have quite a few coordinator flavours).

I think its ok to change it, but we need to label it a breaking change since its shipped this way in 2022.8

Ah ok my bad - didn't think of API stability.

Actually, if i revert c8e0421 it won't be a breaking change.

Jc2k · 2022-08-07T20:43:38Z

TODO: Decide whether to revert 7c48dd5. If we do, this isn't a breaking change. Though we should probably slap a deprecation warning on it?

bdraco · 2022-08-07T20:45:33Z

Its new enough that I would remove it now. Easier to do it take the pain now then when 50 integrations are using this

bdraco · 2022-08-07T20:46:10Z

We will need a breaking change section to tell devs what to do, probably actually better as a blog post on developers.home-assistant.io

Jc2k · 2022-08-07T20:54:08Z

👍 - will pick that up in the morning. long day and i don't think i can write anything coherent right now 😆 I've added a rambling Breaking section in the meantime.

Jc2k · 2022-08-08T07:14:05Z

First draft of blog post: home-assistant/developers.home-assistant#1427

Jc2k · 2022-08-08T08:39:49Z

Last commit fixed missing test coverage, testing failure and recovery of the new method.

Will now start reworking the ActiveDataProcessor from my other PR to work with this instead.

bdraco · 2022-08-08T23:00:01Z

All working as expected. Its definitely makes it a lot more flexible, and less code in the integration as well 👍

homeassistant added the cla-signed label Aug 7, 2022

probot-home-assistant bot added code-quality has-tests integration: bluetooth integration: govee_ble integration: inkbird integration: moat integration: sensorpush integration: xiaomi_ble labels Aug 7, 2022

project-bot bot added this to Needs review in Dev Aug 7, 2022

probot-home-assistant bot assigned bdraco Aug 7, 2022

probot-home-assistant bot added the by-code-owner label Aug 7, 2022

bdraco reviewed Aug 7, 2022

View reviewed changes

bdraco added the breaking-change label Aug 7, 2022

Jc2k force-pushed the bluetooth_parse_in_coordinator branch from c8e0421 to 7ad474c Compare August 7, 2022 20:42

Jc2k changed the title ~~WIP: Allow parsing to happen in PassiveBluetoothProcessorCoordinator~~ Allow parsing to happen in PassiveBluetoothProcessorCoordinator Aug 7, 2022

Jc2k marked this pull request as ready for review August 7, 2022 20:42

Jc2k mentioned this pull request Aug 7, 2022

Support polling the MiFlora battery #76342

Merged

23 tasks

Jc2k added a commit to Jc2k/developers.home-assistant that referenced this pull request Aug 8, 2022

Blog post about home-assistant/core#76384

63c06b5

Jc2k mentioned this pull request Aug 8, 2022

Blog on PassiveBluetoothProcessorCoordinator home-assistant/developers.home-assistant#1427

Merged

5 tasks

Jc2k added 11 commits August 8, 2022 08:23

Allow parsing to happen in PassiveBluetoothProcessorCoordinator

cb3d9d6

Lint fix

4df7775

Workaround typing issue

32ab7f7

Update govee_ble

0469c3a

Update inkbird

c3a82d7

Update SensorPush

0a6441c

Update moat

48e65a8

Remove default

c4b4a1c

Update docs

0d8e4e6

Error logging for new update method

ee0ed00

Add coverage for coordinator update_method failing and then recovering

efd21c2

Jc2k force-pushed the bluetooth_parse_in_coordinator branch from 7ad474c to efd21c2 Compare August 8, 2022 07:23

bdraco self-requested a review August 8, 2022 21:18

bdraco approved these changes Aug 8, 2022

View reviewed changes

Dev automation moved this from Needs review to Reviewer approved Aug 8, 2022

Jc2k merged commit 7d427dd into home-assistant:dev Aug 9, 2022

Dev automation moved this from Reviewer approved to Done Aug 9, 2022

Jc2k deleted the bluetooth_parse_in_coordinator branch August 9, 2022 05:36

github-actions bot locked and limited conversation to collaborators Aug 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow parsing to happen in PassiveBluetoothProcessorCoordinator #76384

Allow parsing to happen in PassiveBluetoothProcessorCoordinator #76384

Jc2k commented Aug 7, 2022 •

edited

probot-home-assistant bot commented Aug 7, 2022

probot-home-assistant bot commented Aug 7, 2022

probot-home-assistant bot commented Aug 7, 2022

probot-home-assistant bot commented Aug 7, 2022

probot-home-assistant bot commented Aug 7, 2022

probot-home-assistant bot commented Aug 7, 2022

bdraco Aug 7, 2022

Jc2k Aug 7, 2022 •

edited

bdraco Aug 7, 2022

Jc2k Aug 7, 2022

Jc2k Aug 7, 2022

Jc2k commented Aug 7, 2022 •

edited

bdraco commented Aug 7, 2022

bdraco commented Aug 7, 2022

Jc2k commented Aug 7, 2022

Jc2k commented Aug 8, 2022

Jc2k commented Aug 8, 2022

bdraco commented Aug 8, 2022

Allow parsing to happen in PassiveBluetoothProcessorCoordinator #76384

Allow parsing to happen in PassiveBluetoothProcessorCoordinator #76384

Conversation

Jc2k commented Aug 7, 2022 • edited

Breaking change

Proposed change

Type of change

Additional information

Checklist

probot-home-assistant bot commented Aug 7, 2022

probot-home-assistant bot commented Aug 7, 2022

probot-home-assistant bot commented Aug 7, 2022

probot-home-assistant bot commented Aug 7, 2022

probot-home-assistant bot commented Aug 7, 2022

probot-home-assistant bot commented Aug 7, 2022

bdraco Aug 7, 2022

Choose a reason for hiding this comment

Jc2k Aug 7, 2022 • edited

Choose a reason for hiding this comment

bdraco Aug 7, 2022

Choose a reason for hiding this comment

Jc2k Aug 7, 2022

Choose a reason for hiding this comment

Jc2k Aug 7, 2022

Choose a reason for hiding this comment

Jc2k commented Aug 7, 2022 • edited

bdraco commented Aug 7, 2022

bdraco commented Aug 7, 2022

Jc2k commented Aug 7, 2022

Jc2k commented Aug 8, 2022

Jc2k commented Aug 8, 2022

bdraco commented Aug 8, 2022

Jc2k commented Aug 7, 2022 •

edited

Jc2k Aug 7, 2022 •

edited

Jc2k commented Aug 7, 2022 •

edited