From 31ddf98b8100c2388e731bc757526c41e288198e Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 30 Jun 2022 09:57:40 -0400 Subject: [PATCH] Assume external attributes that must be non-volatile actually are. (#20144) We have clusters doing non-volatility sanity checks, but for external attributes we can't conclusively say they are volatile, and the clusters don't work right if we assume they are. So instead assume that if the spec says N the external attribute is in fact non-volatile. Fixes https://github.com/project-chip/connectedhomeip/issues/20093 --- src/app/clusters/ias-zone-server/ias-zone-server.cpp | 8 ++++---- src/app/clusters/level-control/level-control.cpp | 8 ++------ .../clusters/mode-select-server/mode-select-server.cpp | 6 +++--- src/app/clusters/on-off-server/on-off-server.cpp | 8 ++------ src/app/util/attribute-metadata.h | 2 +- src/app/util/attribute-storage.cpp | 4 ++-- src/app/util/attribute-table.cpp | 2 +- src/app/util/util.cpp | 4 ++-- src/app/util/util.h | 6 +++++- 9 files changed, 22 insertions(+), 26 deletions(-) diff --git a/src/app/clusters/ias-zone-server/ias-zone-server.cpp b/src/app/clusters/ias-zone-server/ias-zone-server.cpp index cee9b1d3275b7f..29c3abd2fa7332 100644 --- a/src/app/clusters/ias-zone-server/ias-zone-server.cpp +++ b/src/app/clusters/ias-zone-server/ias-zone-server.cpp @@ -590,10 +590,10 @@ uint8_t emberAfPluginIasZoneServerGetZoneId(EndpointId endpoint) //------------------------------------------------------------------------------ static bool areZoneServerAttributesNonVolatile(EndpointId endpoint) { - if (!emberAfIsNonVolatileAttribute(endpoint, IasZone::Id, Attributes::IasCieAddress::Id) || - !emberAfIsNonVolatileAttribute(endpoint, IasZone::Id, Attributes::ZoneState::Id) || - !emberAfIsNonVolatileAttribute(endpoint, IasZone::Id, Attributes::ZoneType::Id) || - !emberAfIsNonVolatileAttribute(endpoint, IasZone::Id, Attributes::ZoneId::Id)) + if (emberAfIsKnownVolatileAttribute(endpoint, IasZone::Id, Attributes::IasCieAddress::Id) || + emberAfIsKnownVolatileAttribute(endpoint, IasZone::Id, Attributes::ZoneState::Id) || + emberAfIsKnownVolatileAttribute(endpoint, IasZone::Id, Attributes::ZoneType::Id) || + emberAfIsKnownVolatileAttribute(endpoint, IasZone::Id, Attributes::ZoneId::Id)) { return false; } diff --git a/src/app/clusters/level-control/level-control.cpp b/src/app/clusters/level-control/level-control.cpp index bb479039bac543..8807846ee69ca3 100644 --- a/src/app/clusters/level-control/level-control.cpp +++ b/src/app/clusters/level-control/level-control.cpp @@ -1122,12 +1122,8 @@ void emberAfLevelControlClusterServerInitCallback(EndpointId endpoint) #ifndef IGNORE_LEVEL_CONTROL_CLUSTER_START_UP_CURRENT_LEVEL static bool areStartUpLevelControlServerAttributesNonVolatile(EndpointId endpoint) { - if (emberAfIsNonVolatileAttribute(endpoint, LevelControl::Id, Attributes::CurrentLevel::Id)) - { - return emberAfIsNonVolatileAttribute(endpoint, LevelControl::Id, Attributes::StartUpCurrentLevel::Id); - } - - return false; + return !emberAfIsKnownVolatileAttribute(endpoint, LevelControl::Id, Attributes::CurrentLevel::Id) && + !emberAfIsKnownVolatileAttribute(endpoint, LevelControl::Id, Attributes::StartUpCurrentLevel::Id); } #endif // IGNORE_LEVEL_CONTROL_CLUSTER_START_UP_CURRENT_LEVEL diff --git a/src/app/clusters/mode-select-server/mode-select-server.cpp b/src/app/clusters/mode-select-server/mode-select-server.cpp index adee979e052911..0266c4dc6e0992 100644 --- a/src/app/clusters/mode-select-server/mode-select-server.cpp +++ b/src/app/clusters/mode-select-server/mode-select-server.cpp @@ -147,7 +147,7 @@ void emberAfModeSelectClusterServerInitCallback(EndpointId endpointId) Attributes::OnMode::TypeInfo::Type onMode; bool onOffValueForStartUp = 0; if (Attributes::OnMode::Get(endpointId, onMode) == EMBER_ZCL_STATUS_SUCCESS && - emberAfIsNonVolatileAttribute(endpointId, OnOff::Id, OnOff::Attributes::StartUpOnOff::Id) && + !emberAfIsKnownVolatileAttribute(endpointId, OnOff::Id, OnOff::Attributes::StartUpOnOff::Id) && OnOffServer::Instance().getOnOffValueForStartUp(endpointId, onOffValueForStartUp) == EMBER_ZCL_STATUS_SUCCESS) { if (onOffValueForStartUp && !onMode.IsNull()) @@ -189,8 +189,8 @@ namespace { */ inline bool areStartUpModeAndCurrentModeNonVolatile(EndpointId endpointId) { - return emberAfIsNonVolatileAttribute(endpointId, ModeSelect::Id, Attributes::CurrentMode::Id) && - emberAfIsNonVolatileAttribute(endpointId, ModeSelect::Id, Attributes::StartUpMode::Id); + return !emberAfIsKnownVolatileAttribute(endpointId, ModeSelect::Id, Attributes::CurrentMode::Id) && + !emberAfIsKnownVolatileAttribute(endpointId, ModeSelect::Id, Attributes::StartUpMode::Id); } } // namespace diff --git a/src/app/clusters/on-off-server/on-off-server.cpp b/src/app/clusters/on-off-server/on-off-server.cpp index 12299050929586..c0006c70c25134 100644 --- a/src/app/clusters/on-off-server/on-off-server.cpp +++ b/src/app/clusters/on-off-server/on-off-server.cpp @@ -580,12 +580,8 @@ void OnOffServer::updateOnOffTimeCommand(chip::EndpointId endpoint) #ifndef IGNORE_ON_OFF_CLUSTER_START_UP_ON_OFF bool OnOffServer::areStartUpOnOffServerAttributesNonVolatile(EndpointId endpoint) { - if (emberAfIsNonVolatileAttribute(endpoint, OnOff::Id, Attributes::OnOff::Id)) - { - return emberAfIsNonVolatileAttribute(endpoint, OnOff::Id, Attributes::StartUpOnOff::Id); - } - - return false; + return !emberAfIsKnownVolatileAttribute(endpoint, OnOff::Id, Attributes::OnOff::Id) && + !emberAfIsKnownVolatileAttribute(endpoint, OnOff::Id, Attributes::StartUpOnOff::Id); } #endif // IGNORE_ON_OFF_CLUSTER_START_UP_ON_OFF diff --git a/src/app/util/attribute-metadata.h b/src/app/util/attribute-metadata.h index b4f2256b1443dc..cf8b04c2bb126b 100644 --- a/src/app/util/attribute-metadata.h +++ b/src/app/util/attribute-metadata.h @@ -202,7 +202,7 @@ struct EmberAfAttributeMetadata * Check whether this attribute is automatically stored in non-volatile * memory. */ - bool IsNonVolatile() const { return (mask & ATTRIBUTE_MASK_NONVOLATILE) && !IsExternal(); } + bool IsAutomaticallyPersisted() const { return (mask & ATTRIBUTE_MASK_NONVOLATILE) && !IsExternal(); } }; /** @brief Returns true if the given attribute type is a string. */ diff --git a/src/app/util/attribute-storage.cpp b/src/app/util/attribute-storage.cpp index b0872604442468..7e8bad31264561 100644 --- a/src/app/util/attribute-storage.cpp +++ b/src/app/util/attribute-storage.cpp @@ -1227,7 +1227,7 @@ void emAfLoadAttributeDefaults(EndpointId endpoint, bool ignoreStorage, Optional ptr = nullptr; // Will get set to the value to write, as needed. // First check for a persisted value. - if (!ignoreStorage && am->IsNonVolatile()) + if (!ignoreStorage && am->IsAutomaticallyPersisted()) { VerifyOrDie(attrStorage && "Attribute persistence needs a persistence provider"); MutableByteSpan bytes(attrData); @@ -1331,7 +1331,7 @@ void emAfSaveAttributeToStorageIfNeeded(uint8_t * data, EndpointId endpoint, Clu const EmberAfAttributeMetadata * metadata) { // Get out of here if this attribute isn't marked non-volatile. - if (!metadata->IsNonVolatile()) + if (!metadata->IsAutomaticallyPersisted()) { return; } diff --git a/src/app/util/attribute-table.cpp b/src/app/util/attribute-table.cpp index e8de154d2b28fb..e0fa107bb24853 100644 --- a/src/app/util/attribute-table.cpp +++ b/src/app/util/attribute-table.cpp @@ -151,7 +151,7 @@ void emberAfPrintAttributeTable(void) emberAfAttributesPrint( " / %x (%x) / %p / %p / ", metaData->attributeType, emberAfAttributeSize(metaData), (metaData->IsReadOnly() ? "RO" : "RW"), - (metaData->IsNonVolatile() ? " nonvolatile " : (metaData->IsExternal() ? " extern " : " RAM "))); + (metaData->IsAutomaticallyPersisted() ? " nonvolatile " : (metaData->IsExternal() ? " extern " : " RAM "))); emberAfAttributesFlush(); status = emAfReadAttribute(ep->endpoint, cluster->clusterId, metaData->attributeId, data, ATTRIBUTE_LARGEST, nullptr); diff --git a/src/app/util/util.cpp b/src/app/util/util.cpp index 02d68a315fa3d2..4ffc01a0143120 100644 --- a/src/app/util/util.cpp +++ b/src/app/util/util.cpp @@ -768,7 +768,7 @@ bool emberAfContainsAttribute(chip::EndpointId endpoint, chip::ClusterId cluster return (emberAfGetServerAttributeIndexByAttributeId(endpoint, clusterId, attributeId) != UINT16_MAX); } -bool emberAfIsNonVolatileAttribute(chip::EndpointId endpoint, chip::ClusterId clusterId, chip::AttributeId attributeId) +bool emberAfIsKnownVolatileAttribute(chip::EndpointId endpoint, chip::ClusterId clusterId, chip::AttributeId attributeId) { const EmberAfAttributeMetadata * metadata = emberAfLocateAttributeMetadata(endpoint, clusterId, attributeId); @@ -777,7 +777,7 @@ bool emberAfIsNonVolatileAttribute(chip::EndpointId endpoint, chip::ClusterId cl return false; } - return metadata->IsNonVolatile(); + return !metadata->IsAutomaticallyPersisted() && !metadata->IsExternal(); } chip::Messaging::ExchangeManager * chip::ExchangeManager() diff --git a/src/app/util/util.h b/src/app/util/util.h index 4d8910a15d4bb8..2948f665674261 100644 --- a/src/app/util/util.h +++ b/src/app/util/util.h @@ -275,7 +275,11 @@ uint8_t emberAfGetChannelFrom8bitEncodedChanPg(uint8_t chanPg); uint8_t emberAfMake8bitEncodedChanPg(uint8_t page, uint8_t channel); bool emberAfContainsAttribute(chip::EndpointId endpoint, chip::ClusterId clusterId, chip::AttributeId attributeId); -bool emberAfIsNonVolatileAttribute(chip::EndpointId endpoint, chip::ClusterId clusterId, chip::AttributeId attributeId); + +/* @brief returns true if the attribute is known to be volatile (i.e. RAM + * storage). + */ +bool emberAfIsKnownVolatileAttribute(chip::EndpointId endpoint, chip::ClusterId clusterId, chip::AttributeId attributeId); namespace chip { chip::Messaging::ExchangeManager * ExchangeManager();