Skip to content

fix(zigbeeEP): review of names and memory allocation #11199

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

Merged
merged 17 commits into from
Apr 5, 2025
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 23 additions & 27 deletions libraries/Zigbee/src/ZigbeeEP.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ ZigbeeEP::ZigbeeEP(uint8_t endpoint) {
_ep_config.endpoint = 0;
_cluster_list = nullptr;
_on_identify = nullptr;
_read_model[0] = '\0';
_read_manufacturer[0] = '\0';
_time_status = 0;
if (!lock) {
lock = xSemaphoreCreateBinary();
Expand All @@ -33,16 +35,24 @@ void ZigbeeEP::setVersion(uint8_t version) {
}

bool ZigbeeEP::setManufacturerAndModel(const char *name, const char *model) {
// Allocate a new array of size length + 2 (1 for the length, 1 for null terminator)
// This memory space can't be deleted or overwritten, therefore using static
static char zb_name[ZB_MAX_NAME_LENGTH + 2];
static char zb_model[ZB_MAX_NAME_LENGTH + 2];

// Convert manufacturer to ZCL string
size_t name_length = strlen(name);
size_t model_length = strlen(model);
if (name_length > 32 || model_length > 32) {
if (name_length > ZB_MAX_NAME_LENGTH || model_length > ZB_MAX_NAME_LENGTH) {
log_e("Manufacturer or model name is too long");
return false;
}
// Allocate a new array of size length + 2 (1 for the length, 1 for null terminator)
char *zb_name = new char[name_length + 2];
char *zb_model = new char[model_length + 2];
// Get and check the basic cluster
esp_zb_attribute_list_t *basic_cluster = esp_zb_cluster_list_get_cluster(_cluster_list, ESP_ZB_ZCL_CLUSTER_ID_BASIC, ESP_ZB_ZCL_CLUSTER_SERVER_ROLE);
if (basic_cluster == nullptr) {
log_e("Failed to get basic cluster");
return false;
}
// Store the length as the first element
zb_name[0] = static_cast<char>(name_length); // Cast size_t to char
zb_model[0] = static_cast<char>(model_length);
Expand All @@ -52,9 +62,7 @@ bool ZigbeeEP::setManufacturerAndModel(const char *name, const char *model) {
// Null-terminate the array
zb_name[name_length + 1] = '\0';
zb_model[model_length + 1] = '\0';

// Get the basic cluster and update the manufacturer and model attributes
esp_zb_attribute_list_t *basic_cluster = esp_zb_cluster_list_get_cluster(_cluster_list, ESP_ZB_ZCL_CLUSTER_ID_BASIC, ESP_ZB_ZCL_CLUSTER_SERVER_ROLE);
// Update the manufacturer and model attributes
esp_err_t ret_name = esp_zb_basic_cluster_add_attr(basic_cluster, ESP_ZB_ZCL_ATTR_BASIC_MANUFACTURER_NAME_ID, (void *)zb_name);
if (ret_name != ESP_OK) {
log_e("Failed to set manufacturer: 0x%x: %s", ret_name, esp_err_to_name(ret_name));
Expand All @@ -63,8 +71,6 @@ bool ZigbeeEP::setManufacturerAndModel(const char *name, const char *model) {
if (ret_model != ESP_OK) {
log_e("Failed to set model: 0x%x: %s", ret_model, esp_err_to_name(ret_model));
}
delete[] zb_name;
delete[] zb_model;
return ret_name == ESP_OK && ret_model == ESP_OK;
}

Expand Down Expand Up @@ -163,10 +169,7 @@ char *ZigbeeEP::readManufacturer(uint8_t endpoint, uint16_t short_addr, esp_zb_i
read_req.attr_number = ZB_ARRAY_LENTH(attributes);
read_req.attr_field = attributes;

if (_read_manufacturer != nullptr) {
free(_read_manufacturer);
}
_read_manufacturer = nullptr;
_read_manufacturer[0] = '\0';

esp_zb_lock_acquire(portMAX_DELAY);
esp_zb_zcl_read_attr_cmd_req(&read_req);
Expand Down Expand Up @@ -201,10 +204,7 @@ char *ZigbeeEP::readModel(uint8_t endpoint, uint16_t short_addr, esp_zb_ieee_add
read_req.attr_number = ZB_ARRAY_LENTH(attributes);
read_req.attr_field = attributes;

if (_read_model != nullptr) {
free(_read_model);
}
_read_model = nullptr;
_read_model[0] = '\0';

esp_zb_lock_acquire(portMAX_DELAY);
esp_zb_zcl_read_attr_cmd_req(&read_req);
Expand Down Expand Up @@ -245,20 +245,16 @@ void ZigbeeEP::zbReadBasicCluster(const esp_zb_zcl_attribute_t *attribute) {
/* Basic cluster attributes */
if (attribute->id == ESP_ZB_ZCL_ATTR_BASIC_MANUFACTURER_NAME_ID && attribute->data.type == ESP_ZB_ZCL_ATTR_TYPE_CHAR_STRING && attribute->data.value) {
zbstring_t *zbstr = (zbstring_t *)attribute->data.value;
char *string = (char *)malloc(zbstr->len + 1);
memcpy(string, zbstr->data, zbstr->len);
string[zbstr->len] = '\0';
log_i("Peer Manufacturer is \"%s\"", string);
_read_manufacturer = string;
memcpy(_read_manufacturer, zbstr->data, zbstr->len);
_read_manufacturer[zbstr->len] = '\0';
log_i("Peer Manufacturer is \"%s\"", _read_manufacturer);
xSemaphoreGive(lock);
}
if (attribute->id == ESP_ZB_ZCL_ATTR_BASIC_MODEL_IDENTIFIER_ID && attribute->data.type == ESP_ZB_ZCL_ATTR_TYPE_CHAR_STRING && attribute->data.value) {
zbstring_t *zbstr = (zbstring_t *)attribute->data.value;
char *string = (char *)malloc(zbstr->len + 1);
memcpy(string, zbstr->data, zbstr->len);
string[zbstr->len] = '\0';
log_i("Peer Model is \"%s\"", string);
_read_model = string;
memcpy(_read_model, zbstr->data, zbstr->len);
_read_model[zbstr->len] = '\0';
log_i("Peer Model is \"%s\"", _read_model);
xSemaphoreGive(lock);
}
}
Expand Down
8 changes: 6 additions & 2 deletions libraries/Zigbee/src/ZigbeeEP.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ typedef enum {
/* Zigbee End Device Class */
class ZigbeeEP {
public:
// constants and limits
static constexpr size_t ZB_MAX_NAME_LENGTH = 32;

// constructors and destructor
ZigbeeEP(uint8_t endpoint = 10);
~ZigbeeEP() {}

Expand Down Expand Up @@ -138,8 +142,8 @@ class ZigbeeEP {
}

private:
char *_read_manufacturer;
char *_read_model;
char _read_manufacturer[ZB_MAX_NAME_LENGTH + 1]; // + 1 for the extra '\0'
char _read_model[ZB_MAX_NAME_LENGTH + 1]; // + 1 for the extra '\0'
void (*_on_identify)(uint16_t time);
time_t _read_time;
int32_t _read_timezone;
Expand Down
Loading