From 58c6789383488e05e735e2e03717fd8482403a3d Mon Sep 17 00:00:00 2001 From: C Freeman Date: Fri, 31 Jan 2025 11:29:46 -0500 Subject: [PATCH 01/12] Python test whl: Use zip files for data_model (#37163) * Python test whl: Use zip files for data_model We are close to the input parameter limit for the builds with the number of files in the data_model directory as-is. With the addition of the 1.4.1 files, we go over. Instead, we can zip the files and have the parser read directly from the zip archive. I think this also makes the whl slightly smaller, which is nice. Spec parsing is tested directly with TestSpecParsingSupport.py and with TestSpecParsingDeviceTypes.py. The TC_DeviceConformance tests also use the parsed spec. * Fix generator script to make the build file in the new format * Add CI checker that build file matches generated * Add an intentional failure so I can see the CI trigger * Add the warning * Add back the CI failure trigger * whoops, missed the part where I check out the code * CI is hard and I am not good at it * Think I got it? Works on act * Revert "Add back the CI failure trigger" This reverts commit 38ad9565a21f63ce848c24f55c511c9d037033c2. * Update src/python_testing/matter_testing_infrastructure/generate_data_model_xmls_gni.py --- .../check-data-model-directory-updates.yaml | 23 ++++++- .../matter_testing_infrastructure/BUILD.gn | 44 +++++++++++-- .../chip/testing/spec_parsing.py | 8 ++- .../data_model_xmls.gni | 16 +++-- .../generate_data_model_xmls_gni.py | 66 ++++++++++--------- 5 files changed, 113 insertions(+), 44 deletions(-) diff --git a/.github/workflows/check-data-model-directory-updates.yaml b/.github/workflows/check-data-model-directory-updates.yaml index 7bd20c87e0e4ec..895ee15c6f20b1 100644 --- a/.github/workflows/check-data-model-directory-updates.yaml +++ b/.github/workflows/check-data-model-directory-updates.yaml @@ -36,4 +36,25 @@ jobs: python3 scripts/dm_xml_ci_change_enforcement.py data_model/1.3 - name: Check for changes to 1.4 data_model directory without a SHA update run: | - python3 scripts/dm_xml_ci_change_enforcement.py data_model/1.4 \ No newline at end of file + python3 scripts/dm_xml_ci_change_enforcement.py data_model/1.4 + + check-data_model-build-file: + name: Check that all data_model files are listed in the data_model_xmls.gni build file + runs-on: ubuntu-latest + container: + image: ghcr.io/project-chip/chip-build + steps: + - name: Checkout + uses: actions/checkout@v4 + - name: Setup pip modules we use + run: | + python3 -m venv out/venv + out/venv/bin/pip3 install \ + jinja2 + - name: Generate build file (data_model_xmls.gni) + run: out/venv/bin/python3 src/python_testing/matter_testing_infrastructure/generate_data_model_xmls_gni.py + - name: Ensure git works in current working directory + run: git config --global --add safe.directory `pwd` + - name: Check for uncommited changes + run: | + git diff --exit-code HEAD -- src/python_testing/matter_testing_infrastructure/data_model_xmls.gni \ No newline at end of file diff --git a/src/python_testing/matter_testing_infrastructure/BUILD.gn b/src/python_testing/matter_testing_infrastructure/BUILD.gn index 28269a3df1fbed..0287a8a064a1d2 100644 --- a/src/python_testing/matter_testing_infrastructure/BUILD.gn +++ b/src/python_testing/matter_testing_infrastructure/BUILD.gn @@ -18,6 +18,7 @@ import("//build_overrides/pigweed.gni") import("//src/python_testing/matter_testing_infrastructure/data_model_xmls.gni") import("$dir_pw_build/python.gni") import("$dir_pw_build/python_dist.gni") +import("$dir_pw_build/zip.gni") pw_python_package("chip-testing-module") { generate_setup = { @@ -55,6 +56,33 @@ pw_python_package("chip-testing-module") { ] } +pw_zip("data_model_zip_1_3") { + inputs = [] + foreach(file, data_model_XMLS_1_3) { + zip_path = rebase_path(file, "${chip_root}/data_model/1.3/", "/") + inputs += [ "${file} > /${zip_path}" ] + } + output = "${root_out_dir}/data_model/zip_1_3.zip" +} + +pw_zip("data_model_zip_1_4") { + inputs = [] + foreach(file, data_model_XMLS_1_4) { + zip_path = rebase_path(file, "${chip_root}/data_model/1.4/", "/") + inputs += [ "${file} > /${zip_path}" ] + } + output = "${root_out_dir}/data_model/zip_1_4.zip" +} + +pw_zip("data_model_zip_master") { + inputs = [] + foreach(file, data_model_XMLS_master) { + zip_path = rebase_path(file, "${chip_root}/data_model/master/", "/") + inputs += [ "${file} > /${zip_path}" ] + } + output = "${root_out_dir}/data_model/zip_master.zip" +} + pw_python_distribution("chip-testing") { packages = [ ":chip-testing-module" ] @@ -65,9 +93,15 @@ pw_python_distribution("chip-testing") { include_extra_files_in_package_data = true } - # Use the imported data_model_XMLS directly - extra_files = [] - foreach(file, data_model_XMLS) { - extra_files += [ "${file} > chip/testing/${file}" ] - } + public_deps = [ + ":data_model_zip_1_3", + ":data_model_zip_1_4", + ":data_model_zip_master", + ] + + extra_files = [ + "${root_out_dir}/data_model/zip_1_3.zip > chip/testing/data_model/1.3/allfiles.zip", + "${root_out_dir}/data_model/zip_1_4.zip > chip/testing/data_model/1.4/allfiles.zip", + "${root_out_dir}/data_model/zip_master.zip > chip/testing/data_model/master/allfiles.zip", + ] } diff --git a/src/python_testing/matter_testing_infrastructure/chip/testing/spec_parsing.py b/src/python_testing/matter_testing_infrastructure/chip/testing/spec_parsing.py index 56c777868d3d87..8aa66382f71b94 100644 --- a/src/python_testing/matter_testing_infrastructure/chip/testing/spec_parsing.py +++ b/src/python_testing/matter_testing_infrastructure/chip/testing/spec_parsing.py @@ -20,6 +20,7 @@ import logging import typing import xml.etree.ElementTree as ElementTree +import zipfile from copy import deepcopy from dataclasses import dataclass from enum import Enum, auto @@ -571,8 +572,11 @@ def get_data_model_directory(data_model_directory: Union[PrebuiltDataModelDirect return data_model_directory # If it's a prebuilt directory, build the path based on the version and data model level - return pkg_resources.files(importlib.import_module('chip.testing')).joinpath( - 'data_model').joinpath(data_model_directory.dirname).joinpath(data_model_level.dirname) + zip_path = pkg_resources.files(importlib.import_module('chip.testing')).joinpath( + 'data_model').joinpath(data_model_directory.dirname).joinpath('allfiles.zip') + path = zipfile.Path(zip_path) + + return path.joinpath(data_model_level.dirname) def build_xml_clusters(data_model_directory: Union[PrebuiltDataModelDirectory, Traversable] = PrebuiltDataModelDirectory.k1_4) -> typing.Tuple[dict[uint, XmlCluster], list[ProblemNotice]]: diff --git a/src/python_testing/matter_testing_infrastructure/data_model_xmls.gni b/src/python_testing/matter_testing_infrastructure/data_model_xmls.gni index fc2d80305e6789..e3aee87a236ad4 100644 --- a/src/python_testing/matter_testing_infrastructure/data_model_xmls.gni +++ b/src/python_testing/matter_testing_infrastructure/data_model_xmls.gni @@ -11,10 +11,15 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +# +# This file is generated by +# src/python_testing/matter_testing_infrastructure/generate_data_model_xmls_gni.py +# +# Manually re-generate this file on any changes to the data_model directory. import("//build_overrides/chip.gni") -data_model_XMLS = [ +data_model_XMLS_1_3 = [ "${chip_root}/data_model/1.3/clusters/ACL-Cluster.xml", "${chip_root}/data_model/1.3/clusters/AccountLogin.xml", "${chip_root}/data_model/1.3/clusters/AdminCommissioningCluster.xml", @@ -116,7 +121,6 @@ data_model_XMLS = [ "${chip_root}/data_model/1.3/clusters/WindowCovering.xml", "${chip_root}/data_model/1.3/clusters/bridge-clusters-ActionsCluster.xml", "${chip_root}/data_model/1.3/clusters/bridge-clusters-BridgedDeviceBasicInformationCluster.xml", - "${chip_root}/data_model/1.3/clusters/cluster_ids.json", "${chip_root}/data_model/1.3/device_types/Aggregator.xml", "${chip_root}/data_model/1.3/device_types/AirPurifier.xml", "${chip_root}/data_model/1.3/device_types/AirQualitySensor.xml", @@ -180,6 +184,9 @@ data_model_XMLS = [ "${chip_root}/data_model/1.3/device_types/WaterValve.xml", "${chip_root}/data_model/1.3/device_types/WindowCovering.xml", "${chip_root}/data_model/1.3/device_types/WindowCoveringController.xml", +] + +data_model_XMLS_1_4 = [ "${chip_root}/data_model/1.4/clusters/ACL-Cluster.xml", "${chip_root}/data_model/1.4/clusters/AccountLogin.xml", "${chip_root}/data_model/1.4/clusters/AdminCommissioningCluster.xml", @@ -291,7 +298,6 @@ data_model_XMLS = [ "${chip_root}/data_model/1.4/clusters/WindowCovering.xml", "${chip_root}/data_model/1.4/clusters/bridge-clusters-ActionsCluster.xml", "${chip_root}/data_model/1.4/clusters/bridge-clusters-BridgedDeviceBasicInformationCluster.xml", - "${chip_root}/data_model/1.4/clusters/cluster_ids.json", "${chip_root}/data_model/1.4/device_types/Aggregator.xml", "${chip_root}/data_model/1.4/device_types/AirPurifier.xml", "${chip_root}/data_model/1.4/device_types/AirQualitySensor.xml", @@ -365,6 +371,9 @@ data_model_XMLS = [ "${chip_root}/data_model/1.4/device_types/WaterValve.xml", "${chip_root}/data_model/1.4/device_types/WindowCovering.xml", "${chip_root}/data_model/1.4/device_types/WindowCoveringController.xml", +] + +data_model_XMLS_master = [ "${chip_root}/data_model/master/clusters/ACL-Cluster.xml", "${chip_root}/data_model/master/clusters/AccountLogin.xml", "${chip_root}/data_model/master/clusters/AdminCommissioningCluster.xml", @@ -482,7 +491,6 @@ data_model_XMLS = [ "${chip_root}/data_model/master/clusters/bridge-clusters-ActionsCluster.xml", "${chip_root}/data_model/master/clusters/bridge-clusters-BridgedDeviceBasicInformationCluster.xml", "${chip_root}/data_model/master/clusters/bridge-clusters-EcosystemInformationCluster.xml", - "${chip_root}/data_model/master/clusters/cluster_ids.json", "${chip_root}/data_model/master/device_types/Aggregator.xml", "${chip_root}/data_model/master/device_types/AirPurifier.xml", "${chip_root}/data_model/master/device_types/AirQualitySensor.xml", diff --git a/src/python_testing/matter_testing_infrastructure/generate_data_model_xmls_gni.py b/src/python_testing/matter_testing_infrastructure/generate_data_model_xmls_gni.py index 76179797f5fe8c..470913b33afc15 100644 --- a/src/python_testing/matter_testing_infrastructure/generate_data_model_xmls_gni.py +++ b/src/python_testing/matter_testing_infrastructure/generate_data_model_xmls_gni.py @@ -13,8 +13,8 @@ # limitations under the License. """ -Generates a gni file containing all data_model files (generally XML and JSON files) -that are typically used by matter_testing_infrastructure. +Generates a gni file containing all data_model files (XML) +that are used by matter_testing_infrastructure. These files are to be bundled with whl packages of the matter_testing_infrastructure so that methods requiring data model files work just by installing the python @@ -26,19 +26,10 @@ # Set chip_root to be dynamically based on the script's location chip_root = os.path.abspath(os.path.join(os.path.dirname(__file__), "../../..")) - -# Directories to search for .xml and .json files relative to chip_root -directories = [ - os.path.join(chip_root, "data_model/1.3/clusters/"), - os.path.join(chip_root, "data_model/1.3/device_types/"), - os.path.join(chip_root, "data_model/1.4/clusters/"), - os.path.join(chip_root, "data_model/1.4/device_types/"), - os.path.join(chip_root, "data_model/master/clusters/"), - os.path.join(chip_root, "data_model/master/device_types/"), -] +data_model_dir = os.path.join(chip_root, "data_model") # Template for generating the GNI file content with proper indentation -GNI_TEMPLATE = """\ +GNI_HEADER = """\ # Copyright (c) 2024 Project CHIP Authors # # Licensed under the Apache License, Version 2.0 (the "License"); @@ -52,43 +43,54 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +# +# This file is generated by +# src/python_testing/matter_testing_infrastructure/generate_data_model_xmls_gni.py +# +# Manually re-generate this file on any changes to the data_model directory. import("//build_overrides/chip.gni") +""" + +GNI_TEMPLATE = """\ -data_model_XMLS = [ +data_model_XMLS_{{ dir }} = [ {% for name in file_list %} - "{{ name }}", + "{{ name }}", {% endfor %} ] -""" -# Function to find and collect all .xml and .json files +""" -def get_data_model_file_names(): +def get_data_model_file_names(directory: str): + ''' Function to find and collect all .xml files''' file_list = [] - for directory in directories: - for root, _, files in os.walk(directory): - for file in files: - if file.endswith(".xml") or file.endswith(".json"): - # Replace absolute path with `${chip_root}` for GNI compatibility - relative_path = os.path.join("${chip_root}", os.path.relpath(root, chip_root), file) - file_list.append(relative_path) + for root, _, files in os.walk(directory): + for file in files: + if file.endswith(".xml"): + # Replace absolute path with `${chip_root}` for GNI compatibility + relative_path = os.path.join("${chip_root}", os.path.relpath(root, chip_root), file) + file_list.append(relative_path) # Sort files alphabetically file_list.sort() return file_list -# Main function to generate the data_model_xmls.gni file - def generate_gni_file(): - # Step 1: Find all files and create the sorted file list - file_list = get_data_model_file_names() - - # Step 2: Render the template with the file list + '''Main function to generate the data_model_xmls.gni file''' environment = jinja2.Environment(trim_blocks=True, lstrip_blocks=True) template = environment.from_string(GNI_TEMPLATE) - output_content = template.render(file_list=file_list) + output_content_per_dir = [] + + # Step 1: Find all files and create the sorted file list + dirs = sorted([d for d in os.listdir(data_model_dir) if os.path.isdir(os.path.join(data_model_dir, d))]) + for directory in dirs: + file_list = get_data_model_file_names(os.path.join(data_model_dir, directory)) + # Step 2: Render the template with the file list + output_content_per_dir.append(template.render(dir=directory.replace('.', '_'), file_list=file_list)) + + output_content = GNI_HEADER + "".join(output_content_per_dir) # Step 3: Dynamically generate the output file path # Get the script's directory (where this script is located) From 48ca7544e9b6ca078c2b1cc7f03768aabedc1c77 Mon Sep 17 00:00:00 2001 From: Junior Martinez <67972863+jmartinez-silabs@users.noreply.github.com> Date: Fri, 31 Jan 2025 12:08:09 -0500 Subject: [PATCH 02/12] [SL-UP] Raise TimerTask to the highest prio (#254) (#37276) --- examples/platform/silabs/FreeRTOSConfig.h | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/examples/platform/silabs/FreeRTOSConfig.h b/examples/platform/silabs/FreeRTOSConfig.h index 462f923aff895b..2d061fad9159be 100644 --- a/examples/platform/silabs/FreeRTOSConfig.h +++ b/examples/platform/silabs/FreeRTOSConfig.h @@ -174,11 +174,8 @@ extern uint32_t SystemCoreClock; /* Software timer related definitions. */ #define configUSE_TIMERS (1) -#ifdef SLI_SI917 +// Keep the timerTask at the highest prio as some of our stacks tasks leverage eventing with timers. #define configTIMER_TASK_PRIORITY (55) /* Highest priority */ -#else -#define configTIMER_TASK_PRIORITY (40) /* Highest priority */ -#endif // SLI_SI917 #define configTIMER_QUEUE_LENGTH (10) #define configTIMER_TASK_STACK_DEPTH (1024) @@ -313,7 +310,7 @@ standard names. */ /* Thread local storage pointers used by the SDK */ #ifndef configNUM_USER_THREAD_LOCAL_STORAGE_POINTERS -#define configNUM_USER_THREAD_LOCAL_STORAGE_POINTERS 2 +#define configNUM_USER_THREAD_LOCAL_STORAGE_POINTERS 0 #endif #ifndef configNUM_SDK_THREAD_LOCAL_STORAGE_POINTERS From 6de6b5078cbc9fc5d8b9315d39cd11dad17188ad Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 31 Jan 2025 12:21:10 -0500 Subject: [PATCH 03/12] Set up an `kInternalDeviceAccess` Auth mode to be used by internal requests when building subject descriptors (#37174) * Support a new auth mode of "internal" * Restyle * Rename kInternal to kInternalDeviceAccess --------- Co-authored-by: Andrei Litvin --- examples/common/pigweed/rpc_services/Attributes.h | 4 ++-- src/access/AccessControl.cpp | 2 ++ src/access/AuthMode.h | 9 +++++---- src/app/dynamic_server/AccessControl.cpp | 3 ++- 4 files changed, 11 insertions(+), 7 deletions(-) diff --git a/examples/common/pigweed/rpc_services/Attributes.h b/examples/common/pigweed/rpc_services/Attributes.h index cfbb400ae2f7f2..eff2af5d66db31 100644 --- a/examples/common/pigweed/rpc_services/Attributes.h +++ b/examples/common/pigweed/rpc_services/Attributes.h @@ -192,7 +192,7 @@ class Attributes : public pw_rpc::nanopb::Attributes::Service return ::pw::Status::NotFound(); } - Access::SubjectDescriptor subjectDescriptor{ .authMode = chip::Access::AuthMode::kPase }; + Access::SubjectDescriptor subjectDescriptor{ .authMode = chip::Access::AuthMode::kInternalDeviceAccess }; app::DataModel::WriteAttributeRequest write_request; write_request.path = path; write_request.operationFlags.Set(app::DataModel::OperationFlags::kInternal); @@ -343,7 +343,7 @@ class Attributes : public pw_rpc::nanopb::Attributes::Service ::pw::Status ReadAttributeIntoTlvBuffer(const app::ConcreteAttributePath & path, MutableByteSpan & tlvBuffer) { - Access::SubjectDescriptor subjectDescriptor{ .authMode = chip::Access::AuthMode::kPase }; + Access::SubjectDescriptor subjectDescriptor{ .authMode = chip::Access::AuthMode::kInternalDeviceAccess }; app::AttributeReportIBs::Builder attributeReports; TLV::TLVWriter writer; TLV::TLVType outer; diff --git a/src/access/AccessControl.cpp b/src/access/AccessControl.cpp index ba149a6a225b54..a223d250529dda 100644 --- a/src/access/AccessControl.cpp +++ b/src/access/AccessControl.cpp @@ -98,6 +98,8 @@ char GetAuthModeStringForLogging(AuthMode authMode) { case AuthMode::kNone: return 'n'; + case AuthMode::kInternalDeviceAccess: + return 'i'; case AuthMode::kPase: return 'p'; case AuthMode::kCase: diff --git a/src/access/AuthMode.h b/src/access/AuthMode.h index c5a2d76820855a..eadaec1d1da9cf 100644 --- a/src/access/AuthMode.h +++ b/src/access/AuthMode.h @@ -27,10 +27,11 @@ namespace Access { // Auth mode should have only one value expressed, which should not be None. enum class AuthMode : uint8_t { - kNone = 0, - kPase = 1 << 5, - kCase = 1 << 6, - kGroup = 1 << 7 + kNone = 0, + kInternalDeviceAccess = 1 << 4, // Not part of an external interaction + kPase = 1 << 5, + kCase = 1 << 6, + kGroup = 1 << 7 }; } // namespace Access diff --git a/src/app/dynamic_server/AccessControl.cpp b/src/app/dynamic_server/AccessControl.cpp index 75385f49308792..17298956eaf51a 100644 --- a/src/app/dynamic_server/AccessControl.cpp +++ b/src/app/dynamic_server/AccessControl.cpp @@ -63,7 +63,8 @@ class AccessControlDelegate : public Access::AccessControl::Delegate return CHIP_ERROR_ACCESS_DENIED; } - if (subjectDescriptor.authMode != AuthMode::kCase && subjectDescriptor.authMode != AuthMode::kPase) + if (subjectDescriptor.authMode != AuthMode::kCase && subjectDescriptor.authMode != AuthMode::kPase && + subjectDescriptor.authMode != AuthMode::kInternalDeviceAccess) { // No idea who is asking; deny for now. return CHIP_ERROR_ACCESS_DENIED; From cdd492bfd956d7016a74553ca85e02a4c023389f Mon Sep 17 00:00:00 2001 From: Alex Sirko <10052495+asirko-soft@users.noreply.github.com> Date: Fri, 31 Jan 2025 17:31:24 +0000 Subject: [PATCH 04/12] TC-FLABEL-2.1 python test (#36744) * implement TC_FLABEL_2_1 with Python * use write_single_attribute method instead of WriteAttribute, remove debug logging * apply linter suggestion * replace @async_test_body with @run_if_endpoint_matches * address review comments * remove unused import * add description * add checks as per the test plan to check that LabelList tuples are of type string and have a length up to 16 bytes * remove yaml test, now has a Python implementation --- scripts/tests/chiptest/__init__.py | 1 - .../certification/Test_TC_FLABEL_2_1.yaml | 86 -------------- src/app/tests/suites/ciTests.json | 1 - src/python_testing/TC_FLABEL_2_1.py | 110 ++++++++++++++++++ 4 files changed, 110 insertions(+), 88 deletions(-) delete mode 100644 src/app/tests/suites/certification/Test_TC_FLABEL_2_1.yaml create mode 100644 src/python_testing/TC_FLABEL_2_1.py diff --git a/scripts/tests/chiptest/__init__.py b/scripts/tests/chiptest/__init__.py index 5cb891a0144b3e..6d15c68349f5c3 100644 --- a/scripts/tests/chiptest/__init__.py +++ b/scripts/tests/chiptest/__init__.py @@ -207,7 +207,6 @@ def _GetDarwinFrameworkToolUnsupportedTests() -> Set[str]: "Test_TC_DGTHREAD_2_2", # Thread Network Diagnostics is not implemented under darwin. "Test_TC_DGTHREAD_2_3", # Thread Network Diagnostics is not implemented under darwin. "Test_TC_DGTHREAD_2_4", # Thread Network Diagnostics is not implemented under darwin. - "Test_TC_FLABEL_2_1", # darwin-framework-tool does not support writing readonly attributes by name "Test_TC_GRPKEY_2_1", # darwin-framework-tool does not support writing readonly attributes by name "Test_TC_LCFG_2_1", # darwin-framework-tool does not support writing readonly attributes by name "Test_TC_OPCREDS_3_7", # darwin-framework-tool does not support the GetCommissionerRootCertificate command. diff --git a/src/app/tests/suites/certification/Test_TC_FLABEL_2_1.yaml b/src/app/tests/suites/certification/Test_TC_FLABEL_2_1.yaml deleted file mode 100644 index 917e5d6017fc6f..00000000000000 --- a/src/app/tests/suites/certification/Test_TC_FLABEL_2_1.yaml +++ /dev/null @@ -1,86 +0,0 @@ -# Copyright (c) 2021 Project CHIP Authors -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -# Auto-generated scripts for harness use only, please review before automation. The endpoints and cluster names are currently set to default - -name: 98.2.1. [TC-FLABEL-2.1] Fixed Label cluster [DUT-server] - -PICS: - - FLABEL.S - -config: - nodeId: 0x12344321 - cluster: "Fixed Label" - endpoint: 1 - -tests: - - label: "Commission DUT to TH" - cluster: "DelayCommands" - command: "WaitForCommissionee" - arguments: - values: - - name: "nodeId" - value: nodeId - - - label: "Step 1: TH reads LabelList from the DUT" - PICS: FLABEL.S.A0000 - command: "readAttribute" - attribute: "LabelList" - response: - constraints: - type: list - - - label: - "Step 2: TH tries to write LabelList attribute of the DUT by setting - Label = Test_Label, Value= Test_Value" - PICS: FLABEL.S.A0000 - command: "writeAttribute" - attribute: "LabelList" - arguments: - value: [{ Label: "Test_Label", Value: "Test_Value" }] - response: - error: UNSUPPORTED_WRITE - - - label: "Step 3: TH reads LabelList from the DUT" - verification: | - ./chip-tool fixedlabel read label-list 1 0 - Verify that the LabelList value is same as value from step 1 On TH(all-clusters-app) : - - [1651124649.820293][2819:2824] CHIP:TOO: Endpoint: 0 Cluster: 0x0000_0040 Attribute 0x0000_0000 DataVersion: 3688229931 - [1651124649.820478][2819:2824] CHIP:TOO: LabelList: 4 entries - [1651124649.820534][2819:2824] CHIP:TOO: [1]: { - [1651124649.820570][2819:2824] CHIP:TOO: Label: room - [1651124649.820602][2819:2824] CHIP:TOO: Value: bedroom 2 - [1651124649.820636][2819:2824] CHIP:TOO: } - [1651124649.820676][2819:2824] CHIP:TOO: [2]: { - [1651124649.820709][2819:2824] CHIP:TOO: Label: orientation - [1651124649.820741][2819:2824] CHIP:TOO: Value: North - [1651124649.820773][2819:2824] CHIP:TOO: } - [1651124649.820812][2819:2824] CHIP:TOO: [3]: { - [1651124649.820845][2819:2824] CHIP:TOO: Label: floor - [1651124649.820875][2819:2824] CHIP:TOO: Value: 2 - [1651124649.820906][2819:2824] CHIP:TOO: } - [1651124649.820945][2819:2824] CHIP:TOO: [4]: { - [1651124649.820977][2819:2824] CHIP:TOO: Label: direction - [1651124649.821008][2819:2824] CHIP:TOO: Value: up - [1651124649.821039][2819:2824] CHIP:TOO: } - [1651124649.821193][2819:2824] CHIP:EM: Sending Standalone Ack for MessageCounter:2439070 on exchange 10798i - cluster: "LogCommands" - command: "UserPrompt" - PICS: PICS_SKIP_SAMPLE_APP && FLABEL.S.A0000 - arguments: - values: - - name: "message" - value: "Enter 'y' after success" - - name: "expectedValue" - value: "y" diff --git a/src/app/tests/suites/ciTests.json b/src/app/tests/suites/ciTests.json index bf446a38478973..9fa23f8a516aa5 100644 --- a/src/app/tests/suites/ciTests.json +++ b/src/app/tests/suites/ciTests.json @@ -57,7 +57,6 @@ "Test_TC_EEVSEM_3_3" ], "FlowMeasurement": ["Test_TC_FLW_2_1"], - "FixedLabel": ["Test_TC_FLABEL_2_1"], "FanControl": [ "Test_TC_FAN_2_1", "Test_TC_FAN_2_2", diff --git a/src/python_testing/TC_FLABEL_2_1.py b/src/python_testing/TC_FLABEL_2_1.py new file mode 100644 index 00000000000000..39544b734d0db5 --- /dev/null +++ b/src/python_testing/TC_FLABEL_2_1.py @@ -0,0 +1,110 @@ +# +# Copyright (c) 2024 Project CHIP Authors +# All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +# === BEGIN CI TEST ARGUMENTS === +# test-runner-runs: +# run1: +# app: ${ALL_CLUSTERS_APP} +# app-args: --discriminator 1234 --KVS kvs1 --trace-to json:${TRACE_APP}.json +# script-args: > +# --storage-path admin_storage.json +# --commissioning-method on-network +# --discriminator 1234 +# --passcode 20202021 +# --endpoint 1 +# --trace-to json:${TRACE_TEST_JSON}.json +# --trace-to perfetto:${TRACE_TEST_PERFETTO}.perfetto +# factory-reset: true +# quiet: true +# === END CI TEST ARGUMENTS === + +import chip.clusters as Clusters +from chip.interaction_model import Status +from chip.testing.matter_testing import MatterBaseTest, TestStep, default_matter_test_main, has_attribute, run_if_endpoint_matches +from mobly import asserts + + +class Test_TC_FLABEL_2_1(MatterBaseTest): + def pics_TC_FLABEL_2_1(self) -> list[str]: + return ["FLABEL.S"] + + def steps_TC_FLABEL_2_1(self) -> list[TestStep]: + return [ + TestStep(1, "Commission DUT to TH", is_commissioning=True), + TestStep(2, "TH reads LabelList from the DUT", "Read is successful"), + TestStep(3, "TH tries to write LabelList attribute", "Write fails with UNSUPPORTED_WRITE"), + TestStep(4, "Verify LabelList hasn't changed", "LabelList matches initial read") + ] + + def desc_TC_FLABEL_2_1(self) -> str: + return "[TC-FLABEL-2.1] Fixed Label Cluster [DUT-server]" + + @run_if_endpoint_matches(has_attribute(Clusters.FixedLabel.Attributes.LabelList)) + async def test_TC_FLABEL_2_1(self): + # Step 1: Commission DUT (already done) + self.step(1) + + # Step 2: Read LabelList attribute + self.step(2) + initial_labels = await self.read_single_attribute_check_success( + cluster=Clusters.Objects.FixedLabel, + attribute=Clusters.Objects.FixedLabel.Attributes.LabelList + ) + asserts.assert_true(isinstance(initial_labels, list), "LabelList should be a list type") + + # Verify each label in the list meets the requirements + for label_struct in initial_labels: + # Verify label field + asserts.assert_true(isinstance(label_struct.label, str), + "Label field must be a string") + asserts.assert_true(len(label_struct.label.encode('utf-8')) <= 16, + f"Label '{label_struct.label}' exceeds 16 bytes") + + # Verify value field + asserts.assert_true(isinstance(label_struct.value, str), + "Value field must be a string") + asserts.assert_true(len(label_struct.value.encode('utf-8')) <= 16, + f"Value '{label_struct.value}' exceeds 16 bytes") + + # Step 3: Attempt to write LabelList (should fail) + self.step(3) + test_label = Clusters.Objects.FixedLabel.Attributes.LabelList( + [Clusters.Objects.FixedLabel.Structs.LabelStruct( + label="Test_Label", + value="Test_Value" + )] + ) + + # Use write_single_attribute with expect_success=False since we expect it to fail + write_status = await self.write_single_attribute( + attribute_value=test_label, + expect_success=False + ) + asserts.assert_equal(write_status, Status.UnsupportedWrite, "Expected UNSUPPORTED_WRITE status") + + # Step 4: Verify LabelList hasn't changed + self.step(4) + final_labels = await self.read_single_attribute_check_success( + cluster=Clusters.Objects.FixedLabel, + attribute=Clusters.Objects.FixedLabel.Attributes.LabelList + ) + asserts.assert_equal(initial_labels, final_labels, + "LabelList should remain unchanged after write attempt") + + +if __name__ == "__main__": + default_matter_test_main() From 435583e5bf4b09c393f0dcf2bd43ee78e9e2ea9c Mon Sep 17 00:00:00 2001 From: yunhanw-google Date: Fri, 31 Jan 2025 09:43:44 -0800 Subject: [PATCH 05/12] update docker for android (#37341) * update docker for android * update base docker chip build version --- integrations/docker/images/base/chip-build/version | 2 +- .../docker/images/stage-3/chip-build-android/Dockerfile | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/integrations/docker/images/base/chip-build/version b/integrations/docker/images/base/chip-build/version index 1939f7e2b4d3b0..aded04f44ab3e9 100644 --- a/integrations/docker/images/base/chip-build/version +++ b/integrations/docker/images/base/chip-build/version @@ -1 +1 @@ -104 : [Telink] Update Docker image (Zephyr update) +105 : Upgrade android docker with new kotlin/gradle diff --git a/integrations/docker/images/stage-3/chip-build-android/Dockerfile b/integrations/docker/images/stage-3/chip-build-android/Dockerfile index 219e6172902cc1..ea60b3db45074e 100644 --- a/integrations/docker/images/stage-3/chip-build-android/Dockerfile +++ b/integrations/docker/images/stage-3/chip-build-android/Dockerfile @@ -28,11 +28,11 @@ RUN set -x \ # Download and install android command line tool (for installing `sdkmanager`) RUN set -x \ - && wget -O /tmp/android-tools.zip https://dl.google.com/android/repository/sdk-tools-linux-3859397.zip \ + && wget -O /tmp/cmdline-tools.zip https://dl.google.com/android/repository/commandlinetools-linux-11076708_latest.zip \ && cd /opt/android/sdk \ - && unzip /tmp/android-tools.zip \ - && rm -f /tmp/android-tools.zip \ - && test -d /opt/android/sdk/tools \ + && unzip /tmp/cmdline-tools.zip \ + && rm -f /tmp/cmdline-tools.zip \ + && test -d /opt/android/sdk/cmdline-tools \ && : # last line # Download and install android NDK From 26341b4e9192146b0ac1c34a612d5ffc372e3fa5 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 31 Jan 2025 16:41:14 -0500 Subject: [PATCH 06/12] Make the wildcard subscription test in TestReadInteraction more maintainable. (#37338) * Prepare for capture testing... * Update test to look more sane * Update includes * More includes fixes * One more update * Restyled by clang-format * Fix types to make arm/xtensa compiles happy * Restyled by clang-format * Make clang tidy happy * Restyled by clang-format * Even more readable code * Restyled by clang-format * Update src/app/tests/TestReadInteraction.cpp Co-authored-by: Andy Salisbury * Update src/app/tests/TestReadInteraction.cpp Co-authored-by: Andy Salisbury * Update src/app/tests/TestReadInteraction.cpp Co-authored-by: Andy Salisbury * Removed odd todo --------- Co-authored-by: Andrei Litvin Co-authored-by: Restyled.io Co-authored-by: Andy Salisbury --- src/app/tests/TestReadInteraction.cpp | 344 +++++++++++++++++++++++--- 1 file changed, 312 insertions(+), 32 deletions(-) diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp index b56db3e82dc1b5..b9313d896fd499 100644 --- a/src/app/tests/TestReadInteraction.cpp +++ b/src/app/tests/TestReadInteraction.cpp @@ -15,8 +15,11 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +#include + #include #include +#include #include #include #include @@ -30,6 +33,7 @@ #include #include #include +#include #include #include #include @@ -37,13 +41,19 @@ #include #include #include +#include +#include +#include #include #include #include #include -#include + +#include namespace { +using namespace chip::app::Clusters::Globals::Attributes; + uint8_t gDebugEventBuffer[128]; uint8_t gInfoEventBuffer[128]; uint8_t gCritEventBuffer[128]; @@ -71,7 +81,6 @@ static bool sUsingSubSync = false; const chip::Test::MockNodeConfig & TestMockNodeConfig() { using namespace chip::app; - using namespace chip::app::Clusters::Globals::Attributes; using namespace chip::Test; // clang-format off @@ -167,6 +176,36 @@ void GenerateEvents() EXPECT_EQ(logMgmt.LogEvent(&testEventGenerator, options2, eid2), CHIP_NO_ERROR); } +/// Represents an expected attribute capture +class AttributeCaptureAssertion +{ +public: + constexpr AttributeCaptureAssertion(chip::EndpointId ep, chip::ClusterId cl, chip::AttributeId at, + std::optional listSize = std::nullopt) : + mEndpoint(ep), + mCluster(cl), mAttribute(at), mListSize(listSize) + {} + + chip::app::ConcreteAttributePath Path() const { return chip::app::ConcreteAttributePath(mEndpoint, mCluster, mAttribute); } + + chip::EndpointId Endpoint() const { return mEndpoint; } + chip::ClusterId Cluster() const { return mCluster; } + chip::AttributeId Attribute() const { return mAttribute; } + std::optional ListSize() const { return mListSize; } + + bool Matches(const chip::app::ConcreteAttributePath & path, const std::optional & listSize) const + { + return (Path() == path) && (mListSize == listSize); + } + +private: + // this is split out because ConcreteAttributePath is NOT constexpr + const chip::EndpointId mEndpoint; + const chip::ClusterId mCluster; + const chip::AttributeId mAttribute; + const std::optional mListSize; +}; + class MockInteractionModelApp : public chip::app::ReadClient::Callback { public: @@ -191,13 +230,19 @@ class MockInteractionModelApp : public chip::app::ReadClient::Callback { if (status.mStatus == chip::Protocols::InteractionModel::Status::Success) { + ChipLogProgress(Test, "Attribute data received 0x%X/" ChipLogFormatMEI "/" ChipLogFormatMEI " Success: LIST: %s", + aPath.mEndpointId, ChipLogValueMEI(aPath.mClusterId), ChipLogValueMEI(aPath.mAttributeId), + aPath.IsListOperation() ? "true" : "false"); mReceivedAttributePaths.push_back(aPath); mNumAttributeResponse++; mGotReport = true; + std::optional listSize = std::nullopt; + if (aPath.IsListItemOperation()) { mNumArrayItems++; + listSize = 1; } else if (aPath.IsListOperation()) { @@ -208,10 +253,18 @@ class MockInteractionModelApp : public chip::app::ReadClient::Callback size_t count = 0; if (chip::TLV::Utilities::Count(*apData, count, /* aRecurse = */ false) == CHIP_NO_ERROR) { + listSize = static_cast(count); mNumArrayItems += static_cast(count); + ChipLogProgress(Test, " List count: %u", static_cast(count)); } } } + mReceivedListSizes.push_back(listSize); + } + else + { + ChipLogError(NotSpecified, "ERROR status for 0x%X/" ChipLogFormatMEI "/" ChipLogFormatMEI, aPath.mEndpointId, + ChipLogValueMEI(aPath.mClusterId), ChipLogValueMEI(aPath.mAttributeId)); } mLastStatusReceived = status; } @@ -242,6 +295,132 @@ class MockInteractionModelApp : public chip::app::ReadClient::Callback } } + // Log the current captures in a code-like format, to more easily update tests + void LogCaptures(const char * heading) + { + ChipLogProgress(Test, "Captured attributes (%s):", heading); + + for (unsigned i = 0; i < mReceivedAttributePaths.size(); i++) + { + const auto & path = mReceivedAttributePaths[i]; + chip::StringBuilder<128> argsBuffer; + + if (path.mEndpointId >= 0xff) + { + argsBuffer.AddFormat("0x%X, ", path.mEndpointId); + } + else + { + argsBuffer.AddFormat("%u, ", path.mEndpointId); + } + + if (path.mClusterId >= 0xff) + { + argsBuffer.AddFormat("0x%lX, ", static_cast(path.mClusterId)); + } + else + { + argsBuffer.AddFormat("%lu, ", static_cast(path.mClusterId)); + } + + switch (path.mAttributeId) + { + case ClusterRevision::Id: + argsBuffer.Add("ClusterRevision::Id"); + break; + case FeatureMap::Id: + argsBuffer.Add("FeatureMap::Id"); + break; + case GeneratedCommandList::Id: + argsBuffer.Add("GeneratedCommandList::Id"); + break; + case AcceptedCommandList::Id: + argsBuffer.Add("AcceptedCommandList::Id"); + break; + case AttributeList::Id: + argsBuffer.Add("AttributeList::Id"); + break; + default: + if (path.mAttributeId >= 0xff) + { + argsBuffer.AddFormat("0x%lX", static_cast(path.mAttributeId)); + } + else + { + argsBuffer.AddFormat("%lu", static_cast(path.mAttributeId)); + } + break; + } + + auto listSize = mReceivedListSizes[i]; + if (listSize.has_value()) + { + argsBuffer.AddFormat(", /* listSize = */ %u", listSize.value()); + } + + ChipLogProgress(Test, " AttributeCaptureAssertion(%s),", argsBuffer.c_str()); + } + } + + void Reset() + { + mNumDataElementIndex = 0; + mGotEventResponse = false; + mNumReadEventFailureStatusReceived = 0; + mNumAttributeResponse = 0; + mNumArrayItems = 0; + mGotReport = false; + mReadError = false; + mError = CHIP_NO_ERROR; + mReceivedAttributePaths.clear(); + mReceivedListSizes.clear(); + } + + bool CapturesMatchExactly(chip::Span captures) + { + if (captures.size() != mReceivedAttributePaths.size()) + { + ChipLogError(Test, "Captures do not match: expected %u, got %u instead", static_cast(captures.size()), + static_cast(mReceivedAttributePaths.size())); + return false; + } + + for (unsigned i = 0; i < mReceivedAttributePaths.size(); i++) + { + if (captures[i].Matches(mReceivedAttributePaths[i], mReceivedListSizes[i])) + { + continue; + } + + ChipLogError(Test, "Failure on expected capture index %u:", i); + + chip::StringBuilder<128> buffer; + buffer.AddFormat("0x%X/" ChipLogFormatMEI "/" ChipLogFormatMEI "", captures[i].Endpoint(), + ChipLogValueMEI(captures[i].Cluster()), ChipLogValueMEI(captures[i].Attribute())); + auto listSize = captures[i].ListSize(); + if (listSize.has_value()) + { + buffer.AddFormat(" - list of %u items", *listSize); + } + + ChipLogError(Test, " Expected: %s", buffer.c_str()); + + buffer.Reset(); + buffer.AddFormat("0x%X/" ChipLogFormatMEI "/" ChipLogFormatMEI "", mReceivedAttributePaths[i].mEndpointId, + ChipLogValueMEI(mReceivedAttributePaths[i].mClusterId), + ChipLogValueMEI(mReceivedAttributePaths[i].mEndpointId)); + listSize = mReceivedListSizes[i]; + if (listSize.has_value()) + { + buffer.AddFormat(" - list of %u items", *listSize); + } + ChipLogError(Test, " Actual: %s", buffer.c_str()); + return false; + } + + return true; + } + int mNumDataElementIndex = 0; bool mGotEventResponse = false; int mNumReadEventFailureStatusReceived = 0; @@ -253,6 +432,11 @@ class MockInteractionModelApp : public chip::app::ReadClient::Callback chip::app::StatusIB mLastStatusReceived; CHIP_ERROR mError = CHIP_NO_ERROR; std::vector mReceivedAttributePaths; + + // For every received attribute path, report the size of the underlying list + // - nullopt if NOT a list + // - list size (including 0) if a list + std::vector> mReceivedListSizes; }; // @@ -1365,11 +1549,14 @@ void TestReadInteraction::TestReadChunking() DrainAndServiceIO(); - // We get one chunk with 4 array elements, and then one chunk per - // element, and the total size of the array is - // kMockAttribute4ListLength. - EXPECT_EQ(delegate.mNumAttributeResponse, 1 + (kMockAttribute4ListLength - 4)); - EXPECT_EQ(delegate.mNumArrayItems, 6); + delegate.LogCaptures("TestReadChunking:"); + + constexpr AttributeCaptureAssertion kExpectedResponses[] = { + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10004, /* listSize = */ 4), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10004, /* listSize = */ 1), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10004, /* listSize = */ 1), + }; + ASSERT_TRUE(delegate.CapturesMatchExactly(chip::Span(kExpectedResponses))); EXPECT_TRUE(delegate.mGotReport); EXPECT_FALSE(delegate.mReadError); // By now we should have closed all exchanges and sent all pending acks, so @@ -2639,21 +2826,86 @@ void TestReadInteraction::TestSubscribeWildcard() // - cluster 0xFFF1'FC03 (2 attributes) // - cluster 0xFFF1'FC04 (2 attributes) // - // For at total of 29 attributes. There are two wildcard subscription - // paths, for a total of 58 attributes. // + // Actual chunk placement is execution defined, however generally // Attribute 0xFFFC::0xFFF1'FC02::0xFFF1'0004 (kMockEndpoint3::MockClusterId(2)::MockAttributeId(4)) - // is a list of kMockAttribute4ListLength elements of size 256 bytes each, which cannot fit in a single - // packet, so gets list chunking applied to it. + // is a list of kMockAttribute4ListLength of size 256 bytes each, which cannot fit + // in a single packet, so chunking is applied (we get a list and then individual elements as + // single items) // - // Because delegate.mNumAttributeResponse counts AttributeDataIB instances, not attributes, - // the count will depend on exactly how the list for attribute - // 0xFFFC::0xFFF1'FC02::0xFFF1'0004 is chunked. For each of the two instances of that attribute - // in the response, there will be one AttributeDataIB for the start of the list (which will include - // some number of 256-byte elements), then one AttributeDataIB for each of the remaining elements. - constexpr size_t kExpectedAttributeResponse = 29 * 2 + (kMockAttribute4ListLength - 4) + (kMockAttribute4ListLength - 4); - EXPECT_EQ((unsigned) delegate.mNumAttributeResponse, kExpectedAttributeResponse); - EXPECT_EQ(delegate.mNumArrayItems, 12); + // The assertions below expect a specific order verified as ok (updates should verify + // that the updates make sense) + + delegate.LogCaptures("TestSubscribeWildcard: initial subscription"); + + constexpr AttributeCaptureAssertion kExpectedResponses[] = { + AttributeCaptureAssertion(0xFFFE, 0xFFF1FC01, ClusterRevision::Id), + AttributeCaptureAssertion(0xFFFE, 0xFFF1FC01, FeatureMap::Id), + AttributeCaptureAssertion(0xFFFE, 0xFFF1FC02, ClusterRevision::Id), + AttributeCaptureAssertion(0xFFFE, 0xFFF1FC02, FeatureMap::Id), + AttributeCaptureAssertion(0xFFFE, 0xFFF1FC02, 0xFFF10001), + AttributeCaptureAssertion(0xFFFD, 0xFFF1FC01, ClusterRevision::Id), + AttributeCaptureAssertion(0xFFFD, 0xFFF1FC01, FeatureMap::Id), + AttributeCaptureAssertion(0xFFFD, 0xFFF1FC02, ClusterRevision::Id), + AttributeCaptureAssertion(0xFFFD, 0xFFF1FC02, FeatureMap::Id), + AttributeCaptureAssertion(0xFFFD, 0xFFF1FC02, 0xFFF10001), + AttributeCaptureAssertion(0xFFFD, 0xFFF1FC02, 0xFFF10002), + AttributeCaptureAssertion(0xFFFD, 0xFFF1FC03, ClusterRevision::Id), + AttributeCaptureAssertion(0xFFFD, 0xFFF1FC03, FeatureMap::Id), + AttributeCaptureAssertion(0xFFFD, 0xFFF1FC03, 0xFFF10001), + AttributeCaptureAssertion(0xFFFD, 0xFFF1FC03, 0xFFF10002), + AttributeCaptureAssertion(0xFFFD, 0xFFF1FC03, 0xFFF10003), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC01, ClusterRevision::Id), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC01, FeatureMap::Id), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC01, 0xFFF10001), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, ClusterRevision::Id), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, FeatureMap::Id), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10001), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10002), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10003), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10004, /* listSize = */ 4), + // Chunking here + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10004, /* listSize = */ 1), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10004, /* listSize = */ 1), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC03, ClusterRevision::Id), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC03, FeatureMap::Id), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC04, ClusterRevision::Id), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC04, FeatureMap::Id), + AttributeCaptureAssertion(0xFFFE, 0xFFF1FC01, ClusterRevision::Id), + AttributeCaptureAssertion(0xFFFE, 0xFFF1FC01, FeatureMap::Id), + AttributeCaptureAssertion(0xFFFE, 0xFFF1FC02, ClusterRevision::Id), + AttributeCaptureAssertion(0xFFFE, 0xFFF1FC02, FeatureMap::Id), + AttributeCaptureAssertion(0xFFFE, 0xFFF1FC02, 0xFFF10001), + AttributeCaptureAssertion(0xFFFD, 0xFFF1FC01, ClusterRevision::Id), + AttributeCaptureAssertion(0xFFFD, 0xFFF1FC01, FeatureMap::Id), + AttributeCaptureAssertion(0xFFFD, 0xFFF1FC02, ClusterRevision::Id), + AttributeCaptureAssertion(0xFFFD, 0xFFF1FC02, FeatureMap::Id), + AttributeCaptureAssertion(0xFFFD, 0xFFF1FC02, 0xFFF10001), + AttributeCaptureAssertion(0xFFFD, 0xFFF1FC02, 0xFFF10002), + AttributeCaptureAssertion(0xFFFD, 0xFFF1FC03, ClusterRevision::Id), + AttributeCaptureAssertion(0xFFFD, 0xFFF1FC03, FeatureMap::Id), + AttributeCaptureAssertion(0xFFFD, 0xFFF1FC03, 0xFFF10001), + AttributeCaptureAssertion(0xFFFD, 0xFFF1FC03, 0xFFF10002), + AttributeCaptureAssertion(0xFFFD, 0xFFF1FC03, 0xFFF10003), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC01, ClusterRevision::Id), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC01, FeatureMap::Id), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC01, 0xFFF10001), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, ClusterRevision::Id), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, FeatureMap::Id), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10001), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10002), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10003), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10004, /* listSize = */ 4), + // Chunking here + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10004, /* listSize = */ 1), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10004, /* listSize = */ 1), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC03, ClusterRevision::Id), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC03, FeatureMap::Id), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC04, ClusterRevision::Id), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC04, FeatureMap::Id), + }; + + ASSERT_TRUE(delegate.CapturesMatchExactly(chip::Span(kExpectedResponses))); EXPECT_EQ(engine->GetNumActiveReadHandlers(ReadHandler::InteractionType::Subscribe), 1u); ASSERT_NE(engine->ActiveHandlerAt(0), nullptr); delegate.mpReadHandler = engine->ActiveHandlerAt(0); @@ -2678,10 +2930,9 @@ void TestReadInteraction::TestSubscribeWildcard() } // Set a endpoint dirty + ChipLogProgress(NotSpecified, "Testing updates after dirty path setting"); { - delegate.mGotReport = false; - delegate.mNumAttributeResponse = 0; - delegate.mNumArrayItems = 0; + delegate.Reset(); AttributePathParams dirtyPath; dirtyPath.mEndpointId = chip::Test::kMockEndpoint3; @@ -2700,16 +2951,45 @@ void TestReadInteraction::TestSubscribeWildcard() } while (last != delegate.mNumAttributeResponse); // Mock endpoint3 has 13 attributes in total, and we subscribed twice. - // And attribute 3/2/4 is a list with 6 elements and list chunking - // is applied to it, but the way the packet boundaries fall we get two of - // its items as a single list, followed by 4 more items for one - // of our subscriptions, and 3 items as a single list followed by 3 - // more items for the other. - // - // Thus we should receive 13*2 + 4 + 3 = 33 attribute data in total. - ChipLogError(DataManagement, "RESPO: %d\n", delegate.mNumAttributeResponse); - EXPECT_EQ(delegate.mNumAttributeResponse, 33); - EXPECT_EQ(delegate.mNumArrayItems, 12); + delegate.LogCaptures("TestSubscribeWildcard: after dirty"); + constexpr AttributeCaptureAssertion kExpectedResponsesAfterDirty[] = { + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC01, ClusterRevision::Id), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC01, FeatureMap::Id), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC01, 0xFFF10001), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, ClusterRevision::Id), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, FeatureMap::Id), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10001), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10002), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10003), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10004, /* listSize = */ 3), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10004, /* listSize = */ 1), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10004, /* listSize = */ 1), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10004, /* listSize = */ 1), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC03, ClusterRevision::Id), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC03, FeatureMap::Id), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC04, ClusterRevision::Id), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC04, FeatureMap::Id), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC01, ClusterRevision::Id), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC01, FeatureMap::Id), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC01, 0xFFF10001), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, ClusterRevision::Id), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, FeatureMap::Id), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10001), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10002), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10003), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10004, /* listSize = */ 2), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10004, /* listSize = */ 1), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10004, /* listSize = */ 1), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10004, /* listSize = */ 1), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC02, 0xFFF10004, /* listSize = */ 1), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC03, ClusterRevision::Id), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC03, FeatureMap::Id), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC04, ClusterRevision::Id), + AttributeCaptureAssertion(0xFFFC, 0xFFF1FC04, FeatureMap::Id), + + }; + + ASSERT_TRUE(delegate.CapturesMatchExactly(chip::Span(kExpectedResponsesAfterDirty))); } } From c56eb2bcf565fdc5e18e6c113d149534b0c5a301 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 31 Jan 2025 17:14:30 -0500 Subject: [PATCH 07/12] Validate for a valid cluster path when Invoke is called (#37207) * Fix invoke verification of data existence of cluster and endpoint * Add unit test for codegen logic issue * Add integration test for return codes * Restule * Remove odd comment * Remove unused import * Use asserts fail * Add unit test that checks invalid cluster return code as well * Fix comment * Code review feedback: listing of commands should fail if the cluster does not exist * Update src/data-model-providers/codegen/CodegenDataModelProvider.cpp Co-authored-by: Boris Zbarsky * Update src/data-model-providers/codegen/CodegenDataModelProvider.cpp Co-authored-by: Boris Zbarsky * Update src/data-model-providers/codegen/CodegenDataModelProvider.cpp Co-authored-by: Boris Zbarsky * Updated comment * Try to reduce amount of code overhead for extra validation check * Update logic yet again since just checking for null fails unit tests and having unit tests seems reasonable * Restyle * Update src/data-model-providers/codegen/EmberMetadata.h Co-authored-by: Boris Zbarsky * Update src/data-model-providers/codegen/EmberMetadata.h Co-authored-by: Boris Zbarsky * Update src/data-model-providers/codegen/CodegenDataModelProvider.cpp Co-authored-by: Boris Zbarsky * Update comment * Update comment * Update based on code review ... if we cannot enforce an interface, we document. Not happy and hoping we can improve in the future --------- Co-authored-by: Boris Zbarsky Co-authored-by: Andrei Litvin --- src/app/data-model-provider/Provider.h | 10 +++ .../codegen/CodegenDataModelProvider.cpp | 17 +++- .../codegen/EmberMetadata.cpp | 29 +++++-- .../codegen/EmberMetadata.h | 9 ++ .../tests/TestCodegenModelViaMocks.cpp | 85 ++++++++++++++++++- src/python_testing/TestInvokeReturnCodes.py | 81 ++++++++++++++++++ 6 files changed, 218 insertions(+), 13 deletions(-) create mode 100644 src/python_testing/TestInvokeReturnCodes.py diff --git a/src/app/data-model-provider/Provider.h b/src/app/data-model-provider/Provider.h index 13d5a4a4f55e7c..530911b484fcc0 100644 --- a/src/app/data-model-provider/Provider.h +++ b/src/app/data-model-provider/Provider.h @@ -85,6 +85,16 @@ class Provider : public ProviderMetadataTree /// This includes cases where command handling and value return will be done asynchronously. /// - returning a value other than Success implies an error reply (error and data are mutually exclusive) /// + /// Preconditions: + /// - `request.path` MUST be valid: Invoke` is only guaranteed to function correctly for + /// VALID paths (i.e. use `ProviderMetadataTree::AcceptedCommands` to check). This is + /// because we assume ACL or flags (like timed invoke) have to happen before invoking + /// this command. + /// - TODO: as interfaces are updated, we may want to make the above requirement more + /// relaxed, as it seems desirable for users of this interface to have guaranteed + /// behavior (like error on invalid paths) where as today this seems unclear as some + /// command intercepts do not validate if the path is valid per endpoints. + /// /// Return value expectations: /// - if a response has been placed into `handler` then std::nullopt MUST be returned. In particular /// note that CHIP_NO_ERROR is NOT the same as std::nullopt: diff --git a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp index a3889fb1e25ebb..74fc5b769189a5 100644 --- a/src/data-model-providers/codegen/CodegenDataModelProvider.cpp +++ b/src/data-model-providers/codegen/CodegenDataModelProvider.cpp @@ -14,6 +14,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +#include "data-model-providers/codegen/EmberMetadata.h" #include #include @@ -324,6 +325,12 @@ const EmberAfCluster * CodegenDataModelProvider::FindServerCluster(const Concret CHIP_ERROR CodegenDataModelProvider::AcceptedCommands(const ConcreteClusterPath & path, DataModel::ListBuilder & builder) { + // Some CommandHandlerInterface instances are registered of ALL endpoints, so make sure first that + // the cluster actually exists on this endpoint before asking the CommandHandlerInterface whether it + // supports the command. + const EmberAfCluster * serverCluster = FindServerCluster(path); + VerifyOrReturnError(serverCluster != nullptr, CHIP_ERROR_NOT_FOUND); + CommandHandlerInterface * interface = CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(path.mEndpointId, path.mClusterId); if (interface != nullptr) @@ -377,8 +384,6 @@ CHIP_ERROR CodegenDataModelProvider::AcceptedCommands(const ConcreteClusterPath VerifyOrReturnError(err == CHIP_ERROR_NOT_IMPLEMENTED, err); } - const EmberAfCluster * serverCluster = FindServerCluster(path); - VerifyOrReturnError(serverCluster != nullptr, CHIP_ERROR_NOT_FOUND); VerifyOrReturnError(serverCluster->acceptedCommandList != nullptr, CHIP_NO_ERROR); const chip::CommandId * endOfList = serverCluster->acceptedCommandList; @@ -404,6 +409,12 @@ CHIP_ERROR CodegenDataModelProvider::AcceptedCommands(const ConcreteClusterPath CHIP_ERROR CodegenDataModelProvider::GeneratedCommands(const ConcreteClusterPath & path, DataModel::ListBuilder & builder) { + // Some CommandHandlerInterface instances are registered of ALL endpoints, so make sure first that + // the cluster actually exists on this endpoint before asking the CommandHandlerInterface whether it + // supports the command. + const EmberAfCluster * serverCluster = FindServerCluster(path); + VerifyOrReturnError(serverCluster != nullptr, CHIP_ERROR_NOT_FOUND); + CommandHandlerInterface * interface = CommandHandlerInterfaceRegistry::Instance().GetCommandHandler(path.mEndpointId, path.mClusterId); if (interface != nullptr) @@ -454,8 +465,6 @@ CHIP_ERROR CodegenDataModelProvider::GeneratedCommands(const ConcreteClusterPath VerifyOrReturnError(err == CHIP_ERROR_NOT_IMPLEMENTED, err); } - const EmberAfCluster * serverCluster = FindServerCluster(path); - VerifyOrReturnError(serverCluster != nullptr, CHIP_ERROR_NOT_FOUND); VerifyOrReturnError(serverCluster->generatedCommandList != nullptr, CHIP_NO_ERROR); const chip::CommandId * endOfList = serverCluster->generatedCommandList; diff --git a/src/data-model-providers/codegen/EmberMetadata.cpp b/src/data-model-providers/codegen/EmberMetadata.cpp index 22e31a569fe702..54842ee6f25882 100644 --- a/src/data-model-providers/codegen/EmberMetadata.cpp +++ b/src/data-model-providers/codegen/EmberMetadata.cpp @@ -56,16 +56,11 @@ FindAttributeMetadata(const ConcreteAttributePath & aPath) if (metadata == nullptr) { - const EmberAfEndpointType * type = emberAfFindEndpointType(aPath.mEndpointId); - if (type == nullptr) - { - return Status::UnsupportedEndpoint; - } + Status status = ValidateClusterPath(aPath); - const EmberAfCluster * cluster = emberAfFindClusterInType(type, aPath.mClusterId, CLUSTER_MASK_SERVER); - if (cluster == nullptr) + if (status != Status::Success) { - return Status::UnsupportedCluster; + return status; } // Since we know the attribute is unsupported and the endpoint/cluster are @@ -76,6 +71,24 @@ FindAttributeMetadata(const ConcreteAttributePath & aPath) return metadata; } +Status ValidateClusterPath(const ConcreteClusterPath & path) +{ + + const EmberAfEndpointType * type = emberAfFindEndpointType(path.mEndpointId); + if (type == nullptr) + { + return Status::UnsupportedEndpoint; + } + + const EmberAfCluster * cluster = emberAfFindClusterInType(type, path.mClusterId, CLUSTER_MASK_SERVER); + if (cluster == nullptr) + { + return Status::UnsupportedCluster; + } + + return Status::Success; +} + } // namespace Ember } // namespace app } // namespace chip diff --git a/src/data-model-providers/codegen/EmberMetadata.h b/src/data-model-providers/codegen/EmberMetadata.h index 64c0bfe736a25f..06e1568be950c7 100644 --- a/src/data-model-providers/codegen/EmberMetadata.h +++ b/src/data-model-providers/codegen/EmberMetadata.h @@ -16,6 +16,7 @@ */ #pragma once +#include "app/ConcreteClusterPath.h" #include #include #include @@ -42,6 +43,14 @@ std::variant FindAttributeMetadata(const ConcreteAttributePath & aPath); +/// Returns the status for a given cluster existing in the ember metadata. +/// +/// Return code will be one of: +/// - Status::UnsupportedEndpoint if the path endpoint does not exist +/// - Status::UnsupportedCluster if the cluster does not exist on the given endpoint +/// - Status::Success if the cluster exists in the ember metadata. +Protocols::InteractionModel::Status ValidateClusterPath(const ConcreteClusterPath & path); + } // namespace Ember } // namespace app } // namespace chip diff --git a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp index 1045114b7f9b4c..cf156a786d0d53 100644 --- a/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp +++ b/src/data-model-providers/codegen/tests/TestCodegenModelViaMocks.cpp @@ -218,6 +218,44 @@ class MockAccessControl : public Access::AccessControl::Delegate, public Access: bool IsDeviceTypeOnEndpoint(DeviceTypeId deviceType, EndpointId endpoint) override { return true; } }; +class MockCommandHandler : public CommandHandler +{ +public: + CHIP_ERROR FallibleAddStatus(const ConcreteCommandPath & aRequestCommandPath, + const Protocols::InteractionModel::ClusterStatusCode & aStatus, + const char * context = nullptr) override + { + // MOCK: do not do anything here + return CHIP_NO_ERROR; + } + + void AddStatus(const ConcreteCommandPath & aRequestCommandPath, const Protocols::InteractionModel::ClusterStatusCode & aStatus, + const char * context = nullptr) override + { + // MOCK: do not do anything here + } + + FabricIndex GetAccessingFabricIndex() const override { return 1; } + + CHIP_ERROR AddResponseData(const ConcreteCommandPath & aRequestCommandPath, CommandId aResponseCommandId, + const DataModel::EncodableToTLV & aEncodable) override + { + return CHIP_NO_ERROR; + } + + void AddResponse(const ConcreteCommandPath & aRequestCommandPath, CommandId aResponseCommandId, + const DataModel::EncodableToTLV & aEncodable) override + {} + + bool IsTimedInvoke() const override { return false; } + + void FlushAcksRightAwayOnSlowCommand() override {} + + Access::SubjectDescriptor GetSubjectDescriptor() const override { return kAdminSubjectDescriptor; } + + Messaging::ExchangeContext * GetExchangeContext() const override { return nullptr; } +}; + /// Overrides Enumerate*Commands in the CommandHandlerInterface to allow /// testing of behaviors when command enumeration is done in the interace. class CustomListCommandHandler : public CommandHandlerInterface @@ -229,7 +267,20 @@ class CustomListCommandHandler : public CommandHandlerInterface } ~CustomListCommandHandler() { CommandHandlerInterfaceRegistry::Instance().UnregisterCommandHandler(this); } - void InvokeCommand(HandlerContext & handlerContext) override { handlerContext.SetCommandNotHandled(); } + void InvokeCommand(HandlerContext & handlerContext) override + { + if (mHandleCommand) + { + handlerContext.mCommandHandler.AddStatus(handlerContext.mRequestPath, Protocols::InteractionModel::Status::Success); + handlerContext.SetCommandHandled(); + } + else + { + handlerContext.SetCommandNotHandled(); + } + } + + void SetHandleCommands(bool handle) { mHandleCommand = handle; } CHIP_ERROR EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) override { @@ -268,6 +319,7 @@ class CustomListCommandHandler : public CommandHandlerInterface private: bool mOverrideAccepted = false; bool mOverrideGenerated = false; + bool mHandleCommand = false; std::vector mAccepted; std::vector mGenerated; @@ -1156,6 +1208,37 @@ TEST_F(TestCodegenModelViaMocks, IterateOverGeneratedCommands) ASSERT_TRUE(cmds.data_equal(Span(expectedCommands3))); } +TEST_F(TestCodegenModelViaMocks, AcceptedGeneratedCommandsOnInvalidEndpoints) +{ + UseMockNodeConfig config(gTestNodeConfig); + CodegenDataModelProviderWithContext model; + + // register a CHI on ALL endpoints + CustomListCommandHandler handler(chip::NullOptional, MockClusterId(1)); + handler.SetHandleCommands(true); + + DataModel::ListBuilder generatedBuilder; + DataModel::ListBuilder acceptedBuilder; + + // valid endpoint will result in valid data (even though list is empty) + ASSERT_EQ(model.GeneratedCommands(ConcreteClusterPath(kMockEndpoint1, MockClusterId(1)), generatedBuilder), CHIP_NO_ERROR); + ASSERT_TRUE(generatedBuilder.IsEmpty()); + ASSERT_EQ(model.AcceptedCommands(ConcreteClusterPath(kMockEndpoint1, MockClusterId(1)), acceptedBuilder), CHIP_NO_ERROR); + ASSERT_TRUE(acceptedBuilder.IsEmpty()); + + // Invalid endpoint fails - we will get no commands there (even though CHI is registered) + ASSERT_EQ(model.GeneratedCommands(ConcreteClusterPath(kEndpointIdThatIsMissing, MockClusterId(1)), generatedBuilder), + CHIP_ERROR_NOT_FOUND); + ASSERT_EQ(model.AcceptedCommands(ConcreteClusterPath(kEndpointIdThatIsMissing, MockClusterId(1)), acceptedBuilder), + CHIP_ERROR_NOT_FOUND); + + // same for invalid cluster ID + ASSERT_EQ(model.GeneratedCommands(ConcreteClusterPath(kMockEndpoint1, MockClusterId(0x1123)), generatedBuilder), + CHIP_ERROR_NOT_FOUND); + ASSERT_EQ(model.AcceptedCommands(ConcreteClusterPath(kMockEndpoint1, MockClusterId(0x1123)), acceptedBuilder), + CHIP_ERROR_NOT_FOUND); +} + TEST_F(TestCodegenModelViaMocks, CommandHandlerInterfaceCommandHandling) { diff --git a/src/python_testing/TestInvokeReturnCodes.py b/src/python_testing/TestInvokeReturnCodes.py new file mode 100644 index 00000000000000..cd01f8882338c4 --- /dev/null +++ b/src/python_testing/TestInvokeReturnCodes.py @@ -0,0 +1,81 @@ +# +# Copyright (c) 2025 Project CHIP Authors +# All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +# === BEGIN CI TEST ARGUMENTS === +# test-runner-runs: +# run1: +# app: ${ALL_CLUSTERS_APP} +# app-args: > +# --discriminator 1234 +# --KVS kvs1 +# --trace-to json:${TRACE_APP}.json +# script-args: > +# --storage-path admin_storage.json +# --commissioning-method on-network +# --discriminator 1234 +# --passcode 20202021 +# --trace-to json:${TRACE_TEST_JSON}.json +# --trace-to perfetto:${TRACE_TEST_PERFETTO}.perfetto +# factory-reset: true +# quiet: true +# === END CI TEST ARGUMENTS === + +import chip.clusters as Clusters +from chip.interaction_model import InteractionModelError, Status +from chip.testing.matter_testing import MatterBaseTest, async_test_body, default_matter_test_main +from mobly import asserts + + +class TestInvokeReturnCodes(MatterBaseTest): + """ + Validates that the invoke action correctly refuses commands + on invalid endpoints. + """ + + @async_test_body + async def test_invalid_endpoint_command(self): + self.print_step(0, "Commissioning - already done") + + self.print_step(1, "Find an invalid endpoint id") + root_parts = await self.read_single_attribute_check_success( + cluster=Clusters.Descriptor, + attribute=Clusters.Descriptor.Attributes.PartsList, + endpoint=0, + ) + endpoints = set(root_parts) + invalid_endpoint_id = 1 + while invalid_endpoint_id in endpoints: + invalid_endpoint_id += 1 + + self.print_step( + 2, + "Attempt to invoke SoftwareDiagnostics::ResetWatermarks on an invalid endpoint", + ) + try: + await self.send_single_cmd( + cmd=Clusters.SoftwareDiagnostics.Commands.ResetWatermarks(), + endpoint=invalid_endpoint_id, + ) + asserts.fail("Unexpected command success on an invalid endpoint") + except InteractionModelError as e: + asserts.assert_equal( + e.status, Status.UnsupportedEndpoint, "Unexpected error returned" + ) + + +if __name__ == "__main__": + default_matter_test_main() From 95ba8a477929ce65c08be4d54ffa3bdf35016375 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Fri, 31 Jan 2025 20:13:48 -0500 Subject: [PATCH 08/12] Add some descriptions to chip-tool read/subscribe-by-id commands. (#37323) --- .../commands/clusters/ModelCommand.h | 6 ++- .../commands/clusters/ReportCommand.h | 48 ++++++++++++------- 2 files changed, 36 insertions(+), 18 deletions(-) diff --git a/examples/chip-tool/commands/clusters/ModelCommand.h b/examples/chip-tool/commands/clusters/ModelCommand.h index f9ae83aed79f0a..d3e45101b5b830 100644 --- a/examples/chip-tool/commands/clusters/ModelCommand.h +++ b/examples/chip-tool/commands/clusters/ModelCommand.h @@ -28,8 +28,10 @@ class ModelCommand : public CHIPCommand { public: - ModelCommand(const char * commandName, CredentialIssuerCommands * credsIssuerConfig, bool supportsMultipleEndpoints = false) : - CHIPCommand(commandName, credsIssuerConfig), mOnDeviceConnectedCallback(OnDeviceConnectedFn, this), + ModelCommand(const char * commandName, CredentialIssuerCommands * credsIssuerConfig, bool supportsMultipleEndpoints = false, + const char * helpText = nullptr) : + CHIPCommand(commandName, credsIssuerConfig, helpText), + mOnDeviceConnectedCallback(OnDeviceConnectedFn, this), mOnDeviceConnectionFailureCallback(OnDeviceConnectionFailureFn, this), mSupportsMultipleEndpoints(supportsMultipleEndpoints) {} diff --git a/examples/chip-tool/commands/clusters/ReportCommand.h b/examples/chip-tool/commands/clusters/ReportCommand.h index e2c0f1db785900..f9d1d1e9ba33ac 100644 --- a/examples/chip-tool/commands/clusters/ReportCommand.h +++ b/examples/chip-tool/commands/clusters/ReportCommand.h @@ -26,8 +26,9 @@ class ReportCommand : public InteractionModelReports, public ModelCommand, public chip::app::ReadClient::Callback { public: - ReportCommand(const char * commandName, CredentialIssuerCommands * credsIssuerConfig) : - InteractionModelReports(this), ModelCommand(commandName, credsIssuerConfig, /* supportsMultipleEndpoints = */ true) + ReportCommand(const char * commandName, CredentialIssuerCommands * credsIssuerConfig, const char * helpText = nullptr) : + InteractionModelReports(this), + ModelCommand(commandName, credsIssuerConfig, /* supportsMultipleEndpoints = */ true, helpText) {} /////////// ReadClient Callback Interface ///////// @@ -133,8 +134,8 @@ class ReportCommand : public InteractionModelReports, public ModelCommand, publi class ReadCommand : public ReportCommand { protected: - ReadCommand(const char * commandName, CredentialIssuerCommands * credsIssuerConfig) : - ReportCommand(commandName, credsIssuerConfig) + ReadCommand(const char * commandName, CredentialIssuerCommands * credsIssuerConfig, const char * helpText = nullptr) : + ReportCommand(commandName, credsIssuerConfig, helpText) {} void OnDone(chip::app::ReadClient * aReadClient) override @@ -147,8 +148,8 @@ class ReadCommand : public ReportCommand class SubscribeCommand : public ReportCommand { protected: - SubscribeCommand(const char * commandName, CredentialIssuerCommands * credsIssuerConfig) : - ReportCommand(commandName, credsIssuerConfig) + SubscribeCommand(const char * commandName, CredentialIssuerCommands * credsIssuerConfig, const char * helpText = nullptr) : + ReportCommand(commandName, credsIssuerConfig, helpText) {} void OnSubscriptionEstablished(chip::SubscriptionId subscriptionId) override @@ -187,7 +188,8 @@ class SubscribeCommand : public ReportCommand class ReadAttribute : public ReadCommand { public: - ReadAttribute(CredentialIssuerCommands * credsIssuerConfig) : ReadCommand("read-by-id", credsIssuerConfig) + ReadAttribute(CredentialIssuerCommands * credsIssuerConfig) : + ReadCommand("read-by-id", credsIssuerConfig, "Read attributes for the given attribute path (which may include wildcards).") { AddArgument("cluster-ids", 0, UINT32_MAX, &mClusterIds, "Comma-separated list of cluster ids to read from (e.g. \"6\" or \"8,0x201\").\n Allowed to be 0xFFFFFFFF to " @@ -198,7 +200,8 @@ class ReadAttribute : public ReadCommand } ReadAttribute(chip::ClusterId clusterId, CredentialIssuerCommands * credsIssuerConfig) : - ReadCommand("read-by-id", credsIssuerConfig), mClusterIds(1, clusterId) + ReadCommand("read-by-id", credsIssuerConfig, "Read attributes from this cluster; allows wildcard endpoint and attribute."), + mClusterIds(1, clusterId) { AddAttributeIdArgument(); AddCommonArguments(); @@ -245,7 +248,9 @@ class ReadAttribute : public ReadCommand class SubscribeAttribute : public SubscribeCommand { public: - SubscribeAttribute(CredentialIssuerCommands * credsIssuerConfig) : SubscribeCommand("subscribe-by-id", credsIssuerConfig) + SubscribeAttribute(CredentialIssuerCommands * credsIssuerConfig) : + SubscribeCommand("subscribe-by-id", credsIssuerConfig, + "Subscribe to attributes for the given attribute path (which may include wildcards).") { AddArgument("cluster-ids", 0, UINT32_MAX, &mClusterIds, "Comma-separated list of cluster ids to subscribe to (e.g. \"6\" or \"8,0x201\").\n Allowed to be 0xFFFFFFFF " @@ -256,7 +261,9 @@ class SubscribeAttribute : public SubscribeCommand } SubscribeAttribute(chip::ClusterId clusterId, CredentialIssuerCommands * credsIssuerConfig) : - SubscribeCommand("subscribe-by-id", credsIssuerConfig), mClusterIds(1, clusterId) + SubscribeCommand("subscribe-by-id", credsIssuerConfig, + "Subscribe to attributes from this cluster; allows wildcard endpoint and attribute."), + mClusterIds(1, clusterId) { AddAttributeIdArgument(); AddCommonArguments(); @@ -312,7 +319,8 @@ class SubscribeAttribute : public SubscribeCommand class ReadEvent : public ReadCommand { public: - ReadEvent(CredentialIssuerCommands * credsIssuerConfig) : ReadCommand("read-event-by-id", credsIssuerConfig) + ReadEvent(CredentialIssuerCommands * credsIssuerConfig) : + ReadCommand("read-event-by-id", credsIssuerConfig, "Read events for the given event path (which may include wildcards).") { AddArgument("cluster-id", 0, UINT32_MAX, &mClusterIds); AddArgument("event-id", 0, UINT32_MAX, &mEventIds); @@ -322,7 +330,8 @@ class ReadEvent : public ReadCommand } ReadEvent(chip::ClusterId clusterId, CredentialIssuerCommands * credsIssuerConfig) : - ReadCommand("read-event-by-id", credsIssuerConfig), mClusterIds(1, clusterId) + ReadCommand("read-event-by-id", credsIssuerConfig, "Read events from this cluster; allows wildcard endpoint and event."), + mClusterIds(1, clusterId) { AddArgument("event-id", 0, UINT32_MAX, &mEventIds); AddArgument("fabric-filtered", 0, 1, &mFabricFiltered); @@ -356,7 +365,9 @@ class ReadEvent : public ReadCommand class SubscribeEvent : public SubscribeCommand { public: - SubscribeEvent(CredentialIssuerCommands * credsIssuerConfig) : SubscribeCommand("subscribe-event-by-id", credsIssuerConfig) + SubscribeEvent(CredentialIssuerCommands * credsIssuerConfig) : + SubscribeCommand("subscribe-event-by-id", credsIssuerConfig, + "Subscribe to events for the given event path (which may include wildcards).") { AddArgument("cluster-id", 0, UINT32_MAX, &mClusterIds); AddArgument("event-id", 0, UINT32_MAX, &mEventIds); @@ -365,7 +376,9 @@ class SubscribeEvent : public SubscribeCommand } SubscribeEvent(chip::ClusterId clusterId, CredentialIssuerCommands * credsIssuerConfig) : - SubscribeCommand("subscribe-event-by-id", credsIssuerConfig), mClusterIds(1, clusterId) + SubscribeCommand("subscribe-event-by-id", credsIssuerConfig, + "Subscribe to events from this cluster; allows wildcard endpoint and event."), + mClusterIds(1, clusterId) { AddArgument("event-id", 0, UINT32_MAX, &mEventIds); AddCommonArguments(); @@ -447,7 +460,8 @@ class ReadNone : public ReadCommand class ReadAll : public ReadCommand { public: - ReadAll(CredentialIssuerCommands * credsIssuerConfig) : ReadCommand("read-all", credsIssuerConfig) + ReadAll(CredentialIssuerCommands * credsIssuerConfig) : + ReadCommand("read-all", credsIssuerConfig, "Read attributes and events for the given paths (which may include wildcards).") { AddArgument("cluster-ids", 0, UINT32_MAX, &mClusterIds, "Comma-separated list of cluster ids to read from (e.g. \"6\" or \"8,0x201\").\n Allowed to be 0xFFFFFFFF to " @@ -513,7 +527,9 @@ class SubscribeNone : public SubscribeCommand class SubscribeAll : public SubscribeCommand { public: - SubscribeAll(CredentialIssuerCommands * credsIssuerConfig) : SubscribeCommand("subscribe-all", credsIssuerConfig) + SubscribeAll(CredentialIssuerCommands * credsIssuerConfig) : + SubscribeCommand("subscribe-all", credsIssuerConfig, + "Subscribe to attributes and events for the given paths (which may include wildcards).") { AddArgument("cluster-ids", 0, UINT32_MAX, &mClusterIds, "Comma-separated list of cluster ids to read from (e.g. \"6\" or \"8,0x201\").\n Allowed to be 0xFFFFFFFF to " From 94936ea0027e763455b65ff8a2c69dced1129a59 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Fri, 31 Jan 2025 23:08:07 -0500 Subject: [PATCH 09/12] Add MTRDeviceController API to get a list of node IDs with data stored for them. (#37344) * Add MTRDeviceController API to get a list of node IDs with data stored for them. To avoid races between modifications to _nodesWithResumptionInfo and our reading it, added a lock to protect _nodesWithResumptionInfo. Also added a cached _nodesWithAttributeInfo (protected by the same lock) so that we don't have to do a sync dispatch to our storage queue to return a value from this API. * Fix unit test that was relying on the storage being the source of truth for the node index. --- .../Framework/CHIP/MTRDeviceController.h | 7 + .../Framework/CHIP/MTRDeviceController.mm | 6 + .../CHIP/MTRDeviceControllerDataStore.h | 6 + .../CHIP/MTRDeviceControllerDataStore.mm | 124 +++++++++++++----- .../CHIP/MTRDeviceController_Concrete.mm | 10 ++ .../Framework/CHIP/MTRDeviceController_XPC.mm | 5 + .../CHIP/XPC Protocol/MTRXPCServerProtocol.h | 1 + .../CHIPTests/MTRPerControllerStorageTests.m | 9 ++ .../TestHelpers/MTRTestDeclarations.h | 2 + 9 files changed, 138 insertions(+), 32 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.h b/src/darwin/Framework/CHIP/MTRDeviceController.h index 8177eda25cd5ba..41c97e805be95d 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.h +++ b/src/darwin/Framework/CHIP/MTRDeviceController.h @@ -309,6 +309,13 @@ MTR_AVAILABLE(ios(16.1), macos(13.0), watchos(9.1), tvos(16.1)) */ - (void)resume MTR_AVAILABLE(ios(18.2), macos(15.2), watchos(11.2), tvos(18.2)); +/** + * Returns the list of node IDs for which this controller has stored + * information. Returns empty list if the controller does not have any + * information stored. + */ +- (NSArray *)nodesWithStoredData MTR_AVAILABLE(ios(18.4), macos(15.4), watchos(11.4), tvos(18.4)); + /** * Shut down the controller. Calls to shutdown after the first one are NO-OPs. * This must be called, either directly or via shutting down the diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index 7bdbf2302740e9..93b02c9a693ea7 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -508,6 +508,12 @@ + (void)forceLocalhostAdvertisingOnly } #endif // DEBUG +- (NSArray *)nodesWithStoredData +{ + MTR_ABSTRACT_METHOD(); + return @[]; +} + #pragma mark - MTRDeviceControllerDelegate management // Note these are implemented in the base class so that XPC subclass can use it as well diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.h b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.h index f4f38855420345..3feaa5a3e480af 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.h +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.h @@ -105,6 +105,12 @@ typedef void (^MTRDeviceControllerDataStoreClusterDataHandler)(NSDictionary *)nodesWithStoredData; + @end NS_ASSUME_NONNULL_END diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm index 7ca30225b84f88..8a0057ca1ed9f2 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm @@ -19,6 +19,7 @@ // Importing MTRBaseDevice.h for the MTRAttributePath class. Needs to change when https://github.com/project-chip/connectedhomeip/issues/31247 is fixed. #import "MTRBaseDevice.h" #import "MTRLogging_Internal.h" +#import "MTRUnfairLock.h" #include #include @@ -111,6 +112,11 @@ @implementation MTRDeviceControllerDataStore { __weak MTRDeviceController * _controller; // Array of nodes with resumption info, oldest-stored first. NSMutableArray * _nodesWithResumptionInfo; + // Array of nodes with attribute info. + NSMutableArray * _nodesWithAttributeInfo; + // Lock protecting access to the _nodesWithAttributeInfo and + // _nodesWithResumptionInfo arrays. + os_unfair_lock _nodeArrayLock; } - (nullable instancetype)initWithController:(MTRDeviceController *)controller @@ -124,8 +130,10 @@ - (nullable instancetype)initWithController:(MTRDeviceController *)controller _controller = controller; _storageDelegate = storageDelegate; _storageDelegateQueue = storageDelegateQueue; + _nodeArrayLock = OS_UNFAIR_LOCK_INIT; __block id resumptionNodeList; + __block NSArray * nodesWithAttributeInfo; dispatch_sync(_storageDelegateQueue, ^{ @autoreleasepool { // NOTE: controller, not our weak ref, since we know it's still @@ -134,6 +142,8 @@ - (nullable instancetype)initWithController:(MTRDeviceController *)controller valueForKey:sResumptionNodeListKey securityLevel:MTRStorageSecurityLevelSecure sharingType:MTRStorageSharingTypeNotShared]; + + nodesWithAttributeInfo = [self _fetchNodeIndex]; } }); if (resumptionNodeList != nil) { @@ -152,6 +162,12 @@ - (nullable instancetype)initWithController:(MTRDeviceController *)controller _nodesWithResumptionInfo = [[NSMutableArray alloc] init]; } + if (nodesWithAttributeInfo != nil) { + _nodesWithAttributeInfo = [nodesWithAttributeInfo mutableCopy]; + } else { + _nodesWithAttributeInfo = [[NSMutableArray alloc] init]; + } + return self; } @@ -196,6 +212,8 @@ - (void)storeResumptionInfo:(MTRCASESessionResumptionInfo *)resumptionInfo removeValueForKey:ResumptionByResumptionIDKey(oldInfo.resumptionID) securityLevel:MTRStorageSecurityLevelSecure sharingType:MTRStorageSharingTypeNotShared]; + + std::lock_guard lock(self->_nodeArrayLock); [_nodesWithResumptionInfo removeObject:resumptionInfo.nodeID]; } @@ -211,9 +229,14 @@ - (void)storeResumptionInfo:(MTRCASESessionResumptionInfo *)resumptionInfo sharingType:MTRStorageSharingTypeNotShared]; // Update our resumption info node list. - [_nodesWithResumptionInfo addObject:resumptionInfo.nodeID]; + NSArray * valueToStore; + { + std::lock_guard lock(self->_nodeArrayLock); + [_nodesWithResumptionInfo addObject:resumptionInfo.nodeID]; + valueToStore = [_nodesWithResumptionInfo copy]; + } [_storageDelegate controller:controller - storeValue:[_nodesWithResumptionInfo copy] + storeValue:valueToStore forKey:sResumptionNodeListKey securityLevel:MTRStorageSecurityLevelSecure sharingType:MTRStorageSharingTypeNotShared]; @@ -226,7 +249,9 @@ - (void)clearAllResumptionInfo VerifyOrReturn(controller != nil); // No way to call delegate without controller. // Can we do less dispatch? We would need to have a version of - // _findResumptionInfoWithKey that assumes we are already on the right queue. + // _findResumptionInfoWithKey that assumes we are already on the right + // queue. + std::lock_guard lock(_nodeArrayLock); for (NSNumber * nodeID in _nodesWithResumptionInfo) { [self _clearResumptionInfoForNodeID:nodeID controller:controller]; } @@ -240,6 +265,8 @@ - (void)clearResumptionInfoForNodeID:(NSNumber *)nodeID VerifyOrReturn(controller != nil); // No way to call delegate without controller. [self _clearResumptionInfoForNodeID:nodeID controller:controller]; + + std::lock_guard lock(_nodeArrayLock); [_nodesWithResumptionInfo removeObject:nodeID]; } @@ -588,6 +615,20 @@ - (void)unitTestPruneEmptyStoredClusterDataBranches [self _pruneEmptyStoredClusterDataBranches]; }); } + +- (void)unitTestRereadNodeIndex +{ + dispatch_sync(_storageDelegateQueue, ^{ + auto * newIndex = [self _fetchNodeIndex]; + + std::lock_guard lock(self->_nodeArrayLock); + if (newIndex != nil) { + self->_nodesWithAttributeInfo = [newIndex mutableCopy]; + } else { + self->_nodesWithAttributeInfo = [[NSMutableArray alloc] init]; + } + }); +} #endif - (void)_pruneEmptyStoredClusterDataBranches @@ -596,9 +637,8 @@ - (void)_pruneEmptyStoredClusterDataBranches NSUInteger storeFailures = 0; - // Fetch node index - NSArray * nodeIndex = [self _fetchNodeIndex]; - NSMutableArray * nodeIndexCopy = [nodeIndex mutableCopy]; + std::lock_guard lock(self->_nodeArrayLock); + NSArray * nodeIndex = [_nodesWithAttributeInfo copy]; for (NSNumber * nodeID in nodeIndex) { // Fetch endpoint index @@ -638,7 +678,7 @@ - (void)_pruneEmptyStoredClusterDataBranches if (endpointIndexCopy.count) { success = [self _storeEndpointIndex:endpointIndexCopy forNodeID:nodeID]; } else { - [nodeIndexCopy removeObject:nodeID]; + [_nodesWithAttributeInfo removeObject:nodeID]; success = [self _deleteEndpointIndexForNodeID:nodeID]; } if (!success) { @@ -648,16 +688,16 @@ - (void)_pruneEmptyStoredClusterDataBranches } } - if (nodeIndex.count != nodeIndexCopy.count) { + if (nodeIndex.count != _nodesWithAttributeInfo.count) { BOOL success; - if (nodeIndexCopy.count) { - success = [self _storeNodeIndex:nodeIndexCopy]; + if (_nodesWithAttributeInfo.count) { + success = [self _storeNodeIndex:_nodesWithAttributeInfo]; } else { success = [self _deleteNodeIndex]; } if (!success) { storeFailures++; - MTR_LOG_ERROR("Store failed in _pruneEmptyStoredClusterDataBranches for nodeIndex (%lu)", static_cast(nodeIndexCopy.count)); + MTR_LOG_ERROR("Store failed in _pruneEmptyStoredClusterDataBranches for nodeIndex (%lu)", static_cast(_nodesWithAttributeInfo.count)); } } @@ -713,18 +753,19 @@ - (void)clearStoredClusterDataForNodeID:(NSNumber *)nodeID { dispatch_async(_storageDelegateQueue, ^{ [self _clearStoredClusterDataForNodeID:nodeID]; - NSArray * nodeIndex = [self _fetchNodeIndex]; - NSMutableArray * nodeIndexCopy = [nodeIndex mutableCopy]; - [nodeIndexCopy removeObject:nodeID]; - if (nodeIndex.count != nodeIndexCopy.count) { + + std::lock_guard lock(self->_nodeArrayLock); + auto oldCount = self->_nodesWithAttributeInfo.count; + [self->_nodesWithAttributeInfo removeObject:nodeID]; + if (self->_nodesWithAttributeInfo.count != oldCount) { BOOL success; - if (nodeIndexCopy.count) { - success = [self _storeNodeIndex:nodeIndexCopy]; + if (self->_nodesWithAttributeInfo.count) { + success = [self _storeNodeIndex:self->_nodesWithAttributeInfo]; } else { success = [self _deleteNodeIndex]; } if (!success) { - MTR_LOG_ERROR("Store failed in clearStoredAttributesForNodeID for nodeIndex (%lu)", static_cast(nodeIndexCopy.count)); + MTR_LOG_ERROR("Store failed in clearStoredAttributesForNodeID for nodeIndex (%lu)", static_cast(self->_nodesWithAttributeInfo.count)); } } }); @@ -804,12 +845,12 @@ - (void)clearAllStoredClusterData { dispatch_async(_storageDelegateQueue, ^{ // Fetch node index - NSArray * nodeIndex = [self _fetchNodeIndex]; - - for (NSNumber * nodeID in nodeIndex) { + std::lock_guard lock(self->_nodeArrayLock); + for (NSNumber * nodeID in self->_nodesWithAttributeInfo) { [self _clearStoredClusterDataForNodeID:nodeID]; } + [self->_nodesWithAttributeInfo removeAllObjects]; BOOL success = [self _deleteNodeIndex]; if (!success) { MTR_LOG_ERROR("Delete failed for nodeIndex"); @@ -826,14 +867,13 @@ - (void)clearAllStoredClusterData __block NSMutableDictionary * clusterDataToReturn = nil; dispatch_sync(_storageDelegateQueue, ^{ - // Fetch node index - NSArray * nodeIndex = [self _fetchNodeIndex]; + std::lock_guard lock(self->_nodeArrayLock); #if ATTRIBUTE_CACHE_VERBOSE_LOGGING - MTR_LOG("Fetch got %lu values for nodeIndex", static_cast(nodeIndex.count)); + MTR_LOG("Fetch got %lu values for nodeIndex", static_cast(self->_nodesWithAttributeInfo.count)); #endif - if (![nodeIndex containsObject:nodeID]) { + if (![self->_nodesWithAttributeInfo containsObject:nodeID]) { // Sanity check and delete if nodeID exists in index NSArray * endpointIndex = [self _fetchEndpointIndexForNodeID:nodeID]; if (endpointIndex) { @@ -1086,14 +1126,13 @@ - (void)storeClusterData:(NSDictionary } // Check if node index needs updating / creation - NSArray * nodeIndex = [self _fetchNodeIndex]; NSArray * nodeIndexToStore = nil; - if (!nodeIndex) { - // Ensure node index exists - MTR_LOG("No entry found for for nodeIndex - creating for node 0x%016llX", nodeID.unsignedLongLongValue); - nodeIndexToStore = [NSArray arrayWithObject:nodeID]; - } else if (![nodeIndex containsObject:nodeID]) { - nodeIndexToStore = [nodeIndex arrayByAddingObject:nodeID]; + { + std::lock_guard lock(self->_nodeArrayLock); + if (![self->_nodesWithAttributeInfo containsObject:nodeID]) { + [self->_nodesWithAttributeInfo addObject:nodeID]; + nodeIndexToStore = [self->_nodesWithAttributeInfo copy]; + } } if (nodeIndexToStore) { @@ -1206,6 +1245,27 @@ - (void)synchronouslyPerformBlock:(void (^_Nullable)(void))block }); } +- (NSArray *)nodesWithStoredData +{ + // We have three types of stored data: + // + // 1) Attribute data + // 2) Session resumption data + // 3) Device data. + // + // Items 1 and 2 come with node indices. Item 3 does not, but in practice + // we should have device data if and only if we have attribute data for that + // node ID, barring odd error conditions. + // + // TODO: Consider changing how we store device data so we can easily recover + // the relevant set of node IDs. + NSMutableSet * nodeSet = [NSMutableSet set]; + std::lock_guard lock(_nodeArrayLock); + [nodeSet addObjectsFromArray:_nodesWithResumptionInfo]; + [nodeSet addObjectsFromArray:_nodesWithAttributeInfo]; + return [nodeSet allObjects]; +} + @end @implementation MTRCASESessionResumptionInfo diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm index 4c427e7dd9fbcb..47e3f75884354c 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm @@ -1720,6 +1720,16 @@ - (nullable NSNumber *)neededReadPrivilegeForClusterID:(NSNumber *)clusterID att return nil; } +- (NSArray *)nodesWithStoredData +{ + if (!self.controllerDataStore) { + // We have nothing stored, if we have no way to store. + return @[]; + } + + return [self.controllerDataStore nodesWithStoredData]; +} + @end /** diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_XPC.mm b/src/darwin/Framework/CHIP/MTRDeviceController_XPC.mm index 72c7151355021c..3be424b955b749 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_XPC.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController_XPC.mm @@ -64,6 +64,11 @@ @implementation MTRDeviceController_XPC : (NSNumber *) nodeID, deleteNodeID : (NSNumber *) nodeID) +MTR_DEVICECONTROLLER_SIMPLE_REMOTE_XPC_GETTER(nodesWithStoredData, + NSArray *, + @[], // Default return value + getNodesWithStoredDataWithReply) + - (void)_updateRegistrationInfo { NSMutableDictionary * registrationInfo = [NSMutableDictionary dictionary]; diff --git a/src/darwin/Framework/CHIP/XPC Protocol/MTRXPCServerProtocol.h b/src/darwin/Framework/CHIP/XPC Protocol/MTRXPCServerProtocol.h index 1c57480565dacb..3e80bc4e67770f 100644 --- a/src/darwin/Framework/CHIP/XPC Protocol/MTRXPCServerProtocol.h +++ b/src/darwin/Framework/CHIP/XPC Protocol/MTRXPCServerProtocol.h @@ -77,6 +77,7 @@ MTR_AVAILABLE(ios(18.3), macos(15.3), watchos(11.3), tvos(18.3)) - (oneway void)deviceController:(NSUUID *)controller unregisterNodeID:(NSNumber *)nodeID; - (oneway void)deviceController:(NSUUID *)controller updateControllerConfiguration:(NSDictionary *)controllerState MTR_AVAILABLE(ios(18.3), macos(15.3), watchos(11.3), tvos(18.3)); +- (oneway void)deviceController:(NSUUID *)controller getNodesWithStoredDataWithReply:(void (^)(NSArray *))reply MTR_AVAILABLE(ios(18.4), macos(15.4), watchos(11.4), tvos(18.4)); @end MTR_AVAILABLE(ios(18.3), macos(15.3), watchos(11.3), tvos(18.3)) diff --git a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m index 2abfda4fee4b8b..779b42abdfc0be 100644 --- a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m +++ b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m @@ -1521,6 +1521,8 @@ - (void)test008_TestDataStoreDirect dispatch_sync(_storageQueue, ^{ [storageDelegate controller:controller storeValues:testBulkValues securityLevel:MTRStorageSecurityLevelSecure sharingType:MTRStorageSharingTypeNotShared]; }); + // Since we messed with the node index, tell the data store to re-sync it's cache. + [controller.controllerDataStore unitTestRereadNodeIndex]; // Verify that the store resulted in the correct values NSDictionary * dataStoreClusterData = [controller.controllerDataStore getStoredClusterDataForNodeID:@(3001)]; XCTAssertEqualObjects(dataStoreClusterData, bulkTestClusterDataDictionary); @@ -1772,6 +1774,10 @@ - (void)test010_TestDataStoreMTRDeviceWithBulkReadWrite XCTAssertNotNil([dataStore findResumptionInfoByNodeID:deviceID]); XCTAssertNotNil([dataStore getStoredDeviceDataForNodeID:deviceID]); XCTAssertNotNil([dataStore getStoredClusterDataForNodeID:deviceID]); + __auto_type * nodesWithStoredData = [controller nodesWithStoredData]; + XCTAssertTrue([nodesWithStoredData containsObject:deviceID]); + XCTAssertEqualObjects(nodesWithStoredData, [dataStore nodesWithStoredData]); + XCTAssertEqualObjects(nodesWithStoredData, deviceAttributeCounts.allKeys); [controller forgetDeviceWithNodeID:deviceID]; deviceAttributeCounts = [controller unitTestGetDeviceAttributeCounts]; @@ -1779,6 +1785,9 @@ - (void)test010_TestDataStoreMTRDeviceWithBulkReadWrite XCTAssertNil([dataStore findResumptionInfoByNodeID:deviceID]); XCTAssertNil([dataStore getStoredDeviceDataForNodeID:deviceID]); XCTAssertNil([dataStore getStoredClusterDataForNodeID:deviceID]); + nodesWithStoredData = [controller nodesWithStoredData]; + XCTAssertFalse([nodesWithStoredData containsObject:deviceID]); + XCTAssertEqualObjects(nodesWithStoredData, [dataStore nodesWithStoredData]); [controller shutdown]; XCTAssertFalse([controller isRunning]); diff --git a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestDeclarations.h b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestDeclarations.h index e2a9ae76df964e..3b6c3da0ea65ac 100644 --- a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestDeclarations.h +++ b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestDeclarations.h @@ -36,6 +36,7 @@ NS_ASSUME_NONNULL_BEGIN - (void)clearStoredClusterDataForNodeID:(NSNumber *)nodeID; - (void)clearAllStoredClusterData; - (void)unitTestPruneEmptyStoredClusterDataBranches; +- (void)unitTestRereadNodeIndex; - (NSString *)_endpointIndexKeyForNodeID:(NSNumber *)nodeID; - (NSString *)_clusterIndexKeyForNodeID:(NSNumber *)nodeID endpointID:(NSNumber *)endpointID; - (NSString *)_clusterDataKeyForNodeID:(NSNumber *)nodeID endpointID:(NSNumber *)endpointID clusterID:(NSNumber *)clusterID; @@ -43,6 +44,7 @@ NS_ASSUME_NONNULL_BEGIN - (nullable NSArray *)_fetchClusterIndexForNodeID:(NSNumber *)nodeID endpointID:(NSNumber *)endpointID; - (nullable MTRDeviceClusterData *)_fetchClusterDataForNodeID:(NSNumber *)nodeID endpointID:(NSNumber *)endpointID clusterID:(NSNumber *)clusterID; - (nullable NSDictionary *)getStoredDeviceDataForNodeID:(NSNumber *)nodeID; +- (NSArray *)nodesWithStoredData; @end // Declare internal methods for testing From 95d5de5c56d959255633e40c5f788685f2ad10f2 Mon Sep 17 00:00:00 2001 From: James Swan <122404367+swan-amazon@users.noreply.github.com> Date: Fri, 31 Jan 2025 20:15:21 -0800 Subject: [PATCH 10/12] Fix compilation errors for android-chip-tool and java-matter-commissioner (#37346) * Fix compilation errors for android-chip-tool and java-matter-commissioner - Corrected logging statement syntax in `AndroidLogDownloadFromNode.cpp` - Fixed type casting issue for `jlong jremoteNodeId` - Resolved incorrect pointer dereference causing a misplaced operation in `OnTransferCallback` - Updated type casting for `msg_length` in `CHIPP256KeypairBridge.cpp` to use `jsize` instead of `uint32_t` - Adjusted JNI function call to use `static_cast(msg_length)` These changes ensure compatibility with JNI and address type inconsistencies that were causing build failures. Testing: ```bash export PATH=$PATH:/opt/kotlin-compiler-1.8.0/bin export ANDROID_HOME=/opt/Android/sdk/ export ANDROID_NDK_HOME=/opt/Android/sdk/ndk/23.2.8568313 export JAVA_HOME=/Library/Java/JavaVirtualMachines/amazon-corretto-11.jdk/Contents/Home export JAVA_PATH=/Library/Java/JavaVirtualMachines/amazon-corretto-11.jdk/Contents/Home sed 's/ -XX:MaxPermSize=2048m//' examples/android/CHIPTool/gradle.properties > examples/android/CHIPTool/gradle.properties.new && mv examples/android/CHIPTool/gradle.properties.new examples/android/CHIPTool/gradle.properties source scripts/activate.sh ./scripts/build/build_examples.py --target android-arm64-chip-tool --target darwin-arm64-java-matter-controller build ``` * fix: Cast GetFabricIndex() return value to jint in AndroidLogDownloadFromNode The change adds an explicit static cast to convert the FabricIndex return value to jint type when assigning to jFabricIndex variable, maintaining type safety in the JNI interface layer. --- src/controller/java/AndroidLogDownloadFromNode.cpp | 9 +++++---- src/controller/java/CHIPP256KeypairBridge.cpp | 4 ++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/controller/java/AndroidLogDownloadFromNode.cpp b/src/controller/java/AndroidLogDownloadFromNode.cpp index 785567476fc125..f439cd64d881fe 100644 --- a/src/controller/java/AndroidLogDownloadFromNode.cpp +++ b/src/controller/java/AndroidLogDownloadFromNode.cpp @@ -152,7 +152,7 @@ void AndroidLogDownloadFromNode::OnResponseRetrieveLogs(void * context, using namespace chip::app::Clusters::DiagnosticLogs; if (data.status == StatusEnum::kSuccess) { - ChipLogProgress(Controller, "Success. Will receive log from BDX protocol.") + ChipLogProgress(Controller, "Success. Will receive log from BDX protocol."); } else if (data.status == StatusEnum::kExhausted) { @@ -210,8 +210,8 @@ void AndroidLogDownloadFromNode::FinishLogDownloadFromNode(void * context, CHIP_ JniLocalReferenceScope scope(env); jobject jCallback = self->mJavaCallback.ObjectRef(); - jint jFabricIndex = self->mController->GetFabricIndex(); - jlong jremoteNodeId = self->mRemoteNodeId; + jint jFabricIndex = static_cast(self->mController->GetFabricIndex()); + jlong jremoteNodeId = static_cast(self->mRemoteNodeId); VerifyOrExit(env != nullptr, ChipLogError(Controller, "Could not get JNIEnv for current thread")); @@ -274,7 +274,8 @@ void AndroidLogDownloadFromNode::OnTransferCallback(FabricIndex fabricIndex, Nod if (ret != JNI_TRUE) { - ChipLogError(Controller, "Transfer will be rejected.") * errInfoOnFailure = CHIP_ERROR_INTERNAL; + ChipLogError(Controller, "Transfer will be rejected."); + *errInfoOnFailure = CHIP_ERROR_INTERNAL; } } diff --git a/src/controller/java/CHIPP256KeypairBridge.cpp b/src/controller/java/CHIPP256KeypairBridge.cpp index a73eb417203c10..a42c5bc50e87a0 100644 --- a/src/controller/java/CHIPP256KeypairBridge.cpp +++ b/src/controller/java/CHIPP256KeypairBridge.cpp @@ -108,14 +108,14 @@ CHIP_ERROR CHIPP256KeypairBridge::ECDSA_sign_msg(const uint8_t * msg, size_t msg return CHIP_ERROR_INCORRECT_STATE; } - VerifyOrReturnError(CanCastTo(msg_length), CHIP_ERROR_INVALID_ARGUMENT); + VerifyOrReturnError(CanCastTo(msg_length), CHIP_ERROR_INVALID_ARGUMENT); CHIP_ERROR err = CHIP_NO_ERROR; jbyteArray jniMsg; jobject signedResult = nullptr; JNIEnv * env = JniReferences::GetInstance().GetEnvForCurrentThread(); VerifyOrReturnError(env != nullptr, err = CHIP_JNI_ERROR_NO_ENV); - err = JniReferences::GetInstance().N2J_ByteArray(env, msg, static_cast(msg_length), jniMsg); + err = JniReferences::GetInstance().N2J_ByteArray(env, msg, static_cast(msg_length), jniMsg); VerifyOrReturnError(err == CHIP_NO_ERROR, err); VerifyOrReturnError(jniMsg != nullptr, err); VerifyOrReturnError(mDelegate.HasValidObjectRef(), CHIP_ERROR_INCORRECT_STATE); From 11a4c4a1f869494cb8569c7bd450a8319c6b0beb Mon Sep 17 00:00:00 2001 From: yunhanw-google Date: Sun, 2 Feb 2025 18:11:34 -0800 Subject: [PATCH 11/12] Fix android docker image (#37348) --- integrations/docker/images/base/chip-build/version | 2 +- .../docker/images/stage-2/chip-build-java/Dockerfile | 4 ++-- .../docker/images/stage-3/chip-build-android/Dockerfile | 9 +++++++-- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/integrations/docker/images/base/chip-build/version b/integrations/docker/images/base/chip-build/version index aded04f44ab3e9..2a9e09092c18c5 100644 --- a/integrations/docker/images/base/chip-build/version +++ b/integrations/docker/images/base/chip-build/version @@ -1 +1 @@ -105 : Upgrade android docker with new kotlin/gradle +106 : Upgrade android docker with java 17 and adjust the location for android cmdline tool diff --git a/integrations/docker/images/stage-2/chip-build-java/Dockerfile b/integrations/docker/images/stage-2/chip-build-java/Dockerfile index bda9cb3e9f3c0a..1619a9d12b8e93 100644 --- a/integrations/docker/images/stage-2/chip-build-java/Dockerfile +++ b/integrations/docker/images/stage-2/chip-build-java/Dockerfile @@ -6,7 +6,7 @@ LABEL org.opencontainers.image.source https://github.com/project-chip/connectedh RUN set -x \ && apt-get update \ && DEBIAN_FRONTEND=noninteractive apt-get install -fy \ - openjdk-11-jdk \ + openjdk-17-jdk \ && rm -rf /var/lib/apt/lists/ \ && : # last line @@ -20,4 +20,4 @@ RUN set -x \ && : # last line ENV PATH $PATH:/usr/lib/kotlinc/bin -ENV JAVA_PATH=/usr/lib/jvm/java-11-openjdk-amd64 +ENV JAVA_PATH=/usr/lib/jvm/java-17-openjdk-amd64 diff --git a/integrations/docker/images/stage-3/chip-build-android/Dockerfile b/integrations/docker/images/stage-3/chip-build-android/Dockerfile index ea60b3db45074e..3f3d8d40f1c396 100644 --- a/integrations/docker/images/stage-3/chip-build-android/Dockerfile +++ b/integrations/docker/images/stage-3/chip-build-android/Dockerfile @@ -27,11 +27,16 @@ RUN set -x \ && : # last line # Download and install android command line tool (for installing `sdkmanager`) +# We need create latest folder inide cmdline-tools, since latest android commandline tool looks for this latest folder +# when running sdkmanager --licenses RUN set -x \ && wget -O /tmp/cmdline-tools.zip https://dl.google.com/android/repository/commandlinetools-linux-11076708_latest.zip \ && cd /opt/android/sdk \ - && unzip /tmp/cmdline-tools.zip \ - && rm -f /tmp/cmdline-tools.zip \ + && mkdir -p temp \ + && unzip /tmp/cmdline-tools.zip -d temp \ + && mkdir -p cmdline-tools/latest \ + && cp -rf temp/cmdline-tools/* cmdline-tools/latest \ + && rm -rf temp \ && test -d /opt/android/sdk/cmdline-tools \ && : # last line From dd0ce899dfb6e247d303ec0cfe6cad6965fc056d Mon Sep 17 00:00:00 2001 From: Shubham Patil Date: Mon, 3 Feb 2025 13:35:03 +0530 Subject: [PATCH 12/12] [ESP32] update document for app_main outside of main component (#37331) * [ESP32] update document for app_main outside of main component Earlier, we needed to fetch and link the components separately. In #36883 we started using `add_prebuilt_library()` so there is no need to explicitly specify the executable component. * Restyled by prettier-markdown --------- Co-authored-by: Restyled.io --- docs/platforms/esp32/config_options.md | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/docs/platforms/esp32/config_options.md b/docs/platforms/esp32/config_options.md index 6dc94b814b8335..8055efb885c142 100644 --- a/docs/platforms/esp32/config_options.md +++ b/docs/platforms/esp32/config_options.md @@ -15,14 +15,12 @@ CONFIG_LWIP_IPV4=n ### Executable component not in "main" component The ESP-IDF framework allows renaming the main component, which can be useful if -you want to place the app_main() function in a different component. +you want to place the `app_main()` function in a different component. For required changes in the executable component, please refer to the [esp-idf documentation](https://docs.espressif.com/projects/esp-idf/en/stable/esp32/api-guides/build-system.html#renaming-main-component). -If you're building applications that support Matter and want to place app_main() -in a component other than main, use the following command: - -``` -idf.py -DEXECUTABLE_COMPONENT_NAME="your_component" build -``` +You need to list the required components in `idf_component_register()`. If this +module contains Matter related code, you may need to include +`chip, app_update, spi_flash, and nvs_flash` as `PRIV_REQUIRES`, along with any +other necessary dependencies.