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 Tuya vibration door sensor _TZE200_kzm5w4iz #3246

Merged
merged 7 commits into from
Aug 27, 2024

Conversation

adam-zlatniczki
Copy link
Contributor

Proposed change

Added quirk for the Tuya Vibration Door Sensor, which didn't have any matching quirks before.

Additional information

--

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

Copy link

codecov bot commented Jul 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.33%. Comparing base (156aaa6) to head (9148010).
Report is 6 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #3246      +/-   ##
==========================================
+ Coverage   88.25%   88.33%   +0.07%     
==========================================
  Files         302      304       +2     
  Lines        9434     9497      +63     
==========================================
+ Hits         8326     8389      +63     
  Misses       1108     1108              

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

@TheJulianJES TheJulianJES changed the title Added quirk for Tuya Vibration Door Sensor. Add Tuya vibration door sensor Aug 3, 2024
@TheJulianJES TheJulianJES added the Tuya Request/PR regarding a Tuya device label Aug 3, 2024
@TheJulianJES
Copy link
Collaborator

Thanks for adding tests btw!

@adam-zlatniczki
Copy link
Contributor Author

_CONSTANT_ATTRIBUTES is a good proposal, I've moved the constant attributes.

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.

I think the quirk looks really good. Added a few small comments.

Comment on lines 69 to 74
# Initialize remaining battery to 100% until a proper measurement arrives.
# Device measures battery in 1% steps, while the ZCL specifies it in 0.5% steps.
self.update_attribute(
attr_name=PowerConfiguration.AttributeDefs.battery_percentage_remaining.name,
value=100 * 2,
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is done in __init__, this will update the attribute to 100% if HA is restarted.
Something like if attr_id not in self._attr_cache could be added before, but I think I'd just remove this for now.
HA should still initialize the sensor. It should just show something like "unknown". How long does it take for the device to report a value?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Removed for now.

Copy link
Contributor Author

@adam-zlatniczki adam-zlatniczki Aug 28, 2024

Choose a reason for hiding this comment

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

As far as I've noticed, the device reports battery state when there's a change in percentage. This doesn't happen too often: either when there's some variability in the measured voltage (e.g. moves from 80% to 81% then back to 80%), or when there's an actual drop. Since this is a door sensor and is supposed to be used frequently, the value will probably get initialized fairly soon, so I'm fine with this decision.

Comment on lines 85 to 93
def __init__(self, endpoint: Endpoint, is_server: bool = True):
"""Init constructor that initializes the major IasZone attributes."""
super().__init__(endpoint=endpoint, is_server=is_server)

# Initialize zone status until a proper update comes.
self.update_attribute(
attr_name=IasZone.AttributeDefs.zone_status.name,
value=0x0000,
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same thing here. This will update, as soon as the sensor is opened/closed once, right?
Do we really need to put "fake" initial data here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Removed for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same reasoning applies: the opening sensor will probably get used fairly soon, it doesn't matter much if it doesn't get initialized.

Comment on lines 104 to 112
def __init__(self, endpoint: Endpoint, is_server: bool = True):
"""Init constructor that initializes the major IasZone attributes."""
super().__init__(endpoint=endpoint, is_server=is_server)

# Initialize zone status until a proper update comes.
self.update_attribute(
attr_name=IasZone.AttributeDefs.zone_status.name,
value=0x0000,
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same thing here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Removed for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is where I disagree. Significant force is very rarely applied to most doors (in fact, my sensor only detected such change when I was manually testing the quirk, but never since then). This leads to an unitialized entity for quite a long time, which I personally don't really like to see on a UI. Initializing the vibration state to "no vibration" seems to me the lesser evil :)

@TheJulianJES TheJulianJES added the smash This PR is close to be merged soon label Aug 26, 2024
@TheJulianJES TheJulianJES changed the title Add Tuya vibration door sensor Add Tuya vibration door sensor _TZE200_kzm5w4iz Aug 26, 2024
zhaquirks/tuya/ts601_door.py Outdated Show resolved Hide resolved
zhaquirks/tuya/ts601_door.py Outdated Show resolved Hide resolved
zhaquirks/tuya/ts601_door.py Outdated Show resolved Hide resolved
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.

Thanks!

I've removed initializing the attributes for now, as they'd always initialize to the defaults specified when rebooting HA.
If the defaults are really needed, they can be re-added with adjustments I mentioned here. For now, the entities should stay unknown until the first update report.

Comment on lines +48 to +53
class CustomBasicCluster(CustomCluster, Basic):
"""Custom Basic cluster to report power source."""

_CONSTANT_ATTRIBUTES = {
Basic.AttributeDefs.power_source.id: Basic.PowerSource.Battery
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure we need this, not an issue for now though. Was it shown wrong on the HA device page?
That data comes from the node descriptor though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ZHA displayed the power source as "battery or unknown", which was bothering me a little, and in hopes of fixing that I tried setting it this way. However, it didn't really help, so I guess that is solely based on the device signature.

@TheJulianJES TheJulianJES merged commit a257c6c into zigpy:dev Aug 27, 2024
7 checks passed
@Jimfizz
Copy link

Jimfizz commented Sep 20, 2024

Hello, I have the same Tuya vibration sensor (TS0601) but the manufacturer is _TZE200_iba1ckek.
Are you able to deal different sensors (of the same type) in the same quirk or should I create a new topic ?
I use it to detect the sound of a doorbell and I would like to manage the sensitivity of the sensor.

EDIT : ooop, it's not strictly the same sensor, mine is only a vibration one. I will create a new entry.

@adam-zlatniczki
Copy link
Contributor Author

adam-zlatniczki commented Sep 20, 2024 via email

@Jimfizz
Copy link

Jimfizz commented Sep 20, 2024

Thank you @adam-zlatniczki : in ZHA the sensor does not report any door state (open/closed), only vibration detection (with reset), battery level (CR2450) & identifier

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

Successfully merging this pull request may close these issues.

3 participants