add support for generic zigbee multi-switch#1922
add support for generic zigbee multi-switch#1922cbaumler merged 1 commit intoSmartThingsCommunity:mainfrom
Conversation
|
Channel deleted. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 83678e9 |
| }) | ||
| end | ||
|
|
||
| local function device_added(driver, device, event) |
There was a problem hiding this comment.
This added function is now going to be run for all devices joined to the zigbee-switch driver, I don't think we should be making a change that affects all those other devices.
There was a problem hiding this comment.
This should be moved to the subdriver as this should only be run for the tuya devices.
EDIT: Oh, I see it was changed to not include the custom stuff so this may be fine to leave in the base driver since it is pretty generic.
There was a problem hiding this comment.
I figured out which devices are affected by this code in wwst certified devices. Most of devices that have multi endpoint has their own added_handler. but there is one device that doesn't have sub-driver.
- id: "ubisys/S2 (5502)"
deviceLabel: ubisys S2
manufacturer: ubisys
model: "S2 (5502)"
deviceProfileName: switch-power-firmware-2
ubisys S2 is multi component device.
name: switch-power-firmware-2
components:
- id: main
capabilities:
- id: powerMeter
version: 1
- id: firmwareUpdate
version: 1
- id: refresh
version: 1
categories:
- name: Switch
- id: switch1
capabilities:
- id: switch
version: 1
categories:
- name: Switch
- id: switch2
capabilities:
- id: switch
version: 1
categories:
- name: Switch
the device will be affected by this code. we need to avoid these code for this device.
There was a problem hiding this comment.
we will add S2 device(MCD) fingerprint in added handler to avoid running our logic.
There was a problem hiding this comment.
We should test that the device is a Zigbee device type as well, as EDGE_CHILD devices are added to the driver and will try to run added code as well.
There was a problem hiding this comment.
It would be ideal if we could test that this correctly generates the create_device calls, and won't trigger for MCD devices.
There was a problem hiding this comment.
Since we do not have WWST MCD device in office, we mocked a device which fingerprint and clusters are totally same with a real one. This create_device call will not be triggered.
|
|
||
|
|
||
| local function send_discover_command(device) | ||
| local discover_attribute_id = 0x0c |
There was a problem hiding this comment.
This looks to be a command ID and not an attribute ID, and this command is not defined in the ZCL spec, which makes it seem like a manufacturer specific function, and we should not be sending a manufacturer specific command to every device that is onboarded.
There was a problem hiding this comment.
I think the intention is to send the global command Discovery Attributes which does have a command ID of 0x0C
There was a problem hiding this comment.
Ah, I had forgotten the default for the frame control was a global command. So that explains the command. I'm still not sure what the reason for sending it is.
| device:send(send_message) | ||
| end | ||
|
|
||
| local function read_attribute_function(device, cluster_id, attr_id) |
There was a problem hiding this comment.
A function like this is already defined in BasicCluster.read_attribute so you don't need to redifine it here.
| return result | ||
| end | ||
|
|
||
| local function all_zigbee_message_handler(self, message_channel) |
There was a problem hiding this comment.
This is going to change how EVERY message processed by ANY Zigbee Switch on the platform is handled. I do not think we should make this change.
pInksenberg
force-pushed
the
add_support_for_zigbee_mutli-switch
branch
from
bc2da4c to
fc3b686
Compare
| end | ||
|
|
||
| local function discover_response_message_handler(driver, device, zb_rx) | ||
| if zb_rx.address_header.cluster.value == clusters.Basic.ID and zb_rx.body.zcl_header.cmd.value == DISCOVER_ATTR_RSP_ID then |
There was a problem hiding this comment.
If this function is called, both of these have already been validated, so you don't need to check again here.
| device:send(send_message) | ||
| end | ||
|
|
||
| local function read_attribute_function(device, cluster_id, attr_id) |
There was a problem hiding this comment.
As mentioned, this code is already defined in the zigbee cluster_base, so you can just require the BasicCluster and then use BasicCluster.read_attribute(device, cluster_id, attr_id) instead of duplicating the code here.
There was a problem hiding this comment.
We try read_attribute(device, cluster_id, attr_id) in cluster_base, it seems that this API can only send single AttributeId,
but Tuya needs a list of attribute to send, we wonder your advice
| end | ||
| table.remove(bytes, 1) -- remove first type which means discover attribute command is completed | ||
| local result = {} | ||
| for i = 1, #bytes, 3 do -- remove data type information |
There was a problem hiding this comment.
I think this loop can be simplified by using the buf lib local buf = buf_lib.Reader(data) and then you can use
https://github.ecodesamsung.com/iot-hub/scripting-engine/blob/main/lua_libs/st/zigbee/data_types/AttributeId.lua
and
https://github.ecodesamsung.com/iot-hub/scripting-engine/blob/main/lua_libs/st/zigbee/data_types/ZigbeeDataType.lua
using their deserialize functions.
So something like
local attr_list = {}
while buf:remain() > 0 do
attr_list[#attr_list+1] = AttributeId.deserialize(buf)
local type_info = ZigbeeDataType.deserialize(buf)
end
you could also create a definition for the discover attribute response global command similar to https://developer.smartthings.com/docs/edge-device-drivers/zigbee/zcl/zcl_commands.html#read-attribute-response so that we can support this from any driver instead of custom in this driver.
| local TUYA_MFR_HEADER = "_TZ" | ||
|
|
||
| local function is_tuya_products(opts, driver, device) | ||
| if string.sub(device:get_manufacturer(),1,3) == TUYA_MFR_HEADER then -- if it is a tuya device, then send the magic packet |
There was a problem hiding this comment.
Should we also check that there is more than 1 zigbee endpoint since this handling is only for multi switch devices?
There was a problem hiding this comment.
we will check more for 1 zigbee endpoint device
There was a problem hiding this comment.
@pInksenberg can you address @varzac 's comment?
There was a problem hiding this comment.
Sure, we will add this logic in next commit tonight.
| end | ||
|
|
||
| local function device_added(driver, device, event) | ||
| device:set_find_child(find_child) |
There was a problem hiding this comment.
I would move setting this function to only be set if at least 1 child device is created.
pInksenberg
force-pushed
the
add_support_for_zigbee_mutli-switch
branch
3 times, most recently
from
c7c1799 to
bc4219c
Compare
|
|
|
|
||
| local function is_mcd_device(device) | ||
| for _, fingerprint in ipairs(MCD_fingerprint) do | ||
| if device:get_manufacturer() == fingerprint.mfr and device:get_model() == fingerprint.model then |
There was a problem hiding this comment.
we shouldn't check that the device is specific model, we should just check if the profile on the device object has multiple components
pInksenberg
force-pushed
the
add_support_for_zigbee_mutli-switch
branch
from
f839401 to
9220461
Compare
| - 0x0B04 #Electrical Measurment | ||
| deviceProfileName: switch-power | ||
| - 0x0702 #Simple Metering | ||
| deviceProfileName: switch-power-energy |
There was a problem hiding this comment.
It looks like all the changes in the fingerprints file is just moving things around, which is making the PR more confusing. I think you should revert all these changes as they don't seem relevant.
There was a problem hiding this comment.
Let me explain that, after we moving fingerprints around, we find that a switch which has 0x0b04 & 0x0702 in server cluster will match with switch-power profile. We want these plug to show as a "switch" not a "plug" in the beginning, then we update its profile to "basic-switch", so from user sides, its icon will not change.
There was a problem hiding this comment.
Changing the order here probably won't affect anything. It might make a difference in your testing, because you are removing and republishing the driver for test, this might do something, but for the already published driver, I don't think there is any guarantee that the order here makes any difference. I think a better solution would be to add a generic fingerprint that includes both clusters, and have it set the profile/icon you want there.
There was a problem hiding this comment.
We will add a new generic fingerprint.
There was a problem hiding this comment.
Hi,
Most devices using clusters 0x0702 and 0x0B04 should have the switch-power-energy profile.
They use cluster 0x0B04 to send Power with the Active Power attribute (0x050B) and cluster 0x0702 to send Total Energy with the CurrentSummationDelivered attribute (0x0000).
hongye-samsung
force-pushed
the
add_support_for_zigbee_mutli-switch
branch
from
9220461 to
1559923
Compare
| local magic_spell = {0x0004, 0x0000, 0x0001, 0x0005, 0x0007, 0xfffe} | ||
| device:send(read_attribute_function(device, clusters.Basic.ID, magic_spell)) |
There was a problem hiding this comment.
It would be ideal to add a test for this behavior.
qy-fang
force-pushed
the
add_support_for_zigbee_mutli-switch
branch
2 times, most recently
from
98d9782 to
9bcf1df
Compare
| if component_count >= 2 then | ||
| return true | ||
| else | ||
| return false | ||
| end |
There was a problem hiding this comment.
| if component_count >= 2 then | |
| return true | |
| else | |
| return false | |
| end | |
| return component_count >= 2 |
| label = name, | ||
| profile = child_profile, | ||
| parent_device_id = device.id, | ||
| parent_assigned_child_key = string.format("%02X", num_switch_server_eps), |
There was a problem hiding this comment.
So I think you used this number num_switch_server_eps in the case that the device has non-contiguous indices for its switch endpoints (i.e. endpoints 1, 3, and 5 are switches, but you want them to appear as 1, 2, and 3), but I think this would break your find_child function if it ever happened. I think it's probably safer to just use ep.id here. The label can still be whatever you like, though.
There was a problem hiding this comment.
Ah, actually it doesn't matter because you increment num_switch_server_eps regardless of whether or not the endpoint is a switch endpoint. But if you wanted the numbering to be contiguous you could change it.
| local main_endpoint = device:get_endpoint(clusters.OnOff.ID) | ||
| local updated_flag = false | ||
| if is_mcd_device(device) == false and device.network_type == device_lib.NETWORK_TYPE_ZIGBEE then | ||
| for _, ep in ipairs(device.zigbee_endpoints) do |
There was a problem hiding this comment.
you could probably use this index in the place of num_switch_server_eps rather than discard it
There was a problem hiding this comment.
Hi,
In my experience with various multi-component device manufacturers, the best way to identify the main endpoint component is to use the value returned by device.fingerprinted_endpoint_id.
Regards
qy-fang
force-pushed
the
add_support_for_zigbee_mutli-switch
branch
from
9bcf1df to
6a47f9b
Compare
| device:try_update_metadata({profile="basic-switch"}) | ||
| updated_flag = true | ||
| device:set_find_child(find_child) |
There was a problem hiding this comment.
Is there a reason you're switching the main device to a basic switch? I figure it could still use whatever functionality we've fingerprinted it with.
There was a problem hiding this comment.
Some switches will match with "switch-power" or "switch-power-energy" profile, but we cannot make power & energy meter value right for every generic switch(some switch use private cluster to meter), so we update their profile to "basic-switch".
There was a problem hiding this comment.
If they use a private cluster for metering how would they fingerprint to the power-energy profile?
There was a problem hiding this comment.
I should explain more accurately. Some switch use private cluster to calibrate power meter value. For example, tuya use 0xe0001's attribute value as a power & electric coefficient, without that coefficient, we may get wrong power & energy meter value.
There was a problem hiding this comment.
That's my point. The code you're adding here should be generic and for devices that use standard clusters and standard attributes. Those devices should have no trouble keeping their power and energy capabilities if they've fingerprinted to them using an appropriate generic fingerprint. Treating them as basic switches because it won't work otherwise for Tuya devices is not generic behavior.
Tuya-specific behavior belongs in a Tuya-specific sub-driver.
There was a problem hiding this comment.
I get your point, but the code we adding here is not just for Tuya. We cannot get every manufacturer's power & electric coefficient so we want these multi-switch work as basic-switch in generic driver first.
Besides, only "no fingerprint to match" & "multi on/off cluster" & "none MCD device" will be effected with that code.
There was a problem hiding this comment.
I understand, but the effort here was to increase functionality for devices we don't have fingerprints for.
Current functionality for, say, a power strip that supports metering each outlet would be to have one metered switch controllable.
With this change, you gain the ability to control on/off of each endpoint, but you lose the metering.
It seems possible to make these changes such that we could keep metering on the main endpoint while adding on/off control to the remaining endpoints.
I understand that this generic solution would not work for the Tuya devices because they use a custom attribute, but it should work for any device using standard zigbee attributes. We poll the multiplier and divisors as part of the base driver's configure method.
If the Tuya devices do not work with the base generic driver, then the non-standard behavior should be accommodated in the sub-driver.
There was a problem hiding this comment.
I appreciate your comments, it will be grate to support more function.
We try your solutions and find that Aqara Z1 series' switch cannot meter power(power meter value is 0 in plugin), so if we just move this to tuya sub-driver, we will encounter this issue. We wonder your opinions.
There was a problem hiding this comment.
Device-specific departures from the zigbee standard are intended to be handled by sub-drivers when the device manufacturer submits the device through the WWST process.
If our base driver uses Zigbee standard behavior, and a device does not function fully with it, that's an issue with the device, (unless you've investigated and found that our driver is acting in a way not up to the standards).
But if a device fingerprints to our generic fingerprint for a metered switch and uses the simple metering cluster, we would expect to be able to report its power/energy consumption.
A device failing to operate using our base drivers is an argument for adding a sub-driver, not for removing functionality from the base driver.
There was a problem hiding this comment.
I understand, we decide to delete "update_profile", thank you for your advices!
pInksenberg
force-pushed
the
add_support_for_zigbee_mutli-switch
branch
2 times, most recently
from
8447586 to
18f796b
Compare
| lazy_load_if_possible("wallhero"), | ||
| lazy_load_if_possible("inovelli-vzm31-sn"), | ||
| lazy_load_if_possible("laisiao") | ||
| lazy_load_if_possible("laisiao"), |
pInksenberg
force-pushed
the
add_support_for_zigbee_mutli-switch
branch
from
1990b29 to
15faf3e
Compare
pInksenberg
force-pushed
the
add_support_for_zigbee_mutli-switch
branch
from
15faf3e to
e76cba8
Compare
pInksenberg
force-pushed
the
add_support_for_zigbee_mutli-switch
branch
from
e76cba8 to
83678e9
Compare
Now stock zigbee swtich edge driver supports generic zigbee multi-endpoint switches. SmartThingsCommunity/SmartThingsEdgeDrivers#1922 This driver is no longer needed for multi- endpoint switches.
9 participants
Check all that apply
Type of Change
Checklist
Description of Change
add support for generic zigbee multi-switch
Summary of Completed Tests