Handle incorrect values reported by some Shelly devices

[mkrasowski]

Proposed change

This Pull Request fixes a problem with incorrect values sometimes reported by some Shelly devices.
The most impactful instance of this problem is Shelly Door/Window2 reporting door opening and closing within 1-2 seconds when the devices incorrectly submits dwIsOpened parameter with value -1.
Other fixed sensors: battery reported as -1%, luminosity reported as -1, temperature reported as -1.

Type of change

• Dependency upgrade
• Bugfix (non-breaking change which fixes an issue)
• New integration (thank you!)
• New feature (which adds functionality to an existing integration)
• Breaking change (fix/feature causing existing functionality to break)
• Code quality improvements to existing code or addition of tests

Additional information

I couldn't find any related open GitHub issues, but there is a thread on Shelly's Official Support Forum: https://www.shelly-support.eu/forum/index.php?thread/10839-shelly-door-window-2-state-changes/ (disclosure: I'm a participant as "mk-usa").

Looks like Alterco Robotics (the manufacturer) is aware of the problem and is working on a firmware update to correct this behavior upstream, but regardless of that, I think Home Assistant still has to handle these values appropriately, especially that they are listed in the CoIoT/COAP specification for Shelly DW2 device (https://shelly-api-docs.shelly.cloud/gen1/#shelly-door-window-1-2-coiot).

Checklist

• The code change is tested and works locally.
• Local tests pass. Your PR cannot be merged unless tests pass
• There is no commented out code in this PR.
• I have followed the development checklist
• The code has been formatted using Black (black --fast homeassistant tests)
• Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

• Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

• The manifest file has all fields filled out correctly.
  Updated and included derived files by running: python3 -m script.hassfest.
• New or updated dependencies have been added to requirements_all.txt.
  Updated by running python3 -m script.gen_requirements_all.
• For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
• Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

• No score or internal
• 🥈 Silver
• 🥇 Gold
• 🏆 Platinum

To help with the load of incoming pull requests:

• I have reviewed two other open pull requests in this repository.

[homeassistant]

Hi @mkrasowski,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

[probot-home-assistant]

Hey there @balloob, @bieniu, @thecode, @chemelli74, mind taking a look at this pull request as it has been labeled with an integration (shelly) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

[bieniu]

LGTM 👍

[thecode]

Looking at the documentation I only find -1 for the vibration sensor (which is missing in this PR) and in this case -1 means the sensor is disabled so we should use the removal_condition and not the available property.
Can you point to where you found the description in the Shelly API docs?

[bieniu]

@thecode Look at the DW2 CoIoT description file:

        {
            "I":3108,
            "T":"S",
            "D":"dwIsOpened",
            "R":[
                "0/1",
                "-1"
            ],
            "L":1
        }, 
        {
            "I":3106,
            "T":"L",
            "D":"luminosity",
            "U":"lux",
            "R":[
                "U32",
                "-1"
            ],
            "L":1
        }, 
        {
            "I":3111,
            "T":"B",
            "D":"battery",
            "R":[
                "0/100",
                "-1"
            ],
            "L":2
        },

You're right, also vibration uses -1 value. And tilt.

[thecode]

I have asked Shelly developers for the correct behavior of -1 for the DW2 sensors, will update when they reply.

[mkrasowski]

Thanks for your comments.

I haven't included tilt and vibration in the PR since they were out of scope of my testing. I don't have any spare DW2 sensors to test tilt (and all my sensors are already fixed to doors that don't tilt), but I can probably enable and take a closer look at vibration if this is what you're asking for.

As @bieniu mentioned, according to the specification -1 is among allowed values for multiple sensors - dwIsOpened, tilt, vibration, luminosity, battery. Seemingly corresponding value for extTemp sensor is 999, which is already used in HA Shelly integration to determine sensor's availability - https://github.com/home-assistant/core/blob/dev/homeassistant/components/shelly/sensor.py#L164.

I'm somewhat reluctant to use -1 values in removal_condition, because based on the observed behavior -1 gets reported not only when the sensor is disabled (which seems to be the case for tilt and vibration), but also intermittently when the device misbehaves.

I don't know if you have access to all the content in the linked Shelly Support Forum post, so I'm posting my findings also below:

Using the mechanism Home Assistant uses for listening for status updates from Shelly devices, I found out that when there are these open+close state changes in HA, the Shelly DW2 sensor sends a strange update (or multiple) to the COAP server with dwIsOpened parameter set to -1 followed up by another message with a correct value (0/1).

This seems to be always immediately before a normal wakeup event (like "sensor", "temperature", or "button").

Example one - this happened automatically, looks like due to temperature sensor state change.

Example two - this happened when I pressed the button on the device.

[thecode]

The -1 was actually due to a broken CoAP message which should be fixed in the next FW release, note that even if you filter it, the sensor behavior will be wrong, since it will cycle through an unavailable state which is wrong (since the sensor was not unavailable at this point).

Current RC release already fixes that, as for the CoAP description, it only shows that -1 is a valid value (which might also be outdated) but does not explain what -1 means.
I can agree that for a door state -1 might mean that there is an error, but not really sure what it would mean for the battery sensor.
Since I approached Shelly dev for the explanation for that let's wait for their reply.

P.S you can easily test vibration and tilt, no need to do anything, just disable them on the web interface and on latest RC version you will see that they are reported as -1.

[mkrasowski]

You're right that cycling through an unavailable state might be incorrect, but since -1 is an allowed value for the door sensor, it's still much better than cycling through an open state despite no opening happened, which could result for example in falsely triggering home alarms.

Even though technically the device is not unavailable, practically the sensor is unavailable since it's reporting invalid/unreliable values.

I'm not sure what would be a better way of handling -1 values for non-disableable sensors (like dwIsOpened or battery. Perhaps disregard the entire message?

I'm curios what Shelly devs say. Please keep us updated. Thanks.

[thecode]

Reply from Shelly is that the following sensors may generate -1 in case of error (although should never happen): dwIsOpened, battery, luminosity.

For vibration and tilt a value of -1 means that the sensor is disabled, so should be under removal_condition
If you want you can add them to this PR otherwise we can continue with this PR and make another one.

Thanks

[mkrasowski]

Thanks!

I'd say -1 values for vibration and tilt fall outside of the scope of this PR, since they are not invalid/incorrect. They seem to be valid values that have a special meaning (sensor disabled).

Though, I think we still need to be careful with removing them right after receiving -1, because in case of an error similar to the one that prompted this PR, -1 will also get reported for tilt and vibration.

I'd say for this PR, let's merge it and open a new issue for tilt and vibration and discuss the best way to handle -1 for them.