alexander-vitishchenko / hc3-to-mqtt

Fibaro HC3 integration with Home Assistant & NodeRed
MIT License
31 stars 13 forks source link

Battery sensor created multiple times #35

Closed MomosX closed 1 year ago

MomosX commented 1 year ago

Hi again :)

I noticed a bug and i can not find a way to solve it. We have battery devices like motion sensors, flood, smoke and so on. They all have a master device which gets discovered in HA and sub devices - entities.

Problem is that for each sub device , like temperature, light sensor and so on , a new battery sensor is created in HA.

image

Why this matters ? Because there are many extra entities that do not have any purpose, it should be only one battery sensor per master device and most important because it messes up the naming once again:

For a motion sensor there is: binary_sensor.pirfibarodining_living - which is the motion sensor sensor.pirfibarodining_living - which is the first battery sensor sensor.lightsensordining_living_2 - which is the actual light sensor sensor.lightsensordining_living - which is the second battery sensor.

So what happened is that the light sensor got the index 2 while the battery sensor got the naming of the lux sensor. So automations do not work anymore. I can not find any way to convince it to give the naming wihtout index to the actual light sensor.

Can this be solved somehow so we have only one battery device ?

thank you !!!

alexander-vitishchenko commented 1 year ago

Hi @MomosX,

Understood.

Just to provide you with more context - this is how Fibaro HC3 is designed. Battery status is assigned to parent and child devices.

Agree and it makes sense to keep one physical battery status when working with Home Assistant.

The problem is that Fibaro considers battery status is an attribute for the existing devices, and Home Assistant considers battery status a separate sensor. This is why it is not a problem for Fibaro, but might introduce user experience issue for Home Assistant users.

I can consider implemented some sort of algorithm for battery status duplicate identification, and omit unnecessary device generation for Home Assistant, which should work for most cases.

On the other hand - Home Assistant allows to hide redundant sensors with device editor UI. Did you try using it?

MomosX commented 1 year ago

I can live with having multiple battery sensors for the same device. But the main problem is the numbering.

In the example above sensor.lightsensordining_living used to be the lux sensor. After a stop/start of the QA and i think also a reboot of HC3 , that name was assigend to the battery sensor and sensor.lightsensordining_living_2 became the lux sensor.

Nothing i did could change them back. Would be great if the baterry sensor received a name like sensor.lightsensordining_living_battery and the lux sensor retained its name.

alexander-vitishchenko commented 1 year ago

Hi @MomosX,

Just finished implementing automated CI/CD, basic regression test suite and versioning, e.g. 1.0.188.

Going back to your question - the thing is that QuickApp does already generate unique numbering, but it doesn't feel used by Home Assistant in a way I would expect.

1) The current QuickApp implementation assigns unique ids for each entity:

As you can see your Home Assistant entity ids use totally different template than defined by the QuickApp.

2) It looks your sensor ids are generated by Home Assistant using entity "friendly name" property... which is weird because entity name can change frequently, while id to remain the same... not depend on "friendly name". I need to check Home Assistant MQTT specification why this is happening in such a way. There should be a good reason for this, and I also need to check if I can control such behaviour, and make Home Assistant use the ids supplied by the QuickApp.

Please let me know if I understood your question correctly?

MomosX commented 1 year ago

Yes you did :) . For as long as i can remember using your QA , HA used the device name from HC3, not any other identifier. The device name was taken from the master device. It used to be numbers like 1020 or 1030.1, but as i could not identify those devices in HA i changed the default names in HC3 with something meaningfull.

image

image

image

alexander-vitishchenko commented 1 year ago
  1. I'm thinking about possible solutions, and one of them would require you to use the new version of the QuickApp (later when it is done) + MQTT data cleanup... would it be fine for you, or maybe it is easier to adjust your automations to use the different id, than doing MQTT cleanup potentially affecting ids for other devices when they are reloaded by Home Assisstant?

  2. By the way - I still struggle to understand how could it be that you primary sensor get "2" suffix.

    TL;TR

    Home Assistant was supposed to assign "2" suffix to the battery level sensor, because your primary sensor already existed.

    Can you please advise if you did any MQTT data clean up before? Or perhaps you did anything else that trigger the main sensor to be removed, and then re-added by the QuickApp again?

    The thing is that the newer versions of the QuickApp do not seem capable for changing the existing Home Assistant entity ids... which make me think you removed your old entities first, then used the new Quick App Version and then Home Assistant reload devices using new names.

    Is this correct assumption?

alexander-vitishchenko commented 1 year ago

Hi @MomosX,

Just checked your comment at #33, and yes you did MQTT cleanup that explains the situation.

I keep thinking on a solution design. Just fyi why this is not a trivial thing:

MomosX commented 1 year ago

I am doing MQTT cleanups as this is the only way to get things going again if something breaks.
Isn't it possible to just append a "_batt" to the sensors having battery property ? They will still be 2 or more for each master device but at least they won't "steal" the naming

MomosX commented 1 year ago

I don't think anyone else would be affected as until 2 weeks ago the battery stuff was not even working

and i'm not talking about influencing HA entity generation, but adding something to those devices in the QA.

No idea how those properties get from QA to entities in HA. Does the QA publish them as separate sensors ?

alexander-vitishchenko commented 1 year ago

Hi @MomosX

1) It is definitely possible to add "_batt"-like suffix to battery/power/energy device names to influence Home Assistant with generating unique entity ids (even it is not part of their MQTT specification).

I understand this is important for you, and I'm already looking for a solution, but trying it to be better (if possible).

If there are no better solution design identified - then I most likely need to stick the the suffix approach.

2) This is just to provide more context why "_batt"-like suffix might not be the ideal solution:

Perhaps I raise a feature/bugfix request for Home Assistant MQTT component, so they use "unique_id" property instead of "device name" for entity id generation in Home Assistant. This looks cleaner to me architecture and more reliable.

Does it help to show the bigger picture, or perhaps I misunderstood anything accidentally?

MomosX commented 1 year ago

It sure shows the bigger picture :) So what happens if i understood correctly is that we have a device in HC3 with the the name "test" for example. that device has some properties - battery and so on. The MQTT integrations puts the main device and the devices generated by properties in the same category - sensor. But then it has multiple sensor entities with the same name so it has to index the names to make them unique. Correct ? Is there an order of generation or is that random ?

alexander-vitishchenko commented 1 year ago

Generally yes.

I guess I understand what you may offer - make the main sensor broadcasted first, then battery/power/energy. So your automation can rely on a fixed main sensor id without suffixes? :-)

This could be a workaround and I'll explore where the message order gets broken.

However I'll keep looking for a better design (with Home Assistant Team) due to few reasons 1) The workaround above should mostly work for battery powered sensors, but leaving the possibility to get the same issues for other type of devices that are not powered by battery, but name the same and put to a single room. And we want to fix the issue for all type of situations, right? :-) 2) Proper solution design should not depend on the message order. Because MQTT itself doesn't guarantee the order => the desired solutions must be resilient to messages delivered in a different order.

Hope this helps.

MomosX commented 1 year ago

"I guess I understand what you may offer - make the main sensor broadcasted first, then battery/power/energy. So your automation can rely on a fixed main sensor id without suffixes? :-)" YES :) took me a while to explain

And fully agree that a more reliable solution is needed but i don't think that can happen without the help of the HA team. Hopefully they will be opened as this is potential trouble with other integrations also.

alexander-vitishchenko commented 1 year ago

Hi @MomosX

Good news - I have checked Home Assistant MQTT component sources and rechecked their MQTT specification - apparently there is an "object_id" attribute that we can use to generate unique entity ids.

So the issue will be resolved soon.

MomosX commented 1 year ago

Ehhh that is indeed good news:) that will solve all the issues with naming and with reloads , restarts oq QA or HC3. No more MQTT clearing :)

alexander-vitishchenko commented 1 year ago

Hi @MomosX - feel free to use the latest 1.0.191 version, and let me know if it works for you?

MomosX commented 1 year ago

"object_id" ? will test tomorrow and let you know.

alexander-vitishchenko commented 1 year ago

Yes

MomosX commented 1 year ago

So,

Deleted the old QA - for some reason i could not copy/paste the files - and installed the new QA. All my devices in HA are gone, about 150 of them.

But looking at entities i like very much what i saw there ! All are finally properly named / recognizable. This is the way they should have been from the beginning :)

So i'll gladly do all the modifications in my HA - again - to all dashboards - quite a lot of work but it is worth it.

There is no way their names/id's can be messed up now.

I'll do the modifications tonight and keep you posted about how it works, but i expect it to be rock solid :)

alexander-vitishchenko commented 1 year ago

Hi @MomosX,

Can you please advise what do you mean by not being able to copy/paste lua files? They are available at GitHub along with the fqa file.

Updating Home Assistant dashboard and automations with new entity ids is exactly what I was meaning earlier, when telling about existing QuickApp users' experience to be impacted :-). At the same time - this should be one-time-effort, while QuickApp stability improvement is long-term.

alexander-vitishchenko commented 1 year ago

Please let me know if you have the issue resolved, so I can close this ticket?

MomosX commented 1 year ago

Let me test it through multiple reboots of HC3, HA, MQTT clearing first pls. I'm still updating all my HA dashboards... they look like this now:

image

I'll finish tonight the update of all entities and proceed to testing :)

alexander-vitishchenko commented 1 year ago

This is exactly what expected after all the entity ids are new (& stable) now

MomosX commented 1 year ago

"Can you please advise what do you mean by not being able to copy/paste lua files? They are available at GitHub along with the fqa file" - copy raw data from github but in the QA edit there is no paste anymore; just cot and copy.

I think it's because it goes over the maximum lines in the QA. have to delete everything in the file, save, then can paste. Nothing to do with the QA :) QA so far is super!!!

alexander-vitishchenko commented 1 year ago

Waiting for your testing results, and I'll add a warning message to README file and QuickApp logs for the existing users so they are aware about the change

MomosX commented 1 year ago

I think we can close this as everything seems to be ok. Reboots, restarts, clearing MQTT, the etities come back always where they are supposed to and with the proper naming:) Great work Alexander and thank you again.

Users will have to do a few modifications now but it's quite easy as you only have to search in HA for the device ID from HC3. Can't go wrong. PS: as a side note , the QA version 1.0.191 was missing the variables for HC3 user/pass.

alexander-vitishchenko commented 1 year ago

Hi @MomosX

There are no missing HC3 user/pass - it is a feature. QuickApp is now able to authorise itself, without making users add those two variables :-)

Quick App is getting better every iteration, I'm not event talking about new CI/CD environment with automated regression testing for better quality of the releases, like for professional commercial projects, except I don't get paid for it but I shouldn't probably complain much about it)))