From e56e8873e010744c415d5aeec80346b6b7230173 Mon Sep 17 00:00:00 2001 From: Shubham Patil Date: Mon, 29 Jan 2024 16:03:54 +0530 Subject: [PATCH] Fix problems with string type attributes - Use the defined consts when creating attributes - Pass in the strlen and the max size of attribute when creating attribute - Allocate max-str-size plus the size for storing the string length when creating the attribute metadata --- RELEASE_NOTES.md | 6 ++++++ components/esp_matter/esp_matter_attribute.cpp | 17 ++++++++++++----- components/esp_matter/esp_matter_attribute.h | 7 +++++-- components/esp_matter/esp_matter_cluster.cpp | 8 ++++---- components/esp_matter/esp_matter_cluster.h | 9 +++++---- components/esp_matter/esp_matter_core.cpp | 17 ++++++++++++++--- components/esp_matter/esp_matter_core.h | 14 +++++++++----- components/esp_matter/esp_matter_feature.cpp | 2 +- components/esp_matter/esp_matter_feature.h | 4 ++-- 9 files changed, 58 insertions(+), 26 deletions(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 8a84c5545..2b97e27f8 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -1,3 +1,9 @@ +# 14-February-2024 + +- An optional argument, `max_val_size`, has been introduced to the `esp_matter::attribute::create()` API. + This argument is utilized specifically when creating attributes of the char string and long char string data types + to specify the maximum supported value size of an attribute. + # 29-January-2024 - Add a new parameter for esp_matter::client::connect() to set the CASESessionManager for finding or establishing the CASE sessions. diff --git a/components/esp_matter/esp_matter_attribute.cpp b/components/esp_matter/esp_matter_attribute.cpp index 2a335182f..7298bc400 100644 --- a/components/esp_matter/esp_matter_attribute.cpp +++ b/components/esp_matter/esp_matter_attribute.cpp @@ -14,6 +14,8 @@ #include #include +#include +#include static const char *TAG = "esp_matter_attribute"; @@ -183,7 +185,7 @@ attribute_t *create_node_label(cluster_t *cluster, char *value, uint16_t length) } return esp_matter::attribute::create(cluster, BasicInformation::Attributes::NodeLabel::Id, ATTRIBUTE_FLAG_WRITABLE | ATTRIBUTE_FLAG_NONVOLATILE, - esp_matter_char_str(value, length)); + esp_matter_char_str(value, length), k_max_node_label_length); } attribute_t *create_location(cluster_t *cluster, char *value, uint16_t length) @@ -908,7 +910,7 @@ attribute_t *create_node_label(cluster_t *cluster, char *value, uint16_t length) } return esp_matter::attribute::create(cluster, BridgedDeviceBasicInformation::Attributes::NodeLabel::Id, ATTRIBUTE_FLAG_WRITABLE | ATTRIBUTE_FLAG_NONVOLATILE, - esp_matter_char_str(value, length)); + esp_matter_char_str(value, length), k_max_node_label_length); } attribute_t *create_hardware_version(cluster_t *cluster, uint16_t value) @@ -3653,7 +3655,9 @@ attribute_t *create_mode_select_description(cluster_t *cluster, const char * val ESP_LOGE(TAG, "Could not create attribute, string length out of bound"); return NULL; } - return esp_matter::attribute::create(cluster, ModeSelect::Attributes::Description::Id, ATTRIBUTE_FLAG_NONE, esp_matter_char_str((char *)value, length)); + return esp_matter::attribute::create(cluster, ModeSelect::Attributes::Description::Id, ATTRIBUTE_FLAG_NONE, + esp_matter_char_str((char *)value, length), + k_max_mode_select_description_length); } attribute_t *create_standard_namespace(cluster_t *cluster, const nullable value) @@ -3709,7 +3713,8 @@ attribute_t *create_description(cluster_t *cluster, const char * value, uint16_t ESP_LOGE(TAG, "Could not create attribute, string length out of bound"); return NULL; } - return esp_matter::attribute::create(cluster, PowerSource::Attributes::Description::Id, ATTRIBUTE_FLAG_NONE, esp_matter_char_str((char *)value, length)); + return esp_matter::attribute::create(cluster, PowerSource::Attributes::Description::Id, ATTRIBUTE_FLAG_NONE, + esp_matter_char_str((char *)value, length), k_max_description_length); } attribute_t *create_wired_assessed_input_voltage(cluster_t *cluster, nullable value, nullable min, nullable max) @@ -3854,7 +3859,9 @@ attribute_t *create_bat_replacement_description(cluster_t *cluster, const char * ESP_LOGE(TAG, "Could not create attribute, string size out of bound"); return NULL; } - return esp_matter::attribute::create(cluster, PowerSource::Attributes::BatReplacementDescription::Id, ATTRIBUTE_FLAG_NONE, esp_matter_char_str((char *)value, length)); + return esp_matter::attribute::create(cluster, PowerSource::Attributes::BatReplacementDescription::Id, + ATTRIBUTE_FLAG_NONE, esp_matter_char_str((char *)value, length), + k_max_bat_replacement_description_length); } attribute_t *create_bat_common_designation(cluster_t *cluster, const uint8_t value, uint8_t min, uint8_t max) diff --git a/components/esp_matter/esp_matter_attribute.h b/components/esp_matter/esp_matter_attribute.h index f65eb3e45..9130965d5 100644 --- a/components/esp_matter/esp_matter_attribute.h +++ b/components/esp_matter/esp_matter_attribute.h @@ -14,7 +14,6 @@ #pragma once -#include #include namespace esp_matter { @@ -63,7 +62,7 @@ attribute_t *create_access_control_entries_per_fabric(cluster_t *cluster, uint16 } /* access_control */ namespace basic_information { -constexpr uint8_t k_max_node_label_length = 32; +constexpr uint8_t k_max_node_label_length = 32; namespace attribute { attribute_t *create_data_model_revision(cluster_t *cluster, uint16_t value); @@ -768,6 +767,9 @@ attribute_t *state_value(cluster_t *cluster, bool value); } /* boolean_state */ namespace localization_configuration { + +constexpr uint8_t k_max_active_locale_length = 35; + namespace attribute { attribute_t *create_active_locale(cluster_t *cluster, char *value, uint16_t length); attribute_t *create_supported_locales(cluster_t *cluster, uint8_t *value, uint16_t length, uint16_t count); @@ -861,6 +863,7 @@ constexpr uint8_t k_max_description_length = 60; constexpr uint8_t k_max_fault_count = 8; constexpr uint8_t k_max_designation_count = 20; constexpr uint8_t k_max_charge_faults_count = 16; +constexpr uint8_t k_max_bat_replacement_description_length = 60; namespace attribute { attribute_t *create_status(cluster_t *cluster, uint8_t value); diff --git a/components/esp_matter/esp_matter_cluster.cpp b/components/esp_matter/esp_matter_cluster.cpp index d40c710e2..b42b090fa 100644 --- a/components/esp_matter/esp_matter_cluster.cpp +++ b/components/esp_matter/esp_matter_cluster.cpp @@ -244,7 +244,7 @@ cluster_t *create(endpoint_t *endpoint, config_t *config, uint8_t flags) /* Attributes not managed internally */ if (config) { global::attribute::create_cluster_revision(cluster, config->cluster_revision); - attribute::create_node_label(cluster, config->node_label, sizeof(config->node_label)); + attribute::create_node_label(cluster, config->node_label, strlen(config->node_label)); } else { ESP_LOGE(TAG, "Config is NULL. Cannot add some attributes."); } @@ -928,7 +928,7 @@ cluster_t *create(endpoint_t *endpoint, config_t *config, uint8_t flags, uint32_ global::attribute::create_cluster_revision(cluster, config->cluster_revision); attribute::create_status(cluster, config->status); attribute::create_order(cluster, config->order, 0x00, 0xFF); - attribute::create_description(cluster, config->description, sizeof(config->description)); + attribute::create_description(cluster, config->description, strlen(config->description)); } else { ESP_LOGE(TAG, "Config is NULL. Cannot add some attributes."); } @@ -2686,7 +2686,7 @@ cluster_t *create(endpoint_t *endpoint, config_t *config, uint8_t flags) if (config) { /* Attributes not managed internally */ global::attribute::create_cluster_revision(cluster, config->cluster_revision); - attribute::create_active_locale(cluster, config->active_locale, sizeof(config->active_locale)); + attribute::create_active_locale(cluster, config->active_locale, strlen(config->active_locale)); /* Attributes managed internally */ attribute::create_supported_locales(cluster, NULL, 0, 0); @@ -2950,7 +2950,7 @@ cluster_t *create(endpoint_t *endpoint, config_t *config, uint8_t flags, uint32_ /** Attributes not managed internally **/ if (config) { global::attribute::create_cluster_revision(cluster, config->cluster_revision); - attribute::create_mode_select_description(cluster, config->mode_select_description, sizeof(config->mode_select_description)); + attribute::create_mode_select_description(cluster, config->mode_select_description, strlen(config->mode_select_description)); attribute::create_standard_namespace(cluster, config->standard_namespace); attribute::create_current_mode(cluster, config->current_mode); } else { diff --git a/components/esp_matter/esp_matter_cluster.h b/components/esp_matter/esp_matter_cluster.h index b4cf691b8..c0999a8cd 100644 --- a/components/esp_matter/esp_matter_cluster.h +++ b/components/esp_matter/esp_matter_cluster.h @@ -15,6 +15,7 @@ #pragma once #include +#include #include #include @@ -70,7 +71,7 @@ cluster_t *create(endpoint_t *endpoint, config_t *config, uint8_t flags); namespace basic_information { typedef struct config { uint16_t cluster_revision; - char node_label[32]; + char node_label[k_max_node_label_length + 1]; config() : cluster_revision(2), node_label{0} {} } config_t; @@ -231,7 +232,7 @@ typedef struct config { uint16_t cluster_revision; uint8_t status; uint8_t order; - char description[61]; + char description[k_max_description_length + 1]; feature::wired::config_t wired; feature::battery::config_t battery; feature::rechargeable::config_t rechargeable; @@ -633,7 +634,7 @@ cluster_t *create(endpoint_t *endpoint, config_t *config, uint8_t flags); namespace localization_configuration { typedef struct config { uint16_t cluster_revision; - char active_locale[35]; + char active_locale[k_max_active_locale_length + 1]; config() : cluster_revision(4), active_locale{0} {} } config_t; @@ -714,7 +715,7 @@ cluster_t *create(endpoint_t *endpoint, config_t *config, uint8_t flags); namespace mode_select { typedef struct config { uint16_t cluster_revision; - char mode_select_description[64]; + char mode_select_description[k_max_mode_select_description_length + 1]; const nullable standard_namespace; uint8_t current_mode; feature::on_off::config_t on_off; diff --git a/components/esp_matter/esp_matter_core.cpp b/components/esp_matter/esp_matter_core.cpp index 0feb60908..1e0175dbd 100644 --- a/components/esp_matter/esp_matter_core.cpp +++ b/components/esp_matter/esp_matter_core.cpp @@ -154,6 +154,9 @@ typedef struct _attribute { esp_matter_attr_bounds_t *bounds; EmberAfDefaultOrMinMaxAttributeValue default_value; uint16_t default_value_size; + // This is required when creating metadata for char string and long char string types of attributes. + // The size in the attribute metadata remains constant and is verified during write operations. + uint16_t max_val_size; attribute::callback_t override_callback; struct _attribute *next; } _attribute_t; @@ -617,8 +620,14 @@ esp_err_t enable(endpoint_t *endpoint) * when writing a longer string. */ if (attribute->val.type == ESP_MATTER_VAL_TYPE_CHAR_STRING || - attribute->val.type == ESP_MATTER_VAL_TYPE_LONG_CHAR_STRING) { - matter_attributes[attribute_index].size = attribute->val.val.a.s; + attribute->val.type == ESP_MATTER_VAL_TYPE_LONG_CHAR_STRING) { + // Once the metadata is created, the attribute size becomes fixed and cannot be modified thereafter. + // For string and long string types, the size should be the maximum size defined in the specification + // plus the size_for_storing_str_len. The length byte is 1 for char string and 2 for long char string. + // For example, the maximum size of the Node-Label in the basic information cluster is 32 bytes, + // and it is a char string. Therefore, the size should be (32 + 1). + uint16_t size_for_storing_str_len = attribute->val.val.a.t - attribute->val.val.a.s; + matter_attributes[attribute_index].size = attribute->max_val_size + size_for_storing_str_len; } matter_clusters[cluster_index].clusterSize += matter_attributes[attribute_index].size; @@ -1094,7 +1103,8 @@ esp_err_t factory_reset() } namespace attribute { -attribute_t *create(cluster_t *cluster, uint32_t attribute_id, uint8_t flags, esp_matter_attr_val_t val) +attribute_t *create(cluster_t *cluster, uint32_t attribute_id, uint8_t flags, esp_matter_attr_val_t val, + uint16_t max_val_size) { /* Find */ if (!cluster) { @@ -1122,6 +1132,7 @@ attribute_t *create(cluster_t *cluster, uint32_t attribute_id, uint8_t flags, es attribute->endpoint_id = current_cluster->endpoint_id; attribute->flags = flags; attribute->flags |= ATTRIBUTE_FLAG_EXTERNAL_STORAGE; + attribute->max_val_size = max_val_size; // After reboot, string and array are treated as Invalid. So need to store val.type and size of attribute value. attribute->val.type = val.type; diff --git a/components/esp_matter/esp_matter_core.h b/components/esp_matter/esp_matter_core.h index d7d95e75d..feb598da1 100644 --- a/components/esp_matter/esp_matter_core.h +++ b/components/esp_matter/esp_matter_core.h @@ -425,15 +425,19 @@ namespace attribute { * * This will create a new attribute and add it to the cluster. * - * @param[in] cluster Cluster handle. - * @param[in] attribute_id Attribute ID for the attribute. - * @param[in] flags Bitmap of `attribute_flags_t`. - * @param[in] val Default type and value of the attribute. Use appropriate elements as per the value type. + * @param[in] cluster Cluster handle. + * @param[in] attribute_id Attribute ID for the attribute. + * @param[in] flags Bitmap of `attribute_flags_t`. + * @param[in] val Default type and value of the attribute. Use appropriate elements as per the value type. + * @param[in] max_val_size For attributes of type char string and long char string, the size should correspond to the + * maximum size defined in the specification. However, for other types of attributes, this + * parameter remains unused, and therefore the default value is set to 0 * * @return Attribute handle on success. * @return NULL in case of failure. */ -attribute_t *create(cluster_t *cluster, uint32_t attribute_id, uint8_t flags, esp_matter_attr_val_t val); +attribute_t *create(cluster_t *cluster, uint32_t attribute_id, uint8_t flags, esp_matter_attr_val_t val, + uint16_t max_val_size = 0); /** Get attribute * diff --git a/components/esp_matter/esp_matter_feature.cpp b/components/esp_matter/esp_matter_feature.cpp index c05d6ba04..c9f2e6d22 100644 --- a/components/esp_matter/esp_matter_feature.cpp +++ b/components/esp_matter/esp_matter_feature.cpp @@ -193,7 +193,7 @@ esp_err_t add(cluster_t *cluster, config_t *config) update_feature_map(cluster, get_id()); /* Attributes not managed internally */ - attribute::create_bat_replacement_description(cluster, config->bat_replacement_description, sizeof(config->bat_replacement_description)); + attribute::create_bat_replacement_description(cluster, config->bat_replacement_description, strlen(config->bat_replacement_description)); attribute::create_bat_quantity(cluster, config->bat_quantity, 0, 255); } else { ESP_LOGE(TAG, "Cluster shall support Battery feature"); diff --git a/components/esp_matter/esp_matter_feature.h b/components/esp_matter/esp_matter_feature.h index ba153b5eb..17c9a1d1e 100644 --- a/components/esp_matter/esp_matter_feature.h +++ b/components/esp_matter/esp_matter_feature.h @@ -14,8 +14,8 @@ #pragma once -#include #include +#include #define ESP_MATTER_NONE_FEATURE_ID 0x0000 @@ -87,7 +87,7 @@ esp_err_t add(cluster_t *cluster, config_t *config); // Replaceable feature one must add Battery feature first. namespace replaceable { typedef struct config { - char bat_replacement_description[61]; + char bat_replacement_description[k_max_bat_replacement_description_length + 1]; uint8_t bat_quantity; config(): bat_replacement_description{0}, bat_quantity(0) {} } config_t;