From f5216e8ec34cabbfd1c0f450871d52ae574c1357 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Tue, 10 Sep 2024 13:30:56 -0400 Subject: [PATCH 1/3] Clean up subscriptions from fabric sync tests (#35485) --- src/python_testing/TC_BRBINFO_4_1.py | 9 +++++++-- src/python_testing/TC_MCORE_FS_1_2.py | 11 ++++++++--- src/python_testing/TC_MCORE_FS_1_5.py | 20 +++++++++++++++----- 3 files changed, 30 insertions(+), 10 deletions(-) diff --git a/src/python_testing/TC_BRBINFO_4_1.py b/src/python_testing/TC_BRBINFO_4_1.py index df922748999dd0..8b064a775f358b 100644 --- a/src/python_testing/TC_BRBINFO_4_1.py +++ b/src/python_testing/TC_BRBINFO_4_1.py @@ -123,6 +123,7 @@ async def setup_class(self): self.set_of_dut_endpoints_before_adding_device = set(root_part_list) super().setup_class() + self._active_change_event_subscription = None self.app_process = None self.app_process_paused = False app = self.user_params.get("th_icd_server_app_path", None) @@ -156,6 +157,10 @@ async def setup_class(self): params.commissioningParameters.setupManualCode, params.commissioningParameters.setupQRCode) def teardown_class(self): + if self._active_change_event_subscription is not None: + self._active_change_event_subscription.Shutdown() + self._active_change_event_subscription = None + # In case the th_icd_server_app_path does not exist, then we failed the test # and there is nothing to remove if self.app_process is not None: @@ -239,8 +244,8 @@ async def test_TC_BRBINFO_4_1(self): self.q = queue.Queue() urgent = 1 cb = SimpleEventCallback("ActiveChanged", event.cluster_id, event.event_id, self.q) - subscription = await self.default_controller.ReadEvent(nodeid=self.dut_node_id, events=[(dynamic_endpoint_id, event, urgent)], reportInterval=[1, 3]) - subscription.SetEventUpdateCallback(callback=cb) + self._active_change_event_subscription = await self.default_controller.ReadEvent(nodeid=self.dut_node_id, events=[(dynamic_endpoint_id, event, urgent)], reportInterval=[1, 3]) + self._active_change_event_subscription.SetEventUpdateCallback(callback=cb) self.step("3") stay_active_duration_ms = 1000 diff --git a/src/python_testing/TC_MCORE_FS_1_2.py b/src/python_testing/TC_MCORE_FS_1_2.py index 816abe5d876b00..b18dcc5b42ef64 100644 --- a/src/python_testing/TC_MCORE_FS_1_2.py +++ b/src/python_testing/TC_MCORE_FS_1_2.py @@ -63,10 +63,15 @@ class TC_MCORE_FS_1_2(MatterBaseTest): @async_test_body async def setup_class(self): super().setup_class() + self._partslist_subscription = None self._app_th_server_process = None self._th_server_kvs = None def teardown_class(self): + if self._partslist_subscription is not None: + self._partslist_subscription.Shutdown() + self._partslist_subscription = None + if self._app_th_server_process is not None: logging.warning("Stopping app with SIGTERM") self._app_th_server_process.send_signal(signal.SIGTERM.value) @@ -142,7 +147,7 @@ async def test_TC_MCORE_FS_1_2(self): subscription_contents = [ (root_endpoint, Clusters.Descriptor.Attributes.PartsList) ] - sub = await self.default_controller.ReadAttribute( + self._partslist_subscription = await self.default_controller.ReadAttribute( nodeid=self.dut_node_id, attributes=subscription_contents, reportInterval=(min_report_interval_sec, max_report_interval_sec), @@ -152,8 +157,8 @@ async def test_TC_MCORE_FS_1_2(self): parts_list_queue = queue.Queue() attribute_handler = AttributeChangeAccumulator( name=self.default_controller.name, expected_attribute=Clusters.Descriptor.Attributes.PartsList, output=parts_list_queue) - sub.SetAttributeUpdateCallback(attribute_handler) - cached_attributes = sub.GetAttributes() + self._partslist_subscription.SetAttributeUpdateCallback(attribute_handler) + cached_attributes = self._partslist_subscription.GetAttributes() step_1_dut_parts_list = cached_attributes[root_endpoint][Clusters.Descriptor][Clusters.Descriptor.Attributes.PartsList] asserts.assert_true(type_matches(step_1_dut_parts_list, list), "PartsList is expected to be a list") diff --git a/src/python_testing/TC_MCORE_FS_1_5.py b/src/python_testing/TC_MCORE_FS_1_5.py index df80072d5126e3..22654c994bf41f 100755 --- a/src/python_testing/TC_MCORE_FS_1_5.py +++ b/src/python_testing/TC_MCORE_FS_1_5.py @@ -61,10 +61,20 @@ class TC_MCORE_FS_1_5(MatterBaseTest): @async_test_body async def setup_class(self): super().setup_class() + self._partslist_subscription = None + self._cadmin_subscription = None self._app_th_server_process = None self._th_server_kvs = None def teardown_class(self): + if self._partslist_subscription is not None: + self._partslist_subscription.Shutdown() + self._partslist_subscription = None + + if self._cadmin_subscription is not None: + self._cadmin_subscription.Shutdown() + self._cadmin_subscription = None + if self._app_th_server_process is not None: logging.warning("Stopping app with SIGTERM") self._app_th_server_process.send_signal(signal.SIGTERM.value) @@ -142,7 +152,7 @@ async def test_TC_MCORE_FS_1_5(self): parts_list_subscription_contents = [ (root_endpoint, Clusters.Descriptor.Attributes.PartsList) ] - parts_list_sub = await self.default_controller.ReadAttribute( + self._partslist_subscription = await self.default_controller.ReadAttribute( nodeid=self.dut_node_id, attributes=parts_list_subscription_contents, reportInterval=(min_report_interval_sec, max_report_interval_sec), @@ -152,8 +162,8 @@ async def test_TC_MCORE_FS_1_5(self): parts_list_queue = queue.Queue() parts_list_attribute_handler = AttributeChangeAccumulator( name=self.default_controller.name, expected_attribute=Clusters.Descriptor.Attributes.PartsList, output=parts_list_queue) - parts_list_sub.SetAttributeUpdateCallback(parts_list_attribute_handler) - parts_list_cached_attributes = parts_list_sub.GetAttributes() + self._partslist_subscription.SetAttributeUpdateCallback(parts_list_attribute_handler) + parts_list_cached_attributes = self._partslist_subscription.GetAttributes() step_1_dut_parts_list = parts_list_cached_attributes[root_endpoint][Clusters.Descriptor][Clusters.Descriptor.Attributes.PartsList] asserts.assert_true(type_matches(step_1_dut_parts_list, list), "PartsList is expected to be a list") @@ -219,7 +229,7 @@ async def test_TC_MCORE_FS_1_5(self): cadmin_subscription_contents = [ (newly_added_endpoint, Clusters.AdministratorCommissioning) ] - cadmin_sub = await self.default_controller.ReadAttribute( + self._cadmin_subscription = await self.default_controller.ReadAttribute( nodeid=self.dut_node_id, attributes=cadmin_subscription_contents, reportInterval=(min_report_interval_sec, max_report_interval_sec), @@ -230,7 +240,7 @@ async def test_TC_MCORE_FS_1_5(self): # This AttributeChangeAccumulator is really just to let us know when new subscription came in cadmin_attribute_handler = AttributeChangeAccumulator( name=self.default_controller.name, expected_attribute=Clusters.AdministratorCommissioning.Attributes.WindowStatus, output=cadmin_queue) - cadmin_sub.SetAttributeUpdateCallback(cadmin_attribute_handler) + self._cadmin_subscription.SetAttributeUpdateCallback(cadmin_attribute_handler) time.sleep(1) self.step(7) From cbd15a16f5feadec2bdf02b63d2e65659cb3e822 Mon Sep 17 00:00:00 2001 From: Petru Lauric <81822411+plauric@users.noreply.github.com> Date: Tue, 10 Sep 2024 13:55:24 -0400 Subject: [PATCH 2/3] initial commit (#35513) --- src/python_testing/TC_SEAR_1_2.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/python_testing/TC_SEAR_1_2.py b/src/python_testing/TC_SEAR_1_2.py index 2c253efa860038..1bed2c8d12cac8 100644 --- a/src/python_testing/TC_SEAR_1_2.py +++ b/src/python_testing/TC_SEAR_1_2.py @@ -180,12 +180,12 @@ async def read_and_validate_progress(self, step): progareaid_list.append(p.areaID) asserts.assert_true(p.areaID in self.areaid_list, f"Progress entry has invalid AreaID value ({p.areaID})") - asserts.assert_true(p.status in (Clusters.ServiceArea.OperationalStatusEnum.kPending, - Clusters.ServiceArea.OperationalStatusEnum.kOperating, - Clusters.ServiceArea.OperationalStatusEnum.kSkipped, - Clusters.ServiceArea.OperationalStatusEnum.kCompleted), + asserts.assert_true(p.status in (Clusters.ServiceArea.Enums.OperationalStatusEnum.kPending, + Clusters.ServiceArea.Enums.OperationalStatusEnum.kOperating, + Clusters.ServiceArea.Enums.OperationalStatusEnum.kSkipped, + Clusters.ServiceArea.Enums.OperationalStatusEnum.kCompleted), f"Progress entry has invalid Status value ({p.status})") - if p.status not in (Clusters.ServiceArea.OperationalStatusEnum.kSkipped, Clusters.ServiceArea.OperationalStatusEnum.kCompleted): + if p.status not in (Clusters.ServiceArea.Enums.OperationalStatusEnum.kSkipped, Clusters.ServiceArea.Enums.OperationalStatusEnum.kCompleted): asserts.assert_true(p.totalOperationalTime is NullValue, f"Progress entry should have a null TotalOperationalTime value (Status is {p.status})") # TODO how to check that InitialTimeEstimate is either null or uint32? From d1279a83eacd21f5d084ca1883c852e98ff89dad Mon Sep 17 00:00:00 2001 From: Karsten Sperling <113487422+ksperling-apple@users.noreply.github.com> Date: Wed, 11 Sep 2024 06:43:17 +1200 Subject: [PATCH 3/3] Add matter_enable_recommended meta-setting (#34942) * Add matter_enable_recommended meta-setting This setting defaults to true which leaves current behavior unchanged. However it can be set to false to achieve a more conservative / minimal set of defaults, without having to manually disable an ever-increasing set of features. Consider using this setting as the default value for other settings that increase code size or add debugging / tracing or similar features that are not always desired. Especially settings that default to true on Linux and Mac ("because you're probably building for development") should likely use this option for their default, e.g. matter_foo = matter_enable_recommended && (current_os == "linux" || current_os == "mac") * Add more documentation and turn off the setting for minimal CI builds * Move matter_enable_recommended into /config/recommended.gni This allows it to be uesd from args.gni (i.e. in the context of a default_args scope, where variables like current_os are not defined) as well as within a build / toolchain context. --- .github/workflows/minimal-build.yaml | 10 ++++----- config/recommended.gni | 31 ++++++++++++++++++++++++++++ examples/shell/shell_common/BUILD.gn | 7 +++---- src/app/common_flags.gni | 5 ++++- src/lib/core/core.gni | 15 ++++++++++---- src/tracing/tracing_args.gni | 6 +++--- 6 files changed, 57 insertions(+), 17 deletions(-) create mode 100644 config/recommended.gni diff --git a/.github/workflows/minimal-build.yaml b/.github/workflows/minimal-build.yaml index 9ba9565b44c25b..ade1bd56ea36ff 100644 --- a/.github/workflows/minimal-build.yaml +++ b/.github/workflows/minimal-build.yaml @@ -17,7 +17,7 @@ name: Minimal Build (Linux / configure) on: push: branches-ignore: - - 'dependabot/**' + - "dependabot/**" pull_request: merge_group: @@ -42,11 +42,11 @@ jobs: - name: Checkout submodules # but don't bootstrap! uses: ./.github/actions/checkout-submodules with: - platform: linux + platform: linux - name: Configure and build All Clusters App run: | - CC=gcc CXX=g++ scripts/configure --project=examples/all-clusters-app/linux && ./ninja-build + CC=gcc CXX=g++ scripts/configure --project=examples/all-clusters-app/linux --enable-recommended=no && ./ninja-build minimal-network-manager: name: Linux / configure build of network-manager-app @@ -64,8 +64,8 @@ jobs: - name: Checkout submodules # but don't bootstrap! uses: ./.github/actions/checkout-submodules with: - platform: linux + platform: linux - name: Configure and build Network Manager App run: | - CC=gcc CXX=g++ scripts/configure --project=examples/network-manager-app/linux && ./ninja-build + CC=gcc CXX=g++ scripts/configure --project=examples/network-manager-app/linux --enable-recommended=no && ./ninja-build diff --git a/config/recommended.gni b/config/recommended.gni new file mode 100644 index 00000000000000..a30045ced7814e --- /dev/null +++ b/config/recommended.gni @@ -0,0 +1,31 @@ +# Copyright (c) 2024 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. + +declare_args() { + # Note for SDK developers: As additional features with their own settings + # are added to the SDK, consider using the `matter_enable_recommended` + # meta-setting instead of a default value of 'true', especially where a + # different default is used based on platform (current_os): Often various + # debugging features have previously been defaulted to on for Linux and/or + # Mac but off for embedded platforms (on the assumption that Linux / Mac + # don't have resource constraints?); build settings of that nature should + # instead reference this meta-setting. E.g. + # enable_flux_capacitor = matter_enable_recommended && current_os == "linux" + + # Enable recommended settings by default. This is a meta-setting + # that is enabled by default, and acts as a default for various + # other settings. Setting it to false produces a more conservative / + # minimal set of defaults. + matter_enable_recommended = true +} diff --git a/examples/shell/shell_common/BUILD.gn b/examples/shell/shell_common/BUILD.gn index e4aa2973f8cc04..645c8ddb510220 100644 --- a/examples/shell/shell_common/BUILD.gn +++ b/examples/shell/shell_common/BUILD.gn @@ -13,13 +13,12 @@ # limitations under the License. import("//build_overrides/chip.gni") -import("//build_overrides/openthread.gni") - +import("${chip_root}/config/recommended.gni") import("${chip_root}/src/platform/device.gni") declare_args() { - # Enable server command only on linux for now. - chip_shell_cmd_server = current_os == "linux" || current_os == "mac" + chip_shell_cmd_server = matter_enable_recommended && + (current_os == "linux" || current_os == "mac") } config("shell_common_config") { diff --git a/src/app/common_flags.gni b/src/app/common_flags.gni index 953afc23a20650..d3e7ce34bf0338 100644 --- a/src/app/common_flags.gni +++ b/src/app/common_flags.gni @@ -12,6 +12,9 @@ # See the License for the specific language governing permissions and # limitations under the License. +import("//build_overrides/chip.gni") +import("${chip_root}/config/recommended.gni") + declare_args() { # Temporary flag for interaction model and echo protocols, set it to true to enable chip_app_use_echo = false @@ -26,7 +29,7 @@ declare_args() { # - disabled: does not use data model interface at all # - check: runs BOTH datamodel and non-data-model (if possible) functionality and compares results # - enabled: runs only the data model interface (does not use the legacy code) - if (current_os == "linux") { + if (matter_enable_recommended && current_os == "linux") { chip_use_data_model_interface = "check" } else { chip_use_data_model_interface = "disabled" diff --git a/src/lib/core/core.gni b/src/lib/core/core.gni index f2189198e36131..eea73a5c56f7b3 100644 --- a/src/lib/core/core.gni +++ b/src/lib/core/core.gni @@ -12,6 +12,9 @@ # See the License for the specific language governing permissions and # limitations under the License. +import("//build_overrides/chip.gni") +import("${chip_root}/config/recommended.gni") + declare_args() { # Enable logging. Shorthand for chip_error_logging, etc. chip_logging = true @@ -28,13 +31,16 @@ declare_args() { chip_progress_logging = chip_logging # Enable detail logging. - chip_detail_logging = chip_logging + chip_detail_logging = matter_enable_recommended && chip_logging # Enable automation logging. - chip_automation_logging = chip_logging + chip_automation_logging = matter_enable_recommended && chip_logging +} +declare_args() { # Configure the maximum size for logged messages - if (current_os == "linux" || current_os == "mac" || current_os == "ios") { + if ((matter_enable_recommended || chip_detail_logging) && + (current_os == "linux" || current_os == "mac" || current_os == "ios")) { # Allow base64 encoding of 1 MTU (4 * (1280 / 3) + padding chip_log_message_max_size = 1708 } else { @@ -88,7 +94,8 @@ declare_args() { chip_config_memory_debug_dmalloc = false # When enabled trace messages using tansport trace hook. - chip_enable_transport_trace = current_os == "linux" || current_os == "mac" + chip_enable_transport_trace = matter_enable_recommended && + (current_os == "linux" || current_os == "mac") # When this is enabled trace messages will be sent to pw_trace. chip_enable_transport_pw_trace = false diff --git a/src/tracing/tracing_args.gni b/src/tracing/tracing_args.gni index d6ddb1fd2e99b8..085844e4a53605 100644 --- a/src/tracing/tracing_args.gni +++ b/src/tracing/tracing_args.gni @@ -11,9 +11,8 @@ # 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. -import("//build_overrides/build.gni") import("//build_overrides/chip.gni") -import("${build_root}/config/compiler/compiler.gni") +import("${chip_root}/config/recommended.gni") import("${chip_root}/src/platform/device.gni") declare_args() { @@ -24,7 +23,8 @@ declare_args() { # Additionally, if tracing is enabled, the main() function has to add # backends explicitly matter_enable_tracing_support = - current_os == "android" || chip_device_platform == "darwin" + matter_enable_recommended && + (current_os == "android" || chip_device_platform == "darwin") # Defines the trace backend. Current matter tracing splits the logic # into two parts: