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

Extended Beca TRV #1459

Closed
wants to merge 9 commits into from
Closed

Extended Beca TRV #1459

wants to merge 9 commits into from

Conversation

rforro
Copy link
Contributor

@rforro rforro commented Apr 3, 2022

I've finalized quirk for BECA TRV which was written by @jacekk015 but PR got stuck because of missing tests. This code was extracted from #1192 which is basically dead, because it is not compatible with current zigpy version.

Note this PR uses AnalogInput cluster which is currently not exposed in HA, but I'm working on it. @Adminiuga @dmulcahey I need your advice, please look at home-assistant/core#69063

After merging, this PR should close following issue #1123

@codecov-commenter
Copy link

codecov-commenter commented Apr 3, 2022

Codecov Report

Merging #1459 (afbe6ed) into dev (bbc69c0) will decrease coverage by 0.57%.
The diff coverage is 70.85%.

@@            Coverage Diff             @@
##              dev    #1459      +/-   ##
==========================================
- Coverage   80.30%   79.72%   -0.58%     
==========================================
  Files         227      228       +1     
  Lines        6687     7123     +436     
==========================================
+ Hits         5370     5679     +309     
- Misses       1317     1444     +127     
Impacted Files Coverage Δ
zhaquirks/tuya/ts0601_trv.py 81.70% <ø> (+0.02%) ⬆️
zhaquirks/tuya/ts0601_trv_beca.py 70.85% <70.85%> (ø)
zhaquirks/tuya/__init__.py 72.54% <0.00%> (+0.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bbc69c0...afbe6ed. Read the comment docs.

@coveralls
Copy link

coveralls commented Apr 3, 2022

Pull Request Test Coverage Report for Build 2140384247

  • 316 of 446 (70.85%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.6%) to 79.773%

Changes Missing Coverage Covered Lines Changed/Added Lines %
zhaquirks/tuya/ts0601_trv_beca.py 316 446 70.85%
Totals Coverage Status
Change from base Build 2112012540: -0.6%
Covered Lines: 5691
Relevant Lines: 7134

💛 - Coveralls

@rforro
Copy link
Contributor Author

rforro commented Apr 9, 2022

PR #1464 was applied

@rforro
Copy link
Contributor Author

rforro commented Apr 15, 2022

@dmulcahey Can you tell me what hinders this PR from being merged?

@dmulcahey
Copy link
Collaborator

dmulcahey commented Apr 16, 2022

@dmulcahey Can you tell me what hinders this PR from being merged?

I need to look at the analog input stuff... we are adding correct support for configuration entities in HA and I do not want to continue abusing analog input clusters.

As an example: home-assistant/core#70110

@bh
Copy link

bh commented Apr 23, 2022

This is an image
This is an image

The "switch" over the cursor in the second screenshot is the childlock switch. The entity of the switch is "switch.tze200_b6wax7g0_ts0601_5bb99efe_on_off_". Can you change it to indicate that this is the child lock switch?

Works fine with Moes BRT-100. Good job!

Copy link
Collaborator

@dmulcahey dmulcahey left a comment

Choose a reason for hiding this comment

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

home-assistant/core#70110 Let's change how this works so we aren't adding a bunch of fake clusters to the device to handle the configuration of the device. We can target the mfg cluster + make + model directly in HA.

@rforro
Copy link
Contributor Author

rforro commented May 4, 2022

home-assistant/core#70110 Let's change how this works so we aren't adding a bunch of fake clusters to the device to handle the configuration of the device. We can target the mfg cluster + make + model directly in HA.

I'm sorry but I don't get what you mean. Should I replace those generic AnalogInput/Output with my own and then add all of them into homeassistant/components/zha but target them to this TRV only?

@dmulcahey
Copy link
Collaborator

Do you have discord?

@miklosbagi
Copy link

I'm not sure if you guys have actually discussed the fake cluster issue came up in this PR, however, I'm happy to report that ts0601_trv_beca.py applied via zha_quirks fixes how my TRVs look like.

So for folks searching for Moes/Tuya/Beca Radiator Thermostat BRT-100-TRV ZHA support as of issues with the currently (2022.8.7) applied zhaquirks.tuya.ts0601_trv.MoesHY368_Type1new quirk (ping #1192 #932), this PR has the best quirk to be applied to date.

Fingers crossed it can get merged soon, I've got like 10 of these devices, so this was great help. Thanks @rforro !

@dmulcahey
Copy link
Collaborator

Is this still needed? If so, can we work towards getting it mergeable?

@MattWestb
Copy link
Contributor

Nearly all tuya TRVs have very basic functionality and not working in ZHA GUI and we have spending weeks of testing and coding so the user can getting there devices working OK in ZHA.

Still If no one can doing it better this PR is much better then no or very bad support for tuya TRVs and more user is getting them and moving to other system (normaly not to de(F)CON) then is not getting any functionality in ZHA GUI.

Also pleas say what is wrong and trying getting it fixed instead letting it dying jacekk015] on Nov 29, 2021 and in some week the PR is needed then its being cold in the north.

@miklosbagi
Copy link

Is this still needed? If so, can we work towards getting it mergeable?

Yes - it solved my problem.

Out of the box the Moes TRV provides proper climate entities. However, on the latest 2022.8.7, battery % is reported as "unknown" and there's a bunch of entities missing compared to what's visible in @bh 's screenshots (child lock, min max temps, temp correction, etc).

It'd be awesome getting this merge-ready.

@TheJulianJES TheJulianJES added the Tuya Request/PR regarding a Tuya device label Jan 16, 2023
@github-actions
Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. Thank you for your contributions.

@github-actions github-actions bot added the stale Issue is inactivate and might get closed soon label Jul 15, 2023
@github-actions github-actions bot closed this Jul 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Issue is inactivate and might get closed soon Tuya Request/PR regarding a Tuya device
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants