Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add initial Sonoff smart water valve quirk #3346

Merged
merged 8 commits into from
Nov 26, 2024
Merged

Conversation

fgsch
Copy link
Contributor

@fgsch fgsch commented Sep 3, 2024

Proposed change

Add quirk for Sonoff SWV.

Additional information

This exposes the water_valve_state attribute in the custom cluster and adds a sensor to HA with the same information.
EDIT: The enum sensor will not be added with this initial PR. We'd like two get two different binary sensors to parse the same attribute. Since quirks v2 can't do this yet, we'll need to do this work in a future PR.
This initial PR will just expose the attribute.

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

Copy link

codecov bot commented Sep 3, 2024

Codecov Report

Attention: Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.67%. Comparing base (213ce10) to head (0438e95).
Report is 3 commits behind head on dev.

Files with missing lines Patch % Lines
zhaquirks/sonoff/swv.py 94.11% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##              dev    #3346   +/-   ##
=======================================
  Coverage   89.67%   89.67%           
=======================================
  Files         316      317    +1     
  Lines       10281    10298   +17     
=======================================
+ Hits         9219     9235   +16     
- Misses       1062     1063    +1     

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

@fgsch fgsch marked this pull request as ready for review September 3, 2024 21:21
@TheJulianJES TheJulianJES changed the title Add Sonoff SWV quirk Add Sonoff smart water valve quirk Sep 16, 2024
@TheJulianJES TheJulianJES added new quirk Adds support for a new device v2 quirk Quirks using v2 API. Might add custom entities that need translation keys in HA. labels Sep 16, 2024
@TheJulianJES TheJulianJES added the needs review This PR should be reviewed soon, as it generally looks good. label Sep 24, 2024
Comment on lines +12 to +15
Normal = 0
Water_Shortage = 1
Water_Leakage = 2
Water_Shortage_And_Leakage = 3
Copy link
Collaborator

@TheJulianJES TheJulianJES Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would two separate binary sensors make more sense for this? (instead of an enum sensor)
We can't really parse it this via v2 quirks (yet), so just wondering.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would vote for 2 binary sensors. This would make it more convenient for triggers and alerts.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zigpy/zha#305 and zigpy/zha#303 would be required for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheJulianJES what about the approach in #3340 to have 2 binary sensors?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would work, but we're creating "fake" attributes there, which isn't optimal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheJulianJES While the work is being decided, can we get this without the enum so at least people can fiddle with the attributes directly using e.g. zha toolkit?

Or you reckon zigpy/zha#305 and zigpy/zha#303 would be handled relatively soon?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's temporarily remove the quirks v2 enum sensor then. We should be able to get the quirk in the HA Core 2024.12 beta for tomorrow then.
Not sure on the timeline of the "attribute converters" yet. I hope we can get to it soon, but no promises.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thank you!

@TheJulianJES TheJulianJES removed the needs review This PR should be reviewed soon, as it generally looks good. label Nov 26, 2024
@TheJulianJES TheJulianJES changed the title Add Sonoff smart water valve quirk Add initial Sonoff smart water valve quirk Nov 26, 2024
Copy link
Collaborator

@TheJulianJES TheJulianJES left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!
I've removed the quirks v2 enum sensor for now. We should be able to get the quirk in the HA Core 2024.12 beta for tomorrow then.

Hopefully, we get to add the "attribute converters" for quirks v2 relatively soon, so we can also add the binary sensor entities for HA.

@TheJulianJES TheJulianJES added the smash This PR is close to be merged soon label Nov 26, 2024
@TheJulianJES TheJulianJES merged commit a5a00d9 into zigpy:dev Nov 26, 2024
9 checks passed
@fgsch fgsch deleted the add-swv branch November 27, 2024 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new quirk Adds support for a new device smash This PR is close to be merged soon v2 quirk Quirks using v2 API. Might add custom entities that need translation keys in HA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants