Skip to content

Add Sonoff water valve leak and supply sensors #3910

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

Merged
merged 4 commits into from
Mar 26, 2025

Conversation

fgsch
Copy link
Contributor

@fgsch fgsch commented Feb 28, 2025

Proposed change

Add sensors for water leaks and supply problems.
While here rename EwelinkCluster to match other Sonoff quirks.

Additional information

Follow-up from #3346.
Supersedes zigpy/zha#189.

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 Feb 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.11%. Comparing base (62765a4) to head (0df73ea).
Report is 9 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #3910      +/-   ##
==========================================
+ Coverage   91.00%   91.11%   +0.10%     
==========================================
  Files         328      333       +5     
  Lines       10656    10774     +118     
==========================================
+ Hits         9698     9817     +119     
+ Misses        958      957       -1     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@fgsch
Copy link
Contributor Author

fgsch commented Feb 28, 2025

I will move this to open later today when I can test it, but FYI @TheJulianJES .

@fgsch fgsch force-pushed the fgsch/swv-add-binary-sensors branch 3 times, most recently from a977aa1 to 223d325 Compare February 28, 2025 13:11
@fgsch fgsch changed the title Add binary sensor for water leakage and shortage Add sensors for water leaks and supply problems Feb 28, 2025
@fgsch fgsch force-pushed the fgsch/swv-add-binary-sensors branch 2 times, most recently from ce3a711 to a0bf713 Compare February 28, 2025 14:25
@fgsch
Copy link
Contributor Author

fgsch commented Feb 28, 2025

Question: Is there any way to define reporting for the water_valve_state attribute and not at the sensor level?
Or does that not work and need to be defined for each separately?

@fgsch fgsch marked this pull request as ready for review February 28, 2025 23:29
@fgsch fgsch marked this pull request as draft February 28, 2025 23:37
@fgsch fgsch force-pushed the fgsch/swv-add-binary-sensors branch from a0bf713 to 8ff2349 Compare March 1, 2025 00:00
@fgsch
Copy link
Contributor Author

fgsch commented Mar 16, 2025

@TheJulianJES Can you let me know the answer to my question above?
Also, do you know why the sensors come as PROBLEM instead of their name on the UI?

Screenshot 2025-03-16 at 21 49 06

@TheJulianJES TheJulianJES added the needs reviewer answer An answer from a reviewer is needed (e.g. why a PR isn't acceptable in the current state). label Mar 16, 2025
@TheJulianJES
Copy link
Collaborator

That is expected if the sensor has a state class due to the way fallback_name is implemented in HA.
It's only an issue when the translation key isn't present in the translations. It will work fine when we PR the changes to Home Assistant. See this for more information: zigpy/zigpy#1523 (comment)

@fgsch
Copy link
Contributor Author

fgsch commented Mar 16, 2025

That is expected if the sensor has a state class due to the way fallback_name is implemented in HA. It's only an issue when the translation key isn't present in the translations. It will work fine when we PR the changes to Home Assistant. See this for more information: zigpy/zigpy#1523 (comment)

Thanks for the information. I thought that might be related but wasn't sure.

Regarding:

Question: Is there any way to define reporting for the water_valve_state attribute and not at the sensor level?
Or does that not work and need to be defined for each separately?

Do you happen to know the answer? I'd like to finish this PR.
Thank you again.

@fgsch fgsch force-pushed the fgsch/swv-add-binary-sensors branch from 8ff2349 to 184d579 Compare March 16, 2025 23:46
@TheJulianJES
Copy link
Collaborator

Only possible to do directly with the sensor function at the moment.

@fgsch
Copy link
Contributor Author

fgsch commented Mar 17, 2025

Only possible to do directly with the sensor function at the moment.

Thanks. I will update this and open It for review.

@fgsch fgsch force-pushed the fgsch/swv-add-binary-sensors branch from 184d579 to 69ea0a6 Compare March 17, 2025 23:34
@fgsch fgsch force-pushed the fgsch/swv-add-binary-sensors branch from 69ea0a6 to 6017e1f Compare March 17, 2025 23:34
@fgsch fgsch marked this pull request as ready for review March 18, 2025 23:21
@fgsch
Copy link
Contributor Author

fgsch commented Mar 18, 2025

Ok, I think this is ready for review now. PTAL.

@fgsch
Copy link
Contributor Author

fgsch commented Mar 18, 2025

@TheJulianJES,

That is expected if the sensor has a state class due to the way fallback_name is implemented in HA. It's only an issue when the translation key isn't present in the translations. It will work fine when we PR the changes to Home Assistant. See this for more information: zigpy/zigpy#1523 (comment)

Does this apply to the entity ID as well? I can see both have binary_sensor.garden_tap_valve_problem.

@abmantis
Copy link
Contributor

@TheJulianJES,

That is expected if the sensor has a state class due to the way fallback_name is implemented in HA. It's only an issue when the translation key isn't present in the translations. It will work fine when we PR the changes to Home Assistant. See this for more information: zigpy/zigpy#1523 (comment)

Does this apply to the entity ID as well? I can see both have binary_sensor.garden_tap_valve_problem.

Yes. The entity will be generated from the name, which will be the fallback_name you specified when it gets integrated in HA.

@TheJulianJES TheJulianJES changed the title Add sensors for water leaks and supply problems Add Sonoff water valve leak and supply sensors Mar 22, 2025
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!

@TheJulianJES TheJulianJES added the smash This PR is close to be merged soon label Mar 22, 2025
reporting_config=ReportingConfig(
min_interval=30, max_interval=900, reportable_change=1
),
translation_key="water_leak",
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we didn't have a translation key for this, the name would be "Moisture" due to the device class.
I think we can leave the "Water leak" name here though, since it's a bit more explicit for this device.

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 again!

@TheJulianJES TheJulianJES merged commit 775de19 into zigpy:dev Mar 26, 2025
9 checks passed
@fgsch fgsch deleted the fgsch/swv-add-binary-sensors branch March 26, 2025 01:20
@fgsch
Copy link
Contributor Author

fgsch commented Mar 26, 2025

Thank you for updating and merging this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs reviewer answer An answer from a reviewer is needed (e.g. why a PR isn't acceptable in the current state). smash This PR is close to be merged soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants