espressif / esp-zigbee-sdk

Espressif Zigbee SDK
Apache License 2.0
156 stars 26 forks source link

esp_zb_af_simple_desc_1_1_t definition not correct (TZ-946) #368

Open rickou opened 3 months ago

rickou commented 3 months ago

Answers checklist.

IDF version.

v5.2.2

esp-zigbee-lib version.

3.2.0

esp-zboss-lib version.

3.2.0

Espressif SoC revision.

ESP32-C6

What is the expected behavior?

the definition of esp_zb_af_simple_desc_1_1_t is not correct. typedef struct esp_zb_af_simple_desc_1_1_s { uint8_t endpoint; /!< Endpoint / uint16_t app_profile_id; /!< Application profile identifier / uint16_t app_device_id; /!< Application device identifier / uint32_t app_device_version: 4; /!< Application device version / uint32_t reserved: 4; /!< Reserved / uint8_t app_input_cluster_count; /!< Application input cluster count / uint8_t app_output_cluster_count; /!< Application output cluster count / uint16_t app_cluster_list[2]; /!< Application input and output cluster list / } ESP_ZB_PACKED_STRUCT esp_zb_af_simple_desc_1_1_t;

What is the actual behavior?

app_cluster_list is only 2 cells..

Steps to reproduce.

it should be a list or something similar. best should be to have 2 separate lists for in and out clusters lists

More Information.

No response

xieqinan commented 3 months ago

@rickou ,

I don't think it is a bug. The app_cluster_list carries app_input_cluster_count + app_output_cluster_count cells in the stack. You can find the app_input_cluster_count at the beginning of the array, and the remaining cells are for app_output_cluster_count.

rickou commented 3 months ago

I understand well the concept and you're right, it not really a bug but a bad coded definition

only the definition is not good. uint16_t app_cluster_list[2]; /!< Application input and output cluster list */

should be uint16_t* app_cluster_list; /!< Application input and output cluster list */

The cluster order is still not clear (in then out, or out then in..) so, for me the best should be to have a definition like this:

uint16_t* app_cluster_in_list; /!< Application input cluster list */
uint16_t* app_cluster_out_list; /!< Application output cluster list */

it doesn't change anything on tthe behavior, but it is more clear.

Eric

xieqinan commented 2 months ago

@rickou ,

Thanks for your suggestion. I think it’s a great idea. However, it involves a breaking change, so we will consider implementing it in the next major version.