Fix wildcard reads + chunking (#12140)

* Fix Wildcard Reads Using the Matter REPL, this PR fixes a number of critical bugs in both the server-side logic as well as the Python APIs to validate all permutations of wildcard reads as well as basic chunking. On the server side, it fixes: - The AttributeExpandPathIterator was incorrectly using emberAfClusterIndex to iterate over clusters on an endpoint, but that isn't its API contract. That function returns something else completely. Fixed that implementation as well as created a new, better named function called emberAfClusterIndexInMatchingEndpoints that actually better describes what it does. - In ReadSingleClusterData, we were chaining a number of MessageDef builder calls one after another and checking the error at the end (by calling GetError()). However, this resulted in the original error encountered in the first call in that chain being lost and replaced with something else by the time it got to the end. This was especially problematic during chunking since the errors CHIP_ERROR_BUFFER_TOO_SMALL and CHIP_ERROR_NO_MEMORY are what the higher level calls in Engine::BuildSingleReportDataAttributeReportIBs expect to correclty handle chunking. Since the error got lost and converted, it resulted in the Engine treating it like a critical failure and stopping the generation of reports entirely. On the client side in Python: - Reading Clusters.OnOff.Attributes.SampleMfgSpecificAttribute0x00000x1049 broke because in the Python code, we were generating attribute IDs that were 16-bit values instead of 32-bits. This was a combination of not generating them correclty, as well as the C++ bindings expecting a c_uint16 instead of c_uint32 - Fixed up ChipDeviceController.ReadAttribute to actually work correctly with wildcards instead of just erroring out due to parsing issues. Validation: - Tested all wildcard read variations using the Python ReadAttribute API in the REPL and ensured it completed successfully. - Added all variations as tests to the mobile device test suite. * Update src/app/util/attribute-storage.h * Rerun Codegen * Do not enable WiFi on cirque tests * Disable wildcards * Fixes some more bugs in the chunking logic + adds a definitive chunking validation test. * Build fixes Co-authored-by: Song Guo <[email protected]> Co-authored-by: Kaku Matsu <[email protected]>
project-chip · Nov 28, 2021 · f58faac · f58faac
1 parent 1ee4c8d
commit f58faac
Show file tree

Hide file tree

Showing 17 changed files with 1,608 additions and 1,245 deletions.
diff --git a/.github/workflows/cirque.yaml b/.github/workflows/cirque.yaml
@@ -102,6 +102,7 @@ jobs:
                     --volume /tmp:/tmp \
                     -- ./gn_build.sh \
                     chip_build_tests=false \
+                    chip_enable_wifi=false \
                     enable_host_gcc_build=true \
                     enable_host_gcc_mbedtls_build=false \
                     enable_host_clang_build=false \

diff --git a/config/standalone/CHIPProjectConfig.h b/config/standalone/CHIPProjectConfig.h
@@ -75,4 +75,6 @@
 #define DYNAMIC_ENDPOINT_COUNT 4
 #endif
 
+#define CONFIG_IM_BUILD_FOR_UNIT_TEST 1
+
 #endif /* CHIPPROJECTCONFIG_H */
diff --git a/src/app/MessageDef/Builder.cpp b/src/app/MessageDef/Builder.cpp
@@ -47,8 +47,7 @@ void Builder::ResetError()
 
 void Builder::ResetError(CHIP_ERROR aErr)
 {
-    mError              = aErr;
-    mOuterContainerType = chip::TLV::kTLVType_NotSpecified;
+    mError = aErr;
 }
 
 void Builder::EndOfContainer()

diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp
@@ -67,6 +67,8 @@ Engine::RetrieveClusterData(FabricIndex aAccessingFabricIndex, AttributeReportIB
     CHIP_ERROR err = CHIP_NO_ERROR;
 
     AttributeReportIB::Builder attributeReport = aAttributeReportIBs.CreateAttributeReport();
+    err                                        = attributeReport.GetError();
+    SuccessOrExit(attributeReport.GetError());
 
     ChipLogDetail(DataManagement, "<RE:Run> Cluster %" PRIx32 ", Attribute %" PRIx32 " is dirty", aPath.mClusterId,
                   aPath.mAttributeId);
@@ -95,10 +97,17 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu
     bool attributeDataWritten = false;
     bool hasMoreChunks        = true;
     TLV::TLVWriter backup;
+    const uint32_t kReservedSizeEndOfReportIBs = 1;
+
     aReportDataBuilder.Checkpoint(backup);
     auto attributeReportIBs = aReportDataBuilder.CreateAttributeReportIBs();
     SuccessOrExit(err = aReportDataBuilder.GetError());
 
+    //
+    // Reserve enough space for closing out the Report IB list
+    //
+    attributeReportIBs.GetWriter()->ReserveBuffer(kReservedSizeEndOfReportIBs);
+
     {
         // TODO: Figure out how AttributePathExpandIterator should handle read
         // vs write paths.
@@ -127,8 +136,8 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu
                     continue;
                 }
             }
-            // If we are processing a read request, or the initial report of a subscription, just regard all paths as dirty paths.
 
+            // If we are processing a read request, or the initial report of a subscription, just regard all paths as dirty paths.
             TLV::TLVWriter attributeBackup;
             attributeReportIBs.Checkpoint(attributeBackup);
             ConcreteReadAttributePath pathForRetrieval(readPath);
@@ -147,19 +156,48 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu
         hasMoreChunks = false;
     }
 exit:
+    //
+    // Running out of space is an error that we're expected to handle - the incompletely written DataIB has already been rolled back
+    // earlier to ensure only whole and complete DataIBs are present in the stream.
+    //
+    // We can safely clear out the error so that the rest of the machinery to close out the reports, etc. will function correctly.
+    // These are are guaranteed to not fail since we've already reserved memory for the remaining 'close out' TLV operations in this
+    // function and its callers.
+    //
     if ((err == CHIP_ERROR_BUFFER_TOO_SMALL) || (err == CHIP_ERROR_NO_MEMORY))
     {
         ChipLogDetail(DataManagement, "<RE:Run> We cannot put more chunks into this report. Enable chunking.");
+
+        //
+        // Reset the error tracked within the builder. Otherwise, any further attempts to write
+        // data through the builder will be blocked by that error.
+        //
+        attributeReportIBs.ResetError();
         err = CHIP_NO_ERROR;
     }
 
+    //
+    // Only close out the report if we haven't hit an error yet so far.
+    //
     if (err == CHIP_NO_ERROR)
     {
+        attributeReportIBs.GetWriter()->UnreserveBuffer(kReservedSizeEndOfReportIBs);
+
         attributeReportIBs.EndOfAttributeReportIBs();
         err = attributeReportIBs.GetError();
+
+        //
+        // We reserved space for this earlier - consequently, the call to end the ReportIBs should
+        // never fail, so assert if we do since that's a logic bug.
+        //
+        VerifyOrDie(err == CHIP_NO_ERROR);
     }
 
-    if (!attributeDataWritten || err != CHIP_NO_ERROR)
+    //
+    // Rollback the the entire ReportIB array if we never wrote any attributes
+    // AND never hit an error.
+    //
+    if (!attributeDataWritten && err == CHIP_NO_ERROR)
     {
         aReportDataBuilder.Rollback(backup);
     }
@@ -269,7 +307,7 @@ CHIP_ERROR Engine::BuildSingleReportDataEventReports(ReportDataMessage::Builder
     ChipLogDetail(DataManagement, "Fetched %zu events", eventCount);
 
 exit:
-    if (err != CHIP_NO_ERROR || eventCount == 0 || eventClean)
+    if (err == CHIP_NO_ERROR && (eventCount == 0 || eventClean))
     {
         aReportDataBuilder.Rollback(backup);
     }
@@ -289,6 +327,12 @@ CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler)
     uint16_t reservedSize                      = 0;
     bool hasMoreChunks                         = false;
 
+    // Reserved size for the MoreChunks boolean flag, which takes up 1 byte for the control tag and 1 byte for the context tag.
+    const uint32_t kReservedSizeForMoreChunksFlag = 1 + 1;
+
+    // Reserved size for the end of report message, which is an end-of-container (i.e 1 byte for the control tag).
+    const uint32_t kReservedSizeForEndOfReportMessage = 1;
+
     VerifyOrExit(!bufHandle.IsNull(), err = CHIP_ERROR_NO_MEMORY);
 
     if (bufHandle->AvailableDataLength() > kMaxSecureSduLengthBytes)
@@ -298,6 +342,10 @@ CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler)
 
     reportDataWriter.Init(std::move(bufHandle));
 
+#if CONFIG_IM_BUILD_FOR_UNIT_TEST
+    reportDataWriter.ReserveBuffer(mReservedSize);
+#endif
+
     // Always limit the size of the generated packet to fit within kMaxSecureSduLengthBytes regardless of the available buffer
     // capacity.
     // Also, we need to reserve some extra space for the MIC field.
@@ -314,15 +362,17 @@ CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler)
         reportDataBuilder.SubscriptionId(subscriptionId);
     }
 
-    SuccessOrExit(err = reportDataWriter.ReserveBuffer(Engine::kReservedSizeForMoreChunksFlag));
+    SuccessOrExit(err = reportDataWriter.ReserveBuffer(kReservedSizeForMoreChunksFlag + kReservedSizeForEndOfReportMessage));
 
     err = BuildSingleReportDataAttributeReportIBs(reportDataBuilder, apReadHandler, &hasMoreChunks);
     SuccessOrExit(err);
 
     err = BuildSingleReportDataEventReports(reportDataBuilder, apReadHandler, &hasMoreChunks);
     SuccessOrExit(err);
 
-    SuccessOrExit(err = reportDataWriter.UnreserveBuffer(Engine::kReservedSizeForMoreChunksFlag));
+    SuccessOrExit(reportDataBuilder.GetError());
+
+    SuccessOrExit(err = reportDataWriter.UnreserveBuffer(kReservedSizeForMoreChunksFlag + kReservedSizeForEndOfReportMessage));
     if (hasMoreChunks)
     {
         reportDataBuilder.MoreChunkedMessages(hasMoreChunks);
@@ -333,7 +383,12 @@ CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler)
     }
 
     reportDataBuilder.EndOfReportDataMessage();
-    SuccessOrExit(err = reportDataBuilder.GetError());
+
+    //
+    // Since we've already reserved space for both the MoreChunked/SuppressResponse flags, as well as
+    // the end-of-container flag for the end of the report, we should never hit an error closing out the message.
+    //
+    VerifyOrDie(reportDataBuilder.GetError() == CHIP_NO_ERROR);
 
     err = reportDataWriter.Finalize(&bufHandle);
     SuccessOrExit(err);

diff --git a/src/app/reporting/Engine.h b/src/app/reporting/Engine.h
@@ -62,6 +62,10 @@ class Engine
 
     void Shutdown();
 
+#if CONFIG_IM_BUILD_FOR_UNIT_TEST
+    void SetWriterReserved(uint32_t aReservedSize) { mReservedSize = aReservedSize; }
+#endif
+
     /**
      * Main work-horse function that executes the run-loop.
      */
@@ -146,8 +150,9 @@ class Engine
      */
     ClusterInfo * mpGlobalDirtySet = nullptr;
 
-    constexpr static uint32_t kReservedSizeForMoreChunksFlag =
-        2; // According to TLV encoding, the TAG length is 1 byte and its type is 1 byte.
+#if CONFIG_IM_BUILD_FOR_UNIT_TEST
+    uint32_t mReservedSize = 0;
+#endif
 };
 
 }; // namespace reporting

diff --git a/src/app/util/attribute-storage.cpp b/src/app/util/attribute-storage.cpp
@@ -710,7 +710,7 @@ EmberAfStatus emAfReadOrWriteAttribute(EmberAfAttributeSearchRecord * attRecord,
 // mask = CLUSTER_MASK_CLIENT -> find client
 // mask = CLUSTER_MASK_SERVER -> find server
 EmberAfCluster * emberAfFindClusterInTypeWithMfgCode(EmberAfEndpointType * endpointType, ClusterId clusterId,
-                                                     EmberAfClusterMask mask, uint16_t manufacturerCode)
+                                                     EmberAfClusterMask mask, uint16_t manufacturerCode, uint8_t * index)
 {
     uint8_t i;
     for (i = 0; i < endpointType->clusterCount; i++)
@@ -725,6 +725,11 @@ EmberAfCluster * emberAfFindClusterInTypeWithMfgCode(EmberAfEndpointType * endpo
              // if the manufacturerCode == EMBER_AF_NULL_MANUFACTURER_CODE
              || manufacturerCode == EMBER_AF_NULL_MANUFACTURER_CODE))
         {
+            if (index)
+            {
+                *index = i;
+            }
+
             return cluster;
         }
     }
@@ -740,7 +745,7 @@ EmberAfCluster * emberAfFindClusterInType(EmberAfEndpointType * endpointType, Cl
 
 // This code is used during unit tests for clusters that do not involve manufacturer code.
 // Should this code be used in other locations, manufacturerCode should be added.
-uint8_t emberAfClusterIndex(EndpointId endpoint, ClusterId clusterId, EmberAfClusterMask mask)
+uint8_t emberAfClusterIndexInMatchingEndpoints(EndpointId endpoint, ClusterId clusterId, EmberAfClusterMask mask)
 {
     uint8_t ep;
     uint8_t index = 0xFF;
@@ -759,6 +764,24 @@ uint8_t emberAfClusterIndex(EndpointId endpoint, ClusterId clusterId, EmberAfClu
     return 0xFF;
 }
 
+uint8_t emberAfClusterIndex(EndpointId endpoint, ClusterId clusterId, EmberAfClusterMask mask)
+{
+    uint8_t ep;
+    uint8_t index = 0xFF;
+    for (ep = 0; ep < emberAfEndpointCount(); ep++)
+    {
+        EmberAfEndpointType * endpointType = emAfEndpoints[ep].endpointType;
+        if (emberAfFindClusterInTypeWithMfgCode(endpointType, clusterId, mask, EMBER_AF_NULL_MANUFACTURER_CODE, &index) != NULL)
+        {
+            if (emAfEndpoints[ep].endpoint == endpoint)
+            {
+                return index;
+            }
+        }
+    }
+    return 0xFF;
+}
+
 // Returns true uf endpoint contains passed cluster
 bool emberAfContainsClusterWithMfgCode(EndpointId endpoint, ClusterId clusterId, uint16_t manufacturerCode)
 {

diff --git a/src/app/util/attribute-storage.h b/src/app/util/attribute-storage.h
@@ -131,11 +131,13 @@ bool emAfMatchCluster(EmberAfCluster * cluster, EmberAfAttributeSearchRecord * a
 bool emAfMatchAttribute(EmberAfCluster * cluster, EmberAfAttributeMetadata * am, EmberAfAttributeSearchRecord * attRecord);
 
 EmberAfCluster * emberAfFindClusterInTypeWithMfgCode(EmberAfEndpointType * endpointType, chip::ClusterId clusterId,
-                                                     EmberAfClusterMask mask, uint16_t manufacturerCode);
+                                                     EmberAfClusterMask mask, uint16_t manufacturerCode, uint8_t * index = nullptr);
 
 EmberAfCluster * emberAfFindClusterInType(EmberAfEndpointType * endpointType, chip::ClusterId clusterId, EmberAfClusterMask mask);
 
-// This function returns the index of cluster for the particular endpoint.
+// For a given cluster and mask, retrieves the list of endpoints sorted by endpoint that contain the matching cluster and returns
+// the index within that list that matches the given endpoint.
+//
 // Mask is either CLUSTER_MASK_CLIENT or CLUSTER_MASK_SERVER
 // For example, if you have 3 endpoints, 10, 11, 12, and cluster X server is
 // located on 11 and 12, and cluster Y server is located only on 10 then
@@ -145,6 +147,13 @@ EmberAfCluster * emberAfFindClusterInType(EmberAfEndpointType * endpointType, ch
 //    clusterIndex(Y,10,CLUSTER_MASK_SERVER) returns 0
 //    clusterIndex(Y,11,CLUSTER_MASK_SERVER) returns 0xFF
 //    clusterIndex(Y,12,CLUSTER_MASK_SERVER) returns 0xFF
+uint8_t emberAfClusterIndexInMatchingEndpoints(chip::EndpointId endpoint, chip::ClusterId clusterId, EmberAfClusterMask mask);
+
+//
+// Given a cluster ID, endpoint ID and a cluster mask, finds a matching cluster within that endpoint
+// with a matching mask. If one is found, the relative index of that cluster within the list of clusters on that
+// endpoint is returned. Otherwise, 0xFF is returned.
+//
 uint8_t emberAfClusterIndex(chip::EndpointId endpoint, chip::ClusterId clusterId, EmberAfClusterMask mask);
 
 // If server == true, returns the number of server clusters,

diff --git a/src/app/util/ember-compatibility-functions.cpp b/src/app/util/ember-compatibility-functions.cpp
@@ -247,7 +247,10 @@ CHIP_ERROR ReadSingleClusterData(FabricIndex aAccessingFabricIndex, const Concre
     aAttributeReport.Checkpoint(backup);
 
     attributeDataIBBuilder = aAttributeReport.CreateAttributeData();
+    ReturnErrorOnFailure(attributeDataIBBuilder.GetError());
+
     attributePathIBBuilder = attributeDataIBBuilder.CreatePath();
+
     attributePathIBBuilder.Endpoint(aPath.mEndpointId)
         .Cluster(aPath.mClusterId)
         .Attribute(aPath.mAttributeId)
@@ -516,6 +519,7 @@ CHIP_ERROR ReadSingleClusterData(FabricIndex aAccessingFabricIndex, const Concre
     {
         ReturnErrorOnFailure(SendFailureStatus(aPath, aAttributeReport, imStatus, backup));
     }
+
     return CHIP_NO_ERROR;
 }
 

diff --git a/src/controller/python/chip/ChipDeviceCtrl.py b/src/controller/python/chip/ChipDeviceCtrl.py
@@ -422,15 +422,19 @@ async def ReadAttribute(self, nodeid: int, attributes: typing.List[typing.Union[
 
         nodeId: Target's Node ID
         attributes: A list of tuples of varying types depending on the type of read being requested:
-            ():                                         Endpoint = *,           Cluster = *,          Attribute = *
-            (Clusters.ClusterA):                        Endpoint = *,           Cluster = specific,   Attribute = *
-            (Clusters.ClusterA.AttributeA):             Endpoint = *,           Cluster = specific,   Attribute = specific
-            (endpoint, Clusters.ClusterA):              Endpoint = specific,    Cluster = specific,   Attribute = *
             (endpoint, Clusters.ClusterA.AttributeA):   Endpoint = specific,    Cluster = specific,   Attribute = specific
+            (endpoint, Clusters.ClusterA):              Endpoint = specific,    Cluster = specific,   Attribute = *
+            (Clusters.ClusterA.AttributeA):             Endpoint = *,           Cluster = specific,   Attribute = specific
+            endpoint:                                   Endpoint = specific,    Cluster = *,          Attribute = *
+            Clusters.ClusterA:                          Endpoint = *,           Cluster = specific,   Attribute = *
+            '*' or ():                                  Endpoint = *,           Cluster = *,          Attribute = *
 
         The cluster and attributes specified above are to be selected from the generated cluster objects.
 
-        NOTE: Only the last variant is currently supported.
+        e.g 
+            ReadAttribute(1, [ 1 ] ) -- case 4 above.
+            ReadAttribute(1, [ Clusters.Basic ] ) -- case 5 above.
+            ReadAttribute(1, [ (1, Clusters.Basic.Attributes.Location ] ) -- case 1 above.
         '''
 
         eventLoop = asyncio.get_running_loop()
@@ -442,19 +446,20 @@ async def ReadAttribute(self, nodeid: int, attributes: typing.List[typing.Union[
             endpoint = None
             cluster = None
             attribute = None
-            if v == () or v == ('*'):
+            if v == ('*') or v == ():
                 # Wildcard
                 pass
-            elif len(v) == 1:
-                if v[0] is int:
-                    endpoint = v[0]
-                elif issubclass(v[0], ClusterObjects.Cluster):
-                    cluster = v[0]
-                elif issubclass(v[0], ClusterObjects.ClusterAttributeDescriptor):
-                    attribute = v[0]
+            elif type(v) is not tuple:
+                print(type(v))
+                if type(v) is int:
+                    endpoint = v
+                elif issubclass(v, ClusterObjects.Cluster):
+                    cluster = v
+                elif issubclass(v, ClusterObjects.ClusterAttributeDescriptor):
+                    attribute = v
                 else:
                     raise ValueError("Unsupported Attribute Path")
-            elif len(v) == 2:
+            else:
                 # endpoint + (cluster) attribute / endpoint + cluster
                 endpoint = v[0]
                 if issubclass(v[1], ClusterObjects.Cluster):

diff --git a/src/controller/python/chip/clusters/Attribute.py b/src/controller/python/chip/clusters/Attribute.py
@@ -133,10 +133,17 @@ def _handleAttributeData(self, path: AttributePath, status: int, data: bytes):
             if attributeType is None:
                 attributeValue = chip.tlv.TLVReader(data).get().get("Any", {})
             else:
-                attributeValue = attributeType.FromTLV(data)
+                try:
+                    attributeValue = attributeType.FromTLV(data)
+                except:
+                    logging.error(
+                        f"Error convering TLV to Cluster Object for path: Endpoint = {path.EndpointId}/Cluster = {path.ClusterId}/Attribute = {path.AttributeId}")
+                    logging.error(
+                        f"Failed Cluster Object: {str(attributeType)}")
+                    raise
 
             self._res.append(AttributeReadResult(
-                Path=path, Status=imStatus, Data=attributeValue))
+                Path=path, Status=imStatus, Data=attributeType(attributeValue)))
         except Exception as ex:
             logging.exception(ex)
 
@@ -196,7 +203,7 @@ def handleDone(self):
 
 
 _OnReadAttributeDataCallbackFunct = CFUNCTYPE(
-    None, py_object, c_uint16, c_uint32, c_uint32, c_uint16, c_void_p, c_size_t)
+    None, py_object, c_uint16, c_uint32, c_uint32, c_uint32, c_void_p, c_size_t)
 _OnReadErrorCallbackFunct = CFUNCTYPE(
     None, py_object, c_uint32)
 _OnReadDoneCallbackFunct = CFUNCTYPE(