[HVAC] Handle null BuiltIn field on Preset write according to spec

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

@@ -58,7 +58,7 @@ class ThermostatDelegate : public Delegate
 
     void InitializePendingPresets() override;
 
-    CHIP_ERROR AppendToPendingPresetList(const Structs::PresetStruct::Type & preset) override;
+    CHIP_ERROR AppendToPendingPresetList(const PresetStructWithOwnedMembers & preset) override;
 
     CHIP_ERROR GetPendingPresetAtIndex(size_t index, PresetStructWithOwnedMembers & preset) override;
 

examples/thermostat/thermostat-common/src/thermostat-delegate-impl.cpp

@@ -177,17 +177,17 @@ void ThermostatDelegate::InitializePendingPresets()
     }
 }
 
-CHIP_ERROR ThermostatDelegate::AppendToPendingPresetList(const PresetStruct::Type & preset)
+CHIP_ERROR ThermostatDelegate::AppendToPendingPresetList(const PresetStructWithOwnedMembers & preset)
 {
     if (mNextFreeIndexInPendingPresetsList < ArraySize(mPendingPresets))
     {
         mPendingPresets[mNextFreeIndexInPendingPresetsList] = preset;
-        if (preset.presetHandle.IsNull())
+        if (preset.GetPresetHandle().IsNull())
         {
             // 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>(preset.presetScenario) };
+            const uint8_t handle[] = { static_cast<uint8_t>(preset.GetPresetScenario()) };
             mPendingPresets[mNextFreeIndexInPendingPresetsList].SetPresetHandle(DataModel::MakeNullable(ByteSpan(handle)));
         }
         mNextFreeIndexInPendingPresetsList++;

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

@@ -103,7 +103,7 @@ class Delegate
      * @return CHIP_NO_ERROR if the preset was appended to the list successfully.
      * @return CHIP_ERROR if there was an error adding the preset to the list.
      */
-    virtual CHIP_ERROR AppendToPendingPresetList(const Structs::PresetStruct::Type & preset) = 0;
+    virtual CHIP_ERROR AppendToPendingPresetList(const PresetStructWithOwnedMembers & preset) = 0;
 
     /**
      * @brief Get the Preset at a given index in the pending presets list.

src/app/clusters/thermostat-server/thermostat-server-presets.cpp

@@ -38,16 +38,16 @@ namespace {
  * @return true If the preset is valid i.e the PresetHandle (if not null) fits within size constraints and the presetScenario enum
  *         value is valid. Otherwise, return false.
  */
-bool IsValidPresetEntry(const PresetStruct::Type & preset)
+bool IsValidPresetEntry(const PresetStructWithOwnedMembers & preset)
 {
     // Check that the preset handle is not too long.
-    if (!preset.presetHandle.IsNull() && preset.presetHandle.Value().size() > kPresetHandleSize)
+    if (!preset.GetPresetHandle().IsNull() && preset.GetPresetHandle().Value().size() > kPresetHandleSize)
     {
         return false;
     }
 
     // Ensure we have a valid PresetScenario.
-    return (preset.presetScenario != PresetScenarioEnum::kUnknownEnumValue);
+    return (preset.GetPresetScenario() != PresetScenarioEnum::kUnknownEnumValue);
 }
 
 /**
@@ -123,7 +123,7 @@ bool MatchingPendingPresetExists(Delegate * delegate, const PresetStructWithOwne
  *
  * @return true if a matching entry was found in the  presets attribute list, false otherwise.
  */
-bool GetMatchingPresetInPresets(Delegate * delegate, const PresetStruct::Type & presetToMatch,
+bool GetMatchingPresetInPresets(Delegate * delegate, const DataModel::Nullable<ByteSpan> & presetHandle,
                                 PresetStructWithOwnedMembers & matchingPreset)
 {
     VerifyOrReturnValue(delegate != nullptr, false);
@@ -143,7 +143,7 @@ bool GetMatchingPresetInPresets(Delegate * delegate, const PresetStruct::Type &
         }
 
         // Note: presets coming from our delegate always have a handle.
-        if (presetToMatch.presetHandle.Value().data_equal(matchingPreset.GetPresetHandle().Value()))
+        if (presetHandle.Value().data_equal(matchingPreset.GetPresetHandle().Value()))
         {
             return true;
         }
@@ -351,53 +351,71 @@ Status ThermostatAttrAccess::SetActivePreset(EndpointId endpoint, DataModel::Nul
     return Status::Success;
 }
 
-CHIP_ERROR ThermostatAttrAccess::AppendPendingPreset(Thermostat::Delegate * delegate, const PresetStruct::Type & preset)
+CHIP_ERROR ThermostatAttrAccess::AppendPendingPreset(Thermostat::Delegate * delegate, const PresetStruct::Type & newPreset)
 {
+    PresetStructWithOwnedMembers preset = newPreset;
     if (!IsValidPresetEntry(preset))
     {
         return CHIP_IM_GLOBAL_STATUS(ConstraintError);
     }
 
-    if (preset.presetHandle.IsNull())
+    if (preset.GetPresetHandle().IsNull())
     {
         if (IsBuiltIn(preset))
         {
             return CHIP_IM_GLOBAL_STATUS(ConstraintError);
         }
+        // Force to be false, if passed as null
+        preset.SetBuiltIn(false);
     }
     else
     {
-        auto & presetHandle = preset.presetHandle.Value();
-
         // Per spec we need to check that:
         // (a) There is an existing non-pending preset with this handle.
         PresetStructWithOwnedMembers matchingPreset;
-        if (!GetMatchingPresetInPresets(delegate, preset, matchingPreset))
+        if (!GetMatchingPresetInPresets(delegate, preset.GetPresetHandle().Value(), matchingPreset))
         {
             return CHIP_IM_GLOBAL_STATUS(NotFound);
         }
 
         // (b) There is no existing pending preset with this handle.
-        if (CountPresetsInPendingListWithPresetHandle(delegate, presetHandle) > 0)
+        if (CountPresetsInPendingListWithPresetHandle(delegate, preset.GetPresetHandle().Value()) > 0)
         {
             return CHIP_IM_GLOBAL_STATUS(ConstraintError);
         }
 
+        const auto & presetBuiltIn         = preset.GetBuiltIn();
+        const auto & matchingPresetBuiltIn = matchingPreset.GetBuiltIn();
         // (c)/(d) The built-in fields do not have a mismatch.
-        // TODO: What's the story with nullability on the BuiltIn field?
-        if (!preset.builtIn.IsNull() && !matchingPreset.GetBuiltIn().IsNull() &&
-            preset.builtIn.Value() != matchingPreset.GetBuiltIn().Value())
+        if (presetBuiltIn.IsNull())
         {
-            return CHIP_IM_GLOBAL_STATUS(ConstraintError);
+            if (matchingPresetBuiltIn.IsNull())
+            {
+                // This really shouldn't happen; internal presets should alway have built-in set
+                return CHIP_IM_GLOBAL_STATUS(InvalidInState);
+            }
+            preset.SetBuiltIn(matchingPresetBuiltIn.Value());
+        }
+        else
+        {
+            if (matchingPresetBuiltIn.IsNull())
+            {
+                // This really shouldn't happen; internal presets should alway have built-in set
+                return CHIP_IM_GLOBAL_STATUS(InvalidInState);
+            }
+            if (presetBuiltIn.Value() != matchingPresetBuiltIn.Value())
+            {
+                return CHIP_IM_GLOBAL_STATUS(ConstraintError);
+            }
         }
     }
 
-    if (!PresetScenarioExistsInPresetTypes(delegate, preset.presetScenario))
+    if (!PresetScenarioExistsInPresetTypes(delegate, preset.GetPresetScenario()))
     {
         return CHIP_IM_GLOBAL_STATUS(ConstraintError);
     }
 
-    if (preset.name.HasValue() && !PresetTypeSupportsNames(delegate, preset.presetScenario))
+    if (preset.GetName().HasValue() && !PresetTypeSupportsNames(delegate, preset.GetPresetScenario()))
     {
         return CHIP_IM_GLOBAL_STATUS(ConstraintError);
     }

src/python_testing/TC_TSTAT_4_2.py

@@ -246,8 +246,12 @@ async def test_TC_TSTAT_4_2(self):
         if self.pics_guard(self.check_pics("TSTAT.S.F08") and self.check_pics("TSTAT.S.A0050") and self.check_pics("TSTAT.S.Cfe.Rsp")):
             await self.send_atomic_request_begin_command()
 
+            # Set the new preset to a null built-in value; will be replaced with false on reading
+            test_presets = copy.deepcopy(new_presets)
+            test_presets[2].builtIn = NullValue
+
             # Write to the presets attribute after calling AtomicRequest command
-            status = await self.write_presets(endpoint=endpoint, presets=new_presets)
+            status = await self.write_presets(endpoint=endpoint, presets=test_presets)
             status_ok = (status == Status.Success)
             asserts.assert_true(status_ok, "Presets write did not return Success as expected")
 
@@ -268,8 +272,12 @@ async def test_TC_TSTAT_4_2(self):
             # Send the AtomicRequest begin command
             await self.send_atomic_request_begin_command()
 
+            # Set the existing preset to a null built-in value; will be replaced with true on reading
+            test_presets = copy.deepcopy(new_presets)
+            test_presets[0].builtIn = NullValue
+
             # Write to the presets attribute after calling AtomicRequest command
-            await self.write_presets(endpoint=endpoint, presets=new_presets)
+            await self.write_presets(endpoint=endpoint, presets=test_presets)
 
             # Send the AtomicRequest commit command
             await self.send_atomic_request_commit_command()