Address review comments

examples/air-purifier-app/air-purifier-common/air-purifier-app.matter

@@ -1278,7 +1278,7 @@ cluster Thermostat = 513 {
     kWake = 4;
     kVacation = 5;
     kGoingToSleep = 6;
-    kUserDefined = 7;
+    kUserDefined = 254;
   }
 
   enum SetpointChangeSourceEnum : enum8 {

examples/all-clusters-app/all-clusters-common/all-clusters-app.matter

@@ -5007,7 +5007,7 @@ cluster Thermostat = 513 {
     kWake = 4;
     kVacation = 5;
     kGoingToSleep = 6;
-    kUserDefined = 7;
+    kUserDefined = 254;
   }
 
   enum SetpointChangeSourceEnum : enum8 {

examples/all-clusters-minimal-app/all-clusters-common/all-clusters-minimal-app.matter

@@ -3552,7 +3552,7 @@ cluster Thermostat = 513 {
     kWake = 4;
     kVacation = 5;
     kGoingToSleep = 6;
-    kUserDefined = 7;
+    kUserDefined = 254;
   }
 
   enum SetpointChangeSourceEnum : enum8 {

examples/chef/devices/rootnode_airpurifier_airqualitysensor_temperaturesensor_humiditysensor_thermostat_56de3d5f45.matter

@@ -1201,7 +1201,7 @@ cluster Thermostat = 513 {
     kWake = 4;
     kVacation = 5;
     kGoingToSleep = 6;
-    kUserDefined = 7;
+    kUserDefined = 254;
   }
 
   enum SetpointChangeSourceEnum : enum8 {

examples/chef/devices/rootnode_heatingcoolingunit_ncdGai1E5a.matter

@@ -1558,7 +1558,7 @@ cluster Thermostat = 513 {
     kWake = 4;
     kVacation = 5;
     kGoingToSleep = 6;
-    kUserDefined = 7;
+    kUserDefined = 254;
   }
 
   enum SetpointChangeSourceEnum : enum8 {

examples/chef/devices/rootnode_roomairconditioner_9cf3607804.matter

@@ -1141,7 +1141,7 @@ cluster Thermostat = 513 {
     kWake = 4;
     kVacation = 5;
     kGoingToSleep = 6;
-    kUserDefined = 7;
+    kUserDefined = 254;
   }
 
   enum SetpointChangeSourceEnum : enum8 {

examples/chef/devices/rootnode_thermostat_bm3fb8dhYi.matter

@@ -1361,7 +1361,7 @@ cluster Thermostat = 513 {
     kWake = 4;
     kVacation = 5;
     kGoingToSleep = 6;
-    kUserDefined = 7;
+    kUserDefined = 254;
   }
 
   enum SetpointChangeSourceEnum : enum8 {

examples/placeholder/linux/apps/app1/config.matter

@@ -4798,7 +4798,7 @@ cluster Thermostat = 513 {
     kWake = 4;
     kVacation = 5;
     kGoingToSleep = 6;
-    kUserDefined = 7;
+    kUserDefined = 254;
   }
 
   enum SetpointChangeSourceEnum : enum8 {
@@ -5148,7 +5148,7 @@ cluster Thermostat = 513 {
     kWake = 4;
     kVacation = 5;
     kGoingToSleep = 6;
-    kUserDefined = 7;
+    kUserDefined = 254;
   }
 
   enum SetpointChangeSourceEnum : enum8 {

examples/placeholder/linux/apps/app2/config.matter

@@ -4755,7 +4755,7 @@ cluster Thermostat = 513 {
     kWake = 4;
     kVacation = 5;
     kGoingToSleep = 6;
-    kUserDefined = 7;
+    kUserDefined = 254;
   }
 
   enum SetpointChangeSourceEnum : enum8 {
@@ -5105,7 +5105,7 @@ cluster Thermostat = 513 {
     kWake = 4;
     kVacation = 5;
     kGoingToSleep = 6;
-    kUserDefined = 7;
+    kUserDefined = 254;
   }
 
   enum SetpointChangeSourceEnum : enum8 {

examples/thermostat/linux/include/thermostat-delegate-impl.h

@@ -35,6 +35,7 @@ namespace Thermostat {
 
 static constexpr uint8_t kMaxNumberOfPresetTypes = 6;
 
+// TODO: #34556 Support multiple presets of each type.
 // We will support only one preset of each preset type.
 static constexpr uint8_t kMaxNumberOfPresetsOfEachType = 1;
 
@@ -83,8 +84,8 @@ class ThermostatDelegate : public Delegate
     uint8_t mNumberOfPresets;
 
     Structs::PresetTypeStruct::Type mPresetTypes[kMaxNumberOfPresetTypes];
-    PresetStructWithOwnedMembers mPresets[kMaxNumberOfPresetTypes * kMaxNumberOfPresetTypesOfEachType];
-    PresetStructWithOwnedMembers mPendingPresets[kMaxNumberOfPresetTypes * kMaxNumberOfPresetTypesOfEachType];
+    PresetStructWithOwnedMembers mPresets[kMaxNumberOfPresetTypes * kMaxNumberOfPresetsOfEachType];
+    PresetStructWithOwnedMembers mPendingPresets[kMaxNumberOfPresetTypes * kMaxNumberOfPresetsOfEachType];
 
     uint8_t mNextFreeIndexInPendingPresetsList;
     uint8_t mNextFreeIndexInPresetsList;

examples/thermostat/linux/thermostat-delegate-impl.cpp

@@ -49,7 +49,7 @@ bool PresetHandlesExistAndMatch(const PresetStructWithOwnedMembers & preset, con
 
 ThermostatDelegate::ThermostatDelegate()
 {
-    mNumberOfPresets                   = kMaxNumberOfPresetTypes * kMaxNumberOfPresetTypesOfEachType;
+    mNumberOfPresets                   = kMaxNumberOfPresetTypes * kMaxNumberOfPresetsOfEachType;
     mNextFreeIndexInPresetsList        = 0;
     mNextFreeIndexInPendingPresetsList = 0;
 
@@ -72,7 +72,7 @@ void ThermostatDelegate::InitializePresetTypes()
     for (PresetScenarioEnum presetScenario : presetScenarioEnumArray)
     {
         mPresetTypes[index].presetScenario  = presetScenario;
-        mPresetTypes[index].numberOfPresets = kMaxNumberOfPresetTypesOfEachType;
+        mPresetTypes[index].numberOfPresets = kMaxNumberOfPresetsOfEachType;
         mPresetTypes[index].presetTypeFeatures =
             (presetScenario == PresetScenarioEnum::kOccupied || presetScenario == PresetScenarioEnum::kUnoccupied)
             ? PresetTypeFeaturesBitmap::kAutomatic
@@ -193,6 +193,8 @@ CHIP_ERROR ThermostatDelegate::GetPendingPresetAtIndex(size_t index, PresetStruc
 
 CHIP_ERROR ThermostatDelegate::ApplyPendingPresets()
 {
+
+    // TODO: #34546 - Need to support deletion of presets that are removed from Presets.
     for (uint8_t indexInPendingPresets = 0; indexInPendingPresets < mNextFreeIndexInPendingPresetsList; indexInPendingPresets++)
     {
         const PresetStructWithOwnedMembers & pendingPreset = mPendingPresets[indexInPendingPresets];
@@ -215,6 +217,9 @@ CHIP_ERROR ThermostatDelegate::ApplyPendingPresets()
 
             mPresets[mNextFreeIndexInPresetsList] = pendingPreset;
 
+            // TODO: #34556 Since we support only one preset of each type, using the octet string containing the preset scenario
+            // suffices as the unique preset handle. Need to fix this to actually provide unique handles once multiple presets of
+            // each type are supported.
             const uint8_t handle[] = { static_cast<uint8_t>(pendingPreset.GetPresetScenario()) };
             mPresets[mNextFreeIndexInPresetsList].SetPresetHandle(DataModel::MakeNullable(ByteSpan(handle)));
             mNextFreeIndexInPresetsList++;

examples/thermostat/linux/thermostat-manager.cpp

@@ -42,6 +42,7 @@ using namespace chip::app::Clusters::Thermostat::Structs;
 using namespace chip::app::Clusters::Thermostat::Attributes;
 using namespace chip::app::Clusters::TemperatureMeasurement;
 using namespace chip::app::Clusters::TemperatureMeasurement::Attributes;
+using namespace Protocols::InteractionModel;
 
 using namespace chip::DeviceLayer;
 
@@ -60,6 +61,12 @@ ThermostatManager ThermostatManager::sThermostatMgr;
 
 namespace {
 
+CHIP_ERROR ChipErrorFromStatusCode(Status status)
+{
+    StatusIB statusIB(status);
+    return statusIB.ToChipError();
+}
+
 template <typename DecodableAttributeType>
 static void OnAttributeChangeReported(const ConcreteDataAttributePath & path, const DecodableAttributeType & value);
 
@@ -70,30 +77,30 @@ void OnAttributeChangeReported<MeasuredValue::TypeInfo::DecodableType>(const Con
     ClusterId clusterId = path.mClusterId;
     if (clusterId != TemperatureMeasurement::Id)
     {
-        ChipLogError(AppServer, "Attribute change reported for TemperatureMeasurement cluster on incorrect cluster id %u",
-                     clusterId);
+        ChipLogError(AppServer, "Attribute change reported for TemperatureMeasurement cluster on incorrect cluster id " ChipLogFormatMEI,
+                     ChipLogValueMEI(clusterId));
         return;
     }
 
     AttributeId attributeId = path.mAttributeId;
     if (attributeId != MeasuredValue::Id)
     {
-        ChipLogError(AppServer, "Attribute change reported for TemperatureMeasurement cluster for incorrect attribute %u",
-                     attributeId);
+        ChipLogError(AppServer, "Attribute change reported for TemperatureMeasurement cluster for incorrect attribute" ChipLogFormatMEI,
+                     ChipLogValueMEI(attributeId));
         return;
     }
 
     if (!value.IsNull())
     {
         ChipLogDetail(AppServer, "Attribute change reported for TemperatureMeasurement cluster - MeasuredValue is %d",
-                      static_cast<short>(value.Value()));
+                      value.Value());
     }
 }
 
 static void OnError(const ConcreteDataAttributePath * path, ChipError err)
 {
-    ChipLogError(AppServer, "Subscribing to cluster Id %u and attribute Id %u failed with error %" CHIP_ERROR_FORMAT,
-                 path->mClusterId, path->mAttributeId, err.Format());
+    ChipLogError(AppServer, "Subscribing to cluster Id " ChipLogFormatMEI " and attribute Id " ChipLogFormatMEI " failed with error %" CHIP_ERROR_FORMAT,
+                 ChipLogValueMEI(path->mClusterId), ChipLogValueMEI(path->mAttributeId), err.Format());
 }
 
 static void OnSubscriptionEstablished(const ReadClient & client, unsigned int value)
@@ -111,7 +118,7 @@ void SubscribeToAttribute(ClusterId clusterId, AttributeId attributeId, const Em
     SubscribeAttribute<DecodableAttributeType>(
         peer_device->GetExchangeManager(), peer_device->GetSecureSession().Value(), binding.remote, clusterId, attributeId,
         &OnAttributeChangeReported<DecodableAttributeType>, &OnError, 0, kMaxIntervalCeilingSeconds, &OnSubscriptionEstablished,
-        nullptr, true /* fabricFiltered */, true /* keepExistingSubscription */);
+        nullptr, true /* fabricFiltered */, false /* keepExistingSubscription */);
 }
 
 static void ThermostatBoundDeviceChangedHandler(const EmberBindingTableEntry & binding, OperationalDeviceProxy * peer_device,
@@ -183,7 +190,7 @@ CHIP_ERROR ThermostatManager::Init()
                  "mSystemMode: %u (%s) \n mRunningMode: %u (%s) \n mLocalTemperature: %d \n mOccupiedHeatingSetpoint: %d \n "
                  "mOccupiedCoolingSetpoint: %d"
                  "NumberOfPresets: %d",
-                 static_cast<uint8_t>(mSystemMode), SystemModeString(mSystemMode), static_cast<uint8_t>(mRunningMode),
+                 to_underlying(mSystemMode), SystemModeString(mSystemMode), to_underlying(mRunningMode),
                  RunningModeString(mRunningMode), mLocalTemperature, mOccupiedHeatingSetpoint, mOccupiedCoolingSetpoint,
                  GetNumberOfPresets());
 
@@ -229,21 +236,21 @@ void ThermostatManager::ThermostatClusterAttributeChangeHandler(AttributeId attr
     switch (attributeId)
     {
     case LocalTemperature::Id: {
-        mLocalTemperature = static_cast<int16_t>(Encoding::LittleEndian::Get16(value));
+        memcpy(&mLocalTemperature, value, size);
         ChipLogError(AppServer, "Local temperature changed to %d", mLocalTemperature);
         EvalThermostatState();
     }
     break;
 
     case OccupiedCoolingSetpoint::Id: {
-        mOccupiedCoolingSetpoint = static_cast<int16_t>(Encoding::LittleEndian::Get16(value));
+        memcpy(&mOccupiedCoolingSetpoint, value, size);
         ChipLogError(AppServer, "Cooling temperature changed to %d", mOccupiedCoolingSetpoint);
         EvalThermostatState();
     }
     break;
 
     case OccupiedHeatingSetpoint::Id: {
-        mOccupiedHeatingSetpoint = static_cast<int16_t>(Encoding::LittleEndian::Get16(value));
+        memcpy(&mOccupiedHeatingSetpoint, value, size);
         ChipLogError(AppServer, "Heating temperature changed to %d", mOccupiedHeatingSetpoint);
         EvalThermostatState();
     }
@@ -313,52 +320,43 @@ uint8_t ThermostatManager::GetNumberOfPresets()
 
 CHIP_ERROR ThermostatManager::SetSystemMode(SystemModeEnum systemMode)
 {
-    uint8_t systemModeValue = static_cast<uint8_t>(systemMode);
+    uint8_t systemModeValue = to_underlying(systemMode);
     if (mSystemMode == systemMode)
     {
         ChipLogDetail(AppServer, "Already in system mode: %u (%s)", systemModeValue, SystemModeString(systemMode));
         return CHIP_NO_ERROR;
     }
 
     ChipLogError(AppServer, "Setting system mode: %u (%s)", systemModeValue, SystemModeString(systemMode));
-    Protocols::InteractionModel::Status status = SystemMode::Set(kThermostatEndpoint, systemMode);
-
-    // TODO: CHIP_ERROR_WRITE_FAILED might not be the best error code to send
-    return (status == Protocols::InteractionModel::Status::Success) ? CHIP_NO_ERROR : CHIP_ERROR_WRITE_FAILED;
+    return ChipErrorFromStatusCode(SystemMode::Set(kThermostatEndpoint, systemMode));
 }
 
 CHIP_ERROR ThermostatManager::SetRunningMode(ThermostatRunningModeEnum runningMode)
 {
-    uint8_t runningModeValue = static_cast<uint8_t>(runningMode);
+    uint8_t runningModeValue = to_underlying(runningMode);
     if (mRunningMode == runningMode)
     {
         ChipLogDetail(AppServer, "Already in running mode: %u (%s)", runningModeValue, RunningModeString(runningMode));
         return CHIP_NO_ERROR;
     }
 
     ChipLogError(AppServer, "Setting running mode: %u (%s)", runningModeValue, RunningModeString(runningMode));
-    Protocols::InteractionModel::Status status = ThermostatRunningMode::Set(kThermostatEndpoint, runningMode);
-
-    // TODO: CHIP_ERROR_WRITE_FAILED might not be the best error code to send
-    return (status == Protocols::InteractionModel::Status::Success) ? CHIP_NO_ERROR : CHIP_ERROR_WRITE_FAILED;
+    return ChipErrorFromStatusCode(ThermostatRunningMode::Set(kThermostatEndpoint, runningMode));
 }
 
 CHIP_ERROR ThermostatManager::SetCurrentTemperature(int16_t temperature)
 {
-    Protocols::InteractionModel::Status status = LocalTemperature::Set(kThermostatEndpoint, temperature);
-    return (status == Protocols::InteractionModel::Status::Success) ? CHIP_NO_ERROR : CHIP_ERROR_WRITE_FAILED;
+    return ChipErrorFromStatusCode(LocalTemperature::Set(kThermostatEndpoint, temperature));
 }
 
 CHIP_ERROR ThermostatManager::SetCurrentHeatingSetPoint(int16_t heatingSetpoint)
 {
-    Protocols::InteractionModel::Status status = OccupiedHeatingSetpoint::Set(kThermostatEndpoint, heatingSetpoint);
-    return (status == Protocols::InteractionModel::Status::Success) ? CHIP_NO_ERROR : CHIP_ERROR_WRITE_FAILED;
+    return ChipErrorFromStatusCode(OccupiedHeatingSetpoint::Set(kThermostatEndpoint, heatingSetpoint));
 }
 
 CHIP_ERROR ThermostatManager::SetCurrentCoolingSetPoint(int16_t coolingSetpoint)
 {
-    Protocols::InteractionModel::Status status = OccupiedCoolingSetpoint::Set(kThermostatEndpoint, coolingSetpoint);
-    return (status == Protocols::InteractionModel::Status::Success) ? CHIP_NO_ERROR : CHIP_ERROR_WRITE_FAILED;
+    return ChipErrorFromStatusCode(OccupiedCoolingSetpoint::Set(kThermostatEndpoint, coolingSetpoint));
 }
 
 void ThermostatManager::EvalThermostatState()
@@ -367,7 +365,7 @@ void ThermostatManager::EvalThermostatState()
                  "Eval Thermostat Running Mode \n "
                  "mSystemMode: %u (%s) \n mRunningMode: %u (%s) \n mLocalTemperature: %d \n mOccupiedHeatingSetpoint: %d \n "
                  "mOccupiedCoolingSetpoint: %d",
-                 static_cast<uint8_t>(mSystemMode), SystemModeString(mSystemMode), static_cast<uint8_t>(mRunningMode),
+                 to_underlying(mSystemMode), SystemModeString(mSystemMode), to_underlying(mRunningMode),
                  RunningModeString(mRunningMode), mLocalTemperature, mOccupiedHeatingSetpoint, mOccupiedCoolingSetpoint);
 
     switch (mSystemMode)
@@ -495,8 +493,8 @@ void MatterPostAttributeChangeCallback(const ConcreteAttributePath & attributePa
     ChipLogProgress(AppServer, "Cluster callback: " ChipLogFormatMEI, ChipLogValueMEI(clusterId));
 
     ChipLogProgress(AppServer,
-                    "Attribute ID changed: " ChipLogFormatMEI " Endpoint: %d ClusterId: %d Type: %u Value: %u, length %u",
-                    ChipLogValueMEI(attributeId), attributePath.mEndpointId, clusterId, type, *value, size);
+                    "Attribute ID changed: " ChipLogFormatMEI " Endpoint: %d ClusterId: " ChipLogFormatMEI " Type: %u Value: %u, length %u",
+                    ChipLogValueMEI(attributeId), attributePath.mEndpointId, ChipLogValueMEI(clusterId), type, *value, size);
 
     ThermostatMgr().AttributeChangeHandler(attributePath.mEndpointId, clusterId, attributeId, value, size);
 }

examples/thermostat/nxp/zap/thermostat_matter_thread.matter

@@ -1952,7 +1952,7 @@ cluster Thermostat = 513 {
     kWake = 4;
     kVacation = 5;
     kGoingToSleep = 6;
-    kUserDefined = 7;
+    kUserDefined = 254;
   }
 
   enum SetpointChangeSourceEnum : enum8 {

examples/thermostat/nxp/zap/thermostat_matter_wifi.matter

@@ -1863,7 +1863,7 @@ cluster Thermostat = 513 {
     kWake = 4;
     kVacation = 5;
     kGoingToSleep = 6;
-    kUserDefined = 7;
+    kUserDefined = 254;
   }
 
   enum SetpointChangeSourceEnum : enum8 {

examples/thermostat/qpg/zap/thermostaticRadiatorValve.matter

@@ -1560,7 +1560,7 @@ cluster Thermostat = 513 {
     kWake = 4;
     kVacation = 5;
     kGoingToSleep = 6;
-    kUserDefined = 7;
+    kUserDefined = 254;
   }
 
   enum SetpointChangeSourceEnum : enum8 {

examples/thermostat/thermostat-common/thermostat.matter

@@ -1740,7 +1740,7 @@ cluster Thermostat = 513 {
     kWake = 4;
     kVacation = 5;
     kGoingToSleep = 6;
-    kUserDefined = 7;
+    kUserDefined = 254;
   }
 
   enum SetpointChangeSourceEnum : enum8 {

src/app/clusters/thermostat-server/thermostat-delegate.h

@@ -27,8 +27,9 @@ namespace Clusters {
 namespace Thermostat {
 
 /** @brief
- *  Defines methods for implementing application-specific logic for the  extensions to the thermostat cluster.
- *  It defines the interfaces that a thermostat should implement to support Presets and other  extension features.
+ *  Defines methods for implementing application-specific logic for handling Presets in the thermostat cluster.
+ *  It defines the interfaces that a thermostat should implement to enable support for reading and writing the
+ *  Presets attribute and reading and writing the ActivePresetHandle attribute.
  */
 class Delegate
 {
@@ -105,8 +106,9 @@ class Delegate
     /**
      * @brief Updates the presets attribute with the content of the pending presets list. If the preset in the pending presets list
      * matches i.e. has the same presetHandle as an existing entry in the Presets attribute, the thermostat will update the entry
-     * with the new preset values, otherwise it will add a new preset to the Presets attribute. This will be called when the
-     * Thermostat receives a CommitPresetsSchedulesRequest command to commit the pending preset changes.
+     * with the new preset values, otherwise it will add a new preset to the Presets attribute. For new presets that get added,
+     * it is the responsibility of this API to allocate unique preset handles to the presets before saving the preset. This will be
+     * called when the Thermostat receives a CommitPresetsSchedulesRequest command to commit the pending preset changes.
      *
      * @return CHIP_NO_ERROR if the updates to the presets attribute has been committed successfully.
      * @return CHIP_ERROR if the updates to the presets attribute failed to commit for some reason.