Skip to content

New Ikea starkvind quirk #1855

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 6 commits into from
Oct 25, 2022
Merged

New Ikea starkvind quirk #1855

merged 6 commits into from
Oct 25, 2022

Conversation

javicalle
Copy link
Collaborator

@javicalle javicalle commented Oct 22, 2022

New quirk version for the Ikea 'STARKVIND Air purifier table'.
Just cloned the previous one with the new signature.

Also some constants have been refactored.

To fix a mypy validation a value check has been added if fan_mode and fan_mode > 1 and fan_mode < 11:.

Related to: #1846

@coveralls
Copy link

coveralls commented Oct 22, 2022

Pull Request Test Coverage Report for Build 3308427843

  • 13 of 14 (92.86%) changed or added relevant lines in 9 files are covered.
  • 92 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.2%) to 80.665%

Changes Missing Coverage Covered Lines Changed/Added Lines %
zhaquirks/ikea/starkvind.py 4 5 80.0%
Files with Coverage Reduction New Missed Lines %
zhaquirks/tuya/init.py 92 71.99%
Totals Coverage Status
Change from base Build 3299655061: 0.2%
Covered Lines: 6066
Relevant Lines: 7520

💛 - Coveralls

@codecov-commenter
Copy link

codecov-commenter commented Oct 22, 2022

Codecov Report

Base: 80.44% // Head: 80.66% // Increases project coverage by +0.22% 🎉

Coverage data is based on head (270077f) compared to base (28f3399).
Patch coverage: 92.85% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1855      +/-   ##
==========================================
+ Coverage   80.44%   80.66%   +0.22%     
==========================================
  Files         240      242       +2     
  Lines        7481     7520      +39     
==========================================
+ Hits         6018     6066      +48     
+ Misses       1463     1454       -9     
Impacted Files Coverage Δ
zhaquirks/ikea/fivebtnremotezha.py 100.00% <ø> (ø)
zhaquirks/ikea/starkvind.py 67.18% <80.00%> (+15.57%) ⬆️
zhaquirks/ikea/__init__.py 71.15% <100.00%> (+1.15%) ⬆️
zhaquirks/ikea/blinds.py 100.00% <100.00%> (ø)
zhaquirks/ikea/fourbtnremote.py 100.00% <100.00%> (ø)
zhaquirks/ikea/motionzha.py 100.00% <100.00%> (ø)
zhaquirks/ikea/opencloseremote.py 67.74% <100.00%> (-1.01%) ⬇️
zhaquirks/ikea/symfonisk.py 100.00% <100.00%> (ø)
zhaquirks/ikea/tradfriplug.py 100.00% <100.00%> (ø)
zhaquirks/ikea/twobtnremote.py 100.00% <100.00%> (ø)
... and 5 more

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.

@MattWestb
Copy link
Contributor

Very nice work done !!

The second IKEA cluster is new and was first seen in Styrbar N Remote and we was not having any name of it so it was called WWAH_CLUSTER_ID (you your imagination for interpreting it).

Then more 3rd gen device is using it i think it better giving it one more "serious" name like IKEA_CLUSTER2_ID or somthing else (perhaps IKEA_CLUSTER1_ID and IKEA_CLUSTER2_ID for 0xFC7C and 0xFC57).
I dont have any strong preference but i like the code look more serious and all is better then crappy naming without any meaning in them.

Also is the MFC cluster being used in ZHA for patching some things and can being broken is changing the names ?

Then looking on the Hex it looks it can being one bit error that IKEA have done 0xFC7C / 0xFC57 ( the second hex is having the 2 bit sett and not set > + - 2).
That is not relevant for us then the devices is on the market and we can changing the data setting in them.

@javicalle
Copy link
Collaborator Author

Also is the MFC cluster being used in ZHA for patching some things and can being broken is changing the names ?

I don't think that the name is used, but I can check it.

I would left the cluster rename for a new PR.

@TheJulianJES
Copy link
Collaborator

The second IKEA cluster is new and was first seen in Styrbar N Remote and we was not having any name of it so it was called WWAH_CLUSTER_ID (you your imagination for interpreting it).

FYI, 0xFC57 should be the cluster ID of the "Works with all Hubs" (WWAHu) "standardized" Zigbee cluster which is now mainly used if the device is directly connected to a Alexa. I'm not sure if the IKEA devices actually implement that properly though, as I've never tried it.

@MattWestb
Copy link
Contributor

Then i think the "Works with all Hubs" / 0xFC57 shall being removed from IKEA part and moved to constants or some other place then its not manufacture (IKEA) related and can being used for more device / manufactures.

Not urgent for this PR but best fixing so not getting more python pasta slate in the future.

Thanks for explaining TheJulianJES and sharing your knowledge !!!

@TheJulianJES
Copy link
Collaborator

Then i think the "Works with all Hubs" / 0xFC57 shall being removed from IKEA part and moved to constants

Yeah, probably. I'm not sure if any other devices even have that cluster currently.
For now, moving it to the main IKEA file is a lot better already than having them in every quirk individually.

@MattWestb
Copy link
Contributor

I agree but its not very urgent like getting devices working that have getting its firmware updated.

We shall putting it on the ToDo list and getting this PR merged so the users is getting full functionality in there devices.

@javicalle
Copy link
Collaborator Author

So we feel comfortable keeping the name WWAH_CLUSTER_ID?

@dmulcahey
Copy link
Collaborator

So we feel comfortable keeping the name WWAH_CLUSTER_ID?

absolutely. I am looking for the SI document that outlines it so we can implement all of it as well.

@dmulcahey dmulcahey merged commit dc3f348 into zigpy:dev Oct 25, 2022
@javicalle javicalle deleted the new_starkvind branch October 25, 2022 15:09
@javicalle
Copy link
Collaborator Author

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants