Remove DuplicatedAreas error (#35126)

* Removed the DuplicatedAreas error from the XML * Generated code after XML update. * The service are server ignores any duplicate values before calling the delegate. Example was updated accodingly. * Updated test SEAR-1.3 following changes to the duplicated areas error. * Restyled by clang-format * Made select areas const. --------- Co-authored-by: Restyled.io <[email protected]>
project-chip · Aug 23, 2024 · 010d982 · 010d982
1 parent daa2a57
commit 010d982
Show file tree

Hide file tree

Showing 12 changed files with 108 additions and 125 deletions.
diff --git a/examples/rvc-app/rvc-common/include/rvc-service-area-delegate.h b/examples/rvc-app/rvc-common/include/rvc-service-area-delegate.h
@@ -79,8 +79,8 @@ class RvcServiceAreaDelegate : public Delegate
     // command support
     bool IsSetSelectedAreasAllowed(MutableCharSpan & statusText) override;
 
-    bool IsValidSelectAreasSet(const ServiceArea::Commands::SelectAreas::DecodableType & req,
-                               ServiceArea::SelectAreasStatus & areaStatus, MutableCharSpan & statusText) override;
+    bool IsValidSelectAreasSet(const Span<const uint32_t> & selectedAreas, ServiceArea::SelectAreasStatus & areaStatus,
+                               MutableCharSpan & statusText) override;
 
     bool HandleSkipArea(uint32_t skippedArea, MutableCharSpan & skipStatusText) override;
 

diff --git a/examples/rvc-app/rvc-common/rvc-app.matter b/examples/rvc-app/rvc-common/rvc-app.matter
@@ -1438,9 +1438,8 @@ provisional cluster ServiceArea = 336 {
   enum SelectAreasStatus : enum8 {
     kSuccess = 0;
     kUnsupportedArea = 1;
-    kDuplicatedAreas = 2;
-    kInvalidInMode = 3;
-    kInvalidSet = 4;
+    kInvalidInMode = 2;
+    kInvalidSet = 3;
   }
 
   enum SkipAreaStatus : enum8 {

diff --git a/examples/rvc-app/rvc-common/src/rvc-service-area-delegate.cpp b/examples/rvc-app/rvc-common/src/rvc-service-area-delegate.cpp
@@ -113,23 +113,12 @@ bool RvcServiceAreaDelegate::IsSetSelectedAreasAllowed(MutableCharSpan & statusT
     return (mIsSetSelectedAreasAllowedDeviceInstance->*mIsSetSelectedAreasAllowedCallback)(statusText);
 };
 
-bool RvcServiceAreaDelegate::IsValidSelectAreasSet(const Commands::SelectAreas::DecodableType & req, SelectAreasStatus & areaStatus,
+bool RvcServiceAreaDelegate::IsValidSelectAreasSet(const Span<const uint32_t> & selectedAreas, SelectAreasStatus & areaStatus,
                                                    MutableCharSpan & statusText)
 {
-    // if req is empty list return true.
+    if (selectedAreas.empty())
     {
-        size_t reqSize;
-        if (req.newAreas.ComputeSize(&reqSize) != CHIP_NO_ERROR)
-        {
-            areaStatus = SelectAreasStatus::kInvalidSet; // todo Not sure this is the correct error to use here
-            CopyCharSpanToMutableCharSpan("error computing number of selected areas"_span, statusText);
-            return false;
-        }
-
-        if (reqSize == 0)
-        {
-            return true;
-        }
+        return true;
     }
 
     // If there is 1 or 0 supported maps, any combination of areas is valid.
@@ -139,42 +128,34 @@ bool RvcServiceAreaDelegate::IsValidSelectAreasSet(const Commands::SelectAreas::
     }
 
     // Check that all the requested areas are in the same map.
-    auto newAreasIter = req.newAreas.begin();
-    newAreasIter.Next();
-
-    AreaStructureWrapper tempArea;
-    uint32_t ignoredIndex;
-    if (!GetSupportedAreaById(newAreasIter.GetValue(), ignoredIndex, tempArea))
     {
-        areaStatus = SelectAreasStatus::kUnsupportedArea;
-        CopyCharSpanToMutableCharSpan("unable to find selected area in supported areas"_span, statusText);
-        return false;
-    }
-
-    auto mapId = tempArea.mapID.Value(); // It is safe to call `.Value()` as we confirmed that there are at least 2 maps.
-
-    while (newAreasIter.Next())
-    {
-        if (!GetSupportedAreaById(newAreasIter.GetValue(), ignoredIndex, tempArea))
+        AreaStructureWrapper tempArea;
+        uint32_t ignoredIndex;
+        if (!GetSupportedAreaById(selectedAreas[0], ignoredIndex, tempArea))
         {
             areaStatus = SelectAreasStatus::kUnsupportedArea;
             CopyCharSpanToMutableCharSpan("unable to find selected area in supported areas"_span, statusText);
             return false;
         }
 
-        if (tempArea.mapID.Value() != mapId)
+        auto mapId = tempArea.mapID.Value(); // It is safe to call `.Value()` as we confirmed that there are at least 2 maps.
+
+        for (const auto & areaId : selectedAreas.SubSpan(1))
         {
-            areaStatus = SelectAreasStatus::kInvalidSet;
-            CopyCharSpanToMutableCharSpan("all selected areas must be in the same map"_span, statusText);
-            return false;
-        }
-    }
+            if (!GetSupportedAreaById(areaId, ignoredIndex, tempArea))
+            {
+                areaStatus = SelectAreasStatus::kUnsupportedArea;
+                CopyCharSpanToMutableCharSpan("unable to find selected area in supported areas"_span, statusText);
+                return false;
+            }
 
-    if (CHIP_NO_ERROR != newAreasIter.GetStatus())
-    {
-        areaStatus = SelectAreasStatus::kInvalidSet;
-        CopyCharSpanToMutableCharSpan("error processing new areas."_span, statusText);
-        return false;
+            if (tempArea.mapID.Value() != mapId)
+            {
+                areaStatus = SelectAreasStatus::kInvalidSet;
+                CopyCharSpanToMutableCharSpan("all selected areas must be in the same map"_span, statusText);
+                return false;
+            }
+        }
     }
 
     return true;

diff --git a/src/app/clusters/service-area-server/service-area-delegate.h b/src/app/clusters/service-area-server/service-area-delegate.h
@@ -80,18 +80,18 @@ class Delegate
      * If the set of locations is invalid, the locationStatus should be set to InvalidSet and
      * the statusText SHALL include a vendor-defined error description.
      *
-     * The caller of this method will ensure that there are no duplicates is the list
+     * The caller of this method will ensure that there are no duplicates in the list
      * and that all the locations in the set are valid supported locations.
      *
-     * @param[in] req List of new selected locations.
+     * @param[in] selectedAreas List of new selected locations.
      * @param[out] locationStatus Success if all checks pass, error code if failure.
      * @param[out] statusText text describing failure (see description above), size kMaxSizeStatusText.
      * @return true if success.
      *
      * @note If the SelectAreas command is allowed when the device is operating and the selected locations change to none, the
      * device must stop.
      */
-    virtual bool IsValidSelectAreasSet(const Commands::SelectAreas::DecodableType & req, SelectAreasStatus & locationStatus,
+    virtual bool IsValidSelectAreasSet(const Span<const uint32_t> & selectedAreas, SelectAreasStatus & locationStatus,
                                        MutableCharSpan & statusText) = 0;
 
     /**

diff --git a/src/app/clusters/service-area-server/service-area-server.cpp b/src/app/clusters/service-area-server/service-area-server.cpp
@@ -240,64 +240,72 @@ void Instance::HandleSelectAreasCmd(HandlerContext & ctx, const Commands::Select
         }
     }
 
+    uint32_t selectedAreasBuffer[kMaxNumSelectedAreas];
+    auto selectedAreasSpan         = Span<uint32_t>(selectedAreasBuffer, kMaxNumSelectedAreas);
+    uint32_t numberOfSelectedAreas = 0;
+
+    // Closure for checking if an area ID exists in the selectedAreasSpan
+    auto areaAlreadyExists = [&numberOfSelectedAreas, &selectedAreasSpan](uint32_t areaId) {
+        for (uint32_t i = 0; i < numberOfSelectedAreas; i++)
+        {
+            if (areaId == selectedAreasSpan[i])
+            {
+                return true;
+            }
+        }
+        return false;
+    };
+
     // if number of selected locations in parameter matches number in attribute - the locations *might* be the same
     bool matchesCurrentSelectedAreas = (numberOfAreas == mDelegate->GetNumberOfSelectedAreas());
 
+    // do as much parameter validation as we can
     if (numberOfAreas != 0)
     {
-        // do as much parameter validation as we can
+        uint32_t ignoredIndex = 0;
+        uint32_t oldSelectedArea;
+        auto iAreaIter = req.newAreas.begin();
+        while (iAreaIter.Next())
         {
-            uint32_t ignoredIndex = 0;
-            uint32_t oldSelectedArea;
-            uint32_t i     = 0;
-            auto iAreaIter = req.newAreas.begin();
-            while (iAreaIter.Next())
-            {
-                uint32_t aSelectedArea = iAreaIter.GetValue();
+            uint32_t selectedArea = iAreaIter.GetValue();
 
-                // each item in this list SHALL match the AreaID field of an entry on the SupportedAreas attribute's list
-                // If the Status field is set to UnsupportedArea, the StatusText field SHALL be an empty string.
-                if (!IsSupportedArea(aSelectedArea))
-                {
-                    exitResponse(SelectAreasStatus::kUnsupportedArea, ""_span);
-                    return;
-                }
+            // If aSelectedArea is already in selectedAreasSpan skip
+            if (areaAlreadyExists(selectedArea))
+            {
+                continue;
+            }
 
-                // Checking for duplicate locations.
-                uint32_t j     = 0;
-                auto jAreaIter = req.newAreas.begin();
-                while (j < i)
-                {
-                    jAreaIter.Next(); // Since j < i and i is valid, we can safely call Next() without checking the return value.
-                    if (jAreaIter.GetValue() == aSelectedArea)
-                    {
-                        exitResponse(SelectAreasStatus::kDuplicatedAreas, ""_span);
-                        return;
-                    }
-                    j += 1;
-                }
+            // each item in this list SHALL match the AreaID field of an entry on the SupportedAreas attribute's list
+            // If the Status field is set to UnsupportedArea, the StatusText field SHALL be an empty string.
+            if (!IsSupportedArea(selectedArea))
+            {
+                exitResponse(SelectAreasStatus::kUnsupportedArea, ""_span);
+                return;
+            }
 
-                // check to see if parameter list and attribute still match
-                if (matchesCurrentSelectedAreas)
+            // check to see if parameter list and attribute still match
+            if (matchesCurrentSelectedAreas)
+            {
+                if (!mDelegate->GetSelectedAreaByIndex(ignoredIndex, oldSelectedArea) || (selectedArea != oldSelectedArea))
                 {
-                    if (!mDelegate->GetSelectedAreaByIndex(ignoredIndex, oldSelectedArea) || (aSelectedArea != oldSelectedArea))
-                    {
-                        matchesCurrentSelectedAreas = false;
-                    }
+                    matchesCurrentSelectedAreas = false;
                 }
-
-                i += 1;
             }
 
-            // after iterating with Next through DecodableType - check for failure
-            if (CHIP_NO_ERROR != iAreaIter.GetStatus())
-            {
-                ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Status::InvalidCommand);
-                return;
-            }
+            selectedAreasSpan[numberOfSelectedAreas] = selectedArea;
+            numberOfSelectedAreas += 1;
+        }
+
+        // after iterating with Next through DecodableType - check for failure
+        if (CHIP_NO_ERROR != iAreaIter.GetStatus())
+        {
+            ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Status::InvalidCommand);
+            return;
         }
     }
 
+    selectedAreasSpan.reduce_size(numberOfSelectedAreas);
+
     // If the newAreas field is the same as the value of the SelectedAreas attribute
     // the SelectAreasResponse command SHALL have the Status field set to Success and
     // the StatusText field MAY be supplied with a human-readable string or include an empty string.
@@ -327,7 +335,7 @@ void Instance::HandleSelectAreasCmd(HandlerContext & ctx, const Commands::Select
     // ask the device to handle SelectAreas Command
     // (note - locationStatusText to be filled out by delegated function for kInvalidInMode and InvalidSet)
     auto locationStatus = SelectAreasStatus::kSuccess;
-    if (!mDelegate->IsValidSelectAreasSet(req, locationStatus, delegateStatusText))
+    if (!mDelegate->IsValidSelectAreasSet(selectedAreasSpan, locationStatus, delegateStatusText))
     {
         exitResponse(locationStatus, delegateStatusText);
         return;
@@ -342,11 +350,10 @@ void Instance::HandleSelectAreasCmd(HandlerContext & ctx, const Commands::Select
 
         if (numberOfAreas != 0)
         {
-            auto locationIter = req.newAreas.begin();
             uint32_t ignored;
-            while (locationIter.Next())
+            for (uint32_t areaId : selectedAreasSpan)
             {
-                mDelegate->AddSelectedArea(locationIter.GetValue(), ignored);
+                mDelegate->AddSelectedArea(areaId, ignored);
             }
         }
 

diff --git a/src/app/zap-templates/zcl/data-model/chip/service-area-cluster.xml b/src/app/zap-templates/zcl/data-model/chip/service-area-cluster.xml
@@ -63,9 +63,8 @@ limitations under the License.
     <cluster code="0x0150"/>
     <item value="0x00" name="Success"/>
     <item value="0x01" name="UnsupportedArea"/>
-    <item value="0x02" name="DuplicatedAreas"/>
-    <item value="0x03" name="InvalidInMode"/>
-    <item value="0x04" name="InvalidSet"/>
+    <item value="0x02" name="InvalidInMode"/>
+    <item value="0x03" name="InvalidSet"/>
   </enum>
 
   <enum name="SkipAreaStatus" type="enum8">

diff --git a/src/controller/data_model/controller-clusters.matter b/src/controller/data_model/controller-clusters.matter
@@ -6463,9 +6463,8 @@ provisional cluster ServiceArea = 336 {
   enum SelectAreasStatus : enum8 {
     kSuccess = 0;
     kUnsupportedArea = 1;
-    kDuplicatedAreas = 2;
-    kInvalidInMode = 3;
-    kInvalidSet = 4;
+    kInvalidInMode = 2;
+    kInvalidSet = 3;
   }
 
   enum SkipAreaStatus : enum8 {

diff --git a/src/controller/python/chip/clusters/Objects.py b/src/controller/python/chip/clusters/Objects.py
diff --git a/src/darwin/Framework/CHIP/zap-generated/MTRBaseClusters.h b/src/darwin/Framework/CHIP/zap-generated/MTRBaseClusters.h
diff --git a/src/python_testing/TC_SEAR_1_3.py b/src/python_testing/TC_SEAR_1_3.py
@@ -109,15 +109,17 @@ async def test_TC_SEAR_1_3(self):
 
         duplicated_areas = [valid_area_id, valid_area_id]
 
-        # FIXME need to check if this is the correct name of this status code
-        await self.send_cmd_select_areas_expect_response(step=3, new_areas=duplicated_areas, expected_response=Clusters.ServiceArea.Enums.SelectAreasStatus.kDuplicatedAreas)
+        await self.send_cmd_select_areas_expect_response(step=3, new_areas=duplicated_areas, expected_response=Clusters.ServiceArea.Enums.SelectAreasStatus.kSuccess)
 
-        await self.send_cmd_select_areas_expect_response(step=4, new_areas=[], expected_response=Clusters.ServiceArea.Enums.SelectAreasStatus.kSuccess)
+        selected_areas = await self.read_selected_areas(step=4)
+        asserts.assert_true(selected_areas == [valid_area_id], "SelectedAreas should be empty")
 
-        selected_areas = await self.read_selected_areas(step=5)
+        await self.send_cmd_select_areas_expect_response(step=5, new_areas=[], expected_response=Clusters.ServiceArea.Enums.SelectAreasStatus.kSuccess)
+
+        selected_areas = await self.read_selected_areas(step=6)
         asserts.assert_true(len(selected_areas) == 0, "SelectedAreas should be empty")
 
-        await self.send_cmd_select_areas_expect_response(step=6, new_areas=[invalid_area_id], expected_response=Clusters.ServiceArea.Enums.SelectAreasStatus.kUnsupportedArea)
+        await self.send_cmd_select_areas_expect_response(step=8, new_areas=[invalid_area_id], expected_response=Clusters.ServiceArea.Enums.SelectAreasStatus.kUnsupportedArea)
 
         if self.check_pics("SEAR.S.M.INVALID_STATE_FOR_SELECT_AREAS") and self.check_pics("SEAR.S.M.HAS_MANUAL_SELAREA_STATE_CONTROL"):
             test_step = "Manually intervene to put the device in a state that prevents it from executing the SelectAreas command"
@@ -127,7 +129,7 @@ async def test_TC_SEAR_1_3(self):
             else:
                 self.wait_for_user_input(prompt_msg=f"{test_step}, and press Enter when done.\n")
 
-            await self.send_cmd_select_areas_expect_response(step=8, new_areas=[valid_area_id], expected_response=Clusters.ServiceArea.Enums.SelectAreasStatus.kInvalidInMode)
+            await self.send_cmd_select_areas_expect_response(step=10, new_areas=[valid_area_id], expected_response=Clusters.ServiceArea.Enums.SelectAreasStatus.kInvalidInMode)
 
         if self.check_pics("SEAR.S.M.VALID_STATE_FOR_SELECT_AREAS") and self.check_pics("SEAR.S.M.HAS_MANUAL_SELAREA_STATE_CONTROL"):
             test_step = f"Manually intervene to put the device in a state that allows it to execute the SelectAreas({supported_area_ids}) command"
@@ -137,27 +139,27 @@ async def test_TC_SEAR_1_3(self):
             else:
                 self.wait_for_user_input(prompt_msg=f"{test_step}, and press Enter when done.\n")
 
-            await self.send_cmd_select_areas_expect_response(step=10, new_areas=supported_area_ids, expected_response=Clusters.ServiceArea.Enums.SelectAreasStatus.kSuccess)
+            await self.send_cmd_select_areas_expect_response(step=11, new_areas=supported_area_ids, expected_response=Clusters.ServiceArea.Enums.SelectAreasStatus.kSuccess)
 
-            selected_areas = await self.read_selected_areas(step=11)
+            selected_areas = await self.read_selected_areas(step=12)
             asserts.assert_true(len(selected_areas) == len(supported_area_ids),
                                 f"SelectedAreas({selected_areas}) should match SupportedAreas({supported_area_ids})")
 
-            await self.send_cmd_select_areas_expect_response(step=12, new_areas=supported_area_ids, expected_response=Clusters.ServiceArea.Enums.SelectAreasStatus.kSuccess)
+            await self.send_cmd_select_areas_expect_response(step=13, new_areas=supported_area_ids, expected_response=Clusters.ServiceArea.Enums.SelectAreasStatus.kSuccess)
 
         if self.check_pics("SEAR.S.M.VALID_STATE_FOR_SELECT_AREAS") and self.check_pics("SEAR.S.M.HAS_MANUAL_SELAREA_STATE_CONTROL") and self.check_pics("SEAR.S.M.SELECT_AREAS_WHILE_NON_IDLE"):
             test_step = f"Manually intervene to put the device in a state that allows it to execute the SelectAreas({valid_area_id}) command, and put the device in a non-idle state"
-            self.print_step("13", test_step)
+            self.print_step("14", test_step)
             if self.is_ci:
                 self.write_to_app_pipe('{"Name": "Reset"}')
                 await self.send_single_cmd(cmd=Clusters.Objects.RvcRunMode.Commands.ChangeToMode(newMode=1), endpoint=self.endpoint)
             else:
                 self.wait_for_user_input(prompt_msg=f"{test_step}, and press Enter when done.\n")
 
             if self.check_pics("SEAR.S.F00"):
-                await self.send_cmd_select_areas_expect_response(step=14, new_areas=[valid_area_id], expected_response=Clusters.ServiceArea.Enums.SelectAreasStatus.kSuccess)
+                await self.send_cmd_select_areas_expect_response(step=15, new_areas=[valid_area_id], expected_response=Clusters.ServiceArea.Enums.SelectAreasStatus.kSuccess)
             else:
-                await self.send_cmd_select_areas_expect_response(step=14, new_areas=[valid_area_id], expected_response=Clusters.ServiceArea.Enums.SelectAreasStatus.kInvalidInMode)
+                await self.send_cmd_select_areas_expect_response(step=15, new_areas=[valid_area_id], expected_response=Clusters.ServiceArea.Enums.SelectAreasStatus.kInvalidInMode)
 
 
 if __name__ == "__main__":

diff --git a/zzz_generated/app-common/app-common/zap-generated/cluster-enums-check.h b/zzz_generated/app-common/app-common/zap-generated/cluster-enums-check.h