Koenkk / zigbee-herdsman-converters

Collection of device converters to be used with zigbee-herdsman
MIT License
858 stars 2.84k forks source link

Use the same battery calculation as ZHA in Tuya TS0203 door sensors #7681

Open deviantintegral opened 1 week ago

deviantintegral commented 1 week ago

This PR improves battery readings for TS0203 sensors, or at least matches ZHA which will allow for greater collaboration and hopefully fewer support threads.

I recently purchased a load tester for batteries, and have been using that to check devices which seemed to have unreliable battery reporting.

TS0203 door sensors:

I haven't truly run one of these batteries out yet though, so I don't know for sure what the lower voltage end is.

ZigBee Cluster Library Frame, Command: Report Attributes, Seq: 215
    Frame Control Field: Profile-wide (0x18)
    Sequence Number: 215
    Command: Report Attributes (0x0a)
    Attribute Field
        Attribute: Battery Voltage (0x0020)
        Data Type: 8-Bit Unsigned Integer (0x20)
        Measured Battery Voltage: 2.5 [V]

ZigBee Cluster Library Frame, Command: Report Attributes, Seq: 219
    Frame Control Field: Profile-wide (0x18)
    Sequence Number: 219
    Command: Report Attributes (0x0a)
    Attribute Field
        Attribute: Battery Percentage Remaining (0x0021)
        Data Type: 8-Bit Unsigned Integer (0x20)
        Remaining Battery Percentage: 1.0 [%]

This has previously been reported at https://github.com/Koenkk/zigbee2mqtt/discussions/17337.

Here's the corresponding code in ZHA, which doesn't have any device specific overrides so applies to these sensors:

https://github.com/zigpy/zha-device-handlers/blob/c6ed94a52a469e72b32ece2a92d528060c7fd034/zhaquirks/__init__.py#L195-L228

I traced the voltage numbers to the PR, which doesn't describe why they were changed:

https://github.com/zigpy/zha-device-handlers/pull/208

Likewise, the issue and commits in zigbee2mqtt don't describe how it's voltage readings were found:

https://github.com/Koenkk/zigbee2mqtt/issues/4466

There's also this discussion with no resolution: https://github.com/Koenkk/zigbee2mqtt/discussions/17337

With all this in mind, I think:

  1. The current battery reporting is unreliable at best. I don't think it's providing helpful information to end users.
  2. The door sensors themselves certainly have broken percentage reporting. I think they may also have incorrect voltage reporting and are under-reporting voltage.
  3. Given the challenges in the space, it makes more sense for us to match what ZHA does so users aren't surprised with vastly different values when switching.

So, here's a PR that does that!

There is a greater risk of devices going from 2500 mV to zero with no warning. Today, the issue is that users may replace perfectly fine batteries prematurely. I'm hoping to track this myself and see if there's a voltage drop I can rely on to tweak this in the future, but that may take many months or even years!

deviantintegral commented 1 week ago

It looks like this change causes some bounciness in battery readings. I'm not sure why, but one of my sensors (_TZ3000_oxslv1c9) just reported with only the battery percentage and not the battery voltage. That caused the percent to go back to 1%, ignoring our own calculations:

ZigBee Cluster Library Frame, Command: Report Attributes, Seq: 226
    Frame Control Field: Profile-wide (0x18)
    Sequence Number: 226
    Command: Report Attributes (0x0a)
    Attribute Field
        Attribute: Battery Percentage Remaining (0x0021)
        Data Type: 8-Bit Unsigned Integer (0x20)
        Remaining Battery Percentage: 1.0 [%]

Other sensors seem to report them at the same time.

Should we remove the battery percent reporting from these devices in the configure function? Or, simply say "this device reports inaccurate battery info, it's not our problem" in the docs? I suppose we could start with just ignoring the models that I have on hand similar to the exposes function.

Koenkk commented 1 week ago

I propose to ignore the battery % by adding a fromZigbee converter which only processes the voltage.

deviantintegral commented 1 week ago

I started down the path of adding a new converter, but I don't think it makes for a clear design.

The problem is that we still need a way to define what battery curve to use - and there's no way to easily pass additional parameters into the converter - except for meta, which we already have:

    batteryUsingOnlyVoltage: {
        cluster: 'genPowerCfg',
        type: ['attributeReport', 'readResponse'],
        convert: (model, msg, publish, options, meta) => {
            const payload: KeyValueAny = {};
            if (msg.data.hasOwnProperty('batteryVoltage') && (msg.data['batteryVoltage'] < 255)) {
                // Deprecated: voltage is = mV now but should be V
                payload.voltage = msg.data['batteryVoltage'] * 100;

                if (model.meta && model.meta.battery && model.meta.battery.voltageToPercentage) {
                    payload.battery = batteryVoltageToPercentage(payload.voltage, model.meta.battery.voltageToPercentage);
                }
            }

            return payload;
        },
    } satisfies Fz.Converter,

I think adding a model.meta.battery.useVoltageOnly property is clearer to those adding devices and be simpler in the converter.

deviantintegral commented 1 week ago

I ended up doing the converter method, because batteryVoltageToPercentage() will error if the battery voltage curve isn't set.