From 1608540b0e547d009e17df2ff30c10c3d117a33d Mon Sep 17 00:00:00 2001 From: lpbeliveau-silabs <112982107+lpbeliveau-silabs@users.noreply.github.com> Date: Mon, 20 Mar 2023 09:43:40 -0400 Subject: [PATCH] Apply suggestions from code review Co-authored-by: Boris Zbarsky --- src/app/clusters/scenes/ExtensionFieldSets.h | 2 +- .../scenes/ExtensionFieldSetsImpl.cpp | 2 +- .../clusters/scenes/ExtensionFieldSetsImpl.h | 10 +-- src/app/clusters/scenes/SceneTable.h | 74 ++++++++++++------- src/app/clusters/scenes/SceneTableImpl.cpp | 15 ++-- src/app/util/mock/attribute-storage.cpp | 2 +- 6 files changed, 61 insertions(+), 44 deletions(-) diff --git a/src/app/clusters/scenes/ExtensionFieldSets.h b/src/app/clusters/scenes/ExtensionFieldSets.h index 174d49cbafb3be..4753995d9421e3 100644 --- a/src/app/clusters/scenes/ExtensionFieldSets.h +++ b/src/app/clusters/scenes/ExtensionFieldSets.h @@ -27,7 +27,7 @@ static constexpr uint8_t kInvalidPosition = 0xff; static constexpr uint8_t kMaxClustersPerScene = CHIP_CONFIG_SCENES_MAX_CLUSTERS_PER_SCENE; static constexpr uint8_t kMaxFieldBytesPerCluster = CHIP_CONFIG_SCENES_MAX_EXTENSION_FIELDSET_SIZE_PER_CLUSTER; -/// @brief class meant serialize all extension ßfield sets of a scene so it can be stored and retrieved from flash memory. +/// @brief A way to serialize and deserialize all cluster attribute data for a scene. class ExtensionFieldSets { public: diff --git a/src/app/clusters/scenes/ExtensionFieldSetsImpl.cpp b/src/app/clusters/scenes/ExtensionFieldSetsImpl.cpp index 0ee7e155481427..1bd09ea64eea62 100644 --- a/src/app/clusters/scenes/ExtensionFieldSetsImpl.cpp +++ b/src/app/clusters/scenes/ExtensionFieldSetsImpl.cpp @@ -99,7 +99,7 @@ CHIP_ERROR ExtensionFieldSetsImpl::InsertFieldSet(const ExtensionFieldSet & fiel } } - // if found, replace at found position, otherwise at insert first free position, otherwise return error + // if found, replace at found position, otherwise insert at first free position, otherwise return error if (firstEmptyPosition < kMaxClusterPerScenes) { mEFS[firstEmptyPosition] = fieldSet; diff --git a/src/app/clusters/scenes/ExtensionFieldSetsImpl.h b/src/app/clusters/scenes/ExtensionFieldSetsImpl.h index 4d822af3410df8..7a5996502a0d28 100644 --- a/src/app/clusters/scenes/ExtensionFieldSetsImpl.h +++ b/src/app/clusters/scenes/ExtensionFieldSetsImpl.h @@ -81,7 +81,7 @@ struct ExtensionFieldSet ReturnErrorOnFailure( writer.StartContainer(TLV::ContextTag(TagEFS::kIndividualContainer), TLV::kTLVType_Structure, container)); - ReturnErrorOnFailure(writer.Put(TLV::ContextTag(TagEFS::kClusterID), static_cast(mID))); + ReturnErrorOnFailure(writer.Put(TLV::ContextTag(TagEFS::kClusterID), mID)); ReturnErrorOnFailure(writer.PutBytes(TLV::ContextTag(TagEFS::kClusterFieldSetData), mBytesBuffer, mUsedBytes)); return writer.EndContainer(container); @@ -113,12 +113,12 @@ struct ExtensionFieldSet mUsedBytes = 0; } - bool IsEmpty() const { return (this->mUsedBytes == 0); } + bool IsEmpty() const { return (mUsedBytes == 0); } bool operator==(const ExtensionFieldSet & other) const { - return (this->mID == other.mID && this->mUsedBytes == other.mUsedBytes && - !memcmp(this->mBytesBuffer, other.mBytesBuffer, this->mUsedBytes)); + return (mID == other.mID && mUsedBytes == other.mUsedBytes && + !memcmp(mBytesBuffer, other.mBytesBuffer, mUsedBytes)); } }; @@ -164,7 +164,7 @@ class ExtensionFieldSetsImpl : public ExtensionFieldSets } protected: - ExtensionFieldSet mEFS[kMaxClusterPerScenes]; + ExtensionFieldSet mFieldSets[kMaxClusterPerScenes]; uint8_t mFieldSetsCount = 0; }; } // namespace scenes diff --git a/src/app/clusters/scenes/SceneTable.h b/src/app/clusters/scenes/SceneTable.h index eb60cf7827131b..cf7e4fcf4caa2c 100644 --- a/src/app/clusters/scenes/SceneTable.h +++ b/src/app/clusters/scenes/SceneTable.h @@ -36,18 +36,25 @@ typedef uint32_t SceneTransitionTime; constexpr GroupId kGlobalGroupSceneId = 0x0000; constexpr SceneIndex kUndefinedSceneIndex = 0xff; constexpr SceneId kUndefinedSceneId = 0xff; -static constexpr uint8_t kMaxScenePerFabric = CHIP_CONFIG_SCENES_MAX_PER_FABRIC; +static constexpr uint8_t kMaxScenesPerFabric = CHIP_CONFIG_SCENES_MAX_PER_FABRIC; + static constexpr size_t kIteratorsMax = CHIP_CONFIG_MAX_SCENES_CONCURRENT_ITERATORS; -static constexpr size_t kSceneNameMax = CHIP_CONFIG_SCENES_CLUSTER_MAXIMUM_NAME_LENGTH; +static constexpr size_t kSceneNameMaxLength = CHIP_CONFIG_SCENES_CLUSTER_MAXIMUM_NAME_LENGTH; + -/// @brief Abstract class allowing different Endpoints interactions with the ExtensionFieldSets added to and retrieved from the -/// scene Table. The Scene Handler's are meant as interface between various clusters and the Scene table. The expected behaviour of -/// the table with the handler is: Once a scene command involving extension field set is received, the Scene Table will go through -/// the list of handlers to either retrieve, populate or apply Extension field sets. Each handler is meant to retrieve an extension -/// field set for a single cluster however it is also possible to use a single generic handler that handles all of them. +/// @brief SceneHandlers are meant as interface between various clusters and the Scene table. +/// When a scene command involving extension field sets is received, the Scene Table will go through +/// the list of handlers to either retrieve, populate or apply those extension field sets. +/// +/// Generally, for each specific pair there should be one and only one handler +/// registered with the scene table that claims to handle that pair. /// -/// @note If more than one handler is implemented for a specific pair, ONLY THE FIRST HANDLER FOR THAT SPECIFIC -/// PAIR will get called on executing extension field sets related ations on the scene table. +/// A SceneHandler can handle a single pair, or many such pairs. +/// +/// @note If more than one handler claims to handl a specific pair, only one of +/// those handlers will get called when executing actions related to extension field sets on the scene +/// table. It is not defined which handler will be selected. + class SceneHandler : public IntrusiveListNodeBase<> { public: @@ -58,8 +65,10 @@ class SceneHandler : public IntrusiveListNodeBase<> /// supported clusters /// @param endpoint target endpoint /// @param clusterBuffer Buffer to hold the supported cluster IDs, cannot hold more than - /// CHIP_CONFIG_SCENES_MAX_CLUSTERS_PER_SCENES, the function shall use the reduce_size() method in the event it is supporting - /// less than CHIP_CONFIG_SCENES_MAX_CLUSTERS_PER_SCENES clusters + /// CHIP_CONFIG_SCENES_MAX_CLUSTERS_PER_SCENE. The function shall use the reduce_size() method in the event it is supporting + + /// less than CHIP_CONFIG_SCENES_MAX_CLUSTERS_PER_SCENE clusters. + virtual void GetSupportedClusters(EndpointId endpoint, Span & clusterBuffer) = 0; /// @brief Returns whether or not a cluster for scenes is supported on an endpoint @@ -69,7 +78,8 @@ class SceneHandler : public IntrusiveListNodeBase<> /// @return true if supported, false if not supported virtual bool SupportsCluster(EndpointId endpoint, ClusterId cluster) = 0; - /// @brief From command AddScene, allows handler to filter through clusters in command to serialize only the supported ones. + /// @brief Called when handling AddScene. Allows the handler to filter through the clusters in the command to serialize only the supported ones. + /// @param endpoint[in] Endpoint ID /// @param extensionFieldSet[in] ExtensionFieldSets provided by the AddScene Command, pre initialized /// @param cluster[out] Cluster in the Extension field set, filled by the function @@ -79,10 +89,10 @@ class SceneHandler : public IntrusiveListNodeBase<> const app::Clusters::Scenes::Structs::ExtensionFieldSet::DecodableType & extensionFieldSet, ClusterId & cluster, MutableByteSpan & serialisedBytes) = 0; - /// @brief From command StoreScene, retrieves ExtensionField from currently active values, it is the function's responsibility - /// to + /// @brief Called when handling StoreScene, and only if the handler supports the given endpoint and cluster. + /// + /// The implementation must write the actual scene data to store to serializedBytes as described below. - /// place the serialized data in serializedBytes as described below. /// @param endpoint[in] Target Endpoint /// @param cluster[in] Target Cluster @@ -92,7 +102,8 @@ class SceneHandler : public IntrusiveListNodeBase<> /// @return CHIP_NO_ERROR if successful, CHIP_ERROR value otherwise virtual CHIP_ERROR SerializeSave(EndpointId endpoint, ClusterId cluster, MutableByteSpan & serializedBytes) = 0; - /// @brief From stored scene (e.g. ViewScene), deserialize ExtensionFieldSet into a cluster object + /// @brief Deserialize an ExtensionFieldSet into a cluster object (e.g. when handling ViewScene). + /// @param endpoint[in] Endpoint ID /// @param cluster[in] Cluster ID to save /// @param serializedBytes[in] ExtensionFieldSet stored in NVM @@ -103,7 +114,8 @@ class SceneHandler : public IntrusiveListNodeBase<> app::Clusters::Scenes::Structs::ExtensionFieldSet::Type & extensionFieldSet) = 0; - /// @brief From stored scene (e.g RecallScene), applies EFS values to cluster at transition time + /// @brief Restore a stored scene for the given cluster instance, over timeMs milliseconds (e.g. when handling RecallScene) + /// @param endpoint[in] Endpoint ID /// @param cluster[in] Cluster ID /// @param serializedBytes[in] ExtensionFieldSet stored in NVM @@ -141,7 +153,8 @@ class SceneTable bool operator==(const SceneStorageId & other) { - return (this->mEndpointId == other.mEndpointId && this->mGroupId == other.mGroupId && this->mSceneId == other.mSceneId); + return (mEndpointId == other.mEndpointId && mGroupId == other.mGroupId && mSceneId == other.mSceneId); + } }; @@ -162,17 +175,20 @@ class SceneTable SceneData(const CharSpan & sceneName = CharSpan(), SceneTransitionTime time = 0) : mSceneTransitionTimeMs(time) { - this->SetName(sceneName); + SetName(sceneName); + } SceneData(EFStype fields, const CharSpan & sceneName = CharSpan(), SceneTransitionTime time = 0) : mSceneTransitionTimeMs(time) { - this->SetName(sceneName); + SetName(sceneName); + mExtensionFieldSets = fields; } SceneData(const SceneData & other) : mSceneTransitionTimeMs(other.mSceneTransitionTimeMs) { - this->SetName(CharSpan(other.mName, other.mNameLength)); + SetName(CharSpan(other.mName, other.mNameLength)); + mExtensionFieldSets = other.mExtensionFieldSets; } ~SceneData(){}; @@ -194,7 +210,8 @@ class SceneTable void Clear() { - this->SetName(CharSpan()); + SetName(CharSpan()); + mSceneTransitionTimeMs = 0; mExtensionFieldSets.Clear(); } @@ -229,13 +246,15 @@ class SceneTable bool operator==(const SceneTableEntry & other) { - return (this->mStorageId == other.mStorageId && this->mStorageData == other.mStorageData); + return (mStorageId == other.mStorageId && mStorageData == other.mStorageData); + } void operator=(const SceneTableEntry & other) { - this->mStorageId = other.mStorageId; - this->mStorageData = other.mStorageData; + mStorageId = other.mStorageId; + mStorageData = other.mStorageData; + } }; @@ -275,12 +294,11 @@ class SceneTable virtual SceneEntryIterator * IterateSceneEntries(FabricIndex fabric_index) = 0; // Handlers - virtual bool HandlerListEmpty() { return (mNumHandlers == 0); } - virtual uint8_t GetHandlerNum() { return this->mNumHandlers; } + virtual bool HandlerListEmpty() { return mHandlerList.Empty(); } + // SceneHandler * mHandlers[kMaxSceneHandlers] = { nullptr }; IntrusiveList mHandlerList; - uint8_t mNumHandlers = 0; }; } // namespace scenes diff --git a/src/app/clusters/scenes/SceneTableImpl.cpp b/src/app/clusters/scenes/SceneTableImpl.cpp index 98276b3c17d995..9a7a69c8492f3a 100644 --- a/src/app/clusters/scenes/SceneTableImpl.cpp +++ b/src/app/clusters/scenes/SceneTableImpl.cpp @@ -167,7 +167,7 @@ struct FabricSceneData : public PersistentData TLV::TLVType container; ReturnErrorOnFailure(writer.StartContainer(TLV::AnonymousTag(), TLV::kTLVType_Structure, container)); - ReturnErrorOnFailure(writer.Put(TLV::ContextTag(TagSceneImpl::kSceneCount), (scene_count))); + ReturnErrorOnFailure(writer.Put(TLV::ContextTag(TagSceneImpl::kSceneCount), scene_count)); // Storing the scene map for (uint8_t i = 0; i < kMaxScenePerFabric; i++) { @@ -387,7 +387,7 @@ void DefaultSceneTableImpl::RegisterHandler(SceneHandler * handler) void DefaultSceneTableImpl::UnregisterHandler(SceneHandler * handler) { // Verify list is populated and handler is not null - VerifyOrReturn(!this->HandlerListEmpty() && !(handler == nullptr)); + VerifyOrReturn(!HandlerListEmpty() && !(handler == nullptr)); mHandlerList.Remove(handler); mNumHandlers--; } @@ -398,7 +398,7 @@ void DefaultSceneTableImpl::UnregisterAllHandlers() { IntrusiveList::Iterator foo = mHandlerList.begin(); SceneHandler * handle = &(*foo); - mHandlerList.Remove((handle)); + mHandlerList.Remove(handle); mNumHandlers--; } } @@ -409,7 +409,7 @@ void DefaultSceneTableImpl::UnregisterAllHandlers() CHIP_ERROR DefaultSceneTableImpl::SceneSaveEFS(SceneTableEntry & scene) { - if (!this->HandlerListEmpty()) + if (!HandlerListEmpty()) { uint8_t clusterCount = 0; clusterId cArray[kMaxClusterPerScenes]; @@ -422,10 +422,9 @@ CHIP_ERROR DefaultSceneTableImpl::SceneSaveEFS(SceneTableEntry & scene) MutableByteSpan EFSSpan = MutableByteSpan(EFS.mBytesBuffer, kMaxFieldBytesPerCluster); EFS.mID = cluster; - IntrusiveList::Iterator iter = mHandlerList.begin(); - while (iter != mHandlerList.end()) + for (auto & handler : mHandlerList) { - if (iter->SupportsCluster(scene.mStorageId.mEndpointId, cluster)) + if (handler.SupportsCluster(scene.mStorageId.mEndpointId, cluster)) { ReturnErrorOnFailure(iter->SerializeSave(scene.mStorageId.mEndpointId, EFS.mID, EFSSpan)); EFS.mUsedBytes = static_cast(EFSSpan.size()); @@ -495,7 +494,7 @@ CHIP_ERROR DefaultSceneTableImpl::RemoveFabric(FabricIndex fabric_index) return fabric.Delete(mStorage); } -/// @brief wrapper function around emberAfGetClustersFromEndpoint to allow testing, shimed in test configuration because +/// @brief wrapper function around emberAfGetClustersFromEndpoint to allow testing, shimmed in test configuration because /// emberAfGetClusterFromEndpoint relies on , which relies on zap generated files uint8_t DefaultSceneTableImpl::GetClustersFromEndpoint(EndpointId endpoint, ClusterId * clusterList, uint8_t listLen) { diff --git a/src/app/util/mock/attribute-storage.cpp b/src/app/util/mock/attribute-storage.cpp index 4e760b8c01c6d5..a1bef6d8cea299 100644 --- a/src/app/util/mock/attribute-storage.cpp +++ b/src/app/util/mock/attribute-storage.cpp @@ -188,7 +188,7 @@ chip::Optional emberAfGetNthClusterId(chip::EndpointId endpoint // for the given endpoint and client/server polarity uint8_t emberAfGetClustersFromEndpoint(EndpointId endpoint, ClusterId * clusterList, uint8_t listLen, bool server) { - uint8_t cluster_Count = emberAfClusterCount(endpoint, server); + uint8_t cluster_count = emberAfClusterCount(endpoint, server); uint8_t i; if (cluster_Count > listLen)