Skip to content

Commit dd5f6b7

Browse files
[ICD] Add counter invalidation events to the ICD Test Event trigger handler (#33174)
* Add counter invalidation events to the ICD Test Event trigger handler * add issue number * address review comments * add missing dependency * Update counter interfaces * Update CheckInCounter to fit counter changes * Fix openiot ci * Add missing type * Fix templates for uint8 type * Apply suggestions from code review Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Fix TestICDManager with counter API * Cover rollover case for AdvanceBy and add unit test to validate behavior * Update comment * restyle * fix macro in template headeR * Update src/lib/support/PersistedCounter.h Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * restyle --------- Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
1 parent a4b59cb commit dd5f6b7

16 files changed

+633
-212
lines changed

src/BUILD.gn

+1
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,7 @@ if (chip_build_tests) {
9999
"${chip_root}/src/lib/support/tests",
100100
"${chip_root}/src/lib/support/tests:tests_nltest",
101101
"${chip_root}/src/protocols/secure_channel/tests",
102+
"${chip_root}/src/protocols/secure_channel/tests:tests_nltest",
102103
"${chip_root}/src/system/tests",
103104
"${chip_root}/src/transport/tests",
104105
]

src/app/icd/server/ICDManager.cpp

+12-2
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,10 @@
3030
namespace {
3131
enum class ICDTestEventTriggerEvent : uint64_t
3232
{
33-
kAddActiveModeReq = 0x0046'0000'00000001,
34-
kRemoveActiveModeReq = 0x0046'0000'00000002,
33+
kAddActiveModeReq = 0x0046'0000'00000001,
34+
kRemoveActiveModeReq = 0x0046'0000'00000002,
35+
kInvalidateHalfCounterValues = 0x0046'0000'00000003,
36+
kInvalidateAllCounterValues = 0x0046'0000'00000004,
3537
};
3638
} // namespace
3739

@@ -666,6 +668,14 @@ CHIP_ERROR ICDManager::HandleEventTrigger(uint64_t eventTrigger)
666668
case ICDTestEventTriggerEvent::kRemoveActiveModeReq:
667669
SetKeepActiveModeRequirements(KeepActiveFlag::kTestEventTriggerActiveMode, false);
668670
break;
671+
#if CHIP_CONFIG_ENABLE_ICD_CIP
672+
case ICDTestEventTriggerEvent::kInvalidateHalfCounterValues:
673+
err = ICDConfigurationData::GetInstance().GetICDCounter().InvalidateHalfCheckInCouterValues();
674+
break;
675+
case ICDTestEventTriggerEvent::kInvalidateAllCounterValues:
676+
err = ICDConfigurationData::GetInstance().GetICDCounter().InvalidateAllCheckInCounterValues();
677+
break;
678+
#endif // CHIP_CONFIG_ENABLE_ICD_CIP
669679
default:
670680
err = CHIP_ERROR_INVALID_ARGUMENT;
671681
break;

src/app/tests/TestICDManager.cpp

+50-2
Original file line numberDiff line numberDiff line change
@@ -70,8 +70,10 @@ constexpr uint8_t kKeyBuffer2b[] = {
7070
// Taken from the ICDManager Implementation
7171
enum class ICDTestEventTriggerEvent : uint64_t
7272
{
73-
kAddActiveModeReq = 0x0046'0000'00000001,
74-
kRemoveActiveModeReq = 0x0046'0000'00000002,
73+
kAddActiveModeReq = 0x0046'0000'00000001,
74+
kRemoveActiveModeReq = 0x0046'0000'00000002,
75+
kInvalidateHalfCounterValues = 0x0046'0000'00000003,
76+
kInvalidateAllCounterValues = 0x0046'0000'00000004,
7577
};
7678

7779
class TestICDStateObserver : public app::ICDStateObserver
@@ -742,6 +744,48 @@ class TestICDManager
742744
NL_TEST_ASSERT(aSuite, ctx->mICDManager.mOperationalState == ICDManager::OperationalState::IdleMode);
743745
}
744746

747+
static void TestHandleTestEventTriggerInvalidateHalfCounterValues(nlTestSuite * aSuite, void * aContext)
748+
{
749+
TestContext * ctx = static_cast<TestContext *>(aContext);
750+
751+
constexpr uint32_t startValue = 1;
752+
constexpr uint32_t expectedValue = 2147483648;
753+
754+
// Set starting value
755+
uint32_t currentValue = ICDConfigurationData::GetInstance().GetICDCounter().GetValue();
756+
uint32_t delta = startValue - currentValue;
757+
758+
NL_TEST_ASSERT(aSuite, ICDConfigurationData::GetInstance().GetICDCounter().AdvanceBy(delta) == CHIP_NO_ERROR);
759+
NL_TEST_ASSERT(aSuite, ICDConfigurationData::GetInstance().GetICDCounter().GetValue() == startValue);
760+
761+
// Trigger ICD kInvalidateHalfCounterValues event
762+
ctx->mICDManager.HandleEventTrigger(static_cast<uint64_t>(ICDTestEventTriggerEvent::kInvalidateHalfCounterValues));
763+
764+
// Validate counter has the expected value
765+
NL_TEST_ASSERT(aSuite, ICDConfigurationData::GetInstance().GetICDCounter().GetValue() == expectedValue);
766+
}
767+
768+
static void TestHandleTestEventTriggerInvalidateAllCounterValues(nlTestSuite * aSuite, void * aContext)
769+
{
770+
TestContext * ctx = static_cast<TestContext *>(aContext);
771+
772+
constexpr uint32_t startValue = 105;
773+
constexpr uint32_t expectedValue = 104;
774+
775+
// Set starting value
776+
uint32_t currentValue = ICDConfigurationData::GetInstance().GetICDCounter().GetValue();
777+
uint32_t delta = startValue - currentValue;
778+
779+
NL_TEST_ASSERT(aSuite, ICDConfigurationData::GetInstance().GetICDCounter().AdvanceBy(delta) == CHIP_NO_ERROR);
780+
NL_TEST_ASSERT(aSuite, ICDConfigurationData::GetInstance().GetICDCounter().GetValue() == startValue);
781+
782+
// Trigger ICD kInvalidateAllCounterValues event
783+
ctx->mICDManager.HandleEventTrigger(static_cast<uint64_t>(ICDTestEventTriggerEvent::kInvalidateAllCounterValues));
784+
785+
// Validate counter has the expected value
786+
NL_TEST_ASSERT(aSuite, ICDConfigurationData::GetInstance().GetICDCounter().GetValue() == expectedValue);
787+
}
788+
745789
/**
746790
* @brief Test verifies when OnEnterIdleMode is called during normal operations.
747791
* Without the ActiveMode timer being extended
@@ -1083,6 +1127,10 @@ static const nlTest sTests[] = {
10831127
NL_TEST_DEF("TestICDStayActive", TestICDManager::TestICDMStayActive),
10841128
NL_TEST_DEF("TestShouldCheckInMsgsBeSentAtActiveModeFunction", TestICDManager::TestShouldCheckInMsgsBeSentAtActiveModeFunction),
10851129
NL_TEST_DEF("TestHandleTestEventTriggerActiveModeReq", TestICDManager::TestHandleTestEventTriggerActiveModeReq),
1130+
NL_TEST_DEF("TestHandleTestEventTriggerInvalidateHalfCounterValues",
1131+
TestICDManager::TestHandleTestEventTriggerInvalidateHalfCounterValues),
1132+
NL_TEST_DEF("TestHandleTestEventTriggerInvalidateAllCounterValues",
1133+
TestICDManager::TestHandleTestEventTriggerInvalidateAllCounterValues),
10861134
NL_TEST_DEF("TestICDStateObserverOnEnterIdleModeActiveModeDuration",
10871135
TestICDManager::TestICDStateObserverOnEnterIdleModeActiveModeDuration),
10881136
NL_TEST_DEF("TestICDStateObserverOnEnterIdleModeActiveModeThreshold",

src/lib/support/CHIPCounter.h

+27-10
Original file line numberDiff line numberDiff line change
@@ -45,16 +45,23 @@ class Counter
4545
virtual ~Counter() {}
4646

4747
/**
48-
* @brief
49-
* Advance the value of the counter.
48+
* @brief Advance the value of the counter.
5049
*
5150
* @return A CHIP error code if anything failed, CHIP_NO_ERROR otherwise.
5251
*/
5352
virtual CHIP_ERROR Advance() = 0;
5453

5554
/**
56-
* @brief
57-
* Get the current value of the counter.
55+
* @brief Advances the current counter value by N
56+
*
57+
* @param value value of N
58+
*
59+
* @return A CHIP error code if anything failed, CHIP_NO_ERROR otherwise.
60+
*/
61+
virtual CHIP_ERROR AdvanceBy(T value) = 0;
62+
63+
/**
64+
* @brief Get the current value of the counter.
5865
*
5966
* @return The current value of the counter.
6067
*/
@@ -76,8 +83,7 @@ class MonotonicallyIncreasingCounter : public Counter<T>
7683
~MonotonicallyIncreasingCounter() override{};
7784

7885
/**
79-
* @brief
80-
* Initialize a MonotonicallyIncreasingCounter object.
86+
* @brief Initialize a MonotonicallyIncreasingCounter object.
8187
*
8288
* @param[in] aStartValue The starting value of the counter.
8389
*
@@ -93,8 +99,7 @@ class MonotonicallyIncreasingCounter : public Counter<T>
9399
}
94100

95101
/**
96-
* @brief
97-
* Advance the value of the counter.
102+
* @brief Advance the value of the counter.
98103
*
99104
* @return A CHIP error code if something fails, CHIP_NO_ERROR otherwise
100105
*/
@@ -108,8 +113,20 @@ class MonotonicallyIncreasingCounter : public Counter<T>
108113
}
109114

110115
/**
111-
* @brief
112-
* Get the current value of the counter.
116+
* @brief Advances the current counter value by N
117+
*
118+
* @param value value of N
119+
*
120+
* @return A CHIP error code if something fails, CHIP_NO_ERROR otherwise
121+
*/
122+
CHIP_ERROR AdvanceBy(T value) override
123+
{
124+
mCounterValue = static_cast<T>(mCounterValue + value);
125+
return CHIP_NO_ERROR;
126+
}
127+
128+
/**
129+
* @brief Get the current value of the counter.
113130
*
114131
* @return The current value of the counter.
115132
*/

src/lib/support/PersistedCounter.h

+62-12
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
#include <lib/support/CHIPCounter.h>
3333
#include <lib/support/CodeUtils.h>
3434
#include <lib/support/DefaultStorageKeyAllocator.h>
35+
#include <limits>
3536

3637
namespace chip {
3738

@@ -104,16 +105,50 @@ class PersistedCounter : public MonotonicallyIncreasingCounter<T>
104105
}
105106
#endif
106107

107-
ReturnErrorOnFailure(PersistNextEpochStart(startValue + aEpoch));
108+
ReturnErrorOnFailure(PersistNextEpochStart(static_cast<T>(startValue + aEpoch)));
108109

109110
// This will set the starting value, after which we're ready.
110111
return MonotonicallyIncreasingCounter<T>::Init(startValue);
111112
}
112113

113114
/**
114-
* @brief
115-
* Increment the counter and write to persisted storage if we've completed
116-
* the current epoch.
115+
* @brief Increment the counter by N and write to persisted storage if we've completed the current epoch.
116+
*
117+
* @param value value of N
118+
*
119+
* @return Any error returned by a write to persisted storage.
120+
*/
121+
CHIP_ERROR AdvanceBy(T value) override
122+
{
123+
VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE);
124+
VerifyOrReturnError(mKey.IsInitialized(), CHIP_ERROR_INCORRECT_STATE);
125+
126+
// If value is 0, we do not need to do anything
127+
VerifyOrReturnError(value > 0, CHIP_NO_ERROR);
128+
129+
// We should update the persisted epoch value if :
130+
// 1- Sum of the current counter and value is greater or equal to the mNextEpoch.
131+
// This is the standard operating case.
132+
// 2- Increasing the current counter by value would cause a roll over. This would cause the current value to be < to the
133+
// mNextEpoch so we force an update.
134+
bool shouldDoEpochUpdate = ((MonotonicallyIncreasingCounter<T>::GetValue() + value) >= mNextEpoch) ||
135+
(MonotonicallyIncreasingCounter<T>::GetValue() > std::numeric_limits<T>::max() - value);
136+
137+
ReturnErrorOnFailure(MonotonicallyIncreasingCounter<T>::AdvanceBy(value));
138+
139+
if (shouldDoEpochUpdate)
140+
{
141+
// Since AdvanceBy allows the counter to be increased by an arbitrary value, it is possible that the new counter value
142+
// is greater than mNextEpoch + mEpoch. As such, we want the next Epoch value to be calculated from the new current
143+
// value.
144+
PersistAndVerifyNextEpochStart(MonotonicallyIncreasingCounter<T>::GetValue());
145+
}
146+
147+
return CHIP_NO_ERROR;
148+
}
149+
150+
/**
151+
* @brief Increment the counter and write to persisted storage if we've completed the current epoch.
117152
*
118153
* @return Any error returned by a write to persisted storage.
119154
*/
@@ -126,18 +161,32 @@ class PersistedCounter : public MonotonicallyIncreasingCounter<T>
126161

127162
if (MonotonicallyIncreasingCounter<T>::GetValue() >= mNextEpoch)
128163
{
129-
// Value advanced past the previously persisted "start point".
130-
// Ensure that a new starting point is persisted.
131-
ReturnErrorOnFailure(PersistNextEpochStart(mNextEpoch + mEpoch));
132-
133-
// Advancing the epoch should have ensured that the current value
134-
// is valid
135-
VerifyOrReturnError(MonotonicallyIncreasingCounter<T>::GetValue() < mNextEpoch, CHIP_ERROR_INTERNAL);
164+
ReturnErrorOnFailure(PersistAndVerifyNextEpochStart(mNextEpoch));
136165
}
166+
137167
return CHIP_NO_ERROR;
138168
}
139169

140170
private:
171+
CHIP_ERROR PersistAndVerifyNextEpochStart(T refEpoch)
172+
{
173+
// Value advanced past the previously persisted "start point".
174+
// Ensure that a new starting point is persisted.
175+
ReturnErrorOnFailure(PersistNextEpochStart(static_cast<T>(refEpoch + mEpoch)));
176+
177+
// Advancing the epoch should have ensured that the current value is valid
178+
VerifyOrReturnError(static_cast<T>(MonotonicallyIncreasingCounter<T>::GetValue() + mEpoch) == mNextEpoch,
179+
CHIP_ERROR_INTERNAL);
180+
181+
// Previous check did not take into consideration that the counter value can be equal to the max counter value or
182+
// rollover.
183+
// TODO(#33175): PersistedCounter allows rollover so this check is incorrect. We need a Counter class that adequatly
184+
// manages rollover behavior for counters that cannot rollover.
185+
// VerifyOrReturnError(MonotonicallyIncreasingCounter<T>::GetValue() < mNextEpoch, CHIP_ERROR_INTERNAL);
186+
187+
return CHIP_NO_ERROR;
188+
}
189+
141190
/**
142191
* @brief
143192
* Write out the counter value to persistent storage.
@@ -146,7 +195,8 @@ class PersistedCounter : public MonotonicallyIncreasingCounter<T>
146195
*
147196
* @return Any error returned by a write to persistent storage.
148197
*/
149-
CHIP_ERROR PersistNextEpochStart(T aStartValue)
198+
CHIP_ERROR
199+
PersistNextEpochStart(T aStartValue)
150200
{
151201
mNextEpoch = aStartValue;
152202
#if CHIP_CONFIG_PERSISTED_COUNTER_DEBUG_LOGGING

src/lib/support/tests/BUILD.gn

+2-2
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,13 @@ chip_test_suite("tests") {
2828
"TestBufferWriter.cpp",
2929
"TestBytesCircularBuffer.cpp",
3030
"TestBytesToHex.cpp",
31+
"TestCHIPCounter.cpp",
3132
"TestDefer.cpp",
3233
"TestErrorStr.cpp",
3334
"TestFixedBufferAllocator.cpp",
3435
"TestFold.cpp",
3536
"TestIniEscaping.cpp",
37+
"TestPersistedCounter.cpp",
3638
"TestPrivateHeap.cpp",
3739
"TestSafeInt.cpp",
3840
"TestSafeString.cpp",
@@ -74,13 +76,11 @@ chip_test_suite_using_nltest("tests_nltest") {
7476
output_name = "libSupportTestsNL"
7577

7678
test_sources = [
77-
"TestCHIPCounter.cpp",
7879
"TestCHIPMem.cpp",
7980
"TestCHIPMemString.cpp",
8081
"TestIntrusiveList.cpp",
8182
"TestJsonToTlv.cpp",
8283
"TestJsonToTlvToJson.cpp",
83-
"TestPersistedCounter.cpp",
8484
"TestPool.cpp",
8585
"TestStateMachine.cpp",
8686
"TestThreadOperationalDataset.cpp",

0 commit comments

Comments
 (0)