diff --git a/src/controller/tests/data_model/TestRead.cpp b/src/controller/tests/data_model/TestRead.cpp index cc314e60469cd2..15b2ee6c1f78bf 100644 --- a/src/controller/tests/data_model/TestRead.cpp +++ b/src/controller/tests/data_model/TestRead.cpp @@ -298,6 +298,11 @@ class TestReadInteraction : public app::ReadHandler::ApplicationCallback // succeed. static void MultipleReadHelper(nlTestSuite * apSuite, TestContext & aCtx, size_t aReadCount); + // Helper for MultipleReadHelper that does not spin the event loop, so we + // don't end up with nested event loops. + static void MultipleReadHelperInternal(nlTestSuite * apSuite, TestContext & aCtx, size_t aReadCount, + uint32_t & aNumSuccessCalls, uint32_t & aNumFailureCalls); + // Establish the given number of subscriptions, then issue the given number // of reads in parallel and wait for them all to succeed. static void SubscribeThenReadHelper(nlTestSuite * apSuite, TestContext & aCtx, size_t aSubscribeCount, size_t aReadCount); @@ -2484,6 +2489,9 @@ void TestReadInteraction::SubscribeThenReadHelper(nlTestSuite * apSuite, TestCon uint32_t numSuccessCalls = 0; uint32_t numSubscriptionEstablishedCalls = 0; + uint32_t numReadSuccessCalls = 0; + uint32_t numReadFailureCalls = 0; + responseDirective = kSendDataResponse; // Passing of stack variables by reference is only safe because of synchronous completion of the interaction. Otherwise, it's @@ -2501,12 +2509,12 @@ void TestReadInteraction::SubscribeThenReadHelper(nlTestSuite * apSuite, TestCon NL_TEST_ASSERT(apSuite, false); }; - auto onSubscriptionEstablishedCb = [&numSubscriptionEstablishedCalls, &apSuite, &aCtx, aSubscribeCount, - aReadCount](const app::ReadClient & readClient) { + auto onSubscriptionEstablishedCb = [&numSubscriptionEstablishedCalls, &apSuite, &aCtx, aSubscribeCount, aReadCount, + &numReadSuccessCalls, &numReadFailureCalls](const app::ReadClient & readClient) { numSubscriptionEstablishedCalls++; if (numSubscriptionEstablishedCalls == aSubscribeCount) { - MultipleReadHelper(apSuite, aCtx, aReadCount); + MultipleReadHelperInternal(apSuite, aCtx, aReadCount, numReadSuccessCalls, numReadFailureCalls); } }; @@ -2522,40 +2530,50 @@ void TestReadInteraction::SubscribeThenReadHelper(nlTestSuite * apSuite, TestCon NL_TEST_ASSERT(apSuite, numSuccessCalls == aSubscribeCount); NL_TEST_ASSERT(apSuite, numSubscriptionEstablishedCalls == aSubscribeCount); + NL_TEST_ASSERT(apSuite, numReadSuccessCalls == aReadCount); + NL_TEST_ASSERT(apSuite, numReadFailureCalls == 0); } -void TestReadInteraction::MultipleReadHelper(nlTestSuite * apSuite, TestContext & aCtx, size_t aReadCount) +// The guts of MultipleReadHelper which take references to the success/failure +// counts to modify and assume the consumer will be spinning the event loop. +void TestReadInteraction::MultipleReadHelperInternal(nlTestSuite * apSuite, TestContext & aCtx, size_t aReadCount, + uint32_t & aNumSuccessCalls, uint32_t & aNumFailureCalls) { - auto sessionHandle = aCtx.GetSessionBobToAlice(); - uint32_t numSuccessCalls = 0; - uint32_t numFailureCalls = 0; + NL_TEST_ASSERT(apSuite, aNumSuccessCalls == 0); + NL_TEST_ASSERT(apSuite, aNumFailureCalls == 0); + + auto sessionHandle = aCtx.GetSessionBobToAlice(); responseDirective = kSendDataResponse; uint16_t firstExpectedResponse = totalReadCount + 1; - // Passing of stack variables by reference is only safe because of synchronous completion of the interaction. Otherwise, it's - // not safe to do so. - auto onFailureCb = [&apSuite, &numFailureCalls](const app::ConcreteDataAttributePath * attributePath, CHIP_ERROR aError) { - numFailureCalls++; + auto onFailureCb = [apSuite, &aNumFailureCalls](const app::ConcreteDataAttributePath * attributePath, CHIP_ERROR aError) { + aNumFailureCalls++; NL_TEST_ASSERT(apSuite, attributePath == nullptr); }; for (size_t i = 0; i < aReadCount; ++i) { - // Passing of stack variables by reference is only safe because of synchronous completion of the interaction. Otherwise, - // it's not safe to do so. - auto onSuccessCb = [&numSuccessCalls, &apSuite, firstExpectedResponse, + auto onSuccessCb = [&aNumSuccessCalls, apSuite, firstExpectedResponse, i](const app::ConcreteDataAttributePath & attributePath, const auto & dataResponse) { NL_TEST_ASSERT(apSuite, dataResponse == firstExpectedResponse + i); - numSuccessCalls++; + aNumSuccessCalls++; }; NL_TEST_ASSERT(apSuite, Controller::ReadAttribute( &aCtx.GetExchangeManager(), sessionHandle, kTestEndpointId, onSuccessCb, onFailureCb) == CHIP_NO_ERROR); } +} + +void TestReadInteraction::MultipleReadHelper(nlTestSuite * apSuite, TestContext & aCtx, size_t aReadCount) +{ + uint32_t numSuccessCalls = 0; + uint32_t numFailureCalls = 0; + + MultipleReadHelperInternal(apSuite, aCtx, aReadCount, numSuccessCalls, numFailureCalls); aCtx.DrainAndServiceIO(); diff --git a/src/system/SystemLayerImplFreeRTOS.cpp b/src/system/SystemLayerImplFreeRTOS.cpp index 79d892b30c54d4..1a9692302c9c48 100644 --- a/src/system/SystemLayerImplFreeRTOS.cpp +++ b/src/system/SystemLayerImplFreeRTOS.cpp @@ -88,10 +88,32 @@ CHIP_ERROR LayerImplFreeRTOS::ScheduleWork(TimerCompleteCallback onComplete, voi { VerifyOrReturnError(mLayerState.IsInitialized(), CHIP_ERROR_INCORRECT_STATE); + // Ideally we would not use a timer here at all, but if we try to just + // ScheduleLambda the lambda needs to capture the following: + // 1) onComplete + // 2) appState + // 3) The `this` pointer, because onComplete needs to be passed a pointer to + // the System::Layer. + // + // On a 64-bit system that's 24 bytes, but lambdas passed to ScheduleLambda + // are capped at CHIP_CONFIG_LAMBDA_EVENT_SIZE which is 16 bytes. + // + // So for now use a timer as a poor-man's closure that captures `this` and + // onComplete and appState in a single pointer, so we fit inside the size + // limit. + // + // TODO: We could do something here where we compile-time condition on the + // sizes of things and use a direct ScheduleLambda if it would fit and this + // setup otherwise. TimerList::Node * timer = mTimerPool.Create(*this, SystemClock().GetMonotonicTimestamp(), onComplete, appState); VerifyOrReturnError(timer != nullptr, CHIP_ERROR_NO_MEMORY); - return ScheduleLambda([this, timer] { this->mTimerPool.Invoke(timer); }); + CHIP_ERROR err = ScheduleLambda([this, timer] { this->mTimerPool.Invoke(timer); }); + if (err != CHIP_NO_ERROR) + { + mTimerPool.Release(timer); + } + return err; } /** diff --git a/src/system/SystemLayerImplSelect.cpp b/src/system/SystemLayerImplSelect.cpp index 98f4fe71547826..88883a2a027520 100644 --- a/src/system/SystemLayerImplSelect.cpp +++ b/src/system/SystemLayerImplSelect.cpp @@ -170,6 +170,14 @@ void LayerImplSelect::CancelTimer(TimerCompleteCallback onComplete, void * appSt VerifyOrReturn(mLayerState.IsInitialized()); TimerList::Node * timer = mTimerList.Remove(onComplete, appState); + if (timer == nullptr) + { + // The timer was not in our "will fire in the future" list, but it might + // be in the "we're about to fire these" chunk we already grabbed from + // that list. Check for it there too, and if found there we still want + // to cancel it. + timer = mExpiredTimers.Remove(onComplete, appState); + } VerifyOrReturn(timer != nullptr); #if CHIP_SYSTEM_CONFIG_USE_DISPATCH @@ -199,8 +207,31 @@ CHIP_ERROR LayerImplSelect::ScheduleWork(TimerCompleteCallback onComplete, void } #endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH - CancelTimer(onComplete, appState); - + // Ideally we would not use a timer here at all, but if we try to just + // ScheduleLambda the lambda needs to capture the following: + // 1) onComplete + // 2) appState + // 3) The `this` pointer, because onComplete needs to be passed a pointer to + // the System::Layer. + // + // On a 64-bit system that's 24 bytes, but lambdas passed to ScheduleLambda + // are capped at CHIP_CONFIG_LAMBDA_EVENT_SIZE which is 16 bytes. + // + // So for now use a timer as a poor-man's closure that captures `this` and + // onComplete and appState in a single pointer, so we fit inside the size + // limit. + // + // TODO: We could do something here where we compile-time condition on the + // sizes of things and use a direct ScheduleLambda if it would fit and this + // setup otherwise. + // + // TODO: But also, unit tests seem to do SystemLayer::ScheduleWork without + // actually running a useful event loop (in the PlatformManager sense), + // which breaks if we use ScheduleLambda here, since that does rely on the + // PlatformManager event loop. So for now, keep scheduling an expires-ASAP + // timer, but just make sure we don't cancel existing timers with the same + // callback and appState, so ScheduleWork invocations don't stomp on each + // other. TimerList::Node * timer = mTimerPool.Create(*this, SystemClock().GetMonotonicTimestamp(), onComplete, appState); VerifyOrReturnError(timer != nullptr, CHIP_ERROR_NO_MEMORY); @@ -469,9 +500,10 @@ void LayerImplSelect::HandleEvents() // Obtain the list of currently expired timers. Any new timers added by timer callback are NOT handled on this pass, // since that could result in infinite handling of new timers blocking any other progress. - TimerList expiredTimers = mTimerList.ExtractEarlier(Clock::Timeout(1) + SystemClock().GetMonotonicTimestamp()); + VerifyOrDieWithMsg(mExpiredTimers.Empty(), DeviceLayer, "Re-entry into HandleEvents from a timer callback?"); + mExpiredTimers = mTimerList.ExtractEarlier(Clock::Timeout(1) + SystemClock().GetMonotonicTimestamp()); TimerList::Node * timer = nullptr; - while ((timer = expiredTimers.PopEarliest()) != nullptr) + while ((timer = mExpiredTimers.PopEarliest()) != nullptr) { mTimerPool.Invoke(timer); } diff --git a/src/system/SystemLayerImplSelect.h b/src/system/SystemLayerImplSelect.h index f193a8860d961c..4243d2aee9f5e1 100644 --- a/src/system/SystemLayerImplSelect.h +++ b/src/system/SystemLayerImplSelect.h @@ -101,6 +101,9 @@ class LayerImplSelect : public LayerSocketsLoop TimerPool mTimerPool; TimerList mTimerList; + // List of expired timers being processed right now. Stored in a member so + // we can cancel them. + TimerList mExpiredTimers; timeval mNextTimeout; // Members for select loop diff --git a/src/system/tests/BUILD.gn b/src/system/tests/BUILD.gn index 47c97b57caa9e1..51f321ef8dfe94 100644 --- a/src/system/tests/BUILD.gn +++ b/src/system/tests/BUILD.gn @@ -31,6 +31,10 @@ chip_test_suite("tests") { "TestTimeSource.cpp", ] + if (chip_device_platform != "esp32" && chip_device_platform != "fake") { + test_sources += [ "TestSystemScheduleWork.cpp" ] + } + cflags = [ "-Wconversion" ] public_deps = [ diff --git a/src/system/tests/TestSystemClock.cpp b/src/system/tests/TestSystemClock.cpp index 0853973b4f5846..1ac5b994a2edb2 100644 --- a/src/system/tests/TestSystemClock.cpp +++ b/src/system/tests/TestSystemClock.cpp @@ -118,7 +118,7 @@ static const nlTest sTests[] = int TestSystemClock(void) { nlTestSuite theSuite = { - "chip-timesource", &sTests[0], nullptr /* setup */, nullptr /* teardown */ + "chip-systemclock", &sTests[0], nullptr /* setup */, nullptr /* teardown */ }; // Run test suit againt one context. diff --git a/src/system/tests/TestSystemScheduleWork.cpp b/src/system/tests/TestSystemScheduleWork.cpp new file mode 100644 index 00000000000000..6c858905f50b76 --- /dev/null +++ b/src/system/tests/TestSystemScheduleWork.cpp @@ -0,0 +1,107 @@ +/* + * Copyright (c) 2021 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 + +#include +#include +#include +#include +#include + +static void IncrementIntCounter(chip::System::Layer *, void * state) +{ + ++(*static_cast(state)); +} + +static void StopEventLoop(chip::System::Layer *, void *) +{ + chip::DeviceLayer::PlatformMgr().StopEventLoopTask(); +} + +static void CheckScheduleWorkTwice(nlTestSuite * inSuite, void * aContext) +{ + int * callCount = new int(0); + NL_TEST_ASSERT(inSuite, chip::DeviceLayer::SystemLayer().ScheduleWork(IncrementIntCounter, callCount) == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, chip::DeviceLayer::SystemLayer().ScheduleWork(IncrementIntCounter, callCount) == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, chip::DeviceLayer::SystemLayer().ScheduleWork(StopEventLoop, nullptr) == CHIP_NO_ERROR); + chip::DeviceLayer::PlatformMgr().RunEventLoop(); + NL_TEST_ASSERT(inSuite, *callCount == 2); + delete callCount; +} + +// Test Suite + +/** + * Test Suite. It lists all the test functions. + */ +// clang-format off +static const nlTest sTests[] = +{ + NL_TEST_DEF("System::TestScheduleWorkTwice", CheckScheduleWorkTwice), + NL_TEST_SENTINEL() +}; +// clang-format on + +static int TestSetup(void * aContext); +static int TestTeardown(void * aContext); + +// clang-format off +static nlTestSuite kTheSuite = +{ + "chip-system-schedule-work", + &sTests[0], + TestSetup, + TestTeardown +}; +// clang-format on + +/** + * Set up the test suite. + */ +static int TestSetup(void * aContext) +{ + if (chip::Platform::MemoryInit() != CHIP_NO_ERROR) + { + return FAILURE; + } + + if (chip::DeviceLayer::PlatformMgr().InitChipStack() != CHIP_NO_ERROR) + return FAILURE; + + return (SUCCESS); +} + +/** + * Tear down the test suite. + * Free memory reserved at TestSetup. + */ +static int TestTeardown(void * aContext) +{ + chip::DeviceLayer::PlatformMgr().Shutdown(); + chip::Platform::MemoryShutdown(); + return (SUCCESS); +} + +int TestSystemScheduleWork(void) +{ + // Run test suit againt one lContext. + nlTestRunner(&kTheSuite, nullptr); + + return nlTestRunnerStats(&kTheSuite); +} + +CHIP_REGISTER_TEST_SUITE(TestSystemScheduleWork) diff --git a/src/system/tests/TestSystemTimer.cpp b/src/system/tests/TestSystemTimer.cpp index 58ff87e85cd8dd..f88cf12f552333 100644 --- a/src/system/tests/TestSystemTimer.cpp +++ b/src/system/tests/TestSystemTimer.cpp @@ -126,6 +126,15 @@ class TestContext TestContext() : mGreedyTimer(GreedyTimer, this), mNumTimersHandled(0) {} }; +static TestContext * gCurrentTestContext = nullptr; + +class ScopedGlobalTestContext +{ +public: + ScopedGlobalTestContext(TestContext * ctx) { gCurrentTestContext = ctx; } + ~ScopedGlobalTestContext() { gCurrentTestContext = nullptr; } +}; + // Test input data. static volatile bool sOverflowTestDone; @@ -204,7 +213,7 @@ static void CheckStarvation(nlTestSuite * inSuite, void * aContext) LayerEvents::ServiceEvents(lSys); } -void CheckOrder(nlTestSuite * inSuite, void * aContext) +static void CheckOrder(nlTestSuite * inSuite, void * aContext) { if (!LayerEvents::HasServiceEvents()) return; @@ -257,6 +266,172 @@ void CheckOrder(nlTestSuite * inSuite, void * aContext) Clock::Internal::SetSystemClockForTesting(savedClock); } +static void CheckCancellation(nlTestSuite * inSuite, void * aContext) +{ + if (!LayerEvents::HasServiceEvents()) + return; + + TestContext & testContext = *static_cast(aContext); + Layer & systemLayer = *testContext.mLayer; + nlTestSuite * const suite = testContext.mTestSuite; + + struct TestState + { + TestState(Layer & aSystemLayer) : mSystemLayer(aSystemLayer) {} + + void Record(char c) + { + size_t n = strlen(record); + if (n + 1 < sizeof(record)) + { + record[n++] = c; + record[n] = 0; + } + } + static void A(Layer * layer, void * state) + { + auto self = static_cast(state); + self->Record('A'); + self->mSystemLayer.CancelTimer(B, state); + self->mSystemLayer.CancelTimer(D, state); + } + static void B(Layer * layer, void * state) { static_cast(state)->Record('B'); } + static void C(Layer * layer, void * state) + { + auto self = static_cast(state); + self->Record('C'); + self->mSystemLayer.CancelTimer(E, state); + } + static void D(Layer * layer, void * state) { static_cast(state)->Record('D'); } + static void E(Layer * layer, void * state) { static_cast(state)->Record('E'); } + char record[6] = { 0 }; + + Layer & mSystemLayer; + }; + TestState testState(systemLayer); + NL_TEST_ASSERT(suite, testState.record[0] == 0); + + Clock::ClockBase * const savedClock = &SystemClock(); + Clock::Internal::MockClock mockClock; + Clock::Internal::SetSystemClockForTesting(&mockClock); + + using namespace Clock::Literals; + systemLayer.StartTimer(0_ms, TestState::A, &testState); + systemLayer.StartTimer(0_ms, TestState::B, &testState); + systemLayer.StartTimer(20_ms, TestState::C, &testState); + systemLayer.StartTimer(30_ms, TestState::D, &testState); + systemLayer.StartTimer(50_ms, TestState::E, &testState); + + mockClock.AdvanceMonotonic(100_ms); + LayerEvents::ServiceEvents(systemLayer); + NL_TEST_ASSERT(suite, strcmp(testState.record, "AC") == 0); + + Clock::Internal::SetSystemClockForTesting(savedClock); +} + +namespace { + +namespace CancelTimerTest { + +// A bit lower than maximum system timers just in case, for systems that +// have some form of limit +constexpr unsigned kCancelTimerCount = CHIP_SYSTEM_CONFIG_NUM_TIMERS - 4; +int gCallbackProcessed[kCancelTimerCount]; + +/// Validates that gCallbackProcessed has valid values (0 or 1) +void ValidateExecutedTimerCounts(nlTestSuite * suite) +{ + for (unsigned i = 0; i < kCancelTimerCount; i++) + { + NL_TEST_ASSERT(suite, (gCallbackProcessed[i] == 0) || (gCallbackProcessed[i] == 1)); + } +} + +unsigned ExecutedTimerCount() +{ + unsigned count = 0; + for (unsigned i = 0; i < kCancelTimerCount; i++) + { + if (gCallbackProcessed[i] != 0) + { + count++; + } + } + return count; +} + +void Callback(Layer * layer, void * state) +{ + unsigned idx = static_cast(reinterpret_cast(state)); + if (gCallbackProcessed[idx] != 0) + { + ChipLogError(Test, "UNEXPECTED EXECUTION at index %u", idx); + } + + gCallbackProcessed[idx]++; + + if (ExecutedTimerCount() == kCancelTimerCount / 2) + { + ChipLogProgress(Test, "Cancelling timers"); + for (unsigned i = 0; i < kCancelTimerCount; i++) + { + if (gCallbackProcessed[i] != 0) + { + continue; + } + ChipLogProgress(Test, "Timer %u is being cancelled", i); + gCurrentTestContext->mLayer->CancelTimer(Callback, reinterpret_cast(static_cast(i))); + gCallbackProcessed[i]++; // pretend executed. + } + } +} + +void Test(nlTestSuite * inSuite, void * aContext) +{ + // Validates that timers can cancel other timers. Generally the test will + // do the following: + // - schedule several timers to start at the same time + // - within each timers, after half of them have run, make one timer + // cancel all the other ones + // - assert that: + // - timers will run if scheduled + // - once cancelled, timers will NOT run (i.e. a timer can cancel + // other timers, even if they are expiring at the same time) + memset(gCallbackProcessed, 0, sizeof(gCallbackProcessed)); + + TestContext & testContext = *static_cast(aContext); + ScopedGlobalTestContext testScope(&testContext); + + Layer & systemLayer = *testContext.mLayer; + nlTestSuite * const suite = testContext.mTestSuite; + + Clock::ClockBase * const savedClock = &SystemClock(); + Clock::Internal::MockClock mockClock; + Clock::Internal::SetSystemClockForTesting(&mockClock); + using namespace Clock::Literals; + + for (unsigned i = 0; i < kCancelTimerCount; i++) + { + NL_TEST_ASSERT( + suite, systemLayer.StartTimer(10_ms, Callback, reinterpret_cast(static_cast(i))) == CHIP_NO_ERROR); + } + + LayerEvents::ServiceEvents(systemLayer); + ValidateExecutedTimerCounts(suite); + NL_TEST_ASSERT(suite, ExecutedTimerCount() == 0); + + mockClock.AdvanceMonotonic(20_ms); + LayerEvents::ServiceEvents(systemLayer); + + ValidateExecutedTimerCounts(suite); + NL_TEST_ASSERT(suite, ExecutedTimerCount() == kCancelTimerCount); + + Clock::Internal::SetSystemClockForTesting(savedClock); +} + +} // namespace CancelTimerTest +} // namespace + // Test the implementation helper classes TimerPool, TimerList, and TimerData. namespace chip { namespace System { @@ -416,7 +591,9 @@ static const nlTest sTests[] = NL_TEST_DEF("Timer::TestOverflow", CheckOverflow), NL_TEST_DEF("Timer::TestTimerStarvation", CheckStarvation), NL_TEST_DEF("Timer::TestTimerOrder", CheckOrder), + NL_TEST_DEF("Timer::TestTimerCancellation", CheckCancellation), NL_TEST_DEF("Timer::TestTimerPool", chip::System::TestTimer::CheckTimerPool), + NL_TEST_DEF("Timer::TestCancelTimer", CancelTimerTest::Test), NL_TEST_SENTINEL() }; // clang-format on