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

Improve bluetooth tracker device code #26067

Merged
merged 10 commits into from Sep 12, 2019

Conversation

@pgilad
Copy link
Contributor

commented Aug 19, 2019

Description:

Improve flow of bluetooth tracker code. This includes:

  • Load the bt_proximity module only if required
  • Extract inline functions
  • Add some typing
  • Convert devices arrays to sets
  • Introduce helper functions
  • Add several logs to help with debugging

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 2 commits Aug 19, 2019
@homeassistant

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

Hi @pgilad,

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!

pgilad added 2 commits Aug 19, 2019
@MartinHjelmare MartinHjelmare moved this from Needs review to Review in progress in Dev Aug 31, 2019
@Mariusthvdb

This comment was marked as off-topic.

Copy link
Contributor

commented Aug 31, 2019

not sure if this is the right place, but please let me point you to an issue I have with a brand new setup, started on 97.2 now on 98.1, not being able to find bluetooth at all. Maybe some check could be entered in the config flow and issue some kind of warning, when

device_tracker:
  - platform: bluetooth_tracker
    new_device_defaults:
      track_new_devices: true

is configured correctly in the configuration.yaml, and no device is setup despite of that.

https://community.home-assistant.io/t/new-97-2-setup-bluetooth-device-tracker-does-nothing/133921

https://community.home-assistant.io/t/how-to-check-if-bluetooth-hardware-on-pi-is-alright/134126

@pgilad

This comment was marked as off-topic.

Copy link
Contributor Author

commented Aug 31, 2019

@Mariusthvdb I have been having some difficulties with scanning new devices on rpi3 as well, but it looks like a kernel issue on the rpi3 that should/was fixed (raspberrypi/firmware#1150)

@Mariusthvdb

This comment was marked as off-topic.

Copy link
Contributor

commented Sep 1, 2019

@pgilad the thing is I can’t see proof of the correct configuration/setup of the device_tracker in the first place... on all other config details we see proof of that in the logs.
So I fear it is not a matter of the tracker not being able to find devices to track , but a matter of the device_tracket itself not being setup correctly.

This is on a Pi which had it working perfectly before...

@MartinHjelmare

This comment was marked as off-topic.

Copy link
Member

commented Sep 1, 2019

Please use the forums for enhancement requests.

@Mariusthvdb

This comment was marked as off-topic.

Copy link
Contributor

commented Sep 1, 2019

Please use the forums for enhancement requests.

sure, but I am not asking for a feature request really, merely asking if it is correct there is no confirmation in the startup log for setting up the bluetooth_tracker.
Only generic mentions of:

2019-08-30 07:22:06 INFO (MainThread) [homeassistant.setup] Setting up device_tracker
2019-08-30 07:22:07 INFO (MainThread) [homeassistant.components.device_tracker] Setting up device_tracker.legacy
2019-08-30 07:22:28 INFO (MainThread) [homeassistant.setup] Setup of domain device_tracker took 22.2 seconds.

and nothing else pertaining to bluetooth_tracker in the setup.

after that, is see these:

2019-09-01 12:12:49 ERROR (MainThread) [homeassistant.components.device_tracker] The see service is not supported for this entity device_tracker.telefoon

repeatedly
please confirm this is correct?

@MartinHjelmare

This comment was marked as off-topic.

Copy link
Member

commented Sep 1, 2019

Please don't use PRs for unrelated questions.

There's currently no specific log message per each platform setup for the legacy device tracker platforms.

pgilad added 3 commits Sep 1, 2019
…into improve-bluetooth-tracker

* 'dev' of https://github.com/home-assistant/home-assistant: (229 commits)
  New template sensor attributes (#26127)
  Add BeeWi SmartClim BLE sensors (#26174)
  UniFi - Simplify getting controller from config entry (#26335)
  Inverted rflink cover (#26038)
  Upgrade tibber library (#26332)
  Migrate Axis, deCONZ and UniFi to use config entry subclass (#26173)
  Upgrade sqlalchemy to 1.3.8 (#26331)
  Change evohome to asyncio client (#26042)
  Add support for Supla switches (#26188)
  Add a keypress service for AlarmDecoder (#26100)
  Fix google_maps scan interval (#26328)
  Upgrade youtube_dl to 2019.09.01 (#26330)
  Fix onvif camera setup error (#24585)
  UniFi - use entity registry disabled_by to control available entities  (#26141)
  Fetch Onkyo current radio preset (#26211)
  Add precision argument to the Range Filter (#25874)
  Fix alexa bad temp sensors (#26307)
  deCONZ - Dont update entry if data is equal
  Add Withings support (#25154)
  Add support for Homekit accessory battery sensors (#26210)
  ...
Dev automation moved this from Review in progress to Reviewer approved Sep 1, 2019
pgilad added 2 commits Sep 4, 2019
* dev: (87 commits)
  Add atome sensor platform (#26197)
  Replaces IOError by OSError (#26428)
  Entity registry doesn't overwrite with None (#24275)
  Add device to mqtt camera (#26238)
  Updated frontend to 20190904.0 (#26421)
  Add prettier to vscode (#26417)
  NSW Rural Fire Service icon for geolocation entities (#26416)
  Update azure-pipelines-translation.yml for Azure Pipelines
  [ci skip] Translation update
  Update azure-pipelines-translation.yml for Azure Pipelines
  Update azure-pipelines-translation.yml for Azure Pipelines
  Update azure-pipelines-translation.yml for Azure Pipelines
  Update azure-pipelines-translation.yml for Azure Pipelines
  Bumped version to 0.98.3
  Bump ISY994's PyISY dependency to 1.1.2 (#26413)
  Fix state report (#26406)
  Update to 0.1.13 (#26402)
  Met, check for existing location (#26400)
  Allow core config updated (#26398)
  Fix race during initial Sonos group construction (#26371)
  ...
@pgilad

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

Sorry had to manually resolve the conflict with dev branch, but it was a simple fix 😸

@pgilad

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2019

🏓 Hey - what's the next step to merge this code?

@pgilad

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2019

Hey @MartinHjelmare anything blocking this from getting merged? Something I can do?

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

We're good.

@MartinHjelmare MartinHjelmare merged commit 25ef4a1 into home-assistant:dev Sep 12, 2019
11 checks passed
11 checks passed
CI Build #20190904.47 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 6acfede...8863244
Details
codecov/project 93.98% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Sep 12, 2019
@pgilad pgilad deleted the pgilad:improve-bluetooth-tracker branch Sep 12, 2019
@lock lock bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
5 participants
You can’t perform that action at this time.