Skip to content
76 changes: 61 additions & 15 deletions src/app/clusters/thermostat-server/thermostat-server-presets.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "thermostat-server-presets.h"
#include "thermostat-server.h"

#include <app-common/zap-generated/attributes/Accessors.h>
#include <platform/internal/CHIPDeviceLayerInternal.h>

using namespace chip;
Expand Down Expand Up @@ -116,18 +117,22 @@ bool MatchingPendingPresetExists(Delegate * delegate, const PresetStructWithOwne

/**
* @brief Finds and returns an entry in the Presets attribute list that matches
* a preset, if such an entry exists. The presetToMatch must have a preset handle.
* the given preset handle, if such an entry exists.
*
* @param[in] delegate The delegate to use.
* @param[in] presetToMatch The preset to match with.
* @param[out] matchingPreset The preset in the Presets attribute list that has the same PresetHandle as the presetToMatch.
* @param[in] presetHandle The preset handle to match against entries in the Presets attribute list.
* @param[out] matchingPreset The preset in the Presets attribute list that has the same PresetHandle as @p presetHandle.
* Only valid if @p found is set to true.
* @param[out] found Set to true if a matching preset was found; set to false if no matching preset exists.
*
* @return true if a matching entry was found in the presets attribute list, false otherwise.
* @return CHIP_NO_ERROR if the lookup completed (regardless of whether a matching preset was found);
* otherwise an appropriate error code.
*/
bool GetMatchingPresetInPresets(Delegate * delegate, const DataModel::Nullable<ByteSpan> & presetHandle,
PresetStructWithOwnedMembers & matchingPreset)
CHIP_ERROR GetMatchingPresetInPresets(Delegate * delegate, const ByteSpan & presetHandle,
PresetStructWithOwnedMembers & matchingPreset, bool & found)
{
VerifyOrReturnValue(delegate != nullptr, false);
VerifyOrReturnError(delegate != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
found = false;

for (uint8_t i = 0; true; i++)
{
Expand All @@ -140,16 +145,17 @@ bool GetMatchingPresetInPresets(Delegate * delegate, const DataModel::Nullable<B
if (err != CHIP_NO_ERROR)
{
ChipLogError(Zcl, "GetMatchingPresetInPresets: GetPresetAtIndex failed with error %" CHIP_ERROR_FORMAT, err.Format());
return false;
return err;
}

// Note: presets coming from our delegate always have a handle.
if (presetHandle.Value().data_equal(matchingPreset.GetPresetHandle().Value()))
if (presetHandle.data_equal(matchingPreset.GetPresetHandle().Value()))
{
return true;
found = true;
return CHIP_NO_ERROR;
}
}
return false;
return CHIP_NO_ERROR;
}

/**
Expand Down Expand Up @@ -302,10 +308,44 @@ Status ThermostatAttrAccess::SetActivePreset(EndpointId endpoint, DataModel::Nul
return Status::InvalidInState;
}

// If the preset handle passed in the command is not present in the Presets attribute, return INVALID_COMMAND.
if (!presetHandle.IsNull() && !IsPresetHandlePresentInPresets(delegate, presetHandle.Value()))
if (!presetHandle.IsNull())
{
return Status::InvalidCommand;
PresetStructWithOwnedMembers matchingPreset;
// Look up the preset once and reuse it to apply any setpoints it carries.
bool found = false;
CHIP_ERROR lookupErr = GetMatchingPresetInPresets(delegate, presetHandle.Value(), matchingPreset, found);
if (lookupErr != CHIP_NO_ERROR)
{
return Status::InvalidInState;
}
if (!found)
{
return Status::InvalidCommand;
}

Optional<int16_t> coolingSetpointValue = matchingPreset.GetCoolingSetpoint();
if (coolingSetpointValue.HasValue())
{
int16_t constrainedCoolingSetpoint = EnforceCoolingSetpointLimits(coolingSetpointValue.Value(), endpoint);
Status status = OccupiedCoolingSetpoint::Set(endpoint, constrainedCoolingSetpoint);
if (status != Status::Success)
{
ChipLogError(Zcl, "Failed to set OccupiedCoolingSetpoint with status %u", to_underlying(status));
return status;
}
Comment on lines +331 to +337
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I addressed the re-entrancy concern directly in SetActivePreset by adding an internal guard for preset-application scope.
When setpoint writes triggered during preset application cause a callback-driven SetActivePreset(nullptr), that nested clear is now ignored, so we avoid transient clears/extra writes while preserving normal explicit clear behavior outside this flow.

I also kept the existing error propagation behavior (StatusIB(...)) for lookup/write failures.

}

Optional<int16_t> heatingSetpointValue = matchingPreset.GetHeatingSetpoint();
if (heatingSetpointValue.HasValue())
{
int16_t constrainedHeatingSetpoint = EnforceHeatingSetpointLimits(heatingSetpointValue.Value(), endpoint);
Status status = OccupiedHeatingSetpoint::Set(endpoint, constrainedHeatingSetpoint);
if (status != Status::Success)
{
ChipLogError(Zcl, "Failed to set OccupiedHeatingSetpoint with status %u", to_underlying(status));
return status;
}
}
Comment on lines +328 to +350
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This re-entrancy path is now guarded in SetActivePreset: while applying a non-null preset, we increment a scoped presetApplicationDepth, and any callback-driven SetActivePreset(..., nullopt) during that window returns early.
So setpoint writes can still trigger the attribute-changed callback, but they no longer clear ActivePresetHandle mid-application (including deadband-adjustment cascades), avoiding transient clears and extra churn/version bumps.

}

CHIP_ERROR err = delegate->SetActivePresetHandle(presetHandle);
Expand Down Expand Up @@ -341,7 +381,13 @@ CHIP_ERROR ThermostatAttrAccess::AppendPendingPreset(Thermostat::Delegate * dele
// Per spec we need to check that:
// (a) There is an existing non-pending preset with this handle.
PresetStructWithOwnedMembers matchingPreset;
if (!GetMatchingPresetInPresets(delegate, preset.GetPresetHandle().Value(), matchingPreset))
bool found = false;
CHIP_ERROR lookupErr = GetMatchingPresetInPresets(delegate, preset.GetPresetHandle().Value(), matchingPreset, found);
if (lookupErr != CHIP_NO_ERROR)
{
return CHIP_IM_GLOBAL_STATUS(InvalidInState);
}
if (!found)
{
return CHIP_IM_GLOBAL_STATUS(NotFound);
}
Expand Down
Loading