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

Refactor Bluetooth Tracker to async #26614

Merged
merged 13 commits into from Sep 13, 2019

Conversation

@pgilad
Copy link
Contributor

commented Sep 12, 2019

Description:

This PR refactors the Bluetooth Tracker code to async. It has much better control flow, and code is simplified. It also leaves room for creating race timeouts on some of the bluetooth methods in case they are not working correctly.

Also added locking to the update_bluetooth method to prevent parallel runs if an execution runs too long.

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the [development checklist][dev-checklist]
pgilad added 5 commits Sep 12, 2019
WIP
WIP
pgilad added 2 commits Sep 13, 2019
Dev automation moved this from Needs review to Review in progress Sep 13, 2019
pgilad added 3 commits Sep 13, 2019
@pgilad

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2019

@MartinHjelmare Many thanks for the review, I think I understood your comments about running blocking I/O in the executor thread pool. Can you review it again please?


interval = config.get(CONF_SCAN_INTERVAL, SCAN_INTERVAL)
if mac in devices_to_track:
await see_device(hass, async_see, mac, device_name)

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Sep 13, 2019

Member

We could optimize this by using asyncio.wait to await all these tasks concurrently.

tasks = []
for mac, device_name in devices:
    ...
    tasks.append(see_device(hass, async_see, mac, device_name))

if tasks:
    await asyncio.wait(tasks)

This comment has been minimized.

Copy link
@pgilad

pgilad Sep 13, 2019

Author Contributor

Nice 👏

_LOGGER.debug("Tracking new devices = %s", track_new)
_LOGGER.debug("Tracking new devices is set to %s", track_new)

devices_to_track, devices_to_not_track = await get_tracking_devices(hass)

if not devices_to_track and not track_new:
_LOGGER.debug("No Bluetooth devices to track and not tracking new devices")

if track_new:

This comment has been minimized.

Copy link
@pgilad

pgilad Sep 13, 2019

Author Contributor

@MartinHjelmare What do you think about removing this? I'm talking about this block. We call update_bluetooth immediately on bootstrap (https://github.com/home-assistant/home-assistant/pull/26614/files#diff-a1148ba3a37cfd77fa0c986c69f86b2dR1980) which is a much more complete solution

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Sep 13, 2019

Member

I guess the difference is that the bluetooth update takes longer time? But it looks like we can remove it.

This comment has been minimized.

Copy link
@pgilad

pgilad Sep 13, 2019

Author Contributor

They both run at startup one after the other, the 2nd run being a more complete discovery and update, so I would say the 1st was redundant

Copy link
Member

left a comment

Nice!

Dev automation moved this from Review in progress to Reviewer approved Sep 13, 2019
@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

Can be merged when build passes.

@pgilad

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2019

Thanks @MartinHjelmare - appreciate the great assistance!

@MartinHjelmare MartinHjelmare merged commit 2f6d567 into home-assistant:dev Sep 13, 2019
11 checks passed
11 checks passed
CI Build #20190913.36 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch Coverage not affected when comparing 32a6a76...8336a68
Details
codecov/project 93.99% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Sep 13, 2019
@pgilad pgilad deleted the pgilad:bluetooth-tracker-to-async branch Sep 13, 2019
@lock lock bot locked and limited conversation to collaborators Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
3 participants
You can’t perform that action at this time.