GledholtHall / beta-3

H E update
2 stars 3 forks source link

HA discovery topic overwritten for each device capability #2

Closed dgz closed 3 years ago

dgz commented 3 years ago

I think I finally understand the missing capabilities Home Assistant is reporting for my discovered color/white bulbs and sensors. With the "Everything" capability broken for anything I've tried adding I need to add my devices to several capabilities further down the list. Example: Xiaomi Temperature/Humidity sensor Hubitat info:

Current States
battery : 85
batteryLastReplaced : Oct 08 2020
driver : v1.0.1.1123
humidity : 66.2
lastCheckin : 2021-01-25 23:06:28
notPresentCounter : 0
presence : present
pressure : 99.30
restoredCounter : 0
temperature : 33.1

Homie topic is published correctly with all selected capabilities (battery, humidity, temperature): image

However the HA discovery topic in the sensor domain is overwritten three times, once for each capability. Each time clobbers the one before it so only the last capability is discovered by HA (in this case, humidity is the last one and temperature and battery are lost): image

What I think should have happened is multiple entities created under the 514 topic instead of one generic "outside-sensor". There should have been "outside-sensor-battery", "outside-sensor-humidity", and "outside-sensor-temperature". The code originally had this set correctly on line 2762 in the HADiscoveryAdvertise function: normName = normalize(name+nameSuffix) but it is currently commented out and line 2763 is used instead normalize(name) which causes the clobbering. I reversed this so 2762 is used and my sensors now have all their capabilities in HA under a single device: image image

This fixed both the temp/humidity sensors and my zigbee RGBW bulbs now include the color picker and color temperature entities where before they only picked up the on/off and dim capabilities. This line was probably commented out for a reason, what did I now break by "fixing" it?

GledholtHall commented 3 years ago

Let me take a look - it certainly looks likely.

I had previously thought that that all the config json info for one 'device' had to be in one single payload and I had thought that the json was being appended to with each write (update) rather than clobbering it which is obviously wrong. But it was a long time back so I can't remember tbh why I changed it. Hopefully your alteration seems to works which makes it far easier.

dgz commented 3 years ago

I think you append the homie $properties by building the string with all detected capabilities at once but the HADiscoveryAdvertise looks like it runs once for each capability so there's no opportunity to build a single large config json. Their MQTT discovery documentation mentions putting multiple values from a sensor under different config topics. They only differ by a single letter in the example provided but that does save having to try and build one giant config json.

Home Assistant looks like it uses the info in the Device section of the payload (name and identifier at least) to group the individual entities together under a single device. It ties in with their device registry which at least for this is really helpful for automatically grouping related entities.

GledholtHall commented 3 years ago

I think I was investigating what was happening here as above that I have a log message that says that the suffix was removed, and indeed in my logs (only) I see that. I think the reversion as you say is fine. I have done the change back and I'll see what happens

GledholtHall commented 3 years ago

please test latest version if you get a chance - may become release 3e. I think 'everything' dropdown works now and initial install

dgz commented 3 years ago

I completely wiped my setup and started from scratch:

"Everything" is still throwing an error with one temperature/humidity sensor added: java.lang.NullPointerException: Cannot execute null+1 on line 5024 (updated) line 5024 is: } else counter = counter +1 counter only appears on line 49 as a global variable declaration but it's commented out, not sure what that counter was supposed to do.

I removed the sensor from "Everything" and added in a couple RGBW bulbs instead. These add successfully but suffer the discovery json config overwrite that sensors originally did and in the case of a light the code changes to publish under separate topics have no effect. I removed the bulbs from Everything and added them to Color Control, Color Temperature, Dimmer, and Light and got the exact same behavior. Then I removed them from Dimmer and it magically started working and I think that's related. "Everything" finds a dim capability and double-publishes the discovery topic. Once as a light with brightness/color/onoff and again as a dimmer/onoff, the second one survives since it's published last and the color control capability is lost.

So in summary,

xAPPO commented 3 years ago

Good feedback, thanks.

‘Everything’ is a difficult one and I can see the dimmer overwriting the RGB in HA discovery if that’s the order the attributes get returned. It will be awkward to fix and I don’t think effects the MQTT publishing. Even if handled as a special case there will be other devices that have problems. Tempted to say just use individual attributes.

RGBW I’ll look at separately and the counter++

many thanks for this feedback - I’ll post a new version later today.

dgz commented 3 years ago

I actually prefer sorting devices into their specific capabilities because then I know exactly what functionality is being published. I'm pretty sure there is precedent for this method in apps like HubConnect and even going back to my SmartThings days I think WebCore required you to authorize devices by capability. For example, my Xiaomi sensors with markus's driver have presence capability intended to detect them dropping off the network but I don't need to publish that.

One possible solution to the RGBW situation is with a few generic device categories at the top of the list before the specific capabilities. It'd be kind of like the "Everything" feature but narrowed down to specific use cases for common device types. For example one called RGB/RGBW Bulbs that if you select a one there, it tags it with color control, color temperature and light but none of the others to avoid the conflicts.

GledholtHall commented 3 years ago

OK - I have patched around the non initialised counter variable.

The everything dropdown (all individuals removed) for a Hue originated RGBW light for me does not add any HA discovery message at all and in the homie mqtt topic I am not seeing any attribute except refresh even though I do see color-temperature, color-name, color-mode,dim,onoff,refresh in the devices $properties. However pretty sure this is because I am not recovering the current/initial attribute values within 'everything'. Once the bulb changes state the respective changed topics appear but still no HA discovery message is sent. See below

I can mimic the behaviour you describe when adding attributes by individual capabilities and 'dimmer' (switchLevel) overwrites the HA discovery message removing the color info as you say. Interestingly the previous message contains the correct dimmer info. So it might work if I can just drop that last update.

image

There seem to be a few things wrong in the code here...

GledholtHall commented 3 years ago

OK - I added a new option in the recent versions that allow HADiscovery (and homieDiscovery) to be skipped on startup instead of running every time - to speed startup. It now can be run manually if disabled.

What some of my issues are, and why they differ to yours, is that skipping HADiscovery causes some 'related' areas not to initialise / function. So I need to go through and double check what those are.

GledholtHall commented 3 years ago

I have posted a pre 7 version that I hope fixes this I still overwrite the HA Discovery advertise but the complete version is the last - could you check if originally advertising this as a monochrome dimmer and then later as color causes any difficulty in the final outcome i.e. as HA sees the device ? Please let me know

GledholtHall commented 3 years ago

Haven't heard back on this - is it fixed for you ? If so I'll close this issue.