From bd6ae0980fde3d20a49385caa6d64adb6aee3276 Mon Sep 17 00:00:00 2001 From: Junior Martinez <67972863+jmartinez-silabs@users.noreply.github.com> Date: Tue, 10 Sep 2024 15:01:55 -0400 Subject: [PATCH 01/10] [Silabs] Increase the max supported KVS entries (#35480) * Increase the maximum KVS entries our implementation can handle * add gn argument to configure the nvm3 max object size --- src/platform/silabs/KeyValueStoreManagerImpl.cpp | 10 +++++----- src/platform/silabs/SilabsConfig.h | 16 +++++++++++----- third_party/silabs/SiWx917_sdk.gni | 2 +- third_party/silabs/efr32_sdk.gni | 3 ++- 4 files changed, 19 insertions(+), 12 deletions(-) diff --git a/src/platform/silabs/KeyValueStoreManagerImpl.cpp b/src/platform/silabs/KeyValueStoreManagerImpl.cpp index 012c951aaaa729..34d88388badf22 100644 --- a/src/platform/silabs/KeyValueStoreManagerImpl.cpp +++ b/src/platform/silabs/KeyValueStoreManagerImpl.cpp @@ -88,9 +88,9 @@ uint16_t KeyValueStoreManagerImpl::hashKvsKeyString(const char * key) const CHIP_ERROR KeyValueStoreManagerImpl::MapKvsKeyToNvm3(const char * key, uint16_t hash, uint32_t & nvm3Key, bool isSlotNeeded) const { CHIP_ERROR err; - char * strPrefix = nullptr; - uint8_t firstEmptyKeySlot = kMaxEntries; - for (uint8_t keyIndex = 0; keyIndex < kMaxEntries; keyIndex++) + char * strPrefix = nullptr; + uint16_t firstEmptyKeySlot = kMaxEntries; + for (uint16_t keyIndex = 0; keyIndex < kMaxEntries; keyIndex++) { if (mKvsKeyMap[keyIndex] == hash) { @@ -165,7 +165,7 @@ void KeyValueStoreManagerImpl::ScheduleKeyMapSave(void) Commit the key map in nvm once it as stabilized. */ SystemLayer().StartTimer( - std::chrono::duration_cast(System::Clock::Seconds32(SILABS_KVS_SAVE_DELAY_SECONDS)), + std::chrono::duration_cast(System::Clock::Seconds32(SL_KVS_SAVE_DELAY_SECONDS)), KeyValueStoreManagerImpl::OnScheduledKeyMapSave, NULL); } @@ -247,7 +247,7 @@ CHIP_ERROR KeyValueStoreManagerImpl::_Delete(const char * key) void KeyValueStoreManagerImpl::ErasePartition(void) { // Iterate over all the Matter Kvs nvm3 records and delete each one... - for (uint32_t nvm3Key = SilabsConfig::kMinConfigKey_MatterKvs; nvm3Key <= SilabsConfig::kMaxConfigKey_MatterKvs; nvm3Key++) + for (uint32_t nvm3Key = SilabsConfig::kMinConfigKey_MatterKvs; nvm3Key <= SilabsConfig::kConfigKey_KvsLastKeySlot; nvm3Key++) { SilabsConfig::ClearConfigValue(nvm3Key); } diff --git a/src/platform/silabs/SilabsConfig.h b/src/platform/silabs/SilabsConfig.h index ccb14beeb8f177..cef9ca323f74d2 100644 --- a/src/platform/silabs/SilabsConfig.h +++ b/src/platform/silabs/SilabsConfig.h @@ -35,9 +35,11 @@ #endif // Delay before Key/Value is actually saved in NVM -#define SILABS_KVS_SAVE_DELAY_SECONDS 5 +#ifndef SL_KVS_SAVE_DELAY_SECONDS +#define SL_KVS_SAVE_DELAY_SECONDS 2 +#endif -static_assert((KVS_MAX_ENTRIES <= 255), "Implementation supports up to 255 Kvs entries"); +static_assert((KVS_MAX_ENTRIES <= 511), "Implementation supports up to 511 Kvs entries"); static_assert((KVS_MAX_ENTRIES >= 30), "Mininimal Kvs entries requirement is not met"); namespace chip { @@ -89,7 +91,8 @@ class SilabsConfig // Persistent counter values set at runtime. Retained during factory reset. static constexpr uint8_t kMatterCounter_KeyBase = 0x74; // Persistent config values set at runtime. Cleared during factory reset. - static constexpr uint8_t kMatterKvs_KeyBase = 0x75; + static constexpr uint8_t kMatterKvs_KeyBase = 0x75; + static constexpr uint8_t kMatterKvs_ExtendedRange = 0x76; // Key definitions for well-known configuration values. // Factory config keys @@ -167,7 +170,8 @@ class SilabsConfig // Matter KVS storage Keys static constexpr Key kConfigKey_KvsStringKeyMap = SilabsConfigKey(kMatterKvs_KeyBase, 0x00); static constexpr Key kConfigKey_KvsFirstKeySlot = SilabsConfigKey(kMatterKvs_KeyBase, 0x01); - static constexpr Key kConfigKey_KvsLastKeySlot = SilabsConfigKey(kMatterKvs_KeyBase, KVS_MAX_ENTRIES); + static constexpr Key kConfigKey_KvsLastKeySlot = + SilabsConfigKey(kMatterKvs_KeyBase + (KVS_MAX_ENTRIES >> 8), KVS_MAX_ENTRIES & UINT8_MAX); // Set key id limits for each group. static constexpr Key kMinConfigKey_MatterFactory = SilabsConfigKey(kMatterFactory_KeyBase, 0x00); @@ -180,7 +184,9 @@ class SilabsConfig static constexpr Key kMaxConfigKey_MatterCounter = SilabsConfigKey(kMatterCounter_KeyBase, 0x1F); static constexpr Key kMinConfigKey_MatterKvs = kConfigKey_KvsStringKeyMap; - static constexpr Key kMaxConfigKey_MatterKvs = kConfigKey_KvsLastKeySlot; + static constexpr Key kMaxConfigKey_MatterKvs = SilabsConfigKey(kMatterKvs_ExtendedRange, 0xFF); + static_assert(kConfigKey_KvsLastKeySlot <= kMaxConfigKey_MatterKvs, + "Configured KVS_MAX_ENTRIES overflows the reserved KVS Key range"); static CHIP_ERROR Init(void); static void DeInit(void); diff --git a/third_party/silabs/SiWx917_sdk.gni b/third_party/silabs/SiWx917_sdk.gni index 30e1595493c60c..e91afe7483b3fa 100644 --- a/third_party/silabs/SiWx917_sdk.gni +++ b/third_party/silabs/SiWx917_sdk.gni @@ -187,7 +187,7 @@ template("siwx917_sdk") { # 1->SOC and 0->NCP "RSI_WLAN_API_ENABLE", "NVM3_DEFAULT_NVM_SIZE=40960", - "NVM3_DEFAULT_MAX_OBJECT_SIZE=4092", + "NVM3_DEFAULT_MAX_OBJECT_SIZE=${sl_nvm3_max_object_size}", "KVS_MAX_ENTRIES=${kvs_max_entries}", "${silabs_mcu}=1", "${silabs_board}=1", diff --git a/third_party/silabs/efr32_sdk.gni b/third_party/silabs/efr32_sdk.gni index 1cd69d31e7084b..3f30a0ca52f867 100644 --- a/third_party/silabs/efr32_sdk.gni +++ b/third_party/silabs/efr32_sdk.gni @@ -38,6 +38,7 @@ declare_args() { # enable by default for thread/non-wifi-ncp builds enable_openthread_cli = !(use_rs9116 || use_wf200 || use_SiWx917) kvs_max_entries = 255 + sl_nvm3_max_object_size = 4092 # Use Silabs factory data provider example. # Users can implement their own. @@ -421,7 +422,7 @@ template("efr32_sdk") { "HARD_FAULT_LOG_ENABLE", "CORTEXM3_EFM32_MICRO", "NVM3_DEFAULT_NVM_SIZE=40960", - "NVM3_DEFAULT_MAX_OBJECT_SIZE=4092", + "NVM3_DEFAULT_MAX_OBJECT_SIZE=${sl_nvm3_max_object_size}", "KVS_MAX_ENTRIES=${kvs_max_entries}", "CORTEXM3=1", "MICRO=EMBER_MICRO_CORTEXM3_EFR32", From 2b7965d9e3fc9423f9ad6530b610bb994b6c63e5 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Tue, 10 Sep 2024 15:22:11 -0400 Subject: [PATCH 02/10] Change TC_CCTRL_2_3.py to no longer use per_endpoint_test (#35516) --- src/python_testing/TC_CCTRL_2_3.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/python_testing/TC_CCTRL_2_3.py b/src/python_testing/TC_CCTRL_2_3.py index 224f283f46d27b..6fa5e7038f88ef 100644 --- a/src/python_testing/TC_CCTRL_2_3.py +++ b/src/python_testing/TC_CCTRL_2_3.py @@ -34,7 +34,7 @@ from chip import ChipDeviceCtrl from chip.interaction_model import InteractionModelError, Status from matter_testing_support import (MatterBaseTest, TestStep, async_test_body, default_matter_test_main, has_cluster, - per_endpoint_test) + run_if_endpoint_matches) from mobly import asserts @@ -109,7 +109,7 @@ def steps_TC_CCTRL_2_3(self) -> list[TestStep]: def default_timeout(self) -> int: return 3*60 - @per_endpoint_test(has_cluster(Clusters.CommissionerControl)) + @run_if_endpoint_matches(has_cluster(Clusters.CommissionerControl)) async def test_TC_CCTRL_2_3(self): self.is_ci = self.check_pics('PICS_SDK_CI_ONLY') From dc397c2a0cd29532a2763b655ff73a81b32deed6 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Tue, 10 Sep 2024 15:49:51 -0400 Subject: [PATCH 03/10] Fix fabric sync tests after name update (#35518) --- src/python_testing/TC_CCTRL_2_2.py | 26 +++++++++---------- src/python_testing/TC_CCTRL_2_3.py | 12 ++++----- src/python_testing/TC_MCORE_FS_1_1.py | 8 +++--- src/python_testing/TC_MCORE_FS_1_3.py | 8 +++--- .../test_testing/test_TC_CCNTL_2_2.py | 4 +-- .../test_testing/test_TC_MCORE_FS_1_1.py | 2 +- 6 files changed, 30 insertions(+), 30 deletions(-) diff --git a/src/python_testing/TC_CCTRL_2_2.py b/src/python_testing/TC_CCTRL_2_2.py index 8e7a8f1fde6ac3..87849e40443d01 100644 --- a/src/python_testing/TC_CCTRL_2_2.py +++ b/src/python_testing/TC_CCTRL_2_2.py @@ -141,7 +141,7 @@ async def test_TC_CCTRL_2_2(self): events = await self.default_controller.ReadEvent(nodeid=self.dut_node_id, events=event_path) self.step(5) - cmd = Clusters.CommissionerControl.Commands.CommissionNode(requestId=1, responseTimeoutSeconds=30) + cmd = Clusters.CommissionerControl.Commands.CommissionNode(requestID=1, responseTimeoutSeconds=30) try: await self.send_single_cmd(cmd) asserts.fail("Unexpected success on CommissionNode") @@ -162,7 +162,7 @@ async def test_TC_CCTRL_2_2(self): self.step(8) good_request_id = 0x1234567887654321 cmd = Clusters.CommissionerControl.Commands.RequestCommissioningApproval( - requestId=good_request_id, vendorId=th_server_vid, productId=th_server_pid) + requestID=good_request_id, vendorID=th_server_vid, productID=th_server_pid) try: await self.send_single_cmd(cmd=cmd, node_id=pase_nodeid) asserts.fail("Unexpected success on RequestCommissioningApproval over PASE") @@ -187,7 +187,7 @@ async def test_TC_CCTRL_2_2(self): not_th_server_vid = 0x6006 asserts.assert_not_equal(not_th_server_vid, th_server_vid, "Test implementation assumption incorrect") cmd = Clusters.CommissionerControl.Commands.RequestCommissioningApproval( - requestId=good_request_id, vendorId=not_th_server_vid, productId=th_server_pid) + requestID=good_request_id, vendorID=not_th_server_vid, productID=th_server_pid) # If no exception is raised, this is success await self.send_single_cmd(cmd) @@ -203,13 +203,13 @@ async def test_TC_CCTRL_2_2(self): new_event = await self.default_controller.ReadEvent(nodeid=self.dut_node_id, events=event_path, eventNumberFilter=max(event_nums)+1) asserts.assert_equal(len(new_event), 1, "Unexpected event list len") asserts.assert_equal(new_event[0].Data.statusCode, 0, "Unexpected status code") - asserts.assert_equal(new_event[0].Data.clientNodeId, + asserts.assert_equal(new_event[0].Data.clientNodeID, self.matter_test_config.controller_node_id, "Unexpected client node id") - asserts.assert_equal(new_event[0].Data.requestId, good_request_id, "Unexpected request ID") + asserts.assert_equal(new_event[0].Data.requestID, good_request_id, "Unexpected request ID") self.step(14) bad_request_id = 0x1234567887654322 - cmd = Clusters.CommissionerControl.Commands.CommissionNode(requestId=bad_request_id, responseTimeoutSeconds=30) + cmd = Clusters.CommissionerControl.Commands.CommissionNode(requestID=bad_request_id, responseTimeoutSeconds=30) try: await self.send_single_cmd(cmd=cmd) asserts.fail("Unexpected success on CommissionNode") @@ -217,7 +217,7 @@ async def test_TC_CCTRL_2_2(self): asserts.assert_equal(e.status, Status.Failure, "Incorrect error returned") self.step(15) - cmd = Clusters.CommissionerControl.Commands.CommissionNode(requestId=good_request_id, responseTimeoutSeconds=29) + cmd = Clusters.CommissionerControl.Commands.CommissionNode(requestID=good_request_id, responseTimeoutSeconds=29) try: await self.send_single_cmd(cmd=cmd) asserts.fail("Unexpected success on CommissionNode") @@ -225,7 +225,7 @@ async def test_TC_CCTRL_2_2(self): asserts.assert_equal(e.status, Status.ConstraintError, "Incorrect error returned") self.step(16) - cmd = Clusters.CommissionerControl.Commands.CommissionNode(requestId=good_request_id, responseTimeoutSeconds=121) + cmd = Clusters.CommissionerControl.Commands.CommissionNode(requestID=good_request_id, responseTimeoutSeconds=121) try: await self.send_single_cmd(cmd=cmd) asserts.fail("Unexpected success on CommissionNode") @@ -233,7 +233,7 @@ async def test_TC_CCTRL_2_2(self): asserts.assert_equal(e.status, Status.ConstraintError, "Incorrect error returned") self.step(17) - cmd = Clusters.CommissionerControl.Commands.CommissionNode(requestId=good_request_id, responseTimeoutSeconds=30) + cmd = Clusters.CommissionerControl.Commands.CommissionNode(requestID=good_request_id, responseTimeoutSeconds=30) resp: Clusters.CommissionerControl.Commands.ReverseOpenCommissioningWindow = await self.send_single_cmd(cmd) asserts.assert_equal(type(resp), Clusters.CommissionerControl.Commands.ReverseOpenCommissioningWindow, "Incorrect response type") @@ -263,7 +263,7 @@ async def test_TC_CCTRL_2_2(self): self.step(22) good_request_id = 0x1234567812345678 cmd = Clusters.CommissionerControl.Commands.RequestCommissioningApproval( - requestId=good_request_id, vendorId=th_server_vid, productId=th_server_pid, label="Test Ecosystem") + requestID=good_request_id, vendorID=th_server_vid, productID=th_server_pid, label="Test Ecosystem") await self.send_single_cmd(cmd) self.step(23) @@ -276,12 +276,12 @@ async def test_TC_CCTRL_2_2(self): new_event = await self.default_controller.ReadEvent(nodeid=self.dut_node_id, events=event_path, eventNumberFilter=max(event_nums)+1) asserts.assert_equal(len(new_event), 1, "Unexpected event list len") asserts.assert_equal(new_event[0].Data.statusCode, 0, "Unexpected status code") - asserts.assert_equal(new_event[0].Data.clientNodeId, + asserts.assert_equal(new_event[0].Data.clientNodeID, self.matter_test_config.controller_node_id, "Unexpected client node id") - asserts.assert_equal(new_event[0].Data.requestId, good_request_id, "Unexpected request ID") + asserts.assert_equal(new_event[0].Data.requestID, good_request_id, "Unexpected request ID") self.step(25) - cmd = Clusters.CommissionerControl.Commands.CommissionNode(requestId=good_request_id, responseTimeoutSeconds=30) + cmd = Clusters.CommissionerControl.Commands.CommissionNode(requestID=good_request_id, responseTimeoutSeconds=30) resp = await self.send_single_cmd(cmd) asserts.assert_equal(type(resp), Clusters.CommissionerControl.Commands.ReverseOpenCommissioningWindow, "Incorrect response type") diff --git a/src/python_testing/TC_CCTRL_2_3.py b/src/python_testing/TC_CCTRL_2_3.py index 6fa5e7038f88ef..2c1c20e17f309f 100644 --- a/src/python_testing/TC_CCTRL_2_3.py +++ b/src/python_testing/TC_CCTRL_2_3.py @@ -125,7 +125,7 @@ async def test_TC_CCTRL_2_3(self): self.step(4) good_request_id = 0x1234567812345678 cmd = Clusters.CommissionerControl.Commands.RequestCommissioningApproval( - requestId=good_request_id, vendorId=th_server_vid, productId=th_server_pid, label="Test Ecosystem") + requestID=good_request_id, vendorID=th_server_vid, productID=th_server_pid, label="Test Ecosystem") await self.send_single_cmd(cmd=cmd) self.step(5) @@ -137,13 +137,13 @@ async def test_TC_CCTRL_2_3(self): events = await self.default_controller.ReadEvent(nodeid=self.dut_node_id, events=event_path) asserts.assert_equal(len(events), 1, "Unexpected event list len") asserts.assert_equal(events[0].Data.statusCode, 0, "Unexpected status code") - asserts.assert_equal(events[0].Data.clientNodeId, + asserts.assert_equal(events[0].Data.clientNodeID, self.matter_test_config.controller_node_id, "Unexpected client node id") - asserts.assert_equal(events[0].Data.requestId, good_request_id, "Unexpected request ID") + asserts.assert_equal(events[0].Data.requestID, good_request_id, "Unexpected request ID") self.step(7) cmd = Clusters.CommissionerControl.Commands.RequestCommissioningApproval( - requestId=good_request_id, vendorId=th_server_vid, productId=th_server_pid) + requestID=good_request_id, vendorID=th_server_vid, productID=th_server_pid) try: await self.send_single_cmd(cmd=cmd) asserts.fail("Unexpected success on CommissionNode") @@ -151,13 +151,13 @@ async def test_TC_CCTRL_2_3(self): asserts.assert_equal(e.status, Status.Failure, "Incorrect error returned") self.step(8) - cmd = Clusters.CommissionerControl.Commands.CommissionNode(requestId=good_request_id, responseTimeoutSeconds=30) + cmd = Clusters.CommissionerControl.Commands.CommissionNode(requestID=good_request_id, responseTimeoutSeconds=30) resp = await self.send_single_cmd(cmd) asserts.assert_equal(type(resp), Clusters.CommissionerControl.Commands.ReverseOpenCommissioningWindow, "Incorrect response type") self.step(9) - cmd = Clusters.CommissionerControl.Commands.CommissionNode(requestId=good_request_id, responseTimeoutSeconds=30) + cmd = Clusters.CommissionerControl.Commands.CommissionNode(requestID=good_request_id, responseTimeoutSeconds=30) try: await self.send_single_cmd(cmd=cmd) asserts.fail("Unexpected success on CommissionNode") diff --git a/src/python_testing/TC_MCORE_FS_1_1.py b/src/python_testing/TC_MCORE_FS_1_1.py index e56a1fb8246b56..4ea9d362655884 100755 --- a/src/python_testing/TC_MCORE_FS_1_1.py +++ b/src/python_testing/TC_MCORE_FS_1_1.py @@ -111,7 +111,7 @@ async def test_TC_MCORE_FS_1_1(self): self.step("3a") good_request_id = 0x1234567812345678 cmd = Clusters.CommissionerControl.Commands.RequestCommissioningApproval( - requestId=good_request_id, vendorId=th_fsa_server_vid, productId=th_fsa_server_pid, label="Test Ecosystem") + requestID=good_request_id, vendorID=th_fsa_server_vid, productID=th_fsa_server_pid, label="Test Ecosystem") await self.send_single_cmd(cmd, endpoint=dut_commissioning_control_endpoint) if not self.is_ci: @@ -125,12 +125,12 @@ async def test_TC_MCORE_FS_1_1(self): asserts.assert_equal(len(new_event), 1, "Unexpected event list len") asserts.assert_equal(new_event[0].Data.statusCode, 0, "Unexpected status code") - asserts.assert_equal(new_event[0].Data.clientNodeId, + asserts.assert_equal(new_event[0].Data.clientNodeID, self.matter_test_config.controller_node_id, "Unexpected client node id") - asserts.assert_equal(new_event[0].Data.requestId, good_request_id, "Unexpected request ID") + asserts.assert_equal(new_event[0].Data.requestID, good_request_id, "Unexpected request ID") self.step("3b") - cmd = Clusters.CommissionerControl.Commands.CommissionNode(requestId=good_request_id, responseTimeoutSeconds=30) + cmd = Clusters.CommissionerControl.Commands.CommissionNode(requestID=good_request_id, responseTimeoutSeconds=30) resp = await self.send_single_cmd(cmd, endpoint=dut_commissioning_control_endpoint) asserts.assert_equal(type(resp), Clusters.CommissionerControl.Commands.ReverseOpenCommissioningWindow, "Incorrect response type") diff --git a/src/python_testing/TC_MCORE_FS_1_3.py b/src/python_testing/TC_MCORE_FS_1_3.py index 1a18896c055952..2a21c977c9fc8b 100644 --- a/src/python_testing/TC_MCORE_FS_1_3.py +++ b/src/python_testing/TC_MCORE_FS_1_3.py @@ -170,9 +170,9 @@ async def commission_via_commissioner_control(self, controller_node_id: int, dev await self.send_single_cmd( node_id=controller_node_id, cmd=Clusters.CommissionerControl.Commands.RequestCommissioningApproval( - requestId=request_id, - vendorId=vendor_id, - productId=product_id, + requestID=request_id, + vendorID=vendor_id, + productID=product_id, ), ) @@ -182,7 +182,7 @@ async def commission_via_commissioner_control(self, controller_node_id: int, dev resp = await self.send_single_cmd( node_id=controller_node_id, cmd=Clusters.CommissionerControl.Commands.CommissionNode( - requestId=request_id, + requestID=request_id, responseTimeoutSeconds=30, ), ) diff --git a/src/python_testing/test_testing/test_TC_CCNTL_2_2.py b/src/python_testing/test_testing/test_TC_CCNTL_2_2.py index d528b5652e1b45..bd28023b8e32e8 100644 --- a/src/python_testing/test_testing/test_TC_CCNTL_2_2.py +++ b/src/python_testing/test_testing/test_TC_CCNTL_2_2.py @@ -94,14 +94,14 @@ def dynamic_event_return(*args, **argv): header = Attribute.EventHeader(EndpointId=0, ClusterId=Clusters.CommissionerControl.id, EventId=Clusters.CommissionerControl.Events.CommissioningRequestResult.event_id, EventNumber=1) data = Clusters.CommissionerControl.Events.CommissioningRequestResult( - requestId=0x1234567887654321, clientNodeId=112233, statusCode=0) + requestID=0x1234567887654321, clientNodeID=112233, statusCode=0) result = Attribute.EventReadResult(Header=header, Status=Status.Success, Data=data) return [result] elif event_call_count == 4: # returned event with new request header = Attribute.EventHeader(EndpointId=0, ClusterId=Clusters.CommissionerControl.id, EventId=Clusters.CommissionerControl.Events.CommissioningRequestResult.event_id, EventNumber=1) data = Clusters.CommissionerControl.Events.CommissioningRequestResult( - requestId=0x1234567812345678, clientNodeId=112233, statusCode=0) + requestID=0x1234567812345678, clientNodeID=112233, statusCode=0) result = Attribute.EventReadResult(Header=header, Status=Status.Success, Data=data) return [result] else: diff --git a/src/python_testing/test_testing/test_TC_MCORE_FS_1_1.py b/src/python_testing/test_testing/test_TC_MCORE_FS_1_1.py index 5b2e29b71915b1..d79b4125309e11 100644 --- a/src/python_testing/test_testing/test_TC_MCORE_FS_1_1.py +++ b/src/python_testing/test_testing/test_TC_MCORE_FS_1_1.py @@ -72,7 +72,7 @@ def dynamic_event_return(*args, **argv): header = Attribute.EventHeader(EndpointId=0, ClusterId=Clusters.CommissionerControl.id, EventId=Clusters.CommissionerControl.Events.CommissioningRequestResult.event_id, EventNumber=1) data = Clusters.CommissionerControl.Events.CommissioningRequestResult( - requestId=0x1234567812345678, clientNodeId=112233, statusCode=0) + requestID=0x1234567812345678, clientNodeID=112233, statusCode=0) result = Attribute.EventReadResult(Header=header, Status=Status.Success, Data=data) return [result] else: From 3d3784ecd5c93141e51dddd8c1f8e2f095313feb Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 10 Sep 2024 17:01:12 -0400 Subject: [PATCH 04/10] Fix MTRCommissionableBrowserTests when remove/add happen after we found all the devices. (#35515) --- .../CHIPTests/MTRCommissionableBrowserTests.m | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/darwin/Framework/CHIPTests/MTRCommissionableBrowserTests.m b/src/darwin/Framework/CHIPTests/MTRCommissionableBrowserTests.m index 15ef7a839de97a..0acfbd9a580371 100644 --- a/src/darwin/Framework/CHIPTests/MTRCommissionableBrowserTests.m +++ b/src/darwin/Framework/CHIPTests/MTRCommissionableBrowserTests.m @@ -55,6 +55,7 @@ @interface DeviceScannerDelegate : NSObject @property (nonatomic, nullable) XCTestExpectation * expectation; @property (nonatomic) NSMutableArray *> * results; @property (nonatomic) NSMutableArray *> * removedResults; +@property (nonatomic) BOOL expectedResultsCountReached; - (instancetype)initWithExpectation:(XCTestExpectation *)expectation; - (void)controller:(MTRDeviceController *)controller didFindCommissionableDevice:(MTRCommissionableBrowserResult *)device; @@ -71,6 +72,7 @@ - (instancetype)initWithExpectation:(XCTestExpectation *)expectation _expectation = expectation; _results = [[NSMutableArray alloc] init]; _removedResults = [[NSMutableArray alloc] init]; + _expectedResultsCountReached = NO; return self; } @@ -83,6 +85,7 @@ - (instancetype)init _expectation = nil; _results = [[NSMutableArray alloc] init]; _removedResults = [[NSMutableArray alloc] init]; + _expectedResultsCountReached = NO; return self; } @@ -111,7 +114,14 @@ - (void)controller:(MTRDeviceController *)controller didFindCommissionableDevice // Ensure that we just saw the same results as our final set popping in and out if things // ever got removed here. XCTAssertEqualObjects(finalResultsSet, allResultsSet); - [self.expectation fulfill]; + + // If we have a remove and re-add after the result count reached the + // expected one, we can end up in this branch again. Doing the above + // checks is fine, but we shouldn't double-fulfill the expectation. + if (self.expectedResultsCountReached == NO) { + self.expectedResultsCountReached = YES; + [self.expectation fulfill]; + } } XCTAssertLessThanOrEqual(_results.count, kExpectedDiscoveredDevicesCount); From 02316de18c09f246a4ecb1fabf4db46ebbd08a96 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 10 Sep 2024 17:55:17 -0400 Subject: [PATCH 05/10] Document mask argument of DECLARE_DYNAMIC_ATTRIBUTE. (#35228) People keep forgetting ATTRIBUTE_MASK_WRITABLE here, then being confused why writes do not work. --- src/app/util/attribute-storage.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/app/util/attribute-storage.h b/src/app/util/attribute-storage.h index b45bfe43cff43e..23a9b9b5fd20c3 100644 --- a/src/app/util/attribute-storage.h +++ b/src/app/util/attribute-storage.h @@ -60,6 +60,12 @@ static constexpr uint16_t kEmberInvalidEndpointIndex = 0xFFFF; } /* cluster revision */ \ } +// The attrMask must contain the relevant ATTRIBUTE_MASK_* bits from +// attribute-metadata.h. Specifically: +// +// * Writable attributes must have ATTRIBUTE_MASK_WRITABLE +// * Nullable attributes (have X in the quality column in the spec) must have ATTRIBUTE_MASK_NULLABLE +// * Attributes that have T in the Access column in the spec must have ATTRIBUTE_MASK_MUST_USE_TIMED_WRITE #define DECLARE_DYNAMIC_ATTRIBUTE(attId, attType, attSizeBytes, attrMask) \ { \ ZAP_EMPTY_DEFAULT(), attId, attSizeBytes, ZAP_TYPE(attType), attrMask | ZAP_ATTRIBUTE_MASK(EXTERNAL_STORAGE) \ From b1829ff5ee77449db5dfee62aa7b81eb9f129601 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mi=C5=82osz=20Tomkiel?= Date: Wed, 11 Sep 2024 00:02:51 +0200 Subject: [PATCH 06/10] [chip-tool] Print DeviceType names next to ids (#35445) * Add DeviceTypeIdToText and it's usage * Update Zap-generated files * Minor ZAP fixes * Update Zap-generated files * Revert empty line * Update missing Zap-generated files --- .../logging/DataModelLogger-src.zapt | 1 + .../templates/logging/EntryToText-src.zapt | 10 ++ .../templates/logging/EntryToText.zapt | 4 +- .../templates/partials/StructLoggerImpl.zapt | 22 +++ .../cluster/logging/DataModelLogger.cpp | 25 +-- .../cluster/logging/EntryToText.cpp | 143 ++++++++++++++++++ .../cluster/logging/EntryToText.h | 2 + 7 files changed, 196 insertions(+), 11 deletions(-) diff --git a/examples/chip-tool/templates/logging/DataModelLogger-src.zapt b/examples/chip-tool/templates/logging/DataModelLogger-src.zapt index 3f76143ffab36c..cbce6c27abf767 100644 --- a/examples/chip-tool/templates/logging/DataModelLogger-src.zapt +++ b/examples/chip-tool/templates/logging/DataModelLogger-src.zapt @@ -1,6 +1,7 @@ {{> header}} #include +#include using namespace chip::app::Clusters; diff --git a/examples/chip-tool/templates/logging/EntryToText-src.zapt b/examples/chip-tool/templates/logging/EntryToText-src.zapt index 646ee2ad872eaf..f7cf8ab51c533b 100644 --- a/examples/chip-tool/templates/logging/EntryToText-src.zapt +++ b/examples/chip-tool/templates/logging/EntryToText-src.zapt @@ -81,4 +81,14 @@ char const * GeneratedCommandIdToText(chip::ClusterId cluster, chip::CommandId i {{/zcl_clusters}} default: return "Unknown"; } +} + +char const * DeviceTypeIdToText(chip::DeviceTypeId id) { + switch(id) + { +{{#zcl_device_types}} + case {{asHex code 8}}: return "{{caption}}"; +{{/zcl_device_types}} + default: return "Unknown"; + } } \ No newline at end of file diff --git a/examples/chip-tool/templates/logging/EntryToText.zapt b/examples/chip-tool/templates/logging/EntryToText.zapt index d1a78f84dc218c..fa77ffc916dced 100644 --- a/examples/chip-tool/templates/logging/EntryToText.zapt +++ b/examples/chip-tool/templates/logging/EntryToText.zapt @@ -10,4 +10,6 @@ char const * AttributeIdToText(chip::ClusterId cluster, chip::AttributeId id); char const * AcceptedCommandIdToText(chip::ClusterId cluster, chip::CommandId id); -char const * GeneratedCommandIdToText(chip::ClusterId cluster, chip::CommandId id); \ No newline at end of file +char const * GeneratedCommandIdToText(chip::ClusterId cluster, chip::CommandId id); + +char const * DeviceTypeIdToText(chip::DeviceTypeId id); \ No newline at end of file diff --git a/examples/chip-tool/templates/partials/StructLoggerImpl.zapt b/examples/chip-tool/templates/partials/StructLoggerImpl.zapt index 3dbd99e7400ef4..e4da56005b8064 100644 --- a/examples/chip-tool/templates/partials/StructLoggerImpl.zapt +++ b/examples/chip-tool/templates/partials/StructLoggerImpl.zapt @@ -3,12 +3,34 @@ CHIP_ERROR DataModelLogger::LogValue(const char * label, size_t indent, const ch DataModelLogger::LogString(label, indent, "{"); {{#zcl_struct_items}} { +{{#if (isEqual type "devtype_id") }} +{{#if isNullable }} + if (value.{{asLowerCamelCase label}}.IsNull()) + { + CHIP_ERROR err = LogValue("{{asUpperCamelCase label}}", indent + 1, value.{{asLowerCamelCase label}}); + if (err != CHIP_NO_ERROR) + { + DataModelLogger::LogString(indent + 1, "Struct truncated due to invalid value for '{{asUpperCamelCase label}}'"); + return err; + } + } + else + { + std::string item = std::to_string(value.{{asLowerCamelCase label}}.Value()) + " (" + DeviceTypeIdToText(value.{{asLowerCamelCase label}}.Value()) + ")"; + DataModelLogger::LogString("{{asUpperCamelCase label}}", indent + 1, item); + } +{{else}} + std::string item = std::to_string(value.{{asLowerCamelCase label}}) + " (" + DeviceTypeIdToText(value.{{asLowerCamelCase label}}) + ")"; + DataModelLogger::LogString("{{asUpperCamelCase label}}", indent + 1, item); +{{/if}} +{{else}} CHIP_ERROR err = LogValue("{{asUpperCamelCase label}}", indent + 1, value.{{asLowerCamelCase label}}); if (err != CHIP_NO_ERROR) { DataModelLogger::LogString(indent + 1, "Struct truncated due to invalid value for '{{asUpperCamelCase label}}'"); return err; } +{{/if}} } {{/zcl_struct_items}} DataModelLogger::LogString(indent, "}"); diff --git a/zzz_generated/chip-tool/zap-generated/cluster/logging/DataModelLogger.cpp b/zzz_generated/chip-tool/zap-generated/cluster/logging/DataModelLogger.cpp index 2d788bd2613215..8def02781d4b9a 100644 --- a/zzz_generated/chip-tool/zap-generated/cluster/logging/DataModelLogger.cpp +++ b/zzz_generated/chip-tool/zap-generated/cluster/logging/DataModelLogger.cpp @@ -18,6 +18,7 @@ // THIS FILE IS GENERATED BY ZAP #include +#include using namespace chip::app::Clusters; @@ -273,12 +274,8 @@ CHIP_ERROR DataModelLogger::LogValue(const char * label, size_t indent, { DataModelLogger::LogString(label, indent, "{"); { - CHIP_ERROR err = LogValue("DeviceType", indent + 1, value.deviceType); - if (err != CHIP_NO_ERROR) - { - DataModelLogger::LogString(indent + 1, "Struct truncated due to invalid value for 'DeviceType'"); - return err; - } + std::string item = std::to_string(value.deviceType) + " (" + DeviceTypeIdToText(value.deviceType) + ")"; + DataModelLogger::LogString("DeviceType", indent + 1, item); } { CHIP_ERROR err = LogValue("Revision", indent + 1, value.revision); @@ -641,11 +638,19 @@ DataModelLogger::LogValue(const char * label, size_t indent, } } { - CHIP_ERROR err = LogValue("DeviceType", indent + 1, value.deviceType); - if (err != CHIP_NO_ERROR) + if (value.deviceType.IsNull()) { - DataModelLogger::LogString(indent + 1, "Struct truncated due to invalid value for 'DeviceType'"); - return err; + CHIP_ERROR err = LogValue("DeviceType", indent + 1, value.deviceType); + if (err != CHIP_NO_ERROR) + { + DataModelLogger::LogString(indent + 1, "Struct truncated due to invalid value for 'DeviceType'"); + return err; + } + } + else + { + std::string item = std::to_string(value.deviceType.Value()) + " (" + DeviceTypeIdToText(value.deviceType.Value()) + ")"; + DataModelLogger::LogString("DeviceType", indent + 1, item); } } DataModelLogger::LogString(indent, "}"); diff --git a/zzz_generated/chip-tool/zap-generated/cluster/logging/EntryToText.cpp b/zzz_generated/chip-tool/zap-generated/cluster/logging/EntryToText.cpp index 07bac4319c7c61..8f320e64e682b8 100644 --- a/zzz_generated/chip-tool/zap-generated/cluster/logging/EntryToText.cpp +++ b/zzz_generated/chip-tool/zap-generated/cluster/logging/EntryToText.cpp @@ -6700,3 +6700,146 @@ char const * GeneratedCommandIdToText(chip::ClusterId cluster, chip::CommandId i return "Unknown"; } } + +char const * DeviceTypeIdToText(chip::DeviceTypeId id) +{ + switch (id) + { + case 0x0000000A: + return "Matter Door Lock"; + case 0x0000000B: + return "Matter Door Lock Controller"; + case 0x0000000E: + return "Matter Aggregator"; + case 0x0000000F: + return "Matter Generic Switch"; + case 0x00000011: + return "Matter Power Source"; + case 0x00000012: + return "Matter OTA Requestor"; + case 0x00000013: + return "Matter Bridged Device"; + case 0x00000014: + return "Matter OTA Provider"; + case 0x00000015: + return "Matter Contact Sensor"; + case 0x00000016: + return "Matter Root Node"; + case 0x00000019: + return "Matter Secondary Network Interface Device Type"; + case 0x00000022: + return "Matter Speaker"; + case 0x00000023: + return "Matter Casting Video Player"; + case 0x00000024: + return "Matter Content App"; + case 0x00000027: + return "Matter Mode Select"; + case 0x00000028: + return "Matter Basic Video Player"; + case 0x00000029: + return "Matter Casting Video Client"; + case 0x0000002A: + return "Matter Video Remote Control"; + case 0x0000002B: + return "Matter Fan"; + case 0x0000002C: + return "Matter Air Quality Sensor"; + case 0x0000002D: + return "Matter Air Purifier"; + case 0x00000041: + return "Matter Water Freeze Detector"; + case 0x00000042: + return "Matter Water Valve"; + case 0x00000043: + return "Matter Water Leak Detector"; + case 0x00000044: + return "Matter Rain Sensor"; + case 0x00000070: + return "Matter Refrigerator"; + case 0x00000071: + return "Matter Temperature Controlled Cabinet"; + case 0x00000072: + return "Matter Room Air Conditioner"; + case 0x00000073: + return "Matter Laundry Washer"; + case 0x00000074: + return "Matter Robotic Vacuum Cleaner"; + case 0x00000075: + return "Matter Dishwasher"; + case 0x00000076: + return "Matter Smoke CO Alarm"; + case 0x00000077: + return "Matter Cook Surface"; + case 0x00000078: + return "Matter Cooktop"; + case 0x00000079: + return "Matter Microwave Oven"; + case 0x0000007A: + return "Matter Extractor Hood"; + case 0x0000007B: + return "Matter Oven"; + case 0x0000007C: + return "Matter Laundry Dryer"; + case 0x00000090: + return "Matter Network Infrastructure Manager"; + case 0x00000091: + return "Matter Thread Border Router"; + case 0x00000100: + return "Matter On/Off Light"; + case 0x00000101: + return "Matter Dimmable Light"; + case 0x00000103: + return "Matter On/Off Light Switch"; + case 0x00000104: + return "Matter Dimmer Switch"; + case 0x00000105: + return "Matter Color Dimmer Switch"; + case 0x00000106: + return "Matter Light Sensor"; + case 0x00000107: + return "Matter Occupancy Sensor"; + case 0x0000010A: + return "Matter On/Off Plug-in Unit"; + case 0x0000010B: + return "Matter Dimmable Plug-in Unit"; + case 0x0000010C: + return "Matter Color Temperature Light"; + case 0x0000010D: + return "Matter Extended Color Light"; + case 0x00000202: + return "Matter Window Covering"; + case 0x00000203: + return "Matter Window Covering Controller"; + case 0x00000300: + return "Matter Heating/Cooling Unit"; + case 0x00000301: + return "Matter Thermostat"; + case 0x00000302: + return "Matter Temperature Sensor"; + case 0x00000303: + return "Matter Pump"; + case 0x00000304: + return "Matter Pump Controller"; + case 0x00000305: + return "Matter Pressure Sensor"; + case 0x00000306: + return "Matter Flow Sensor"; + case 0x00000307: + return "Matter Humidity Sensor"; + case 0x0000050C: + return "Matter EVSE"; + case 0x00000510: + return "Matter Electrical Sensor"; + case 0x00000840: + return "Matter Control Bridge"; + case 0x00000850: + return "Matter On/Off Sensor"; + case 0xFFF10001: + return "Matter Orphan Clusters"; + case 0xFFF10003: + return "Matter All-clusters-app Server Example"; + default: + return "Unknown"; + } +} diff --git a/zzz_generated/chip-tool/zap-generated/cluster/logging/EntryToText.h b/zzz_generated/chip-tool/zap-generated/cluster/logging/EntryToText.h index 27fa6512fbdd01..f65ab6e45f549a 100644 --- a/zzz_generated/chip-tool/zap-generated/cluster/logging/EntryToText.h +++ b/zzz_generated/chip-tool/zap-generated/cluster/logging/EntryToText.h @@ -28,3 +28,5 @@ char const * AttributeIdToText(chip::ClusterId cluster, chip::AttributeId id); char const * AcceptedCommandIdToText(chip::ClusterId cluster, chip::CommandId id); char const * GeneratedCommandIdToText(chip::ClusterId cluster, chip::CommandId id); + +char const * DeviceTypeIdToText(chip::DeviceTypeId id); From 702fae3c22e6c0b19066a8487b3a411971be9a53 Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Tue, 10 Sep 2024 17:47:45 -0700 Subject: [PATCH 07/10] [Darwin] MTRDeviceController initial device retain should not depend on matter queue (#35498) --- src/darwin/Framework/CHIP/MTRDeviceController.mm | 2 +- src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index 69bb02d5f1f03d..246955afdfd55b 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -859,7 +859,7 @@ - (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams // // Note that this is just an optimization to avoid throwing the information away and immediately // re-reading it from storage. - dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t) (kSecondsToWaitBeforeAPIClientRetainsMTRDevice * NSEC_PER_SEC)), self.chipWorkQueue, ^{ + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t) (kSecondsToWaitBeforeAPIClientRetainsMTRDevice * NSEC_PER_SEC)), dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0), ^{ MTR_LOG("%@ un-retain devices loaded at startup %lu", self, static_cast(deviceList.count)); }); }]; diff --git a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm index c8f8950579b350..bdcbafae1b6c9d 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm @@ -761,7 +761,7 @@ - (BOOL)startup:(MTRDeviceControllerStartupParamsInternal *)startupParams // // Note that this is just an optimization to avoid throwing the information away and immediately // re-reading it from storage. - dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t) (kSecondsToWaitBeforeAPIClientRetainsMTRDevice * NSEC_PER_SEC)), self.chipWorkQueue, ^{ + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t) (kSecondsToWaitBeforeAPIClientRetainsMTRDevice * NSEC_PER_SEC)), dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0), ^{ MTR_LOG("%@ un-retain devices loaded at startup %lu", self, static_cast(deviceList.count)); }); }]; From 1f2f6be08137d7c9117920afbedb69cf3dff7241 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 10 Sep 2024 21:08:20 -0400 Subject: [PATCH 08/10] Add a step timeout for Matter.framework unit tests. (#35530) If a test hangs, we end up running until job timeout after 6 hours. But that skips later steps, including log uploads, which makes it harder to figure out why the hang happened. The fix is to use a timeout on the test job that is somewhat shorter than the 6-hour job timeout, so we allow time for the log upload too. --- .github/workflows/darwin.yaml | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/.github/workflows/darwin.yaml b/.github/workflows/darwin.yaml index 360c5bb6963217..9eeac9118ef9d3 100644 --- a/.github/workflows/darwin.yaml +++ b/.github/workflows/darwin.yaml @@ -105,10 +105,9 @@ jobs: run: | scripts/examples/gn_build_example.sh examples/ota-requestor-app/linux out/debug/ota-requestor-app chip_config_network_layer_ble=false non_spec_compliant_ota_action_delay_floor=0 - name: Run Framework Tests - # For now disable unguarded-availability-new warnings because we - # internally use APIs that we are annotating as only available on - # new enough versions. Maybe we should change out deployment - # target versions instead? + # We want to ensure that our log upload runs on timeout, so use a timeout here shorter + # than the 6-hour overall job timeout. 4.5 hours should be plenty. + timeout-minutes: 270 working-directory: src/darwin/Framework run: | mkdir -p /tmp/darwin/framework-tests From 9257ca775ebb50467eae0678b9751c4162800b10 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 10 Sep 2024 21:43:06 -0400 Subject: [PATCH 09/10] Fix deadlock in MTRDeviceController suspend/resume. (#35526) If a resume happened before the async work from a suspend finished running on the Matter queue, we could deadlock: the Matter queue would be holding the device lock and try to acquire the lock to read "suspended" from the controller, while the controller would be holding the "suspended" lock and trying to acquire the device lock as part of synchronous resume work. The fix is to: 1) Stop trying to read the suspended state of the controller in MTRDevice_Concrete except during MTRDevice_Concrete initialization. Instead, store the suspended state directly in MTRDevice_Concrete at init time and update it when the controller notifies about suspend/resume. 2) Fix controller suspend/resume to guarantee that we create the device list snapshot and set our suspended state in one atomic operation, so that devices are either created before that op (and then end up in the list) or after it (and then pick up the new controller suspended state. 3) Remove the locking around controller suspend/resume, because it's proving hard to reason about. This will be replaced by something better once we figure out how threadsafety should work in controller API in general. In the meantime, suspend/resume on a given controller should only be done on a single serial queue. 4) Use an atomic for the suspended state, so reading it can be done from any thread/queue even without locking. --- .../Framework/CHIP/MTRDeviceController.mm | 71 ++++++++++--------- .../Framework/CHIP/MTRDevice_Concrete.mm | 16 +++-- 2 files changed, 49 insertions(+), 38 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index 246955afdfd55b..2f40df76a7086d 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -154,7 +154,11 @@ @implementation MTRDeviceController { MTRP256KeypairBridge _signingKeypairBridge; MTRP256KeypairBridge _operationalKeypairBridge; - BOOL _suspended; + // For now, we just ensure that access to _suspended is atomic, but don't + // guarantee atomicity of the the entire suspend/resume operation. The + // expectation is that suspend/resume on a given controller happen on some + // specific queue, so can't race against each other. + std::atomic _suspended; // Counters to track assertion status and access controlled by the _assertionLock NSUInteger _keepRunningAssertionCounter; @@ -378,9 +382,7 @@ - (BOOL)isRunning - (BOOL)isSuspended { - @synchronized(self) { - return _suspended; - } + return _suspended; } - (void)_notifyDelegatesOfSuspendState @@ -397,48 +399,49 @@ - (void)suspend { MTR_LOG("%@ suspending", self); - @synchronized(self) { + NSArray * devicesToSuspend; + { + std::lock_guard lock(*self.deviceMapLock); + // Set _suspended under the device map lock. This guarantees that + // for any given device exactly one of two things is true: + // * It is in the snapshot we are creating + // * It is created after we have changed our _suspended state. _suspended = YES; + devicesToSuspend = [self.nodeIDToDeviceMap objectEnumerator].allObjects; + } - NSArray * devicesToSuspend; - { - std::lock_guard lock(*self.deviceMapLock); - devicesToSuspend = [self.nodeIDToDeviceMap objectEnumerator].allObjects; - } - - for (MTRDevice * device in devicesToSuspend) { - [device controllerSuspended]; - } - - // TODO: In the concrete class, consider what should happen with: - // - // * Active commissioning sessions (presumably close them?) - // * CASE sessions in general. - // * Possibly try to see whether we can change our fabric entry to not advertise and restart advertising. - - [self _notifyDelegatesOfSuspendState]; + for (MTRDevice * device in devicesToSuspend) { + [device controllerSuspended]; } + + // TODO: In the concrete class, consider what should happen with: + // + // * Active commissioning sessions (presumably close them?) + // * CASE sessions in general. + // * Possibly try to see whether we can change our fabric entry to not advertise and restart advertising. + [self _notifyDelegatesOfSuspendState]; } - (void)resume { MTR_LOG("%@ resuming", self); - @synchronized(self) { + NSArray * devicesToResume; + { + std::lock_guard lock(*self.deviceMapLock); + // Set _suspended under the device map lock. This guarantees that + // for any given device exactly one of two things is true: + // * It is in the snapshot we are creating + // * It is created after we have changed our _suspended state. _suspended = NO; + devicesToResume = [self.nodeIDToDeviceMap objectEnumerator].allObjects; + } - NSArray * devicesToResume; - { - std::lock_guard lock(*self.deviceMapLock); - devicesToResume = [self.nodeIDToDeviceMap objectEnumerator].allObjects; - } - - for (MTRDevice * device in devicesToResume) { - [device controllerResumed]; - } - - [self _notifyDelegatesOfSuspendState]; + for (MTRDevice * device in devicesToResume) { + [device controllerResumed]; } + + [self _notifyDelegatesOfSuspendState]; } - (BOOL)matchesPendingShutdownControllerWithOperationalCertificate:(nullable MTRCertificateDERBytes)operationalCertificate andRootCertificate:(nullable MTRCertificateDERBytes)rootCertificate diff --git a/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm b/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm index 055c8a31ac45c5..64ba651f0ce192 100644 --- a/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm +++ b/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm @@ -62,6 +62,7 @@ @interface MTRDevice_Concrete () @property (nonatomic, readwrite) MTRDeviceState state; @property (nonatomic, readwrite, nullable) NSDate * estimatedStartTime; @property (nonatomic, readwrite, nullable, copy) NSNumber * estimatedSubscriptionLatency; +@property (nonatomic, readwrite, assign) BOOL suspended; @end @@ -395,6 +396,8 @@ - (instancetype)initWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceControlle }]; } + self.suspended = controller.suspended; + MTR_LOG_DEBUG("%@ init with hex nodeID 0x%016llX", self, _nodeID.unsignedLongLongValue); } return self; @@ -719,8 +722,8 @@ - (BOOL)_subscriptionsAllowed { os_unfair_lock_assert_owner(&self->_lock); - // We should not allow a subscription for suspended controllers or device controllers over XPC. - return _deviceController.isSuspended == NO && ![_deviceController isKindOfClass:MTRDeviceControllerOverXPC.class]; + // We should not allow a subscription when we are suspended or for device controllers over XPC. + return self.suspended == NO && ![_deviceController isKindOfClass:MTRDeviceControllerOverXPC.class]; } - (void)_delegateAdded @@ -1290,7 +1293,7 @@ - (void)_doHandleSubscriptionReset:(NSNumber * _Nullable)retryDelay os_unfair_lock_assert_owner(&_lock); - if (_deviceController.isSuspended) { + if (self.suspended) { MTR_LOG("%@ ignoring expected subscription reset on controller suspend", self); return; } @@ -1378,7 +1381,6 @@ - (void)_reattemptSubscriptionNowIfNeededWithReason:(NSString *)reason } MTR_LOG("%@ reattempting subscription with reason %@", self, reason); - self.reattemptingSubscription = NO; [self _setupSubscriptionWithReason:reason]; } @@ -2302,6 +2304,10 @@ - (void)_setupSubscriptionWithReason:(NSString *)reason os_unfair_lock_assert_owner(&self->_lock); + // If we have a pending subscription reattempt, make sure it does not + // actually happen, since we are trying to do a subscription now. + self.reattemptingSubscription = NO; + if (![self _subscriptionsAllowed]) { MTR_LOG("%@ _setupSubscription: Subscriptions not allowed. Do not set up subscription (reason: %@)", self, reason); return; @@ -3991,6 +3997,7 @@ - (void)controllerSuspended [super controllerSuspended]; std::lock_guard lock(self->_lock); + self.suspended = YES; [self _resetSubscriptionWithReasonString:@"Controller suspended"]; // Ensure that any pre-existing resubscribe attempts we control don't try to @@ -4003,6 +4010,7 @@ - (void)controllerResumed [super controllerResumed]; std::lock_guard lock(self->_lock); + self.suspended = NO; if (![self _delegateExists]) { MTR_LOG("%@ ignoring controller resume: no delegates", self); From 9a6694fca3ee23ed4af5bfa168c4851e0dca00cf Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 11 Sep 2024 00:47:25 -0400 Subject: [PATCH 10/10] Add better logging when doing Matter frameworks controller suspend/resume. (#35520) --- src/darwin/Framework/CHIP/MTRDeviceController.mm | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index 2f40df76a7086d..dc36aeb694d1e7 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -410,6 +410,7 @@ - (void)suspend devicesToSuspend = [self.nodeIDToDeviceMap objectEnumerator].allObjects; } + MTR_LOG("%@ found %lu devices to suspend", self, static_cast(devicesToSuspend.count)); for (MTRDevice * device in devicesToSuspend) { [device controllerSuspended]; } @@ -437,6 +438,7 @@ - (void)resume devicesToResume = [self.nodeIDToDeviceMap objectEnumerator].allObjects; } + MTR_LOG("%@ found %lu devices to resume", self, static_cast(devicesToResume.count)); for (MTRDevice * device in devicesToResume) { [device controllerResumed]; }