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

Clean up Danfoss TRV custom attributes definitions and commands #2150

Merged
merged 93 commits into from
Apr 15, 2024

Conversation

Caius-Bonus
Copy link
Contributor

@Caius-Bonus Caius-Bonus commented Jan 29, 2023

This PR refactors and improves the quirk to make it possible to add binary_sensors, sensors, switches, numbers and selects in the ZHA integration.

Main changes:
1. It splits up the custom clusters into proprietary clusters with ids. This makes it so that ZHA doesn't need to know which models and vendors are supported. It only needs to know the cluster names.

Miscellaneous:

  1. It fixes up the proprietary attribute access and types.
  2. It renames some of the badly named proprietary attributes.
    This might break some users using ZHA clusters to address the TRV
  3. It adds a preheat_command.
  4. It also adds the Popp eT093WRG and Hive TRV003.
  5. It maps System Mode Off to change setpoint to 5 C
  6. It fixes Danfoss not requesting the time at connection by writing it on bind.
  • 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.
  • The code has been formatted using Black

In ZHA I added this complementary PR: home-assistant/core#86907

This PR will have an effect on the following issues:
#2115 (I don't know what could cause this, but it might change)
#1726 (It will now have this)
#717 (It will now have this)
#2728 (It will now have this)

sources (I appreciate the documentation and updates):

  1. The initial version is based on the following documentation: https://web.archive.org/web/20231215170505/https://assets.danfoss.com/documents/202524/AU417130778872en-000101.pdf (old revision), https://assets.danfoss.com/documents/367874/AU417130778872en-000102.pdf (new revision)

  2. Other documentation used is: https://assets.danfoss.com/documents/284715/AM375549618098en-000103.pdf

  3. Source for information regarding the new revision in the firmware package: https://www.danfoss.com/nl-nl/products/dhs/smart-heating/smart-heating/danfoss-ally/danfoss-ally-support/#tab-software

This documentation contradicts information regarding programming_operation_mode. Because the 2nd documentation seems more precise and supports both the old and the new G-serial TRVs, this version will be used

…unsupported ones. split up proprietary clusters. add preheat_command (no usage yet). force read before bind of cluster. Add 2 thermostat models. Refactoring
@coveralls
Copy link

coveralls commented Jan 29, 2023

Pull Request Test Coverage Report for Build 4093705668

  • 46 of 88 (52.27%) changed or added relevant lines in 2 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.3%) to 83.38%

Changes Missing Coverage Covered Lines Changed/Added Lines %
zhaquirks/danfoss/thermostat.py 44 86 51.16%
Files with Coverage Reduction New Missed Lines %
zhaquirks/danfoss/thermostat.py 2 57.94%
Totals Coverage Status
Change from base Build 4093658845: -0.3%
Covered Lines: 6878
Relevant Lines: 8249

💛 - Coveralls

@codecov-commenter
Copy link

codecov-commenter commented Jan 29, 2023

Codecov Report

Attention: Patch coverage is 98.01325% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 88.10%. Comparing base (12dd80d) to head (0aa4acf).

Files Patch % Lines
zhaquirks/danfoss/thermostat.py 98.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #2150      +/-   ##
==========================================
+ Coverage   87.91%   88.10%   +0.19%     
==========================================
  Files         300      299       -1     
  Lines        9226     9358     +132     
==========================================
+ Hits         8111     8245     +134     
+ Misses       1115     1113       -2     

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

@Caius-Bonus Caius-Bonus marked this pull request as draft February 1, 2023 12:29
…_scheduled, cluster_revision. Redirect reads from custom clusters. fix _update_attribute. Place SimpleDescriptor back. Move reads at bind to ZHA.
@Caius-Bonus Caius-Bonus marked this pull request as ready for review February 4, 2023 22:09
@Caius-Bonus
Copy link
Contributor Author

Caius-Bonus commented Feb 4, 2023

I fixed the merge-conflicts caused by the automatic checks.

@TheJulianJES TheJulianJES added enhancement Improve an existing quirk breaking change PR is a breaking change labels Apr 11, 2023
@Caius-Bonus Caius-Bonus marked this pull request as draft August 12, 2023 01:42
@Caius-Bonus
Copy link
Contributor Author

Converting to a draft, because the code changed a lot and needs to be retested with hardware.

@Caius-Bonus
Copy link
Contributor Author

Thanks, I just responded to all feedback of the ZHA PR and this PR is ready.

@TheJulianJES
Copy link
Collaborator

TheJulianJES commented Feb 23, 2024

@Caius-Bonus Could this technically be merged without breaking any current user-exposed functionality even if the ZHA PR doesn't make it for the .3 release for some reason?
(Custom attributes are not user-exposed IMO. Only the entities)

@Caius-Bonus
Copy link
Contributor Author

@Caius-Bonus Could this technically be merged without breaking any current user-exposed functionality even if the ZHA PR doesn't make it for the .3 release for some reason? (Custom attributes are not user-exposed IMO. Only the entities)

It shouldn't break anything, because there are no entities in ZHA currently depending on this quirk. The original quirk just changed the way setpoint changes were done and that behaviour will be exactly identical.

@Caius-Bonus
Copy link
Contributor Author

@TheJulianJES, there were a few issues with the checks not running correctly, but after 2 retries it works again. Ready for merge.

@Mikaka27
Copy link
Contributor

Mikaka27 commented Mar 28, 2024

@TheJulianJES do you know if this will make it into 2024.04?? I'm wondering whether to buy this device. Thanks :)

Edit: Or 2024.05 :)

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.

Trying to get this wrapped up (finally 😄 ). Few minor comments.

Also, can you rebase this once more? There were some ruff changes and I just want to make sure this PR still passes then.

I'll merge this then. For the custom functionality in ZHA, we should ideally try to move this into a v2 quirk afterwards. The docs aren't ready yet, but it's not that difficult. Most of the code (clusters and so on) stay the same. It's just the CustomDevice that gets modified. But again, that's for another PR, not this one.
It would allow us to move all/most of the functionality from your HA/ZHA PR into quirks directly which also makes future changes way easier, as it's all in quirks.

zhaquirks/danfoss/thermostat.py Outdated Show resolved Hide resolved
zhaquirks/danfoss/thermostat.py Outdated Show resolved Hide resolved
tests/test_danfoss.py Outdated Show resolved Hide resolved
tests/test_danfoss.py Outdated Show resolved Hide resolved
tests/test_danfoss.py Outdated Show resolved Hide resolved
tests/test_danfoss.py Outdated Show resolved Hide resolved
tests/test_danfoss.py Outdated Show resolved Hide resolved
zhaquirks/danfoss/thermostat.py Outdated Show resolved Hide resolved
tests/test_danfoss.py Outdated Show resolved Hide resolved
@Caius-Bonus
Copy link
Contributor Author

@TheJulianJES Ready

@Caius-Bonus
Copy link
Contributor Author

@Mikaka27 Keep in mind that even when this is merged, home-assistant/core#86907 needs to be merged before this is useful in home assistant.

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.

LGTM, thanks!
The timezone stuff also looks cleaner now.

@TheJulianJES TheJulianJES removed the breaking change PR is a breaking change label Apr 9, 2024
@TheJulianJES
Copy link
Collaborator

Codecov is still broken, but three lines are not covered by tests. I'm guessing this is another case of the "write time" stuff with bind(). As that's simple enough, that can be addressed in a future PR. This one is good to go IMO.

Name Stmts Miss Cover
zhaquirks/danfoss/thermostat.py 162 3 98%

@TheJulianJES TheJulianJES merged commit 24217a3 into zigpy:dev Apr 15, 2024
6 checks passed
lgraf pushed a commit to lgraf/zha-device-handlers that referenced this pull request May 6, 2024
…y#2150)

Danfoss thermostat: Fix attributes access, types, names and remove unsupported ones. split up proprietary clusters. add preheat_command (no usage yet). force read before bind of cluster. Add 2 thermostat models. Refactoring

---------

Co-authored-by: TheJulianJES <[email protected]>
@Caius-Bonus Caius-Bonus deleted the FixDanfossThermostat branch June 19, 2024 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve an existing quirk
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants