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

Add _VALID_ATTRIBUTES to LocalDataCluster, fix Tuya datapoint mappings on LocalDataClusters #3415

Merged
merged 4 commits into from
Oct 16, 2024

Conversation

TheJulianJES
Copy link
Collaborator

@TheJulianJES TheJulianJES commented Oct 11, 2024

UNTESTED

This should probably be tested with some actual devices. Unit tests pass already.

Proposed change

  • This adds _VALID_ATTRIBUTES to LocalDataCluster.
    This allows entities in ZHA to be created*, as the initial attribute reads will return None with success status, instead of "unsupported attribute" for all attributes in the _VALID_ATTRIBUTES list.

  • The PR also automatically adds all mapped to attributes from Tuya datapoints on LocalDataClusters to _VALID_ATTRIBUTES to ensure entities in ZHA can be created on pairing.

  • A basic test checking that attribute reads work as expected on a LocalDataCluster is also added.


*: Do note that some entities in ZHA will not be created for None as a value in the attribute cache. This is only the case for configuration switch, configuration select, and configuration number entities IIRC. This should not be an issue, as this PR is mainly intended to fix the "actual ZCL entities" like temperature sensors and so on. Those are also created with None values on pairing.

I explicitly chose to not populate the attribute cache with "valid" numbers like 0, as the data type can vary and that's somewhat unclean in general. If needed, we can add an initial_value to DPToAttributeMapping in the future, but I do not think this will be an issue at all.
For ZCL devices, the attribute cache will also contain None at first. We just follow that behavior for Tuya devices that use a LocalDataCluster with an attribute being mapped to from a Tuya datapoint.

Additional information

Other non-Tuya quirks should likely also utilize _VALID_ATTRIBUTES if they are currently using a LocalDataCluster where some attributes are only updated later and not when a device is initially paired.

I've also noticed that a bunch of Tuya mappings do not exist on some quirks. It's not really an issue though, but I needed to account for that.

If the PR approach is good, then I'll probably split the PR to only include the _VALID_ATTRIBUTES part in this PR and have a second PR with the third commit for the Tuya mapped attributes being initialized as "valid".
EDIT: It was both done in this PR now.

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

This list should be populated with the attribute ids of attributes that will not be populated at first, but only later.
All attribute reads on the `LocalDataCluster` for those specified attributes will return `None` with success status when no value is in the attribute cache.
This allows ZHA entity creation for those entities (except some configuration entities that explicitly check for `None` attributes).

If they are not in the valid attributes list, they will retain the previous behavior returning as "unsupported attribute".
This prevents ZHA entity creation for those attributes.
This adds a test which checks if the reading of the following works as expected on a `LocalDataCluster`:
- invalid attribute reading returning unsupported
- constant attribute reading working
- valid attribute reading returning `None` with success status
…s valid

This allows entities in ZHA to be created, as the initial attribute reads will return `None` with success status, instead of "unsupported attribute".
Copy link

codecov bot commented Oct 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.25%. Comparing base (93af47a) to head (a68a1ed).
Report is 1 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #3415      +/-   ##
==========================================
+ Coverage   89.23%   89.25%   +0.01%     
==========================================
  Files         307      307              
  Lines        9864     9881      +17     
==========================================
+ Hits         8802     8819      +17     
  Misses       1062     1062              

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

@TheJulianJES
Copy link
Collaborator Author

Let’s try this on HA dev. Should work fine.

@TheJulianJES TheJulianJES added enhancement Improve an existing quirk Tuya Request/PR regarding a Tuya device labels Oct 16, 2024
@TheJulianJES TheJulianJES merged commit fff11ff into zigpy:dev Oct 16, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve an existing quirk Tuya Request/PR regarding a Tuya device
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant