ThomDietrich / miflora-mqtt-daemon

Linux service to collect and transfer Xiaomi Mi Flora plant sensor data via MQTT to your smart home system, with cluster support 🌱🌼🥀🏡🌳
MIT License
610 stars 140 forks source link

Improve/fix Home Assistant MQTT integration #101

Closed bangom closed 4 years ago

bangom commented 4 years ago

Could you please make small addition in your code so Home Assistant (HA) integration via internal HA MQTT Mosquitto broker works properly? The current version of miflora-mqtt-daemon allows only partial integration because of missing information in MQTT data which is sent by miflora-mqtt-daemon resulting in partial plant sensor visibility and HA GUI warnings like "This entity does not have a unique ID, therefore its settings cannot be managed from the UI"...

Goals: The main goal of this request is to have each Miflora Plant sensor visible under Mosquitto broker as a separate HA Device with its Entities/Sensors (temperature, moisture, conductivity, light intensity, battery). Result: ha-miflora02

Another goal is to be able to place miflora sensor into HA "Areas". This requires the main goal to be implemented (miflora sensor needs to be visible as Device in HA not as separate entities/sensors). Result: ha-miflora01

Explanation: According to HA MQTT Sensor Discovery (https://www.home-assistant.io/integrations/sensor.mqtt/) miflora-mqtt-daemon should also send sensor's "unique_id" along with it's "parent device" information (identifiers, connections, firmware, ... ) configuration variables. After that, the discovered MQTT sensor is added automatically into HA Device Registry (https://developers.home-assistant.io/docs/device_registry_index/) and visible in HA GUI.

Tested patch which needs to be merged:

diff --git a/miflora-mqtt-daemon.py b/miflora-mqtt-daemon.py
index 1209779..096c5d3 100755
--- a/miflora-mqtt-daemon.py
+++ b/miflora-mqtt-daemon.py
@@ -307,6 +307,15 @@ elif reporting_mode == 'homeassistant-mqtt':
             payload['name'] = "{} {}".format(flora_name, sensor.title())
             if 'device_class' in params:
                 payload['device_class'] = params['device_class']
+            payload['unique_id'] = "%s-%s" % ( flora['mac'].lower().replace(":", ""), sensor, )
+            payload['device'] = {
+                    'identifiers': [ "MiFlora{}".format( flora['mac'].lower().replace(":", "") )],
+                    'connections': [[ "mac", flora['mac'].lower() ]],
+                    'manufacturer': 'Xiaomi',
+                    'name': flora_name,
+                    'model': 'HHCCJCY01',
+                    'sw_version': flora['firmware']
+                    }
             mqtt_client.publish('{}/{}_{}/config'.format(topic_path, flora_name, sensor).lower(), json.dumps(payload), 1, True)
 elif reporting_mode == 'wirenboard-mqtt':
     print_line('Announcing Mi Flora devices to MQTT broker for auto-discovery ...')

Thank you!

PS: In case no new Miflora devices are visible in your HA after patching miflora-mqtt-daemon, try restarting HA.

ThomDietrich commented 4 years ago

Hey @bangom, thanks for this. I'll patch this in the next 1-3 days. Out of personal interest: Where does your miflora-mqtt-daemon run, on a normal Linux machine or inside docker on a raspberry pi with Hass.io?

bangom commented 4 years ago

Great, should I create git pull request or attached patch is enough?

I'm running miflora-mqtt-daemon on multiple separate raspberrypi-zero devices which work as btle gateways for nearby miflora sensors. Hass.io is on a separate central device.

I'm also thinking to enhance miflora-mqtt-daemon with passive btle readings by default (each miflora broadcasts everything except battery status multiple times per minute) which will save miflora batteries in long-term use. Active btle pooling only few times per day for battery status. Passive btle Miflora reading, see ESPhome or HA component sensor.mitemp_bt. Miflora-mqtt-daemon would be first to combine passive and active (battery status) reading.

po 23. 3. 2020 v 15:38 odesílatel Thomas Dietrich notifications@github.com napsal:

Hey @bangom https://github.com/bangom, thanks for this. I'll patch this in the next 1-3 days. Out of personal interest: Where does your miflora-mqtt-daemon run, on a normal Linux machine or inside docker on a raspberry pi with Hass.io?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ThomDietrich/miflora-mqtt-daemon/issues/101#issuecomment-602635428, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKBL3AEL4EGOP3BBEFIC7RDRI5X4ZANCNFSM4LRODVTQ .

-- Moris Bangoura

ThomDietrich commented 4 years ago

Just recently I gave Home Assistant a chance and tested the built-in miflora integration. I am not happy with it and will continue to use the miflora-mqtt-daemon. I was thinking to build a Dockerfile to provide a Hassio add-on.

Regarding your patch: I could commit it but please open a pull request and the contribution will be in your name ;)

ThomDietrich commented 4 years ago

Regarding passive reading: 1) I wasn't aware of that. Did you bring it up in the miflora library repository? Ideally we should try to contribute this kind of addition there. 2) I found that a polling rate of even 30min will not drain the battery fast enough for me to really care about the issue. Especially as the battery level itself is known and can be used to trigger a "please change battery" notification.

bangom commented 4 years ago

Just recently I gave Home Assistant a chance and tested the built-in miflora integration. I am not happy with it and will continue to use the miflora-mqtt-daemon. I was thinking to build a Dockerfile to provide a Hassio add-on.

Regarding your patch: I could commit it but please open a pull request and the contribution will be in your name ;)

I'm not saying that HA sensor.mitemp_bt is best solution. For example for me, this approach is not feasible because I need multiple miflora btle gateways running outside of hass.io device to cover large area.

But passive Miflora reading can be usable, increase coverage, improve communication and also fix some problems like inaccurate light reading because of blinking led during active btle communication (https://github.com/open-homeautomation/miflora/issues/136). I will suggest this in miflora library repo.

Of course, Dockerfile can be usable in some deployments.

Pull request finished :)

bangom commented 4 years ago

OK, merged pull works OK. Thanks!

You can also update main readme so it says, latest 3.2.1 firmware is working (also Vegtrug sensors -with MAC prefix "80:EA:CA").

Just from curiosity, why did you change MQTT topic structure for home-assistant?? Now each sensor from Miflora is advertised straight in homeassistant/sensor/ level. Also state topic moved from /homeassistant/sensor/flora_name/state to /homeassistant/sensor/flora_name.

IMHO if you have a lot of Miflora sensors, you don't want to have each sensor (lux, temp, ...) as separate topic under homeassistant/sensor/ but rather grouped under common Miflora device.

This also supports HA documentation (https://www.home-assistant.io/docs/mqtt/discovery/): Recommended MQTT HA structure is: //[/]/config, where:

ThomDietrich commented 4 years ago

Hey! Thanks for your continued comments!

I've added the firmware version info.

Just from curiosity, why did you change MQTT topic structure for home-assistant??

The code had an issue with the base topic even before your Pull Request. I believe communication is better structured when announcements appear under homeassistant/# topics but actual data under a clean e.g. miflora/# structure (it's logically not part of the home assistant domain, if you know what I mean). I fixed that and "tuned" the topics while doing so. I adopted the topic structure from Tasmota devices in my network and while I was surprised that the structure is homeassistant/sensor/devicename_Humidity/config I might have thought this is as it is intended without even looking at documentation. You are of course right, adding an optional node_id is useful and we should fix that.

<object_id> = a flora sensor (flora_name_light, flora_name_moisture, ...).

The documentation is a bit unclear here imho. I believe we can leave out the redundant flora_name. Wdyt?

Also state topic moved from /homeassistant/sensor/flora_name/state to /homeassistant/sensor/flora_name.

For me it is miflora/sensor/flora_name and I left the "state" part out as it is kind of redundant. Now I can see the this is also part of the Home Assistant discovery definition. I'll re-add it. (Even though documentation hints that this "automatic setting of state_topic is deprecated")

Did a direct commit instead of creating a PR branch. Commit: https://github.com/ThomDietrich/miflora-mqtt-daemon/commit/04e43f23feb349ce062345e6c2d17bfe6f94ddef


Btw. feel free to finalize your help here by adding miflora-mqtt-daemon under https://www.home-assistant.io/docs/mqtt/discovery/#support-by-third-party-tools

bangom commented 4 years ago

Hey! Thanks for your continued comments!

I've added the firmware version info.

Just from curiosity, why did you change MQTT topic structure for home-assistant??

The code had an issue with the base topic even before your Pull Request. I believe communication is better structured when announcements appear under homeassistant/# topics but actual data under a clean e.g. miflora/# structure (it's logically not part of the home assistant domain, if you know what I mean). I fixed that and "tuned" the topics while doing so. I adopted the topic structure from Tasmota devices in my network and while I was surprised that the structure is homeassistant/sensor/devicename_Humidity/config I might have thought this is as it is intended without even looking at documentation. You are of course right, adding an optional node_id is useful and we should fix that.

<object_id> = a flora sensor (flora_name_light, flora_name_moisture, ...).

The documentation is a bit unclear here imho. I believe we can leave out the redundant flora_name. Wdyt?

Will HA generate sensor names correctly? In your example if you have Plant01 Miflora sensor advertising via miflora-mqtt-daemon, then you will have

MQTT: homeassistant/sensor/plant01/light homeassistant/sensor/plant01/conductivity ...

From HA point of view, autogenerated name of the sensor is light, conductivity... so we will have sensors: sensor.light sensor.conductivity (not sure if good thing will happen when adding another Miflora device...)

My proposition:

MQTT: homeassistant/sensor/plant01/plant01_light homeassistant/sensor/plant01/plant01_conductivity ... HA sensor autogenerated names: sensor.plant01_light sensor.plant01_conductivity

Also state topic moved from /homeassistant/sensor/flora_name/state to /homeassistant/sensor/flora_name.

For me it is miflora/sensor/flora_name

This is good idea, will divide miflora mqtt topics configuration in the same manner. In future we can add there for example miflora-mqtt-daemon distributed configuration of "slave daemons" #35 .

and I left the "state" part out as it is kind of redundant. Now I can see the this is also part of the Home Assistant discovery definition. I'll re-add it. (Even though documentation hints that this "automatic setting of state_topic is deprecated")

Did a direct commit instead of creating a PR branch. Commit: 04e43f2

Btw. feel free to finalize your help here by adding miflora-mqtt-daemon under https://www.home-assistant.io/docs/mqtt/discovery/#support-by-third-party-tools

OK, will do that when we finalize this improvement.

ThomDietrich commented 4 years ago

Will HA generate sensor names correctly?

I believe so. Check this out: https://github.com/home-assistant/core/issues/10220

The name field from your PR is used as the entity_id. Why HA installation works with the latest version of this daemon

In feature we can add there

Believe it or not, that is why even though I removed "/state" initially, I left "/sensor/" in. For #35 I envision "miflora/config/..."

bangom commented 4 years ago

``

Will HA generate sensor names correctly?

I believe so. Check this out: home-assistant/core#10220

The name field from your PR is used as the entity_id. Why HA installation works with the latest version of this daemon

Good point, homeassistant/sensor/plant01/light is OK then. Thanks for re-checking.

In feature we can add there

Believe it or not, that is why even though I removed "/state" initially, I left "/sensor/" in. For #35 I envision "miflora/config/..."

Great, looking forward to seeing your idea of #35.