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

Initial xiaomi_ble integration #75618

Merged
merged 15 commits into from
Jul 22, 2022
Merged

Conversation

Jc2k
Copy link
Member

@Jc2k Jc2k commented Jul 22, 2022

Proposed change

Based on @bdraco's work on bluetooth and HA, this adds support for the Xiaomi BLE devices as found in (ble_monitor)[https://github.com/custom-components/ble_monitor].

This uses sensorpush as a template - see #75531 for the discussion around the design.

Note that the migration for python 3.10 has broken integrations that rely on bluepy. This includes MiFlora (#74585). This integration has basic support for MiFlora, so my hope is to deprecate/remove that integration too.

Screenshot 2022-07-22 at 11 27 47

@Ernst79 interest.

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.

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:

@Jc2k
Copy link
Member Author

Jc2k commented Jul 22, 2022

(Will work on brand/doc updates next; have child care duty so it might be this evening).

Co-authored-by: Ernst Klamer <e.klamer@gmail.com>
Jc2k and others added 2 commits July 22, 2022 11:47
Co-authored-by: Ernst Klamer <e.klamer@gmail.com>
Co-authored-by: Ernst Klamer <e.klamer@gmail.com>

from .const import DOMAIN

SENSOR_DESCRIPTIONS = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume the integration now picks up all sensors that send temperature, humidity, pressure and battery. This might add all sensors, as almost all of them have battery messages. Does this mean that these sensors are added with only a battery entity? Not a problem, I guess, but we can also decide to comment out the sensors that aren't fully supported yet in the pypi package.

Copy link
Member Author

Choose a reason for hiding this comment

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

Pressure isnt used yet, but yes. I've left all the parsers active and able to capture fields that aren't turned into entities so we can do a second pass and capture them.

The can ignore any devices they don't want to add in the UI, so i'd slightly prefer to leave as is I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

I’m just a bit afraid of all the issues that are going to be created, when people notice that their device only shows a battery entity. There is also one device that has added a counter in the battery data. Like every tenth advertisement, it will send a counter (as battery data) that is increased with one. I had some filter in BLE monitor for that (not in the parser, but in sensor.py. It will look it up, which one it was…)

Copy link
Member Author

Choose a reason for hiding this comment

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

Eh, I'm less worried about that. It's been that way with homekit_controller for a few years.

We can add some boilerplate to the docs about what device classes are support and/or a whitelist of devices we expect to work.

(I actually have a dashboard just for batteries, so I'm totally up for battery only sensors).

Copy link
Contributor

@Ernst79 Ernst79 Jul 22, 2022

Choose a reason for hiding this comment

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

The CGPR1 is the device with the odd battery sensor. I made a workaround in BLE monitor by storing the last 5 readings in a list, and only process the battery reading if it is in the list (only for the CGPR1, the others work fine without the workaround)

https://github.com/custom-components/ble_monitor/blob/de09d4396f2d9ce8f4d44c10c6ef90ebf6483c90/custom_components/ble_monitor/sensor.py#L259

The counter is actually counting downwards, this is what you get without the workaround.

image

Copy link
Member Author

Choose a reason for hiding this comment

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

I've moved this into Bluetooth-Devices/xiaomi-ble#3 and will handle it seperately to this PR.

Jc2k and others added 2 commits July 22, 2022 11:58
Co-authored-by: Ernst Klamer <e.klamer@gmail.com>
Co-authored-by: Ernst Klamer <e.klamer@gmail.com>
Co-authored-by: Ernst Klamer <e.klamer@gmail.com>
@MartinHjelmare
Copy link
Member

Could this integration also replace the mitemp_bt integration?

https://www.home-assistant.io/integrations/mitemp_bt/

@Jc2k
Copy link
Member Author

Jc2k commented Jul 22, 2022

I noticed that one and I think it could - but haven't checked yet.

@Ernst79
Copy link
Contributor

Ernst79 commented Jul 22, 2022

@MartinHjelmare Yes it can (and will). MiTemp integration is for the LYWSDCGQ, which is just one of supported temperature and humidity sensors (Xiaomi MiBeacon sensors). We can remove that integration as well.

@Jc2k
Copy link
Member Author

Jc2k commented Jul 22, 2022

@bdraco
Copy link
Member

bdraco commented Jul 22, 2022

This will break when I merge #75600 because its going to need a test fixture to enable bluetooth. I'll copy it in and push it here if I merge that one first

@bdraco
Copy link
Member

bdraco commented Jul 22, 2022

I pushed the fix to mock enable bluetooth that is now needed after #75600 and the test

@Jc2k
Copy link
Member Author

Jc2k commented Jul 22, 2022

Cheers for helping with that!

Copy link
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!

Copy link
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.

Thanks!

@MartinHjelmare MartinHjelmare merged commit 402e533 into home-assistant:dev Jul 22, 2022
@Jc2k Jc2k deleted the xiaomi_ble branch July 22, 2022 22:55
@github-actions github-actions bot locked and limited conversation to collaborators Jul 23, 2022
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.

5 participants