From a2bb5d6110427a4056b96b52c71448f96a254bc4 Mon Sep 17 00:00:00 2001 From: Nivedita Sarkar Date: Fri, 26 Jul 2024 20:38:09 -0700 Subject: [PATCH] Address review comments. --- .../air-purifier-app.matter | 2 +- .../all-clusters-app.matter | 2 +- .../all-clusters-minimal-app.matter | 2 +- ...umiditysensor_thermostat_56de3d5f45.matter | 2 +- ...tnode_heatingcoolingunit_ncdGai1E5a.matter | 2 +- ...tnode_roomairconditioner_9cf3607804.matter | 2 +- .../rootnode_thermostat_bm3fb8dhYi.matter | 2 +- .../placeholder/linux/apps/app1/config.matter | 4 +- .../placeholder/linux/apps/app2/config.matter | 4 +- .../linux/include/thermostat-delegate-impl.h | 5 +- .../linux/thermostat-delegate-impl.cpp | 9 ++- .../thermostat/linux/thermostat-manager.cpp | 60 +++++++++---------- .../nxp/zap/thermostat_matter_thread.matter | 2 +- .../nxp/zap/thermostat_matter_wifi.matter | 2 +- .../qpg/zap/thermostaticRadiatorValve.matter | 2 +- .../thermostat-common/thermostat.matter | 2 +- .../thermostat-server/thermostat-delegate.h | 10 ++-- .../thermostat-server/thermostat-server.cpp | 38 ++++++------ .../thermostat-server/thermostat-server.h | 2 +- .../data-model/chip/thermostat-cluster.xml | 2 +- .../data_model/controller-clusters.matter | 2 +- .../python/chip/clusters/Objects.py | 4 +- .../CHIP/zap-generated/MTRBaseClusters.h | 2 +- .../app-common/zap-generated/cluster-enums.h | 4 +- 24 files changed, 87 insertions(+), 81 deletions(-) diff --git a/examples/air-purifier-app/air-purifier-common/air-purifier-app.matter b/examples/air-purifier-app/air-purifier-common/air-purifier-app.matter index 9e3b6da2deaa71..b9daf0cb550080 100644 --- a/examples/air-purifier-app/air-purifier-common/air-purifier-app.matter +++ b/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 { diff --git a/examples/all-clusters-app/all-clusters-common/all-clusters-app.matter b/examples/all-clusters-app/all-clusters-common/all-clusters-app.matter index 719f13ce379f84..59886807143f6f 100644 --- a/examples/all-clusters-app/all-clusters-common/all-clusters-app.matter +++ b/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 { diff --git a/examples/all-clusters-minimal-app/all-clusters-common/all-clusters-minimal-app.matter b/examples/all-clusters-minimal-app/all-clusters-common/all-clusters-minimal-app.matter index 224d37a2a32af1..c8fa5ea7e1c6e1 100644 --- a/examples/all-clusters-minimal-app/all-clusters-common/all-clusters-minimal-app.matter +++ b/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 { diff --git a/examples/chef/devices/rootnode_airpurifier_airqualitysensor_temperaturesensor_humiditysensor_thermostat_56de3d5f45.matter b/examples/chef/devices/rootnode_airpurifier_airqualitysensor_temperaturesensor_humiditysensor_thermostat_56de3d5f45.matter index 1eb8cd15cabcc9..27e38141a4e1b6 100644 --- a/examples/chef/devices/rootnode_airpurifier_airqualitysensor_temperaturesensor_humiditysensor_thermostat_56de3d5f45.matter +++ b/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 { diff --git a/examples/chef/devices/rootnode_heatingcoolingunit_ncdGai1E5a.matter b/examples/chef/devices/rootnode_heatingcoolingunit_ncdGai1E5a.matter index 7ae87d191298b2..22111d45d62eab 100644 --- a/examples/chef/devices/rootnode_heatingcoolingunit_ncdGai1E5a.matter +++ b/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 { diff --git a/examples/chef/devices/rootnode_roomairconditioner_9cf3607804.matter b/examples/chef/devices/rootnode_roomairconditioner_9cf3607804.matter index c21a5adf0fdb78..66d34c451a7f92 100644 --- a/examples/chef/devices/rootnode_roomairconditioner_9cf3607804.matter +++ b/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 { diff --git a/examples/chef/devices/rootnode_thermostat_bm3fb8dhYi.matter b/examples/chef/devices/rootnode_thermostat_bm3fb8dhYi.matter index e76f99dd0e63e7..eb6b301b559a3c 100644 --- a/examples/chef/devices/rootnode_thermostat_bm3fb8dhYi.matter +++ b/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 { diff --git a/examples/placeholder/linux/apps/app1/config.matter b/examples/placeholder/linux/apps/app1/config.matter index dff3bdb93c0011..5b91706480d3f9 100644 --- a/examples/placeholder/linux/apps/app1/config.matter +++ b/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 { diff --git a/examples/placeholder/linux/apps/app2/config.matter b/examples/placeholder/linux/apps/app2/config.matter index 10a4ff89f5285c..3499926c146506 100644 --- a/examples/placeholder/linux/apps/app2/config.matter +++ b/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 { diff --git a/examples/thermostat/linux/include/thermostat-delegate-impl.h b/examples/thermostat/linux/include/thermostat-delegate-impl.h index dfb36e1957dc26..c4daef5fde1d6a 100644 --- a/examples/thermostat/linux/include/thermostat-delegate-impl.h +++ b/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; diff --git a/examples/thermostat/linux/thermostat-delegate-impl.cpp b/examples/thermostat/linux/thermostat-delegate-impl.cpp index 04dc3b63f14e84..fa7bd9a259a475 100644 --- a/examples/thermostat/linux/thermostat-delegate-impl.cpp +++ b/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(pendingPreset.GetPresetScenario()) }; mPresets[mNextFreeIndexInPresetsList].SetPresetHandle(DataModel::MakeNullable(ByteSpan(handle))); mNextFreeIndexInPresetsList++; diff --git a/examples/thermostat/linux/thermostat-manager.cpp b/examples/thermostat/linux/thermostat-manager.cpp index ad5bdb7b700a18..526c38f9fb375c 100644 --- a/examples/thermostat/linux/thermostat-manager.cpp +++ b/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 static void OnAttributeChangeReported(const ConcreteDataAttributePath & path, const DecodableAttributeType & value); @@ -70,30 +77,30 @@ void OnAttributeChangeReported(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(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( peer_device->GetExchangeManager(), peer_device->GetSecureSession().Value(), binding.remote, clusterId, attributeId, &OnAttributeChangeReported, &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(mSystemMode), SystemModeString(mSystemMode), static_cast(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(Encoding::LittleEndian::Get16(value)); + memcpy(&mLocalTemperature, value, size); ChipLogError(AppServer, "Local temperature changed to %d", mLocalTemperature); EvalThermostatState(); } break; case OccupiedCoolingSetpoint::Id: { - mOccupiedCoolingSetpoint = static_cast(Encoding::LittleEndian::Get16(value)); + memcpy(&mOccupiedCoolingSetpoint, value, size); ChipLogError(AppServer, "Cooling temperature changed to %d", mOccupiedCoolingSetpoint); EvalThermostatState(); } break; case OccupiedHeatingSetpoint::Id: { - mOccupiedHeatingSetpoint = static_cast(Encoding::LittleEndian::Get16(value)); + memcpy(&mOccupiedHeatingSetpoint, value, size); ChipLogError(AppServer, "Heating temperature changed to %d", mOccupiedHeatingSetpoint); EvalThermostatState(); } @@ -313,7 +320,7 @@ uint8_t ThermostatManager::GetNumberOfPresets() CHIP_ERROR ThermostatManager::SetSystemMode(SystemModeEnum systemMode) { - uint8_t systemModeValue = static_cast(systemMode); + uint8_t systemModeValue = to_underlying(systemMode); if (mSystemMode == systemMode) { ChipLogDetail(AppServer, "Already in system mode: %u (%s)", systemModeValue, SystemModeString(systemMode)); @@ -321,15 +328,12 @@ CHIP_ERROR ThermostatManager::SetSystemMode(SystemModeEnum systemMode) } 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(runningMode); + uint8_t runningModeValue = to_underlying(runningMode); if (mRunningMode == runningMode) { ChipLogDetail(AppServer, "Already in running mode: %u (%s)", runningModeValue, RunningModeString(runningMode)); @@ -337,28 +341,22 @@ CHIP_ERROR ThermostatManager::SetRunningMode(ThermostatRunningModeEnum runningMo } 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(mSystemMode), SystemModeString(mSystemMode), static_cast(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); } diff --git a/examples/thermostat/nxp/zap/thermostat_matter_thread.matter b/examples/thermostat/nxp/zap/thermostat_matter_thread.matter index 8a0304c57b1e13..76dace386a25f2 100644 --- a/examples/thermostat/nxp/zap/thermostat_matter_thread.matter +++ b/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 { diff --git a/examples/thermostat/nxp/zap/thermostat_matter_wifi.matter b/examples/thermostat/nxp/zap/thermostat_matter_wifi.matter index 3d823c4fb62a85..f8154996ad20e2 100644 --- a/examples/thermostat/nxp/zap/thermostat_matter_wifi.matter +++ b/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 { diff --git a/examples/thermostat/qpg/zap/thermostaticRadiatorValve.matter b/examples/thermostat/qpg/zap/thermostaticRadiatorValve.matter index 6f8adc8f2cf3f6..8060f63590c3c4 100644 --- a/examples/thermostat/qpg/zap/thermostaticRadiatorValve.matter +++ b/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 { diff --git a/examples/thermostat/thermostat-common/thermostat.matter b/examples/thermostat/thermostat-common/thermostat.matter index 9209bdff98df28..f7139b07b43595 100644 --- a/examples/thermostat/thermostat-common/thermostat.matter +++ b/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 { diff --git a/src/app/clusters/thermostat-server/thermostat-delegate.h b/src/app/clusters/thermostat-server/thermostat-delegate.h index 4a01e5b5afb585..86c1e532b92fc2 100644 --- a/src/app/clusters/thermostat-server/thermostat-delegate.h +++ b/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. diff --git a/src/app/clusters/thermostat-server/thermostat-server.cpp b/src/app/clusters/thermostat-server/thermostat-server.cpp index 81a1b9c31ca9ad..2aed5ab9bb0610 100644 --- a/src/app/clusters/thermostat-server/thermostat-server.cpp +++ b/src/app/clusters/thermostat-server/thermostat-server.cpp @@ -408,6 +408,8 @@ uint8_t CountUpdatedPresetsAfterApplyingPendingPresets(Delegate * delegate) numberOfPendingPresets++; } + // TODO: #34546 - Need to support deletion of presets that are removed from Presets. + // This API needs to modify its logic for the deletion case. return static_cast(numberOfPresets + numberOfPendingPresets - numberOfMatches); } @@ -471,12 +473,12 @@ uint8_t CountPresetsInPendingListWithPresetHandle(Delegate * delegate, const Byt } /** - * @brief Checks if the presetType for the given preset scenario supports names in the presetTypeFeatures bitmap. + * @brief Checks if the presetType for the given preset scenario supports name in the presetTypeFeatures bitmap. * * @param[in] delegate The delegate to use. * @param[in] presetScenario The presetScenario to match with. * - * @return true if the presetType for the given preset scenario supports names, false otherwise. + * @return true if the presetType for the given preset scenario supports name, false otherwise. */ bool PresetTypeSupportsNames(Delegate * delegate, PresetScenarioEnum scenario) { @@ -493,7 +495,7 @@ bool PresetTypeSupportsNames(Delegate * delegate, PresetScenarioEnum scenario) if (presetType.presetScenario == scenario) { - return (presetType.presetTypeFeatures == PresetTypeFeaturesBitmap::kSupportsNames); + return (presetType.presetTypeFeatures.Has(PresetTypeFeaturesBitmap::kSupportsNames)); } } return false; @@ -783,9 +785,6 @@ CHIP_ERROR ThermostatAttrAccess::Read(const ConcreteReadAttributePath & aPath, A } break; case PresetsSchedulesEditable::Id: { - Delegate * delegate = GetDelegate(aPath.mEndpointId); - VerifyOrReturnError(delegate != nullptr, CHIP_ERROR_INCORRECT_STATE, ChipLogError(Zcl, "Delegate is null")); - ReturnErrorOnFailure(aEncoder.Encode(GetPresetsEditable(aPath.mEndpointId))); } break; @@ -861,7 +860,13 @@ CHIP_ERROR ThermostatAttrAccess::Write(const ConcreteDataAttributePath & aPath, // Check if the OriginatorScopedNodeId at the endpoint is the same as the node editing the presets, // otherwise return BUSY. const Access::SubjectDescriptor subjectDescriptor = aDecoder.GetSubjectDescriptor(); - ScopedNodeId scopedNodeId = ScopedNodeId(subjectDescriptor.subject, subjectDescriptor.fabricIndex); + ScopedNodeId scopedNodeId = ScopedNodeId(); + + // Get the node id if the authentication mode is CASE. + if (subjectDescriptor.authMode == Access::AuthMode::kCase) + { + scopedNodeId = ScopedNodeId(subjectDescriptor.subject, subjectDescriptor.fabricIndex); + } if (GetOriginatorScopedNodeId(endpoint) != scopedNodeId) { @@ -941,7 +946,7 @@ void emberAfThermostatClusterServerInitCallback(chip::EndpointId endpoint) // or should this just be the responsibility of the thermostat application? } -imcode MatterThermostatClusterServerPreAttributeChangedCallback(const app::ConcreteAttributePath & attributePath, +Protocols::InteractionModel::Status MatterThermostatClusterServerPreAttributeChangedCallback(const app::ConcreteAttributePath & attributePath, EmberAfAttributeType attributeType, uint16_t size, uint8_t * value) { EndpointId endpoint = attributePath.mEndpointId; @@ -1376,7 +1381,8 @@ bool emberAfThermostatClusterCommitPresetsSchedulesRequestCallback( // StartPresetsSchedulesEditRequest, return UNSUPPORTED_ACCESS. if (gThermostatAttrAccess.GetOriginatorScopedNodeId(endpoint) != sourceNodeId) { - return SendResponseAndCleanUp(delegate, endpoint, commandObj, commandPath, imcode::UnsupportedAccess); + commandObj->AddStatus(commandPath, imcode::UnsupportedAccess); + return true; } PresetStructWithOwnedMembers preset; @@ -1453,12 +1459,6 @@ bool emberAfThermostatClusterCommitPresetsSchedulesRequestCallback( } bool isPendingPresetWithNullPresetHandle = pendingPreset.GetPresetHandle().IsNull(); - if (isPendingPresetWithNullPresetHandle) - { - // If the presetHandle for a preset is null, the device should set a unique presetHandle. - const uint8_t handle[] = { static_cast(pendingPreset.GetPresetScenario()) }; - pendingPreset.SetPresetHandle(DataModel::MakeNullable(ByteSpan(handle))); - } // If the preset handle is null and the built in field is set to true, return CONSTRAINT_ERROR. if (isPendingPresetWithNullPresetHandle && IsBuiltIn(pendingPreset)) @@ -1504,19 +1504,19 @@ bool emberAfThermostatClusterCommitPresetsSchedulesRequestCallback( // If the presetScenario is not found in the preset types, return CONSTRAINT_ERROR. PresetScenarioEnum presetScenario = pendingPreset.GetPresetScenario(); - if (!FindPresetScenarioInPresetTypes(delegate, presetScenario)) + if (!PresetScenarioExistsInPresetTypes(delegate, presetScenario)) { return SendResponseAndCleanUp(delegate, endpoint, commandObj, commandPath, imcode::ConstraintError); } - // If the preset type for the preset scenario does not supports name and a name is specified, return CONSTRAINT_ERROR. + // If the preset type for the preset scenario does not support name and a name is specified, return CONSTRAINT_ERROR. if (!PresetTypeSupportsNames(delegate, presetScenario) && pendingPreset.GetName().HasValue()) { return SendResponseAndCleanUp(delegate, endpoint, commandObj, commandPath, imcode::ConstraintError); } - // Enforce the Setpoint Limits for both the cooling and heating - Optional coolingSetpointValue = preset.GetCoolingSetpoint(); + // Enforce the Setpoint Limits for both the cooling and heating setpoints in the pending preset. + Optional coolingSetpointValue = pendingPreset.GetCoolingSetpoint(); if (coolingSetpointValue.HasValue()) { pendingPreset.SetCoolingSetpoint(MakeOptional(EnforceCoolingSetpointLimits(coolingSetpointValue.Value(), endpoint))); diff --git a/src/app/clusters/thermostat-server/thermostat-server.h b/src/app/clusters/thermostat-server/thermostat-server.h index 4f4fae159e1a81..955ab9e5c5a777 100644 --- a/src/app/clusters/thermostat-server/thermostat-server.h +++ b/src/app/clusters/thermostat-server/thermostat-server.h @@ -17,7 +17,7 @@ /**************************************************************************** * @file - * @brief APIs for the Thermostat cluster extension. + * @brief APIs for the Thermostat cluster. * ******************************************************************************* ******************************************************************************/ diff --git a/src/app/zap-templates/zcl/data-model/chip/thermostat-cluster.xml b/src/app/zap-templates/zcl/data-model/chip/thermostat-cluster.xml index 982ea0e9f7b73e..0e2de159d0b6d0 100644 --- a/src/app/zap-templates/zcl/data-model/chip/thermostat-cluster.xml +++ b/src/app/zap-templates/zcl/data-model/chip/thermostat-cluster.xml @@ -201,7 +201,7 @@ limitations under the License. - + diff --git a/src/controller/data_model/controller-clusters.matter b/src/controller/data_model/controller-clusters.matter index 78a39945cd932e..626ebc5bcfa446 100644 --- a/src/controller/data_model/controller-clusters.matter +++ b/src/controller/data_model/controller-clusters.matter @@ -6590,7 +6590,7 @@ cluster Thermostat = 513 { kWake = 4; kVacation = 5; kGoingToSleep = 6; - kUserDefined = 7; + kUserDefined = 254; } enum SetpointChangeSourceEnum : enum8 { diff --git a/src/controller/python/chip/clusters/Objects.py b/src/controller/python/chip/clusters/Objects.py index 3eef2869c8ba6e..480ab3e7d0226c 100644 --- a/src/controller/python/chip/clusters/Objects.py +++ b/src/controller/python/chip/clusters/Objects.py @@ -32198,12 +32198,12 @@ class PresetScenarioEnum(MatterIntEnum): kWake = 0x04 kVacation = 0x05 kGoingToSleep = 0x06 - kUserDefined = 0x07 + kUserDefined = 0xFE # All received enum values that are not listed above will be mapped # to kUnknownEnumValue. This is a helper enum value that should only # be used by code to process how it handles receiving and unknown # enum value. This specific should never be transmitted. - kUnknownEnumValue = 8, + kUnknownEnumValue = 7, class SetpointChangeSourceEnum(MatterIntEnum): kManual = 0x00 diff --git a/src/darwin/Framework/CHIP/zap-generated/MTRBaseClusters.h b/src/darwin/Framework/CHIP/zap-generated/MTRBaseClusters.h index 3e603ff3763437..bcdee4fc6798b6 100644 --- a/src/darwin/Framework/CHIP/zap-generated/MTRBaseClusters.h +++ b/src/darwin/Framework/CHIP/zap-generated/MTRBaseClusters.h @@ -19743,7 +19743,7 @@ typedef NS_ENUM(uint8_t, MTRThermostatPresetScenario) { MTRThermostatPresetScenarioWake MTR_PROVISIONALLY_AVAILABLE = 0x04, MTRThermostatPresetScenarioVacation MTR_PROVISIONALLY_AVAILABLE = 0x05, MTRThermostatPresetScenarioGoingToSleep MTR_PROVISIONALLY_AVAILABLE = 0x06, - MTRThermostatPresetScenarioUserDefined MTR_PROVISIONALLY_AVAILABLE = 0x07, + MTRThermostatPresetScenarioUserDefined MTR_PROVISIONALLY_AVAILABLE = 0xFE, } MTR_PROVISIONALLY_AVAILABLE; typedef NS_ENUM(uint8_t, MTRThermostatSetpointChangeSource) { diff --git a/zzz_generated/app-common/app-common/zap-generated/cluster-enums.h b/zzz_generated/app-common/app-common/zap-generated/cluster-enums.h index c18204ae3327df..d8092bee42e3ea 100644 --- a/zzz_generated/app-common/app-common/zap-generated/cluster-enums.h +++ b/zzz_generated/app-common/app-common/zap-generated/cluster-enums.h @@ -3936,12 +3936,12 @@ enum class PresetScenarioEnum : uint8_t kWake = 0x04, kVacation = 0x05, kGoingToSleep = 0x06, - kUserDefined = 0x07, + kUserDefined = 0xFE, // All received enum values that are not listed above will be mapped // to kUnknownEnumValue. This is a helper enum value that should only // be used by code to process how it handles receiving and unknown // enum value. This specific should never be transmitted. - kUnknownEnumValue = 8, + kUnknownEnumValue = 7, }; // Enum for SetpointChangeSourceEnum