Merge branch 'bugfix/set_val' into 'main'

components/esp-matter: Fix the set_val call and add RAII lock.

See merge request app-frameworks/esp-matter!1350

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;
 }
 

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));

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;

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 */
 

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;
 }
 

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
 

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
 

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));
     }

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
 }

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()