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 custom attributes for ThirdReality nightlight 3RSNL02043Z #3276

Merged
merged 1 commit into from
Aug 26, 2024

Conversation

llhappier
Copy link
Contributor

@llhappier llhappier commented Aug 1, 2024

Change-Id: I136b856588fb42e69274b899cdafa3c5e9ce819f

Proposed change

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 Aug 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.26%. Comparing base (e3c115e) to head (8fe1f6a).
Report is 10 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #3276      +/-   ##
==========================================
+ Coverage   88.18%   88.26%   +0.07%     
==========================================
  Files         301      302       +1     
  Lines        9412     9440      +28     
==========================================
+ Hits         8300     8332      +32     
+ Misses       1112     1108       -4     

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

@llhappier llhappier force-pushed the dev branch 2 times, most recently from d3e9b13 to 0e4cb16 Compare August 1, 2024 07:54
@puddly
Copy link
Contributor

puddly commented Aug 4, 2024

Can you describe what these attributes do?

@llhappier
Copy link
Contributor Author

Hi,
Through this private cluster, these attributes are used to set the coldown time for pir sensor, the duration of lthe ocal routines, and the threshold for reporting changes in lux sensor values.

@puddly
Copy link
Contributor

puddly commented Aug 5, 2024

Right, that's clear from the names of the attributes.

  • What are local routines?
  • PIR cool down time can be set using the PIR Configuration Set attributes from the ZCL: PIROCcupiedToUnoccupiedTime, etc. Does this need to be a manufacturer specific attribute?
  • Why does the lux sensor have a separate attribute for configuring attribute reporting? This is normally done using ZCL attribute reporting config, which sets the min/max reporting interval and the minimum change.

@llhappier
Copy link
Contributor Author

Hi, puddly. This is a multifunction device and It contains light, pir sensor, and illuminance sensor.

  1. This local routine is performs the actions of turning on and off the lights based on the status of the pir sensor and illuminance sensor and does not require sending Zigbee commands. The modification this time is the interface for the duration of the light on.
  2. Yes, the IAS zone cluster can be used for pir sensor. However, if the IAS zone cluster is registered, this device may be recognized as a sensor on some platforms instead of the primary feature light, such as on Amazon's Echo v4 etc.
  3. We want freely set the reporting sensitivity of the illuminance sensor.
    By the way,we can also use service: zhatoolkit.attr_write to write any attribute of any cluster and config report, it's just that installing this ZHA toolkit is inconvenient, so we changed this script.
    Thank you.

@llhappier llhappier force-pushed the dev branch 2 times, most recently from 4700bff to dde7d28 Compare August 7, 2024 02:39
@llhappier
Copy link
Contributor Author

Copy that, thank you.

@llhappier
Copy link
Contributor Author

OK. Got it. I'm sorrry about this mistake and will definitely pay attention next time. Thanks.

Change-Id: I136b856588fb42e69274b899cdafa3c5e9ce819f
@TheJulianJES
Copy link
Collaborator

All good! By the way, you don't need to squash and force push your commits every time. You can just add another commit changing things. The whole PR will be squashed automatically when merging, so no need to do it manually before.

@llhappier
Copy link
Contributor Author

Okay, I will understand and use it next time. Thank you

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 this should be fine for now, since it just adds the custom attribute definitions.
In the future, we can think about replacing the Nightlight/CustomDevice v1 quirk with a v2 quirk that could look something like this:

(
    QuirkBuilder(THIRD_REALITY, "3RSNL02043Z")
    .replaces(LocalIasZone)
    .replaces(ThirdRealitySpecificCluster)
    .number(
        ThirdRealitySpecificCluster.AttributeDefs.cooldown_time.name,
        ThirdRealitySpecificCluster.cluster_id,
        min_value=0,  # change
        max_value=100,  # change
        step=1,  # change
        unit="s",  # change?
    )
    .number(
        ThirdRealitySpecificCluster.AttributeDefs.local_routine_time.name,
        ThirdRealitySpecificCluster.cluster_id,
        min_value=0,  # change
        max_value=100,  # change
        step=1,  # change
        unit="s",  # change
    )
    .number(
        ThirdRealitySpecificCluster.AttributeDefs.lux_threshold.name,
        ThirdRealitySpecificCluster.cluster_id,
        min_value=0,  # change
        max_value=100,  # change
        step=1,  # change
        unit="s",  # change
    )
    .add_to_registry()
)

(untested and unfinished code above, just a quick draft)

It would allow users to configure the custom attributes from entities in HA. Translation keys will also need to be added to HA, but we should do that in the quirks/ZHA bump PR.

To confirm, @puddly, you're also good with this PR for now?

@TheJulianJES TheJulianJES changed the title Add read-write interfaces for private cluster in 3RSNL02043Z Add custom attributes for ThirdReality nightlight 3RSNL02043Z Aug 26, 2024
@TheJulianJES TheJulianJES added the smash This PR is close to be merged soon label Aug 26, 2024
@puddly
Copy link
Contributor

puddly commented Aug 26, 2024

Yeah, there's nothing new, it's just mapping attributes. A followup PR should expose them as entities.

@puddly puddly merged commit 6ffa8cb into zigpy:dev Aug 26, 2024
6 checks passed
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants