Add quirk for Sonoff SWV Smart Water Valve #3340

benbancroft · 2024-08-31T19:28:32Z

Proposed change

Adds support for Sonoff SWV Smart Water Valve custom cluster, which exposes a new attribute called valve_status for querying if the valve is leaking or if the water supply to the valve has stopped.

Code should hopefully should be self explanatory, the only complication is the overridden _is_manuf_specific() property in SonoffFC11Cluster class. This is required, as if the device receives an attribute read or notify zigbee command which has manufacturer_specific = True set (default if cluster's ID is within manufacturer specific range which this is), it will return UNSUPPORTED_ATTRIBUTE.

This feature was requested as part of #3298

Additional information

zha pull request for Home Assistant Sensors: Add sensors for Sonoff SWV Smart Water Valve status zha#189
Home Assistant core pull request for language strings for sensor: Add ZHA language strings for Sonoff SWV home-assistant/core#124989

Checklist

The changes are tested and work correctly
pre-commit checks pass / the code has been formatted using Black
Tests have been added to verify that the new code works

codecov · 2024-08-31T19:31:31Z

Codecov Report

Attention: Patch coverage is 69.23077% with 8 lines in your changes missing coverage. Please review.

Project coverage is 88.66%. Comparing base (fa734ad) to head (c768029).
Report is 64 commits behind head on dev.

Files with missing lines	Patch %	Lines
zhaquirks/sonoff/swv.py	69.23%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #3340      +/-   ##
==========================================
- Coverage   88.71%   88.66%   -0.06%     
==========================================
  Files         306      307       +1     
  Lines        9820     9846      +26     
==========================================
+ Hits         8712     8730      +18     
- Misses       1108     1116       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

fgsch · 2024-09-01T01:34:46Z

zhaquirks/sonoff/swv.py

+
+    Water_Shortage = 0x1
+    Water_Leakage = 0x2
+


I think you want Normal (0) and probably Leakage and Shortage (3).

So the absence of Normal (0) and Leakage and Shortage (3) was intentional - if you look at class you will see that this is a bitfield not an enum. Leakage and Shortage (3) would just be Water_Shortage (1) & Water_Leakage (2)

If you look what I've done in ZHR PR (zigpy/zha#189), I'm actually exporting two binary sensors, one for each bit. I personally think this is a lot nicer than how they did it in Zigbee2MQTT.

Ah, I see, I missed the bitmap8.
Two related comments:

The docstrings say enum :)

While this is fine for the sensor, the other part is the response when reading the attribute directly via Manage Zigbee Device interface. This is not a big issue, but it'd be nice to have the full range of values IMO.

Actually, is there any reason you cannot use an enum here and check for the specific values on the other PR?

fgsch · 2024-09-01T01:37:00Z

zhaquirks/sonoff/swv.py

+
+    cluster_id = SONOFF_CLUSTER_FC11_ID
+    ep_attribute = "sonoff_manufacturer"
+    attributes = {ATTR_SONOFF_VALVE_STATUS: ("valve_status", ValveStatusBitmap, False)}


I believe attributes are deprecated. It might be better to use ZCLAttributeDef to define this.

Thanks - I based this off the other Sonoff quirk, with no prior experience writing quirks. Let me look into ZCLAttributeDef and see how easy it is to convert over.

fgsch · 2024-09-01T01:37:48Z

Nice! I was testing a similar change using the Quirks Builder v2 and exposing the state as a sensor.

I'll probably open a PR on Monday, as I wrote most of the code already, but I've left some comments in this PR.

benbancroft · 2024-09-01T07:48:30Z

Hi @fgsch

Thanks for your review comments, much appreciated! Given you've gone to the work of writing yours, I would definetly push it up. I wasn't aware of Quirks Builder v2 (with this being my first quirk), but looking at API it looks very nice and I guess it allows you to avoid a ZHA change also? What might be a pain is exporting two not one sensors like I have (which in my personal opinion makes more sense for these sensors).

fgsch · 2024-09-02T21:09:58Z

Hi @fgsch

Thanks for your review comments, much appreciated! Given you've gone to the work of writing yours, I would definetly push it up. I wasn't aware of Quirks Builder v2 (with this being my first quirk), but looking at API it looks very nice and I guess it allows you to avoid a ZHA change also? What might be a pain is exporting two not one sensors like I have (which in my personal opinion makes more sense for these sensors).

My original plan was to expose the valve status as a sensor, but I've been struggling to do so as I envisioned.
I don't know if it's me or if it's just not possible, but unfortunately, no other zha quirks are using this.
Might be a good time to ask around.

As for my PR, I sort of ran out of time today. I will try to do it tomorrow.

fgsch · 2024-09-03T21:24:18Z

And finally, here's my PR: #3346

I managed to get the sensor working as I wanted with help from some devs in discord. Unfortunately, it is not yet possible to do the same with binary sensors.

RogerSero · 2024-10-12T21:01:16Z

Hi, does this feature work? Where can I get the full file?

benbancroft · 2024-10-13T15:09:54Z

Hi @fgsch

Apologies for the big delay in getting back to you since you posted your pull request. I was finally able to try it out locally, V2 quirk interface is certainly a big improvement.
Based on your commit, I think I've come up with a clean way of handling the binary sensors instead of enum approach. Please let me know what you think.

Looks like there are still some issues with test line coverage - need to work out how to write a test for V2 quirk interface next.

fgsch · 2024-10-15T21:19:39Z

zhaquirks/sonoff/swv.py

+    Water_Shortage = 1
+    Water_Leakage = 2


To show all the possible values if querying the cluster directly, can you also add the Normal (0) and Water leakage and shortage (3) values?

fgsch · 2024-10-15T21:26:38Z

zhaquirks/sonoff/swv.py

+                return value & ValveState.Water_Shortage
+            case self.AttributeDefs.valve_leak_state.name:
+                return value & ValveState.Water_Leakage
+        raise ValueError(f"Unknown attribute {key}")


Q: I suppose key would only be one of the supported attributes but perhaps for unknown keys this should return the value?

fgsch · 2024-10-15T21:30:21Z

I left some comments, but it looks good. I won't be able to test it until this weekend, though.

RogerSero · 2024-10-16T08:43:45Z

I'm doing something wrong, because with the quirk file, it doesn't show me the water flow. What could it be?

benbancroft · 2024-10-16T12:22:50Z

Hi @RogerSero

To clarify, this PR/quirk only adds the sensors for diagnostic of the valve (e.g. water supply or leak). I can see from your screenshot that this appears to be working. If you want to see the sensor names to appear correctly, then zigpy/zha#234 will also be required.

As it sounds like you are looking for water flow rate in particular, there is a separate PR implementing that on ZHA repository, please see here: zigpy/zha#187. I likely confused you with my screenshot above - as I had both patches locally when testing this. This implements the whole Flow cluster (with it being a Zigbee standard cluster) and therefore we don't require any special device side quirk like this patch does for the additional sensors.

benbancroft · 2024-10-16T12:24:42Z

Much appreciated @fgsch - let me also go over your comments in more detail (hopefully this week) and get back to you.

fgsch · 2024-11-03T10:17:16Z

I've tested this quickly, and it works as expected.

If you could address #3340 (comment) I think I'd be ready to go from my POV.

benbancroft mentioned this pull request Aug 31, 2024

Add sensors for Sonoff SWV Smart Water Valve status zigpy/zha#189

Open

benbancroft mentioned this pull request Aug 31, 2024

Add ZHA language strings for Sonoff SWV home-assistant/core#124989

Draft

19 tasks

benbancroft force-pushed the sonoff-swv branch from 4be7627 to 79bfbfe Compare August 31, 2024 19:40

benbancroft mentioned this pull request Aug 31, 2024

[Device Support Request] Sonoff SWV-BSP Smart Water Valve #3298

Open

fgsch reviewed Sep 1, 2024

View reviewed changes

TheJulianJES added the new quirk Adds support for a new device label Sep 16, 2024

fgsch and others added 2 commits October 13, 2024 15:24

Add Sonoff SWV quirk

830de55

Change from enum to two binary sensors

c768029

benbancroft force-pushed the sonoff-swv branch from 834e068 to c768029 Compare October 13, 2024 15:04

fgsch reviewed Oct 15, 2024

View reviewed changes

TheJulianJES added the v2 quirk Quirks using v2 API. Might add custom entities that need translation keys in HA. label Nov 24, 2024

fgsch mentioned this pull request Nov 25, 2024

Add initial Sonoff smart water valve quirk #3346

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add quirk for Sonoff SWV Smart Water Valve #3340

Add quirk for Sonoff SWV Smart Water Valve #3340

benbancroft commented Aug 31, 2024 •

edited

Loading

codecov bot commented Aug 31, 2024 •

edited

Loading

fgsch Sep 1, 2024

benbancroft Sep 1, 2024

fgsch Sep 2, 2024 •

edited

Loading

fgsch Sep 6, 2024

fgsch Sep 1, 2024 •

edited

Loading

benbancroft Sep 1, 2024

fgsch commented Sep 1, 2024

benbancroft commented Sep 1, 2024

fgsch commented Sep 2, 2024

fgsch commented Sep 3, 2024

RogerSero commented Oct 12, 2024

benbancroft commented Oct 13, 2024

fgsch Oct 15, 2024

fgsch Oct 15, 2024 •

edited

Loading

fgsch commented Oct 15, 2024

RogerSero commented Oct 16, 2024

benbancroft commented Oct 16, 2024

benbancroft commented Oct 16, 2024

fgsch commented Nov 3, 2024

Add quirk for Sonoff SWV Smart Water Valve #3340

Are you sure you want to change the base?

Add quirk for Sonoff SWV Smart Water Valve #3340

Conversation

benbancroft commented Aug 31, 2024 • edited Loading

Proposed change

Additional information

Checklist

codecov bot commented Aug 31, 2024 • edited Loading

Codecov Report

fgsch Sep 1, 2024

Choose a reason for hiding this comment

benbancroft Sep 1, 2024

Choose a reason for hiding this comment

fgsch Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

fgsch Sep 6, 2024

Choose a reason for hiding this comment

fgsch Sep 1, 2024 • edited Loading

Choose a reason for hiding this comment

benbancroft Sep 1, 2024

Choose a reason for hiding this comment

fgsch commented Sep 1, 2024

benbancroft commented Sep 1, 2024

fgsch commented Sep 2, 2024

fgsch commented Sep 3, 2024

RogerSero commented Oct 12, 2024

benbancroft commented Oct 13, 2024

fgsch Oct 15, 2024

Choose a reason for hiding this comment

fgsch Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

fgsch commented Oct 15, 2024

RogerSero commented Oct 16, 2024

benbancroft commented Oct 16, 2024

benbancroft commented Oct 16, 2024

fgsch commented Nov 3, 2024

benbancroft commented Aug 31, 2024 •

edited

Loading

codecov bot commented Aug 31, 2024 •

edited

Loading

fgsch Sep 2, 2024 •

edited

Loading

fgsch Sep 1, 2024 •

edited

Loading

fgsch Oct 15, 2024 •

edited

Loading