diff --git a/components/esp_matter/data_model/esp_matter_attribute_utils.cpp b/components/esp_matter/data_model/esp_matter_attribute_utils.cpp index 219ffd11e..e7531cfa8 100644 --- a/components/esp_matter/data_model/esp_matter_attribute_utils.cpp +++ b/components/esp_matter/data_model/esp_matter_attribute_utils.cpp @@ -636,23 +636,18 @@ esp_err_t update(uint16_t endpoint_id, uint32_t cluster_id, uint32_t attribute_i attribute_t *attr = get(endpoint_id, cluster_id, attribute_id); ESP_RETURN_ON_FALSE(attr, ESP_ERR_INVALID_ARG, TAG, "Failed to get attribute handle"); - /* Take lock if not already taken */ - lock::status_t lock_status = lock::chip_stack_lock(portMAX_DELAY); - VerifyOrReturnError(lock_status != lock::FAILED, ESP_FAIL, ESP_LOGE(TAG, "Could not get task context")); + lock::ScopedChipStackLock lock(portMAX_DELAY); /* Here, the val_print function gets called on attribute write.*/ attribute::val_print(endpoint_id, cluster_id, attribute_id, val, false); - esp_err_t err = attribute::set_val_internal(attr, val); + esp_err_t err = attribute::set_val(attr, val); if (err == ESP_OK) { data_model::provider::get_instance().Temporary_ReportAttributeChanged( chip::app::AttributePathParams(endpoint_id, cluster_id, attribute_id)); } else if (err == ESP_ERR_NOT_FINISHED) { err = ESP_OK; } - if (lock_status == lock::SUCCESS) { - lock::chip_stack_unlock(); - } - return err; + return ESP_OK; } esp_err_t report(uint16_t endpoint_id, uint32_t cluster_id, uint32_t attribute_id, esp_matter_attr_val_t *val) @@ -661,14 +656,11 @@ esp_err_t report(uint16_t endpoint_id, uint32_t cluster_id, uint32_t attribute_i attribute_t *attr = get(endpoint_id, cluster_id, attribute_id); ESP_RETURN_ON_FALSE(attr, ESP_ERR_INVALID_ARG, TAG, "Failed to get attribute handle"); - /* Take lock if not already taken */ - lock::status_t lock_status = lock::chip_stack_lock(portMAX_DELAY); - VerifyOrReturnError(lock_status != lock::FAILED, ESP_FAIL, ESP_LOGE(TAG, "Could not get task context")); - + lock::ScopedChipStackLock lock(portMAX_DELAY); /* Here, the val_print function gets called on attribute write.*/ attribute::val_print(endpoint_id, cluster_id, attribute_id, val, false); - esp_err_t err = attribute::set_val_internal(attr, val, false); + esp_err_t err = attribute::set_val(attr, val, false); if (err == ESP_OK) { /* Report attribute */ MatterReportingAttributeChangeCallback(endpoint_id, cluster_id, attribute_id); @@ -676,9 +668,6 @@ esp_err_t report(uint16_t endpoint_id, uint32_t cluster_id, uint32_t attribute_i err = ESP_OK; } - if (lock_status == lock::SUCCESS) { - lock::chip_stack_unlock(); - } return ESP_OK; } diff --git a/components/esp_matter/data_model/esp_matter_data_model.cpp b/components/esp_matter/data_model/esp_matter_data_model.cpp index e235defa5..84cb26536 100644 --- a/components/esp_matter/data_model/esp_matter_data_model.cpp +++ b/components/esp_matter/data_model/esp_matter_data_model.cpp @@ -981,13 +981,11 @@ static esp_err_t set_val_via_write_attribute(uint16_t endpoint_id, uint32_t clus chip::app::AttributeValueDecoder decoder(reader, subjectDescriptor); // we need to ensure locks are in place to avoid assertion errors - esp_matter::lock::chip_stack_lock(portMAX_DELAY); + esp_matter::lock::ScopedChipStackLock lock(portMAX_DELAY); chip::app::DataModel::ActionReturnStatus status = esp_matter::data_model::provider::get_instance().WriteAttribute(request, decoder); - esp_matter::lock::chip_stack_unlock(); - if (status.IsError()) { chip::app::DataModel::ActionReturnStatus::StringStorage storage; ESP_LOGE(TAG, "WriteAttribute failed with status: %s", status.c_str(storage)); diff --git a/components/esp_matter/esp_matter_core.cpp b/components/esp_matter/esp_matter_core.cpp index 074587bb4..7e3763df5 100644 --- a/components/esp_matter/esp_matter_core.cpp +++ b/components/esp_matter/esp_matter_core.cpp @@ -141,7 +141,7 @@ FabricDelegateImpl s_fabric_delegate; namespace lock { #define DEFAULT_TICKS (500 / portTICK_PERIOD_MS) /* 500 ms in ticks */ -status_t chip_stack_lock(uint32_t ticks_to_wait) +status_t ScopedChipStackLock::chip_stack_lock(uint32_t ticks_to_wait) { #if CHIP_STACK_LOCK_TRACKING_ENABLED VerifyOrReturnValue(!PlatformMgr().IsChipStackLockedByCurrentThread(), ALREADY_TAKEN); @@ -160,7 +160,7 @@ status_t chip_stack_lock(uint32_t ticks_to_wait) return FAILED; } -esp_err_t chip_stack_unlock() +esp_err_t ScopedChipStackLock::chip_stack_unlock() { PlatformMgr().UnlockChipStack(); return ESP_OK; diff --git a/components/esp_matter/esp_matter_core.h b/components/esp_matter/esp_matter_core.h index 75b9b9a38..72d28d465 100644 --- a/components/esp_matter/esp_matter_core.h +++ b/components/esp_matter/esp_matter_core.h @@ -109,28 +109,69 @@ typedef enum status { SUCCESS, } status_t; -/** Stack lock +/** + * @brief RAII-style scoped lock for the Matter (CHIP) stack. * - * This API should be called before calling any upstream APIs. + * This class provides automatic locking and unlocking of the Matter stack using the + * RAII (Resource Acquisition Is Initialization) pattern. The lock is acquired when + * the object is constructed and automatically released when it goes out of scope. * - * @param[in] ticks_to_wait number of ticks to wait for trying to take the lock. Accepted values: 0 to portMAX_DELAY. + * Use this class whenever you need to access Matter stack APIs from a non-Matter context + * (e.g., from application tasks, callbacks, or interrupt handlers). The Matter stack is + * single-threaded and requires synchronization when accessed from external contexts. * - * @return FAILED if the lock was not taken within the specified ticks. - * @return ALREADY_TAKEN if the lock was already taken by the same task context. - * @return SUCCESS if the lock was taken successfully. + * @section advantages Advantages + * - **Exception Safety**: The lock is automatically released even if an exception occurs + * or the function returns early, preventing deadlocks. + * - **Simplified Code**: No need to manually call lock/unlock functions or worry about + * missing unlock calls in error paths. + * - **Scope-based Lifetime**: The lock duration is clearly defined by the variable's scope, + * making the code easier to read and maintain. + * - **Copy Prevention**: The class is non-copyable to prevent accidental lock duplication + * which could lead to undefined behavior. + * + * @note Always use the smallest possible scope for the lock to minimize blocking time. */ -status_t chip_stack_lock(uint32_t ticks_to_wait); +class ScopedChipStackLock { +public: + ScopedChipStackLock(uint32_t ticks_to_wait) { + status = chip_stack_lock(ticks_to_wait); + } + ~ScopedChipStackLock() { + if (status == SUCCESS) { + chip_stack_unlock(); + } + } +private: + ScopedChipStackLock(const ScopedChipStackLock &) = delete; + ScopedChipStackLock &operator=(const ScopedChipStackLock &) = delete; + ScopedChipStackLock(ScopedChipStackLock &&) = default; + ScopedChipStackLock &operator=(ScopedChipStackLock &&) = default; + status_t status; -/** Stack unlock - * - * This API should be called after the upstream APIs have been done calling. - * - * @note: This should only be called if `chip_stack_lock()` returns `SUCCESS`. - * - * @return ESP_OK on success. - * @return error in case of failure. - */ -esp_err_t chip_stack_unlock(); + /** Stack lock + * + * This API should be called before calling any upstream APIs. + * + * @param[in] ticks_to_wait number of ticks to wait for trying to take the lock. Accepted values: 0 to portMAX_DELAY. + * + * @return FAILED if the lock was not taken within the specified ticks. + * @return ALREADY_TAKEN if the lock was already taken by the same task context. + * @return SUCCESS if the lock was taken successfully. + */ + status_t chip_stack_lock(uint32_t ticks_to_wait); + + /** Stack unlock + * + * This API should be called after the upstream APIs have been done calling. + * + * @note: This should only be called if `chip_stack_lock()` returns `SUCCESS`. + * + * @return ESP_OK on success. + * @return error in case of failure. + */ + esp_err_t chip_stack_unlock(); +}; } /* lock */ diff --git a/components/esp_matter_bridge/esp_matter_bridge.cpp b/components/esp_matter_bridge/esp_matter_bridge.cpp index d8e5f5e55..d18a72f45 100644 --- a/components/esp_matter_bridge/esp_matter_bridge.cpp +++ b/components/esp_matter_bridge/esp_matter_bridge.cpp @@ -252,9 +252,7 @@ static esp_err_t cluster_server_init(endpoint_t *endpoint) return ESP_ERR_INVALID_ARG; } ESP_LOGI(TAG, "Cluster server init for the new added endpoint"); - /* Take lock if not already taken */ - lock::status_t lock_status = lock::chip_stack_lock(portMAX_DELAY); - VerifyOrReturnError(lock_status != lock::FAILED, ESP_FAIL, ESP_LOGE(TAG, "Could not get task context")); + lock::ScopedChipStackLock lock(portMAX_DELAY); cluster_t *cluster = cluster::get_first(endpoint); while (cluster) { uint8_t flags = cluster::get_flags(cluster); @@ -271,9 +269,6 @@ static esp_err_t cluster_server_init(endpoint_t *endpoint) } cluster = cluster::get_next(cluster); } - if (lock_status == lock::SUCCESS) { - lock::chip_stack_unlock(); - } return ESP_OK; } diff --git a/docs/en/faq.rst b/docs/en/faq.rst index 38ce28441..9df7b0407 100644 --- a/docs/en/faq.rst +++ b/docs/en/faq.rst @@ -232,13 +232,12 @@ operations using the server's object, such as opening or closing the commissioni To address this, there are two possible approaches: -- Locking the Matter thread +- Using ScopedChipStackLock to lock the Matter thread :: - lock::chip_stack_lock(portMAX_DELAY); + lock::ScopedChipStackLock lock(portMAX_DELAY); ... // eg: access Matter attribute, open/close commissioning window. - lock::chip_stack_unlock(); - Scheduling the work on Matter thread diff --git a/examples/all_device_types_app/main/app_main.cpp b/examples/all_device_types_app/main/app_main.cpp index 2350fe063..b734f52c6 100644 --- a/examples/all_device_types_app/main/app_main.cpp +++ b/examples/all_device_types_app/main/app_main.cpp @@ -207,7 +207,7 @@ static void ElectricalMeasurementWorkHandler(intptr_t context) #ifdef CONFIG_ENABLE_ESP_DIAGNOSTICS_TRACE void diagnostic_init() { - lock::status_t lock_status = lock::chip_stack_lock(portMAX_DELAY); + lock::ScopedChipStackLock lock(portMAX_DELAY); LogProvider::LogProviderInit providerInit = { .endUserBuffer = endUserBuffer, .endUserBufferSize = CONFIG_END_USER_BUFFER_SIZE, @@ -215,9 +215,6 @@ void diagnostic_init() .retrievalBufferSize = CONFIG_RETRIEVAL_BUFFER_SIZE, }; logProvider.Init(providerInit); - if (lock_status == lock::SUCCESS) { - lock::chip_stack_unlock(); - } } #endif // CONFIG_ENABLE_ESP_DIAGNOSTICS_TRACE diff --git a/examples/bridge_apps/esp-now_bridge_light/main/app_espnow.cpp b/examples/bridge_apps/esp-now_bridge_light/main/app_espnow.cpp index aade7583b..ff2843941 100644 --- a/examples/bridge_apps/esp-now_bridge_light/main/app_espnow.cpp +++ b/examples/bridge_apps/esp-now_bridge_light/main/app_espnow.cpp @@ -47,9 +47,8 @@ static void espnow_ctrl_onoff(espnow_addr_t src_addr, bool status) ESP_LOGI(TAG, "Using bridge endpoint: %d", bridged_switch_endpoint_id); if (bridged_switch_endpoint_id != chip::kInvalidEndpointId) { - lock::chip_stack_lock(portMAX_DELAY); + lock::ScopedChipStackLock lock(portMAX_DELAY); client::cluster_update(bridged_switch_endpoint_id, &req_handle); - lock::chip_stack_unlock(); } else { ESP_LOGE(TAG, "Can't find endpoint for bridged device: " MACSTR, MAC2STR(src_addr)); } diff --git a/examples/controller/main/app_main.cpp b/examples/controller/main/app_main.cpp index d59954353..745b10859 100644 --- a/examples/controller/main/app_main.cpp +++ b/examples/controller/main/app_main.cpp @@ -99,9 +99,8 @@ extern "C" void app_main() ABORT_APP_ON_FAILURE(err == ESP_OK, ESP_LOGE(TAG, "Failed to start Matter, err:%d", err)); #if CONFIG_ESP_MATTER_COMMISSIONER_ENABLE - esp_matter::lock::chip_stack_lock(portMAX_DELAY); + esp_matter::lock::ScopedChipStackLock lock(portMAX_DELAY); esp_matter::controller::matter_controller_client::get_instance().init(112233, 1, 5580); esp_matter::controller::matter_controller_client::get_instance().setup_commissioner(); - esp_matter::lock::chip_stack_unlock(); #endif // CONFIG_ESP_MATTER_COMMISSIONER_ENABLE } diff --git a/examples/light_switch/main/app_driver.cpp b/examples/light_switch/main/app_driver.cpp index d0ff1c72d..612e1c246 100644 --- a/examples/light_switch/main/app_driver.cpp +++ b/examples/light_switch/main/app_driver.cpp @@ -310,9 +310,8 @@ static void app_driver_button_toggle_cb(void *arg, void *data) req_handle.command_path.mClusterId = OnOff::Id; req_handle.command_path.mCommandId = OnOff::Commands::Toggle::Id; - lock::chip_stack_lock(portMAX_DELAY); + lock::ScopedChipStackLock lock(portMAX_DELAY); client::cluster_update(switch_endpoint_id, &req_handle); - lock::chip_stack_unlock(); } app_driver_handle_t app_driver_switch_init()