Apply suggestions from code review

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

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:

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;

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<uint32_t>(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

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 <endpoint, cluster> 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 <endpoint, cluster> 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 <endpoint, cluster> pair, or many such pairs.
+///
+/// @note If more than one handler claims to handl a specific <endpoint, cluster> 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<ClusterId> & 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<SceneHandler> mHandlerList;
- uint8_t mNumHandlers = 0;
 };
 
 } // namespace scenes

src/app/clusters/scenes/SceneTableImpl.cpp

@@ -167,7 +167,7 @@ struct FabricSceneData : public PersistentData<kPersistentFabricBufferMax>
  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<SceneHandler>::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<SceneHandler>::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<uint8_t>(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 <app/util/attribute-storage.h>, which relies on zap generated files
 uint8_t DefaultSceneTableImpl::GetClustersFromEndpoint(EndpointId endpoint, ClusterId * clusterList, uint8_t listLen)
 {

src/app/util/mock/attribute-storage.cpp

@@ -188,7 +188,7 @@ chip::Optional<chip::ClusterId> 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)