diff --git a/src/app/clusters/thermostat-server/ThermostatCluster.h b/src/app/clusters/thermostat-server/ThermostatCluster.h index 5ef8e1fb6205..cfe06023439b 100644 --- a/src/app/clusters/thermostat-server/ThermostatCluster.h +++ b/src/app/clusters/thermostat-server/ThermostatCluster.h @@ -353,6 +353,16 @@ class ThermostatCluster : public ThermostatClusterBase, public AtomicWriteSessio { if (auto status = mPresets.OnAtomicWriteCommit(attributeId)) { + if constexpr (kHasSuggestions) + { + // Per spec § 4.3.11.50, removing a preset that is referenced by a ThermostatSuggestions entry + // or by CurrentThermostatSuggestion must cascade. This is best-effort since the Presets commit + // above has already taken effect. + if (*status == Protocols::InteractionModel::Status::Success && attributeId == Attributes::Presets::Id) + { + mSuggestions.OnPresetsCommitted(); + } + } return *status; } } diff --git a/src/app/clusters/thermostat-server/ThermostatClusterSuggestions.cpp b/src/app/clusters/thermostat-server/ThermostatClusterSuggestions.cpp index ff994f94db97..5c0b56677f55 100644 --- a/src/app/clusters/thermostat-server/ThermostatClusterSuggestions.cpp +++ b/src/app/clusters/thermostat-server/ThermostatClusterSuggestions.cpp @@ -26,6 +26,8 @@ #include #include +#include + using namespace chip::app::Clusters::Globals::Structs; using namespace chip::app::Clusters::Thermostat; using namespace chip::app::Clusters::Thermostat::Attributes; @@ -66,6 +68,42 @@ CHIP_ERROR RemoveExpiredSuggestions(chip::app::Clusters::Thermostat::ThermostatS return err; } +/** + * @brief Determines whether a preset handle still exists in the Presets attribute list. + * + * Unlike IsPresetHandlePresentInPresets(), which treats a GetPresetAtIndex() enumeration error the same as "not + * found", this distinguishes the two so callers that take a destructive action (e.g. removing a stale thermostat + * suggestion) on "not found" do not do so on a transient enumeration error. + * + * @param[in] delegate The Presets delegate to use. + * @param[in] presetHandle The preset handle to look for. + * @param[out] exists Set to true if a preset with this handle is present in the Presets attribute list, false + * otherwise. + * + * @return CHIP_NO_ERROR if the Presets attribute list was enumerated successfully, an error code if not. + */ +CHIP_ERROR PresetHandleStillExists(chip::app::Clusters::Thermostat::ThermostatPresets::Delegate & delegate, + const chip::ByteSpan & presetHandle, bool & exists) +{ + exists = false; + PresetStructWithOwnedMembers preset; + for (uint8_t i = 0; true; i++) + { + CHIP_ERROR err = delegate.GetPresetAtIndex(i, preset); + if (err == CHIP_ERROR_PROVIDER_LIST_EXHAUSTED) + { + return CHIP_NO_ERROR; + } + ReturnErrorOnFailure(err); + + if (!preset.GetPresetHandle().IsNull() && preset.GetPresetHandle().Value().data_equal(presetHandle)) + { + exists = true; + return CHIP_NO_ERROR; + } + } +} + Status RemoveFromThermostatSuggestionsList(chip::app::Clusters::Thermostat::ThermostatSuggestions::Delegate & delegate, uint8_t uniqueIDToRemove) { @@ -94,6 +132,29 @@ Status RemoveFromThermostatSuggestionsList(chip::app::Clusters::Thermostat::Ther return Status::Success; } +/** + * @brief Determines whether CurrentThermostatSuggestion actually changed between two snapshots of it. + * + * @param[in] before A snapshot of CurrentThermostatSuggestion taken before some operation. + * @param[in] after A snapshot of CurrentThermostatSuggestion taken after that operation. + * + * @return true if the null-ness differs, or both are non-null but their UniqueID differs. + */ +bool CurrentSuggestionChanged( + const chip::app::DataModel::Nullable & before, + const chip::app::DataModel::Nullable & after) +{ + if (before.IsNull() != after.IsNull()) + { + return true; + } + if (after.IsNull()) + { + return false; + } + return before.Value().GetUniqueID() != after.Value().GetUniqueID(); +} + } // anonymous namespace namespace chip { @@ -268,7 +329,7 @@ ThermostatSuggestions::RemoveThermostatSuggestion(CommandHandler * commandObj, c return Status::Success; } -void ThermostatSuggestions::ReEvaluateCurrentSuggestion() +bool ThermostatSuggestions::ReEvaluateCurrentSuggestion() { DataModel::Nullable currentSuggestionBeforeReevaluation; mDelegate.GetCurrentThermostatSuggestion(currentSuggestionBeforeReevaluation); @@ -281,14 +342,14 @@ void ThermostatSuggestions::ReEvaluateCurrentSuggestion() if (err != CHIP_NO_ERROR) { ChipLogError(Zcl, "Failed to GetActivePresetHandle with error: %" CHIP_ERROR_FORMAT, err.Format()); - return; + return false; } err = mDelegate.ReEvaluateCurrentSuggestion(); if (err != CHIP_NO_ERROR) { ChipLogError(Zcl, "Failed to ReEvaluateCurrentSuggestion with error: %" CHIP_ERROR_FORMAT, err.Format()); - return; + return false; } DataModel::Nullable currentSuggestionAfterReevaluation; @@ -314,22 +375,130 @@ void ThermostatSuggestions::ReEvaluateCurrentSuggestion() err = mPresets.GetDelegate().GetActivePresetHandle(afterReevaluationHandle); if (err != CHIP_NO_ERROR) { - return; + return suggestionChanged; + } + + bool activePresetHandleChanged = !(beforeReevaluationHandle.IsNull() && afterReevaluationHandle.IsNull()) && + !(!beforeReevaluationHandle.IsNull() && !afterReevaluationHandle.IsNull() && + beforeReevaluationHandle.Value().data_equal(afterReevaluationHandle.Value())); + if (activePresetHandleChanged) + { + // If the active preset handle changed, notify the attribute changed. + mCluster.NotifyAttributeChanged(ActivePresetHandle::Id); + } + + return suggestionChanged; +} + +bool ThermostatSuggestions::RemoveThermostatSuggestionsForRemovedPresets() +{ + uint8_t numSuggestions = mDelegate.GetNumberOfThermostatSuggestions(); + + // Caches, for each suggestion index, whether its preset is still present -- sized to the type's maximum + // possible list length (uint8_t) rather than numSuggestions itself, to avoid a variable-length array. Populated + // by the first pass below and consumed by the second, so the second pass never re-queries the Presets delegate + // and cannot itself fail partway through a removal. + bool presetStillExists[std::numeric_limits::max()] = { false }; + + // First pass: check every suggestion's preset for existence without mutating ThermostatSuggestions. If any + // check fails, abort with no removals at all: an inconclusive answer for one entry must not cause a partial + // cleanup of the entries already checked. + for (uint8_t i = 0; i < numSuggestions; i++) + { + ThermostatSuggestionStructWithOwnedMembers suggestion; + CHIP_ERROR err = mDelegate.GetThermostatSuggestionAtIndex(i, suggestion); + if (err != CHIP_NO_ERROR) + { + ChipLogError(Zcl, + "RemoveThermostatSuggestionsForRemovedPresets: GetThermostatSuggestionAtIndex failed with error " + "%" CHIP_ERROR_FORMAT, + err.Format()); + return false; + } + + err = PresetHandleStillExists(mPresets.GetDelegate(), suggestion.GetPresetHandle(), presetStillExists[i]); + if (err != CHIP_NO_ERROR) + { + ChipLogError( + Zcl, "RemoveThermostatSuggestionsForRemovedPresets: PresetHandleStillExists failed with error %" CHIP_ERROR_FORMAT, + err.Format()); + return false; + } + } + + bool didRemoveAnEntry = false; + bool abortedEarly = false; + + // Second pass: every preset check above succeeded and is cached, so it's now safe to actually remove the stale + // entries using the cached results, without any further Presets delegate lookups that could themselves fail + // partway through. Walk backwards so removing an entry does not shift the indices of entries not yet visited. + for (int i = static_cast(numSuggestions) - 1; i >= 0; i--) + { + if (presetStillExists[static_cast(i)]) + { + continue; + } + + CHIP_ERROR err = mDelegate.RemoveFromThermostatSuggestionsList(static_cast(i)); + if (err != CHIP_NO_ERROR) + { + ChipLogError(Zcl, + "RemoveThermostatSuggestionsForRemovedPresets: RemoveFromThermostatSuggestionsList failed with error " + "%" CHIP_ERROR_FORMAT, + err.Format()); + abortedEarly = true; + break; + } + didRemoveAnEntry = true; } - if (beforeReevaluationHandle.IsNull() && afterReevaluationHandle.IsNull()) + if (didRemoveAnEntry) { + mCluster.NotifyAttributeChanged(Attributes::ThermostatSuggestions::Id); + } + + // A removal-phase delegate error leaves stale entries still present, so this cascade did not complete per the + // documented contract: report false even though some entries may already have been removed. + return !abortedEarly; +} + +void ThermostatSuggestions::OnPresetsCommitted() +{ + // RemoveFromThermostatSuggestionsList() nulls CurrentThermostatSuggestion, per its API contract, when the + // removed entry was current. Snapshot it here so that if the cascade below removes it and nothing later + // replaces it, the true transition can still be detected and reported: ReEvaluateCurrentSuggestion()'s own + // before/after diff can't, since by the time it takes its "before" snapshot the cascade has already nulled the + // delegate's state. + DataModel::Nullable currentBeforeCleanup; + mDelegate.GetCurrentThermostatSuggestion(currentBeforeCleanup); + + if (!RemoveThermostatSuggestionsForRemovedPresets()) + { + // The cascade aborted on a delegate error, leaving potentially-stale suggestions in place. Skip + // re-evaluation: it could otherwise pick a suggestion whose preset no longer exists. The partial cleanup + // that ran before the failure may already have removed the current suggestion, though, and that real + // transition must still be reported rather than silently dropped. + DataModel::Nullable currentAfterPartialCleanup; + mDelegate.GetCurrentThermostatSuggestion(currentAfterPartialCleanup); + if (CurrentSuggestionChanged(currentBeforeCleanup, currentAfterPartialCleanup)) + { + mCluster.NotifyAttributeChanged(CurrentThermostatSuggestion::Id); + } return; } - if (!beforeReevaluationHandle.IsNull() && !afterReevaluationHandle.IsNull() && - beforeReevaluationHandle.Value().data_equal(afterReevaluationHandle.Value())) + bool alreadyNotifiedCurrentSuggestion = ReEvaluateCurrentSuggestion(); + if (alreadyNotifiedCurrentSuggestion) { return; } - // If the active preset handle changed, notify the attribute changed. - mCluster.NotifyAttributeChanged(ActivePresetHandle::Id); + DataModel::Nullable currentAfter; + mDelegate.GetCurrentThermostatSuggestion(currentAfter); + if (CurrentSuggestionChanged(currentBeforeCleanup, currentAfter)) + { + mCluster.NotifyAttributeChanged(CurrentThermostatSuggestion::Id); + } } CHIP_ERROR ThermostatSuggestions::Attributes(const ConcreteClusterPath & path, diff --git a/src/app/clusters/thermostat-server/ThermostatClusterSuggestions.h b/src/app/clusters/thermostat-server/ThermostatClusterSuggestions.h index 1c998c72169c..66343bf1ae08 100644 --- a/src/app/clusters/thermostat-server/ThermostatClusterSuggestions.h +++ b/src/app/clusters/thermostat-server/ThermostatClusterSuggestions.h @@ -152,9 +152,40 @@ class ThermostatSuggestions RemoveThermostatSuggestion(CommandHandler * commandObj, const ConcreteCommandPath & commandPath, const Commands::RemoveThermostatSuggestion::DecodableType & commandData); - void ReEvaluateCurrentSuggestion(); + /** + * @brief Re-evaluates and, if it changed, notifies CurrentThermostatSuggestion (and ActivePresetHandle, if the + * delegate's re-evaluation moved it). + * + * @return true if CurrentThermostatSuggestion was found to have changed (and was notified), false otherwise. + */ + bool ReEvaluateCurrentSuggestion(); + + /** + * @brief Reacts to a successful Presets atomic-write commit: cascades the removal of any ThermostatSuggestions + * entries whose preset no longer exists (see RemoveThermostatSuggestionsForRemovedPresets()) and, if + * that cascade completed, re-evaluates the current suggestion. + * + * Per spec § 4.3.11.50, this must run after a Presets atomic write commits a preset removal. + */ + void OnPresetsCommitted(); private: + /** + * @brief Removes every entry in the ThermostatSuggestions attribute list whose PresetHandle no longer matches a + * preset in the Presets attribute list, and notifies ThermostatSuggestions changed if any entry was + * removed. Does not itself evaluate or notify CurrentThermostatSuggestion; see OnPresetsCommitted(). + * + * This is best-effort: if a delegate lookup fails partway through the first pass, the cascade logs the error and + * leaves every entry untouched rather than risk a partial cleanup. If a delegate removal fails partway through + * the second pass, the entries already removed stay removed, but the cascade still reports that it did not + * complete, since one or more stale entries may remain. + * + * @return true if the cascade completed (whether or not anything was removed), false if it aborted early on a + * delegate error, in which case stale entries may remain even though some entries may already have been + * removed. + */ + bool RemoveThermostatSuggestionsForRemovedPresets(); + ThermostatClusterBase & mCluster; Delegate & mDelegate; ThermostatPresets & mPresets; diff --git a/src/app/clusters/thermostat-server/tests/TestThermostatSuggestionsAndHold.cpp b/src/app/clusters/thermostat-server/tests/TestThermostatSuggestionsAndHold.cpp index 599bb5cd52ad..4f99f10223a0 100644 --- a/src/app/clusters/thermostat-server/tests/TestThermostatSuggestionsAndHold.cpp +++ b/src/app/clusters/thermostat-server/tests/TestThermostatSuggestionsAndHold.cpp @@ -22,6 +22,8 @@ #include #include +#include + using namespace chip; using namespace chip::app; using namespace chip::app::Clusters; @@ -31,6 +33,7 @@ using namespace chip::app::Clusters::Thermostat::Commands; using namespace chip::Protocols::InteractionModel; using namespace chip::System::Clock; using namespace chip::Testing; +using chip::app::Clusters::Globals::AtomicRequestTypeEnum; namespace { @@ -241,4 +244,492 @@ TEST_F(ThermostatTestFixture, TestSuggestionsAttributesAndAddRemoveCommands) cluster.Shutdown(ClusterShutdownType::kClusterShutdown); } +TEST_F(ThermostatTestFixture, TestPresetRemovalCascadesToStaleSuggestion) +{ + BitFlags features(Feature::kHeating, Feature::kCooling, Feature::kPresets, Feature::kThermostatSuggestions); + + // Two presets: A survives the atomic write below, B does not. Both are pre-existing (committed) presets, so + // BuiltIn must be non-null on both them and their replacements below, per AppendPendingPreset's built-in + // consistency check. + PresetStructWithOwnedMembers presetA; + presetA.SetPresetScenario(PresetScenarioEnum::kOccupied); + uint8_t handleA[4] = { 1, 1, 1, 1 }; + EXPECT_EQ(presetA.SetPresetHandle(DataModel::MakeNullable(ByteSpan(handleA))), CHIP_NO_ERROR); + presetA.SetBuiltIn(DataModel::MakeNullable(true)); + mPresetsDelegate.mPresets.push_back(presetA); + + PresetStructWithOwnedMembers presetB; + presetB.SetPresetScenario(PresetScenarioEnum::kOccupied); + uint8_t handleB[4] = { 2, 2, 2, 2 }; + EXPECT_EQ(presetB.SetPresetHandle(DataModel::MakeNullable(ByteSpan(handleB))), CHIP_NO_ERROR); + presetB.SetBuiltIn(DataModel::MakeNullable(false)); + mPresetsDelegate.mPresets.push_back(presetB); + + Structs::PresetTypeStruct::Type ptype; + ptype.presetScenario = PresetScenarioEnum::kOccupied; + ptype.numberOfPresets = 5; + mPresetsDelegate.mPresetTypes.push_back(ptype); + + // A suggestion referencing preset B, and it is the current suggestion. + ThermostatSuggestionStructWithOwnedMembers suggestion; + suggestion.SetUniqueID(7); + EXPECT_EQ(suggestion.SetPresetHandle(ByteSpan(handleB)), CHIP_NO_ERROR); + suggestion.SetEffectiveTime(Seconds32(0)); + suggestion.SetExpirationTime(Seconds32(1000000)); + mSuggestionsDelegate.mSuggestions.push_back(suggestion); + mSuggestionsDelegate.mCurrentSuggestion.SetNonNull(suggestion); + + ThermostatCluster cluster(kTestEndpointId, features, MakeConfig(), mThermostatDelegate, mHeatingDelegate, mCoolingDelegate, + mPresetsDelegate, mSuggestionsDelegate); + ClusterTester tester(cluster); + SetupTesterSubject(tester); + ASSERT_EQ(cluster.Startup(tester.GetServerClusterContext()), CHIP_NO_ERROR); + + // Remove preset B by committing a Presets atomic write that only re-lists preset A. + Commands::AtomicRequest::Type beginReq; + beginReq.requestType = AtomicRequestTypeEnum::kBeginWrite; + chip::AttributeId attrIds[] = { Attributes::Presets::Id }; + beginReq.attributeRequests = DataModel::List(attrIds, 1); + beginReq.timeout = MakeOptional(static_cast(5000)); + ASSERT_TRUE(tester.Invoke(beginReq).IsSuccess()); + + Structs::PresetStruct::Type keepA; + keepA.presetScenario = PresetScenarioEnum::kOccupied; + keepA.presetHandle = DataModel::MakeNullable(ByteSpan(handleA)); + keepA.builtIn = DataModel::MakeNullable(true); + Structs::PresetStruct::Type newList[] = { keepA }; + auto listPayload = DataModel::List(newList, 1); + ASSERT_EQ(tester.WriteAttribute(Attributes::Presets::Id, listPayload, ListWritingPattern::ReplaceAll), Status::Success); + + Commands::AtomicRequest::Type commitReq; + commitReq.requestType = AtomicRequestTypeEnum::kCommitWrite; + commitReq.attributeRequests = DataModel::List(attrIds, 1); + ASSERT_TRUE(tester.Invoke(commitReq).IsSuccess()); + + // The stale suggestion referencing the removed preset B is gone, and CurrentThermostatSuggestion is null + // (there is nothing left to pick as a replacement), with both attributes reported dirty. + EXPECT_EQ(mSuggestionsDelegate.mSuggestions.size(), 0u); + EXPECT_TRUE(mSuggestionsDelegate.mCurrentSuggestion.IsNull()); + EXPECT_TRUE(tester.IsAttributeDirty(Attributes::ThermostatSuggestions::Id)); + EXPECT_TRUE(tester.IsAttributeDirty(CurrentThermostatSuggestion::Id)); + + cluster.Shutdown(ClusterShutdownType::kClusterShutdown); +} + +TEST_F(ThermostatTestFixture, TestPresetRemovalLeavesSurvivingPresetSuggestionAlone) +{ + BitFlags features(Feature::kHeating, Feature::kCooling, Feature::kPresets, Feature::kThermostatSuggestions); + + PresetStructWithOwnedMembers presetA; + presetA.SetPresetScenario(PresetScenarioEnum::kOccupied); + uint8_t handleA[4] = { 1, 1, 1, 1 }; + EXPECT_EQ(presetA.SetPresetHandle(DataModel::MakeNullable(ByteSpan(handleA))), CHIP_NO_ERROR); + presetA.SetBuiltIn(DataModel::MakeNullable(true)); + mPresetsDelegate.mPresets.push_back(presetA); + + PresetStructWithOwnedMembers presetB; + presetB.SetPresetScenario(PresetScenarioEnum::kOccupied); + uint8_t handleB[4] = { 2, 2, 2, 2 }; + EXPECT_EQ(presetB.SetPresetHandle(DataModel::MakeNullable(ByteSpan(handleB))), CHIP_NO_ERROR); + presetB.SetBuiltIn(DataModel::MakeNullable(false)); + mPresetsDelegate.mPresets.push_back(presetB); + + Structs::PresetTypeStruct::Type ptype; + ptype.presetScenario = PresetScenarioEnum::kOccupied; + ptype.numberOfPresets = 5; + mPresetsDelegate.mPresetTypes.push_back(ptype); + + // The only suggestion references the surviving preset A. + ThermostatSuggestionStructWithOwnedMembers suggestion; + suggestion.SetUniqueID(9); + EXPECT_EQ(suggestion.SetPresetHandle(ByteSpan(handleA)), CHIP_NO_ERROR); + suggestion.SetEffectiveTime(Seconds32(0)); + suggestion.SetExpirationTime(Seconds32(1000000)); + mSuggestionsDelegate.mSuggestions.push_back(suggestion); + mSuggestionsDelegate.mCurrentSuggestion.SetNonNull(suggestion); + + ThermostatCluster cluster(kTestEndpointId, features, MakeConfig(), mThermostatDelegate, mHeatingDelegate, mCoolingDelegate, + mPresetsDelegate, mSuggestionsDelegate); + ClusterTester tester(cluster); + SetupTesterSubject(tester); + ASSERT_EQ(cluster.Startup(tester.GetServerClusterContext()), CHIP_NO_ERROR); + + // Remove preset B, leaving A (and its suggestion) untouched. + Commands::AtomicRequest::Type beginReq; + beginReq.requestType = AtomicRequestTypeEnum::kBeginWrite; + chip::AttributeId attrIds[] = { Attributes::Presets::Id }; + beginReq.attributeRequests = DataModel::List(attrIds, 1); + beginReq.timeout = MakeOptional(static_cast(5000)); + ASSERT_TRUE(tester.Invoke(beginReq).IsSuccess()); + + Structs::PresetStruct::Type keepA; + keepA.presetScenario = PresetScenarioEnum::kOccupied; + keepA.presetHandle = DataModel::MakeNullable(ByteSpan(handleA)); + keepA.builtIn = DataModel::MakeNullable(true); + Structs::PresetStruct::Type newList[] = { keepA }; + auto listPayload = DataModel::List(newList, 1); + ASSERT_EQ(tester.WriteAttribute(Attributes::Presets::Id, listPayload, ListWritingPattern::ReplaceAll), Status::Success); + + Commands::AtomicRequest::Type commitReq; + commitReq.requestType = AtomicRequestTypeEnum::kCommitWrite; + commitReq.attributeRequests = DataModel::List(attrIds, 1); + ASSERT_TRUE(tester.Invoke(commitReq).IsSuccess()); + + ASSERT_EQ(mSuggestionsDelegate.mSuggestions.size(), 1u); + EXPECT_EQ(mSuggestionsDelegate.mSuggestions[0].GetUniqueID(), 9); + ASSERT_FALSE(mSuggestionsDelegate.mCurrentSuggestion.IsNull()); + EXPECT_EQ(mSuggestionsDelegate.mCurrentSuggestion.Value().GetUniqueID(), 9); + + cluster.Shutdown(ClusterShutdownType::kClusterShutdown); +} + +TEST_F(ThermostatTestFixture, TestPresetRemovalCascadeAbortsOnEnumerationFailure) +{ + BitFlags features(Feature::kHeating, Feature::kCooling, Feature::kPresets, Feature::kThermostatSuggestions); + + PresetStructWithOwnedMembers presetA; + presetA.SetPresetScenario(PresetScenarioEnum::kOccupied); + uint8_t handleA[4] = { 1, 1, 1, 1 }; + EXPECT_EQ(presetA.SetPresetHandle(DataModel::MakeNullable(ByteSpan(handleA))), CHIP_NO_ERROR); + presetA.SetBuiltIn(DataModel::MakeNullable(true)); + mPresetsDelegate.mPresets.push_back(presetA); + + Structs::PresetTypeStruct::Type ptype; + ptype.presetScenario = PresetScenarioEnum::kOccupied; + ptype.numberOfPresets = 5; + mPresetsDelegate.mPresetTypes.push_back(ptype); + + // A suggestion referencing a preset that will no longer exist after the atomic write below. + uint8_t handleB[4] = { 2, 2, 2, 2 }; + ThermostatSuggestionStructWithOwnedMembers suggestion; + suggestion.SetUniqueID(3); + EXPECT_EQ(suggestion.SetPresetHandle(ByteSpan(handleB)), CHIP_NO_ERROR); + suggestion.SetEffectiveTime(Seconds32(0)); + suggestion.SetExpirationTime(Seconds32(1000000)); + mSuggestionsDelegate.mSuggestions.push_back(suggestion); + mSuggestionsDelegate.mCurrentSuggestion.SetNonNull(suggestion); + + ThermostatCluster cluster(kTestEndpointId, features, MakeConfig(), mThermostatDelegate, mHeatingDelegate, mCoolingDelegate, + mPresetsDelegate, mSuggestionsDelegate); + ClusterTester tester(cluster); + SetupTesterSubject(tester); + ASSERT_EQ(cluster.Startup(tester.GetServerClusterContext()), CHIP_NO_ERROR); + + Commands::AtomicRequest::Type beginReq; + beginReq.requestType = AtomicRequestTypeEnum::kBeginWrite; + chip::AttributeId attrIds[] = { Attributes::Presets::Id }; + beginReq.attributeRequests = DataModel::List(attrIds, 1); + beginReq.timeout = MakeOptional(static_cast(5000)); + ASSERT_TRUE(tester.Invoke(beginReq).IsSuccess()); + + Structs::PresetStruct::Type keepA; + keepA.presetScenario = PresetScenarioEnum::kOccupied; + keepA.presetHandle = DataModel::MakeNullable(ByteSpan(handleA)); + keepA.builtIn = DataModel::MakeNullable(true); + Structs::PresetStruct::Type newList[] = { keepA }; + auto listPayload = DataModel::List(newList, 1); + ASSERT_EQ(tester.WriteAttribute(Attributes::Presets::Id, listPayload, ListWritingPattern::ReplaceAll), Status::Success); + + // The Presets commit itself will enumerate the (now single-entry) committed list cleanly; only fail the + // first call the cascade's own scan makes afterwards, to exercise its abort-on-error path specifically + // rather than an unrelated failure earlier in PrecommitPresets' own enumeration. + mPresetsDelegate.mFailGetPresetAtIndexOnCall = 4; + + Commands::AtomicRequest::Type commitReq; + commitReq.requestType = AtomicRequestTypeEnum::kCommitWrite; + commitReq.attributeRequests = DataModel::List(attrIds, 1); + ASSERT_TRUE(tester.Invoke(commitReq).IsSuccess()); + + // The Presets commit itself succeeded; the cascade aborted on the enumeration failure without removing + // anything, rather than treating the failure as "preset not found" and dropping the suggestion. + ASSERT_EQ(mPresetsDelegate.mPresets.size(), 1u); + EXPECT_EQ(mSuggestionsDelegate.mSuggestions.size(), 1u); + EXPECT_FALSE(mSuggestionsDelegate.mCurrentSuggestion.IsNull()); + + cluster.Shutdown(ClusterShutdownType::kClusterShutdown); +} + +TEST_F(ThermostatTestFixture, TestPresetRemovalCascadeAbortsOnSuggestionLookupFailure) +{ + BitFlags features(Feature::kHeating, Feature::kCooling, Feature::kPresets, Feature::kThermostatSuggestions); + + PresetStructWithOwnedMembers presetA; + presetA.SetPresetScenario(PresetScenarioEnum::kOccupied); + uint8_t handleA[4] = { 1, 1, 1, 1 }; + EXPECT_EQ(presetA.SetPresetHandle(DataModel::MakeNullable(ByteSpan(handleA))), CHIP_NO_ERROR); + presetA.SetBuiltIn(DataModel::MakeNullable(true)); + mPresetsDelegate.mPresets.push_back(presetA); + + Structs::PresetTypeStruct::Type ptype; + ptype.presetScenario = PresetScenarioEnum::kOccupied; + ptype.numberOfPresets = 5; + mPresetsDelegate.mPresetTypes.push_back(ptype); + + // A suggestion referencing a preset that will no longer exist after the atomic write below. + uint8_t handleB[4] = { 2, 2, 2, 2 }; + ThermostatSuggestionStructWithOwnedMembers suggestion; + suggestion.SetUniqueID(3); + EXPECT_EQ(suggestion.SetPresetHandle(ByteSpan(handleB)), CHIP_NO_ERROR); + suggestion.SetEffectiveTime(Seconds32(0)); + suggestion.SetExpirationTime(Seconds32(1000000)); + mSuggestionsDelegate.mSuggestions.push_back(suggestion); + mSuggestionsDelegate.mCurrentSuggestion.SetNonNull(suggestion); + + ThermostatCluster cluster(kTestEndpointId, features, MakeConfig(), mThermostatDelegate, mHeatingDelegate, mCoolingDelegate, + mPresetsDelegate, mSuggestionsDelegate); + ClusterTester tester(cluster); + SetupTesterSubject(tester); + ASSERT_EQ(cluster.Startup(tester.GetServerClusterContext()), CHIP_NO_ERROR); + + Commands::AtomicRequest::Type beginReq; + beginReq.requestType = AtomicRequestTypeEnum::kBeginWrite; + chip::AttributeId attrIds[] = { Attributes::Presets::Id }; + beginReq.attributeRequests = DataModel::List(attrIds, 1); + beginReq.timeout = MakeOptional(static_cast(5000)); + ASSERT_TRUE(tester.Invoke(beginReq).IsSuccess()); + + Structs::PresetStruct::Type keepA; + keepA.presetScenario = PresetScenarioEnum::kOccupied; + keepA.presetHandle = DataModel::MakeNullable(ByteSpan(handleA)); + keepA.builtIn = DataModel::MakeNullable(true); + Structs::PresetStruct::Type newList[] = { keepA }; + auto listPayload = DataModel::List(newList, 1); + ASSERT_EQ(tester.WriteAttribute(Attributes::Presets::Id, listPayload, ListWritingPattern::ReplaceAll), Status::Success); + + // Fail the cascade's own first (lookup) pass over ThermostatSuggestions itself, rather than its scan of + // Presets: this exercises GetThermostatSuggestionAtIndex()'s error path (as opposed to + // TestPresetRemovalCascadeAbortsOnEnumerationFailure, which exercises PresetHandleStillExists()'s). + mSuggestionsDelegate.mFailGetThermostatSuggestionAtIndexOnCall = 1; + + Commands::AtomicRequest::Type commitReq; + commitReq.requestType = AtomicRequestTypeEnum::kCommitWrite; + commitReq.attributeRequests = DataModel::List(attrIds, 1); + ASSERT_TRUE(tester.Invoke(commitReq).IsSuccess()); + + // The Presets commit itself succeeded; the cascade aborted on the lookup failure without removing anything or + // re-evaluating the current suggestion. + ASSERT_EQ(mPresetsDelegate.mPresets.size(), 1u); + EXPECT_EQ(mSuggestionsDelegate.mSuggestions.size(), 1u); + EXPECT_FALSE(mSuggestionsDelegate.mCurrentSuggestion.IsNull()); + EXPECT_FALSE(mSuggestionsDelegate.mReEvaluateCalled); + EXPECT_FALSE(tester.IsAttributeDirty(Attributes::ThermostatSuggestions::Id)); + + cluster.Shutdown(ClusterShutdownType::kClusterShutdown); +} + +TEST_F(ThermostatTestFixture, TestPresetRemovalCascadeAbortsOnRemovalFailure) +{ + BitFlags features(Feature::kHeating, Feature::kCooling, Feature::kPresets, Feature::kThermostatSuggestions); + + Structs::PresetTypeStruct::Type ptype; + ptype.presetScenario = PresetScenarioEnum::kOccupied; + ptype.numberOfPresets = 5; + mPresetsDelegate.mPresetTypes.push_back(ptype); + + // No presets survive the atomic write below, so both suggestions below become stale. + uint8_t handleB[4] = { 2, 2, 2, 2 }; + uint8_t handleC[4] = { 3, 3, 3, 3 }; + + ThermostatSuggestionStructWithOwnedMembers suggestion1; + suggestion1.SetUniqueID(11); + EXPECT_EQ(suggestion1.SetPresetHandle(ByteSpan(handleB)), CHIP_NO_ERROR); + suggestion1.SetEffectiveTime(Seconds32(0)); + suggestion1.SetExpirationTime(Seconds32(1000000)); + mSuggestionsDelegate.mSuggestions.push_back(suggestion1); + + ThermostatSuggestionStructWithOwnedMembers suggestion2; + suggestion2.SetUniqueID(12); + EXPECT_EQ(suggestion2.SetPresetHandle(ByteSpan(handleC)), CHIP_NO_ERROR); + suggestion2.SetEffectiveTime(Seconds32(0)); + suggestion2.SetExpirationTime(Seconds32(1000000)); + mSuggestionsDelegate.mSuggestions.push_back(suggestion2); + + ThermostatCluster cluster(kTestEndpointId, features, MakeConfig(), mThermostatDelegate, mHeatingDelegate, mCoolingDelegate, + mPresetsDelegate, mSuggestionsDelegate); + ClusterTester tester(cluster); + SetupTesterSubject(tester); + ASSERT_EQ(cluster.Startup(tester.GetServerClusterContext()), CHIP_NO_ERROR); + + Commands::AtomicRequest::Type beginReq; + beginReq.requestType = AtomicRequestTypeEnum::kBeginWrite; + chip::AttributeId attrIds[] = { Attributes::Presets::Id }; + beginReq.attributeRequests = DataModel::List(attrIds, 1); + beginReq.timeout = MakeOptional(static_cast(5000)); + ASSERT_TRUE(tester.Invoke(beginReq).IsSuccess()); + + // Remove every preset, so both suggestions above become stale. + DataModel::List emptyListPayload; + ASSERT_EQ(tester.WriteAttribute(Attributes::Presets::Id, emptyListPayload, ListWritingPattern::ReplaceAll), Status::Success); + + // Fail the cascade's second-pass removal of the lower-index stale entry, after it has already removed the + // higher-index one (the backward walk visits index 1 first, then index 0). + mSuggestionsDelegate.mFailRemoveFromThermostatSuggestionsListOnCall = 2; + + Commands::AtomicRequest::Type commitReq; + commitReq.requestType = AtomicRequestTypeEnum::kCommitWrite; + commitReq.attributeRequests = DataModel::List(attrIds, 1); + ASSERT_TRUE(tester.Invoke(commitReq).IsSuccess()); + + // The higher-index entry was removed before the failure, but the lower-index one was not: the cascade is + // partial, so it must report incomplete and OnPresetsCommitted() must skip re-evaluating the current + // suggestion rather than treat the partial cleanup as done. + ASSERT_EQ(mSuggestionsDelegate.mSuggestions.size(), 1u); + EXPECT_EQ(mSuggestionsDelegate.mSuggestions[0].GetUniqueID(), 11); + EXPECT_TRUE(tester.IsAttributeDirty(Attributes::ThermostatSuggestions::Id)); + EXPECT_FALSE(mSuggestionsDelegate.mReEvaluateCalled); + EXPECT_FALSE(tester.IsAttributeDirty(CurrentThermostatSuggestion::Id)); + + cluster.Shutdown(ClusterShutdownType::kClusterShutdown); +} + +TEST_F(ThermostatTestFixture, TestPresetRemovalCascadeReportsCurrentSuggestionChangeOnPartialAbort) +{ + BitFlags features(Feature::kHeating, Feature::kCooling, Feature::kPresets, Feature::kThermostatSuggestions); + + Structs::PresetTypeStruct::Type ptype; + ptype.presetScenario = PresetScenarioEnum::kOccupied; + ptype.numberOfPresets = 5; + mPresetsDelegate.mPresetTypes.push_back(ptype); + + // No presets survive the atomic write below, so both suggestions below become stale. The backward removal + // walk visits the current suggestion (index 1) before the other one (index 0), so it is removed successfully + // before the failure below aborts the cascade partway through. + uint8_t handleB[4] = { 2, 2, 2, 2 }; + uint8_t handleC[4] = { 3, 3, 3, 3 }; + + ThermostatSuggestionStructWithOwnedMembers otherSuggestion; + otherSuggestion.SetUniqueID(31); + EXPECT_EQ(otherSuggestion.SetPresetHandle(ByteSpan(handleC)), CHIP_NO_ERROR); + otherSuggestion.SetEffectiveTime(Seconds32(0)); + otherSuggestion.SetExpirationTime(Seconds32(1000000)); + mSuggestionsDelegate.mSuggestions.push_back(otherSuggestion); + + ThermostatSuggestionStructWithOwnedMembers currentSuggestion; + currentSuggestion.SetUniqueID(32); + EXPECT_EQ(currentSuggestion.SetPresetHandle(ByteSpan(handleB)), CHIP_NO_ERROR); + currentSuggestion.SetEffectiveTime(Seconds32(0)); + currentSuggestion.SetExpirationTime(Seconds32(1000000)); + mSuggestionsDelegate.mSuggestions.push_back(currentSuggestion); + mSuggestionsDelegate.mCurrentSuggestion.SetNonNull(currentSuggestion); + + ThermostatCluster cluster(kTestEndpointId, features, MakeConfig(), mThermostatDelegate, mHeatingDelegate, mCoolingDelegate, + mPresetsDelegate, mSuggestionsDelegate); + ClusterTester tester(cluster); + SetupTesterSubject(tester); + ASSERT_EQ(cluster.Startup(tester.GetServerClusterContext()), CHIP_NO_ERROR); + + Commands::AtomicRequest::Type beginReq; + beginReq.requestType = AtomicRequestTypeEnum::kBeginWrite; + chip::AttributeId attrIds[] = { Attributes::Presets::Id }; + beginReq.attributeRequests = DataModel::List(attrIds, 1); + beginReq.timeout = MakeOptional(static_cast(5000)); + ASSERT_TRUE(tester.Invoke(beginReq).IsSuccess()); + + // Remove every preset, so both suggestions above become stale. + DataModel::List emptyListPayload; + ASSERT_EQ(tester.WriteAttribute(Attributes::Presets::Id, emptyListPayload, ListWritingPattern::ReplaceAll), Status::Success); + + // Fail the cascade's second-pass removal of the lower-index (non-current) entry, after it has already + // removed the higher-index current suggestion (the backward walk visits index 1 first, then index 0). + mSuggestionsDelegate.mFailRemoveFromThermostatSuggestionsListOnCall = 2; + + Commands::AtomicRequest::Type commitReq; + commitReq.requestType = AtomicRequestTypeEnum::kCommitWrite; + commitReq.attributeRequests = DataModel::List(attrIds, 1); + ASSERT_TRUE(tester.Invoke(commitReq).IsSuccess()); + + // The current suggestion was really removed (per RemoveFromThermostatSuggestionsList()'s API contract) before + // the cascade aborted, so that transition is real and must be reported, even though the cascade as a whole is + // incomplete and must still skip re-evaluating a replacement. + ASSERT_EQ(mSuggestionsDelegate.mSuggestions.size(), 1u); + EXPECT_EQ(mSuggestionsDelegate.mSuggestions[0].GetUniqueID(), 31); + EXPECT_TRUE(mSuggestionsDelegate.mCurrentSuggestion.IsNull()); + EXPECT_FALSE(mSuggestionsDelegate.mReEvaluateCalled); + EXPECT_TRUE(tester.IsAttributeDirty(Attributes::ThermostatSuggestions::Id)); + EXPECT_TRUE(tester.IsAttributeDirty(CurrentThermostatSuggestion::Id)); + + cluster.Shutdown(ClusterShutdownType::kClusterShutdown); +} + +TEST_F(ThermostatTestFixture, TestPresetRemovalCascadeDoesNotDoubleNotifyCurrentSuggestion) +{ + BitFlags features(Feature::kHeating, Feature::kCooling, Feature::kPresets, Feature::kThermostatSuggestions); + + PresetStructWithOwnedMembers presetA; + presetA.SetPresetScenario(PresetScenarioEnum::kOccupied); + uint8_t handleA[4] = { 1, 1, 1, 1 }; + EXPECT_EQ(presetA.SetPresetHandle(DataModel::MakeNullable(ByteSpan(handleA))), CHIP_NO_ERROR); + presetA.SetBuiltIn(DataModel::MakeNullable(true)); + mPresetsDelegate.mPresets.push_back(presetA); + + PresetStructWithOwnedMembers presetB; + presetB.SetPresetScenario(PresetScenarioEnum::kOccupied); + uint8_t handleB[4] = { 2, 2, 2, 2 }; + EXPECT_EQ(presetB.SetPresetHandle(DataModel::MakeNullable(ByteSpan(handleB))), CHIP_NO_ERROR); + presetB.SetBuiltIn(DataModel::MakeNullable(false)); + mPresetsDelegate.mPresets.push_back(presetB); + + Structs::PresetTypeStruct::Type ptype; + ptype.presetScenario = PresetScenarioEnum::kOccupied; + ptype.numberOfPresets = 5; + mPresetsDelegate.mPresetTypes.push_back(ptype); + + // A surviving suggestion referencing preset A, and a stale one referencing preset B that is the current + // suggestion. Removing the current suggestion's preset must let the surviving suggestion become the new + // current one, reported changed exactly once: ReEvaluateCurrentSuggestion() already notifies that internally, + // so OnPresetsCommitted()'s own before/after diff must not notify it a second time. + ThermostatSuggestionStructWithOwnedMembers survivingSuggestion; + survivingSuggestion.SetUniqueID(21); + EXPECT_EQ(survivingSuggestion.SetPresetHandle(ByteSpan(handleA)), CHIP_NO_ERROR); + survivingSuggestion.SetEffectiveTime(Seconds32(0)); + survivingSuggestion.SetExpirationTime(Seconds32(1000000)); + mSuggestionsDelegate.mSuggestions.push_back(survivingSuggestion); + + ThermostatSuggestionStructWithOwnedMembers staleCurrentSuggestion; + staleCurrentSuggestion.SetUniqueID(22); + EXPECT_EQ(staleCurrentSuggestion.SetPresetHandle(ByteSpan(handleB)), CHIP_NO_ERROR); + staleCurrentSuggestion.SetEffectiveTime(Seconds32(0)); + staleCurrentSuggestion.SetExpirationTime(Seconds32(1000000)); + mSuggestionsDelegate.mSuggestions.push_back(staleCurrentSuggestion); + mSuggestionsDelegate.mCurrentSuggestion.SetNonNull(staleCurrentSuggestion); + + ThermostatCluster cluster(kTestEndpointId, features, MakeConfig(), mThermostatDelegate, mHeatingDelegate, mCoolingDelegate, + mPresetsDelegate, mSuggestionsDelegate); + ClusterTester tester(cluster); + SetupTesterSubject(tester); + ASSERT_EQ(cluster.Startup(tester.GetServerClusterContext()), CHIP_NO_ERROR); + + Commands::AtomicRequest::Type beginReq; + beginReq.requestType = AtomicRequestTypeEnum::kBeginWrite; + chip::AttributeId attrIds[] = { Attributes::Presets::Id }; + beginReq.attributeRequests = DataModel::List(attrIds, 1); + beginReq.timeout = MakeOptional(static_cast(5000)); + ASSERT_TRUE(tester.Invoke(beginReq).IsSuccess()); + + Structs::PresetStruct::Type keepA; + keepA.presetScenario = PresetScenarioEnum::kOccupied; + keepA.presetHandle = DataModel::MakeNullable(ByteSpan(handleA)); + keepA.builtIn = DataModel::MakeNullable(true); + Structs::PresetStruct::Type newList[] = { keepA }; + auto listPayload = DataModel::List(newList, 1); + ASSERT_EQ(tester.WriteAttribute(Attributes::Presets::Id, listPayload, ListWritingPattern::ReplaceAll), Status::Success); + + Commands::AtomicRequest::Type commitReq; + commitReq.requestType = AtomicRequestTypeEnum::kCommitWrite; + commitReq.attributeRequests = DataModel::List(attrIds, 1); + ASSERT_TRUE(tester.Invoke(commitReq).IsSuccess()); + + ASSERT_EQ(mSuggestionsDelegate.mSuggestions.size(), 1u); + EXPECT_EQ(mSuggestionsDelegate.mSuggestions[0].GetUniqueID(), 21); + ASSERT_FALSE(mSuggestionsDelegate.mCurrentSuggestion.IsNull()); + EXPECT_EQ(mSuggestionsDelegate.mCurrentSuggestion.Value().GetUniqueID(), 21); + + auto & dirtyList = tester.GetDirtyList(); + auto currentSuggestionPath = + ConcreteAttributePath(cluster.GetPaths()[0].mEndpointId, cluster.GetPaths()[0].mClusterId, CurrentThermostatSuggestion::Id); + EXPECT_EQ(std::count(dirtyList.begin(), dirtyList.end(), currentSuggestionPath), 1); + + cluster.Shutdown(ClusterShutdownType::kClusterShutdown); +} + } // namespace diff --git a/src/app/clusters/thermostat-server/tests/ThermostatTestCommon.h b/src/app/clusters/thermostat-server/tests/ThermostatTestCommon.h index 5212bba13ec6..a3560d89cf4d 100644 --- a/src/app/clusters/thermostat-server/tests/ThermostatTestCommon.h +++ b/src/app/clusters/thermostat-server/tests/ThermostatTestCommon.h @@ -387,6 +387,11 @@ class MockPresetsDelegate : public ThermostatPresets::Delegate CHIP_ERROR GetPresetAtIndex(size_t index, PresetStructWithOwnedMembers & preset) override { + mGetPresetAtIndexCallCount++; + if (mFailGetPresetAtIndexOnCall.has_value() && mGetPresetAtIndexCallCount == *mFailGetPresetAtIndexOnCall) + { + return CHIP_ERROR_INTERNAL; + } if (index >= mPresets.size()) { return CHIP_ERROR_PROVIDER_LIST_EXHAUSTED; @@ -475,6 +480,11 @@ class MockPresetsDelegate : public ThermostatPresets::Delegate CHIP_ERROR mSetActivePresetHandleError = CHIP_NO_ERROR; CHIP_ERROR mCommitPendingPresetsError = CHIP_NO_ERROR; std::optional mMaxAtomicWriteTimeout = System::Clock::Milliseconds16(10000); + // When set, the Nth call (1-based, across all indices) to GetPresetAtIndex() returns CHIP_ERROR_INTERNAL + // instead of consulting mPresets. Used to simulate a transient delegate enumeration failure at a specific + // point in a caller's scan, regardless of earlier unrelated scans over the same list. + size_t mGetPresetAtIndexCallCount = 0; + std::optional mFailGetPresetAtIndexOnCall; }; class MockSuggestionsDelegate : public ThermostatSuggestions::Delegate @@ -485,6 +495,12 @@ class MockSuggestionsDelegate : public ThermostatSuggestions::Delegate CHIP_ERROR GetThermostatSuggestionAtIndex(size_t index, ThermostatSuggestionStructWithOwnedMembers & suggestion) override { + mGetThermostatSuggestionAtIndexCallCount++; + if (mFailGetThermostatSuggestionAtIndexOnCall.has_value() && + mGetThermostatSuggestionAtIndexCallCount == *mFailGetThermostatSuggestionAtIndexOnCall) + { + return CHIP_ERROR_INTERNAL; + } if (index >= mSuggestions.size()) { return CHIP_ERROR_PROVIDER_LIST_EXHAUSTED; @@ -525,10 +541,21 @@ class MockSuggestionsDelegate : public ThermostatSuggestions::Delegate CHIP_ERROR RemoveFromThermostatSuggestionsList(size_t index) override { + mRemoveFromThermostatSuggestionsListCallCount++; + if (mFailRemoveFromThermostatSuggestionsListOnCall.has_value() && + mRemoveFromThermostatSuggestionsListCallCount == *mFailRemoveFromThermostatSuggestionsListOnCall) + { + return CHIP_ERROR_INTERNAL; + } if (index >= mSuggestions.size()) { return CHIP_ERROR_NOT_FOUND; } + // Per the API contract, removing the entry that is CurrentThermostatSuggestion nulls it out. + if (!mCurrentSuggestion.IsNull() && mCurrentSuggestion.Value().GetUniqueID() == mSuggestions[index].GetUniqueID()) + { + mCurrentSuggestion.SetNull(); + } mSuggestions.erase(mSuggestions.begin() + static_cast(index)); return CHIP_NO_ERROR; } @@ -552,6 +579,16 @@ class MockSuggestionsDelegate : public ThermostatSuggestions::Delegate bool mFailGetUniqueID = false; bool mFailAppend = false; bool mReEvaluateCalled = false; + // When set, the Nth call (1-based, across all indices) to GetThermostatSuggestionAtIndex() returns + // CHIP_ERROR_INTERNAL instead of consulting mSuggestions. Used to simulate a transient delegate enumeration + // failure at a specific point in a caller's scan. + size_t mGetThermostatSuggestionAtIndexCallCount = 0; + std::optional mFailGetThermostatSuggestionAtIndexOnCall; + // When set, the Nth call (1-based) to RemoveFromThermostatSuggestionsList() returns CHIP_ERROR_INTERNAL instead + // of removing the entry. Used to simulate a transient delegate removal failure partway through a caller's + // removal loop. + size_t mRemoveFromThermostatSuggestionsListCallCount = 0; + std::optional mFailRemoveFromThermostatSuggestionsListOnCall; std::vector mSuggestions; DataModel::Nullable mCurrentSuggestion = DataModel::NullNullable; DataModel::Nullable mNotFollowingReason = DataModel::NullNullable;