[HVAC] Handle null BuiltIn field on Preset write according to spec (#35161)
* Add support for Presets attributes and commands to the Thermostat cluster
Clean up the Thermostat cluster and remove the TemperatureSetpointHoldPolicy attribute
and SetTemperatureSetpointHoldPolicy command
* Restyled by whitespace
* Restyled by clang-format
* Restyled by gn.
* Fix build error for Linux configure build of all-clusters-app
* Fix Darwin CI issues
Editorial fixes
* Restyled by clang-format
* More fixes
* Restyled by clang-format
* BUILD.gn fixes for CI
* Apply suggestions from code review
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
* Address review comments.
* Restyled by clang-format
* Regenerate Thermostat XML from spec
* Move atomic enum to global-enums.xml, actually
# Conflicts:
# src/app/zap-templates/zcl/data-model/chip/global-structs.xml
* Regenerate XML and convert thermostat-server to atomic writes
* Pull in ACCapacityFormat typo un-fix
* Update Test_TC_TSTAT_1_1 to know about AtomicResponse command.
* Restyled patch
* Fix weird merge with upstream
* Fix emberAfIsTypeSigned not understanding temperature type
* Merge fixes from atomic write branch
* Relocate thermostat-manager sample code to all-clusters-common
* Fix g++ build error on linux
* Fix C formatter for long int, cast whole expression
* Sync cast fix with master
* Add thermostat-common dependency to thermostat app under linux
* Remove MatterPostAttributeChangeCallback from thermostat-manager, as it conflicts with other implementations
* Convert Atomic enums and structs to global
* Restyled patch
* Apply suggestions from code review
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
* Regen with alchemy 0.6.1
* Updates based on comments
* Add TC_MCORE_FS_1_3.py test implementation (#34650)
* Fix most TC-SWTCH-2.4 remaining issues (#34677)
- Move 2.4 in a better place in the file
- Add test steps properly
- Allow default button press position override
Issue #34656
Testing done:
- Test still passes on DUT with automation
* Initial test script for Fabric Sync TC_MCORE_FS_1_2 (#34675)
* Initial test script for Fabric Sync TC_MCORE_FS_1_2
* Apply suggestions from code review
Co-authored-by: C Freeman <cecille@google.com>
* Address Review Comments
* Address review comments
* Fix default timeout after other timeouts changed
* Restyled by autopep8
* Fix linter error
---------
Co-authored-by: C Freeman <cecille@google.com>
Co-authored-by: Restyled.io <commits@restyled.io>
* Test automation for FabricSync ICD BridgedDeviceBasicInfoCluster (#34628)
* WIP Bridged ICD, commissioning to both fabrics
* wip testing sending KeepActive
* wip most steps implemented
* using SIGSTOP and SIGCONT to control ICD server pausing
* Update src/python_testing/TC_BRBINFO_4_1.py
Co-authored-by: Terence Hampson <thampson@google.com>
* comments addressed
* more comments addressed
* lint pass
* Update src/python_testing/TC_BRBINFO_4_1.py
Co-authored-by: C Freeman <cecille@google.com>
* comments addressed, incl TH_SERVER configurable
* added setupQRCode and setupManualCode as options for DUT commissioning
* Restyled by autopep8
* Restyled by isort
* Update src/python_testing/TC_BRBINFO_4_1.py
Co-authored-by: Terence Hampson <thampson@google.com>
* Update src/python_testing/TC_BRBINFO_4_1.py
Co-authored-by: Terence Hampson <thampson@google.com>
* Update src/python_testing/TC_BRBINFO_4_1.py
Co-authored-by: Terence Hampson <thampson@google.com>
* comments addressed
* Restyled by autopep8
---------
Co-authored-by: Terence Hampson <thampson@google.com>
Co-authored-by: C Freeman <cecille@google.com>
Co-authored-by: Restyled.io <commits@restyled.io>
* ServiceArea test scripts (#34548)
* initial commit
* fix bugs
* fix issues reported by the linter
* fix bug in checking for unique areaDesc
* add TC 1.5
* Update src/python_testing/TC_SEAR_1_2.py
Co-authored-by: William <hicklin@users.noreply.github.com>
* Update src/python_testing/TC_SEAR_1_2.py
Co-authored-by: William <hicklin@users.noreply.github.com>
* address code review comments
* fix issue introduced by the previous commit
* address code review feedback
* Update src/python_testing/TC_SEAR_1_2.py
Co-authored-by: Kiel Oleson <kielo@apple.com>
* address code review feedback
* remove PICS checked by the TC_SEAR_1.6
* more code review updates
* Restyled by autopep8
---------
Co-authored-by: William <hicklin@users.noreply.github.com>
Co-authored-by: Kiel Oleson <kielo@apple.com>
Co-authored-by: Restyled.io <commits@restyled.io>
* Remove manual tests for Thermostat presets (#34679)
* Dump details about leaked ExchangeContexts before aborting (#34617)
* Dump details about leaked ExchangeContexts before aborting
This is implemented via a VerifyOrDieWithObject() variant of the existing
VerifyOrDie() macro that calls a DumpToLog() method on the provided object if
it exists (otherwise this is simply a no-op).
If CHIP_CONFIG_VERBOSE_VERIFY_OR_DIE is not enabled, VerifyOrDieWithObject()
simply behaves like a plain VerifyOrDie(). DumpToLog() implementations can use
ChipLogFormatRtti to log type information about an object (usually a delegate);
if RTTI is disabled this simply outputs whether the object was null or not.
* Address review comments
* Make gcc happy and improve documentation
* Remove unused include
* Fix compile error without CHIP_CONFIG_VERBOSE_VERIFY_OR_DIE
* Avoid unused parameter warning
* [TI] CC13x4_26x4 build fixes (#34682)
* lwip pbuf, map file, and hex creation when OTA is disabled
* added cc13x4 family define around the non OTA hex creation
* whitespace fix
* reversed custom factoy data flash with cc13x4 check
* more whitespace fixes
* [ICD] Add missing polling function to NoWifi connectivity manager (#34684)
* Add missing polling function to NoWifi connectivity manager
* Update GenericConnectivityManagerImpl_NoWiFi.h
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
---------
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
* [OPSTATE] Add Q test script for CountdownTime (#34632)
* Add Q test
* Added test to test set
* Remove unused var
* Restyled by autopep8
* Restyled by isort
* Fix name
* Use pics over other method
* Removed unused stuff
* Added pipe commands
* Fix reset
* Get example to report appropriate changes.
* WiP
* Added some comments
* Changes to make things work
* Removed dev msgs
* Missed some
* Removed dev msgs
* Straggler
* Restyled by clang-format
* Restyled by autopep8
* Restyled by isort
* Commented unused var
* Update examples/all-clusters-app/linux/AllClustersCommandDelegate.cpp
* Fix bug
---------
Co-authored-by: Restyled.io <commits@restyled.io>
* YAML update to BRBINFO, ProductId (#34513)
* Bridged Device Information Cluster, Attribute ProductID test reflects marking as O, not X
* Update src/app/tests/suites/certification/Test_TC_BRBINFO_2_1.yaml
Co-authored-by: Terence Hampson <thampson@google.com>
* corrected pics
* corrected pics
* WIP Bridged ICD, commissioning to both fabrics
* wip testing sending KeepActive
* update to bridged-device-basic-information.xml and zap generated files
* removed unrelated file
---------
Co-authored-by: Terence Hampson <thampson@google.com>
Co-authored-by: Andrei Litvin <andy314@gmail.com>
* Fix simplified Linux tv-casting-app gn build error. (#34692)
* adding parallel execution to restyle-diff (#34663)
* adding parallel execution to restyle-diff
* using xargs to call restyle-paths
* fixing Copyright year
* restyle the restyler
* Add some bits to exercise global structs/enums to Unit Testing cluster. (#34540)
* Adds things to the Unit Testing cluster XML.
* This requires those things to be enabled in all-clusters-app,
all-clusters-minimal-app, and one of the chef contact sensors to pass CI.
* That requires an implementation in test-cluster-server
* At which point might as well add a YAML test to exercise it all.
* [Silabs] Port platform specific Multi-Chip OTA work (#34440)
* Pull request #1836: Cherry multi ota
Merge in WMN_TOOLS/matter from cherry-multi-ota to silabs_slc_1.3
Squashed commit of the following:
commit 4320bb46571658bc44fb82345348265def394991
Author: Michael Rupp <michael.rupp@silabs.com>
Date: Fri May 10 14:26:07 2024 -0400
remove some unwanted diffs in provision files
commit be160931dc600de7e7ead378b70d6a43c3945e46
Author: Michael Rupp <michael.rupp@silabs.com>
Date: Fri May 10 14:24:25 2024 -0400
revert changes to generator.project.mak
commit 14b6605887166e6d5284a61feb2bf407d850bdcf
Author: Michael Rupp <michael.rupp@silabs.com>
Date: Fri May 10 13:06:12 2024 -0400
revert NVM key changes and script changes
... and 8 more commits
* Restyled by whitespace
* Restyled by clang-format
* Restyled by gn
* Restyled by autopep8
* remove unused libs caught by linter
* update doctree with new readmes
* rerun CI, cirque failing for unknown reasons
* fix include guards in provision examples
* Restyled by clang-format
---------
Co-authored-by: Restyled.io <commits@restyled.io>
* Add python tests for Thermostat presets feature (#34693)
* Add python tests for Thermostat presets feature
* Restyled by autopep8
* Restyled by isort
* Update the PICS code for presets attribute
---------
Co-authored-by: Restyled.io <commits@restyled.io>
* removing unneccessary git fetch (#34698)
* Restyle patch
* Regen to fix ordering of global structs
* Apply suggestions from code review
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
* Return correct AtomicResponse when committing or rolling back
* Patch tests for atomic write of presets
* Fix tests to work with the new setup.
Specific changes:
* Enable SetActivePresetRequest command in all-clusters-app.
* Fix assignment of a PresetStructWithOwnedMembers to another
PresetStructWithOwnedMembers to actually work correctly.
* Move constraint checks that happen on write from commit to write.
* Fix sending of atomic responses to not have use-stack-after-return.
* Fix PICS for the tests involved.
* Fix PICS values for atomic requests
* Remove PresetsSchedulesEditable and QueuedPreset from various places
* Restyled patch
* Restyled patch, again
* Remove PICS value for PresetsSchedulesEditable
* clang-tidy fixes
* clang-tidy fixes
* Clear associated atomic writes when fabric is removed
* Add tests for fabric removal and lockout of clients outside of atomic write
* Python linter
* Restyled patch
* Clear timer when fabric is removed
* Check for open atomic write before resetting
* Revert auto delegate declaration on lines where there's no collision
* Allow Thermostat delegate to provide timeout for atomic requests
* Relocate thermostat example code to thermostat-common
* Remove thermostat-manager code, replace with thermostat delegate
* Sync atomic write error order with spec
* Restyle patch
* Drop memset of atomic write sessions
* Add PreCommit stage to allow rollback of multiple attributes when only one fails
* Separate OnTimerExpired method, vs ResetWrite
* Method documentation
* Apply suggestions from code review
Co-authored-by: Nivi Sarkar <55898241+nivi-apple@users.noreply.github.com>
* Remove unused InWrite check
* Drop imcode alias
* Switch AtomicWriteState to enum class
* DRY up atomic write manager
* Apply suggestions from code review
Co-authored-by: Nivi Sarkar <55898241+nivi-apple@users.noreply.github.com>
* Drop duplicate doc comments
* Rename GetAtomicWriteScopedNodeId to GetAtomicWriteOriginatorScopedNodeId
* Updates based on comments
* Add MatterReportingAttributeChangeCallback calls for updated attributes
* Relocate thermostat example code to thermostat-common, and remove thermostat-manager
* Merge atomic write code back into thermostat-server
* Apply suggestions from code review
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
* Fix build after suggestions
* Actually track attribute IDs associated with atomic write
* Only commit presets if all attribute precommits were successful
* Fix scope on err
* Add documentation to methods
* Remove duplicate preset check.
* Move various functions into anonymous namespaces, or Thermostat namespace
* Drop impossible non-atomic attribute status after rollback
* Allow null BuiltIn field when saving Presets
* Namespace workaround for compilers on other platforms
* Fix bad merge
* Fix readability issue
* Force built-in to false on new presets
---------
Co-authored-by: Nivedita Sarkar <nivedita_sarkar@apple.com>
Co-authored-by: Restyled.io <commits@restyled.io>
Co-authored-by: Nivi Sarkar <55898241+nivi-apple@users.noreply.github.com>
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
Co-authored-by: Terence Hampson <thampson@google.com>
Co-authored-by: Tennessee Carmel-Veilleux <tennessee.carmelveilleux@gmail.com>
Co-authored-by: Chris Letnick <cletnick@google.com>
Co-authored-by: C Freeman <cecille@google.com>
Co-authored-by: Douglas Rocha Ferraz <rochaferraz@google.com>
Co-authored-by: Petru Lauric <81822411+plauric@users.noreply.github.com>
Co-authored-by: William <hicklin@users.noreply.github.com>
Co-authored-by: Kiel Oleson <kielo@apple.com>
Co-authored-by: Karsten Sperling <113487422+ksperling-apple@users.noreply.github.com>
Co-authored-by: Anu Biradar <104591549+abiradarti@users.noreply.github.com>
Co-authored-by: mkardous-silabs <84793247+mkardous-silabs@users.noreply.github.com>
Co-authored-by: Rob Bultman <rob.Bultman@gmail.com>
Co-authored-by: Andrei Litvin <andy314@gmail.com>
Co-authored-by: Shao Ling Tan <161761051+shaoltan-amazon@users.noreply.github.com>
Co-authored-by: Amine Alami <43780877+Alami-Amine@users.noreply.github.com>
Co-authored-by: Michael Rupp <95718139+mykrupp@users.noreply.github.com>diff --git a/examples/thermostat/thermostat-common/include/thermostat-delegate-impl.h b/examples/thermostat/thermostat-common/include/thermostat-delegate-impl.h
index 9edf13f..b57ee24 100644
--- a/examples/thermostat/thermostat-common/include/thermostat-delegate-impl.h
+++ b/examples/thermostat/thermostat-common/include/thermostat-delegate-impl.h
@@ -58,7 +58,7 @@
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;
diff --git a/examples/thermostat/thermostat-common/src/thermostat-delegate-impl.cpp b/examples/thermostat/thermostat-common/src/thermostat-delegate-impl.cpp
index 8c411cd..7991e48 100644
--- a/examples/thermostat/thermostat-common/src/thermostat-delegate-impl.cpp
+++ b/examples/thermostat/thermostat-common/src/thermostat-delegate-impl.cpp
@@ -177,17 +177,17 @@
}
}
-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++;
diff --git a/src/app/clusters/thermostat-server/thermostat-delegate.h b/src/app/clusters/thermostat-server/thermostat-delegate.h
index ccb690a..5f11ab5 100644
--- a/src/app/clusters/thermostat-server/thermostat-delegate.h
+++ b/src/app/clusters/thermostat-server/thermostat-delegate.h
@@ -103,7 +103,7 @@
* @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.
diff --git a/src/app/clusters/thermostat-server/thermostat-server-presets.cpp b/src/app/clusters/thermostat-server/thermostat-server-presets.cpp
index a56c94f..9413513 100644
--- a/src/app/clusters/thermostat-server/thermostat-server-presets.cpp
+++ b/src/app/clusters/thermostat-server/thermostat-server-presets.cpp
@@ -38,16 +38,16 @@
* @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 @@
*
* @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 @@
}
// 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 @@
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);
}
diff --git a/src/python_testing/TC_TSTAT_4_2.py b/src/python_testing/TC_TSTAT_4_2.py
index 563d6f3..b133b1b 100644
--- a/src/python_testing/TC_TSTAT_4_2.py
+++ b/src/python_testing/TC_TSTAT_4_2.py
@@ -246,8 +246,12 @@
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 @@
# 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()
