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

RFC: Tuya data refactoring (implicit conversion of tuya.Data) #2017

Merged
merged 1 commit into from
Feb 22, 2023

Conversation

Shulyaka
Copy link
Contributor

@Shulyaka Shulyaka commented Dec 17, 2022

Ok, I am not sure all this is needed and I need your advice.

This PR adds implicit conversions for tuya.Data class.

tuya.Data.to_value and tuya.Data.from_value were removed. Instead the type conversion function were added.

@coveralls
Copy link

coveralls commented Dec 17, 2022

Pull Request Test Coverage Report for Build 4217877523

  • 20 of 20 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 83.724%

Totals Coverage Status
Change from base Build 4217554240: 0.02%
Covered Lines: 6857
Relevant Lines: 8190

💛 - Coveralls

@codecov-commenter
Copy link

codecov-commenter commented Dec 17, 2022

Codecov Report

Base: 83.54% // Head: 83.46% // Decreases project coverage by -0.08% ⚠️

Coverage data is based on head (e2e3a0e) compared to base (c2b2893).
Patch coverage: 85.07% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #2017      +/-   ##
==========================================
- Coverage   83.54%   83.46%   -0.09%     
==========================================
  Files         249      249              
  Lines        8022     8037      +15     
==========================================
+ Hits         6702     6708       +6     
- Misses       1320     1329       +9     
Impacted Files Coverage Δ
zhaquirks/tuya/ts0601_rcbo.py 98.59% <ø> (ø)
zhaquirks/tuya/ts0601_siren.py 93.96% <ø> (ø)
zhaquirks/tuya/ts0601_valve.py 92.30% <ø> (ø)
zhaquirks/tuya/__init__.py 73.24% <84.37%> (+0.29%) ⬆️
zhaquirks/tuya/mcu/__init__.py 99.44% <100.00%> (-0.07%) ⬇️
zhaquirks/tuya/ts0601_illuminance.py 100.00% <100.00%> (ø)
zhaquirks/tuya/ts0601_sensor.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@javicalle
Copy link
Collaborator

javicalle commented Dec 18, 2022

Hi @Shulyaka, thanks once again for your contributions.

Firts I will try to put some background info and then I will try to check the code.

When firsts Tuya dimmers come, I noticed that will need some work to make it work. I check the code and looking for a solution I found the TuyaNewManufCluster.
Once I understood how it works, I decided to use that approach for new implementations, but (due to my poor knowledge of Python) I tried to avoid affecting existing implementations as much as possible. It is for this reason that I create and put most of the implementations in zhaquirks.tuya.mcu.__init__.py extending the zhaquirks.tuya.__init__.py classes (instead of implementing on top of them).
That approach allowed me to create new implementations without the risk of 'breaking' existing implementations and also segregate the MCU classes from other standard Zigbee implementations.

My goal was also to bring over time all the implementations related to the Tuya MCU devices to the new package as the knowledge and development of the classes allowed me and (once I was confident enough) to migrate all the references and quirks to the new ones classes and delete the old classes so that nobody uses them.

Obviously, my forecast has not been fulfilled but in my head it is still the goal to meet.

I hope all this explanation helps you understand what I did in zhaquirks.tuya.mcu.__init__.py and my approach in my review.

@Shulyaka
Copy link
Contributor Author

Totally agree on both approach and future goals. By the way, are all tuya devices MCU? I mean, can they all in theory be migrated to TuyaMCUCluster?
Python is not my native language either.
But I think some of these ideas might be worth including. I've seen you were going to do part of it yourself (remove reversing of TuyaData.raw on deserializing and do it in TuyaData.payload when needed)

@MattWestb
Copy link
Contributor

First gen tuya TRVs that is using tuya MCU was having the Zigbee manufacture name _TYST11_* and then updated Zigbee module all is having TS0601 as Zigbee model id.

But i can being "hybrid devices" that looks like normal Zigbee devices but is also sending tuya MCU commands.

@javicalle
Copy link
Collaborator

are all tuya devices MCU? I mean, can they all in theory be migrated to TuyaMCUCluster?

No, they are some Tuya devices behaving 'standard' zigbee devices.

Tuya devices are like a bizarre forest where you can find any kind of devices.

But I think some of these ideas might be worth including.

Yes, for sure.

I've seen you were going to do part of it yourself

Well, I was looking at the TuyaData.VALUE but you're right. I'll try to check it over your implementation.

@Shulyaka
Copy link
Contributor Author

I've split this PR into 3, now it has only left a single change.

@Shulyaka Shulyaka marked this pull request as ready for review December 30, 2022 16:45
@Shulyaka Shulyaka changed the title RFC: Tuya data refactoring RFC: Tuya data refactoring (implicit conversion of tuya.Data Dec 30, 2022
@Shulyaka Shulyaka changed the title RFC: Tuya data refactoring (implicit conversion of tuya.Data RFC: Tuya data refactoring (implicit conversion of tuya.Data) Dec 30, 2022
@javicalle
Copy link
Collaborator

Thanks for not giving up.

@Shulyaka
Copy link
Contributor Author

I am having fun:)

@TheJulianJES TheJulianJES added the Tuya Request/PR regarding a Tuya device label Jan 16, 2023
@dmulcahey
Copy link
Collaborator

@Shulyaka is this still being worked on? I know you split a bunch into separate PR’s. Just checking on this one. Thanks!

@Shulyaka
Copy link
Contributor Author

Shulyaka commented Feb 19, 2023

Hi! Yes. This is the remaining part.
I've just rebased it.
Sorry for the delay.
From my perspective, it is ready.

Copy link
Collaborator

@javicalle javicalle left a comment

Choose a reason for hiding this comment

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

Despite that all the Data methods are beyond my level I am confident in the tests so...
LGTM

@TheJulianJES
Copy link
Collaborator

TheJulianJES commented Feb 22, 2023

Do we want to merge this for the next zha-quirks release? (so for HA Core 2023.3.0)

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 – don't see any obvious issues 😄

@javicalle
Copy link
Collaborator

Do we want to merge this for the next zha-quirks release? (so for HA Core 2023.3.0)

Why not? 😉
Let's put it on the beta release.

@TheJulianJES TheJulianJES merged commit 2318fda into zigpy:dev Feb 22, 2023
@kukudemajia
Copy link

This is a terrible change, after all, many end users are not experts in this area.

@javicalle
Copy link
Collaborator

This is a terrible change, after all, many end users are not experts in this area.

Can you elaborate it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tuya Request/PR regarding a Tuya device
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants