[Scenes] EFS TLV container fix (#30663)

* Removed un-necessaery containter in extension field set and changed comment about max size tested given past optimisation * Removed the tag from ExtensionFieldSetsImpl.Serialize() signature * Removed logging of empty efs array * Update src/lib/core/CHIPConfig.h Co-authored-by: Boris Zbarsky <[email protected]> * Remove the conditionnal storage of EFS --------- Co-authored-by: Boris Zbarsky <[email protected]>
project-chip · Dec 7, 2023 · 23f11a3 · 23f11a3
1 parent ad34317
commit 23f11a3
Show file tree

Hide file tree

Showing 6 changed files with 25 additions and 38 deletions.
diff --git a/src/app/clusters/scenes-server/ExtensionFieldSets.h b/src/app/clusters/scenes-server/ExtensionFieldSets.h
@@ -34,10 +34,10 @@ class ExtensionFieldSets
     ExtensionFieldSets(){};
     virtual ~ExtensionFieldSets() = default;
 
-    virtual CHIP_ERROR Serialize(TLV::TLVWriter & writer, TLV::Tag structTa) const = 0;
-    virtual CHIP_ERROR Deserialize(TLV::TLVReader & reader, TLV::Tag structTa)     = 0;
-    virtual void Clear()                                                           = 0;
-    virtual bool IsEmpty() const                                                   = 0;
+    virtual CHIP_ERROR Serialize(TLV::TLVWriter & writer) const = 0;
+    virtual CHIP_ERROR Deserialize(TLV::TLVReader & reader)     = 0;
+    virtual void Clear()                                        = 0;
+    virtual bool IsEmpty() const                                = 0;
     /// @brief Gets a count of how many initialized fields sets are in the object
     /// @return The number of initialized field sets the object
     /// @note Field set refers to extension field sets, from the scene cluster (see 1.4.6.2 ExtensionFieldSet in Matter Application

diff --git a/src/app/clusters/scenes-server/ExtensionFieldSetsImpl.cpp b/src/app/clusters/scenes-server/ExtensionFieldSetsImpl.cpp
@@ -20,12 +20,8 @@
 namespace chip {
 namespace scenes {
 
-// ExtensionFieldSetsImpl::ExtensionFieldSetsImpl() : ExtensionFieldSets() {}
-
-CHIP_ERROR ExtensionFieldSetsImpl::Serialize(TLV::TLVWriter & writer, TLV::Tag structTag) const
+CHIP_ERROR ExtensionFieldSetsImpl::Serialize(TLV::TLVWriter & writer) const
 {
-    TLV::TLVType structureContainer;
-    ReturnErrorOnFailure(writer.StartContainer(structTag, TLV::kTLVType_Structure, structureContainer));
     TLV::TLVType arrayContainer;
     ReturnErrorOnFailure(
         writer.StartContainer(TLV::ContextTag(TagEFS::kFieldSetArrayContainer), TLV::kTLVType_Array, arrayContainer));
@@ -34,16 +30,11 @@ CHIP_ERROR ExtensionFieldSetsImpl::Serialize(TLV::TLVWriter & writer, TLV::Tag s
         ReturnErrorOnFailure(mFieldSets[i].Serialize(writer));
     }
 
-    ReturnErrorOnFailure(writer.EndContainer(arrayContainer));
-    return writer.EndContainer(structureContainer);
+    return writer.EndContainer(arrayContainer);
 }
 
-CHIP_ERROR ExtensionFieldSetsImpl::Deserialize(TLV::TLVReader & reader, TLV::Tag structTag)
+CHIP_ERROR ExtensionFieldSetsImpl::Deserialize(TLV::TLVReader & reader)
 {
-    TLV::TLVType structureContainer;
-    ReturnErrorOnFailure(reader.Next(TLV::kTLVType_Structure, structTag));
-    ReturnErrorOnFailure(reader.EnterContainer(structureContainer));
-
     TLV::TLVType arrayContainer;
     ReturnErrorOnFailure(reader.Next(TLV::kTLVType_Array, TLV::ContextTag(TagEFS::kFieldSetArrayContainer)));
     ReturnErrorOnFailure(reader.EnterContainer(arrayContainer));
@@ -69,8 +60,7 @@ CHIP_ERROR ExtensionFieldSetsImpl::Deserialize(TLV::TLVReader & reader, TLV::Tag
         return err;
     }
 
-    ReturnErrorOnFailure(reader.ExitContainer(arrayContainer));
-    return reader.ExitContainer(structureContainer);
+    return reader.ExitContainer(arrayContainer);
 }
 
 void ExtensionFieldSetsImpl::Clear()

diff --git a/src/app/clusters/scenes-server/ExtensionFieldSetsImpl.h b/src/app/clusters/scenes-server/ExtensionFieldSetsImpl.h
@@ -122,8 +122,8 @@ class ExtensionFieldSetsImpl : public ExtensionFieldSets
     ~ExtensionFieldSetsImpl() override{};
 
     // overrides
-    CHIP_ERROR Serialize(TLV::TLVWriter & writer, TLV::Tag structTag) const override;
-    CHIP_ERROR Deserialize(TLV::TLVReader & reader, TLV::Tag structTag) override;
+    CHIP_ERROR Serialize(TLV::TLVWriter & writer) const override;
+    CHIP_ERROR Deserialize(TLV::TLVReader & reader) override;
     void Clear() override;
     bool IsEmpty() const override { return (mFieldSetsCount == 0); }
     uint8_t GetFieldSetCount() const override { return mFieldSetsCount; };

diff --git a/src/app/clusters/scenes-server/SceneTableImpl.cpp b/src/app/clusters/scenes-server/SceneTableImpl.cpp
@@ -39,7 +39,6 @@ enum class TagScene : uint8_t
     kSceneID,
     kName,
     kTransitionTimeMs,
-    kExtensionFieldSetsContainer,
 };
 
 using SceneTableEntry = DefaultSceneTableImpl::SceneTableEntry;
@@ -99,7 +98,7 @@ struct EndpointSceneCount : public PersistentData<kPersistentBufferSceneCountByt
     }
 };
 
-// Worst case tested: Add Scene Command with EFS using the default SerializeAdd Method. This yielded a serialized scene of 212bytes
+// Worst case tested: Add Scene Command with EFS using the default SerializeAdd Method. This yielded a serialized scene of 175 bytes
 // when using the OnOff, Level Control and Color Control as well as the maximal name length of 16 bytes. Putting 256 gives some
 // slack in case different clusters are used. Value obtained by using writer.GetLengthWritten at the end of the SceneTableData
 // Serialize method.
@@ -150,8 +149,7 @@ struct SceneTableData : public SceneTableEntry, PersistentData<kPersistentSceneB
         }
 
         ReturnErrorOnFailure(writer.Put(TLV::ContextTag(TagScene::kTransitionTimeMs), mStorageData.mSceneTransitionTimeMs));
-        ReturnErrorOnFailure(
-            mStorageData.mExtensionFieldSets.Serialize(writer, TLV::ContextTag(TagScene::kExtensionFieldSetsContainer)));
+        ReturnErrorOnFailure(mStorageData.mExtensionFieldSets.Serialize(writer));
 
         return writer.EndContainer(container);
     }
@@ -173,7 +171,7 @@ struct SceneTableData : public SceneTableEntry, PersistentData<kPersistentSceneB
         ReturnErrorOnFailure(reader.Next());
         TLV::Tag currTag = reader.GetTag();
         VerifyOrReturnError(TLV::ContextTag(TagScene::kName) == currTag || TLV::ContextTag(TagScene::kTransitionTimeMs) == currTag,
-                            CHIP_ERROR_WRONG_TLV_TYPE);
+                            CHIP_ERROR_INVALID_TLV_TAG);
 
         CharSpan nameSpan;
         // A name may or may not have been stored.  Check whether it was.
@@ -184,10 +182,9 @@ struct SceneTableData : public SceneTableEntry, PersistentData<kPersistentSceneB
         }
         // Empty name will be initialized if the name wasn't stored
         mStorageData.SetName(nameSpan);
-
         ReturnErrorOnFailure(reader.Get(mStorageData.mSceneTransitionTimeMs));
-        ReturnErrorOnFailure(
-            mStorageData.mExtensionFieldSets.Deserialize(reader, TLV::ContextTag(TagScene::kExtensionFieldSetsContainer)));
+
+        ReturnErrorOnFailure(mStorageData.mExtensionFieldSets.Deserialize(reader));
 
         return reader.ExitContainer(container);
     }

diff --git a/src/app/tests/TestExtensionFieldSets.cpp b/src/app/tests/TestExtensionFieldSets.cpp
@@ -235,15 +235,15 @@ void TestSerializeDerializeExtensionFieldSet(nlTestSuite * aSuite, void * aConte
     // All ExtensionFieldSets serialize / deserialize
     writer.Init(sceneEFSBuffer);
     writer.StartContainer(TLV::AnonymousTag(), TLV::kTLVType_Structure, outer);
-    NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == EFS->Serialize(writer, TLV::ContextTag(TagTestEFS::kEFS)));
+    NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == EFS->Serialize(writer));
     writer.EndContainer(outer);
     sceneEFS_serialized_length = writer.GetLengthWritten();
     NL_TEST_ASSERT(aSuite, sceneEFS_serialized_length <= kPersistentSceneBufferMax);
 
     reader.Init(sceneEFSBuffer);
     NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == reader.Next());
     NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == reader.EnterContainer(outerRead));
-    NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == testSceneEFS.Deserialize(reader, TLV::ContextTag(TagTestEFS::kEFS)));
+    NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == testSceneEFS.Deserialize(reader));
 
     NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == reader.ExitContainer(outerRead));
     NL_TEST_ASSERT(aSuite, *EFS == testSceneEFS);

diff --git a/src/lib/core/CHIPConfig.h b/src/lib/core/CHIPConfig.h
@@ -1484,16 +1484,16 @@ extern const char CHIP_NON_PRODUCTION_MARKER[];
  *  }
  *
  *  Including all the TLV fields, the following values can help estimate the needed size for a scenes given a number of clusters:
- *  Empty EFS Scene Max name size: 33bytes
- *  Scene Max name size + OnOff : 51 bytes
- *  Scene Max name size + LevelControl : 60 bytes
- *  Scene Max name size + ColorControl : 126 bytes
- *  Scene Max name size + OnOff + LevelControl + ColoControl : 171 bytes
+ *  Empty EFS Scene Max name size: 37 bytes
+ *  Scene Max name size + OnOff : 55 bytes
+ *  Scene Max name size + LevelControl : 64 bytes
+ *  Scene Max name size + ColorControl : 130 bytes
+ *  Scene Max name size + OnOff + LevelControl + ColoControl : 175 bytes
  *
  *  Cluster Sizes:
- *  OnOff Cluster Max Size: 18 bytes
- *  LevelControl Cluster Max Size: 27 bytes
- *  Color Control Cluster Max Size: 93 bytes
+ *  OnOff Cluster Max Size: 21 bytes
+ *  LevelControl Cluster Max Size: 30 bytes
+ *  Color Control Cluster Max Size: 96 bytes
  * */
 #ifndef CHIP_CONFIG_SCENES_MAX_SERIALIZED_SCENE_SIZE_BYTES
 #define CHIP_CONFIG_SCENES_MAX_SERIALIZED_SCENE_SIZE_BYTES 256