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 Sonoff SNZB-03P motion sensor #3979

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Conversation

panther7
Copy link

@panther7 panther7 commented Mar 25, 2025

Proposed change

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 Mar 25, 2025

Codecov Report

Attention: Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.

Project coverage is 91.05%. Comparing base (2fd3586) to head (0ce8446).
Report is 13 commits behind head on dev.

Files with missing lines Patch % Lines
zhaquirks/sonoff/snzb03p.py 94.44% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##              dev    #3979   +/-   ##
=======================================
  Coverage   91.04%   91.05%           
=======================================
  Files         331      332    +1     
  Lines       10698    10716   +18     
=======================================
+ Hits         9740     9757   +17     
- Misses        958      959    +1     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

(
QuirkBuilder("eWeLink", "SNZB-03P")
.replaces(SonoffIlluminationCluster)
.removes(IasZone.cluster_id) # IasZone cluster 0x0500: remove motion detection
Copy link
Collaborator

Choose a reason for hiding this comment

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

So the IasZone cluster doesn't do anything at all?
Can we use .prevent_default_entity_creation(endpoint_id=1, cluster_id=IasZone.cluster_id) here instead?
It's a new functionality that doesn't entirely remove the cluster for us, just disables the auto discovery for ZHA entities on that cluster.

Copy link
Author

@panther7 panther7 Mar 26, 2025

Choose a reason for hiding this comment

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

Yes, cluster is always "unavailable".

Comment on lines +34 to +36
@property
def _is_manuf_specific(self):
return False
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can use manufacturer_id_override: t.uint16_t = foundation.ZCLHeader.NO_MANUFACTURER_ID instead. See this PR as an example: https://github.com/zigpy/zha-device-handlers/pull/3910/files#diff-6c9212ca1af0b4604a20edb89247ceb6d30b010041e81fde812465a2f73a3ccbR24

Copy link
Author

Choose a reason for hiding this comment

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

Can not be used.

Replace with manufacturer_id_override: t.uint16_t = foundation.ZCLHeader.NO_MANUFACTURER_ID is broken:

  • eWeLink SNZB-03P is always detected
  • Last illumination state is Unavailable

2025-03-26 10 11 35

With _is_manuf_specific(self): return False is right:

2025-03-26 10 14 24

.replaces(SonoffIlluminationCluster)
.removes(IasZone.cluster_id) # IasZone cluster 0x0500: remove motion detection
.number(
OccupancySensing.AttributeDefs.ultrasonic_o_to_u_delay.name,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, interesting. This is a default attribute. It's also used by the Hue motion sensors, though it's reported as an "unsupported attribute" if we read it during pairing, for some reason, so I've not added that as a config entity for now. We could add the entities via quirks v2 for both sensors, but we might be able to add this as a general entity in ZHA. I'll have to come back to this.

Copy link
Author

Choose a reason for hiding this comment

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

If removed, then same issue like #3979 (comment)

@TheJulianJES TheJulianJES added v2 quirk Quirks using v2 API. Might add custom entities that need translation keys in HA. needs reviewer answer An answer from a reviewer is needed (e.g. why a PR isn't acceptable in the current state). labels Mar 25, 2025
Comment on lines +34 to +36
@property
def _is_manuf_specific(self):
return False
Copy link
Collaborator

Choose a reason for hiding this comment

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

Z2M seems to specify a manufacturer code: https://github.com/Koenkk/zigbee-herdsman-converters/blob/216ba1a56f1b21fda7de7204839f0fb838df0b85/src/devices/sonoff.ts#L1162
Do we even need this at all? By default, zigpy will use the manufacturer code for the device if the attribute is marked as is_manufacturer_specific (it is) and if the cluster is in the manufacturer specific range (it is).

I guess this is only really needed if we explicitly poll/read the attribute ourselves though (through the "Manage Zigbee device -> Clusters" debug menu).

@panther7
Copy link
Author

Thanks for huge feedback.

This PR is only request with originial code from @BillyBlaze and with (little) mods.

I have zero experience with Python and device handlers, bit i will try it.

@panther7
Copy link
Author

@TheJulianJES What next now? If i use some from your suggest, some functions wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs reviewer answer An answer from a reviewer is needed (e.g. why a PR isn't acceptable in the current state). v2 quirk Quirks using v2 API. Might add custom entities that need translation keys in HA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants