pvvx / ZigbeeTLc

Custom firmware for Zigbee 3.0 IoT devices on the TLSR825x chip
Other
364 stars 20 forks source link

[BUG] little bug in zigbeetlc_v0121.js #106

Closed Bodengriller closed 2 months ago

Bodengriller commented 3 months ago

[EDIT] The bug is limited to the 4 parameters: comfortTemperatureMin, comfortTemperatureMax, comfortHumidityMin, comfortHumidityMax have a look at the third comment And can so far only be confirmed for the following model: LYWSD03MMC-z


Thank you that the V0.1.2.1 is now compatible for z2m without an external converter. But I think that there is still a small bug in the converter file image

I think there's only a wrong value for "scale" i.e.:

    comfortTemperatureMin: numeric({
            name: 'comfort_temperature_min',
            unit: '°C',
            cluster: 'hvacUserInterfaceCfg',
            attribute: {ID: 0x0102, type: herdsman.Zcl.DataType.int16},
            valueMin: -50.0,
            valueMax: 120.0,
            valueStep: 0.01,
            scale: 10,
            description: 'Comfort parameters/Temperature minimum, in 0.01°C steps, default 20.00°C.',
        }),

I think the following adjustments to the parameters valueMin, valueMax, valueStep, scale would solve the problem

    tempCalibration: numeric({
            name: 'temperature_calibration',
            unit: '°C',
            cluster: 'hvacUserInterfaceCfg',
            attribute: {ID: 0x0100, type: herdsman.Zcl.DataType.int16},
            valueMin: -50.00,
            valueMax: 50.00,
            valueStep: 0.01,
            scale: 100,
            description: 'Temperature calibration, in 0.01° steps, default 0%.',
        }),
    humidityCalibration: numeric({
            name: 'humidity_calibration',
            unit: '%',
            cluster: 'hvacUserInterfaceCfg',
            attribute: {ID: 0x0101, type: herdsman.Zcl.DataType.int16},
            valueMin: -50.00,
            valueMax: 50.00,
            valueStep: 0.01,
            scale: 100,
            description: 'Humidity calibration, in 0.01% steps, default 0%.',
        }),
    comfortTemperatureMin: numeric({
            name: 'comfort_temperature_min',
            unit: '°C',
            cluster: 'hvacUserInterfaceCfg',
            attribute: {ID: 0x0102, type: herdsman.Zcl.DataType.int16},
            valueMin: -50.00,
            valueMax: 120.00,
            valueStep: 0.01,
            scale: 100,
            description: 'Comfort parameters/Temperature minimum, in 0.01°C steps, default 20.00°C.',
        }),
    comfortTemperatureMax: numeric({
            name: 'comfort_temperature_max',
            unit: '°C',
            cluster: 'hvacUserInterfaceCfg',
            attribute: {ID: 0x0103, type: herdsman.Zcl.DataType.int16},
            valueMin: -50.00,
            valueMax: 120.00,
            valueStep: 0.01,
            scale: 100,
            description: 'Comfort parameters/Temperature maximum, in 0.01°C steps, default 25.00°C.',
        }),
    comfortHumidityMin: numeric({
            name: 'comfort_humidity_min',
            unit: '%',
            cluster: 'hvacUserInterfaceCfg',
            attribute: {ID: 0x0104, type: herdsman.Zcl.DataType.uint16},
            valueMin: 0.00,
            valueMax: 100.00,
            valueStep: 1,
            scale: 100,
            description: 'Comfort parameters/Humidity minimum, in 1% steps, default 40.00%',
        }),
    comfortHumidityMax: numeric({
            name: 'comfort_humidity_max',
            unit: '%',
            cluster: 'hvacUserInterfaceCfg',
            attribute: {ID: 0x0105, type: herdsman.Zcl.DataType.uint16},
            valueMin: 0.00,
            valueMax: 100.00,
            valueStep: 1,
            scale: 100,
            description: 'Comfort parameters/Humidity maximum, in 1% steps, default 60.00%.',
    }),
robvanoostenrijk commented 3 months ago

I've tested your suggested change. I remember looked into this before and indeed the results are interesting.

Test scenario: CGDK2 thermostat with 26.4 °C room temperature.

In Z2M setting a 5 °C degree offset for tempCalibration with a scale of 100, results in 76 °C on the display. Similarly but with scale 10, results in 31.4 °C temperature.

Are your results on a display thermostat different?

Note, I'm not using the external convertor but the device support coming with Zigbee2MQTT. Remove the ZigbeeTLC.js external convertor because it should no longer be needed.

image

Unable to reproduce your screenshot. Maybe its CGDK2 firmware specific difference?

Bodengriller commented 3 months ago

hm okay, that's strange. I have the model LYWSD03MMC-z

To be honest, I wasn't able to test my suggested change. When I try to load the linked external converter file, it leads to other errors - I can only pair the device cleanly with V0.1.2.1 if I don't specify an external converter at all. That's why I didn't pursue this any further and honestly just guessed into the blue with the suggested change above :D

Note, I'm not using the external convertor but the device support coming with Zigbee2MQTT.

dito - I assumed that the external converter posted here was passed on to the official support / is a copy of what the "Official Converter" is

what I've tested is: comfortTemperatureMin (default: 20.00°C - shown as 200°C in z2m) comfortTemperatureMax (default 25.00°C - shown as 250°C in z2m) comfortHumidityMin (default 40.00% - shown as 400% in z2m) comfortHumidityMax (default 60.00% - shown as 600% in z2m)

These values are probably correct in the end device, but incorrectly displayed in z2m.

Example: We currently have 21.8°C here. If I set comfortTemperatureMin in Z2M to the value 230 -> the smiley becomes sad. (so it has taken over 23°C) If I set the value to 23, the smiley stays happy (accepted value must be 2.3°C)

I tested the same with humidity.

The release notes state that the temp/hum offsets and comfort parameters have been adjusted, so I assumed it would affect the offset in the same way

I have now tested the whole thing again with the official default converter and can confirm that the offsets for Temp and Hum are not affected.:

image

this is strang...

The bug is therefore limited to the 4 parameters: comfortTemperatureMin, comfortTemperatureMax, comfortHumidityMin, comfortHumidityMax

Bodengriller commented 3 months ago

Just a quick search

https://github.com/pvvx/ZigbeeTLc/blob/master/src/sensors_shtc3_4x.c

                measured_data.temp = ((s32)(17500*_temp) >> 16) - 4500 + g_zcl_thermostatUICfgAttrs.temp_offset * 10;; // x 0.01 C
                if (sensor_i2c_addr == (SHTC3_I2C_ADDR << 1))
                    measured_data.humi = ((u32)(10000*_humi) >> 16) + g_zcl_thermostatUICfgAttrs.humi_offset * 10; // x 0.01 %
                 else
                    measured_data.humi = ((u32)(12500*_humi) >> 16) - 600 + g_zcl_thermostatUICfgAttrs.humi_offset * 10; // x 0.01 %
                if (measured_data.humi < 0) measured_data.humi = 0;
                else if (measured_data.humi > 9999) measured_data.humi = 9999;

I think that the Firmware simply multiplies the offset value internally by 10, but does not do this with the comfort thresholds. That would explain the behavior.

As I said, it's just a tiny bug that doesn't cause any major problems - I just wanted to report it :) Now the question is whether it is easier to adapt the converter or the firmware

robvanoostenrijk commented 3 months ago

@Bodengriller,

Thanks for looking into the difference. I don't have a device with the comfort parameters, so I wasn't able to test that scenario. But given the code excerpt you linked above it all makes sense.

Here is an updated test convertor. Drop it it in zigbee2mqtt/node_modules/zigbee-herdsman-converters/devices, overriding the existing zigbeetlc.js and restart Zigbee2MQTT.

If you confirm this solves the issue, I'll raise a change request against zigbee-herdsman-convertors upstream repository.

zigbeetlc.txt

Bodengriller commented 3 months ago

@robvanoostenrijk

when I restart my Z2M container in HomeAssistant, the new created file will be deleted and the old one is there again :D

robvanoostenrijk commented 3 months ago

I guess it runs npm ci on boot-up to reinstall all modules. Only thing you can do is test the change on an external converter or manually restart Z2M inside the container.

Bodengriller commented 3 months ago

hmm... nope...sorry, I cannot test that. I changed the zigbeetlx.js and did a soft-restart in Z2M Interface. I completeley deleted one of my devices and paired it again -> no changes, stoll 200 instead of 20

can I simply use your file as an external converter?

robvanoostenrijk commented 3 months ago

zigbeetlc-ext.txt

You can use the updated attached file as external convertor for testing. If the test is successful, I can adapt it to Z2M default device as well.

Bodengriller commented 3 months ago

thank you!

hm.. this external converter seems to have another problem - I cannot send new values to the devices: Error 2024-07-07 17:09:19z2m: Publish 'set' 'comfort_temperature_max' to '0xa4c138a130e2c2f3' failed: 'Error: ZCL command 0xa4c138a130e2c2f3/1 hvacUserInterfaceCfg.write({"259":{"value":2500}}, {"timeout":10000,"disableResponse":false,"disableRecovery":false,"disableDefaultResponse":true,"direction":0,"srcEndpoint":null,"reservedBits":0,"manufacturerCode":null,"transactionSequenceNumber":null,"writeUndiv":false}) failed (Cannot write USE_DATA_TYPE without dataType option specified)'

but the other values look good: image

(even the comfort-smiley is working between 20-25°C and 40-60% humi

robvanoostenrijk commented 3 months ago

I believe the above is enough to confirm the change required. We know that the values of comfort * options are incorrectly scaled. And the firmware code you referenced supports the behaviour seen.

Pull-request against zigbee-herdsman-converters raised: https://github.com/Koenkk/zigbee-herdsman-converters/pull/7748

slingel commented 3 months ago

On the ts0201 with default converter I am getting these:

ts0201

Bodengriller commented 3 months ago

@slingel I had the same - remove the device completely from Z2M and repair it again, solved this issue for all of my three devices.

pvvx commented 2 months ago

The offset format error is only associated with thermometers with STHv3 or SHT4x sensors. Fixed in version 0.1.2.2.

robvanoostenrijk commented 2 months ago

As @pvvx just mentioned its sensor specific and a firmware update is now available, I've closed the upstream pull request. Converter is meant for both firmwares and would otherwise not work.

robvanoostenrijk commented 2 months ago

Raised an updated pull request https://github.com/Koenkk/zigbee-herdsman-converters/pull/7753, to align with the changes @pvvx has made for version 0.1.2.2

robvanoostenrijk commented 2 months ago

@Bodengriller PR was merged upstream. Feel free to close this issue.