Koenkk / zigbee2mqtt

Zigbee 🐝 to MQTT bridge 🌉, get rid of your proprietary Zigbee bridges 🔨
https://www.zigbee2mqtt.io
GNU General Public License v3.0
11.57k stars 1.63k forks source link

Danfoss Ally TRV after upgrade to 1.22 Error external room sensor invalid value -8000.0 (range 0.0 - 100.0) #9972

Closed WouterKr closed 2 years ago

WouterKr commented 2 years ago

Hi,

After upgrading to 1.22 I see the following error in the log for the Danfoss Ally TRV's:

2021-12-03 06:38:48 ERROR (MainThread) [homeassistant.components.mqtt.number] Invalid value for number.danfoss_radiatorthermostat_external_measured_room_sensor: -8000.0 (range 0.0 - 100.0)

And appears every minute for all the Danfoss Ally TRV's

Also setting the external room sensor temperature is not possible anymore. For example the external room value for 21 = 2100 and is not accepted and will give the same error.

If not updated the default value is set to -8000

Debug info

Zigbee2MQTT version: 1.22.1-1 Adapter hardware: Conbee II

Regards, Wouter

sjorge commented 2 years ago

@robertalexa I think this is because -8000 is the 'undefined' value the TRV resets too right (when it doesn't get an update for X time on the value)?

Maybe we need something like this: https://github.com/Koenkk/zigbee-herdsman-converters/blob/1799f367ff90c7dbd66bfed4f5b2e91568ecc6c9/lib/exposes.js#L273

Where we set a range but also a set of 'presets', in this case the range will be min=0, max=100 and then one preset with value -8000 that is 'unset' or something?

WouterKr commented 2 years ago

The value -8000 is its initial value and is set when is does not get an update for X-time that's correct. But setting is not possible either. For instance setting the external room value to 21 -> 2100 results in the same error:

Error external room sensor invalid value 2100.0 (range 0.0 - 100.0)

Btw. the same happens with the load_room_mean parameter.

robertalexa commented 2 years ago

I have updated to latest z2m version and I am seeing the same behaviour now.

Looking at the Release log I have noticed this: image And the commit https://github.com/Koenkk/zigbee2mqtt/commit/0efd0cfe174770ad41e1733866fb51cabd6bbf62

So by the looks of it, any numeric expose without a range will not have a "fallback" so it will default to 0 and 100. So that might be impacting a lot more than these 2 particular entities?

@Koenkk Could you confirm, the commit above is yours. If not, I can try to set up a testing HA and try to have a look

robertalexa commented 2 years ago

Similar issue happening here: https://github.com/Koenkk/zigbee2mqtt/issues/9971

robertalexa commented 2 years ago

@sjorge the 0 - 100 range is the range that the entity gets in HA (MQTT Number), rather than the limits of the device, or the range in z2m. https://www.home-assistant.io/integrations/number.mqtt/

So in the case of the mentioned attribute there is no limit defined in z2m image

But the if statement in Koen's commit does not have a fallback, so the MQTT Number will default to 0 and 100 as min and max, hence what we are seeing.

I understand why Koen removed that fallback, as in that described issue OP was using value 444740 outside of -65535.0 - 65535.0 but given the above circumstances, that original issue would still not be resolved now? Is it worth us setting some ridiculous limits? Alternatively we could set limits to all number attributes of all devices in z2m, so you have granular control, but that might be impossible to do without knowledge of each device.

sjorge commented 2 years ago

I guess whatever the limit of uint8, uint16, or uint32 is based on the attribute, but those are very large numbers.

robertalexa commented 2 years ago

I guess whatever the limit of uint8, uint16, or uint32 is based on the attribute, but those are very large numbers.

Yep. The only disadvantage with that in terms of HA is that will define the actual slider limits, which will make it horrible to use (a loooot of possible values in a very small space of the slider). But yet again, my personal opinion is that the attributes that are manually used, would usually have "presets" and limits in z2m, and everything that doesn't is usually handled in automations rather than manual. So maybe it doesn't matter that much? I can't think of anything better than slapping a bit limit on it

LE: screenshots image

sjorge commented 2 years ago

Well going back and setting a correct min/max where known is probably the best course of action. But I guess for some we just don't know.

robertalexa commented 2 years ago

Alternatively we could set limits to all number attributes of all devices in z2m, so you have granular control, but that might be impossible to do without knowledge of each device.

Yep.

I was just having a look now at all the numeric attributes with a SET and ALL access, some of them maybe we can find, some of them might be challenging. Maybe we fix what we can and then deal with left overs as and when they come up, and maybe that way people that know more about those devices get involved?

I don't mind trying to do this leg work if this is the direction we decide on.

sjorge commented 2 years ago

Let's wait and see if @Koenkk has a better idea

Koenkk commented 2 years ago

Preferably the converter should be adapted to publish null instead of -8000

robertalexa commented 2 years ago

Preferably the converter should be adapted to publish null instead of -8000

@Koenkk

The 0 to 100 range is still wrong though. That does not fix the issue.

The issue will impact all the numeric exposes that do not have a z2m min and max defined, but the value can be a lot different thant the default 0 to 100 of the mqtt number

Peyoz commented 2 years ago

+1 Here, that change totally screwed my TRV and smart heating automations.

robertalexa commented 2 years ago

@Koenkk What i meant by the above is with regards to the lack of fallback if that makes sense. When no mix max are defined in z2m, then the default 0 - 100 is applied by MQTT Number at the time of discovery.

I can put in a PR that will fix this device by providing min and max (-8000 to 9999 | Not set to 99.99 Celsius) but the issue will still remain for any other number entity across all devices supported by z2m which do not have a range. This applies to the original DIY device which lead you to the above mentioned commit.

Koenkk commented 2 years ago

@robertalexa while I agree the default of 0 - 100 is not ideal, having something very big like -65535 and 65535 doesn't make sense either (since that will make the slider unusable)

robertalexa commented 2 years ago

So should we fix this on a device by device basis?

PS: the issue https://github.com/Koenkk/zigbee2mqtt/issues/9012 which you have tried to resolve with that commit will also be impacted by this issue. So that might come back again

Koenkk commented 2 years ago

So should we fix this on a device by device basis?

Yes, created a PR: https://github.com/Koenkk/zigbee-herdsman-converters/pull/3461

robertalexa commented 2 years ago

@Koenkk Can you also close this? https://github.com/Koenkk/zigbee2mqtt/issues/9971

Fixed by this as well