From 31f73a7363434b00b91b915033305a79f5dca7f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ludovic=20BOU=C3=89?= Date: Thu, 21 Mar 2024 08:26:19 +0100 Subject: [PATCH 01/11] [Silabs] Fix typo for light-switch-app (#32645) * Update ShellCommands.h * Update ShellCommands.cpp * Update LightSwitchMgr.cpp --- examples/light-switch-app/silabs/include/ShellCommands.h | 4 ++-- examples/light-switch-app/silabs/src/LightSwitchMgr.cpp | 2 +- examples/light-switch-app/silabs/src/ShellCommands.cpp | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/examples/light-switch-app/silabs/include/ShellCommands.h b/examples/light-switch-app/silabs/include/ShellCommands.h index 576ad5235ffd6b..f9c6b5d9864d75 100644 --- a/examples/light-switch-app/silabs/include/ShellCommands.h +++ b/examples/light-switch-app/silabs/include/ShellCommands.h @@ -21,10 +21,10 @@ #if defined(ENABLE_CHIP_SHELL) -namespace LightSwtichCommands { +namespace LightSwitchCommands { void RegisterSwitchCommands(); -} // namespace LightSwtichCommands +} // namespace LightSwitchCommands #endif // defined(ENABLE_CHIP_SHELL) diff --git a/examples/light-switch-app/silabs/src/LightSwitchMgr.cpp b/examples/light-switch-app/silabs/src/LightSwitchMgr.cpp index dddff2fc047616..b33ac42630b80c 100644 --- a/examples/light-switch-app/silabs/src/LightSwitchMgr.cpp +++ b/examples/light-switch-app/silabs/src/LightSwitchMgr.cpp @@ -62,7 +62,7 @@ CHIP_ERROR LightSwitchMgr::Init(EndpointId lightSwitchEndpoint, chip::EndpointId } #if defined(ENABLE_CHIP_SHELL) - LightSwtichCommands::RegisterSwitchCommands(); + LightSwitchCommands::RegisterSwitchCommands(); #endif // defined(ENABLE_CHIP_SHELL) return error; diff --git a/examples/light-switch-app/silabs/src/ShellCommands.cpp b/examples/light-switch-app/silabs/src/ShellCommands.cpp index ee5872276efe19..565f425f8af344 100644 --- a/examples/light-switch-app/silabs/src/ShellCommands.cpp +++ b/examples/light-switch-app/silabs/src/ShellCommands.cpp @@ -29,7 +29,7 @@ using namespace chip; using namespace chip::app; -namespace LightSwtichCommands { +namespace LightSwitchCommands { using Shell::Engine; using Shell::shell_command_t; @@ -286,6 +286,6 @@ void RegisterSwitchCommands() Engine::Root().RegisterCommands(&sSwitchCommand, 1); } -} // namespace LightSwtichCommands +} // namespace LightSwitchCommands #endif // ENABLE_CHIP_SHELL From 7f1413364e012fbf03cdba0aabd2fb73624e9311 Mon Sep 17 00:00:00 2001 From: Junior Martinez <67972863+jmartinez-silabs@users.noreply.github.com> Date: Thu, 21 Mar 2024 09:18:17 -0400 Subject: [PATCH 02/11] fix BLE ADV random address configuration (#32657) --- src/platform/silabs/efr32/BLEManagerImpl.cpp | 50 ++++++++------------ 1 file changed, 21 insertions(+), 29 deletions(-) diff --git a/src/platform/silabs/efr32/BLEManagerImpl.cpp b/src/platform/silabs/efr32/BLEManagerImpl.cpp index 4df91ad23368bb..db9939819b77b7 100644 --- a/src/platform/silabs/efr32/BLEManagerImpl.cpp +++ b/src/platform/silabs/efr32/BLEManagerImpl.cpp @@ -476,29 +476,33 @@ CHIP_ERROR BLEManagerImpl::ConfigureAdvertisingData(void) ReturnErrorOnFailure(EncodeAdditionalDataTlv()); #endif - if (0xff != advertising_set_handle) + if (advertising_set_handle == 0xff) { - sl_bt_advertiser_delete_set(advertising_set_handle); - advertising_set_handle = 0xff; - } - - ret = sl_bt_advertiser_create_set(&advertising_set_handle); - if (ret != SL_STATUS_OK) - { - err = MapBLEError(ret); - ChipLogError(DeviceLayer, "sl_bt_advertiser_create_set() failed: %s", ErrorStr(err)); - ExitNow(); + ret = sl_bt_advertiser_create_set(&advertising_set_handle); + VerifyOrExit(ret == SL_STATUS_OK, { + err = MapBLEError(ret); + ChipLogError(DeviceLayer, "sl_bt_advertiser_create_set() failed: %s", ErrorStr(err)); + }); + + bd_addr randomizedAddr = {}; + ret = sl_bt_advertiser_set_random_address(advertising_set_handle, sl_bt_gap_random_resolvable_address, randomizedAddr, + &randomizedAddr); + VerifyOrExit(ret == SL_STATUS_OK, { + err = MapBLEError(ret); + ChipLogError(DeviceLayer, "sl_bt_advertiser_set_random_address() failed: %s", ErrorStr(err)); + }); + ChipLogDetail(DeviceLayer, "BLE Resolvable private random address %02X:%02X:%02X:%02X:%02X:%02X", randomizedAddr.addr[5], + randomizedAddr.addr[4], randomizedAddr.addr[3], randomizedAddr.addr[2], randomizedAddr.addr[1], + randomizedAddr.addr[0]); } ret = sl_bt_legacy_advertiser_set_data(advertising_set_handle, sl_bt_advertiser_advertising_data_packet, index, (uint8_t *) advData); - if (ret != SL_STATUS_OK) - { + VerifyOrExit(ret == SL_STATUS_OK, { err = MapBLEError(ret); ChipLogError(DeviceLayer, "sl_bt_legacy_advertiser_set_data() - Advertising Data failed: %s", ErrorStr(err)); - ExitNow(); - } + }); index = 0; @@ -515,12 +519,10 @@ CHIP_ERROR BLEManagerImpl::ConfigureAdvertisingData(void) ret = sl_bt_legacy_advertiser_set_data(advertising_set_handle, sl_bt_advertiser_scan_response_packet, index, (uint8_t *) responseData); - if (ret != SL_STATUS_OK) - { + VerifyOrExit(ret == SL_STATUS_OK, { err = MapBLEError(ret); ChipLogError(DeviceLayer, "sl_bt_legacy_advertiser_set_data() - Scan Response failed: %s", ErrorStr(err)); - ExitNow(); - } + }); err = MapBLEError(ret); @@ -548,11 +550,6 @@ CHIP_ERROR BLEManagerImpl::StartAdvertising(void) ChipLogDetail(DeviceLayer, "Start BLE advertisement"); } - const uint8_t kResolvableRandomAddrType = 2; // Private resolvable random address type - bd_addr unusedBdAddr; // We can ignore this field when setting random address. - sl_bt_advertiser_set_random_address(advertising_set_handle, kResolvableRandomAddrType, unusedBdAddr, &unusedBdAddr); - (void) unusedBdAddr; - err = ConfigureAdvertisingData(); SuccessOrExit(err); @@ -582,11 +579,6 @@ CHIP_ERROR BLEManagerImpl::StartAdvertising(void) #endif } - // TODO(#32274): Explain why we cannot have interval_min == interval_max. - if (interval_min == interval_max) - { - ++interval_max; - } ChipLogProgress(DeviceLayer, "Starting advertising with interval_min=%u, intverval_max=%u (units of 625us)", static_cast(interval_min), static_cast(interval_max)); ret = sl_bt_advertiser_set_timing(advertising_set_handle, interval_min, interval_max, 0, 0); From 84913af2f3a2f98a7a67a7c2a88c23d6cd1fba01 Mon Sep 17 00:00:00 2001 From: Matthew Swartwout Date: Thu, 21 Mar 2024 07:43:16 -0700 Subject: [PATCH 03/11] Add equality operator for ConcreteDataAttributePath (#32309) * Add equality operator for ConcreteDataAttributePath * Restyled by clang-format * Add inequality tests --------- Co-authored-by: Restyled.io --- src/app/ConcreteAttributePath.h | 12 ++- src/app/tests/TestConcreteAttributePath.cpp | 99 +++++++++++++++++++++ 2 files changed, 108 insertions(+), 3 deletions(-) diff --git a/src/app/ConcreteAttributePath.h b/src/app/ConcreteAttributePath.h index 253b590ef90739..cfde88ab9bd374 100644 --- a/src/app/ConcreteAttributePath.h +++ b/src/app/ConcreteAttributePath.h @@ -144,9 +144,15 @@ struct ConcreteDataAttributePath : public ConcreteAttributePath bool MatchesConcreteAttributePath(const ConcreteAttributePath & aOther) { return ConcreteAttributePath::operator==(aOther); } - bool operator==(const ConcreteDataAttributePath & aOther) const = delete; - bool operator!=(const ConcreteDataAttributePath & aOther) const = delete; - bool operator<(const ConcreteDataAttributePath & aOther) const = delete; + bool operator==(const ConcreteDataAttributePath & aOther) const + { + return ConcreteAttributePath::operator==(aOther) && (mListIndex == aOther.mListIndex) && (mListOp == aOther.mListOp) && + (mDataVersion == aOther.mDataVersion); + } + + bool operator!=(const ConcreteDataAttributePath & aOther) const { return !(*this == aOther); } + + bool operator<(const ConcreteDataAttributePath & aOther) const = delete; // // This index is only valid if `mListOp` is set to a list item operation, i.e diff --git a/src/app/tests/TestConcreteAttributePath.cpp b/src/app/tests/TestConcreteAttributePath.cpp index 6e3451be26a78f..a598c2f23c121f 100644 --- a/src/app/tests/TestConcreteAttributePath.cpp +++ b/src/app/tests/TestConcreteAttributePath.cpp @@ -69,6 +69,87 @@ void TestConcreteDataAttributePathMatchesConcreteAttributePathInequality(nlTestS NL_TEST_ASSERT(aSuite, !data_path.MatchesConcreteAttributePath(path)); } +void TestConcreteDataAttributePathEqualityDefaultConstructor(nlTestSuite * aSuite, void * aContext) +{ + ConcreteDataAttributePath one; + ConcreteDataAttributePath two; + NL_TEST_ASSERT(aSuite, one == two); +} + +void TestConcreteDataAttributePathEqualityConcreteAttributePathConstructor(nlTestSuite * aSuite, void * aContext) +{ + ConcreteAttributePath path(/*aEndpointId=*/1, /*aClusterId=*/2, /*aAttributeId=*/3); + ConcreteDataAttributePath one(path); + ConcreteDataAttributePath two(path); + NL_TEST_ASSERT(aSuite, one == two); +} + +void TestConcreteDataAttributePathInequalityConcreteAttributePathConstructorDifferentAttributeId(nlTestSuite * aSuite, + void * aContext) +{ + ConcreteAttributePath path_one(/*aEndpointId=*/1, /*aClusterId=*/2, /*aAttributeId=*/3); + ConcreteAttributePath path_two(/*aEndpointId=*/1, /*aClusterId=*/2, /*aAttributeId=*/4); + ConcreteDataAttributePath one(path_one); + ConcreteDataAttributePath two(path_two); + NL_TEST_ASSERT(aSuite, one != two); +} + +void TestConcreteDataAttributePathEqualityConcreteAttributePathArgsConstructor(nlTestSuite * aSuite, void * aContext) +{ + ConcreteDataAttributePath one(/*aEndpointId=*/1, /*aClusterId=*/2, /*aAttributeId=*/3); + ConcreteDataAttributePath two(/*aEndpointId=*/1, /*aClusterId=*/2, /*aAttributeId=*/3); + NL_TEST_ASSERT(aSuite, one == two); +} + +void TestConcreteDataAttributePathInequalityConcreteAttributePathArgsConstructorDifferentAttributeId(nlTestSuite * aSuite, + void * aContext) +{ + ConcreteDataAttributePath one(/*aEndpointId=*/1, /*aClusterId=*/2, /*aAttributeId=*/3); + ConcreteDataAttributePath two(/*aEndpointId=*/1, /*aClusterId=*/2, /*aAttributeId=*/4); + NL_TEST_ASSERT(aSuite, one != two); +} + +void TestConcreteDataAttributePathEqualityDataVersionConstructor(nlTestSuite * aSuite, void * aContext) +{ + ConcreteDataAttributePath one(/*aEndpointId=*/1, /*aClusterId=*/2, /*aAttributeId=*/3, /*aDataVersion=*/MakeOptional(4U)); + ConcreteDataAttributePath two(/*aEndpointId=*/1, /*aClusterId=*/2, /*aAttributeId=*/3, /*aDataVersion=*/MakeOptional(4U)); + NL_TEST_ASSERT(aSuite, one == two); +} + +void TestConcreteDataAttributePathInequalityDataVersionConstructorDifferentDataVersion(nlTestSuite * aSuite, void * aContext) +{ + ConcreteDataAttributePath one(/*aEndpointId=*/1, /*aClusterId=*/2, /*aAttributeId=*/3, /*aDataVersion=*/MakeOptional(4U)); + ConcreteDataAttributePath two(/*aEndpointId=*/1, /*aClusterId=*/2, /*aAttributeId=*/3, /*aDataVersion=*/MakeOptional(5U)); + NL_TEST_ASSERT(aSuite, one != two); +} + +void TestConcreteDataAttributePathEqualityListConstructor(nlTestSuite * aSuite, void * aContext) +{ + ConcreteDataAttributePath one(/*aEndpointId=*/1, /*aClusterId=*/2, /*aAttributeId=*/3, + ConcreteDataAttributePath::ListOperation::ReplaceAll, /*aListIndex=*/5); + ConcreteDataAttributePath two(/*aEndpointId=*/1, /*aClusterId=*/2, /*aAttributeId=*/3, + ConcreteDataAttributePath::ListOperation::ReplaceAll, /*aListIndex=*/5); + NL_TEST_ASSERT(aSuite, one == two); +} + +void TestConcreteDataAttributePathInequalityListConstructorDifferentListOp(nlTestSuite * aSuite, void * aContext) +{ + ConcreteDataAttributePath one(/*aEndpointId=*/1, /*aClusterId=*/2, /*aAttributeId=*/3, + ConcreteDataAttributePath::ListOperation::ReplaceAll, /*aListIndex=*/5); + ConcreteDataAttributePath two(/*aEndpointId=*/1, /*aClusterId=*/2, /*aAttributeId=*/3, + ConcreteDataAttributePath::ListOperation::ReplaceItem, /*aListIndex=*/5); + NL_TEST_ASSERT(aSuite, one != two); +} + +void TestConcreteDataAttributePathInequalityListConstructorDifferentListIndex(nlTestSuite * aSuite, void * aContext) +{ + ConcreteDataAttributePath one(/*aEndpointId=*/1, /*aClusterId=*/2, /*aAttributeId=*/3, + ConcreteDataAttributePath::ListOperation::ReplaceAll, /*aListIndex=*/5); + ConcreteDataAttributePath two(/*aEndpointId=*/1, /*aClusterId=*/2, /*aAttributeId=*/3, + ConcreteDataAttributePath::ListOperation::ReplaceAll, /*aListIndex=*/6); + NL_TEST_ASSERT(aSuite, one != two); +} + const nlTest sTests[] = { NL_TEST_DEF("TestConcreteAttributePathEqualityDefaultConstructor", TestConcreteAttributePathEqualityDefaultConstructor), NL_TEST_DEF("TestConcreteAttributePathEquality", TestConcreteAttributePathEquality), @@ -77,6 +158,24 @@ const nlTest sTests[] = { TestConcreteDataAttributePathMatchesConcreteAttributePathEquality), NL_TEST_DEF("TestConcreteDataAttributePathMatchesConcreteAttributePathInequality", TestConcreteDataAttributePathMatchesConcreteAttributePathInequality), + NL_TEST_DEF("TestConcreteDataAttributePathEqualityDefaultConstructor", TestConcreteDataAttributePathEqualityDefaultConstructor), + NL_TEST_DEF("TestConcreteDataAttributePathEqualityConcreteAttributePathConstructor", + TestConcreteDataAttributePathEqualityConcreteAttributePathConstructor), + NL_TEST_DEF("TestConcreteDataAttributePathInequalityConcreteAttributePathConstructorDifferentAttributeId", + TestConcreteDataAttributePathInequalityConcreteAttributePathConstructorDifferentAttributeId), + NL_TEST_DEF("TestConcreteDataAttributePathEqualityConcreteAttributePathArgsConstructor", + TestConcreteDataAttributePathEqualityConcreteAttributePathArgsConstructor), + NL_TEST_DEF("TestConcreteDataAttributePathInequalityConcreteAttributePathArgsConstructorDifferentAttributeId", + TestConcreteDataAttributePathInequalityConcreteAttributePathArgsConstructorDifferentAttributeId), + NL_TEST_DEF("TestConcreteDataAttributePathEqualityDataVersionConstructor", + TestConcreteDataAttributePathEqualityDataVersionConstructor), + NL_TEST_DEF("TestConcreteDataAttributePathInequalityDataVersionConstructorDifferentDataVersion", + TestConcreteDataAttributePathInequalityDataVersionConstructorDifferentDataVersion), + NL_TEST_DEF("TestConcreteDataAttributePathEqualityListConstructor", TestConcreteDataAttributePathEqualityListConstructor), + NL_TEST_DEF("TestConcreteDataAttributePathInequalityListConstructorDifferentListOp", + TestConcreteDataAttributePathInequalityListConstructorDifferentListOp), + NL_TEST_DEF("TestConcreteDataAttributePathInequalityListConstructorDifferentListIndex", + TestConcreteDataAttributePathInequalityListConstructorDifferentListIndex), NL_TEST_SENTINEL() }; From 6b82061fa32f11f2fc4880c31793a5e027412bf2 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 21 Mar 2024 12:35:18 -0400 Subject: [PATCH 04/11] Use `curl` to download github artifacts during bloat report (#32666) * Use CURL to download artifact data. ghapi does not seem to work, however the documented curl way does * Switch back to github_token * Restyle * Remove unused item --- .github/workflows/bloat_check.yaml | 2 +- scripts/tools/memory/memdf/util/github.py | 27 ++++++++++++++++++++++- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/.github/workflows/bloat_check.yaml b/.github/workflows/bloat_check.yaml index 7b503b865081c9..148426d99ccd6b 100644 --- a/.github/workflows/bloat_check.yaml +++ b/.github/workflows/bloat_check.yaml @@ -50,4 +50,4 @@ jobs: --github-limit-artifacts 500 \ --github-limit-comments 20 \ --github-repository project-chip/connectedhomeip \ - --github-api-token "${{ secrets.BLOAT_REPORT }}" + --github-api-token "${{ secrets.GITHUB_TOKEN }}" diff --git a/scripts/tools/memory/memdf/util/github.py b/scripts/tools/memory/memdf/util/github.py index 5312c1702a5829..1c7e34273776c0 100644 --- a/scripts/tools/memory/memdf/util/github.py +++ b/scripts/tools/memory/memdf/util/github.py @@ -18,6 +18,7 @@ import itertools import logging import os +import subprocess from typing import Iterable, Mapping, Optional import dateutil # type: ignore @@ -173,7 +174,31 @@ def download_artifact(self, artifact_id: int): logging.debug('Downloading artifact %d', artifact_id) try: assert self.ghapi - return self.ghapi.actions.download_artifact(artifact_id, 'zip') + + # It seems like github artifact download is at least partially broken + # (see https://github.com/project-chip/connectedhomeip/issues/32656) + # + # This makes `self.ghapi.actions.download_artifact` not work + # + # Oddly enough downloading via CURL seems ok + owner = self.config['github.owner'] + repo = self.config['github.repo'] + token = self.config['github.token'] + + download_url = f"https://api.github.com/repos/{owner}/{repo}/actions/artifacts/{artifact_id}/zip" + + # Follow https://docs.github.com/en/rest/actions/artifacts?apiVersion=2022-11-28#download-an-artifact + return subprocess.check_output( + [ + 'curl', + '-L', + '-H', 'Accept: application/vnd.github+json', + '-H', f'Authorization: Bearer {token}', + '-H', 'X-GitHub-Api-Version: 2022-11-28', + '--output', '-', + download_url + ] + ) except Exception as e: logging.error('Failed to download artifact %d: %s', artifact_id, e) return None From 1f2aff46470a9dcb9c37fab3be6ade62fe083051 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 21 Mar 2024 15:49:11 -0400 Subject: [PATCH 05/11] Start moving matter pre/post callbacks from `__weak__` handling to a injectable class instead. (#32648) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add MatterCallbacks.cpp to include callbacks and redirection * Fix compile dependencies * Replace a few more callbacks * Fix lint file - mattercallbacks is now tracked * Restyle * Update ApplicationCallbacks to DataModelCallbacks * Update src/app/util/MatterCallbacks.cpp Co-authored-by: Arkadiusz Bokowy * Update src/app/util/MatterCallbacks.h Co-authored-by: Damian Królik <66667989+Damian-Nordic@users.noreply.github.com> * Updated comments --------- Co-authored-by: Arkadiusz Bokowy Co-authored-by: Damian Królik <66667989+Damian-Nordic@users.noreply.github.com> --- .github/workflows/lint.yml | 1 - src/app/BUILD.gn | 2 + src/app/CommandHandler.cpp | 17 ++---- src/app/WriteHandler.cpp | 17 +++--- src/app/reporting/Engine.cpp | 13 +++-- src/app/util/BUILD.gn | 12 ++++ src/app/util/MatterCallbacks.cpp | 99 ++++++++++++++++++++++++++++++++ src/app/util/MatterCallbacks.h | 72 ++++++++++++++++------- 8 files changed, 185 insertions(+), 48 deletions(-) create mode 100644 src/app/util/MatterCallbacks.cpp diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 59c0d4e59a7c9b..38c1dac8188b02 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -117,7 +117,6 @@ jobs: --known-failure app/util/generic-callbacks.h \ --known-failure app/util/generic-callback-stubs.cpp \ --known-failure app/util/im-client-callbacks.h \ - --known-failure app/util/MatterCallbacks.h \ --known-failure app/util/util.cpp \ --known-failure app/util/util.h \ --known-failure app/WriteHandler.h \ diff --git a/src/app/BUILD.gn b/src/app/BUILD.gn index 2641356da45308..4cf96b3a1a84c8 100644 --- a/src/app/BUILD.gn +++ b/src/app/BUILD.gn @@ -210,6 +210,7 @@ static_library("interaction-model") { "${chip_root}/src/app/icd/server:icd-server-config", "${chip_root}/src/app/icd/server:observer", "${chip_root}/src/app/util:af-types", + "${chip_root}/src/app/util:callbacks", "${chip_root}/src/lib/address_resolve", "${chip_root}/src/lib/support", "${chip_root}/src/lib/support:static-support", @@ -318,6 +319,7 @@ static_library("app") { ":interaction-model", "${chip_root}/src/app/data-model", "${chip_root}/src/app/icd/server:icd-server-config", + "${chip_root}/src/app/util:callbacks", "${chip_root}/src/lib/address_resolve", "${chip_root}/src/lib/support", "${chip_root}/src/messaging", diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index 8170b57abd7d23..f80f41e9a63c61 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -449,9 +449,9 @@ Status CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommandElem { ChipLogDetail(DataManagement, "Received command for Endpoint=%u Cluster=" ChipLogFormatMEI " Command=" ChipLogFormatMEI, concretePath.mEndpointId, ChipLogValueMEI(concretePath.mClusterId), ChipLogValueMEI(concretePath.mCommandId)); - SuccessOrExit(err = MatterPreCommandReceivedCallback(concretePath, GetSubjectDescriptor())); + SuccessOrExit(err = DataModelCallbacks::GetInstance()->PreCommandReceived(concretePath, GetSubjectDescriptor())); mpCallback->DispatchCommand(*this, concretePath, commandDataReader); - MatterPostCommandReceivedCallback(concretePath, GetSubjectDescriptor()); + DataModelCallbacks::GetInstance()->PostCommandReceived(concretePath, GetSubjectDescriptor()); } exit: @@ -555,11 +555,11 @@ Status CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aComman continue; } } - if ((err = MatterPreCommandReceivedCallback(concretePath, GetSubjectDescriptor())) == CHIP_NO_ERROR) + if ((err = DataModelCallbacks::GetInstance()->PreCommandReceived(concretePath, GetSubjectDescriptor())) == CHIP_NO_ERROR) { TLV::TLVReader dataReader(commandDataReader); mpCallback->DispatchCommand(*this, concretePath, dataReader); - MatterPostCommandReceivedCallback(concretePath, GetSubjectDescriptor()); + DataModelCallbacks::GetInstance()->PostCommandReceived(concretePath, GetSubjectDescriptor()); } else { @@ -1053,12 +1053,3 @@ void CommandHandler::TestOnlyInvokeCommandRequestWithFaultsInjected(Messaging::E } // namespace app } // namespace chip - -CHIP_ERROR __attribute__((weak)) MatterPreCommandReceivedCallback(const chip::app::ConcreteCommandPath & commandPath, - const chip::Access::SubjectDescriptor & subjectDescriptor) -{ - return CHIP_NO_ERROR; -} -void __attribute__((weak)) MatterPostCommandReceivedCallback(const chip::app::ConcreteCommandPath & commandPath, - const chip::Access::SubjectDescriptor & subjectDescriptor) -{} diff --git a/src/app/WriteHandler.cpp b/src/app/WriteHandler.cpp index f7fc1f3273d7c8..911c2f594095cd 100644 --- a/src/app/WriteHandler.cpp +++ b/src/app/WriteHandler.cpp @@ -328,7 +328,9 @@ CHIP_ERROR WriteHandler::ProcessAttributeDataIBs(TLV::TLVReader & aAttributeData mProcessingAttributeIsList = dataAttributePath.IsListOperation(); mProcessingAttributePath.SetValue(dataAttributePath); - MatterPreAttributeWriteCallback(dataAttributePath); + DataModelCallbacks::GetInstance()->AttributeOperation(DataModelCallbacks::OperationType::Write, + DataModelCallbacks::OperationOrder::Pre, dataAttributePath); + TLV::TLVWriter backup; DataVersion version = 0; mWriteResponseBuilder.GetWriteResponses().Checkpoint(backup); @@ -348,7 +350,9 @@ CHIP_ERROR WriteHandler::ProcessAttributeDataIBs(TLV::TLVReader & aAttributeData mWriteResponseBuilder.GetWriteResponses().Rollback(backup); err = AddStatus(dataAttributePath, StatusIB(err)); } - MatterPostAttributeWriteCallback(dataAttributePath); + + DataModelCallbacks::GetInstance()->AttributeOperation(DataModelCallbacks::OperationType::Write, + DataModelCallbacks::OperationOrder::Post, dataAttributePath); SuccessOrExit(err); } @@ -482,7 +486,8 @@ CHIP_ERROR WriteHandler::ProcessGroupAttributeDataIBs(TLV::TLVReader & aAttribut chip::TLV::TLVReader tmpDataReader(dataReader); - MatterPreAttributeWriteCallback(dataAttributePath); + DataModelCallbacks::GetInstance()->AttributeOperation(DataModelCallbacks::OperationType::Write, + DataModelCallbacks::OperationOrder::Pre, dataAttributePath); err = WriteSingleClusterData(subjectDescriptor, dataAttributePath, tmpDataReader, this); if (err != CHIP_NO_ERROR) @@ -493,7 +498,8 @@ CHIP_ERROR WriteHandler::ProcessGroupAttributeDataIBs(TLV::TLVReader & aAttribut mapping.endpoint_id, ChipLogValueMEI(dataAttributePath.mClusterId), ChipLogValueMEI(dataAttributePath.mAttributeId), err.Format()); } - MatterPostAttributeWriteCallback(dataAttributePath); + DataModelCallbacks::GetInstance()->AttributeOperation(DataModelCallbacks::OperationType::Write, + DataModelCallbacks::OperationOrder::Post, dataAttributePath); } dataAttributePath.mEndpointId = kInvalidEndpointId; @@ -677,6 +683,3 @@ void WriteHandler::MoveToState(const State aTargetState) } // namespace app } // namespace chip - -void __attribute__((weak)) MatterPreAttributeWriteCallback(const chip::app::ConcreteAttributePath & attributePath) {} -void __attribute__((weak)) MatterPostAttributeWriteCallback(const chip::app::ConcreteAttributePath & attributePath) {} diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp index e21080839ed248..91e1e1f52891ec 100644 --- a/src/app/reporting/Engine.cpp +++ b/src/app/reporting/Engine.cpp @@ -86,9 +86,15 @@ Engine::RetrieveClusterData(const SubjectDescriptor & aSubjectDescriptor, bool a { ChipLogDetail(DataManagement, " Cluster %" PRIx32 ", Attribute %" PRIx32 " is dirty", aPath.mClusterId, aPath.mAttributeId); - MatterPreAttributeReadCallback(aPath); + + DataModelCallbacks::GetInstance()->AttributeOperation(DataModelCallbacks::OperationType::Read, + DataModelCallbacks::OperationOrder::Pre, aPath); + ReturnErrorOnFailure(ReadSingleClusterData(aSubjectDescriptor, aIsFabricFiltered, aPath, aAttributeReportIBs, aEncoderState)); - MatterPostAttributeReadCallback(aPath); + + DataModelCallbacks::GetInstance()->AttributeOperation(DataModelCallbacks::OperationType::Read, + DataModelCallbacks::OperationOrder::Post, aPath); + return CHIP_NO_ERROR; } @@ -996,9 +1002,6 @@ void Engine::ScheduleUrgentEventDeliverySync(Optional fabricIndex) } // namespace app } // namespace chip -void __attribute__((weak)) MatterPreAttributeReadCallback(const chip::app::ConcreteAttributePath & attributePath) {} -void __attribute__((weak)) MatterPostAttributeReadCallback(const chip::app::ConcreteAttributePath & attributePath) {} - // TODO: MatterReportingAttributeChangeCallback should just live in libCHIP, // instead of being in ember-compatibility-functions. It does not depend on any // app-specific generated bits. diff --git a/src/app/util/BUILD.gn b/src/app/util/BUILD.gn index dfb2e4c248dc92..dccbc68c0d943e 100644 --- a/src/app/util/BUILD.gn +++ b/src/app/util/BUILD.gn @@ -50,3 +50,15 @@ source_set("af-types") { "${chip_root}/src/protocols/interaction_model", ] } + +source_set("callbacks") { + sources = [ + "MatterCallbacks.cpp", + "MatterCallbacks.h", + ] + + deps = [ + "${chip_root}/src/access", + "${chip_root}/src/app:paths", + ] +} diff --git a/src/app/util/MatterCallbacks.cpp b/src/app/util/MatterCallbacks.cpp new file mode 100644 index 00000000000000..71ab7fdc9df65d --- /dev/null +++ b/src/app/util/MatterCallbacks.cpp @@ -0,0 +1,99 @@ +/* + * 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. + */ +#include "MatterCallbacks.h" + +// The defines below are using link-time callback and should be removed +// +// TODO: applications should be converted to use DataModelCallbacks instead +// of relying on weak linkage +void __attribute__((weak)) MatterPreAttributeReadCallback(const chip::app::ConcreteAttributePath & attributePath) {} +void __attribute__((weak)) MatterPostAttributeReadCallback(const chip::app::ConcreteAttributePath & attributePath) {} +void __attribute__((weak)) MatterPreAttributeWriteCallback(const chip::app::ConcreteAttributePath & attributePath) {} +void __attribute__((weak)) MatterPostAttributeWriteCallback(const chip::app::ConcreteAttributePath & attributePath) {} +CHIP_ERROR __attribute__((weak)) MatterPreCommandReceivedCallback(const chip::app::ConcreteCommandPath & commandPath, + const chip::Access::SubjectDescriptor & subjectDescriptor) +{ + return CHIP_NO_ERROR; +} +void __attribute__((weak)) MatterPostCommandReceivedCallback(const chip::app::ConcreteCommandPath & commandPath, + const chip::Access::SubjectDescriptor & subjectDescriptor) +{} + +namespace chip { +namespace { + +class WeakRedirectCallbacks : public DataModelCallbacks +{ +public: + void AttributeOperation(OperationType operation, OperationOrder order, const chip::app::ConcreteAttributePath & path) override + { + switch (operation) + { + case OperationType::Read: + switch (order) + { + case OperationOrder::Pre: + MatterPreAttributeReadCallback(path); + break; + case OperationOrder::Post: + MatterPostAttributeReadCallback(path); + break; + } + break; + case OperationType::Write: + switch (order) + { + case OperationOrder::Pre: + MatterPreAttributeWriteCallback(path); + break; + case OperationOrder::Post: + MatterPostAttributeWriteCallback(path); + break; + } + break; + } + } + + CHIP_ERROR PreCommandReceived(const chip::app::ConcreteCommandPath & commandPath, + const chip::Access::SubjectDescriptor & subjectDescriptor) override + { + return MatterPreCommandReceivedCallback(commandPath, subjectDescriptor); + } + + void PostCommandReceived(const chip::app::ConcreteCommandPath & commandPath, + const chip::Access::SubjectDescriptor & subjectDescriptor) override + { + + MatterPostCommandReceivedCallback(commandPath, subjectDescriptor); + } +}; + +WeakRedirectCallbacks gWeakCallbacks; +DataModelCallbacks * gInstance = &gWeakCallbacks; + +} // namespace + +DataModelCallbacks * DataModelCallbacks::GetInstance() +{ + return gInstance; +} + +DataModelCallbacks * DataModelCallbacks::SetInstance(DataModelCallbacks * newInstance) +{ + return std::exchange(gInstance, newInstance); +} + +} // namespace chip diff --git a/src/app/util/MatterCallbacks.h b/src/app/util/MatterCallbacks.h index 6a0b69388d67a5..05d1653da84e1f 100644 --- a/src/app/util/MatterCallbacks.h +++ b/src/app/util/MatterCallbacks.h @@ -1,5 +1,4 @@ /* - * * Copyright (c) 2021 Project CHIP Authors * * Licensed under the Apache License, Version 2.0 (the "License"); @@ -14,32 +13,61 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - -// THIS FILE IS GENERATED BY ZAP - #pragma once #include #include #include -void MatterPreAttributeReadCallback(const chip::app::ConcreteAttributePath & attributePath); -void MatterPostAttributeReadCallback(const chip::app::ConcreteAttributePath & attributePath); -void MatterPreAttributeWriteCallback(const chip::app::ConcreteAttributePath & attributePath); -void MatterPostAttributeWriteCallback(const chip::app::ConcreteAttributePath & attributePath); +namespace chip { -/** @brief Matter Pre Command Received - * - * This callback is called once the message has been determined to be a command, and - * before the command is dispatched to the receiver. - */ -CHIP_ERROR MatterPreCommandReceivedCallback(const chip::app::ConcreteCommandPath & commandPath, - const chip::Access::SubjectDescriptor & subjectDescriptor); +/// Allows for application hooks for processing attribute and command operations +class DataModelCallbacks +{ +public: + enum class OperationType + { + Read, + Write + }; -/** @brief Matter Post Command Received - * - * This callback is called once the message has been determined to be a command, but - * after it being dispatched to the receiver. - */ -void MatterPostCommandReceivedCallback(const chip::app::ConcreteCommandPath & commandPath, - const chip::Access::SubjectDescriptor & subjectDescriptor); + enum class OperationOrder + { + Pre, + Post + }; + + virtual ~DataModelCallbacks() = default; + + /// This callback is called on attribute operations: + /// - for reads and writes + /// - both before and after attribute read/writes + /// + /// NOTE: PostRead is only called on read success. + virtual void AttributeOperation(OperationType operation, OperationOrder order, const chip::app::ConcreteAttributePath & path) {} + + /// This callback is called once for every command dispatch, before the dispatch is actually + /// done towards the receiver. + /// + /// This method is called once for every CommandDataIB (i.e. it may be called several times + /// in the case of batch invoke, where a single `InvokeRequestMessage` may contain several + /// CommandDataIB entries). + /// + /// Returning an error here will prevent the command to be dispatched further + virtual CHIP_ERROR PreCommandReceived(const chip::app::ConcreteCommandPath & commandPath, + const chip::Access::SubjectDescriptor & subjectDescriptor) + { + return CHIP_NO_ERROR; + } + + /// This callback is called once the message has been determined to be a command, but + /// after it being dispatched to the receiver. + virtual void PostCommandReceived(const chip::app::ConcreteCommandPath & commandPath, + const chip::Access::SubjectDescriptor & subjectDescriptor) + {} + + static DataModelCallbacks * GetInstance(); + static DataModelCallbacks * SetInstance(DataModelCallbacks * newInstance); +}; + +} // namespace chip From 53273d15a36a24666fdc941b673d62bf38e238eb Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Thu, 21 Mar 2024 15:29:05 -0700 Subject: [PATCH 06/11] [Darwin] MTRDevice should persist data version by cluster (#32674) --- src/darwin/Framework/CHIP/MTRDevice.mm | 131 +++++++++++-- .../Framework/CHIP/MTRDeviceController.mm | 5 + .../CHIP/MTRDeviceControllerDataStore.h | 3 + .../CHIP/MTRDeviceControllerDataStore.mm | 175 +++++++++++++++++- .../Framework/CHIP/MTRDevice_Internal.h | 16 +- .../CHIPTests/MTRPerControllerStorageTests.m | 19 ++ .../TestHelpers/MTRTestDeclarations.h | 3 + 7 files changed, 333 insertions(+), 19 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDevice.mm b/src/darwin/Framework/CHIP/MTRDevice.mm index 493d3a0cb18a95..9e2d1699691f10 100644 --- a/src/darwin/Framework/CHIP/MTRDevice.mm +++ b/src/darwin/Framework/CHIP/MTRDevice.mm @@ -153,6 +153,43 @@ typedef NS_ENUM(NSUInteger, MTRDeviceWorkItemDuplicateTypeID) { MTRDeviceWorkItemDuplicateReadTypeID = 1, }; +@implementation MTRDeviceClusterData + +static NSString * const sDataVersionKey = @"dataVersion"; + ++ (BOOL)supportsSecureCoding +{ + return YES; +} + +- (NSString *)description +{ + return [NSString stringWithFormat:@"", _dataVersion]; +} + +- (nullable instancetype)initWithCoder:(NSCoder *)decoder +{ + self = [super init]; + if (self == nil) { + return nil; + } + + _dataVersion = [decoder decodeObjectOfClass:[NSNumber class] forKey:sDataVersionKey]; + if (_dataVersion != nil && ![_dataVersion isKindOfClass:[NSNumber class]]) { + MTR_LOG_ERROR("MTRDeviceClusterData got %@ for data version, not NSNumber.", _dataVersion); + return nil; + } + + return self; +} + +- (void)encodeWithCoder:(NSCoder *)coder +{ + [coder encodeObject:self.dataVersion forKey:sDataVersionKey]; +} + +@end + @interface MTRDevice () @property (nonatomic, readonly) os_unfair_lock lock; // protects the caches and device state // protects against concurrent time updates by guarding timeUpdateScheduled flag which manages time updates scheduling, @@ -228,6 +265,8 @@ @implementation MTRDevice { NSUInteger _unitTestAttributesReportedSinceLastCheck; #endif BOOL _delegateDeviceCachePrimedCalled; + NSMutableDictionary * _clusterData; + NSMutableDictionary * _clusterDataToPersist; } - (instancetype)initWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceController *)controller @@ -244,6 +283,7 @@ - (instancetype)initWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceControlle _expectedValueCache = [NSMutableDictionary dictionary]; _asyncWorkQueue = [[MTRAsyncWorkQueue alloc] initWithContext:self]; _state = MTRDeviceStateUnknown; + _clusterData = [NSMutableDictionary dictionary]; MTR_LOG_INFO("%@ init with hex nodeID 0x%016llX", self, _nodeID.unsignedLongLongValue); } return self; @@ -777,6 +817,13 @@ - (void)_handleReportEnd _receivingPrimingReport = NO; _estimatedStartTimeFromGeneralDiagnosticsUpTime = nil; + BOOL dataStoreExists = _deviceController.controllerDataStore != nil; + if (dataStoreExists && _clusterDataToPersist.count) { + MTR_LOG_DEFAULT("%@ Storing cluster information (data version) count: %lu", self, static_cast(_clusterDataToPersist.count)); + [_deviceController.controllerDataStore storeClusterData:_clusterDataToPersist forNodeID:_nodeID]; + _clusterDataToPersist = nil; + } + // For unit testing only #ifdef DEBUG id delegate = _weakDelegate.strongObject; @@ -924,19 +971,11 @@ - (void)_handleEventReport:(NSArray *> *)eventRepor NSMutableDictionary * dataVersions = [NSMutableDictionary dictionary]; std::lock_guard lock(_lock); - for (MTRAttributePath * path in _readCache) { - NSDictionary * dataValue = _readCache[path]; - NSNumber * dataVersionNumber = dataValue[MTRDataVersionKey]; - if (dataVersionNumber) { - MTRClusterPath * clusterPath = [MTRClusterPath clusterPathWithEndpointID:path.endpoint clusterID:path.cluster]; - NSNumber * currentDataVersion = dataVersions[clusterPath]; - // Use the highest data version - if (currentDataVersion.unsignedLongValue < dataVersionNumber.unsignedLongValue) { - dataVersions[clusterPath] = dataVersionNumber; - } - } + for (MTRClusterPath * path in _clusterData) { + dataVersions[path] = _clusterData[path].dataVersion; } - MTR_LOG_INFO("%@ _getCachedDataVersions dataVersions count: %lu readCache count: %lu", self, static_cast(dataVersions.count), static_cast(_readCache.count)); + + MTR_LOG_INFO("%@ _getCachedDataVersions dataVersions count: %lu", self, static_cast(dataVersions.count)); return dataVersions; } @@ -1873,9 +1912,45 @@ - (void)_performScheduledExpirationCheck - (BOOL)_attributeDataValue:(NSDictionary *)one isEqualToDataValue:(NSDictionary *)theOther { - // Attribute data-value dictionary should be all standard containers which - // means isEqual: comparisons all the way down, making a deep comparison. - return [one isEqualToDictionary:theOther]; + // Attribute data-value dictionaries are equal if type and value are equal + return [one[MTRTypeKey] isEqual:theOther[MTRTypeKey]] && ((one[MTRValueKey] == theOther[MTRValueKey]) || [one[MTRValueKey] isEqual:theOther[MTRValueKey]]); +} + +// Utility to return data value dictionary without data version +- (NSDictionary *)_dataValueWithoutDataVersion:(NSDictionary *)attributeValue; +{ + if (attributeValue[MTRValueKey]) { + return @{ MTRTypeKey : attributeValue[MTRTypeKey], MTRValueKey : attributeValue[MTRValueKey] }; + } else { + return @{ MTRTypeKey : attributeValue[MTRTypeKey] }; + } +} + +// Update cluster data version and also note the change, so at onReportEnd it can be persisted +- (void)_updateDataVersion:(NSNumber *)dataVersion forClusterPath:(MTRClusterPath *)clusterPath +{ + BOOL dataVersionChanged = NO; + MTRDeviceClusterData * clusterData = _clusterData[clusterPath]; + if (!clusterData) { + clusterData = [[MTRDeviceClusterData alloc] init]; + _clusterData[clusterPath] = clusterData; + dataVersionChanged = YES; + } else if (![clusterData.dataVersion isEqualToNumber:dataVersion]) { + dataVersionChanged = YES; + } + + if (dataVersionChanged) { + clusterData.dataVersion = dataVersion; + + // Set up for persisting if there is data store + BOOL dataStoreExists = _deviceController.controllerDataStore != nil; + if (dataStoreExists) { + if (!_clusterDataToPersist) { + _clusterDataToPersist = [NSMutableDictionary dictionary]; + } + _clusterDataToPersist[clusterPath] = clusterData; + } + } } // assume lock is held @@ -1919,10 +1994,29 @@ - (NSArray *)_getAttributesToReportWithReportedValues:(NSArray *)attributeValues reportChan } } +- (void)setClusterData:(NSDictionary *)clusterData +{ + [_clusterData addEntriesFromDictionary:clusterData]; +} + - (BOOL)deviceCachePrimed { std::lock_guard lock(_lock); diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm index ae6669cdab5add..d51b5cd7712b5c 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceController.mm @@ -935,6 +935,11 @@ - (MTRDevice *)deviceForNodeID:(NSNumber *)nodeID if (attributesFromCache.count) { [deviceToReturn setAttributeValues:attributesFromCache reportChanges:NO]; } + NSDictionary * clusterData = [_controllerDataStore getStoredClusterDataForNodeID:nodeID]; + MTR_LOG_INFO("Loaded %lu cluster data from storage for %@", static_cast(clusterData.count), deviceToReturn); + if (clusterData.count) { + [deviceToReturn setClusterData:clusterData]; + } } return deviceToReturn; diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.h b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.h index 8df10f63e1d27d..90007dbb2df861 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.h +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.h @@ -17,6 +17,7 @@ #import #import #import +#import #if MTR_PER_CONTROLLER_STORAGE_ENABLED #import #else @@ -67,7 +68,9 @@ NS_ASSUME_NONNULL_BEGIN * Storage for MTRDevice attribute read cache. This is local-only storage as an optimization. New controller devices using MTRDevice API can prime their own local cache from devices directly. */ - (nullable NSArray *)getStoredAttributesForNodeID:(NSNumber *)nodeID; +- (nullable NSDictionary *)getStoredClusterDataForNodeID:(NSNumber *)nodeID; - (void)storeAttributeValues:(NSArray *)dataValues forNodeID:(NSNumber *)nodeID; +- (void)storeClusterData:(NSDictionary *)clusterData forNodeID:(NSNumber *)nodeID; - (void)clearStoredAttributesForNodeID:(NSNumber *)nodeID; - (void)clearAllStoredAttributes; diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm index 26d68921591f75..6ed3b00eeae3ee 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm @@ -398,6 +398,28 @@ - (BOOL)_deleteClusterIndexForNodeID:(NSNumber *)nodeID endpointID:(NSNumber *)e return [self _removeAttributeCacheValueForKey:[self _clusterIndexKeyForNodeID:nodeID endpointID:endpointID]]; } +static NSString * sAttributeCacheClusterDataKeyPrefix = @"attrCacheClusterData"; + +- (NSString *)_clusterDataKeyForNodeID:(NSNumber *)nodeID endpointID:(NSNumber *)endpointID clusterID:(NSNumber *)clusterID +{ + return [sAttributeCacheClusterDataKeyPrefix stringByAppendingFormat:@":0x%016llX:%0x04X:0x%08lX", nodeID.unsignedLongLongValue, endpointID.unsignedShortValue, clusterID.unsignedLongValue]; +} + +- (nullable MTRDeviceClusterData *)_fetchClusterDataForNodeID:(NSNumber *)nodeID endpointID:(NSNumber *)endpointID clusterID:(NSNumber *)clusterID +{ + return [self _fetchAttributeCacheValueForKey:[self _clusterDataKeyForNodeID:nodeID endpointID:endpointID clusterID:clusterID] expectedClass:[MTRDeviceClusterData class]]; +} + +- (BOOL)_storeClusterData:(MTRDeviceClusterData *)clusterData forNodeID:(NSNumber *)nodeID endpointID:(NSNumber *)endpointID clusterID:(NSNumber *)clusterID +{ + return [self _storeAttributeCacheValue:clusterData forKey:[self _clusterDataKeyForNodeID:nodeID endpointID:endpointID clusterID:clusterID]]; +} + +- (BOOL)_deleteClusterDataForNodeID:(NSNumber *)nodeID endpointID:(NSNumber *)endpointID clusterID:(NSNumber *)clusterID +{ + return [self _removeAttributeCacheValueForKey:[self _clusterDataKeyForNodeID:nodeID endpointID:endpointID clusterID:clusterID]]; +} + static NSString * sAttributeCacheAttributeIndexKeyPrefix = @"attrCacheAttributeIndex"; - (NSString *)_attributeIndexKeyForNodeID:(NSNumber *)nodeID endpointID:(NSNumber *)endpointID clusterID:(NSNumber *)clusterID @@ -565,16 +587,22 @@ - (void)_pruneEmptyStoredAttributesBranches if (attributeIndex.count != attributeIndexCopy.count) { BOOL success; + BOOL clusterDataSuccess = YES; if (attributeIndexCopy.count) { success = [self _storeAttributeIndex:attributeIndexCopy forNodeID:nodeID endpointID:endpointID clusterID:clusterID]; } else { [clusterIndexCopy removeObject:clusterID]; success = [self _deleteAttributeIndexForNodeID:nodeID endpointID:endpointID clusterID:clusterID]; + clusterDataSuccess = [self _deleteClusterDataForNodeID:nodeID endpointID:endpointID clusterID:clusterID]; } if (!success) { storeFailures++; MTR_LOG_INFO("Store failed in _pruneEmptyStoredAttributesBranches for attributeIndex (%lu) @ node 0x%016llX endpoint %u cluster 0x%08lX", static_cast(attributeIndexCopy.count), nodeID.unsignedLongLongValue, endpointID.unsignedShortValue, clusterID.unsignedLongValue); } + if (!clusterDataSuccess) { + storeFailures++; + MTR_LOG_INFO("Store failed in _pruneEmptyStoredAttributesBranches for clusterData @ node 0x%016llX endpoint %u cluster 0x%08lX", nodeID.unsignedLongLongValue, endpointID.unsignedShortValue, clusterID.unsignedLongValue); + } } } @@ -719,9 +747,11 @@ - (void)_clearStoredAttributesForNodeID:(NSNumber *)nodeID { NSUInteger endpointsClearAttempts = 0; NSUInteger clustersClearAttempts = 0; + NSUInteger clusterDataClearAttempts = 0; NSUInteger attributesClearAttempts = 0; NSUInteger endpointsCleared = 0; NSUInteger clustersCleared = 0; + NSUInteger clusterDataCleared = 0; NSUInteger attributesCleared = 0; // Fetch endpoint index @@ -733,6 +763,7 @@ - (void)_clearStoredAttributesForNodeID:(NSNumber *)nodeID NSArray * clusterIndex = [self _fetchClusterIndexForNodeID:nodeID endpointID:endpointID]; clustersClearAttempts += clusterIndex.count; + clusterDataClearAttempts += clusterIndex.count; // Assuming every cluster has cluster data for (NSNumber * clusterID in clusterIndex) { // Fetch attribute index NSArray * attributeIndex = [self _fetchAttributeIndexForNodeID:nodeID endpointID:endpointID clusterID:clusterID]; @@ -753,6 +784,13 @@ - (void)_clearStoredAttributesForNodeID:(NSNumber *)nodeID } else { clustersCleared++; } + + success = [self _deleteClusterDataForNodeID:nodeID endpointID:endpointID clusterID:clusterID]; + if (!success) { + MTR_LOG_INFO("Delete failed for clusterData @ node 0x%016llX endpoint %u cluster 0x%08lX", nodeID.unsignedLongLongValue, endpointID.unsignedShortValue, clusterID.unsignedLongValue); + } else { + clusterDataCleared++; + } } BOOL success = [self _deleteClusterIndexForNodeID:nodeID endpointID:endpointID]; @@ -768,7 +806,7 @@ - (void)_clearStoredAttributesForNodeID:(NSNumber *)nodeID MTR_LOG_INFO("Delete failed for endpointrIndex @ node 0x%016llX", nodeID.unsignedLongLongValue); } - MTR_LOG_INFO("clearStoredAttributesForNodeID: deleted endpoints %lu/%lu clusters %lu/%lu attributes %lu/%lu", static_cast(endpointsCleared), static_cast(endpointsClearAttempts), static_cast(clustersCleared), static_cast(clustersClearAttempts), static_cast(attributesCleared), static_cast(attributesClearAttempts)); + MTR_LOG_INFO("clearStoredAttributesForNodeID: deleted endpoints %lu/%lu clusters %lu/%lu clusterData %lu/%lu attributes %lu/%lu", static_cast(endpointsCleared), static_cast(endpointsClearAttempts), static_cast(clustersCleared), static_cast(clustersClearAttempts), static_cast(clusterDataCleared), static_cast(clusterDataClearAttempts), static_cast(attributesCleared), static_cast(attributesClearAttempts)); } - (void)clearStoredAttributesForNodeID:(NSNumber *)nodeID @@ -809,6 +847,140 @@ - (void)clearAllStoredAttributes }); } +- (nullable NSDictionary *)getStoredClusterDataForNodeID:(NSNumber *)nodeID +{ + __block NSMutableDictionary * clusterDataToReturn = nil; + dispatch_sync(_storageDelegateQueue, ^{ + // Fetch node index + NSArray * nodeIndex = [self _fetchNodeIndex]; + +#if ATTRIBUTE_CACHE_VERBOSE_LOGGING + MTR_LOG_INFO("Fetch got %lu values for nodeIndex", static_cast(nodeIndex.count)); +#endif + + if (![nodeIndex containsObject:nodeID]) { + // Sanity check and delete if nodeID exists in index + NSArray * endpointIndex = [self _fetchEndpointIndexForNodeID:nodeID]; + if (endpointIndex) { + MTR_LOG_ERROR("Persistent attribute cache contains orphaned entry for nodeID %@ - deleting", nodeID); + [self clearStoredAttributesForNodeID:nodeID]; + } + + MTR_LOG_INFO("Fetch got no value for endpointIndex @ node 0x%016llX", nodeID.unsignedLongLongValue); + clusterDataToReturn = nil; + return; + } + + // Fetch endpoint index + NSArray * endpointIndex = [self _fetchEndpointIndexForNodeID:nodeID]; + +#if ATTRIBUTE_CACHE_VERBOSE_LOGGING + MTR_LOG_INFO("Fetch got %lu values for endpointIndex @ node 0x%016llX", static_cast(endpointIndex.count), nodeID.unsignedLongLongValue); +#endif + + for (NSNumber * endpointID in endpointIndex) { + // Fetch endpoint index + NSArray * clusterIndex = [self _fetchClusterIndexForNodeID:nodeID endpointID:endpointID]; + +#if ATTRIBUTE_CACHE_VERBOSE_LOGGING + MTR_LOG_INFO("Fetch got %lu values for clusterIndex @ node 0x%016llX %u", static_cast(clusterIndex.count), nodeID.unsignedLongLongValue, endpointID.unsignedShortValue); +#endif + + for (NSNumber * clusterID in clusterIndex) { + // Fetch cluster data + MTRDeviceClusterData * clusterData = [self _fetchClusterDataForNodeID:nodeID endpointID:endpointID clusterID:clusterID]; + if (!clusterData) { + MTR_LOG_INFO("Fetch got no value for clusterData @ node 0x%016llX endpoint %u cluster 0x%08lX", nodeID.unsignedLongLongValue, endpointID.unsignedShortValue, clusterID.unsignedLongValue); + continue; + } + +#if ATTRIBUTE_CACHE_VERBOSE_LOGGING + MTR_LOG_INFO("Fetch got clusterData @ node 0x%016llX endpoint %u cluster 0x%08lX", nodeID.unsignedLongLongValue, endpointID.unsignedShortValue, clusterID.unsignedLongValue); +#endif + + MTRClusterPath * path = [MTRClusterPath clusterPathWithEndpointID:endpointID clusterID:clusterID]; + if (!clusterDataToReturn) { + clusterDataToReturn = [NSMutableDictionary dictionary]; + } + clusterDataToReturn[path] = clusterData; + } + } + }); + + return clusterDataToReturn; +} + +- (void)storeClusterData:(NSDictionary *)clusterData forNodeID:(NSNumber *)nodeID +{ + dispatch_async(_storageDelegateQueue, ^{ + NSUInteger storeFailures = 0; + + for (MTRClusterPath * path in clusterData) { + MTRDeviceClusterData * data = clusterData[path]; + +#if ATTRIBUTE_CACHE_VERBOSE_LOGGING + MTR_LOG_INFO("Attempt to store clusterData @ node 0x%016llX endpoint %u cluster 0x%08lX", nodeID.unsignedLongLongValue, path.endpoint.unsignedShortValue, path.cluster.unsignedLongValue); +#endif + + BOOL storeFailed = NO; + // Ensure node index exists + NSArray * nodeIndex = [self _fetchNodeIndex]; + if (!nodeIndex) { + nodeIndex = [NSArray arrayWithObject:nodeID]; + storeFailed = ![self _storeNodeIndex:nodeIndex]; + } else if (![nodeIndex containsObject:nodeID]) { + storeFailed = ![self _storeNodeIndex:[nodeIndex arrayByAddingObject:nodeID]]; + } + if (storeFailed) { + storeFailures++; + MTR_LOG_INFO("Store failed for nodeIndex"); + continue; + } + + // Ensure endpoint index exists + NSArray * endpointIndex = [self _fetchEndpointIndexForNodeID:nodeID]; + if (!endpointIndex) { + endpointIndex = [NSArray arrayWithObject:path.endpoint]; + storeFailed = ![self _storeEndpointIndex:endpointIndex forNodeID:nodeID]; + } else if (![endpointIndex containsObject:path.endpoint]) { + storeFailed = ![self _storeEndpointIndex:[endpointIndex arrayByAddingObject:path.endpoint] forNodeID:nodeID]; + } + if (storeFailed) { + storeFailures++; + MTR_LOG_INFO("Store failed for endpointIndex @ node 0x%016llX", nodeID.unsignedLongLongValue); + continue; + } + + // Ensure cluster index exists + NSArray * clusterIndex = [self _fetchClusterIndexForNodeID:nodeID endpointID:path.endpoint]; + if (!clusterIndex) { + clusterIndex = [NSArray arrayWithObject:path.cluster]; + storeFailed = ![self _storeClusterIndex:clusterIndex forNodeID:nodeID endpointID:path.endpoint]; + } else if (![clusterIndex containsObject:path.cluster]) { + storeFailed = ![self _storeClusterIndex:[clusterIndex arrayByAddingObject:path.cluster] forNodeID:nodeID endpointID:path.endpoint]; + } + if (storeFailed) { + storeFailures++; + MTR_LOG_INFO("Store failed for clusterIndex @ node 0x%016llX endpoint %u", nodeID.unsignedLongLongValue, path.endpoint.unsignedShortValue); + continue; + } + + // Store cluster data + storeFailed = ![self _storeClusterData:data forNodeID:nodeID endpointID:path.endpoint clusterID:path.cluster]; + if (storeFailed) { + storeFailures++; + MTR_LOG_INFO("Store failed for clusterDAta @ node 0x%016llX endpoint %u cluster 0x%08lX", nodeID.unsignedLongLongValue, path.endpoint.unsignedShortValue, path.cluster.unsignedLongValue); + } + } + + // In the rare event that store fails, allow all attribute store attempts to go through and prune empty branches at the end altogether. + if (storeFailures) { + [self _pruneEmptyStoredAttributesBranches]; + MTR_LOG_ERROR("Store failed in -storeAttributeValues:forNodeID: failure count %lu", static_cast(storeFailures)); + } + }); +} + @end @implementation MTRCASESessionResumptionInfo @@ -895,6 +1067,7 @@ - (void)encodeWithCoder:(NSCoder *)coder [NSDictionary class], [NSString class], [MTRAttributePath class], + [MTRDeviceClusterData class], ]]; return sStorageClasses; } diff --git a/src/darwin/Framework/CHIP/MTRDevice_Internal.h b/src/darwin/Framework/CHIP/MTRDevice_Internal.h index bda6cceaace1c2..cb6aa46eb6f4ed 100644 --- a/src/darwin/Framework/CHIP/MTRDevice_Internal.h +++ b/src/darwin/Framework/CHIP/MTRDevice_Internal.h @@ -20,8 +20,7 @@ #import #import "MTRAsyncWorkQueue.h" - -#include +#import "MTRDefines_Internal.h" NS_ASSUME_NONNULL_BEGIN @@ -29,6 +28,15 @@ NS_ASSUME_NONNULL_BEGIN typedef void (^MTRDevicePerformAsyncBlock)(MTRBaseDevice * baseDevice); +/** + * Information about a cluster, currently is just data version + */ +MTR_TESTABLE +@interface MTRDeviceClusterData : NSObject +@property (nonatomic) NSNumber * dataVersion; +// TODO: add cluster attributes in this object, and remove direct attribute storage +@end + @interface MTRDevice () - (instancetype)initWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceController *)controller; @@ -73,6 +81,10 @@ typedef void (^MTRDevicePerformAsyncBlock)(MTRBaseDevice * baseDevice); // reportChanges : if set to YES, attribute reports will also sent to the delegate if new values are different - (void)setAttributeValues:(NSArray *)attributeValues reportChanges:(BOOL)reportChanges; +// Method to insert cluster data +// Currently contains data version information +- (void)setClusterData:(NSDictionary *)clusterData; + @end #pragma mark - Utility for clamping numbers diff --git a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m index efa53d9dabcefb..0fede084217997 100644 --- a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m +++ b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m @@ -20,6 +20,7 @@ #import #import "MTRDeviceTestDelegate.h" +#import "MTRDevice_Internal.h" #import "MTRErrorTestUtils.h" #import "MTRFabricInfoChecker.h" #import "MTRTestDeclarations.h" @@ -1082,6 +1083,19 @@ - (void)test008_TestDataStoreDirect [controller.controllerDataStore storeAttributeValues:testAttributes forNodeID:@(1002)]; [controller.controllerDataStore storeAttributeValues:testAttributes forNodeID:@(1003)]; + MTRDeviceClusterData * testClusterData1 = [[MTRDeviceClusterData alloc] init]; + testClusterData1.dataVersion = @(1); + MTRDeviceClusterData * testClusterData2 = [[MTRDeviceClusterData alloc] init]; + testClusterData2.dataVersion = @(2); + MTRDeviceClusterData * testClusterData3 = [[MTRDeviceClusterData alloc] init]; + testClusterData3.dataVersion = @(3); + NSDictionary * testClusterData = @{ + [MTRClusterPath clusterPathWithEndpointID:@(1) clusterID:@(1)] : testClusterData1, + [MTRClusterPath clusterPathWithEndpointID:@(1) clusterID:@(2)] : testClusterData2, + [MTRClusterPath clusterPathWithEndpointID:@(1) clusterID:@(3)] : testClusterData3, + }; + [controller.controllerDataStore storeClusterData:testClusterData forNodeID:@(1001)]; + // Check values are written and can be fetched NSArray * dataStoreValues = [controller.controllerDataStore getStoredAttributesForNodeID:@(1001)]; XCTAssertEqual(dataStoreValues.count, 9); @@ -1123,6 +1137,11 @@ - (void)test008_TestDataStoreDirect } } + NSDictionary * dataStoreClusterData = [controller.controllerDataStore getStoredClusterDataForNodeID:@(1001)]; + for (MTRClusterPath * path in testClusterData) { + XCTAssertEqualObjects(testClusterData[path].dataVersion, dataStoreClusterData[path].dataVersion); + } + [controller.controllerDataStore clearStoredAttributesForNodeID:@(1001)]; dataStoreValues = [controller.controllerDataStore getStoredAttributesForNodeID:@(1001)]; XCTAssertEqual(dataStoreValues.count, 0); diff --git a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestDeclarations.h b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestDeclarations.h index 02a711ee183470..68abb9a8fa319f 100644 --- a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestDeclarations.h +++ b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestDeclarations.h @@ -22,10 +22,13 @@ NS_ASSUME_NONNULL_BEGIN #pragma mark - Declarations for internal methods +@class MTRDeviceClusterData; // MTRDeviceControllerDataStore.h includes C++ header, and so we need to declare the methods separately @protocol MTRDeviceControllerDataStoreAttributeStoreMethods - (nullable NSArray *)getStoredAttributesForNodeID:(NSNumber *)nodeID; +- (nullable NSDictionary *)getStoredClusterDataForNodeID:(NSNumber *)nodeID; - (void)storeAttributeValues:(NSArray *)dataValues forNodeID:(NSNumber *)nodeID; +- (void)storeClusterData:(NSDictionary *)clusterData forNodeID:(NSNumber *)nodeID; - (void)clearStoredAttributesForNodeID:(NSNumber *)nodeID; - (void)clearAllStoredAttributes; - (void)unitTestPruneEmptyStoredAttributesBranches; From 88afa3361bbb89b1999624016d79b4eb92245427 Mon Sep 17 00:00:00 2001 From: Nivi Sarkar <55898241+nivi-apple@users.noreply.github.com> Date: Thu, 21 Mar 2024 15:31:18 -0700 Subject: [PATCH 07/11] Fix the dnssd code that browses on both the local and srp domains (#32675) * Fix the dnssd code that browses on both the local and srp domains - Fixes the UAF issue with the timer - Resolves critical comments from PR #32631 * Fix GetDomainFromHostName to get the domain name correctly * Restyled by clang-format --------- Co-authored-by: Restyled.io --- src/platform/Darwin/DnssdContexts.cpp | 105 ++++++++++++---------- src/platform/Darwin/DnssdImpl.cpp | 121 ++++++++++++++------------ src/platform/Darwin/DnssdImpl.h | 20 ++--- 3 files changed, 136 insertions(+), 110 deletions(-) diff --git a/src/platform/Darwin/DnssdContexts.cpp b/src/platform/Darwin/DnssdContexts.cpp index c6067a55168e12..a8ae75e34d2eb5 100644 --- a/src/platform/Darwin/DnssdContexts.cpp +++ b/src/platform/Darwin/DnssdContexts.cpp @@ -458,27 +458,35 @@ ResolveContext::ResolveContext(void * cbContext, DnssdResolveCallback cb, chip:: std::shared_ptr && consumerCounterToUse) : browseThatCausedResolve(browseCausingResolve) { - type = ContextType::Resolve; - context = cbContext; - callback = cb; - protocol = GetProtocol(cbAddressType); - instanceName = instanceNameToResolve; - consumerCounter = std::move(consumerCounterToUse); + type = ContextType::Resolve; + context = cbContext; + callback = cb; + protocol = GetProtocol(cbAddressType); + instanceName = instanceNameToResolve; + consumerCounter = std::move(consumerCounterToUse); + hasSrpTimerStarted = false; } ResolveContext::ResolveContext(CommissioningResolveDelegate * delegate, chip::Inet::IPAddressType cbAddressType, const char * instanceNameToResolve, std::shared_ptr && consumerCounterToUse) : browseThatCausedResolve(nullptr) { - type = ContextType::Resolve; - context = delegate; - callback = nullptr; - protocol = GetProtocol(cbAddressType); - instanceName = instanceNameToResolve; - consumerCounter = std::move(consumerCounterToUse); + type = ContextType::Resolve; + context = delegate; + callback = nullptr; + protocol = GetProtocol(cbAddressType); + instanceName = instanceNameToResolve; + consumerCounter = std::move(consumerCounterToUse); + hasSrpTimerStarted = false; } -ResolveContext::~ResolveContext() {} +ResolveContext::~ResolveContext() +{ + if (this->hasSrpTimerStarted) + { + CancelSrpTimer(this); + } +} void ResolveContext::DispatchFailure(const char * errorStr, CHIP_ERROR err) { @@ -526,8 +534,7 @@ void ResolveContext::DispatchSuccess() for (auto interfaceIndex : priorityInterfaceIndices) { - // Try finding interfaces for domains kLocalDot and kOpenThreadDot and delete them. - if (TryReportingResultsForInterfaceIndex(static_cast(interfaceIndex), std::string(kLocalDot))) + if (TryReportingResultsForInterfaceIndex(interfaceIndex)) { if (needDelete) { @@ -536,7 +543,7 @@ void ResolveContext::DispatchSuccess() return; } - if (TryReportingResultsForInterfaceIndex(static_cast(interfaceIndex), std::string(kOpenThreadDot))) + if (TryReportingResultsForInterfaceIndex(interfaceIndex)) { if (needDelete) { @@ -548,7 +555,8 @@ void ResolveContext::DispatchSuccess() for (auto & interface : interfaces) { - if (TryReportingResultsForInterfaceIndex(interface.first.first, interface.first.second)) + auto interfaceId = interface.first.first; + if (TryReportingResultsForInterfaceIndex(interfaceId)) { break; } @@ -560,7 +568,7 @@ void ResolveContext::DispatchSuccess() } } -bool ResolveContext::TryReportingResultsForInterfaceIndex(uint32_t interfaceIndex, std::string domainName) +bool ResolveContext::TryReportingResultsForInterfaceIndex(uint32_t interfaceIndex) { if (interfaceIndex == 0) { @@ -568,36 +576,44 @@ bool ResolveContext::TryReportingResultsForInterfaceIndex(uint32_t interfaceInde return false; } - std::pair interfaceKey = std::make_pair(interfaceIndex, domainName); - auto & interface = interfaces[interfaceKey]; - auto & ips = interface.addresses; - - // Some interface may not have any ips, just ignore them. - if (ips.size() == 0) + std::map, InterfaceInfo>::iterator iter = interfaces.begin(); + while (iter != interfaces.end()) { - return false; - } + std::pair key = iter->first; + if (key.first == interfaceIndex) + { + auto & interface = interfaces[key]; + auto & ips = interface.addresses; - ChipLogProgress(Discovery, "Mdns: Resolve success on interface %" PRIu32, interfaceIndex); + // Some interface may not have any ips, just ignore them. + if (ips.size() == 0) + { + return false; + } - auto & service = interface.service; - auto addresses = Span(ips.data(), ips.size()); - if (nullptr == callback) - { - auto delegate = static_cast(context); - DiscoveredNodeData nodeData; - service.ToDiscoveredNodeData(addresses, nodeData); - delegate->OnNodeDiscovered(nodeData); - } - else - { - callback(context, &service, addresses, CHIP_NO_ERROR); - } + ChipLogProgress(Discovery, "Mdns: Resolve success on interface %" PRIu32, interfaceIndex); - return true; + auto & service = interface.service; + auto addresses = Span(ips.data(), ips.size()); + if (nullptr == callback) + { + auto delegate = static_cast(context); + DiscoveredNodeData nodeData; + service.ToDiscoveredNodeData(addresses, nodeData); + delegate->OnNodeDiscovered(nodeData); + } + else + { + callback(context, &service, addresses, CHIP_NO_ERROR); + } + + return true; + } + } + return false; } -CHIP_ERROR ResolveContext::OnNewAddress(const std::pair interfaceKey, const struct sockaddr * address) +CHIP_ERROR ResolveContext::OnNewAddress(const std::pair & interfaceKey, const struct sockaddr * address) { // If we don't have any information about this interfaceId, just ignore the // address, since it won't be usable anyway without things like the port. @@ -605,7 +621,7 @@ CHIP_ERROR ResolveContext::OnNewAddress(const std::pair i // on the system, because the hostnames we are looking up all end in // ".local". In other words, we can get regular DNS results in here, not // just DNS-SD ones. - uint32_t interfaceId = interfaceKey.first; + auto interfaceId = interfaceKey.first; if (interfaces.find(interfaceKey) == interfaces.end()) { @@ -720,8 +736,7 @@ void ResolveContext::OnNewInterface(uint32_t interfaceId, const char * fullname, } std::pair interfaceKey = std::make_pair(interfaceId, domainFromHostname); - - interfaces.insert(std::make_pair(interfaceKey, std::move(interface))); + interfaces.insert(std::make_pair(std::move(interfaceKey), std::move(interface))); } bool ResolveContext::HasInterface() diff --git a/src/platform/Darwin/DnssdImpl.cpp b/src/platform/Darwin/DnssdImpl.cpp index bf0fc96dc7cc4f..94bbdc477d9b64 100644 --- a/src/platform/Darwin/DnssdImpl.cpp +++ b/src/platform/Darwin/DnssdImpl.cpp @@ -32,8 +32,12 @@ using namespace chip::Dnssd::Internal; namespace { -// The extra time in milliseconds that we will wait for the resolution on the open thread domain to complete. -constexpr uint16_t kOpenThreadTimeoutInMsec = 250; +constexpr char kLocalDot[] = "local."; + +constexpr char kSrpDot[] = "default.service.arpa."; + +// The extra time in milliseconds that we will wait for the resolution on the srp domain to complete. +constexpr uint16_t kSrpTimeoutInMsec = 250; constexpr DNSServiceFlags kRegisterFlags = kDNSServiceFlagsNoAutoRename; constexpr DNSServiceFlags kBrowseFlags = kDNSServiceFlagsShareConnection; @@ -144,59 +148,57 @@ std::string GetDomainFromHostName(const char * hostnameWithDomain) { std::string hostname = std::string(hostnameWithDomain); - // Find the last occurence of '.' - size_t last_pos = hostname.find_last_of("."); - if (last_pos != std::string::npos) - { - // Get a substring without last '.' - std::string substring = hostname.substr(0, last_pos); - - // Find the last occurence of '.' in the substring created above. - size_t pos = substring.find_last_of("."); - if (pos != std::string::npos) - { - // Return the domain name between the last 2 occurences of '.' including the trailing dot'.'. - return std::string(hostname.substr(pos + 1, last_pos)); - } - } - return std::string(); -} + // Find the first occurence of '.' + size_t first_pos = hostname.find("."); -Global MdnsContexts::sInstance; + // if not found, return empty string + VerifyOrReturnValue(first_pos != std::string::npos, std::string()); -namespace { + // Get a substring after the first occurence of '.' to the end of the string + return hostname.substr(first_pos + 1, hostname.size()); +} /** - * @brief Callback that is called when the timeout for resolving on the kOpenThreadDot domain has expired. + * @brief Callback that is called when the timeout for resolving on the kSrpDot domain has expired. * * @param[in] systemLayer The system layer. * @param[in] callbackContext The context passed to the timer callback. */ -void OpenThreadTimerExpiredCallback(System::Layer * systemLayer, void * callbackContext) +void SrpTimerExpiredCallback(System::Layer * systemLayer, void * callbackContext) { - ChipLogProgress(Discovery, "Mdns: Timer expired for resolve to complete on the open thread domain."); + ChipLogProgress(Discovery, "Mdns: Timer expired for resolve to complete on the srp domain."); auto sdCtx = static_cast(callbackContext); VerifyOrDie(sdCtx != nullptr); - - if (sdCtx->hasOpenThreadTimerStarted) - { - sdCtx->Finalize(); - } + sdCtx->Finalize(); } /** - * @brief Starts a timer to wait for the resolution on the kOpenThreadDot domain to happen. + * @brief Starts a timer to wait for the resolution on the kSrpDot domain to happen. * * @param[in] timeoutSeconds The timeout in seconds. * @param[in] ResolveContext The resolve context. */ -void StartOpenThreadTimer(uint16_t timeoutInMSecs, ResolveContext * ctx) +CHIP_ERROR StartSrpTimer(uint16_t timeoutInMSecs, ResolveContext * ctx) { - VerifyOrReturn(ctx != nullptr, ChipLogError(Discovery, "Can't schedule open thread timer since context is null")); - DeviceLayer::SystemLayer().StartTimer(System::Clock::Milliseconds16(timeoutInMSecs), OpenThreadTimerExpiredCallback, - reinterpret_cast(ctx)); + VerifyOrReturnValue(ctx != nullptr, CHIP_ERROR_INCORRECT_STATE); + return DeviceLayer::SystemLayer().StartTimer(System::Clock::Milliseconds16(timeoutInMSecs), SrpTimerExpiredCallback, + reinterpret_cast(ctx)); } +/** + * @brief Cancels the timer that was started to wait for the resolution on the kSrpDot domain to happen. + * + * @param[in] ResolveContext The resolve context. + */ +void CancelSrpTimer(ResolveContext * ctx) +{ + DeviceLayer::SystemLayer().CancelTimer(SrpTimerExpiredCallback, reinterpret_cast(ctx)); +} + +Global MdnsContexts::sInstance; + +namespace { + static void OnRegister(DNSServiceRef sdRef, DNSServiceFlags flags, DNSServiceErrorType err, const char * name, const char * type, const char * domain, void * context) { @@ -248,17 +250,17 @@ CHIP_ERROR Browse(BrowseHandler * sdCtx, uint32_t interfaceId, const char * type auto err = DNSServiceCreateConnection(&sdCtx->serviceRef); VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err)); - // We will browse on both the local domain and the open thread domain. + // We will browse on both the local domain and the srp domain. ChipLogProgress(Discovery, "Browsing for: %s on domain %s", StringOrNullMarker(type), kLocalDot); auto sdRefLocal = sdCtx->serviceRef; // Mandatory copy because of kDNSServiceFlagsShareConnection err = DNSServiceBrowse(&sdRefLocal, kBrowseFlags, interfaceId, type, kLocalDot, OnBrowse, sdCtx); VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err)); - ChipLogProgress(Discovery, "Browsing for: %s on domain %s", StringOrNullMarker(type), kOpenThreadDot); + ChipLogProgress(Discovery, "Browsing for: %s on domain %s", StringOrNullMarker(type), kSrpDot); - DNSServiceRef sdRefOpenThread = sdCtx->serviceRef; // Mandatory copy because of kDNSServiceFlagsShareConnection - err = DNSServiceBrowse(&sdRefOpenThread, kBrowseFlags, interfaceId, type, kOpenThreadDot, OnBrowse, sdCtx); + auto sdRefSrp = sdCtx->serviceRef; // Mandatory copy because of kDNSServiceFlagsShareConnection + err = DNSServiceBrowse(&sdRefSrp, kBrowseFlags, interfaceId, type, kSrpDot, OnBrowse, sdCtx); VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err)); return MdnsContexts::GetInstance().Add(sdCtx, sdCtx->serviceRef); @@ -307,25 +309,37 @@ static void OnGetAddrInfo(DNSServiceRef sdRef, DNSServiceFlags flags, uint32_t i { VerifyOrReturn(sdCtx->HasAddress(), sdCtx->Finalize(kDNSServiceErr_BadState)); - if (domainName.compare(kOpenThreadDot) == 0) + if (domainName.compare(kSrpDot) == 0) { - ChipLogProgress(Discovery, "Mdns: Resolve completed on the open thread domain."); + ChipLogProgress(Discovery, "Mdns: Resolve completed on the srp domain."); + + // Cancel the timer if one has been started + if (sdCtx->hasSrpTimerStarted) + { + CancelSrpTimer(sdCtx); + } sdCtx->Finalize(); } else if (domainName.compare(kLocalDot) == 0) { - ChipLogProgress( - Discovery, - "Mdns: Resolve completed on the local domain. Starting a timer for the open thread resolve to come back"); + ChipLogProgress(Discovery, + "Mdns: Resolve completed on the local domain. Starting a timer for the srp resolve to come back"); - // Usually the resolution on the local domain is quicker than on the open thread domain. We would like to give the - // resolution on the open thread domain around 250 millisecs more to give it a chance to resolve before finalizing + // Usually the resolution on the local domain is quicker than on the srp domain. We would like to give the + // resolution on the srp domain around 250 millisecs more to give it a chance to resolve before finalizing // the resolution. - if (!sdCtx->hasOpenThreadTimerStarted) + if (!sdCtx->hasSrpTimerStarted) { - // Schedule a timer to allow the resolve on OpenThread domain to complete. - StartOpenThreadTimer(kOpenThreadTimeoutInMsec, sdCtx); - sdCtx->hasOpenThreadTimerStarted = true; + // Schedule a timer to allow the resolve on Srp domain to complete. + CHIP_ERROR error = StartSrpTimer(kSrpTimeoutInMsec, sdCtx); + + // If the timer fails to start, finalize the context and return. + if (error != CHIP_NO_ERROR) + { + sdCtx->Finalize(); + return; + } + sdCtx->hasSrpTimerStarted = true; } } } @@ -367,8 +381,7 @@ static void OnResolve(DNSServiceRef sdRef, DNSServiceFlags flags, uint32_t inter if (!sdCtx->isResolveRequested) { GetAddrInfo(sdCtx); - sdCtx->isResolveRequested = true; - sdCtx->hasOpenThreadTimerStarted = false; + sdCtx->isResolveRequested = true; } } } @@ -382,13 +395,13 @@ static CHIP_ERROR Resolve(ResolveContext * sdCtx, uint32_t interfaceId, chip::In auto err = DNSServiceCreateConnection(&sdCtx->serviceRef); VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err)); - // Similar to browse, will try to resolve using both the local domain and the open thread domain. + // Similar to browse, will try to resolve using both the local domain and the srp domain. auto sdRefLocal = sdCtx->serviceRef; // Mandatory copy because of kDNSServiceFlagsShareConnection err = DNSServiceResolve(&sdRefLocal, kResolveFlags, interfaceId, name, type, kLocalDot, OnResolve, sdCtx); VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err)); - auto sdRefOpenThread = sdCtx->serviceRef; // Mandatory copy because of kDNSServiceFlagsShareConnection - err = DNSServiceResolve(&sdRefOpenThread, kResolveFlags, interfaceId, name, type, kOpenThreadDot, OnResolve, sdCtx); + auto sdRefSrp = sdCtx->serviceRef; // Mandatory copy because of kDNSServiceFlagsShareConnection + err = DNSServiceResolve(&sdRefSrp, kResolveFlags, interfaceId, name, type, kSrpDot, OnResolve, sdCtx); VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err)); auto retval = MdnsContexts::GetInstance().Add(sdCtx, sdCtx->serviceRef); diff --git a/src/platform/Darwin/DnssdImpl.h b/src/platform/Darwin/DnssdImpl.h index 713a465c236afc..8a9853082b59ee 100644 --- a/src/platform/Darwin/DnssdImpl.h +++ b/src/platform/Darwin/DnssdImpl.h @@ -27,15 +27,17 @@ #include #include -constexpr char kLocalDot[] = "local."; - -constexpr char kOpenThreadDot[] = "default.service.arpa."; - namespace chip { namespace Dnssd { +struct BrowseWithDelegateContext; +struct RegisterContext; +struct ResolveContext; + std::string GetDomainFromHostName(const char * hostname); +void CancelSrpTimer(ResolveContext * ctx); + enum class ContextType { Register, @@ -62,10 +64,6 @@ struct GenericContext CHIP_ERROR FinalizeInternal(const char * errorStr, CHIP_ERROR err); }; -struct BrowseWithDelegateContext; -struct RegisterContext; -struct ResolveContext; - class MdnsContexts { public: @@ -239,7 +237,7 @@ struct ResolveContext : public GenericContext bool isResolveRequested = false; std::shared_ptr consumerCounter; BrowseContext * const browseThatCausedResolve; // Can be null - bool hasOpenThreadTimerStarted = false; + bool hasSrpTimerStarted = false; // browseCausingResolve can be null. ResolveContext(void * cbContext, DnssdResolveCallback cb, chip::Inet::IPAddressType cbAddressType, @@ -252,7 +250,7 @@ struct ResolveContext : public GenericContext void DispatchFailure(const char * errorStr, CHIP_ERROR err) override; void DispatchSuccess() override; - CHIP_ERROR OnNewAddress(const std::pair interfaceKey, const struct sockaddr * address); + CHIP_ERROR OnNewAddress(const std::pair & interfaceKey, const struct sockaddr * address); bool HasAddress(); void OnNewInterface(uint32_t interfaceId, const char * fullname, const char * hostname, uint16_t port, uint16_t txtLen, @@ -266,7 +264,7 @@ struct ResolveContext : public GenericContext * Returns true if information was reported, false if not (e.g. if there * were no IP addresses, etc). */ - bool TryReportingResultsForInterfaceIndex(uint32_t interfaceIndex, std::string domainName); + bool TryReportingResultsForInterfaceIndex(uint32_t interfaceIndex); }; } // namespace Dnssd From ea2953135a957b74eb318b862fa1b8403ed0765c Mon Sep 17 00:00:00 2001 From: Justin Wood Date: Thu, 21 Mar 2024 15:31:38 -0700 Subject: [PATCH 08/11] Making this new protocol optional, so as to not break clients (#32676) --- .../Framework/CHIP/MTRDeviceController+XPC.h | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceController+XPC.h b/src/darwin/Framework/CHIP/MTRDeviceController+XPC.h index 46543090951080..23844afb9afb33 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceController+XPC.h +++ b/src/darwin/Framework/CHIP/MTRDeviceController+XPC.h @@ -94,14 +94,6 @@ typedef void (^MTRValuesHandler)(id _Nullable values, NSError * _Nullable error) */ @protocol MTRDeviceControllerServerProtocol -@optional -/** - * Gets device controller ID corresponding to a specific fabric ID - */ -- (void)getDeviceControllerWithFabricId:(uint64_t)fabricId - completion:(MTRDeviceControllerGetterHandler)completion - MTR_DEPRECATED("This never called.", ios(16.1, 16.4), macos(13.0, 13.3), watchos(9.1, 9.4), tvos(16.1, 16.4)); - @required /** * Gets any available device controller ID @@ -184,6 +176,15 @@ typedef void (^MTRValuesHandler)(id _Nullable values, NSError * _Nullable error) attributeId:(NSNumber * _Nullable)attributeId completion:(MTRValuesHandler)completion; +@optional + +/** + * Gets device controller ID corresponding to a specific fabric ID + */ +- (void)getDeviceControllerWithFabricId:(uint64_t)fabricId + completion:(MTRDeviceControllerGetterHandler)completion + MTR_DEPRECATED("This never called.", ios(16.1, 16.4), macos(13.0, 13.3), watchos(9.1, 9.4), tvos(16.1, 16.4)); + /** * Requests downloading some logs */ From 049c529592e81f5fce73a1d2171d5957680407a2 Mon Sep 17 00:00:00 2001 From: pankore <86098180+pankore@users.noreply.github.com> Date: Fri, 22 Mar 2024 21:08:30 +0800 Subject: [PATCH 09/11] [Ameba] Update Docker image (#32683) --- integrations/docker/images/base/chip-build/version | 2 +- integrations/docker/images/stage-2/chip-build-ameba/Dockerfile | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/integrations/docker/images/base/chip-build/version b/integrations/docker/images/base/chip-build/version index 318ee31de299a4..3e7d598f90dadd 100644 --- a/integrations/docker/images/base/chip-build/version +++ b/integrations/docker/images/base/chip-build/version @@ -1 +1 @@ -41 : [nrfconnect] Update nRF Connect SDK version. +42 : [Ameba] Update Ameba System time diff --git a/integrations/docker/images/stage-2/chip-build-ameba/Dockerfile b/integrations/docker/images/stage-2/chip-build-ameba/Dockerfile index f1d39bb7024d73..08c0cf878392fd 100644 --- a/integrations/docker/images/stage-2/chip-build-ameba/Dockerfile +++ b/integrations/docker/images/stage-2/chip-build-ameba/Dockerfile @@ -4,7 +4,7 @@ LABEL org.opencontainers.image.source https://github.com/project-chip/connectedh # Setup Ameba ARG AMEBA_DIR=/opt/ameba -ARG TAG_NAME=ameba_update_2023_12_15 +ARG TAG_NAME=ameba_update_2024_03_22 RUN set -x \ && apt-get update \ && mkdir ${AMEBA_DIR} \ From 1061ff351ae2f17bcf4e692e1302c63d06743356 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 22 Mar 2024 09:24:47 -0400 Subject: [PATCH 10/11] Fix google-api-core (#32687) --- scripts/setup/constraints.txt | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/scripts/setup/constraints.txt b/scripts/setup/constraints.txt index 90b80f7588af5b..5d0ca805416433 100644 --- a/scripts/setup/constraints.txt +++ b/scripts/setup/constraints.txt @@ -290,3 +290,10 @@ setuptools==68.0.0 # via # pip-tools # west + +# Manual edits: + +# Higher versions depend on proto-plus, which break +# nanopb code generation (due to name conflict of the 'proto' module) +google-api-core==2.17.0 + From 0f9542b5c5a14d18b8f554f6c433ae34de69446a Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 22 Mar 2024 09:51:30 -0400 Subject: [PATCH 11/11] Switch cloudbuild to image version 41 (#32688) --- integrations/cloudbuild/build-all.yaml | 6 +++--- integrations/cloudbuild/chef.yaml | 6 +++--- integrations/cloudbuild/smoke-test.yaml | 14 +++++++------- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/integrations/cloudbuild/build-all.yaml b/integrations/cloudbuild/build-all.yaml index cdefbcf1ddbe15..8a625883594c52 100644 --- a/integrations/cloudbuild/build-all.yaml +++ b/integrations/cloudbuild/build-all.yaml @@ -6,7 +6,7 @@ steps: - "--init" - "--recursive" id: Submodules - - name: "ghcr.io/project-chip/chip-build-vscode:36" + - name: "ghcr.io/project-chip/chip-build-vscode:41" env: - PW_ENVIRONMENT_ROOT=/pwenv args: @@ -21,7 +21,7 @@ steps: path: /pwenv timeout: 900s - - name: "ghcr.io/project-chip/chip-build-vscode:36" + - name: "ghcr.io/project-chip/chip-build-vscode:41" env: - PW_ENVIRONMENT_ROOT=/pwenv args: @@ -85,7 +85,7 @@ steps: --target k32w-shell build --create-archives /workspace/artifacts/ - - name: "ghcr.io/project-chip/chip-build-vscode:36" + - name: "ghcr.io/project-chip/chip-build-vscode:41" env: - PW_ENVIRONMENT_ROOT=/pwenv args: diff --git a/integrations/cloudbuild/chef.yaml b/integrations/cloudbuild/chef.yaml index 8dcf827d7e56aa..ecadd504eda617 100644 --- a/integrations/cloudbuild/chef.yaml +++ b/integrations/cloudbuild/chef.yaml @@ -1,5 +1,5 @@ steps: - - name: "ghcr.io/project-chip/chip-build-vscode:35" + - name: "ghcr.io/project-chip/chip-build-vscode:41" env: - PW_ENVIRONMENT_ROOT=/pwenv args: @@ -12,7 +12,7 @@ steps: path: /pwenv timeout: 2700s - - name: "ghcr.io/project-chip/chip-build-vscode:35" + - name: "ghcr.io/project-chip/chip-build-vscode:41" env: - PW_ENVIRONMENT_ROOT=/pwenv args: @@ -27,7 +27,7 @@ steps: - name: pwenv path: /pwenv - - name: "ghcr.io/project-chip/chip-build-vscode:35" + - name: "ghcr.io/project-chip/chip-build-vscode:41" env: - PW_ENVIRONMENT_ROOT=/pwenv args: diff --git a/integrations/cloudbuild/smoke-test.yaml b/integrations/cloudbuild/smoke-test.yaml index 87e22dbbea99d0..2d88ff2dc73da3 100644 --- a/integrations/cloudbuild/smoke-test.yaml +++ b/integrations/cloudbuild/smoke-test.yaml @@ -1,5 +1,5 @@ steps: - - name: "ghcr.io/project-chip/chip-build-vscode:36" + - name: "ghcr.io/project-chip/chip-build-vscode:41" entrypoint: "bash" args: - "-c" @@ -7,7 +7,7 @@ steps: git config --global --add safe.directory "*" git submodule update --init --recursive id: Submodules - - name: "ghcr.io/project-chip/chip-build-vscode:36" + - name: "ghcr.io/project-chip/chip-build-vscode:41" env: - PW_ENVIRONMENT_ROOT=/pwenv args: @@ -22,7 +22,7 @@ steps: path: /pwenv timeout: 900s - - name: "ghcr.io/project-chip/chip-build-vscode:36" + - name: "ghcr.io/project-chip/chip-build-vscode:41" id: ESP32 env: - PW_ENVIRONMENT_ROOT=/pwenv @@ -43,7 +43,7 @@ steps: volumes: - name: pwenv path: /pwenv - - name: "ghcr.io/project-chip/chip-build-vscode:36" + - name: "ghcr.io/project-chip/chip-build-vscode:41" id: NRFConnect env: - PW_ENVIRONMENT_ROOT=/pwenv @@ -64,7 +64,7 @@ steps: - name: pwenv path: /pwenv - - name: "ghcr.io/project-chip/chip-build-vscode:36" + - name: "ghcr.io/project-chip/chip-build-vscode:41" id: EFR32 env: - PW_ENVIRONMENT_ROOT=/pwenv @@ -86,7 +86,7 @@ steps: - name: pwenv path: /pwenv - - name: "ghcr.io/project-chip/chip-build-vscode:36" + - name: "ghcr.io/project-chip/chip-build-vscode:41" id: Linux env: - PW_ENVIRONMENT_ROOT=/pwenv @@ -139,7 +139,7 @@ steps: - name: pwenv path: /pwenv - - name: "ghcr.io/project-chip/chip-build-vscode:36" + - name: "ghcr.io/project-chip/chip-build-vscode:41" id: Android env: - PW_ENVIRONMENT_ROOT=/pwenv