Skip to content

Conversation

@ThatMishakov
Copy link

@ThatMishakov ThatMishakov commented Oct 11, 2025

This should add full support for scenes (saving OnOff attribute of relay clusters), identify cluster (needed for touchlink and I think it's mandatory) on relay endpoints, scenes, Basic cluster on all endpoints, and set the right simple description device IDs for the relay clusters.

The identify cluster either toggles the relay output, or blinks the indicator LEDs on the switches that have them. Since I only have relays, I haven't been able to test that, so if someone could test it, it would be appreciated.

Some considerations:

  • I haven't tested it on 4 gang switches, but I think the default 32 clusters setting might not be enough on devices with many inputs and outputs (ZCL_CLUSTER_NUM_MAX in stack_cfg.h). This changes it to 48.
  • I think that 8 scenes is very little for switches, since if I'm not mistaken, each endpoint uses 1 table entry per scene, so in a 4 gang switch you will be left with 2 scenes if you want to control all the 4 outputs (ZCL_SCENE_TABLE_NUM). I have tried it with the size of 32, and it seemed to work just fine, so perhaps that could also be adjusted (?)
  • The spec is not very clear to me, but I think identify is also mandatory on the switch endpoints, but I don't really know what would be the best way to implement it.
  • On a separate note, I think Basic cluster is also needed for each endpoint (I have it implemented in a different branch together with touchlink changes, but I couldn't get touchlink to work yet...) I've changed my mind, I think this can also be added (?)
  • All of my relays are running on the new SDK, so I haven't really tried the main branch with these changes, besides flashing one of my test relays

SETUP_ATTR(9, ZCL_ATTRID_BASIC_DATE_CODE, ZCL_DATA_TYPE_CHAR_STR, ACCESS_CONTROL_READ, dateCode);
SETUP_ATTR(10, ZCL_ATTRID_GLOBAL_CLUSTER_REVISION, ZCL_DATA_TYPE_UINT16, ACCESS_CONTROL_READ, zcl_attr_global_clusterRevision);
SETUP_ATTR(11, ZCL_ATTRID_BASIC_DEVICE_CONFIG, ZCL_DATA_TYPE_LONG_CHAR_STR, ACCESS_CONTROL_READ | ACCESS_CONTROL_WRITE, device_config_str);
//SETUP_ATTR(7, ZCL_ATTRID_BASIC_DEV_ENABLED, ZCL_DATA_TYPE_BOOLEAN, ACCESS_CONTROL_READ | ACCESS_CONTROL_WRITE, cluster->deviceEnable);
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you remove the DeviceEnabled attribute it because it doesn't work? I tried it and it didn't work for me either, despite it is seemingly implemented in the SDK: https://github.com/telink-semi/telink_zigbee_sdk/blob/V3.6.8.6/tl_zigbee_sdk/zigbee/zcl/zcl.c#L834

I can imagine it could useful be in rare situations, where you want to stop a device from doing anything triggered by Zigbee without removing it from the network, for example because it's installed in the wall and you cannot power it off, and you want to avoid resetting it because don't want to reconfigure it from scratch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I guess it didn't work because the SDK doesn't store it, it would have to be implemented by us.

Copy link
Author

@ThatMishakov ThatMishakov Oct 11, 2025

Choose a reason for hiding this comment

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

I've commented it out since this attribute was per-endpoint if I understood it correctly, and it didn't make sense to make it global, and I also thought it was not used (I didn't know it was actually implemented)

This where the SDK uses it https://github.com/telink-semi/telink_zigbee_sdk/blob/master/tl_zigbee_sdk/zigbee/zcl/zcl.c#L817

But it can totally be implemented

Comment on lines 231 to +245

basic_cluster_add_to_endpoint(&basic_cluster, &endpoints[0]);
basic_cluster_populate(&basic_cluster);
zigbee_endpoint_add_cluster(&endpoints[0], 0, ZCL_CLUSTER_OTA);

for (int index = 0; index < switch_clusters_cnt; index++)
{
zigbee_endpoint_init(&endpoints[index], HA_DEV_ONOFF_SWITCH);
switch_cluster_add_to_endpoint(&switch_clusters[index], &endpoints[index]);
}
for (int index = 0; index < relay_clusters_cnt; index++)
{
relay_clusters[index].scene_cluster = scene_clusters + index;
scene_clusters[index].relay_cluster = relay_clusters + index;

zigbee_endpoint_init(&endpoints[index], HA_DEV_ONOFF_OUTPUT);
Copy link
Contributor

@marazmarci marazmarci Oct 23, 2025

Choose a reason for hiding this comment

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

When I built your branch, it bricked my 3-gang device, so I went through the commits and found that commit 07a2970 caused it. I also found a fix for it, and now my device boots up. (Disclaimer: I didn't yet test it further than that)

I found two problems:

  1. The endpoint index passed to the second call to zigbee_endpoint_init needs to be incremented by switch_clusters_cnt, so the same endpoint doesn't get initialized twice.
  2. Clusters cannot be added to uninitialized endpoints, so the zigbee_endpoint_init calls need to be moved before adding the clusters.

Here's a suggestion that fixes these issues:

Suggested change
basic_cluster_add_to_endpoint(&basic_cluster, &endpoints[0]);
basic_cluster_populate(&basic_cluster);
zigbee_endpoint_add_cluster(&endpoints[0], 0, ZCL_CLUSTER_OTA);
for (int index = 0; index < switch_clusters_cnt; index++)
{
zigbee_endpoint_init(&endpoints[index], HA_DEV_ONOFF_SWITCH);
switch_cluster_add_to_endpoint(&switch_clusters[index], &endpoints[index]);
}
for (int index = 0; index < relay_clusters_cnt; index++)
{
relay_clusters[index].scene_cluster = scene_clusters + index;
scene_clusters[index].relay_cluster = relay_clusters + index;
zigbee_endpoint_init(&endpoints[index], HA_DEV_ONOFF_OUTPUT);
for (int index = 0; index < switch_clusters_cnt; index++)
{
zigbee_endpoint_init(&endpoints[index], HA_DEV_ONOFF_SWITCH);
}
for (int index = 0; index < relay_clusters_cnt; index++)
{
zigbee_endpoint_init(&endpoints[switch_clusters_cnt + index], HA_DEV_ONOFF_OUTPUT);
}
basic_cluster_populate(&basic_cluster);
zigbee_endpoint_add_cluster(&endpoints[0], 0, ZCL_CLUSTER_OTA);
for (int index = 0; index < switch_clusters_cnt; index++)
{
switch_cluster_add_to_endpoint(&switch_clusters[index], &endpoints[index]);
}
for (int index = 0; index < relay_clusters_cnt; index++)
{
relay_clusters[index].scene_cluster = scene_clusters + index;
scene_clusters[index].relay_cluster = relay_clusters + index;

@marazmarci
Copy link
Contributor

The On With Timed Off changes will need to be rewritten because in the HAL PR #178, the polling-based logic is replaced with a callback-based approach, and the logic in this PR relies on polling for counting the remaining time.

Probably the way to go would be to use "tasks", but I don't yet know how they work in the Telink SDK, so I cannot provide more details now.

@ThatMishakov
Copy link
Author

The On With Timed Off changes will need to be rewritten because in the HAL PR #178, the polling-based logic is replaced with a callback-based approach, and the logic in this PR relies on polling for counting the remaining time.

Probably the way to go would be to use "tasks", but I don't yet know how they work in the Telink SDK, so I cannot provide more details now.

I'll take a look at it, the last time I've looked at the timers it didn't look very difficult with the Telink SDK, although I'm not sure how is the Silabs' SDK

Also, I'll fix the issue with the clusters, I think I've picked the wrong commit when rebasing since it worked well in my branch

@marazmarci
Copy link
Contributor

I'll take a look at it, the last time I've looked at the timers it didn't look very difficult with the Telink SDK, although I'm not sure how is the Silabs' SDK

Pro tip: check out hal_tasks_schedule on the hal branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants