Skip to content

Refactor Bluetooth Tracker to async#26614

Merged
MartinHjelmare merged 13 commits into
home-assistant:devfrom
pgilad:bluetooth-tracker-to-async
Sep 13, 2019
Merged

Refactor Bluetooth Tracker to async#26614
MartinHjelmare merged 13 commits into
home-assistant:devfrom
pgilad:bluetooth-tracker-to-async

Conversation

@pgilad
Copy link
Copy Markdown
Contributor

@pgilad pgilad 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]

Comment thread homeassistant/components/bluetooth_tracker/device_tracker.py Outdated
Comment thread homeassistant/components/bluetooth_tracker/device_tracker.py Outdated
Comment thread homeassistant/components/bluetooth_tracker/device_tracker.py Outdated
@pgilad
Copy link
Copy Markdown
Contributor Author

pgilad 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?

Comment thread homeassistant/components/bluetooth_tracker/device_tracker.py Outdated

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

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nice 👏

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

if track_new:
Copy link
Copy Markdown
Contributor Author

@pgilad pgilad Sep 13, 2019

Choose a reason for hiding this comment

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

@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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Nice!

@MartinHjelmare
Copy link
Copy Markdown
Member

Can be merged when build passes.

@pgilad
Copy link
Copy Markdown
Contributor Author

pgilad commented Sep 13, 2019

Thanks @MartinHjelmare - appreciate the great assistance!

@MartinHjelmare MartinHjelmare merged commit 2f6d567 into home-assistant:dev Sep 13, 2019
@pgilad pgilad deleted the bluetooth-tracker-to-async branch September 13, 2019 19:14
@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

None yet

Development

Successfully merging this pull request may close these issues.

3 participants