From a6964162597eda98ec381a0c8851436e564e4403 Mon Sep 17 00:00:00 2001 From: Hasty Granbery Date: Tue, 27 Aug 2024 13:41:58 -0700 Subject: [PATCH] [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 * 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 * 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 * Address Review Comments * Address review comments * Fix default timeout after other timeouts changed * Restyled by autopep8 * Fix linter error --------- Co-authored-by: C Freeman Co-authored-by: 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 * comments addressed * more comments addressed * lint pass * Update src/python_testing/TC_BRBINFO_4_1.py Co-authored-by: C Freeman * 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 * Update src/python_testing/TC_BRBINFO_4_1.py Co-authored-by: Terence Hampson * Update src/python_testing/TC_BRBINFO_4_1.py Co-authored-by: Terence Hampson * comments addressed * Restyled by autopep8 --------- Co-authored-by: Terence Hampson Co-authored-by: C Freeman Co-authored-by: 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 * Update src/python_testing/TC_SEAR_1_2.py Co-authored-by: William * 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 * address code review feedback * remove PICS checked by the TC_SEAR_1.6 * more code review updates * Restyled by autopep8 --------- Co-authored-by: William Co-authored-by: Kiel Oleson Co-authored-by: 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 --------- Co-authored-by: Boris Zbarsky * [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 * 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 * 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 Co-authored-by: Andrei Litvin * 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 Date: Fri May 10 14:26:07 2024 -0400 remove some unwanted diffs in provision files commit be160931dc600de7e7ead378b70d6a43c3945e46 Author: Michael Rupp Date: Fri May 10 14:24:25 2024 -0400 revert changes to generator.project.mak commit 14b6605887166e6d5284a61feb2bf407d850bdcf Author: Michael Rupp 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 * 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 * removing unneccessary git fetch (#34698) * Restyle patch * Regen to fix ordering of global structs * Apply suggestions from code review Co-authored-by: Boris Zbarsky * 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 * 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 Co-authored-by: Restyled.io Co-authored-by: Nivi Sarkar <55898241+nivi-apple@users.noreply.github.com> Co-authored-by: Boris Zbarsky Co-authored-by: Terence Hampson Co-authored-by: Tennessee Carmel-Veilleux Co-authored-by: Chris Letnick Co-authored-by: C Freeman Co-authored-by: Douglas Rocha Ferraz Co-authored-by: Petru Lauric <81822411+plauric@users.noreply.github.com> Co-authored-by: William Co-authored-by: Kiel Oleson 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 Co-authored-by: Andrei Litvin 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> --- .../include/thermostat-delegate-impl.h | 2 +- .../src/thermostat-delegate-impl.cpp | 6 +-- .../thermostat-server/thermostat-delegate.h | 2 +- .../thermostat-server-presets.cpp | 52 +++++++++++++------ src/python_testing/TC_TSTAT_4_2.py | 12 ++++- 5 files changed, 50 insertions(+), 24 deletions(-) diff --git a/examples/thermostat/thermostat-common/include/thermostat-delegate-impl.h b/examples/thermostat/thermostat-common/include/thermostat-delegate-impl.h index 9edf13f839df44..b57ee2492122e7 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 @@ 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; diff --git a/examples/thermostat/thermostat-common/src/thermostat-delegate-impl.cpp b/examples/thermostat/thermostat-common/src/thermostat-delegate-impl.cpp index 8c411cd5a9176e..7991e48323a62f 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 @@ 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(preset.presetScenario) }; + const uint8_t handle[] = { static_cast(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 ccb690a34fba60..5f11ab59889430 100644 --- a/src/app/clusters/thermostat-server/thermostat-delegate.h +++ b/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. diff --git a/src/app/clusters/thermostat-server/thermostat-server-presets.cpp b/src/app/clusters/thermostat-server/thermostat-server-presets.cpp index a56c94fa49834d..9413513fd0cedd 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 @@ 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 & 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); } diff --git a/src/python_testing/TC_TSTAT_4_2.py b/src/python_testing/TC_TSTAT_4_2.py index 563d6f3f2eddfc..b133b1be2c8f22 100644 --- a/src/python_testing/TC_TSTAT_4_2.py +++ b/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()