From 3608fdae5232ceb0851263e4138a5ecb0e2fca27 Mon Sep 17 00:00:00 2001 From: Tennessee Carmel-Veilleux Date: Tue, 28 Jun 2022 14:27:46 -0400 Subject: [PATCH] Apply review comments --- src/app/FailSafeContext.cpp | 2 +- src/app/FailSafeContext.h | 3 ++- .../general-commissioning-server.cpp | 4 +++- src/app/server/CommissioningWindowManager.cpp | 2 +- src/app/tests/TestFailSafeContext.cpp | 4 ++-- src/credentials/FabricTable.cpp | 2 +- src/credentials/tests/TestFabricTable.cpp | 7 ++++--- .../GenericNetworkCommissioningThreadDriver.cpp | 10 ++-------- 8 files changed, 16 insertions(+), 18 deletions(-) diff --git a/src/app/FailSafeContext.cpp b/src/app/FailSafeContext.cpp index bf69ff360ee886..f85e538d2e5543 100644 --- a/src/app/FailSafeContext.cpp +++ b/src/app/FailSafeContext.cpp @@ -84,7 +84,7 @@ void FailSafeContext::ScheduleFailSafeCleanup(FabricIndex fabricIndex, bool addN PlatformMgr().ScheduleWork(HandleDisarmFailSafe, reinterpret_cast(this)); } -CHIP_ERROR FailSafeContext::ArmFailSafe(FabricIndex accessingFabricIndex, uint16_t expiryLengthSeconds) +CHIP_ERROR FailSafeContext::ArmFailSafe(FabricIndex accessingFabricIndex, System::Clock::Seconds16 expiryLengthSeconds) { CHIP_ERROR err = CHIP_NO_ERROR; bool cancelTimersIfError = false; diff --git a/src/app/FailSafeContext.h b/src/app/FailSafeContext.h index 4cbc3ba897bcff..e2759d85e71f06 100644 --- a/src/app/FailSafeContext.h +++ b/src/app/FailSafeContext.h @@ -26,6 +26,7 @@ #include #include #include +#include namespace chip { namespace app { @@ -41,7 +42,7 @@ class FailSafeContext * when the fail-safe timer is currently armed, the currently-running fail-safe timer will * first be cancelled, then the fail-safe timer will be re-armed. */ - CHIP_ERROR ArmFailSafe(FabricIndex accessingFabricIndex, uint16_t expiryLengthSeconds); + CHIP_ERROR ArmFailSafe(FabricIndex accessingFabricIndex, System::Clock::Seconds16 expiryLengthSeconds); /** * @brief Cleanly disarm failsafe timer, such as on CommissioningComplete diff --git a/src/app/clusters/general-commissioning-server/general-commissioning-server.cpp b/src/app/clusters/general-commissioning-server/general-commissioning-server.cpp index 1f71bc83a10cef..a4eddf662406b0 100644 --- a/src/app/clusters/general-commissioning-server/general-commissioning-server.cpp +++ b/src/app/clusters/general-commissioning-server/general-commissioning-server.cpp @@ -196,7 +196,9 @@ bool emberAfGeneralCommissioningClusterArmFailSafeCallback(app::CommandHandler * } else { - CheckSuccess(failSafeContext.ArmFailSafe(accessingFabricIndex, commandData.expiryLengthSeconds), Failure); + CheckSuccess( + failSafeContext.ArmFailSafe(accessingFabricIndex, System::Clock::Seconds16(commandData.expiryLengthSeconds)), + Failure); Breadcrumb::Set(commandPath.mEndpointId, commandData.breadcrumb); response.errorCode = CommissioningError::kOk; commandObj->AddResponse(commandPath, response); diff --git a/src/app/server/CommissioningWindowManager.cpp b/src/app/server/CommissioningWindowManager.cpp index 7c48b1825fdb29..ef876c9d55103b 100644 --- a/src/app/server/CommissioningWindowManager.cpp +++ b/src/app/server/CommissioningWindowManager.cpp @@ -170,7 +170,7 @@ void CommissioningWindowManager::OnSessionEstablished(const SessionHandle & sess } else { - err = failSafeContext.ArmFailSafe(kUndefinedFabricId, 60); + err = failSafeContext.ArmFailSafe(kUndefinedFabricId, System::Clock::Seconds16(60)); if (err != CHIP_NO_ERROR) { ChipLogError(AppServer, "Error arming failsafe on PASE session establishment completion"); diff --git a/src/app/tests/TestFailSafeContext.cpp b/src/app/tests/TestFailSafeContext.cpp index 85d6e096da0207..9e5d11dc320b73 100644 --- a/src/app/tests/TestFailSafeContext.cpp +++ b/src/app/tests/TestFailSafeContext.cpp @@ -60,7 +60,7 @@ static void TestFailSafeContext_ArmFailSafe(nlTestSuite * inSuite, void * inCont chip::app::FailSafeContext failSafeContext; - err = failSafeContext.ArmFailSafe(kTestAccessingFabricIndex1, 1); + err = failSafeContext.ArmFailSafe(kTestAccessingFabricIndex1, System::Clock::Seconds16(1)); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); NL_TEST_ASSERT(inSuite, failSafeContext.IsFailSafeArmed() == true); NL_TEST_ASSERT(inSuite, failSafeContext.GetFabricIndex() == kTestAccessingFabricIndex1); @@ -77,7 +77,7 @@ static void TestFailSafeContext_NocCommandInvoked(nlTestSuite * inSuite, void * chip::app::FailSafeContext failSafeContext; - err = failSafeContext.ArmFailSafe(kTestAccessingFabricIndex1, 1); + err = failSafeContext.ArmFailSafe(kTestAccessingFabricIndex1, System::Clock::Seconds16(1)); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); NL_TEST_ASSERT(inSuite, failSafeContext.GetFabricIndex() == kTestAccessingFabricIndex1); diff --git a/src/credentials/FabricTable.cpp b/src/credentials/FabricTable.cpp index 743149ef20a0f3..a91feb68bdcbfb 100644 --- a/src/credentials/FabricTable.cpp +++ b/src/credentials/FabricTable.cpp @@ -1848,7 +1848,7 @@ CHIP_ERROR FabricTable::CommitPendingFabricData() { if (mStateFlags.Has(StateFlags::kAbortCommitForTest)) { - // Clear state so that shutdown doesn't attemp clean-up + // Clear state so that shutdown doesn't attempt clean-up mStateFlags.ClearAll(); mFabricIndexWithPendingState = kUndefinedFabricIndex; mPendingFabric.Reset(); diff --git a/src/credentials/tests/TestFabricTable.cpp b/src/credentials/tests/TestFabricTable.cpp index 724217f5385087..d2daa0b0c2fc4d 100644 --- a/src/credentials/tests/TestFabricTable.cpp +++ b/src/credentials/tests/TestFabricTable.cpp @@ -1885,8 +1885,9 @@ void TestCommitMarker(nlTestSuite * inSuite, void * inContext) fabricTable.SetForceAbortCommitForTest(true); NL_TEST_ASSERT(inSuite, fabricTable.CommitPendingFabricData() == CHIP_ERROR_INTERNAL); - // Check that there are more keys now, partially committed. - NL_TEST_ASSERT(inSuite, storage.GetNumKeys() > numStorageKeysAfterFirstAdd); + // Check that there are more keys now, partially committed: at least a Commit Marker (+1) + // and some more keys from the aborted process. + NL_TEST_ASSERT(inSuite, storage.GetNumKeys() > (numStorageKeysAfterFirstAdd + 1)); } } @@ -1919,7 +1920,7 @@ void TestCommitMarker(nlTestSuite * inSuite, void * inContext) // Make sure that all other pending storage got deleted NL_TEST_ASSERT(inSuite, storage.GetNumKeys() == numStorageKeysAfterFirstAdd); - // Verify we canm only see 1 fabric with the iterator + // Verify we can only see 1 fabric with the iterator { size_t numFabricsIterated = 0; bool saw1 = false; diff --git a/src/platform/OpenThread/GenericNetworkCommissioningThreadDriver.cpp b/src/platform/OpenThread/GenericNetworkCommissioningThreadDriver.cpp index 08253f9430d38b..514df51585abdc 100644 --- a/src/platform/OpenThread/GenericNetworkCommissioningThreadDriver.cpp +++ b/src/platform/OpenThread/GenericNetworkCommissioningThreadDriver.cpp @@ -86,15 +86,9 @@ CHIP_ERROR GenericThreadDriver::RevertConfiguration() // If no backup could be found, it means that the network configuration has not been modified // since the fail-safe was armed, so return with no error. ReturnErrorCodeIf(error == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND, CHIP_NO_ERROR); + ReturnErrorOnFailure(error); - if (error == CHIP_NO_ERROR) - { - ChipLogError(NetworkProvisioning, "Found Thread configuration backup: reverting configuration"); - } - else - { - ReturnErrorOnFailure(error); - } + ChipLogError(NetworkProvisioning, "Found Thread configuration backup: reverting configuration"); // Not all KVS implementations support zero-length values, so handle a special value representing an empty dataset. ByteSpan dataset(datasetBytes, datasetLength);