-
Notifications
You must be signed in to change notification settings - Fork 484
generic_zigbee_sensor_improvement #2093
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
base: main
Are you sure you want to change the base?
generic_zigbee_sensor_improvement #2093
Conversation
Duplicate profile check: Passed - no duplicate profiles detected. |
Test Results 66 files 427 suites 0s ⏱️ Results for commit 9946a16. ♻️ This comment has been updated with latest results. |
Minimum allowed coverage is Generated by 🐒 cobertura-action against 9946a16 |
0f03911
to
21f22c9
Compare
9267b81
to
b81729b
Compare
- 0xfe00 # EZVIZ private cluster | ||
- 0xfe05 # EZVIZ private cluster |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we usually capitalize the letters in our hex
local generate_event_from_emergency = function(driver, device) | ||
device:emit_event(capabilities.button.button.pushed({ state_change = true })) | ||
end | ||
|
||
local ias_ace_emergency_handler = function(driver, device) | ||
generate_event_from_emergency(driver, device) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these functions can be consolidated
local generate_event_from_zone_status = function(driver, device, zone_status, zb_rx) | ||
local event | ||
if zone_status:is_alarm1_set() or zone_status:is_alarm2_set() then | ||
event = capabilities.contactSensor.contact.open() | ||
else | ||
event = capabilities.contactSensor.contact.closed() | ||
end | ||
if event ~= nil then | ||
device:emit_event_for_endpoint( | ||
zb_rx.address_header.src_endpoint.value, | ||
event) | ||
end | ||
end | ||
|
||
|
||
local ias_zone_status_attr_handler = function(driver, device, zone_status, zb_rx) | ||
generate_event_from_zone_status(driver, device, zone_status, zb_rx) | ||
end | ||
|
||
local ias_zone_status_change_handler = function(driver, device, zb_rx) | ||
generate_event_from_zone_status(driver, device, zb_rx.body.zcl_body.zone_status, zb_rx) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we overwriting the defaults somewhere that makes it necessary to redefine them here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right, we double check, and the redefine handler is not necessary and will be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd appreciate if we could separate the zigbee generic sensor driver from the Tuya drivers.
Currently we have divided tuya and zigbee_generic_sensor into different directories, specifically located within "/drivers/generic_zigbee_sensor" and "/Unofficial/tuya" floder. Are you suggesting that we should further separate the Tuya-powered manufacturers, such as Eviz and Meian, into the "/Unofficial/tuya" folder? |
6a5a0dd
to
29fae3a
Compare
@pInksenberg I meant have them as two separate pull requests |
29fae3a
to
9946a16
Compare
Invitation URL: |
@greens , sure, I separated tuya devices support into another PR, please kindly check~ |
- id: "generic-button-sensor" | ||
deviceLabel: "Zigbee Generic Button" | ||
clusters: | ||
server: | ||
- 0x0500 | ||
- 0xFE00 # EZVIZ private cluster | ||
- 0xFE05 # EZVIZ private cluster | ||
deviceProfileName: generic-remote-control |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this device is a button it should be in the zigbee button driver
As far as I can tell it doesn't do any sensing, so it doesn't make much sense to put it here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After our team discussions, we have no better solution for this yet. Could you please share your suggestions? How about just put it to Unofficial/ezviz directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could do that or you could put this exact same fingerprint in the zigbee-button
driver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@greens
Actually, this device is non-WWST device, so we can't put exact fingerprint in zigbee-button driver for this device.
As you know, zigbee-sensor have only generic fingerprint for generic purpose and scalability. I think by adding like this, we can support more ezviz button if they use same private cluster in the future(or now) for their new button devices. and If we need to support more other different types of ezviz devices in the future, then let's move this device into Unofficial/ezviz. but now, Let's just keep this device into zigbee-sensor for a while. What do you think?
- id: "generic-button-sensor"
deviceLabel: "Zigbee Generic Button"
clusters:
server:
- 0x0500
- 0xFE00 # EZVIZ private cluster
- 0xFE05 # EZVIZ private cluster
deviceProfileName: generic-remote-control
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put this fingerprint:
- id: "generic-button-sensor"
deviceLabel: "Zigbee Generic Button"
clusters:
server:
- 0x0500
- 0xFE00 # EZVIZ private cluster
- 0xFE05 # EZVIZ private cluster
deviceProfileName: generic-remote-control
under a new zigbeeGeneric:
section here: https://github.com/SmartThingsCommunity/SmartThingsEdgeDrivers/blob/main/drivers/SmartThings/zigbee-button/fingerprints.yml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
buttons should use the button driver
}, | ||
can_handle = is_water_sensor | ||
} | ||
defaults.register_for_default_handlers(generic_water_sensor, generic_water_sensor.supported_capabilities) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@varzac Does only registering for defaults in a specific sub-driver work like this?
require("meian"), | ||
require("ezviz") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about changing to 'meian-button' / 'ezviz-button' for the clarity? (including the folder name of each sub drivers)
Check all that apply
Type of Change
Checklist
Description of Change
Refactoring zigbee-sensor driver, and adding support for tuya zigbee devices.
Summary of Completed Tests